Re: [PATCH 1/3] tag: prevent recursive tags

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 26, 2019, 10:10 UTC
Message-ID: <87imw6aquw.fsf@evledraar.gmail.com>
In-Reply-To: <c371a653b4049256f3427e467b144385ee47ef43.1553586707.git.liu.denton@gmail.com>

On Tue, Mar 26 2019, Denton Liu wrote:

Show 11 quoted lines

>  static const char * const git_tag_usage[] = {
> -	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
> +	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [--allow-recursive-tag]\n"
> +		"\t\t<tagname> [<head>]"),
>  	N_("git tag -d <tagname>..."),
> -	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]"
> -		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
> +	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
> +		"\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
>  	N_("git tag -v [--format=<format>] <tagname>..."),
>  	NULL

Better to split the refactoring part of this into a leading change. I.e. the latter 2 removed/2 added here, and the former could also start with a wrap to 79 chars...

>  	type = oid_object_info(the_repository, object, NULL);
>  	if (type <= OBJ_NONE)
> -	    die(_("bad object type."));
> +		die(_("bad object type."));

Ditto this 4 space -> tab change.

Show 7 quoted lines

> +
> +	if (type == OBJ_TAG && !opt->allow_recursive_tag) {
> +		error(_("refusing to make a recursive tag"));
> +		if (advice_recursive_tag)
> +			advise(_(message_advice_recursive_tag), tag, tag, tag);
> +		exit(1);
> +	}

This patch of mine didn't end up making it in, but shows how instead of "tag, tag, tag" here you can just pass it once and use positional printf arguments in the message. It makes things a lot more self-explanatory: https://public-inbox.org/git/20181026192734.9609-8-avarab@gmail.com/

Show 136 quoted lines

