Re: [PATCH 2/2] git-rebase: error out when incompatible options passed

[Phillip Wood <phillip.wood@talktalk.net>]

On 11/06/18 16:49, Elijah Newren wrote:
> Another thing I missed...
> 
> On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> On 07/06/18 06:06, Elijah Newren wrote:
> 
>>> Some exceptions I left out:
>>>
>>>    * --merge and --interactive are technically incompatible since they are
>>>      supposed to run different underlying scripts, but with a few small
>>>      changes, --interactive can do everything that --merge can.  In fact,
>>>      I'll shortly be sending another patch to remove git-rebase--merge and
>>>      reimplement it on top of git-rebase--interactive.
>>
>> Excellent I've wondered about doing that but never got round to it. One
>> thing I was slightly concerned about was that someone maybe parsing the
>> output of git-rebase--merge and they'll get a nasty shock when that output
>> changes as a result of using the sequencer.
> 
> I can see the minor worry, but I think upon inspection it's not
> something that concerns me, for a few reasons:
> 
> In terms of use, given that rebase --merge was introduced to handle
> renames in mid-2006, but plain rebase has been able to handle them
> since late 2008 (though directory renames changes that again), the
> utility of rebase --merge has been somewhat limited.  I think that
> limits the exposure.  But to address the 'break' more directly...
> 
> If we were to agree that we needed to support folks parsing rebase
> output, that would be a really strict requirement that I think would
> prevent lots of fixes.  And if so, it's one we've violated a number of
> times.  For example, I certainly wasn't thinking about rebase when I
> modified messages in merge-recursive.c over the years, but they'd leak
> through for rebase --merge.  (Those messages would not leak through to
> rebase --interactive as much, since the sequencer sets o.buffer_output
> and then only conditionally shows the output.)  Also, changes that
> have occurred in the past, like adding 'git gc --auto' to rebase,
> modifying error messages directly found in git-rebase--merge.sh would
> have been considered breaks.
> 
> Finally, looking over all the differences in output while fixing up
> testcases makes me think we've done much less around designing the
> output based on what we want the user to see, and more around what
> minimal fixups can we do to these lower level commands that provide
> useful functionality to the user?  We linearize history differently
> for different rebase modes, have different entries in the reflog
> depending on which mode, and we often times implement features for
> just one mode and then sometimes add it to others.  In fact, I think
> the primary reason that am-based and merge-based rebases had a --quiet
> option and the interactive rebases didn't, is mostly attributable to
> the defaults of the lower level commands these three were built on top
> of (git-am vs. git-merge-recursive vs. git-cherry-pick).  The noiser
> two merited a quiet option, and the option was never added for the
> last one.
> 
> Anyway, that's my rationale.  I'm curious to hear if folks disagree or
> see things I'm overlooking or have reasons I might be weighting
> tradeoffs less than optimally.
> 

I agree that there are already plenty of inconsistencies, (it's great to
see you reducing them). If we can avoid emulating the ouput of
git-rebase--merge.sh in sequencer.c that would definitely be my
preferred option (the code is already a bit hard to follow in places
where there it's doing slightly different things for cherry-pick and
rebase -i). Hopefully no one is relying on the output, as you say it's
just whatever the underlying plumbing prints rather than designed for a
specific purpose.

Best Wishes

Phillip