[PATCH 0/6] test: make the test suite work with zsh

[PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

It's not difficult to make the testing library work for zsh, so I did that in
the first patch.

The rest of the patches are basically to deal with some variables that are
special in zsh, workaround a bug, and a minor discrepancy.

Felipe Contreras (6):
  test: fix build for zsh
  test: avoid `stat` variable
  test: avoid `options` variable
  test: avoid `path` variable
  test: hack for zsh
  mergetools: vimdiff: check for empty fields

 mergetools/vimdiff               |  4 +--
 t/annotate-tests.sh              | 10 +++----
 t/lib-bash.sh                    |  2 +-
 t/t0001-init.sh                  |  4 +--
 t/t0003-attributes.sh            | 16 +++++------
 t/t1450-fsck.sh                  | 16 +++++------
 t/t3305-notes-fanout.sh          | 12 ++++----
 t/t3432-rebase-fast-forward.sh   |  4 +--
 t/t4013-diff-various.sh          |  6 ++--
 t/t4046-diff-unmerged.sh         | 49 ++++++++++++++++----------------
 t/t4051-diff-function-context.sh |  4 +--
 t/t5329-pack-objects-cruft.sh    |  8 +++---
 t/t5512-ls-remote.sh             |  4 +--
 t/t5516-fetch-push.sh            |  8 +++---
 t/t9300-fast-import.sh           |  4 +--
 t/test-lib.sh                    | 24 ++++++++++------
 16 files changed, 92 insertions(+), 83 deletions(-)

-- 
2.39.2.13.g1fb56cf030

[PATCH 1/6] test: fix build for zsh

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/lib-bash.sh |  2 +-
 t/test-lib.sh | 20 ++++++++++++++------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index b0b6060929..798b6d0265 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -10,7 +10,7 @@ elif type bash >/dev/null 2>&1
 then
 	# execute in full-on bash mode
 	unset POSIXLY_CORRECT
-	exec bash "$0" "$@"
+	exec bash "${ZSH_ARGZERO-$0}" "$@"
 else
 	echo '1..0 #SKIP skipping bash completion tests; bash not available'
 	exit 0
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 62136caee5..cc705df981 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,6 +15,14 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .
 
+if test -n "${ZSH_VERSION-}"
+then
+	emulate sh
+	ARGZERO="$ZSH_ARGZERO"
+else
+	ARGZERO="$0"
+fi
+
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
@@ -310,7 +318,7 @@ then
 fi
 
 TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
-TEST_NAME="$(basename "$0" .sh)"
+TEST_NAME="$(basename "$ARGZERO" .sh)"
 TEST_NUMBER="${TEST_NAME%%-*}"
 TEST_NUMBER="${TEST_NUMBER#t}"
 TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
@@ -388,7 +396,7 @@ then
 			      { test -z "$stress_limit" ||
 				test $cnt -le $stress_limit ; }
 			do
-				$TEST_SHELL_PATH "$0" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
+				$TEST_SHELL_PATH "$ARGZERO" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
 				test_pid=$!
 
 				if wait $test_pid
@@ -442,7 +450,7 @@ then
 	# from any previous runs.
 	>"$GIT_TEST_TEE_OUTPUT_FILE"
 
-	(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
+	(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$ARGZERO" "$@" 2>&1;
 	 echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
 	test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
 	exit
@@ -467,7 +475,7 @@ then
 	then
 		: Executed by a Bash version supporting BASH_XTRACEFD.  Good.
 	else
-		echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+		echo >&2 "warning: ignoring -x; '$ARGZERO' is untraceable without BASH_XTRACEFD"
 		trace=
 	fi
 fi
@@ -1595,7 +1603,7 @@ fi
 
 if test "${GIT_TEST_CHAIN_LINT:-1}" != 0
 then
-	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" ||
+	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$ARGZERO" ||
 		BUG "lint error (see '?!...!? annotations above)"
 fi
 
@@ -1637,7 +1645,7 @@ fi
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
 
-start_test_output "$0"
+start_test_output "$ARGZERO"
 
 # Convenience
 # A regexp to match 5 and 35 hexdigits
-- 
2.39.2.13.g1fb56cf030

[PATCH 2/6] test: avoid `stat` variable

[Felipe Contreras]

It has special meaning in zsh.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t0001-init.sh                | 4 ++--
 t/t3432-rebase-fast-forward.sh | 4 ++--
 t/t4013-diff-various.sh        | 6 +++---
 t/t5512-ls-remote.sh           | 4 ++--
 t/t5516-fetch-push.sh          | 8 ++++----
 t/test-lib.sh                  | 4 ++--
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 30a6edca1d..88c9de8f53 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -362,12 +362,12 @@ test_lazy_prereq GETCWD_IGNORES_PERMS '
 		cd $base/dir &&
 		test-tool getcwd
 	)
-	status=$?
+	stat=$?
 
 	chmod 700 $base &&
 	rm -rf $base ||
 	BUG "cannot clean $base"
-	return $status
+	return $stat
 '
 
 check_long_base_path () {
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 7f1a5dd3de..49ecf8f18b 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -41,7 +41,7 @@ test_rebase_same_head () {
 }
 
 test_rebase_same_head_ () {
-	status="$1" &&
+	stat="$1" &&
 	shift &&
 	what="$1" &&
 	shift &&
@@ -57,7 +57,7 @@ test_rebase_same_head_ () {
 	else
 		msg="git rebase$flag $* with $changes is $what with $cmp HEAD"
 	fi &&
-	test_expect_$status "$msg" "
+	test_expect_$stat "$msg" "
 		if test $abbreviate -eq 1
 		then
 			test_config rebase.abbreviateCommands true
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5de1d19075..8f82946c2f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -178,7 +178,7 @@ process_diffs () {
 V=$(git version | sed -e 's/^git version //' -e 's/\./\\./g')
 while read magic cmd
 do
-	status=success
+	stat=success
 	case "$magic" in
 	'' | '#'*)
 		continue ;;
@@ -188,7 +188,7 @@ do
 		case "$magic" in
 		noellipses) ;;
 		failure)
-			status=failure
+			stat=failure
 			magic=
 			label="$cmd" ;;
 		*)
@@ -203,7 +203,7 @@ do
 	expect="$TEST_DIRECTORY/t4013/diff.$test"
 	actual="$pfx-diff.$test"
 
-	test_expect_$status "git $cmd # magic is ${magic:-(not used)}" '
+	test_expect_$stat "git $cmd # magic is ${magic:-(not used)}" '
 		{
 			echo "$ git $cmd"
 			case "$magic" in
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 20d063fb9a..88f0f81baf 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -167,9 +167,9 @@ test_expect_success 'confuses pattern as remote when no remote specified' '
 test_expect_success 'die with non-2 for wrong repository even with --exit-code' '
 	{
 		git ls-remote --exit-code ./no-such-repository
-		status=$?
+		stat=$?
 	} &&
-	test $status != 2 && test $status != 0
+	test $stat != 2 && test $stat != 0
 '
 
 test_expect_success 'Report success even when nothing matches' '
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 19ebefa5ac..ed8b676156 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1732,9 +1732,9 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 
 	git update-index -q --refresh &&
 	git read-tree -u -m HEAD "$1" || {
-		status=$?
+		stat=$?
 		echo >&2 read-tree failed
-		exit $status
+		exit $stat
 	}
 	EOF
 
@@ -1800,9 +1800,9 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
 	no)
 		git read-tree -u -m "$1" ;;
 	esac || {
-		status=$?
+		stat=$?
 		echo >&2 read-tree failed
-		exit $status
+		exit $stat
 	}
 	EOF
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index cc705df981..621a10941d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1866,12 +1866,12 @@ test_lazy_prereq SANITY '
 
 	! test -r SANETESTD.1/x &&
 	! rm SANETESTD.1/x && ! test -f SANETESTD.2/x
-	status=$?
+	stat=$?
 
 	chmod +rwx SANETESTD.1 SANETESTD.2 &&
 	rm -rf SANETESTD.1 SANETESTD.2 ||
 	BUG "cannot clean SANETESTD"
-	return $status
+	return $stat
 '
 
 test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
-- 
2.39.2.13.g1fb56cf030

Re: [PATCH 2/6] test: avoid `stat` variable

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 28 2023, Felipe Contreras wrote:

> It has special meaning in zsh.

Nit: If we are going to rename these I'd prefer (just from grepping
existing examples) $exit_code, $code, $ret etc. a "$stat" at a glance
would seem to refer to stat() somehow, which is confusing.

Re: [PATCH 2/6] test: avoid `stat` variable

[Taylor Blau]

On Tue, Mar 28, 2023 at 11:39:28AM -0600, Felipe Contreras wrote:
> ---
>  t/t0001-init.sh                | 4 ++--
>  t/t3432-rebase-fast-forward.sh | 4 ++--
>  t/t4013-diff-various.sh        | 6 +++---
>  t/t5512-ls-remote.sh           | 4 ++--
>  t/t5516-fetch-push.sh          | 8 ++++----
>  t/test-lib.sh                  | 4 ++--
>  6 files changed, 15 insertions(+), 15 deletions(-)

Maybe I'm misreading your patch or its subject line, but all of these
hunks convert from something (usually "status") _to_ "stat". Is the
point to avoid "status", not "stat"? Or the other way around?

Thanks,
Taylor

Re: [PATCH 2/6] test: avoid `stat` variable

[Felipe Contreras]

On Fri, Mar 31, 2023 at 6:05 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Mar 28, 2023 at 11:39:28AM -0600, Felipe Contreras wrote:
> > ---
> >  t/t0001-init.sh                | 4 ++--
> >  t/t3432-rebase-fast-forward.sh | 4 ++--
> >  t/t4013-diff-various.sh        | 6 +++---
> >  t/t5512-ls-remote.sh           | 4 ++--
> >  t/t5516-fetch-push.sh          | 8 ++++----
> >  t/test-lib.sh                  | 4 ++--
> >  6 files changed, 15 insertions(+), 15 deletions(-)
>
> Maybe I'm misreading your patch or its subject line, but all of these
> hunks convert from something (usually "status") _to_ "stat".

No, the title is wrong; the purpose is to avoid `status`, which in zsh
it's linked to $?

-- 
Felipe Contreras

[PATCH 3/6] test: avoid `options` variable

[Felipe Contreras]

It has a special meaning in zsh.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/annotate-tests.sh              | 10 +++++-----
 t/t4051-diff-function-context.sh |  4 ++--
 t/t9300-fast-import.sh           |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index f1b9a6ce4d..3b57d4dbb5 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -15,19 +15,19 @@ fi
 check_count () {
 	head= &&
 	file='file' &&
-	options= &&
+	args= &&
 	while :
 	do
 		case "$1" in
 		-h) head="$2"; shift; shift ;;
 		-f) file="$2"; shift; shift ;;
-		-L*) options="$options $(sanitize_L "$1")"; shift ;;
-		-*) options="$options $1"; shift ;;
+		-L*) args="$args $(sanitize_L "$1")"; shift ;;
+		-*) args="$args $1"; shift ;;
 		*) break ;;
 		esac
 	done &&
-	echo "$PROG $options $file $head" >&4 &&
-	$PROG $options $file $head >actual &&
+	echo "$PROG $args $file $head" >&4 &&
+	$PROG $args $file $head >actual &&
 	perl -e '
 		my %expect = (@ARGV);
 		my %count = map { $_ => 0 } keys %expect;
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 725278ad19..3bf4f0b6b0 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -30,10 +30,10 @@ last_context_line () {
 check_diff () {
 	name=$1
 	desc=$2
-	options="-W $3"
+	args="-W $3"
 
 	test_expect_success "$desc" '
-		git diff $options "$name^" "$name" >"$name.diff"
+		git diff $args "$name^" "$name" >"$name.diff"
 	'
 
 	test_expect_success ' diff applies' '
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index aa55b41b9a..c30503870f 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3222,7 +3222,7 @@ test_expect_success 'U: validate root delete result' '
 # fast-import process is still running using background_import_still_running
 # *after* evaluating the test conditions.
 background_import_then_checkpoint () {
-	options=$1
+	args=$1
 	input_file=$2
 
 	mkfifo V.input
@@ -3234,7 +3234,7 @@ background_import_then_checkpoint () {
 	rm V.output
 
 	(
-		git fast-import $options <&8 >&9 &
+		git fast-import $args <&8 >&9 &
 		echo $! >&9
 		wait $!
 		echo >&2 "background fast-import terminated too early with exit code $?"
-- 
2.39.2.13.g1fb56cf030

[PATCH 4/6] test: avoid `path` variable

[Felipe Contreras]

In zsh it's linked to `PATH`.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t0003-attributes.sh         | 16 ++++++------
 t/t1450-fsck.sh               | 16 ++++++------
 t/t3305-notes-fanout.sh       | 12 ++++-----
 t/t4046-diff-unmerged.sh      | 48 +++++++++++++++++------------------
 t/t5329-pack-objects-cruft.sh |  8 +++---
 5 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 89b306cb11..b956b2fc29 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -7,10 +7,10 @@ TEST_CREATE_REPO_NO_TEMPLATE=1
 . ./test-lib.sh
 
 attr_check_basic () {
-	path="$1" expect="$2" git_opts="$3" &&
+	fpath="$1" expect="$2" git_opts="$3" &&
 
-	git $git_opts check-attr test -- "$path" >actual 2>err &&
-	echo "$path: test: $expect" >expect &&
+	git $git_opts check-attr test -- "$fpath" >actual 2>err &&
+	echo "$fpath: test: $expect" >expect &&
 	test_cmp expect actual
 }
 
@@ -20,18 +20,18 @@ attr_check () {
 }
 
 attr_check_quote () {
-	path="$1" quoted_path="$2" expect="$3" &&
+	fpath="$1" quoted_path="$2" expect="$3" &&
 
-	git check-attr test -- "$path" >actual &&
+	git check-attr test -- "$fpath" >actual &&
 	echo "\"$quoted_path\": test: $expect" >expect &&
 	test_cmp expect actual
 }
 
 attr_check_source () {
-	path="$1" expect="$2" source="$3" git_opts="$4" &&
+	fpath="$1" expect="$2" source="$3" git_opts="$4" &&
 
-	git $git_opts check-attr --source $source test -- "$path" >actual 2>err &&
-	echo "$path: test: $expect" >expect &&
+	git $git_opts check-attr --source $source test -- "$fpath" >actual 2>err &&
+	echo "$fpath: test: $expect" >expect &&
 	test_cmp expect actual &&
 	test_must_be_empty err
 }
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bca46378b2..489e03865c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -589,9 +589,9 @@ test_expect_success 'fsck notices submodule entry pointing to null sha1' '
 	)
 '
 
-while read name path pretty; do
+while read name fpath pretty; do
 	while read mode type; do
-		: ${pretty:=$path}
+		: ${pretty:=$fpath}
 		test_expect_success "fsck notices $pretty as $type" '
 		(
 			git init $name-$type &&
@@ -603,7 +603,7 @@ while read name path pretty; do
 			blob=$(git rev-parse :file) &&
 			tree=$(git rev-parse HEAD^{tree}) &&
 			value=$(eval "echo \$$type") &&
-			printf "$mode $type %s\t%s" "$value" "$path" >bad &&
+			printf "$mode $type %s\t%s" "$value" "$fpath" >bad &&
 			bad_tree=$(git mktree <bad) &&
 			git fsck 2>out &&
 			test_i18ngrep "warning.*tree $bad_tree" out
@@ -674,8 +674,8 @@ create_repo_missing () {
 		unrelated=$(echo unrelated | git hash-object --stdin -w) &&
 		git tag -m foo tag $unrelated &&
 		sha1=$(git rev-parse --verify "$1") &&
-		path=$(echo $sha1 | sed 's|..|&/|') &&
-		rm .git/objects/$path
+		fpath=$(echo $sha1 | sed 's|..|&/|') &&
+		rm .git/objects/$fpath
 	)
 }
 
@@ -781,11 +781,11 @@ test_expect_success 'fsck --name-objects' '
 test_expect_success 'alternate objects are correctly blamed' '
 	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
 	name=$(test_oid numeric) &&
-	path=$(test_oid_to_path "$name") &&
+	fpath=$(test_oid_to_path "$name") &&
 	git init --bare alt.git &&
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
-	mkdir alt.git/objects/$(dirname $path) &&
-	>alt.git/objects/$(dirname $path)/$(basename $path) &&
+	mkdir alt.git/objects/$(dirname $fpath) &&
+	>alt.git/objects/$(dirname $fpath)/$(basename $fpath) &&
 	test_must_fail git fsck >out 2>&1 &&
 	test_i18ngrep alt.git out
 '
diff --git a/t/t3305-notes-fanout.sh b/t/t3305-notes-fanout.sh
index 1ec1fb6715..a0d8ec85a1 100755
--- a/t/t3305-notes-fanout.sh
+++ b/t/t3305-notes-fanout.sh
@@ -6,10 +6,10 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 path_has_fanout() {
-	path=$1 &&
+	fpath=$1 &&
 	fanout=$2 &&
 	after_last_slash=$(($(test_oid hexsz) - $fanout * 2)) &&
-	echo $path | grep -q -E "^([0-9a-f]{2}/){$fanout}[0-9a-f]{$after_last_slash}$"
+	echo $fpath | grep -q -E "^([0-9a-f]{2}/){$fanout}[0-9a-f]{$after_last_slash}$"
 }
 
 touched_one_note_with_fanout() {
@@ -17,17 +17,17 @@ touched_one_note_with_fanout() {
 	modification=$2 &&  # 'A' for addition, 'D' for deletion
 	fanout=$3 &&
 	diff=$(git diff-tree --no-commit-id --name-status --root -r $notes_commit) &&
-	path=$(echo $diff | sed -e "s/^$modification[\t ]//") &&
-	path_has_fanout "$path" $fanout;
+	fpath=$(echo $diff | sed -e "s/^$modification[\t ]//") &&
+	path_has_fanout "$fpath" $fanout;
 }
 
 all_notes_have_fanout() {
 	notes_commit=$1 &&
 	fanout=$2 &&
 	git ls-tree -r --name-only $notes_commit |
-	while read path
+	while read fpath
 	do
-		path_has_fanout $path $fanout || return 1
+		path_has_fanout $fpath $fanout || return 1
 	done
 }
 
diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
index ffaf69335f..4998a1ce27 100755
--- a/t/t4046-diff-unmerged.sh
+++ b/t/t4046-diff-unmerged.sh
@@ -19,10 +19,10 @@ test_expect_success setup '
 		do
 			for t in o x
 			do
-				path="$b$o$t" &&
-				case "$path" in ooo) continue ;; esac &&
-				paths="$paths$path " &&
-				p="	$path" &&
+				fpath="$b$o$t" &&
+				case "$fpath" in ooo) continue ;; esac &&
+				paths="$paths$fpath " &&
+				p="	$fpath" &&
 				case "$b" in x) echo "$m1$p" ;; esac &&
 				case "$o" in x) echo "$m2$p" ;; esac &&
 				case "$t" in x) echo "$m3$p" ;; esac ||
