I'll make TreeState::snapshot() return a boolean denoting whether tree_state
is updated or not. New tree_id can be obtained from TreeState, so let's
remove it from the return value.
While making tree_state() return RefMut<TreeState> instead of RefMut<Option<_>>,
I felt uncomfortable that tree_state(&self) returned a mutable reference. So
this patch splits it into tree_state() and tree_state_mut().
I was reading a draft of "Git Rev News: Edition 91" [1] where Peff
mentions some unfinished patches to allow negative timestamps in
Git. So I figured I should add support for that before I forget. I
haven't checked if libgit2 supports it, so it might be that our Git
backend still doesn't support it after this patch.
[1] https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-91.md
When the remote rejects a push, we now say something like this:
```
Remote rejected the update of some refs (do you have permission to push to ["refs/heads/main"]?)
```
That message could be formatted better, but this seems good enough for
now. We should probably have some more custom conversion from
`GitPushError` to `CommandError` in the CLI layer.
This changes `RepoLoader` to take a map of functions that load a
specific type of backend, keyed by the backend type. The backend type
is read from `.jj/repo/store/backend`.
We currently determine if the repo uses the Git backend or the local
backend by checking for presence of a `.jj/repo/store/git_target`
file. To make it easier to add out-of-tree backends, let's instead add
a file that indicates which backend to use.
The `ReadonlyRepo::init_*()` functions were unused or used only in
tests. Let's remove them, thereby making the repo less aware of
specific backend implementations.
By calling `repo.loader()` on the existing repo, we reuse the same
commit store etc. That should make it easier to add out-of-tree
backends by removing one place we'd otherwise need to pass in a way of
creating a backend (now we reuse the instance that was already created
for the existing `repo`).
In many of these places, we don't need an owned value, so using a
reference means we don't force the caller to clone the value. I really
doubt it will have any noticeable impact on performance (I think these
are all once-per-repo paths); it's just a little simpler this way.
I feel it doesn't make sense for a simple getter function to create an
owned vec after 0108673087 "backend: let each backend handle root commit
on write."
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`.
I had made the backends unaware of the virtual root commit because
they don't need to know about it, and we could avoid some duplicated
code by putting that in `Store` instead. However, as we saw in
b21a123bc8, the root commit being virtual has some user-visible
effects (they can't create a merge with the root and some other
commit). So I'm thinking that we may want to make the root commit an
actual commit, depending on which backend is used. Specificially, when
using the Git backend, we cannot record the root commit as an actual
parent since Git would fail when trying to look it up. Backends that
don't need compatibility can make the root commit an actual commit,
however.
This commit therefore makes the backends aware of the root commit. It
makes it remain a virtual commit in the Git backend, and makes it an
actual commit in the `LocalBackend`.
This commit breaks any existing repos using the `LocalBackend`, but
there shouldn't be any such repos other than for testing.
I feel the original -------/+++++++ pair is slightly confusing because
each half can be a separator by itself. I don't know what character other
than '-'/'+' is preferred, but let's pick '%' (for "mod") per @martinvonz
suggestion.
`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.
This will be a basic building block of 'jj log PATH'. The implementation
is naive, but works fine for small repos like jj. For mid-size repos,
there would be various areas which need to be optimized.
Our current version of `cpufeatures` was yanked so we needed to
upgrade at least that. Note that I had to add "UNICODE-DFS-2016" as an
allowed license for `cargo-deny`. I also had to upgrade `chrono` from
0.4.20 to 0.4.22 in the `Cargo.toml` files to prevent `cargo update`
from *downgrading* it in the lock file.
Otherwise a file could be created out of the working copy directory.
This only works for untracked symlinks and sequentially "added" symlinks
and files. For "removed" and "modified" entries, the parent directories are
considered valid and fs::remove_file() will be called. This also doesn't
prevent race conditions caused by concurrent checkouts.
New create_parent_dirs() would be slightly slower than the original because
it traverses directories from the root whereas fs::create_dir_all() does that
from the leaf and exits when reached to a directory.
This doesn't work yet since write_file() overwrites the existing file, which
will be fixed by the next patch.
I've added a callback parameter to update() just because that's the easiest
option. If we want to report the number of the conflicting files (through
CheckoutStats), the callback interface wouldn't work nicely and the error
handling would have to be moved to the update() body. If we want to make
both check_out() and set_sparse_patterns() ignore EEXIST error, we can
eliminate the calback parameter at all.
There is a bit of duplicated across the three functions for creating a
`Workspace` and `Repo`. This patch reduces that duplication by passing
in a closure.
In addition to reducing duplication, this is a step towards making it
easier to add new backends.
One advantage of our conflict marker style (compared to the usual
3-way markers) is that they provide the user with the diff between the
base and one side so the user doesn't have to do that in their head
(which is how I use 3-way markers anyway). However, since we currently
always use the "first" side for the diff, that diff can be larger than
if we had picked the other side, which makes the marker style worse
than the usual 3-way markers. This has bothered me many times and it's
about time we fix it.
We had a recent bug where we used a repo reference from before we
started a transaction and modified the repo. While it's often safe and
correct to use such references, it isn't always. This patch removes
all such cases. I think it generally makes the code clearer, and
better prepared for #50, if we ever get around to that. I found these
by temporarily making `WorkspaceCommandHelper::start_transaction()`
take a mutable reference.
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.
I think I had not added tests for successful push before because I
thought there was some issue with testing it with libgit2. There *is*
an issue, which is that libgit2 requires the remote to be bare when
it's on local disk, but we can very easily make the git repo bare in
this test.
I also updated the error handling in the `git` module to not let
follow-on errors hide the real error and to not fail if two branches
moved to the same commit.
By adding `ui.open-commits=false` in your config, you can now make `jj
checkout` always create a new working-copy commit on top of the
specified commit. If the config is set, open commits will also appear
in the same color as closed commits in `jj log` etc. This will let
some of us experiment with the new UX before we decide if it's a good
idea or not. I left `jj close` in place because it's useful for
setting a description and creating a new commit in one step.
I didn't mention the new config in the release notes because I hope we
can reach a decision and remove the config before the next release.
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.
I think it's conceptually simpler to create a new commit and set that
commit to be the checkout in each workspace than to check out the
commit in one workspace and edit in the others.
If a file gets replaced by a directory right after list files in a
directory but before we stat the file, we currently crash. Let's
instead treat it as a missing file, using the mechanism introduced for
#258.
This patch makes us treat special files (e.g. Unix sockets) as absent
when snapshotting the working copy. We can consider later reporting
such files back to the caller (possibly via callback) so it can inform
the user about them.
Closes#258
This patch is essentially f6a516ff6d taken further, to also apply to
when we write a symlink or a conflict. As with regular files, these
races seem very unlikely to happen, but I found these cases while
working on #258, so let's fix. Fixing it also means that we don't need
to handle these transition cases in the next patch (when
`file_states()` can indicate that the file is e.g. a socket).
I plan to use this matcher for some future `jj add` command (for
#323). The idea is that we'll do a path-restricted walk of the working
copy based on the intersection of the sparse patterns and any patterns
specified by the user. However, I think it will be useful before that,
for @arxanas's fsmonitor feature (#362).
The new `Visit::Nothing` variant lets us more easily restructure
`DifferenceMatcher::visit()` to make it handle the case of not
removing anything. I think this makes the code a little clearer.
I didn't initially create a `Visit::Nothing` variant because I was
worried that the fact that there then are two ways of expressing this
value (there's also `Visit::Specific` with empty sets). However, the
value is quite useful for pattern matching, so I'm now thinking it's
worth the risk.
If a commit's author field has the placeholder user/email values
(i.e. "(no name configured)" and "(no email configured)"), and they
have now configured their email and username, they probably want us to
update the author field with the new information, so that's what this
patch does. Thanks to durin42@ for the suggestion on #322.
If the source commit becomes empty as a result of
`move/squash/unsquash`, we abandon it. However, perhaps we shouldn't
do that if the source commit is a working-copy commit because
working-copy commits are often work-in-progress commits.
The background for this change is that @arxanas had just started a new
change and had set a description on it, and then decided to make some
changes in the working copy that should be in the parent
commit. Running `jj squash` then abandoned the working-copy commit,
resuling in the description getting lost.
The regular `Display` format is (not surprisingly) more user-friendly,
as pointed out by @yuja.
I also switched to using format strings for these cases, and some
nearby strings for consistency.
We can easily make the `DirEntry` available here, so we can call
`.metadata()` on that instead of on the `Path`. I think that avoids
walking the path. I'm sure this has no significant impact on
performance, but it's also almost as readable.
When we have just written a file to disk on checkout, let's record the
size we expected instead of what we got from `fstat()`. This should
address a race where the file was modified between the time we wrote
it and the time we requested its stat. I just happened to notice this
while going through the code; it seems very unlikely to be noticed in
practice.
When we have just written a file or conflict, we can get metadata for
it via the open file descriptor instead of using the path. That
removes the risk of a race where the file got removed or replaced by
another file type (at least on Unix).
These assertions were there to catch bugs, but when the bugs happen,
the assertions can obsure the underlying error (as @tp-woven found out
on #258). Let's just print errors instead.
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).
We don't need the `config` crate's support for JSON etc., so let's
just enable the TOML feature. (Trying to import all the JSON, RON,
dependencies etc. into Google's source control was a pain.)
This adds a `--reversed` flag to `jj log` to show commits with later
commits further down. It works both with and without the graph.
Since the graph-drawing code is already independent of the
relationship between commits, it doesn't need any updating.
The request to show the log output with more recent commits at the
bottom comes up once in a while (among Mercurial users, and now also
for jj from @arxanas). It's pretty easy to implement by adding an
adapter to the current `RevsetGraphIterator`. It works by first
collecting all nodes and edges into a vector and then yielding them in
reverse order and with reversed edges. That means it's no longer lazy,
but that seems fine since the feature is optional. Also, it's only the
subset of nodes that are in the selected revset that will be
collected.
Making the CLI use the new iterator adapter will come in a later
patch.
It's much easier to update the tests with `insta`.
It also presents you with the bad output including real newlines (a
diff, actually), so we can remove the `println!()` calls we had in
order to get readable output without escaped newlines.
The biggest difference in the API is that fields are now public. The
exception from that is `oneof` fields, which still require setters and
getters.
I couldn't measure any difference in performance. I didn't expect any
difference either, but it's good that it didn't seem to regress. I
timed `jj debug operation <some hash prefix>`, which will read the
whole operation log (to check that the prefix is unambiguous).
Tree merges can currently fail because of a failure to look up an
object, or because of a failure to read its contents. Both results in
`BackendError` because of a `impl From<std::io::Error> for
BackendError`. That's kind of correct in this case, but it wasn't
intentional (that impl was from `local_backend`), and we need to
making errors more specific for better error handling.
I think I copied the name `write_tree()` from Git, but I find it quite
confusing, since it's not clear if it write a tree to the working copy
or reads the working copy and writes a tree to the store (it's the
former).
It seems to me that we have never created a Git index in order to
create a commit, not even in the earliest versions of the code (before
it was moved to Git).
Now that I'm using GitHub PRs instead of pushing directly to the main
branch, it's quite annoying to have to abandon the old commits after
GitHub rebases them. This patch makes it so we compare the remote's
previous heads to the new heads and abandons any commits that were
removed on the remote. As usual, that means that descendants get
rebased onto the closest remaining commit.
This is half of #241. The other half is to detect rewritten branches
and rebase on top.
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.
When committing the working copy, we try to not visit ignored
directories (as e.g. `target/` often is), but we need to visit it if
there are already tracked files in it. I initially missed that in
c1060610bd and then fixed it in a028f33e3b. The fix works by
checking if the next path after the ignored path is inside the ignore
path (viewed as a directory). However, I forgot to handle the case
where there are no paths at all after the ignored path. So, for
example, if the `target/` directory should be ignored and it there
were no tracked paths after `target/` in alphabetical order, we would
still visit the directory. That's why the bug reproduced in the
`git-branchless` repo but not in the `jj` repo (because there are
files under `testing/` and `tests/` here).
Closes#247.
This patch makes room for sparse patterns in the `TreeState` proto
message. We also start setting that value to a list of just the
pattern `.` when we create new working copies. Old working copies
without the sparse patterns are also interpreted as having that single
pattern. Note that this absence of sparse patterns is different from a
present list of no patterns. The latter is a valid state and means
that no paths are included in the sparse checkout.
This adds a matcher that takes two input matchers and creates a new
matcher from them. The composite matcher matches paths matched by the
first matcher but not matched by the second matcher. I plan to use
this for sparse checkouts. They'll also be useful if we add support
for negative patterns to filter e.g. `jj files` by.
Knowing that a matchers matches everything recursively from a certain
directory is useful for various optimizations. For example, it lets
you avoid visiting a directory if you're using a matcher with a
negative condition (so you return what does *not* match).
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.
When a directory is missing in one merge input (base or one side), we
would consider that a merge conflict. This patch changes that so we
instead merge trees by treating the missing tree as empty.
This introduces a `connected(x)` function, which is simply the same as
`x:x`. It's occasionally useful if `x` is a long expression. It's also
useful as a building block for `root(x)` (coming soon).
This release is mostly about the fix for #177, which looks pretty bad
even though I think it is actually harmless. It also has `jj log -p`
contributed by @yuja!
We do it for all the other kinds of objects already. It's useful to
have the path for backends that store objects by path (we don't have
any such backends yet). I think the reason I didn't do it from the
beginning was because we had separate `RepoPath` types for files and
directories back then.
We depend on comparing the workspace root with the Git repo's path to
know if we're sharing the working copy with it. For that to work
reliably, we need the paths to be canonicalized, so that's what this
patch tries to do.
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).
This involved copying `UnresolvedHeadRepo::resolve()` into the CLI
crate (and modifying it a bit to print number of rebased commit),
which is unfortunate.
The function is now pretty simple, and there's only one caller, so
let's inline it. It probably makes sense to move the code out of
`repo.rs` at some point.
It's the transaction's job to create a new operation, and that's where
the knowledge of parent operations is. By moving the logic for merging
in another operation there, we can make it contiuously update its set
of parent operations. That removes the risk of forgetting to add the
merged-in operation as a parent. It also makes it easier to reuse the
function from the CLI so we can inform the user about the process
(which is what I was investigating when I noticed that this cleanup
was possible).
If we have recorded in `MutableRepo` that commits have been abandoned
or rewritten, we should always rebase descendants before committing
the transaction (otherwise there's no reason to record the
rewrites). That's not much of a risk in the CLI because we already
have that logic in a central place there (`finish_transaction()`), but
other users of the library crate could easily miss it. Perhaps we
should automatically do any necessary rebasing we commit the
transaction in the library crate instead, but for now let's just have
a check for that to catch such bugs.
Certain commands should never rewrite commits, or they take care of
rebasing descendants themselves. We have an optimization in
`commands.rs` for those commands, so they skip the usual automatic
rebasing before committing the transaction. That's risky to have to
remember and `MutableRepo` already knows if any commits have been
rewritten (that wasn't the case before, in the Evolution-based
code). So let's just have `MutableRepo` do the check instead.
It's useful for the UI layer to know that there's been concurrent
operations, so it can inform the user that that happened. It'll be
even more useful when we soon start making the resolution involve
rebasing commits, since that's even more important for the UI layer to
present to the user. This patch gets us a bit closer to that by moving
the resolution to the repo level.
