[PATCH 0/6] fix test failure with busybox

[PATCH 0/6] fix test failure with busybox

[Đoàn Trần Công Danh]

Despite some non-compiance from busybox's sh(1), grep(1), diff(1),
Alpine Linux is still a popular choice for container these days.

Fix false-positive failure in testsuite when run in Alpine Linux.

t5703.{4,5,6,7} is still failing.
Despite git pack-objects behaves correctly,
upload-pack.c works incorrectly on this platform.

I haven't dig deeper, yet.

Đoàn Trần Công Danh (6):
  t4061: use POSIX compliance regex(7)
  test-lib-functions: test_cmp: eval $GIT_TEST_CMP
  t5003: skip conversion test if unzip -a is unavailable
  t5616: use rev-parse instead to get HEAD's object_id
  t7063: use POSIX find(1) syntax
  t4124: fix test for non-compliance diff

 t/t4061-diff-indent.sh            |  2 +-
 t/t4124-apply-ws-rule.sh          |  6 ++++++
 t/t5003-archive-zip.sh            | 20 ++++++++++++++------
 t/t5616-partial-clone.sh          |  2 +-
 t/t7063-status-untracked-cache.sh |  2 +-
 t/test-lib-functions.sh           |  2 +-
 6 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.26.0.rc2.234.g969ad452ca

[PATCH 1/6] t4061: use POSIX compliance regex(7)

[Đoàn Trần Công Danh]

BRE interprets `+` literally, and
`\+` is undefined for POSIX BRE, from:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_02

> The interpretation of an ordinary character preceded
> by an unescaped <backslash> ( '\\' ) is undefined, except for:
> - The characters ')', '(', '{', and '}'
> - The digits 1 to 9 inclusive
> - A character inside a bracket expression

This test is failing with busybox sed, the default sed of Alpine Linux

Fix it by using literal `+` instead.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t4061-diff-indent.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 2affd7a100..0f7a6d97a8 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -17,7 +17,7 @@ compare_diff () {
 # Compare blame output using the expectation for a diff as reference.
 # Only look for the lines coming from non-boundary commits.
 compare_blame () {
-	sed -n -e "1,4d" -e "s/^\+//p" <"$1" >.tmp-1
+	sed -n -e "1,4d" -e "s/^+//p" <"$1" >.tmp-1
 	sed -ne "s/^[^^][^)]*) *//p" <"$2" >.tmp-2
 	test_cmp .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
 }
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:

> BRE interprets `+` literally, and
> `\+` is undefined for POSIX BRE, from:
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_02
> 
> > The interpretation of an ordinary character preceded
> > by an unescaped <backslash> ( '\\' ) is undefined, except for:
> > - The characters ')', '(', '{', and '}'
> > - The digits 1 to 9 inclusive
> > - A character inside a bracket expression
> 
> This test is failing with busybox sed, the default sed of Alpine Linux
> 
> Fix it by using literal `+` instead.

This makes sense, I think. It could hurt a sed which is expected ERE and
needs the "+" escaped, but I think such a sed would be wrong (and I
imagine would break things elsewhere).

-Peff

Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)

[Eric Sunshine]

On Thu, Mar 19, 2020 at 11:53 AM Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:
> > Fix it by using literal `+` instead.
>
> This makes sense, I think. It could hurt a sed which is expected ERE and
> needs the "+" escaped, but I think such a sed would be wrong (and I
> imagine would break things elsewhere).

I had the same thought and considered suggesting a character class:

    sed -n -e "1,4d" -e "s/^[+]//p" <"$1" >.tmp-1

to make it painfully obvious that "+" is not special in the
expression. But then I thought better of it -- for the same reason as
you (to wit: such a 'sed' would be wrong) -- and decided against
saying anything.

Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Mar 19, 2020 at 11:53 AM Jeff King <peff@peff.net> wrote:
>> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:
>> > Fix it by using literal `+` instead.
>>
>> This makes sense, I think. It could hurt a sed which is expected ERE and
>> needs the "+" escaped, but I think such a sed would be wrong (and I
>> imagine would break things elsewhere).
>
> I had the same thought and considered suggesting a character class:
>
>     sed -n -e "1,4d" -e "s/^[+]//p" <"$1" >.tmp-1
>
> to make it painfully obvious that "+" is not special in the
> expression. But then I thought better of it -- for the same reason as
> you (to wit: such a 'sed' would be wrong) -- and decided against
> saying anything.

I have only one thing that needs fixing, which is s/compliance/compliant/;
on the title.  Other than that, it looks good.

Having said that, I would have done the [+] thing if I were doing
this patch myself.  As long as we see no "wrong" sed that is broken
by this change, I am OK with it, though.

Thanks.

Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)

[Danh Doan]

On 2020-03-19 15:02:37-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Thu, Mar 19, 2020 at 11:53 AM Jeff King <peff@peff.net> wrote:
> >> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:
> >> > Fix it by using literal `+` instead.
> >>
> >> This makes sense, I think. It could hurt a sed which is expected ERE and
> >> needs the "+" escaped, but I think such a sed would be wrong (and I
> >> imagine would break things elsewhere).
> >
> > I had the same thought and considered suggesting a character class:
> >
> >     sed -n -e "1,4d" -e "s/^[+]//p" <"$1" >.tmp-1
> >
> > to make it painfully obvious that "+" is not special in the
> > expression. But then I thought better of it -- for the same reason as
> > you (to wit: such a 'sed' would be wrong) -- and decided against
> > saying anything.
> 
> I have only one thing that needs fixing, which is s/compliance/compliant/;
> on the title.  Other than that, it looks good.
> 
> Having said that, I would have done the [+] thing if I were doing
> this patch myself.  As long as we see no "wrong" sed that is broken
> by this change, I am OK with it, though.

