[PATCH] t7610 test for mktemp existence

[Armin Kunaschik]

Subject: t7610: test for mktemp before test execution

mktemp is not available on all platforms, so the test
'temporary filenames are used with mergetool.writeToTemp'
fails there.
This patch does not replace mktemp but just disables
the test that otherwise would fail.
mergetool checks itself before executing mktemp and
reports an error.

Signed-off-by: Armin Kunaschik <megabr...@googlemail.com>

---

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 76306cf..9279bf5 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -589,7 +589,12 @@ test_expect_success 'filenames seen by tools start with 
./' '
git reset --hard master >/dev/null 2>&1
 '

-test_expect_success 'temporary filenames are used with mergetool.writeToTemp' '
+test_lazy_prereq MKTEMP '
+   tempdir=$(mktemp -d -t foo.XXX) &&
+   test -d "$tempdir"
+'
+
+test_expect_success MKTEMP 'temporary filenames are used with 
mergetool.writeToTemp' '
git checkout -b test16 branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&

--
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: t7610-mergetool.sh test failure

[Armin Kunaschik]

Another thread I'm trying to revive... the discussion went away quite a bit
from the suggested patch to conditionally run this one test only when mktemp
is available.

I'll create a patch when there are chances it is accepted.

I could think of a way to replace mktemp with a perl one-liner (or
small script)...
conditionally, when mktemp is not available... maybe in the build process?
As far as I can see, perl is absolutely necessary and can therefore be used to
"solve" the mktemp problem...

...or maybe I should stop bringing this up again :-)

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: [PATCH] t7800 readlink not found

[Armin Kunaschik]

On Tue, May 31, 2016 at 7:51 AM, Junio C Hamano  wrote:
> Torsten Bögershausen  writes:
>
>>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>>> index 7ce4cd7..905035c 100755
>>> --- a/t/t7800-difftool.sh
>>> +++ b/t/t7800-difftool.sh
>>> @@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
>>>   for f in file file2 sub/sub
>>>   do
>>>  echo "$f"
>>> -readlink "$2/$f"
>>> +ls -ld "$2/$f" | sed -e 's/.* -> //'
>>>   done >actual
>>>   EOF
>>>
>> I don't know how portable #ls -ld" really is.
>
> The parts with mode bits, nlinks, uid, gid, size, and date part do
> have some variations.  For example, we have been burned on ACL
> enabled systems having some funny suffix after the usual mode bits
> stuff.
>
> However, as far as this test is concerned, I do not think "how
> portable is the output from ls -ld" is an especially relevant
> question.  None of the things we expect early in the output (the
> fields I enumerated in the previous paragraph) would contain " -> ".
> And we know that we do not use a filename that has " -> " (or "->")
> as a substring in our tests.
>
> We don't have to use readlink, even on platforms where we do have
> readlink.  Building the conditional to be checked at runtime and
> providing a shell function read_link that uses "ls -ld | sed" or
> "readlink" depending on the runtime check is wasteful.
>

Just a short, curious question: Is this patch to be accepted/included some time?
I didn't see it in 2.8.4 nor 2.9.0. Maybe it just fell off the table...

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

[PATCH] t7800 readlink not found

[Armin Kunaschik]

On 05/27/2016 06:19 AM, David Aguilar wrote:
> On Wed, May 25, 2016 at 11:33:33AM +0200, Armin Kunaschik wrote:
> 
> Would you mind submitting a patch so that we can support these
> tests when running on AIX/HP-UX?

I don't feel comfortable to submit patches for tests I can't verify. I
don't have valgrind and python/p4 here. Looking to the code I'd say,
patching the p4 tests with "ls -ld | sed" looks quite save.
But I'm not sure about the test-lib.sh. When you are really super
paranoid, as written in the comment, you should probably use perl like

perl -e 'print readlink $ARGV[0]' $name

as a replacement.

So, as suggested by Junio, here the readlink workaround for t7800 only.
(hopefully whitespace clean this time)

--- 8< --- 8< ---
From: Armin Kunaschik <megabr...@googlemail.com>
Subject: t7800: readlink is not portable

The readlink(1) command is not available on all platforms (notably not
on AIX and HP-UX) and can be replaced in this test with the "workaround"

