Fixup of a fixup not working right

Fixup of a fixup not working right

[Robert Dailey]

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.

Re: Fixup of a fixup not working right

[Philip Oakley]

From: "Robert Dailey" <rcdailey.lists@gmail.com>
> 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.

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.

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

Re: Fixup of a fixup not working right

[Junio C Hamano]

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

Re: Fixup of a fixup not working right

[Robert Dailey]

On Fri, Sep 2, 2016 at 9:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

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

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.

Re: Fixup of a fixup not working right

[Philip Oakley]

Hi Robert,
From: "Robert Dailey" <rcdailey.lists@gmail.com>
> On Fri, Sep 2, 2016 at 9:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
[..]
> So this is mostly for my education, since I don't see a difference
> from a user-standpoint.

This was a problem in the past.

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

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.


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

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';-) 

Re: Fixup of a fixup not working right

[Johannes Schindelin]

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.

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

Re: Fixup of a fixup not working right

[Philip Oakley]

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 

Re: Fixup of a fixup not working right

[Johannes Schindelin]

Hi Philip,

On Sun, 4 Sep 2016, Philip Oakley wrote:

> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> 
> > 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.

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.

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

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

Re: Fixup of a fixup not working right

[Philip Oakley]

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi Philip,
>
> On Sun, 4 Sep 2016, Philip Oakley wrote:
>
>> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
>>
>> > 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.
>
> 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.

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)

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

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

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

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.

> Ciao,
> Dscho
>
--
Philip 

Re: Fixup of a fixup not working right

[Johannes Schindelin]

Hi Philip,

On Tue, 6 Sep 2016, Philip Oakley wrote:

> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> >
> > On Sun, 4 Sep 2016, Philip Oakley wrote:
> >
> > > 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.
> >
> > 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.
> 
> 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).

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.

> > > > 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)
> 
> I note that you don't have two fixup!s for that commit

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.

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

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