>
>  	strbuf_addf(&header,
>  		    "object %s\n"
> @@ -403,6 +423,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  					N_("use another key to sign the tag")),
>  		OPT__FORCE(&force, N_("replace the tag if exists"), 0),
>  		OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a reflog")),
> +		OPT_BOOL(0, "allow-recursive-tag", &opt.allow_recursive_tag,
> +					N_("allow recursive tags to be made")),
>
>  		OPT_GROUP(N_("Tag listing options")),
>  		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> index 6da48a2e0a..841f922e07 100644
> --- a/t/annotate-tests.sh
> +++ b/t/annotate-tests.sh
> @@ -70,7 +70,7 @@ test_expect_success 'blame 1 author' '
>
>  test_expect_success 'blame by tag objects' '
>  	git tag -m "test tag" testTag &&
> -	git tag -m "test tag #2" testTag2 testTag &&
> +	git tag -m "test tag #2" --allow-recursive-tag testTag2 testTag &&
>  	check_count -h testTag A 2 &&
>  	check_count -h testTag2 A 2
>  '
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index bce02788e6..5f06c2d76f 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -16,7 +16,7 @@ pack_as_from_promisor () {
>
>  promise_and_delete () {
>  	HASH=$(git -C repo rev-parse "$1") &&
> -	git -C repo tag -a -m message my_annotated_tag "$HASH" &&
> +	git -C repo tag -a -m message my_annotated_tag --allow-recursive-tag "$HASH" &&
>  	git -C repo rev-parse my_annotated_tag | pack_as_from_promisor &&
>  	# tag -d prints a message to stdout, so redirect it
>  	git -C repo tag -d my_annotated_tag >/dev/null &&
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index f42a69faa2..018550f3b2 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -511,7 +511,7 @@ test_expect_success 'set up log decoration tests' '
>
>  test_expect_success 'log decoration properly follows tag chain' '
>  	git tag -a tag1 -m tag1 &&
> -	git tag -a tag2 -m tag2 tag1 &&
> +	git tag -a tag2 -m tag2 --allow-recursive-tag tag1 &&
>  	git tag -d tag1 &&
>  	git commit --amend -m shorter &&
>  	git log --no-walk --tags --pretty="%H %d" --decorate=full >actual &&
> diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh
> index a5eca210b8..c99850c1c0 100755
> --- a/t/t5305-include-tag.sh
> +++ b/t/t5305-include-tag.sh
> @@ -68,7 +68,7 @@ test_expect_success 'check unpacked result (have commit, have tag)' '
>  test_expect_success 'create hidden inner tag' '
>  	test_commit commit &&
>  	git tag -m inner inner HEAD &&
> -	git tag -m outer outer inner &&
> +	git tag -m outer --allow-recursive-tag outer inner &&
>  	git tag -d inner
>  '
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 49c540b1e1..c549b37aec 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -562,7 +562,7 @@ test_expect_success 'test --all wrt tag to non-commits' '
>  		hello tag
>  	EOF
>  	) &&
> -	git tag -a -m "tag -> tag" tag-to-tag $tag &&
> +	git tag -a -m "tag -> tag" --allow-recursive-tag tag-to-tag $tag &&
>
>  	# `fetch-pack --all` should succeed fetching all those objects.
>  	mkdir fetchall &&
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fc067ed672..f7b56ae195 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -12,7 +12,7 @@ test_expect_success 'setup some history and refs' '
>  	git checkout -b side &&
>  	test_commit four &&
>  	git tag -m "An annotated tag" annotated-tag &&
> -	git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
> +	git tag -m "Annonated doubly" --allow-recursive-tag doubly-annotated-tag annotated-tag &&
>
>  	# Note that these "signed" tags might not actually be signed.
>  	# Tests which care about the distinction should be marked
> @@ -24,7 +24,7 @@ test_expect_success 'setup some history and refs' '
>  		sign=
>  	fi &&
>  	git tag $sign -m "A signed tag" signed-tag &&
> -	git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
> +	git tag $sign -m "Signed doubly" --allow-recursive-tag doubly-signed-tag signed-tag &&
>
>  	git checkout master &&
>  	git update-ref refs/odd/spot master
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 0b01862c23..7a7c0ccee9 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1265,7 +1265,7 @@ echo "A message for another tag" >>expect
>  echo '-----BEGIN PGP SIGNATURE-----' >>expect
>  test_expect_success GPG \
>  	'creating a signed tag pointing to another tag should succeed' '
> -	git tag -s -m "A message for another tag" tag-signed-tag signed-tag &&
> +	git tag -s -m "A message for another tag" --allow-recursive-tag tag-signed-tag signed-tag &&
>  	get_tag_msg tag-signed-tag >actual &&
>  	test_cmp expect actual
>  '
> @@ -1690,7 +1690,7 @@ test_expect_success '--points-at finds annotated tags of commits' '
>  '
>
>  test_expect_success '--points-at finds annotated tags of tags' '
> -	git tag -m "describing the v4.0 tag object" \
> +	git tag -m "describing the v4.0 tag object" --allow-recursive-tag \
>  		annotated-again-v4.0 annotated-v4.0 &&
>  	cat >expect <<-\EOF &&
>  	annotated-again-v4.0
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 5690fe2810..b5ed7e119a 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -441,8 +441,8 @@ test_expect_success 'set-up a few more tags for tag export tests' '
>  	HEAD_TREE=$(git show -s --pretty=raw HEAD | grep tree | sed "s/tree //") &&
>  	git tag    tree_tag        -m "tagging a tree" $HEAD_TREE &&
>  	git tag -a tree_tag-obj    -m "tagging a tree" $HEAD_TREE &&
> -	git tag    tag-obj_tag     -m "tagging a tag" tree_tag-obj &&
> -	git tag -a tag-obj_tag-obj -m "tagging a tag" tree_tag-obj
> +	git tag    tag-obj_tag     -m "tagging a tag" --allow-recursive-tag tree_tag-obj &&
> +	git tag -a tag-obj_tag-obj -m "tagging a tag" --allow-recursive-tag tree_tag-obj
>  '
>
>  test_expect_success 'tree_tag'        '

At least some of these tests should change the existing test line to a "test_must_fail", i.e. assert that without the new option these aren't created, maybe for good measure test --force too.