[RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command

Hi Everyone,

This patch series adds fixup [-C|-c] options to interactive rebase. In
addition to amending the contents of the commit as the `fixup` command
does now, `fixup -C` replaces the commit message of the original commit
which we are fixing up with the message of the fixup commit.
And to edit the fixup commit message before committing, `fixup -c`
command is used instead of `fixup -C`. This convention is similar to
the existing `merge` command in the interactive rebase, that also supports
`-c` and `-C` options with similar meanings.

This patch series also changes `rebase -i --autosquash` to rearrange
commits whose message starts with "amend!" and replaces the pick
command with `fixup -C`. The creation of "amend!" commits will be
rebase -i: add options to fixup command added in another patch series.

This is done in reference to the issue opened by Dscho as here[1] and
further discussed briefly[2]. Also, there is discussion[3] regarding the
implementation of reword! commit. The past patches of Phillip Wood’s
work[4], implements 'reword!' as 'amend!' in git rebase and these patches
uses those as the initial base.
And to avoid the extra command in the interactive rebase, this patch
series instead add options to the current fixup command in interactive
rebase (fixup [-C | -c]) as discussed earlier[5].

Looking forward for the reviews.

Thanks and Regards,
Charvi

P.S - I thanks Christian and Phillip for mentoring in this project and
happy to work in community.

[1] https://github.com/gitgitgadget/git/issues/259
[2] https://public-inbox.org/git/alpine.DEB.2.21.1.1710151754070.40514@virtualbox/
[3] https://lore.kernel.org/git/95cc6fb2-d1bc-11de-febe-c2b5c78a6850@gmail.com/
[4] https://github.com/phillipwood/git/commits/wip/rebase-amend
[5] https://lore.kernel.org/git/29fc2f59-1cca-a3db-5586-bbd7b2e4806d@gmail.com/

Charvi Mendiratta (6):
  sequencer: pass todo_item to do_pick_commit()
  sequencer: use const variable for commit message comments
  rebase -i: add fixup [-C | -c] command
  t3437: test script for fixup [-C|-c] options in interactive rebase
  rebase -i: teach --autosquash to work with amend!
  doc/git-rebase: add documentation for fixup [-C|-c] options

Phillip Wood (3):
  rebase -i: only write fixup-message when it's needed
  sequencer: factor out code to append squash message
  rebase -i: comment out squash!/fixup! subjects from squash message

 Documentation/git-rebase.txt    |  12 +-
 rebase-interactive.c            |   4 +-
 sequencer.c                     | 307 +++++++++++++++++++++++++++-----
 t/lib-rebase.sh                 |   4 +
 t/t3415-rebase-autosquash.sh    |  27 ++-
 t/t3437-rebase-fixup-options.sh | 202 +++++++++++++++++++++
 t/t3437/expected-squash-message |  51 ++++++
 t/t3900-i18n-commit.sh          |   4 -
 8 files changed, 549 insertions(+), 62 deletions(-)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-squash-message

--
2.29.0.rc1

From: Phillip Wood <redacted>

The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
commands, there's no point in writing it for squash commands as it is
immediately deleted.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..f888a7ed3b 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r,
 			return error(_("could not read HEAD's commit message"));
 
 		find_commit_subject(head_message, &body);
-		if (write_message(body, strlen(body),
-				  rebase_path_fixup_msg(), 0)) {
-			unuse_commit_buffer(head_commit, head_message);
-			return error(_("cannot write '%s'"),
-				     rebase_path_fixup_msg());
+		if (command == TODO_FIXUP) {
+			if (write_message(body, strlen(body),
+					  rebase_path_fixup_msg(), 0)) {
+				unuse_commit_buffer(head_commit, head_message);
+				return error(_("cannot write '%s'"),
+					     rebase_path_fixup_msg());
+			}
 		}
 
 		strbuf_addf(&buf, "%c ", comment_line_char);

-- 
2.29.0.rc1

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:

I'm nit-picking here, but would this be clearer instead as:

    if (command == TODO_FIXUP && write_message(...) < 0) {
      unuse_commit_buffer(...);
      // ...
    }

There are two changes there. One is two squash the two if-statements
together, and the latter is to add a check that 'write_message()'
returns an error. This explicit '< 0' checking was discussed recently in
another thread[1], and I think makes the conditional here read more
clearly.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/

On Thu, 14 Jan 2021 at 00:13, Taylor Blau [off-list ref] wrote:

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
commands, there's no point in writing it for squash commands as it is
immediately deleted.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..f888a7ed3b 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r,
                      return error(_("could not read HEAD's commit message"));

              find_commit_subject(head_message, &body);
-             if (write_message(body, strlen(body),
-                               rebase_path_fixup_msg(), 0)) {
-                     unuse_commit_buffer(head_commit, head_message);
-                     return error(_("cannot write '%s'"),
-                                  rebase_path_fixup_msg());
+             if (command == TODO_FIXUP) {
+                     if (write_message(body, strlen(body),
+                                       rebase_path_fixup_msg(), 0)) {
+                             unuse_commit_buffer(head_commit, head_message);
+                             return error(_("cannot write '%s'"),
+                                          rebase_path_fixup_msg());
+                     }

I'm nit-picking here, but would this be clearer instead as:

    if (command == TODO_FIXUP && write_message(...) < 0) {
      unuse_commit_buffer(...);
      // ...
    }

There are two changes there. One is two squash the two if-statements
together, and the latter is to add a check that 'write_message()'
returns an error. This explicit '< 0' checking was discussed recently in
another thread[1], and I think makes the conditional here read more
clearly.

Okay, I got this and will change it.

Thanks and Regards,
Charvi

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/

Hi Taylor and Charvi

On 14/01/2021 08:12, Charvi Mendiratta wrote:

On Thu, 14 Jan 2021 at 00:13, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
commands, there's no point in writing it for squash commands as it is
immediately deleted.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
  sequencer.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..f888a7ed3b 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r,
                       return error(_("could not read HEAD's commit message"));

               find_commit_subject(head_message, &body);
-             if (write_message(body, strlen(body),
-                               rebase_path_fixup_msg(), 0)) {
-                     unuse_commit_buffer(head_commit, head_message);
-                     return error(_("cannot write '%s'"),
-                                  rebase_path_fixup_msg());
+             if (command == TODO_FIXUP) {
+                     if (write_message(body, strlen(body),
+                                       rebase_path_fixup_msg(), 0)) {
+                             unuse_commit_buffer(head_commit, head_message);
+                             return error(_("cannot write '%s'"),
+                                          rebase_path_fixup_msg());
+                     }

I'm nit-picking here, but would this be clearer instead as:

     if (command == TODO_FIXUP && write_message(...) < 0) {
       unuse_commit_buffer(...);
       // ...
     }

There are two changes there. One is two squash the two if-statements
together, and the latter is to add a check that 'write_message()'
returns an error. This explicit '< 0' checking was discussed recently in
another thread[1], and I think makes the conditional here read more
clearly.

I don't feel that strongly but the addition of '< 0' feels like it is 
adding an unrelated change to this commit. It also leaves a code base 
where most callers of `write_message()` do not check the sign of the 
return value but a couple do (there appears to be one that checks the 
sign already and a couple that completely ignore the return value). If 
we want to standardize on always checking the sign of the return value 
of functions when checking for errors even when they never return a 
positive value then I think someone in favor of that change should 
propose a patch to the coding guidelines so it is clear what our policy 
is. When I see a '< 0`' check I tend to think the positive value has a 
non-error meaning.

Best Wishes

Phillip

Okay, I got this and will change it.

Thanks and Regards,
Charvi

quoted

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/

On Thu, 14 Jan 2021 at 16:16, Phillip Wood [off-list ref] wrote:

Hi Taylor and Charvi

On 14/01/2021 08:12, Charvi Mendiratta wrote:

quoted

On Thu, 14 Jan 2021 at 00:13, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

The file "$GIT_DIR/rebase-merge/fixup-message" is only used for fixup
commands, there's no point in writing it for squash commands as it is
immediately deleted.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
  sequencer.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..f888a7ed3b 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1757,11 +1757,13 @@ static int update_squash_messages(struct repository *r,
                       return error(_("could not read HEAD's commit message"));

               find_commit_subject(head_message, &body);
-             if (write_message(body, strlen(body),
-                               rebase_path_fixup_msg(), 0)) {
-                     unuse_commit_buffer(head_commit, head_message);
-                     return error(_("cannot write '%s'"),
-                                  rebase_path_fixup_msg());
+             if (command == TODO_FIXUP) {
+                     if (write_message(body, strlen(body),
+                                       rebase_path_fixup_msg(), 0)) {
+                             unuse_commit_buffer(head_commit, head_message);
+                             return error(_("cannot write '%s'"),
+                                          rebase_path_fixup_msg());
+                     }

I'm nit-picking here, but would this be clearer instead as:

     if (command == TODO_FIXUP && write_message(...) < 0) {
       unuse_commit_buffer(...);
       // ...
     }

There are two changes there. One is two squash the two if-statements
together, and the latter is to add a check that 'write_message()'
returns an error. This explicit '< 0' checking was discussed recently in
another thread[1], and I think makes the conditional here read more
clearly.

I don't feel that strongly but the addition of '< 0' feels like it is
adding an unrelated change to this commit. It also leaves a code base
where most callers of `write_message()` do not check the sign of the
return value but a couple do (there appears to be one that checks the
sign already and a couple that completely ignore the return value). If
we want to standardize on always checking the sign of the return value
of functions when checking for errors even when they never return a
positive value then I think someone in favor of that change should
propose a patch to the coding guidelines so it is clear what our policy
is. When I see a '< 0`' check I tend to think the positive value has a
non-error meaning.

Okay, I looked into the write_message(...) and agree that it does not return
a positive value and only returns non-zero for error case and zero for
success. So, for this patch maybe we can ignore checking '< 0' here and
later add another patch to make this function follow the convention of
"negative is error".

Thanks and Regards,
Charvi

Best Wishes

Phillip

quoted

Okay, I got this and will change it.

Thanks and Regards,
Charvi

quoted

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/

From: Phillip Wood <redacted>

This code is going to grow over the next two commits so move it to
its own function.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index f888a7ed3b..5062976d10 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,6 +1718,17 @@ static int is_pick_or_similar(enum todo_command command)
 	}
 }
 
+static void append_squash_message(struct strbuf *buf, const char *body,
+				  struct replay_opts *opts)
+{
+	unlink(rebase_path_fixup_msg());
+	strbuf_addf(buf, "\n%c ", comment_line_char);
+	strbuf_addf(buf, _("This is the commit message #%d:"),
+		    ++opts->current_fixup_count + 1);
+	strbuf_addstr(buf, "\n\n");
+	strbuf_addstr(buf, body);
+}
+
 static int update_squash_messages(struct repository *r,
 				  enum todo_command command,
 				  struct commit *commit,

@@ -1782,12 +1793,7 @@ static int update_squash_messages(struct repository *r,
 	find_commit_subject(message, &body);
 
 	if (command == TODO_SQUASH) {
-		unlink(rebase_path_fixup_msg());
-		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addf(&buf, _("This is the commit message #%d:"),
-			    ++opts->current_fixup_count + 1);
-		strbuf_addstr(&buf, "\n\n");
-		strbuf_addstr(&buf, body);
+		append_squash_message(&buf, body, opts);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),

-- 
2.29.0.rc1

From: Phillip Wood <redacted>

When squashing commit messages the squash!/fixup! subjects are not of
interest so comment them out to stop them becoming part of the final
message.

This change breaks a bunch of --autosquash tests which rely on the
"squash! <subject>" line appearing in the final commit message. This is
addressed by adding a second line to the commit message of the "squash!
..." commits and testing for that.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c                  | 25 ++++++++++++++++++++++++-
 t/t3415-rebase-autosquash.sh | 27 +++++++++++++--------------
 t/t3900-i18n-commit.sh       |  4 ----
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5062976d10..b050a9a212 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,15 +1718,38 @@ static int is_pick_or_similar(enum todo_command command)
 	}
 }
 
+static size_t subject_length(const char *body)
+{
+	size_t i, len = 0;
+	char c;
+	int blank_line = 1;
+	for (i = 0, c = body[i]; c; c = body[++i]) {
+		if (c == '\n') {
+			if (blank_line)
+				return len;
+			len = i + 1;
+			blank_line = 1;
+		} else if (!isspace(c)) {
+			blank_line = 0;
+		}
+	}
+	return blank_line ? len : i;
+}
+
 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)
 {
+	size_t commented_len = 0;
+
 	unlink(rebase_path_fixup_msg());
+	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+		commented_len = subject_length(body);
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _("This is the commit message #%d:"),
 		    ++opts->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
-	strbuf_addstr(buf, body);
+	strbuf_add_commented_lines(buf, body, commented_len);
+	strbuf_addstr(buf, body + commented_len);
 }
 
 static int update_squash_messages(struct repository *r,

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7bab6000dc..45fe6a227e 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh

@@ -81,8 +81,7 @@ test_auto_squash () {
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! first" &&
-
+	git commit -m "squash! first" -m "extra para for first" &&
 	git tag $1 &&
 	test_tick &&
 	git rebase $2 -i HEAD^^^ &&

@@ -139,7 +138,7 @@ test_expect_success 'auto squash that matches 2 commits' '
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! first" &&
+	git commit -m "squash! first" -m "extra para for first" &&
 	git tag final-multisquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD~4 &&

@@ -192,7 +191,7 @@ test_expect_success 'auto squash that matches a sha1' '
 	git add -u &&
 	test_tick &&
 	oid=$(git rev-parse --short HEAD^) &&
-	git commit -m "squash! $oid" &&
+	git commit -m "squash! $oid" -m "extra para" &&
 	git tag final-shasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&

@@ -203,7 +202,7 @@ test_expect_success 'auto squash that matches a sha1' '
 	git cat-file blob HEAD^:file1 >actual &&
 	test_cmp expect actual &&
 	git cat-file commit HEAD^ >commit &&
-	grep squash commit >actual &&
+	grep "extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 

@@ -213,7 +212,7 @@ test_expect_success 'auto squash that matches longer sha1' '
 	git add -u &&
 	test_tick &&
 	oid=$(git rev-parse --short=11 HEAD^) &&
-	git commit -m "squash! $oid" &&
+	git commit -m "squash! $oid" -m "extra para" &&
 	git tag final-longshasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
 	git cat-file blob HEAD^:file1 >actual &&
 	test_cmp expect actual &&
 	git cat-file commit HEAD^ >commit &&
-	grep squash commit >actual &&
+	grep "extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 

@@ -233,7 +232,7 @@ test_auto_commit_flags () {
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit --$1 first-commit &&
+	git commit --$1 first-commit -m "extra para for first" &&
 	git tag final-commit-$1 &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&

@@ -261,11 +260,11 @@ test_auto_fixup_fixup () {
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "$1! first" &&
+	git commit -m "$1! first" -m "extra para for first" &&
 	echo 2 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "$1! $2! first" &&
+	git commit -m "$1! $2! first" -m "second extra para for first" &&
 	git tag "final-$1-$2" &&
 	test_tick &&
 	(

@@ -326,12 +325,12 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
 	git add -u &&
 	test_tick &&
 	oid=$(git rev-parse --short HEAD^) &&
-	git commit -m "squash! $oid" &&
+	git commit -m "squash! $oid" -m "extra para for first" &&
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
 	subject=$(git log -n 1 --format=%s HEAD~2) &&
-	git commit -m "squash! $subject" &&
+	git commit -m "squash! $subject" -m "second extra para for first" &&
 	git tag final-squash-instFmt &&
 	test_tick &&
 	git rebase --autosquash -i HEAD~4 &&

@@ -342,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
 	git cat-file blob HEAD^:file1 >actual &&
 	test_cmp expect actual &&
 	git cat-file commit HEAD^ >commit &&
-	grep squash commit >actual &&
-	test_line_count = 2 actual
+	grep first commit >actual &&
+	test_line_count = 3 actual
 '
 
 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

-- 
2.29.0.rc1

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character. So
"foo bar\n\nbaz" would return 7, but "foo bar\nbaz" would return 11.

Makes sense. (Apologies for stating the obvious here, I just had to read
this function to myself a couple of times to make sure that I understood
what it was doing.)

 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)
 {
+	size_t commented_len = 0;
+
 	unlink(rebase_path_fixup_msg());
+	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+		commented_len = subject_length(body);
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _("This is the commit message #%d:"),
 		    ++opts->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
-	strbuf_addstr(buf, body);
+	strbuf_add_commented_lines(buf, body, commented_len);
+	strbuf_addstr(buf, body + commented_len);

Very nice; the subject gets commented when it starts with "squash!" or
"fixup!", but the body remains uncommented. Makes sense to me.

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

    -	grep squash commit >actual &&
    +	grep "^# squash" commit >actual &&
    +	grep "extra para" commit >actual &&

Ditto.

Thanks,
Taylor

On Thu, 14 Jan 2021 at 00:31, Taylor Blau [off-list ref] wrote:

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

When squashing commit messages the squash!/fixup! subjects are not of
interest so comment them out to stop them becoming part of the final
message.

This change breaks a bunch of --autosquash tests which rely on the
"squash! <subject>" line appearing in the final commit message. This is
addressed by adding a second line to the commit message of the "squash!
..." commits and testing for that.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c                  | 25 ++++++++++++++++++++++++-
 t/t3415-rebase-autosquash.sh | 27 +++++++++++++--------------
 t/t3900-i18n-commit.sh       |  4 ----
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5062976d10..b050a9a212 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,15 +1718,38 @@ static int is_pick_or_similar(enum todo_command command)
      }
 }

+static size_t subject_length(const char *body)
+{
+     size_t i, len = 0;
+     char c;
+     int blank_line = 1;
+     for (i = 0, c = body[i]; c; c = body[++i]) {
+             if (c == '\n') {
+                     if (blank_line)
+                             return len;
+                     len = i + 1;
+                     blank_line = 1;
+             } else if (!isspace(c)) {
+                     blank_line = 0;
+             }
+     }
+     return blank_line ? len : i;
+}
+

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character. So
"foo bar\n\nbaz" would return 7, but "foo bar\nbaz" would return 11.

Makes sense. (Apologies for stating the obvious here, I just had to read
this function to myself a couple of times to make sure that I understood
what it was doing.)

Earlier while testing patch, I also went through in the same way and
now got confirmed as you described here.

quoted

 static void append_squash_message(struct strbuf *buf, const char *body,
                                struct replay_opts *opts)
 {
+     size_t commented_len = 0;
+
      unlink(rebase_path_fixup_msg());
+     if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+             commented_len = subject_length(body);
      strbuf_addf(buf, "\n%c ", comment_line_char);
      strbuf_addf(buf, _("This is the commit message #%d:"),
                  ++opts->current_fixup_count + 1);
      strbuf_addstr(buf, "\n\n");
-     strbuf_addstr(buf, body);
+     strbuf_add_commented_lines(buf, body, commented_len);
+     strbuf_addstr(buf, body + commented_len);

Very nice; the subject gets commented when it starts with "squash!" or
"fixup!", but the body remains uncommented. Makes sense to me.

I agree and Thanks to Phillip, for the patch.

quoted

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
      git cat-file blob HEAD^:file1 >actual &&
      test_cmp expect actual &&
      git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
+     grep "extra para" commit >actual &&
      test_line_count = 1 actual
 '

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

    -   grep squash commit >actual &&
    +   grep "^# squash" commit >actual &&
    +   grep "extra para" commit >actual &&

quoted

@@ -342,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
      git cat-file blob HEAD^:file1 >actual &&
      test_cmp expect actual &&
      git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
-     test_line_count = 2 actual
+     grep first commit >actual &&
+     test_line_count = 3 actual
 '

Ditto.

Okay, I will add it .

Thanks and Regards,
Charvi

Thanks,
Taylor

Hi Charvi and Taylor

On 14/01/2021 08:27, Charvi Mendiratta wrote:

On Thu, 14 Jan 2021 at 00:31, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

When squashing commit messages the squash!/fixup! subjects are not of
interest so comment them out to stop them becoming part of the final
message.

This change breaks a bunch of --autosquash tests which rely on the
"squash! <subject>" line appearing in the final commit message. This is
addressed by adding a second line to the commit message of the "squash!
..." commits and testing for that.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
  sequencer.c                  | 25 ++++++++++++++++++++++++-
  t/t3415-rebase-autosquash.sh | 27 +++++++++++++--------------
  t/t3900-i18n-commit.sh       |  4 ----
  3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5062976d10..b050a9a212 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,15 +1718,38 @@ static int is_pick_or_similar(enum todo_command command)
       }
  }

+static size_t subject_length(const char *body)
+{
+     size_t i, len = 0;
+     char c;
+     int blank_line = 1;
+     for (i = 0, c = body[i]; c; c = body[++i]) {
+             if (c == '\n') {
+                     if (blank_line)
+                             return len;
+                     len = i + 1;
+                     blank_line = 1;
+             } else if (!isspace(c)) {
+                     blank_line = 0;
+             }
+     }
+     return blank_line ? len : i;
+}
+

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character.

The length of the subject is the run of characters before a line 
containing only whitespace, "hello\n there" would return 13 "hello\n 
\nthere" would return 5. Looking again at my code there must be a way of 
writing that function that is easier to follow.

quoted

So
"foo bar\n\nbaz" would return 7, but "foo bar\nbaz" would return 11.

Makes sense. (Apologies for stating the obvious here, I just had to read
this function to myself a couple of times to make sure that I understood
what it was doing.)

Earlier while testing patch, I also went through in the same way and
now got confirmed as you described here.

quoted

quoted

  static void append_squash_message(struct strbuf *buf, const char *body,
                                 struct replay_opts *opts)
  {
+     size_t commented_len = 0;
+
       unlink(rebase_path_fixup_msg());
+     if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+             commented_len = subject_length(body);
       strbuf_addf(buf, "\n%c ", comment_line_char);
       strbuf_addf(buf, _("This is the commit message #%d:"),
                   ++opts->current_fixup_count + 1);
       strbuf_addstr(buf, "\n\n");
-     strbuf_addstr(buf, body);
+     strbuf_add_commented_lines(buf, body, commented_len);
+     strbuf_addstr(buf, body + commented_len);

Very nice; the subject gets commented when it starts with "squash!" or
"fixup!", but the body remains uncommented. Makes sense to me.

I agree and Thanks to Phillip, for the patch.

quoted

quoted

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
       git cat-file blob HEAD^:file1 >actual &&
       test_cmp expect actual &&
       git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
+     grep "extra para" commit >actual &&
       test_line_count = 1 actual
  '

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

     -   grep squash commit >actual &&
     +   grep "^# squash" commit >actual &&
     +   grep "extra para" commit >actual &&

This test is checking the message that gets committed, not the contents 
of the file passed to the editor. I like the idea of checking that the 
squash! line is indeed commented out, but we'd need to test it with

grep -v squash

Looking at the changes to the tests in this commit it highlights the 
fact that I don't think we ever check exactly what the user sees in 
their editor. We do add such a test for the new `fixup -C` functionality 
in a later patch but perhaps we should improve the test coverage of the 
squash message presented to the user before then.

Best Wishes

Phillip

quoted

quoted

@@ -342,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
       git cat-file blob HEAD^:file1 >actual &&
       test_cmp expect actual &&
       git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
-     test_line_count = 2 actual
+     grep first commit >actual &&
+     test_line_count = 3 actual
  '

Ditto.

Okay, I will add it .

Thanks and Regards,
Charvi

quoted

Thanks,
Taylor

Hi Phillip,

On Thu, 14 Jan 2021 at 15:59, Phillip Wood [off-list ref] wrote:

Hi Charvi and Taylor

On 14/01/2021 08:27, Charvi Mendiratta wrote:

quoted

On Thu, 14 Jan 2021 at 00:31, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

When squashing commit messages the squash!/fixup! subjects are not of
interest so comment them out to stop them becoming part of the final
message.

This change breaks a bunch of --autosquash tests which rely on the
"squash! <subject>" line appearing in the final commit message. This is
addressed by adding a second line to the commit message of the "squash!
..." commits and testing for that.

Signed-off-by: Phillip Wood <redacted>
Signed-off-by: Charvi Mendiratta <redacted>
---
  sequencer.c                  | 25 ++++++++++++++++++++++++-
  t/t3415-rebase-autosquash.sh | 27 +++++++++++++--------------
  t/t3900-i18n-commit.sh       |  4 ----
  3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5062976d10..b050a9a212 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,15 +1718,38 @@ static int is_pick_or_similar(enum todo_command command)
       }
  }

+static size_t subject_length(const char *body)
+{
+     size_t i, len = 0;
+     char c;
+     int blank_line = 1;
+     for (i = 0, c = body[i]; c; c = body[++i]) {
+             if (c == '\n') {
+                     if (blank_line)
+                             return len;
+                     len = i + 1;
+                     blank_line = 1;
+             } else if (!isspace(c)) {
+                     blank_line = 0;
+             }
+     }
+     return blank_line ? len : i;
+}
+

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character.

The length of the subject is the run of characters before a line
containing only whitespace, "hello\n there" would return 13 "hello\n
\nthere" would return 5. Looking again at my code there must be a way of
writing that function that is easier to follow.

Okay. I will look into it once, thanks for explaining.

quoted

quoted

So
"foo bar\n\nbaz" would return 7, but "foo bar\nbaz" would return 11.

Makes sense. (Apologies for stating the obvious here, I just had to read
this function to myself a couple of times to make sure that I understood
what it was doing.)

Earlier while testing patch, I also went through in the same way and
now got confirmed as you described here.

quoted

quoted

  static void append_squash_message(struct strbuf *buf, const char *body,
                                 struct replay_opts *opts)
  {
+     size_t commented_len = 0;
+
       unlink(rebase_path_fixup_msg());
+     if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+             commented_len = subject_length(body);
       strbuf_addf(buf, "\n%c ", comment_line_char);
       strbuf_addf(buf, _("This is the commit message #%d:"),
                   ++opts->current_fixup_count + 1);
       strbuf_addstr(buf, "\n\n");
-     strbuf_addstr(buf, body);
+     strbuf_add_commented_lines(buf, body, commented_len);
+     strbuf_addstr(buf, body + commented_len);

Very nice; the subject gets commented when it starts with "squash!" or
"fixup!", but the body remains uncommented. Makes sense to me.

I agree and Thanks to Phillip, for the patch.

quoted

quoted

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
       git cat-file blob HEAD^:file1 >actual &&
       test_cmp expect actual &&
       git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
+     grep "extra para" commit >actual &&
       test_line_count = 1 actual
  '

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

     -   grep squash commit >actual &&
     +   grep "^# squash" commit >actual &&
     +   grep "extra para" commit >actual &&

This test is checking the message that gets committed, not the contents
of the file passed to the editor. I like the idea of checking that the
squash! line is indeed commented out, but we'd need to test it with

grep -v squash

Looking at the changes to the tests in this commit it highlights the
fact that I don't think we ever check exactly what the user sees in
their editor. We do add such a test for the new `fixup -C` functionality
in a later patch but perhaps we should improve the test coverage of the
squash message presented to the user before then.

Okay and for checking "squash" I will add it in the above way.

Thanks and Regards,
Charvi

Best Wishes

Phillip

quoted

quoted

quoted

@@ -342,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
       git cat-file blob HEAD^:file1 >actual &&
       test_cmp expect actual &&
       git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
-     test_line_count = 2 actual
+     grep first commit >actual &&
+     test_line_count = 3 actual
  '

Ditto.

Okay, I will add it .

Thanks and Regards,
Charvi

quoted

Thanks,
Taylor

Hi Charvi,

On Fri, Jan 15, 2021 at 9:35 AM Charvi Mendiratta [off-list ref] wrote:

On Thu, 14 Jan 2021 at 15:59, Phillip Wood [off-list ref] wrote:

quoted

On 14/01/2021 08:27, Charvi Mendiratta wrote:

quoted

On Thu, 14 Jan 2021 at 00:31, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

quoted

quoted

quoted

quoted

+static size_t subject_length(const char *body)
+{
+     size_t i, len = 0;
+     char c;
+     int blank_line = 1;
+     for (i = 0, c = body[i]; c; c = body[++i]) {
+             if (c == '\n') {
+                     if (blank_line)
+                             return len;
+                     len = i + 1;
+                     blank_line = 1;
+             } else if (!isspace(c)) {
+                     blank_line = 0;
+             }
+     }
+     return blank_line ? len : i;
+}

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character.

The length of the subject is the run of characters before a line
containing only whitespace, "hello\n there" would return 13 "hello\n
\nthere" would return 5. Looking again at my code there must be a way of
writing that function that is easier to follow.

Okay. I will look into it once, thanks for explaining.

You may want to look at other places in the code where we deal with
the subject. For example there is find_commit_subject() in commit.c
and find_trailer_start() in trailer.c that might have interesting code
for our purpose.

Best,
Christian.

Hi Christian,

On Fri, 15 Jan 2021 at 14:15, Christian Couder
[off-list ref] wrote:

Hi Charvi,

On Fri, Jan 15, 2021 at 9:35 AM Charvi Mendiratta [off-list ref] wrote:

quoted

On Thu, 14 Jan 2021 at 15:59, Phillip Wood [off-list ref] wrote:

quoted

quoted

On 14/01/2021 08:27, Charvi Mendiratta wrote:

quoted

On Thu, 14 Jan 2021 at 00:31, Taylor Blau [off-list ref] wrote:

quoted

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:

quoted

From: Phillip Wood <redacted>

quoted

quoted

quoted

quoted

quoted

+static size_t subject_length(const char *body)
+{
+     size_t i, len = 0;
+     char c;
+     int blank_line = 1;
+     for (i = 0, c = body[i]; c; c = body[++i]) {
+             if (c == '\n') {
+                     if (blank_line)
+                             return len;
+                     len = i + 1;
+                     blank_line = 1;
+             } else if (!isspace(c)) {
+                     blank_line = 0;
+             }
+     }
+     return blank_line ? len : i;
+}

OK, so this gets the length of the subject in "body", which is defined
as the run of characters before a newline and then a space character.

The length of the subject is the run of characters before a line
containing only whitespace, "hello\n there" would return 13 "hello\n
\nthere" would return 5. Looking again at my code there must be a way of
writing that function that is easier to follow.

Okay. I will look into it once, thanks for explaining.

You may want to look at other places in the code where we deal with
the subject. For example there is find_commit_subject() in commit.c
and find_trailer_start() in trailer.c that might have interesting code
for our purpose.

Thanks for the pointer, I will look at it.

Thanks and Regards,
Charvi

Best,
Christian.

Hi Phillip and Taylor,

[...]

quoted

quoted

quoted

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
       git cat-file blob HEAD^:file1 >actual &&
       test_cmp expect actual &&
       git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
+     grep "extra para" commit >actual &&
       test_line_count = 1 actual
  '

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

     -   grep squash commit >actual &&
     +   grep "^# squash" commit >actual &&
     +   grep "extra para" commit >actual &&

This test is checking the message that gets committed, not the contents
of the file passed to the editor. I like the idea of checking that the
squash! line is indeed commented out, but we'd need to test it with

grep -v squash

It seems to me that you suggest to use 'grep -v squash' instead of
grep "^#squash". So I added to check the test as here:

             -   grep squash commit >actual &&
             +   grep -v "squash" commit >actual &&
             +   grep "extra para" commit >actual &&

Looking at the changes to the tests in this commit it highlights the
fact that I don't think we ever check exactly what the user sees in
their editor. We do add such a test for the new `fixup -C` functionality
in a later patch but perhaps we should improve the test coverage of the
squash message presented to the user before then.

I agree and in this test  it's now just checking if the commit message body of
"squash!" i.e  line "extra para", is added in commit message or not. So, I am
doubtful if the above is the right way to test whether squash! line is commented
out or not , as "grep "extra para" commit >actual &&" will rewrite the
'actual' file.

Thanks and Regards,
Charvi

Hi Charvi

On 17/01/2021 03:39, Charvi Mendiratta wrote:

Hi Phillip and Taylor,

quoted

[...]

quoted

quoted

quoted

@@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
        git cat-file blob HEAD^:file1 >actual &&
        test_cmp expect actual &&
        git cat-file commit HEAD^ >commit &&
-     grep squash commit >actual &&
+     grep "extra para" commit >actual &&
        test_line_count = 1 actual
   '

Worth checking that "squash" doesn't appear in an uncommented part of
actual? Or better yet, checking that "# squash ..." _does_ appear.

I.e., that we would leave this as:

      -   grep squash commit >actual &&
      +   grep "^# squash" commit >actual &&
      +   grep "extra para" commit >actual &&

This test is checking the message that gets committed, not the contents
of the file passed to the editor. I like the idea of checking that the
squash! line is indeed commented out, but we'd need to test it with

grep -v squash

It seems to me that you suggest to use 'grep -v squash' instead of
grep "^#squash".

That's correct

So I added to check the test as here:

              -   grep squash commit >actual &&
              +   grep -v "squash" commit >actual &&

There is no need to redirect the output of this one - we don't expect 
any output and the test will fail if there is so we want to see what the 
output is.

              +   grep "extra para" commit >actual &&

quoted

Looking at the changes to the tests in this commit it highlights the
fact that I don't think we ever check exactly what the user sees in
their editor. We do add such a test for the new `fixup -C` functionality
in a later patch but perhaps we should improve the test coverage of the
squash message presented to the user before then.

I agree and in this test  it's now just checking if the commit message body of
"squash!" i.e  line "extra para", is added in commit message or not. So, I am
doubtful if the above is the right way to test whether squash! line is commented
out or not , as "grep "extra para" commit >actual &&" will rewrite the
'actual' file.

The test above gives us reassurance that "squash! ..." does not make it 
into the commit message which is important. We'd want a separate test to 
check the message that is presented to the user however looking at patch 
7 the test 'sequence of fixup, fixup -C & squash --signoff works' checks 
that a "squash! ..." subject line gets commented out so I wouldn't worry 
about an additional test here

Best Wishes

Phillip

Thanks and Regards,
Charvi

Hi Phillip,

On Mon, 18 Jan 2021 at 23:59, Phillip Wood [off-list ref] wrote:

[...]

quoted

So I added to check the test as here:

              -   grep squash commit >actual &&
              +   grep -v "squash" commit >actual &&

There is no need to redirect the output of this one - we don't expect
any output and the test will fail if there is so we want to see what the
output is.

Okay,

quoted

              +   grep "extra para" commit >actual &&

quoted

Looking at the changes to the tests in this commit it highlights the
fact that I don't think we ever check exactly what the user sees in
their editor. We do add such a test for the new `fixup -C` functionality
in a later patch but perhaps we should improve the test coverage of the
squash message presented to the user before then.

I agree and in this test  it's now just checking if the commit message body of
"squash!" i.e  line "extra para", is added in commit message or not. So, I am
doubtful if the above is the right way to test whether squash! line is commented
out or not , as "grep "extra para" commit >actual &&" will rewrite the
'actual' file.

The test above gives us reassurance that "squash! ..." does not make it
into the commit message which is important. We'd want a separate test to
check the message that is presented to the user however looking at patch
7 the test 'sequence of fixup, fixup -C & squash --signoff works' checks
that a "squash! ..." subject line gets commented out so I wouldn't worry
about an additional test here

I agree, thanks for confirming.

Thanks and Regards,
Charvi

As an additional member of the structure todo_item will be required in
future commits pass the complete structure.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index b050a9a212..cdafc2e0e8 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1884,8 +1884,7 @@ static void record_in_rewritten(struct object_id *oid,
 }
 
 static int do_pick_commit(struct repository *r,
-			  enum todo_command command,
-			  struct commit *commit,
+			  struct todo_item *item,
 			  struct replay_opts *opts,
 			  int final_fixup, int *check_todo)
 {

@@ -1898,6 +1897,8 @@ static int do_pick_commit(struct repository *r,
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res, unborn = 0, reword = 0, allow, drop_commit;
+	enum todo_command command = item->command;
+	struct commit *commit = item->commit;
 
 	if (opts->no_commit) {
 		/*

@@ -4147,8 +4148,8 @@ static int pick_commits(struct repository *r,
 				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
 					command_to_string(item->command), NULL),
 					1);
-			res = do_pick_commit(r, item->command, item->commit,
-					     opts, is_final_fixup(todo_list),
+			res = do_pick_commit(r, item, opts,
+					     is_final_fixup(todo_list),
 					     &check_todo);
 			if (is_rebase_i(opts))
 				setenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);

@@ -4610,11 +4611,14 @@ static int single_pick(struct repository *r,
 		       struct replay_opts *opts)
 {
 	int check_todo;
+	struct todo_item item;
+
+	item.command = opts->action == REPLAY_PICK ?
+			TODO_PICK : TODO_REVERT;
+	item.commit = cmit;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	return do_pick_commit(r, opts->action == REPLAY_PICK ?
-			      TODO_PICK : TODO_REVERT, cmit, opts, 0,
-			      &check_todo);
+	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }
 
 int sequencer_pick_revisions(struct repository *r,

-- 
2.29.0.rc1

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cdafc2e0e8..b9295b5a02 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1736,6 +1736,15 @@ static size_t subject_length(const char *body)
 	return blank_line ? len : i;
 }
 
+static const char first_commit_msg_str[] =
+N_("This is the 1st commit message:");
+static const char nth_commit_msg_fmt[] =
+N_("This is the commit message #%d:");
+static const char skip_nth_commit_msg_fmt[] =
+N_("The commit message #%d will be skipped:");
+static const char combined_commit_msg_str[] =
+N_("This is a combination of %d commits.");
+
 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)
 {

@@ -1745,7 +1754,7 @@ static void append_squash_message(struct strbuf *buf, const char *body,
 	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
 		commented_len = subject_length(body);
 	strbuf_addf(buf, "\n%c ", comment_line_char);
-	strbuf_addf(buf, _("This is the commit message #%d:"),
+	strbuf_addf(buf, _(nth_commit_msg_fmt),
 		    ++opts->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
 	strbuf_add_commented_lines(buf, body, commented_len);

@@ -1774,7 +1783,7 @@ static int update_squash_messages(struct repository *r,
 			buf.buf : strchrnul(buf.buf, '\n');
 
 		strbuf_addf(&header, "%c ", comment_line_char);
-		strbuf_addf(&header, _("This is a combination of %d commits."),
+		strbuf_addf(&header, _(combined_commit_msg_str),
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);

@@ -1801,9 +1810,9 @@ static int update_squash_messages(struct repository *r,
 		}
 
 		strbuf_addf(&buf, "%c ", comment_line_char);
-		strbuf_addf(&buf, _("This is a combination of %d commits."), 2);
+		strbuf_addf(&buf, _(combined_commit_msg_str), 2);
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addstr(&buf, _("This is the 1st commit message:"));
+		strbuf_addstr(&buf, _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_addstr(&buf, body);
 

@@ -1819,7 +1828,7 @@ static int update_squash_messages(struct repository *r,
 		append_squash_message(&buf, body, opts);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),
+		strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
 			    ++opts->current_fixup_count + 1);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_add_commented_lines(&buf, body, strlen(body));

-- 
2.29.0.rc1

On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:

Two nit-picks here. The line break after the '=' is a little awkward,
since two of these lines are less than 80 characters combined, and the
other two are just slightly longer than 80 characters. The other nitpick
is that its typical to see 'char *foo' instead of 'char foo[]'.

So, I'd write these as:

    static const char *first_commit_msg_str = N_("This is the 1st commit message:");
    static const char *nth_commit_msg_fmt = N_("This is the commit message #%d:");
    static const char *skip_nth_commit_msg_fmt = N_("The commit message #%d will be skipped:");
    static const char *combined_commit_msg_str = N_("This is a combination of %d commits.");

I also noticed that you suffix these with _fmt or _str depending on
whether or not there are arguments that get filled in later on. That
makes 'combined_commit_msg_str' labeled incorrectly (it should be
'combined_commit_msg_fmt').

I'm curious to see where down the road these messages will be used over
again, but I'm sure that that is coming in subsequent patch(es).

 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)

This and the three below uses are good, since they still translate
these messages. Nice.

Thanks,
Taylor

On Thu, 14 Jan 2021 at 00:44, Taylor Blau [off-list ref] wrote:

On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:

quoted

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 sequencer.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cdafc2e0e8..b9295b5a02 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1736,6 +1736,15 @@ static size_t subject_length(const char *body)
      return blank_line ? len : i;
 }

+static const char first_commit_msg_str[] =
+N_("This is the 1st commit message:");
+static const char nth_commit_msg_fmt[] =
+N_("This is the commit message #%d:");
+static const char skip_nth_commit_msg_fmt[] =
+N_("The commit message #%d will be skipped:");
+static const char combined_commit_msg_str[] =
+N_("This is a combination of %d commits.");
+

Two nit-picks here. The line break after the '=' is a little awkward,
since two of these lines are less than 80 characters combined, and the
other two are just slightly longer than 80 characters. The other nitpick
is that its typical to see 'char *foo' instead of 'char foo[]'.

So, I'd write these as:

    static const char *first_commit_msg_str = N_("This is the 1st commit message:");
    static const char *nth_commit_msg_fmt = N_("This is the commit message #%d:");
    static const char *skip_nth_commit_msg_fmt = N_("The commit message #%d will be skipped:");
    static const char *combined_commit_msg_str = N_("This is a combination of %d commits.");

Okay, I will change it and write in the same line,

Also, I agree with Junio and Christian to use array instead of pointer
here as it will take the extra memory for pointer.

I also noticed that you suffix these with _fmt or _str depending on
whether or not there are arguments that get filled in later on. That
makes 'combined_commit_msg_str' labeled incorrectly (it should be
'combined_commit_msg_fmt').

I admit, I was not aware about the difference of _fmt or _str but now I got
this and will change it.

I'm curious to see where down the road these messages will be used over
again, but I'm sure that that is coming in subsequent patch(es).

 static void append_squash_message(struct strbuf *buf, const char *body,
                                  struct replay_opts *opts)

quoted

 static void append_squash_message(struct strbuf *buf, const char *body,
                                struct replay_opts *opts)
 {

@@ -1745,7 +1754,7 @@ static void append_squash_message(struct strbuf *buf, const char *body,
      if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
              commented_len = subject_length(body);
      strbuf_addf(buf, "\n%c ", comment_line_char);
-     strbuf_addf(buf, _("This is the commit message #%d:"),
+     strbuf_addf(buf, _(nth_commit_msg_fmt),
                  ++opts->current_fixup_count + 1);

This and the three below uses are good, since they still translate
these messages. Nice.

Thanks for the reviews,

Thanks and Regards,
Charvi

Thanks,
Taylor

Add options to `fixup` command to fixup both the commit contents and
message. `fixup -C` command is used to replace the original commit
message and `fixup -c`, additionally allows to edit the commit message.

Original-patch-by: Phillip Wood [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 rebase-interactive.c |   4 +-
 sequencer.c          | 212 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 196 insertions(+), 20 deletions(-)

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 762853bc7e..c3bd02adee 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c

@@ -44,7 +44,9 @@ void append_todo_help(int command_count,
 "r, reword <commit> = use commit, but edit the commit message\n"
 "e, edit <commit> = use commit, but stop for amending\n"
 "s, squash <commit> = use commit, but meld into previous commit\n"
-"f, fixup <commit> = like \"squash\", but discard this commit's log message\n"
+"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n"
+"                   commit's log message. Use -C to replace with this\n"
+"                   commit message or -c to edit the commit message\n"
 "x, exec <command> = run command (the rest of the line) using shell\n"
 "b, break = stop here (continue rebase later with 'git rebase --continue')\n"
 "d, drop <commit> = remove commit\n"

diff --git a/sequencer.c b/sequencer.c
index b9295b5a02..db016717d1 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command)
 	}
 }
 
+enum todo_item_flags {
+	TODO_EDIT_MERGE_MSG    = (1 << 0),
+	TODO_REPLACE_FIXUP_MSG = (1 << 1),
+	TODO_EDIT_FIXUP_MSG    = (1 << 2),
+};
+
 static size_t subject_length(const char *body)
 {
 	size_t i, len = 0;

@@ -1740,34 +1746,177 @@ static const char first_commit_msg_str[] =
 N_("This is the 1st commit message:");
 static const char nth_commit_msg_fmt[] =
 N_("This is the commit message #%d:");
+static const char skip_first_commit_msg_str[] =
+N_("The 1st commit message will be skipped:");
 static const char skip_nth_commit_msg_fmt[] =
 N_("The commit message #%d will be skipped:");
 static const char combined_commit_msg_str[] =
 N_("This is a combination of %d commits.");
 
-static void append_squash_message(struct strbuf *buf, const char *body,
-				  struct replay_opts *opts)
+static int check_fixup_flag(enum todo_command command,
+			    enum todo_item_flags flag)
+{
+	return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) ||
+					 (flag & TODO_EDIT_FIXUP_MSG));
+}
+
+/*
+ * Wrapper around strbuf_add_commented_lines() which avoids double
+ * commenting commit subjects.
+ */
+static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
+{
+	const char *s = str;
+	while (len > 0 && s[0] == comment_line_char) {
+		size_t count;
+		const char *n = memchr(s, '\n', len);
+		if (!n)
+			count = len;
+		else
+			count = n - s + 1;
+		strbuf_add(buf, s, count);
+		s += count;
+		len -= count;
+	}
+	strbuf_add_commented_lines(buf, s, len);
+}
+
+/* Does the current fixup chain contain a squash command? */
+static int seen_squash(struct replay_opts *opts)
 {
-	size_t commented_len = 0;
+	return starts_with(opts->current_fixups.buf, "squash") ||
+		strstr(opts->current_fixups.buf, "\nsquash");
+}
 
-	unlink(rebase_path_fixup_msg());
-	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
+{
+	strbuf_setlen(buf1, 2);
+	strbuf_addf(buf1, nth_commit_msg_fmt, n);
+	strbuf_addch(buf1, '\n');
+	strbuf_setlen(buf2, 2);
+	strbuf_addf(buf2, skip_nth_commit_msg_fmt, n);
+	strbuf_addch(buf2, '\n');
+}
+
+/*
+ * Comment out any un-commented commit messages, updating the message comments
+ * to say they will be skipped but do not comment out the empty lines that
+ * surround commit messages and their comments.
+ */
+static void update_squash_message_for_fixup(struct strbuf *msg)
+{
+	void (*copy_lines)(struct strbuf *, const void *, size_t) = strbuf_add;
+	struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT;
+	const char *s, *start;
+	char *orig_msg;
+	size_t orig_msg_len;
+	int i = 1;
+
+	strbuf_addf(&buf1, "# %s\n", first_commit_msg_str);
+	strbuf_addf(&buf2, "# %s\n", skip_first_commit_msg_str);
+	s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
+	while (s) {
+		const char *next;
+		size_t off;
+		if (skip_prefix(s, buf1.buf, &next)) {
+			/*
+			 * Copy the last message, preserving the blank line
+			 * preceding the current line
+			 */
+			off = (s > start + 1 && s[-2] == '\n') ? 1 : 0;
+			copy_lines(msg, start, s - start - off);
+			if (off)
+				strbuf_addch(msg, '\n');
+			/*
+			 * The next message needs to be commented out but the
+			 * message header is already commented out so just copy
+			 * it and the blank line that follows it.
+			 */
+			strbuf_addbuf(msg, &buf2);
+			if (*next == '\n')
+				strbuf_addch(msg, *next++);
+			start = s = next;
+			copy_lines = add_commented_lines;
+			update_comment_bufs(&buf1, &buf2, ++i);
+		} else if (skip_prefix(s, buf2.buf, &next)) {
+			off = (s > start + 1 && s[-2] == '\n') ? 1 : 0;
+			copy_lines(msg, start, s - start - off);
+			start = s - off;
+			s = next;
+			copy_lines = strbuf_add;
+			update_comment_bufs(&buf1, &buf2, ++i);
+		} else {
+			s = strchr(s, '\n');
+			if (s)
+				s++;
+		}
+	}
+	copy_lines(msg, start, orig_msg_len - (start - orig_msg));
+	free(orig_msg);
+	strbuf_release(&buf1);
+	strbuf_release(&buf2);
+}
+
+static int append_squash_message(struct strbuf *buf, const char *body,
+			 enum todo_command command, struct replay_opts *opts,
+			 enum todo_item_flags flag)
+{
+	const char *fixup_msg;
+	size_t commented_len = 0, fixup_off;
+	/*
+	 * amend is non-interactive and not normally used with fixup!
+	 * or squash! commits, so only comment out those subjects when
+	 * squashing commit messages.
+	 */
+	if (starts_with(body, "amend!") ||
+	    ((command == TODO_SQUASH || seen_squash(opts)) &&
+	     (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
 		commented_len = subject_length(body);
+
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
 		    ++opts->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
 	strbuf_add_commented_lines(buf, body, commented_len);
+	/* buf->buf may be reallocated so store an offset into the buffer */
+	fixup_off = buf->len;
 	strbuf_addstr(buf, body + commented_len);
+
+	/* fixup -C after squash behaves like squash */
+	if (check_fixup_flag(command, flag) && !seen_squash(opts)) {
+		/*
+		 * We're replacing the commit message so we need to
+		 * append the Signed-off-by: trailer if the user
+		 * requested '--signoff'.
+		 */
+		if (opts->signoff)
+			append_signoff(buf, 0, 0);
+
+		if ((command == TODO_FIXUP) &&
+		    (flag & TODO_REPLACE_FIXUP_MSG)) {
+			fixup_msg = skip_blank_lines(buf->buf + fixup_off);
+			if (write_message(fixup_msg, strlen(fixup_msg),
+					rebase_path_fixup_msg(), 0))
+				return error(_("cannot write '%s'"),
+					rebase_path_fixup_msg());
+		} else {
+			unlink(rebase_path_fixup_msg());
+		}
+	} else  {
+		unlink(rebase_path_fixup_msg());
+	}
+
+	return 0;
 }
 
 static int update_squash_messages(struct repository *r,
 				  enum todo_command command,
 				  struct commit *commit,
-				  struct replay_opts *opts)
+				  struct replay_opts *opts,
+				  enum todo_item_flags flag)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int res;
+	int res = 0;
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 

@@ -1787,6 +1936,8 @@ static int update_squash_messages(struct repository *r,
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
+		if (check_fixup_flag(command, flag) && !seen_squash(opts))
+			update_squash_message_for_fixup(&buf);
 	} else {
 		struct object_id head;
 		struct commit *head_commit;

@@ -1800,7 +1951,7 @@ static int update_squash_messages(struct repository *r,
 			return error(_("could not read HEAD's commit message"));
 
 		find_commit_subject(head_message, &body);
-		if (command == TODO_FIXUP) {
+		if (command == TODO_FIXUP && !flag) {
 			if (write_message(body, strlen(body),
 					  rebase_path_fixup_msg(), 0)) {
 				unuse_commit_buffer(head_commit, head_message);

@@ -1808,13 +1959,17 @@ static int update_squash_messages(struct repository *r,
 					     rebase_path_fixup_msg());
 			}
 		}
-
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_str), 2);
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
-		strbuf_addstr(&buf, _(first_commit_msg_str));
+		strbuf_addstr(&buf, check_fixup_flag(command, flag) ?
+			      _(skip_first_commit_msg_str) :
+			      _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
-		strbuf_addstr(&buf, body);
+		if (check_fixup_flag(command, flag))
+			strbuf_add_commented_lines(&buf, body, strlen(body));
+		else
+			strbuf_addstr(&buf, body);
 
 		unuse_commit_buffer(head_commit, head_message);
 	}

@@ -1824,8 +1979,8 @@ static int update_squash_messages(struct repository *r,
 			     oid_to_hex(&commit->object.oid));
 	find_commit_subject(message, &body);
 
-	if (command == TODO_SQUASH) {
-		append_squash_message(&buf, body, opts);
+	if (command == TODO_SQUASH || check_fixup_flag(command, flag)) {
+		res = append_squash_message(&buf, body, command, opts, flag);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),