@@ -36,22 +36,22 @@ test_expect_success setup '
 '
 
 test_expect_success 'diff-files -0' '
-	for path in $paths
+	for fpath in $paths
 	do
-		>"$path" &&
-		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$path" || return 1
+		>"$fpath" &&
+		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$fpath" || return 1
 	done >diff-files-0.expect &&
 	git diff-files -0 >diff-files-0.actual &&
 	test_cmp diff-files-0.expect diff-files-0.actual
 '
 
 test_expect_success 'diff-files -1' '
-	for path in $paths
+	for fpath in $paths
 	do
-		>"$path" &&
-		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$path" &&
-		case "$path" in
-		x??) echo ":100644 100644 $blob1 $ZERO_OID M	$path"
+		>"$fpath" &&
+		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$fpath" &&
+		case "$fpath" in
+		x??) echo ":100644 100644 $blob1 $ZERO_OID M	$fpath"
 		esac || return 1
 	done >diff-files-1.expect &&
 	git diff-files -1 >diff-files-1.actual &&
@@ -59,12 +59,12 @@ test_expect_success 'diff-files -1' '
 '
 
 test_expect_success 'diff-files -2' '
-	for path in $paths
+	for fpath in $paths
 	do
-		>"$path" &&
-		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$path" &&
-		case "$path" in
-		?x?) echo ":100644 100644 $blob2 $ZERO_OID M	$path"
+		>"$fpath" &&
+		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$fpath" &&
+		case "$fpath" in
+		?x?) echo ":100644 100644 $blob2 $ZERO_OID M	$fpath"
 		esac || return 1
 	done >diff-files-2.expect &&
 	git diff-files -2 >diff-files-2.actual &&
@@ -74,12 +74,12 @@ test_expect_success 'diff-files -2' '
 '
 
 test_expect_success 'diff-files -3' '
-	for path in $paths
+	for fpath in $paths
 	do
-		>"$path" &&
-		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$path" &&
-		case "$path" in
-		??x) echo ":100644 100644 $blob3 $ZERO_OID M	$path"
+		>"$fpath" &&
+		echo ":000000 100644 $ZERO_OID $ZERO_OID U	$fpath" &&
+		case "$fpath" in
+		??x) echo ":100644 100644 $blob3 $ZERO_OID M	$fpath"
 		esac || return 1
 	done >diff-files-3.expect &&
 	git diff-files -3 >diff-files-3.actual &&
@@ -87,9 +87,9 @@ test_expect_success 'diff-files -3' '
 '
 
 test_expect_success 'diff --stat' '
-	for path in $paths
+	for fpath in $paths
 	do
-		echo " $path | Unmerged" || return 1
+		echo " $fpath | Unmerged" || return 1
 	done >diff-stat.expect &&
 	echo " 0 files changed" >>diff-stat.expect &&
 	git diff --cached --stat >diff-stat.actual &&
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 303f7a5d84..86838d2b0f 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -28,8 +28,8 @@ basic_cruft_pack_tests () {
 				git rev-list --objects --no-object-names base..loose |
 				while read oid
 				do
-					path="$objdir/$(test_oid_to_path "$oid")" &&
-					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$path")" ||
+					fpath="$objdir/$(test_oid_to_path "$oid")" &&
+					printf "%s %d\n" "$oid" "$(test-tool chmtime --get "$fpath")" ||
 					echo "object list generation failed for $oid"
 				done |
 				sort -k1
@@ -413,8 +413,8 @@ test_expect_success 'loose objects mtimes upsert others' '
 		git repack &&
 
 		tip="$(git rev-parse cruft)" &&
-		path="$objdir/$(test_oid_to_path "$tip")" &&
-		test-tool chmtime --get +1000 "$path" >expect &&
+		fpath="$objdir/$(test_oid_to_path "$tip")" &&
+		test-tool chmtime --get +1000 "$fpath" >expect &&
 
 		git checkout main &&
 		git branch -D other &&
-- 
2.39.2.13.g1fb56cf030

[PATCH 5/6] test: hack for zsh

[Felipe Contreras]

See: https://www.zsh.org/mla/workers/2023/msg00336.html

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t4046-diff-unmerged.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t4046-diff-unmerged.sh b/t/t4046-diff-unmerged.sh
index 4998a1ce27..ca31771070 100755
--- a/t/t4046-diff-unmerged.sh
+++ b/t/t4046-diff-unmerged.sh
@@ -21,7 +21,8 @@ test_expect_success setup '
 			do
 				fpath="$b$o$t" &&
 				case "$fpath" in ooo) continue ;; esac &&
-				paths="$paths$fpath " &&
+				: hack for zsh &&
+				{ paths="$paths$fpath "; } &&
 				p="	$fpath" &&
 				case "$b" in x) echo "$m1$p" ;; esac &&
 				case "$o" in x) echo "$m2$p" ;; esac &&
-- 
2.39.2.13.g1fb56cf030

Re: [PATCH 5/6] test: hack for zsh

[Felipe Contreras]

On Tue, Mar 28, 2023 at 11:39 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> See: https://www.zsh.org/mla/workers/2023/msg00336.html

In case anyone is interested this has just been fixed:

https://github.com/zsh-users/zsh/commit/12e5db145b098a62ff11b88eea26f473ea2ecdcf

-- 
Felipe Contreras

[PATCH 6/6] mergetools: vimdiff: check for empty fields

[Felipe Contreras]