ls -ld  | sed -e 's/.* -> //'

This is no universal readlink replacement but works in the controlled
test environment good enough.

Signed-off-by: Armin Kunaschik <megabr...@googlemail.com>
---

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 7ce4cd7..905035c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -446,7 +446,7 @@ write_script .git/CHECK_SYMLINKS <<\EOF
 for f in file file2 sub/sub
 do
echo "$f"
-   readlink "$2/$f"
+   ls -ld "$2/$f" | sed -e 's/.* -> //'
 done >actual
 EOF

--
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: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 7:36 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Armin Kunaschik <megabr...@googlemail.com> writes:
>>
>> Ok, how can this be implemented within the test environment?
>
> I actually think an unconditional check like this is sufficient.

Ah, good. My thoughts were a bit more complicated.
Anyway, this works for me.
Thanks!

>  t/t7800-difftool.sh | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 7ce4cd7..f304228 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -442,15 +442,16 @@ run_dir_diff_test 'difftool --dir-diff with unmerged 
> files' '
> test_cmp expect actual
>  '
>
> -write_script .git/CHECK_SYMLINKS <<\EOF
> -for f in file file2 sub/sub
> -do
> -   echo "$f"
> -   readlink "$2/$f"
> -done >actual
> -EOF
> -
>  test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without 
> unstaged changes' '
> +
> +   write_script .git/CHECK_SYMLINKS <<-\EOF &&
> +   for f in file file2 sub/sub
> +   do
> +   echo "$f"
> +   ls -ld "$2/$f" | sed -e "s/.* -> //"
> +   done >actual
> +   EOF
> +
> cat >expect <<-EOF &&
> file
> $PWD/file
--
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: t7800 test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 6:57 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Matthieu Moy <matthieu@grenoble-inp.fr> writes:
>
>> Armin Kunaschik <megabr...@googlemail.com> writes:
>>
>>> t7800 fails on systems where readlink (GNUism?) is not available.
>>
>> I don't think it's POSIX, but it is present on all POSIX-like systems I
>> know. On which system did you get the issue?

It's not available in AIX or HP-UX.

>>> +readlink() { ls -ld "$1" | sed 's/.* -> //'; }
>>
>> This is much less robust than the actual readlink. For example, if ->
>> appears in the link name, it breaks.
>
> I wouldn't allow it in our scripted Porcelain, but the environment
> of our test scripts are under our control, so I do not think it is a
> problem ("ls piped to sed" has been an established idiom before
> readlink(1) was widely accepted, by the way).

I think so too. Maybe I can improve the sed expression a bit, but
it will never be a universal readlink replacement. But it doesn't have to.
It's defined locally for this one test only and it does the specific job.

>> It would be acceptable as a fall-back if readlink is not present, but
>> shouldn't activate the "ls" hack by default.
>
> Yup.

Ok, how can this be implemented within the test environment?
--
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: t7610-mergetool.sh test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 6:45 PM, Junio C Hamano  wrote:

> 3. find and install mktemp?
Sure, but which one? :-) mktemp is not mentioned in POSIX.
http://stackoverflow.com/questions/2792675/how-portable-is-mktemp1
--
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

t7610-mergetool.sh test failure

[Armin Kunaschik]

t7610-mergetool.sh fails on systems without mktemp.

mktemp is used in git-mergetool.sh and throws an error when it's not available.
error: mktemp is needed when 'mergetool.writeToTemp' is true

I see 2 options:
1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
2. disable the test when mktemp is not available

>From my point of view option 2 would be enough.
Any opinions about that?

Peff suggested something like this... works for me.
This patch is probably whitespace damaged... I could not yet figure out a way
to use and preserve tabs with Google mail.

---
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 76306cf..9279bf5 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -589,7 +589,12 @@ test_expect_success 'filenames seen by tools
start with ./' '
git reset --hard master >/dev/null 2>&1
 '

-test_expect_success 'temporary filenames are used with mergetool.writeToTemp' '
+test_lazy_prereq MKTEMP '
+   tempdir=$(mktemp -d -t foo.XXX) &&
+   test -d "$tempdir"
+'
+
+test_expect_success MKTEMP 'temporary filenames are used with
mergetool.writeToTemp' '
git checkout -b test16 branch1 &&
test_config mergetool.writeToTemp true &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
--
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

