Re: [PATCH/RFD 0/2] revision.c: --merges, --no-merges and --merges-only

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Mar 18, 2011, 07:56 UTC
Message-ID: <4D831043.1060709@drmicha.warpmail.net>
In-Reply-To: <20110317195905.GG20508@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 17.03.2011 20:59:

Show 12 quoted lines

> On Thu, Mar 17, 2011 at 12:33:56PM +0100, Michael J Gruber wrote:
> 
>> This mini series makes it so that --no-merges undoes --merges
>> and vice versa, as the user should be able to expect,
>> and that --merges-only is a separate option.
> 
> Having recently been confused by this (and frustrated at the lack of an
> equivalent to your new "--merges"), I do think the result is better.
> 
> However, this is totally changing the meaning of an option to plumbing
> like rev-list (among others). Is it worth the breakage? If so, what's
> the migration plan? Did I miss a discussion somewhere?

You missed only the "D" in RFD :)

The meaning of a plumbing option can't be changed light-heartedly, of course. OTOH, the current design is *really bad* from the ui point of view. The expectation that

"cmd --no-foo --foo" is eq. to "cmd --foo"

and

"cmd --foo --no-foo" is eq. to "cmd --no-foo"

should be valid universally. In the long run, we might even try and convert revision.c to parse_options, thereby gaining --no-foo for every --foo.

So, my RFD really consists of two things:

- provide a way to override --no-merges/no_merges
- sane naming

The first step + half of the second could be achieved by:

- provide "--merges-also" which does "no_merges = 0"
- provide "--merges-only" which does "merges_only = 1", i.e. an alias
for current "--merges"

"--merges-also --no-merges" would be "--no-merges" and vice versa. "--merges-also --merges-only" would be "--merges-only" "--merges-only --merges-also" would be "--merges-only" which all make sense sematically

"--merges-only --no-merges" would be no commits with the current implementation (just like "--merges --no-merges"), but we could catch this case easily and make it do "--no-merges".

I don't think we need to care about keeping the current behavior of "--merges --no-merges", do we?

So, no breakage here. Along with it we could issue deprecation notices in doc and relnotes for "--merges" or just make them less visible. That is, it would serve the same purpose as Junio's tri-state option.

In 2.0 or so, we could change "--merges" to be an alias for "--merges-also" rather than "--merges-only" (but don't have to).

>From the ui perspective I'm somehow not a big fan of tri-state options

but can't give hard reasons why; maybe because they force you to use option arguments. Also, the only reasonable name for a tri-state option would be "--merges". Can a tri-state have a default? Then we could default "--merges" to the current behavior, i.e. "--merges=only" (assuming we have "--merges=only/also/no").

Junio: Sorry for making you explain "D" more rather than doing it myself
upfront.

Michael