We have a few places where we have a `MergedTreeValue` and need to
read the data associated with it so we can write to the working copy
or include it in a diff. Let's extract some of that shared logic to a
function so we can reuse it. I plan to use it for reading file
contents in advance while streaming a diff in `local_working_copy`
soon (and probably in `jj diff` thereafter), but I think it seems like
an improvement on its own.
Remove a couple of unnecessary unsafes:
- The NonZeroUsize is a constant where the unwrap will optimize away
anyway and we don't have an unsafe without any good reason there :)
- The other two were simply not needed, lifetimes worked fine, maybe
Rust became better since that code was written? NLL? Anyway, they're
gone now
As discussed in Discord, it's less useful if remote_branches() included
Git-tracking branches. Users wouldn't consider the backing Git repo as
a remote.
We could allow explicit 'remote_branches(remote=exact:"git")' query by changing
the default remote pattern to something like 'remote=~exact:"git"'. I don't
know which will be better overall, but we don't have support for negative
patterns anyway.
Since the concurrent diff algorithm is significantly slower when using
the Git backend, I think we'll have to use switch between the two
algorithms depending on backend. Even if the concurrent version always
performed as well as the sequential version, exactly how concurrent it
should be probably still depends on the backend. This commit therefore
adds a function to the `Backend` trait, so each backend can say how
much concurrency they deal well with. I then use that number for
choosing between the sequential and concurrent versions in
`MergedTree::diff_stream()`, and also to decide the number of
concurrent reads to do in the concurrent version.
One less git2 API use in CLI.
The function name GitBackend::init_colocated() is a bit odd, but we need to
specify the work-tree path, not the ".git" repo path. So we can't eliminate
the notion of the working copy path anyway.
This also adds `jobs`, the argument reading the thread count to use and `shell_command`.
While we're at it, make `execute` a no-op and teach `run` to resolve the passed revsets.
I also fixed my misunderstanding of `Clap` which makes
`jj run 'echo hello world' -r 'mine() & ~origin@remote' --jobs 4` parse correctly.
Also contains a small fix in the `pre-commit` example for it.
Summary: This is currently used by `new.rs`, `workspace.rs`, and `rebase.rs`,
and may be useful for other commands and custom CLIs. So just go ahead and move
it into the parent module hierarchy.
Also rename the function to `resolve_all_revs`, as it isn't actually specific to
rebase at all.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: I0ea12afd8107f95a37a91340820221a0
Summary: A natural extension of the existing support, as suggested by Scott
Olson. Closes#2496.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Change-Id: I91c9c8c377ad67ccde7945ed41af6c79
What make rebase_to_dest_parent a good candidate for jj_lib::rewrite module:
- It is used both in obslog and interdiff. It's a sign that it may be moved to a lower layer
- CommandError is returned by converting from TreeMergeError. Not explicitly.
- It only use jj_lib::rewrite fonctions.
I'm going to implement a `Stream`-based version optimized for
high-latency (RPC-based) commit backends. So far, that implementation
is about 20% slower in the Linux repo when running `jj diff
--ignore-working-copy -s --from v5.0 --to v6.0`. I think that's almost
only because the algorithm is different, not because it's async per
se.
This commit adds a `Stream`-based version of `MergedTree::diff()` that
just wraps the regular iterator in stream. I updated `jj diff` to use
it. I couldn't measure any difference on the command above in the
Linux repo. I think that means we can safely use the same
`Stream`-based interface regardless of backend, even if we end up
needing two different implementations of the `Stream`. We would then
be using the wrapped iterator from this commit for local backends, and
the new implementation for remote backends. But ideally we can make
the remote-friendly implementation fast enough that we don't need two
implementations.
During the transition to using more async code, I keep running into
https://github.com/rust-lang/futures-rs/issues/2090. Right now, I want
to convert `MergedTree::diff()` into a `Stream`. I don't want to
update all call sites at once, so instead I'm adding a
`MergedTree::diff_stream()` method, which just wraps
`MergedTree::diff()` in a `Stream. However, since the iterator is
synchronous, it needs to block on the async `Backend::read_tree()`
calls. If we then also block on the `Stream` in the CLI, we run into
the panic.
We had similar code in two places for restoring paths from one tree to
another. Let's reuse it instead.
I put the new function in the `rewrite` module. I'm not sure if that's
right place. Maybe it belongs in `tree`?
Since gix::Repository::config_snapshot() borrows the repo instance, it has to
be allocated in caller's stack. That's why GitBackend::git_config() is removed.
My gut feeling is that gitoxide aims to be more transparent than libgit2. We'll
need to know more about the underlying Git data model.
Random comments on gix API:
* gix::Repository provides API similar to git2::Repository, but has less
"convenient" functions. For example, we need to use .find_object() +
.try_to/into_<kind>() instead of .find_<kind>().
* gix::Object, Blob, etc. own raw data as bytes. gix::object and gix::objs
types provide high-level views on such data.
* Tree building is pretty low-level compared to git2.
* gix leverages bstr (i.e. bytes) extensively.
It's probably not difficult to migrate git::import/export_refs(). It might
help eliminate the startup overhead of libssl initialization. The gix-based
GitBackend appears to be a bit faster, but that wouldn't practically matter.
#2316
If we create a `ColorFormatter`, add some labels to it, print
something using the configured style, and then return early because of
an error, we would leave the terminal in a bad state. I think many
shells reset color codes after a command returns, but let's still do
our best.
Like "jj log PATHS...", unmatched name isn't an error. I don't think
"jj branch list glob:'push-*'" should fail just because there are no in-flight
PR branches.
This avoids https://github.com/rust-lang/futures-rs/issues/2090. I
don't think we need to worry about reading legacy conflicts
asynchronously - async is really only useful for Google's backend
right now, and we don't use the legacy format at Google. In
particular, I don't want `MergedTree::value()` to have to be async.
Both local and remote refs are backed by the same value type since we'll need
some kind of runtime abstraction to represent "branches" keyword (which is a
list of local + remote branches.) It's tedious to implement separate
local/remote/both ref types.
The "unsynced" flag is inverted just because the positive term is slightly
easier to document.
I'm going to change "branches" to return a list instead of formatted string,
and I don't think "if(branches, ..)" should be invalidated by that. Perhaps,
a container type like String, Vec<T>, Option<T> can implement the cast.
Makes sure that, at the point the commit summary for the new commit is written,
the original commit that is being rewritten is already abandoned. Otherwise,
once we show divergent change ids (in a subsequent commit) in the short commit
template, the commits would be shown as divergent.
This also has an effect on whether branches are displayed next to the commit;
the changes in test_resotre_command happen because, now, the branch is properly
propagated to the restored commit before its summary is displayed.
test_templater_alias(), test_templater_alias_override(), and
test_templater_bad_alias_decl() aren't moved since they also test config loading
and error formatting. The first test in test_templater_parse_error() is left for
the same reason. test_templater_upper_lower() depends on the commit templater.
I don't think many of the tests in test_templater.rs should use "jj log" command
as they check very specific template syntax and function behaviors. Let's move
them to in-module tests. We could add a separate test file, but we would have
to export a couple of templater macros.
test_templater_timestamp_method() is migrated as example.
I want to fix error propagation before I start using async in this
code. This makes the diff iterator propagate errors from reading tree
objects.
Errors include the path and don't stop the iteration. The idea is that
we should be able to show the user an error inline in diff output if
we failed to read a tree. That's going to be especially useful for
backends that can return `BackendError::AccessDenied`. That error
variant doesn't yet exist, but I plan to add it, and use it in
Google's internal backend.
Reasons to introduce this alias:
* Reduces complexity of a type, to silence Clippy warnings in the
future if we use this type as a type parameter
* The type is used quite frequently, so it makes sense to have a name
for it
* It's easier to visually scan for the end of the type when you don't
have to match opening and closing angle brackets
Since "jj git fetch --branch" supports glob patterns, users would expect that
"jj git push --branch glob:.." also works.
The error handling bits are copied from "branch" sub commands. We might want to
extract it to a common helper function, but I haven't figured out a reasonable
boundary point yet.
Thanks to @glencbz for noticing that VS Code works fine now as a
merge tool, and thanks to @solson for suggesting
`merge-tool-edits-conflict-markers = true`.
AFAICT, all callers of `Merge::to_file_merge()` are already well
prepared for working with executable files. It's called from these
places:
* `local_working_copy.rs`: Materialized conflicts are correctly
updated using `Merge::with_new_file_ids()`.
* `merge_tools/`: Same as above.
* `cmd_cat()`: We already ignore the executable bit when we print
non-conflicted files, so it makes sense to also ignore it for
conflicted files.
* `git_diff_part()`: We print all conflicts with mode "100644" (the
mode for regular files). Maybe it's best to use "100755" for
conflicts that are unambiguously executable, or maybe it's better to
use a fake mode like "000000" for all conflicts. Either way, the
current behavior seems fine.
* `diff_content()`: We use the diff content in various diff
formats. We could add more detail about the executable bits in some
of them, but I think the current output is fine. For example,
instead of our current "Created conflict in my-file", we could say
"Created conflict in executable file my-file" or "Created conflict
in ambiguously executable file my-file". That's getting verbose,
though.
So, I think all we need to do is to make `Merge::to_file_merge()` not
require its inputs to be non-executable.
Closes#1279.