Well, `[+]` thing was my first version for this change,
but I change in to this version afterward.

However, your comment in a later patch:

> IOW, I do not have trouble changing the test so that it works with
> noncompliant "diff".  But then in the same series, I would prefer to
> see the existing test keeps working with a possibly noncompliant
> "sed" implementation that has been working well with the tests.

changed my mind.

I will use `[+]` here in the reroll.

-- 
Danh

[PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP

[Đoàn Trần Công Danh]

Shell recognises first non-assignment token as command name.
Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:

> test_cmp:1: command not found: diff -u

Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 352c213d52..ab0e47ae17 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -905,7 +905,7 @@ test_expect_code () {
 # - not all diff versions understand "-u"
 
 test_cmp() {
-	$GIT_TEST_CMP "$@"
+	eval "$GIT_TEST_CMP" '"$@"'
 }
 
 # Check that the given config key has the expected value.
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:

> Shell recognises first non-assignment token as command name.
> Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:
> 
> > test_cmp:1: command not found: diff -u
> 
> Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.

Adding an "eval" here will subtly change the behavior for other shells.
Right now they'd just do whitespace splitting (which presumably busybox
is not), but with this we'd expand metachars, etc.

I suspect that's fine (and maybe even preferable, because if you really
do have a space in the path you can actually escape it). So I don't mind
this change.

I do worry that this whitespace splitting behavior could bite us in
other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
have any problem with it, though.

-Peff

Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP

[Eric Sunshine]

On Thu, Mar 19, 2020 at 12:02 PM Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:
> > > test_cmp:1: command not found: diff -u
> >
> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.
>
> I do worry that this whitespace splitting behavior could bite us in
> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
> have any problem with it, though.

I had the same reaction upon reading the patch. It's not just other
scripts in which the problem might manifest, but a change to the value
of some variable which is used as a command invocation. Providing a
"fix" for this one particular case may help get past test a failure on
Alpine under busybox, but it is not a good general solution. (Some
sort of helper function which smooths away differences like this --
say git_indirect_cmd() or something -- which can be used wherever a
$VARIABLE is invoked as a command might be a better approach, but I
haven't really thought it through.)

Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP

[Danh Doan]

On 2020-03-19 12:02:11-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Shell recognises first non-assignment token as command name.
> > Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:
> > 
> > > test_cmp:1: command not found: diff -u
> > 
> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.
> 
> Adding an "eval" here will subtly change the behavior for other shells.
> Right now they'd just do whitespace splitting (which presumably busybox
> is not), but with this we'd expand metachars, etc.
> 
> I suspect that's fine (and maybe even preferable, because if you really
> do have a space in the path you can actually escape it). So I don't mind
> this change.
> 
> I do worry that this whitespace splitting behavior could bite us in
> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to
> have any problem with it, though.

Ah, this patch wasn't meant for busybox.
It run into failure with both bash and dash in VoidLinux, and bash in
ArchLinux, I couldn't start p0000 in Ubuntu docker image due to
missing /usr/bin/time (even if I have /usr/bin/time installed).

-- 
Danh

[PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable

[Đoàn Trần Công Danh]

Alpine Linux's default unzip(1) doesn't support `-a`.

Skip those tests on that platform.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t5003-archive-zip.sh | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 106eddbd85..78fb4bf323 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -15,6 +15,14 @@ test_lazy_prereq UNZIP_SYMLINKS '
 	)
 '
 
