Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to "test_commit"

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

A new option is welcome, but shouldn't we be straightening out the
variables that keep track of the options around tagging?  It's not
like it is possible to have 4 (two variables that can independently
set to 'yes') possibilities, so something along the lines of ...

         test_commit () {
        +	tag=light &&
                notick= &&
         ...
		while test $# != 0
		do
			case "$1" in
			...
			--no-tag)
	-			no_tag=yes
	+			tag=none
				;;
	+		--annotated)
	+			tag=annotated
	+			;;
		...
	-	if test -z "$no_tag"
	-	then
	+	case "$tag" in
	+	none)
	+		;;
	+	light)
			git ${indir:+ -C "$indir") tag "${4:-$1}"
	+		;;
	+	annotated)
	+		git ${indir:+ -C "$indir") tag -m "$1" "${4:-$1}"
	+		;;
	+	esac
		...

after this step (meaning, we may want to start from fixing the no_tag=yes
to tag=none before introducing this new feature).

Thanks.

On Mon, Mar 01 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

@@ -220,6 +225,9 @@ test_commit () {
 		--no-tag)
 			no_tag=yes
 			;;
+		--annotated-tag)
+			annotated_tag=yes
+			;;

A new option is welcome, but shouldn't we be straightening out the
variables that keep track of the options around tagging?  It's not
like it is possible to have 4 (two variables that can independently
set to 'yes') possibilities, so something along the lines of ...

         test_commit () {
        +	tag=light &&
                notick= &&
         ...
		while test $# != 0
		do
			case "$1" in
			...
			--no-tag)
	-			no_tag=yes
	+			tag=none
				;;
	+		--annotated)
	+			tag=annotated
	+			;;
		...
	-	if test -z "$no_tag"
	-	then
	+	case "$tag" in
	+	none)
	+		;;
	+	light)
			git ${indir:+ -C "$indir") tag "${4:-$1}"
	+		;;
	+	annotated)
	+		git ${indir:+ -C "$indir") tag -m "$1" "${4:-$1}"
	+		;;
	+	esac
		...

after this step (meaning, we may want to start from fixing the no_tag=yes
to tag=none before introducing this new feature).

Yeah, as noted in the last paragraph of the commit message:
    
    The placement of --annotated-tag after "notick" in the case of the
    documentation, and then after "no_tag" in the case of the code is
    slightly inconsistent. It's to evade a merge conflict with two other
    commits adding a --printf option, and another one adding documentation
    for --no-tag.

So the existing patch + not doing a bigger refactoring is because I
didn't want to cause conflicts for you to solve with other in-flight
topics. It would be easier with ab/pickaxe-pcre2 merged down :)

I'd prefer to keep this as-is for now, and just refactor this function a
bit after the current topics land.

In particular I'd like to make the the "message file contents tags"
arguments optionally have --long-option versions, so e.g. tests that
need a separate --commit-message and --tag-message can use the helper,
there's also quite a few that could use a --file=<existing file>
v.s. echo-ing a "message" to a "file".

quoted

@@ -244,7 +252,15 @@ test_commit () {
 	    $signoff -m "$1" &&
 	if test -z "$no_tag"
 	then
-		git ${indir:+ -C "$indir"} tag "${4:-$1}"
+		if test -n "$annotated_tag"
+		then
+			if test -z "$notick"
+			then
+				test_tick
+			fi &&
+			test_tick
+		fi &&
+		git ${indir:+ -C "$indir"} tag ${annotated_tag:+ -a -m "$1"} "${4:-$1}"
 	fi
 }