{"thread":{"id":"45083","subject":"Bug with fixup and autosquash","startedAt":"2017-02-08T10:18:06Z","lastAt":"2017-04-25T11:30:53Z","messageCount":14,"participants":["Ashutosh Bapat","Junio C Hamano","Michael J Gruber","Johannes Schindelin","Philip Oakley"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"311048","messageId":"CAFjFpRe8zqxs4OLbCrjnuEzF=75sbBJ+HuZqek49B=O=TFHq8A@mail.gmail.com","threadId":"45083","inReplyTo":null,"subject":"Bug with fixup and autosquash","fromName":"Ashutosh Bapat","fromEmail":"ashutosh.bapat@enterprisedb.com","sentAt":"2017-02-08T10:10:07Z","receivedAt":"2017-02-08T10:18:06Z","isPatch":false,"sender":{"key":"ashutosh.bapat@enterprisedb.com","avatar":null},"body":"Hi Git-developers,\nFirst of all thanks for developing this wonderful scm tool. It's sooo versatile.\n\nI have been using git rebase heavily these days and seem to have found a bug.\n\nIf there are two commit messages which have same prefix e.g.\nyyyyyy This is prefix\nxxxxxx This is prefix and message\n\nxxxxxx comitted before yyyyyy\n\nNow I commit a fixup to yyyyyy using git commit --fixup yyyyyy\nzzzzzz fixup! This is prefix\n\nWhen I run git rebase -i --autosquash, the script it shows me looks like\npick xxxxxx This is prefix and message\nfixup zzzzzz fixup! This is prefix\npick yyyyyy This is prefix\n\nI think the correct order is\npick xxxxxx This is prefix and message\npick yyyyyy This is prefix\nfixup zzzzzz fixup! This is prefix\n\nIs that right?\n\nI am using\n[ubuntu]git --version\ngit version 1.7.9.5\n\n--\nBest Wishes,\nAshutosh Bapat\nEnterpriseDB Corporation\nThe Postgres Database Company\n"},{"id":"311114","messageId":"xmqqbmucuwb0.fsf@gitster.mtv.corp.google.com","threadId":"45083","inReplyTo":"CAFjFpRe8zqxs4OLbCrjnuEzF=75sbBJ+HuZqek49B=O=TFHq8A@mail.gmail.com","subject":"Re: Bug with fixup and autosquash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-08T22:55:47Z","receivedAt":"2017-02-08T22:56:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ashutosh Bapat writes:\n\n> I have been using git rebase heavily these days and seem to have found a bug.\n>\n> If there are two commit messages which have same prefix e.g.\n> yyyyyy This is prefix\n> xxxxxx This is prefix and message\n>\n> xxxxxx comitted before yyyyyy\n>\n> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy\n> zzzzzz fixup! This is prefix\n>\n> When I run git rebase -i --autosquash, the script it shows me looks like\n> pick xxxxxx This is prefix and message\n> fixup zzzzzz fixup! This is prefix\n> pick yyyyyy This is prefix\n>\n> I think the correct order is\n> pick xxxxxx This is prefix and message\n> pick yyyyyy This is prefix\n> fixup zzzzzz fixup! This is prefix\n>\n> Is that right?\n\nBecause \"commit\" pretends as if it took the exact commit object name\nto be fixed up (after all, it accepts \"yyyyyy\" that is a name of the\ncommit object), it would be nice if the fixup is applied to that\nexact commit, even if you had many commits that share exactly the\nsame title (i.e. not just shared prefix).\n\nUnfortunately, \"rebase -i --autosquash\" reorders the entries by\nidentifying the commit by its title, and it goes with prefix match\nso that fix-up commits created without using --fixup option but\nmanually records the title's prefix substring can also work. \n\nWe could argue that the logic should notice that there is one exact\nmatch and another non-exact prefix match and favor the former, and\ncertainly such a change would make your made-up example (you didn't\nactually have a commit whose title is literally \"This is prefix\")\nabove work better.\n\nBut I am not sure if adding such heuristics would really help in\ngeneral. It would not help those whose commits are mostly titled\nultra-vaguely, like \"fix\", \"bugfix\", \"docfix\", etc.\n\nAnother possibility is to teach \"commit --fixup\" to cast exact\ncommit object name in stone. That certainly would solve your\nimmediate problem, but it has a grave negative impact when the user\nrebases the same topic many times (which happens often).\n\nFor example, I may have a series of commits A and B, notice that A\ncould be done a bit better and have \"fixup A\" on top, build a new\ncommit C on it, and then realize that the next step (i.e. D) would\nneed support from a newer codebase than where I started (i.e. A^).\n\nAt that point, I would have a 4-commit series (A, B, \"fixup A\", and\nC), and I would rebase them on top of something newer. I am\nundecided if that \"fixup A\" is really an improvement or unnecessary,\nwhen I do this, but I do know that I want to build the series on top\nof a different commit. So I do rebase without --autosquash (I would\nprobably rebase without --interactive for this one).\n\nThen I keep working and add a new commit D on top. After all that,\nI would have a more-or-less completed series and would be ready to\nre-assess the whole series. I perhaps decide that \"fixup A\" is\nreally an improvement. And then I would \"rebase -i\" to squash the\nfix-up into A.\n\nBut notice that at this point, what we are calling A has different\nobject name than the original A the fixup was written for, because\nwe rebased once on top of a newer codebase. That commit can still\nbe identified by its title, but not with its original commit object\nname.\n\nI think that is why \"commit --fixup \" turns the commit\nobject name (your \"yyyyyy\") into a string (your \"This is prefix\")\nand that is a reasonable design decision [*1*].\n\nSo from that point of view, if we were to address your issue, it\nshould happen in \"rebase -i --autosquash\" side, not \"commit --fixup\"\nside.\n\nLet's hear from some of those (Cc'ed) who were involved in an\nearlier --autosquash thread.\n\nhttps://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/\n\n\n[Footnote]\n\n*1* \"rebase -i --autosquash\" does understand \"fixup! yyyyyy\", so if\n you are willing to accept the consequence of not being able to\n rebase twice, you could instead do\n\n\t$ git commit -m \"fixup! yyyyyy\"\n\n I would think.\n"},{"id":"311143","messageId":"CAFjFpReXnchHGFiTKU1UjjPhpCNVA1jVPbTD9s_JPB_R9xVt1Q@mail.gmail.com","threadId":"45083","inReplyTo":"xmqqbmucuwb0.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug with fixup and autosquash","fromName":"Ashutosh Bapat","fromEmail":"ashutosh.bapat@enterprisedb.com","sentAt":"2017-02-09T04:06:19Z","receivedAt":"2017-02-09T04:36:27Z","isPatch":false,"sender":{"key":"ashutosh.bapat@enterprisedb.com","avatar":null},"body":"On Thu, Feb 9, 2017 at 4:25 AM, Junio C Hamano wrote:\n> Ashutosh Bapat writes:\n>\n>> I have been using git rebase heavily these days and seem to have found a bug.\n>>\n>> If there are two commit messages which have same prefix e.g.\n>> yyyyyy This is prefix\n>> xxxxxx This is prefix and message\n>>\n>> xxxxxx comitted before yyyyyy\n>>\n>> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy\n>> zzzzzz fixup! This is prefix\n>>\n>> When I run git rebase -i --autosquash, the script it shows me looks like\n>> pick xxxxxx This is prefix and message\n>> fixup zzzzzz fixup! This is prefix\n>> pick yyyyyy This is prefix\n>>\n>> I think the correct order is\n>> pick xxxxxx This is prefix and message\n>> pick yyyyyy This is prefix\n>> fixup zzzzzz fixup! This is prefix\n>>\n>> Is that right?\n>\n> Because \"commit\" pretends as if it took the exact commit object name\n> to be fixed up (after all, it accepts \"yyyyyy\" that is a name of the\n> commit object), it would be nice if the fixup is applied to that\n> exact commit, even if you had many commits that share exactly the\n> same title (i.e. not just shared prefix).\n>\n> Unfortunately, \"rebase -i --autosquash\" reorders the entries by\n> identifying the commit by its title, and it goes with prefix match\n> so that fix-up commits created without using --fixup option but\n> manually records the title's prefix substring can also work.\n>\n> We could argue that the logic should notice that there is one exact\n> match and another non-exact prefix match and favor the former, and\n> certainly such a change would make your made-up example (you didn't\n> actually have a commit whose title is literally \"This is prefix\")\n> above work better.\n\nThis is a \"real life situation\" I ended up with yesterday. I can't\nshare exact commit message here so redacted it, keeping its essence. I\nexecuted git rebase -i --autosquash and got into a conflict merge\nsince the fixup was applied after wrong commit. I had to execute git\nrebase --abort and git rebase -i --autosquash. That's when I noticed\nwhat's gone wrong.\n\n>\n> But I am not sure if adding such heuristics would really help in\n> general. It would not help those whose commits are mostly titled\n> ultra-vaguely, like \"fix\", \"bugfix\", \"docfix\", etc.\n>\n> Another possibility is to teach \"commit --fixup\" to cast exact\n> commit object name in stone. That certainly would solve your\n> immediate problem, but it has a grave negative impact when the user\n> rebases the same topic many times (which happens often).\n>\n> For example, I may have a series of commits A and B, notice that A\n> could be done a bit better and have \"fixup A\" on top, build a new\n> commit C on it, and then realize that the next step (i.e. D) would\n> need support from a newer codebase than where I started (i.e. A^).\n>\n> At that point, I would have a 4-commit series (A, B, \"fixup A\", and\n> C), and I would rebase them on top of something newer. I am\n> undecided if that \"fixup A\" is really an improvement or unnecessary,\n> when I do this, but I do know that I want to build the series on top\n> of a different commit. So I do rebase without --autosquash (I would\n> probably rebase without --interactive for this one).\n>\n> Then I keep working and add a new commit D on top. After all that,\n> I would have a more-or-less completed series and would be ready to\n> re-assess the whole series. I perhaps decide that \"fixup A\" is\n> really an improvement. And then I would \"rebase -i\" to squash the\n> fix-up into A.\n>\n> But notice that at this point, what we are calling A has different\n> object name than the original A the fixup was written for, because\n> we rebased once on top of a newer codebase. That commit can still\n> be identified by its title, but not with its original commit object\n> name.\n>\n> I think that is why \"commit --fixup \" turns the commit\n> object name (your \"yyyyyy\") into a string (your \"This is prefix\")\n> and that is a reasonable design decision [*1*].\n>\n> So from that point of view, if we were to address your issue, it\n> should happen in \"rebase -i --autosquash\" side, not \"commit --fixup\"\n> side.\n\nI agree with this analysis.\n\n-- \nBest Wishes,\nAshutosh Bapat\nEnterpriseDB Corporation\nThe Postgres Database Company\n"},{"id":"311182","messageId":"07eb2367-9509-afb0-2494-f02a44304bc4@drmicha.warpmail.net","threadId":"45083","inReplyTo":"xmqqbmucuwb0.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug with fixup and autosquash","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2017-02-09T15:07:22Z","receivedAt":"2017-02-09T15:07:50Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:\n> Ashutosh Bapat writes:\n> \n>> I have been using git rebase heavily these days and seem to have found a bug.\n>>\n>> If there are two commit messages which have same prefix e.g.\n>> yyyyyy This is prefix\n>> xxxxxx This is prefix and message\n>>\n>> xxxxxx comitted before yyyyyy\n>>\n>> Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy\n>> zzzzzz fixup! This is prefix\n>>\n>> When I run git rebase -i --autosquash, the script it shows me looks like\n>> pick xxxxxx This is prefix and message\n>> fixup zzzzzz fixup! This is prefix\n>> pick yyyyyy This is prefix\n>>\n>> I think the correct order is\n>> pick xxxxxx This is prefix and message\n>> pick yyyyyy This is prefix\n>> fixup zzzzzz fixup! This is prefix\n>>\n>> Is that right?\n> \n> Because \"commit\" pretends as if it took the exact commit object name\n> to be fixed up (after all, it accepts \"yyyyyy\" that is a name of the\n> commit object), it would be nice if the fixup is applied to that\n> exact commit, even if you had many commits that share exactly the\n> same title (i.e. not just shared prefix).\n> \n> Unfortunately, \"rebase -i --autosquash\" reorders the entries by\n> identifying the commit by its title, and it goes with prefix match\n> so that fix-up commits created without using --fixup option but\n> manually records the title's prefix substring can also work. \n> \n> We could argue that the logic should notice that there is one exact\n> match and another non-exact prefix match and favor the former, and\n> certainly such a change would make your made-up example (you didn't\n> actually have a commit whose title is literally \"This is prefix\")\n> above work better.\n> \n> But I am not sure if adding such heuristics would really help in\n> general. It would not help those whose commits are mostly titled\n> ultra-vaguely, like \"fix\", \"bugfix\", \"docfix\", etc.\n> \n> Another possibility is to teach \"commit --fixup\" to cast exact\n> commit object name in stone. That certainly would solve your\n> immediate problem, but it has a grave negative impact when the user\n> rebases the same topic many times (which happens often).\n> \n> For example, I may have a series of commits A and B, notice that A\n> could be done a bit better and have \"fixup A\" on top, build a new\n> commit C on it, and then realize that the next step (i.e. D) would\n> need support from a newer codebase than where I started (i.e. A^).\n> \n> At that point, I would have a 4-commit series (A, B, \"fixup A\", and\n> C), and I would rebase them on top of something newer. I am\n> undecided if that \"fixup A\" is really an improvement or unnecessary,\n> when I do this, but I do know that I want to build the series on top\n> of a different commit. So I do rebase without --autosquash (I would\n> probably rebase without --interactive for this one).\n> \n> Then I keep working and add a new commit D on top. After all that,\n> I would have a more-or-less completed series and would be ready to\n> re-assess the whole series. I perhaps decide that \"fixup A\" is\n> really an improvement. And then I would \"rebase -i\" to squash the\n> fix-up into A.\n> \n> But notice that at this point, what we are calling A has different\n> object name than the original A the fixup was written for, because\n> we rebased once on top of a newer codebase. That commit can still\n> be identified by its title, but not with its original commit object\n> name.\n> \n> I think that is why \"commit --fixup \" turns the commit\n> object name (your \"yyyyyy\") into a string (your \"This is prefix\")\n> and that is a reasonable design decision [*1*].\n> \n> So from that point of view, if we were to address your issue, it\n> should happen in \"rebase -i --autosquash\" side, not \"commit --fixup\"\n> side.\n> \n> Let's hear from some of those (Cc'ed) who were involved in an\n> earlier --autosquash thread.\n> \n> https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/\n> \n> \n> [Footnote]\n> \n> *1* \"rebase -i --autosquash\" does understand \"fixup! yyyyyy\", so if\n> you are willing to accept the consequence of not being able to\n> rebase twice, you could instead do\n> \n> \t$ git commit -m \"fixup! yyyyyy\"\n> \n> I would think.\n\nDoesn't this indicate that rebase is fine as is? How about:\n\n- introduce \"git commit --fixup-fixed=\" or the like which parses\n commits \"-m fixup! \"\n\n- teach \"git commit --fixup=\" to check for duplicates (same prefix,\nmaybe only in \"recent\" history) and make it issue a warning, say:\n\nprefix matches the following commits:\n \nIssue\ngit commit --amend -m 'fixup! '\nto fixup a specific commit.\n\n(Or git commit --amend --fixup-fixed= if that flies.)\n\nAdditionally, we could teach commit --fixup-fixed to commit -m \"fixup!\n \" so that we have both uniqueness and verbosity in the\nrebase-i-editor. This would allow \"rebase -i\" to fall back to the old\nmode when \"\" is not in the range it operates on. We could also try\nto rewrite s when rebasing (without squashing) fixup!-commits, of\ncourse.\n\nMichael\n"},{"id":"311203","messageId":"xmqqpoirp2or.fsf@gitster.mtv.corp.google.com","threadId":"45083","inReplyTo":"07eb2367-9509-afb0-2494-f02a44304bc4@drmicha.warpmail.net","subject":"Re: Bug with fixup and autosquash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T19:46:44Z","receivedAt":"2017-02-09T19:47:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Junio C Hamano venit, vidit, dixit 08.02.2017 23:55:\n>\n>> Let's hear from some of those (Cc'ed) who were involved in an\n>> earlier --autosquash thread.\n>> \n>> https://public-inbox.org/git/cover.1259934977.git.mhagger@alum.mit.edu/\n>> \n>> \n>> [Footnote]\n>> \n>> *1* \"rebase -i --autosquash\" does understand \"fixup! yyyyyy\", so if\n>> you are willing to accept the consequence of not being able to\n>> rebase twice, you could instead do\n>> \n>> \t$ git commit -m \"fixup! yyyyyy\"\n>> \n>> I would think.\n>\n> Doesn't this indicate that rebase is fine as is?\n\nNot really, unless you ignore \"if you are willing to accept\" part,\nwhich actually is a big downside. And --fixup-fixed will make it\nworse, unfortunately.\n\n> - teach \"git commit --fixup=\" to check for duplicates (same prefix,\n> maybe only in \"recent\" history) and make it issue a warning, say:\n\nThis is a very good idea worth pursuing, and (I didn't think things\nthrough, though) we may even be able to bound \"recent\" without\nheuristics---scanning between and the tip for duplicates might\nbe sufficient.\n\n> Additionally, we could teach commit --fixup-fixed to commit -m \"fixup!\n> \" so that we have both uniqueness and verbosity in the\n> rebase-i-editor. This would allow \"rebase -i\" to fall back to the old\n> mode when \"\" is not in the range it operates on.\n\nThis is also a possibility, but it needs cooperation between both\n\"commit\" and \"rebase -i\".\n\nI personally do not think rewriting \"fixup! yyyyyy\" on the title\nduring rebase is worth doing, but that is not because I have a\nconcrete reason against it but it just sounds like too much magic to\nmy gut feeling. Perhaps it can be done reliably with minimal change\nto the code. I dunno.\n\nThanks.\n\n"},{"id":"311209","messageId":"alpine.DEB.2.20.1702092142020.3496@virtualbox","threadId":"45083","inReplyTo":"xmqqbmucuwb0.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug with fixup and autosquash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-09T20:55:48Z","receivedAt":"2017-02-09T20:57:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ashutosh and Junio,\n\nOn Wed, 8 Feb 2017, Junio C Hamano wrote:\n\n> Ashutosh Bapat writes:\n> \n> > I have been using git rebase heavily these days and seem to have found\n> > a bug.\n> >\n> > If there are two commit messages which have same prefix e.g.\n> > yyyyyy This is prefix\n> > xxxxxx This is prefix and message\n> >\n> > xxxxxx comitted before yyyyyy\n> >\n> > Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy\n> > zzzzzz fixup! This is prefix\n> >\n> > When I run git rebase -i --autosquash, the script it shows me looks like\n> > pick xxxxxx This is prefix and message\n> > fixup zzzzzz fixup! This is prefix\n> > pick yyyyyy This is prefix\n> >\n> > I think the correct order is\n> > pick xxxxxx This is prefix and message\n> > pick yyyyyy This is prefix\n> > fixup zzzzzz fixup! This is prefix\n> >\n> > Is that right?\n> \n> [...]\n> \n> Unfortunately, \"rebase -i --autosquash\" reorders the entries by\n> identifying the commit by its title, and it goes with prefix match so\n> that fix-up commits created without using --fixup option but manually\n> records the title's prefix substring can also work. \n\nThis prefix match also happens to introduce a serious performance problem,\nwhich is why I \"fixed\" this issue in the rebase--helper already (which is\nthe case if you are using Git for Windows, whose master branch builds on\nLinux and MacOSX as well). I quoted \"fix\" because my motivation was to fix\nthe performance problem, not the \"incorrect match\" problem.\n\nThe rebase--helper code (specifically, the patch moving autosquash logic\ninto it: https://github.com/dscho/git/commit/7d0831637f) tries to match\nexact onelines first, and falls back to prefix matching only after that.\n\nNow that the sequencer-i patch series is in `master`, the next step is to\nsend the patch series introducing the rebase--helper. The patch series\nincluding the fix discussed above relies on that one. Meaning that it will\ntake a while to get through the mill.\n\nSo please do not hold your breath until this feature/fix hits an official\nGit version. If you need it[*1*] faster, feel free to build Git for\nWindows' master and run with that for a while.\n\nCiao,\nJohannes\n\nFootnote: By \"it\" I mean \"the feature/fix\", not \"an official Git version\"\nnor \"your breath\".\n"},{"id":"311220","messageId":"xmqqwpcznjqi.fsf@gitster.mtv.corp.google.com","threadId":"45083","inReplyTo":"alpine.DEB.2.20.1702092142020.3496@virtualbox","subject":"Re: Bug with fixup and autosquash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:21:25Z","receivedAt":"2017-02-09T21:54:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This prefix match also happens to introduce a serious performance problem,\n> which is why I \"fixed\" this issue in the rebase--helper already (which is\n> the case if you are using Git for Windows, whose master branch builds on\n> Linux and MacOSX as well). I quoted \"fix\" because my motivation was to fix\n> the performance problem, not the \"incorrect match\" problem.\n\nIn other words, regardless of your motivation, you \"fix\"ed both,\nwhich is very nice ;-)\n"},{"id":"311223","messageId":"alpine.DEB.2.20.1702092301070.3496@virtualbox","threadId":"45083","inReplyTo":"xmqqwpcznjqi.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug with fixup and autosquash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-09T22:02:51Z","receivedAt":"2017-02-09T22:12:21Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 9 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > This prefix match also happens to introduce a serious performance problem,\n> > which is why I \"fixed\" this issue in the rebase--helper already (which is\n> > the case if you are using Git for Windows, whose master branch builds on\n> > Linux and MacOSX as well). I quoted \"fix\" because my motivation was to fix\n> > the performance problem, not the \"incorrect match\" problem.\n> \n> In other words, regardless of your motivation, you \"fix\"ed both,\n> which is very nice ;-)\n\nAlmost. While I fixed the performance issues as well as the design\nallowed, I happened to \"fix\" the problem where an incomplete prefix match\ncould be favored over an exact match.\n\nI really fixed the performance issues. Not \"fixed\" them.\n\nCiao,\nJohannes\n"},{"id":"311227","messageId":"xmqqd1ernh7g.fsf@gitster.mtv.corp.google.com","threadId":"45083","inReplyTo":"alpine.DEB.2.20.1702092301070.3496@virtualbox","subject":"Re: Bug with fixup and autosquash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T22:16:03Z","receivedAt":"2017-02-09T22:24:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Almost. While I fixed the performance issues as well as the design\n> allowed, I happened to \"fix\" the problem where an incomplete prefix match\n> could be favored over an exact match.\n\nHmph. Would it require too much further work to do what you said\nthe code does:\n\n> The rebase--helper code (specifically, the patch moving autosquash logic\n> into it: https://github.com/dscho/git/commit/7d0831637f) tries to match\n> exact onelines first, and falls back to prefix matching only after that.\n\nIf the code matches exact onlines and then falls back to prefix, I\ndo not think incomplete prefix would be mistakenly chosen over an\nexact one, so perhaps your code already does the right thing?\n"},{"id":"311237","messageId":"454E7D934160418EB4C4871ED209CBCA@PhilipOakley","threadId":"45083","inReplyTo":"alpine.DEB.2.20.1702092142020.3496@virtualbox","subject":"Re: Bug with fixup and autosquash","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-02-09T23:34:05Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Johannes Schindelin\" \nSent: Thursday, February 09, 2017 8:55 PM\n> Hi Ashutosh and Junio,\n>\n> On Wed, 8 Feb 2017, Junio C Hamano wrote:\n>\n>> Ashutosh Bapat writes:\n>>\n>> > I have been using git rebase heavily these days and seem to have found\n>> > a bug.\n>> >\n>> > If there are two commit messages which have same prefix e.g.\n>> > yyyyyy This is prefix\n>> > xxxxxx This is prefix and message\n>> >\n>> > xxxxxx comitted before yyyyyy\n>> >\n>> > Now I commit a fixup to yyyyyy using git commit --fixup yyyyyy\n>> > zzzzzz fixup! This is prefix\n>> >\n>> > When I run git rebase -i --autosquash, the script it shows me looks \n>> > like\n>> > pick xxxxxx This is prefix and message\n>> > fixup zzzzzz fixup! This is prefix\n>> > pick yyyyyy This is prefix\n>> >\n>> > I think the correct order is\n>> > pick xxxxxx This is prefix and message\n>> > pick yyyyyy This is prefix\n>> > fixup zzzzzz fixup! This is prefix\n>> >\n>> > Is that right?\n>>\n>> [...]\n>>\n>> Unfortunately, \"rebase -i --autosquash\" reorders the entries by\n>> identifying the commit by its title, and it goes with prefix match so\n>> that fix-up commits created without using --fixup option but manually\n>> records the title's prefix substring can also work.\n>\n> This prefix match also happens to introduce a serious performance problem,\n> which is why I \"fixed\" this issue in the rebase--helper already (which is\n> the case if you are using Git for Windows, whose master branch builds on\n> Linux and MacOSX as well). I quoted \"fix\" because my motivation was to fix\n> the performance problem, not the \"incorrect match\" problem.\n>\n> The rebase--helper code (specifically, the patch moving autosquash logic\n> into it: https://github.com/dscho/git/commit/7d0831637f) tries to match\n> exact onelines first,\n\nWhile I think this is an improvement, and will strongly support the `git \ncommit --fixup=` option which will, if the sha1/oid is given, create \nthe exact commit subject line.\n\nHowever it would also be useful if the actual commit subject line could have \na similar format option, so that those who use say the git gui (rather than \nthe cli) for the commit message, could easily create the `!fixup ` \nmessage which would allow a broader range of ways of spelling the commit \n(e.g. giving a sha1(min length) that is within the rebase todo list).\n\n> and falls back to prefix matching only after that.\n>\n> Now that the sequencer-i patch series is in `master`, the next step is to\n> send the patch series introducing the rebase--helper. The patch series\n> including the fix discussed above relies on that one. Meaning that it will\n> take a while to get through the mill.\n>\n> So please do not hold your breath until this feature/fix hits an official\n> Git version. If you need it[*1*] faster, feel free to build Git for\n> Windows' master and run with that for a while.\n>\n> Ciao,\n> Johannes\n>\n> Footnote: By \"it\" I mean \"the feature/fix\", not \"an official Git version\"\n> nor \"your breath\".\n>\n--\nPhilip \n\n"},{"id":"311265","messageId":"alpine.DEB.2.20.1702101459200.3496@virtualbox","threadId":"45083","inReplyTo":"454E7D934160418EB4C4871ED209CBCA@PhilipOakley","subject":"Re: Bug with fixup and autosquash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T14:02:15Z","receivedAt":"2017-02-10T14:02:49Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philip,\n\nOn Thu, 9 Feb 2017, Philip Oakley wrote:\n\n> From: \"Johannes Schindelin\" \n> Sent: Thursday, February 09, 2017 8:55 PM\n>\n> > The rebase--helper code (specifically, the patch moving autosquash\n> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries\n> > to match exact onelines first,\n> \n> While I think this is an improvement, and will strongly support the `git\n> commit --fixup=` option which will, if the sha1/oid is given,\n> create the exact commit subject line.\n\nThat is already the case (with the exception that it is not the \"exact\ncommit subject line\" but the oneline, i.e. unwrapped first paragraph).\n\n> However it would also be useful if the actual commit subject line could\n> have a similar format option, so that those who use say the git gui\n> (rather than the cli) for the commit message, could easily create the\n> `!fixup ` message which would allow a broader range of ways of\n> spelling the commit (e.g. giving a sha1(min length) that is within the\n> rebase todo list).\n\nIt is already the case that `fixup! ` is accepted, see the code\nreplaced by above-mentioned commit:\n\nhttps://github.com/dscho/git/commit/7d0831637f#diff-0f15aff45d5dd346465c35597a5f274eL780\n\n... and its replacement code in C:\n\nhttps://github.com/dscho/git/commit/7d0831637f#diff-79231f0693f84f3951daeea17065aad9R2800\n\nNote that both preimage and postimage code try to match onelines first,\nwith the new code changing behavior ever so slightly: it tries to match\nthe exact oneline first, then a commit SHA-1 of an already-seen `pick`\nline, and only then falls back to the (expensive) prefix match.\n\nCiao,\nDscho\n"},{"id":"311273","messageId":"alpine.DEB.2.20.1702101654500.3496@virtualbox","threadId":"45083","inReplyTo":"xmqqd1ernh7g.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug with fixup and autosquash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T15:57:32Z","receivedAt":"2017-02-10T15:58:09Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 9 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Almost. While I fixed the performance issues as well as the design\n> > allowed, I happened to \"fix\" the problem where an incomplete prefix\n> > match could be favored over an exact match.\n> \n> Hmph. Would it require too much further work to do what you said the\n> code does:\n\nI was just being overly precise. I *did* fix the problem. But since it was\nnot my intention, I quoted the verb \"fix\".\n\n> > The rebase--helper code (specifically, the patch moving autosquash\n> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries\n> > to match exact onelines first, and falls back to prefix matching only\n> > after that.\n> \n> If the code matches exact onlines and then falls back to prefix, I do\n> not think incomplete prefix would be mistakenly chosen over an exact\n> one, so perhaps your code already does the right thing?\n\nThe code does exactly that. It does even more: as `fixup! ` is\nallowed (for SHA-1s that have been mentioned in previous `pick` lines), it\ntries to match that before falling back to the incomplete prefix match.\n\nCiao,\nJohannes\n"},{"id":"311290","messageId":"93E7A279BF61414D9EE8BB12B88E16E8@PhilipOakley","threadId":"45083","inReplyTo":"alpine.DEB.2.20.1702101654500.3496@virtualbox","subject":"Re: Bug with fixup and autosquash","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-02-10T19:02:14Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Johannes Schindelin\" \n> Hi Junio,\n>\n> On Thu, 9 Feb 2017, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > Almost. While I fixed the performance issues as well as the design\n>> > allowed, I happened to \"fix\" the problem where an incomplete prefix\n>> > match could be favored over an exact match.\n>>\n>> Hmph. Would it require too much further work to do what you said the\n>> code does:\n>\n> I was just being overly precise. I *did* fix the problem. But since it was\n> not my intention, I quoted the verb \"fix\".\n>\n>> > The rebase--helper code (specifically, the patch moving autosquash\n>> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries\n>> > to match exact onelines first, and falls back to prefix matching only\n>> > after that.\n>>\n>> If the code matches exact onlines and then falls back to prefix, I do\n>> not think incomplete prefix would be mistakenly chosen over an exact\n>> one, so perhaps your code already does the right thing?\n>\n> The code does exactly that. It does even more: as `fixup! ` is\n> allowed (for SHA-1s that have been mentioned in previous `pick` lines), it\n> tries to match that before falling back to the incomplete prefix match.\n>\n> Ciao,\n> Johannes\n\nNow just the doc update to do.... ;-)\n\nI definitely think the 'fix' that allows the `fixup! ` as the subject \nline is a good way to go for those who mix in the use of the gui and gitk \ninto their workflow (*)\n\n--\nPhilip\n(*) I just don't see the point of having multiple cli tty windows, and then \nnot using the gui/k windows that are part of the tool set.\n\n"},{"id":"317802","messageId":"alpine.DEB.2.20.1704251328480.3480@virtualbox","threadId":"45083","inReplyTo":"93E7A279BF61414D9EE8BB12B88E16E8@PhilipOakley","subject":"Re: Bug with fixup and autosquash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-25T11:30:30Z","receivedAt":"2017-04-25T11:30:53Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philip,\n\nOn Fri, 10 Feb 2017, Philip Oakley wrote:\n\n> From: \"Johannes Schindelin\" \n> >\n> > On Thu, 9 Feb 2017, Junio C Hamano wrote:\n> >\n> > > Johannes Schindelin writes:\n> > >\n> > > > Almost. While I fixed the performance issues as well as the design\n> > > > allowed, I happened to \"fix\" the problem where an incomplete\n> > > > prefix match could be favored over an exact match.\n> > >\n> > > Hmph. Would it require too much further work to do what you said\n> > > the code does:\n> >\n> > I was just being overly precise. I *did* fix the problem. But since it\n> > was not my intention, I quoted the verb \"fix\".\n> >\n> > > > The rebase--helper code (specifically, the patch moving autosquash\n> > > > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries\n> > > > to match exact onelines first, and falls back to prefix matching only\n> > > > after that.\n> > >\n> > > If the code matches exact onlines and then falls back to prefix, I do\n> > > not think incomplete prefix would be mistakenly chosen over an exact\n> > > one, so perhaps your code already does the right thing?\n> >\n> > The code does exactly that. It does even more: as `fixup! ` is\n> > allowed (for SHA-1s that have been mentioned in previous `pick` lines), it\n> > tries to match that before falling back to the incomplete prefix match.\n> \n> Now just the doc update to do.... ;-)\n\nRight. I finally managed to work that in, and will send out v2 (is it only\nthe second iteration? ... wow...) as soon as the test suite passes on\nWindows (which will take a bit over an hour, as you know).\n\nCiao,\nDscho\n"}]}