[PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Michael J Gruber]

bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)
replaced "/bin/sh" in exec.sh by the shell specified in SHELL_PATH, but
that breaks the subtest which checks for a specific checksum of a tree
containing.

Revert that change that was not explained in the commit message anyways
(exec.sh is never executed).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---

 t/t9100-git-svn-basic.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 5464b5b..936913c 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -30,7 +30,8 @@ test_expect_success \
 		echo "deep dir" >dir/a/b/c/d/e/file &&
 		mkdir bar &&
 		echo "zzz" >bar/zzz &&
-		write_script exec.sh </dev/null &&
+		echo "#!/bin/sh" >exec.sh &&
+		chmod +x exec.sh &&
 		svn_cmd import -m "import for git svn" . "$svnrepo" >/dev/null
 	) &&
 	rm -rf import &&
-- 
2.7.0.373.g083f1fe

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Jeff King]

On Sun, Feb 07, 2016 at 08:11:37PM +0100, Michael J Gruber wrote:

> bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)
> replaced "/bin/sh" in exec.sh by the shell specified in SHELL_PATH, but
> that breaks the subtest which checks for a specific checksum of a tree
> containing.
> 
> Revert that change that was not explained in the commit message anyways
> (exec.sh is never executed).

I think this just re-breaks things on Windows. That first setup test
used "chmod +x" (which is brought back by your patch), without having
the POSIXPERM prerequisite.

We probably do not want to mark the whole setup test as POSIXPERM, as
that would effectively break all of the other tests on Windows. The rest
of the tests need to be able to work whether or not the "chmod +x" was
run. It may be simpler to just break the executable-bit tests, including
setup, out to their own section of the script.

That being said, t9100 seems to pass for me, even at bcb11f1. Can you
show us the breakage you are seeing?

-Peff

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Michael J Gruber]

[warning: experimenting with forwarding to and replying from gmail...]
2016-02-08 14:50 GMT+01:00 Jeff King <peff@peff.net>:
> On Sun, Feb 07, 2016 at 08:11:37PM +0100, Michael J Gruber wrote:
>
>> bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)
>> replaced "/bin/sh" in exec.sh by the shell specified in SHELL_PATH, but
>> that breaks the subtest which checks for a specific checksum of a tree
>> containing.
>>
>> Revert that change that was not explained in the commit message anyways
>> (exec.sh is never executed).
>
> I think this just re-breaks things on Windows. That first setup test
> used "chmod +x" (which is brought back by your patch), without having
> the POSIXPERM prerequisite.
>
> We probably do not want to mark the whole setup test as POSIXPERM, as
> that would effectively break all of the other tests on Windows. The rest
> of the tests need to be able to work whether or not the "chmod +x" was
> run. It may be simpler to just break the executable-bit tests, including
> setup, out to their own section of the script.
>

The commit message does not explain that part of the patch at all - to
me it looks as if the direct "echo" and "chmod +x" is simply replaced
by calling a function which does just that, or more exactly, not quite:

> That being said, t9100 seems to pass for me, even at bcb11f1. Can you
> show us the breakage you are seeing?
>
> -Peff

SHELL_PATH=/bin/dash (in config.mak)

As I explained in my commit message, the problem arises when
SHELL_PATH is not "/bin/sh" and, consequently,
the generated "exec.sh" results in a blob with a different sha1.

Just try "/usr/bin/sh" for good measure...

Michael

[This time plain text, hopefully, and thus vger-palatable. How do I
make this default...]

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Jeff King]

On Mon, Feb 08, 2016 at 05:27:30PM +0100, Michael J Gruber wrote:

> > I think this just re-breaks things on Windows. That first setup test
> > used "chmod +x" (which is brought back by your patch), without having
> > the POSIXPERM prerequisite.
> >
> > We probably do not want to mark the whole setup test as POSIXPERM, as
> > that would effectively break all of the other tests on Windows. The rest
> > of the tests need to be able to work whether or not the "chmod +x" was
> > run. It may be simpler to just break the executable-bit tests, including
> > setup, out to their own section of the script.
> >
> The commit message does not explain that part of the patch at all - to me
> it looks as if the direct "echo" and "chmod +x" is simply replaced
> by calling a function which does just that, or more exactly, not quite:

Ah, right. I figured that systems that don't handle `chmod +x` would
omit it from write_script(). But it looks like we don't. I guess the
logic is that on Windows "chmod +x" doesn't _complain_, it's simply a
noop for adding the file to the index (because we unset core.filemode).

