[PATCH v2 0/8] fix test failure with busybox

[PATCH v2 0/8] fix test failure with busybox

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

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

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

Đoàn Trần Công Danh (8):
  t4061: use POSIX compliant regex(7)
  test-lib-functions: test_cmp: eval $GIT_TEST_CMP
  t5003: drop the subshell in test_lazy_prereq
  t5003: skip conversion test if unzip -a is unavailable
  t5616: use rev-parse instead to get HEAD's object_id
  t7063: drop non-POSIX argument "-ls" from find(1)
  t4124: fix test for non-compliant diff(1)
  t5703: feed raw data into test-tool unpack-sideband

 t/helper/test-pkt-line.c           |  2 +-
 t/t4061-diff-indent.sh             |  2 +-
 t/t4124-apply-ws-rule.sh           |  6 ++++++
 t/t5003-archive-zip.sh             | 24 ++++++++++++------------
 t/t5616-partial-clone.sh           |  2 +-
 t/t5703-upload-pack-ref-in-want.sh |  5 +----
 t/t7063-status-untracked-cache.sh  |  2 +-
 t/test-lib-functions.sh            |  2 +-
 8 files changed, 24 insertions(+), 21 deletions(-)

Range-diff against v1:
1:  4830bd3aaf ! 1:  288e343d09 t4061: use POSIX compliance regex(7)
    @@ Metadata
     Author: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    t4061: use POSIX compliance regex(7)
    +    t4061: use POSIX compliant regex(7)
     
         BRE interprets `+` literally, and
         `\+` is undefined for POSIX BRE, from:
    @@ Commit message
     
         This test is failing with busybox sed, the default sed of Alpine Linux
     
    -    Fix it by using literal `+` instead.
    +    We have 2 options here:
    +
    +    - Using literal `+` because BRE will interpret it as-is, or
    +    - Using character class `[+]` to defend against a sed that expects ERE
    +
    +    ERE-expected sed is theoretical at this point,
    +    but we haven't found it, yet.
    +    And, we may run into other problems with that sed.
    +    Let's go with first option and fix it later if that sed could be found.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
2:  d3fafd61ec ! 2:  9d3823e849 test-lib-functions: test_cmp: eval $GIT_TEST_CMP
    @@ Commit message
         test-lib-functions: test_cmp: eval $GIT_TEST_CMP
     
         Shell recognises first non-assignment token as command name.
    -    Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:
    +    With /bin/sh linked to either /bin/bash or /bin/dash,
    +    `cd t/perf && ./p0000-perf-lib-sanity.sh -d -i -v` reports:
     
         > test_cmp:1: command not found: diff -u
     
