diff mbox series

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

Message ID 20210228195414.21372-5-avarab@gmail.com (mailing list archive)
State New
Headers show
Series describe: dont abort too early when searching tags | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 28, 2021, 7:54 p.m. UTC
Add an --annotated-tag option to test_commit. The tag will share the
same message as the commit, and we'll call test_tick before creating
it (unless --notick) is provided.

There's quite a few tests that could be simplified with this
construct. I've picked one to convert in this change as a
demonstration.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

Comments

Junio C Hamano March 1, 2021, 9:41 p.m. UTC | #1
Æ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).

Thanks.

> @@ -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
>  }
Ævar Arnfjörð Bjarmason March 2, 2021, 9:34 a.m. UTC | #2
On Mon, Mar 01 2021, Junio C Hamano wrote:

> Æ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".


>> @@ -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
>>  }
Junio C Hamano March 3, 2021, 6:35 a.m. UTC | #3
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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 :)

As things that are not yet in 'master' will not be merged down for a
few weeks anyway, the conflicts don't matter that much---it is a
valid choice not to accept new topics that conflicts with topics
cooking in 'next' after all.  That way, the number of topics that
folks have to look at will not increase, which gives more opportunity
for topics that are already in 'seen' to get reviewed ;-)
diff mbox series

Patch

diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 6ce62f878c..7c873033e9 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -7,11 +7,9 @@  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 test_expect_success setup '
-	test_commit A &&
-	git tag -f -a -m "annotated A" A &&
+	test_commit --annotated-tag A &&
 	git checkout -b side &&
-	test_commit B &&
-	git tag -f -a -m "annotated B" B &&
+	test_commit --annotated-tag B &&
 	git checkout main &&
 	test_commit C &&
 	git branch B A^0
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d733..c6cdabf53e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -171,6 +171,10 @@  debug () {
 #	Run all git commands in directory <dir>
 #   --notick
 #	Do not call test_tick before making a commit
+#   --annotated-tag
+#	Create an annotated tag with "-a -m <message>". Calls
+#	test_tick between making the commit and tag unless --notick is
+#	given.
 #   --append
 #	Use "echo >>" instead of "echo >" when writing "<contents>" to
 #	"<file>"
@@ -191,6 +195,7 @@  test_commit () {
 	signoff= &&
 	indir= &&
 	no_tag= &&
+	annotated_tag= &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -220,6 +225,9 @@  test_commit () {
 		--no-tag)
 			no_tag=yes
 			;;
+		--annotated-tag)
+			annotated_tag=yes
+			;;
 		*)
 			break
 			;;
@@ -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
 }