So in that sense, Windows is fine with that setup either way.

I wondered why it would not later fail the same sha1 check, since "git
add" would not respect the executable bit on such a system. But the
answer is that we do not "git add" the result; we import it using svn,
and then convert that to a git tree.

> > That being said, t9100 seems to pass for me, even at bcb11f1. Can you
> > show us the breakage you are seeing?
> 
> SHELL_PATH=/bin/dash (in config.mak)
> 
> As I explained in my commit message, the problem arises when SHELL_PATH is
> not "/bin/sh" and, consequently,
> the generated "exec.sh" results in a blob with a different sha1.

Oh, of course. I forgot that my SHELL_PATH is in fact /bin/sh. Sorry for
being thick.

Assuming your patch works on Windows (and from the logic above, I think
it should be the case?), then I think it's a good solution.

-Peff

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Johannes Schindelin]

Hi,

On Mon, 8 Feb 2016, Jeff King wrote:

> Assuming your patch works on Windows

If it re-introduces that chmod +x, it won't.

Please note that my *original* patch actually only guarded the chmod +x,
but Junio suggested switching to write_script and since it passed the test
suite here, I though it would be safe.

I still think write_script is the better alternative.

So why not just prefix it with `SHELL_PATH=/bin/sh`?

-- snipsnap --
t a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 56acc1e..6ad8537 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -30,7 +30,7 @@ test_expect_success \
 		echo "deep dir" >dir/a/b/c/d/e/file &&
 		mkdir bar &&
 		echo "zzz" >bar/zzz &&
-		write_script exec.sh </dev/null &&
+		SHELL_PATH=/bin/sh write_script exec.sh </dev/null &&
 		svn_cmd import -m "import for git svn" . "$svnrepo" >/dev/null
 	) &&
 	rm -rf import &&

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Jeff King]

On Mon, Feb 08, 2016 at 08:31:54PM +0100, Johannes Schindelin wrote:

> On Mon, 8 Feb 2016, Jeff King wrote:
> 
> > Assuming your patch works on Windows
> 
> If it re-introduces that chmod +x, it won't.
> 
> Please note that my *original* patch actually only guarded the chmod +x,
> but Junio suggested switching to write_script and since it passed the test
> suite here, I though it would be safe.
> 
> I still think write_script is the better alternative.

I'm confused why it matters. write_script() unconditionally calls "chmod
+x", doesn't it?

I just double-checked its definition in test-lib-function.sh; am I
missing some Windows-specific magic that kicks in?

> So why not just prefix it with `SHELL_PATH=/bin/sh`?

But then what is write_script buying us?

-Peff

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm confused why it matters. write_script() unconditionally calls "chmod
> +x", doesn't it?

Yeah, that was exactly my thought, too.  Sorry for not noticing that
this depended the "interpreter" exactly be /bin/sh, though (it is
not even executed).

> I just double-checked its definition in test-lib-function.sh; am I
> missing some Windows-specific magic that kicks in?
>
>> So why not just prefix it with `SHELL_PATH=/bin/sh`?
>
> But then what is write_script buying us?

The correct way to write a script for a specific interpreter is to
give a second parameter to write_script, i.e.

		write_script exec.sh /bin/sh </dev/null &&

and the answer to the question is "it will save us one line".

The version in 'master' that does

                echo "#!/bin/sh" >exec.sh &&
                chmod +x exec.sh &&

should be equivalent, so dropping that hunk from the patch is the
right resolution perhaps?

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Jeff King]

On Mon, Feb 08, 2016 at 11:43:19AM -0800, Junio C Hamano wrote:

> > But then what is write_script buying us?
> 
> The correct way to write a script for a specific interpreter is to
> give a second parameter to write_script, i.e.
> 
> 		write_script exec.sh /bin/sh </dev/null &&
> 
> and the answer to the question is "it will save us one line".

At the cost of a useless "cat" invocation, though. :)

> The version in 'master' that does
> 
>                 echo "#!/bin/sh" >exec.sh &&
>                 chmod +x exec.sh &&
> 
> should be equivalent, so dropping that hunk from the patch is the
> right resolution perhaps?

Yeah, but I still don't understand why the original did not work on
Windows, once all the other hunks from bcb11f1 are applied.

-Peff

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Johannes Schindelin]

Hi Peff (and other interested parties),

