Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> However, documenting this as "-l <pattern>" was never correct, as
>> these both worked before Jeff's change:
>>
>>     git tag -l 'v*'
>>     git tag 'v*' -l
>
> Actually, we do not particularly care about the latter, and quite
> honestly, I'd prefer we do not advertise and encourage the latter.
> Most Git commands take dashed options first and then non-dashed
> arguments, and so should "git tag".  A more important example to
> show why "-l <pattern>" that pretends <pattern> is an argument to
> the option is wrong is this:

I for one do care about the latter in my CLI use. I.e. I'm fairly used
to GNU-style getopt parsing where if you type "ls foo*" and forget the
-l you don't have to "^Mb-l <RET>" to produce "ls -l foo*" as you
would on the BSD's, you just type " -l<RET>".

I don't see any reason for why we'd force users to migrate to strict
BSD-like getopt parsing for commands like tag/branch which accept
these form of arguments, although one could argue that it's worth it
for consistency with the likes of git-log might have better reasons to
require it.

I.e. are there cases where we encounter genuine ambiguities in our
option parsing because of this that don't involve the usual suspect of
e.g. pattern that starts with "-" needing "<opts> -- <pattern>" to
resolve the ambiguity?

As for this patch, I don't think accurately documenting an option like
--list in terms of what it actually does is advertising and
encouraging this use. All the examples are still of the form "git tag
-l <pattern>" not "git tag <pattern> -l".

I think the main point of reference documentation like this should be
to accurately and exhaustively document what something actually does.
If we'd like to change it & deprecate some mode of operation in the
future, fine, but surely the first step towards that is to document
what the command does *now*.

You should be able to look at a git command, then read the
documentation, and without having run the command or inspected the
code be confident that you understand what the command will do when
you run it.

Right now that isn't the case with "tag --list" at all, because it's
documented as taking a pattern as an argument, but that isn't how it
works.

>         git tag -l --merged X 'v*'
>
> and this one
>
>>     git tag --list 'v*rc*' '*2.8*'
>
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index 74fc82a3c0..d36cd51fe2 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '
>>       git tag
>>  '
>>
>> +cat >expect <<EOF
>> +mytag
>> +EOF
>> +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
>> +     git tag -l -l >actual &&
>> +     test_cmp expect actual &&
>> +     git tag --list --list >actual &&
>> +     test_cmp expect actual &&
>> +     git tag --list -l --list >actual &&
>> +     test_cmp expect actual
>> +'
>
> OK.  I do not care too deeply about this one, but somebody may want
> to tighten up the command line parsing to detect conflicting or
> duplicated cmdmode as an error in the future, and at that time this
> will require updating.  I am not sure if we want to promise that
> giving multiple -l will keep working.
>
>> +test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '
>> +     git tag -l "v1*" "v0*" >actual &&
>
> This is good thing to promise that we will keep it working.
>
>> +     test_cmp expect actual &&
>> +     git tag -l "v1*" --list "v0*" >actual &&
>> +     test_cmp expect actual &&
>> +     git tag -l "v1*" "v0*" -l --list >actual &&
>> +     test_cmp expect actual
>
> I'd prefer we do *not* promise that it will keep working if you give
> pattern and then later any dashed-option like -l to the command by
> having tests like these.

To continue the above, I don't agree with this take on the issue at
all. We should as much as possible aim for full coverage on our tests,
just because something's tested for doesn't implicitly mean that
there's a future promise that the functionality will always work that
way, it's just testing for both intentional & unintentional
regressions when the code is changed.

Then if we decide to e.g. change to stricter parsing or BSD-style
parsing we'll hopefully have an exhaustive list of the cases we're
changing.

It might make sense in cases where we're testing for a feature that
might get deprecated in the future to have some test prefix for that,
i.e. similar to "test_must_fail" but "test_might_get_deprecated" or
whatever.

Although that might just as likely turn out to be a useless catalog of
things we never actually end up changing, see e.g. that TODO test for
the exit code of "git tag -l" which I removed at the start of this
series.