Re: Fixup of a fixup not working right

["Philip Oakley" <philipoakley@iee.org>]

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi Junio & Philip,
>
> On Fri, 2 Sep 2016, Junio C Hamano wrote:
>
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>> > As I understand this it's implied by design. The issue is that the
>> > rebase is looking for that named commit within its current rebase
>> > range, and can't find it, so ignores it.
>> >
>> > There is a separate issue that all the fixup! fixup! messages are
>> > essentially treated as being concatenations of the original fixup!, no
>> > matter how many time the fiup is present.
>>
>> They can be handled separately, but they come from the same "design"
>> that could be improved.  When the "original" is not in the range to
>> be rebased for whatever reason (including the most likely one, i.e.
>> it has already graduated to become part of the public history), the
>> best thing the user could do at that point may be, as you suggested
>> to Robert in your message, to turn the "fixup! original" that did
>> not make in time before "original" hit the public record into a
>> standalone "fix original" follow-up change, and then to squash
>> subsequent "fixup! fixup! original" (and other "fixup! original",
>> too) into that commit.  And a good direction forward may be to see
>> if "rebase -i" can be taught to be more helpful for the user who
>> wants to do that.
>>
>> Perhaps a change like this to "rebase -i":
>>
>>  - The search for "original" when handling "pick fixup! original",
>>    when it does not find "original", could turn it into "reword
>>    fixup! original" without changing its position in the instruction
>>    sequence.
>>
>>  - The search for "original" when handling "pick fixup! fixup!
>>    original", could be (probably unconditionally) changed to look
>>    for "fixup! original" to amend, instead of looking for "original"
>>    as the current code (this is your "separate issue").  The same
>>    "if the commit to be amended is not found, turn it into reword"
>>    rule from the above applies to this one, too.
>>
>> may be an improvement?
>
> I would be *very* careful with such a change.

I agree about the need for care. The use case must be well understood.

> The point is that fixup! messages are really special, and are always
> intended to be squashed into the referenced commit *before* the latter
> hits `master`.

I think it's here that we have the hidden use case. I agree that all fixups 
should be squashed before they hit the blessed golden  repository.

I suspect that some use cases have intermediate repositories that contain a 
'master' branch (it's just a name ;-) that isn't blessed and golden, e.g. at 
the team review repo level. In such cases it is possible for a fixup! to be 
passed up as part of the review, though it's not the current 
norm/expectation.

>
> The entire design of the fixup! feature (using the commit subject as
> identifier, which is only "unique enough" in a topic branch that is still
> being developed) points to that.
>
> I am fairly certain that we would run into tons of problems if we diluted
> the concept of fixup! commits by changing the design so that fixup!
> commits all of a sudden become their own, "real" commits that can be fixed
> up themselves, as much of the current code simply does not expect that.

We already had that. the commit 22c5b13 (rebase -i: handle fixup! fixup! 
in --autosquash, 2013-06-27) was an attempt to work around misunderstandings 
about what fixed what.

In Robert's scenario (IIUC) that patch was too aggressive for the case where 
the original commit is not part of the rebase. The patch lept in a little 
too early in the processing so as to pretend that if it saw repeated "fixup! 
" strings at the start of the messages it pretented there was only one, so 
that they all applied in the original sequence.

I _think_ that the right approach would be to just bring such fixups 
together in the to-do list ("as normal"), but still have the minimal common 
commit message string still present, so that it would, in this case, still 
have a resulting squashed commit that starts "!fixup ". It's important to be 
moderately lenient to user choices - they may know something we don't, or at 
least accept that their use case is 'unusual' [1]

>
> In short, I am opposed to this change.

It's not like G4W doesn't need fixup!s on the side branches e.g. 5eaffe9 
("fixup! Handle new t1501 test case properly with MinGW", 2016-07-12)

> And even if I am overruled, I would strongly suggest to implement this on
> top of my rebase-i-extra branch (i.e. in the rebase--helper instead of the
> shell script) to avoid double churn.

I definitely agree there.

>
> Ciao,
> Johannes
>
--
Philip
[1] The removal of the "theirs" merge strategy is one I'd add to that list. 
Calling it, for example, "reversed" would have kept it available while 
reduced it's visibility. See http://marc.info/?l=git&m=121637513604413&w=2