On Mon, 8 Feb 2016, Jeff King wrote:

> On Mon, Feb 08, 2016 at 11:43:19AM -0800, Junio C Hamano wrote:
> 
> > The version in 'master' that does
> > 
> >                 echo "#!/bin/sh" >exec.sh &&
> >                 chmod +x exec.sh &&
> > 
> > should be equivalent, so dropping that hunk from the patch is the
> > right resolution perhaps?
> 
> Yeah, but I still don't understand why the original did not work on
> Windows, once all the other hunks from bcb11f1 are applied.

And indeed it passes. With MSYS2. Because it simply ignores that chmod +x
cannot flip an executable bit.

The original patch (the one that guarded the chmod behind the MINGW
prereq) originated in the MSys (AKA MSys1) times, where 1) chmod would
fail, and 2) POSIXPERM did not yet exist.

Besides, I am pretty certain that there is a test in t9100 that *does*
test the executable bit, properly requiring POSIXPERM.

So I still would be in favor of using write_script: 1) our *intention* is
to write a script, even if we do not currently execute it, and 2) if
anybody is interested in supporting MSys1 (*cough* Hannes & Sebastian
*cough*), they have a *much* easier time to fix it.

Ciao,
Dscho

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Besides, I am pretty certain that there is a test in t9100 that *does*
> test the executable bit, properly requiring POSIXPERM.
>
> So I still would be in favor of using write_script: 1) our *intention* is
> to write a script, even if we do not currently execute it, and 2) if
> anybody is interested in supporting MSys1 (*cough* Hannes & Sebastian
> *cough*), they have a *much* easier time to fix it.

I do not think our intention is to write a script there, though.
You have commented out a different test, where a file called "file"
whose executable bit is flipped, and the result of the test depends
on the filesystem actually flipping the bit, using POSIXPERM.  I do
not see anything more special in the way "exec.sh" is used in the
test compared to the use of that "file".  They both are merely used
as a payload, with expected contents and expected setting of the
executable bit at each points in the test sequence.

By using POSIXPERM, js/mingw-tests topic cleanly identifies and
skips the tests that rely on the "chmod +x" acting as expected, but
by hiding the creation and setting of the executable bit behind
write_script for "exec.sh" (and for this file alone), the reader is
still forced to know the subtle linkage between write_script and
permission bits.  On a system where executable bit is not needed to
execute a script (perhaps the file extension tells if a file is
executable on such a system), it is a plausible enhancement to
write_script to make it not to even attempt "chmod +x" on a system
without executable bit--after all, the helper _is_ about writing a
script.  The executable bit is an implementation detail that nobody
cares about.

But the way "exec.sh" is used by that test is different.  It merely
wants to have a file with that name with a fixed contents that has
executable bit set.  As I already said, I didn't notice that during
the review and incorrectly suggested use of write_script, but that
was a mistake.

Ideally, it would have been better if the test was structured in
such a way that a set of pure-contents tests that do not involve the
executable bit and symbolic links are done first, with separate set
of tests that require POSIXPERM and/or SYMLINK.  Then we wouldn't be
having this conversation.  You would skip the whole thing in the
latter category.  As the test that originally was written needs
POSIXPERM (but curiously not SYMLINK) even in the very initial
'setup' stage, a hack to make the set-up step behave differently
depending on POSIXPERM is unavoidable and tolerable, if we do not
want to rewrite the entire script (and I do not think neither of us
want to see that).

But that needs to be spelled out explicitly to allow people follow
what is going on more easily, e.g.

    echo "#!/bin/sh" >exec.sh &&
    { test_have_prereq !POSIXPERM || chmod +x exec.sh } &&

The "file" test would not need such a construct as the whole thing
is skipped without POSIXPERM (and SYMLINK where it is aliased to
exec-2.sh).

While reviewing the change to this script, another thing I noticed
is this bit:

    name='remove executable bit from a file'
    test_expect_success POSIXPERM "$name" '
            rm -f "$GIT_DIR"/index &&
            git checkout -f -b mybranch5 ${remotes_git_svn} &&
            chmod -x exec.sh &&
            git update-index exec.sh &&
            git commit -m "$name" &&
            git svn set-tree --find-copies-harder --rmdir \
                    ${remotes_git_svn}..mybranch5 &&
            svn_cmd up "$SVN_TREE" &&
            test ! -x "$SVN_TREE"/exec.sh'