t7800 test failure

[Armin Kunaschik]

t7800 fails on systems where readlink (GNUism?) is not available.
An easy workaround for the very basic readlink usage of this test
would be to use a shell function like this:

---
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ff7a9e9..be3d19f 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -420,6 +420,7 @@ run_dir_diff_test 'difftool --dir-diff when
worktree file is missing' '
 '

 write_script .git/CHECK_SYMLINKS <<\EOF
+readlink() { ls -ld "$1" | sed 's/.* -> //'; }
 for f in file file2 sub/sub
 do
echo "$f"
--
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

t1400 test failures

[Armin Kunaschik]

Hello,

I have 2 failed tests here:
large transaction creating branches does not burst open file limit
large transaction deleting branches does not burst open file limit

both tests fail because of the decreased file limit:
git-2.8.3/t/../bin-wrappers/git: bad file unit number

The tests run fine on AIX with ksh88 when I increase the ulimit to 63 or higher.
bash and ksh93 test fine with the present ulimit of 32.

Changing the shell interpreter of the git-wrapper script to /bin/bash or
/bin/ksh93 is enough to make these 2 tests work with the lower ulimit.
All other scripts can be run with any shell.

I have no idea why the ksh88 consumes so much file descriptors, I also
don't have an idea if or how this test can be "fixed".

So I'll leave this here for information purposes :-)

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: t4204-patch-id failures

[Armin Kunaschik]

On Tue, May 24, 2016 at 12:23 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Junio C Hamano <gits...@pobox.com> writes:

> Having said all that, this illustrates the root cause of different
> behaviours better, but it is harder to reason about than simply
> changing the variable name used in this shell function.  POSIX reads
> a bit fuzzy to me around here:
>
> ... each command of a multi-command pipeline is in a subshell
> environment; as an extension, however, any or all commands in a
> pipeline may be executed in the current environment. All other
> commands shall be executed in the current shell environment.
>
> That essentially says nothing useful; it does not guarantee that
> each command on a pipeline runs in its own subshell environment, and
> a portable script must be prepared to see some of them run in the
> current shell environment.

Writing portable shell scripts sometimes is quite a mess :-)

> So let's do this instead:
>
> -- >8 --
> Subject: t4204: do not let $name variable clobbered
>
> test_patch_id_file_order shell function uses $name variable to hold
> one filename, and calls another shell function calc_patch_id as a
> downstream of one pipeline.  The called function, however, also uses
> the same $name variable.  With a shell implementation that runs the
> callee in the current shell environment, the caller's $name would
> be clobbered by the callee's use of the same variable.
>
> This hasn't been an issue with dash and bash.  ksh93 reveals the
> breakage in the test script.

Maybe add ksh88 too, just to be "feature" complete.

> Fix it by using a distinct variable name in the callee.

Agreed.

> Reported-by: Armin Kunaschik <megabr...@googlemail.com>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
--
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: [PATCH] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

On Fri, May 20, 2016 at 6:10 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
>>> index 89544dd..b425f3a 100755
>>> --- a/t/t0008-ignores.sh
>>> +++ b/t/t0008-ignores.sh
>>> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>>> a/b/.gitignore:8:!on*   a/b/one
>>> a/b/.gitignore:8:!on*   a/b/one one
>
> The patch was whitespace-damaged and didn't apply, so I had to redo
> it from scratch while updating the log message.  If this looks good
> to you, there is no need to resend.

