git: update jj git clone|fetch to use new GitFetch api directly. #5444
+241
−189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
yuja
reviewed
Jan 24, 2025
Comment on lines
+60
to
+79
| let signature = | ||
| git2::Signature::new("Some One", "[email protected]", &git2::Time::new(0, 0)).unwrap(); | ||
| let mut tree_builder = git_repo.treebuilder(None).unwrap(); | ||
| let file_oid = git_repo.blob(remote.as_bytes()).unwrap(); | ||
| tree_builder | ||
| .insert("file", file_oid, git2::FileMode::Blob.into()) | ||
| .unwrap(); | ||
| let tree_oid = tree_builder.write().unwrap(); | ||
| let tree = git_repo.find_tree(tree_oid).unwrap(); | ||
| // our branch name is the same as the source branch name | ||
| git_repo | ||
| .commit( | ||
| Some(&format!("refs/heads/{src_remote}")), | ||
| &signature, | ||
| &signature, | ||
| "message", | ||
| &tree, | ||
| &[], | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
nit: probably better to extract add_git_commit() function. I don't think clone + add_commit is common operation.
|
|
||
| let bookmark_output1 = get_bookmark_output(&test_env1, &repo_path1); | ||
| let bookmark_output2 = get_bookmark_output(&test_env2, &repo_path2); | ||
| assert_eq!(bookmark_output1, bookmark_output2); |
There was a problem hiding this comment.
It's not important that the conflicts are sorted by remote name.
A bigger problem reported in #4920 is that an abandoned commit in one remote becomes hidden (or abandoned) even if the commit is referenced by the other remote, and the descendants were rebased iirc. Can you add the test for this scenario?
|
Forgot to upload new version? |
bsdinis
force-pushed
the
bsdinis/push-urozxlpwnrmp
branch
from
7d63a2e to
496e6f9
Compare
|
updated now. was trying to understand the test case you were referring too, need more time to understand it |
It's something that a ref is rewritten (or abandoned) by one remote, and the same ref is moved forward by the other remote. rm -fR remote1 remote2 local
jj git init remote1
jj -R remote1 desc -m 'remote 1'
jj -R remote1 bookmark create foo
jj -R remote1 git export
jj git clone remote1/.jj/repo/store/git remote2
jj -R remote2 bookmark track glob:'foo@*'
jj -R remote2 git export
jj git clone remote1/.jj/repo/store/git --remote=remote1 local
jj -R local git remote add remote2 remote2/.jj/repo/store/git
jj -R local git fetch --remote=remote2
jj -R local bookmark track glob:'foo@*'
jj -R local log -r '::(visible_heads() | remote_bookmarks())'
# rewrite bookmarked commit
jj -R remote1 desc -m 'remote 1+'
jj -R remote1 git export
jj -R remote1 log -r'::(visible_heads() | remote_bookmarks())'
# move bookmark forward
jj -R remote2 new foo -m 'remote 2+'
jj -R remote2 bookmark move --to=@ foo
jj -R remote2 git export
jj -R remote2 log -r'::(visible_heads() | remote_bookmarks())'
# the result should be roughly equivalent to
# jj -R local git fetch --remote=remote1
# jj -R local git fetch --remote=remote2
jj -R local git fetch --remote=remote1 --remote=remote2
jj -R local log -r '::(visible_heads() | remote_bookmarks())' |
Currently, the subprocessing code paths in the library were relying on git2 to figure out if a remote exists. This is because when git errors out for a remote not existing it outputs the same error as when the repository is not found. As the cli tool makes a distinction between the two (e.g., after cloning, the remote must exist and as such we cannot send out an error that is shared by `no remote found` and `no repository found`), this was a hack put in place to sort out the difference. It calls out to `git remote` to list out the remotes.
* 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. Original commit by @essiene
bsdinis
force-pushed
the
bsdinis/push-urozxlpwnrmp
branch
from
496e6f9 to
3e24640
Compare
bsdinis
force-pushed
the
bsdinis/push-zosznwvkmtnt
branch
3 times, most recently
from
f7fbd21 to
1857065
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GitFetchapi public.git::fetchtolib/tests/test_git.rsasgit_fetch, to minimize churn in the tests. Update test call sites to usegit_fetchgit::fetchfromlib/src/git.rs.jj git cloneandgit_fetchincli/src/git_utils.rsto use the new api directly. Removing one redundant layer of indirection.jj git fetchwith multiple remotes can abandon reachable commits (and rebase reachable descendants) #4920 as it first fetches from all remotes beforeimport_refs()is called, so there is no race condition if the same commit is treated differently in different remotes specified in the same command.Original commit by @essiene #4960