+test_lazy_prereq UNZIP_CONVERT '
+	(
+		mkdir unzip-convert &&
+		cd unzip-convert &&
+		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
+	)
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst
@@ -39,33 +47,33 @@ check_zip() {
 	extracted=${dir_with_prefix}a
 	original=a
 
-	test_expect_success UNZIP " extract ZIP archive with EOL conversion" '
+	test_expect_success UNZIP_CONVERT " extract ZIP archive with EOL conversion" '
 		(mkdir $dir && cd $dir && "$GIT_UNZIP" -a ../$zipfile)
 	'
 
-	test_expect_success UNZIP " validate that text files are converted" "
+	test_expect_success UNZIP_CONVERT " validate that text files are converted" "
 		test_cmp_bin $extracted/text.cr $extracted/text.crlf &&
 		test_cmp_bin $extracted/text.cr $extracted/text.lf
 	"
 
-	test_expect_success UNZIP " validate that binary files are unchanged" "
+	test_expect_success UNZIP_CONVERT " validate that binary files are unchanged" "
 		test_cmp_bin $original/binary.cr   $extracted/binary.cr &&
 		test_cmp_bin $original/binary.crlf $extracted/binary.crlf &&
 		test_cmp_bin $original/binary.lf   $extracted/binary.lf
 	"
 
-	test_expect_success UNZIP " validate that diff files are converted" "
+	test_expect_success UNZIP_CONVERT " validate that diff files are converted" "
 		test_cmp_bin $extracted/diff.cr $extracted/diff.crlf &&
 		test_cmp_bin $extracted/diff.cr $extracted/diff.lf
 	"
 
-	test_expect_success UNZIP " validate that -diff files are unchanged" "
+	test_expect_success UNZIP_CONVERT " validate that -diff files are unchanged" "
 		test_cmp_bin $original/nodiff.cr   $extracted/nodiff.cr &&
 		test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
 		test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf
 	"
 
-	test_expect_success UNZIP " validate that custom diff is unchanged " "
+	test_expect_success UNZIP_CONVERT " validate that custom diff is unchanged " "
 		test_cmp_bin $original/custom.cr   $extracted/custom.cr &&
 		test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
 		test_cmp_bin $original/custom.lf   $extracted/custom.lf
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:04PM +0700, Đoàn Trần Công Danh wrote:

> Alpine Linux's default unzip(1) doesn't support `-a`.
> 
> Skip those tests on that platform.

Makes sense. One minor nit:

> +test_lazy_prereq UNZIP_CONVERT '
> +	(
> +		mkdir unzip-convert &&
> +		cd unzip-convert &&
> +		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
> +	)
> +'

Lazy prereqs are already evaluated in a throw-away directory, so you can
drop the subshell and mkdir/cd.

-Peff

Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable

[Danh Doan]

On 2020-03-19 12:03:10-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:04PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Alpine Linux's default unzip(1) doesn't support `-a`.
> > 
> > Skip those tests on that platform.
> 
> Makes sense. One minor nit:
> 
> > +test_lazy_prereq UNZIP_CONVERT '
> > +	(
> > +		mkdir unzip-convert &&
> > +		cd unzip-convert &&
> > +		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
> > +	)
> > +'
> 
> Lazy prereqs are already evaluated in a throw-away directory, so you can
> drop the subshell and mkdir/cd.

I was trying to keep it consistent with UNZIP_SYMLINKS above it.
Maybe, it's worth to clean it, too.

-- 
Danh

Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable

[Jeff King]

On Fri, Mar 20, 2020 at 07:39:21AM +0700, Danh Doan wrote:

> > > +test_lazy_prereq UNZIP_CONVERT '
> > > +	(
> > > +		mkdir unzip-convert &&
> > > +		cd unzip-convert &&
> > > +		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
> > > +	)
> > > +'
> > 
> > Lazy prereqs are already evaluated in a throw-away directory, so you can
> > drop the subshell and mkdir/cd.
> 
> I was trying to keep it consistent with UNZIP_SYMLINKS above it.
> Maybe, it's worth to clean it, too.

Ah, sorry, I should have looked at more context. I think it would be
worth cleaning up the existing one.

-Peff

[PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id

[Đoàn Trần Công Danh]

Only HEAD's object_id is necessary, rev-list is an overkill.

Despite POSIX requires grep(1) treat single pattern with <newline>
as multiple patterns.
busybox's grep(1) (as of v1.31.1) haven't implemented it yet.

Use rev-parse to simplify the test and avoid busybox unimplemented
features.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t5616-partial-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 77bb91e976..135187c5b5 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '
 test_expect_success 'verify that .promisor file contains refs fetched' '
 	ls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&
 	test_line_count = 1 promisorlist &&
-	git -C srv.bare rev-list HEAD >headhash &&
+	git -C srv.bare rev-parse HEAD >headhash &&
 	grep "$(cat headhash) HEAD" $(cat promisorlist) &&
 	grep "$(cat headhash) refs/heads/master" $(cat promisorlist)
 '
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:05PM +0700, Đoàn Trần Công Danh wrote:

> Only HEAD's object_id is necessary, rev-list is an overkill.
> 
> Despite POSIX requires grep(1) treat single pattern with <newline>
> as multiple patterns.
> busybox's grep(1) (as of v1.31.1) haven't implemented it yet.
> 
> Use rev-parse to simplify the test and avoid busybox unimplemented
> features.

That makes sense. It would also future-proof us against the test
changing the graph such that HEAD actually has ancestors.

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 77bb91e976..135187c5b5 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '
>  test_expect_success 'verify that .promisor file contains refs fetched' '
>  	ls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&
>  	test_line_count = 1 promisorlist &&
> -	git -C srv.bare rev-list HEAD >headhash &&
> +	git -C srv.bare rev-parse HEAD >headhash &&

Maybe worth using "rev-parse --verify" which would double check that we
produced a useful hash (it seems like an unlikely failure mode, but it's
easy enough to cover).

-Peff

Re: [PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id

[Danh Doan]

On 2020-03-19 12:07:07-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:05PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Only HEAD's object_id is necessary, rev-list is an overkill.
> > 
> > Despite POSIX requires grep(1) treat single pattern with <newline>
> > as multiple patterns.
> > busybox's grep(1) (as of v1.31.1) haven't implemented it yet.
> > 
> > Use rev-parse to simplify the test and avoid busybox unimplemented
> > features.
> 
> That makes sense. It would also future-proof us against the test
> changing the graph such that HEAD actually has ancestors.
> 
> > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> > index 77bb91e976..135187c5b5 100755
> > --- a/t/t5616-partial-clone.sh
> > +++ b/t/t5616-partial-clone.sh
> > @@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '
> >  test_expect_success 'verify that .promisor file contains refs fetched' '
> >  	ls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&
> >  	test_line_count = 1 promisorlist &&
> > -	git -C srv.bare rev-list HEAD >headhash &&
> > +	git -C srv.bare rev-parse HEAD >headhash &&
> 
> Maybe worth using "rev-parse --verify" which would double check that we
> produced a useful hash (it seems like an unlikely failure mode, but it's
> easy enough to cover).

Yeah, I also thought it wasn't expected to be a failure in this case.
Using `--verify` doesn't cost anything in this case, though.
I will add it in the reroll.

-- 
Danh

[PATCH 5/6] t7063: use POSIX find(1) syntax

[Đoàn Trần Công Danh]

Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
update feature, 2016-08-03), we started to use ls as a trick to update
directory's mtime.

However, `-ls` flag isn't required by POSIX's find(1), and
busybox(1) doesn't implement it.

Use an equivalence `-exec ls -dils {} +` instead.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t7063-status-untracked-cache.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 190ae149cf..c2731d445a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true
 export GIT_FORCE_UNTRACKED_CACHE
 
 sync_mtime () {
-	find . -type d -ls >/dev/null
+	find . -type d -exec ls -dils {} + >/dev/null
 }
 
 avoid_racy() {
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:

> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
> update feature, 2016-08-03), we started to use ls as a trick to update
> directory's mtime.
> 
> However, `-ls` flag isn't required by POSIX's find(1), and
> busybox(1) doesn't implement it.
> 
> Use an equivalence `-exec ls -dils {} +` instead.

Makes sense. I wonder if we need all of "-dils", but it's not clear to
me which syscalls actually trigger the FreeBSD lazy-update behavior. I
guess probably it's stat()ing the directory, so "ls -ld" would be
sufficient (and that's implied by the examples in 6b7728db81).

But I doubt the extra options would create a portability problem, so I
think it's fine either way.

-Peff

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:
>
>> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
>> update feature, 2016-08-03), we started to use ls as a trick to update
>> directory's mtime.
>> 
>> However, `-ls` flag isn't required by POSIX's find(1), and
>> busybox(1) doesn't implement it.
>> 
>> Use an equivalence `-exec ls -dils {} +` instead.
>
> Makes sense. I wonder if we need all of "-dils", but it's not clear to
> me which syscalls actually trigger the FreeBSD lazy-update behavior. I
> guess probably it's stat()ing the directory, so "ls -ld" would be
> sufficient (and that's implied by the examples in 6b7728db81).
>
> But I doubt the extra options would create a portability problem, so I
> think it's fine either way.

Thanks.  I too wondered if -dils is really needed (POSIX of course
have all of them, but we have to deal with non-POSIX systems, too,
and I am not sure how things like "-i" works there).

s/equivalence/equivalent/; perhaps?

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Danh Doan]

On 2020-03-19 15:16:09-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:
> >
> >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
> >> update feature, 2016-08-03), we started to use ls as a trick to update
> >> directory's mtime.
> >> 
> >> However, `-ls` flag isn't required by POSIX's find(1), and
> >> busybox(1) doesn't implement it.
> >> 
> >> Use an equivalence `-exec ls -dils {} +` instead.
> >
> > Makes sense. I wonder if we need all of "-dils", but it's not clear to

From the original commit message, I think whichever flags that call
stat(2) would be do it. It's `-d` (to check is_directory), and `-i`
for inode number.

This make make wonder, will it be enough to just use:

	find . -type d >/dev/null

> > me which syscalls actually trigger the FreeBSD lazy-update behavior. I
> > guess probably it's stat()ing the directory, so "ls -ld" would be
> > sufficient (and that's implied by the examples in 6b7728db81).
> >
> > But I doubt the extra options would create a portability problem, so I
> > think it's fine either way.
> 
> Thanks.  I too wondered if -dils is really needed (POSIX of course
> have all of them, but we have to deal with non-POSIX systems, too,
> and I am not sure how things like "-i" works there).

I think "-i" asks for stat(2) to get inode number,
which will ask FreeBSD sync mtime.
> 
> s/equivalence/equivalent/; perhaps?

Will do, I've never correctly used -ence and -ent pairs of words.

-- 
Danh

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Danh Doan]

On 2020-03-20 08:41:42+0700, Danh Doan <congdanhqx@gmail.com> wrote:
> On 2020-03-19 15:16:09-0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:
> > >
> > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
> > >> update feature, 2016-08-03), we started to use ls as a trick to update
> > >> directory's mtime.
> > >> 
> > >> However, `-ls` flag isn't required by POSIX's find(1), and
> > >> busybox(1) doesn't implement it.
> > >> 
> > >> Use an equivalence `-exec ls -dils {} +` instead.
> > >
> > > Makes sense. I wonder if we need all of "-dils", but it's not clear to
> 
> From the original commit message, I think whichever flags that call
> stat(2) would be do it. It's `-d` (to check is_directory), and `-i`
> for inode number.
> 
> This make make wonder, will it be enough to just use:
> 
> 	find . -type d >/dev/null

