sequencer: comment out the 'squash!' line

From: Michael Rappazzo <[email protected]>

When performing a squash commit, the commit comments are combined into a single commit. Since the subject line of the squash commit is used to identify the squash-to target commit, it cannot offer any useful contribution to the new commit message. Therefore, the squash commit subject line it commented out from the combined message (much like a fixup commit's full comment).

The body of a squash commit may contain additional content to add to the commit message, so this part of the squash commit message is retained.

Since this change what the expected post-rebase commit comment would look like, related test expectations are adjusted to reflect the the new expectation. A new test is added for the new expectation.

Signed-off-by Michael Rappazzo <[email protected]>

---
  sequencer.c                   |  1 +
  t/t3404-rebase-interactive.sh |  4 +---
  t/t3415-rebase-autosquash.sh  | 36 +++++++++++++++++++++++++++--------
  t/t3900-i18n-commit.sh        |  4 ----
  4 files changed, 30 insertions(+), 15 deletions(-)

sequencer.c +1 -0

index 763ccbbc45..e5602686d7 100644
@@ -1756,6 +1756,7 @@ static int update_squash_messages(struct repository *r,
  		strbuf_addf(&buf, _("This is the commit message #%d:"),
  			    ++opts->current_fixup_count + 1);
  		strbuf_addstr(&buf, "\n\n");
+		strbuf_addf(&buf, "%c ", comment_line_char);
  		strbuf_addstr(&buf, body);
  	} else if (command == TODO_FIXUP) {
  		strbuf_addf(&buf, "\n%c ", comment_line_char);

t/t3404-rebase-interactive.sh +1 -3

index ae6e55ce79..57d178d431 100755
@@ -513,8 +513,6 @@ test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messa
  	cat >expect-squash-fixup <<-\EOF &&
  	B
 
-	D
-
  	ONCE
  	EOF
  	git checkout -b squash-fixup E &&

@@ -1325,7 +1323,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
  	test_cmp_rev HEAD F &&
  	rm file6 &&
  	git rebase --continue &&
-	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
  	git reset --hard original-branch2
  '
 

t/t3415-rebase-autosquash.sh +1 -2

index 22d218698e..51c5a94aea 100755
@@ -59,7 +59,6 @@ test_auto_squash () {
  	git add -u &&
  	test_tick &&
  	git commit -m "squash! first" &&
-
  	git tag $1 &&
  	test_tick &&
  	git rebase $2 -i HEAD^^^ &&

@@ -67,7 +66,7 @@ test_auto_squash () {
  	test_line_count = 3 actual &&
  	git diff --exit-code $1 &&
  	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
  }
 
  test_expect_success 'auto squash (option)' '

@@ -82,6 +81,27 @@ test_expect_success 'auto squash (config)' '
  	test_must_fail test_auto_squash final-squash-config-false

t/t3415-rebase-autosquash.sh +9 -6

+	test 1 = $(grep Additional actual-full | wc -l)
+'
+
  test_expect_success 'misspelled auto squash' '
  	git reset --hard base &&
  	echo 1 >file1 &&

@@ -114,7 +134,7 @@ test_expect_success 'auto squash that matches 2 commits' '
  	test_line_count = 4 actual &&
  	git diff --exit-code final-multisquash &&
  	test 1 = "$(git cat-file blob HEAD^^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
+	test 1 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
  	test 1 = $(git cat-file commit HEAD | grep first | wc -l)
  '
 

@@ -152,7 +172,7 @@ test_expect_success 'auto squash that matches a sha1' '
  	test_line_count = 3 actual &&
  	git diff --exit-code final-shasquash &&
  	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_expect_success 'auto squash that matches longer sha1' '

@@ -168,7 +188,7 @@ test_expect_success 'auto squash that matches longer sha1' '
  	test_line_count = 3 actual &&
  	git diff --exit-code final-longshasquash &&
  	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_auto_commit_flags () {

@@ -192,7 +212,7 @@ test_expect_success 'use commit --fixup' '
  '
 
  test_expect_success 'use commit --squash' '
-	test_auto_commit_flags squash 2
+	test_auto_commit_flags squash 1
  '
 
  test_auto_fixup_fixup () {

@@ -228,7 +248,7 @@ test_auto_fixup_fixup () {
  		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
  	elif test "$1" = "squash"
  	then
-		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
  	else
  		false
  	fi

@@ -268,7 +288,7 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
  	test_line_count = 3 actual &&
  	git diff --exit-code final-squash-instFmt &&
  	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_expect_success 'autosquash with empty custom instructionFormat' '

t/t3900-i18n-commit.sh +0 -4

index d277a9f4b7..bfab245eb3 100755
@@ -226,10 +226,6 @@ test_commit_autosquash_multi_encoding () {
  		git rev-list HEAD >actual &&
  		test_line_count = 3 actual &&
  		iconv -f $old -t UTF-8 "$TEST_DIRECTORY"/t3900/$msg >expect &&
-		if test $flag = squash; then
-			subject="$(head -1 expect)" &&
-			printf "\nsquash! %s\n" "$subject" >>expect
-		fi &&
  		git cat-file commit HEAD^ >raw &&
  		(sed "1,/^$/d" raw | iconv -f $new -t utf-8) >actual &&
  		test_cmp expect actual

Since this change what the expected post-rebase commit comment would look like, related test expectations are adjusted to reflect the the new expectation. A new test is added for the new expectation.

Signed-off-by Michael Rappazzo [email protected] [[email protected]]

Michael Rappazzo (1):
sequencer: comment out the 'squash!' line

 sequencer.c                   |  1 +
 t/t3404-rebase-interactive.sh |  4 +---
 t/t3415-rebase-autosquash.sh  | 36 +++++++++++++++++++++++++++--------
 t/t3900-i18n-commit.sh        |  4 ----
 4 files changed, 30 insertions(+), 15 deletions(-)

base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-511%2Frappazzo%2Fcomment-squash-subject-line-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-511/rappazzo/comment-squash-subject-line-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/511

Hi Michael

Quoted 8 lines · Michael Rappazzo via GitGitGadget

From: Michael Rappazzo <[email protected]>

When performing a squash commit, the commit comments are combined into a single commit. Since the subject line of the squash commit is used to identify the squash-to target commit, it cannot offer any useful contribution to the new commit message. Therefore, the squash commit subject line it commented out from the combined message (much like a fixup commit's full comment).

I like the idea but I think it would be better to only comment out the subject of the commit message if it starts with squash! for fixup! otherwise it may well be a useful part of the message. For correctness I think it would be better to comment out the subject (everything before the first blank line as returned by git log --pretty=%s) rather than just the first line. I've actually implemented this as part of a longer series that I've never got round to posting to the list[1] - feel free to use any of that which you find useful. That commit also shows an alternative way to change the --autosquash tests.

[1]
https://github.com/phillipwood/git/commit/b91b492e4aba1ac8d244859361379d5063cfc2b8

Quoted 72 lines

The body of a squash commit may contain additional content to add to the commit message, so this part of the squash commit message is retained.

Since this change what the expected post-rebase commit comment would look like, related test expectations are adjusted to reflect the the new expectation. A new test is added for the new expectation.

Signed-off-by Michael Rappazzo <[email protected]>

---
  sequencer.c                   |  1 +
  t/t3404-rebase-interactive.sh |  4 +---
  t/t3415-rebase-autosquash.sh  | 36 +++++++++++++++++++++++++++--------
  t/t3900-i18n-commit.sh        |  4 ----
  4 files changed, 30 insertions(+), 15 deletions(-)

sequencer.c +1 -0

index 763ccbbc45..e5602686d7 100644
@@ -1756,6 +1756,7 @@ static int update_squash_messages(struct repository *r,
              strbuf_addf(&buf, _("This is the commit message #%d:"),
                          ++opts->current_fixup_count + 1);
              strbuf_addstr(&buf, "\n\n");
+             strbuf_addf(&buf, "%c ", comment_line_char);
              strbuf_addstr(&buf, body);
      } else if (command == TODO_FIXUP) {
              strbuf_addf(&buf, "\n%c ", comment_line_char);

t/t3404-rebase-interactive.sh +1 -3

index ae6e55ce79..57d178d431 100755
@@ -513,8 +513,6 @@ test_expect_success C_LOCALE_OUTPUT 'squash and fixup generate correct log messa
      cat >expect-squash-fixup <<-\EOF &&
      B
 
-     D
-
      ONCE
      EOF
      git checkout -b squash-fixup E &&

@@ -1325,7 +1323,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
      test_cmp_rev HEAD F &&
      rm file6 &&
      git rebase --continue &&
-     test $(git cat-file commit HEAD | sed -ne \$p) = I &&
+     test $(git cat-file commit HEAD | sed -ne \$p) = F &&
      git reset --hard original-branch2
  '
 

t/t3415-rebase-autosquash.sh +1 -2

index 22d218698e..51c5a94aea 100755
@@ -59,7 +59,6 @@ test_auto_squash () {
      git add -u &&
      test_tick &&
      git commit -m "squash! first" &&
-
      git tag $1 &&
      test_tick &&
      git rebase $2 -i HEAD^^^ &&

@@ -67,7 +66,7 @@ test_auto_squash () {
      test_line_count = 3 actual &&
      git diff --exit-code $1 &&
      test 1 = "$(git cat-file blob HEAD^:file1)" &&
-     test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+     test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
  }
 
  test_expect_success 'auto squash (option)' '

@@ -82,6 +81,27 @@ test_expect_success 'auto squash (config)' '
      test_must_fail test_auto_squash final-squash-config-false

t/t3415-rebase-autosquash.sh test_expect_success 'auto squash (config)' ' · line 91

Quoted 10 lines

t/t3415-rebase-autosquash.sh +8 -0

  '
 
+test_expect_success 'auto squash includes squash body but not squash directive' '
+     git reset --hard base &&
+     echo 1 >file1 &&
+     git add -u &&
+     test_tick &&
+     git commit -m "squash! first
+
+Additional Body" &&

git commit --squash=first -m "Additional Body"
would avoid the multi line argument

t/t3415-rebase-autosquash.sh test_expect_success 'auto squash (config)' ' · line 96

Quoted 5 lines

t/t3415-rebase-autosquash.sh +5 -0

+     git tag squash-with-body &&
+     test_tick &&
+     git rebase --autosquash -i HEAD^^^ &&
+     git log --oneline >actual &&
+     git log --oneline --format="%s%n%b" >actual-full &&

git log --format=%B ?

t/t3415-rebase-autosquash.sh test_expect_success 'auto squash (config)' ' · line 101

Quoted 6 lines

t/t3415-rebase-autosquash.sh +5 -0

+     test_line_count = 3 actual &&
+     git diff --exit-code squash-with-body &&
+     test 1 = "$(git cat-file blob HEAD^:file1)" &&
+     test 1 = $(git cat-file commit HEAD^ | grep first | wc -l) &&
+     test 0 = $(grep squash actual-full | wc -l) &&

grep -v squash actual-full
is simpler I think

Best Wishes

Phillip

Quoted 76 lines

t/t3415-rebase-autosquash.sh +9 -6

+     test 1 = $(grep Additional actual-full | wc -l)
+'
+
  test_expect_success 'misspelled auto squash' '
      git reset --hard base &&
      echo 1 >file1 &&

@@ -114,7 +134,7 @@ test_expect_success 'auto squash that matches 2 commits' '
      test_line_count = 4 actual &&
      git diff --exit-code final-multisquash &&
      test 1 = "$(git cat-file blob HEAD^^:file1)" &&
-     test 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
+     test 1 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
      test 1 = $(git cat-file commit HEAD | grep first | wc -l)
  '
 

@@ -152,7 +172,7 @@ test_expect_success 'auto squash that matches a sha1' '
      test_line_count = 3 actual &&
      git diff --exit-code final-shasquash &&
      test 1 = "$(git cat-file blob HEAD^:file1)" &&
-     test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+     test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_expect_success 'auto squash that matches longer sha1' '

@@ -168,7 +188,7 @@ test_expect_success 'auto squash that matches longer sha1' '
      test_line_count = 3 actual &&
      git diff --exit-code final-longshasquash &&
      test 1 = "$(git cat-file blob HEAD^:file1)" &&
-     test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+     test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_auto_commit_flags () {

@@ -192,7 +212,7 @@ test_expect_success 'use commit --fixup' '
  '
 
  test_expect_success 'use commit --squash' '
-     test_auto_commit_flags squash 2
+     test_auto_commit_flags squash 1
  '
 
  test_auto_fixup_fixup () {

@@ -228,7 +248,7 @@ test_auto_fixup_fixup () {
              test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
      elif test "$1" = "squash"
      then
-             test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+             test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
      else
              false
      fi

@@ -268,7 +288,7 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
      test_line_count = 3 actual &&
      git diff --exit-code final-squash-instFmt &&
      test 1 = "$(git cat-file blob HEAD^:file1)" &&
-     test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+     test 0 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
 
  test_expect_success 'autosquash with empty custom instructionFormat' '

t/t3900-i18n-commit.sh +0 -4

index d277a9f4b7..bfab245eb3 100755
@@ -226,10 +226,6 @@ test_commit_autosquash_multi_encoding () {
              git rev-list HEAD >actual &&
              test_line_count = 3 actual &&
              iconv -f $old -t UTF-8 "$TEST_DIRECTORY"/t3900/$msg >expect &&
-             if test $flag = squash; then
-                     subject="$(head -1 expect)" &&
-                     printf "\nsquash! %s\n" "$subject" >>expect
-             fi &&
              git cat-file commit HEAD^ >raw &&
              (sed "1,/^$/d" raw | iconv -f $new -t utf-8) >actual &&
              test_cmp expect actual

Quoted 3 lines · Michael Rappazzo via GitGitGadget

Since this change what the expected post-rebase commit comment would look like, related test expectations are adjusted to reflect the the new expectation. A new test is added for the new expectation.

Doesn't that mean automated tools people may have written require similar adjustment to continue working correctly if this change is applied?

Can you tell us more about your expected use case? I am imagining that most people use the log messages from both/all commits being squashed when manually editing to perfect the final log message (as opposed to mechanically processing the concatenated message), so it shouldn't matter if the squash! title is untouched or commented out to them, and those (probably minority) who are mechanical processing will be hurt with this change, so I do not quite see the point of this patch.

Quoted 8 lines

Can you tell us more about your expected use case? I am imagining that most people use the log messages from both/all commits being squashed when manually editing to perfect the final log message (as opposed to mechanically processing the concatenated message), so it shouldn't matter if the squash! title is untouched or commented out to them, and those (probably minority) who are mechanical processing will be hurt with this change, so I do not quite see the point of this patch.

This change isn't removing the subject line from the commit message during the edit phase, it is only commenting it out. With the subject being commented out, it can minimize the effort to edit during the squash.

Furthermore, it can help to eliminate accidental inclusion in the final message. Ultimately, the accidental inclusion is my motivation for submitting this.

This change isn't removing the subject line from the commit message during the edit phase, it is only commenting it out. With the subject being commented out, it can minimize the effort to edit during the squash.

Furthermore, it can help to eliminate accidental inclusion in the final message. Ultimately, the accidental inclusion is my motivation for submitting this.

Quoted 11 lines · Junio C Hamano

Quoted 3 lines · Michael Rappazzo via GitGitGadget

Since this change what the expected post-rebase commit comment would look like, related test expectations are adjusted to reflect the the new expectation. A new test is added for the new expectation.

Doesn't that mean automated tools people may have written require similar adjustment to continue working correctly if this change is applied?

Quoted 8 lines

Can you tell us more about your expected use case? I am imagining that most people use the log messages from both/all commits being squashed when manually editing to perfect the final log message (as opposed to mechanically processing the concatenated message), so it shouldn't matter if the squash! title is untouched or commented out to them, and those (probably minority) who are mechanical processing will be hurt with this change, so I do not quite see the point of this patch.

This change isn't removing the subject line from the commit message during the edit phase, it is only commenting it out. With the subject being commented out, it can minimize the effort to edit during the squash.

Furthermore, it can help to eliminate accidental inclusion in the final message. Ultimately, the accidental inclusion is my motivation for submitting this.

But I thought that was the point of "squash" versus "fixup"? One includes the commit message, and the other does not.

I do think "commit --squash" is mostly useless for that reason, and I suspect we could do a better job in the documentation about pushing people to "--fixup".

But --squash _can_ be useful with other options to populate the commit message (e.g., "--edit", which just pre-populates the subject with the right "squash!" line but lets you otherwise write a normal commit message). If that's the workflow you're using, then I'm sympathetic to auto-removing just a "squash!" line, as it's automated garbage that is only meant as a signal for --autosquash.

Quoted 7 lines · Jeff King

But I thought that was the point of "squash" versus "fixup"? One includes the commit message, and the other does not.

I do think "commit --squash" is mostly useless for that reason, and I suspect we could do a better job in the documentation about pushing people to "--fixup".

Quoted 6 lines

But --squash _can_ be useful with other options to populate the commit message (e.g., "--edit", which just pre-populates the subject with the right "squash!" line but lets you otherwise write a normal commit message). If that's the workflow you're using, then I'm sympathetic to auto-removing just a "squash!" line, as it's automated garbage that is only meant as a signal for --autosquash.

It's a signal for --autosquash and it gives a visual signal to humans of where the squashed commit came from.

--squash already implies --edit, supporting this kind of workflow.

If we could turn back time and start over, would we want something like the following?

1. if someone leaves the squash! message as is, include it as is in the commit message without commenting out

2. if someone edits the squash! commit message to include a body describing what is being squashed in, include the squash! line as part of the commented marker

3. if someone leaves the (uncommented) squash! message in after being presented with an editor at --autosquash time, reopen the editor with some text verifying they really meant to do that

It's rare that concatenated commit messages make sense to be used as is, especially when trailers (sign-offs, Fixes, etc) are involved. I suspect that (3) is more important than (2) here --- we're using the same space in the editor for input and output, and the result is a kind of error-prone process of getting the output right.

Since we can't turn back time, one possibility would be to make tools like "git show --check" notice the squash! lines. Would that be useful?

I can see the argument that this makes it a little harder for mechanical processing across versions, but I suspect most of that looks something like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be affected.

We do make occasional slightly incompatible changes across versions in order to improve user experience, and I think a lot of folks who use squash commits will find this a pleasant improvement.

Quoted 4 lines · brian m. carlson

I can see the argument that this makes it a little harder for mechanical processing across versions, but I suspect most of that looks something like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be affected.

With the left-anchoring, the search pattern will no longer find that line if "squash!" is commented out, but people tend to be sloppy and do not anchor the pattern would not notice the difference. Perhaps the downside may not be too severe? I dunno.

Quoted 11 lines · Junio C Hamano

Quoted 4 lines · brian m. carlson

I can see the argument that this makes it a little harder for mechanical processing across versions, but I suspect most of that looks something like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be affected.

With the left-anchoring, the search pattern will no longer find that line if "squash!" is commented out, but people tend to be sloppy and do not anchor the pattern would not notice the difference. Perhaps the downside may not be too severe? I dunno.

Sorry, I was perhaps bad at explaining this. In my example, it would no longer remove that line, but the user wouldn't care, because it would be commented out and removed automatically. So while the code wouldn't work, what the user wanted would be done by Git automatically.

Quoted 11 lines · Junio C Hamano

Quoted 4 lines · brian m. carlson

I can see the argument that this makes it a little harder for mechanical processing across versions, but I suspect most of that looks something like "sed -i -e '/^squash! /d' COMMIT_EDITMSG" and it probably won't be affected.

With the left-anchoring, the search pattern will no longer find that line if "squash!" is commented out, but people tend to be sloppy and do not anchor the pattern would not notice the difference. Perhaps the downside may not be too severe? I dunno.

Sorry, I was perhaps bad at explaining this. In my example, it would no longer remove that line, but the user wouldn't care, because it would be commented out and removed automatically. So while the code wouldn't work, what the user wanted would be done by Git automatically.