In zsh 'foo#bar#' produces an empty field at the end.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 mergetools/vimdiff | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 06937acbf5..27d1644faf 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -87,7 +87,7 @@ gen_cmd_aux () {
 	IFS=#
 	for c in $(echo "$LAYOUT" | sed 's:.:&#:g')
 	do
-		if test "$c" = " "
+		if test "$c" = " " || test -z "$c"
 		then
 			continue
 		fi
@@ -155,7 +155,7 @@ gen_cmd_aux () {
 	do
 		i=$(( i + 1 ))
 
-		if test "$c" = " "
+		if test "$c" = " " || test -z "$c"
 		then
 			continue
 		fi
-- 
2.39.2.13.g1fb56cf030

Re: [PATCH 0/6] test: make the test suite work with zsh

[brian m. carlson]

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

On 2023-03-28 at 17:39:26, Felipe Contreras wrote:
> It's not difficult to make the testing library work for zsh, so I did that in
> the first patch.
> 
> The rest of the patches are basically to deal with some variables that are
> special in zsh, workaround a bug, and a minor discrepancy.

There was a point at which the tests worked entirely in sh mode with
zsh.  I know because I fixed a handful of tests there, ending with
c64368e3a2a47, and I patched zsh to run all commands in a pipeline in a
subshell in sh mode to fix the remaining tests.

If I symlink zsh (zsh 5.9 (x86_64-debian-linux-gnu)) to sh in a
temporary directory and use it in SHELL_PATH, I get only the following
failures:

  t4046-diff-unmerged.sh                           (Wstat: 256 (exited 1) Tests: 6 Failed: 5)
    Failed tests:  2-6
    Non-zero exit status: 1
  t7609-mergetool--lib.sh                          (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
    Failed test:  1
    Non-zero exit status: 1

I haven't investigated further.

I don't care a lot of other folks want to make zsh run the testsuite in
zsh mode, but I'd think that using sh mode would be simpler and less
likely to break in general, and would avoid us needing to carry a lot of
patches to work around various variables that are special in zsh mode.
It would also be easier to potentially test in the testsuite as well.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Tue, Mar 28, 2023 at 6:57 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2023-03-28 at 17:39:26, Felipe Contreras wrote:
> > It's not difficult to make the testing library work for zsh, so I did that in
> > the first patch.
> >
> > The rest of the patches are basically to deal with some variables that are
> > special in zsh, workaround a bug, and a minor discrepancy.
>
> There was a point at which the tests worked entirely in sh mode with
> zsh.  I know because I fixed a handful of tests there, ending with
> c64368e3a2a47, and I patched zsh to run all commands in a pipeline in a
> subshell in sh mode to fix the remaining tests.
>
> If I symlink zsh (zsh 5.9 (x86_64-debian-linux-gnu)) to sh in a
> temporary directory and use it in SHELL_PATH, I get only the following
> failures:

That would defeat my motivation behind the patches, which is to be
able to run one test file in zsh.

Only the first patch is needed for that, the rest were in case anyone
cared to run all the tests.

> I don't care a lot of other folks want to make zsh run the testsuite in
> zsh mode, but I'd think that using sh mode would be simpler and less
> likely to break in general, and would avoid us needing to carry a lot of
> patches to work around various variables that are special in zsh mode.

We don't need to carry the patches if the patches are applied.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 28 2023, Felipe Contreras wrote:

> On Tue, Mar 28, 2023 at 6:57 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>>
>> On 2023-03-28 at 17:39:26, Felipe Contreras wrote:
>> > It's not difficult to make the testing library work for zsh, so I did that in
>> > the first patch.
>> >
>> > The rest of the patches are basically to deal with some variables that are
>> > special in zsh, workaround a bug, and a minor discrepancy.
>>
>> There was a point at which the tests worked entirely in sh mode with
>> zsh.  I know because I fixed a handful of tests there, ending with
>> c64368e3a2a47, and I patched zsh to run all commands in a pipeline in a
>> subshell in sh mode to fix the remaining tests.
>>
>> If I symlink zsh (zsh 5.9 (x86_64-debian-linux-gnu)) to sh in a
>> temporary directory and use it in SHELL_PATH, I get only the following
>> failures:
>
> That would defeat my motivation behind the patches, which is to be
> able to run one test file in zsh.

"One" as in one specific file you have in mind, or a "one-off run"? The
1/6 here looks like it fixes most of the issues, but e.g. the
test-lib.sh fix in 2/6 would be needed by any test that reached that
code, wouldn't it?

If it's the latter, I don't really see the point of making just some of
it work with zsh's native mode (if I'm getting this correctly).

But for that case, wouldn't:

	zsh --some-options t0001-init.sh

Or whatever work, which just from skimming the help might be some of the
--posix options? But I can see that being more of a hassle, presumably
you want to use it as /bin/sh, and to have it pick up the script and
have it Just Work.

Some details on all of this in an updated commit message would be most
welcome...

> Only the first patch is needed for that, the rest were in case anyone
> cared to run all the tests.
>
>> I don't care a lot of other folks want to make zsh run the testsuite in
>> zsh mode, but I'd think that using sh mode would be simpler and less
>> likely to break in general, and would avoid us needing to carry a lot of
>> patches to work around various variables that are special in zsh mode.
>
> We don't need to carry the patches if the patches are applied.

But we do need to carry some hacks going forward, some of it seems
pretty isolated & easy to spot, but e.g. the 6/6 fix of:

-		if test "$c" = " "
+		if test "$c" = " " || test -z "$c"

Is quite subtle, you might look at that and be convinced that the RHS is
redundant, and be right, but only because you assume POSIX semantics.

If we are going to include this I think the relevant t/README and
Documentation/CodingGuidelines parts should be updated to note that
we're not targeting POSIX shellscripts anymore, but the subset of it
that zsh is happy with.

And, to avoid the inevitable re-breakage have this tested in CI,
usurping 1/2 OSX jobs for that might be a good target (or one of the
other linux jobs)...

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Wed, Mar 29, 2023 at 3:58 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Mar 28 2023, Felipe Contreras wrote:
>
> > On Tue, Mar 28, 2023 at 6:57 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> >>
> >> On 2023-03-28 at 17:39:26, Felipe Contreras wrote:
> >> > It's not difficult to make the testing library work for zsh, so I did that in
> >> > the first patch.
> >> >
> >> > The rest of the patches are basically to deal with some variables that are
> >> > special in zsh, workaround a bug, and a minor discrepancy.
> >>
> >> There was a point at which the tests worked entirely in sh mode with
> >> zsh.  I know because I fixed a handful of tests there, ending with
> >> c64368e3a2a47, and I patched zsh to run all commands in a pipeline in a
> >> subshell in sh mode to fix the remaining tests.
> >>
> >> If I symlink zsh (zsh 5.9 (x86_64-debian-linux-gnu)) to sh in a
> >> temporary directory and use it in SHELL_PATH, I get only the following
> >> failures:
> >
> > That would defeat my motivation behind the patches, which is to be
> > able to run one test file in zsh.
>
> "One" as in one specific file you have in mind, or a "one-off run"?

One specific file.

I just did a quick porting of the code in my fork, and this should
give you a good idea (it's preliminary):

https://github.com/felipec/git/blob/zsh/tests/t/t9904-completion-zsh.sh

I had already sent versions of this test file that run in bash, but
it's much better to fix test-lib.sh so that it works for zsh and they
can be run directly.

> The
> 1/6 here looks like it fixes most of the issues, but e.g. the
> test-lib.sh fix in 2/6 would be needed by any test that reached that
> code, wouldn't it?

Yes, but my test file doesn't reach that code.

> Some details on all of this in an updated commit message would be most
> welcome...

Well, those details depended on the response.

Since nobody seems to be interested in the tests running with vanilla
zsh, I can just drop them and the explanation.

> > Only the first patch is needed for that, the rest were in case anyone
> > cared to run all the tests.
> >
> >> I don't care a lot of other folks want to make zsh run the testsuite in
> >> zsh mode, but I'd think that using sh mode would be simpler and less
> >> likely to break in general, and would avoid us needing to carry a lot of
> >> patches to work around various variables that are special in zsh mode.
> >
> > We don't need to carry the patches if the patches are applied.
>
> But we do need to carry some hacks going forward, some of it seems
> pretty isolated & easy to spot, but e.g. the 6/6 fix of:
>
> -               if test "$c" = " "
> +               if test "$c" = " " || test -z "$c"
>
> Is quite subtle, you might look at that and be convinced that the RHS is
> redundant, and be right, but only because you assume POSIX semantics.
>
> If we are going to include this I think the relevant t/README and
> Documentation/CodingGuidelines parts should be updated to note that
> we're not targeting POSIX shellscripts anymore, but the subset of it
> that zsh is happy with.

There's no point in that. I consider it a bug in zsh, along with 5/6,
so presumably at some point it's going to be fixed.

And if nobody cares about running these tests in zsh, it doesn't matter anyway.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Wed, Mar 29, 2023 at 5:19 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, Mar 29, 2023 at 3:58 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:

> > But we do need to carry some hacks going forward, some of it seems
> > pretty isolated & easy to spot, but e.g. the 6/6 fix of:
> >
> > -               if test "$c" = " "
> > +               if test "$c" = " " || test -z "$c"
> >
> > Is quite subtle, you might look at that and be convinced that the RHS is
> > redundant, and be right, but only because you assume POSIX semantics.

Actually, that isn't even true (see below).

> > If we are going to include this I think the relevant t/README and
> > Documentation/CodingGuidelines parts should be updated to note that
> > we're not targeting POSIX shellscripts anymore, but the subset of it
> > that zsh is happy with.

But in this particular case the exact opposite is true: the script is
*not* POSIX, it just happens to work on bash and other shells.

You *assume* it's POSIX because it works on bash and it doesn't work
on zsh, but in this particular case bash is the non-POSIX one, zsh is
following POSIX correctly.

> There's no point in that. I consider it a bug in zsh, along with 5/6,
> so presumably at some point it's going to be fixed.

Actually, no. I've changed my mind.

I was going to report to the zsh dev mailing list the fact that this
created an extra empty field at the end (in sh mode):

  IFS=, ; str='foo,bar,,roo,'; printf '"%s"\n' $str

But then I read the POSIX specification, and the section 2.6.5 Field
Splitting [1] is very clear on what should happen.

What muddles the waters is the distinction between `IFS white space`
characters (newline, space and tab), and non-`IFS white space`
characters (all the other).

If we ignore all the shite space stuff and concentrate on the rules
for non-`IFS white space` characters (as comma is), then we arrive at
this subitem:

3.b. "Each occurrence in the input of an IFS character that is not IFS
white space, along with any adjacent IFS white space, shall delimit a
field, as described previously."

In other words: each occurence of a non-`IFS white space` character
shall delimit a field. Or: each occurence of a comma should delimit a
field.

The script only works if the last delimiter does *not* delimit a
field, and thus it's not following POSIX, it just happens to work on
most shells. My patch does make it align with POSIX.

I've reported bash's non-compliance with POSIX to their mailing list [2].

But I bet nobody here will care, because POSIX is just an excuse to
segregate the shells the main developers want to make work, from the
ones they are not (Brian even used the language of certain shells
being one of "the good ones").

Cheers.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_05
[2] https://lists.gnu.org/archive/html/bug-bash/2023-03/msg00152.html

--
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I don't care a lot of other folks want to make zsh run the
> testsuite in zsh mode, but I'd think that using sh mode would be
> simpler and less likely to break in general, and would avoid us
> needing to carry a lot of patches to work around various variables
> that are special in zsh mode.  It would also be easier to
> potentially test in the testsuite as well.

While these patches may make it "work" with zsh in its native mode,
because zsh that is running in its native mode is sufficiently
distant from the more POSIXy portable variants of Bourne shells like
dash and bash, I find it hard to justify the cost of maintaining the
resulting codebase to "work" that way.

We need to force our developers to pay attention to rules set by
zsh, like "$0 is not the way to spell the name of the script",
"$status is not usable by end-users", etc.  Making the problem
worse, the rules are enforced only by zsh.  The situation is similar
to the "when to use $(pwd) and when to use $PWD?" rules, which folks
like me without Windows have to spend extra minutes to remember
every time they use one of these constructs, but the number of rules
seem to be much larger as these patches seem to show.

Thanks.

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Wed, Mar 29, 2023 at 9:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > I don't care a lot of other folks want to make zsh run the
> > testsuite in zsh mode, but I'd think that using sh mode would be
> > simpler and less likely to break in general, and would avoid us
> > needing to carry a lot of patches to work around various variables
> > that are special in zsh mode.  It would also be easier to
> > potentially test in the testsuite as well.
>
> While these patches may make it "work" with zsh in its native mode,
> because zsh that is running in its native mode is sufficiently
> distant from the more POSIXy portable variants of Bourne shells like
> dash and bash, I find it hard to justify the cost of maintaining the
> resulting codebase to "work" that way.

Why do we follow POSIX anyway?

Is it a) because following POSIX is just good? Or b) because POSIX is
a standard that many shells attempt to follow and thus by following
POSIX we maximize the amount of code that works on most shells?

A quick non-exhaustive search of the history of Git, shows these:

74ec8cf674 Conform to POSIX (works in mksh/lksh)
c5c39f4e34 Fix for dash
7d661e5ed1 Conform with POSIX (works with dash and mksh)
ddf3a1152d Remove a warning in bash
9b67c9942e Workaround for ksh
5c63920190 Consideration for some shells (e.g. ksh93)
e9980419cb Fails in ksh88
268ef4d3d0 Aligns with POSIX
8e98b35f87 ksh88 on AIX considers it a syntax error
8f92c7755e Code would break under mksh
b082687cba Not allowed in mksh
661bfd13b4 Fails in xpdg4/sh and ksh
f5799e05c0 Public domain ksh and ksh on IRIX 6.5 have a problem
365c2aaafc Doesn't work in pdksh, POSIX unclear
482ce70e14 Doesn't work in dash and ash
77e572653b Doesn't work in dash
0e418e568f Workaround for ksh
49a43f5468 ksh doesn't like this
c289c315c2 Doesn't work in xpg4/sh
933e4f090d Workaround possible dash bug
c7053aa88f Doesn't seem to be supported by dash

It does seem like in only around 15% of these patches the
consideration was "this aligns with POSIX", in most cases the patch
was to make the code work for a particular shell.

In most cases the mention of POSIX is used as a justification for the
proposed code, but it was clear the intention was to make it work for
a specific shell (usually ksh).

In the few cases where POSIX was the main rationale, the shells in
which the current code doesn't work are mentioned. Not a single patch
says "this aligns with POSIX" without mentioning any shell.

> We need to force our developers to pay attention to rules set by
> zsh, like "$0 is not the way to spell the name of the script",
> "$status is not usable by end-users", etc.  Making the problem
> worse, the rules are enforced only by zsh.

No, we don't. $0 is not something a test script needs. In fact, there
doesn't seem to be a single instance in which $0 is used except test
infrastructure (e.g. test-lib.sh)

This sounds like a rationalization post-decision rather than a reason
for the decision.

Looking at the history it seems the test code has been more than happy
to make accommodations for xpg4/sh, pdksh, ash, and even mksh, even
when the code was already POSIX-compliant.

In truth all the patches regarding shell portability have been along
the lines of: "this code makes $x shell work, doesn't break other
shells, and isn't against POSIX". In some cases even when the Austin
group disagreed on what POSIX actually said, we did whatever worked in
most shells.

Is there some sort of predisposition against zsh?

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> While these patches may make it "work" with zsh in its native mode,
>> because zsh that is running in its native mode is sufficiently
>> distant from the more POSIXy portable variants of Bourne shells like
>> dash and bash, I find it hard to justify the cost of maintaining the
>> resulting codebase to "work" that way.
>
> Why do we follow POSIX anyway?

It is not what we follow that is at the primary issue.

The criteria is more about what our developers are expected to be
familiar with, and what is reasonable to force our developers to
become sufficiently familiar with.  Even among POSIXy Bourne
variants, dash being stricter than bash already gives some new
folks, who only know bash, things to learn to avoid.  But at least
what they learn through such an effort would help them write more
portable scripts.  It is my impression, however, that zsh in its
native mode is even further out and away, pushing it on the other
side of the line of being reasonable to force our develoerps to
adjust to.  What they will learn through such an effort would be
more of "what to do when you are forced to use zsh" than "how to
write your shell script portably".

> In truth all the patches regarding shell portability have been along
> the lines of: "this code makes $x shell work, doesn't break other
> shells, and isn't against POSIX". In some cases even when the Austin
> group disagreed on what POSIX actually said, we did whatever worked in
> most shells.

One aspect that is missing in the above is the extra burden on our
developers.

> Is there some sort of predisposition against zsh?

There isn't.  If somebody makes the tests to also work with csh or
/bin/sh on SunOS, I would pretty much say the same thing.  The
end-result may work perfectly well, but the cost to maintain it
working over the long haul may be too high for our developers.

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Thu, Mar 30, 2023 at 4:15 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> While these patches may make it "work" with zsh in its native mode,
> >> because zsh that is running in its native mode is sufficiently
> >> distant from the more POSIXy portable variants of Bourne shells like
> >> dash and bash, I find it hard to justify the cost of maintaining the
> >> resulting codebase to "work" that way.
> >
> > Why do we follow POSIX anyway?
>
> It is not what we follow that is at the primary issue.
>
> The criteria is more about what our developers are expected to be
> familiar with, and what is reasonable to force our developers to
> become sufficiently familiar with.

That is not true.

This patch 77e572653b (t0050: fix printf format strings for
portability, 2010-12-21) fixed a problem that was specific with dash.
Did our developers have to learn the details of such issue? No.

How about f5799e05c0 (git-submodule.sh: separate parens by a space to
avoid confusing some shells, 2011-05-26)? Did anybody have to learn
that `((foo` causes problems in ksh in order to apply the patch?

No, these patches fixed specific issues for specific shells, and did
not affect what our developers were expected to be familiar with.

> It is my impression, however, that zsh in its native mode is even
> further out and away, pushing it on the other side of the line of
> being reasonable to force our develoerps to adjust to.

Just because it's your impression doesn't mean it's true.

> What they will learn through such an effort would be more of "what to
> do when you are forced to use zsh" than "how to write your shell
> script portably".

No, they don't have to learn anything, just like they don't have to
learn that ksh93 returns signals that are 256+n: 0e418e568f (t0005:
work around strange $? in ksh when program terminated by a signal,
2010-07-09).

You just picked the patch that fixed a specific problem for a specific
shell, and we all forgot about it.

> > Is there some sort of predisposition against zsh?
>
> There isn't.  If somebody makes the tests to also work with csh or
> /bin/sh on SunOS, I would pretty much say the same thing.

Except you did pick the patches that fixed specific issues for these
shells when the existing code was already compliant with POSIX.

--
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Taylor Blau]

