git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>, Lars Hjemli <hjemli@gmail.com>, Jeff King <peff@peff.net>, Christian Couder <christian.couder@gmail.com>, Carlos Rica <jasampler@gmail.com>, Samuel Tardieu <sam@rfc1149.net>, Tom Grennan <tmgrennan@gmail.com>
Subject: Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation
Date: Sat, 18 Mar 2017 20:49:19 +0100
Message-ID: <CACBZZX5LN8nhs85K18XVg4Y9_qb9zKGBoFnnQHSsRZVOP=OkDw@mail.gmail.com> (raw)
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:
> Æ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.

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-18 10:32 [PATCH 0/8] Various changes to the "tag" command Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 1/8] tag: Remove a TODO item from the test suite Ævar Arnfjörð Bjarmason
2017-03-18 18:14   ` Junio C Hamano
2017-03-18 18:42     ` [PATCH 0/2] doc/SubmittingPatches: A couple of minor improvements Ævar Arnfjörð Bjarmason
2017-03-18 18:42     ` [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..." Ævar Arnfjörð Bjarmason
2017-03-18 19:04       ` Junio C Hamano
2017-03-18 19:16         ` Ævar Arnfjörð Bjarmason
2017-03-18 20:07           ` Junio C Hamano
2017-03-18 18:42     ` [PATCH 2/2] doc/SubmittingPatches: show how to get a CLI commit summary Ævar Arnfjörð Bjarmason
2017-03-18 19:07       ` Junio C Hamano
2017-03-19  0:48   ` [PATCH 1/8] tag: Remove a TODO item from the test suite Jakub Narębski
2017-03-19  6:43     ` Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 2/8] tag: Refactor the options handling code to be less bizarro Ævar Arnfjörð Bjarmason
2017-03-18 18:35   ` Junio C Hamano
2017-03-18 19:13     ` Ævar Arnfjörð Bjarmason
2017-03-18 19:27       ` Junio C Hamano
2017-03-18 20:00         ` Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 3/8] tag: Change misleading --list <pattern> documentation Ævar Arnfjörð Bjarmason
2017-03-18 18:43   ` Junio C Hamano
2017-03-18 19:49     ` Ævar Arnfjörð Bjarmason [this message]
2017-03-20  3:44     ` Jeff King
2017-03-20 15:55       ` Junio C Hamano
2017-03-20 17:07         ` Junio C Hamano
2017-03-20 16:09       ` Ævar Arnfjörð Bjarmason
2017-03-20 16:11         ` Jeff King
2017-03-18 10:32 ` [PATCH 4/8] tag: Implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
2017-03-20  3:55   ` Jeff King
2017-03-20  9:16     ` Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 5/8] tag: Implicitly supply --list given the -n option Ævar Arnfjörð Bjarmason
2017-03-20  4:02   ` Jeff King
2017-03-18 10:32 ` [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref Ævar Arnfjörð Bjarmason
2017-03-20  4:25   ` Jeff King
2017-03-20  9:32     ` Ævar Arnfjörð Bjarmason
2017-03-20 19:52       ` Jeff King
2017-03-20 20:04         ` Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 8/8] tag: Change --point-at to default to HEAD Ævar Arnfjörð Bjarmason
2017-03-18 18:54   ` Junio C Hamano
2017-03-18 19:52     ` Ævar Arnfjörð Bjarmason
2017-03-20  4:26 ` [PATCH 0/8] Various changes to the "tag" command Jeff King
2017-03-20 15:57   ` Junio C Hamano
2017-03-21 15:51 [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary SZEDER Gábor
2017-03-21 17:58 ` Junio C Hamano
2017-03-21 18:47   ` Ævar Arnfjörð Bjarmason
2017-03-21 20:01   ` SZEDER Gábor
2017-03-21 20:13     ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACBZZX5LN8nhs85K18XVg4Y9_qb9zKGBoFnnQHSsRZVOP=OkDw@mail.gmail.com' \
    --to=avarab@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=hjemli@gmail.com \
    --cc=jasampler@gmail.com \
    --cc=peff@peff.net \
    --cc=sam@rfc1149.net \
    --cc=tmgrennan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox