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

From: Jeff King <peff@peff.net>
Date: Mar 20, 2017, 19:52 UTC
Message-ID: <20170320195209.oyivf2ta3m3kg264@sigill.intra.peff.net>
In-Reply-To: <CACBZZX6FWjG1bXrk+ee8y=T5=ovxxybfrGzkkDxjskwDzhKPuA@mail.gmail.com>

On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> > 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.

> 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.

-Peff