Copied from MergedTree::has_conflict(). I feel it's slightly better since
RefTarget "is" always a Conflict-based type. It could be inverted to
RefTarget::is_resolved(), but refs are usually resolved, and all callers
have special case for !is_resolved() state.
Since RefTarget will be reimplemented on top of Conflict<Option<CommitId>>,
we won't be able to simply return a slice of type &[CommitId]. These functions
are also renamed in order to disambiguate from Conflict::adds()/removes().
It's named after Conflict::from_legacy_form(). If RefTarget is migrated to
new Conflict type, from_legacy_form([], [add]) will create a normal target,
and from_legacy_form([], []) will be equivalent to the current None target.
That's why this function isn't named as RefTarget::conflict().
If RefTarget is migrated to new Conflict type, this function will create
RefTarget(Conflict::resolved(Some(id))).
We still need some .unwrap() to insert Option<RefTarget> into map, but maps
will be changed to store new RefTarget type, and their mutation API will
guarantee that Conflict::resolved(None) is eliminated.
Almost everyone calls the project "jj", and there seeems to be
consensus that we should rename the crates. I originally wanted the
crates to be called `jj` and `jj-lib`, but `jj` was already
taken. `jj-cli` is probably at least as good for it anyway.
Once we've published a 0.8.0 under the new names, we'll release 0.7.1
versions under the old names with pointers to the new crates names.
We already have `create_random_commit()`, which returns a
`CommitBuilder`. Most callers directly write that to a
`MutableRepo`. That currently returns a `Commit`, but I'm about to
make it propagate errors from the backend. That would add an
`unwrap()` to this sequence, making it longer. Let's create a simple
helper for these callers to simplify this common pattern.
When you're done with the `CommitBuilder`, you're going to have to
call `write_to_repo()`, passing it a mutable `MutableRepo`
reference. It's a bit simpler to pass that reference when we create
the `CommitBuilder` instead, so that's what this patch does.
A drawback of passing in the mutable reference when we create the
builder is that we can't have multiple unfinished `CommitBuilder`
instance live at the same time. We don't have any such use cases yet,
and it's not hard to work around them, so I think this change is worth
it.
The next commit will introduce a newtype for -m/--message argument which
can be converted Into<String>.
Since CommitBuilder is a thin wrapper, code bloat caused by generic parameters
wouldn't matter. I have another set of commits that makes all builder methods
accept Into/IntoIterator, which will remove some of .clone() calls from tests.
Let's acknowledge everyone's contributions by replacing "Google LLC"
in the copyright header by "The Jujutsu Authors". If I understand
correctly, it won't have any legal effect, but maybe it still helps
reduce concerns from contributors (though I haven't heard any
concerns).
Google employees can read about Google's policy at
go/releasing/contributions#copyright.
We currently get the hostname and username from the `whoami` crate. We
do that in lib crate, without giving the caller a way to override
them. That seems wrong since it might be used in a server and
performing operations on behalf of some other user. This commit makes
the hostname and username configurable, so the calling crate can pass
them in. If they have not been passed in, we still default to the
values from the `whoami` crate.
The `testutils` module should ideally not be part of the library
dependencies. Since they're used by the integration tests (and the CLI
tests), we need to move them to a separate crate to achieve that.
This moves the logic for handling the root commit when writing commits
from `CommitBuilder` into the individual backends. It always bothered
me a bit that the `commit::Commit` wrapper had a different idea of the
number of parents than the wrapped `backend::Commit` had.
With this change, the `LocalBackend` will now write the root commit in
the list of parents if it's there in the argument to
`write_commit()`. Note that root commit itself won't be written. The
main argument for not writing it is that we can then keep the fake
all-zeros hash for it. One argument for writing it, if we were to do
so, is that it would make the set of written objects consistent, so
any future processing of them (such as GC) doesn't have to know to
ignore the root commit in the list of parents.
We still treat the two backends the same, so the user won't be allowed
to create merges including the root commit even when using the
`LocalBackend`.
`wc_commit` seems clearer than `checkout` and not too much longer. I
considered `working_copy` but it was less clear (could be the path to
the working copy, or an instance of `WorkingCopy`). I also considered
`working_copy_commit`, but that seems a bit too long.
The `CommitBuilder::store` field is used only in
`CommitBuilder::write_to_repo()`, but we can easily get access to the
`Store` from the `repo` argument there, so let's remove the field.
When rebasing commits after rewrites, we also update all workspaces'
checkouts. If the new commit is closed, we create a new commit on
top. Since we're hoping to remove the open/closed concept, we need a
new condition. I considered creating a new commit on top if the change
ID was different from before the rewrite. However, that would make at
least `jj split` more complicated because it makes the first commit
keep the change ID but it wants the second commit to be checked
out. This patch instead creates the new commit on top only when the
original commit was abandoned.
We don't even have any settings that affect the repo, so there's no
point in passing the settings. I think this was a leftover from before
we separated out the "workspace" concept; now we no longer create a
working-copy commit when we initialize a repo (we do that when we
attach the workspace).
Let's say we have a simple history like this:
```
B C D
\|/
A
```
Branch `main` initially points to commit B. Two concurrent operations
then move the branch to commits C and D. When the two concurrent
operations get merged, the branch will be recorded as pointing to
"C+D-B". If a subsequent operation now abandons commit B, we would
update the "removed" side of the branch conflict. That seems a little
dishonest. I think the reason I did it that way was in order to not
keep B visible back when having it present in the "removed" side would
keep it visible (before 33bf6ce1d5).
I noticed this issue while working on #241 because
`test_import_refs_reimport()` started failing. That test case is
pretty much exactly the case above.
The `DescendantRebaser` keeps a map of branches from the source
commit, so it gets efficient lookup of branches to update when a
commit has been rebased. This map was not kept up to date as we
rebased. That could lead to branches getting left on hidden
intermediate commits. Specifically, if a commit with a branch was
rewritten by some command, and an ancestor of it was also rewritten,
then we'd only update the branch only the first step and not update it
again when rebasing onto the rewritten ancestor.
There was a TODO about adding a test case for a delete/modify conflict
in a branch target that got resolved by abandoning a commit. The
resolution is to delete the branch. That case couldn't happend with
our old evolution-based mechanism for tracking rewrites (because we
couldn't un-prune a commit then).
Most tests need a repo but don't need a working copy. Let's have a
function for setting up a test repo. But first, let's free up the name
`init_repo()` by renaming it to `init_workspace()` (which is also more
accurate).
The `DescendantRebaser` was designed to help with rebasing in two
different use cases: 1) after regular rewriting of commits where the
change ID is preserved, and 2) after importing moved branches from
other repo (e.g. backing Git repo or remote). Many of the tests are
for the second use case, such as where a branch was moved
forward. However, I just noticed that there's a pretty common scenario
from the first use case that is not supported.
Let's say you have this history:
```
D
|
C C'
|/
B B'
|/
A
```
Here we want C' to be rebased onto B' and then D to be rebased onto
C''. However, because of the support for moving branches forward, we
would not rebase commits that were already rewritten, such as C' here
(see affected tests for details), which resulted in D getting rebased
onto C', and both B and B' remaining visible.
I think I was thinking when I designed it that it would be nice if you
could just tell `DescendantRebaser` that any descendants of a commit
should be moved forward. That may be useful, but I don't think we'll
want that for the general case of a branch moving forward. Perhaps
we'll want to make it configurable which branches it should happen
for. Either way, the way it was coded by not rebasing already
rewritten commits did not work for the case above. We may be able to
handle both cases better by considering each rewrite separately
instead of all destinations at once. For now, however, I've decided to
keep it simple, so I'm fixing the case above by sacrificing some of
the potentially useful functionality for moving branches forward.
Another fix necessary for the scenario shown above was to make sure we
always rebase C' before D. Before this patch, that depended on the
order in the index. This patch fixes that by modifying the topological
order to take rewrites into account, making D depend not only on C but
also on C'. (I suppose you could instead say that C depends on both B
and C'; I don't know if that'd make a difference.)
Despite what the documentation said, we don't clear the record of
rewritten and abandoned commits at the end. This change fixes that,
and adds a test showing that it's possible to call
`MutableRepo::rebase_descendants()` multiple times.
If you rewrite a commit that's also available on some remote, you'll
currently see both the old version and the new version in the view,
which means they're divergent. They're not logically divergent (the
rewritten version should replace the old version), so this is a UX
bug. I think it indicates that the set of current heads should be
redefined to be the *desired* heads. That's also what I had suspected
in the TODO removed by this change. I think another indication that
we should hide the old heads even if they have e.g. a remote branch
pointing to them is that we don't want them to be rebased if we
rewrite an ancestor.
So that's what I decided to do: let the view's heads be the desired
heads. The user can still define revsets for showing non-current
commits pointed to by e.g. remote branches.
This patch teaches `DescendantRebaser` to also update heads. That's
done at the end of the rebase (when `rebase_next()` starts returning
`None`), which is a little weird. We should probably change the
interface, but this will do for now.
With this change, we should no longer need to remove hidden heads when
the transaction commits. That will remove one of the last bits of
dependence on evolution from most commands (#32).
This is similar to how a recent change taught `DescendantRebaser` to
update branches pointing to rewritten commits. Now we also update the
checkout if it pointed to a rewritten commit.
This patch moves the logic for updating branches from
`update_branches_after_rewrite()` into `DescendantRebaser`. The
branches are now updated along with each rebased commit rather than
all being updated at the end. The new code uses the information about
rewritten and abandoned commits that `DescendantRebaser` gets from
`MutableRepo`. That is different from the old code, which used the
evolution state. This patch thus moves us one step closer to removing
evolution (#32).
I'm going to teach `DescendantRebaser` to also update local branches
pointing to rewritten commits, taking over the responsibility from
`rewrite::update_branches_after_rewrite()`. For commits that have been
rewritten as multiple new commits (divergent, not split), that
function makes local branches pointing to the old commit point to all
the new commits. To replicate that behavior in `DescendantRebaser`, it
needs to know about divergent changes. This change addresses that.
