Re: t4151 missing quotes

[Jeff King]

On Tue, May 10, 2016 at 03:44:32PM +0200, Armin Kunaschik wrote:

> I'm building on a quite current AIX 6.1 where /bin/sh defaults to /bin/ksh
> which is a posix shell (ksh88).
> Using /bin/bash doesn't work because SHELL_PATH is only used in
> git scripts but not in any t* test scripts.

If you run "make test" (or just "make" inside "t/") the test scripts
will be executed with SHELL_PATH. If you run:

  ./t1234-whatever.sh

then obviously no, they will not be. Don't do that. Either use:

  make t1234-whatever.sh

or:

  $YOUR_SHELL_PATH t1234-whatever.sh

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Armin Kunaschik]

Sorry for any duplicate mails, the list blocked my html mail.
Note to self: Don't use GMail on a tablet.

On Mon, May 9, 2016 at 11:35 PM, Eric Sunshine  wrote:
>>
>> Hmph, do we have a broken &&-chain?
>
> I don't know. Unfortunately, Armin didn't provide much information in
> his initial email, saying only "skipping through some failed tests",
> which doesn't necessarily indicate if those tests failed or if he
> somehow manually skipped them.

In t4151 there was only a problem with this test. All other tests
inside t4151 were ok.
Skipping through the tests referred to all git tests, not just t4151.

>> If an earlier test fails and leaves an unmerged path, "ls-files -u"
>> would give some output, so "test -z" would get one or more non-empty
>> strings; if we feed multiple, this would fail.  But we would not have
>> even run "test -z" as long as we properly &&-chain these tests.
>>
>> I think the real issue is when the earlier step succeeds and does
>> not leave any unmerged path.  In that case, we would run "test -z"
>> without anything else on the command line, which would lead to an
>> syntax error.

Yes. While debugging the test, I saw a syntax error. I did not try to find out
why the test argument is empty. It seems not necessary.. the test logic
is still the same.

>> Side Note: /usr/bin/test and test (built into bash and dash)
>> seem not to care about the lack of string in "test -z "
>> and "test -n ".  It appears to me that they just take
>> "-z" and "-n" without "" as a special case of "test
>> " that is fed "-z" or "-n" as .  Apparently, the
>> platform Armin is working on doesn't.
>
> I also tested on Mac OS X and BSD, and they happily accept bare "test
> -n", as well (though, I don't doubt that there are old shells which
> complain).

I'm building on a quite current AIX 6.1 where /bin/sh defaults to /bin/ksh
which is a posix shell (ksh88).
Using /bin/bash doesn't work because SHELL_PATH is only used in
git scripts but not in any t* test scripts.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 4:45 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano  wrote:
>>> Something like this follows Documentation/SubmittingPatches [...]
>>>
>>> -- >8 --
>>> From: Armin Kunaschik 
>>> Subject: t4151: make sure argument to 'test -z' is given
>>>
>>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
>>> 2015-06-06), unlike all the other patches in the series, forgot to
>>> quote the output from "$(git ls-files -u)" when using it as the
>>> argument to "test -z", leading to a syntax error.
>>
>> To make it clear that this was not a syntax error in the typical case,
>> it might make sense to say:
>>
>> ...potentially leading to a syntax error if some earlier tests failed.
>
> Hmph, do we have a broken &&-chain?

I don't know. Unfortunately, Armin didn't provide much information in
his initial email, saying only "skipping through some failed tests",
which doesn't necessarily indicate if those tests failed or if he
somehow manually skipped them.

> If an earlier test fails and leaves an unmerged path, "ls-files -u"
> would give some output, so "test -z" would get one or more non-empty
> strings; if we feed multiple, this would fail.  But we would not have
> even run "test -z" as long as we properly &&-chain these tests.
>
> I think the real issue is when the earlier step succeeds and does
> not leave any unmerged path.  In that case, we would run "test -z"
> without anything else on the command line, which would lead to an
> syntax error.
>
> Side Note: /usr/bin/test and test (built into bash and dash)
> seem not to care about the lack of string in "test -z "
> and "test -n ".  It appears to me that they just take
> "-z" and "-n" without "" as a special case of "test
> " that is fed "-z" or "-n" as .  Apparently, the
> platform Armin is working on doesn't.