Thanks. Looks like even Google Mail interface in plain text mode eats
white spaces :-(
--
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: [PATCH] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

On Fri, May 20, 2016 at 5:16 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Armin Kunaschik <megabr...@googlemail.com> writes:
>
>> From: Armin Kunaschik <megabr...@googlemail.com>
>>
>> \" in the test t0008 is not treated the same way in bash and in ksh.
>
> Could you refrain from singling out "bash"?  We don't write for
> "bash" specifically (and the test I ran are with "dash" before I
> push things out).
I can name it "other shells" if this is more comfortable. But I tested
this only with bash and ksh88 on AIX.

> Ideally, if you can try ksh93 and if you find out that ksh93 works,
> then the above can be made in line with your "Subject" to mark ksh88
> as broken (as opposed to other POSIX shells)?  That would help us by
> reminding that running test fine with ksh93 is not a sufficient
> check to make sure we didn't break ksh88 users.
>
>> In ksh the \ disappears and generates false expect data to
>> compare with.
>> Using \\" works portable, the same way in bash and in ksh and
>> is less ambigous.
>
> All of the above would need s/ksh/&88/g; I'd think.  I just tried
>
> make SHELL_PATH=/bin/ksh93
> cd t && /bin/ksh93 t0008-*.sh
>
> and this patch is not necessary for ksh93.
Yes, the patch is not necessary with ksh93 on AIX, but it works :-)
The patch is targeting "ksh" on AIX (which actually is a ksh88).

In the discussion Jeff took a look into the POSIX specification
and described the behavior like this:


I think either is reasonable (there is no need to backslash-escape a
double-quote inside a here-doc, but one assumes that backslash would
generally have its usual behavior). I'm not quite sure how to interpret
POSIX here (see below), but it seems clear that spelling it with two
backslashes as you suggest is the best bet.


I'd not declare ksh88 on AIX broken just because of this ambiguity
since it is not 100% clear in the POSIX description.

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

[PATCH] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

From: Armin Kunaschik <megabr...@googlemail.com>

\" in the test t0008 is not treated the same way in bash and in ksh.
In ksh the \ disappears and generates false expect data to
compare with.
Using \\" works portable, the same way in bash and in ksh and
is less ambigous.

Acked-by: Jeff King <p...@peff.net>
Signed-off-by: Armin Kunaschik <megabr...@googlemail.com>
---
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 89544dd..b425f3a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
a/b/.gitignore:8:!on*   a/b/one
a/b/.gitignore:8:!on*   a/b/one one
a/b/.gitignore:8:!on*   a/b/one two
-   a/b/.gitignore:8:!on*   "a/b/one\"three"
+   a/b/.gitignore:8:!on*   "a/b/one\\"three"
a/b/.gitignore:9:!two   a/b/two
a/.gitignore:1:two* a/b/twooo
$global_excludes:2:!globaltwo   globaltwo
@@ -686,7 +686,7 @@ cat <<-EOF >expected-all
a/b/.gitignore:8:!on*   b/one
a/b/.gitignore:8:!on*   b/one one
a/b/.gitignore:8:!on*   b/one two
-   a/b/.gitignore:8:!on*   "b/one\"three"
+   a/b/.gitignore:8:!on*   "b/one\\"three"
a/b/.gitignore:9:!two   b/two
::  b/not-ignored
a/.gitignore:1:two* b/twooo
--
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

t3404 static check of bad SHA-1 failure

[Armin Kunaschik]

Hello,

in t3404 test 91 - static check of bad SHA-1 fails (with ksh) with a
syntax error in git-rebase.
git-rebase[6]: test: argument expected

Reason is, again, a test -z without quotes in git-rebase--interactive:
*** ../git-rebase--interactive.orig Tue May 10 17:36:59 2016
--- ../git-rebase--interactive  Fri May 13 17:57:27 2016
***
*** 870,876 
badsha=1
else
sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
!   if test -z $sha1_verif
then
badsha=1
fi
--- 870,876 
badsha=1
else
sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
!   if test -z "$sha1_verif"
then
badsha=1
fi

Maybe it would be a good idea, to quote the test -z $1 in the same
function check_commit_sha too.
The test completes successfully without it, but since it's an user
option, I think it should be quoted.
Then there would be no more unquoted test -z anymore :-)

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

t0008 test fails with ksh

[Armin Kunaschik]

Hello,

