Patchwork [1,of,2] convert: fix a test failure due to git change (issue3428)

login
register
mail settings
Submitter Augie Fackler
Date Feb. 8, 2013, 1:38 p.m.
Message ID <d6b5ff41e899ffcd73eb.1360330734@augie-macbookair>
Download mbox | patch
Permalink /patch/829/
State Accepted
Headers show

Comments

Augie Fackler - Feb. 8, 2013, 1:38 p.m.
# HG changeset patch
# User Ross Lagerwall <rosslagerwall@gmail.com>
# Date 1343838158 -7200
# Node ID d6b5ff41e899ffcd73ebbfaa9199cc2d235f918e
# Parent  8bd338c7c4c9831850e1c4ddf24b747222a44131
convert: fix a test failure due to git change (issue3428)

Since git v1.7.8.2-327-g926f1dd (the change was first released in
git 1.7.10), git does not return non-zero when
"git ls-remote --tags ..." is run and the repository is damaged.
This causes the "damaged repository with missing commit" test in
test-convert-git.t to unexpectedly succeed.

Fix by aborting if git outputs any lines beginning with "error:".

In addition, this error would show up only intermittently since the
test depended on the order of the directories returned by os.walk.

The damage repository test would delete the first object file it came
across. However, the order of the directory listing is arbitrary (it seems
to depend on the filesystem). This meant that sometimes a commit object
was deleted, sometimes a blob object and sometimes a tree object.

So, fix by hardcoding which object to delete. Delete a commit object, a
blob object and a tree object in three separate tests.
Augie Fackler - Feb. 8, 2013, 1:47 p.m.
I'm going to split this patch some and then push.

