From: Ævar Arnfjörð Bjarmason <firstname.lastname@example.org> To: Junio C Hamano <email@example.com> Cc: Git Mailing List <firstname.lastname@example.org>, Lars Hjemli <email@example.com>, Jeff King <firstname.lastname@example.org>, Christian Couder <email@example.com>, Carlos Rica <firstname.lastname@example.org>, Samuel Tardieu <email@example.com>, Tom Grennan <firstname.lastname@example.org> 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: <email@example.com> On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano <firstname.lastname@example.org> wrote: > Ævar Arnfjörð Bjarmason <email@example.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.
next prev parent 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' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /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
email@example.com 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