On Thu, Mar 30, 2023 at 08:19:08AM -0600, Felipe Contreras wrote:
> > It is my impression, however, that zsh in its native mode is even
> > further out and away, pushing it on the other side of the line of
> > being reasonable to force our develoerps to adjust to.
>
> Just because it's your impression doesn't mean it's true.

Sure, but zsh's incompatibility with bash is clear evidence that it is
further out from the POSIX standard. But I think the argument is missing
the point, anyway, which is that it's incompatible with POSIX.

Is it worth adapting our explicitly POSIX-compatible test suite to work
with a non-POSIX shell? I think not.

Thanks,
Taylor

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Fri, Mar 31, 2023 at 6:04 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Mar 30, 2023 at 08:19:08AM -0600, Felipe Contreras wrote:
> > > It is my impression, however, that zsh in its native mode is even
> > > further out and away, pushing it on the other side of the line of
> > > being reasonable to force our develoerps to adjust to.
> >
> > Just because it's your impression doesn't mean it's true.
>
> Sure, but zsh's incompatibility with bash is clear evidence that it is
> further out from the POSIX standard.

No, the POSIX standard is not whatever bash does; bash does many
things that are not part of POSIX, and it even has a POSIX mode
precisely because it does not follow it to a tee: `bash --posix`.

