[PATCH 0/1] sequencer: comment out the 'squash!' line

[PATCH 0/1] sequencer: comment out the 'squash!' line

[Michael Rappazzo via GitGitGadget]

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 rappazzo@gmail.com [rappazzo@gmail.com]

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
-- 
gitgitgadget

[PATCH 1/1] sequencer: comment out the 'squash!' line

[Michael Rappazzo via GitGitGadget]

From: Michael Rappazzo <rappazzo@gmail.com>

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 <rappazzo@gmail.com>
---
 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(-)

diff --git a/sequencer.c b/sequencer.c
index 763ccbbc45..e5602686d7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ae6e55ce79..57d178d431 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -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
 '
 
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 22d218698e..51c5a94aea 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -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
 '
 
+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 tag squash-with-body &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	git log --oneline --format="%s%n%b" >actual-full &&
+	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) &&
+	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' '
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index d277a9f4b7..bfab245eb3 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH 1/1] sequencer: comment out the 'squash!' line

[Phillip Wood]

Hi Michael

On 06/01/2020 16:04, Michael Rappazzo via GitGitGadget wrote:
> From: Michael Rappazzo <rappazzo@gmail.com>
> 
> 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

> 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 <rappazzo@gmail.com>
> ---
>   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(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 763ccbbc45..e5602686d7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ae6e55ce79..57d178d431 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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
>   '
>   
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 22d218698e..51c5a94aea 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -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
>   '
>   
> +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

> +	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 ?

> +	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

> +	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' '
> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index d277a9f4b7..bfab245eb3 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -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
> 

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Junio C Hamano]

"Michael Rappazzo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

Thanks.

>
> Signed-off-by: Michael Rappazzo rappazzo@gmail.com [rappazzo@gmail.com]
>
> 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

Re: [PATCH 1/1] sequencer: comment out the 'squash!' line

[Mike Rappazzo]

On Mon, Jan 6, 2020 at 12:10 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Michael
>
> On 06/01/2020 16:04, Michael Rappazzo via GitGitGadget wrote:
> > From: Michael Rappazzo <rappazzo@gmail.com>
> >
> > 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

That makes sense.  Since your implementation is probably better, it
seems like the
only thing useful left from mine is the test that I added.  I'll look
to resubmit the patch
with your commit.

> > 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 <rappazzo@gmail.com>
> > ---
> >   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(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 763ccbbc45..e5602686d7 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -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);
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index ae6e55ce79..57d178d431 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -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
> >   '
> >
> > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> > index 22d218698e..51c5a94aea 100755
> > --- a/t/t3415-rebase-autosquash.sh
> > +++ b/t/t3415-rebase-autosquash.sh
> > @@ -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
> >   '
> >
> > +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
>
> > +     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 ?

The difference is that %B has the extra blank line.  The check below wouldn't
see the difference, so I guess %B is easier to read.

>
> > +     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
>
> > +     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' '
> > diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> > index d277a9f4b7..bfab245eb3 100755
> > --- a/t/t3900-i18n-commit.sh
> > +++ b/t/t3900-i18n-commit.sh
> > @@ -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
> >

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Mike Rappazzo]

On Mon, Jan 6, 2020 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Rappazzo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.

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.

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Jeff King]

On Mon, Jan 06, 2020 at 02:20:09PM -0500, Mike Rappazzo wrote:

> > 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.

-Peff

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Junio C Hamano]

Mike Rappazzo <rappazzo@gmail.com> writes:

> 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.

Which means existing automation will be broken if they are not
taught to be aware that these subject lines can now be commented out
if their Git is recent enough, which does not sound like a good thing.

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

Yes, but that is why the concatenated messages are given to the
editor to be "edited" by you, to be better than just a
concatenation, right?  When I deal with a "squash" (not "fixup"),
the end result would have a log message for a single commit that
describes the single thing it does, which would not resemble to the
original of any of the squashed message---and removing extra title
lines would be the smallest part of such an edit.  So...

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On 2020-01-06 at 19:20:09, Mike Rappazzo wrote:
> On Mon, Jan 6, 2020 at 12:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Michael Rappazzo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > 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.
> 
> 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.

I think this series would be useful.  I've occasionally included the
"squash!" line in my commit even after I've edited the rest of the
commit message.  It's not super frequent, but it is a hassle to have to
delete it, and it does happen occasionally.  Usually I catch it before I
send out the series for review.

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Jonathan Nieder]

Jeff King wrote:

> 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.

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?

One nice thing about (2) is that it's unlikely to affect scripted use.
Thoughts?

Thanks,
Jonathan

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Jeff King]

On Mon, Jan 06, 2020 at 07:36:39PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > 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.
> 
> It's a signal for --autosquash and it gives a visual signal to humans
> of where the squashed commit came from.

True, but I think any proposal here would continue to include that text
in the human-readable output (I was sloppy to say "auto-remove"; it is
really "auto-comment").

Or do you mean that it's useful in the final, squashed commit? I'd argue
that a normal subject line might be so, but the "squash!" line doesn't
saying anything that's not in the main subject already. It tells you
that there _was_ a squash, but isn't erasing that origin kind of the
point of a squash?

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

Ah, that makes sense. I don't use it myself, so I did a quick test
earlier. But I jumped too quickly to assuming I needed "--edit" (the
"--squash" entry in git-commit(1) talks about being able to use "-m",
which I read too much into).

> 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?

What if (3) issued a warning to stderr insted of re-invoking the editor?
Then "git commit --amend" could be used to fix it, with no change in
behavior.

-Peff

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> 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.

> 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.

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On 2020-01-07 at 16:15:16, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > 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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Johannes Schindelin]

Hi brian,

On Wed, 8 Jan 2020, brian m. carlson wrote:

> On 2020-01-07 at 16:15:16, Junio C Hamano wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> > > 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.

Sounds reasonable to me.

Ciao,
Dscho

Re: [PATCH 0/1] sequencer: comment out the 'squash!' line

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2020-01-07 at 16:15:16, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> 
>> > 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.

I didn't realize that you only care about 'd' there; you're right of
course if you limit the scope of the discussion that way.

I was talking in a more general terms where "^squash!" is used as a
marker that signals the boundary between two original commits and
the processing is done possibly differently on each part.