From the conversation in: xmqqmvktakvt.fsf@gitster.mtv.corp.google.com
I think

	find . -type d

would trigger enough lstat(2), and `-ls` was added for separated stat(2).

I guess either `-exec ls -d` or `-exec ls -i` will be enough.

> 
> > > me which syscalls actually trigger the FreeBSD lazy-update behavior. I
> > > guess probably it's stat()ing the directory, so "ls -ld" would be
> > > sufficient (and that's implied by the examples in 6b7728db81).
> > >
> > > But I doubt the extra options would create a portability problem, so I
> > > think it's fine either way.
> > 
> > Thanks.  I too wondered if -dils is really needed (POSIX of course
> > have all of them, but we have to deal with non-POSIX systems, too,
> > and I am not sure how things like "-i" works there).
> 
> I think "-i" asks for stat(2) to get inode number,
> which will ask FreeBSD sync mtime.
> > 
> > s/equivalence/equivalent/; perhaps?
> 
> Will do, I've never correctly used -ence and -ent pairs of words.

-- 
Danh

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Jeff King]

On Fri, Mar 20, 2020 at 08:41:42AM +0700, Danh Doan wrote:

> On 2020-03-19 15:16:09-0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:
> > >
> > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
> > >> update feature, 2016-08-03), we started to use ls as a trick to update
> > >> directory's mtime.
> > >> 
> > >> However, `-ls` flag isn't required by POSIX's find(1), and
> > >> busybox(1) doesn't implement it.
> > >> 
> > >> Use an equivalence `-exec ls -dils {} +` instead.
> > >
> > > Makes sense. I wonder if we need all of "-dils", but it's not clear to
> 
> From the original commit message, I think whichever flags that call
> stat(2) would be do it. It's `-d` (to check is_directory), and `-i`
> for inode number.
> 
> This make make wonder, will it be enough to just use:
> 
> 	find . -type d >/dev/null