in t0008 I see tests fails with
not ok 374 - --stdin -v
#
#   expect_from_stdin expected-all
.gitignore:1:one../one
::  ../not-ignored
.gitignore:1:oneone
::  not-ignored
a/b/.gitignore:8:!on*   b/on
a/b/.gitignore:8:!on*   b/one
a/b/.gitignore:8:!on*   b/one one
a/b/.gitignore:8:!on*   b/one two
a/b/.gitignore:8:!on*   "b/one\"three"
a/b/.gitignore:9:!two   b/two
::  b/not-ignored
a/.gitignore:1:two* b/twooo
$global_excludes:2:!globaltwo   ../globaltwo
$global_excludes:2:!globaltwo   globaltwo
$global_excludes:2:!globaltwo   b/globaltwo
$global_excludes:2:!globaltwo   ../b/globaltwo
::  c/not-ignored
EOF

behaves differently in bash and in ksh.
a/b/.gitignore:8:!on*   "b/one\"three" comes out unmodified
with bash but with ksh it becomes
a/b/.gitignore:8:!on*   "b/one"three"
I'm not sure what shell is "wrong" here, but when I modify the line to
a/b/.gitignore:8:!on*   "b/one\\"three"
both shells generate the "right" output:
a/b/.gitignore:8:!on*   "b/one\"three"

>From what I've learned I'd expect the double backslash to be the
"right" way to escape one
backslash. Do you agree or am I wrong?

This snippet appears twice in this test, generates expected-all and
expected-verbose.

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

syntax error in git-rebase while running t34* tests

[Armin Kunaschik]

Hello,

