Sometimes a tagged commit is not in any branch on the remote, but we
should still consider them upstream and not include them in the
default log template.
This was reported by @colemickens but now that I think about it, I
remember seeing such commits in the Git core repo (v1.4.4.5 and a few
commits before it were never merged into main).
We don't have a good way of testing this because we don't have a
command for creating tags.
Closes#681
The function has only one caller since 25b922cd0b and it's pretty
small. Inlining also means we can reuse the joined paths created in
it, so I did that by extracting variables for them.
Since 'merges()' just filters the candidates set per item, it doesn't need
a candidates argument. Perhaps, 'merges(x)' could be a predicate to select
merge commits within a subgraph 'x', but I don't know if that would be
useful.
Since 'filter(expr)' is identical to 'expr & filter()', it can be rewritten
in order to minimize the candidates set to be scanned.
The implementation is somewhat similar to revsetlang.optimize() of Mercurial,
but I've split the tree rewriting logic to several steps. I think that's good
for maintainability and should help us conclude that a recursion will
eventually terminate.
This helps to match '(filter, _) | (_, filter)' to rewrite the expression
tree. Only one predicate is allowed for now, but I think it can be extended
to internalize 'f(c) & g(c)' as '(g*f)(c)' to eliminate redundant lookup
of commit object.
I had `init.defaultBranch = main` in my global config (just being
rolled out internally at Google, it seems), which made
`test_import_refs_reimport_head_removed()` and
`test_fetch_initial_commit()` fail. This fixes it.
Since d56ae79d3f, `WorkingCopy` no longer reads `.gitignores`
directly from `$HOME/.gitignore`, so we don't need the workaround to
prevent it in the tests.
More workspace-derived parameters will be added, and I don't think wrapping
with Option for each makes sense because all parameters should be available
if workspace exists.
It seems a bit invasive that RepoPath constructor processes an environment
like cwd, but we need an unmodified input string to build a readable error.
The error could be rewrapped at cli boundary, but I don't think it would
worth inserting indirection just for that.
I made s/file_path/fs_path/ change because there's already to_fs_path()
function, and "file path" in RepoPath context may be ambiguous.
The native backend is just a proof of concept and there's no real
reason to use it other than for testing, so let's reduce the risk of
accidentally creating repos using it.
Previously an expression 'foo-bar-' failed to parse because
1. try first rule: 'foo-bar-' matches (identifier_part+ ~ '-')+, but the
trailing '' doesn't match identifier_part+
2. fall back to second rule: 'foo' matches identifier_part+
=> (identifier 'foo')
Instead, we need to consume as much (identifier_part ~ '-' ~ ...) as possible
before falling back to the identifier_part rule.
I think the trailing + of identifier_part+ is redundant, so removed it as
well.
Let WorkspaceCommandHelper clone it. WorkspaceCommandHelper could return
workspace_id by reference, but doing that would introduce noisy .clone()
calls and lifetime mess.
For consistency with the tree_state handling. This isn't that simple and
concise compared to the tree_state one, so I'm fine to drop the series.
I've extracted {operation_id, workspace_id} pair so these values can be
safely initialized by OnceCell. The extracted struct is named after the
"checkout" file.
Here OnceCell<T> serves as RefCell<Option<T>>, but it doesn't require runtime
Ref/RefMut wrapper. This allows us to get rid of some .clone() calls needed to
hide Ref<_> from public interface.
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.
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.
