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
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
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 <redacted>
Signed-off-by: Jeff King <redacted>
---
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
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
On Sat, Jan 07, 2017 at 11:51:23AM +0100, Johannes Schindelin wrote:
quoted
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