threads / patch / 49652

patchClear --exclude list after 'git rev-parse --all'

Subject: [PATCH] Clear --exclude list after 'git rev-parse --all'

## tl;dr

10 messages between Oct 23, 2018 and Oct 27, 2018. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Andreas Gruenbacher· Oct 23, 2018, 19:17 UTC · lore

Commit [1] added the --exclude option to revision.c. The --all, --branches, --tags, --remotes, and --glob options clear the exclude list. Shortly therafter, commit [2] added the same to 'git rev-parse', but without clearing the exclude list for the --all option. Fix that.

[1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30) [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01)

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 builtin/rev-parse.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/rev-parse.c +1 −0
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 0f09bbbf6..c71e3b104 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--all")) {
 				for_each_ref(show_reference, NULL);
+				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
 			if (skip_prefix(arg, "--disambiguate=", &arg)) {
-- 
2.17.2
Junio C Hamano· Oct 24, 2018, 04:35 UTC · re: Andreas Gruenbacher · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

Andreas Gruenbacher <agruenba@redhat.com> writes:
Show 12 quoted lines
> Commit [1] added the --exclude option to revision.c.  The --all,
> --branches, --tags, --remotes, and --glob options clear the exclude
> list.  Shortly therafter, commit [2] added the same to 'git rev-parse',
> but without clearing the exclude list for the --all option.  Fix that.
>
> [1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30)
> [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01)
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  builtin/rev-parse.c | 1 +
>  1 file changed, 1 insertion(+)

All other glob options do show_reference with for_each_ref_in() and then calls clear_ref_exclusion(), and logically the patch makes sense.

What is the "problem" this patch fixes, though? Is it easy to add a new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or whatever that clears exclusion list without this patch) works correctly but "--all" misbehaves without this change?

Thanks.
Show 12 quoted lines
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 0f09bbbf6..c71e3b104 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			}
>  			if (!strcmp(arg, "--all")) {
>  				for_each_ref(show_reference, NULL);
> +				clear_ref_exclusion(&ref_excludes);
>  				continue;
>  			}
>  			if (skip_prefix(arg, "--disambiguate=", &arg)) {
Andreas Gruenbacher· Oct 24, 2018, 09:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

On Wed, 24 Oct 2018 at 06:35, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> Andreas Gruenbacher <agruenba@redhat.com> writes:
>
> > Commit [1] added the --exclude option to revision.c.  The --all,
> > --branches, --tags, --remotes, and --glob options clear the exclude
> > list.  Shortly therafter, commit [2] added the same to 'git rev-parse',
> > but without clearing the exclude list for the --all option.  Fix that.
> >
> > [1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30)
> > [2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01)
> >
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
> >  builtin/rev-parse.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> All other glob options do show_reference with for_each_ref_in() and
> then calls clear_ref_exclusion(), and logically the patch makes
> sense.
>
> What is the "problem" this patch fixes, though?  Is it easy to add a
> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
> whatever that clears exclusion list without this patch) works
> correctly but "--all" misbehaves without this change?

The test suite doesn't cover clearing the exclusion list for any of those rev-parse options and I also didn't write such a test case. I ran into this inconsistency during code review.

Andreas
Junio C Hamano· Oct 24, 2018, 09:24 UTC · re: Andreas Gruenbacher · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

Andreas Gruenbacher <agruenba@redhat.com> writes:
Show 12 quoted lines
>> All other glob options do show_reference with for_each_ref_in() and
>> then calls clear_ref_exclusion(), and logically the patch makes
>> sense.
>>
>> What is the "problem" this patch fixes, though?  Is it easy to add a
>> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
>> whatever that clears exclusion list without this patch) works
>> correctly but "--all" misbehaves without this change?
>
> The test suite doesn't cover clearing the exclusion list for any of
> those rev-parse options and I also didn't write such a test case. I
> ran into this inconsistency during code review.

That is why I asked what "problem" this patch fixes. Without answering that question, it is unclear if the patch is completing missing coverage for "--all", or it is cargo culting an useless clearing done for "--glob" and friends to code for "--all" that did not do the same useless clearing. IOW, there are two ways to address the "inconsistency", and the proposed log message (nor your answer above) does not make a convincing argument why adding the same code to the "--all" side is the right way to achieve consistency---rather than removing the call to clear from the existing ones.

Thanks.
Andreas Gruenbacher· Oct 24, 2018, 09:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

On Wed, 24 Oct 2018 at 11:24, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Andreas Gruenbacher <agruenba@redhat.com> writes:
> >> All other glob options do show_reference with for_each_ref_in() and
> >> then calls clear_ref_exclusion(), and logically the patch makes
> >> sense.
> >>
> >> What is the "problem" this patch fixes, though?  Is it easy to add a
> >> new test to t/6018-rev-list-glob.sh to demonstrate that "--glob" (or
> >> whatever that clears exclusion list without this patch) works
> >> correctly but "--all" misbehaves without this change?
> >
> > The test suite doesn't cover clearing the exclusion list for any of
> > those rev-parse options and I also didn't write such a test case. I
> > ran into this inconsistency during code review.
>
> That is why I asked what "problem" this patch fixes.  Without
> answering that question, it is unclear if the patch is completing
> missing coverage for "--all", or it is cargo culting an useless
> clearing done for "--glob" and friends to code for "--all" that did
> not do the same useless clearing.
This is how the --exclude option is described:
       --exclude=<glob-pattern>
           Do not include refs matching <glob-pattern> that the next
--all, --branches,
           --tags, --remotes, or --glob would otherwise consider.
Repetitions of this
           option accumulate exclusion patterns up to the next --all,
--branches, --tags,
           --remotes, or --glob option (other options or arguments do not clear
           accumulated patterns).

I'm assuming that some thought has gone into making the options behave in this particular way. The implementation in revision.c follows this pattern, but the implementation in builtin/rev-parse.c only almost does.

Andreas
Jeff King· Oct 25, 2018, 10:45 UTC · re: Andreas Gruenbacher · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

On Wed, Oct 24, 2018 at 11:49:06AM +0200, Andreas Gruenbacher wrote:
Show 22 quoted lines
> > That is why I asked what "problem" this patch fixes.  Without
> > answering that question, it is unclear if the patch is completing
> > missing coverage for "--all", or it is cargo culting an useless
> > clearing done for "--glob" and friends to code for "--all" that did
> > not do the same useless clearing.
> 
> This is how the --exclude option is described:
> 
>        --exclude=<glob-pattern>
>            Do not include refs matching <glob-pattern> that the next
> --all, --branches,
>            --tags, --remotes, or --glob would otherwise consider.
> Repetitions of this
>            option accumulate exclusion patterns up to the next --all,
> --branches, --tags,
>            --remotes, or --glob option (other options or arguments do not clear
>            accumulated patterns).
> 
> I'm assuming that some thought has gone into making the options behave
> in this particular way. The implementation in revision.c follows this
> pattern, but the implementation in builtin/rev-parse.c only almost
> does.

Yeah. I think this is just a bug in 9dc01bf063 (rev-parse: introduce --exclude=<glob> to tame wildcards, 2013-11-01), in that it's handling of --all differed from e7b432c521 (revision: introduce --exclude=<glob> to tame wildcards, 2013-08-30). The clearing is very much intentional and documented, and in general rev-parse should behave the same as rev-list.

An easy test is:
  $ git rev-list --no-walk --exclude='*' --all --all
  3289ca716320457af5d2dd550b716282ad22da11
  ...a bunch of other tip commits...
  $ git rev-parse --exclude='*' --all --all
  [no output, but it should print those same tip commits]
-Peff
Junio C Hamano· Oct 25, 2018, 23:43 UTC · re: Jeff King · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> An easy test is:
>
>   $ git rev-list --no-walk --exclude='*' --all --all
>   3289ca716320457af5d2dd550b716282ad22da11
>   ...a bunch of other tip commits...
>
>   $ git rev-parse --exclude='*' --all --all
>   [no output, but it should print those same tip commits]

I actually was hoping to see a test that contrasts "--all" (which lacks the alleged "clear exclude" bug) with another option that does have the "clear exclude", both used with rev-parse, i.e.

    $ git rev-parse --exclude='*' --glob='*' --glob='*'
    ... all the ref tips ...
    $ git rev-parse --exclude='*' --all --all
    ... ought to be equivalent, but is empty due to the bug ...

would have been a good demonstration that shows what bug we are fixing d(and would have been a good test to accompany the patch.

Jeff King· Oct 26, 2018, 07:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

On Fri, Oct 26, 2018 at 08:43:37AM +0900, Junio C Hamano wrote:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > An easy test is:
> >
> >   $ git rev-list --no-walk --exclude='*' --all --all
> >   3289ca716320457af5d2dd550b716282ad22da11
> >   ...a bunch of other tip commits...
> >
> >   $ git rev-parse --exclude='*' --all --all
> >   [no output, but it should print those same tip commits]
> 
> I actually was hoping to see a test that contrasts "--all" (which
> lacks the alleged "clear exclude" bug) with another option that does
> have the "clear exclude", both used with rev-parse, i.e.
> 
>     $ git rev-parse --exclude='*' --glob='*' --glob='*'
>     ... all the ref tips ...
>     $ git rev-parse --exclude='*' --all --all
>     ... ought to be equivalent, but is empty due to the bug ...
> 
> would have been a good demonstration that shows what bug we are
> fixing d(and would have been a good test to accompany the patch.

Yeah, I agree that would be fine, too. I think there are two dimensions in which to look at the problem, like so:

         rev-list  rev-parse
         --------  ---------
--glob    clears    clears
--all     clears    does not clear
Testing either the row or the column (or both) works for me. :)
-Peff
Junio C Hamano· Oct 27, 2018, 07:12 UTC · re: Jeff King · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
>> I actually was hoping to see a test that contrasts "--all" (which
>> lacks the alleged "clear exclude" bug) with another option that does
>> have the "clear exclude", both used with rev-parse, i.e.
>> 
>>     $ git rev-parse --exclude='*' --glob='*' --glob='*'
>>     ... all the ref tips ...
>>     $ git rev-parse --exclude='*' --all --all
>>     ... ought to be equivalent, but is empty due to the bug ...
>> 
>> would have been a good demonstration that shows what bug we are
>> fixing d(and would have been a good test to accompany the patch.
>
> Yeah, I agree that would be fine, too. I think there are two dimensions
> in which to look at the problem, like so:
>
>          rev-list  rev-parse
>          --------  ---------
> --glob    clears    clears
> --all     clears    does not clear
>
> Testing either the row or the column (or both) works for me. :)

OK, so let's not leave this loose end untied. This may be good enough to squash in.

 builtin/rev-parse.c      |  1 -
 t/t6018-rev-list-glob.sh | 12 ++++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 2 files +12 −1

builtin/rev-parse.c, t/t6018-rev-list-glob.sh

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f4847d3008..a1e680b5e9 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -760,7 +760,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "--all")) {
 				for_each_ref(show_reference, NULL);
-				clear_ref_exclusion(&ref_excludes);
 				continue;
 			}
 			if (skip_prefix(arg, "--disambiguate=", &arg)) {
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index d3453c583c..b28075b65d 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -141,6 +141,18 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '
 	compare rev-parse "--exclude=refs/remotes/* --exclude=refs/tags/* --all" --branches
 '
 
+test_expect_success 'rev-parse --branches clears --exclude' '
+	compare rev-parse "--exclude=* --branches --branches" "--branches"
+'
+
+test_expect_success 'rev-parse --tags clears --exclude' '
+	compare rev-parse "--exclude=* --tags --tags" "--tags"
+'
+
+test_expect_success 'rev-parse --all clears --exclude' '
+	compare rev-parse "--exclude=* --all --all" "--all"
+'
+
 test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
 
 	compare rev-list "subspace/one subspace/two" "--glob=refs/heads/subspace/*"
Jeff King· Oct 27, 2018, 07:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] Clear --exclude list after 'git rev-parse --all'

On Sat, Oct 27, 2018 at 04:12:40PM +0900, Junio C Hamano wrote:
Show 24 quoted lines
> > Yeah, I agree that would be fine, too. I think there are two dimensions
> > in which to look at the problem, like so:
> >
> >          rev-list  rev-parse
> >          --------  ---------
> > --glob    clears    clears
> > --all     clears    does not clear
> >
> > Testing either the row or the column (or both) works for me. :)
> 
> OK, so let's not leave this loose end untied.  This may be good
> enough to squash in.
> [...]
> +test_expect_success 'rev-parse --branches clears --exclude' '
> +	compare rev-parse "--exclude=* --branches --branches" "--branches"
> +'
> +
> +test_expect_success 'rev-parse --tags clears --exclude' '
> +	compare rev-parse "--exclude=* --tags --tags" "--tags"
> +'
> +
> +test_expect_success 'rev-parse --all clears --exclude' '
> +	compare rev-parse "--exclude=* --all --all" "--all"
> +'

Yes, this looks good to me. In theory a more intricate test might catch other kinds of bugs (e.g., a more limited exclude and making sure it was applied correctly in each place), but I don't think it's really worth the effort.

-Peff

← back to recent threads