On Sun, Mar 12, 2017 at 4:19 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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)
>
>> 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?
>
>> 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';
>
>> @@ -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?
I'll fix this up and make sure to do more sanity checks with the
different option combinations before resending.