On Feb 8, 2013, at 1:38 PM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Ross Lagerwall <rosslagerwall@gmail.com>
> # Date 1343838158 -7200
> # Node ID d6b5ff41e899ffcd73ebbfaa9199cc2d235f918e
> # Parent  8bd338c7c4c9831850e1c4ddf24b747222a44131
> convert: fix a test failure due to git change (issue3428)
> 
> Since git v1.7.8.2-327-g926f1dd (the change was first released in
> git 1.7.10), git does not return non-zero when
> "git ls-remote --tags ..." is run and the repository is damaged.
> This causes the "damaged repository with missing commit" test in
> test-convert-git.t to unexpectedly succeed.
> 
> Fix by aborting if git outputs any lines beginning with "error:".
> 
> In addition, this error would show up only intermittently since the
> test depended on the order of the directories returned by os.walk.
> 
> The damage repository test would delete the first object file it came
> across. However, the order of the directory listing is arbitrary (it seems
> to depend on the filesystem). This meant that sometimes a commit object
> was deleted, sometimes a blob object and sometimes a tree object.
> 
> So, fix by hardcoding which object to delete. Delete a commit object, a
> blob object and a tree object in three separate tests.
> 
> diff --git a/hgext/convert/git.py b/hgext/convert/git.py
> --- a/hgext/convert/git.py
> +++ b/hgext/convert/git.py
> @@ -6,6 +6,7 @@
> # GNU General Public License version 2 or any later version.
> 
> import os
> +import subprocess
> from mercurial import util, config
> from mercurial.node import hex, nullid
> from mercurial.i18n import _
> @@ -29,13 +30,15 @@
>     # cannot remove environment variable. Just assume none have
>     # both issues.
>     if util.safehasattr(os, 'unsetenv'):
> -        def gitopen(self, s, noerr=False):
> +        def gitopen(self, s, err=None):
>             prevgitdir = os.environ.get('GIT_DIR')
>             os.environ['GIT_DIR'] = self.path
>             try:
> -                if noerr:
> +                if err == subprocess.PIPE:
>                     (stdin, stdout, stderr) = util.popen3(s)
>                     return stdout
> +                elif err == subprocess.STDOUT:
> +                    return self.popen_with_stderr(s)
>                 else:
>                     return util.popen(s, 'rb')
>             finally:
> @@ -44,13 +47,26 @@
>                 else:
>                     os.environ['GIT_DIR'] = prevgitdir
>     else:
> -        def gitopen(self, s, noerr=False):
> -            if noerr:
> +        def gitopen(self, s, err=None):
> +            if err == subprocess.PIPE:
>                 (sin, so, se) = util.popen3('GIT_DIR=%s %s' % (self.path, s))
>                 return so
> +            elif err == subprocess.STDOUT:
> +                    return self.popen_with_stderr(s)
>             else:
>                 return util.popen('GIT_DIR=%s %s' % (self.path, s), 'rb')
> 
> +    close_fds = os.name == 'posix'
> +    def popen_with_stderr(self, s):
> +        p = subprocess.Popen(s, shell=True, bufsize=-1,
> +                             close_fds=self.close_fds,
> +                             stdin=subprocess.PIPE,
> +                             stdout=subprocess.PIPE,
> +                             stderr=subprocess.STDOUT,
> +                             universal_newlines=False,
> +                             env=None)
> +        return p.stdout
> +
>     def gitread(self, s):
>         fh = self.gitopen(s)
>         data = fh.read()
> @@ -209,12 +225,15 @@
>     def gettags(self):
>         tags = {}
>         alltags = {}
> -        fh = self.gitopen('git ls-remote --tags "%s"' % self.path)
> +        fh = self.gitopen('git ls-remote --tags "%s"' % self.path,
> +                          err=subprocess.STDOUT)
>         prefix = 'refs/tags/'
> 
>         # Build complete list of tags, both annotated and bare ones
>         for line in fh:
>             line = line.strip()
> +            if line.startswith("error:"):
> +                raise util.Abort(_('cannot read tags from %s') % self.path)
>             node, tag = line.split(None, 1)
>             if not tag.startswith(prefix):
>                 continue
> @@ -266,7 +285,7 @@
>         # Origin heads
>         for reftype in gitcmd:
>             try:
> -                fh = self.gitopen(gitcmd[reftype], noerr=True)
> +                fh = self.gitopen(gitcmd[reftype], err=subprocess.PIPE)
>                 for line in fh:
>                     line = line.strip()
>                     rev, name = line.split(None, 1)
> diff --git a/tests/test-convert-git.t b/tests/test-convert-git.t
> --- a/tests/test-convert-git.t
> +++ b/tests/test-convert-git.t
> @@ -281,24 +281,6 @@
>   abort: --sourcesort is not supported by this data source
>   [255]
> 
> -damage git repository and convert again
> -
> -  $ cat > damage.py <<EOF
> -  > import os
> -  > import stat
> -  > for root, dirs, files in os.walk('git-repo4/.git/objects'):
> -  >     if files:
> -  >         path = os.path.join(root, files[0])
> -  >         if os.name == 'nt':
> -  >             os.chmod(path, stat.S_IWUSR)
> -  >         os.remove(path)
> -  >         break
> -  > EOF
> -  $ python damage.py
> -  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | \
> -  >     grep 'abort:' | sed 's/abort:.*/abort:/g'
> -  abort:
> -
> test sub modules
> 
>   $ mkdir git-repo5
> @@ -345,3 +327,32 @@
>   $ cd git-repo5
>   $ cat foo
>   sub
> +
> +  $ cd ../..
> +
> +damaged git repository tests:
> +In case the hard-coded hashes change, the following commands can be used to
> +list the hashes and their corresponding types in the repository:
> +cd git-repo4/.git/objects
> +find . -type f | cut -c 3- | sed 's_/__' | xargs -n 1 -t git cat-file -t
> +cd ../../..
> +
> +damage git repository by renaming a commit object
> +  $ COMMIT_OBJ=1c/0ce3c5886f83a1d78a7b517cdff5cf9ca17bdd
> +  $ mv git-repo4/.git/objects/$COMMIT_OBJ git-repo4/.git/objects/$COMMIT_OBJ.tmp
> +  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
> +  abort: cannot read tags from git-repo4/.git
> +  $ mv git-repo4/.git/objects/$COMMIT_OBJ.tmp git-repo4/.git/objects/$COMMIT_OBJ
> +damage git repository by renaming a blob object
> +
> +  $ BLOB_OBJ=8b/137891791fe96927ad78e64b0aad7bded08bdc
> +  $ mv git-repo4/.git/objects/$BLOB_OBJ git-repo4/.git/objects/$BLOB_OBJ.tmp
> +  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
> +  abort: cannot read 'blob' object at 8b137891791fe96927ad78e64b0aad7bded08bdc
> +  $ mv git-repo4/.git/objects/$BLOB_OBJ.tmp git-repo4/.git/objects/$BLOB_OBJ
> +damage git repository by renaming a tree object
> +
> +  $ TREE_OBJ=72/49f083d2a63a41cc737764a86981eb5f3e4635
> +  $ mv git-repo4/.git/objects/$TREE_OBJ git-repo4/.git/objects/$TREE_OBJ.tmp
> +  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
> +  abort: cannot read changes in 1c0ce3c5886f83a1d78a7b517cdff5cf9ca17bdd
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/convert/git.py b/hgext/convert/git.py
--- a/hgext/convert/git.py
+++ b/hgext/convert/git.py
@@ -6,6 +6,7 @@ 
 # GNU General Public License version 2 or any later version.
 
 import os
+import subprocess
 from mercurial import util, config
 from mercurial.node import hex, nullid
 from mercurial.i18n import _
@@ -29,13 +30,15 @@ 
     # cannot remove environment variable. Just assume none have
     # both issues.
     if util.safehasattr(os, 'unsetenv'):
-        def gitopen(self, s, noerr=False):
+        def gitopen(self, s, err=None):
             prevgitdir = os.environ.get('GIT_DIR')
             os.environ['GIT_DIR'] = self.path
             try:
-                if noerr:
+                if err == subprocess.PIPE:
                     (stdin, stdout, stderr) = util.popen3(s)
                     return stdout
+                elif err == subprocess.STDOUT:
+                    return self.popen_with_stderr(s)
                 else:
                     return util.popen(s, 'rb')
             finally:
@@ -44,13 +47,26 @@ 
                 else:
                     os.environ['GIT_DIR'] = prevgitdir
     else:
-        def gitopen(self, s, noerr=False):
-            if noerr:
+        def gitopen(self, s, err=None):
+            if err == subprocess.PIPE:
                 (sin, so, se) = util.popen3('GIT_DIR=%s %s' % (self.path, s))
                 return so
+            elif err == subprocess.STDOUT:
+                    return self.popen_with_stderr(s)
             else:
                 return util.popen('GIT_DIR=%s %s' % (self.path, s), 'rb')
 
+    close_fds = os.name == 'posix'
+    def popen_with_stderr(self, s):
+        p = subprocess.Popen(s, shell=True, bufsize=-1,
+                             close_fds=self.close_fds,
+                             stdin=subprocess.PIPE,
+                             stdout=subprocess.PIPE,
+                             stderr=subprocess.STDOUT,
+                             universal_newlines=False,
+                             env=None)
+        return p.stdout
+
     def gitread(self, s):
         fh = self.gitopen(s)
         data = fh.read()
@@ -209,12 +225,15 @@ 
     def gettags(self):
         tags = {}
         alltags = {}
-        fh = self.gitopen('git ls-remote --tags "%s"' % self.path)
+        fh = self.gitopen('git ls-remote --tags "%s"' % self.path,
+                          err=subprocess.STDOUT)
         prefix = 'refs/tags/'
 
         # Build complete list of tags, both annotated and bare ones
         for line in fh:
             line = line.strip()
+            if line.startswith("error:"):
+                raise util.Abort(_('cannot read tags from %s') % self.path)
             node, tag = line.split(None, 1)
             if not tag.startswith(prefix):
                 continue
@@ -266,7 +285,7 @@ 
         # Origin heads
         for reftype in gitcmd:
             try:
-                fh = self.gitopen(gitcmd[reftype], noerr=True)
+                fh = self.gitopen(gitcmd[reftype], err=subprocess.PIPE)
                 for line in fh:
                     line = line.strip()
                     rev, name = line.split(None, 1)
diff --git a/tests/test-convert-git.t b/tests/test-convert-git.t
--- a/tests/test-convert-git.t
+++ b/tests/test-convert-git.t
@@ -281,24 +281,6 @@ 
   abort: --sourcesort is not supported by this data source
   [255]
 
-damage git repository and convert again
-
-  $ cat > damage.py <<EOF
-  > import os
-  > import stat
-  > for root, dirs, files in os.walk('git-repo4/.git/objects'):
-  >     if files:
-  >         path = os.path.join(root, files[0])
-  >         if os.name == 'nt':
-  >             os.chmod(path, stat.S_IWUSR)
-  >         os.remove(path)
-  >         break
-  > EOF
-  $ python damage.py
-  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | \
-  >     grep 'abort:' | sed 's/abort:.*/abort:/g'
-  abort:
-
 test sub modules
 
   $ mkdir git-repo5
@@ -345,3 +327,32 @@ 
   $ cd git-repo5
   $ cat foo
   sub
+
+  $ cd ../..
+
+damaged git repository tests:
+In case the hard-coded hashes change, the following commands can be used to
+list the hashes and their corresponding types in the repository:
+cd git-repo4/.git/objects
+find . -type f | cut -c 3- | sed 's_/__' | xargs -n 1 -t git cat-file -t
+cd ../../..
+
+damage git repository by renaming a commit object
+  $ COMMIT_OBJ=1c/0ce3c5886f83a1d78a7b517cdff5cf9ca17bdd
+  $ mv git-repo4/.git/objects/$COMMIT_OBJ git-repo4/.git/objects/$COMMIT_OBJ.tmp
+  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
+  abort: cannot read tags from git-repo4/.git
+  $ mv git-repo4/.git/objects/$COMMIT_OBJ.tmp git-repo4/.git/objects/$COMMIT_OBJ
+damage git repository by renaming a blob object
+
+  $ BLOB_OBJ=8b/137891791fe96927ad78e64b0aad7bded08bdc
+  $ mv git-repo4/.git/objects/$BLOB_OBJ git-repo4/.git/objects/$BLOB_OBJ.tmp
+  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
+  abort: cannot read 'blob' object at 8b137891791fe96927ad78e64b0aad7bded08bdc
+  $ mv git-repo4/.git/objects/$BLOB_OBJ.tmp git-repo4/.git/objects/$BLOB_OBJ
+damage git repository by renaming a tree object
+
+  $ TREE_OBJ=72/49f083d2a63a41cc737764a86981eb5f3e4635
+  $ mv git-repo4/.git/objects/$TREE_OBJ git-repo4/.git/objects/$TREE_OBJ.tmp
+  $ hg convert git-repo4 git-repo4-broken-hg 2>&1 | grep 'abort:'
+  abort: cannot read changes in 1c0ce3c5886f83a1d78a7b517cdff5cf9ca17bdd