I think the CLI should check if the target of `jj edit` is rewritable
before calling the library to update the repo. The other commands
already do that. Then, if calling `MutableRepo::edit()` fails, it's
always an internal error, which makes error handling simpler in coming
commits.
I considered adding .optional() helper to lift Result to Result<Option<_>>,
but it's much simpler to expect all config sections (and maybe all keys?)
are defined by default.
The error message is a bit cryptic, but it should be improved by the following
PR if accepted.
https://github.com/mehcode/config-rs/pull/413
When implementing FormattablePropertyTemplate, I tried a generic 'property: P'
first, and I couldn't figure out how to constrain the output type.
impl<C, O, P> Template<C> for FormattablePropertyTemplate<P>
where
P: TemplateProperty<C, O>, // 'O' isn't constrained by type
O: Template<()>,
According to the book, the problem is that we can add multiple implementations
of 'TemplateProperty<C, *>'. Since TemplateProperty is basically a function
to extract data from 'C', I think the output parameter shouldn't be freely
chosen.
https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
With this change, I can express the type constraint as follows:
impl<C, P> Template<C> for FormattablePropertyTemplate<P>
where
P: TemplateProperty<C>,
P::Output: Template<()>,
This is an example of labeled output of structured value types. I think
"{name} <{email}>" is a good default formatting, but I should note that
the signature also contains timestamp field.
Now we have 'impl Template<()> for String', 'LiteralTemplate(String)' is
a bit redundant. Let's generalize it for any 'Template<()>'. I noticed
'ConstantTemplateProperty' serves as a similar role, so unified these.
This is a slightly better version of the one I described in:
https://github.com/martinvonz/jj/pull/1098#issuecomment-1399476487
These impls will replace coerce_to_string() to support labeled outputs.
We could allow arbitrary context type 'C', but I feel uncomfortable with
that. So let's start with () until we find it doesn't work out.
This makes us sanitize ANSI escape bytes in the output if it goes to
the terminal, even when it's not colored (by us), such as when using
`--color=never`. That means that e.g. `jj cat
tests/test_commit_template.rs` will not be colored, but `jj cat
tests/test_commit_template.rs | cat` will be. Sanitizing output sent
to the terminal might help reduce some security threats based on
hiding content by using ANSI escapes.
We could add a config option for sanitizing the output, but I'm not
sure it'll be useful.
Since `ColorFormatter` itself outputs ANSI escape codes, we should not
let the caller also include ANSI escape codes. This commit makes
`ColorFormatter` replace them by a unicode "␛".
For graphlog output, we use a separate formatter for each commit. The
output from the formatter is written to a buffer in memory. Then we
write it to graphlog renderer. Since the buffer already has any color
codes, we should not pass it through the top-level formatter (the one
bound to stdout). It hasn't mattered much so far, but it will when we
start sanitizing output written to formatters. This commit adds a
method to the `Formatter` trait for getting access to the raw
underlying output. It also starts passing that output to the graphlog
renderer.
We add a top-level `log` label to the output from `jj log`, but we
never define any colors based on it. More importantly, it was
inconsistent between the graph and non-graph cases. When showing the
graph, any colors set based on it would only apply to the graph itself
[1] because we use a separate formatter for each commit in the
graphlog and that formatter didn't inherit the `log` label. So let's
just remove the label for now. We can consider adding it back for each
commit later. That's what we do for `jj op log`, but it's simpler in
that case because it doens't have a `--no-graph` version.
[1] Well, mostly; it would also apply to any uncolored element
immediately right of the graph.
Since IdIndex is immutable, we don't need fast insertion provided by BTreeMap.
Let's simply use Vec for some speed up. More importantly, this allows us to
store multiple (ChangeId, CommitId) pairs for the same change id, and will
unblock the use of IdIndex in revset::resolve_symbol().
Some benchmark numbers (against my "linux" repo) follow.
Command:
hyperfine --warmup 3 "jj log -r master \
-T 'commit_id.short_prefix_and_brackets()' \
--no-commit-working-copy --no-graph"
Original:
Time (mean ± σ): 1.892 s ± 0.031 s [User: 1.800 s, System: 0.092 s]
Range (min … max): 1.833 s … 1.935 s 10 runs
This commit:
Time (mean ± σ): 867.5 ms ± 2.7 ms [User: 809.9 ms, System: 57.7 ms]
Range (min … max): 862.3 ms … 871.0 ms 10 runs
I think this is a remainder of 68ad712e12 "Templater: Combine Change and
Commit id templates." It doesn't make sense that description.short() prints
the first 12 characters.
With my "jj" work repo, this saves ~4ms to show the log with default revset.
Command:
JJ_CONFIG=/dev/null hyperfine --warmup 3 --runs 100 \
"jj log -T 'commit_id.short_prefix_and_brackets() \
change_id.short_prefix_and_brackets()' \
--no-commit-working-copy"
Baseline (a7541e1ba4):
Time (mean ± σ): 54.1 ms ± 16.4 ms [User: 46.4 ms, System: 7.8 ms]
Range (min … max): 36.5 ms … 78.1 ms 100 runs
This commit:
Time (mean ± σ): 49.5 ms ± 16.4 ms [User: 42.4 ms, System: 7.2 ms]
Range (min … max): 31.4 ms … 70.9 ms 100 runs
This iterator will be used to merge neighbor commit ids across segments.
resolve_prefix() is simplified to non-short-circuiting loop. I think that's
fine because visiting parents is cheap, and the costly operation here is
segment_resolve_prefix().
entry_by_pos() could also be migrated to iterator, but I leave the unsafe
bits there.
ReadonlyIndex implementation leverages the existing binary search
function. MutableIndex one is basically the same as repo::IdIndex.
Shortest prefix length could be calculated for each segment, but I think
returning neighbors is better for testing.
This is ugly, but we need a special case because root_change_id and
root_commit_id aren't equal but share the same prefix bytes. In practice,
no one would care for the shortest root id prefix, but we'll need to deal
with a similar problem when migrating prefix id resolution to repo layer.
This helps us to migrate commit_id index to ReadonlyIndex. For large
repositories, this also reduces initialization cost, but that's not the main
intent of this change.
https://github.com/martinvonz/jj/pull/1041#issuecomment-1399225876
common_hex_len() and iter_half_bytes() are added to backend.rs since more
call sites will be added to index.rs, and I feel index.rs isn't a good place
to host this kind of utility functions.
I made it a free function. Alternatively, the root id could be instantiated
by and obtained through backend, but I don't think we'll need such level of
abstraction.
I'm going to add a workaround for shortest prefix calculation of the root ids,
where this function will be used.
The flake currently has some odd behavior, such as copying .jj and
target/, and treating the jj src as an input to the dev shell. This
avoids making life rough for direnv users while those are outstanding.
Make op resolution a closed operation, powered by a callback provided by the
caller which runs under an internal lock scope. This allows for greatly
simplifying the internal lifetime structuring.