-:  ---------- > 3:  8e85b5c15c t5003: drop the subshell in test_lazy_prereq
3:  ddeeefeae2 ! 4:  4f0ac0867c t5003: skip conversion test if unzip -a is unavailable
    @@ Commit message
     
      ## t/t5003-archive-zip.sh ##
     @@ t/t5003-archive-zip.sh: test_lazy_prereq UNZIP_SYMLINKS '
    - 	)
    + 	test -h symlink
      '
      
     +test_lazy_prereq UNZIP_CONVERT '
    -+	(
    -+		mkdir unzip-convert &&
    -+		cd unzip-convert &&
    -+		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
    -+	)
    ++	"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
     +'
     +
      check_zip() {
4:  ce976e850c ! 5:  ee7ef352a7 t5616: use rev-parse instead to get HEAD's object_id
    @@ t/t5616-partial-clone.sh: test_expect_success 'do partial clone 1' '
      	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 &&
    ++	git -C srv.bare rev-parse --verify HEAD >headhash &&
      	grep "$(cat headhash) HEAD" $(cat promisorlist) &&
      	grep "$(cat headhash) refs/heads/master" $(cat promisorlist)
      '
5:  1993b28a12 ! 6:  59e3f73784 t7063: use POSIX find(1) syntax
    @@ Metadata
     Author: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    t7063: use POSIX find(1) syntax
    +    t7063: drop non-POSIX argument "-ls" from find(1)
     
         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
    @@ Commit message
         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.
    +    From the original conversation, it seems like find(1) with "-type d"
    +    could trigger enough "lstat(2)" to ask FreeBSD update mtime.
    +
    +    Use only filter "-type d" for now.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
    @@ t/t7063-status-untracked-cache.sh: GIT_FORCE_UNTRACKED_CACHE=true
      
      sync_mtime () {
     -	find . -type d -ls >/dev/null
    -+	find . -type d -exec ls -dils {} + >/dev/null
    ++	find . -type d >/dev/null
      }
      
      avoid_racy() {
6:  cd86febed9 ! 7:  10f39c3d30 t4124: fix test for non-compliance diff
    @@ Metadata
     Author: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    t4124: fix test for non-compliance diff
    +    t4124: fix test for non-compliant diff(1)
     
         POSIX's diff(1) requires output in normal diff format.
         However, busybox's diff's output is written in unified format.
    @@ t/t4124-apply-ws-rule.sh: test_fix () {
      
      	# find touched lines
      	$DIFF file target | sed -n -e "s/^> //p" >fixed
    ++	# busybox's diff(1) output unified format
     +	if ! test -s fixed; then
     +		$DIFF file target |
    -+		grep '^+' |
    -+		grep -v '^+++' |
    -+		sed -e "s/+//" >fixed
    ++		grep -v '^+++ target' |
    ++		sed -e "/^+/s/+//" >fixed
     +	fi
      
      	# the changed lines are all expected to change
-:  ---------- > 8:  d4507d381e t5703: feed raw data into test-tool unpack-sideband
-- 
2.26.0.rc2.310.g2932bb562d

[PATCH v2 1/8] t4061: use POSIX compliant 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

We have 2 options here:

- Using literal `+` because BRE will interpret it as-is, or
- Using character class `[+]` to defend against a sed that expects ERE

ERE-expected sed is theoretical at this point,
but we haven't found it, yet.
And, we may run into other problems with that sed.
Let's go with first option and fix it later if that sed could be found.

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.310.g2932bb562d

[PATCH v2 2/8] 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.
With /bin/sh linked to either /bin/bash or /bin/dash,
`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.310.g2932bb562d

[PATCH v2 3/8] t5003: drop the subshell in test_lazy_prereq

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

test_lazy_prereq will be evaluated in a throw-away directory.

Drop unnecessary subshell and mkdir.

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

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 106eddbd85..df1374a312 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -7,12 +7,8 @@ test_description='git archive --format=zip test'
 SUBSTFORMAT=%H%n
 
 test_lazy_prereq UNZIP_SYMLINKS '
-	(
-		mkdir unzip-symlinks &&
-		cd unzip-symlinks &&
-		"$GIT_UNZIP" "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip &&
-		test -h symlink
-	)
+	"$GIT_UNZIP" "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip &&
+	test -h symlink
 '
 
 check_zip() {
-- 
2.26.0.rc2.310.g2932bb562d

[PATCH v2 4/8] 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 | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index df1374a312..3b76d2eb65 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -11,6 +11,10 @@ test_lazy_prereq UNZIP_SYMLINKS '
 	test -h symlink
 '
 
+test_lazy_prereq UNZIP_CONVERT '
+	"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst
@@ -35,33 +39,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.310.g2932bb562d

[PATCH v2 5/8] 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..09e640cae4 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 --verify HEAD >headhash &&
 	grep "$(cat headhash) HEAD" $(cat promisorlist) &&
 	grep "$(cat headhash) refs/heads/master" $(cat promisorlist)
 '
-- 
2.26.0.rc2.310.g2932bb562d

[PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Đ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.

From the original conversation, it seems like find(1) with "-type d"
could trigger enough "lstat(2)" to ask FreeBSD update mtime.

Use only filter "-type d" for now.

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..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() {
-- 
2.26.0.rc2.310.g2932bb562d

[PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Đ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..075b1912be 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
+	# busybox's diff(1) output unified format
+	if ! test -s fixed; then
+		$DIFF file target |
+		grep -v '^+++ target' |
+		sed -e "/^+/s/+//" >fixed
+	fi
 
 	# the changed lines are all expected to change
 	fixed_cnt=$(wc -l <fixed)
-- 
2.26.0.rc2.310.g2932bb562d

[PATCH v2 8/8] t5703: feed raw data into test-tool unpack-sideband

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

busybox's sed isn't binary clean.
Thus, triggers false-negative on this test.

We could replace sed with perl on this usecase.
But, we could slightly modify the helper to discard unwanted data in the
beginning.

Fix the false negative by updating this helper.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/helper/test-pkt-line.c           | 2 +-
 t/t5703-upload-pack-ref-in-want.sh | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

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
-- 
2.26.0.rc2.310.g2932bb562d

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

[Jeff King]

On Sun, Mar 22, 2020 at 07:55:09AM +0700, Đoàn Trần Công Danh wrote:

> Đoàn Trần Công Danh (8):
>   t4061: use POSIX compliant regex(7)
>   test-lib-functions: test_cmp: eval $GIT_TEST_CMP
>   t5003: drop the subshell in test_lazy_prereq
>   t5003: skip conversion test if unzip -a is unavailable
>   t5616: use rev-parse instead to get HEAD's object_id
>   t7063: drop non-POSIX argument "-ls" from find(1)
>   t4124: fix test for non-compliant diff(1)
>   t5703: feed raw data into test-tool unpack-sideband

These all look OK to me. Thanks for the extra discussion on BRE vs ERE
in the commit message of the first one.

-Peff

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

Hi,

On Sun, 22 Mar 2020, Đ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.
>
> 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..075b1912be 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
> +	# busybox's diff(1) output unified format
> +	if ! test -s fixed; then
> +		$DIFF file target |
> +		grep -v '^+++ target' |
> +		sed -e "/^+/s/+//" >fixed
> +	fi

In my patches (which are too unpolished to contribute, I have not found
time to clean them up in several years), I do this differently:

-- snip --
commit cb2f3a28dbf40b92d3d9ca0f3177cd5afb7c4196
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Wed Jul 5 22:21:57 2017 +0200

    t4124: avoid using "normal" diff mode

    Everybody and their dogs, cats and other pets settled on using unified
    diffs. It is a really quaint holdover from a long-gone era that GNU diff
    outputs "normal" diff by default.

    Yet, t4124 relied on that mode.

    This mode is so out of fashion in the meantime, though, that e.g.
    BusyBox' diff decided not even to bother to support it. It only supports
    unified diffs.

    So let's just switch away from "normal" diffs and use unified diffs, as
    we really are only interested in the `+` lines.

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 971a5a7512ac..133557b99333 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -51,7 +51,7 @@ test_fix () {
        apply_patch --whitespace=fix || return 1

        # find touched lines
-       $DIFF file target | sed -n -e "s/^> //p" >fixed
+       $DIFF -u file target | sed -n -e "3,\$s/^+//p" >fixed

        # the changed lines are all expected to change
        fixed_cnt=$(wc -l <fixed)
-- snap --

Food for thought?

Ciao,
Dscho

>
>  	# the changed lines are all expected to change
>  	fixed_cnt=$(wc -l <fixed)
> --
> 2.26.0.rc2.310.g2932bb562d
>
>
>

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

Hi,

On Sun, 22 Mar 2020, Đ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.
>
> >From the original conversation, it seems like find(1) with "-type d"
> could trigger enough "lstat(2)" to ask FreeBSD update mtime.

This rationale  makes me uneasy: why did Duy add _both_ `-type d` *and*
`-ls` if the former would have been enough?

> Use only filter "-type d" for now.
>
> 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..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

A more conservative patch would be the following:

-- snip --
commit 1680a64fae24b1073dbf1b844889a9953823b7a2
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Wed Jul 19 22:13:16 2017 +0200

    t7063: when running under BusyBox, avoid unsupported find option

    BusyBox' find implementation does not understand the -ls option, so
    let's not use it when we're running inside BusyBox.

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/t/t7063-status-untracked-cache.sh
b/t/t7063-status-untracked-cache.sh
index 190ae149cf3c..ab7e8b5fea01 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,7 +18,12 @@ GIT_FORCE_UNTRACKED_CACHE=true
 export GIT_FORCE_UNTRACKED_CACHE

 sync_mtime () {
-       find . -type d -ls >/dev/null
+       if test_have_prereq BUSYBOX
+       then
+               find . -type d -print0 | xargs -0r ls -ld >/dev/null
+       else
+               find . -type d -ls >/dev/null
+       fi
 }

 avoid_racy() {
-- snap --

I have this in Git for Windows' fork, although I have to admit that there
is no CI set up to verify that this is all working as I expect it to.

Ciao,
Dscho

>  }
>
>  avoid_racy() {
> --
> 2.26.0.rc2.310.g2932bb562d
>
>
>

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Torsten Bögershausen]

On Mon, Mar 23, 2020 at 03:11:50PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 22 Mar 2020, Đ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.
> >
> > >From the original conversation, it seems like find(1) with "-type d"
> > could trigger enough "lstat(2)" to ask FreeBSD update mtime.
>
> This rationale  makes me uneasy: why did Duy add _both_ `-type d` *and*
> `-ls` if the former would have been enough?

man readdir on my Linux gives:
[]
DESCRIPTION
       The  readdir() function returns a pointer to a dirent structure representing the next
       directory entry in the directory stream pointed to by dirp.  It returns
       NULL on reaching the end of the directory stream or if an error occurred.

       In the glibc implementation, the dirent structure is defined as follows:

           struct dirent {
               ino_t          d_ino;       /* Inode number */
               off_t          d_off;       /* Not an offset; see below */
               unsigned short d_reclen;    /* Length of this record */
               unsigned char  d_type;      /* Type of file; not supported
                                              by all filesystem types */
               char           d_name[256]; /* Null-terminated filename */
           };

So we could imagine that `find` is clever enough to extract "-type d" already
when calling readdir(), and the "good old" following stat() is not needed here.

So I would strongly agree with Dscho to keep the `ls`

[snip]

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Danh Doan]

On 2020-03-23 14:58:13+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Sun, 22 Mar 2020, Đ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.
> >
> > 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..075b1912be 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
> > +	# busybox's diff(1) output unified format
> > +	if ! test -s fixed; then
> > +		$DIFF file target |
> > +		grep -v '^+++ target' |
> > +		sed -e "/^+/s/+//" >fixed
> > +	fi
> 
> In my patches (which are too unpolished to contribute, I have not found
> time to clean them up in several years), I do this differently:
> 
> -- snip --
> commit cb2f3a28dbf40b92d3d9ca0f3177cd5afb7c4196
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Date:   Wed Jul 5 22:21:57 2017 +0200
> 
>     t4124: avoid using "normal" diff mode
> 
>     Everybody and their dogs, cats and other pets settled on using unified
>     diffs. It is a really quaint holdover from a long-gone era that GNU diff
>     outputs "normal" diff by default.
> 
>     Yet, t4124 relied on that mode.
> 
>     This mode is so out of fashion in the meantime, though, that e.g.
>     BusyBox' diff decided not even to bother to support it. It only supports
>     unified diffs.
> 
>     So let's just switch away from "normal" diffs and use unified diffs, as
>     we really are only interested in the `+` lines.
> 
>     Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 971a5a7512ac..133557b99333 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -51,7 +51,7 @@ test_fix () {
>         apply_patch --whitespace=fix || return 1
> 
>         # find touched lines
> -       $DIFF file target | sed -n -e "s/^> //p" >fixed
> +       $DIFF -u file target | sed -n -e "3,\$s/^+//p" >fixed
> 
>         # the changed lines are all expected to change
>         fixed_cnt=$(wc -l <fixed)
> -- snap --
> 
> Food for thought?

A comment in test-lib-functions::test_cmp mentions that
there _is_ a diff out there that doesn't understand "-u".
I don't know which one is it.

If we choose to use "diff -u" here, we've made a certain assumption,
should we flip the switch in test_cmp, too?

-- 
Danh

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Danh Doan]

On 2020-03-23 15:11:50+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Sun, 22 Mar 2020, Đ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.
> >
> > >From the original conversation, it seems like find(1) with "-type d"
> > could trigger enough "lstat(2)" to ask FreeBSD update mtime.
> 
> This rationale  makes me uneasy: why did Duy add _both_ `-type d` *and*
> `-ls` if the former would have been enough?
> 
> > Use only filter "-type d" for now.
> >
> > 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..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
> 
> A more conservative patch would be the following:
> 
> -- snip --
> commit 1680a64fae24b1073dbf1b844889a9953823b7a2
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Date:   Wed Jul 19 22:13:16 2017 +0200
> 
>     t7063: when running under BusyBox, avoid unsupported find option
> 
>     BusyBox' find implementation does not understand the -ls option, so
>     let's not use it when we're running inside BusyBox.

Yes, this patch is a conservative patch.
In v1, I went with "-exec ls -dils {} +".

And, Jeff worries about a lot of flags passed to ls may run into
compatibility issue.

> 
>     Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> diff --git a/t/t7063-status-untracked-cache.sh
> b/t/t7063-status-untracked-cache.sh
> index 190ae149cf3c..ab7e8b5fea01 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -18,7 +18,12 @@ GIT_FORCE_UNTRACKED_CACHE=true
>  export GIT_FORCE_UNTRACKED_CACHE
> 
>  sync_mtime () {
> -       find . -type d -ls >/dev/null
> +       if test_have_prereq BUSYBOX
> +       then
> +               find . -type d -print0 | xargs -0r ls -ld >/dev/null

Can we just change back to what Duy proposed time ago:

	find . -type d -exec ls -ld {} \;

> +       else
> +               find . -type d -ls >/dev/null
> +       fi
>  }
> 
>  avoid_racy() {
> -- snap --
> 
> I have this in Git for Windows' fork, although I have to admit that there
> is no CI set up to verify that this is all working as I expect it to.

I'd find some time (later) to setup a Travis build with Alpine and VoidLinux,
mainly for musl check.

-- 
Danh

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Junio C Hamano]

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

> diff --git a/t/t7063-status-untracked-cache.sh
> b/t/t7063-status-untracked-cache.sh
> index 190ae149cf3c..ab7e8b5fea01 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -18,7 +18,12 @@ GIT_FORCE_UNTRACKED_CACHE=true
>  export GIT_FORCE_UNTRACKED_CACHE
>
>  sync_mtime () {
> -       find . -type d -ls >/dev/null
> +       if test_have_prereq BUSYBOX
> +       then
> +               find . -type d -print0 | xargs -0r ls -ld >/dev/null
> +       else
> +               find . -type d -ls >/dev/null
> +       fi

Given that this is only to work around the lazy mtime sync found on
BSDs, if we were to have any if/then/else, shouldn't we be rather
doing

	if test_have_prereq NEEDS_SYNC_MTIME_BECAUSE_WE_ARE_ON_BSD
	then
		find . -type d -ls >/dev/null
	fi

to make it a no-op for most everybody (including Windows)?

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Junio C Hamano]

Danh Doan <congdanhqx@gmail.com> writes:

> Can we just change back to what Duy proposed time ago:
>
> 	find . -type d -exec ls -ld {} \;

Yup, that sounds just as conservative as it can get.  On platforms
that are not FreeBSD, anything done in this helper function is an
expensive noop, but provided that the working tree is small enough
and the overhead would not matter, something very simple like the
above sounds like the way to go.  If the overhead is too much, on
the other hand, then we can do it only conditionally on certain
filesystems where the issue exists.

> I'd find some time (later) to setup a Travis build with Alpine and VoidLinux,
> mainly for musl check.

Thanks.

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Junio C Hamano]

Danh Doan <congdanhqx@gmail.com> writes:

> A comment in test-lib-functions::test_cmp mentions that
> there _is_ a diff out there that doesn't understand "-u".

That came from 82ebb0b6 (add test_cmp function for test scripts,
2008-03-12).

The change history at the end of the page:

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html

suggests that POSIX did not require "diff -u" until Issue 7, so it
is not surprising that the lack of "diff -u" to cause test_cmp to
fail was of real concern back in March 2008, as the application of
the errata have been only an year and half old, according to:

  https://www.opengroup.org/austin/docs/austin_325.txt

I vaguely recall that some open source projects only took the copied
context diffs and not unified ones the last time I checked, but
admittedly (1) that "last time" was a long time ago and (2) our
popularity and the fact that we do not understand copied context
[*1*] may have forced people to move away from "-c" and adopt "-u"
at the same time.  

So it might be OK to write off any system that does not understand
"diff -u" as an unusable trash these days ;-)

IOW, I think I am fine with assuming "diff -u" is available, like
Dscho's patch does.

Thanks.


[Footnote]

*1* It used to be my desire to teach "git apply" and "git diff" to
    also work with copied context format, but procrastination made
    it less and less relevant X-<.

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Danh Doan]

On 2020-03-23 13:50:48-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Danh Doan <congdanhqx@gmail.com> writes:
> 
> > A comment in test-lib-functions::test_cmp mentions that
> > there _is_ a diff out there that doesn't understand "-u".
> 
> That came from 82ebb0b6 (add test_cmp function for test scripts,
> 2008-03-12).
> 
> The change history at the end of the page:
> 
>   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
> 
> suggests that POSIX did not require "diff -u" until Issue 7, so it
> is not surprising that the lack of "diff -u" to cause test_cmp to
> fail was of real concern back in March 2008, as the application of
> the errata have been only an year and half old, according to:
> 
>   https://www.opengroup.org/austin/docs/austin_325.txt
> 
> I vaguely recall that some open source projects only took the copied
> context diffs and not unified ones the last time I checked, but
> admittedly (1) that "last time" was a long time ago and (2) our
> popularity and the fact that we do not understand copied context
> [*1*] may have forced people to move away from "-c" and adopt "-u"
> at the same time.  
> 
> So it might be OK to write off any system that does not understand
> "diff -u" as an unusable trash these days ;-)

From the setting in "config.mak.uname", it's likely those systems
don't understand "diff -u":

- Solaris 5.6, 5.7, 5.8, 5.9: both of them are un-supported version
  by their vendor
- AIX, only AIX 5.3 doesn't understand "-u",
  the end-of-support AIX 6.1's diff(1) understand "-u" (1)
- HP-UX: From what I can collect, HP-UX still conforms to UNIX-03, and
  its diff(1) doesn't understand "-u"

Hence, if we're going to drop support for system that doesn't
understand "diff -u", we're going to:
- remove support for those variables:
  + GIT_TEST_CMP_USE_COPIED_CONTEXT
  + GIT_TEST_CMP
- drop support for:
  + Solaris 5.{6,7,8,} and AIX 5.3, which will be fine
  + HP-UX: which needs to be discussed

[1]: https://public.dhe.ibm.com/systems/power/docs/aix/61/aixcmds2_pdf.pdf page 133(143)

-- 
Danh

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Junio C Hamano]

Danh Doan <congdanhqx@gmail.com> writes:

> Hence, if we're going to drop support for system that doesn't
> understand "diff -u", we're going to:
> - remove support for those variables:
>   + GIT_TEST_CMP_USE_COPIED_CONTEXT

Folks who prefer "-c" even their platforms all are capable of "-u"
may miss this, but I think that is a very small minority, and they
would be OK (it is after all only needed when diagnosing test
failures of our test suite, and by definition, those who are working
to improve Git would know how to read "-u" output).

>   + GIT_TEST_CMP

I am not sure why you need to drop this one?  This is more about
switching between "diff" and "cmp", and there are reasons why folks
prefer latter especially when they are not debugging the tests.

> - drop support for:
>   + Solaris 5.{6,7,8,} and AIX 5.3, which will be fine
>   + HP-UX: which needs to be discussed
>
> [1]: https://public.dhe.ibm.com/systems/power/docs/aix/61/aixcmds2_pdf.pdf page 133(143)

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Johannes Schindelin]

Hi,

On Tue, 24 Mar 2020, Danh Doan wrote:

> On 2020-03-23 13:50:48-0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Danh Doan <congdanhqx@gmail.com> writes:
> >
> > > A comment in test-lib-functions::test_cmp mentions that
> > > there _is_ a diff out there that doesn't understand "-u".
> >
> > That came from 82ebb0b6 (add test_cmp function for test scripts,
> > 2008-03-12).
> >
> > The change history at the end of the page:
> >
> >   https://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
> >
> > suggests that POSIX did not require "diff -u" until Issue 7, so it
> > is not surprising that the lack of "diff -u" to cause test_cmp to
> > fail was of real concern back in March 2008, as the application of
> > the errata have been only an year and half old, according to:
> >
> >   https://www.opengroup.org/austin/docs/austin_325.txt
> >
> > I vaguely recall that some open source projects only took the copied
> > context diffs and not unified ones the last time I checked, but
> > admittedly (1) that "last time" was a long time ago and (2) our
> > popularity and the fact that we do not understand copied context
> > [*1*] may have forced people to move away from "-c" and adopt "-u"
> > at the same time.
> >
> > So it might be OK to write off any system that does not understand
> > "diff -u" as an unusable trash these days ;-)
>
> From the setting in "config.mak.uname", it's likely those systems
> don't understand "diff -u":
>
> - Solaris 5.6, 5.7, 5.8, 5.9: both of them are un-supported version
>   by their vendor
> - AIX, only AIX 5.3 doesn't understand "-u",
>   the end-of-support AIX 6.1's diff(1) understand "-u" (1)
> - HP-UX: From what I can collect, HP-UX still conforms to UNIX-03, and
>   its diff(1) doesn't understand "-u"
>
> Hence, if we're going to drop support for system that doesn't
> understand "diff -u", we're going to:
> - remove support for those variables:
>   + GIT_TEST_CMP_USE_COPIED_CONTEXT
>   + GIT_TEST_CMP
> - drop support for:
>   + Solaris 5.{6,7,8,} and AIX 5.3, which will be fine
>   + HP-UX: which needs to be discussed
>
> [1]: https://public.dhe.ibm.com/systems/power/docs/aix/61/aixcmds2_pdf.pdf page 133(143)

With this explanation, I totally agree that your patch is better than
mine.

Thanks,
Dscho

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Johannes Schindelin]

Hi Junio,

On Mon, 23 Mar 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > diff --git a/t/t7063-status-untracked-cache.sh
> > b/t/t7063-status-untracked-cache.sh
> > index 190ae149cf3c..ab7e8b5fea01 100755
> > --- a/t/t7063-status-untracked-cache.sh
> > +++ b/t/t7063-status-untracked-cache.sh
> > @@ -18,7 +18,12 @@ GIT_FORCE_UNTRACKED_CACHE=true
> >  export GIT_FORCE_UNTRACKED_CACHE
> >
> >  sync_mtime () {
> > -       find . -type d -ls >/dev/null
> > +       if test_have_prereq BUSYBOX
> > +       then
> > +               find . -type d -print0 | xargs -0r ls -ld >/dev/null
> > +       else
> > +               find . -type d -ls >/dev/null
> > +       fi
>
> Given that this is only to work around the lazy mtime sync found on
> BSDs, if we were to have any if/then/else, shouldn't we be rather
> doing
>
> 	if test_have_prereq NEEDS_SYNC_MTIME_BECAUSE_WE_ARE_ON_BSD
> 	then
> 		find . -type d -ls >/dev/null
> 	fi
>
> to make it a no-op for most everybody (including Windows)?

I like that approach.

Thank you,
Dscho

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Junio C Hamano]

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

> With this explanation, I totally agree that your patch is better than
> mine.

Meaning that we'd have a fallback checker that understands "diff -u"
output, because the presense of "diff -u" cannot be relied upon and
we'd need to keep the original that understands "diff -c" anyway?

If that is what you meant, I am 100% fine with that.

Re: [PATCH v2 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Junio C Hamano]

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

> Hi Junio,
>
> On Mon, 23 Mar 2020, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > diff --git a/t/t7063-status-untracked-cache.sh
>> > b/t/t7063-status-untracked-cache.sh
>> > index 190ae149cf3c..ab7e8b5fea01 100755
>> > --- a/t/t7063-status-untracked-cache.sh
>> > +++ b/t/t7063-status-untracked-cache.sh
>> > @@ -18,7 +18,12 @@ GIT_FORCE_UNTRACKED_CACHE=true
>> >  export GIT_FORCE_UNTRACKED_CACHE
>> >
>> >  sync_mtime () {
>> > -       find . -type d -ls >/dev/null
>> > +       if test_have_prereq BUSYBOX
>> > +       then
>> > +               find . -type d -print0 | xargs -0r ls -ld >/dev/null
>> > +       else
>> > +               find . -type d -ls >/dev/null
>> > +       fi
>>
>> Given that this is only to work around the lazy mtime sync found on
>> BSDs, if we were to have any if/then/else, shouldn't we be rather
>> doing
>>
>> 	if test_have_prereq NEEDS_SYNC_MTIME_BECAUSE_WE_ARE_ON_BSD
>> 	then
>> 		find . -type d -ls >/dev/null
>> 	fi
>>
>> to make it a no-op for most everybody (including Windows)?
>
> I like that approach.

I actually think I was being half stupid --- what are we going to
tell those who want a stripped down system with busybox based on
BSD?  I think the condition for "if" is OK (as long as we have a
real prereq suitable for the approach), but the actual "let's make
sure the inodes are stat(2)ed" should avoid relying on "find -ls".

If we can assume that a typical busybox build offers find with -print0
and xargs with -0, I think the version you sent is probably the best
(with or without "do so only on BSD" I suggested) approach.

Thanks.

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Danh Doan]

On 2020-03-24 11:47:32-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Danh Doan <congdanhqx@gmail.com> writes:
> 
> > Hence, if we're going to drop support for system that doesn't
> > understand "diff -u", we're going to:
> > - remove support for those variables:
> >   + GIT_TEST_CMP_USE_COPIED_CONTEXT
> 
> Folks who prefer "-c" even their platforms all are capable of "-u"
> may miss this, but I think that is a very small minority, and they
> would be OK (it is after all only needed when diagnosing test
> failures of our test suite, and by definition, those who are working
> to improve Git would know how to read "-u" output).
> 
> >   + GIT_TEST_CMP
> 
> I am not sure why you need to drop this one?  This is more about
> switching between "diff" and "cmp", and there are reasons why folks
> prefer latter especially when they are not debugging the tests.

I was thinking it could simplify the test_cmp code.
There was a problem which needs to be addressed by 2/8.

Yes, people may prefer to use cmp(1) because cmp(1) should be faster
than diff(1).

Anyway, it seems like we've decided to keep using normal-diff because
of HP-UX.

-- 
Danh

Re: [PATCH v2 7/8] t4124: fix test for non-compliant diff(1)

[Johannes Schindelin]

Hi Junio,

On Tue, 24 Mar 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > With this explanation, I totally agree that your patch is better than
> > mine.
>
> Meaning that we'd have a fallback checker that understands "diff -u"
> output, because the presense of "diff -u" cannot be relied upon and
> we'd need to keep the original that understands "diff -c" anyway?
>
> If that is what you meant, I am 100% fine with that.

Yes, that's what I meant. Thanks for clearing this up!

Ciao,
Dscho

[PATCH v4 0/8] fix test failure with busybox

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

Please ignore v3, there is an incorrect change in t4124.
I found it when working on travis for linux-musl

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

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

Đoàn Trần Công Danh (8):
  t4061: use POSIX compliant regex(7)
  test-lib-functions: test_cmp: eval $GIT_TEST_CMP
  t5003: drop the subshell in test_lazy_prereq
  t5003: skip conversion test if unzip -a is unavailable
  t5616: use rev-parse instead to get HEAD's object_id
  t7063: drop non-POSIX argument "-ls" from find(1)
  t4124: fix test for non-compliant diff(1)
  t5703: feed raw data into test-tool unpack-sideband

 t/helper/test-pkt-line.c           |  2 +-
 t/t4061-diff-indent.sh             |  2 +-
 t/t4124-apply-ws-rule.sh           |  6 ++++++
 t/t5003-archive-zip.sh             | 24 ++++++++++++------------
 t/t5616-partial-clone.sh           |  2 +-
 t/t5703-upload-pack-ref-in-want.sh |  5 +----
 t/t7063-status-untracked-cache.sh  |  2 +-
 t/test-lib-functions.sh            |  2 +-
 8 files changed, 24 insertions(+), 21 deletions(-)

Range-diff against v3:
 -:  ---------- >  1:  780308d060 t3404: use test_cmp_rev
 -:  ---------- >  2:  5b7a64df40 cherry-pick: add test for `--skip` advice in `git commit`
 -:  ---------- >  3:  f028d661c7 cherry-pick: check commit error messages
 -:  ---------- >  4:  21b11c6d1d sequencer: write CHERRY_PICK_HEAD for reword and edit
 -:  ---------- >  5:  8d57f75749 commit: use enum value for multiple cherry-picks
 -:  ---------- >  6:  901ba7b1ef commit: encapsulate determine_whence() for sequencer
 -:  ---------- >  7:  430b75f720 commit: give correct advice for empty commit during a rebase
 -:  ---------- >  8:  0b4396f068 git-p4: make python2.7 the oldest supported version
 -:  ---------- >  9:  484d09c303 git-p4: change the expansion test from basestring to list
 -:  ---------- > 10:  1f8b46d0a4 git-p4: remove string type aliasing
 -:  ---------- > 11:  6cec21a82f git-p4: encode/decode communication with p4 for python3
 -:  ---------- > 12:  86dca24b7b git-p4: encode/decode communication with git for python3
 -:  ---------- > 13:  d38208a297 git-p4: convert path to unicode before processing them
 -:  ---------- > 14:  5a5577d808 git-p4: open .gitp4-usercache.txt in text mode
 -:  ---------- > 15:  50da1e7393 git-p4: use marshal format version 2 when sending to p4
 -:  ---------- > 16:  4294d741cc git-p4: fix freezing while waiting for fast-import progress
 -:  ---------- > 17:  a6b1306735 git-p4: use functools.reduce instead of reduce
 -:  ---------- > 18:  2e2aa8d903 git-p4: use dict.items() iteration for python3 compatibility
 -:  ---------- > 19:  ce425eb4e1 git-p4: simplify regex pattern generation for parsing diff-tree
 -:  ---------- > 20:  7575f4fdec git-p4: use python3's input() everywhere
 -:  ---------- > 21:  06ac2b3b6e advice: extract vadvise() from advise()
 -:  ---------- > 22:  fef0c76f18 advice: change "setupStreamFailure" to "setUpstreamFailure"
 -:  ---------- > 23:  b3b18d1621 advice: revamp advise API
 -:  ---------- > 24:  f665d63a91 tag: use new advice API to check visibility
 -:  ---------- > 25:  88238e02d5 http: add client cert support for HTTPS proxies
 -:  ---------- > 26:  af026519c9 http: add environment variable support for HTTPS proxies
 -:  ---------- > 27:  0915a5b4cd set_git_dir: fix crash when used with real_path()
 -:  ---------- > 28:  564956f358 update how-to-maintain-git
 -:  ---------- > 29:  6bb40ed20a ci: use python3 in linux-gcc and osx-gcc and python2 elsewhere
 -:  ---------- > 30:  3d7747e318 real_path: remove unsafe API
 -:  ---------- > 31:  4530a85b4c real_path_if_valid(): remove unsafe API
 -:  ---------- > 32:  49d3c4b481 get_superproject_working_tree(): return strbuf
 -:  ---------- > 33:  7422b2a0a1 commit-slab: clarify slabname##_peek()'s return value
 -:  ---------- > 34:  9c688735f6 pull: document more passthru options
 -:  ---------- > 35:  b739d971e5 connected.c: reprepare packs for corner cases
 -:  ---------- > 36:  757c2ba3e2 oidset: remove unnecessary include
 -:  ---------- > 37:  8312aa7d74 separate tar.* config to its own source file
 -:  ---------- > 38:  d00a5bdd50 submodule--helper.c: Rename 'cb_foreach' to 'foreach_cb'
 -:  ---------- > 39:  a7d14a4428 The first batch post 2.26 cycle
 1:  34f96548de = 40:  08839cac15 t4061: use POSIX compliant regex(7)
 2:  50f46986a6 = 41:  31d4a9b2e8 test-lib-functions: test_cmp: eval $GIT_TEST_CMP
 3:  8719a07753 = 42:  f871a7f395 t5003: drop the subshell in test_lazy_prereq
 4:  457eecaf9b = 43:  bc6bcc786d t5003: skip conversion test if unzip -a is unavailable
 5:  d3bc855e17 = 44:  c6f7498d93 t5616: use rev-parse instead to get HEAD's object_id
 6:  64472ec3bc ! 45:  b3acf4adcf t7063: drop non-POSIX argument "-ls" from find(1)
    @@ Commit message
         However, `-ls` flag isn't required by POSIX's find(1), and
         busybox(1) doesn't implement it.
     
    -    From the original conversation, it seems like find(1) with "-type d"
    -    could trigger enough "lstat(2)" to ask FreeBSD update mtime.
    -
    -    Use only filter "-type d" for now.
    +    Use "-exec ls -ld {} +" instead.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
    @@ t/t7063-status-untracked-cache.sh: GIT_FORCE_UNTRACKED_CACHE=true
      
      sync_mtime () {
     -	find . -type d -ls >/dev/null
    -+	find . -type d >/dev/null
    ++	find . -type d -exec ls -ld {} + >/dev/null
      }
      
      avoid_racy() {
 7:  51df6dd12d ! 46:  02c115e93a t4124: fix test for non-compliant diff(1)
    @@ Commit message
         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.
    +    HP-UX's diff(1) doesn't understand "-u" as of now.
     
    -    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
    +    Workaround this problem by checking "diff -u" output,
         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: test_fix () {
      
      	# find touched lines
      	$DIFF file target | sed -n -e "s/^> //p" >fixed
    -+	# busybox's diff(1) output unified format
    ++	# busybox's diff(1) doesn't output normal format
     +	if ! test -s fixed; then
    -+		$DIFF file target |
    ++		$DIFF -u file target |
     +		grep -v '^+++ target' |
    -+		sed -e "/^+/s/+//" >fixed
    ++		sed -ne "/^+/s/+//p" >fixed
     +	fi
      
      	# the changed lines are all expected to change
 8:  843c7f66d9 = 47:  2b8e75aaa5 t5703: feed raw data into test-tool unpack-sideband
-- 
2.26.0.rc2.357.g1e1ba0441d

[PATCH v4 1/8] t4061: use POSIX compliant 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

We have 2 options here:

- Using literal `+` because BRE will interpret it as-is, or
- Using character class `[+]` to defend against a sed that expects ERE

ERE-expected sed is theoretical at this point,
but we haven't found it, yet.
And, we may run into other problems with that sed.
Let's go with first option and fix it later if that sed could be found.

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.357.g1e1ba0441d

[PATCH v4 2/8] 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.
With /bin/sh linked to either /bin/bash or /bin/dash,
`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.357.g1e1ba0441d

[PATCH v4 3/8] t5003: drop the subshell in test_lazy_prereq

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

test_lazy_prereq will be evaluated in a throw-away directory.

Drop unnecessary subshell and mkdir.

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

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 106eddbd85..df1374a312 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -7,12 +7,8 @@ test_description='git archive --format=zip test'
 SUBSTFORMAT=%H%n
 
 test_lazy_prereq UNZIP_SYMLINKS '
-	(
-		mkdir unzip-symlinks &&
-		cd unzip-symlinks &&
-		"$GIT_UNZIP" "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip &&
-		test -h symlink
-	)
+	"$GIT_UNZIP" "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip &&
+	test -h symlink
 '
 
 check_zip() {
-- 
2.26.0.rc2.357.g1e1ba0441d

[PATCH v4 4/8] 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 | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index df1374a312..3b76d2eb65 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -11,6 +11,10 @@ test_lazy_prereq UNZIP_SYMLINKS '
 	test -h symlink
 '
 
+test_lazy_prereq UNZIP_CONVERT '
+	"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-symlinks.zip
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst
@@ -35,33 +39,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.357.g1e1ba0441d

[PATCH v4 5/8] 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..09e640cae4 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 --verify HEAD >headhash &&
 	grep "$(cat headhash) HEAD" $(cat promisorlist) &&
 	grep "$(cat headhash) refs/heads/master" $(cat promisorlist)
 '
-- 
2.26.0.rc2.357.g1e1ba0441d

[PATCH v4 6/8] t7063: drop non-POSIX argument "-ls" from find(1)

[Đ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 "-exec ls -ld {} +" 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..6738497ea7 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 -ld {} + >/dev/null
 }
 
 avoid_racy() {
-- 
2.26.0.rc2.357.g1e1ba0441d

[PATCH v4 7/8] t4124: fix test for non-compliant diff(1)

[Đ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.

HP-UX's diff(1) doesn't understand "-u" as of now.

Workaround this problem by checking "diff -u" output,
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..4a1bb141d9 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
+	# busybox's diff(1) doesn't output normal format
+	if ! test -s fixed; then
+		$DIFF -u file target |
+		grep -v '^+++ target' |
+		sed -ne "/^+/s/+//p" >fixed
+	fi
 
 	# the changed lines are all expected to change
 	fixed_cnt=$(wc -l <fixed)
-- 
2.26.0.rc2.357.g1e1ba0441d

[PATCH v4 8/8] t5703: feed raw data into test-tool unpack-sideband

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

busybox's sed isn't binary clean.
Thus, triggers false-negative on this test.

We could replace sed with perl on this usecase.
But, we could slightly modify the helper to discard unwanted data in the
beginning.

Fix the false negative by updating this helper.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/helper/test-pkt-line.c           | 2 +-
 t/t5703-upload-pack-ref-in-want.sh | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

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
-- 
2.26.0.rc2.357.g1e1ba0441d

Re: [PATCH v4 7/8] t4124: fix test for non-compliant diff(1)

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> +	# busybox's diff(1) doesn't output normal format
> +	if ! test -s fixed; then

I've touched this line up and tweaked the log message while
applying, as I didn't see anything else that is iffy and wanted to
save one round-trip.  Please see what's pushed out in perhaps a few
hours (and please complain loudly if you don't like it), so that we
can merge it down to 'next' soonish.

Thanks.