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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/src/commit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ impl Commit {
&self.data.parents
}

pub fn single_parent(&self) -> Option<&CommitId> {
if self.data.parents.len() == 1 {
Some(&self.data.parents[0])
} else {
None
}
Comment on lines +97 to +101
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?

}

pub fn parents(&self) -> impl Iterator<Item = BackendResult<Commit>> + use<'_> {
self.data.parents.iter().map(|id| self.store.get_commit(id))
}
Expand Down
45 changes: 37 additions & 8 deletions lib/src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,7 @@ pub fn export_some_refs(
update_git_head(
&git_repo,
gix::refs::transaction::PreviousValue::MustExistAndMatch(old_target),
current_oid,
current_oid.map(gix::refs::Target::Object),
)
.map_err(GitExportError::from_git)?;
}
Expand Down Expand Up @@ -1191,11 +1191,11 @@ fn update_git_ref(
fn update_git_head(
git_repo: &gix::Repository,
expected_ref: gix::refs::transaction::PreviousValue,
new_oid: Option<gix::ObjectId>,
new_target: Option<gix::refs::Target>,
) -> Result<(), gix::reference::edit::Error> {
let mut ref_edits = Vec::new();
let new_target = if let Some(oid) = new_oid {
gix::refs::Target::Object(oid)
let new_target = if let Some(target) = new_target {
target
} else {
// Can't detach HEAD without a commit. Use placeholder ref to nullify
// the HEAD. The placeholder ref isn't a normal branch ref. Git CLI
Expand Down Expand Up @@ -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

let expected_ref = if let Some(id) = old_head_target.as_normal() {
// We have to check the actual HEAD state because we don't record a
// symbolic ref as such.
Expand All @@ -1276,9 +1276,38 @@ pub fn reset_head(mut_repo: &mut MutableRepo, wc_commit: &Commit) -> Result<(),
// Just overwrite if unborn (or conflict), which is also unusual.
gix::refs::transaction::PreviousValue::MustExist
};
let new_oid = new_head_target
.as_normal()
.map(|id| gix::ObjectId::from_bytes_or_panic(id.as_bytes()));

let new_oid = new_head_target.as_normal().map(|id| {
let single_parent_bookmark = wc_commit
.single_parent()
.and_then(|_| {
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.

.exactly_one()
.ok()
})
.and_then(|(name, _)| {
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)
Comment on lines +1292 to +1299
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

});

match single_parent_bookmark {
Some(target) => target,
None => {
let id = gix::ObjectId::from_bytes_or_panic(id.as_bytes());
gix::refs::Target::Object(id)
}
}
});

update_git_head(&git_repo, expected_ref, new_oid)
.map_err(|err| GitResetHeadError::UpdateHeadRef(err.into()))?;
mut_repo.set_git_head_target(new_head_target);
Expand Down
Loading