Perhaps we can get a friendly FreeBSD developer (cc'd) to run a quick
test for us.

Ed, the question is whether:

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 190ae149cf..6791c6b95a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true
 export GIT_FORCE_UNTRACKED_CACHE
 
 sync_mtime () {
-	find . -type d -ls >/dev/null
+	find . -type d >/dev/null
 }
 
 avoid_racy() {

lets t7063 consistently pass on FreeBSD.

-Peff

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Danh Doan]

On 2020-03-20 01:37:30-0400, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 20, 2020 at 08:41:42AM +0700, Danh Doan wrote:
> 
> > On 2020-03-19 15:16:09-0700, Junio C Hamano <gitster@pobox.com> wrote:
> > > Jeff King <peff@peff.net> writes:
> > > 
> > > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:
> > > >
> > > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime
> > > >> update feature, 2016-08-03), we started to use ls as a trick to update
> > > >> directory's mtime.
> > > >> 
> > > >> However, `-ls` flag isn't required by POSIX's find(1), and
> > > >> busybox(1) doesn't implement it.
> > > >> 
> > > >> Use an equivalence `-exec ls -dils {} +` instead.
> > > >
> > > > Makes sense. I wonder if we need all of "-dils", but it's not clear to
> > 
> > From the original commit message, I think whichever flags that call
> > stat(2) would be do it. It's `-d` (to check is_directory), and `-i`
> > for inode number.
> > 
> > This make make wonder, will it be enough to just use:
> > 
> > 	find . -type d >/dev/null
> 
> Perhaps we can get a friendly FreeBSD developer (cc'd) to run a quick
> test for us.
> 
> Ed, the question is whether:
> 
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index 190ae149cf..6791c6b95a 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true
>  export GIT_FORCE_UNTRACKED_CACHE
>  
>  sync_mtime () {
> -	find . -type d -ls >/dev/null
> +	find . -type d >/dev/null
>  }
>  
>  avoid_racy() {
> 
> lets t7063 consistently pass on FreeBSD.

I've tried myself with a FreeBSD VM which stays on top of an HDD,
t7063 consistently pass for 1000 run.

I guess it should be fine

I'll resend with this while waiting for Ed's response.

-- 
Danh

Re: [PATCH 5/6] t7063: use POSIX find(1) syntax

[Jeff King]

On Sun, Mar 22, 2020 at 07:37:03AM +0700, Danh Doan wrote:

> >  sync_mtime () {
> > -	find . -type d -ls >/dev/null
> > +	find . -type d >/dev/null
> >  }
> >  
> >  avoid_racy() {
> > 
> > lets t7063 consistently pass on FreeBSD.
> 
> I've tried myself with a FreeBSD VM which stays on top of an HDD,
> t7063 consistently pass for 1000 run.
> 
> I guess it should be fine
> 
> I'll resend with this while waiting for Ed's response.

Thanks, that's good enough for me. I just didn't want us committing
something without _somebody_ trying it on FreeBSD (and I was too lazy to
install a VM myself).

-Peff

[PATCH 6/6] t4124: fix test for non-compliance diff

[Đoàn Trần Công Danh]

POSIX's diff(1) requires output in normal diff format.
However, busybox's diff's output is written in unified format.

POSIX requires no option for normal-diff format.

A hint in test-lib-functions::test_cmp said `diff -u` isn't available
everywhere.

Workaround this problem by assuming `diff(1)` output is unified
if we couldn't make anything from normal-diff format.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t4124-apply-ws-rule.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 971a5a7512..2a54ce96b5 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -52,6 +52,12 @@ test_fix () {
 
 	# find touched lines
 	$DIFF file target | sed -n -e "s/^> //p" >fixed
+	if ! test -s fixed; then
+		$DIFF file target |
+		grep '^+' |
+		grep -v '^+++' |
+		sed -e "s/+//" >fixed
+	fi
 
 	# the changed lines are all expected to change
 	fixed_cnt=$(wc -l <fixed)
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:

> POSIX's diff(1) requires output in normal diff format.
> However, busybox's diff's output is written in unified format.

That's a pretty big difference. I'm surprised this only produces one
problem in the test scripts. ;)

> POSIX requires no option for normal-diff format.
> 
> A hint in test-lib-functions::test_cmp said `diff -u` isn't available
> everywhere.
> 
> Workaround this problem by assuming `diff(1)` output is unified
> if we couldn't make anything from normal-diff format.

I wonder if we could use "git diff" here. We have to be careful about
circular reasoning in our tests (i.e., making sure we're not verifying
output with the same code that we're testing), but I think here we're
checking how "apply --whitespace=fix" works.

But if this is the only spot, then adjusting to handle unified or normal
diff isn't too bad.

> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 971a5a7512..2a54ce96b5 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -52,6 +52,12 @@ test_fix () {
>  
>  	# find touched lines
>  	$DIFF file target | sed -n -e "s/^> //p" >fixed
> +	if ! test -s fixed; then
> +		$DIFF file target |
> +		grep '^+' |
> +		grep -v '^+++' |
> +		sed -e "s/+//" >fixed
> +	fi

I think those greps could be lumped into sed like:

  sed -ne "s/^+[^+]//p"

(at the cost of missing blank lines, but I think that's OK for our
purposes here; it could be fixed with an ERE).

Could we then make a single invocation that covers both diff formats? We
can further observe that the only thing we do with the "fixed" file is
count the lines, so we can leave the markers. Which means we could ditch
sed entirely and use grep. Something like:

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 971a5a7512..15cb0c81b7 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -50,8 +50,9 @@ test_fix () {
 	# fix should not barf
 	apply_patch --whitespace=fix || return 1
 
-	# find touched lines
-	$DIFF file target | sed -n -e "s/^> //p" >fixed
+	# find touched lines; handle either normal or unified
+	# diff, as system diff may generate either
+	$DIFF file target | grep '^[>+][^+]' >fixed
 
 	# the changed lines are all expected to change
 	fixed_cnt=$(wc -l <fixed)

seems to work for with both busybox diff and GNU diff.

-Peff

Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:
>
>> POSIX's diff(1) requires output in normal diff format.
>> However, busybox's diff's output is written in unified format.
>
> That's a pretty big difference. I'm surprised this only produces one
> problem in the test scripts. ;)
>
>> POSIX requires no option for normal-diff format.
>> 
>> A hint in test-lib-functions::test_cmp said `diff -u` isn't available
>> everywhere.
>> 
>> Workaround this problem by assuming `diff(1)` output is unified
>> if we couldn't make anything from normal-diff format.

I do not mind working it around, but I am a bit disturbed by an
uneven attitude towards POSIX noncompliance this series has.  If
we were willing to break other people's "sed" that does not do BRE
correctly, instead of using '[+]' trick to accomodate them while
making sure that an implementation that does not use nonstandard
extension and does only BRE, we should just similarly be writing
such an implementation of noncompliant diff off as broken, yet we
bend backwards over to make sure we can work with them here.  

IOW, I do not have trouble changing the test so that it works with
noncompliant "diff".  But then in the same series, I would prefer to
see the existing test keeps working with a possibly noncompliant
"sed" implementation that has been working well with the tests.

> But if this is the only spot, then adjusting to handle unified or normal
> diff isn't too bad.

Yup.

> Could we then make a single invocation that covers both diff formats? We
> can further observe that the only thing we do with the "fixed" file is
> count the lines, so we can leave the markers. Which means we could ditch
> sed entirely and use grep. Something like:
>
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 971a5a7512..15cb0c81b7 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -50,8 +50,9 @@ test_fix () {
>  	# fix should not barf
>  	apply_patch --whitespace=fix || return 1
>  
> -	# find touched lines
> -	$DIFF file target | sed -n -e "s/^> //p" >fixed
> +	# find touched lines; handle either normal or unified
> +	# diff, as system diff may generate either
> +	$DIFF file target | grep '^[>+][^+]' >fixed
>  
>  	# the changed lines are all expected to change
>  	fixed_cnt=$(wc -l <fixed)
>
> seems to work for with both busybox diff and GNU diff.

Sounds OK to me.

Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Jeff King]

On Thu, Mar 19, 2020 at 03:58:51PM -0700, Junio C Hamano wrote:

> >> Workaround this problem by assuming `diff(1)` output is unified
> >> if we couldn't make anything from normal-diff format.
> 
> I do not mind working it around, but I am a bit disturbed by an
> uneven attitude towards POSIX noncompliance this series has.  If
> we were willing to break other people's "sed" that does not do BRE
> correctly, instead of using '[+]' trick to accomodate them while
> making sure that an implementation that does not use nonstandard
> extension and does only BRE, we should just similarly be writing
> such an implementation of noncompliant diff off as broken, yet we
> bend backwards over to make sure we can work with them here.
> 
> IOW, I do not have trouble changing the test so that it works with
> noncompliant "diff".  But then in the same series, I would prefer to
> see the existing test keeps working with a possibly noncompliant
> "sed" implementation that has been working well with the tests.

I don't think it's inconsistent. Real-world experience trumps standards.
We _know_ that there is a real-world diff that generates only unified
diffs, and it is not too hard to work around it. So we should do so.

A sed that uses ERE and requires backslash-escaping pluses is
theoretical at this point. POSIX forbids it, and I would guess that
working around it would be more than just the "[+]" we found, because
other patterns probably need it, too. But we won't know until we find
one to test on.

So I'm not entirely against "[+]" as a defensive measure. But I have a
slight preference to avoid it until we know it's needed, not because
it's hard to do once, but because I don't want to grow too many
defensive superstitions if we don't know they're warranted.

-Peff

Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Danh Doan]

On 2020-03-19 12:33:34-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:
> 
> > POSIX's diff(1) requires output in normal diff format.
> > However, busybox's diff's output is written in unified format.
> 
> That's a pretty big difference. I'm surprised this only produces one
> problem in the test scripts. ;)
> 
> > POSIX requires no option for normal-diff format.
> > 
> > A hint in test-lib-functions::test_cmp said `diff -u` isn't available
> > everywhere.
> > 
> > Workaround this problem by assuming `diff(1)` output is unified
> > if we couldn't make anything from normal-diff format.
> 
> I wonder if we could use "git diff" here. We have to be careful about
> circular reasoning in our tests (i.e., making sure we're not verifying
> output with the same code that we're testing), but I think here we're
> checking how "apply --whitespace=fix" works.
> 
> But if this is the only spot, then adjusting to handle unified or normal
> diff isn't too bad.
> 
> > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> > index 971a5a7512..2a54ce96b5 100755
> > --- a/t/t4124-apply-ws-rule.sh
> > +++ b/t/t4124-apply-ws-rule.sh
> > @@ -52,6 +52,12 @@ test_fix () {
> >  
> >  	# find touched lines
> >  	$DIFF file target | sed -n -e "s/^> //p" >fixed
> > +	if ! test -s fixed; then
> > +		$DIFF file target |
> > +		grep '^+' |
> > +		grep -v '^+++' |
> > +		sed -e "s/+//" >fixed
> > +	fi
> 
> I think those greps could be lumped into sed like:
> 
>   sed -ne "s/^+[^+]//p"
> 
> (at the cost of missing blank lines, but I think that's OK for our
> purposes here; it could be fixed with an ERE).
> 
> Could we then make a single invocation that covers both diff formats? We
> can further observe that the only thing we do with the "fixed" file is
> count the lines, so we can leave the markers. Which means we could ditch
> sed entirely and use grep. Something like:
> 
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 971a5a7512..15cb0c81b7 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -50,8 +50,9 @@ test_fix () {
>  	# fix should not barf
>  	apply_patch --whitespace=fix || return 1
>  
> -	# find touched lines
> -	$DIFF file target | sed -n -e "s/^> //p" >fixed
> +	# find touched lines; handle either normal or unified
> +	# diff, as system diff may generate either
> +	$DIFF file target | grep '^[>+][^+]' >fixed
>  
>  	# the changed lines are all expected to change
>  	fixed_cnt=$(wc -l <fixed)
> 
> seems to work for with both busybox diff and GNU diff.

3 lines after this one:

	?*) expect_cnt=$(grep "[$1]" <fixed | wc -l) ;;

As of now, we could simply replace sed with grep entirely,
because ! "$1" ~"[>+]".

Considering the complicated of:

	test_expect_success "rule=$rule" '
		git config core.whitespace "$rule" &&
		test_fix "$tt$ts$ti$th"
	'

I think it's better to use sed here.

-- 
Danh

Re: [PATCH 6/6] t4124: fix test for non-compliance diff

[Jeff King]

On Fri, Mar 20, 2020 at 08:52:23AM +0700, Danh Doan wrote:

> > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> > index 971a5a7512..15cb0c81b7 100755
> > --- a/t/t4124-apply-ws-rule.sh
> > +++ b/t/t4124-apply-ws-rule.sh
> > @@ -50,8 +50,9 @@ test_fix () {
> >  	# fix should not barf
> >  	apply_patch --whitespace=fix || return 1
> >  
> > -	# find touched lines
> > -	$DIFF file target | sed -n -e "s/^> //p" >fixed
> > +	# find touched lines; handle either normal or unified
> > +	# diff, as system diff may generate either
> > +	$DIFF file target | grep '^[>+][^+]' >fixed
> >  
> >  	# the changed lines are all expected to change
> >  	fixed_cnt=$(wc -l <fixed)
> > 
> > seems to work for with both busybox diff and GNU diff.
> 
> 3 lines after this one:
> 
> 	?*) expect_cnt=$(grep "[$1]" <fixed | wc -l) ;;
> 
> As of now, we could simply replace sed with grep entirely,
> because ! "$1" ~"[>+]".
> 
> Considering the complicated of:
> 
> 	test_expect_success "rule=$rule" '
> 		git config core.whitespace "$rule" &&
> 		test_fix "$tt$ts$ti$th"
> 	'
> 
> I think it's better to use sed here.

Fair enough. I think it's OK now, but I agree that it puts a pretty
subtle assumption into the test_fix function (and that's far removed
from what actual tests will call it with, so it's easy to miss). Using
sed should be more maintainable.

-Peff

Re: [PATCH 0/6] fix test failure with busybox

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:01PM +0700, Đoàn Trần Công Danh wrote:

> Despite some non-compiance from busybox's sh(1), grep(1), diff(1),
> Alpine Linux is still a popular choice for container these days.
> 
> Fix false-positive failure in testsuite when run in Alpine Linux.
> 
> t5703.{4,5,6,7} is still failing.
> Despite git pack-objects behaves correctly,
> upload-pack.c works incorrectly on this platform.
> 
> I haven't dig deeper, yet.

I was able to reproduce the problems on Debian (with busybox installed)
with:

  mkdir /tmp/bb
  (cd /tmp/bb
   bb=$(which busybox)
   for i in $($bb --list); do
     ln -s $bb $i
   done)
  PATH=/tmp/bb:$PATH make test TEST_SHELL_PATH=/tmp/bb/sh

The issue in t5703 is the sed call in get_actual_commits(). It's trying
to cut off the early (text) part of the file, and pass through the
binary goo of the packfile. Presumably busybox's sed isn't binary-clean.

Our usual strategy here would be to switch to perl, which is more
predictable about binary bytes.

We're also feeding this into a test-tool helper. It's possible that
helper could be made smart enough to replace both this sed invocation
and the one in get_actual_refs().

-Peff

Re: [PATCH 0/6] fix test failure with busybox

[Danh Doan]

On 2020-03-19 11:51:36-0400, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2020 at 09:00:01PM +0700, Đoàn Trần Công Danh wrote:
> 
> > Despite some non-compiance from busybox's sh(1), grep(1), diff(1),
> > Alpine Linux is still a popular choice for container these days.
> > 
> > Fix false-positive failure in testsuite when run in Alpine Linux.
> > 
> > t5703.{4,5,6,7} is still failing.
> > Despite git pack-objects behaves correctly,
> > upload-pack.c works incorrectly on this platform.
> > 
> > I haven't dig deeper, yet.
> 
> I was able to reproduce the problems on Debian (with busybox installed)
> with:
> 
>   mkdir /tmp/bb
>   (cd /tmp/bb
>    bb=$(which busybox)
>    for i in $($bb --list); do
>      ln -s $bb $i
>    done)
>   PATH=/tmp/bb:$PATH make test TEST_SHELL_PATH=/tmp/bb/sh
> 
> The issue in t5703 is the sed call in get_actual_commits(). It's trying
> to cut off the early (text) part of the file, and pass through the
> binary goo of the packfile. Presumably busybox's sed isn't binary-clean.

I've checked, busybox's sed think every input is text file,
and in POSIX, every line in every text file must be terminated by
<newline>.

Thus, busybox's sed append a <newline> after `0000` marker, render the
pack file invalid.

> Our usual strategy here would be to switch to perl, which is more
> predictable about binary bytes.

Perl works fine here.

> We're also feeding this into a test-tool helper. It's possible that
> helper could be made smart enough to replace both this sed invocation
> and the one in get_actual_refs().

I looked into this direction, I guess you meant something like this:

-------------8<--------------
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 282d536384..1d62143dbc 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "test-tool.h"
 #include "pkt-line.h"
+#include "strbuf.h"
 
 static void pack_line(const char *line)
 {
@@ -53,6 +54,13 @@ static void unpack(void)
 static void unpack_sideband(void)
 {
 	struct packet_reader reader;
+	struct strbuf buf = STRBUF_INIT;
+
+	while (strbuf_getline(&buf, stdin) == 0)
+		if (strstr(buf.buf, "packfile"))
+		    break;
+	strbuf_release(&buf);
+
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_GENTLE_ON_EOF |
 			   PACKET_READ_CHOMP_NEWLINE);
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 7fba3063bf..a34460f7d8 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -13,10 +13,7 @@ get_actual_refs () {
 }
 
 get_actual_commits () {
-	sed -n -e '/packfile/,/0000/{
-		/packfile/d
-		p
-		}' <out | test-tool pkt-line unpack-sideband >o.pack &&
+	test-tool pkt-line unpack-sideband <out >o.pack &&
 	git index-pack o.pack &&
 	git verify-pack -v o.idx >objs &&
 	grep commit objs | cut -d" " -f1 | sort >actual_commits
----------------------->8-------------------

But, this doesn't work. as `packet_reader_read` reads directly from fd 0.

I think perl should be good for now.

-- 
Danh

Re: [PATCH 0/6] fix test failure with busybox

[Jeff King]

On Fri, Mar 20, 2020 at 07:37:15AM +0700, Danh Doan wrote:

> > The issue in t5703 is the sed call in get_actual_commits(). It's trying
> > to cut off the early (text) part of the file, and pass through the
> > binary goo of the packfile. Presumably busybox's sed isn't binary-clean.
> 
> I've checked, busybox's sed think every input is text file,
> and in POSIX, every line in every text file must be terminated by
> <newline>.
> 
> Thus, busybox's sed append a <newline> after `0000` marker, render the
> pack file invalid.

Ah, thanks for tracking it down.

> > We're also feeding this into a test-tool helper. It's possible that
> > helper could be made smart enough to replace both this sed invocation
> > and the one in get_actual_refs().
> 
> I looked into this direction, I guess you meant something like this:
> [...]
> @@ -53,6 +54,13 @@ static void unpack(void)
>  static void unpack_sideband(void)
>  {
>  	struct packet_reader reader;
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	while (strbuf_getline(&buf, stdin) == 0)
> +		if (strstr(buf.buf, "packfile"))
> +		    break;
> +	strbuf_release(&buf);
> +
>  	packet_reader_init(&reader, 0, NULL, 0,
>  			   PACKET_READ_GENTLE_ON_EOF |
>  			   PACKET_READ_CHOMP_NEWLINE);
> [...]
> But, this doesn't work. as `packet_reader_read` reads directly from fd 0.

You'd want to decode the packets first first, and then skip until after
you see the packet with "packfile" in it. Or to be more generic, just
wait until you see packets with actual sideband data. Something like the
patch below works (though I'm also fine with just switching to perl).

diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 282d536384..12ca698e17 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -67,7 +67,7 @@ static void unpack_sideband(void)
 		case PACKET_READ_NORMAL:
 			band = reader.line[0] & 0xff;
 			if (band < 1 || band > 2)
-				die("unexpected side band %d", band);
+				continue; /* skip non-sideband packets */
 			fd = band;
 
 			write_or_die(fd, reader.line + 1, reader.pktlen - 1);
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 7fba3063bf..a34460f7d8 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -13,10 +13,7 @@ get_actual_refs () {
 }
 
 get_actual_commits () {
-	sed -n -e '/packfile/,/0000/{
-		/packfile/d
-		p
-		}' <out | test-tool pkt-line unpack-sideband >o.pack &&
+	test-tool pkt-line unpack-sideband <out >o.pack &&
 	git index-pack o.pack &&
 	git verify-pack -v o.idx >objs &&
 	grep commit objs | cut -d" " -f1 | sort >actual_commits

Re: [PATCH 0/6] fix test failure with busybox

[Jeff King]

On Thu, Mar 19, 2020 at 09:00:01PM +0700, Đoàn Trần Công Danh wrote:

> Despite some non-compiance from busybox's sh(1), grep(1), diff(1),
> Alpine Linux is still a popular choice for container these days.
> 
> Fix false-positive failure in testsuite when run in Alpine Linux.

Thanks, these all looked like sensible directions. I left a few small
comments that I think are worth addressing.

-Peff