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

bug(phases): mark remote HEAD and all ancestors as public #600

Closed
vegerot opened this issue Apr 10, 2023 · 4 comments
Closed

bug(phases): mark remote HEAD and all ancestors as public #600

vegerot opened this issue Apr 10, 2023 · 4 comments

Comments

@vegerot
Copy link
Contributor

vegerot commented Apr 10, 2023

steps to reproduce

  1. sl clone a git repo with many commits and no branches named main or master (e.g. a repo with one branch named develop)
  2. run sl sl

expected: should show one commit that is the public HEAD

actual:

$ sl sl
smartlog: too many (2699) commits, not rendering all of them
(consider running 'sl doctor' to hide unrelated commits)
@    652  Mar 31 at 12:27  Max.Coplan remote/develop
... shows every commit up until the initial commit

note: this bug is also visible in the commit message for c8b66c6

$ lhg clone --git https://github.com/cli/cli
$ cd cli
$ lhg
@  179e9c256  Yesterday at 14:13  2159081+qoega  remote/trunk
│  Add projectsV2 support to issue create, issue edit, pr create, and pr edit (#6735)
│
o    06ae07f97  Yesterday at 07:25  mislav
├─╮  Merge pull request #6880 from cli/setdefault-bare-repo
│ │
o │    351226d34  Yesterday at 07:17  mislav
├───╮  Merge pull request #6881 from cli/reviewers-json-fix
│ │ │
o │ │    241f9197e  Wednesday at 21:15  rmw
├─────╮  Merge pull request #6815 from cli/intake-doc
...

Explanation

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.

Suggested fix

When cloning a repository, mark the cloned head as public

@vegerot
Copy link
Contributor Author

vegerot commented Apr 10, 2023

I skimmed phases.py to see if anything stood out to me as a way to fix this bug, but didn't see anything. Would appreciate some direction on where to start looking

@zzl0
Copy link
Contributor

zzl0 commented Apr 13, 2023

@vegerot thanks for reporting the issue. You can considering add the remote name in repo config

You might need to add write logic for the repo config.

vegerot added a commit to vegerot/sapling that referenced this issue 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 issue 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

@zzl0 thanks for the ideas! What do you think of what I did in #607?

@zzl0
Copy link
Contributor

zzl0 commented Apr 18, 2023

@vegerot thanks for fixing it, commented in the PR

vegerot added a commit to vegerot/sapling that referenced this issue 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 issue 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 issue 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`
vegerot added a commit to vegerot/sapling that referenced this issue 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`
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 a pull request may close this issue.

2 participants