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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 18, 2017, 19:49 UTC
Message-ID: <CACBZZX5LN8nhs85K18XVg4Y9_qb9zKGBoFnnQHSsRZVOP=OkDw@mail.gmail.com>
In-Reply-To: <xmqqmvci2zoc.fsf@gitster.mtv.corp.google.com>

On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Æ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.

Show 46 quoted lines

>         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.