This basically removes 'a lifetime from these wrappers, and add trait bounds
instead. I have no idea which version would look less scary, but let's see.
I also added trait bounds to constructor functions. They aren't strictly
required, but help type inference of closure (and will probably improve
an error message.)
Fixes#787
If `jj squash` is run on an empty commit, it fails with "Error: No changes selected"
With this change such squash command will behave like `jj abandon`.
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.