I also tested on Mac OS X and BSD, and they happily accept bare "test
-n", as well (though, I don't doubt that there are old shells which
complain).

> Perhaps
>
> ... leading to a syntax error on some platforms whose "test"
> does not interpret "test -z" (no other arguments) as testing if
> a string "-z" is the null string (which GNU test and test that
> is built into bash and dash seem to do).
>
> would be an improvement?

Yes, that sounds good.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, May 9, 2016 at 2:56 PM, Junio C Hamano  wrote:
>> Something like this follows Documentation/SubmittingPatches [...]
>>
>> -- >8 --
>> From: Armin Kunaschik 
>> Subject: t4151: make sure argument to 'test -z' is given
>>
>> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
>> 2015-06-06), unlike all the other patches in the series, forgot to
>> quote the output from "$(git ls-files -u)" when using it as the
>> argument to "test -z", leading to a syntax error.
>
> To make it clear that this was not a syntax error in the typical case,
> it might make sense to say:
>
> ...potentially leading to a syntax error if some earlier tests failed.

Hmph, do we have a broken &&-chain?

If an earlier test fails and leaves an unmerged path, "ls-files -u"
would give some output, so "test -z" would get one or more non-empty
strings; if we feed multiple, this would fail.  But we would not have
even run "test -z" as long as we properly &&-chain these tests.

I think the real issue is when the earlier step succeeds and does
not leave any unmerged path.  In that case, we would run "test -z"
without anything else on the command line, which would lead to an
syntax error.

Side Note: /usr/bin/test and test (built into bash and dash)
seem not to care about the lack of string in "test -z "
and "test -n ".  It appears to me that they just take
"-z" and "-n" without "" as a special case of "test
" that is fed "-z" or "-n" as .  Apparently, the
platform Armin is working on doesn't.

Perhaps

... leading to a syntax error on some platforms whose "test"
does not interpret "test -z" (no other arguments) as testing if
a string "-z" is the null string (which GNU test and test that
is built into bash and dash seem to do).

would be an improvement?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Stefan Beller]

On Mon, May 9, 2016 at 11:56 AM, Junio C Hamano  wrote:
> Something like this follows Documentation/SubmittingPatches, except
> that it further needs your Sign-off before mine, which I can forge
> if you say it is OK.

The sign-off is a simple line at the end of the explanation for
the patch, which certifies that you wrote it or otherwise have
the right to pass it on as a open-source patch.  The rules are
pretty simple: if you can certify the below:

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.


>
> Thanks for a report and an analysis of the issue.
>
> -- >8 --
> From: Armin Kunaschik 
> Subject: t4151: make sure argument to 'test -z' is given
>
> 88d50724 (am --skip: revert changes introduced by failed 3way merge,
> 2015-06-06), unlike all the other patches in the series, forgot to
> quote the output from "$(git ls-files -u)" when using it as the
> argument to "test -z", leading to a syntax error.
>
> Note that $(git ls-files -u | wc -l) are deliberately left unquoted,
> as some implementations of "wc -l" includes extra blank characters
> in its output and cannot be compared as string, i.e. "test 0 = $(...)".
>
> Signed-off-by:
> Signed-off-by: Junio C Hamano 
> ---
>  t/t4151-am-abort.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
> index 833e7b2..b878c21 100755
> --- a/t/t4151-am-abort.sh
> +++ b/t/t4151-am-abort.sh
> @@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> -   test -z $(git ls-files -u) &&
> +   test -z "$(git ls-files -u)" &&
> test_path_is_missing otherfile-4
>  '
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Junio C Hamano]

Something like this follows Documentation/SubmittingPatches, except
that it further needs your Sign-off before mine, which I can forge
if you say it is OK.

