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>,
Karthik Nayak <karthik.188@gmail.com>
Subject: Re: [PATCH v3 12/16] tag: implicitly supply --list given another list-like option
Date: Thu, 23 Mar 2017 21:32:40 +0100 [thread overview]
Message-ID: <CACBZZX6HnUZSV49c2uRqW3u-+Tc-5LtaaEG=Yype1155qR0ifw@mail.gmail.com> (raw)
In-Reply-To: <xmqqlgrvewak.fsf@gitster.mtv.corp.google.com>
On Thu, Mar 23, 2017 at 6:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 2640c6955c..491dba065c 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -82,10 +82,11 @@ OPTIONS
>>
>> -n<num>::
>> <num> specifies how many lines from the annotation, if any,
>> - are printed when using -l.
>> - The default is not to print any annotation lines.
>> - If no number is given to `-n`, only the first line is printed.
>> - If the tag is not annotated, the commit message is displayed instead.
>> + are printed when using -l. Implies `--list`.
>> ++
>> +The default is not to print any annotation lines.
>> +If no number is given to `-n`, only the first line is printed.
>> +If the tag is not annotated, the commit message is displayed instead.
>
> I thought we would want to have -n thing separate as we are much
> less sure than the others, but let's forget about that and commit
> to default to -l with -n.
>
> Given the confusion I had during the review of the previous round, I
> would think this should clarify what it means "If no number is given
> to -n" a bit more strongly. Namely, the behaviour we see in this
> test:
>
>> +test_expect_success 'The -n 100 invocation means -n --list 100, not -n100' '
>
> can be a common mistake and needs to be warned about.
>
> I'd drop "The default is not to print any annotation lines". It is
> not just unnecessary (we make a specific mention about the default
> when we act as if an option were given even if the user doesn't
> explicitly give it, and not triggering a special feature when it is
> not asked for _is_ the default everywhere else), but is confusing,
> because it is unclear if it is talking about the default case of not
> giving any -n<num> option, or the default case of not giving <num>
> but still the -n option.
>
> Perhaps something along the lines of ...
>
> -n<num>::
> Show the message of the annotated tag when using the
> `--list` mode. A number <num> can be directly attached to
> the `-n` option (e.g. `-n100`; not `-n 100` as separate
> arguments) to specify how many lines of the message to show.
> Without <num>, only the first line of the message is shown.
> For an unannotated tag that points at a commit, the commit
> message is used instead.
>
> ... may be less confusing?
Sounds good, but a topic for another series I think. I'd rather stop
adding new things to this already big series, and it's not introducing
any new confusion with -n, just making the now confusing "tag -l -n
100" be equivalently written as "tag -n 100".
>> @@ -1496,7 +1517,6 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
>> test_must_fail git tag -l -v &&
>> test_must_fail git tag -l -d &&
>> test_must_fail git tag -l -v -d &&
>> - test_must_fail git tag -n 100 &&
>
> Hmph, wouldn't we want to instead replace it with
>
> ... something to set expectation ... >expect &&
> git tag -n 100 >actual &&
> test_cmp expect actual &&
>
> here?
Nope, we already have detailed tests for "tag -n 100" and the like and
what that output should actually return earlier in the patch, this bit
is just removing it from the test for failing invocations.
>> test_must_fail git tag -n 100 -v &&
>> test_must_fail git tag -l -m msg &&
>> test_must_fail git tag -l -F some file &&
>
> Thanks.
next prev parent reply other threads:[~2017-03-23 20:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-23 13:05 [PATCH v3 00/16] Various changes to the "tag" command & related Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 01/16] tag doc: move the description of --[no-]merged earlier Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 02/16] tag doc: split up the --[no-]merged documentation Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 03/16] tag doc: reword --[no-]merged to talk about commits, not tips Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 04/16] ref-filter: make combining --merged & --no-merged an error Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 05/16] ref-filter: add test for --contains on a non-commit Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 06/16] tag: remove a TODO item from the test suite Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 07/16] tag tests: fix a typo in a test description Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 08/16] for-each-ref: partly change <object> to <commit> in help Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 09/16] tag: add more incompatibles mode tests Ævar Arnfjörð Bjarmason
2017-03-23 17:13 ` Junio C Hamano
2017-03-23 20:37 ` Ævar Arnfjörð Bjarmason
2017-03-23 21:36 ` Junio C Hamano
2017-03-23 22:13 ` Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 10/16] parse-options: add OPT_NONEG to the "contains" option Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 11/16] tag: change misleading --list <pattern> documentation Ævar Arnfjörð Bjarmason
2017-03-23 17:16 ` Junio C Hamano
2017-03-23 13:05 ` [PATCH v3 12/16] tag: implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
2017-03-23 17:29 ` Junio C Hamano
2017-03-23 20:32 ` Ævar Arnfjörð Bjarmason [this message]
2017-03-23 21:33 ` Junio C Hamano
2017-03-23 13:05 ` [PATCH v3 13/16] tag: change --point-at to default to HEAD Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 14/16] ref-filter: add --no-contains option to tag/branch/for-each-ref Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 15/16] ref-filter: reflow recently changed branch/tag/for-each-ref docs Ævar Arnfjörð Bjarmason
2017-03-23 13:05 ` [PATCH v3 16/16] tag: add tests for --with and --without Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly 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='CACBZZX6HnUZSV49c2uRqW3u-+Tc-5LtaaEG=Yype1155qR0ifw@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=karthik.188@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).