[PATCH 0/9] subtree: fix split and merge after annotated tag was squash-merged

[PATCH 0/9] subtree: fix split and merge after annotated tag was squash-merged

[Philippe Blain via GitGitGadget]

This series fixes a limitation of 'git subtree merge' and 'git subtree
split' that would fail when the previous squash-merge merged an annotated
tag of the subtree repository that is missing locally.

The 5 first commits are small improvements for coherency with the rest of
the code base, robustness, user experience and maintanability.

Commits 6-7 are small refactors that prepare for the fix in the next
commits. Commits 8-9 fix the behaviour for merge and split, respectively.

I'm CC-ing people that worked on 'git subtree' recently.

Cheers!

Philippe Blain (9):
  test-lib-functions: mark 'test_commit' variables as 'local'
  subtree: use 'git rev-parse --verify [--quiet]' for better error
    messages
  subtree: add 'die_incompatible_opt' function to reduce duplication
  subtree: prefix die messages with 'fatal'
  subtree: define a variable before its first use in
    'find_latest_squash'
  subtree: use named variables instead of "$@" in cmd_pull
  subtree: process 'git-subtree-split' trailer in separate function
  subtree: fix squash merging after annotated tag was squashed merged
  subtree: fix split after annotated tag was squashed merged

 contrib/subtree/git-subtree.sh     | 177 +++++++++++++++++++----------
 contrib/subtree/git-subtree.txt    |  16 ++-
 contrib/subtree/t/t7900-subtree.sh |  60 +++++++++-
 t/test-lib-functions.sh            |  16 +--
 4 files changed, 192 insertions(+), 77 deletions(-)


base-commit: 45c9f05c44b1cb6bd2d6cb95a22cf5e3d21d5b63
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1390%2Fphil-blain%2Fsubtree-pull-tag-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1390/phil-blain/subtree-pull-tag-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1390
-- 
gitgitgadget

[PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

Some variables in 'test_commit' have names that are common enough that
it is very likely that test authors might use them in a test. If they do
so and use 'test_commit' between setting such a variable and using it,
the variable value from 'test_commit' will leak back into the test and
most likely break it.

Prevent that by marking all variables in 'test_commit' as 'local'. This
allow a subsequent commit to use a 'tag' variable.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/test-lib-functions.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 527a7145000..adc0fb6330c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -273,13 +273,13 @@ debug () {
 # <file>, <contents>, and <tag> all default to <message>.
 
 test_commit () {
-	notick= &&
-	echo=echo &&
-	append= &&
-	author= &&
-	signoff= &&
-	indir= &&
-	tag=light &&
+	local notick= &&
+	local echo=echo &&
+	local append= &&
+	local author= &&
+	local signoff= &&
+	local indir= &&
+	local tag=light &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -322,7 +322,7 @@ test_commit () {
 		shift
 	done &&
 	indir=${indir:+"$indir"/} &&
-	file=${2:-"$1.t"} &&
+	local file=${2:-"$1.t"} &&
 	if test -n "$append"
 	then
 		$echo "${3-$1}" >>"$indir$file"
-- 
gitgitgadget

[PATCH 2/9] subtree: use 'git rev-parse --verify [--quiet]' for better error messages

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

There are three occurences of 'git rev-parse <rev>' in 'git-subtree.sh'
where the command expects a revision and the script dies or exits if the
revision can't be found. In that case, the error message from 'git
rev-parse' is:

    $ git rev-parse <bad rev>
    <bad rev>
    fatal: ambiguous argument '<bad rev>': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

This is a little confusing to the user, since this error message is
outputed by 'git subtree'.

At these points in the script, we know that we are looking for a single
revision, so be explicit by using '--verify', resulting in a little
better error message:

    $ git rev-parse --verify <bad rev>
    fatal: Needed a single revision

In the two occurences where we 'die' if 'git rev-parse' fails, 'git
subtree' outputs "could not rev-parse split hash $b from commit $sq", so
we actually do not need the supplementary error message from 'git
rev-parse'; add '--quiet' to silence it.

In the third occurence, we 'exit', so keep the error message from 'git
rev-parse'. Note that this messsage is still suboptimal since it can be
understood to mean that 'git rev-parse' did not receive a single
revision as argument, which is not the case here: the command did
receive a single revision, but the revision is not resolvable to an
available object.

The alternative would be to use '--' after the revision, as suggested by
the first error message, resulting in a clearer error message:

    $ git rev-parse <bad rev> --
    fatal: bad revision '<bad rev>'

Unfortunately we can't use that syntax because in the more common case
of the revision resolving to a known object, the command outputs the
object's hash, a newline, and the dashdash, which breaks the 'git
subtree' script.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7562a395c24..49ef493ef92 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -387,7 +387,7 @@ find_latest_squash () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			sub="$(git rev-parse "$b^{commit}")" ||
+			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
 			die "could not rev-parse split hash $b from commit $sq"
 			;;
 		END)
@@ -439,7 +439,7 @@ find_existing_splits () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			sub="$(git rev-parse "$b^{commit}")" ||
+			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
 			die "could not rev-parse split hash $b from commit $sq"
 			;;
 		END)
@@ -843,7 +843,7 @@ cmd_add_commit () {
 	git checkout -- "$dir" || exit $?
 	tree=$(git write-tree) || exit $?
 
-	headrev=$(git rev-parse HEAD) || exit $?
+	headrev=$(git rev-parse --verify HEAD) || exit $?
 	if test -n "$headrev" && test "$headrev" != "$rev"
 	then
 		headp="-p $headrev"
-- 
gitgitgadget

[PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)
added validation code to check that options given to 'git subtree <cmd>'
made sense with the command being used.

Refactor these checks by adding a 'die_incompatible_opt' function to
reduce code duplication.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 49ef493ef92..f5eab198c80 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -102,6 +102,14 @@ assert () {
 	fi
 }
 
+# Usage: die_incompatible_opt OPTION COMMAND
+die_incompatible_opt () {
+	assert test "$#" = 2
+	opt="$1"
+	arg_command="$2"
+	die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+}
+
 main () {
 	if test $# -eq 0
 	then
@@ -176,16 +184,16 @@ main () {
 			arg_debug=1
 			;;
 		--annotate)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_annotate="$1"
 			shift
 			;;
 		--no-annotate)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_annotate=
 			;;
 		-b)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_branch="$1"
 			shift
 			;;
@@ -194,7 +202,7 @@ main () {
 			shift
 			;;
 		-m)
-			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command"
 			arg_addmerge_message="$1"
 			shift
 			;;
@@ -202,34 +210,34 @@ main () {
 			arg_prefix=
 			;;
 		--onto)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_onto="$1"
 			shift
 			;;
 		--no-onto)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_onto=
 			;;
 		--rejoin)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			;;
 		--no-rejoin)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			;;
 		--ignore-joins)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_ignore_joins=1
 			;;
 		--no-ignore-joins)