Thanks for a report and an analysis of the issue.

-- >8 --
From: Armin Kunaschik 
Subject: t4151: make sure argument to 'test -z' is given

88d50724 (am --skip: revert changes introduced by failed 3way merge,
2015-06-06), unlike all the other patches in the series, forgot to
quote the output from "$(git ls-files -u)" when using it as the
argument to "test -z", leading to a syntax error.

Note that $(git ls-files -u | wc -l) are deliberately left unquoted,
as some implementations of "wc -l" includes extra blank characters
in its output and cannot be compared as string, i.e. "test 0 = $(...)".

Signed-off-by: 
Signed-off-by: Junio C Hamano 
---
 t/t4151-am-abort.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
index 833e7b2..b878c21 100755
--- a/t/t4151-am-abort.sh
+++ b/t/t4151-am-abort.sh
@@ -82,7 +82,7 @@ test_expect_success 'am -3 --abort removes otherfile-4' '
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
-   test -z $(git ls-files -u) &&
+   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
 '
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Armin Kunaschik]

I'm currently concentrating on finding problems with my setup... this
is already a tough job :-)
I'm a git beginner, and Documentation/SubmittingPatches would keep me
busy for a week.
So anybody feel free to submit this thingy.

Armin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

[please don't top-post on this list]

On Mon, May 9, 2016 at 12:35 PM, Armin Kunaschik
 wrote:
> Sorry, this was my first patch to the list. I'll do better :-)
> You are right about the "wc -l" parts. Maybe I was a bit over
> pessimistic. Throw away my last mail.

Done :-)

> In my case test 9 ran unsuccessful because of an empty "git ls-files -u"

Okay, that makes perfect sense and does indeed deserve to be fixed.

> This reduces the diff to this one (hopefully the right way now):

Perhaps you can turn this into a proper patch acceptable for inclusion
in the project. If you're interested in attempting this, take a look
at Documentation/Submitting.

> *** ./t4151-am-abort.sh.origFri Apr 29 23:37:00 2016
> --- ./t4151-am-abort.sh Mon May  9 18:28:18 2016
> ***
> *** 82,88 
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> !   test -z $(git ls-files -u) &&
> test_path_is_missing otherfile-4
>   '
>
> --- 82,88 
> test 4 = "$(cat otherfile-4)" &&
> git am --abort &&
> test_cmp_rev initial HEAD &&
> !   test -z "$(git ls-files -u)" &&
> test_path_is_missing otherfile-4
>   '

This fix looks fine. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Armin Kunaschik]

Sorry, this was my first patch to the list. I'll do better :-)
You are right about the "wc -l" parts. Maybe I was a bit over
pessimistic. Throw away my last mail.
In my case test 9 ran unsuccessful because of an empty "git ls-files -u"

This reduces the diff to this one (hopefully the right way now):
*** ./t4151-am-abort.sh.origFri Apr 29 23:37:00 2016
--- ./t4151-am-abort.sh Mon May  9 18:28:18 2016
***
*** 82,88 
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z $(git ls-files -u) &&
test_path_is_missing otherfile-4
  '

--- 82,88 
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test_cmp_rev initial HEAD &&
!   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
  '

All the other similar occurrences are correctly quoted.

