Re: [PATCH] branch & tag: Add a --no-contains option

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 9, 2017 at 11:46 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 09, 2017 at 11:41:59AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I almost suggested that there simply be an option to invert the match
>> > (like --invert-contains or something).  But what you have here is more
>> > flexible, if somebody ever wanted to do:
>> >
>> >   git tag --contains X --no-contains Y
>>
>> Yeah that's really useful. E.g. this shows the branches I branched off
>> (or have locally) from 2.6..2.8:
>>
>>     $ ./git branch --contains v2.6.0 --no-contains v2.8.0
>>       avar/monkeypatch-untracked-cache-disabled
>>       avar/uc-notifs21
>>       dturner/pclouds-watchman-noshm
>
> Oh, that's a clever application.
>
>> But I'd expect this to show all the tags between the two:
>>
>>     $ ./git tag --contains v2.6.0 --no-contains v2.8.0
>>     $
>>
>> But it just returns an empty list. Manually disabling the
>> contains_tag_algo() path (i.e. effectively locally reverting your
>> ffc4b8012d) makes it "work", but of course it's much slower now. I
>> haven't dug into why it's not working yet.
>
> I'm almost certain this is because the contains_tag_algo one doesn't
> clean up the flag bits it sets on the commit objects. So running it
> twice in the same process is going to give you nonsense results.

Yeah indeed.

I tried to hack something up to avoid this, but the
lookup_commit_reference_gently() we call will return the same
object.parent pointer for two invocations, i.e. the underlying
{commit,object}.c API has a cache of objects it returns, couldn't find
some way to quickly make it burst that cache.

The other approach of making contains_tag_algo() itself detect that
it's been called before (or us passing a flag) and going around
setting commit.object.flags on everything to 0 seemed even more
brittle, particularly since I think between filter->with_commit &
filter->no_commit we might end up visiting different commits, so it's
not easy to just clear it.

I'm happy to hack on it given some pointers, will visit it again, but
for now unless I'm missing something obvious / you can point out some
way to hack this up I'll just submit v2 with the combination of
--contains & --no-contains dying with a TODO message.

The patch without that functionality is still really useful, and we
can implement that later.

> Coincidentally, I've been looking into resurrecting the cleaner approach
> that I sent long ago:
>
>   http://public-inbox.org/git/20140625233429.GA20457@sigill.intra.peff.net/
>
> But it's sufficiently complex that it's probably worth fixing the
> existing algorithm to clean up its bits in the meantime.
>
>> Also I wonder if this should be an error:
>>
>>     $ ./git [tag|branch|for-each-ref] --contains A --no-contains A
>>
>> I.e. when you give the same argument to both, this can never return
>> anything for obvious reasons.
>
> It's clearly nonsense, but I don't think there's any need for it to be
> an error. GIGO.

Yeah, make sense.