-			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
 			arg_split_ignore_joins=
 			;;
 		--squash)
-			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command"
 			arg_addmerge_squash=1
 			;;
 		--no-squash)
-			test -n "$allow_addmerge" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+			test -n "$allow_addmerge" || die_incompatible_opt "$opt" "$arg_command"
 			arg_addmerge_squash=
 			;;
 		--)
-- 
gitgitgadget

[PATCH 4/9] subtree: prefix die messages with 'fatal'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

Just as was done in 0008d12284 (submodule: prefix die messages with
'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages
outputed by 'git-subtree.sh' more in line with the rest of the code base
by prefixing them with "fatal: ", and do not capitalize their first
letter.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh     | 60 +++++++++++++++---------------
 contrib/subtree/t/t7900-subtree.sh | 12 +++---
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index f5eab198c80..89f1eb756f0 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -98,7 +98,7 @@ progress () {
 assert () {
 	if ! "$@"
 	then
-		die "assertion failed: $*"
+		die "fatal: assertion failed: $*"
 	fi
 }
 
@@ -107,7 +107,7 @@ die_incompatible_opt () {
 	assert test "$#" = 2
 	opt="$1"
 	arg_command="$2"
-	die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
+	die "fatal: the '$opt' flag does not make sense with 'git subtree $arg_command'."
 }
 
 main () {
@@ -155,7 +155,7 @@ main () {
 		allow_addmerge=$arg_split_rejoin
 		;;
 	*)
-		die "Unknown command '$arg_command'"
+		die "fatal: unknown command '$arg_command'"
 		;;
 	esac
 	# Reset the arguments array for "real" flag parsing.
@@ -244,7 +244,7 @@ main () {
 			break
 			;;
 		*)
-			die "Unexpected option: $opt"
+			die "fatal: unexpected option: $opt"
 			;;
 		esac
 	done
@@ -252,17 +252,17 @@ main () {
 
 	if test -z "$arg_prefix"
 	then
-		die "You must provide the --prefix option."
+		die "fatal: you must provide the --prefix option."
 	fi
 
 	case "$arg_command" in
 	add)
 		test -e "$arg_prefix" &&
-			die "prefix '$arg_prefix' already exists."
+			die "fatal: prefix '$arg_prefix' already exists."
 		;;
 	*)
 		test -e "$arg_prefix" ||
-			die "'$arg_prefix' does not exist; use 'git subtree add'"
+			die "fatal: '$arg_prefix' does not exist; use 'git subtree add'"
 		;;
 	esac
 
@@ -282,11 +282,11 @@ cache_setup () {
 	assert test $# = 0
 	cachedir="$GIT_DIR/subtree-cache/$$"
 	rm -rf "$cachedir" ||
-		die "Can't delete old cachedir: $cachedir"
+		die "fatal: can't delete old cachedir: $cachedir"
 	mkdir -p "$cachedir" ||
-		die "Can't create new cachedir: $cachedir"
+		die "fatal: can't create new cachedir: $cachedir"
 	mkdir -p "$cachedir/notree" ||
-		die "Can't create new cachedir: $cachedir/notree"
+		die "fatal: can't create new cachedir: $cachedir/notree"
 	debug "Using cachedir: $cachedir" >&2
 }
 
@@ -342,7 +342,7 @@ cache_set () {
 		test "$oldrev" != "latest_new" &&
 		test -e "$cachedir/$oldrev"
 	then
-		die "cache for $oldrev already exists!"
+		die "fatal: cache for $oldrev already exists!"
 	fi
 	echo "$newrev" >"$cachedir/$oldrev"
 }
@@ -396,7 +396,7 @@ find_latest_squash () {
 			;;
 		git-subtree-split:)
 			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
-			die "could not rev-parse split hash $b from commit $sq"
+			die "fatal: could not rev-parse split hash $b from commit $sq"
 			;;
 		END)
 			if test -n "$sub"
@@ -448,7 +448,7 @@ find_existing_splits () {
 			;;
 		git-subtree-split:)
 			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
-			die "could not rev-parse split hash $b from commit $sq"
+			die "fatal: could not rev-parse split hash $b from commit $sq"
 			;;
 		END)
 			debug "Main is: '$main'"
@@ -498,7 +498,7 @@ copy_commit () {
 			cat
 		) |
 		git commit-tree "$2" $3  # reads the rest of stdin
-	) || die "Can't copy commit $1"
+	) || die "fatal: can't copy commit $1"
 }
 
 # Usage: add_msg DIR LATEST_OLD LATEST_NEW
@@ -726,11 +726,11 @@ ensure_clean () {
 	assert test $# = 0
 	if ! git diff-index HEAD --exit-code --quiet 2>&1
 	then
-		die "Working tree has modifications.  Cannot add."
+		die "fatal: working tree has modifications.  Cannot add."
 	fi
 	if ! git diff-index --cached HEAD --exit-code --quiet 2>&1
 	then
-		die "Index has modifications.  Cannot add."
+		die "fatal: index has modifications.  Cannot add."
 	fi
 }
 
