Squashing commits with git rebase -i may miscount commits in commit message

Squashing commits with git rebase -i may miscount commits in commit message

[Brandon Tolsch]

git --version: 2.11.0

When using git rebase -i to squash a series of commits that includes
more than 10 commits, the generated commit message you are given to
edit counts the old messages incorrectly.  It will say the total
number of commits is (actual % 10) (if they were 0-based) and it will
also count the commits as (actual % 10).

For example, 15, 25, 35, etc. commits:
# This is a combination of 5 commits.
# This is the 1st commit message:
msg

# This is the commit message #2:
...
...
# This is commit message #10:
msg

# This is commit message #1:
...

# This is commit message #5:
msg

While not a big issue, it did make me double check what I was doing
when I saw "a combination of 10 commits" instead of 20 in the commit
message.

-Brandon Tolsch

Re: Squashing commits with git rebase -i may miscount commits in commit message

[Johannes Schindelin]

Hi,

On Fri, 6 Jan 2017, Brandon Tolsch wrote:

> git --version: 2.11.0
> 
> When using git rebase -i to squash a series of commits that includes
> more than 10 commits, the generated commit message you are given to
> edit counts the old messages incorrectly.  It will say the total
> number of commits is (actual % 10) (if they were 0-based) and it will
> also count the commits as (actual % 10).
> 
> For example, 15, 25, 35, etc. commits:
> # This is a combination of 5 commits.
> # This is the 1st commit message:
> msg
> 
> # This is the commit message #2:
> ...
> ...
> # This is commit message #10:
> msg
> 
> # This is commit message #1:
> ...
> 
> # This is commit message #5:
> msg
> 
> While not a big issue, it did make me double check what I was doing
> when I saw "a combination of 10 commits" instead of 20 in the commit
> message.

Just for the record: I verified that the rebase--helper based interactive
rebase (which is already in Git for Windows since v2.10.0) does *not* have
this bug.

Maybe a point in favor rebase--helper...

Ciao,
Johannes

[PATCH] rebase--interactive: count squash commits above 10 correctly

[Jeff King]

On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:

> git --version: 2.11.0
> 
> When using git rebase -i to squash a series of commits that includes
> more than 10 commits, the generated commit message you are given to
> edit counts the old messages incorrectly.  It will say the total
> number of commits is (actual % 10) (if they were 0-based) and it will
> also count the commits as (actual % 10).

Looks like a regression in v2.10. Here's the fix.

-- >8 --
Subject: rebase--interactive: count squash commits above 10 correctly

We generate the squash commit message incrementally running
a sed script once for each commit. It parses "This is
a combination of <N> commits" from the first line of the
existing message, adds one to <N>, and uses the result as
the number of our current message.

Since f2d17068fd (i18n: rebase-interactive: mark comments of
squash for translation, 2016-06-17), the first line may be
localized, and sed uses a pretty liberal regex, looking for:

  /^#.*([0-9][0-9]*)/

The "[0-9][0-9]*" tries to match double digits, but it
doesn't quite work.  The first ".*" is greedy, so if you
have:

  This is a combination of 10 commits.

it will eat up "This is a combination of 1", leaving "0" to
match the first "[0-9]" digit, and then skipping the
optional match of "[0-9]*".

As a result, the count resets every 10 commits, and a
15-commit squash would end up as:

  # This is a combination of 5 commits.
  # This is the 1st commit message:
  ...
  # This is the commit message #2:
  ... and so on ..
  # This is the commit message #10:
  ...
  # This is the commit message #1:
  ...
  # This is the commit message #2:
  ... etc, up to 5 ...

We can fix this by making the ".*" less greedy. Instead of
depending on ".*?" working portably, we can just limit the
match to non-digit characters, which accomplishes the same
thing.

Reported-by: Brandon Tolsch <btolsch@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---

I didn't include a test here, mostly because this bug is so weirdly
specific that it seems unlikely to happen again. Especially in light of
this code going away in favor of the C rebase helper, which Dscho
already confirmed is not buggy (and I did not look at the code, but it
cannot possibly do anything as gross as these repeated sed invocations).

It also is a little tricky to automatically extract the comments (you
have to override GIT_EDITOR, but we also invoke GIT_EDITOR for the insn
sheet).

I was able to replicate and confirm the fix manually with:

  git commit -q --allow-empty -m base
  git commit -q --allow-empty -m squash
  git tag base
  for i in $(seq 15); do
    echo $i >$i
    git add $i
    git commit -qm "squash! squash"
  done
  git rebase --autosquash -i base

I'd also suggest that "the commit message #4" is grammatically
questionable. Probably "This is commit message #4" would be fine.

 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b0a6f2b7ba..4734094a3f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -425,7 +425,7 @@ update_squash_messages () {
 	if test -f "$squash_msg"; then
 		mv "$squash_msg" "$squash_msg".bak || exit
 		count=$(($(sed -n \
-			-e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
+			-e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
 			-e "q" < "$squash_msg".bak)+1))
 		{
 			printf '%s\n' "$comment_char $(eval_ngettext \
-- 
2.11.0.527.gfef230ca76

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Johannes Schindelin]

Hi Peff,

On Sat, 7 Jan 2017, Jeff King wrote:

> On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:
> 
> We can fix this by making the ".*" less greedy. Instead of
> depending on ".*?" working portably, we can just limit the
> match to non-digit characters, which accomplishes the same
> thing.

Or we could simply require a space before the first digit, which would
maybe look a little simpler.

Ciao,
Dscho

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Jeff King]

On Sat, Jan 07, 2017 at 11:51:23AM +0100, Johannes Schindelin wrote:

> > We can fix this by making the ".*" less greedy. Instead of
> > depending on ".*?" working portably, we can just limit the
> > match to non-digit characters, which accomplishes the same
> > thing.
> 
> Or we could simply require a space before the first digit, which would
> maybe look a little simpler.

Yeah, if we can assume that all translations will always have a space
there. It is almost certainly true for Western languages, but I don't
know about others. It looks like the Korean translation omits a space
_after_ the digits, but there is still one before.

-Peff

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>  git-rebase--interactive.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index b0a6f2b7ba..4734094a3f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -425,7 +425,7 @@ update_squash_messages () {
>  	if test -f "$squash_msg"; then
>  		mv "$squash_msg" "$squash_msg".bak || exit
>  		count=$(($(sed -n \
> -			-e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
> +			-e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \

I would have written it to match ".*[^1-9]\([1-9][0-9]*\)", though.
Even if a foreign language expresses all its words as digits (or has
a digit as the last letter of the last word before the number), a
translation into it must have a non-digit before the number to
disambiguate---but I guess I am being ultra-pedantic, and I think
what you wrote would be sufficient in practice.

As you guys discussed, this code will hopefully disappear in a cycle
or two anyway ;-)  Let's queue this as-is.

Thanks.

>  			-e "q" < "$squash_msg".bak)+1))
>  		{
>  			printf '%s\n' "$comment_char $(eval_ngettext \