Now that there's a single implementation of `Revset`, I think it makes
more sense for `is_empty()` to be defined there. Maybe different
revset engines have different ways of implementing it. Even if they
don't, this is trivial to re-implement in each revset engine.
As the comment above `ToPredicateFn` says, it could be a private
type. This commit makes that happen by making the private `Revset`
implementations (`DifferenceRevset` etc.) instead implement an
internal revset type called `InternalRevset`. That type is what
extends `ToPredicateFn`, so the public type doesn't have to. The new
type will not need to implement the new functions I'm about to add to
the `Revset` trait.
We don't want the public `Revset` interface to know about
`ToPredicateFn`. In order to hide it, I'm wrapping the internal type
in another type, so only the internal type can keep implementing
`ToPredicateFn`.
I'd like to be able to change the return type of `evaluate_revset()`
to be an internal type. Since all external callers currently call the
function via `RevsetExpression::evaluate()`, it turns out it's easy to
make it private. To benefit from an internal type, we also need to
make the recursive calls be directly to the internal function.
@joyously found `o` confusing because it's a valid change id prefix. I
don't have much preference, but `●` seems fine. The "ascii",
"ascii-large", and "legacy" graph styles still use "o".
I didn't change `@` since it seems useful to have that match the
symbol used on the CLI. I don't think we want to have users do
something like `jj co ◎-`.
I'm about to make the default (non-working-copy) node symbol be a
unicode symbol, but we only want that when using a unicode graph, so
users with a terminal that doesn't support unicode can get plain ASCII
output by setting e.g. `ui.graph.style = "ascii"`.
The command grepped for 'o ' and picked the third line. That was meant
to match the graph nodes only, but it also matched the 'jj co master'
line. Let's match only 'o' at the beginning of the line, and throw in
another space for good measure (since that's what we get from the new
default graph style from Sapling).
The tests adding and removing heads to the repo mostly want to verify
that the set of heads is expected. Some of them also check that
commits are available in the index. But they shouldn't care about the
exact index stats.
I don't think there's much to gain from making the index match exactly
what's reachable from the view. FWIW, our cloud-based implementation
at Google will probably make everyone's commits visible in the index
regardless of which operation they're at.
We don't want custom index implementations to have to conform to the
same kind of stats as the default implementation. This commit also
makes the command error out on non-default index types.
I broke the commands in a27da7d8d5 and thought I just fixed it in
c7cf914694a8. However, as I added a test, I realized that I made it
only reindex the commits since the previous operation. I meant for the
command to do a full reindexing of th repo. This fixes that.
I'm thinking of rewriting the evaluation part as a simple interpreter. It
will increase the runtime cost (about a few microseconds per entry I suppose),
but will greatly reduce the complexity of generic property function chaining.
The extracted template_builder module is the part I'm going to reimplement.
I broke `jj debug reindex` in a27da7d8d5. From that commit, we no
longer delete the pointer to the old index, so nothing happens when we
reload the index. This commit fixes that, and also makes the command
error out if run on a repo with a non-default index type.
This is yet another step towards making the index pluggable. The
`IndexStore` trait seems reasonable after this commit. There's still a
lot of work to remove `IndexPosition` from the `Index` trait.
I didn't make `ReadonlyIndex` extend `Index` because it needed an
`as_index()` to convert to `&dyn Index` trait object
anyway. Separating the types also gives us flexibility to implement
the two traits on different types.
Not all index implementations may want to store the readonly index
implementation in an Arc. Exposing the Arc in the interface is also
problematic because `Arc<IndexImpl>` cannot be cast to `Arc<dyn
Index>`.
These two files are closely related, and `Index` and `IndexStore` are
expected to be customized together, so it seems better to keep them in
a single file.
This is another step towards allowing a custom `jj` binary to have its
own index type. We're going to have a server-backed index
implementation at Google, for example.
This is a step towards making the index storage pluggable. The
interface will probably change a bit soon, but let's start with
functions that match the current implementation.
I called the current implementation the `DefaultIndexStore`. Calling
it `SimpleIndexStore` (like `SimpleOpStore` and `SimpleOpHeadsStore`)
didn't seem accurate.
Unlike Mercurial, this isn't a template keyword/function, but a config knob.
Exposing graph_width to templater wouldn't be easy, and I don't think it's
better to handle terminal wrapping in template.
I'm not sure if patch content should be wrapped, so this option only applies
to the template output for now.
Closes#1043
I'm going to add $COLUMNS override, and it should work even if ioctl() on tty
failed. This means that the return type has to be (Option<u16>, Option<u16>).
Since we don't use the row count, I decided to drop it.
I've set up a jj-security@googlegroups.com list. The template comes
from Google's internal web.
I have no experience with GitHub's Security Advisory database, but it
seems like a good practice, so let's use it.
The parameter order follows indent()/label() functions, but this might be
a bad idea because fill() is more likely to have optional parameters. We can
instead add template.fill(width) method as well as .indent(prefix). If we take
this approach, we'll probably need to add string.fill()/indent() methods,
and/or implicit cast at method resolution. The good thing about the method
syntax is that we can add string.refill(), etc. for free, without inventing
generic labeled template functions.
For #1043, I think it's better to add a config like ui.log-word-wrap = true.
We could add term_width/graph_width keywords to the templater, but the
implementation would be more complicated, and is difficult to use for the
basic use case. Unlike Mercurial, our templater doesn't have a context map
to override the graph_width stub.
wrap_bytes() is similar to textwrap::wrap(), but can process arbitrary bytes.
More importantly, it guarantees that byte offsets can be reconstructed from
the split slices. This allows us to interleave push/pop_label()s with split
text fragments.
We could calculate byte offsets upfront, but using slice API is more
convenient. That's why I didn't add inner function returning Vec<Range>.
New word-wrap function will be implemented in two passes. The first pass
splits byte slice to lines, and the second pass inserts "\n" based on that
while interleaving push/pop_label() calls and text fragments. Since the second
pass combines multiple data sources, byte indices are more convenient than
slices there.
It's getting confusing since we now have a list property type.
expand/normalize_list() functions aren't renamed since they are also applied
to a list of function arguments.