@@ -738,7 +738,7 @@ ensure_clean () {
 ensure_valid_ref_format () {
 	assert test $# = 1
 	git check-ref-format "refs/heads/$1" ||
-		die "'$1' does not look like a ref"
+		die "fatal: '$1' does not look like a ref"
 }
 
 # Usage: process_split_commit REV PARENTS
@@ -804,7 +804,7 @@ cmd_add () {
 	if test $# -eq 1
 	then
 		git rev-parse -q --verify "$1^{commit}" >/dev/null ||
-			die "'$1' does not refer to a commit"
+			die "fatal: '$1' does not refer to a commit"
 
 		cmd_add_commit "$@"
 
@@ -819,7 +819,7 @@ cmd_add () {
 
 		cmd_add_repository "$@"
 	else
-		say >&2 "error: parameters were '$*'"
+		say >&2 "fatal: parameters were '$*'"
 		die "Provide either a commit or a repository and commit."
 	fi
 }
@@ -882,9 +882,9 @@ cmd_split () {
 	elif test $# -eq 1
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
-			die "'$1' does not refer to a commit"
+			die "fatal: '$1' does not refer to a commit"
 	else
-		die "You must provide exactly one revision.  Got: '$*'"
+		die "fatal: you must provide exactly one revision.  Got: '$*'"
 	fi
 
 	if test -n "$arg_split_rejoin"
@@ -927,7 +927,7 @@ cmd_split () {
 	latest_new=$(cache_get latest_new) || exit $?
 	if test -z "$latest_new"
 	then
-		die "No new revisions were found"
+		die "fatal: no new revisions were found"
 	fi
 
 	if test -n "$arg_split_rejoin"
@@ -948,7 +948,7 @@ cmd_split () {
 		then
 			if ! git merge-base --is-ancestor "$arg_split_branch" "$latest_new"
 			then
-				die "Branch '$arg_split_branch' is not an ancestor of commit '$latest_new'."
+				die "fatal: branch '$arg_split_branch' is not an ancestor of commit '$latest_new'."
 			fi
 			action='Updated'
 		else
@@ -965,9 +965,9 @@ cmd_split () {
 # Usage: cmd_merge REV
 cmd_merge () {
 	test $# -eq 1 ||
-		die "You must provide exactly one revision.  Got: '$*'"
+		die "fatal: you must provide exactly one revision.  Got: '$*'"
 	rev=$(git rev-parse -q --verify "$1^{commit}") ||
-		die "'$1' does not refer to a commit"
+		die "fatal: '$1' does not refer to a commit"
 	ensure_clean
 
 	if test -n "$arg_addmerge_squash"
@@ -975,7 +975,7 @@ cmd_merge () {
 		first_split="$(find_latest_squash "$dir")" || exit $?
 		if test -z "$first_split"
 		then
-			die "Can't squash-merge: '$dir' was never added."
+			die "fatal: can't squash-merge: '$dir' was never added."
 		fi
 		set $first_split
 		old=$1
@@ -1003,7 +1003,7 @@ cmd_merge () {
 cmd_pull () {
 	if test $# -ne 2
 	then
-		die "You must provide <repository> <ref>"
+		die "fatal: you must provide <repository> <ref>"
 	fi
 	ensure_clean
 	ensure_valid_ref_format "$2"
@@ -1015,7 +1015,7 @@ cmd_pull () {
 cmd_push () {
 	if test $# -ne 2
 	then
-		die "You must provide <repository> <refspec>"
+		die "fatal: you must provide <repository> <refspec>"
 	fi
 	if test -e "$dir"
 	then
@@ -1030,13 +1030,13 @@ cmd_push () {
 		fi
 		ensure_valid_ref_format "$remoteref"
 		localrev_presplit=$(git rev-parse -q --verify "$localrevname_presplit^{commit}") ||
-			die "'$localrevname_presplit' does not refer to a commit"
+			die "fatal: '$localrevname_presplit' does not refer to a commit"
 
 		echo "git push using: " "$repository" "$refspec"
 		localrev=$(cmd_split "$localrev_presplit") || die
 		git push "$repository" "$localrev":"refs/heads/$remoteref"
 	else
-		die "'$dir' must already exist. Try 'git subtree add'."
+		die "fatal: '$dir' must already exist. Try 'git subtree add'."
 	fi
 }
 
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 1c1f76f04aa..249743ab9aa 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -277,7 +277,7 @@ test_expect_success 'split requires option --prefix' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" FETCH_HEAD &&
-		echo "You must provide the --prefix option." >expected &&
+		echo "fatal: you must provide the --prefix option." >expected &&
 		test_must_fail git subtree split >actual 2>&1 &&
 		test_debug "printf '"expected: "'" &&
 		test_debug "cat expected" &&
@@ -296,7 +296,7 @@ test_expect_success 'split requires path given by option --prefix must exist' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" FETCH_HEAD &&
-		echo "'\''non-existent-directory'\'' does not exist; use '\''git subtree add'\''" >expected &&
+		echo "fatal: '\''non-existent-directory'\'' does not exist; use '\''git subtree add'\''" >expected &&
 		test_must_fail git subtree split --prefix=non-existent-directory >actual 2>&1 &&
 		test_debug "printf '"expected: "'" &&
 		test_debug "cat expected" &&
@@ -570,7 +570,7 @@ test_expect_success 'pull requires option --prefix' '
 		cd "$test_count" &&
 		test_must_fail git subtree pull ./"sub proj" HEAD >out 2>err &&
 
-		echo "You must provide the --prefix option." >expected &&
+		echo "fatal: you must provide the --prefix option." >expected &&
 		test_must_be_empty out &&
 		test_cmp expected err
 	)
@@ -584,7 +584,7 @@ test_expect_success 'pull requires path given by option --prefix must exist' '
 	(
 		test_must_fail git subtree pull --prefix="sub dir" ./"sub proj" HEAD >out 2>err &&
 
-		echo "'\''sub dir'\'' does not exist; use '\''git subtree add'\''" >expected &&
+		echo "fatal: '\''sub dir'\'' does not exist; use '\''git subtree add'\''" >expected &&
 		test_must_be_empty out &&
 		test_cmp expected err
 	)
@@ -643,7 +643,7 @@ test_expect_success 'push requires option --prefix' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" FETCH_HEAD &&
-		echo "You must provide the --prefix option." >expected &&
+		echo "fatal: you must provide the --prefix option." >expected &&
 		test_must_fail git subtree push "./sub proj" from-mainline >actual 2>&1 &&
 		test_debug "printf '"expected: "'" &&
 		test_debug "cat expected" &&
@@ -662,7 +662,7 @@ test_expect_success 'push requires path given by option --prefix must exist' '
 		cd "$test_count" &&
 		git fetch ./"sub proj" HEAD &&
 		git subtree add --prefix="sub dir" FETCH_HEAD &&
-		echo "'\''non-existent-directory'\'' does not exist; use '\''git subtree add'\''" >expected &&
+		echo "fatal: '\''non-existent-directory'\'' does not exist; use '\''git subtree add'\''" >expected &&
 		test_must_fail git subtree push --prefix=non-existent-directory "./sub proj" from-mainline >actual 2>&1 &&
 		test_debug "printf '"expected: "'" &&
 		test_debug "cat expected" &&
-- 
gitgitgadget

[PATCH 5/9] subtree: define a variable before its first use in 'find_latest_squash'

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

The function 'find_latest_squash' takes a single argument, 'dir', but a
debug statement uses this variable before it takes its value from $1.

This statement thus gets the value of 'dir' from the calling function,
which currently is the same as the 'dir' argument, so it works but it
is confusing.

Move the definition of 'dir' before its first use.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 89f1eb756f0..d91a9679075 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -374,10 +374,10 @@ try_remove_previous () {
 # Usage: find_latest_squash DIR
 find_latest_squash () {
 	assert test $# = 1
+	dir="$1"
 	debug "Looking for latest squash ($dir)..."
 	local indent=$(($indent + 1))
 
-	dir="$1"
 	sq=
 	main=
 	sub=
-- 
gitgitgadget

[PATCH 6/9] subtree: use named variables instead of "$@" in cmd_pull

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

'cmd_pull' already checks that only two arguments are given,
'repository' and 'ref'. Define variables with these names instead of
using the positional parameter $2 and "$@".

This will allow a subsequent commit to pass 'repository' to 'cmd_merge'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index d91a9679075..2b3c429991b 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -1005,9 +1005,11 @@ cmd_pull () {
 	then
 		die "fatal: you must provide <repository> <ref>"
 	fi
+	repository="$1"
+	ref="$2"
 	ensure_clean
-	ensure_valid_ref_format "$2"
-	git fetch "$@" || exit $?
+	ensure_valid_ref_format "$ref"
+	git fetch "$repository" "$ref" || exit $?
 	cmd_merge FETCH_HEAD
 }
 
-- 
gitgitgadget

[PATCH 7/9] subtree: process 'git-subtree-split' trailer in separate function

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

Both functions 'find_latest_squash' (called by 'git subtree merge
--squash' and 'git subtree split --rejoin') and 'find_existing_splits'
(called by git 'subtree split') loop through commits that have a
'git-subtree-dir' trailer, and then process the 'git-subtree-mainline'
and 'git-subtree-split' trailers for those commits.

The processing done for the 'git-subtree-split' trailer is simple: we
check if the object exists with 'rev-parse' and set the variable
'sub' to the object name, or we die if the object does not exist.

In a future commit we will add more steps to the processing of this
trailer in order to make the code more robust.

To reduce code duplication, move the processing of the
'git-subtree-split' trailer to a dedicated function,
'process_subtree_split_trailer'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 2b3c429991b..b90ca0036f7 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -371,6 +371,15 @@ try_remove_previous () {
 	fi
 }
 
+# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH
+process_subtree_split_trailer () {
+	assert test $# = 2
+	b="$1"
+	sq="$2"
+	sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
+	die "fatal: could not rev-parse split hash $b from commit $sq"
+}
+
 # Usage: find_latest_squash DIR
 find_latest_squash () {
 	assert test $# = 1
@@ -395,8 +404,7 @@ find_latest_squash () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
-			die "fatal: could not rev-parse split hash $b from commit $sq"
+			process_subtree_split_trailer "$b" "$sq"
 			;;
 		END)
 			if test -n "$sub"
@@ -447,8 +455,7 @@ find_existing_splits () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
-			die "fatal: could not rev-parse split hash $b from commit $sq"
+			process_subtree_split_trailer "$b" "$sq"
 			;;
 		END)
 			debug "Main is: '$main'"
-- 
gitgitgadget

[PATCH 8/9] subtree: fix squash merging after annotated tag was squashed merged

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

When 'git subtree merge --squash $ref' is invoked, either directly or
through 'git subtree pull --squash $repo $ref', the code looks for the
latest squash merge of the subtree in order to create the new merge
commit as a child of the previous squash merge.

This search is done in function 'process_subtree_split_trailer', invoked
by 'find_latest_squash', which looks for the most recent commit with a
'git-subtree-split' trailer; that trailer's value is the object name in
the subtree repository of the ref that was last squash-merged. The
function verifies that this object is present locally with 'git
rev-parse', and aborts if it's not.

The hash referenced by the 'git-subtree-split' trailer is guaranteed to
correspond to a commit since it is the result of running 'git rev-parse
-q --verify "$1^{commit}"' on the first argument of 'cmd_merge' (this
corresponds to 'rev' in 'cmd_merge' which is passed through to
'new_squash_commit' and 'squash_msg').

But this is only the case since e4f8baa88a (subtree: parse revs in
individual cmd_ functions, 2021-04-27), which went into Git 2.32. Before
that commit, 'cmd_merge' verified the revision it was given using 'git
rev-parse --revs-only "$@"'. Such an invocation, when fed the name of an
annotated tag, would return the hash of the tag, not of the commit
referenced by the tag.

This leads to a failure in 'find_latest_squash' when squash-merging if
the most recent squash-merge merged an annotated tag of the subtree
repository, using a pre-2.32 version of 'git subtree', unless that
previous annotated tag is present locally (which is not usually the
case).

We can fix this by fetching the object directly by its hash in
'process_subtree_split_trailer' when 'git rev-parse' fails, but in order
to do so we need to know the name or URL of the subtree repository.
This is not possible in general for 'git subtree merge', but is easy
when it is invoked through 'git subtree pull' since in that case the
subtree repository is passed by the user at the command line.

Allow the 'git subtree pull' scenario to work out-of-the-box by adding
an optional 'repository' argument to functions 'cmd_merge',
'find_latest_squash' and 'process_subtree_split_trailer', and invoke
'cmd_merge' with that 'repository' argument in 'cmd_pull'.

If 'repository' is absent in 'process_subtree_split_trailer', instruct
the user to try fetching the missing object directly.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh     | 56 +++++++++++++++++++++++-------
 contrib/subtree/git-subtree.txt    |  9 +++--
 contrib/subtree/t/t7900-subtree.sh | 36 +++++++++++++++++++
 3 files changed, 86 insertions(+), 15 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b90ca0036f7..2c67989fe8a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -371,20 +371,45 @@ try_remove_previous () {
 	fi
 }
 
-# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH
+# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
 process_subtree_split_trailer () {
-	assert test $# = 2
+	assert test $# = 2 -o $# = 3
 	b="$1"
 	sq="$2"
-	sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
-	die "fatal: could not rev-parse split hash $b from commit $sq"
+	repository=""
+	if test "$#" = 3
+	then
+		repository="$3"
+	fi
+	fail_msg="fatal: could not rev-parse split hash $b from commit $sq"
+	if ! sub="$(git rev-parse --verify --quiet "$b^{commit}")"
+	then
+		# if 'repository' was given, try to fetch the 'git-subtree-split' hash
+		# before 'rev-parse'-ing it again, as it might be a tag that we do not have locally
+		if test -n "${repository}"
+		then
+			git fetch "$repository" "$b"
+			sub="$(git rev-parse --verify --quiet "$b^{commit}")" ||
+				die "$fail_msg"
+		else
+			hint1=$(printf "hint: hash might be a tag, try fetching it from the subtree repository:")
+			hint2=$(printf "hint:    git fetch <subtree-repository> $b")
+			fail_msg=$(printf "$fail_msg\n$hint1\n$hint2")
+			die "$fail_msg"
+		fi
+	fi
 }
 
-# Usage: find_latest_squash DIR
+# Usage: find_latest_squash DIR [REPOSITORY]
 find_latest_squash () {
-	assert test $# = 1
+	assert test $# = 1 -o $# = 2
 	dir="$1"
-	debug "Looking for latest squash ($dir)..."
+	repository=""
+	if test "$#" = 2
+	then
+		repository="$2"
+	fi
+	debug "Looking for latest squash (dir=$dir, repository=$repository)..."
 	local indent=$(($indent + 1))
 
 	sq=
@@ -404,7 +429,7 @@ find_latest_squash () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			process_subtree_split_trailer "$b" "$sq"
+			process_subtree_split_trailer "$b" "$sq" "$repository"
 			;;
 		END)
 			if test -n "$sub"
@@ -969,17 +994,22 @@ cmd_split () {
 	exit 0
 }
 
-# Usage: cmd_merge REV
+# Usage: cmd_merge REV [REPOSITORY]
 cmd_merge () {
-	test $# -eq 1 ||
-		die "fatal: you must provide exactly one revision.  Got: '$*'"
+	test $# -eq 1 -o $# -eq 2 ||
+		die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
 	rev=$(git rev-parse -q --verify "$1^{commit}") ||
 		die "fatal: '$1' does not refer to a commit"
+	repository=""
+	if test "$#" = 2
+	then
+		repository="$2"
+	fi
 	ensure_clean
 
 	if test -n "$arg_addmerge_squash"
 	then
-		first_split="$(find_latest_squash "$dir")" || exit $?
+		first_split="$(find_latest_squash "$dir" "$repository")" || exit $?
 		if test -z "$first_split"
 		then
 			die "fatal: can't squash-merge: '$dir' was never added."
@@ -1017,7 +1047,7 @@ cmd_pull () {
 	ensure_clean
 	ensure_valid_ref_format "$ref"
 	git fetch "$repository" "$ref" || exit $?
-	cmd_merge FETCH_HEAD
+	cmd_merge FETCH_HEAD "$repository"
 }
 
 # Usage: cmd_push REPOSITORY [+][LOCALREV:]REMOTEREF
diff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt
index 9cddfa26540..0e7524d7864 100644
--- a/contrib/subtree/git-subtree.txt
+++ b/contrib/subtree/git-subtree.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git subtree' [<options>] -P <prefix> add <local-commit>
 'git subtree' [<options>] -P <prefix> add <repository> <remote-ref>
-'git subtree' [<options>] -P <prefix> merge <local-commit>
+'git subtree' [<options>] -P <prefix> merge <local-commit> [<repository>]
 'git subtree' [<options>] -P <prefix> split [<local-commit>]
 
 [verse]
@@ -76,7 +76,7 @@ add <repository> <remote-ref>::
 	only a single commit from the subproject, rather than its
 	entire history.
 
-merge <local-commit>::
+merge <local-commit> [<repository>]::
 	Merge recent changes up to <local-commit> into the <prefix>
 	subtree.  As with normal 'git merge', this doesn't
 	remove your own local changes; it just merges those
@@ -88,6 +88,11 @@ If you use '--squash', the merge direction doesn't always have to be
 forward; you can use this command to go back in time from v2.5 to v2.4,
 for example.  If your merge introduces a conflict, you can resolve it in
 the usual ways.
++
+When using '--squash', and the previous merge with '--squash' merged an
+annotated tag of the subtree repository, that tag needs to be available locally.
+If <repository> is given, a missing tag will automatically be fetched from that
+repository.
 
 split [<local-commit>]::
 	Extract a new, synthetic project history from the
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 249743ab9aa..d0671676c7a 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -43,6 +43,30 @@ last_commit_subject () {
 	git log --pretty=format:%s -1
 }
 
+# Upon 'git subtree add|merge --squash' of an annotated tag,
+# pre-2.32.0 versions of 'git subtree' would write the hash of the tag
+# (sub1 below), instead of the commit (sub1^{commit}) in the
+# "git-subtree-split" trailer.
+# We immitate this behaviour below using a replace ref.
+# This function creates 3 repositories:
+# - $1
+# - $1-sub (added as subtree "sub" in $1)
+# - $1-clone (clone of $1)
+test_create_pre2_32_repo () {
+	subtree_test_create_repo "$1" &&
+	subtree_test_create_repo "$1-sub" &&
+	test_commit -C "$1" main1 &&
+	test_commit -C "$1-sub" --annotate sub1 &&
+	git -C "$1" subtree add --prefix="sub" --squash "../$1-sub" sub1 &&
+	tag=$(git -C "$1" rev-parse FETCH_HEAD) &&
+	commit=$(git -C "$1" rev-parse FETCH_HEAD^{commit}) &&
+	git -C "$1" log -1 --format=%B HEAD^2 >msg &&
+	test_commit -C "$1-sub" --annotate sub2 &&
+	git clone --no-local "$1" "$1-clone" &&
+	new_commit=$(cat msg | sed -e "s/$commit/$tag/" | git -C "$1-clone" commit-tree HEAD^2^{tree}) &&
+	git -C "$1-clone" replace HEAD^2 $new_commit
+}
+
 test_expect_success 'shows short help text for -h' '
 	test_expect_code 129 git subtree -h >out 2>err &&
 	test_must_be_empty err &&
@@ -264,6 +288,13 @@ test_expect_success 'merge new subproj history into subdir/ with a slash appende
 	)
 '
 
+test_expect_success 'merge with --squash after annotated tag was added/merged with --squash pre-v2.32.0 ' '
+	test_create_pre2_32_repo "$test_count" &&
+	git -C "$test_count-clone" fetch "../$test_count-sub" sub2  &&
+	test_must_fail git -C "$test_count-clone" subtree merge --prefix="sub" --squash FETCH_HEAD &&
+	git -C "$test_count-clone" subtree merge --prefix="sub" --squash FETCH_HEAD  "../$test_count-sub"
+'
+
 #
 # Tests for 'git subtree split'
 #
@@ -630,6 +661,11 @@ test_expect_success 'pull rejects flags for split' '
 	)
 '
 
+test_expect_success 'pull with --squash after annotated tag was added/merged with --squash pre-v2.32.0 ' '
+	test_create_pre2_32_repo "$test_count" &&
+	git -C "$test_count-clone" subtree -d pull --prefix="sub" --squash "../$test_count-sub" sub2
+'
+
 #
 # Tests for 'git subtree push'
 #
-- 
gitgitgadget

[PATCH 9/9] subtree: fix split after annotated tag was squashed merged

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

The previous commit fixed a failure in 'git subtree merge --squash' when
the previous squash-merge merged an annotated tag of the subtree
repository which is missing locally.

The same failure happens in 'git subtree split', either directly or when
called by 'git subtree push', under the same circumstances: 'cmd_split'
invokes 'find_existing_splits', which loops through previous commits and
invokes 'git rev-parse' (via 'process_subtree_split_trailer') on the
value of any 'git subtree-split' trailer it finds. This fails if this
value is the hash of an annotated tag which is missing locally.

Add a new optional argument 'repository' to 'cmd_split' and
'find_existing_splits', and invoke 'cmd_split' with that argument from
'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch
the missing tag from the 'repository' if it's not available locally,
mirroring the new behaviour of 'git subtree pull' and 'git subtree
merge'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------
 contrib/subtree/git-subtree.txt    |  7 ++++++-
 contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 2c67989fe8a..10c9c87839a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -453,14 +453,19 @@ find_latest_squash () {
 	done || exit $?
 }
 
-# Usage: find_existing_splits DIR REV
+# Usage: find_existing_splits DIR REV [REPOSITORY]
 find_existing_splits () {
-	assert test $# = 2
+	assert test $# = 2 -o $# = 3
 	debug "Looking for prior splits..."
 	local indent=$(($indent + 1))
 
 	dir="$1"
 	rev="$2"
+	repository=""
+	if test "$#" = 3
+	then
+		repository="$3"
+	fi
 	main=
 	sub=
 	local grep_format="^git-subtree-dir: $dir/*\$"
@@ -480,7 +485,7 @@ find_existing_splits () {
 			main="$b"
 			;;
 		git-subtree-split:)
-			process_subtree_split_trailer "$b" "$sq"
+			process_subtree_split_trailer "$b" "$sq" "$repository"
 			;;
 		END)
 			debug "Main is: '$main'"
@@ -906,17 +911,22 @@ cmd_add_commit () {
 	say >&2 "Added dir '$dir'"
 }
 
-# Usage: cmd_split [REV]
+# Usage: cmd_split [REV] [REPOSITORY]
 cmd_split () {
 	if test $# -eq 0
 	then
 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1
+	elif test $# -eq 1 -o $# -eq 2
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"
 	else
-		die "fatal: you must provide exactly one revision.  Got: '$*'"
+		die "fatal: you must provide exactly one revision, and optionnally a repository.  Got: '$*'"
+	fi
+	repository=""
+	if test "$#" = 2
+	then
+		repository="$2"
 	fi
 
 	if test -n "$arg_split_rejoin"
@@ -940,7 +950,7 @@ cmd_split () {
 		done || exit $?
 	fi
 
-	unrevs="$(find_existing_splits "$dir" "$rev")" || exit $?
+	unrevs="$(find_existing_splits "$dir" "$rev" "$repository")" || exit $?
 
 	# We can't restrict rev-list to only $dir here, because some of our
 	# parents have the $dir contents the root, and those won't match.
@@ -1072,7 +1082,7 @@ cmd_push () {
 			die "fatal: '$localrevname_presplit' does not refer to a commit"
 
 		echo "git push using: " "$repository" "$refspec"
-		localrev=$(cmd_split "$localrev_presplit") || die
+		localrev=$(cmd_split "$localrev_presplit" "$repository") || die
 		git push "$repository" "$localrev":"refs/heads/$remoteref"
 	else
 		die "fatal: '$dir' must already exist. Try 'git subtree add'."
diff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt
index 0e7524d7864..004abf415b8 100644
--- a/contrib/subtree/git-subtree.txt
+++ b/contrib/subtree/git-subtree.txt
@@ -94,7 +94,7 @@ annotated tag of the subtree repository, that tag needs to be available locally.
 If <repository> is given, a missing tag will automatically be fetched from that
 repository.
 
-split [<local-commit>]::
+split [<local-commit>] [<repository>]::
 	Extract a new, synthetic project history from the
 	history of the <prefix> subtree of <local-commit>, or of
 	HEAD if no <local-commit> is given.  The new history
@@ -114,6 +114,11 @@ settings passed to 'split' (such as '--annotate') are the same.
 Because of this, if you add new commits and then re-split, the new
 commits will be attached as commits on top of the history you
 generated last time, so 'git merge' and friends will work as expected.
++
+When a previous merge with '--squash' merged an annotated tag of the
+subtree repository, that tag needs to be available locally.
+If <repository> is given, a missing tag will automatically be fetched from that
+repository.
 
 pull <repository> <remote-ref>::
 	Exactly like 'merge', but parallels 'git pull' in that
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index d0671676c7a..341c169eca7 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -582,6 +582,12 @@ test_expect_success 'split "sub dir"/ with --branch for an incompatible branch'
 	)
 '
 
+test_expect_success 'split after annotated tag was added/merged with --squash pre-v2.32.0' '
+	test_create_pre2_32_repo "$test_count" &&
+	test_must_fail git -C "$test_count-clone" subtree split --prefix="sub" HEAD &&
+	git -C "$test_count-clone" subtree split --prefix="sub" HEAD "../$test_count-sub"
+'
+
 #
 # Tests for 'git subtree pull'
 #
@@ -989,6 +995,12 @@ test_expect_success 'push "sub dir"/ with a local rev' '
 	)
 '
 
+test_expect_success 'push after annotated tag was added/merged with --squash pre-v2.32.0' '
+	test_create_pre2_32_repo "$test_count" &&
+	test_create_commit "$test_count-clone" sub/main-sub1 &&
+	git -C "$test_count-clone" subtree push --prefix="sub" "../$test_count-sub" from-mainline
+'
+
 #
 # Validity checking
 #
-- 
gitgitgadget

Re: [PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)
> added validation code to check that options given to 'git subtree <cmd>'
> made sense with the command being used.
>
> Refactor these checks by adding a 'die_incompatible_opt' function to
> reduce code duplication.

This looks good, but...

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 49ef493ef92..f5eab198c80 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -102,6 +102,14 @@ assert () {
>  	fi
>  }
>  
> +# Usage: die_incompatible_opt OPTION COMMAND
> +die_incompatible_opt () {
> +	assert test "$#" = 2
> +	opt="$1"
> +	arg_command="$2"
> +	die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
> +}
> +
>  main () {
>  	if test $# -eq 0
>  	then
> @@ -176,16 +184,16 @@ main () {
>  			arg_debug=1
>  			;;
>  		--annotate)
> -			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
> +			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
>  			arg_split_annotate="$1"
>  			shift
>  			;;

Since this is all in this form I wonder why not (maybe adding some "local" and/or "&&" too while at it):

	die_if_other_opt {
		assert test "$#" = 3
		other="$1"
                shift
                if test -z "$other"
		then
			return
		fi
		[...the rest of your version]
	}

Then:

>  		--no-annotate)
> -			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
> +			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"

Instead of this:

	die_if_other_opt "$allow_split" "$opt" "$arg_command"

Or actually just:

	die_if_other_opt "$allow_split"

Maybe you disagree, but since the function will see the variables & this
is purely a helper for this getopts parse loop I think it's fine just to
assume we can read "$opt" and "$arg_command" there..., so, urm, maybe just:

	test -n "$allow_split" || die_incompatible_opt

? :) Anyway, an easy bike shed, you should go for whatever variant you
prefer :) Thanks for indulging me.

Re: [PATCH 4/9] subtree: prefix die messages with 'fatal'

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Just as was done in 0008d12284 (submodule: prefix die messages with
> 'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages
> outputed by 'git-subtree.sh' more in line with the rest of the code base
> by prefixing them with "fatal: ", and do not capitalize their first
> letter.

I don't really care since we're unlikely to ever give git-subtree the
i18n treatment, so translators don't need to worry about the churn.

But given how few in-tree-users we have of "die" and "git-sh-setup" this
would be much shorter & future-proof as just e.g. (untested):
	
	diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
	index 7562a395c24..0d8f87c5a20 100755
	--- a/contrib/subtree/git-subtree.sh
	+++ b/contrib/subtree/git-subtree.sh
	@@ -25,6 +25,8 @@ then
	 	exit 126
	 fi
	 
	+GIT_SH_SETUP_DIE_PREFIX='fatal: '
	+
	 OPTS_SPEC="\
	 git subtree add   --prefix=<prefix> <commit>
	 git subtree add   --prefix=<prefix> <repository> <ref>
	diff --git a/git-sh-setup.sh b/git-sh-setup.sh
	index ce273fe0e48..81456d7266e 100644
	--- a/git-sh-setup.sh
	+++ b/git-sh-setup.sh
	@@ -53,7 +53,7 @@ die () {
	 die_with_status () {
	 	status=$1
	 	shift
	-	printf >&2 '%s\n' "$*"
	+	printf >&2 '%s%s\n' "$GIT_SH_SETUP_DIE_PREFIX" "$*"
	 	exit "$status"
	 }
	 
> -		die "assertion failed: $*"
> +		die "fatal: assertion failed: $*"

Then you could just leave this, but...

> -		die "Unknown command '$arg_command'"
> +		die "fatal: unknown command '$arg_command'"

...would still need to change these for the capitalization change.

Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The previous commit fixed a failure in 'git subtree merge --squash' when
> the previous squash-merge merged an annotated tag of the subtree
> repository which is missing locally.
>
> The same failure happens in 'git subtree split', either directly or when
> called by 'git subtree push', under the same circumstances: 'cmd_split'
> invokes 'find_existing_splits', which loops through previous commits and
> invokes 'git rev-parse' (via 'process_subtree_split_trailer') on the
> value of any 'git subtree-split' trailer it finds. This fails if this
> value is the hash of an annotated tag which is missing locally.
>
> Add a new optional argument 'repository' to 'cmd_split' and
> 'find_existing_splits', and invoke 'cmd_split' with that argument from
> 'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch
> the missing tag from the 'repository' if it's not available locally,
> mirroring the new behaviour of 'git subtree pull' and 'git subtree
> merge'.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------
>  contrib/subtree/git-subtree.txt    |  7 ++++++-
>  contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++
>  3 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 2c67989fe8a..10c9c87839a 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -453,14 +453,19 @@ find_latest_squash () {
>  	done || exit $?
>  }
>  
> -# Usage: find_existing_splits DIR REV
> +# Usage: find_existing_splits DIR REV [REPOSITORY]
>  find_existing_splits () {
> -	assert test $# = 2
> +	assert test $# = 2 -o $# = 3

This "test" syntax is considered unportable, I'm too lazy to dig up the
reference, but we've removed it in the past. Maybe it's OK with
git-subtree.sh", but anyway, it's esay enough to change...

...but looking at "master" I see one instance of it in git-subtree.sh
already, so maybe nobody cares...

Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged

[Eric Sunshine]

On Fri, Oct 21, 2022 at 12:48 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:
> > +# Usage: find_existing_splits DIR REV [REPOSITORY]
> >  find_existing_splits () {
> > +     assert test $# = 2 -o $# = 3
>
> This "test" syntax is considered unportable, I'm too lazy to dig up the
> reference, but we've removed it in the past. Maybe it's OK with
> git-subtree.sh", but anyway, it's esay enough to change...

You may be referring to POSIX[1] long considering -o/-a to be
obsolescent, and GNU warning[2] against them for a couple decades or
more.

[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16
[2]: https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Limitations-of-Builtins.html

Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'

[Junio C Hamano]

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Some variables in 'test_commit' have names that are common enough that
> it is very likely that test authors might use them in a test. If they do
> so and use 'test_commit' between setting such a variable and using it,
> the variable value from 'test_commit' will leak back into the test and
> most likely break it.
>
> Prevent that by marking all variables in 'test_commit' as 'local'. This
> allow a subsequent commit to use a 'tag' variable.

This is the right thing to do, if done onn day 1 of the project, and
it is the right thing to do for the longer term health of the
project.  But it is a bit scary thing to do in the middle.

I wonder if there is an easy way to detect that a set of callers of
test_commit are relying on the fact that calling test_commit without
passing --author option cleared their $author variable (similarly
for other variables that are cleared or set to a known value as a
side effect of calling test_commit).  If their correctness depends
on $author becoming empty after calling the script today, they will
get broken by this change.

While it is OK to argue that they deserve it, we would have to be
finding and fixing them ourselves after all.

> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  t/test-lib-functions.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 527a7145000..adc0fb6330c 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -273,13 +273,13 @@ debug () {
>  # <file>, <contents>, and <tag> all default to <message>.
>  
>  test_commit () {
> -	notick= &&
> -	echo=echo &&
> -	append= &&
> -	author= &&
> -	signoff= &&
> -	indir= &&
> -	tag=light &&
> +	local notick= &&
> +	local echo=echo &&
> +	local append= &&
> +	local author= &&
> +	local signoff= &&
> +	local indir= &&
> +	local tag=light &&
>  	while test $# != 0
>  	do
>  		case "$1" in
> @@ -322,7 +322,7 @@ test_commit () {
>  		shift
>  	done &&
>  	indir=${indir:+"$indir"/} &&
> -	file=${2:-"$1.t"} &&
> +	local file=${2:-"$1.t"} &&
>  	if test -n "$append"
>  	then
>  		$echo "${3-$1}" >>"$indir$file"

Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'

[Philippe Blain]

Hi Junio,

Le 2022-10-21 à 17:06, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> Some variables in 'test_commit' have names that are common enough that
>> it is very likely that test authors might use them in a test. If they do
>> so and use 'test_commit' between setting such a variable and using it,
>> the variable value from 'test_commit' will leak back into the test and
>> most likely break it.
>>
>> Prevent that by marking all variables in 'test_commit' as 'local'. This
>> allow a subsequent commit to use a 'tag' variable.
> 
> This is the right thing to do, if done onn day 1 of the project, and
> it is the right thing to do for the longer term health of the
> project.  But it is a bit scary thing to do in the middle.
> 
> I wonder if there is an easy way to detect that a set of callers of
> test_commit are relying on the fact that calling test_commit without
> passing --author option cleared their $author variable (similarly
> for other variables that are cleared or set to a known value as a
> side effect of calling test_commit).  If their correctness depends
> on $author becoming empty after calling the script today, they will
> get broken by this change.
> 
> While it is OK to argue that they deserve it, we would have to be
> finding and fixing them ourselves after all.

Isn't the fact that the test suite passes with this change enough for us
to be confident that nothing is broken by it ?

In any case, I did a quick manual grep of each of the variables and could not
find a test that uses these names, so I think we are safe.

Thanks,
Philippe.

Re: [PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication

[Philippe Blain]

Hi Ævar,

Le 2022-10-21 à 12:22, Ævar Arnfjörð Bjarmason a écrit :
> 
> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)
>> added validation code to check that options given to 'git subtree <cmd>'
>> made sense with the command being used.
>>
>> Refactor these checks by adding a 'die_incompatible_opt' function to
>> reduce code duplication.
> 
> This looks good, but...
> 
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------
>>  1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 49ef493ef92..f5eab198c80 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -102,6 +102,14 @@ assert () {
>>  	fi
>>  }
>>  
>> +# Usage: die_incompatible_opt OPTION COMMAND
>> +die_incompatible_opt () {
>> +	assert test "$#" = 2
>> +	opt="$1"
>> +	arg_command="$2"
>> +	die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
>> +}
>> +
>>  main () {
>>  	if test $# -eq 0
>>  	then
>> @@ -176,16 +184,16 @@ main () {
>>  			arg_debug=1
>>  			;;
>>  		--annotate)
>> -			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
>> +			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
>>  			arg_split_annotate="$1"
>>  			shift
>>  			;;
> 
> Since this is all in this form I wonder why not (maybe adding some "local" and/or "&&" too while at it):
> 
> 	die_if_other_opt {
> 		assert test "$#" = 3
> 		other="$1"
>                 shift
>                 if test -z "$other"
> 		then
> 			return
> 		fi
> 		[...the rest of your version]
> 	}
> 
> Then:
> 
>>  		--no-annotate)
>> -			test -n "$allow_split" || die "The '$opt' flag does not make sense with 'git subtree $arg_command'."
>> +			test -n "$allow_split" || die_incompatible_opt "$opt" "$arg_command"
> 
> Instead of this:
> 
> 	die_if_other_opt "$allow_split" "$opt" "$arg_command"
> 
> Or actually just:
> 
> 	die_if_other_opt "$allow_split"
> 
> Maybe you disagree, but since the function will see the variables & this
> is purely a helper for this getopts parse loop I think it's fine just to
> assume we can read "$opt" and "$arg_command" there..., so, urm, maybe just:
> 
> 	test -n "$allow_split" || die_incompatible_opt
> 
> ? :) Anyway, an easy bike shed, you should go for whatever variant you
> prefer :) Thanks for indulging me.
> 

Thanks for your suggestion, but I think I'll stick to what I have for now :)

Philippe.

Re: [PATCH 4/9] subtree: prefix die messages with 'fatal'

[Philippe Blain]

Le 2022-10-21 à 12:30, Ævar Arnfjörð Bjarmason a écrit :
> 
> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> Just as was done in 0008d12284 (submodule: prefix die messages with
>> 'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages
>> outputed by 'git-subtree.sh' more in line with the rest of the code base
>> by prefixing them with "fatal: ", and do not capitalize their first
>> letter.
> 
> I don't really care since we're unlikely to ever give git-subtree the
> i18n treatment, so translators don't need to worry about the churn.
> 
> But given how few in-tree-users we have of "die" and "git-sh-setup" this
> would be much shorter & future-proof as just e.g. (untested):
> 	
> 	diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> 	index 7562a395c24..0d8f87c5a20 100755
> 	--- a/contrib/subtree/git-subtree.sh
> 	+++ b/contrib/subtree/git-subtree.sh
> 	@@ -25,6 +25,8 @@ then
> 	 	exit 126
> 	 fi
> 	 
> 	+GIT_SH_SETUP_DIE_PREFIX='fatal: '
> 	+
> 	 OPTS_SPEC="\
> 	 git subtree add   --prefix=<prefix> <commit>
> 	 git subtree add   --prefix=<prefix> <repository> <ref>
> 	diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> 	index ce273fe0e48..81456d7266e 100644
> 	--- a/git-sh-setup.sh
> 	+++ b/git-sh-setup.sh
> 	@@ -53,7 +53,7 @@ die () {
> 	 die_with_status () {
> 	 	status=$1
> 	 	shift
> 	-	printf >&2 '%s\n' "$*"
> 	+	printf >&2 '%s%s\n' "$GIT_SH_SETUP_DIE_PREFIX" "$*"
> 	 	exit "$status"
> 	 }
> 	 
>> -		die "assertion failed: $*"
>> +		die "fatal: assertion failed: $*"
> 
> Then you could just leave this, but...
> 
>> -		die "Unknown command '$arg_command'"
>> +		die "fatal: unknown command '$arg_command'"
> 
> ...would still need to change these for the capitalization change.
>

Yeah, I guess to this could be done in a later refactor if someone wishes.

It would probably require changes to other tests scripts (since there are other users
of 'die'), though, and so would widen the scope of the series a bit too much, I would say. 

Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged

[Philippe Blain]

Le 2022-10-21 à 12:37, Ævar Arnfjörð Bjarmason a écrit :
> 
> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:
> 
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> The previous commit fixed a failure in 'git subtree merge --squash' when
>> the previous squash-merge merged an annotated tag of the subtree
>> repository which is missing locally.
>>
>> The same failure happens in 'git subtree split', either directly or when
>> called by 'git subtree push', under the same circumstances: 'cmd_split'
>> invokes 'find_existing_splits', which loops through previous commits and
>> invokes 'git rev-parse' (via 'process_subtree_split_trailer') on the
>> value of any 'git subtree-split' trailer it finds. This fails if this
>> value is the hash of an annotated tag which is missing locally.
>>
>> Add a new optional argument 'repository' to 'cmd_split' and
>> 'find_existing_splits', and invoke 'cmd_split' with that argument from
>> 'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch
>> the missing tag from the 'repository' if it's not available locally,
>> mirroring the new behaviour of 'git subtree pull' and 'git subtree
>> merge'.
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------
>>  contrib/subtree/git-subtree.txt    |  7 ++++++-
>>  contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++
>>  3 files changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 2c67989fe8a..10c9c87839a 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -453,14 +453,19 @@ find_latest_squash () {
>>  	done || exit $?
>>  }
>>  
>> -# Usage: find_existing_splits DIR REV
>> +# Usage: find_existing_splits DIR REV [REPOSITORY]
>>  find_existing_splits () {
>> -	assert test $# = 2
>> +	assert test $# = 2 -o $# = 3
> 
> This "test" syntax is considered unportable, I'm too lazy to dig up the
> reference, but we've removed it in the past. Maybe it's OK with
> git-subtree.sh", but anyway, it's esay enough to change...
> 
> ...but looking at "master" I see one instance of it in git-subtree.sh
> already, so maybe nobody cares...
> 

I did not know it was not portable, in fact I used this form because while
reading the rest of the script I found that was the style used already. 

So I guess I would leave this as a further cleanup for someone wishing to make
'git subtree' more POSIX...

Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>> While it is OK to argue that they deserve it, we would have to be
>> finding and fixing them ourselves after all.
>
> Isn't the fact that the test suite passes with this change enough for us
> to be confident that nothing is broken by it ?

I am not brave enough to claim that, actually.

But even if the tests were subtly changing their behaviour and no
longer testing what they originally meant to test, I think that is
OK.  It will not be the first time for us to later find out that
some tests were passing for a wrong reason, or some that expect
failure were failing for a wrong reason.  We had multiple such cases
we later found and fixed, so even if these "local" introduced changes
in behaviour to some tests, I am OK ;-).