git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: Bug with fixup and autosquash

From
Michael J Gruber <git@drmicha.warpmail.net>
Date
Feb 9, 2017, 15:07 UTC
Message-ID
<07eb2367-9509-afb0-2494-f02a44304bc4@drmicha.warpmail.net>
In-Reply-To
<xmqqbmucuwb0.fsf@gitster.mtv.corp.google.com>
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
Previous: Ashutosh BapatNext: Junio C Hamano
Message 4 of 14 in “Bug with fixup and autosquash”
  1. Ashutosh BapatFeb 8, 2017
  2. Junio C HamanoFeb 8, 2017
  3. Ashutosh BapatFeb 9, 2017
  4. Michael J GruberFeb 9, 2017
  5. Junio C HamanoFeb 9, 2017
  6. Johannes SchindelinFeb 9, 2017
  7. Junio C HamanoFeb 9, 2017
  8. Johannes SchindelinFeb 9, 2017
  9. Junio C HamanoFeb 9, 2017
  10. Johannes SchindelinFeb 10, 2017
  11. Philip OakleyFeb 10, 2017
  12. Johannes SchindelinApr 25, 2017
  13. Philip OakleyFeb 9, 2017
  14. Johannes SchindelinFeb 10, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.