Re: [PATCH] t3513: do not compress backup tar file

Stefan Beller [off-list ref] writes:

Armin Kunaschik [off-list ref] wrote:

quoted

I'm trying to compile/test/use git 2.8.2 on AIX 6.1 with
no bash available.

...

quoted

make test does not make it through t3513-revert-submodule.sh anymore.
The test is not portable since it uses the z-flags of GNU-tar. When -z
is removed, (and extension is changed back to tar) everything runs and
tests smoothly.

Signed-off-by: Stefan Beller <redacted>
---

Thanks for a quick fix.  Even though "no bash" and "AIX 6.1" are
interesting details that are part of a good bug report, these are
irrelevant noise for a commit that fixes a bug that is unrelated to
them, so let's rephrase the message and queue it, like this:

    t3513: do not compress backup tar file

    The test uses the 'z' option, i.e. "compress the output while at
    it", which is GNUism and not portable.

    Reported-by: Armin Kunaschik [off-list ref]
    Signed-off-by: Stefan Beller [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

This is not a new problem, but these "ls -1pR" and "rm -rf *" makes
me wonder if it is the best way to test what is being tested.

The title says "revert can handle submodules", but when it sees that
revert finishes successfully, it discards the resulting working tree
state with "rm -rf *" (Yuck) and repopulates with the state before
the 'checkout && revert' sequence, so the 'status' and 'ls' are not
testing what 'revert' did at all.

Shouldn't it be testing HEAD^{tree} before "checkout && revert" and
after and make sure they match, and checking the working state left
by 'revert' without clobbering it with tarball extract?

On Fri, May 6, 2016 at 3:45 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

Armin Kunaschik [off-list ref] wrote:

quoted

I'm trying to compile/test/use git 2.8.2 on AIX 6.1 with
no bash available.

...

quoted

make test does not make it through t3513-revert-submodule.sh anymore.
The test is not portable since it uses the z-flags of GNU-tar. When -z
is removed, (and extension is changed back to tar) everything runs and
tests smoothly.

Signed-off-by: Stefan Beller <redacted>
---

Thanks for a quick fix.  Even though "no bash" and "AIX 6.1" are
interesting details that are part of a good bug report, these are
irrelevant noise for a commit that fixes a bug that is unrelated to
them, so let's rephrase the message and queue it, like this:

    t3513: do not compress backup tar file

    The test uses the 'z' option, i.e. "compress the output while at
    it", which is GNUism and not portable.

    Reported-by: Armin Kunaschik [off-list ref]
    Signed-off-by: Stefan Beller [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

Thanks for rewriting the message. :)

quoted

 t/t3513-revert-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index a1c4e02..db93781 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh

@@ -14,11 +14,11 @@ test_description='revert can handle submodules'
 git_revert () {
      git status -su >expect &&
      ls -1pR * >>expect &&
-     tar czf "$TRASH_DIRECTORY/tmp.tgz" * &&
+     tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
      git checkout "$1" &&
      git revert HEAD &&
      rm -rf * &&
-     tar xzf "$TRASH_DIRECTORY/tmp.tgz" &&
+     tar xf "$TRASH_DIRECTORY/tmp.tar" &&
      git status -su >actual &&
      ls -1pR * >>actual &&
      test_cmp expect actual &&

This is not a new problem, but these "ls -1pR" and "rm -rf *" makes
me wonder if it is the best way to test what is being tested.

The title says "revert can handle submodules", but when it sees that
revert finishes successfully, it discards the resulting working tree
state with "rm -rf *" (Yuck) and repopulates with the state before
the 'checkout && revert' sequence, so the 'status' and 'ls' are not
testing what 'revert' did at all.

Shouldn't it be testing HEAD^{tree} before "checkout && revert" and
after and make sure they match, and checking the working state left
by 'revert' without clobbering it with tarball extract?

Reading the comment above that function indicates that the middle revert
is not the interesting revert. The later revert is the actually tested revert.