git: update `jj git clone|fetch` to use new GitFetch api directly. #4960

essiene · 2024-11-24T03:17:37Z

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 jj git fetch with multiple remotes can abandon reachable commits (and rebase reachable descendants) #4920 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

Checklist

If applicable:

I have updated CHANGELOG.md
I have added tests to cover my changes

lib/src/git.rs

CHANGELOG.md

yuja · 2024-12-01T13:52:48Z

CHANGELOG.md

+* `jj git fetch` with multiple remotes will now fetch from all remotes before
+  importing refs into the jj repo. This fixes a race condition where the treatment
+  of a commit that is found in multiple fetch remotes depended on the order the
+  remotes where specified.


Can you add a test for this issue?

cli/src/commands/git/clone.rs

cli/src/git_util.rs

lib/src/git.rs

martinvonz

git: refactor jj git clone|fetch to use new GitFetch api directly.
[...]
This fixes #4920 as it first fetches from all remotes before import_refs()

nit: I think "refactor" is supposed to mean that it has no impact on functionality, so just say "git: use new GitFetch api directly in jj git clone|fetch" or something instead

essiene · 2024-12-15T21:34:31Z

git: refactor jj git clone|fetch to use new GitFetch api directly.
[...]
This fixes #4920 as it first fetches from all remotes before import_refs()

nit: I think "refactor" is supposed to mean that it has no impact on functionality, so just say "git: use new GitFetch api directly in jj git clone|fetch" or something instead

makes sense. Updated.

* Make the new `GitFetch` api public. * Move `git::fetch` to `lib/tests/test_git.rs` as `git_fetch`, to minimize churn in the tests. Update test call sites to use `git_fetch` * Delete the `git::fetch` from `lib/src/git.rs`. * 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 #4920 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. * Add test for the race condition. Fixes: #4920

yuja

Thanks.

yuja · 2024-12-16T10:09:17Z

CHANGELOG.md

Restore formatting changes in CHANGELOG.md

yuja · 2024-12-16T10:09:59Z

cli/src/commands/git/clone.rs

