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

From: Andreas Gruenbacher <agruenba@redhat.com>
Date: Oct 24, 2018, 09:49 UTC
Message-ID: <CAHc6FU5CpBNpzfPs0hkNm9Hw1iYnALvUY4WL1z=sPVT10fq+zw@mail.gmail.com>
In-Reply-To: <xmqqr2gfn1cy.fsf@gitster-ct.c.googlers.com>

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