threads / bug / 45083

Bug with fixup and autosquash

Subject: Bug with fixup and autosquash

## tl;dr

14 messages between Feb 8, 2017 and Apr 25, 2017.

replies: 13people: 5as markdown or json

Ashutosh Bapat· Feb 8, 2017, 10:10 UTC · lore

Hi Git-developers, First of all thanks for developing this wonderful scm tool. It's sooo versatile.

I have been using git rebase heavily these days and seem to have found a bug.

If there are two commit messages which have same prefix e.g. yyyyyy This is prefix xxxxxx This is prefix and message

xxxxxx comitted before yyyyyy

Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy zzzzzz fixup! This is prefix

When I run git rebase -i --autosquash, the script it shows me looks like pick xxxxxx This is prefix and message fixup zzzzzz fixup! This is prefix pick yyyyyy This is prefix

I think the correct order is pick xxxxxx This is prefix and message pick yyyyyy This is prefix fixup zzzzzz fixup! This is prefix

Is that right?

I am using [ubuntu]git --version git version 1.7.9.5

-- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company

Junio C Hamano· Feb 8, 2017, 22:55 UTC · re: Ashutosh Bapat · lore

Re: Bug with fixup and autosquash

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
Show 22 quoted lines
> I have been using git rebase heavily these days and seem to have found a bug.
>
> If there are two commit messages which have same prefix e.g.
> yyyyyy This is prefix
> xxxxxx This is prefix and message
>
> xxxxxx comitted before yyyyyy
>
> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
> zzzzzz fixup! This is prefix
>
> When I run git rebase -i --autosquash, the script it shows me looks like
> pick xxxxxx This is prefix and message
> fixup zzzzzz fixup! This is prefix
> pick yyyyyy This is prefix
>
> I think the correct order is
> pick xxxxxx This is prefix and message
> pick yyyyyy This is prefix
> fixup zzzzzz fixup! This is prefix
>
> Is that right?

Because "commit" pretends as if it took the exact commit object name to be fixed up (after all, it accepts "yyyyyy" that is a name of the commit object), it would be nice if the fixup is applied to that exact commit, even if you had many commits that share exactly the same title (i.e. not just shared prefix).

Unfortunately, "rebase -i --autosquash" reorders the entries by identifying the commit by its title, and it goes with prefix match so that fix-up commits created without using --fixup option but manually records the title's prefix substring can also work.

We could argue that the logic should notice that there is one exact match and another non-exact prefix match and favor the former, and certainly such a change would make your made-up example (you didn't actually have a commit whose title is literally "This is prefix") above work better.

But I am not sure if adding such heuristics would really help in general. It would not help those whose commits are mostly titled ultra-vaguely, like "fix", "bugfix", "docfix", etc.

Another possibility is to teach "commit --fixup" to cast exact commit object name in stone. That certainly would solve your immediate problem, but it has a grave negative impact when the user rebases the same topic many times (which happens often).

For example, I may have a series of commits A and B, notice that A could be done a bit better and have "fixup A" on top, build a new commit C on it, and then realize that the next step (i.e. D) would need support from a newer codebase than where I started (i.e. A^).

At that point, I would have a 4-commit series (A, B, "fixup A", and C), and I would rebase them on top of something newer. I am undecided if that "fixup A" is really an improvement or unnecessary, when I do this, but I do know that I want to build the series on top of a different commit. So I do rebase without --autosquash (I would probably rebase without --interactive for this one).

Then I keep working and add a new commit D on top. After all that, I would have a more-or-less completed series and would be ready to re-assess the whole series. I perhaps decide that "fixup A" is really an improvement. And then I would "rebase -i" to squash the fix-up into A.

But notice that at this point, what we are calling A has different object name than the original A the fixup was written for, because we rebased once on top of a newer codebase. That commit can still be identified by its title, but not with its original commit object name.

I think that is why "commit --fixup <commit>" turns the commit object name (your "yyyyyy") into a string (your "This is prefix") and that is a reasonable design decision [*1*].

So from that point of view, if we were to address your issue, it should happen in "rebase -i --autosquash" side, not "commit --fixup" side.

Let's hear from some of those (Cc'ed) who were involved in an earlier --autosquash thread.

