Improve fetch from multiple remotes #4923
Conversation
mateon1
force-pushed
the
multifetch
branch
2 times, most recently
from
e81be24 to
bbb44a7
Compare
|
Here's how fetching looks like in the situation given in #4920 with this PR: For comparison, here are the situations with separate fetch operations (behavior not affected by this PR) b, then cc, then b |
|
Perhaps, the easiest workaround is to do diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs
index 74ebe2d2fa..8949218232 100644
--- a/cli/src/git_util.rs
+++ b/cli/src/git_util.rs
@@ -496,6 +496,11 @@
_ => user_error(err),
})?;
print_git_import_stats(ui, tx.repo(), &stats.import_stats, true)?;
+ let settings = tx.settings().clone(); // XXX silly clone
+ let num_rebased = tx.repo_mut().rebase_descendants(&settings)?;
+ if num_rebased > 0 {
+ writeln!(ui.status(), "Rebased {num_rebased} descendant commits")?;
+ }
}
warn_if_branches_not_found(
ui,It might be cleaner to do import at the end, but it's weird that |
|
Yuja's patch does work, with this result: So it's a direct fix to the current bug while keeping existing behavior. Re: default_branch, that's also a bit weird. It's only used from jj git clone, which only supports a single remote. Even if it did support multiple, how could you possibly know which remote is 'most authoritative' for choosing the default branch? What if you clone two forks of the same repo? What if you clone two completely independent repos? |
You can turn it off by
Perhaps, the API can be refactored to something like this: let mut peer = GitFetch::new(..);
let default_branch = peer.fetch(remote_name, branch_names, ..);
...
let stats = peer.import_refs();(and @essiene might be interested. |
This seems like a good approach. We could call peer.fetch() in a loop for all remotes and at the end, call peer.import_refs just once.
We could also have a thin wrapper for If there are no objections, I go ahead and send out a few PRs that do this.
Indeed. Thanks for looping me in. |
Sounds good, thanks. BTW, I used |
Yeah, makes sense. |
* This allows more control over the stages of the fetch.
* Provide thin and directed wrappers for clone and fetch
which allow slightly different overall actions.
* Allow `git::fetch` to handle multiple remotes.
* One important side-effect to note (as demonstrated in the changed
cli test) is that when there are multiple remotes, all fetches
must pass before `import_refs` starts; no branch imports if a
fetch from any remote fails.
* Update call sites to use new `git::clone` and `git::fetch`
Fixes: #4923
* This allows more control over the stages of the fetch.
* Provide thin and directed wrappers for clone and fetch
which allow slightly different overall actions.
* Allow `git::fetch` to handle multiple remotes.
* One important side-effect to note (as demonstrated in the changed
cli test) is that when there are multiple remotes, all fetches
must pass before `import_refs` starts; no branch imports if a
fetch from any remote fails.
* Update call sites to use new `git::clone` and `git::fetch`
Fixes: #4923
* Reimplement `git::fetch` in terms of the new lower-level `GitFetch` api, which allows more control over the stages of the fetch. * Make `git::fetch` to accept multiple remotes, fetch from them all, before calling `import_refs`. * Set `default_branch` to None in the `GitFetchStats` return value from `git::fetch`. * Update call sites to use new `git::clone` and `git::fetch` * Update tests: One important side-effect to note (as demonstrated in the changed cli test) is that when there are multiple remotes, all fetches must pass before `import_refs` starts; no branch imports if a fetch from any remote fails. This is WAI. Fixes: #4923
* This allows more control over the stages of the fetch. * Implement `git::clone` in terms of the new api. * Update call sites to use new `git::clone`. * Update tests to explicitly use `git::clone` where relevant. Issue: #4923
* Reimplement `git::fetch` in terms of the new lower-level `GitFetch` api, which allows more control over the stages of the fetch. * Make `git::fetch` to accept multiple remotes, fetch from them all, before calling `import_refs`. * Set `default_branch` to None in the `GitFetchStats` return value from `git::fetch`. * Update call sites to use new `git::clone` and `git::fetch` * Update tests: One important side-effect to note (as demonstrated in the changed cli test) is that when there are multiple remotes, all fetches must pass before `import_refs` starts; no branch imports if a fetch from any remote fails. This is WAI. Fixes: #4923
* Reimplement `git::fetch` in terms of the new lower-level `GitFetch` api, which allows more control over the stages of the fetch. * Make `git::fetch` to accept multiple remotes, fetch from them all, before calling `import_refs`. * Set `default_branch` to None in the `GitFetchStats` return value from `git::fetch`. * Update call sites to use new `git::clone` and `git::fetch` * Update tests: One important side-effect to note (as demonstrated in the changed cli test) is that when there are multiple remotes, all fetches must pass before `import_refs` starts; no branch imports if a fetch from any remote fails. This is WAI. Fixes: #4923
|
* This allows more control over the stages of the fetch. * Implement `git::clone` in terms of the new api. * Update call sites to use new `git::clone`. * Update tests to explicitly use `git::clone` where relevant. Issue: #4923
* This allows more control over the stages of the fetch. * Implement `git::fetch_and_get_default_branch` in terms of the new api. * Update the cli/src/commands/Git/clone.rs to use new `git::fetch_and_get_default_branch`. * Update tests to explicitly use `git::fetch_and_get_default_branch` where relevant. * Update tests to reflect that `git::fetch` doesn't return the default branch. Issue: #4923
* This allows more control over the stages of the fetch. * Implement `git::fetch_and_get_default_branch` in terms of the new api. * Update the cli/src/commands/Git/clone.rs to use new `git::fetch_and_get_default_branch`. * Update tests to explicitly use `git::fetch_and_get_default_branch` where relevant. * Update tests to reflect that `git::fetch` doesn't return the default branch. Issue: #4923
* This allows more control over the stages of the fetch. * Implement `git::fetch_and_get_default_branch` in terms of the new api. * Update the cli/src/commands/Git/clone.rs to use new `git::fetch_and_get_default_branch`. * Update tests to explicitly use `git::fetch_and_get_default_branch` where relevant. * Update tests to reflect that `git::fetch` doesn't return the default branch. Issue: #4923
* This allows more control over the stages of the fetch. * Implement `git::fetch` in terms of the new api. * Update tests to explicitly use `git::fetch_and_get_default_branch` where relevant. Issue: #4923
* This allows more control over the stages of the fetch. * Implement `git::fetch` in terms of the new api. * Update tests to explicitly use `git::fetch_and_get_default_branch` where relevant. Issue: #4923
* GitFetch{} separates `fetch()` from `import_refs()`.
* This allows more control over the stages of the fetch so multiple fetches
can happen before `import_refs` resolves the changes in the local jj repo.
* Implement `git::fetch` in terms of the new api.
* Add a test case for initial fetch from a repo without `HEAD` set. This tests
the default branch retrieving behaviour.
Issue: #4923
…rectly. * Make the new `GitFetch` api public. * Rewrite all tests that used `git::fetch*` to use the new API. One interesting artifact of the new API is that it holds onto a reference of the `tx.repo_mut()`, so the GitFetch object needs to fall go of scope completely before we can make any other mutable calls to `tx`. * Delete the `git::fetch` * Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use the new api directly. Removing one redundant layer of indirection. * This fixes #4923 as it first fetches from all remotes before `import_refs()` is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command. Fixes: #4923
…rectly. * Make the new `GitFetch` api public. * Rewrite all tests that used `git::fetch*` to use the new API. One interesting artifact of the new API is that it holds onto a reference of the `tx.repo_mut()`, so the GitFetch object needs to fall go of scope completely before we can make any other mutable calls to `tx`. * Delete the `git::fetch` * Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use the new api directly. Removing one redundant layer of indirection. * This fixes #4923 as it first fetches from all remotes before `import_refs()` is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command. Fixes: #4923
* GitFetch{} separates `fetch()` from `import_refs()`.
* This allows more control over the stages of the fetch so multiple fetches
can happen before `import_refs` resolves the changes in the local jj repo.
* Implement `git::fetch` in terms of the new api.
* Add a test case for initial fetch from a repo without `HEAD` set. This tests
the default branch retrieving behaviour.
Issue: #4923
…rectly. * Make the new `GitFetch` api public. * Rewrite all tests that used `git::fetch*` to use the new API. One interesting artifact of the new API is that it holds onto a reference of the `tx.repo_mut()`, so the GitFetch object needs to fall go of scope completely before we can make any other mutable calls to `tx`. * Delete the `git::fetch` * Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use the new api directly. Removing one redundant layer of indirection. * This fixes #4923 as it first fetches from all remotes before `import_refs()` is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command. Fixes: #4923
…rectly. * Make the new `GitFetch` api public. * Rewrite all tests that used `git::fetch*` to use the new API. One interesting artifact of the new API is that it holds onto a reference of the `tx.repo_mut()`, so the GitFetch object needs to fall go of scope completely before we can make any other mutable calls to `tx`. * Delete the `git::fetch` * Update `jj git clone` and `git_fetch` in `cli/src/git_utils.rs` to use the new api directly. Removing one redundant layer of indirection. * This fixes #4923 as it first fetches from all remotes before `import_refs()` is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command. Fixes: #4920
See #4920 for rationale.
Checklist
If applicable:
CHANGELOG.md