Skip to content

Commit

Permalink
wip: set publicheads to remote HEAD when cloning
Browse files Browse the repository at this point in the history
  • Loading branch information
vegerot committed Apr 16, 2023
1 parent 9b1da2c commit 5cf6cd1
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
10 changes: 9 additions & 1 deletion eden/scm/edenscm/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import bindings
from edenscm import tracing

from . import bookmarks as bookmod, error, identity, progress, util
from . import bookmarks as bookmod, error, identity, progress, util, rcutil
from .i18n import _
from .node import bin, hex, nullid

Expand Down Expand Up @@ -153,6 +153,14 @@ def clone(ui, url, destpath=None, update=True, pullnames=None):
# If `git ls-remote --symref <url> HEAD` failed to yield a name,
# fall back to the using the names in the config.
pullnames = bookmod.selectivepullbookmarknames(repo)
default_publicheads = repo.ui.config('remotenames', 'publicheads').split(',')
# this is wrong when the remote HEAD is one of the defaults (or
# no remote found), because it will duplicate remote/master
remote_publicheads = ['remote/' + path for path in pullnames]
all_publicheads = ','.join(default_publicheads+ remote_publicheads)

This comment has been minimized.

Copy link
@strager

strager Apr 16, 2023

Easy solution to the duplicate problem: Use a set.

                all_publicheads = ','.join(sorted(set(default_publicheads + remote_publicheads)))

(I added sorted to make the output deterministic.)

This comment has been minimized.

Copy link
@strager

strager Apr 16, 2023

I personally think we shouldn't use default_publicheads at all. But I don't know the implications.

This comment has been minimized.

Copy link
@vegerot

vegerot Apr 18, 2023

Author Owner

I'm not sure of the implications either. Here's what I know:

  • 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.

This comment has been minimized.

Copy link
@vegerot

vegerot Apr 18, 2023

Author Owner

upon closer inspection, 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)

# TODO: .hg
rcutil.editconfig(repo.ui, destpath + '/.sl/config', 'remotenames', 'publicheads', all_publicheads)

# Make sure we pull "update". If it looks like a hash, add to
# "nodes", otherwise to "names".
Expand Down
10 changes: 10 additions & 0 deletions eden/scm/tests/test-git.t
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ Test clone with flags (--noupdate, --updaterev):
remote/master 3f5848713286
$ cd ..

#This test is VERY suspicious. It's supposed to test `ls-remote` symref HEAD parsing, but it passes `--updaterev` that totally circumvents all the code it's supposed to test
$ hg clone --git "$TESTTMP/gitrepo" -u foo cloned1
From $TESTTMP/gitrepo
* [new ref] 3f5848713286c67b8a71a450e98c7fa66787bde2 -> remote/master
Expand All @@ -268,6 +269,15 @@ Test clone with flags (--noupdate, --updaterev):
$ hg --cwd cloned1 log -r . -T '{node|short} {remotenames} {desc}\n'
57eda5013e06 remote/foo alpha3

$ (cd $TESTTMP/gitrepo && git switch foo)
$ hg clone --git "$TESTTMP/gitrepo" cloned3
From $TESTTMP/gitrepo
* [new ref] 3f5848713286c67b8a71a450e98c7fa66787bde2 -> remote/master
* [new ref] 57eda5013e068ac543a52ad073cec3d7750113b5 -> remote/foo
2 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg --config publicheads
remote/master,remote/foo

This comment has been minimized.

Copy link
@strager

strager Apr 16, 2023

Buck the trend and make a dedicated test file for this, perhaps?

This comment has been minimized.

Copy link
@vegerot

vegerot Apr 18, 2023

Author Owner

Good idea. You can see this change and everything else in facebook#607


$ hg clone --updaterev foo --git "$TESTTMP/gitrepo" cloned2
From $TESTTMP/gitrepo
* [new ref] 3f5848713286c67b8a71a450e98c7fa66787bde2 -> remote/master
Expand Down

0 comments on commit 5cf6cd1

Please sign in to comment.