This uses "chmod -x", but it does not need to.  The only thing it
cares about is that the tree object that contains exec.sh record
that path with executable bit off, so "update-index --chmod" would
have allowed a !POSIXPERM system to record the same result.

There probably are more instance of similar "chmod" that you do not
have to skip with !POSIXPERM.

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Johannes Schindelin]

Hi Peff,

On Mon, 8 Feb 2016, Jeff King wrote:

> On Mon, Feb 08, 2016 at 08:31:54PM +0100, Johannes Schindelin wrote:
> 
> > On Mon, 8 Feb 2016, Jeff King wrote:
> > 
> > > Assuming your patch works on Windows
> > 
> > If it re-introduces that chmod +x, it won't.
> > 
> > Please note that my *original* patch actually only guarded the chmod +x,
> > but Junio suggested switching to write_script and since it passed the test
> > suite here, I though it would be safe.
> > 
> > I still think write_script is the better alternative.
> 
> I'm confused why it matters. write_script() unconditionally calls "chmod
> +x", doesn't it?

Hmpf, you're right. I'll check tomorrow what's going wrong.

> I just double-checked its definition in test-lib-function.sh; am I
> missing some Windows-specific magic that kicks in?

No Windows magic I know of. But actually, the patch could be simplified to

-- snip --
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 56acc1e..ee85cc7 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -30,7 +30,7 @@ test_expect_success \
 		echo "deep dir" >dir/a/b/c/d/e/file &&
 		mkdir bar &&
 		echo "zzz" >bar/zzz &&
-		write_script exec.sh </dev/null &&
+		write_script exec.sh /bin/sh </dev/null &&
 		svn_cmd import -m "import for git svn" . "$svnrepo" >/dev/null
 	) &&
 	rm -rf import &&
-- snap --

> > So why not just prefix it with `SHELL_PATH=/bin/sh`?
> 
> But then what is write_script buying us?

write_script is a semantically unambiguous way to specify what we *want*.
And it would allow us to handle chmod specifically for Windows *in one
place only*.

But as I said, I have to investigate what's going on.

Ciao,
Dscho

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> write_script is a semantically unambiguous way to specify what we *want*.
> And it would allow us to handle chmod specifically for Windows *in one
> place only*.

Correct.  write_script, for the intended target of the helper, is a
way to write a script that can later be invoked by the test with the
name "$1".  It is conceivable for write_script on UNIX to be writing
into "$1" while Windows version to be writing into "$1.bat" and the
script, i.e. the user of the write_script helper, to do this

	write_script foo <<EOF &&
        ...
        EOF
	...
        foo

which may result in foo.bat running on Windows without us having to
adjust the test script.  So it indeed is a very nice abstraction to
have.

But the way the test uses this exec.sh script is not consistent with
that.  exec.sh for this test is merely a data, whose content must
exactly match what later tests expect, i.e. it wants it to begin
with "#!/bin/sh" and its execute bit on, even though the test does
not have no intention to run it as a script.

So I think it was doubly wrong for me to suggest write_script
without realizing that this is _not_ writing a script in the usual
sense for us to write with write_script.



        	

Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh

[Johannes Schindelin]

Hi Junio,

On Mon, 8 Feb 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > write_script is a semantically unambiguous way to specify what we *want*.
> > And it would allow us to handle chmod specifically for Windows *in one
> > place only*.
> 
> Correct.  write_script, for the intended target of the helper, is a
> way to write a script that can later be invoked by the test with the
> name "$1".

And whose executable bit is set, contingent on the POSIXPERM prereq.

> It is conceivable for write_script on UNIX to be writing
> into "$1" while Windows version to be writing into "$1.bat"

Oy vey. Good thing you did not see my first reaction.

Shell scripts and batch scripts have *very* different semantics. Therefore
it would be a major nightmare (for me, not for you) to support writing
them *using the same write_script invocation*.

Let's just not go there.

> But the way the test uses this exec.sh script is not consistent with
> that.  exec.sh for this test is merely a data, whose content must
> exactly match what later tests expect, i.e. it wants it to begin with
> "#!/bin/sh" and its execute bit on, even though the test does not have
> no intention to run it as a script.

The important part, of course, is "and its execute bit on" which makes it
a moot point to ask whether we intend to execute the script or not. A
script is what we want, and a script is what we write. Therefore,
write_script is what we call. With the $2 fix-up to keep DrMicha happy.

Ciao,
Dscho