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

On 11/06/18 16:49, Elijah Newren wrote:

Another thing I missed...

On Sun, Jun 10, 2018 at 12:40 PM, Phillip Wood
[off-list ref] wrote:

quoted

On 07/06/18 06:06, Elijah Newren wrote:

quoted

quoted

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