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.
I often redirect the jj output to pager, so I set ui.color = "always" in
config file. This patch allows me to remove such config, and instead specify
--color=always only when needed.
This allows us to reconfigure ui with the parsed --color option.
I tried if implementing formatter.into_output() would make sense, and it
turned out to be a bit mess as the Formatter trait doesn't know the lifetime
of the underlying output. Ui could own the formatter behind Color|Plain enum
variant in place of Box<dyn>, but that seemed to unnecessarily change the
Ui interface with little benefit.
Since we just want to reinitialize the ui at very early stage, I think
recreating the formatters is the simplest way to go.
Regarding the formatter API, I have a feeling that Ui should keep the
underlying stdout/stderr/color_map instead of the stateful formatters.
ui.stdout_formatter() will return a temporary formatter, and maybe dropping
it will automatically clear labels. This would also means the temporary
formatter could be created with stdout.lock().
ColorChoice implements FromStr so it can be a clap argument. It could
leverage the ArgEnum derive macro, but I don't think the ui is the layer
which can depend on clap.
According to the NO_COLOR FAQ, "user-level configuration files [...] should
override $NO_COLOR." https://no-color.org/
Unfortunately this makes it harder to test the $NO_COLOR behavior since the
test environment isn't attached to a tty. We could allocate a pty or
LD_PRELOAD shim to intercept isatty(), but I feel it would be too much to do.
https://github.com/assert-rs/assert_cmd/issues/138
We have `edition = "2021"` in our `Cargo.toml` files, so it makes
sense to have `rustfmt` use the same edition. That will let us format
code using `async` and `await`. I noticed this while looking at
@arxanas's fsmonitor PR.
This patch prevents perhaps pushing commits with an empty description
or the placeholder "(no user/email configured)" values for
author/committer.
Closes#322.
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.
None of our subcommands have any arguments at the level of the
subcommand family (e.g. no `jj git --foo fetch`), so we don't need to
wrap the `Subcommand` attribute in an `Args` attribute.
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.
We had tests for `jj move` and `jj squash`, but not for `jj
unsquash`. I'm about to add a `--keep` option, so let's add tests for
the current behavior first.
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).
Before this change, `jj new` would check out the new commit only if it
was created on top of the current commit. I never liked that
special-casing, and after thinking more about how the open/closed
should work (see discussion #321), I think we want `jj new` to behave
similar to how `git/hg checkout` works, so it can effectively replace
the current `jj checkout` command for the use case of starting new
work on top of an existing commit.
The help text said you can `jj abandon; jj co @-` to go to the parent
commit (it it's an open commit), but `jj abandon` already takes you to
the parent.
