Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 20, 2017, 20:04 UTC
- Message-ID
- <CACBZZX5GErSBsiCz0Y2SgtvckcPE51ekfUgBpvYjNF1u44piMA@mail.gmail.com>
- In-Reply-To
- <20170320195209.oyivf2ta3m3kg264@sigill.intra.peff.net>
On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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.
Show 13 quoted lines
>> 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.