If `ksh` does something that bash does differently, is that evidence
that `ksh` is further out from the POSIX standard?

This argument does not make sense.

> But I think the argument is missing the point, anyway, which is that it's incompatible with POSIX.

So is bash.

All shells in the world are incompatible with the POSIX standard in
one way or another.

POSIX doesn't say anything about the $COLUMNS variable, and yet bash
sets it (and many other variables). Does that mean bash is
"non-POSIX"?

Of course not; POSIX doesn't say "thou shall not set the $COLUMNS
environment variable", so it's OK for bash to do that, it's still
compatible with posix (as long as you use `--posix`).

This's just not how standards work.

> Is it worth adapting our explicitly POSIX-compatible test suite to work
> with a non-POSIX shell? I think not.

All shells are non-POSIX.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> The criteria is more about what our developers are expected to be
>> familiar with, and what is reasonable to force our developers to
>> become sufficiently familiar with.
>
> That is not true.

We probably should agree to disagree.  Let me respond by picking the
first example from your message and then stop.

> This patch 77e572653b (t0050: fix printf format strings for
> portability, 2010-12-21) fixed a problem that was specific with dash.
> Did our developers have to learn the details of such issue? No.

The code before that change was feeding "\xc3\xa4" to printf,
expecting that it would be an acceptable way to spell hexadecimal
byte values, which was wrong.  The commit improved portability by
rewriting them to spell the same byte values in octal.

Yes, our developers have to learn to avoid hexadecimal byte values
and the commit serves as a reminder for them to learn from.  When a
developer writes printf format with '\xCC' hexadecimal, reviewers
would need to catch it as a mistake, and that commit makes a good
reference why we insist on such a rule.

By learning that practice, our developers will be trained to write
scripts that not just work with bash but also with dash, which is a
small good thing.  But still, it *is* forcing our developers to
learn one more rule.

There is a trade off: is it worth supporting dash by forcing our
developers to stick to the rule to write octal and not hex?  dash is
used as the default for some distros and considered one of the
standard ones, and is worth supporting even if we need to stay away
from some stuff people may have picked up from other shells like
bash.

If it were a different shell, the equation may have been different.
If industry standards like POSIX.1 required supporting hex literals,
the equation may also have been different.

	Note: though, we do not blindly say "it is in POSIX, your
        shell behaves differently and we won't support it".

In any case, once we declare that "we aim for our scripts and tests
to work with dash and bash and these other shells", our developers
are forced to stick to intersection of these supported ones.

It takes a judgement call.  And "don't write literals you feed
printf in hex, instead do so in octal, because printf built into
some shells do not like it" is something reasonable to force our
developers to stick to (as it allows "dash" to be thrown into the
set of supported shells).  I however do not see "don't use path or
status or options or 0 as shell variable names" falls into the same
category (even though it may allow "zsh" in native mode to be
included into the supported shells).  Do benefits outweigh
downsides?

And somebody has to draw that line.  Judgement calls are just that.
There are no absolutely right or wrong answers and they will not
please everybody.  Some folks may not agree with where the line gets
drawn.  Tough.

But the job of maintainer is not about being loved by everybody.
Just drawing the line somewhere in order to allow folks to move on,
without having everybody dragged into and getting stuck in endless
arguments, in which there is no absolute right or wrong.  That is
what needs to be done, and that is what I just did.

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Fri, Mar 31, 2023 at 7:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> The criteria is more about what our developers are expected to be
> >> familiar with, and what is reasonable to force our developers to
> >> become sufficiently familiar with.
> >
> > That is not true.
>
> We probably should agree to disagree.

You can decide to disagree with a fact, but it's still a fact.

It is a fact that portability fixes have been merged to the code with
zero consideration to what "our developers" would have to be
"sufficiently familiar with" (which is very little, if anything).

I provided dozens of examples, and in this particular response chose
to only pick two.

>Let me respond by picking the first example from your message and then stop.

Sure, but that's just *one* example of many that prove what I'm saying
is a fact, and I explain below it doesn't apply.

> > This patch 77e572653b (t0050: fix printf format strings for
> > portability, 2010-12-21) fixed a problem that was specific with dash.
> > Did our developers have to learn the details of such issue? No.
>
> The code before that change was feeding "\xc3\xa4" to printf,
> expecting that it would be an acceptable way to spell hexadecimal
> byte values, which was wrong.  The commit improved portability by
> rewriting them to spell the same byte values in octal.
>
> Yes, our developers have to learn to avoid hexadecimal byte values
> and the commit serves as a reminder for them to learn from.

