threads / patch / 53053

v2, 8 partsfix test failure with busybox

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

## tl;dr

36 messages between Mar 22, 2020 and Mar 27, 2020. Diffs are folded; open one to read it.

replies: 35people: 5as markdown or json

Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · lore

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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v2 1/8] t4061: use POSIX compliant regex(7)

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

Show 5 quoted lines
> 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(-)
Show changes to t/t4061-diff-indent.sh +1 −1
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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/t5003-archive-zip.sh +2 −6
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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/test-lib-functions.sh +1 −1
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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v2 4/8] t5003: skip conversion test if unzip -a is unavailable

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(-)
Show changes to t/t5003-archive-zip.sh +10 −6
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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v2 5/8] t5616: use rev-parse instead to get HEAD's object_id

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(-)
Show changes to t/t5616-partial-clone.sh +1 −1
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
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v2 6/8] 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 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(-)
Show changes to t/t7063-status-untracked-cache.sh +1 −1
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
Johannes Schindelin· Mar 23, 2020, 14:11 UTC · re: Đoàn Trần Công Danh · lore

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

Hi,
On Sun, 22 Mar 2020, Đoàn Trần Công Danh wrote:
Show 9 quoted lines
> 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?

Show 17 quoted lines
> 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>
Show changes to t/t7063-status-untracked-cache.sh +6 −2
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
>
>
>
Torsten Bögershausen· Mar 23, 2020, 14:37 UTC · re: Johannes Schindelin · lore

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

On Mon, Mar 23, 2020 at 03:11:50PM +0100, Johannes Schindelin wrote:
Show 16 quoted lines
> 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]
Danh Doan· Mar 23, 2020, 15:11 UTC · re: Johannes Schindelin · lore

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

On 2020-03-23 15:11:50+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 46 quoted lines
> 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.

Show 16 quoted lines
> 
>     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 {} \;
Show 10 quoted lines
> +       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
Junio C Hamano· Mar 23, 2020, 20:30 UTC · re: Danh Doan · lore

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

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.
Junio C Hamano· Mar 23, 2020, 15:55 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 16 quoted lines
> 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)?
Johannes Schindelin· Mar 24, 2020, 22:31 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Mon, 23 Mar 2020, Junio C Hamano wrote:
Show 29 quoted lines
> 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

Junio C Hamano· Mar 24, 2020, 23:46 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 35 quoted lines
> 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.
Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v2 7/8] 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.

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(+)
Show changes to t/t4124-apply-ws-rule.sh +6 −0
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
Johannes Schindelin· Mar 23, 2020, 13:58 UTC · re: Đoàn Trần Công Danh · lore

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

Hi,
On Sun, 22 Mar 2020, Đoàn Trần Công Danh wrote:
Show 30 quoted lines
> 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>
Show changes to t/t4124-apply-ws-rule.sh +1 −2
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
>
>
>
Danh Doan· Mar 23, 2020, 15:04 UTC · re: Johannes Schindelin · lore

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

On 2020-03-23 14:58:13+0100, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 76 quoted lines
> 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
Junio C Hamano· Mar 23, 2020, 20:50 UTC · re: Danh Doan · lore

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

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-<.
Danh Doan· Mar 24, 2020, 03:40 UTC · re: Junio C Hamano · lore

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

On 2020-03-23 13:50:48-0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> 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
Junio C Hamano· Mar 24, 2020, 18:47 UTC · re: Danh Doan · lore

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

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.

Show 5 quoted lines
> - 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 Doan· Mar 25, 2020, 14:24 UTC · re: Junio C Hamano · lore

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

On 2020-03-24 11:47:32-0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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
Johannes Schindelin· Mar 24, 2020, 22:29 UTC · re: Danh Doan · lore

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

Hi,
On Tue, 24 Mar 2020, Danh Doan wrote:
Show 50 quoted lines
> 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

Junio C Hamano· Mar 24, 2020, 23:37 UTC · re: Johannes Schindelin · lore

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

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.
Johannes Schindelin· Mar 25, 2020, 18:23 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Tue, 24 Mar 2020, Junio C Hamano wrote:
Show 10 quoted lines
> 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

Đoàn Trần Công Danh· Mar 22, 2020, 00:55 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to 2 files +2 −5

t/helper/test-pkt-line.c, t/t5703-upload-pack-ref-in-want.sh

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
Jeff King· Mar 22, 2020, 06:08 UTC · re: Đoàn Trần Công Danh · lore

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

On Sun, Mar 22, 2020 at 07:55:09AM +0700, Đoàn Trần Công Danh wrote:
Show 9 quoted lines
> Đ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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

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

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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v4 1/8] t4061: use POSIX compliant regex(7)

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

Show 5 quoted lines
> 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(-)
Show changes to t/t4061-diff-indent.sh +1 −1
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/test-lib-functions.sh +1 −1
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to t/t5003-archive-zip.sh +2 −6
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v4 4/8] t5003: skip conversion test if unzip -a is unavailable

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(-)
Show changes to t/t5003-archive-zip.sh +10 −6
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v4 5/8] t5616: use rev-parse instead to get HEAD's object_id

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(-)
Show changes to t/t5616-partial-clone.sh +1 −1
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v4 7/8] 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.

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(+)
Show changes to t/t4124-apply-ws-rule.sh +6 −0
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
Junio C Hamano· Mar 27, 2020, 01:18 UTC · re: Đoàn Trần Công Danh · lore

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

Đ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.
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

[PATCH v4 6/8] 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 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(-)
Show changes to t/t7063-status-untracked-cache.sh +1 −1
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
Đoàn Trần Công Danh· Mar 26, 2020, 04:37 UTC · re: Đoàn Trần Công Danh · lore

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

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(-)
Show changes to 2 files +2 −5

t/helper/test-pkt-line.c, t/t5703-upload-pack-ref-in-want.sh

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

← back to recent threads