Patchwork D8265: git: key off `git` in .hg/requirements rather than separate file

login
register
mail settings
Submitter phabricator
Date March 7, 2020, 10:57 p.m.
Message ID <differential-rev-PHID-DREV-5oj5ug3nkoagy6w37vod-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45615/
State Superseded
Headers show

Comments

phabricator - March 7, 2020, 10:57 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

AFFECTED FILES
  hgext/git/__init__.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 8, 2020, 5:12 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

> __init__.py:104
> +            reqs = {l.strip() for l in f}
> +    if b'git' in reqs and os.path.exists(os.path.join(storebasepath, b'..', b'.git')):
>          return gitstore(storebasepath, vfstype)

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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 10, 2020, 4:35 p.m.
durin42 added inline comments.
durin42 marked an inline comment as done.

INLINE COMMENTS

> mharbison72 wrote in __init__.py:104
> 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?

Good catch! Yes, that makes sense.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 10, 2020, 4:40 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> __init__.py:89
>  def _makestore(orig, requirements, storebasepath, vfstype):
> -    # Check for presence of pygit2 only here. The assumption is that we'll
> -    # run this code iff we'll later need pygit2.
> -    if gitutil.get_pygit2() is None:
> -        raise error.Abort(
> -            _(
> -                b'the git extension requires the Python '
> -                b'pygit2 library to be installed'
> +    reqsf = os.path.join(storebasepath, b'requirements')
> +    reqs = set()

Why not use `.hg/requires`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: martinvonz, mharbison72, mercurial-devel
phabricator - March 10, 2020, 5:03 p.m.
pulkit added inline comments.

INLINE COMMENTS

> dirstate.py:79
>      def p1(self):
> -        return self.git.head.peel().id.raw
> +        try:
> +            return self.git.head.peel().id.raw

looks like unrelated change which should be in separate patch?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: pulkit, martinvonz, mharbison72, mercurial-devel
phabricator - March 10, 2020, 5:04 p.m.
durin42 added inline comments.

INLINE COMMENTS

> martinvonz wrote in __init__.py:89
> Why not use `.hg/requires`?

Because I have the dumb. Will fix shortly.

> pulkit wrote in dirstate.py:79
> looks like unrelated change which should be in separate patch?

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: pulkit, martinvonz, mharbison72, mercurial-devel
phabricator - March 10, 2020, 5:08 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> __init__.py:89-94
> +    reqsf = os.path.join(storebasepath, b'requires')
> +    reqs = set()
> +    if os.path.exists(reqsf):
> +        with open(reqsf, 'rb') as f:
> +            reqs = {l.strip() for l in f}
> +    if b'git' in reqs:

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

> test-git-interop.t:5-11
>    > 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_COMMITTER_NAME="$GIT_AUTHOR_NAME"; export GIT_COMMITTER_NAME
>    > GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL"; export GIT_COMMITTER_EMAIL
>    > GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"; export GIT_COMMITTER_DATE
> -
>    > count=10
>    > gitcommit() {

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: pulkit, martinvonz, mharbison72, mercurial-devel
phabricator - March 10, 2020, 5:14 p.m.
durin42 added inline comments.
durin42 marked 4 inline comments as done.

INLINE COMMENTS

> martinvonz wrote in test-git-interop.t:5-11
> `$` prefix on these too since you're cleaning up anyway?

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: pulkit, martinvonz, mharbison72, mercurial-devel
phabricator - March 10, 2020, 5:19 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> durin42 wrote in 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.

I'm fine with leaving it as it is.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8265/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8265

To: durin42, #hg-reviewers
Cc: pulkit, martinvonz, mharbison72, mercurial-devel

Patch

diff --git a/hgext/git/__init__.py b/hgext/git/__init__.py
--- a/hgext/git/__init__.py
+++ b/hgext/git/__init__.py
@@ -96,9 +96,12 @@ 
 
 
 def _makestore(orig, requirements, storebasepath, vfstype):
-    if os.path.exists(
-        os.path.join(storebasepath, b'this-is-git')
-    ) and os.path.exists(os.path.join(storebasepath, b'..', b'.git')):
+    reqsf = os.path.join(storebasepath, b'requirements')
+    reqs = set()
+    if os.path.exists(reqsf):
+        with open(reqsf, 'rb') as f:
+            reqs = {l.strip() for l in f}
+    if b'git' in reqs and os.path.exists(os.path.join(storebasepath, b'..', b'.git')):
         return gitstore(storebasepath, vfstype)
     return orig(requirements, storebasepath, vfstype)
 
@@ -128,8 +131,6 @@ 
             os.path.join(path, b'.git', b'info', b'exclude'), 'ab'
         ) as exclude:
             exclude.write(b'\n.hg\n')
-    with open(os.path.join(dothg, b'this-is-git'), 'wb') as f:
-        pass
     with open(os.path.join(dothg, b'requirements'), 'wb') as f:
         f.write(b'git\n')