Re: [PATCH] tag: Implicitly supply --list given another list-like option

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2017, 03:19 UTC
Message-ID: <xmqq4lyzw5az.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170311120855.16756-1-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change these invocations which currently error out without the -l, to
> behave as if though -l was provided:
>
>     git tag -l [--contains|--points-at|--[no-]merged] <commit>

Shouldn't this be

	git tag -l [[--[no-]contains|--points-at|--[no-]merged]	<commit>] [<pattern>]

i.e. if you are giving <commit> you need how that commit is used in filtering, but you do not have to give any such filter when listing, and <pattern>, when given, is used to further limit the output, but it also is optional.

> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option

s/Implicit/implicit/ (ask "git shortlog --no-merges" over recent history)

Show 11 quoted lines

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 525737a5d8..c80d9e11ba 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -94,6 +94,9 @@ OPTIONS
>  	lists all tags. The pattern is a shell wildcard (i.e., matched
>  	using fnmatch(3)).  Multiple patterns may be given; if any of
>  	them matches, the tag is shown.
> ++
> +We supply this option implicitly if any other list-like option is
> +provided. E.g. `--contains`, `--points-at` etc.

Who are "we"?

	When any option that only makes sense in the list mode
	(e.g. `--contains`) is given, the command defaults to
	the `--list` mode.

By the way, do we catch it as a command line error when options like `--points-at` are given when we are creating a new tag?

Show 10 quoted lines

> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..6ab65bcf6b 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>  
> +	/* We implicitly supply --list with --contains, --points-at,
> +	   --merged and --no-merged, just like git-branch */

	/*
         * We write multi-line comments like this,
	 * without anything other than slash-asterisk or
	 * asterisk-slash on the first and last lines.
	 */

> +	if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> +		cmdmode = 'l';

Don't we want to make sure we do the defaulting only upon !cmdmode? Doesn't this start ignoring

	tag -a -m foo --points-at HEAD bar

as an error otherwise?

> +	/* Just plain "git tag" is like "git tag --list" */
>  	if (argc == 0 && !cmdmode)
>  		cmdmode = 'l';

Show 10 quoted lines

> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	if (filter.lines != -1)
>  		die(_("-n option is only allowed with -l."));
> -	if (filter.with_commit)
> -		die(_("--contains option is only allowed with -l."));
> -	if (filter.points_at.nr)
> -		die(_("--points-at option is only allowed with -l."));
> -	if (filter.merge_commit)
> -		die(_("--merged and --no-merged option are only allowed with -l"));

And I do not think removal of these check is a good idea at all. Perhaps you were too focused on '-l' that you forgot that people may be giving an explicit option like -a or -s, in which case these error checks are still very sensible things to have, no?