Fixup of a fixup not working right

Suppose I have a branch with 4 commits, in the following order (as you might see during interactive rebase):

pick 123 Original Change pick 789 fixup! Original Change pick 456 Some Other Thing pick abc fixup! fixup! Original Change

However, let's say the first commit is already pushed upstream on a topic branch. Since there are multiple developers on this topic branch, I do not want to rebase right now. Instead, I want to document future fixups via fixup commits and then when we're ready to merge, do a final rebase prior to the merge to master to clean things up after we're all done collaborating.

For this specific situation, since the first commit is already pushed, I want to perform a fixup on the 1st fixup commit. When I perform an interactive rebase against upstream topic, I get the following:

pick 789 fixup! Original Change pick 456 Some Other Thing pick abc fixup! fixup! Original Change

The tip commit (abc in this case) is not marked as a fixup. What I expect to see is:

pick 789 fixup! Original Change fixup abc fixup! fixup! Original Change pick 456 Some Other Thing

Is this by design, or a defect? I assumed that Git would only look at the first occurrence of "fixup!" and treat everything else after as the commit description to match. But it seems in this case that it stops at the last occurrence of "fixup!", which would explain why it isn't matching in the interactive rebase. I haven't looked at the code, though.

Thoughts? Also I'm perfectly willing to accept feedback involving me just using the feature wrong or as not intended. Thanks in advance.

From: "Robert Dailey" <rcdailey.lists@gmail.com>

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.

In the mean time you should reword those commit messages as being 'bug-fixes' as they are (you say) already upstream and hence published. You can make the first as a bug-fix and the following ones a fixup!s.

"Philip Oakley" <philipoakley@iee.org> writes:

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?

On Fri, Sep 2, 2016 at 9:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

If it did this, then later when I'm ready to merge and all contributions upstream are completed, I would most likely lose track of which original commit to fixup to. This would only be an issue if you gave the option to reword the "fixup! original". Rewording it, to me, inherently means you no longer want to meld it into another commit and instead intend on it being independent (looking at this semantically, without relying on translating the commit message itself, even though it may clearly tell me to squash it to something else, that's not as reliable as the fixup! mechanics IMHO).

So this is mostly for my education, since I don't see a difference from a user-standpoint. Why would "fixup! fixup! original" look for "original" instead of "fixup! original"? As far as I can see, the behavior would be the same and the order would be retained since you are essentially "chaining" the fixups together this way. This also gives you the behavior you want directly because the algorithm will naturally tie to "fixup! original" even if "original" doesn't exist, because Git would expect that "fixup! original" will automatically manage its own order, and that subsequent processed nested "fixup!" commits would not need to depend on any other commits.

Hi Robert,
From: "Robert Dailey" <rcdailey.lists@gmail.com>

[..]

This was a problem in the past.

The patch that introduced the effect is 22c5b13 (rebase -i: handle fixup! fixup! in --autosquash, 2013-06-27). At the time it was possible that the commits would be re-ordered based on the full multi-fixup message, and so, if you thought you'd corrected a poor fixup, rather correcting the original, the conceptual order would change. While the combined diffs still notionally add to the same effect, the changes to the context lines may mean they don't apply cleanly - keeping them in order makes for a clean application. The solution was to ignore multiple fixup!s at the start.

There is a question as to whether the commit you pushed upstream, is classed as 'published' and immutable, or still part of the review and modification process. At the moment the presumption, in general, is that it would be the former, so that you can't fixup the original. I don't see that changing, however Junio's suggestion that these extra fixups are 'reworded' so that they will rebase (--autosquash) locally to become one commit titled something like "fix! original", and then a final rebase that rewords that commit back to "fixup! " for pushing upstream to the 'review' process, where they can request that it be 'fixed';-)

Hi Junio & Philip,

On Fri, 2 Sep 2016, Junio C Hamano wrote:

I would be *very* careful with such a change.

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`.

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.

In short, I am opposed to this change.

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.

Ciao, Johannes

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>

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

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.

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]

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)

I definitely agree there.

-- 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

Hi Philip,

On Sun, 4 Sep 2016, Philip Oakley wrote:

In such a case (which can totally arise when criss-crossing Pull Requests on GitHub, for example, where a Pull Request's purpose may be to fix up commits in another Pull Request before the latter is merged), the most appropriate course of action is... to not reorder the fixup!s prematurely.

Yeah, well, Git for Windows' `master` branch is special, in that it is constantly rebased (as "merging rebases", to keep fast-forwardability). I would not necessarily use Git for Windows as a role model in this respect.

Ciao, Dscho

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>

We just need to be careful about that plural just there.

If it is multiple fixup!s for the same commit, then I believe they should be grouped together at the same point as the first fixup! commit (in their original order).

If they are for different commits, then they should stay in their place in the commit series (for their first occurrence, then rule 1 applies)

I note that you don't have two fixup!s for that commit

I don't see GfW as 'special', rather as being a representative of a broader realpolitik where some of the rugged individualism of open source is moderated in some way or another.

-- Philip

Hi Philip,

On Tue, 6 Sep 2016, Philip Oakley wrote:

We should they be grouped together? In the final rebase (the one that actually also includes the commit that is to be rewritten), they will be grouped together anyway.

And it is not like users cannot regroup manually if they choose to perform an intermediate rebase. In that case, the user will also choose whether she wants to simply regroup the fixups, or squash them into a single fixup, too.

Not for that one, no. But there have been cases where I had to add two or more fixups for the same commit, in preparation for the next merging rebase.

Sure, it is an example of a project that needs to solve the problem where patch series are accumulated, to be submitted to an upstream project, and so we have to keep fast-forwardability at the same time as we have to rebase.

But Git for Windows is special in the way it solves the problem. I am not aware of anybody else performing merging rebases.

Ciao, Dscho