Skip to content
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

fix(dotgit): set publicheads correctly for transparent git mode #969

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

Conversation

vegerot
Copy link
Contributor

@vegerot vegerot commented Oct 22, 2024

Summary:
0ce7010 taught Sapling to use the correct public head based on the upstream
default branch. However, this doesn't work when using the transparent git mode
through git clone.

This diff will correctly set the publicheads config when initializing sapling

Test Plan: see test-git-clone-sets-publicheads.t

Summary:
0ce7010 taught Sapling to use the correct public head based on the upstream
default branch.  However, this doesn't work when using the transparent git mode
through `git clone`.

This diff will correctly set the publicheads config when initializing sapling

Test Plan: see test-git-clone-sets-publicheads.t
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

let config = String::from_utf8(out.stdout)?;
let remotes_out = self
.git_cmd(
"ls-remote", &["--symref", ".", "HEAD"])
Copy link
Contributor

Choose a reason for hiding this comment

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

This performs a network call, which might be unexpected or undesirable.

How about parsing the output of git rev-parse --symbolic refs/remotes/origin/HEAD instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! If it's as robust as ls-remote then this will save a network call.

Copy link
Contributor

@quark-zju quark-zju Oct 22, 2024

Choose a reason for hiding this comment

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

You can use lookup_reference to resolve the reference without the spawning process overhead.

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

Successfully merging this pull request may close these issues.

4 participants