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.
