{"thread":{"id":"44822","subject":"Squashing commits with git rebase -i may miscount commits in commit message","startedAt":"2017-01-06T09:11:33Z","lastAt":"2017-01-08T03:26:40Z","messageCount":6,"participants":["Brandon Tolsch","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308848","messageId":"CAMWRQeRaQQQcJ-R8eHc7f0KqZF2eEkYJOyTb9n7ds78pTqV-AA@mail.gmail.com","threadId":"44822","inReplyTo":null,"subject":"Squashing commits with git rebase -i may miscount commits in commit message","fromName":"Brandon Tolsch","fromEmail":"btolsch@gmail.com","sentAt":"2017-01-06T09:04:05Z","receivedAt":"2017-01-06T09:11:33Z","isPatch":false,"sender":{"key":"btolsch@gmail.com","avatar":null},"body":"git --version: 2.11.0\n\nWhen using git rebase -i to squash a series of commits that includes\nmore than 10 commits, the generated commit message you are given to\nedit counts the old messages incorrectly.  It will say the total\nnumber of commits is (actual % 10) (if they were 0-based) and it will\nalso count the commits as (actual % 10).\n\nFor example, 15, 25, 35, etc. commits:\n# This is a combination of 5 commits.\n# This is the 1st commit message:\nmsg\n\n# This is the commit message #2:\n...\n...\n# This is commit message #10:\nmsg\n\n# This is commit message #1:\n...\n\n# This is commit message #5:\nmsg\n\nWhile not a big issue, it did make me double check what I was doing\nwhen I saw \"a combination of 10 commits\" instead of 20 in the commit\nmessage.\n\n-Brandon Tolsch\n"},{"id":"308854","messageId":"alpine.DEB.2.20.1701061445130.3469@virtualbox","threadId":"44822","inReplyTo":"CAMWRQeRaQQQcJ-R8eHc7f0KqZF2eEkYJOyTb9n7ds78pTqV-AA@mail.gmail.com","subject":"Re: Squashing commits with git rebase -i may miscount commits in commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-06T13:46:17Z","receivedAt":"2017-01-06T13:47:17Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 6 Jan 2017, Brandon Tolsch wrote:\n\n> git --version: 2.11.0\n> \n> When using git rebase -i to squash a series of commits that includes\n> more than 10 commits, the generated commit message you are given to\n> edit counts the old messages incorrectly.  It will say the total\n> number of commits is (actual % 10) (if they were 0-based) and it will\n> also count the commits as (actual % 10).\n> \n> For example, 15, 25, 35, etc. commits:\n> # This is a combination of 5 commits.\n> # This is the 1st commit message:\n> msg\n> \n> # This is the commit message #2:\n> ...\n> ...\n> # This is commit message #10:\n> msg\n> \n> # This is commit message #1:\n> ...\n> \n> # This is commit message #5:\n> msg\n> \n> While not a big issue, it did make me double check what I was doing\n> when I saw \"a combination of 10 commits\" instead of 20 in the commit\n> message.\n\nJust for the record: I verified that the rebase--helper based interactive\nrebase (which is already in Git for Windows since v2.10.0) does *not* have\nthis bug.\n\nMaybe a point in favor rebase--helper...\n\nCiao,\nJohannes\n"},{"id":"308921","messageId":"20170107082318.jj3izthx2ylscrns@sigill.intra.peff.net","threadId":"44822","inReplyTo":"CAMWRQeRaQQQcJ-R8eHc7f0KqZF2eEkYJOyTb9n7ds78pTqV-AA@mail.gmail.com","subject":"[PATCH] rebase--interactive: count squash commits above 10 correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-07T08:23:19Z","receivedAt":"2017-01-07T08:23:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:\n\n> git --version: 2.11.0\n> \n> When using git rebase -i to squash a series of commits that includes\n> more than 10 commits, the generated commit message you are given to\n> edit counts the old messages incorrectly.  It will say the total\n> number of commits is (actual % 10) (if they were 0-based) and it will\n> also count the commits as (actual % 10).\n\nLooks like a regression in v2.10. Here's the fix.\n\n-- >8 --\nSubject: rebase--interactive: count squash commits above 10 correctly\n\nWe generate the squash commit message incrementally running\na sed script once for each commit. It parses \"This is\na combination of <N> commits\" from the first line of the\nexisting message, adds one to <N>, and uses the result as\nthe number of our current message.\n\nSince f2d17068fd (i18n: rebase-interactive: mark comments of\nsquash for translation, 2016-06-17), the first line may be\nlocalized, and sed uses a pretty liberal regex, looking for:\n\n  /^#.*([0-9][0-9]*)/\n\nThe \"[0-9][0-9]*\" tries to match double digits, but it\ndoesn't quite work.  The first \".*\" is greedy, so if you\nhave:\n\n  This is a combination of 10 commits.\n\nit will eat up \"This is a combination of 1\", leaving \"0\" to\nmatch the first \"[0-9]\" digit, and then skipping the\noptional match of \"[0-9]*\".\n\nAs a result, the count resets every 10 commits, and a\n15-commit squash would end up as:\n\n  # This is a combination of 5 commits.\n  # This is the 1st commit message:\n  ...\n  # This is the commit message #2:\n  ... and so on ..\n  # This is the commit message #10:\n  ...\n  # This is the commit message #1:\n  ...\n  # This is the commit message #2:\n  ... etc, up to 5 ...\n\nWe can fix this by making the \".*\" less greedy. Instead of\ndepending on \".*?\" working portably, we can just limit the\nmatch to non-digit characters, which accomplishes the same\nthing.\n\nReported-by: Brandon Tolsch <btolsch@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n\nI didn't include a test here, mostly because this bug is so weirdly\nspecific that it seems unlikely to happen again. Especially in light of\nthis code going away in favor of the C rebase helper, which Dscho\nalready confirmed is not buggy (and I did not look at the code, but it\ncannot possibly do anything as gross as these repeated sed invocations).\n\nIt also is a little tricky to automatically extract the comments (you\nhave to override GIT_EDITOR, but we also invoke GIT_EDITOR for the insn\nsheet).\n\nI was able to replicate and confirm the fix manually with:\n\n  git commit -q --allow-empty -m base\n  git commit -q --allow-empty -m squash\n  git tag base\n  for i in $(seq 15); do\n    echo $i >$i\n    git add $i\n    git commit -qm \"squash! squash\"\n  done\n  git rebase --autosquash -i base\n\nI'd also suggest that \"the commit message #4\" is grammatically\nquestionable. Probably \"This is commit message #4\" would be fine.\n\n git-rebase--interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex b0a6f2b7ba..4734094a3f 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -425,7 +425,7 @@ update_squash_messages () {\n \tif test -f \"$squash_msg\"; then\n \t\tmv \"$squash_msg\" \"$squash_msg\".bak || exit\n \t\tcount=$(($(sed -n \\\n-\t\t\t-e \"1s/^$comment_char.*\\([0-9][0-9]*\\).*/\\1/p\" \\\n+\t\t\t-e \"1s/^$comment_char[^0-9]*\\([0-9][0-9]*\\).*/\\1/p\" \\\n \t\t\t-e \"q\" < \"$squash_msg\".bak)+1))\n \t\t{\n \t\t\tprintf '%s\\n' \"$comment_char $(eval_ngettext \\\n-- \n2.11.0.527.gfef230ca76\n\n"},{"id":"308926","messageId":"alpine.DEB.2.20.1701071150490.3469@virtualbox","threadId":"44822","inReplyTo":"20170107082318.jj3izthx2ylscrns@sigill.intra.peff.net","subject":"Re: [PATCH] rebase--interactive: count squash commits above 10 correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-07T10:51:23Z","receivedAt":"2017-01-07T10:51:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 7 Jan 2017, Jeff King wrote:\n\n> On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:\n> \n> We can fix this by making the \".*\" less greedy. Instead of\n> depending on \".*?\" working portably, we can just limit the\n> match to non-digit characters, which accomplishes the same\n> thing.\n\nOr we could simply require a space before the first digit, which would\nmaybe look a little simpler.\n\nCiao,\nDscho\n"},{"id":"308927","messageId":"20170107110539.jcx25pdy7cosojcp@sigill.intra.peff.net","threadId":"44822","inReplyTo":"alpine.DEB.2.20.1701071150490.3469@virtualbox","subject":"Re: [PATCH] rebase--interactive: count squash commits above 10 correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-07T11:05:39Z","receivedAt":"2017-01-07T11:05:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 07, 2017 at 11:51:23AM +0100, Johannes Schindelin wrote:\n\n> > We can fix this by making the \".*\" less greedy. Instead of\n> > depending on \".*?\" working portably, we can just limit the\n> > match to non-digit characters, which accomplishes the same\n> > thing.\n> \n> Or we could simply require a space before the first digit, which would\n> maybe look a little simpler.\n\nYeah, if we can assume that all translations will always have a space\nthere. It is almost certainly true for Western languages, but I don't\nknow about others. It looks like the Korean translation omits a space\n_after_ the digits, but there is still one before.\n\n-Peff\n"},{"id":"308947","messageId":"xmqqlgum1bbt.fsf@gitster.mtv.corp.google.com","threadId":"44822","inReplyTo":"20170107082318.jj3izthx2ylscrns@sigill.intra.peff.net","subject":"Re: [PATCH] rebase--interactive: count squash commits above 10 correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-08T03:26:30Z","receivedAt":"2017-01-08T03:26:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  git-rebase--interactive.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index b0a6f2b7ba..4734094a3f 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -425,7 +425,7 @@ update_squash_messages () {\n>  \tif test -f \"$squash_msg\"; then\n>  \t\tmv \"$squash_msg\" \"$squash_msg\".bak || exit\n>  \t\tcount=$(($(sed -n \\\n> -\t\t\t-e \"1s/^$comment_char.*\\([0-9][0-9]*\\).*/\\1/p\" \\\n> +\t\t\t-e \"1s/^$comment_char[^0-9]*\\([0-9][0-9]*\\).*/\\1/p\" \\\n\nI would have written it to match \".*[^1-9]\\([1-9][0-9]*\\)\", though.\nEven if a foreign language expresses all its words as digits (or has\na digit as the last letter of the last word before the number), a\ntranslation into it must have a non-digit before the number to\ndisambiguate---but I guess I am being ultra-pedantic, and I think\nwhat you wrote would be sufficient in practice.\n\nAs you guys discussed, this code will hopefully disappear in a cycle\nor two anyway ;-)  Let's queue this as-is.\n\nThanks.\n\n>  \t\t\t-e \"q\" < \"$squash_msg\".bak)+1))\n>  \t\t{\n>  \t\t\tprintf '%s\\n' \"$comment_char $(eval_ngettext \\\n"}]}