-    })?;
-    print_git_import_stats(ui, fetch_tx.repo(), &stats.import_stats, true)?;
+    let default_branch =
+        with_remote_git_callbacks(ui, None, |cb| -> Result<Option<String>, CommandError> {


nit: type annotation isn't needed

yuja · 2024-12-16T10:10:45Z

cli/src/commands/git/clone.rs

+        GitFetchStats {
+            default_branch,
+            import_stats,
+        },


nit: we don't need to return import_stats. It will help remove struct GitFetchStats. (can be addressed later)

yuja · 2024-12-16T10:11:57Z

cli/src/git_util.rs

-            GitFetchError::GitImportError(err) => err.into(),
-            GitFetchError::InternalGitError(err) => map_git_error(err),
-            _ => user_error(err),
+        with_remote_git_callbacks(ui, None, |cb| -> Result<(), CommandError> {


nit: type annotation isn't needed if ?; Ok(()) is removed

yuja · 2024-12-16T10:14:05Z

cli/tests/test_git_fetch.rs

+      @rem1: qxosxrvv 6a211027 message
+    rem2: yszkquru 2497a8a0 message
+      @rem2: yszkquru 2497a8a0 message
+    "###);


Does this test fail without this patch? iirc, the issue was that a bookmark was moved in one remote, and descendant commit of that bookmark was added by the other remote, or something like that.

yuja · 2024-12-16T10:15:06Z

lib/src/git.rs

@@ -1394,31 +1393,6 @@ pub struct GitFetchStats {
    pub import_stats: GitImportStats,
 }

-#[tracing::instrument(skip(mut_repo, git_repo, callbacks))]


nit: perhaps, tracing::instruments() will have to be moved to fetch() (and maybe import_refs())

essiene requested a review from yuja November 24, 2024 03:20

essiene mentioned this pull request Nov 24, 2024

Improve fetch from multiple remotes #4923

Open

5 tasks

yuja reviewed Nov 24, 2024

View reviewed changes

lib/src/git.rs Outdated Show resolved Hide resolved

essiene force-pushed the essiene/nwtzqzumrzyw branch 3 times, most recently from c35a6f7 to c2e4cb0 Compare November 30, 2024 18:35

essiene force-pushed the essiene/knwwrnytzrry branch from 08525a0 to a846163 Compare November 30, 2024 22:49

essiene force-pushed the essiene/nwtzqzumrzyw branch 4 times, most recently from ed6876d to d5c1a07 Compare December 1, 2024 11:48

essiene force-pushed the essiene/knwwrnytzrry branch 2 times, most recently from 4950b09 to 336bbdd Compare December 1, 2024 12:09

essiene force-pushed the essiene/nwtzqzumrzyw branch from d5c1a07 to 5275f9a Compare December 1, 2024 12:09

Base automatically changed from essiene/nwtzqzumrzyw to main December 1, 2024 12:22

essiene force-pushed the essiene/knwwrnytzrry branch from 336bbdd to 11bf992 Compare December 1, 2024 12:23

essiene changed the title ~~jj-lib: git::fetch takes multiple remotes.~~ jj-lib: refactor jj git clone|fetch and `to use new GitFetch api directly. Dec 1, 2024

essiene force-pushed the essiene/knwwrnytzrry branch 2 times, most recently from 0f2cc4d to 6182c03 Compare December 1, 2024 12:26

essiene changed the title ~~jj-lib: refactor jj git clone|fetch and `to use new GitFetch api directly.~~ jj-lib: refactor jj git clone|fetch to use new GitFetch api directly. Dec 1, 2024

essiene force-pushed the essiene/knwwrnytzrry branch 2 times, most recently from 813822d to 51d8892 Compare December 1, 2024 12:32

yuja reviewed Dec 1, 2024

View reviewed changes

essiene changed the title ~~jj-lib: refactor jj git clone|fetch to use new GitFetch api directly.~~ git: refactor jj git clone|fetch to use new GitFetch api directly. Dec 1, 2024

essiene force-pushed the essiene/knwwrnytzrry branch 5 times, most recently from d661392 to d96eb38 Compare December 8, 2024 19:44

martinvonz reviewed Dec 8, 2024

View reviewed changes

essiene force-pushed the essiene/knwwrnytzrry branch 2 times, most recently from ba5b940 to b2e66aa Compare December 15, 2024 21:32

essiene changed the title ~~git: refactor jj git clone|fetch to use new GitFetch api directly.~~ git: update jj git clone|fetch to use new GitFetch api directly. Dec 15, 2024

essiene force-pushed the essiene/knwwrnytzrry branch from b2e66aa to b733e17 Compare December 15, 2024 21:45

essiene force-pushed the essiene/knwwrnytzrry branch from b733e17 to 6757530 Compare December 15, 2024 21:49

yuja reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git: update `jj git clone|fetch` to use new GitFetch api directly. #4960

git: update `jj git clone|fetch` to use new GitFetch api directly. #4960

essiene commented Nov 24, 2024 •

edited

Loading

yuja Dec 1, 2024

martinvonz left a comment

essiene commented Dec 15, 2024

yuja left a comment

yuja Dec 16, 2024

yuja Dec 16, 2024

yuja Dec 16, 2024

yuja Dec 16, 2024

yuja Dec 16, 2024

yuja Dec 16, 2024

git: update jj git clone|fetch to use new GitFetch api directly. #4960

Are you sure you want to change the base?

git: update jj git clone|fetch to use new GitFetch api directly. #4960

Conversation

essiene commented Nov 24, 2024 • edited Loading

Checklist

yuja Dec 1, 2024

Choose a reason for hiding this comment

martinvonz left a comment

Choose a reason for hiding this comment

essiene commented Dec 15, 2024

yuja left a comment

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

yuja Dec 16, 2024

Choose a reason for hiding this comment

git: update `jj git clone|fetch` to use new GitFetch api directly. #4960

git: update `jj git clone|fetch` to use new GitFetch api directly. #4960

essiene commented Nov 24, 2024 •

edited

Loading