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

✨(clone): set publicheads to remote HEAD when cloning #607

Closed
wants to merge 1 commit into from

Conversation

vegerot
Copy link
Contributor

@vegerot vegerot commented Apr 18, 2023

✨(clone): set publicheads to remote HEAD when cloning

Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case develop), instead of main/master by default.

However, this doesn't mark remote/develop and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes #600

Pull Request resolved: #607

Test Plan:

  • Added test test-git-clone-sets-publicheads.t

vegerot added a commit to vegerot/sapling that referenced this pull request Apr 18, 2023
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

Test Plan:
- Added test `test-git-clone-sets-publicheads.t`
vegerot added a commit to vegerot/sapling that referenced this pull request Apr 18, 2023
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

Test Plan:
- Added test `test-git-clone-sets-publicheads.t`
@vegerot
Copy link
Contributor Author

vegerot commented Apr 18, 2023

question for reviewer: should I add remote/master and remote/main to the publicheads by default? At first I thought I wanted to because:

  • if you have not set remotenames.publicheads, then sl config remotenames.publicheads returns remote/master,remote/main
  • if you set remotenames.publicheads to be foo, then sl config remotenames.publicheads returns foo.

If a repo sets its default branch to dev, should main and master still be considered public commits? I think so, which is why I re-add default_publicheads.

However, upon further inspection I might be wrong. Maybe I don't need to re-add default_publicheads. It looks like it might be a hardcoded public commit, even when it's not in remotenames.publicheads.

Steps to reproduce:

  1. sl config remotenames.publicheads=asdf
  2. sl hide remote/master
❯ sl hide remote/master
abort: cannot hide immutable commit: 1b4babc6843b
(see 'sl help phases' for details)

Copy link
Contributor

@zzl0 zzl0 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing it, looks goods to me!

I just had some minor suggestions

eden/scm/edenscm/git.py Outdated Show resolved Hide resolved
rcutil.editconfig(repo.ui,
repo.localvfs.join(configfilename),
'remotenames', 'publicheads',
all_publicheads)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could you move this logic into a separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think? I added a function update_and_persist_repo_config in rcutil, but maybe it would be better to add a method to localrepository so this line would become

repo.update_and_persist_config(`remotenames`, `publicheads`, all_publicheads)

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 went ahead and did it. Please see the latest version of this PR.

(If you want to see what I did before, check Version 4 in ReviewStack https://reviewstack.dev/facebook/sapling/pull/607)

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer keep it in git.py, but I don't think it should be a blocker for this PR. I am importing it now.

eden/scm/tests/test-git.t Outdated Show resolved Hide resolved
@zzl0
Copy link
Contributor

zzl0 commented Apr 18, 2023

sl config remotenames.publicheads=asdf

You need to use sl config -l remotenames.publicheads=asdf to update the repo config

@zzl0
Copy link
Contributor

zzl0 commented Apr 20, 2023

Hi @vegerot, just wondering if you plan to update this PR to fix those small comments? it would be great if we can include in the next release.

If you don't have time, I can also import this PR and fix them.

@vegerot
Copy link
Contributor Author

vegerot commented Apr 20, 2023

@zzl0 most of my free time is on the weekends. When's the cutoff for the next Sapling release?

@zzl0
Copy link
Contributor

zzl0 commented Apr 20, 2023

@vegerot there is no exact cutoff date, our next release schedule is next week.

With that said, it's not urgent, take your time. I just wanted to check if it's on your plan.

@vegerot
Copy link
Contributor Author

vegerot commented Apr 20, 2023

@zzl0 thanks! I'll probably do it this weekend. If not, feel free to import and patch this

vegerot added a commit to vegerot/sapling that referenced this pull request Apr 22, 2023
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

Test Plan:
- Added test `test-git-clone-sets-publicheads.t`
vegerot added a commit to vegerot/sapling that referenced this pull request Apr 22, 2023
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

Test Plan:
- Added test `test-git-clone-sets-publicheads.t`
vegerot added a commit to vegerot/sapling that referenced this pull request Apr 24, 2023
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

Test Plan:
- Added test `test-git-clone-sets-publicheads.t`
Summary:
c8b66c6 added support for cloning the remote git repo's HEAD (in this
case `develop`), instead of main/master by default.

However, this doesn't mark `remote/develop` and its ancestors as public
commits.

This commit will also set remote git repo's HEAD as a public commit

Closes facebook#600

Pull Request resolved: facebook#607

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

@zzl0 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@zzl0 merged this pull request in 0ce7010.

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.

bug(phases): mark remote HEAD and all ancestors as public
3 participants