I don't think Workspace::load() should be permissive in that regard.
WorkspaceLoader could provide such function, but I feel it's more like
CLI business. CLI can also look for parent '.git' directory to suggest
'jj init --git-repo=..' if needed.
The divergent changes are marked with ?? (I found a single ? a bit easy to
overlook), and this should be consistent.
Ideally, the conflicted branches would also be red, but this takes a bit
larger changes to `templater.rs`: another `Property` as well as changes to
https://github.com/martinvonz/jj/blob/7f9a0a28/src/template_parser.rs#L385-L395
This allows us to load workspace by using &Result<T, CommandError>. Even
though load_workspace() wouldn't be called more than once, consuming self
there is annoying.
I don't see a good reason to let e.g. "added diff" to match added text
inside a diff when we already allow "diff added" for that. Allowing
both means that we have to decide which should take precedence. With
the recent change to add labels for methods, we no longer depend on it
for the "timestamp author" case ("author timestamp" now
matches). Thanks to @yuja for noticing that dependency.
We now already have the "timestamp" label from the auto-labeling of
method calls.
Without the use of the "email" label in the previous commit, we would
now emit an unnecessary color code when the "author" label is added
but before the "timestamp" is added. So that patch wasn't just about
simplifiying the config :)
I'm going to make `parse_method_chain` also return a list of labels to
add, so we can make e.g. `author.timestamp()` automatically get
labeled with both "author" and "timestamp".
It's clearly the parser's job to split labels in a string provided by
the user. This patch moves the splitting we were doing in
`LabelTemplate` and `DynamicLabelTemplate` to the parser. In the
former case, the string isn't even provided by the user and it doesn't
contain whitespace, we can drop the splitting altogether.
The `:` was a bit noisy. With upcoming color support, it seems unnecessary.
This should also be a little better for any tools that want to parse the
output.
Finally, `insta` also seems to want to rewrite test snapshots to be shorter.
They seem equivalent.
This will match `jj`'s behavior of being unable to resolve such conflicts or to
show readable diffs for them, in the pre-#978 state.
This is a fixup of 621293d7c6.
Since per-repo config may contain CLI settings, it must be visible to CLI.
Therefore, UserSettings::with_repo() -> RepoSettings isn't used, and its
implementation is nullified by this commit.
#616
It's unclear whether parse_args() or its caller should update LayeredConfigs.
--config-toml is processed by callee to apply --color early. -R/--repository
will be processed by caller since it will instantiate WorkspaceLoader.
Maybe --config-toml can be removed from EarlyArgs, and handle_early_args()
just updates ui state based on --color argument?
Thinking of config precedence, we'll probably need to store "base" (default +
env_base + user) config and "override" (env_overrides + --config-toml) config
separately to support repo config. Suppose env_overrides() is a temporary
value, it's better to override any file-derived values with $JJ_ env.
UserSettings will be instantiated after both user and repo configs are
loaded. We might want to add a wrapper for CLI settings, but I have no idea
how that should be structured. Let's use bare config::Config until then.
Still UserSettings is cloned to WorkspaceCommandHelper, but I think this is
slightly better since CommandHelper and WorkspaceCommandHelper are scoped
based on call stack. Perhaps, UserSettings can be shared by Arc or immutable
reference.
This prepares for migration from ui.settings() to command.settings(). We could
instead add workspace_command.settings(), but I'm not sure which would be less
bad. Anyway, these functions live in command layer, so taking CommandHelper
makes sense.
This is a temporary workaround. I think UserSettings can be constructed after
user, repo, and --config-toml values are all set. parse_args() will probably
return config::Config or its wrapper instead of mutating UserSettings.
I'm going to remove owned UserSettings from Ui so that UserSettings can be
instantiated after both user and repo configs are loaded. ui.cwd() belongs
to the same category (random environment stuff), and Ui doesn't depend on it,
so let's remove it first from Ui.
I'm not pretty sure if CommandHelper and WorkspaceCommandHelper should be
a permanent home for cwd and settings, but it works for now as CommandHelper
is immutable.
editor_name_from_settings() needs &mut Ui to show hint, but we're lucky that
the caller has a clone of UserSettings. This is one reason I want to remove
ui.settings(). A function taking (&mut Ui, &UserSettings) sounds reasonable,
but we can't pass in (&mut ui, ui.settings()) to it.
This and the subsequent patches prepare for the removal of ui.settings().
Ui will be a consumer of UserSettings (or config::Config) to make it clear
that Ui can be constructed before UserSettings is fully set up.
parse_args() doesn't know all parameters needed to set up CommandHelper,
and we have to set StoreFactories later. That's okay for now, but I'm going
to add more parameters derived from CliRunner.
The next commit will move CommandHelper construction to the caller of
parse_args(). Without this change, parse_args() would have to return a tuple
of 3 elements, which seemed a bit too much.
This allows us to chain custom dispatch functions. If CommandHelper were
moved or passed by mutable reference, weird thing could happen depending
on the call order.
parse_args() will probably become more involved to deal with --config-toml,
-R, and repository configs. This builder will hopefully allow us to move
things around without changing the high-level interface.
Can be used with tools like taplo-lsp to show hints & validation in
editors/IDEs. Won't apply automatically to config files until it's
submitted to schemastore.org, but in the meantime it can be used via a
schema directive
(https://taplo.tamasfe.dev/configuration/directives.html#the-schema-directive)
or other manual config mechanism.
Context in #879.
Since ui object is needed to report read_config() error, it makes sense to
create ui first without fallible user configuration. Ui::for_terminal() will
be replaced with this function and ui.reset(read_config()?).
Default::default() is also added to silence clippy. If we prefer, Ui::new()
can be replaced with Ui::default().
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.
When we fail to read the user's config, it seems obviously better to
use the default config than to not use it. It doesn't matter yet, but
it will matter when I've moved color configs out of `formatter.rs` and
into a `.toml` file. Without this change, we'd lose the default
coloring of the error message for config errors.
The divergent label is most relevant when the user is about to
refer to a commit by its change id.
It's also good to put it far from the `conflicts` label, to
reduce confusion between very different concepts that have
similar names.
Finally, I think it is a feature rather than a bug that the
`divergent` label now upsets the alignment of different lines
of `jj log`.
Since CR+LF vs LF things shouldn't matter in commit description, it's probably
better to normalize newline characters.
In Mercurial, ui.edit() and changelog.stripdesc() handle line normalization,
and trailing newlines are stripped. In Git, cleanup_message() handles that,
and the last newline is added after stripping trailing newlines.
Otherwise the description set by -m would differ from the one set by editor.
This fixes test_describe() which says "make no changes", but previously "\n"
would be added by the second "jj describe".
As you can see, almost all hashes change in CLI tests. This means in-flight
PRs will need to be rebased to update insta snapshots.
Description text could be normalized by CommitBuilder, but the caller would
have to normalize it beforehand to compare with the current description, so
we would need an explicit function anyway. Another idea is to add a newtype
that represents a normalized description, and make CommitBuilder require it.
Commit::description() will return &Description in place of &str to ensure
that commit.description() == raw_str wouldn't compile.
Git CLI provides --cleanup=<mode> option to switch normalization rules, but
I don't think we'll need such feature.
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.
After I changed `merge-tools.vimdiff.program` to `vim`, using
`vimdiff` as a diff editor wouldn't work at all.
Out of the box, it's still not a good experience. I included a
recommendation of a plugin to install to make it better.
The implementation has some hoops to jump through because Rust does not allow
`self: &Arc<Self>` on trait methods, and two of the OpHeadsStore functions need
to return cloned selves. This is worked around by making the implementation type
itself a wrapper around Arc<>.
This is not particularly note worthy for the current implementation type where
the only data copied is a PathBuf, but for extensions it is likely to be more
critical that the lifetime management of the OpHeadsStore is properly
maintained.
I ran an upgraded Clippy on the codebase. All the changes seem to be
about using variables directly in format strings instead of passing
them as separate arguments.
"jj log -p --summary" now shows summary and color-words diff, like
"hg log -p --stat".
Handling of "-p" is tricky. I first considered "-p" would turn on the default
diff output, but I found it would be confusing if "jj log -p --git" showed
both color-words and git diffs. So the default format is inserted only if
no --git nor --color-words is explicitly specified.
This allows us to generate diff in different formats. There are various ways
to achieve that:
a. build TreeDiffIterator for each format (this patch)
b. make TreeDiffIterator clonable
c. collect TreeDiffIterator and reuse the resulting vec
(a) and (b) are practically the same. (c) would be more efficient if building
and iterating TreeDiffIterator were expensive, but I don't think it is because
Tree is cached by Store. If we add $GIT_EXTERNAL_DIFF-like feature, we'll
probably need Tree objects to snapshot them to /tmp. So I chose (a).
The author timestamp is rarely useful (in my experience). The
committer timestamp, on the other hand, can be useful for
understanding when a change was most recently modified. IIRC, I
originally picked the author timestamp to match the email (which is
the author's), but it's probably not confusing to use the author email
and the committer timestamp. I suspect few users will even reflect on
it.
Clap bails parsing when an "error" is encountered, e.g. a subcommand is missing,
"--help" is passed, or the "help" subcommand is invoked. This means that the
current approach of parsing args does not handle flags like `--no-pager` or
`--color` when an error is encountered.
Fix this by separating early args into their own struct and preprocessing them
using `ignore_errors` (per https://github.com/clap-rs/clap/issues/1880).
The early args are in a new `EarlyArgs` struct because of a known bug where
`ignore_errors` causes default values not to be respected
(https://github.com/clap-rs/clap/issues/4391 specifically calls out bool, but
strings may also be ignored), so when `ignore_errors` is given, the default
values will be missing and parsing will fail unless the right arg types are used
(e.g`. Option`). By parsing only early args (using the new struct) we only need
to adjust `no_pager`, instead of adjusting all args with a default value.
The number of lines in the diff output is unchanged.
This makes diffs a little more readable when the "..." would otherwise hide a
single line of code that helps in understanding the surrounding context lines.
This change mostly rearranges the loop that consumes the diff lines, so it can
buffer up to num_context_lines*2+1 lines instead of just num_context_lines.
There's a bit of extra code to handle times when a "..." replaces the last line
of a diff.
Note that `jj diff --git` is unchanged, and will still output `@@` lines that
replace a single line of context.
This fixes the bug described in the previous commit.
Because we now print the message about failed exports also while
snapshotting, we may end up reporting it twice on one command. I'm not
sure it's worth worrying about that. We can deal with that later if it
turns out to be a common complaint.
Unfortunately, config::Value is lax and '[7]' could be parsed as '["7"]'.
I don't like it, but I think that's actually better for consistency as we
use config.get_string() in various places.
When a workspace's working-copy commit is updated from another
workspace, the workspace becomes "stale". That means that the working
copy on disk doesn't represent the commit that the repo's view says it
should. In this state, we currently automatically it to the desired
commit next time the user runs any command in the workspace. That can
be undesirable e.g. if the user had a slow build or test run started
in the working copy. It can also be surprising that a checkout happens
when the user ran a seemingly readonly command like `jj status`.
This patch makes most commands instead error out if the working copy
is stale, and adds a `jj workspace update-stale` to update it. The
user can still run commands with `--no-commit-working-copy` in this
state (doing e.g. `jj --no-commit-working-copy rebase -r @ -d @--` is
another way of getting into the stale-working-copy state, by the way).
I want to be able to create a `WorkspaceCommandHelper` without
snapshotting the working copy. That will be useful when adding a
command for updating a stale working copy.
This patch makes the function checking for a stale working copy return
a specialized error instead of `CommandError`. That has several advantages:
* It makes it easier to change the behavior so we don't automatically
update a stale working copy until the user explicitly tells us.
* It allows us to move the function to the library crate, to be
shared by non-CLI UIs (and server applications).
Similar to the previous commit, the checking for stale working copy is
a big part of `commit_working_copy()` and it's logically quite
separated from the rest, so let's extract it to a function.
The code for creating a `WorkspaceCommandHelper` for a given
`Workspace` is a well-separated part of the large
`workspace_helper()`, so lets extract it to a function.
Also allows several paths to be specified. By default, `jj resolve`
will find the first conflict that matches provided paths (if any)
and try to resolve it.
It seems like I forgot to update the `jj status` output when I decided
(years ago?) that the changes in a commit should always be compared to
the auto-merged parents. I was very confused before I realized that
`jj status` was showing the diff summary against the first parent. I
suppose the fact that `jj status` lists only one parent should have
been a hint. Thanks to ilyagr@ for finding this odd behavior. This
patch fixes it by making the command list all parents, and changes the
diff summary to be against the auto-merged parents.
The `print` command shows the contents of a file, so that is obviously
often more than a page long. Both `hg cat file` and `git show
HEAD:file` page the output.
The output from `files` is often longer than a screen, so the pager is
useful, even though this command is probably used mostly by
scripts. As with `status`, `hg` pages its output, but `git` doesn't.
The status output may be long, so the pager can useful. Now that we
pass `-F` to the pager by default, it should also be fine to use the
pager for short output. For reference, `hg` pages `status` output, but
`git` doesn't.
As dbarnett@ reported on #9, our default of `less`, combined with our
default of enabling color on TTYs, means that we print ANSI codes to
`less` by default. Unless the user has set e.g. `$LESS=R`, `less` is
going to escape those codes, resulting in garbage like this:
```
@ ESC[1;35mbb39c26a29feESC[0m ESC[1;33m(no email configured)ESC[0m ESC[1;36m2022-12-03....
```
I guess most of us didn't notice because we have something like
`$LESS=FRX` set.
This patch changes our default from `less` to `less -FRX`. Those are
the flags we're using for our internal hg distribution at Google, and
that has seemed quite uncontroversial.
I added a pointer from the changelog to the tracking issue while at
it.
If this new option is not specified, we start with empty output
file and trust the merge tool did a complete merge no matter
what the file contains.
Includes tests.
This command uses an external merge tool to resolve conflicts
simple enough that they can be resolved with a 3-way merge.
This commit provides a very basic version of `jj resolve` that
is hardcoded to use vimdiff.
This also slightly changes the errors of the Diff Editor, so that
both the diff editor and `jj resolve can share an error type.
It should be more reliable than parsing a command string into array.
Also updated some of the doc example to use array syntax. I don't think
"C:/Program Files" was parsed properly, but might work thanks to Windows
magic.
It implements Deserialize because config.get() requires that. We could instead
add TryFrom<config::Value>, but we'll need Deserialize anyway if we want to
parse a struct containing FullCommandArgs.
I don't know if src/config.rs is the right place, but I feel it's slightly
better than messing up ui.rs.
The example for the `-b` flag was completely incorrect. It looks like
I have copied the example from `-r` and then forgotten to update
it. This fixes that, and also adds some more commits to the example to
hopefully clarify.
Teach Ui's writing functions to write to a pager without touching the
process's file descriptors. This is done by introducing UiOutput::Paged,
which spawns a pager that Ui's functions can write to.
The pager program can be chosen via `ui.pager`. (defaults to Defaults to
$PAGER, and 'less' if that is unset (falling back to 'less' also makes
the tests pass).
Currently, commands are paginated if:
- they have "long" output (as defined by jj developers)
- jj is invoked in a terminal
The next commit will allow pagination to be turned off via a CLI option.
More complex pagination toggling (e.g. showing a pager even if the
output doesn't look like a terminal, using a pager for shorter ouput) is
left for a future PR.
We'll add a variant that isn't a pair. Also add a function to create a
new UiOutput::Terminal, we will create this variant in a few places
because we want to fall back to it.
I can't see any reason the user would want to specify revisions
matching the empty string, so let's disallow it. I created a custom
type for revision arguments instead of repeating `value_parser =
NonEmptyStringValueParser::new()`.
If the user creates a branch with an empty name, it seems very likely
to be an accident. Let's help them realize that by erroring out.
I didn't add the same checks to `jj branch delete`, since that would
make it hard to delete a branch with an empty name from existing
repos.
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.
Aliases are loaded at WorkspaceCommandHelper::new() as it's easier to warn
invalid declarations there. Not all commands use revsets, but many do, so
I think it's okay to always pay the loading cost. Parsing the declaration
part (i.e. a symbol) should be fast anyway.
The nested error message isn't super readable, but seems good enough.
Config syntax to bikeshed:
- naming: [revset-alias] vs [revset-aliases] ?
- function alias will need quotes: 'f(x)' = 'x'
The CLI will load aliases from config, insert them one by one, and warn if
declaration part is invalid. That's why RevsetAliasesMap is a public struct
and needs to be instantiated by the caller.
This adds a warning whenever export to the backing Git repo fails,
whether it's by an explicit `jj git export` or an automatic export. It
might be too spammy to print the message after every failed command in
the colocated case, but let's try it and see.
This is a simple workaround to make operation IDs predictable (so they
don't include the path to `.../target/debug/jj` or similar). The full
path can probably be useful for troubleshooting, but we can deal with
that later if we see a need for it. It might also be less confusing
for Windows users if it says "jj.exe".
It would be nice to be able to use snapshot testing and not have to
parse the output of `jj op log`. This patch lets us do that by
providing a new environment variable and config for overriding the
timestamps. Unlike `operation.hostname` and `operation.username`,
these are only meant for tests.
This makes the tests more hermetic, even though I don't think the
default values (taken from `whoami`) can break any tests (then we
would have already seen them break). Now we just need to make the
operation log's timestamps predictable and then we can start using
operation IDs in snapshot tests.
The `atty` crate seems unmaintained. There's
https://rustsec.org/advisories/RUSTSEC-2021-0145 filed against it,
which `cargo-deny` complains about. A fix for that has been open for
well over a year without being fixed
(https://github.com/softprops/atty/pull/51). It turns out the
functionality is also available via the `crossterm` crate (thanks,
@yuja), which we already depend on.
Since we also depend on `atty` via `clap`, I also added an exception
to the `cargo-deny` config.
There are no "non-normal" files, so "normal" is not needed. We have
symlinks and conflicts, but they are not files, so I think just "file"
is unambiguous.
I left `testutils::write_normal_file()` because there it's used to
mean "not executable file" (there's also a `write_executable_file()`).
I left `working_copy::FileType::Normal` since renaming `Normal` there
to `File` would also suggest we should rename `FileType`, and I don't
know what would be a better name for that type.
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.
We have talked about showing the commit ID only for divergent changes
because it's generally easier to work with the change ID, and it's
less likely to result in a divergent change. However, it's useful to
have the commit ID available for pasting into e.g. a commit message or
the GitHub UI. To try to steer users towards using the change ID, this
commit moves the commit ID off to the right in the log output.
I put it just after the "divergent" field, because that makes it close
to how I imagine it would look if we decided to hide the commit ID
except for divergent changes. I was thinking that could be rendered as
"divergent (abc123)". So if we add config to hide the commit ID, then
it would be rendered almost the same for divergent commits (just with
the added parentheses). It would also make sense to replace the
"divergent" field by a question mark on the change ID, since change
IDs basically behave like branches. If we do that, then the placement
of the commit ID I picked in this commit does not make sense.
Given how easy this was, I can't believe I didn't make the change
sooner.
I haven't updated the screenshots in the readme because I plan to make
some further changes to the default template. I'll update them after
those changes.
I'm going to add other error variants for when we fail to read/write
to disk, and I don't think we need to have a custom message for each
in the CLI crate. It's easier to pass along the message from the lib
crate.
(`ConflictedBranch`, on the other hand, is an expected error so I
think we should continue to let let the CLI crate define the error
message for it.)
Several lines of red text can be overwhelming, and makes it harder to
tell the hint from the error. Let's separate the hint from the error
instead. This matches what hg does. Having the hints separated out
also means that we could have a single config to turn them off.
I want to add a separate field for a hint, so that can be printed in a
different color (than red). Having a factory function is useful then,
since most call sites don't want to pass a hint. Also, by using a
factory function instead of using the constructor directly means that
we can accept `&str` arguments instead of forcing the caller to
convert a string literal to `String`.
It's useful to know when you've modified a branch that exists on a
remote. A typical case is when you have pushed a branch to a remote
and then rewritten it. This commit adds an indication in the
`branches` template keyword. A branch that needs to be pushed to a
remote now has a `*` at the end (similar to how conflicted branches
have a `?` at the end). Note that the indication only considers
remotes where the branch currently exists, so there won't be an
indication that the branch has not been pushed to a remote.
Closes#254
Example:
JJ: Enter commit description for the first part (parent).
Better split commit message
JJ: This part contains the following changes:
JJ: M src/formatter.rs
JJ:
JJ: Lines starting with "JJ: " (like this one) will be removed.
Unfortunately, TOML requires quotes around the argument. So, the
usage is `jj --config-toml ui.color=\"always\"` in bash. The plan is
to eventually have a `--config` option with simpler syntax for
simple cases.
As discussed in https://github.com/martinvonz/jj/discussions/688.
It seems very likely that we're going to remove support for open
commits, but it's still useful to have a `commit` command that lets
the user enter a description and starts a new change. Calling it
`commit` seems good to make the transition from other VCSs simpler.
Use clap::Command::try_get_matches_from() to parse args, and handle the
resulting error as a CommandError. The exit code 2 matches the exit code that
clap would output (this is tested by test_alias).
With this new abstraction, teach the clap output to respect "ui.color"
(--color doesn't seem to be understood). We'll also make use of it when
we route output through the pager.
The "errors" returned by try_get_matches_from() can be found at
https://docs.rs/clap/latest/clap/error/enum.ErrorKind.html. I've tested:
- no args
- --help
- help subcommand
- --version
- subcommand with --help
- subcommand with bad args
This lets us write to stderr, but unlike write_error(), this won't write
with formatting. This will be used for preformatted strings, e.g. those
coming from clap.
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
This was actually what I meant in my code-review comment but I used
the wrong word ("clip") :) I wasn't going to bother changing it, but
Clippy nightly just started warning about it.
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.
Unlike matcher_from_values(), this function is trivial and isn't widely
used. Let's simply do .map() and .collect().
TODO comment is relocated to matcher_from_values(). I think glob and
fileset-like stuff will be added to the matcher parser, not to the
'Path -> RepoPath' function. And it's probably implemented in lib crate.
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.
