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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 2, 2021, 09:34 UTC
Message-ID: <87czwhkif6.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqr1kyo8l8.fsf@gitster.c.googlers.com>

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

Show 47 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> @@ -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".

Show 16 quoted lines

>> @@ -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
>>  }