Re: [PATCH v2 15/16] tag: implicitly supply --list given the -n option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 21, 2017, 19:33 UTC
Message-ID: <CACBZZX6Ak0JOd7Fh5Z2p0md_P8vG__N_gOkfoNueERVF=jVyBg@mail.gmail.com>
In-Reply-To: <xmqqtw6mo2l2.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 21, 2017 at 8:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Yeah I see now that this is rather badly explained. I'll fix this up
>> for v3. All of this worked already:
>>
>>     $ ./git tag 100
>>     $ ./git tag -n -l 100
>>     100             tag: add tests for --with and --without
>>     $ ./git tag -l -n 100
>>     100             tag: add tests for --with and --without
>>
>> So actually thinking about it again it doesn't add any more ambiguity
>> than we had before. The change is just strictly getting rid of the
>> need for -l for consistency with --contains, --points-at etc.
>>
>> I see now that the whole thing that led me down this golden path was
>> that I was removing the failing "git tag -n 100" test,...
>
> Wait a minute.  I do not think I would agree with the behaviour of
> the last one, if "tag -l -n 100" is taking 100 as a pattern, not a
> numerical argument to "-n".  That sounds utterly broken.
>
> Is it because we use it OPT_OPTARG, which requires it to be spelled
> as "-n100" or "-n=100" or somesuch?
>
> In any case, it is not a new confusion this series introduces, so
> let's include it in the series, but I'd prefer to see it kept as a
> separate patch, at least for now.  Maybe somebody else have an idea
> to resolve this apparent confusion in a cleaner way.

Yup, that's why. This:

diff --git a/builtin/tag.c b/builtin/tag.c
index dbc6f5b74b..1346341413 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -403,7 +403,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
                OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
                { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
                                N_("print <n> lines of each tag message"),
-                               PARSE_OPT_OPTARG, NULL, 1 },
+                               0, NULL, 1 },
                OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
                OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'),

Changes it so that "git tag -n 100 '*1.6.6*rc*'" does what "git tag
-n100 '*1.6.6*rc*'" does. But that breaks a bunch of tests / possibly
some scripts in the wild, especially because "git tag -n '*1.6.6*rc*'"
now becomes an error, i.e. we'll try to treat the pattern as the
numeric argument.