Skip to contentSkip to conversation

fix some exclude patterns being ignored

PATCH In review Rafael Ascensão 3 participants 5 messages Nov 12, 2018 – Nov 13, 2018

lore
Share & tools
lore

Rafael Ascensão

author
#0 lore raw

cover fix some exclude patterns being ignored

While trying to set up some aliases for my own use, I found out that --exclude with --branches behave differently depending if the latter uses globs.

I tried to fix it making my 2nd contribution. :)

Cheers,

Rafael Ascensão (2):
refs: show --exclude failure with --branches/tags/remotes=glob
refs: fix some exclude patterns being ignored

 refs.c                   |  4 +++
 t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 3 deletions(-)
Signature · 1 line
2.19.1
author
#1 lore raw

PATCH 1/2 refs: show --exclude failure with --branches/tags/remotes=glob

The documentation of --exclude= option from rev-list and rev-parse explicitly states that exclude patterns *should not* start with 'refs/' when used with --branches, --tags or --remotes.

However, following this advice results in refereces not being excluded if the next --branches, --tags, --remotes use the optional inclusive glob.

Demonstrate this failure.

Signed-off-by Rafael Ascensão <[email protected]>
---
 t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)
t/t6018-rev-list-glob.sh +57 -3
index 0bf10d0686..8e2b136356 100755
@@ -36,7 +36,13 @@ test_expect_success 'setup' '
 	git tag foo/bar master &&
 	commit master3 &&
 	git update-ref refs/remotes/foo/baz master &&
-	commit master4
+	commit master4 &&
+	git update-ref refs/remotes/upstream/one subspace/one &&
+	git update-ref refs/remotes/upstream/two subspace/two &&
+	git update-ref refs/remotes/upstream/x subspace-x &&
+	git tag qux/one subspace/one &&
+	git tag qux/two subspace/two &&
+	git tag qux/x subspace-x
 '
 
 test_expect_success 'rev-parse --glob=refs/heads/subspace/*' '

@@ -141,6 +147,54 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '
 	compare rev-parse "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
 '
 
+test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
+	compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'
+
+test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
+	compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'
+
+test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
+	compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"
+'
+
+test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
+	compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'
+
+test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
+	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'
+
+test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
+	compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
+	compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --branches=glob' '
+	compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --tags=glob' '
+	compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '
+	compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'
+
 test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
 
 	compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"

@@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '
 
 test_expect_success 'rev-list --tags' '
 
-	compare rev-list "foo/bar" "--tags"
+	compare rev-list "foo/bar qux/x qux/two qux/one" "--tags"
 
 '
 

@@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
 	  "master other/three someref subspace-x subspace/one subspace/two" \
 	  "--glob=heads/*" &&
 	compare shortlog foo/bar --tags=foo &&
-	compare shortlog foo/bar --tags &&
+	compare shortlog "foo/bar qux/one qux/two qux/x" --tags &&
 	compare shortlog foo/baz --remotes=foo
 
 '
Signature · 1 line
2.19.1
#2 lore raw

Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob

Quoted 8 lines · Rafael Ascensão

The documentation of --exclude= option from rev-list and rev-parse explicitly states that exclude patterns *should not* start with 'refs/' when used with --branches, --tags or --remotes.

However, following this advice results in refereces not being excluded if the next --branches, --tags, --remotes use the optional inclusive glob.

Quoted 6 lines

Demonstrate this failure.

Signed-off-by Rafael Ascensão <[email protected]>
---
 t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

For a trivially small change/fix like this (i.e. the real fix in 2/2 is just 4 lines), it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough.

Often the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. For this particular test, s/_failure/_success/ shows everything in the verification phase, but the entire set-up for these tests cannot be seen while reviewing 2/2. Unlike that, a single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches.

Quoted 7 lines
t/t6018-rev-list-glob.sh +0 -0
index 0bf10d0686..8e2b136356 100755
@@ -36,7 +36,13 @@ test_expect_success 'setup' '
 	git tag foo/bar master &&
 	commit master3 &&

t/t6018-rev-list-glob.sh test_expect_success 'setup' ' · line 46

Quoted 10 lines
t/t6018-rev-list-glob.sh +7 -1
 	git update-ref refs/remotes/foo/baz master &&
-	commit master4
+	commit master4 &&
+	git update-ref refs/remotes/upstream/one subspace/one &&
+	git update-ref refs/remotes/upstream/two subspace/two &&
+	git update-ref refs/remotes/upstream/x subspace-x &&
+	git tag qux/one subspace/one &&
+	git tag qux/two subspace/two &&
+	git tag qux/x subspace-x
 '

Let me follow along.

We add three remote-tracking looking branches for 'upstream', and three tags under refs/tags/qux/.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 152

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
+	compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'

We want to list all branches that begin with "sub", but we do not want ones that begin with "subspace-". subspace/{one,two} should pass that criteria, while subspace-x, other/three, someref, and master should not. Makes sense.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 156

Quoted 4 lines
t/t6018-rev-list-glob.sh +4 -0
+
+test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
+	compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'

We want all tags that begin with "qux/" but we do not want qux/ followed by just a single letter. qux/{one,two} are in, qux/x is out. Makes sense.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 160

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'

Similarly for refs/remotes/upstream/ hierarchy.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 164

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
+	compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two
+'

This is almost a repeat of the first new one. As subspace-* in branches only match subspace-x, this should give the same result as that one.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 168

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
+	compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'

Likewise.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 172

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'

Likewise.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 176

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
+	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'

And then the same pattern continues with rev-list.

Quoted 10 lines
t/t6018-rev-list-glob.sh +10 -0
+test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
+	compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
+	compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --branches=glob' '
+	compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 197

Quoted 10 lines
t/t6018-rev-list-glob.sh +10 -0
+'
+
+test_expect_failure 'rev-list --exclude=ref with --tags=glob' '
+	compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '
+	compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'
+

With the ordering of these tests, it is fairly clear that you are exhaustively testing all the combinations

for command in rev-parse rev-list:
for exclude in glob ref:
for specifc in glob ref:
for kind in branches tags remotes:
compare $command exclude=$exclude --$kind=$specific

which is very good. No, I am not suggesting to write a shell loop to drive these tests; I am saying that the list of tests are in the same order as such a nested loop would invoke compare, which makes it predictable for the readers who pay attention, and it is a good thing.

t/t6018-rev-list-glob.sh test_expect_success 'rev-list --tags=foo' ' · line 290

Quoted 9 lines

test_expect_success 'rev-list --glob=refs/heads/subspace/*' '

t/t6018-rev-list-glob.sh +1 -1
 	compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"

@@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '
 
 test_expect_success 'rev-list --tags' '
 
-	compare rev-list "foo/bar" "--tags"
+	compare rev-list "foo/bar qux/x qux/two qux/one" "--tags"

Of course, you'd need to compensate for new stuff here ...

t/t6018-rev-list-glob.sh test_expect_success 'shortlog accepts --glob/--tags/--remotes' ' · line 349

Quoted 9 lines
t/t6018-rev-list-glob.sh +1 -1
 '
 

@@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
 	  "master other/three someref subspace-x subspace/one subspace/two" \
 	  "--glob=heads/*" &&
 	compare shortlog foo/bar --tags=foo &&
-	compare shortlog foo/bar --tags &&
+	compare shortlog "foo/bar qux/one qux/two qux/x" --tags &&

... and here.

t/t6018-rev-list-glob.sh test_expect_success 'shortlog accepts --glob/--tags/--remotes' ' · line 350

Quoted 1 line
t/t6018-rev-list-glob.sh +0 -0
 	compare shortlog foo/baz --remotes=foo

All makes sense. Will queue.

#3 lore raw

Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob

Hi Junio,

Quoted 17 lines · Junio C Hamano
Quoted 8 lines · Rafael Ascensão

The documentation of --exclude= option from rev-list and rev-parse explicitly states that exclude patterns *should not* start with 'refs/' when used with --branches, --tags or --remotes.

However, following this advice results in refereces not being excluded if the next --branches, --tags, --remotes use the optional inclusive glob.

Quoted 6 lines

Demonstrate this failure.

Signed-off-by Rafael Ascensão <[email protected]>
---
 t/t6018-rev-list-glob.sh | 60 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)
Quoted 5 lines

For a trivially small change/fix like this (i.e. the real fix in 2/2 is just 4 lines), it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough.

I wish you were not so adamant about this. I really consider it poor style to smoosh those together, and there is nothing easy about disentangling changes that have been thrown into the same commit. Please stop saying that this is easy. It is as easy as maintaining Linux kernel development using .tar files and patches. It is possible, yes, and Linus Torvalds did it for years. It is also error-prone and the entire reason we have Git. And nobody wants to go back anymore to .tar files and patches. Likewise, I do not want to read anybody recommending some semi-understandable diff|apply-R dance when the alternative would be a simple cherry-pick. I do not even want to read such a recommendation from you. I respect you a lot for what you do, and for your knowledge, but this is simply bad advice and I would wish you stopped giving it.

Besides, we spent a decade trying to come up with clear-cut rules how to organize commits, and we ended up pretty quickly with recommending logically-separate changes belonging to separate commits. A typo fix should not be thrown in with a regression fix, they are two different things. Likewise, demonstrating a bug is a different thing from fixing it.

If you need more arguments to make the case, here is another one: it is reflecting the reality a lot better if the regression test comes first, and then the fix. This is how Rafael did it, too, according to what he said on IRC. And reflecting this in the commit history is a good thing, not a bad thing.

It goes further: obviously, Rafael had really good success with this strategy, even figuring out part of the bug while trying to write the regression test.

I, myself wrote a regression test yesterday that completely short-circuited the bug hunt: originally, I thought the left-over MERGE_HEAD files in the rebase -r stemmed from mere conflicts during a merge command, and somehow git commit not cleaning it properly. But when I wrote that regression test and ran it, it failed to show a regression. So then I took my (rather lengthy: >200 todo commands) real-world example, and condensed it into the regression test that you saw yesterday. I would estimate that this saved me about 1-3 hours of debugging in vain.

So it is a very, very good idea to start with the regression test, and only then analyze the bug.

Reading the commit history this way makes therefore not only sense, but also sets a good example for new contributors to follow.

For these reasons, and many more, I implore you to stop suggesting to conflate the demonstration of a bug with the fix.

Instead, we should be happy to see good practices in action and encourage more of the same.

Thank you,
Dscho

Quoted 153 lines

Often the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. For this particular test, s/_failure/_success/ shows everything in the verification phase, but the entire set-up for these tests cannot be seen while reviewing 2/2. Unlike that, a single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix.

Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches.

Quoted 7 lines
t/t6018-rev-list-glob.sh +0 -0
index 0bf10d0686..8e2b136356 100755
@@ -36,7 +36,13 @@ test_expect_success 'setup' '
 	git tag foo/bar master &&
 	commit master3 &&

t/t6018-rev-list-glob.sh test_expect_success 'setup' ' · line 46

Quoted 10 lines
t/t6018-rev-list-glob.sh +7 -1
 	git update-ref refs/remotes/foo/baz master &&
-	commit master4
+	commit master4 &&
+	git update-ref refs/remotes/upstream/one subspace/one &&
+	git update-ref refs/remotes/upstream/two subspace/two &&
+	git update-ref refs/remotes/upstream/x subspace-x &&
+	git tag qux/one subspace/one &&
+	git tag qux/two subspace/two &&
+	git tag qux/x subspace-x
 '

Let me follow along.

We add three remote-tracking looking branches for 'upstream', and three tags under refs/tags/qux/.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 152

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
+	compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'

We want to list all branches that begin with "sub", but we do not want ones that begin with "subspace-". subspace/{one,two} should pass that criteria, while subspace-x, other/three, someref, and master should not. Makes sense.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 156

Quoted 4 lines
t/t6018-rev-list-glob.sh +4 -0
+
+test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
+	compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'

We want all tags that begin with "qux/" but we do not want qux/ followed by just a single letter. qux/{one,two} are in, qux/x is out. Makes sense.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 160

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'

Similarly for refs/remotes/upstream/ hierarchy.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 164

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
+	compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two
+'

This is almost a repeat of the first new one. As subspace-* in branches only match subspace-x, this should give the same result as that one.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 168

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
+	compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'

Likewise.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 172

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
+	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'

Likewise.

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 176

Quoted 3 lines
t/t6018-rev-list-glob.sh +3 -0
+test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
+	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
+'

And then the same pattern continues with rev-list.

Quoted 10 lines
t/t6018-rev-list-glob.sh +10 -0
+test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
+	compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
+	compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --branches=glob' '
+	compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"

t/t6018-rev-list-glob.sh test_expect_success 'rev-parse accumulates multiple --exclude' ' · line 197

Quoted 10 lines
t/t6018-rev-list-glob.sh +10 -0
+'
+
+test_expect_failure 'rev-list --exclude=ref with --tags=glob' '
+	compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
+'
+
+test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '
+	compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
+'
+

With the ordering of these tests, it is fairly clear that you are exhaustively testing all the combinations

for command in rev-parse rev-list:
for exclude in glob ref:
for specifc in glob ref:
for kind in branches tags remotes:
compare $command exclude=$exclude --$kind=$specific

which is very good. No, I am not suggesting to write a shell loop to drive these tests; I am saying that the list of tests are in the same order as such a nested loop would invoke compare, which makes it predictable for the readers who pay attention, and it is a good thing.

t/t6018-rev-list-glob.sh test_expect_success 'rev-list --tags=foo' ' · line 290

Quoted 9 lines

test_expect_success 'rev-list --glob=refs/heads/subspace/*' '

t/t6018-rev-list-glob.sh +1 -1
 	compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"

@@ -233,7 +287,7 @@ test_expect_success 'rev-list --tags=foo' '
 
 test_expect_success 'rev-list --tags' '
 
-	compare rev-list "foo/bar" "--tags"
+	compare rev-list "foo/bar qux/x qux/two qux/one" "--tags"

Of course, you'd need to compensate for new stuff here ...

t/t6018-rev-list-glob.sh test_expect_success 'shortlog accepts --glob/--tags/--remotes' ' · line 349

Quoted 9 lines
t/t6018-rev-list-glob.sh +1 -1
 '
 

@@ -292,7 +346,7 @@ test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
 	  "master other/three someref subspace-x subspace/one subspace/two" \
 	  "--glob=heads/*" &&
 	compare shortlog foo/bar --tags=foo &&
-	compare shortlog foo/bar --tags &&
+	compare shortlog "foo/bar qux/one qux/two qux/x" --tags &&

... and here.

t/t6018-rev-list-glob.sh test_expect_success 'shortlog accepts --glob/--tags/--remotes' ' · line 350

Quoted 1 line
t/t6018-rev-list-glob.sh +0 -0
 	compare shortlog foo/baz --remotes=foo

All makes sense. Will queue.

author
#4 lore raw

PATCH 2/2 refs: fix some exclude patterns being ignored

In reply to Rafael Ascensão

--exclude from rev-list and rev-parse fails to exclude references if the next --branches, --tags or --remotes use the optional inclusive glob because those options are implemented as particular cases of --glob=, which itself requires that exclude patterns begin with 'refs/'.

But it makes sense for --branches=glob and friends to be aware that exclusions patterns for them shouldn't be 'refs/<type>/' prefixed, the same way exclude patterns for --branches and friends (without the optional glob) already are.

Let's record in 'refs.c:struct ref_filter' which context the exclude pattern is tied to, so refs.c:filter_refs() can decide if it should ignore the prefix when trying to match.

Signed-off-by Rafael Ascensão <[email protected]>
---
 refs.c                   |  4 ++++
 t/t6018-rev-list-glob.sh | 24 ++++++++++++------------
 2 files changed, 16 insertions(+), 12 deletions(-)
refs.c +4 -0
index de81c7be7c..539f385f61 100644
@@ -217,6 +217,7 @@ char *resolve_refdup(const char *refname, int resolve_flags,
 /* The argument to filter_refs */
 struct ref_filter {
 	const char *pattern;
+	const char *prefix;
 	each_ref_fn *fn;
 	void *cb_data;
 };

@@ -296,6 +297,8 @@ static int filter_refs(const char *refname, const struct object_id *oid,
 
 	if (wildmatch(filter->pattern, refname, 0))
 		return 0;
+	if (filter->prefix)
+		skip_prefix(refname, filter->prefix, &refname);
 	return filter->fn(refname, oid, flags, filter->cb_data);
 }
 

@@ -458,6 +461,7 @@ int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 	}
 
 	filter.pattern = real_pattern.buf;
+	filter.prefix = prefix;
 	filter.fn = fn;
 	filter.cb_data = cb_data;
 	ret = for_each_ref(filter_refs, &filter);
t/t6018-rev-list-glob.sh +12 -12
index 8e2b136356..7dc6cbdc42 100755
@@ -147,51 +147,51 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '
 	compare rev-parse "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
 '
 
-test_expect_failure 'rev-parse --exclude=glob with --branches=glob' '
+test_expect_success 'rev-parse --exclude=glob with --branches=glob' '
 	compare rev-parse "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
 '
 
-test_expect_failure 'rev-parse --exclude=glob with --tags=glob' '
+test_expect_success 'rev-parse --exclude=glob with --tags=glob' '
 	compare rev-parse "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
 '
 
-test_expect_failure 'rev-parse --exclude=glob with --remotes=glob' '
+test_expect_success 'rev-parse --exclude=glob with --remotes=glob' '
 	compare rev-parse "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
 '
 
-test_expect_failure 'rev-parse --exclude=ref with --branches=glob' '
+test_expect_success 'rev-parse --exclude=ref with --branches=glob' '
 	compare rev-parse "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"
 '
 
-test_expect_failure 'rev-parse --exclude=ref with --tags=glob' '
+test_expect_success 'rev-parse --exclude=ref with --tags=glob' '
 	compare rev-parse "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
 '
 
-test_expect_failure 'rev-parse --exclude=ref with --remotes=glob' '
+test_expect_success 'rev-parse --exclude=ref with --remotes=glob' '
 	compare rev-parse "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
 '
 
-test_expect_failure 'rev-list --exclude=glob with --branches=glob' '
+test_expect_success 'rev-list --exclude=glob with --branches=glob' '
 	compare rev-list "--exclude=subspace-* --branches=sub*" "subspace/one subspace/two"
 '
 
-test_expect_failure 'rev-list --exclude=glob with --tags=glob' '
+test_expect_success 'rev-list --exclude=glob with --tags=glob' '
 	compare rev-list "--exclude=qux/? --tags=qux/*" "qux/one qux/two"
 '
 
-test_expect_failure 'rev-list --exclude=glob with --remotes=glob' '
+test_expect_success 'rev-list --exclude=glob with --remotes=glob' '
 	compare rev-list "--exclude=upstream/? --remotes=upstream/*" "upstream/one upstream/two"
 '
 
-test_expect_failure 'rev-list --exclude=ref with --branches=glob' '
+test_expect_success 'rev-list --exclude=ref with --branches=glob' '
 	compare rev-list "--exclude=subspace-x --branches=sub*" "subspace/one subspace/two"
 '
 
-test_expect_failure 'rev-list --exclude=ref with --tags=glob' '
+test_expect_success 'rev-list --exclude=ref with --tags=glob' '
 	compare rev-list "--exclude=qux/x --tags=qux/*" "qux/one qux/two"
 '
 
-test_expect_failure 'rev-list --exclude=ref with --remotes=glob' '
+test_expect_success 'rev-list --exclude=ref with --remotes=glob' '
 	compare rev-list "--exclude=upstream/x --remotes=upstream/*" "upstream/one upstream/two"
 '
Signature · 1 line
2.19.1