Re: [PATCH] tag: Implicitly supply --list given another list-like option

From: Jeff King <peff@peff.net>
Date: Mar 12, 2017, 11:19 UTC
Message-ID: <20170312111952.ryhkkrgldub26qky@sigill.intra.peff.net>
In-Reply-To: <20170311120855.16756-1-avarab@gmail.com>

On Sat, Mar 11, 2017 at 12:08:55PM +0000, Ævar Arnfjörð Bjarmason wrote:

Show 17 quoted lines

> Jeff King pointed out in
> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to
> that::
> 
>     The difference between "branch" and "tag" here is that "branch
>     --contains" implies "--list" (and the argument becomes a pattern).
>     Whereas "tag --contains" just detects the situation and complains.
> 
>     If anything, I'd think we would consider teaching "tag" to behave
>     more like "branch".
> 
> I agree. This change does that, the only tests that broke as a result
> of this were tests that were explicitly checking that this
> "branch-like" usage wasn't permitted, i.e. no actual breakages
> occurred, and I can't imagine an invocation that would negatively
> impact backwards compatibility, i.e. these invocations all just
> errored out before.

Trying to think of counter-arguments, the best I could come up with is that the situation is potentially ambiguous, and some user could be confused by us doing the wrong thing. I don't find that particularly compelling, especially as the "wrong thing" is to list tags, which is not a destructive operation.

So I think this is an improvement. As for the patch itself:

> +	/* We implicitly supply --list with --contains, --points-at,
> +	   --merged and --no-merged, just like git-branch */
> +	if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> +		cmdmode = 'l';

I was about to complain that this needs "!cmdmode", but I just looked forward in the thread and saw that Junio already covered that in more detail. I concur.

Thanks for working on this.

-Peff