From: "Johannes Schindelin" <Johannes.Schindelin@xxxxxx>
Hi Junio & Philip,
On Fri, 2 Sep 2016, Junio C Hamano wrote:
"Philip Oakley" <philipoakley@xxxxxxx> 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