I would expect `set_up_fake_[diff_]editor()` to create an empty script
but it turns out they didn't even create the files. That means that
the caller needs to write an empty script to them if they want the
fake editors to not do anything. Let's instead write the empty
scripts, for a less surprising behavior.
revset::resolve_change_id() for ReadonlyRepo will be replaced with this
implementation. This doesn't mean revset query will speed up. A trivial
query will become slower due to the initialization cost of the change id
index. "jj log -r hex" will get faster since we have to pay the cost anyway.
Benchmark numbers (against my "linux" repo):
Command:
hyperfine --warmup 3 --runs 20 \
"jj log -r $hex -T '' --no-commit-working-copy --no-graph"
Linear search (e874570947):
Time (mean ± σ): 223.9 ms ± 16.2 ms [User: 181.2 ms, System: 42.7 ms]
Range (min … max): 207.7 ms … 247.6 ms 50 runs
Building IdIndex:
Time (mean ± σ): 855.0 ms ± 21.7 ms [User: 788.4 ms, System: 66.6 ms]
Range (min … max): 822.6 ms … 927.5 ms 50 runs
Building IdIndex, but hacked to store SmallVec<[u8; 20]>:
Time (mean ± σ): 406.1 ms ± 15.9 ms [User: 354.1 ms, System: 52.0 ms]
Range (min … max): 382.2 ms … 428.6 ms 50 runs
For my "jj" work repo, changes are < ~1ms.
I've preferred "working-copy commit" over "checkout" for a while
because I think it's clearer, but there were lots of places still
using "checkout". I've left "checkout" in places where it refers to
the action of updating the working copy or the working-copy commit.
`SimpleOpHeadsStore` currently stores its files in
`.jj/repo/op_heads/simple_op_heads/`. The `.jj/repo/op_heads/type`
file indicates the type of op-heads backend. If that contains
"simple_op_head_store", we use the `SimpleOpHeadsStore`
backend. There's no need for the `simple_op_heads` directory to also
indicate the type of backend in its name. I kept just the `heads` in
the name to make it less redundant with the parent directory (which is
`op_heads)`. We could alternatively call the directory `values` or
similar.
I didn't make this change before because there are many immutable/mutable
borrow conflicts. Now most of the problems are consolidated to the transaction
wrapper, we can simply make it return a reference.
This ensures that helper methods that depend on repo aren't used by mistake
while transaction is in progress. Still it provides an escape hatch to invoke
e.g. select_diff() with the base repo, but such invocations are more explicit.
Some MutableRepo methods are proxied through the wrapper to get around
borrowing errors.
This should eliminate lifetime issue I would have to deal with in the
next commit. evaluate_revset() only borrows RepoRef, but such precise
dependency can't be expressed as of now.
Since this function depends on both index and view, it can't be moved to
one of the storage objects. If we go forward with this approach, some
revset::resolve_*() functions will also be migrated to RepoRef.
This patch slightly changes the function name since a "prefix" might have
various meanings.
This should fix the panic in the case reported in #1107. It's a bit
hard to reproduce because we normally notice the missing commit when
we snapshot the working copy, but it's possible to reproduce it using
`--no-commit-working-copy`.
I suspect the added test is too brittle because it checks the exact
error message. On the other hand, it might be useful to have one test
case like this so we catch accidental changes in the format.
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
