Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Lars Hjemli <hjemli@gmail.com>,
	Christian Couder <christian.couder@gmail.com>,
	Carlos Rica <jasampler@gmail.com>,
	Samuel Tardieu <sam@rfc1149.net>,
	Tom Grennan <tmgrennan@gmail.com>
Subject: Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref
Date: Mon, 20 Mar 2017 21:04:32 +0100	[thread overview]
Message-ID: <CACBZZX5GErSBsiCz0Y2SgtvckcPE51ekfUgBpvYjNF1u44piMA@mail.gmail.com> (raw)
In-Reply-To: <20170320195209.oyivf2ta3m3kg264@sigill.intra.peff.net>

On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think the more relevant comparison is "--no-merged", and it behaves
>> > the same way as your new --no-contains. I don't think I saw this
>> > subtlety in the documentation, though. It might be worth mentioning
>> > (unless I just missed it).
>>
>> For --contains we explicitly document "contain the specified commit",
>> i.e. you couldn't expect this to list tree-test, and indeed it
>> doesn't:
>>
>>     $ git tag tree-test master^{tree}
>>     $ git tag -l --contains master '*test*'
>
> Right, "--contains" cannot have a commit inside a tree, so we were
> correct to skip the computation entirely. But does that mean that
> "--no-contains" should be the complement of that, or should it only
> include tags whose "contains" can be computed in the first place?
>
> IOW, I don't think --contains or --merged are interesting here; they
> give the right answer by skipping the computation. The question is what
> the "--no-" variants should do.

I think both should only ever find commits. I only came up with this
tree/blob scenario for the purposes of tests, but it would make the
command less useful & way slower in practice. E.g. now you want to
find what to revert to and some blob tag shows up.

>> However the --[no-]merged option says "reachable [...] from the
>> specified commit", which seems to me to be a bit more ambiguous as to
>> whether you could expect it to print tree/blob tags.
>
> I suspect that --no-merged behaves the way it does because it originally
> came from git-branch, where you only have commits in the first place.
> The other commands only learned about it during the move to ref-filter,
> and nobody thought about this corner case.
>
> So we could just treat it like a bug and fix it.  But I doubt anybody
> cares that much in practice either way, so documenting it as "any use of
> --contains, --no-contains, --no-merged, or --merged requires that the
> ref in question be a commit" is fine, too.

It's fixed in my soon-to-be resent series.