Even if that were true (which it isn't), that has *absolutely nothing*
to do with the patch in question.

Developers would have to avoid that practice because it breaks dash,
not because you picked the patch.

This is a red herring: merging or not merging the patch makes no
difference to the practice.

And this is a prescriptive claim from you. *You* say that our
developers have to learn that practice, and that's your opinion of
what our developers should do. But that doesn't mean that our
developers did actually learn that.

> When a developer writes printf format with '\xCC' hexadecimal,
> reviewers would need to catch it as a mistake, and that commit makes a
> good reference why we insist on such a rule.

Yes, according to you, and regardless of whether or not you merge the patch.

> But still, it *is* forcing our developers to learn one more rule.

Not true. The fact that such code breaks dash is what is forcing our
developers to learn that rule, not you merging the patch.

Most developers would not even notice that you merged the patch. And
such practice would not be shared in a memo, notes from the
maintainer, or update of the CodingGuidelines.

You may be expecting such practice from developers after you merge
such patch, but that's very different from developers actually knowing
that they have to adopt this practice.

> There is a trade off: is it worth supporting dash by forcing our
> developers to stick to the rule to write octal and not hex?  dash is
> used as the default for some distros and considered one of the
> standard ones, and is worth supporting even if we need to stay away
> from some stuff people may have picked up from other shells like
> bash.

zsh is the default in macOS.

Is macOS less important than some distros?

> In any case, once we declare that "we aim for our scripts and tests
> to work with dash and bash and these other shells", our developers
> are forced to stick to intersection of these supported ones.

Once again: red herring. Merging a patch has absolutely nothing to do
with you declaring such an aim.

And if that were true, there would be a declaration of "Git now
supports dash in its test suite".

When did such a declaration take place?

Moreover, the declaration has nothing to do with the patch.

> It takes a judgement call.  And "don't write literals you feed
> printf in hex, instead do so in octal, because printf built into
> some shells do not like it" is something reasonable to force our
> developers to stick to (as it allows "dash" to be thrown into the
> set of supported shells).

Nothing to do with the patch.

> I however do not see "don't use path or status or options or 0 as
> shell variable names" falls into the same category

Another red herring: because this is not something that anybody is
suggesting to actually do.

---

The fact is that $0 is used nowhere in the tests.

You can decide to disagree with what `git grep '$0' t/t????-*.sh`
shows, but it's an unobjectionable fact.

Not that it matters, because I just sent another patch with `emulate
sh -o POSIX_ARGZERO`, which doesn't require changing $0, so you'll
need another rationale to not support zsh.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[Taylor Blau]

On Thu, Mar 30, 2023 at 03:15:48AM -0700, Junio C Hamano wrote:
> > In truth all the patches regarding shell portability have been along
> > the lines of: "this code makes $x shell work, doesn't break other
> > shells, and isn't against POSIX". In some cases even when the Austin
> > group disagreed on what POSIX actually said, we did whatever worked in
> > most shells.
>
> One aspect that is missing in the above is the extra burden on our
> developers.

Well said.

Having to remember that we need to write "$ARGZERO" instead of "$0" is
yet another burden to place on developers who want to make changes to
the test suite.

Is that a big deal? Probably not. But it's a slippery slope, and a weird
gotcha to remember when dealing with our otherwise POSIX-y test suite.

So I'm not convinced that it's a worthwhile price to pay for zsh support
in our test suite.

Thanks,
Taylor

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Fri, Mar 31, 2023 at 6:00 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Mar 30, 2023 at 03:15:48AM -0700, Junio C Hamano wrote:
> > > In truth all the patches regarding shell portability have been along
> > > the lines of: "this code makes $x shell work, doesn't break other
> > > shells, and isn't against POSIX". In some cases even when the Austin
> > > group disagreed on what POSIX actually said, we did whatever worked in
> > > most shells.
> >
> > One aspect that is missing in the above is the extra burden on our
> > developers.
>
> Well said.
>
> Having to remember that we need to write "$ARGZERO" instead of "$0"

When have you ever used "$0" in the test suite?

A quick grep shows zero results (`git log --author=me@ttaylorr.com
-S'$0'`), so I think you are talking about a hypothetical, not
something that would actually happen in reality.

Sometimes preemptive optimization pays off in the future, but other
times it doesn't, and it's just wasted mental effort.

This is one of those times when worrying about a future that will
never happen in reality does not pay off.

> Is that a big deal? Probably not. But it's a slippery slope, and a weird
> gotcha to remember when dealing with our otherwise POSIX-y test suite.

You won't have to remember that because you'll never use $0 in the
test suite. Nobody does use it, and nobody ever will.

*If* for some weird reason somebody needs to use $0, we can worry
about it *then*.

---

But this is a red herring. The reality is that developers do not have
to worry about every little aspect of the test suite. When somebody
uses `seq`, somebody else reminds them to use `test_seq` instead, and
if for some reason a `seq` slips by and it breaks the test in some
obscure platform, the test is updated to fix that. It's not a big
deal.

Why are many tests using `chmod` instead of `test_chmod`? Did the
introduction of `test_chmod` imply an extra burden to "our developers"
to remember using that instead of `chmod`? No, *in reality* what
everyone cares about is that the test runs on the platforms of the
real world.

If we could run the test suite on 100 hypothetical platforms that
don't exist in the real world, it would break all over the place.

In reality all the portability considerations of the test suite are
geared towards certain platforms that exist in the real world. Nobody
cares that the test suite doesn't run on some hypothetical platforms.

So no, nobody needs to remember to use test_chmod, or test_seq, and
nobody will ever have to remember to use $ARGZERO instead of $0, and
if some hypothetical person does use that in some hypothetical test
and that breaks the tests for zsh in native mode, nobody will care,
except the person running those tests (likely me), which would
promptly fix that single hypothetical instance of $0.

Nobody will die if a instance of $0 slipped by (which will never happen).

This is simply not a real consideration.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/6] test: make the test suite work with zsh

[brian m. carlson]

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

On 2023-03-29 at 15:34:26, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > I don't care a lot of other folks want to make zsh run the
> > testsuite in zsh mode, but I'd think that using sh mode would be
> > simpler and less likely to break in general, and would avoid us
> > needing to carry a lot of patches to work around various variables
> > that are special in zsh mode.  It would also be easier to
> > potentially test in the testsuite as well.
> 
> While these patches may make it "work" with zsh in its native mode,
> because zsh that is running in its native mode is sufficiently
> distant from the more POSIXy portable variants of Bourne shells like
> dash and bash, I find it hard to justify the cost of maintaining the
> resulting codebase to "work" that way.

I think that's a fine position to take.  We have found some fun bugs in
our testsuite via zsh's sh mode, and zsh (in sh mode) is a valid shell
according to Debian's /bin/sh rules, so I think it's fair to address
shell fixes for it in sh mode where we can, even though it sounds like
we'd prefer to skip the zsh mode.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 0/6] test: make the test suite work with zsh

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> While these patches may make it "work" with zsh in its native mode,
>> because zsh that is running in its native mode is sufficiently
>> distant from the more POSIXy portable variants of Bourne shells like
>> dash and bash, I find it hard to justify the cost of maintaining the
>> resulting codebase to "work" that way.
>
> I think that's a fine position to take.  We have found some fun bugs in
> our testsuite via zsh's sh mode, and zsh (in sh mode) is a valid shell
> according to Debian's /bin/sh rules, so I think it's fair to address
> shell fixes for it in sh mode where we can, even though it sounds like
> we'd prefer to skip the zsh mode.

Yeah, as long as the sh mode is sufficiently compatible, I would be
happy to see the shell supported as one of the /bin/sh we can use,
and I agree that it is fair to address shell portability fixes for
zsh running in the sh compatible mode.

If the native mode didn't impose too much burden on our developers'
to maintain, it would have been nicer, but judging from the contents
of these patches, I am afraid that it falls on the other side of the
line.

Thanks.

Re: [PATCH 0/6] test: make the test suite work with zsh

[Felipe Contreras]

On Wed, Mar 29, 2023 at 9:15 PM Junio C Hamano <gitster@pobox.com> wrote:

> If the native mode didn't impose too much burden on our developers'
> to maintain, it would have been nicer, but judging from the contents
> of these patches, I am afraid that it falls on the other side of the
> line.

There is zero burden to maintain.

What burden did this fix specific for ksh93 cause to "our developers"
0e418e568f (t0005: work around strange $? in ksh when program
terminated by a signal, 2010-07-09)?

--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -13,6 +13,7 @@ test_expect_success 'sigchain works' '
        test-sigchain >actual
        case "$?" in
        143) true ;; # POSIX w/ SIGTERM=15
+       271) true ;; # ksh w/ SIGTERM=15
          3) true ;; # Windows
          *) false ;;
        esac &&

This patch was applied once and forgotten about for 6 six years until
one developer refactored the code in 9b67c9942e (tests: factor
portable signal check out of t0005, 2016-06-30).

"Our developers" did not have to care about code that affected only
one shell. Not since 2010, not since 2016, not even today.

Even the original author admitted he didn't understand how this code
truly interacted in other situations [1]:

> Frankly, I don't know how bash's 'kill -TERM' and a Windows program
> interact.  I've avoided this topic like the plague so far.

It's not true that applying patches to fix a situation for a specific
shell causes a hypothetical burden to our developers to magically
appear.

In reality the patch is applied once and forgotten about it until a)
another developer stumbles upon a similar problem, or b) another
developer stumbles upon that specific code.

No developer cared about 0e418e568f being applied. It fixed the
situation for one shell, it didn't affect other shells negatively, and
that's all anyone needed to know.

[1] https://lore.kernel.org/git/576DA6FB.1050108@kdbg.org

-- 
Felipe Contreras