We could fix do_git_clone() instead, but it seemed a bit weird that the
git_repo_path is relative to the store path which is unknown to callers.
Fixes#2374
There's a subtle behavior change. Unlike the original remove_remote_branch(),
remote_views entry is not discarded when the branches map becomes empty. The
reasoning here is that the remote view can be added/removed when the remote
is added/removed respectively, though that's not implemented yet. Since the
serialized data cannot represent an empty remote, such view may generate
non-unique content hash.
These functions depend heavily on the underlying data structure, and I haven't
decided abstract View API to access to per-remote data types. Let's use the
underlying data type for now.
I'm planning to add support for untracked remote branches, and under that
model, there will be many remote branches without local counterparts. That's
the main reason why remote branches are grouped by remote, not by branch name.
The added helper functions will be used by simple_op_store and view.
get_branch() would need to reconstruct the remote_targets map if we migrate
the underlying data structure to per-remote views. Let's remove the method as
it is only used in tests.
It seems pretty clear from the context. Turns out we only use the
function in a test case. Maybe we don't even need it. It's easy to
provide it, though.
The `TreeStateError` type is specific to the current local-disk
working-copy backend, so it should not be part of the generic
working-copy interface I'm trying to create.
I think some of the errors variants in `CheckoutError` are too
specific to the local-disk implementation. Let's merge them and make
them less specific, so it's easier to define a reasonable trait for
the working copy.
As I'm going to split branches into per-remote map, .get_branch(name) will need
to gather remote branches by name to construct remote_targets map. Let's instead
iterate local/remote branches separately. I also migrated diffing of the other
kinds of refs to filter out unchanged entries upfront.
As we now diff incoming git refs against our known remote branches, the problem
described in the comment no longer occurs. test_branch_forget_fetched_branch()
passes, and the inline comments in the test are still valid.
As we need to build a set of all branch names anyway, we can also put old/new
targets there. InvalidGitName is moved to caller since the diff function no
longer converts RefName to "refs/" string.
The idea is that the "remote" refs could have been "op restore"-d whereas
view.git_refs() will never be. The next commit will update known_remote_refs
to be constructed from the current remote branches.
Instead of building these lists in a single loop, we could load new git_refs
to the view first, and then build diffs of the remote refs. I considered that,
but I feel it would be a bit awkward to update refs before importing commits
to the view.
The "remote" refs are stored in BTreeMap since merging order should be stable.
As I'm going to add separate lists of changed git_refs/remote_refs, it'll
become a bit unclear which one we should check for reserved remotes. The
diff might also be reorganized as a list of (remote, name, kind, old_target,
new_target) where remote == "git" means the git-tracking branch. In this
data structure, the notion of reserved remote name would be lost.
I'm going to rewrite `TreeDiffIterator` to fetch one level (depth) of
the tree at a time and concurrently. One step towards that is to
convert the iterator to a `Stream`. I'd like to do that by making the
current `Iterator` implementation call the new `Stream`
implementation. However, we can't call `futures::executor::block_on()`
on a future that itself calls `futures::executor::block_on()` (as
`Store::read_tree()` does), so the first step is to bubble up the
async-ness a bit. This patch does that by fetching both sides of the
diff concurrently. That should give close to a 2x speedup on
high-latency backends. (It doesn't help with our backend at Google,
however, because we have a daemon process that does some speculative
prefetching that usually downloads the child trees anyway.)
`futures::stream::Stream::collect()` requires a collection that
implements `Default` and `Extend`, and I would like to to be able to
collect a stream of trees.
The commit backend at Google is cloud-based (and so are the other
backends); it reads and writes commits from/to a server, which stores
them in a database. That makes latency much higher than for disk-based
backends. To reduce the latency, we have a local daemon process that
caches and prefetches objects. There are still many cases where
latency is high, such as when diffing two uncached commits. We can
improve that by changing some of our (jj's) algorithms to read many
objects concurrently from the backend. In the case of tree-diffing, we
can fetch one level (depth) of the tree at a time. There are several
ways of doing that:
* Make the backend methods `async`
* Use many threads for reading from the backend
* Add backend methods for batch reading
I don't think we typically need CPU parallelism, so it's wasteful to
have hundreds of threads running in order to fetch hundreds of objects
in parallel (especially when using a synchronous backend like the Git
backend). Batching would work well for the tree-diffing case, but it's
not as composable as `async`. For example, if we wanted to fetch some
commits at the same time as we were doing a diff, it's hard to see how
to do that with batching. Using async seems like our best bet.
I didn't make the backend interface's write functions async because
writes are already async with the daemon we have at Google. That
daemon will hash the object and immediately return, and then send the
object to the server in the background. I think any cloud-based
solution will need a similar daemon process. However, we may need to
reconsider this if/when jj gets used on a server with a custom backend
that writes directly to a database (i.e. no async daemon in between).
I've tried to measure the performance impact. That's the largest
difference I've been able to measure was on `jj diff
--ignore-working-copy -s --from v5.0 --to v6.0` in the Linux repo,
which increases from 749 ms to 773 ms (3.3%). In most cases I've
tested, there's no measurable difference. I've tried diffing from the
root commit, as well as `jj --ignore-working-copy log --no-graph -r
'::v3.0 & author(torvalds)' -T 'commit_id ++ "\n"'` (to test a
commit-heavy load).
This effectively undoes d8a313cdd4, which is no longer needed since
we just changed that error handling. It should make it easier to share
some of the current if/else blocks.
Before this patch, when updating to a commit that has a file that's
currently an ignored file on disk, jj would crash. After this patch,
we instead leave the conflicting files or directories on disk. We
print a helpful message about how to inspect the differences between
the intended working copy and the actual working copy, and how to
discard the unintended changes.
Closes#976.
I'm about to add handling of parent dirs that are existing ignored
files, so it's better to have it in one place. The only functional
difference should be that we now create parent directories for git
submodules. I don't think that matters.
It's about time we make the working copy a pluggable backend like we
have for the other storage. We will use it at Google for at least two
reasons:
* To support our virtual file system. That will be a completely
separate working copy backend, which will interact with the virtual
file system to update and snapshot the working copy.
* On local disk, we need to tell our build system where to find the
paths that are not in the sparse patterns. We plan to do that by
wrapping the standard local working copy backend (the one moved in
this commit), writing a symlink that points to the mainline commit
where the "background" files can be read from.
Let's start by renaming the exising implementation to
`local_working_copy`.
I've added a boolean flag to the store to ensure that the migration never runs
more than once after the view gets "op restore"-d. I'll probably reorganize the
branches structure to support non-tracking branches later, but updating the
storage format in a single commit would be too involved.
If jj is downgraded, these "git" remote refs would be exported to the Git repo.
Users might have to remove them manually.
I'm going to migrate "refs/heads/" branches to .remote_targets["git"]. This
commit will simplify the story as we won't have to exclude "refs/remotes/git/"
refs when diffing or renaming/removing remote.
Since both has_id() and resolve_prefix() do binary search, their costs are
practically the same. I think has_id() would complete with fewer ops, but such
level of optimization wouldn't be needed here. More importantly, this ensures
that unreachable commits aren't imported by GitBackend::read_commit().
One problematic scenario is that we have commits imported by old jj, and all
of their descendant commits are created by jj. Therefore import_head_commits()
wouldn't reach the old ancestor commits.
This change might bury a real bug, but I don't have a better alternative. Maybe
we can remove this hack after a couple of jj releases, and add a debug command
that imports all reachable Git commits from all historical heads.
Closes#2343
As we can set HEAD to an arbitrary ref by using .reference_symbolic(), we don't
have to manage a ref that can also be valid as a branch name.
Fixes#1495
I'll add a workaround for the root parent issue #1495 there. We can pass in
the wc parent id instead of the wc_commit object, but we might want to use
wc_commit.id() to generate a unique placeholder ref name.
While debugging git issues, I often ended up creating a deadlock by adding
debug prints. It's also not obvious that git::export_refs() works even if the
git_repo() has already been locked, whereas git::import_refs() wouldn't. Let's
consolidate lock handling to the backend implementation.
I think most users who change the set of immutable heads away from
`trunk() | tags()` are going to also want to change the default log
revset to include the newly mutable commit and to exclude the newly
immutable commits. So let's update the default log revset to use
`immutable_heads()` instead.
`test_templater` changed because we have overridden the set of
immutable commits there so `jj log` now includes the remote branch.
If we made @git branches real .remote_targets["git"], remotely-rewritten
commits could also be pinned by the @git branches, and therefore wouldn't be
abandoned. We could exclude the "git" remote, but I don't think local commits
should be pinned by remote refs in general. If we squashed a fetched commit,
remote ref would point to a hidden commit anyway.
I ran into a bug the other day where `jj status` said there was a
conflict in a file but there were no conflict markers in the working
copy. The commit was created when I squashed a conflict resolution
into the commit's parent. The rebased child commit then ended up in
this state. I.e., it looked something like this before squashing:
```
C (no conflict)
|
| B conflict
|/
A conflict
```
The conflict in B was different from the conflict in A. When I
squashed in C, jj would try to resolve the conflicts by first creating
a 7-way conflict (3 from A, 3 from B, 1 from C). Because of the exact
content-level changes, the 7-way conflict couldn't be automatically
resolved by `files::merge()` (the way it currently works
anyway). However, after simplifying the conflict, it could be
resolved. Because `MergedTree::merge()` does another round of conflict
simplification of the result at the end of the function, it was the
simplifed version that actually got stored in the commit. So when
inspecting the conflict later (e.g. in the working copy, as I did), it
could be automatically resolved.
I think there are at least two ways to solve this. One is to call
`merge_trees()` again after calling `tree.simplify()` in
`MergedTree::merge()`. However, I think it would only matter in the
case of content-level conflicts. Therefore, it seems better to make
the content-level resolution solve this case to start with. I've done
that by simplifying the conflict before passing it into
`files::merge()`. We could even do the fix in `files::merge()`, but
doing it before calling it has the advantage that we can avoid reading
some unchanged content from the backend.
All non-test callers already have a `Merge` object, so let's pass that
instead. We thereby simplify the callers a little, and we enforce the
"adds.len() == removes.len() + 1" constraint in the type.
When there's a single parent, we can determine if a commit is empty by
just comparing the tree ids. Also, when using tree-level conflicts, we
don't need to read the trees to determine if there's a conflict. This
patch adds both of those fast paths, speeding up `jj log -r ::main`
from 317 ms to 227 ms (-28.4%). It has much larger impact with our
cloud-based backend at Google (~5x faster).
I made the same fix in the revset engine and the Git push code (thanks
to Yuya for the suggestion).
This adds a new `revset-aliases.immutable_heads()s` config for
defining the set of immutable commits. The set is defined as the
configured revset, as well as its ancestors, and the root commit
commit (even if the configured set is empty).
This patch also adds enforcement of the config where we already had
checks preventing rewrite of the root commit. The working-copy commit
is implicitly assumed to be writable in most cases. Specifically, we
won't prevent amending the working copy even if the user includes it
in the config but we do prevent `jj edit @` in that case. That seems
good enough to me. Maybe we should emit a warning when the working
copy is in the set of immutable commits.
Maybe we should add support for something more like [Mercurial's
phases](https://wiki.mercurial-scm.org/Phases), which is propagated on
push and pull. There's already some affordance for that in the view
object's `public_heads` field. However, this is simpler, especially
since we can't propagate the phase to Git remotes, and seems like a
good start. Also, it lets you say that commits authored by other users
are immutable, for example.
For now, the functionality is in the CLI library. I'm not sure if we
want to move it into the library crate. I'm leaning towards letting
library users do whatever they want without being restricted by
immutable commits. I do think we should move the functionality into a
future `ui-lib` or `ui-util` crate. That crate would have most of the
functionality in the current `cli_util` module (but in a
non-CLI-specific form).
I don't think the backend should matter for any of these tests, so
let's test with only one, and let's make that the strictest one - the
new test backend.
This reduces the number of tests by 74 (from 974 to 900), but saves no
measurable run time.
The `#[tokio::main]` annotation uses a multi-threaded runtime by
default. We don't need that for querying watchman. Switching to the
single-threaded runtime saves about 20 ms.
I originally attempted to embed function parameters in RevsetAliasId. That's
probably why these getters return id. Let's move id construction to callers
since the id only serves as a recursion blocker.
Spotted while porting it to per-remote views. Undone fetch/push is tricky. If
we want to detect git/jj conflicts in that scenario, we would need to track
both "known" and "current" remote targets. It's probably okay to export jj's
remote targets as we do the reverse for import_refs(), but I need to think
that a bit more.
Only tests dealing with Git submodules care about the backend type.
Switching the tests to use the test backend also uncovered another bug
in `MergedTree`, so I fixed that too. The bug only happens with legacy
trees (path-level conflicts) and backends that care about the conflict
path, so it wouldn't happen with Git backends, and it wouldn't happen
at Google either (because we use tree-level conflicts).
I don't think there's any reason to use the local backend in tests
instead of using the stricter test backend.
I think we should generally use the test backend in tests and only use
the local backend or git backend when there's a particular reason to
do so (such as in `test_bad_locking` where the on-disk directory
structure matters). But this patch only deals with the simpler cases
where we were only testing with the local backend.
This fixes a bug where we used the parent directory's path when trying
read trees and files for a child entry. Many tests in
`test_merged_tree` fail after switching to the test backend there
without this fix/
We ran into a bug in `MergedTree` with our commit backend at
Google. The problem there was that `MergedTree` sometimes uses the
wrong path when reading files and trees. We didn't catch the bug in
our tests (outside of Google) because both our backends let you read
files and trees at any path.
This commit introduces a stricter backend that we can use in tests to
catch this kind of bug. For simplicity, it stores all data in
memory. Since tests are short-lived, I think that should be fine.
For now, this backend is stricter only in that it doesn't mix objects
written to different paths. We can make it strict/lossy in other ways
later (e.g. modifying written commit objects).
I think having a backend designed for tests can also be useful for
later making it possible to control the backend, e.g. to inject
errors.
We may want to replace almost all uses of the local backend in tests
with uses of this new test backend.
It makes the call sites clearer if we pass the `TestRepoBackend` enum
instead of the boolean `use_git` value. It's also more extensible (I
plan to add another backend for tests).
I don't think there's much reason to run most tests with a `.git`
directory outside of `.jj`. I think it's just that way for historical
reasons. It's been that way since I added support for `.jj`-internal
repos in a8a9f7dedd.
The reason I want to switch is to make it a little easier to create
test repos for different backends. The problem with `.jj`-external git
repos is that they depend on an additional path.
I had to update `test_bad_locking.rs` to make the code merging
directories able handle missing directories on some side, because
git's loose objects result in directories getting created on one or
both sides.
I ran into some issues here when switching our tests to use
`.jj`-internal git repos. For example, the `std::fs::copy()` calls
started failing, which may be related to #2103. I think one problem is
that we could end up calling `merge_directories()` twice for the same
directory. This patch fixes that by deduping the paths we call with,
and makes the function assume that the output directory doesn't exist.
I've noticed `WorkspaceCommandHelper::format_file_path()` appear in
profiles a few times. A big part of that is spent in
`RepoPath::to_fs_path()`. I think I had been thinking that
`PathBuf::join()` takes `self` by value and mutates it, but it turns
out it creates a new instance. So our `result = result.join(...)` in a
loop was copying the `PathBuf` over and over. This fixes that and also
reserves the expected size. That speeds up `jj files
--ignore-working-copy -r v6.0` in the Linux repo from 546.0 s to 509.3
s (6.7%).
The main goal of this change is to enable tree-level conflict format, but it
also allows us to bulk-import commits on clone/init. I think a separate method
will help if we want to provide progress information, enable check for
.jjconflict entries under certain condition, etc.
Since git::import_refs() now depends on GitBackend type, it might be better to
remove git_repo from the function arguments.
Since we have overloaded operator symbols, we need to deduplicate them
upfront. Legacy and compat operators are also removed from the suggestion.
It's a bit ugly to mutate the error struct before calling Error::renamed_rule(),
but I think it's still better than reimplementing message formatting function.
Since e7e49527ef "git: ensure that remote branches never diverge", the last
known "refs/remotes" ref should be synced with the corresponding remote branch.
So we can always trust the branch@remote expression. We don't need "refs/tags"
lookup either since tags should have been imported by git::import_refs().
FWIW, I'm thinking of reorganizing view.git_refs() map as per-remote views.
It would be nice if we can get rid of revsets and template keywords exposing
low-level Git ref primitives.
Git doesn't have a root commit, so we should skip branches pointing to
it on export, just like we do with conflicted branches (which Git also
doesn't support).
Before this patch, the order would depend on the reason we failed to
export a ref, because we would add to the `failed_branches` list in
several different places. What's worse, when the export failed because
the branch was conflicted or had an invalid name (from Git's
perspective), it was non-deterministic because we iterated over a
HashSet. This patch fixes that by sorting at the end.
Note that we still want the `branches_to_update` map to be a
`BTreeMap` so we update branches in deterministic order. Otherwise the
error when trying to export both branches `main` and `main/sub` will
become non-deterministic.
Suppose "x::y" is the operator that defaults to "root()::visible_heads()"
respectively, "::" is identical to "all()". Since we've just changed the
behavior of "..y", ".." is now "root()..visible_heads()" meaning "~root()".
I was looking into some overly verbose logs and happened to notice
that `Store` uses the default derived, which presumably means it's
going to include all objects in its cached. Just including the
backend's debug string seems enough.
In `LockedWorkingCopy::drop()`, we panic if the caller had not called
`finish()`. IIRC, the idea was both to find bugs where we forgot to
call `finish()` and to prevent continuing with a modified
`WorkingCopy` instance. I don't think the former has been a problem in
practice. It has been a problem in practice to call `discard()` to
avoid the panic, though. To address that, we can make the `Drop`
implementation discard the changes (forcing a reload of the state if
the working copy is accessed again).
When restoring (`jj restore`) a 3-sided conflict from one tree into a
2-sided tree (or a resolved tree), we'll need to extend the size arity
of the target tree to that of the source tree. I had not considered
this case before. This patch relaxes the constraint in
`MergedTreeBuilder` to allow such cases. The additional trees are
based on empty trees with only the larger overrides in them.
Many (most?) callers of `Store::empty_tree_id()` really want a
`MergedTreeId`, so let's create a helper for that. It returns the
`Legacy` variant, which is what all current callers used. That should
be all we need since the two variants compare equal these days, and
since trees built based on the legacy variant can get promoted to the
new variant on write if the config is enabled.
When we start writing tree-level conflicts in an existing repo, we
don't want commits that change the format to be non-empty if they
don't change any content. This patch updates `MergeTreeId::eq()` to
consider two resolved trees equal even if only their `MergedTreeId`
variant is different (one is path-level and one is tree-level).
I think I've gone through all places we compare tree ids and checked
that it's safe to compare them this way. One consequence is that
rebasing a commit without changing the parents (typically
auto-rebasing after `jj describe`) will not lead to the tree id
getting upgraded, due to an optimization we have for that case. I
don't think that's serious enough to handle specially; we'll have to
support the old format for existing repos for a while regardless of a
few commits not getting upgraded right away.
The number of failing tests with the config option enabled drop from
108 to 11 with this patch.
We're finally ready to start writing trees using the new format where
we represent conflicts by having multiple trees in the commit instead
of having a single tree with multiple entries at a path. This patch
adds a config option for that. It's not ready to be used yet, so I
haven't updated the release notes or other documentation.
I added only a simple CLI test for testing what happens when the
config is enabled in an existing repo. 108 tests currently fail if we
flip the default.
I think most tests want a `MergedTree`, so this makes `create_tree()`
return that. I kept the old function as `create_single_tree()`. That's
now only used in `test_merge_trees` and `test_merged_tree`.
I also consistently imported the functions now, something I've
considered doing for a long time.
I made it simply fail on explicit fetch/import, and ignored on implicit import.
Since the error mode is predictable and less likely to occur. I don't think it
makes sense to implement warning propagation just for this.
Closes#1690.
With the already existing `MergedTree::resolve()` and all the recent
refactorings into `Merge<T>`, it's now very easy to add support for
3-way merging of `MergedTree` instances.