https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/
[Footnote]
*1* "rebase -i --autosquash" does understand "fixup! yyyyyy", so if
    you are willing to accept the consequence of not being able to
    rebase twice, you could instead do
	$ git commit -m "fixup! yyyyyy"
    I would think.
Ashutosh Bapat· Feb 9, 2017, 04:06 UTC · re: Junio C Hamano · lore

Re: Bug with fixup and autosquash

On Thu, Feb 9, 2017 at 4:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>
>> I have been using git rebase heavily these days and seem to have found a bug.
>>
>> If there are two commit messages which have same prefix e.g.
>> yyyyyy This is prefix
>> xxxxxx This is prefix and message
>>
>> xxxxxx comitted before yyyyyy
>>
>> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
>> zzzzzz fixup! This is prefix
>>
>> When I run git rebase -i --autosquash, the script it shows me looks like
>> pick xxxxxx This is prefix and message
>> fixup zzzzzz fixup! This is prefix
>> pick yyyyyy This is prefix
>>
>> I think the correct order is
>> pick xxxxxx This is prefix and message
>> pick yyyyyy This is prefix
>> fixup zzzzzz fixup! This is prefix
>>
>> Is that right?
>
> Because "commit" pretends as if it took the exact commit object name
> to be fixed up (after all, it accepts "yyyyyy" that is a name of the
> commit object), it would be nice if the fixup is applied to that
> exact commit, even if you had many commits that share exactly the
> same title (i.e. not just shared prefix).
>
> Unfortunately, "rebase -i --autosquash" reorders the entries by
> identifying the commit by its title, and it goes with prefix match
> so that fix-up commits created without using --fixup option but
> manually records the title's prefix substring can also work.
>
> We could argue that the logic should notice that there is one exact
> match and another non-exact prefix match and favor the former, and
> certainly such a change would make your made-up example (you didn't
> actually have a commit whose title is literally "This is prefix")
> above work better.

This is a "real life situation" I ended up with yesterday. I can't share exact commit message here so redacted it, keeping its essence. I executed git rebase -i --autosquash and got into a conflict merge since the fixup was applied after wrong commit. I had to execute git rebase --abort and git rebase -i --autosquash. That's when I noticed what's gone wrong.

Show 41 quoted lines
>
> But I am not sure if adding such heuristics would really help in
> general.  It would not help those whose commits are mostly titled
> ultra-vaguely, like "fix", "bugfix", "docfix", etc.
>
> Another possibility is to teach "commit --fixup" to cast exact
> commit object name in stone.  That certainly would solve your
> immediate problem, but it has a grave negative impact when the user
> rebases the same topic many times (which happens often).
>
> For example, I may have a series of commits A and B, notice that A
> could be done a bit better and have "fixup A" on top, build a new
> commit C on it, and then realize that the next step (i.e. D) would
> need support from a newer codebase than where I started (i.e. A^).
>
> At that point, I would have a 4-commit series (A, B, "fixup A", and
> C), and I would rebase them on top of something newer.  I am
> undecided if that "fixup A" is really an improvement or unnecessary,
> when I do this, but I do know that I want to build the series on top
> of a different commit.  So I do rebase without --autosquash (I would
> probably rebase without --interactive for this one).
>
> Then I keep working and add a new commit D on top.  After all that,
> I would have a more-or-less completed series and would be ready to
> re-assess the whole series.  I perhaps decide that "fixup A" is
> really an improvement.  And then I would "rebase -i" to squash the
> fix-up into A.
>
> But notice that at this point, what we are calling A has different
> object name than the original A the fixup was written for, because
> we rebased once on top of a newer codebase.  That commit can still
> be identified by its title, but not with its original commit object
> name.
>
> I think that is why "commit --fixup <commit>" turns the commit
> object name (your "yyyyyy") into a string (your "This is prefix")
> and that is a reasonable design decision [*1*].
>
> So from that point of view, if we were to address your issue, it
> should happen in "rebase -i --autosquash" side, not "commit --fixup"
> side.
I agree with this analysis.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Michael J Gruber· Feb 9, 2017, 15:07 UTC · re: Junio C Hamano · lore

Re: Bug with fixup and autosquash

Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:
Show 98 quoted lines
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> 
>> I have been using git rebase heavily these days and seem to have found a bug.
>>
>> If there are two commit messages which have same prefix e.g.
>> yyyyyy This is prefix
>> xxxxxx This is prefix and message
>>
>> xxxxxx comitted before yyyyyy
>>
>> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
>> zzzzzz fixup! This is prefix
>>
>> When I run git rebase -i --autosquash, the script it shows me looks like
>> pick xxxxxx This is prefix and message
>> fixup zzzzzz fixup! This is prefix
>> pick yyyyyy This is prefix
>>
>> I think the correct order is
>> pick xxxxxx This is prefix and message
>> pick yyyyyy This is prefix
>> fixup zzzzzz fixup! This is prefix
>>
>> Is that right?
> 
> Because "commit" pretends as if it took the exact commit object name
> to be fixed up (after all, it accepts "yyyyyy" that is a name of the
> commit object), it would be nice if the fixup is applied to that
> exact commit, even if you had many commits that share exactly the
> same title (i.e. not just shared prefix).
> 
> Unfortunately, "rebase -i --autosquash" reorders the entries by
> identifying the commit by its title, and it goes with prefix match
> so that fix-up commits created without using --fixup option but
> manually records the title's prefix substring can also work.  
> 
> We could argue that the logic should notice that there is one exact
> match and another non-exact prefix match and favor the former, and
> certainly such a change would make your made-up example (you didn't
> actually have a commit whose title is literally "This is prefix")
> above work better.
> 
> But I am not sure if adding such heuristics would really help in
> general.  It would not help those whose commits are mostly titled
> ultra-vaguely, like "fix", "bugfix", "docfix", etc.
> 
> Another possibility is to teach "commit --fixup" to cast exact
> commit object name in stone.  That certainly would solve your
> immediate problem, but it has a grave negative impact when the user
> rebases the same topic many times (which happens often).
> 
> For example, I may have a series of commits A and B, notice that A
> could be done a bit better and have "fixup A" on top, build a new
> commit C on it, and then realize that the next step (i.e. D) would
> need support from a newer codebase than where I started (i.e. A^).
> 
> At that point, I would have a 4-commit series (A, B, "fixup A", and
> C), and I would rebase them on top of something newer.  I am
> undecided if that "fixup A" is really an improvement or unnecessary,
> when I do this, but I do know that I want to build the series on top
> of a different commit.  So I do rebase without --autosquash (I would
> probably rebase without --interactive for this one).
> 
> Then I keep working and add a new commit D on top.  After all that,
> I would have a more-or-less completed series and would be ready to
> re-assess the whole series.  I perhaps decide that "fixup A" is
> really an improvement.  And then I would "rebase -i" to squash the
> fix-up into A.
> 
> But notice that at this point, what we are calling A has different
> object name than the original A the fixup was written for, because
> we rebased once on top of a newer codebase.  That commit can still
> be identified by its title, but not with its original commit object
> name.
> 
> I think that is why "commit --fixup <commit>" turns the commit
> object name (your "yyyyyy") into a string (your "This is prefix")
> and that is a reasonable design decision [*1*].
> 
> So from that point of view, if we were to address your issue, it
> should happen in "rebase -i --autosquash" side, not "commit --fixup"
> side.
> 
> Let's hear from some of those (Cc'ed) who were involved in an
> earlier --autosquash thread.
> 
> https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/
> 
> 
> [Footnote]
> 
> *1* "rebase -i --autosquash" does understand "fixup! yyyyyy", so if
>     you are willing to accept the consequence of not being able to
>     rebase twice, you could instead do
> 
> 	$ git commit -m "fixup! yyyyyy"
> 
>     I would think.
Doesn't this indicate that rebase is fine as is? How about:
- introduce "git commit --fixup-fixed=<rev>" or the like which parses
<rev> commits "-m fixup! <sha1>"
- teach "git commit --fixup=<rev>" to check for duplicates (same prefix,
maybe only in "recent" history) and make it issue a warning, say:

prefix <prefix> matches the following commits: <sha1> <subject> Issue git commit --amend -m 'fixup! <sha1>' to fixup a specific commit.

(Or git commit --amend --fixup-fixed=<rev> if that flies.)

Additionally, we could teach commit --fixup-fixed to commit -m "fixup! <sha1> <prefix>" so that we have both uniqueness and verbosity in the rebase-i-editor. This would allow "rebase -i" to fall back to the old mode when "<sha1>" is not in the range it operates on. We could also try to rewrite <sha1>s when rebasing (without squashing) fixup!-commits, of course.

Michael
Junio C Hamano· Feb 9, 2017, 19:46 UTC · re: Michael J Gruber · lore

Re: Bug with fixup and autosquash

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 19 quoted lines
> Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:
>
>> Let's hear from some of those (Cc'ed) who were involved in an
>> earlier --autosquash thread.
>> 
>> https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/
>> 
>> 
>> [Footnote]
>> 
>> *1* "rebase -i --autosquash" does understand "fixup! yyyyyy", so if
>>     you are willing to accept the consequence of not being able to
>>     rebase twice, you could instead do
>> 
>> 	$ git commit -m "fixup! yyyyyy"
>> 
>>     I would think.
>
> Doesn't this indicate that rebase is fine as is?

Not really, unless you ignore "if you are willing to accept" part, which actually is a big downside. And --fixup-fixed will make it worse, unfortunately.

> - teach "git commit --fixup=<rev>" to check for duplicates (same prefix,
> maybe only in "recent" history) and make it issue a warning, say:

This is a very good idea worth pursuing, and (I didn't think things through, though) we may even be able to bound "recent" without heuristics---scanning between <rev> and the tip for duplicates might be sufficient.

> Additionally, we could teach commit --fixup-fixed to commit -m "fixup!
> <sha1> <prefix>" so that we have both uniqueness and verbosity in the
> rebase-i-editor. This would allow "rebase -i" to fall back to the old
> mode when "<sha1>" is not in the range it operates on.

This is also a possibility, but it needs cooperation between both "commit" and "rebase -i".

I personally do not think rewriting "fixup! yyyyyy" on the title during rebase is worth doing, but that is not because I have a concrete reason against it but it just sounds like too much magic to my gut feeling. Perhaps it can be done reliably with minimal change to the code. I dunno.

Thanks.
Johannes Schindelin· Feb 9, 2017, 20:55 UTC · re: Junio C Hamano · lore

Re: Bug with fixup and autosquash

Hi Ashutosh and Junio,
On Wed, 8 Feb 2017, Junio C Hamano wrote:
Show 32 quoted lines
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> 
> > I have been using git rebase heavily these days and seem to have found
> > a bug.
> >
> > If there are two commit messages which have same prefix e.g.
> > yyyyyy This is prefix
> > xxxxxx This is prefix and message
> >
> > xxxxxx comitted before yyyyyy
> >
> > Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
> > zzzzzz fixup! This is prefix
> >
> > When I run git rebase -i --autosquash, the script it shows me looks like
> > pick xxxxxx This is prefix and message
> > fixup zzzzzz fixup! This is prefix
> > pick yyyyyy This is prefix
> >
> > I think the correct order is
> > pick xxxxxx This is prefix and message
> > pick yyyyyy This is prefix
> > fixup zzzzzz fixup! This is prefix
> >
> > Is that right?
> 
> [...]
> 
> Unfortunately, "rebase -i --autosquash" reorders the entries by
> identifying the commit by its title, and it goes with prefix match so
> that fix-up commits created without using --fixup option but manually
> records the title's prefix substring can also work.  

This prefix match also happens to introduce a serious performance problem, which is why I "fixed" this issue in the rebase--helper already (which is the case if you are using Git for Windows, whose master branch builds on Linux and MacOSX as well). I quoted "fix" because my motivation was to fix the performance problem, not the "incorrect match" problem.

The rebase--helper code (specifically, the patch moving autosquash logic into it: https://github.com/dscho/git/commit/7d0831637f) tries to match exact onelines first, and falls back to prefix matching only after that.

Now that the sequencer-i patch series is in `master`, the next step is to send the patch series introducing the rebase--helper. The patch series including the fix discussed above relies on that one. Meaning that it will take a while to get through the mill.

So please do not hold your breath until this feature/fix hits an official Git version. If you need it[*1*] faster, feel free to build Git for Windows' master and run with that for a while.

Ciao, Johannes

Footnote: By "it" I mean "the feature/fix", not "an official Git version"
nor "your breath".
Junio C Hamano· Feb 9, 2017, 21:21 UTC · re: Johannes Schindelin · lore

Re: Bug with fixup and autosquash

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
> This prefix match also happens to introduce a serious performance problem,
> which is why I "fixed" this issue in the rebase--helper already (which is
> the case if you are using Git for Windows, whose master branch builds on
> Linux and MacOSX as well). I quoted "fix" because my motivation was to fix
> the performance problem, not the "incorrect match" problem.

In other words, regardless of your motivation, you "fix"ed both, which is very nice ;-)

Johannes Schindelin· Feb 9, 2017, 22:02 UTC · re: Junio C Hamano · lore

Re: Bug with fixup and autosquash

Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > This prefix match also happens to introduce a serious performance problem,
> > which is why I "fixed" this issue in the rebase--helper already (which is
> > the case if you are using Git for Windows, whose master branch builds on
> > Linux and MacOSX as well). I quoted "fix" because my motivation was to fix
> > the performance problem, not the "incorrect match" problem.
> 
> In other words, regardless of your motivation, you "fix"ed both,
> which is very nice ;-)

Almost. While I fixed the performance issues as well as the design allowed, I happened to "fix" the problem where an incomplete prefix match could be favored over an exact match.

I really fixed the performance issues. Not "fixed" them.

Ciao, Johannes

Junio C Hamano· Feb 9, 2017, 22:16 UTC · re: Johannes Schindelin · lore

Re: Bug with fixup and autosquash

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Almost. While I fixed the performance issues as well as the design
> allowed, I happened to "fix" the problem where an incomplete prefix match
> could be favored over an exact match.

Hmph. Would it require too much further work to do what you said the code does:

> The rebase--helper code (specifically, the patch moving autosquash logic
> into it: https://github.com/dscho/git/commit/7d0831637f) tries to match
> exact onelines first, and falls back to prefix matching only after that.

If the code matches exact onlines and then falls back to prefix, I do not think incomplete prefix would be mistakenly chosen over an exact one, so perhaps your code already does the right thing?

Johannes Schindelin· Feb 10, 2017, 15:57 UTC · re: Junio C Hamano · lore

Re: Bug with fixup and autosquash

Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Almost. While I fixed the performance issues as well as the design
> > allowed, I happened to "fix" the problem where an incomplete prefix
> > match could be favored over an exact match.
> 
> Hmph.  Would it require too much further work to do what you said the
> code does:

I was just being overly precise. I *did* fix the problem. But since it was not my intention, I quoted the verb "fix".

Show 8 quoted lines
> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first, and falls back to prefix matching only
> > after that.
> 
> If the code matches exact onlines and then falls back to prefix, I do
> not think incomplete prefix would be mistakenly chosen over an exact
> one, so perhaps your code already does the right thing?

The code does exactly that. It does even more: as `fixup! <SHA-1>` is allowed (for SHA-1s that have been mentioned in previous `pick` lines), it tries to match that before falling back to the incomplete prefix match.

Ciao, Johannes

Philip Oakley· Feb 10, 2017, 19:02 UTC · re: Johannes Schindelin · lore

Re: Bug with fixup and autosquash

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Show 31 quoted lines
> Hi Junio,
>
> On Thu, 9 Feb 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Almost. While I fixed the performance issues as well as the design
>> > allowed, I happened to "fix" the problem where an incomplete prefix
>> > match could be favored over an exact match.
>>
>> Hmph.  Would it require too much further work to do what you said the
>> code does:
>
> I was just being overly precise. I *did* fix the problem. But since it was
> not my intention, I quoted the verb "fix".
>
>> > The rebase--helper code (specifically, the patch moving autosquash
>> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
>> > to match exact onelines first, and falls back to prefix matching only
>> > after that.
>>
>> If the code matches exact onlines and then falls back to prefix, I do
>> not think incomplete prefix would be mistakenly chosen over an exact
>> one, so perhaps your code already does the right thing?
>
> The code does exactly that. It does even more: as `fixup! <SHA-1>` is
> allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
> tries to match that before falling back to the incomplete prefix match.
>
> Ciao,
> Johannes
Now just the doc update to do.... ;-)

I definitely think the 'fix' that allows the `fixup! <SHA-1>` as the subject line is a good way to go for those who mix in the use of the gui and gitk into their workflow (*)

-- Philip (*) I just don't see the point of having multiple cli tty windows, and then not using the gui/k windows that are part of the tool set.

Johannes Schindelin· Apr 25, 2017, 11:30 UTC · re: Philip Oakley · lore

Re: Bug with fixup and autosquash

Hi Philip,
On Fri, 10 Feb 2017, Philip Oakley wrote:
Show 30 quoted lines
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> >
> > On Thu, 9 Feb 2017, Junio C Hamano wrote:
> >
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > > > Almost. While I fixed the performance issues as well as the design
> > > > allowed, I happened to "fix" the problem where an incomplete
> > > > prefix match could be favored over an exact match.
> > >
> > > Hmph.  Would it require too much further work to do what you said
> > > the code does:
> >
> > I was just being overly precise. I *did* fix the problem. But since it
> > was not my intention, I quoted the verb "fix".
> >
> > > > The rebase--helper code (specifically, the patch moving autosquash
> > > > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > > > to match exact onelines first, and falls back to prefix matching only
> > > > after that.
> > >
> > > If the code matches exact onlines and then falls back to prefix, I do
> > > not think incomplete prefix would be mistakenly chosen over an exact
> > > one, so perhaps your code already does the right thing?
> >
> > The code does exactly that. It does even more: as `fixup! <SHA-1>` is
> > allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
> > tries to match that before falling back to the incomplete prefix match.
> 
> Now just the doc update to do.... ;-)

Right. I finally managed to work that in, and will send out v2 (is it only the second iteration? ... wow...) as soon as the test suite passes on Windows (which will take a bit over an hour, as you know).

Ciao, Dscho

Philip Oakley· Feb 9, 2017, 23:34 UTC · re: Johannes Schindelin · lore

Re: Bug with fixup and autosquash

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Sent: Thursday, February 09, 2017 8:55 PM
Show 47 quoted lines
> Hi Ashutosh and Junio,
>
> On Wed, 8 Feb 2017, Junio C Hamano wrote:
>
>> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>
>> > I have been using git rebase heavily these days and seem to have found
>> > a bug.
>> >
>> > If there are two commit messages which have same prefix e.g.
>> > yyyyyy This is prefix
>> > xxxxxx This is prefix and message
>> >
>> > xxxxxx comitted before yyyyyy
>> >
>> > Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy
>> > zzzzzz fixup! This is prefix
>> >
>> > When I run git rebase -i --autosquash, the script it shows me looks 
>> > like
>> > pick xxxxxx This is prefix and message
>> > fixup zzzzzz fixup! This is prefix
>> > pick yyyyyy This is prefix
>> >
>> > I think the correct order is
>> > pick xxxxxx This is prefix and message
>> > pick yyyyyy This is prefix
>> > fixup zzzzzz fixup! This is prefix
>> >
>> > Is that right?
>>
>> [...]
>>
>> Unfortunately, "rebase -i --autosquash" reorders the entries by
>> identifying the commit by its title, and it goes with prefix match so
>> that fix-up commits created without using --fixup option but manually
>> records the title's prefix substring can also work.
>
> This prefix match also happens to introduce a serious performance problem,
> which is why I "fixed" this issue in the rebase--helper already (which is
> the case if you are using Git for Windows, whose master branch builds on
> Linux and MacOSX as well). I quoted "fix" because my motivation was to fix
> the performance problem, not the "incorrect match" problem.
>
> The rebase--helper code (specifically, the patch moving autosquash logic
> into it: https://github.com/dscho/git/commit/7d0831637f) tries to match
> exact onelines first,

While I think this is an improvement, and will strongly support the `git commit --fixup=<commit>` option which will, if the sha1/oid is given, create the exact commit subject line.

However it would also be useful if the actual commit subject line could have a similar format option, so that those who use say the git gui (rather than the cli) for the commit message, could easily create the `!fixup <commit>` message which would allow a broader range of ways of spelling the commit (e.g. giving a sha1(min length) that is within the rebase todo list).

Show 17 quoted lines
> and falls back to prefix matching only after that.
>
> Now that the sequencer-i patch series is in `master`, the next step is to
> send the patch series introducing the rebase--helper. The patch series
> including the fix discussed above relies on that one. Meaning that it will
> take a while to get through the mill.
>
> So please do not hold your breath until this feature/fix hits an official
> Git version. If you need it[*1*] faster, feel free to build Git for
> Windows' master and run with that for a while.
>
> Ciao,
> Johannes
>
> Footnote: By "it" I mean "the feature/fix", not "an official Git version"
> nor "your breath".
>

-- Philip

Johannes Schindelin· Feb 10, 2017, 14:02 UTC · re: Philip Oakley · lore

Re: Bug with fixup and autosquash

Hi Philip,
On Thu, 9 Feb 2017, Philip Oakley wrote:
Show 10 quoted lines
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Sent: Thursday, February 09, 2017 8:55 PM
>
> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first,
> 
> While I think this is an improvement, and will strongly support the `git
> commit --fixup=<commit>` option which will, if the sha1/oid is given,
> create the exact commit subject line.

That is already the case (with the exception that it is not the "exact commit subject line" but the oneline, i.e. unwrapped first paragraph).

Show 6 quoted lines
> However it would also be useful if the actual commit subject line could
> have a similar format option, so that those who use say the git gui
> (rather than the cli) for the commit message, could easily create the
> `!fixup <commit>` message which would allow a broader range of ways of
> spelling the commit (e.g. giving a sha1(min length) that is within the
> rebase todo list).

It is already the case that `fixup! <sha1>` is accepted, see the code replaced by above-mentioned commit:

https://github.com/dscho/git/commit/7d0831637f#diff-0f15aff45d5dd346465c35597a5f274eL780
... and its replacement code in C:
https://github.com/dscho/git/commit/7d0831637f#diff-79231f0693f84f3951daeea17065aad9R2800

Note that both preimage and postimage code try to match onelines first, with the new code changing behavior ever so slightly: it tries to match the exact oneline first, then a commit SHA-1 of an already-seen `pick` line, and only then falls back to the (expensive) prefix match.

Ciao, Dscho

← back to recent threads