*[PATCH 0/8] Various changes to the "tag" command@ 2017-03-18 10:32 Æ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
` (8 more replies)0 siblings, 9 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 10:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan, Ævar Arnfjörð Bjarmason
This series incorporates and replaces all the "tag" patches I have
floating around the list, and adds a lot in the mix which discovered
while working on the initial two patches, but which made sense as
separate patches.
It's based no top of Jeff's gitster/jk/ref-filter-flags-cleanup.
I'm bundling this all together because a lot of these patches touch
the exact same code, and in other cases subsequent patches make use of
test suite improvements I made earlier in the series. So although some
could be split out entirely (e.g. the --point-at patch), I'd like to
just present them all for review together & split out any if there's
strong objections to basing them on top of each other.
I think this series changes addresses all the points brought up by
Junio/Jeff about my previous patches, except there's no extensive
discussion of how the filtering mechanism works in general as pointed
out by Junio in <xmqqwpbvumrk.fsf@gitster.mtv.corp.google.com>.
I think it makes sense to have that, but in the interest of getting
something out the door I'm not working on that for now.
Ævar Arnfjörð Bjarmason (8):
tag: Remove a TODO item from the test suite
tag: Refactor the options handling code to be less bizarro
tag: Change misleading --list <pattern> documentation
tag: Implicitly supply --list given another list-like option
tag: Implicitly supply --list given the -n option
ref-filter: Add --no-contains option to tag/branch/for-each-ref
tag: Add tests for --with and --without
tag: Change --point-at to default to HEAD
Documentation/git-branch.txt | 15 ++-
Documentation/git-for-each-ref.txt | 6 +-
Documentation/git-tag.txt | 44 ++++---
builtin/branch.c | 4 +-
builtin/for-each-ref.c | 3 +-
builtin/tag.c | 36 ++++--
contrib/completion/git-completion.bash | 4 +-
parse-options.h | 4 +-
ref-filter.c | 19 ++-
ref-filter.h | 1 +
t/t3201-branch-contains.sh | 51 +++++++-
t/t6302-for-each-ref-filter.sh | 16 +++
t/t7004-tag.sh | 226 +++++++++++++++++++++++++++++++--
13 files changed, 371 insertions(+), 58 deletions(-)
--
2.11.0
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro
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ð Bjarmason0 siblings, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2017-03-18 18:35 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..0bba3fd070 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> }
> create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>
> - if (argc == 0 && !cmdmode)
> + if (argc == 0 && !cmdmode && !create_tag_object)
> cmdmode = 'l';
So with this change, if we cannot infer that we are creating a tag
object from other options, we leave cmdmode to its original 0.
> - if ((create_tag_object || force) && (cmdmode != 0))
> + if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
> usage_with_options(git_tag_usage, options);
And then immediately after that, we complain by detecting that we
know we are creating a tag and a non-zero cmdmode is in effect
(i.e. 'l', 'd' or 'v', none of which is about creating a tag). The
way we used to detect that we are doing something other than tag
creation was by seeing cmdmode is set to anything. Because of your
earlier change, that no longer is true. You need to separately
check (!cmdmode && !argc).
By following the logic that way, I can see how this change at this
step is a no-op, but I have to say that the code with this patch
looks much more bizarre than the original.
I am not sure why you want to do the first change at this step in
the first place. Is it because you'd want to take over (!cmdmode &&
!argc) condtion to default to 'list'? With the change in 4/8 and
5/8, you are ensuring that cmdmode is set to 'l' for these new cases
before the code hits the check to call usage_with_options(). And at
that point, you can use the original "are we creating and !cmdmode
says we are not? That's contradiction" logic without making it more
bizarre with this patch, no?
^permalinkrawreply [flat|threaded] 49+ messages in thread

*[PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."
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 ` Ævar Arnfjörð Bjarmason
2017-03-18 19:04 ` 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
2 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 18:42 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
Amend the section which describes how the first line of the subject
should look like to say that the ":" in "area: " shouldn't be treated
like a full stop for the purposes of letter casing.
Change the two subject examples to make this new paragraph clearer,
i.e. "unstar" is not a common word, and "git-cherry-pick.txt" is a
much longer string than "githooks.txt". Pick two recent commits from
git.git that fit better for the description.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/SubmittingPatches | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3faf7eb884..9ef624ce38 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -98,12 +98,17 @@ should skip the full stop. It is also conventional in most cases to
prefix the first line with "area: " where the area is a filename or
identifier for the general area of the code being modified, e.g.
- . archive: ustar header checksum is computed unsigned
- . git-cherry-pick.txt: clarify the use of revision range notation
+ . doc: clarify distinction between sign-off and pgp-signing
+ . githooks.txt: improve the intro section
If in doubt which identifier to use, run "git log --no-merges" on the
files you are modifying to see the current conventions.
+It's customary to start the remainder of the first line after "area: "
+with a lower-case letter. E.g. "doc: clarify...", not "doc:
+Clarify...", or "githooks.txt: improve...", not "githooks.txt:
+Improve...".
+
The body should provide a meaningful commit message, which:
. explains the problem the change tries to solve, iow, what is wrong
--
2.11.0
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro
2017-03-18 18:35 ` Junio C Hamano@ 2017-03-18 19:13 ` Ævar Arnfjörð Bjarmason
2017-03-18 19:27 ` Junio C Hamano0 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 19:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
On Sat, Mar 18, 2017 at 7:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index ad29be6923..0bba3fd070 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> }
>> create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>>
>> - if (argc == 0 && !cmdmode)
>> + if (argc == 0 && !cmdmode && !create_tag_object)
>> cmdmode = 'l';
>
> So with this change, if we cannot infer that we are creating a tag
> object from other options, we leave cmdmode to its original 0.
>
>> - if ((create_tag_object || force) && (cmdmode != 0))
>> + if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
>> usage_with_options(git_tag_usage, options);
>
> And then immediately after that, we complain by detecting that we
> know we are creating a tag and a non-zero cmdmode is in effect
> (i.e. 'l', 'd' or 'v', none of which is about creating a tag). The
> way we used to detect that we are doing something other than tag
> creation was by seeing cmdmode is set to anything. Because of your
> earlier change, that no longer is true. You need to separately
> check (!cmdmode && !argc).
>
> By following the logic that way, I can see how this change at this
> step is a no-op, but I have to say that the code with this patch
> looks much more bizarre than the original.
>
> I am not sure why you want to do the first change at this step in
> the first place. Is it because you'd want to take over (!cmdmode &&
> !argc) condtion to default to 'list'? With the change in 4/8 and
> 5/8, you are ensuring that cmdmode is set to 'l' for these new cases
> before the code hits the check to call usage_with_options(). And at
> that point, you can use the original "are we creating and !cmdmode
> says we are not? That's contradiction" logic without making it more
> bizarre with this patch, no?
Nothing about this patch is needed for the rest of the series. I just
tried rebasing it out now and all tests pass & everything works as
expected.
The reason I'm submitting it is because while this works *now* and
there's no cases I can see currently where cmdmode is 'l' after the
current `((create_tag_object || force) && (cmdmode != 0))`, during a
lengthy debugging session when I was hacking on a subsequent patch in
this series it took me a long time to track down a segfault later in
the file because surely it was impossible that I was in cmdmode = 'l'
with only "git tag -a".
Partly that was late night hacking session blindness after having read
the !argc and assuming that the -a would be counted towards argc.
But I thought it was very a very bizarre pattern to set us to cmdmode
= 'l' when we're not in that mode at all just to, as can be seen in
the diff, get around a slightly more verbose one-time if-check.
So I think it's worth it to include this for less confusion in any
subsequent patches to tag.c.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."
2017-03-18 19:04 ` Junio C Hamano@ 2017-03-18 19:16 ` Ævar Arnfjörð Bjarmason
2017-03-18 20:07 ` Junio C Hamano0 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 19:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
On Sat, Mar 18, 2017 at 8:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> prefix the first line with "area: " where the area is a filename or
>> identifier for the general area of the code being modified, e.g.
>>
>> - . archive: ustar header checksum is computed unsigned
>> - . git-cherry-pick.txt: clarify the use of revision range notation
>> + . doc: clarify distinction between sign-off and pgp-signing
>> + . githooks.txt: improve the intro section
>
> Sorry, but I fail to spot why this is an improvement (it is not
> making things worse, either).
Because...
>> If in doubt which identifier to use, run "git log --no-merges" on the
>> files you are modifying to see the current conventions.
>>
>> +It's customary to start the remainder of the first line after "area: "
>> +with a lower-case letter. E.g. "doc: clarify...", not "doc:
>> +Clarify...", or "githooks.txt: improve...", not "githooks.txt:
>> +Improve...".
...it makes this subsequent example more succinct and clear, because
e.g. "githooks.txt" is shorter than "git-cherry-pick.txt", and
"clarify" is obviously a normal looking word which you'd expect to be
capitalized after a full stop, but it might take a couple of readings
to understand that "unstar" without a hyphen isn't some jargon.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro
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ð Bjarmason0 siblings, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2017-03-18 19:27 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> But I thought it was very a very bizarre pattern to set us to cmdmode
> = 'l' when we're not in that mode at all just to, as can be seen in
> the diff, get around a slightly more verbose one-time if-check.
When I wrote my response, I viewed that setting as committing to be
in the "list" mode, not as a workaround. So checking with !cmdmode
to make sure that the command line is not asking to create a tag
makes tons of sense; the new test makes it unreadable from that
point of view.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation
2017-03-18 18:43 ` Junio C Hamano@ 2017-03-18 19:49 ` Ævar Arnfjörð Bjarmason
2017-03-20 3:44 ` Jeff King1 sibling, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 19:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
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.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro
2017-03-18 19:27 ` Junio C Hamano@ 2017-03-18 20:00 ` Ævar Arnfjörð Bjarmason0 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 20:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Lars Hjemli, Jeff King, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
On Sat, Mar 18, 2017 at 8:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> But I thought it was very a very bizarre pattern to set us to cmdmode
>> = 'l' when we're not in that mode at all just to, as can be seen in
>> the diff, get around a slightly more verbose one-time if-check.
>
> When I wrote my response, I viewed that setting as committing to be
> in the "list" mode, not as a workaround. So checking with !cmdmode
> to make sure that the command line is not asking to create a tag
> makes tons of sense; the new test makes it unreadable from that
> point of view.
Makes sense. Looking at this more carefully there's never any cases
where `create_tag_object && cmdmode == 'l'` is true once we make it
past `usage_with_options`, which was the bug I introduced locally
which made this whole thing a bit confusing for me.
I'll drop this patch from v2.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 0/8] Various changes to the "tag" command
2017-03-20 4:26 ` [PATCH 0/8] Various changes to the "tag" command Jeff King
@ 2017-03-20 15:57 ` Junio C Hamano0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2017-03-20 15:57 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Lars Hjemli, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
Jeff King <peff@peff.net> writes:
> On Sat, Mar 18, 2017 at 10:32:48AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> This series incorporates and replaces all the "tag" patches I have
>> floating around the list, and adds a lot in the mix which discovered
>> while working on the initial two patches, but which made sense as
>> separate patches.
>
> I had a few small comments, and I agree with the points that Junio
> raised. But aside from that, it looks good to me.
Thanks for catching issues I missed, and thank you to both of you to
working their solution out quickly. It seems that a final reroll
will be rather an uncontroversial one that can be merged to 'next'
and then to 'master' soonish.
Thanks for working on this.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref
2017-03-20 9:32 ` Ævar Arnfjörð Bjarmason@ 2017-03-20 19:52 ` Jeff King
2017-03-20 20:04 ` Ævar Arnfjörð Bjarmason0 siblings, 1 reply; 49+ messages in thread
From: Jeff King @ 2017-03-20 19:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I think the more relevant comparison is "--no-merged", and it behaves
> > the same way as your new --no-contains. I don't think I saw this
> > subtlety in the documentation, though. It might be worth mentioning
> > (unless I just missed it).
>
> For --contains we explicitly document "contain the specified commit",
> i.e. you couldn't expect this to list tree-test, and indeed it
> doesn't:
>
> $ git tag tree-test master^{tree}
> $ git tag -l --contains master '*test*'
Right, "--contains" cannot have a commit inside a tree, so we were
correct to skip the computation entirely. But does that mean that
"--no-contains" should be the complement of that, or should it only
include tags whose "contains" can be computed in the first place?
IOW, I don't think --contains or --merged are interesting here; they
give the right answer by skipping the computation. The question is what
the "--no-" variants should do.
> However the --[no-]merged option says "reachable [...] from the
> specified commit", which seems to me to be a bit more ambiguous as to
> whether you could expect it to print tree/blob tags.
I suspect that --no-merged behaves the way it does because it originally
came from git-branch, where you only have commits in the first place.
The other commands only learned about it during the move to ref-filter,
and nobody thought about this corner case.
So we could just treat it like a bug and fix it. But I doubt anybody
cares that much in practice either way, so documenting it as "any use of
--contains, --no-contains, --no-merged, or --merged requires that the
ref in question be a commit" is fine, too.
-Peff
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref
2017-03-20 19:52 ` Jeff King@ 2017-03-20 20:04 ` Ævar Arnfjörð Bjarmason0 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-20 20:04 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Lars Hjemli, Christian Couder, Carlos Rica, Samuel Tardieu, Tom Grennan
On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think the more relevant comparison is "--no-merged", and it behaves
>> > the same way as your new --no-contains. I don't think I saw this
>> > subtlety in the documentation, though. It might be worth mentioning
>> > (unless I just missed it).
>>
>> For --contains we explicitly document "contain the specified commit",
>> i.e. you couldn't expect this to list tree-test, and indeed it
>> doesn't:
>>
>> $ git tag tree-test master^{tree}
>> $ git tag -l --contains master '*test*'
>
> Right, "--contains" cannot have a commit inside a tree, so we were
> correct to skip the computation entirely. But does that mean that
> "--no-contains" should be the complement of that, or should it only
> include tags whose "contains" can be computed in the first place?
>
> IOW, I don't think --contains or --merged are interesting here; they
> give the right answer by skipping the computation. The question is what
> the "--no-" variants should do.
I think both should only ever find commits. I only came up with this
tree/blob scenario for the purposes of tests, but it would make the
command less useful & way slower in practice. E.g. now you want to
find what to revert to and some blob tag shows up.
>> However the --[no-]merged option says "reachable [...] from the
>> specified commit", which seems to me to be a bit more ambiguous as to
>> whether you could expect it to print tree/blob tags.
>
> I suspect that --no-merged behaves the way it does because it originally
> came from git-branch, where you only have commits in the first place.
> The other commands only learned about it during the move to ref-filter,
> and nobody thought about this corner case.
>
> So we could just treat it like a bug and fix it. But I doubt anybody
> cares that much in practice either way, so documenting it as "any use of
> --contains, --no-contains, --no-merged, or --merged requires that the
> ref in question be a commit" is fine, too.
It's fixed in my soon-to-be resent series.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*[PATCH v2 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."
2017-03-18 19:07 ` Junio C Hamano
2017-03-21 14:21 ` [PATCH v2 0/2] A couple of minor improvements Ævar Arnfjörð Bjarmason
@ 2017-03-21 14:21 ` Ævar Arnfjörð Bjarmason
2017-03-21 14:21 ` [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-21 14:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
Amend the section which describes how the first line of the subject
should look like to say that the ":" in "area: " shouldn't be treated
like a full stop for the purposes of letter casing.
Change the two subject examples to make this new paragraph clearer,
i.e. "unstar" is not a common word, and "git-cherry-pick.txt" is a
much longer string than "githooks.txt". Pick two recent commits from
git.git that fit better for the description.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/SubmittingPatches | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3faf7eb884..9ef624ce38 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -98,12 +98,17 @@ should skip the full stop. It is also conventional in most cases to
prefix the first line with "area: " where the area is a filename or
identifier for the general area of the code being modified, e.g.
- . archive: ustar header checksum is computed unsigned
- . git-cherry-pick.txt: clarify the use of revision range notation
+ . doc: clarify distinction between sign-off and pgp-signing
+ . githooks.txt: improve the intro section
If in doubt which identifier to use, run "git log --no-merges" on the
files you are modifying to see the current conventions.
+It's customary to start the remainder of the first line after "area: "
+with a lower-case letter. E.g. "doc: clarify...", not "doc:
+Clarify...", or "githooks.txt: improve...", not "githooks.txt:
+Improve...".
+
The body should provide a meaningful commit message, which:
. explains the problem the change tries to solve, iow, what is wrong
--
2.11.0
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary
2017-03-21 14:21 ` [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary Ævar Arnfjörð Bjarmason
@ 2017-03-21 15:51 ` SZEDER Gábor
2017-03-21 17:58 ` Junio C Hamano0 siblings, 1 reply; 49+ messages in thread
From: SZEDER Gábor @ 2017-03-21 15:51 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: SZEDER Gábor, Junio C Hamano, git
> Amend the section which describes how to get a commit summary to show
> how do to that with "git show", currently the documentation only shows
> how to do that with gitk.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> Documentation/SubmittingPatches | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 9ef624ce38..d8c88153c1 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -134,8 +134,17 @@ with the subject enclosed in a pair of double-quotes, like this:
> noticed that ...
>
> The "Copy commit summary" command of gitk can be used to obtain this
> -format.
> +format, or this invocation of "git show":
>
> + git show -s --date=short --pretty='format:%h ("%s", %ad)' <commit>
> +
> +To turn that into a handy alias:
> +
> + git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
> +
> +And then to get the commit summary:
> +
> + git git-commit-summary <commit>
- 'tformat:' is a better fit than 'format:' in this case, because it
adds a trailing newline.
- I actually have a pretty format alias exactly for this purpose:
pretty.commitref=tformat:%h (%s, %as)
and a shorter 'git log -1 --pretty=commitref <commit>' does the
trick. Perhaps it would be worth to provide this as a builtin
pretty format.
(Of course this relies on '%as' being interpreted as short author
date to make the format work on its own, without the additional
'--date=short'. Alas, git doesn't support this, see [1].
If only there were a pretty format specifier for suppressing diff
output, then the '-s' could go away as well.)
- I find that the two subsequent 'git's in 'git git-<whatever>' look
strange. However, to make this point moot right away:
- I don't think SubmittingPatches is the right place to show how to
create and use a command alias.
[1] - http://public-inbox.org/git/1444235305-8718-1-git-send-email-szeder@ira.uka.de/T/#u^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary
2017-03-21 15:51 ` 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ábor0 siblings, 2 replies; 49+ messages in thread
From: Junio C Hamano @ 2017-03-21 17:58 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Ævar Arnfjörð Bjarmason, git
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> The "Copy commit summary" command of gitk can be used to obtain this
>> -format.
>> +format, or this invocation of "git show":
>>
>> + git show -s --date=short --pretty='format:%h ("%s", %ad)' <commit>
>> +
>> +To turn that into a handy alias:
>> +
>> + git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>> +
>> +And then to get the commit summary:
>> +
>> + git git-commit-summary <commit>
>
> - 'tformat:' is a better fit than 'format:' in this case, because it
> adds a trailing newline.
That depends on what you use it for. I most often use mine to
insert the reference that flows in a sentence, not as a separate
displayed material, e.g.
1f6b1afe ("Git 2.12.1", 2017-03-20)
so for that purpose, not adding a trailing newline is a feature.
> - I find that the two subsequent 'git's in 'git git-<whatever>' look
> strange. However, to make this point moot right away:
>
> - I don't think SubmittingPatches is the right place to show how to
> create and use a command alias.
These two I do agree with.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary
2017-03-21 17:58 ` Junio C Hamano@ 2017-03-21 18:47 ` Ævar Arnfjörð Bjarmason
2017-03-21 20:01 ` SZEDER Gábor1 sibling, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-21 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, Git Mailing List
On Tue, Mar 21, 2017 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> The "Copy commit summary" command of gitk can be used to obtain this
>>> -format.
>>> +format, or this invocation of "git show":
>>>
>>> + git show -s --date=short --pretty='format:%h ("%s", %ad)' <commit>
>>> +
>>> +To turn that into a handy alias:
>>> +
>>> + git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>>> +
>>> +And then to get the commit summary:
>>> +
>>> + git git-commit-summary <commit>
>>
>> - 'tformat:' is a better fit than 'format:' in this case, because it
>> adds a trailing newline.
>
> That depends on what you use it for. I most often use mine to
> insert the reference that flows in a sentence, not as a separate
> displayed material, e.g.
>
> 1f6b1afe ("Git 2.12.1", 2017-03-20)
>
> so for that purpose, not adding a trailing newline is a feature.
I agree with tformat. I didn't notice this because I've been screwing
around with my pager settings and my configuration was implicitly
adding a newline. Do you mind fixing that up Junio, or should I
re-send it?
>> - I find that the two subsequent 'git's in 'git git-<whatever>' look
>> strange. However, to make this point moot right away:
>>
>> - I don't think SubmittingPatches is the right place to show how to
>> create and use a command alias.
>
> These two I do agree with.
I don't think it disturbs the flow of the document, and since someone
going through SubmittingPatches is likely about to submit a patch,
providing that one-liner is handy, not as some "here's how to add
aliases" tutorial, but so you don't need to go and copy/paste it, add
\'s for the "'s etc.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary
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 Hamano1 sibling, 1 reply; 49+ messages in thread
From: SZEDER Gábor @ 2017-03-21 20:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, Git mailing list
On Tue, Mar 21, 2017 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> The "Copy commit summary" command of gitk can be used to obtain this
>>> -format.
>>> +format, or this invocation of "git show":
>>>
>>> + git show -s --date=short --pretty='format:%h ("%s", %ad)' <commit>
>>> +
>>> +To turn that into a handy alias:
>>> +
>>> + git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>>> +
>>> +And then to get the commit summary:
>>> +
>>> + git git-commit-summary <commit>
>>
>> - 'tformat:' is a better fit than 'format:' in this case, because it
>> adds a trailing newline.
>
> That depends on what you use it for. I most often use mine to
> insert the reference that flows in a sentence, not as a separate
> displayed material, e.g.
>
> 1f6b1afe ("Git 2.12.1", 2017-03-20)
>
> so for that purpose, not adding a trailing newline is a feature.
Perhaps we are running it differently.
I use its output that way, too, usually running the command in a
terminal and copy-pasting its output into an editor. For this I find
'tformat:' clearly better, because the reference ends up on its own
line: it's separate from the next prompt and easy to copy the whole
thing with a triple-click anywhere on that line.
With 'format:' the subsequent shell prompt is the direct continuation
of the reference:
~/src/git (master %)$ git show -s --date=short --pretty='format:%h
("%s", %ad)' master
c0f9c7058 ("Sync with 2.12.1", 2017-03-20)~/src/git (master %)$
I don't like this, because it looks ugly, is a bit more difficult to
copy, and if I press the up key for shell history, then for some
reason it gets messed up like this:
c0f9c7058 ("Sync with 2.12.1", 2017-03-20)~/src/git (master %)$ git
show -s --da
masterrt --pretty='format:%h ("%s", %ad)' m
with the cursor blinking on the last line after 'master'.
^permalinkrawreply [flat|threaded] 49+ messages in thread

*Re: [PATCH v2 2/2] doc/SubmittingPatches: show how to get a CLI commit summary
2017-03-21 20:01 ` SZEDER Gábor@ 2017-03-21 20:13 ` Junio C Hamano0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2017-03-21 20:13 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Ævar Arnfjörð Bjarmason, Git mailing list
SZEDER Gábor <szeder.dev@gmail.com> writes:
>> That depends on what you use it for. I most often use mine to
>> insert the reference that flows in a sentence, not as a separate
>> displayed material, e.g.
>>
>> 1f6b1afe ("Git 2.12.1", 2017-03-20)
>>
>> so for that purpose, not adding a trailing newline is a feature.
>
> Perhaps we are running it differently.
>
> I use its output that way, too, usually running the command in a
> terminal and copy-pasting its output into an editor.
I do \C-u\M-!git one<ENTER> from Emacs in the middle of typing a
sentence (where "git one" is aliased to that --format thing), and
for this I obviously do not want the terminating newline.
Of course --pretty=format:... has the opposite effect and in a
terminal to make it easier to cut&paste you do want to have its
output separated from the prompt string.
So as I said, "That depends on what you use it for."
As this is a mere example, we should just shoot for brevity instead?
Both "--pretty=format:" and "--pretty=tformat:" are much longer than
"--format=", so we can just mention
git show --date=short -s --format='%h ("%s", %ad)'
and let the users to customize it for their needs?
^permalinkrawreply [flat|threaded] 49+ messages in thread