* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-11 12:08 ` [PATCH] tag: Implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
@ 2017-03-11 12:14 ` Ævar Arnfjörð Bjarmason
2017-03-12 2:51 ` Junio C Hamano
2017-03-12 3:19 ` Junio C Hamano
2017-03-12 11:19 ` Jeff King
2 siblings, 1 reply; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-11 12:14 UTC (permalink / raw)
To: Git Mailing List
Cc: Junio C Hamano, Jake Goulding, Jeff King, Tom Grennan,
Karthik Nayak, Ævar Arnfjörð Bjarmason
On Sat, Mar 11, 2017 at 1:08 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change these invocations which currently error out without the -l, to
> behave as if though -l was provided:
>
> git tag -l [--contains|--points-at|--[no-]merged] <commit>
Oops, this should be:
git tag -l [--contains|--points-at|--[no-]merged] <commit> <pattern>
Will fix in v2 pending other comments.
> I ran into what turned out to be not-a-bug in "branch" where it,
> unlike "tag" before this patch, accepts input like:
>
> git branch --contains v2.8.0 <pattern>
>
> Jeff King pointed out in
> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to
> that::
>
> The difference between "branch" and "tag" here is that "branch
> --contains" implies "--list" (and the argument becomes a pattern).
> Whereas "tag --contains" just detects the situation and complains.
>
> If anything, I'd think we would consider teaching "tag" to behave
> more like "branch".
>
> I agree. This change does that, the only tests that broke as a result
> of this were tests that were explicitly checking that this
> "branch-like" usage wasn't permitted, i.e. no actual breakages
> occurred, and I can't imagine an invocation that would negatively
> impact backwards compatibility, i.e. these invocations all just
> errored out before.
>
> Spelunking through the history via:
>
> git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'
>
> Reveals that there was no good reason for this in the first place. The
> --contains option added in v1.6.1.1-243-g32c35cfb1e made this an
> error, and all the other subsequent options I'm changing here just
> copy/pasted its pattern.
>
> I've changed the failing tests to check that this invocation mode is
> permitted instead, and added extra tests for the list-like options we
> weren't testing.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Junio: This will merge conflict with my in-flight --no-contains
> patch. I can re-send either one depending on which you want to accept
> first, this patch will need an additional test for --no-contains. I
> just wanted to get this on the ML for review before the --no-contains
> patch hit "master".
>
> Documentation/git-tag.txt | 3 +++
> builtin/tag.c | 12 ++++++------
> t/t7004-tag.sh | 25 +++++++++++++++++++++----
> 3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 525737a5d8..c80d9e11ba 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -94,6 +94,9 @@ OPTIONS
> lists all tags. The pattern is a shell wildcard (i.e., matched
> using fnmatch(3)). Multiple patterns may be given; if any of
> them matches, the tag is shown.
> ++
> +We supply this option implicitly if any other list-like option is
> +provided. E.g. `--contains`, `--points-at` etc.
The "this option" I'm referring to is --list, this is a patch to the
--list section in "git help tag".
>
> --sort=<key>::
> Sort based on the key given. Prefix `-` to sort in
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..6ab65bcf6b 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> }
> create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>
> + /* We implicitly supply --list with --contains, --points-at,
> + --merged and --no-merged, just like git-branch */
> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> + cmdmode = 'l';
> +
> + /* Just plain "git tag" is like "git tag --list" */
> if (argc == 0 && !cmdmode)
> cmdmode = 'l';
>
> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> }
> if (filter.lines != -1)
> die(_("-n option is only allowed with -l."));
> - if (filter.with_commit)
> - die(_("--contains option is only allowed with -l."));
> - if (filter.points_at.nr)
> - die(_("--points-at option is only allowed with -l."));
> - if (filter.merge_commit)
> - die(_("--merged and --no-merged option are only allowed with -l"));
> if (cmdmode == 'd')
> return for_each_tag_name(argv, delete_tag, NULL);
> if (cmdmode == 'v') {
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index b4698ab5f5..e0306ee5a8 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1453,6 +1453,11 @@ test_expect_success 'checking that initial commit is in all tags' "
> test_cmp expected actual
> "
>
> +test_expect_success 'checking that --contains can be used in non-list mode' '
> + git tag --contains $hash1 v* >actual &&
> + test_cmp expected actual
> +'
> +
> # mixing modes and options:
>
> test_expect_success 'mixing incompatibles modes and options is forbidden' '
> @@ -1466,8 +1471,10 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
>
> # check points-at
>
> -test_expect_success '--points-at cannot be used in non-list mode' '
> - test_must_fail git tag --points-at=v4.0 foo
> +test_expect_success '--points-at can be used in non-list mode' '
> + echo v4.0 >expect &&
> + git tag --points-at=v4.0 "v*" >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success '--points-at finds lightweight tags' '
> @@ -1744,8 +1751,13 @@ test_expect_success 'setup --merged test tags' '
> git tag mergetest-3 HEAD
> '
>
> -test_expect_success '--merged cannot be used in non-list mode' '
> - test_must_fail git tag --merged=mergetest-2 foo
> +test_expect_success '--merged can be used in non-list mode' '
> + cat >expect <<-\EOF &&
> + mergetest-1
> + mergetest-2
> + EOF
> + git tag --merged=mergetest-2 "mergetest*" >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success '--merged shows merged tags' '
> @@ -1765,6 +1777,11 @@ test_expect_success '--no-merged show unmerged tags' '
> test_cmp expect actual
> '
>
> +test_expect_success '--no-merged can be used in non-list mode' '
> + git tag --no-merged=mergetest-2 mergetest-* >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'ambiguous branch/tags not marked' '
> git tag ambiguous &&
> git branch ambiguous &&
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-11 12:14 ` Ævar Arnfjörð Bjarmason
@ 2017-03-12 2:51 ` Junio C Hamano
2017-03-12 3:00 ` Junio C Hamano
2017-03-12 9:15 ` Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2017-03-12 2:51 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Jake Goulding, Jeff King, Tom Grennan,
Karthik Nayak
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> Junio: This will merge conflict with my in-flight --no-contains
>> patch. I can re-send either one depending on which you want to accept
>> first, this patch will need an additional test for --no-contains. I
>> just wanted to get this on the ML for review before the --no-contains
>> patch hit "master".
I haven't looked at the patch text of this one closely yet, but I
think the goals of both make sense, so we would eventually want to
have them both.
I also think that "if you said --contains, --merged, etc. you are
already asking to give you a list and cannot be creating a new one",
which is the topic of this patch, makes sense even if nobody were
interested in asking "--no-contains".
So perhaps you would want this applied first, so that existing three
can already benefit from "implicit --list" before waiting for the
other one?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-12 2:51 ` Junio C Hamano
@ 2017-03-12 3:00 ` Junio C Hamano
2017-03-12 9:15 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2017-03-12 3:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Jake Goulding, Jeff King, Tom Grennan,
Karthik Nayak
Junio C Hamano <gitster@pobox.com> writes:
> I haven't looked at the patch text of this one closely yet, but I
> think the goals of both make sense, so we would eventually want to
> have them both.
> ...
> So perhaps you would want this applied first, so that existing three
> can already benefit from "implicit --list" before waiting for the
> other one?
Ah, I see that you already have the other topic polished to v4
(sorry, but I am a bit behind the list traffic). If it makes your
life easier, it is OK to make the current three to four by adding
"--no-contains" first and then make all/any of them imply "--list"
on top.
IOW, either is fine. Personally, I have a suspicion that sending
both at the same time, expecting the maintainer to be able to
resolve the potential conflicts correctly, would also be fine ;-)
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-12 2:51 ` Junio C Hamano
2017-03-12 3:00 ` Junio C Hamano
@ 2017-03-12 9:15 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-12 9:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Jake Goulding, Jeff King, Tom Grennan,
Karthik Nayak
On Sun, Mar 12, 2017 at 3:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Junio: This will merge conflict with my in-flight --no-contains
>>> patch. I can re-send either one depending on which you want to accept
>>> first, this patch will need an additional test for --no-contains. I
>>> just wanted to get this on the ML for review before the --no-contains
>>> patch hit "master".
>
> I haven't looked at the patch text of this one closely yet, but I
> think the goals of both make sense, so we would eventually want to
> have them both.
>
> I also think that "if you said --contains, --merged, etc. you are
> already asking to give you a list and cannot be creating a new one",
> which is the topic of this patch, makes sense even if nobody were
> interested in asking "--no-contains".
>
> So perhaps you would want this applied first, so that existing three
> can already benefit from "implicit --list" before waiting for the
> other one?
Yes, let's do this one first. I'll address the comments that have come
up & just make this all part of one series on top of JK's patches.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-11 12:08 ` [PATCH] tag: Implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
2017-03-11 12:14 ` Ævar Arnfjörð Bjarmason
@ 2017-03-12 3:19 ` Junio C Hamano
2017-03-12 9:15 ` Ævar Arnfjörð Bjarmason
2017-03-12 11:19 ` Jeff King
2 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2017-03-12 3:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jake Goulding, Jeff King, Tom Grennan, Karthik Nayak
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change these invocations which currently error out without the -l, to
> behave as if though -l was provided:
>
> git tag -l [--contains|--points-at|--[no-]merged] <commit>
Shouldn't this be
git tag -l [[--[no-]contains|--points-at|--[no-]merged] <commit>] [<pattern>]
i.e. if you are giving <commit> you need how that commit is used in
filtering, but you do not have to give any such filter when listing,
and <pattern>, when given, is used to further limit the output, but
it also is optional.
> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option
s/Implicit/implicit/ (ask "git shortlog --no-merges" over recent history)
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 525737a5d8..c80d9e11ba 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -94,6 +94,9 @@ OPTIONS
> lists all tags. The pattern is a shell wildcard (i.e., matched
> using fnmatch(3)). Multiple patterns may be given; if any of
> them matches, the tag is shown.
> ++
> +We supply this option implicitly if any other list-like option is
> +provided. E.g. `--contains`, `--points-at` etc.
Who are "we"?
When any option that only makes sense in the list mode
(e.g. `--contains`) is given, the command defaults to
the `--list` mode.
By the way, do we catch it as a command line error when options like
`--points-at` are given when we are creating a new tag?
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..6ab65bcf6b 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> }
> create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>
> + /* We implicitly supply --list with --contains, --points-at,
> + --merged and --no-merged, just like git-branch */
/*
* We write multi-line comments like this,
* without anything other than slash-asterisk or
* asterisk-slash on the first and last lines.
*/
> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> + cmdmode = 'l';
Don't we want to make sure we do the defaulting only upon !cmdmode?
Doesn't this start ignoring
tag -a -m foo --points-at HEAD bar
as an error otherwise?
> + /* Just plain "git tag" is like "git tag --list" */
> if (argc == 0 && !cmdmode)
> cmdmode = 'l';
> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> }
> if (filter.lines != -1)
> die(_("-n option is only allowed with -l."));
> - if (filter.with_commit)
> - die(_("--contains option is only allowed with -l."));
> - if (filter.points_at.nr)
> - die(_("--points-at option is only allowed with -l."));
> - if (filter.merge_commit)
> - die(_("--merged and --no-merged option are only allowed with -l"));
And I do not think removal of these check is a good idea at all.
Perhaps you were too focused on '-l' that you forgot that people may
be giving an explicit option like -a or -s, in which case these
error checks are still very sensible things to have, no?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-12 3:19 ` Junio C Hamano
@ 2017-03-12 9:15 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-12 9:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Jake Goulding, Jeff King, Tom Grennan,
Karthik Nayak
On Sun, Mar 12, 2017 at 4:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Change these invocations which currently error out without the -l, to
>> behave as if though -l was provided:
>>
>> git tag -l [--contains|--points-at|--[no-]merged] <commit>
>
> Shouldn't this be
>
> git tag -l [[--[no-]contains|--points-at|--[no-]merged] <commit>] [<pattern>]
>
> i.e. if you are giving <commit> you need how that commit is used in
> filtering, but you do not have to give any such filter when listing,
> and <pattern>, when given, is used to further limit the output, but
> it also is optional.
>
>> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option
>
> s/Implicit/implicit/ (ask "git shortlog --no-merges" over recent history)
>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 525737a5d8..c80d9e11ba 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -94,6 +94,9 @@ OPTIONS
>> lists all tags. The pattern is a shell wildcard (i.e., matched
>> using fnmatch(3)). Multiple patterns may be given; if any of
>> them matches, the tag is shown.
>> ++
>> +We supply this option implicitly if any other list-like option is
>> +provided. E.g. `--contains`, `--points-at` etc.
>
> Who are "we"?
>
> When any option that only makes sense in the list mode
> (e.g. `--contains`) is given, the command defaults to
> the `--list` mode.
>
> By the way, do we catch it as a command line error when options like
> `--points-at` are given when we are creating a new tag?
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index ad29be6923..6ab65bcf6b 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> }
>> create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>>
>> + /* We implicitly supply --list with --contains, --points-at,
>> + --merged and --no-merged, just like git-branch */
>
> /*
> * We write multi-line comments like this,
> * without anything other than slash-asterisk or
> * asterisk-slash on the first and last lines.
> */
>
>> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
>> + cmdmode = 'l';
>
> Don't we want to make sure we do the defaulting only upon !cmdmode?
> Doesn't this start ignoring
>
> tag -a -m foo --points-at HEAD bar
>
> as an error otherwise?
>
>> + /* Just plain "git tag" is like "git tag --list" */
>> if (argc == 0 && !cmdmode)
>> cmdmode = 'l';
>
>> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> }
>> if (filter.lines != -1)
>> die(_("-n option is only allowed with -l."));
>> - if (filter.with_commit)
>> - die(_("--contains option is only allowed with -l."));
>> - if (filter.points_at.nr)
>> - die(_("--points-at option is only allowed with -l."));
>> - if (filter.merge_commit)
>> - die(_("--merged and --no-merged option are only allowed with -l"));
>
> And I do not think removal of these check is a good idea at all.
> Perhaps you were too focused on '-l' that you forgot that people may
> be giving an explicit option like -a or -s, in which case these
> error checks are still very sensible things to have, no?
I'll fix this up and make sure to do more sanity checks with the
different option combinations before resending.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tag: Implicitly supply --list given another list-like option
2017-03-11 12:08 ` [PATCH] tag: Implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
2017-03-11 12:14 ` Ævar Arnfjörð Bjarmason
2017-03-12 3:19 ` Junio C Hamano
@ 2017-03-12 11:19 ` Jeff King
2 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2017-03-12 11:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Jake Goulding, Tom Grennan, Karthik Nayak
On Sat, Mar 11, 2017 at 12:08:55PM +0000, Ævar Arnfjörð Bjarmason wrote:
> Jeff King pointed out in
> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to
> that::
>
> The difference between "branch" and "tag" here is that "branch
> --contains" implies "--list" (and the argument becomes a pattern).
> Whereas "tag --contains" just detects the situation and complains.
>
> If anything, I'd think we would consider teaching "tag" to behave
> more like "branch".
>
> I agree. This change does that, the only tests that broke as a result
> of this were tests that were explicitly checking that this
> "branch-like" usage wasn't permitted, i.e. no actual breakages
> occurred, and I can't imagine an invocation that would negatively
> impact backwards compatibility, i.e. these invocations all just
> errored out before.
Trying to think of counter-arguments, the best I could come up with is
that the situation is potentially ambiguous, and some user could be
confused by us doing the wrong thing. I don't find that particularly
compelling, especially as the "wrong thing" is to list tags, which is
not a destructive operation.
So I think this is an improvement. As for the patch itself:
> + /* We implicitly supply --list with --contains, --points-at,
> + --merged and --no-merged, just like git-branch */
> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> + cmdmode = 'l';
I was about to complain that this needs "!cmdmode", but I just looked
forward in the thread and saw that Junio already covered that in more
detail. I concur.
Thanks for working on this.
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread