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
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
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
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
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
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
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
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
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')