This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8265

git: key off `git` in .hg/requires rather than separate file
ClosedPublic

Authored by durin42 on Mar 7 2020, 5:57 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG02c47b74366c: git: key off `git` in .hg/requires rather than separate file

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Mar 7 2020, 5:57 PM
mharbison72 added a subscriber: mharbison72.Mar 8 2020, 12:12 AM
mharbison72 added inline comments.
hgext/git/__init__.py
111

Shouldn't it be an error if git is in requirements, but .git doesn't exist? Or is there something in the original function that will raise an error in this case?

durin42 updated this revision to Diff 20664.Mar 10 2020, 12:31 PM
durin42 updated this revision to Diff 20665.Mar 10 2020, 12:35 PM
durin42 marked an inline comment as done.Mar 10 2020, 12:35 PM
durin42 added inline comments.
hgext/git/__init__.py
111

Good catch! Yes, that makes sense.

martinvonz added a subscriber: martinvonz.Mar 10 2020, 12:40 PM
martinvonz added inline comments.
hgext/git/__init__.py
89–95

Why not use .hg/requires?

durin42 marked an inline comment as done.Mar 10 2020, 12:46 PM
durin42 updated this revision to Diff 20666.
durin42 updated this revision to Diff 20667.Mar 10 2020, 12:59 PM
pulkit added a subscriber: pulkit.Mar 10 2020, 1:03 PM
pulkit added inline comments.
hgext/git/dirstate.py
79 ↗(On Diff #20667)

looks like unrelated change which should be in separate patch?

durin42 added inline comments.Mar 10 2020, 1:04 PM
hgext/git/__init__.py
89–95

Because I have the dumb. Will fix shortly.

hgext/git/dirstate.py
79 ↗(On Diff #20667)

Sigh, you're right, and I'm being lazy. I'll split this out in a few minutes.

martinvonz added inline comments.Mar 10 2020, 1:08 PM
hgext/git/__init__.py
89–96

is this different from if b'git' in requirements?

tests/test-git-interop.t
5–11

$ prefix on these too since you're cleaning up anyway?

durin42 marked 4 inline comments as done.Mar 10 2020, 1:14 PM
durin42 added inline comments.
tests/test-git-interop.t
5–11

My understanding is that the > will cause the lines to all happen in the same shell invocation, whereas $ will do separate evaluations of each line, so it's (more or less) intentional. I believe it was a bug that the first line had a > instead of a $ (and since I was rearranging the intro stanza I fixed it up).

I can revert that top byte change if you'd prefer.

durin42 marked an inline comment as done.Mar 10 2020, 1:15 PM
durin42 edited parent revisions, added: D8271: git: correctly handle p1() on dirstate when underlying git repo is empty; removed: D8264: gitlog: implement __iter__ on changelog.
durin42 retitled this revision from git: key off `git` in .hg/requirements rather than separate file to git: key off `git` in .hg/requires rather than separate file.
durin42 updated this revision to Diff 20669.
martinvonz added inline comments.Mar 10 2020, 1:19 PM
tests/test-git-interop.t
5–11

I'm fine with leaving it as it is.

durin42 added a commit: rHG02c47b74366c: git: key off `git` in .hg/requires rather than separate file.Mar 10 2020, 1:26 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG02c47b74366c: git: key off `git` in .hg/requires rather than separate file (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/git/__init__.py (37 lines)
Mtests/test-git-interop.t (26 lines)

Diff 20671

hgext/git/__init__.py

pass pass
def write(self, tr=None): def write(self, tr=None):
# normally this handles things like fncache writes, which we don't have # normally this handles things like fncache writes, which we don't have
pass pass
def _makestore(orig, requirements, storebasepath, vfstype): def _makestore(orig, requirements, storebasepath, vfstype):
if b'git' in requirements:
if not os.path.exists(os.path.join(storebasepath, b'..', b'.git')):
raise error.Abort(
_(
b'repository specified git format in '
b'.hg/requires but has no .git directory'
)
martinvonzUnsubmitted
Done

Why not use .hg/requires?

martinvonz: Why not use `.hg/requires`?
durin42AuthorUnsubmitted
Done

Because I have the dumb. Will fix shortly.

durin42: Because I have the dumb. Will fix shortly.
)
martinvonzUnsubmitted
Done

is this different from if b'git' in requirements?

martinvonz: is this different from `if b'git' in requirements`?
# Check for presence of pygit2 only here. The assumption is that we'll # Check for presence of pygit2 only here. The assumption is that we'll
# run this code iff we'll later need pygit2. # run this code iff we'll later need pygit2.
if gitutil.get_pygit2() is None: if gitutil.get_pygit2() is None:
raise error.Abort( raise error.Abort(
_( _(
b'the git extension requires the Python ' b'the git extension requires the Python '
b'pygit2 library to be installed' b'pygit2 library to be installed'
) )
) )
if os.path.exists(
os.path.join(storebasepath, b'this-is-git')
) and os.path.exists(os.path.join(storebasepath, b'..', b'.git')):
return gitstore(storebasepath, vfstype) return gitstore(storebasepath, vfstype)
return orig(requirements, storebasepath, vfstype) return orig(requirements, storebasepath, vfstype)
class gitfilestorage(object): class gitfilestorage(object):
mharbison72Unsubmitted
Done

Shouldn't it be an error if git is in requirements, but .git doesn't exist? Or is there something in the original function that will raise an error in this case?

mharbison72: Shouldn't it be an error if `git` is in requirements, but `.git` doesn't exist? Or is there…
durin42AuthorUnsubmitted
Done

Good catch! Yes, that makes sense.

durin42: Good catch! Yes, that makes sense.
def file(self, path): def file(self, path):
if path[0:1] == b'/': if path[0:1] == b'/':
path = path[1:] path = path[1:]
return gitlog.filelog(self.store.git, self.store._db, path) return gitlog.filelog(self.store.git, self.store._db, path)
def _makefilestorage(orig, requirements, features, **kwargs): def _makefilestorage(orig, requirements, features, **kwargs):
store = kwargs['store'] store = kwargs['store']
if isinstance(store, gitstore): if isinstance(store, gitstore):
return gitfilestorage return gitfilestorage
return orig(requirements, features, **kwargs) return orig(requirements, features, **kwargs)
def _setupdothg(ui, path): def _setupdothg(ui, path):
dothg = os.path.join(path, b'.hg') dothg = os.path.join(path, b'.hg')
if os.path.exists(dothg): if os.path.exists(dothg):
ui.warn(_(b'git repo already initialized for hg\n')) ui.warn(_(b'git repo already initialized for hg\n'))
else: else:
os.mkdir(os.path.join(path, b'.hg')) os.mkdir(os.path.join(path, b'.hg'))
# TODO is it ok to extend .git/info/exclude like this? # TODO is it ok to extend .git/info/exclude like this?
with open( with open(
os.path.join(path, b'.git', b'info', b'exclude'), 'ab' os.path.join(path, b'.git', b'info', b'exclude'), 'ab'
) as exclude: ) as exclude:
exclude.write(b'\n.hg\n') exclude.write(b'\n.hg\n')
with open(os.path.join(dothg, b'this-is-git'), 'wb') as f: with open(os.path.join(dothg, b'requires'), 'wb') as f:
pass
with open(os.path.join(dothg, b'requirements'), 'wb') as f:
f.write(b'git\n') f.write(b'git\n')
_BMS_PREFIX = 'refs/heads/' _BMS_PREFIX = 'refs/heads/'
class gitbmstore(object): class gitbmstore(object):
def __init__(self, gitrepo): def __init__(self, gitrepo):
@property @property
def _bookmarks(self): def _bookmarks(self):
return gitbmstore(self.store.git) return gitbmstore(self.store.git)
repo.__class__ = gitlocalrepo repo.__class__ = gitlocalrepo
return repo return repo
def _featuresetup(ui, supported):
# don't die on seeing a repo with the git requirement
supported |= {b'git'}
def extsetup(ui): def extsetup(ui):
extensions.wrapfunction(localrepo, b'makestore', _makestore) extensions.wrapfunction(localrepo, b'makestore', _makestore)
extensions.wrapfunction(localrepo, b'makefilestorage', _makefilestorage) extensions.wrapfunction(localrepo, b'makefilestorage', _makefilestorage)
# Inject --git flag for `hg init` # Inject --git flag for `hg init`
entry = extensions.wrapcommand(commands.table, b'init', init) entry = extensions.wrapcommand(commands.table, b'init', init)
entry[1].extend( entry[1].extend(
[(b'', b'git', None, b'setup up a git repository instead of hg')] [(b'', b'git', None, b'setup up a git repository instead of hg')]
) )
localrepo.featuresetupfuncs.add(_featuresetup)

tests/test-git-interop.t

#require pygit2 #require pygit2
Setup: Setup:
> GIT_AUTHOR_NAME='test'; export GIT_AUTHOR_NAME $ GIT_AUTHOR_NAME='test'; export GIT_AUTHOR_NAME
> GIT_AUTHOR_EMAIL='test@example.org'; export GIT_AUTHOR_EMAIL > GIT_AUTHOR_EMAIL='test@example.org'; export GIT_AUTHOR_EMAIL
> GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0000"; export GIT_AUTHOR_DATE > GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0000"; export GIT_AUTHOR_DATE
> GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME"; export GIT_COMMITTER_NAME > GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME"; export GIT_COMMITTER_NAME
> GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL"; export GIT_COMMITTER_EMAIL > GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL"; export GIT_COMMITTER_EMAIL
> GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"; export GIT_COMMITTER_DATE > GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"; export GIT_COMMITTER_DATE
> count=10 > count=10
> gitcommit() { > gitcommit() {
martinvonzUnsubmitted
Done

$ prefix on these too since you're cleaning up anyway?

martinvonz: `$` prefix on these too since you're cleaning up anyway?
durin42AuthorUnsubmitted
Done

My understanding is that the > will cause the lines to all happen in the same shell invocation, whereas $ will do separate evaluations of each line, so it's (more or less) intentional. I believe it was a bug that the first line had a > instead of a $ (and since I was rearranging the intro stanza I fixed it up).

I can revert that top byte change if you'd prefer.

durin42: My understanding is that the > will cause the lines to all happen in the same shell invocation…
martinvonzUnsubmitted
Not Done

I'm fine with leaving it as it is.

martinvonz: I'm fine with leaving it as it is.
> GIT_AUTHOR_DATE="2007-01-01 00:00:$count +0000"; > GIT_AUTHOR_DATE="2007-01-01 00:00:$count +0000";
> GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" > GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"
> git commit "$@" >/dev/null 2>/dev/null || echo "git commit error" > git commit "$@" >/dev/null 2>/dev/null || echo "git commit error"
> count=`expr $count + 1` > count=`expr $count + 1`
> } > }
> echo "[extensions]" >> $HGRCPATH
Test auto-loading extension works:
$ mkdir nogit
$ cd nogit
$ mkdir .hg
$ echo git >> .hg/requires
$ hg status
abort: repository specified git format in .hg/requires but has no .git directory
[255]
$ git init
Initialized empty Git repository in $TESTTMP/nogit/.git/
This status invocation shows some hg gunk because we didn't use
`hg init --git`, which fixes up .git/info/exclude for us.
$ hg status
? .hg/cache/git-commits.sqlite
? .hg/cache/git-commits.sqlite-shm
? .hg/cache/git-commits.sqlite-wal
? .hg/requires
$ cd ..
Now globally enable extension for the rest of the test:
$ echo "[extensions]" >> $HGRCPATH
> echo "git=" >> $HGRCPATH > echo "git=" >> $HGRCPATH
Make a new repo with git: Make a new repo with git:
$ mkdir foo $ mkdir foo
$ cd foo $ cd foo
$ git init $ git init
Initialized empty Git repository in $TESTTMP/foo/.git/ Initialized empty Git repository in $TESTTMP/foo/.git/
Ignore the .hg directory within git: Ignore the .hg directory within git: