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

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

[Andreas Gruenbacher]

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(+)

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

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

[Junio C Hamano]

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?

Thanks.

> 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)) {

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

[Andreas Gruenbacher]

On Wed, 24 Oct 2018 at 06:35, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.  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.

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

[Andreas Gruenbacher]

On Wed, 24 Oct 2018 at 11:24, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Jeff King]

On Wed, Oct 24, 2018 at 11:49:06AM +0200, Andreas Gruenbacher wrote:

> > 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Fri, Oct 26, 2018 at 08:43:37AM +0900, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 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(-)

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/*"

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

[Jeff King]

On Sat, Oct 27, 2018 at 04:12:40PM +0900, Junio C Hamano wrote:

> > 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