On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@xxxxxxxx> 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.