On Mon, May 9, 2016 at 6:22 PM, Eric Sunshine  wrote:
> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
>  wrote:
>> skipping through some failed tests I found more (smaller) problems
>> inside the test... when test arguments are empty they need to be
>> quoted (quite a lot test in this sentence).
>>
>> Error is like
>> t4151-am-abort.sh[5]: test: argument expected
>>
>> My patch:
>>
>> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
>> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
>> ***
>> *** 67,73 
>>   test_expect_success 'am -3 --skip removes otherfile-4' '
>> git reset --hard initial &&
>> test_must_fail git am -3 0003-*.patch &&
>> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
>> test 4 = "$(cat otherfile-4)" &&
>> git am --skip &&
>> test_cmp_rev initial HEAD &&
>> --- 67,73 
>>   test_expect_success 'am -3 --skip removes otherfile-4' '
>> git reset --hard initial &&
>> test_must_fail git am -3 0003-*.patch &&
>> !   test 3 -eq $(git ls-files -u | wc -l) &&
>> test 4 = "$(cat otherfile-4)" &&
>> git am --skip &&
>> test_cmp_rev initial HEAD &&
>> ***
>
> Some comments:
>
> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
> since the output contains leading whitespace which won't match the "3"
> on the other side of the '='.
>
> Your diff is backward, comparing 'current' against 'original', which
> makes it difficult to read. Reviewers on this list expect to see
> 'original' compared against 'current'.
>
> Use a unified format to make the diff easier to read; or just use
> git-diff or git-format patch, which is even simpler.
>
> It's not clear how the output of 'wc -l' could ever be the empty
> string. Perhaps git-ls-files is dying and causing the pipe to abort
> before 'wc -l' ever outputs anything? Without additional information
> about the problem you're experiencing, it's difficult to judge if this
> change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 12:22 PM, Eric Sunshine  wrote:
> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
>  wrote:
>> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
>> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
>> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
>> !   test 3 -eq $(git ls-files -u | wc -l) &&
>
> Some comments:
>
> Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
> since the output contains leading whitespace which won't match the "3"
> on the other side of the '='.

This isn't quite accurate. Since the test is using '-eq' rather than
'=', the leading whitespace won't be a problem, but it can still be
problematic if you're somehow getting an empty result from the
pipeline:

% test 3 -eq "$(echo)"
-bash: test: : integer expression expected

> Your diff is backward, comparing 'current' against 'original', which
> makes it difficult to read. Reviewers on this list expect to see
> 'original' compared against 'current'.
>
> Use a unified format to make the diff easier to read; or just use
> git-diff or git-format patch, which is even simpler.
>
> It's not clear how the output of 'wc -l' could ever be the empty
> string. Perhaps git-ls-files is dying and causing the pipe to abort
> before 'wc -l' ever outputs anything? Without additional information
> about the problem you're experiencing, it's difficult to judge if this
> change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: t4151 missing quotes

[Eric Sunshine]

On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
 wrote:
> skipping through some failed tests I found more (smaller) problems
> inside the test... when test arguments are empty they need to be
> quoted (quite a lot test in this sentence).
>
> Error is like
> t4151-am-abort.sh[5]: test: argument expected
>
> My patch:
>
> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
> --- t4151-am-abort.sh.orig  Fri Apr 29 23:37:00 2016
> ***
> *** 67,73 
>   test_expect_success 'am -3 --skip removes otherfile-4' '
> git reset --hard initial &&
> test_must_fail git am -3 0003-*.patch &&
> !   test 3 -eq "$(git ls-files -u | wc -l)" &&
> test 4 = "$(cat otherfile-4)" &&
> git am --skip &&
> test_cmp_rev initial HEAD &&
> --- 67,73 
>   test_expect_success 'am -3 --skip removes otherfile-4' '
> git reset --hard initial &&
> test_must_fail git am -3 0003-*.patch &&
> !   test 3 -eq $(git ls-files -u | wc -l) &&
> test 4 = "$(cat otherfile-4)" &&
> git am --skip &&
> test_cmp_rev initial HEAD &&
> ***

Some comments:

Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD
since the output contains leading whitespace which won't match the "3"
on the other side of the '='.

Your diff is backward, comparing 'current' against 'original', which
makes it difficult to read. Reviewers on this list expect to see
'original' compared against 'current'.

Use a unified format to make the diff easier to read; or just use
git-diff or git-format patch, which is even simpler.

It's not clear how the output of 'wc -l' could ever be the empty
string. Perhaps git-ls-files is dying and causing the pipe to abort
before 'wc -l' ever outputs anything? Without additional information
about the problem you're experiencing, it's difficult to judge if this
change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html