Skip to content

git: export git ref to parent branch when possible #6546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakobhellermann
Copy link
Contributor

This is helpful for

  • editors with git integration that show a warning when on an detached head (intellij etc.)
  • using git and jj together on a repository

It is only done if there is a single parent with a single bookmark, otherwise it wouldn't be obvious which one to pick.

Checklist

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

This is helpful for
- editors with git integration that show a warning when on an detached
  head (intellij etc.)
- using git and jj together on a repository

It is only done if there is a single parent with a single bookmark,
otherwise it wouldn't be obvious which one to pick.
mut_repo
.view()
.bookmarks()
.filter(|(_, bookmark)| bookmark.local_target == &new_head_target)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a scan over all bookmarks. The commit template uses an CommitRefsIndex to accelerate this, but as far as I can tell there's nothing to improve this available here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can use local_bookmarks_for_commit()? It's not optimized in any sense, though.

Comment on lines +1292 to +1299
to_git_ref_name(
GitRefKind::Bookmark,
name.to_remote_symbol(REMOTE_NAME_FOR_LOCAL_GIT_REPO),
)
.and_then(|name| {
gix::refs::FullName::try_from(BString::from(name.into_string())).ok()
})
.map(gix::refs::Target::Symbolic)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the best way to go from bookmark to gix target.

Also I don't think it it guaranteed that the bookmarks have been exported to git, so the code would have to check if the bookmark exists in git as well?

Alternatively, instead of checking if the parent jj commit has a jj bookmark, we could check if the parent git commit has a branch and check that out using gix, regardless of the jj state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, we can check if <name> bookmark is in sync with <name>@git. If they point to the same commit, the bookmark would exist in Git (ignoring TOCTOU issue.) A stricter option might be to issue RefEdit to update the HEAD ref, plus noop RefEdit to update the target bookmark (to the same value.0

@@ -1259,7 +1259,7 @@ pub fn reset_head(mut_repo: &mut MutableRepo, wc_commit: &Commit) -> Result<(),

// If the first parent of the working copy has changed, reset the Git HEAD.
let old_head_target = mut_repo.git_head();
if old_head_target != new_head_target {
if old_head_target != new_head_target || true {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to leave the || true there? If so, what is accomplishing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that was just for testing. I'll remove it

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a config knob to opt in to this behavior. It would be surprising if HEAD becomes undetached by checking out an arbitrary commit.

mut_repo
.view()
.bookmarks()
.filter(|(_, bookmark)| bookmark.local_target == &new_head_target)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can use local_bookmarks_for_commit()? It's not optimized in any sense, though.

Comment on lines +1292 to +1299
to_git_ref_name(
GitRefKind::Bookmark,
name.to_remote_symbol(REMOTE_NAME_FOR_LOCAL_GIT_REPO),
)
.and_then(|name| {
gix::refs::FullName::try_from(BString::from(name.into_string())).ok()
})
.map(gix::refs::Target::Symbolic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, we can check if <name> bookmark is in sync with <name>@git. If they point to the same commit, the bookmark would exist in Git (ignoring TOCTOU issue.) A stricter option might be to issue RefEdit to update the HEAD ref, plus noop RefEdit to update the target bookmark (to the same value.0

Comment on lines +97 to +101
if self.data.parents.len() == 1 {
Some(&self.data.parents[0])
} else {
None
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: .as_slice() then pattern match?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants