Bug with fixup and autosquash

Bug with fixup and autosquash

[Ashutosh Bapat]

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

Re: Bug with fixup and autosquash

[Junio C Hamano]

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.

Re: Bug with fixup and autosquash

[Ashutosh Bapat]

On Thu, Feb 9, 2017 at 4:25 AM, Junio C Hamano <gitster@pobox.com> 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?
>
> 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.

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

Re: Bug with fixup and autosquash

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:
> 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

Re: Bug with fixup and autosquash

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

Re: Bug with fixup and autosquash

[Johannes Schindelin]

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

Re: Bug with fixup and autosquash

[Junio C Hamano]

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

Re: Bug with fixup and autosquash

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Feb 2017, Junio C Hamano wrote:

> 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

Re: Bug with fixup and autosquash

[Junio C Hamano]

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?

Re: Bug with fixup and autosquash

[Philip Oakley]

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Sent: Thursday, February 09, 2017 8:55 PM
> 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).

> 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 

Re: Bug with fixup and autosquash

[Johannes Schindelin]

Hi Philip,

On Thu, 9 Feb 2017, Philip Oakley wrote:

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

> 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

Re: Bug with fixup and autosquash

[Johannes Schindelin]

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

Re: Bug with fixup and autosquash

[Philip Oakley]

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

Re: Bug with fixup and autosquash

[Johannes Schindelin]

Hi Philip,

On Fri, 10 Feb 2017, Philip Oakley wrote:

> 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