I noticed in my environment (AIX, ksh) that all rebase tests don't work.
git-rebase terminates with
git-rebase[6]: Syntax error at line 3 : `newline or ;' is not expected.
Digging deeper I found the problem in git-rebase--interactive line 85


strategy_args=${strategy:+--strategy=$strategy}
eval '
for strategy_opt in '"$strategy_opts"'
do
strategy_args="$strategy_args -X$(git rev-parse
--sq-quote "${strategy_opt#--}")"
done
'


This snippet fails when $strategy_opts is empty or undefined... and my
eval seems not to like this.
The for loop runs fine, even with empty strategy_opts, but not inside eval.
I fail to see why eval is really necessary here. Things can probably
be done without eval, but I
don't feel to see the big picture around this code.

My quick and dirty hotfix is to place a
test -n "$strategy_opts" &&
in front of the eval.
The tests run fine after this change.

What do you think?

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

[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

[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: [PATCH] t6041: do not compress backup tar file

[Armin Kunaschik]

Hi Stefan,

I'm currently in the process of skipping through the failed tests on my AIX box.
There are more tests which require GNU tools like mktemp
(t7610-mergetool.sh) or readlink (t7800-difftool.sh).
But I don't have a solution or workaround for these two.
But at least there are not more failing compressed tar tests :-)

Thanks,
Armin

On Mon, May 9, 2016 at 7:09 PM, Stefan Beller <sbel...@google.com> wrote:
> The test uses the 'z' option, i.e. "compress the output while at
> it", which is GNUism and not portable.
>
> Reported-by: Armin Kunaschik <megabr...@googlemail.com>
> Signed-off-by: Stefan Beller <sbel...@google.com>
> ---
>
>  Thanks Armin for reporting these GNUism!
>  Are there any more? (So we can do these patches as a
>  series instead of one by one:)
>
>  developed on top of origin/sb/z-is-gnutar-ism
>
>  Thanks,
>  Stefan
--
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

t5601-clone.sh failures

[Armin Kunaschik]

Hi list,

eight t5601 tests don't run with my version of sed.
The reason is a trailing space in the sed expression. See below:

#IPv6
for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]:
[user@::1] [user@::1]:
do
ehost=$(echo $tuah | sed -e "s/1]:/1]/ "| tr -d "[]")
test_expect_success "clone ssh://$tuah/home/user/repo" "
  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
"
done

I can not imagine a reason why there should be a space character...
The tests run fine here without the space.
When there isn't any reason, the trailing space should be removed.

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

t6041-bisect-submodule.sh tar -z not portable

[Armin Kunaschik]

Hi,

similar to t3513, in t6041 tar is used with the -z flag which is not portable
and should be removed the same way as in t3513.

Regards,
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

[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 <sunsh...@sunshineco.com> wrote:
> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
> <megabr...@googlemail.com> 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

t4151 missing quotes

[Armin Kunaschik]

Hi there,

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 &&
***
*** 78,88 
  test_expect_success 'am -3 --abort 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 --abort &&
test_cmp_rev initial HEAD &&
!   test -z "$(git ls-files -u)" &&
test_path_is_missing otherfile-4
  '

--- 78,88 
  test_expect_success 'am -3 --abort 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 --abort &&
test_cmp_rev initial HEAD &&
!   test -z $(git ls-files -u) &&
test_path_is_missing otherfile-4
  '

***
*** 146,152 
git reset &&
rm -f otherfile-4 otherfile-2 file-1 file-2 &&
test_must_fail git am -3 initial.patch 0003-*.patch &&
!   test 3 -eq "$(git ls-files -u | wc -l)" &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test -z "$(git ls-files -u)" &&
--- 146,152 
git reset &&
rm -f otherfile-4 otherfile-2 file-1 file-2 &&
test_must_fail git am -3 initial.patch 0003-*.patch &&
!   test 3 -eq $(git ls-files -u | wc -l) &&
test 4 = "$(cat otherfile-4)" &&
git am --abort &&
test -z "$(git ls-files -u)" &&

Regards,
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: Portability of git shell scripts?

[Armin Kunaschik]

On Wed, May 4, 2016 at 11:20 PM, Jeff King <p...@peff.net> wrote:
> On Wed, May 04, 2016 at 08:17:38PM +0200, Armin Kunaschik wrote:
>
>> I'm trying to compile/test/use git 2.8.2 on AIX 6.1 with no bash available.
>> /bin/sh is a hard link to /bin/ksh which is a ksh88, a posix shell.
>> Is this supposed to work?
>
> We aim for a practical subset of Bourne shells, including bash, dash,
> ash, ksh, etc. There's at least one Bourne-ish shell known not to work,
> which is Solaris /bin/sh[1]. POSIX is usually a good guide, but we aim
> for practical portability more than adhering strictly to the standards
> document.
>
> I've tested with mksh in the past (though it's possible that we've
> introduced a regression since then). But I think we've run into problems
> with ksh93[2]. I don't know about ksh88, or what construct it doesn't
> like.  It may or may not be easy to work around.

In general ksh (88 or 93) is posix compliant... and bash is moving away
from posix. :-) But I know what you mean.

>> As an example: make test fails on nearly every t34* test and on tests
>> which contain rebase.
>> The installation of bash (and manually changing the shebang to
>> /bin/bash) "fixes" all rebase test failures. So obviously git-rebase
>> is not portable at some point.
>
> Right. Any modern-ish Bourne shell will do, so moving to bash is one way
> to fix it.

My last compile of git 2.2.2 did far better than the current 2.8.2. So
it looks like
there were more recent changes that broke portability.

>> Does it make any sense to put work into making these scripts portable,
>> that is, work with posix shells?
>
> Maybe. :) If you can find what it is that ksh88 is unhappy with, we can
> see how painful it is to adapt to. But given my looking into ksh93 in
> [2], I suspect it will be easier to just use a more modern shell.

Regarding [2] this was a bug which was fixed quite fast. To me this is no
real showstopper. Modernity of ksh93 depends on the letter after the 93 :-)

>> And, as last resort, is it possible to configure git use bash in some
>> or all shell scripts?
>
> You can set SHELL_PATH in your config.mak file.

I tried a build with SHELL_PATH=/bin/bash. Many problems "went away".
Others appeared. I'll give it a few more days to look into it.

First finding:
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.

Is this report enough to start the magic to change things?

Regards,
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

Portability of git shell scripts?

[Armin Kunaschik]

Hi list,

I'm trying to compile/test/use git 2.8.2 on AIX 6.1 with no bash available.
/bin/sh is a hard link to /bin/ksh which is a ksh88, a posix shell.
Is this supposed to work?

As an example: make test fails on nearly every t34* test and on tests
which contain rebase.
The installation of bash (and manually changing the shebang to
/bin/bash) "fixes" all rebase test failures. So obviously git-rebase
is not portable at some point.

Does it make any sense to put work into making these scripts portable,
that is, work with posix shells?
And, as last resort, is it possible to configure git use bash in some
or all shell scripts?

Regards,
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