This is more consistent with the other method and it makes some extension operations easier, by giving access to the OpStore and other relevant context for custom index extensions.
`RewriteType::Rewritten` must have exactly one replacement. I think
it's better to encode that in the type by attaching the value to the
enum variant. I also renamed the type to just `Rewrite` since it now
has attached data and `Type` sounds like a traditional data-free enum
to me.
Looks like I forgot this in some recent refactoring.
I don't really see any harm in making the type public later. I might
want to make `rebase_descendants()` not clear `parent_mapping` and
instead provide a way of accessing it afterwards (removing the need
for the `_return_map()` flavors). We'll see if that ends up
happening. For now it can be private anyway.
For example,
```
<<<<<<< Conflict 1 of 3
+++++++ Contents of side #1
left 3.1
left 3.2
left 3.3
%%%%%%% Changes from base to side #2
-line 3
+right 3.1
>>>>>>>
```
or
```
<<<<<<< Conflict 1 of 1
%%%%%%% Changes from base to side #1
-line 3
+right 3.1
+++++++ Contents of side #2
left 3.1
left 3.2
left 3.3
>>>>>>>
```
Currently, there is no way to disable these, this is TODO for a future
PR. Other TODOs for future PRs: make these labels configurable. After
that, we could support a `diff3/git`-like conflict format as well, in
principle.
Counting conflicts helps with knowing whether you fixed all the
conflicts while you are in the editor.
While labeling "side #1", etc, does not tell you the commit id or
description as requested in #1176, I still think it's an improvement.
Most importantly, I hope this will make `jj`'s conflict format less
scary-looking for new users.
I've used this for a bit, and I like it. Without the labels, I would see
that the two conflicts have a different order of conflict markers, but I
wouldn't be able to remember what that means. For longer diffs, it can
be tricky for me to quickly tell that it's a diff as opposed to one of
the sides. This also creates some hope of being able to navigate a
conflict with more than 2 sides.
Another not-so-secret goal for this is explained in
https://github.com/martinvonz/jj/pull/3109#issuecomment-2014140627. The
idea is a little weird, but I *think* it could be helpful, and I'd like
to experiment with it.
The format is 7 characters of the separator followed by a space and arbitrary
text, followed by a newline. Separator followed by a newline is also allowed.
E.g.:
<<<<<<< Random text
%%%%%%% Random text
line 2
-line 3
+left
line 4
+++++++ Random text
right
%%%%%%% Random text
line 2
+forward
line 3
line 4
>>>>>>> Random text
This commit only allows reading such conflicts.
I considered allowing longer separators (`<<<<<<<<<<<<<< Random text`), but we
wouldn't currently write them, so let's be strict for now.
7 characters if they are followed by a space and arbitrary text
We already have two uses for this function and I think we're soon
going to have more.
The function record the old commit as abandoned with the new parents,
which is typically what you want. We could record it as abandoned with
the old parents instead but then we'd have to do an extra iteration to
find the parents when rebasing any children. It would also be
confusing if
`rewriter.set_parents(new_parents).record_abandoned_commit()` didn't
respect the new parents.
Since fileset/revset/template expressions are specified as command-line
arguments, it's sometimes convenient to use single quotes instead of double
quotes. Various scripting languages parse single-quoted strings in various ways,
but I choose the TOML rule because it's simple and practically useful. TOML is
our config language, so copying the TOML syntax would be less surprising than
borrowing it from another language.
https://github.com/toml-lang/toml/issues/188
While I like strict parsing, it's not uncommon that we have to deal with file
names containing spaces, and doubly-quoted strings such as '"Foo Bar"' look
ugly. So, this patch adds an exception that accepts top-level bare strings.
This parsing rule is specific to command arguments, and won't be enabled when
loading fileset aliases.
When you use e.g. `git switch` to check out a conflicted commit,
you're going to end up with the `.jjconflicts-*` directories in your
working copy. It's probably not obvious what those mean. This patch
adds a README file to the root tree to try to explain to users what's
going on and how to recover.
The authoritative information about conflicts is stored in the
`jj:trees` commit header. The contents of conflicted commits is only
used for preventing GC. We can therefore add contents to the tree
without much consequence.
This addresses the test instability. The underlying problem still exists, but
it's unlikely to trigger user-facing issues because of that. A repo instance
won't be reused after gc() call.
Fixes#3537
Apparently, these gc() invocations rely on that the previous "git gc" packed
all refs so there are no loose refs to compare mtimes. If there were new (or
remaining) loose refs, mtime comparison could fail. I also added +1sec to
effectively turn off the keep_newer option, which isn't important in these
tests.
CommitIds are often manipulated by reference, so this makes the API more
flexible for cases where the caller doesn't already have a Vec or array of
owned CommitIds.
In many cases `rewrite_parents()` does not even need to clone the input
CommitIds. This refactor allows the clone to be avoided if it's unnecessary.
There might be other APIs that would benefit from a similar change. In general,
it seems like there are a lot of places where we're writing
`&[commit_x.id().clone, commit_y.id().clone()]` and similiar.
- [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/flexibility.html#functions-minimize-assumptions-about-parameters-by-using-generics-c-generic)
## Feature Description
If enabled in the user or repository settings, the local branches pointing to the
parents of the revision targeted by `jj commit` will be advanced to the newly
created commit. Support for `jj new` will be added in a future change.
This behavior can be enabled by default for all branches by setting
the following in the config.toml:
```
[experimental-advance-branches]
enabled-branches = ["glob:*"]
```
Specific branches can also be disabled:
```
[experimental-advance-branches]
enabled-branches = ["glob:*"]
disabled-branches = ["main"]
```
Branches that match a disabled pattern will not be advanced, even if they also
match an enabled pattern.
This implements feature request #2338.
It's reasonable for a `WorkingCopy` implementation to want to return
an error. `LocalWorkingCopyFactory` doesn't because it loads all data
lazily. The VFS-based one at Google wants to be able to return an
error, however.
Previously, this command would work:
jj --config-toml='snapshot.max-new-file-size="1"' st
And is equivalent to this:
jj --config-toml='snapshot.max-new-file-size="1B"' st
But this would not work, despite looking like it should:
jj --config-toml='snapshot.max-new-file-size=1' st
This is extremely confusing for users.
This config value is deserialized via serde; and while the `HumanByteSize`
struct allegedly implemented Serde's `visit_u64` method, it was not called by
the deserialize visitor. Strangely, adding an `visit_i64` method *did* work, but
then requires handling of overflow, etc. This is likely because TOML integers
are naturally specified in `i64`.
Instead, just don't bother with any of that; implement a `TryFrom<String>`
instance for `HumanByteSize` that uses `u64::from_str` to try parsing the string
immediately; *then* fall back to `parse_human_byte_size` if that doesn't work.
This not only fixes the behavior but, IMO, is much simpler to reason about; we
get our `Deserialize` instance for free from the `TryFrom` instance.
Finally, this adjusts the test for `max-new-file-size` to now use a raw integer
literal, to ensure it doesn't regress. (There are already in-crate tests for
parsing the human readable strings.)
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: I8dafa2358d039ad1c07e9a512c1d10fed5845738
`jj parallelize` was a good example of a command that can be
simplified by the new API, so I decided to rewrite it as an example.
The rewritten version is more flexible and doesn't actually need the
restrictions from the old version (such as checking that the commits
are connected). I still left the check for now to keep this patch
somewhat small. A subsequent commit will remove the restrictions.
There are several existing commands that would benefit from an API
that makes it easier to rewrite a whole graph of commits while
transforming them in some way.
`jj squash` is one example. When squashing into an ancestor, that
command currently rewrites the ancestor, then rebases descendants, and
then rewrites the rewritten source commit. It would be better to
rewrite the source commit (and any descendants) only once.
Another example is the future `jj fix`. That command will want to
rewrite a graph while updating the trees. There's currently no good
API for that; you have to manually iterate over descendants and
rewrite them.
This patch adds a new `MutableRepo::transform_descendants()` method
that takes a callback which gets a `CommitRewriter` passed to it. The
callback can then decide to change the parents, the tree, etc. The
callback is also free to leave the commit in place or to abandon it.
I updated the regular `rebase_descendants()` to use the new function
in order to exercise it. I hope we can replace all of the
`rebase_descendant_*()` flavors later.
I added a `replace_parent()` method that was a bit useful for the test
case. It could easily be hard-coded in the test case instead, but I
think the method will be useful for `jj git sync` and similar in the
future.
`CommitRewriter` wraps 3 of the arguments, so I think it makes sense
to pass it instead. More importantly, I hope to continue refactoring
so many of the callers already have a `CommitRewriter`.
The new `rebase()` method is meant to be called after deciding on the
new parents (typically by leaving them unchanged). It returns a
`CommitBuilder` for setting any additional values.
There will probably be a `reparent()` method in the future.
This patch adds a struct that's meant to help when rewriting
commits. It contains the old commits and the new parents. I hope to
move most of the logic from `rebase_commit_with_options()` onto it in
coming patches. Then this type can be passed in a callback to make it
easier to do custom rewriting of commits that is currently hard to do
because `rebase_descendants()` does not give the caller any control
over the process.
The helper is similar to `CommmitBuilder`, but it is a bit different
by also embedding information about the source commit, so I don't
think the API would be as convenient if we just used `CommitBuilder`
directly.
Mercurial appears to resolve cwd-relative path first, so "glob:*.c" could be
parsed as "**/*.c" if cwd was literally "**". It wouldn't practically matter,
but isn't correct. Instead, jj's parser first splits glob into literal part
and pattern. That's mainly because we want to parse the user input texts into
type-safe objects, and (RepoPathBuf, glob::Pattern) pairs are the simplest
ones. The current parser can't handle patterns like "foo/*/.." (= "foo" ?),
and errors out. I believe this restriction is acceptable.
Unlike literal paths, the 'glob:' pattern anchors to the whole file path. I
don't think "prefix"-matching glob is useful, and making it the default would
be rather confusing.
Patterns are specified as (dir, pattern) pairs because we need to handle
parse errors prior to constructing a matcher, and it's convenient to split
literal directory paths there.
It's cheap to look up commits again from the cache in `Store` but it
can be expensive to look up commits we didn't end up needing. This
will make it easier to refactor further and be able to cheaply set
preliminary parents for a rewritten commits and then let the caller
update them.
I'm going to add a helper struct to help with rewriting commits. I
want to make that struct own the old commit and the new parents to
simplify lifetimes. This patch prepares for that by passing the
commits by value to `rebase_commit()`.
Running `cargo publish` from a non-colocated repo (such as my usual
repo) is currently quite scary because it uploads all non-hidden
files, even if they're ignored by `.gitignore`
(https://github.com/rust-lang/cargo/issues/2063). I noticed this a
while ago and have always run the command from a fresh clone since
then. To avoid the need for that, let's use the workaround mentioned
on the bug, which is to explicitly list patterns we want to publish.
This prepares for adding glob matcher, which will be backed by
RepoPathTree<Vec<glob::Pattern>>.
FilesNodeKind/PrefixNodeKind are basically boolean types, but implemented as
enums for better code readability.
The is_dir flag will be removed soon. Since FilesMatcher doesn't set is_dir
flag explicitly, is_dir is equivalent to !entries.is_empty(). OTOH,
PrefixMatcher always sets is_dir, so all tree nodes are directories.
Perhaps, I didn't do that because it's important to initialize is_dir/file to
false. Since I'm going to extract a generic map-like API, and is_dir/file will
be an enum, this won't be a problem.
I'm going to extract generic map from RepoPathTree, and .get_visit_sets()
will be inlined into FilesMatcher/PrefixMatcher. These removed tests should
be covered by the corresponding matcher tests.
The functions now depend only on `MutableRepo`, so I think they belong
on that type. This gets us closer to being able to make
`parent_mapping` private again.
I think the recent refactorings (especially 9c382fd8c6) make it
pretty clear that `DescendantRebaser` will not attempt to rebase the
same commit twice, so I think we can remove the assertions. This
removes some of the places where `DescendantRebaser` reaches into
`MutableRepo`'s internals.
The Pijul maintainer has opinions that I don't understand about how we
mention Pijul (they consider the current mentions offensive as
"bashing Pijul"). Let's just remove the references so we don't have to
deal with it. I think the references to Darcs we already had in most
of these places are sufficient.
This implements the other workaround described in 57167cefda "git: on
import_refs(), don't abandon ancestors of newly fetched refs":
> I think there are two ways to fix the problem:
> a. pin non-tracking remote branches just like local refs
> b. pin newly fetched refs in addition to local refs
> This patch implements (b) because it's simpler and more obvious that the
> fetched commits would never be abandoned immediately.
The idea of (a) is that untracked remote branches are independent read-only
refs, and read-only branches shouldn't be rewritten implicitly. Once the
branch gets rewritten or abandoned by user, these remote refs will be hidden,
and won't be pinned anymore.
Since (a) effectively supersedes (b), this patch also removes the original
workaround.
Fixes#3495
If we ever implement some sort of ABI for dynamic extension loading, we'll need these underlying APIs to support multiple extensions, so we might as well do that first.
If this doesn't work out, maybe we can try one of these:
a. fall back to bare file name if expression doesn't contain any operator-like
characters (e.g. "f(x" is an error, but "f x" can be parsed as bare string)
b. introduce command-line flag to opt in (e.g. -e FILESET)
c. introduce pattern prefix to opt in (e.g. set:FILESET)
Closes#3239, #2915, #2286
This command checks not only whether Watchman works, but also whether
it's enabled in the config. Also, the output is easier to understand
than that of the other `jj debug watchman` commands.
It would be nice if `jj debug watchman` called `jj debug watchman
status`, but it's not trivial in `clap` to have a default subcommand.
The primary use case is to warn unmatched paths. I originally thought paths in
negated expressions shouldn't be checked, but doing that seems rather
inconsistent than useful. For example, "~x" in "jj split '~x'" should match at
least one file to split to non-empty revisions.
Since fileset is primarily used in CLI, it's better to avoid inner quoting if
possible. For example, ".." would have to be quoted in the original grammar
derived from the revset.
This patch also adds a stricter version of an identifier rule. If we add a
symbol alias, it will follow the "strict_identifier" rule.
The fileset grammar is basically a stripped-down version of the revset grammar,
with a few adjustments:
* extract function call to "function" rule (like templater)
* inline "symbol" rule (because "identifier" and "string" should be treated
differently at the early parsing stage.)
The parser will have a separate name resolution stage. This will help to do
alias substitution properly. I'll probably rewrite the revset parser in the
same way. It will also help if we want to embed fileset expression in file()
revset.
There are no more callers of parse_function_argument_to_string(), so it's
removed. This function was a thin wrapper of literal parser, and can be
easily reintroduced if needed.
FilesetExpression is similar to RevsetExpression, but there are two major
differences:
- Union is represented as N-ary operator,
- Expression node isn't Rc-ed.
The former is because of the nature of the runtime Matcher objects. It's easier
to construct a Matcher from flattened union expressions than from a binary tree.
The latter choice comes from UnionAll(Vec<FilesetExpression>), which doesn't
have to be Vec<Rc<FilesetExpression>>, and Rc<[FilesetExpression]> can't be
constructed from [Rc<_>, ..]. Anyway, the internal representation may change as
needed.
Another design decision I made is Vec<Pattern(RepoPathBuf)> vs
Pattern(Vec<RepoPathBuf>). I chose the former because it will be more closer
to the parsed tree of the fileset language.
The nightly compiler has several clippy fix-its that, if applied, break the
build. There are various bugs about this, but there isn't enough space in the
margins to detail it all.
Just ignore these on a per-function basis; about 70% of them are just multiple
instances happening inside a single function.
This makes `cargo clippy --workspace --all-targets` run clean, even with the
nightly compiler.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: Ic26a025d3c62b12fbf096171308b56e38f7d1bb9
This will be needed to concatenate patterns of different types (such as
"prefix/dir" exact:"file/path".)
The implementation is basically a copy of IntersectionMatcher, with some
logical adjustments. In Mercurial, unionmatcher supports list of matchers
as input, but I think binary version is good enough.
In order to implement a fileset, we'll need owned variants of these matchers.
We can of course let callers move Box<dyn Matcher> into these adapters, but
we might need to somehow clone Box<dyn Matcher>. So, I simply made adapters
generic.
This function doesn't actually need commits, it only needs their IDs. In some
contexts we may only have commit IDs, so there's no need to require an iterator
of Commits.
This commit also adds a `CommitIteratorExt` that makes it easy to convert an
iterator of `&Commit` to an iterator of `&CommitId`.
Templater doesn't have the one yet, but I think it belongs to the same
category.
For clap::Error, we could use clap's own mechanism to render suggestions as
"tip: ...", but I feel "Hint: ..." looks better because our error/hint message
is capitalized.
I'm going to add RevsetParseError constructor for InvalidFunctionArguments,
with/without a source error, and I don't want to duplicate code for all
combinations. The templater change is just for consistency.
I couldn't find a good naming convention for the builder-like API, so it's
called .with_source(mut self, _). Another option was .source_set(source).
Apparently, it's not uncommon to name consuming constructor as
with_<something>().
Now that we no longer bother to keep the set of heads to add and
remove updated while we rewrite descendants, we can simplify how we
find the set of heads to remove - it's simply all commits that have
been marked rewritten, divergent, or abandoned, i.e. the keys in
`parent_mapping`.
I don't think we have any transactions that mark commit as abandoned
and then later mark it as rewritten or divergent. But if we ever do, I
think it should be considered just rewritten/divergent. So let's
enforce that invariant by removing the old value from the set of
abandoned commits.
This commit moves the parse_string_pattern helper function into the
str_util module in jj lib and adds tests for it.
I'd like to reuse this code in a function defined by `UserSettings`, which is
part of the jj lib crate and cannot use functions from the cli crate.
This makes the summary line more informative. Even though it just duplicates
the message printed later, I think it's easier to follow.
This patch also adjusts some RevsetParseError messages because it seemed
redundant to repeat "revset function", "argument", etc.
Because the CLI error handler now prints error sources in multi-line format,
it doesn't make much sense to render Revset/TemplateParseError differently.
This patch also fixes the source() of the SyntaxError kind. It should be
self.pest_error.source() (= None), not self.pest_error.
I'm going to make TemplateParseError hold RevsetParseError as Box<dyn _>, but
Box<dyn std::error::Error ..> doesn't implement Eq. I could remove Eq from
ErrorKind enums, but it's handly if these enums remain as value types.
This change will also simplify fmt::Display and error::Error impls.
It's common to normalize an empty directory path as ".". This change unblocks
the use of from_relative_path() in edit_sparse().
There are a couple of callers who do to_fs_path(Path::new("")), but they all
translate non-directory paths, which should never be empty.
We currently include the commits in `parent_mapping` and `abandoned`
in the set of commits to visit when rebasing descendants. The reason
was that we used to update branches and working copies when we visited
these commits. Since we started updating refs after rebasing all
commits, there's no need to even visit these commits.
We only use `new_commits` in `update_heads()`, so let's calculate it
there. It should also be more correct in case other commits were
created after we initialized `DescendantRebaser`.
Now that we only call `update_references()` in one place, there's no
reason to have it also update `heads_to_add` and `heads_to_remove`. By
moving it out of the function, we can consolidate the logic in one
place.
When `rebase_commit_with_options()` decides to abandons a commit, it
records the new parents in the `MutableRepo`, but it's currently the
caller's responsibility to remember to mark it as abandoned. Let's
move that logic into the function to reduce the risk of future bugs.
By adding the abandoned commit's parents to `parent_mapping`, we can
remove a bit more of the special handling of abandoned commitsin
`DescendantRebaser`.
In the normal case when we don't abandon a commit because it became
empty, then `CommitBuilder::write()` will have recorded the new commit
as a rewrite of the old commit. We don't need to do that again in
`rebase_one()`.
A subset of the state in `DescendantRebaser` now matches exactly what
`MutableRepo` already stores, so we can avoid copying that state and
have `DescendantRebaser` use it directly instead. Having a single
source of truth for the state will enable further simplifications and
improvements.
I'm going to make `DescendantRebaser` share the state about rewritten
commits with `MutableRepo` next. That means that the call to
`rebase_commit_with_options()` will update that state, which would
make this assertion fail. So let's move it a little earlier to avoid
that.
This is just to match `DescendantRebaser`, to make the next commit a
bit simpler. I think `MutableRepo` still has few enough fields that
just `abandoned` is clear enough. Maybe we'll move the three
rewrite-related fields into a new struct at some point.
With this patch, `MutableRepo` has the same tracking of rewritten
commits as `DescendantRebaser`, so we can simply pass that state into
`DescendantRebaser` when we create it. The next step is to remove the
state from `DescendantRebaser`.
I don't think we have any callers left that call
`record_rewritten_commit()` multiple times within a transaction and
expect it to result in divergence. I think we should consider it a bug
to do that.
When rebasing descendants, we generally move branches, child commits,
the working copy to the rewritten commit(s). However, we don't move
the working copy to the new rewritten commit (s) if the old commit had
been abandoned, and we don't move child commits if the rewriten was
divergent.
This patch aims to make it clearer that there's only one mapping from
old to new parents, and that is in `parent_mapping`. It does so by
merging the current `divergent` map into it, and makes the `divergent`
just a set instead. When finding the new parents for a child, we leave
the existing parent if it's in the set.
My longer-term goal is to move `parent_mapping`, `abandoned`, and
`divergent` into `MutableRepo` (maybe in a nested struct), so we can
do some transformations on descendants as we rebase them. By having
the state in a single place (not moving it from `MutableRepo` to
`DescendantRebaser` as we currently do), I hope it will be easier to
write a `MutableRepo::transform_descendants(callback)`, where the
callback gets a `CommitBuilder` and can change parents of the commit,
for example.
Apart from (IMO) looking nicer, this will also sidestep the potential problem
that if the file contains actual jj conflict markers (`>>>>>>>` in the beginning
of a line, for example), jj would currently have trouble materializing and
subsequently parsing conflicts in the file if it actually became conflicted.
I'll demo this bug in either this or a subsequent PR. It's the kind of bug that
sounds serious in theory but might never cause a problem in practice.
After this PR, only `docs/tutorial.md` has a conflict marker that's not indented.
There's only one there, so hopefully it won't be too much of a pain to deal with.
I also indented other strings in `test_conflicts.rs`. IMO, this looks nice and
more consistent with the `insta::assert_snapshot` output. I didn't spend the
time to do the same for `test_resolve_command`.
Suppose we have an alias 'immutable()' = '::immutable_heads()', user can
express (visible) mutable set as '~immutable()'. 'immutable_heads()..' can
terminate early, but a generic difference 'all() & ~immutable()' can't.
Suppose the generation value is usually small, it should be faster to do
bounded range look up first 'y-', then walk ancestors with the unwanted set
'y-..x'.
When an operation is missing and we recover the workspace, we create a
new working-copy commit on top of the desired working-copy commit (per
the available head operation). We then reset the working copy to an
empty tree because it shouldn't really matter much which commit we
reset to. However, when the workspace is sparse, it does matter, as
the test case from the previous patch shows. This patch fixes it by
replacing the `reset_to_empty()` method by a new `recover(&Commit)`,
which effectively resets to the empty tree and then resets to the
commit. That way, any subsequent snapshotting will result keep the
paths from that tree for paths outside the sparse patterns.
Prepares for removing &CompositeIndex from the RevsetGraphIterator struct.
The input iterator will also be changed to position-based.
I've turned self.look_ahead.get().unwrap() into assertion, but it's not super
important here. It's just for sanity that we've mapped missing edges properly.
FWIW, we could say RevsetGraphIterator is an example of iterating *and* testing
membership of the input revset (though the yielded entries are discarded.)
For the same reason as the previous commit. Since self.inner.positions()
basically clones the underlying evaluation tree, there is no reason to stick
to &self lifetime. Perhaps, some of the CLI utility can be changed to not
collect() the iterator.
Migrating iter_graph() requires non-trivial changes, so it will be done
separately.
This allows callers to cache the returned function at 'index lifetime. It's
important in templater. It also means the returned function could be 'static
if the index were Arc<_> and we had a trait interface to achieve that.
Option<Box<dyn ..>> is removed since RevWalk is fused.
This makes the whole evaluation tree 'static, and we can freely move it without
keeping the root RevsetImpl object alive.
Perhaps, "Self: 'a" can be replaced with 'static, but let's leave it for now.
It's not technically wrong to store lifetimed object in InternalRevset.
Prepares for dropping &self lifetime from to_predicate_fn(). All predicate
functions could be wrapped as Box::new(PurePredicateFn(Rc::new(f))) instead, but
I don't think the .clone() cost matters.
This is the step towards removing &CompositeIndex references from the revset
evaluation tree. The filter input is changed from &IndexEntry to IndexPosition
to simplify the lifetime thingy. We might want to pass around CommitId or
Commit object once it's loaded, but that can be implemented later. I don't
see significant performance difference in revset benches.
This serves the same role as templater::Literal. I'm going to add basic
RevWalk adapters so that the revset evaluation tree can be constructed without
capturing the index. EagerRevWalk will help to write tests for these adapters.
Now as default and elided node symbols come from the config, the next logical
step is to use them directly bypassing GraphLog. Note that commands like `jj op
log` and `jj obslog` do not use the elided node symbol at all.
Just a minor code cleanup. We still need Index for &CompositeIndex because the
type is unsized, and unsized type cannot be converted to another dyn reference.
This helps to eliminate higher-ranked trait bounds from RevWalkRevset and
RevWalk combinators to be added. Since &CompositeIndex is now a real reference,
it can be passed to functions as index: &T.
This helps to migrate CompositeIndex<'_> wrapper to &CompositeIndex. If
the wrapped reference had a lifetimed field, it couldn't be represented as
a trivial reference type.
We haven't used custom Git commit headers for two main reasons:
1. I don't want commits created by jj to be different from any other
commits. I don't want Git projects to get annoyed by such commit
and reject them.
2. I've been concerned that tools don't know how to handle such
headers, perhaps even resulting in crashes.
The first argument doesn't apply to commits with conflicts because
such commits would never be accepted by a project whether or not they
use custom commit headers. The second argument is less relevant for
conflicted commits because most tools will be confused by such commits
anyway.
Storing conflict information in commit headers means that we can
transfer them via the regular Git wire protocol. We already include
the tree objects nested inside the root-level tree, so they will also
be transferred.
So, let's start by writing the information redundantly to the commit
header and to the existing storage. That way we can roll it back if we
realize there's a problem with using commit headers.
Initially we were thinking to have `Revset` return something like
`CachedRevset`:
```
pub trait CachedRevset {
fn iter(&self) -> Box<dyn Iterator<Item = Commit>>;
fn contains(&self, &CommitId) -> bool;
}
```
But we weren't sure what use case for `iter` would be, so we dropped the `iter`
method. `CachedRevset` with single `contains` method needed a better name. We
weren't able to come up with one, so we decided instead to have a method on
`Revset` that returns a closure to check if a commit is in a revset.
"for<'index> RevWalk<CompositeIndex<'index>, .." works as of now, but it won't
be composed well. So I'll turn CompositeIndex<'_> into &CompositeIndex in the
next batch, and remove "for<'index>".
This eliminates lifetimed fields from RevWalk objects, and the RevWalk object
will be embedded directly in RevWalkRevset.
This patch adds two separate iterator adapters. They are identical at this
point, but I'm going to add detach/reattach methods only to the borrowed
version. I'm also planning to change CompositeIndex<'_> to &CompositeIndex
to get around higher-ranked trait bound restrictions.
This simplifies the RevWalkIndex API. It would probably add fractional msecs of
overhead per next() call, but I don't see significant difference in revset
benches.
I'm going to make CompositeIndex<'_> detachable from the RevWalk, and
"F: Fn(CompositeIndex) -> Box<dyn Iterator<..>>" of RevWalkRevset<F> will
be replaced with "W: RevWalk<CompositeIndex>". This will simplify the code
structure, but also means that we can no longer apply .take_while() here and
convert it back to RevWalk. Fortunately, ancestors_until_roots() is the only
function I need to reimplement.
It doesn't make sense to build BinaryHeap with intermediate type, and I'm
going to reimplement take_until_roots() in a way that the queue drops
uninteresting items.
The current RevWalk constructors insert intermediate items to BinaryHeap
and convert them as needed. This is redundant, and I'm going to add another
parameter that should be applied to the queue first. That's why I decided
to factor out a builder type. I considered adding a few set of factory
functions that receive all parameters, but they looked messy because most of
the parameters are of [IndexPosition] type.
This patch also adds must_use to the builder and its return types, which are
all iterator-like.
Although watchman client appears to fail at decoding non-UTF-8 path (somewhere
in serde), jj shouldn't panic if watchman could deal with that.
The outer error message "path not in the repo" would sounds odd, but I think
that's okay because 1. it's unlikely that a user input is not UTF-8, and 2.
it's technically correct that a non-UTF-8 path is not contained in the repo.
This should address both use cases:
1. If from_relative_path() is directly called, the error says ".." shouldn't
be included in the (normalized) relative path.
2. If parse_fs_path() is used, the error message contains paths relative to
cwd. #3216
Some of the RevWalk methods could be generalized, but I decided to not try that
for now. I'll probably need to do more cleanup to (hopefully) remove 'index
lifetime from these types.
