git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command
@ 2021-01-08  9:23 Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
                   ` (18 more replies)
  0 siblings, 19 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin, Charvi Mendiratta

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2964 bytes --]

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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-13 18:43   ` Taylor Blau
  2021-01-08  9:23 ` [RFC PATCH 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
                   ` (17 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 2/9] sequencer: factor out code to append squash message
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
                   ` (16 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-13 19:01   ` Taylor Blau
  2021-01-08  9:23 ` [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
                   ` (15 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit()
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (2 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
                   ` (14 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

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

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 5/9] sequencer: use const variable for commit message comments
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (3 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-13 19:14   ` Taylor Blau
  2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
                   ` (13 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (4 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-14  9:23   ` Christian Couder
  2021-01-08  9:23 ` [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
                   ` (12 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

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 <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 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),
@@ -1836,7 +1991,9 @@ static int update_squash_messages(struct repository *r,
 		return error(_("unknown command: %d"), command);
 	unuse_commit_buffer(commit, message);
 
-	res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
+	if (!res)
+		res = write_message(buf.buf, buf.len, rebase_path_squash_msg(),
+				    0);
 	strbuf_release(&buf);
 
 	if (!res) {
@@ -2037,7 +2194,8 @@ static int do_pick_commit(struct repository *r,
 	if (command == TODO_REWORD)
 		reword = 1;
 	else if (is_fixup(command)) {
-		if (update_squash_messages(r, command, commit, opts))
+		if (update_squash_messages(r, command, commit,
+					   opts, item->flags))
 			return -1;
 		flags |= AMEND_MSG;
 		if (!final_fixup)
@@ -2202,10 +2360,6 @@ static int read_and_refresh_cache(struct repository *r,
 	return 0;
 }
 
-enum todo_item_flags {
-	TODO_EDIT_MERGE_MSG = 1
-};
-
 void todo_list_release(struct todo_list *todo_list)
 {
 	strbuf_release(&todo_list->buf);
@@ -2292,6 +2446,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		return 0;
 	}
 
+	if (item->command == TODO_FIXUP) {
+		if (skip_prefix(bol, "-C", &bol) &&
+		   (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_REPLACE_FIXUP_MSG;
+		} else if (skip_prefix(bol, "-c", &bol) &&
+				  (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_EDIT_FIXUP_MSG;
+		}
+	}
+
 	if (item->command == TODO_MERGE) {
 		if (skip_prefix(bol, "-C", &bol))
 			bol += strspn(bol, " \t");
@@ -5298,6 +5464,14 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 					  short_commit_name(item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
+			if (item->command == TODO_FIXUP) {
+				if (item->flags & TODO_EDIT_FIXUP_MSG)
+					strbuf_addstr(buf, " -c");
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+					strbuf_addstr(buf, " -C");
+				}
+			}
+
 			if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (5 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
                   ` (11 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh                 |   4 +
 t/t3437-rebase-fixup-options.sh | 190 ++++++++++++++++++++++++++++++++
 t/t3437/expected-squash-message |  51 +++++++++
 3 files changed, 245 insertions(+)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-squash-message

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index b72c051f47..e10e38060b 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -4,6 +4,7 @@
 #
 # - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
+# - copy the original commit message to a file with $FAKE_MESSAGE_COPY
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
 # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -33,6 +34,7 @@ set_fake_editor () {
 			exit
 		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
 		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
+		test -z "$FAKE_MESSAGE_COPY" || cat "$1" >"$FAKE_MESSAGE_COPY"
 		exit
 		;;
 	esac
@@ -51,6 +53,8 @@ set_fake_editor () {
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
+		merge_*|fixup_*)
+			action=$(echo "$line" | sed 's/_/ /g');;
 		"#")
 			echo '# comment' >> "$1";;
 		">")
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
new file mode 100755
index 0000000000..9e62d74249
--- /dev/null
+++ b/t/t3437-rebase-fixup-options.sh
@@ -0,0 +1,190 @@
+#!/bin/sh
+#
+# Copyright (c) 2018 Phillip Wood
+#
+
+test_description='git rebase interactive fixup options
+
+This test checks the "fixup [-C|-c]" command of rebase interactive.
+In addition to amending the contents of the commit, "fixup -C"
+replaces the original commit message with the message of the fixup
+commit. "fixup -c" also replaces the original message, but opens the
+editor to allow the user to edit the message before committing.
+'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+EMPTY=""
+
+test_commit_message () {
+	rev="$1" && # commit or tag we want to test
+	file="$2" && # test against the content of a file
+	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
+	if test "$2" = -m
+	then
+		str="$3" && # test against a string
+		printf "%s\n" "$str" >tmp-expected-message &&
+		file="tmp-expected-message"
+	fi
+	test_cmp "$file" actual-message
+}
+
+get_author () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%an %ae" "$rev"
+}
+
+test_expect_success 'setup' '
+	cat >message <<-EOF &&
+		amend! B
+		${EMPTY}
+		new subject
+		${EMPTY}
+		new
+		body
+		EOF
+
+	sed "1,2d" message >expected-message &&
+
+	test_commit A A &&
+	test_commit B B &&
+	get_author HEAD >expected-author &&
+	ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" &&
+	ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" &&
+	GIT_AUTHOR_NAME="Amend Author" &&
+	GIT_AUTHOR_EMAIL="amend@example.com" &&
+	test_commit "$(cat message)" A A1 A1 &&
+	test_commit A2 A &&
+	test_commit A3 A &&
+	GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" &&
+	GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" &&
+	git checkout -b conflicts-branch A &&
+	test_commit conflicts A &&
+
+	set_fake_editor &&
+	git checkout -b branch B &&
+	echo B1 >B &&
+	test_tick &&
+	git commit --fixup=HEAD -a &&
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		EOF
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		EOF
+	echo B2 >B &&
+	test_tick &&
+	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
+	echo B3 >B &&
+	test_tick &&
+	git commit -a -F - <<-EOF &&
+		amend! amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		${EMPTY}
+		edited 3
+		EOF
+
+	GIT_AUTHOR_NAME="Rebase Author" &&
+	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
+	GIT_COMMITTER_NAME="Rebase Committer" &&
+	GIT_COMMITTER_EMAIL="rebase.committer@example.com"
+'
+
+test_expect_success 'simple fixup -C works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD -m "A2"
+'
+
+test_expect_success 'simple fixup -c works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	git log -1 --pretty=format:%B >expected-fixup-message &&
+	test_write_lines "" "Modified A2" >>expected-fixup-message &&
+	FAKE_LINES="1 fixup_-c 2" \
+		FAKE_COMMIT_AMEND="Modified A2" \
+		git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD expected-fixup-message
+'
+
+test_expect_success 'fixup -C removes amend! from message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'fixup -C with conflicts gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
+	git checkout --theirs -- A &&
+	git add A &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_cmp_rev HEAD^ conflicts &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_write_lines "" edited >>expected-message &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'skipping fixup -C after fixup gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	git reset --hard &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_commit_message HEAD -m "B"
+'
+
+test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
+	git checkout --detach branch &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
+		FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
+test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout branch && git checkout --detach branch~2 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_commit_message HEAD expected-message
+'
+
+test_done
diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message
new file mode 100644
index 0000000000..ab2434f90e
--- /dev/null
+++ b/t/t3437/expected-squash-message
@@ -0,0 +1,51 @@
+# This is a combination of 6 commits.
+# The 1st commit message will be skipped:
+
+# B
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# The commit message #2 will be skipped:
+
+# fixup! B
+
+# The commit message #3 will be skipped:
+
+# amend! B
+#
+# B
+#
+# edited 1
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #4:
+
+# amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #5:
+
+# squash! amend! amend! B
+
+edited squash
+
+# This is the commit message #6:
+
+# amend! amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+edited 3
+squashed
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend!
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (6 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-08  9:23 ` [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
                   ` (10 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

If the commit subject starts with "amend!" then rearrange it like a
"fixup!" commit and replace `pick` command with `fixup -C` command,
which is used to fixup up the content if any and replaces the original
commit message with amend! commit's message.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c                     | 23 ++++++++++++++++-------
 t/t3437-rebase-fixup-options.sh | 12 ++++++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index db016717d1..4b2e71cbda 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5672,6 +5672,12 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
+static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+	return skip_prefix(subject, "fixup! ", p) ||
+	       skip_prefix(subject, "amend! ", p) ||
+	       skip_prefix(subject, "squash! ", p);
+}
+
 /*
  * Rearrange the todo list that has both "pick commit-id msg" and "pick
  * commit-id fixup!/squash! msg" in it so that the latter is put immediately
@@ -5730,15 +5736,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if ((skip_prefix(subject, "fixup! ", &p) ||
-		     skip_prefix(subject, "squash! ", &p))) {
+		if (skip_fixup_amend_squash(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_prefix(p, "fixup! ", &p) &&
-				    !skip_prefix(p, "squash! ", &p))
+				if (!skip_fixup_amend_squash(p, &p))
 					break;
 			}
 
@@ -5768,9 +5772,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		}
 		if (i2 >= 0) {
 			rearranged = 1;
-			todo_list->items[i].command =
-				starts_with(subject, "fixup!") ?
-				TODO_FIXUP : TODO_SQUASH;
+			if (starts_with(subject, "fixup!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+			} else if (starts_with(subject, "amend!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+				todo_list->items[i].flags = TODO_REPLACE_FIXUP_MSG;
+			} else {
+				todo_list->items[i].command = TODO_SQUASH;
+			}
 			if (tail[i2] < 0) {
 				next[i] = next[i2];
 				next[i2] = i;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 9e62d74249..532dc1c11b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -187,4 +187,16 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 	test_commit_message HEAD expected-message
 '
 
+test_expect_success 'fixup -C works upon --autosquash with amend!' '
+	git checkout --detach branch &&
+	FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --autosquash \
+						--signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
 test_done
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (7 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
@ 2021-01-08  9:23 ` Charvi Mendiratta
  2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (9 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-08  9:23 UTC (permalink / raw)
  To: git
  Cc: christian.couder, phillip.wood, Johannes.Schindelin,
	Charvi Mendiratta, Christian Couder

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a0487b5cc5..776507e0cc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -887,9 +887,15 @@ If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be
 attributed to the author of the first commit.  The suggested commit
-message for the folded commit is the concatenation of the commit
-messages of the first commit and of those with the "squash" command,
-but omits the commit messages of commits with the "fixup" command.
+message for the folded commit is created as follows:
+
+ - It is made using the commit message of a commit with the "fixup -C"
+   or "fixup -c" command. In the later case an editor is opened to edit
+   the commit message.
+ - Otherwise it's the concatenation of the commit messages of the first
+   commit and of those with the "squash" command.
+ - It omits the commit messages of commits with the "fixup"
+   (without -C or -c) command.
 
 'git rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
@ 2021-01-13 18:43   ` Taylor Blau
  2021-01-14  8:12     ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Taylor Blau @ 2021-01-13 18:43 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: git, christian.couder, phillip.wood, Johannes.Schindelin

On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  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.

Thanks,
Taylor

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
@ 2021-01-13 19:01   ` Taylor Blau
  2021-01-14  8:27     ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Taylor Blau @ 2021-01-13 19:01 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: git, christian.couder, phillip.wood, Johannes.Schindelin

On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  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.)

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

> @@ -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 &&

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

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments
  2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
@ 2021-01-13 19:14   ` Taylor Blau
  2021-01-13 20:37     ` Junio C Hamano
  2021-01-14  8:57     ` Charvi Mendiratta
  0 siblings, 2 replies; 66+ messages in thread
From: Taylor Blau @ 2021-01-13 19:14 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: git, christian.couder, phillip.wood, Johannes.Schindelin,
	Christian Couder

On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:
> This makes it easier to use and reuse the comments.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  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.");

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)
>  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,
Taylor

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments
  2021-01-13 19:14   ` Taylor Blau
@ 2021-01-13 20:37     ` Junio C Hamano
  2021-01-14  7:40       ` Christian Couder
  2021-01-14  8:57     ` Charvi Mendiratta
  1 sibling, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2021-01-13 20:37 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Charvi Mendiratta, git, christian.couder, phillip.wood,
	Johannes.Schindelin, Christian Couder

Taylor Blau <me@ttaylorr.com> writes:

> 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 actually am OK with []; it saves sizeof(char*) bytes from each of
the variable, doesn't it?

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

Good eyes.

Thanks.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments
  2021-01-13 20:37     ` Junio C Hamano
@ 2021-01-14  7:40       ` Christian Couder
  0 siblings, 0 replies; 66+ messages in thread
From: Christian Couder @ 2021-01-14  7:40 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, Charvi Mendiratta, git, Phillip Wood,
	Johannes Schindelin, Christian Couder

On Wed, Jan 13, 2021 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Taylor Blau <me@ttaylorr.com> writes:

[...]

> > 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 actually am OK with []; it saves sizeof(char*) bytes from each of
> the variable, doesn't it?

Yeah, that's my understanding too.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-13 18:43   ` Taylor Blau
@ 2021-01-14  8:12     ` Charvi Mendiratta
  2021-01-14 10:46       ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-14  8:12 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Christian Couder, Phillip Wood, Johannes.Schindelin

On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > 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 <phillip.wood@dunelm.org.uk>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> > ---
> >  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/

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-13 19:01   ` Taylor Blau
@ 2021-01-14  8:27     ` Charvi Mendiratta
  2021-01-14 10:29       ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-14  8:27 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Christian Couder, Phillip Wood, Johannes.Schindelin

On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > 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 <phillip.wood@dunelm.org.uk>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> > ---
> >  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.

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

> > @@ -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 &&
>
> > @@ -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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments
  2021-01-13 19:14   ` Taylor Blau
  2021-01-13 20:37     ` Junio C Hamano
@ 2021-01-14  8:57     ` Charvi Mendiratta
  1 sibling, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-14  8:57 UTC (permalink / raw)
  To: Taylor Blau
  Cc: git, Christian Couder, Phillip Wood, Junio C Hamano, Johannes.Schindelin

On Thu, 14 Jan 2021 at 00:44, Taylor Blau <me@ttaylorr.com> wrote:
>
> On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:
> > This makes it easier to use and reuse the comments.
> >
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> > ---
> >  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)
> >  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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command
  2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
@ 2021-01-14  9:23   ` Christian Couder
  2021-01-14  9:45     ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Christian Couder @ 2021-01-14  9:23 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: git, Phillip Wood, Johannes Schindelin, Christian Couder

On Fri, Jan 8, 2021 at 10:26 AM Charvi Mendiratta <charvi077@gmail.com> wrote:

> @@ -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 update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
> +{
> +       strbuf_setlen(buf1, 2);
> +       strbuf_addf(buf1, nth_commit_msg_fmt, n);

Should "_(...)" be used around "nth_commit_msg_fmt" here...

> +       strbuf_addch(buf1, '\n');
> +       strbuf_setlen(buf2, 2);
> +       strbuf_addf(buf2, skip_nth_commit_msg_fmt, n);

...and around "skip_nth_commit_msg_fmt" here?

> +       strbuf_addch(buf2, '\n');
> +}

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command
  2021-01-14  9:23   ` Christian Couder
@ 2021-01-14  9:45     ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-14  9:45 UTC (permalink / raw)
  To: Christian Couder; +Cc: git, Phillip Wood, Johannes Schindelin, Christian Couder

Hi Christian,

On Thu, 14 Jan 2021 at 14:54, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Fri, Jan 8, 2021 at 10:26 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
>
> > @@ -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 update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
> > +{
> > +       strbuf_setlen(buf1, 2);
> > +       strbuf_addf(buf1, nth_commit_msg_fmt, n);
>
> Should "_(...)" be used around "nth_commit_msg_fmt" here...

>
> > +       strbuf_addch(buf1, '\n');
> > +       strbuf_setlen(buf2, 2);
> > +       strbuf_addf(buf2, skip_nth_commit_msg_fmt, n);
>
> ...and around "skip_nth_commit_msg_fmt" here?
>

Yes, I forgot to put  "_(...)" here for translation. I will add it, thanks.

Thanks and Regards,
Charvi

> > +       strbuf_addch(buf2, '\n');
> > +}

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-14  8:27     ` Charvi Mendiratta
@ 2021-01-14 10:29       ` Phillip Wood
  2021-01-15  8:35         ` Charvi Mendiratta
  2021-01-17  3:39         ` Charvi Mendiratta
  0 siblings, 2 replies; 66+ messages in thread
From: Phillip Wood @ 2021-01-14 10:29 UTC (permalink / raw)
  To: Charvi Mendiratta, Taylor Blau
  Cc: git, Christian Couder, Phillip Wood, Johannes.Schindelin

Hi Charvi and Taylor

On 14/01/2021 08:27, Charvi Mendiratta wrote:
> On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@ttaylorr.com> wrote:
>>
>> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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 <phillip.wood@dunelm.org.uk>
>>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
>>> ---
>>>   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.

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

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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-14  8:12     ` Charvi Mendiratta
@ 2021-01-14 10:46       ` Phillip Wood
  2021-01-15  8:38         ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2021-01-14 10:46 UTC (permalink / raw)
  To: Charvi Mendiratta, Taylor Blau
  Cc: git, Christian Couder, Phillip Wood, Johannes.Schindelin

Hi Taylor and Charvi

On 14/01/2021 08:12, Charvi Mendiratta wrote:
> On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote:
>>
>> On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> 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 <phillip.wood@dunelm.org.uk>
>>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
>>> ---
>>>   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
> 
>> Thanks,
>> Taylor
>>
>> [1]: https://lore.kernel.org/git/xmqqlfcz8ggj.fsf@gitster.c.googlers.com/


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-14 10:29       ` Phillip Wood
@ 2021-01-15  8:35         ` Charvi Mendiratta
  2021-01-15  8:44           ` Christian Couder
  2021-01-17  3:39         ` Charvi Mendiratta
  1 sibling, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-15  8:35 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Taylor Blau, git, Christian Couder, Phillip Wood, Johannes Schindelin

Hi Phillip,

On Thu, 14 Jan 2021 at 15:59, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Charvi and Taylor
>
> On 14/01/2021 08:27, Charvi Mendiratta wrote:
> > On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@ttaylorr.com> wrote:
> >>
> >> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> >>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>>
> >>> 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 <phillip.wood@dunelm.org.uk>
> >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> >>> ---
> >>>   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.

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-14 10:46       ` Phillip Wood
@ 2021-01-15  8:38         ` Charvi Mendiratta
  2021-01-15 17:22           ` Junio C Hamano
  0 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-15  8:38 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Taylor Blau, git, Christian Couder, Phillip Wood, Johannes Schindelin

On Thu, 14 Jan 2021 at 16:16, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Taylor and Charvi
>
> On 14/01/2021 08:12, Charvi Mendiratta wrote:
> > On Thu, 14 Jan 2021 at 00:13, Taylor Blau <me@ttaylorr.com> wrote:
> >>
> >> On Fri, Jan 08, 2021 at 02:53:39PM +0530, Charvi Mendiratta wrote:
> >>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>>
> >>> 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 <phillip.wood@dunelm.org.uk>
> >>> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> >>> ---
> >>>   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
>
> > 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/
>

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-15  8:35         ` Charvi Mendiratta
@ 2021-01-15  8:44           ` Christian Couder
  2021-01-15 11:12             ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Christian Couder @ 2021-01-15  8:44 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: Phillip Wood, Taylor Blau, git, Phillip Wood, Johannes Schindelin

Hi Charvi,

On Fri, Jan 15, 2021 at 9:35 AM Charvi Mendiratta <charvi077@gmail.com> wrote:

> On Thu, 14 Jan 2021 at 15:59, Phillip Wood <phillip.wood123@gmail.com> wrote:

> > On 14/01/2021 08:27, Charvi Mendiratta wrote:
> > > On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@ttaylorr.com> wrote:
> > >>
> > >> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> > >>> From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-15  8:44           ` Christian Couder
@ 2021-01-15 11:12             ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-15 11:12 UTC (permalink / raw)
  To: Christian Couder
  Cc: Phillip Wood, Taylor Blau, git, Phillip Wood, Johannes Schindelin

Hi Christian,

On Fri, 15 Jan 2021 at 14:15, Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi Charvi,
>
> On Fri, Jan 15, 2021 at 9:35 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
>
> > On Thu, 14 Jan 2021 at 15:59, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> > > On 14/01/2021 08:27, Charvi Mendiratta wrote:
> > > > On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@ttaylorr.com> wrote:
> > > >>
> > > >> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> > > >>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> > > >>> +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.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-15  8:38         ` Charvi Mendiratta
@ 2021-01-15 17:22           ` Junio C Hamano
  2021-01-16  4:49             ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2021-01-15 17:22 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: Phillip Wood, Taylor Blau, git, Christian Couder, Phillip Wood,
	Johannes Schindelin

Charvi Mendiratta <charvi077@gmail.com> writes:

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

Please don't.  There is a higher cognitive cost to readers when you write

	if (write_message(...)) {

The reader is forced to look at its implementation to see if it
returns positive in a non-error situation.

If you write it like so from the beginning

	if (write_message(...) < 0) {

the reader can trust that the code follows "negative is an error"
convention.  One fewer thing readers have to worry about.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-15 17:22           ` Junio C Hamano
@ 2021-01-16  4:49             ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-16  4:49 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Phillip Wood, Taylor Blau, git, Christian Couder, Phillip Wood,
	Johannes Schindelin

Hi Junio,

On Fri, 15 Jan 2021 at 22:52, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > 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".
>
> Please don't.  There is a higher cognitive cost to readers when you write
>
>         if (write_message(...)) {
>
> The reader is forced to look at its implementation to see if it
> returns positive in a non-error situation.
>
> If you write it like so from the beginning
>
>         if (write_message(...) < 0) {
>
> the reader can trust that the code follows "negative is an error"
> convention.  One fewer thing readers have to worry about.

I agree, earlier I was confused as there were many instances of
write_message() without '< 0' in sequencer.c but considering the
above I completely agree to follow the convention.

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-14 10:29       ` Phillip Wood
  2021-01-15  8:35         ` Charvi Mendiratta
@ 2021-01-17  3:39         ` Charvi Mendiratta
  2021-01-18 18:29           ` Phillip Wood
  1 sibling, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-17  3:39 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Taylor Blau, git, Christian Couder, Phillip Wood, Johannes Schindelin

Hi Phillip and Taylor,

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-17  3:39         ` Charvi Mendiratta
@ 2021-01-18 18:29           ` Phillip Wood
  2021-01-19  4:08             ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2021-01-18 18:29 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: Taylor Blau, git, Christian Couder, Phillip Wood, Johannes Schindelin

Hi Charvi

On 17/01/2021 03:39, Charvi Mendiratta wrote:
> Hi Phillip and Taylor,
> 
>> [...]
>>>>> @@ -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 &&
> 
>> 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
> 


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-18 18:29           ` Phillip Wood
@ 2021-01-19  4:08             ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  4:08 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Taylor Blau, git, Christian Couder, Phillip Wood, Johannes Schindelin

Hi Phillip,

On Mon, 18 Jan 2021 at 23:59, Phillip Wood <phillip.wood123@gmail.com> wrote:
> [...]
> > 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,

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (8 preceding siblings ...)
  2021-01-08  9:23 ` [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
@ 2021-01-19  7:40 ` Charvi Mendiratta
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
  2021-01-19  7:40 ` [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
                   ` (8 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:40 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3017 bytes --]

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

Changes from v1 :
* added '< 0' to check that 'write_message()' returns an error
* checks if squash! line commented out in t3415-rebase-autosquash.sh
* removing line break and changing suffix from _str to _fmt in sequencer.c

[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                     | 297 +++++++++++++++++++++++++++-----
 t/lib-rebase.sh                 |   4 +
 t/t3415-rebase-autosquash.sh    |  30 ++--
 t/t3437-rebase-fixup-options.sh | 202 ++++++++++++++++++++++
 t/t3437/expected-squash-message |  51 ++++++
 t/t3900-i18n-commit.sh          |   4 -
 8 files changed, 543 insertions(+), 61 deletions(-)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-squash-message

--
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (9 preceding siblings ...)
  2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
@ 2021-01-19  7:40 ` Charvi Mendiratta
  2021-01-19  7:40 ` [PATCH v2 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
                   ` (7 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:40 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..a59e0c84af 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1757,11 +1757,10 @@ 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)) {
+		if (command == TODO_FIXUP && write_message(body, strlen(body),
+							rebase_path_fixup_msg(), 0) < 0) {
 			unuse_commit_buffer(head_commit, head_message);
-			return error(_("cannot write '%s'"),
-				     rebase_path_fixup_msg());
+			return error(_("cannot write '%s'"), rebase_path_fixup_msg());
 		}
 
 		strbuf_addf(&buf, "%c ", comment_line_char);
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 2/9] sequencer: factor out code to append squash message
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (10 preceding siblings ...)
  2021-01-19  7:40 ` [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
@ 2021-01-19  7:40 ` Charvi Mendiratta
  2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
                   ` (6 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:40 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a59e0c84af..08cce40834 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,
@@ -1779,12 +1790,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (11 preceding siblings ...)
  2021-01-19  7:40 ` [PATCH v2 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
@ 2021-01-19  7:40 ` Charvi Mendiratta
  2021-01-21  1:38   ` Junio C Hamano
  2021-01-19  7:40 ` [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
                   ` (5 subsequent siblings)
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:40 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c                  | 25 ++++++++++++++++++++++++-
 t/t3415-rebase-autosquash.sh | 30 ++++++++++++++++--------------
 t/t3900-i18n-commit.sh       |  4 ----
 3 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 08cce40834..9acb9c333e 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..551dc06bc3 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,8 @@ 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 -v "squash" commit &&
+	grep "extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 
@@ -213,7 +213,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 +224,8 @@ 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 -v "squash" commit &&
+	grep "extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 
@@ -233,7 +234,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 +262,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 +327,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 +343,9 @@ 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 -v "squash" commit &&
+	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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit()
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (12 preceding siblings ...)
  2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
@ 2021-01-19  7:40 ` Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
                   ` (4 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:40 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

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

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9acb9c333e..8a9adb6ff3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1881,8 +1881,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)
 {
@@ -1895,6 +1894,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) {
 		/*
@@ -4144,8 +4145,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);
@@ -4607,11 +4608,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 5/9] sequencer: use const variable for commit message comments
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (13 preceding siblings ...)
  2021-01-19  7:40 ` [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
@ 2021-01-19  7:41 ` Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:41 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8a9adb6ff3..571000cf68 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1736,6 +1736,11 @@ 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_fmt[] = 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 +1750,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 +1779,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_fmt),
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
@@ -1798,9 +1803,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_fmt), 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);
 
@@ -1816,7 +1821,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (14 preceding siblings ...)
  2021-01-19  7:41 ` [PATCH v2 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
@ 2021-01-19  7:41 ` Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:41 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

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 <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 rebase-interactive.c |   4 +-
 sequencer.c          | 211 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 195 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 571000cf68..3c0cd487f9 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;
@@ -1738,32 +1744,174 @@ static size_t subject_length(const char *body)
 
 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_fmt[] = 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)
+{
+	return starts_with(opts->current_fixups.buf, "squash") ||
+		strstr(opts->current_fixups.buf, "\nsquash");
+}
+
+static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
 {
-	size_t commented_len = 0;
+	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');
+}
 
-	unlink(rebase_path_fixup_msg());
-	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+/*
+ * 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) < 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();
 
@@ -1783,6 +1931,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;
@@ -1796,18 +1946,22 @@ 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 && write_message(body, strlen(body),
+		if (command == TODO_FIXUP && !flag && write_message(body, strlen(body),
 							rebase_path_fixup_msg(), 0) < 0) {
 			unuse_commit_buffer(head_commit, head_message);
 			return error(_("cannot write '%s'"), rebase_path_fixup_msg());
 		}
-
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_fmt), 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);
 	}
@@ -1817,8 +1971,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),
@@ -1829,7 +1983,9 @@ static int update_squash_messages(struct repository *r,
 		return error(_("unknown command: %d"), command);
 	unuse_commit_buffer(commit, message);
 
-	res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
+	if (!res)
+		res = write_message(buf.buf, buf.len, rebase_path_squash_msg(),
+				    0);
 	strbuf_release(&buf);
 
 	if (!res) {
@@ -2030,7 +2186,8 @@ static int do_pick_commit(struct repository *r,
 	if (command == TODO_REWORD)
 		reword = 1;
 	else if (is_fixup(command)) {
-		if (update_squash_messages(r, command, commit, opts))
+		if (update_squash_messages(r, command, commit,
+					   opts, item->flags))
 			return -1;
 		flags |= AMEND_MSG;
 		if (!final_fixup)
@@ -2195,10 +2352,6 @@ static int read_and_refresh_cache(struct repository *r,
 	return 0;
 }
 
-enum todo_item_flags {
-	TODO_EDIT_MERGE_MSG = 1
-};
-
 void todo_list_release(struct todo_list *todo_list)
 {
 	strbuf_release(&todo_list->buf);
@@ -2285,6 +2438,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		return 0;
 	}
 
+	if (item->command == TODO_FIXUP) {
+		if (skip_prefix(bol, "-C", &bol) &&
+		   (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_REPLACE_FIXUP_MSG;
+		} else if (skip_prefix(bol, "-c", &bol) &&
+				  (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_EDIT_FIXUP_MSG;
+		}
+	}
+
 	if (item->command == TODO_MERGE) {
 		if (skip_prefix(bol, "-C", &bol))
 			bol += strspn(bol, " \t");
@@ -5291,6 +5456,14 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 					  short_commit_name(item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
+			if (item->command == TODO_FIXUP) {
+				if (item->flags & TODO_EDIT_FIXUP_MSG)
+					strbuf_addstr(buf, " -c");
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+					strbuf_addstr(buf, " -C");
+				}
+			}
+
 			if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (15 preceding siblings ...)
  2021-01-19  7:41 ` [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
@ 2021-01-19  7:41 ` Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:41 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh                 |   4 +
 t/t3437-rebase-fixup-options.sh | 190 ++++++++++++++++++++++++++++++++
 t/t3437/expected-squash-message |  51 +++++++++
 3 files changed, 245 insertions(+)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-squash-message

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index b72c051f47..e10e38060b 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -4,6 +4,7 @@
 #
 # - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
+# - copy the original commit message to a file with $FAKE_MESSAGE_COPY
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
 # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -33,6 +34,7 @@ set_fake_editor () {
 			exit
 		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
 		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
+		test -z "$FAKE_MESSAGE_COPY" || cat "$1" >"$FAKE_MESSAGE_COPY"
 		exit
 		;;
 	esac
@@ -51,6 +53,8 @@ set_fake_editor () {
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
+		merge_*|fixup_*)
+			action=$(echo "$line" | sed 's/_/ /g');;
 		"#")
 			echo '# comment' >> "$1";;
 		">")
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
new file mode 100755
index 0000000000..9e62d74249
--- /dev/null
+++ b/t/t3437-rebase-fixup-options.sh
@@ -0,0 +1,190 @@
+#!/bin/sh
+#
+# Copyright (c) 2018 Phillip Wood
+#
+
+test_description='git rebase interactive fixup options
+
+This test checks the "fixup [-C|-c]" command of rebase interactive.
+In addition to amending the contents of the commit, "fixup -C"
+replaces the original commit message with the message of the fixup
+commit. "fixup -c" also replaces the original message, but opens the
+editor to allow the user to edit the message before committing.
+'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+EMPTY=""
+
+test_commit_message () {
+	rev="$1" && # commit or tag we want to test
+	file="$2" && # test against the content of a file
+	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
+	if test "$2" = -m
+	then
+		str="$3" && # test against a string
+		printf "%s\n" "$str" >tmp-expected-message &&
+		file="tmp-expected-message"
+	fi
+	test_cmp "$file" actual-message
+}
+
+get_author () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%an %ae" "$rev"
+}
+
+test_expect_success 'setup' '
+	cat >message <<-EOF &&
+		amend! B
+		${EMPTY}
+		new subject
+		${EMPTY}
+		new
+		body
+		EOF
+
+	sed "1,2d" message >expected-message &&
+
+	test_commit A A &&
+	test_commit B B &&
+	get_author HEAD >expected-author &&
+	ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" &&
+	ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" &&
+	GIT_AUTHOR_NAME="Amend Author" &&
+	GIT_AUTHOR_EMAIL="amend@example.com" &&
+	test_commit "$(cat message)" A A1 A1 &&
+	test_commit A2 A &&
+	test_commit A3 A &&
+	GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" &&
+	GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" &&
+	git checkout -b conflicts-branch A &&
+	test_commit conflicts A &&
+
+	set_fake_editor &&
+	git checkout -b branch B &&
+	echo B1 >B &&
+	test_tick &&
+	git commit --fixup=HEAD -a &&
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		EOF
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		EOF
+	echo B2 >B &&
+	test_tick &&
+	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
+	echo B3 >B &&
+	test_tick &&
+	git commit -a -F - <<-EOF &&
+		amend! amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		${EMPTY}
+		edited 3
+		EOF
+
+	GIT_AUTHOR_NAME="Rebase Author" &&
+	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
+	GIT_COMMITTER_NAME="Rebase Committer" &&
+	GIT_COMMITTER_EMAIL="rebase.committer@example.com"
+'
+
+test_expect_success 'simple fixup -C works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD -m "A2"
+'
+
+test_expect_success 'simple fixup -c works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	git log -1 --pretty=format:%B >expected-fixup-message &&
+	test_write_lines "" "Modified A2" >>expected-fixup-message &&
+	FAKE_LINES="1 fixup_-c 2" \
+		FAKE_COMMIT_AMEND="Modified A2" \
+		git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD expected-fixup-message
+'
+
+test_expect_success 'fixup -C removes amend! from message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'fixup -C with conflicts gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
+	git checkout --theirs -- A &&
+	git add A &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_cmp_rev HEAD^ conflicts &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_write_lines "" edited >>expected-message &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'skipping fixup -C after fixup gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	git reset --hard &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_commit_message HEAD -m "B"
+'
+
+test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
+	git checkout --detach branch &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
+		FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
+test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout branch && git checkout --detach branch~2 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_commit_message HEAD expected-message
+'
+
+test_done
diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message
new file mode 100644
index 0000000000..ab2434f90e
--- /dev/null
+++ b/t/t3437/expected-squash-message
@@ -0,0 +1,51 @@
+# This is a combination of 6 commits.
+# The 1st commit message will be skipped:
+
+# B
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# The commit message #2 will be skipped:
+
+# fixup! B
+
+# The commit message #3 will be skipped:
+
+# amend! B
+#
+# B
+#
+# edited 1
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #4:
+
+# amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #5:
+
+# squash! amend! amend! B
+
+edited squash
+
+# This is the commit message #6:
+
+# amend! amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+edited 3
+squashed
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend!
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (16 preceding siblings ...)
  2021-01-19  7:41 ` [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
@ 2021-01-19  7:41 ` Charvi Mendiratta
  2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
  18 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:41 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

If the commit subject starts with "amend!" then rearrange it like a
"fixup!" commit and replace `pick` command with `fixup -C` command,
which is used to fixup up the content if any and replaces the original
commit message with amend! commit's message.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c                     | 23 ++++++++++++++++-------
 t/t3437-rebase-fixup-options.sh | 12 ++++++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3c0cd487f9..2a3f8d2fae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5664,6 +5664,12 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
+static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+	return skip_prefix(subject, "fixup! ", p) ||
+	       skip_prefix(subject, "amend! ", p) ||
+	       skip_prefix(subject, "squash! ", p);
+}
+
 /*
  * Rearrange the todo list that has both "pick commit-id msg" and "pick
  * commit-id fixup!/squash! msg" in it so that the latter is put immediately
@@ -5722,15 +5728,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if ((skip_prefix(subject, "fixup! ", &p) ||
-		     skip_prefix(subject, "squash! ", &p))) {
+		if (skip_fixup_amend_squash(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_prefix(p, "fixup! ", &p) &&
-				    !skip_prefix(p, "squash! ", &p))
+				if (!skip_fixup_amend_squash(p, &p))
 					break;
 			}
 
@@ -5760,9 +5764,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		}
 		if (i2 >= 0) {
 			rearranged = 1;
-			todo_list->items[i].command =
-				starts_with(subject, "fixup!") ?
-				TODO_FIXUP : TODO_SQUASH;
+			if (starts_with(subject, "fixup!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+			} else if (starts_with(subject, "amend!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+				todo_list->items[i].flags = TODO_REPLACE_FIXUP_MSG;
+			} else {
+				todo_list->items[i].command = TODO_SQUASH;
+			}
 			if (tail[i2] < 0) {
 				next[i] = next[i2];
 				next[i2] = i;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 9e62d74249..532dc1c11b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -187,4 +187,16 @@ test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fix
 	test_commit_message HEAD expected-message
 '
 
+test_expect_success 'fixup -C works upon --autosquash with amend!' '
+	git checkout --detach branch &&
+	FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --autosquash \
+						--signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
 test_done
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
                   ` (17 preceding siblings ...)
  2021-01-19  7:41 ` [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
@ 2021-01-19  7:41 ` Charvi Mendiratta
  2021-01-19 14:37   ` Marc Branchaud
  18 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19  7:41 UTC (permalink / raw)
  To: git; +Cc: chriscool, phillip.wood, me, gitster, Charvi Mendiratta

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a0487b5cc5..776507e0cc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -887,9 +887,15 @@ If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be
 attributed to the author of the first commit.  The suggested commit
-message for the folded commit is the concatenation of the commit
-messages of the first commit and of those with the "squash" command,
-but omits the commit messages of commits with the "fixup" command.
+message for the folded commit is created as follows:
+
+ - It is made using the commit message of a commit with the "fixup -C"
+   or "fixup -c" command. In the later case an editor is opened to edit
+   the commit message.
+ - Otherwise it's the concatenation of the commit messages of the first
+   commit and of those with the "squash" command.
+ - It omits the commit messages of commits with the "fixup"
+   (without -C or -c) command.
 
 'git rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
@ 2021-01-19 14:37   ` Marc Branchaud
  2021-01-19 17:13     ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Marc Branchaud @ 2021-01-19 14:37 UTC (permalink / raw)
  To: Charvi Mendiratta, git; +Cc: chriscool, phillip.wood, me, gitster

Hi!

On 2021-01-19 2:41 a.m., Charvi Mendiratta wrote:
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>   Documentation/git-rebase.txt | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index a0487b5cc5..776507e0cc 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -887,9 +887,15 @@ If you want to fold two or more commits into one, replace the command
>   "pick" for the second and subsequent commits with "squash" or "fixup".
>   If the commits had different authors, the folded commit will be
>   attributed to the author of the first commit.  The suggested commit
> -message for the folded commit is the concatenation of the commit
> -messages of the first commit and of those with the "squash" command,
> -but omits the commit messages of commits with the "fixup" command.
> +message for the folded commit is created as follows:
> +
> + - It is made using the commit message of a commit with the "fixup -C"
> +   or "fixup -c" command. In the later case an editor is opened to edit
> +   the commit message.

s/later/latter/

What happens if there is more than one "fixup -C/-c" command?

Thanks for this work!

		M.

> + - Otherwise it's the concatenation of the commit messages of the first
> +   commit and of those with the "squash" command.
> + - It omits the commit messages of commits with the "fixup"
> +   (without -C or -c) command.
>   
>   'git rebase' will stop when "pick" has been replaced with "edit" or
>   when a command fails due to merge errors. When you are done editing
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-19 14:37   ` Marc Branchaud
@ 2021-01-19 17:13     ` Charvi Mendiratta
  2021-01-19 22:05       ` Marc Branchaud
  2021-01-20 11:04       ` Phillip Wood
  0 siblings, 2 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-19 17:13 UTC (permalink / raw)
  To: Marc Branchaud
  Cc: git, Christian Couder, Phillip Wood, Taylor Blau, Junio C Hamano

Hi Marc,

On Tue, 19 Jan 2021 at 20:07, Marc Branchaud <marcnarc@xiplink.com> wrote:
[...]
> >   "pick" for the second and subsequent commits with "squash" or "fixup".
> >   If the commits had different authors, the folded commit will be
> >   attributed to the author of the first commit.  The suggested commit
> > -message for the folded commit is the concatenation of the commit
> > -messages of the first commit and of those with the "squash" command,
> > -but omits the commit messages of commits with the "fixup" command.
> > +message for the folded commit is created as follows:
> > +
> > + - It is made using the commit message of a commit with the "fixup -C"
> > +   or "fixup -c" command. In the later case an editor is opened to edit
> > +   the commit message.
>
> s/later/latter/
>

Thanks, will fix it.

> What happens if there is more than one "fixup -C/-c" command?
>

Upon running interactive rebase, in todo list if we use for example sequence of
commands like `fixup -C`, `fixup -C` , `fixup -C` then it will fixup
content of all and
for commit message it will replace with the commit message of end `fixup -C`

Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
then also it will fixup
up all the content and here it allow user to edit the message, so
opens the editor once
in this case and shows the commit msg of end `fixup -c` to edit and
also contains
commented commit messages of complete fixup chain. So, for any sequence of fixup
chains `fixup -c` works as similar to the `squash` command.

Hope it explains the working.

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-19 17:13     ` Charvi Mendiratta
@ 2021-01-19 22:05       ` Marc Branchaud
  2021-01-20  7:10         ` Charvi Mendiratta
  2021-01-20 11:04       ` Phillip Wood
  1 sibling, 1 reply; 66+ messages in thread
From: Marc Branchaud @ 2021-01-19 22:05 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: git, Christian Couder, Phillip Wood, Taylor Blau, Junio C Hamano

On 2021-01-19 12:13 p.m., Charvi Mendiratta wrote:
> Hi Marc,
> 
> On Tue, 19 Jan 2021 at 20:07, Marc Branchaud <marcnarc@xiplink.com> wrote:
> [...]
>>>    "pick" for the second and subsequent commits with "squash" or "fixup".
>>>    If the commits had different authors, the folded commit will be
>>>    attributed to the author of the first commit.  The suggested commit
>>> -message for the folded commit is the concatenation of the commit
>>> -messages of the first commit and of those with the "squash" command,
>>> -but omits the commit messages of commits with the "fixup" command.
>>> +message for the folded commit is created as follows:
>>> +
>>> + - It is made using the commit message of a commit with the "fixup -C"
>>> +   or "fixup -c" command. In the later case an editor is opened to edit
>>> +   the commit message.
>>
>> s/later/latter/
>>
> 
> Thanks, will fix it.
> 
>> What happens if there is more than one "fixup -C/-c" command?
>>
> 
> Upon running interactive rebase, in todo list if we use for example sequence of
> commands like `fixup -C`, `fixup -C` , `fixup -C` then it will fixup
> content of all and
> for commit message it will replace with the commit message of end `fixup -C`
> 
> Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
> then also it will fixup
> up all the content and here it allow user to edit the message, so
> opens the editor once
> in this case and shows the commit msg of end `fixup -c` to edit and
> also contains
> commented commit messages of complete fixup chain. So, for any sequence of fixup
> chains `fixup -c` works as similar to the `squash` command.
> 
> Hope it explains the working.

Yes, that does explain things.

I'm also under the impression that the "fixup -C/-c" commit's patch is 
still incorporated into the folded commit.  Is that right?

So I think the documentation could be improved.  Assuming that a "fixup 
-C/-c" commit's patch is incorporated just like with plain "fixup", 
something like:

The suggested commit message for the folded commit is the concatenation 
of the first commit's message with those identified by "squash" 
commands, omitting the messages of commits identified by "fixup" 
commands, `unless` "fixup -c" is used.  In that case the suggested 
commit message is `only` the message of the "fixup -c" commit, and an 
editor is opened allowing you to edit the message.  The contents (patch) 
of the "fixup -c" commit are still incorporated into the folded commit. 
  If there is more than one "fixup -c" commit, the message from the last 
last one is used.  You can also use "fixup -C" to get the same behavior 
as "fixup -c" except without opening an editor.

Thanks!

		M.


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-19 22:05       ` Marc Branchaud
@ 2021-01-20  7:10         ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-20  7:10 UTC (permalink / raw)
  To: Marc Branchaud
  Cc: git, Christian Couder, Phillip Wood, Taylor Blau, Junio C Hamano

On Wed, 20 Jan 2021 at 03:35, Marc Branchaud <marcnarc@xiplink.com> wrote:

[...]
> I'm also under the impression that the "fixup -C/-c" commit's patch is
> still incorporated into the folded commit.  Is that right?
>

yes, it's still incorporated into the folded commit, as it just adds
options to the `fixup` command to replace or edit the commit message.

> So I think the documentation could be improved.  Assuming that a "fixup
> -C/-c" commit's patch is incorporated just like with plain "fixup",
> something like:
>
> The suggested commit message for the folded commit is the concatenation
> of the first commit's message with those identified by "squash"
> commands, omitting the messages of commits identified by "fixup"
> commands, `unless` "fixup -c" is used.  In that case the suggested
> commit message is `only` the message of the "fixup -c" commit, and an
> editor is opened allowing you to edit the message.  The contents (patch)
> of the "fixup -c" commit are still incorporated into the folded commit.
>   If there is more than one "fixup -c" commit, the message from the last
> last one is used.  You can also use "fixup -C" to get the same behavior
> as "fixup -c" except without opening an editor.
>

I agree, this is now more clear and explains for more than one `fixup
[-C|-c]` or `squash` command also. Thanks for the suggestions, I will
change this in the next revision.

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-19 17:13     ` Charvi Mendiratta
  2021-01-19 22:05       ` Marc Branchaud
@ 2021-01-20 11:04       ` Phillip Wood
  2021-01-20 12:31         ` Charvi Mendiratta
  1 sibling, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2021-01-20 11:04 UTC (permalink / raw)
  To: Charvi Mendiratta, Marc Branchaud
  Cc: git, Christian Couder, Phillip Wood, Taylor Blau, Junio C Hamano

Hi Charvi

On 19/01/2021 17:13, Charvi Mendiratta wrote:
> Hi Marc,
> 
> On Tue, 19 Jan 2021 at 20:07, Marc Branchaud <marcnarc@xiplink.com> wrote:
> [...]
>>>    "pick" for the second and subsequent commits with "squash" or "fixup".
>>>    If the commits had different authors, the folded commit will be
>>>    attributed to the author of the first commit.  The suggested commit
>>> -message for the folded commit is the concatenation of the commit
>>> -messages of the first commit and of those with the "squash" command,
>>> -but omits the commit messages of commits with the "fixup" command.
>>> +message for the folded commit is created as follows:
>>> +
>>> + - It is made using the commit message of a commit with the "fixup -C"
>>> +   or "fixup -c" command. In the later case an editor is opened to edit
>>> +   the commit message.
>>
>> s/later/latter/
>>
> 
> Thanks, will fix it.
> 
>> What happens if there is more than one "fixup -C/-c" command?
>>
> 
> Upon running interactive rebase, in todo list if we use for example sequence of
> commands like `fixup -C`, `fixup -C` , `fixup -C` then it will fixup
> content of all and
> for commit message it will replace with the commit message of end `fixup -C`
> 
> Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
> then also it will fixup
> up all the content and here it allow user to edit the message, so
> opens the editor once

It is good that we only open the editor once in this case - I'd not 
thought about chains of `fixup -c` before reading this. Do we have a 
test to verify that the editor is only opened once?

Best Wishes

Phillip

> in this case and shows the commit msg of end `fixup -c` to edit and
> also contains
> commented commit messages of complete fixup chain. So, for any sequence of fixup
> chains `fixup -c` works as similar to the `squash` command.
> 
> Hope it explains the working.
> 
> Thanks and Regards,
> Charvi
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-20 11:04       ` Phillip Wood
@ 2021-01-20 12:31         ` Charvi Mendiratta
  2021-01-20 14:29           ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-20 12:31 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Marc Branchaud, git, Christian Couder, Taylor Blau, Junio C Hamano

Hi Phillip,

On Wed, 20 Jan 2021 at 16:34, Phillip Wood <phillip.wood123@gmail.com> wrote:

> >[...]
> > Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
> > then also it will fixup
> > up all the content and here it allow user to edit the message, so
> > opens the editor once
>
> It is good that we only open the editor once in this case - I'd not
> thought about chains of `fixup -c` before reading this. Do we have a
> test to verify that the editor is only opened once?
>

No, we don't. But I also agree, it's a good idea to add a test for it.
I think may be one sequence with 'fixup -C', 'fixup -c', 'fixup -c'
and the other 'squash' , 'fixup -C', 'fixup -c', is sufficient for
testing. Or any other suggestions for testing it ?

Thanks and regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-20 12:31         ` Charvi Mendiratta
@ 2021-01-20 14:29           ` Phillip Wood
  2021-01-20 16:09             ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2021-01-20 14:29 UTC (permalink / raw)
  To: Charvi Mendiratta, Phillip Wood
  Cc: Marc Branchaud, git, Christian Couder, Taylor Blau, Junio C Hamano

Hi Charvi

On 20/01/2021 12:31, Charvi Mendiratta wrote:
> Hi Phillip,
> 
> On Wed, 20 Jan 2021 at 16:34, Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>>> [...]
>>> Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
>>> then also it will fixup
>>> up all the content and here it allow user to edit the message, so
>>> opens the editor once
>>
>> It is good that we only open the editor once in this case - I'd not
>> thought about chains of `fixup -c` before reading this. Do we have a
>> test to verify that the editor is only opened once?
>>
> 
> No, we don't. But I also agree, it's a good idea to add a test for it.
> I think may be one sequence with 'fixup -C', 'fixup -c', 'fixup -c'
> and the other 'squash' , 'fixup -C', 'fixup -c', is sufficient for
> testing.

Those are both good sequences to test. I think we should check 'fixup 
-c' 'fixup' as well - with 'squash' 'fixup' we open the editor after the 
fixup so the user can see all the changes that will be committed when 
they edit the message, we should do the same for 'fixup -c' 'fixup'. 
Also 'fixup -c' 'squash' might be worth testing as well.

Best Wishes

Phillip


  Or any other suggestions for testing it ?
> 
> Thanks and regards,
> Charvi
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-20 14:29           ` Phillip Wood
@ 2021-01-20 16:09             ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-20 16:09 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Marc Branchaud, git, Christian Couder, Taylor Blau, Junio C Hamano

On Wed, 20 Jan 2021 at 19:59, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Charvi
>
> On 20/01/2021 12:31, Charvi Mendiratta wrote:
> > Hi Phillip,
> >
> > On Wed, 20 Jan 2021 at 16:34, Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> >>> [...]
> >>> Similarly, if we have sequence like `fixup -c`, `fixup -c`, `fixup -c`
> >>> then also it will fixup
> >>> up all the content and here it allow user to edit the message, so
> >>> opens the editor once
> >>
> >> It is good that we only open the editor once in this case - I'd not
> >> thought about chains of `fixup -c` before reading this. Do we have a
> >> test to verify that the editor is only opened once?
> >>
> >
> > No, we don't. But I also agree, it's a good idea to add a test for it.
> > I think may be one sequence with 'fixup -C', 'fixup -c', 'fixup -c'
> > and the other 'squash' , 'fixup -C', 'fixup -c', is sufficient for
> > testing.
>
> Those are both good sequences to test. I think we should check 'fixup
> -c' 'fixup' as well - with 'squash' 'fixup' we open the editor after the
> fixup so the user can see all the changes that will be committed when
> they edit the message, we should do the same for 'fixup -c' 'fixup'.
> Also 'fixup -c' 'squash' might be worth testing as well.
>

Agree, I will add these two test cases also and send the next revision of
patches.

Thanks for reviews!

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
@ 2021-01-21  1:38   ` Junio C Hamano
  2021-01-21 14:02     ` Charvi Mendiratta
  0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2021-01-21  1:38 UTC (permalink / raw)
  To: Charvi Mendiratta; +Cc: git, chriscool, phillip.wood, me

Charvi Mendiratta <charvi077@gmail.com> writes:

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

I cannot quite tell what this loop is trying to compute at the first
glance.

 - If body[0] == '\n', then i==0, c==LF, blank_line==1 and len==0
   so len==0 is returned immediately.

 - If the first line has only SP, HT, CR, etc. whitespace,
   blank_line stays 1 and at the end of the line when we see
   c=='\n', body[i] is pointing at that '\n', blank_line is true, so
   len is returned from the previous iteration (e.g. body="   \n"
   returns 0)

 - If the first line has some non space, blank_line becomes false,
   so at the end of that line when we see c=='\n', body[i] is
   pointing at that '\n', len==i+1 becomes one past that LF and then
   we reset blank_line to true??? and go on to the next line.

So when we see LF, if we have seen any non whitespace byte on that
line, blank_line is false.  Only when we saw LF followed by zero or
more whitespace before seeing another LF, we return len that was set
when we saw the previous LF (which is one past that LF).
    
So... is this trying to find the first paragraph-break-looking line
to find the end of the first paragraph.  OK.

There must be an easier-to-read way to write all this, though, I
would think (or don't we already have an existing code that is
waiting to be factored out?).

In any case, let's keep reading.

>  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);

As add_commented_lines places the comment character at the beginning
of each line, it is OK for body[0..commented_len) to contain more than
one lines.  Good.

> +	strbuf_addstr(buf, body + commented_len);

And we add everything after the beginning of the paragraph-break
looking line.  This code may add a line, immediately after the
previous "commented out" block, bunch of whitespaces and then a LF.
It will be cleaned up with stripspace most of the time, but
depending on the end-user settings, it may be left behind.  I am
guessing that is what we want, but thought it would not hurt to
double check.

> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 7bab6000dc..551dc06bc3 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" &&

It is not "extra"; that's the beginning of the "body" ;-).

>  	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,8 @@ 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 -v "squash" commit &&

This says that the file must have at least one line that does not
say "squash" or the test is a failure.  It does not say "there
should be no line that has "squash" on it".  Intended?

> +	grep "extra para" commit >actual &&

I can tell that you want the "extra para" to still remain, but how
does the grep that is not anchored guarantee that?  Perhaps look for

	grep "^extra para" commit

to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?

>  	test_line_count = 1 actual
>  '

Thanks.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21  1:38   ` Junio C Hamano
@ 2021-01-21 14:02     ` Charvi Mendiratta
  2021-01-21 15:21       ` Christian Couder
  0 siblings, 1 reply; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-21 14:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Christian Couder, Phillip Wood, Taylor Blau

Hi Junio,

On Thu, 21 Jan 2021 at 07:08, Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > +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;
> > +}
>
> I cannot quite tell what this loop is trying to compute at the first
> glance.
>

Oops, I think Phillip and Christian also pointed in the last revision
to look for alternatives to make it easy. I mistook that point and
forgot to look at it.

>  - If body[0] == '\n', then i==0, c==LF, blank_line==1 and len==0
>    so len==0 is returned immediately.
>
>  - If the first line has only SP, HT, CR, etc. whitespace,
>    blank_line stays 1 and at the end of the line when we see
>    c=='\n', body[i] is pointing at that '\n', blank_line is true, so
>    len is returned from the previous iteration (e.g. body="   \n"
>    returns 0)
>

yes, it returns the same result as given in this example (But I am
not sure what you are taking " SP, HT, CR, etc " ? otherwise if its
whitespace, then its works the same).

>  - If the first line has some non space, blank_line becomes false,
>    so at the end of that line when we see c=='\n', body[i] is
>    pointing at that '\n', len==i+1 becomes one past that LF and then
>    we reset blank_line to true??? and go on to the next line.
>
> So when we see LF, if we have seen any non whitespace byte on that
> line, blank_line is false.  Only when we saw LF followed by zero or
> more whitespace before seeing another LF, we return len that was set
> when we saw the previous LF (which is one past that LF).
>
> So... is this trying to find the first paragraph-break-looking line
> to find the end of the first paragraph.  OK.
>

I followed and agreed with the above.

> There must be an easier-to-read way to write all this, though, I
> would think (or don't we already have an existing code that is
> waiting to be factored out?).
>

I look into the code again and wonder if we can change this function like this :

static int subject_length(const char *body)
{
               const char *p = body;
               while (*p) {
                if (*p == '\n' && p[1] =='\n') {
                            break;
                } else {
                            p++;
               }
               }
               return p - body;
}

I think checking again '\n' will also serve the purpose as we separate
the commit message subject and its body with the newline. Also, this
is also
true that this function is only called when the message starts with
(squash! or amend! or fixup!)

> In any case, let's keep reading.
>
> >  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);
>
> As add_commented_lines places the comment character at the beginning
> of each line, it is OK for body[0..commented_len) to contain more than
> one lines.  Good.
>
> > +     strbuf_addstr(buf, body + commented_len);
>
> And we add everything after the beginning of the paragraph-break
> looking line.  This code may add a line, immediately after the
> previous "commented out" block, bunch of whitespaces and then a LF.
> It will be cleaned up with stripspace most of the time, but
> depending on the end-user settings, it may be left behind.  I am
> guessing that is what we want, but thought it would not hurt to
> double check.
>

I agree this working does the same and comments out the subject of the
commit message starting with squash! or fixup! or amend!, upon
squashing the two or more commits.

> > diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> > index 7bab6000dc..551dc06bc3 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" &&
>
> It is not "extra"; that's the beginning of the "body" ;-).
>

Okay, maybe we can use "message body" here.

> >       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,8 @@ 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 -v "squash" commit &&
>
> This says that the file must have at least one line that does not
> say "squash" or the test is a failure.  It does not say "there
> should be no line that has "squash" on it".  Intended?
>

Ohh yes ..

> > +     grep "extra para" commit >actual &&
>
> I can tell that you want the "extra para" to still remain, but how
> does the grep that is not anchored guarantee that?

.. but now I think to remove this `grep -v "squash" commit` as also
discussed with Phillip earlier that in this test script we are not
checking for the commented commit message.

> Perhaps look for
>
>         grep "^extra para" commit
>
> to ensure that you are not seeing a commented out but somehow failed
> to get stripspaced out?
>
I am not sure, what does failing to get stripspaced mean?

Thanks for the review !

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21 14:02     ` Charvi Mendiratta
@ 2021-01-21 15:21       ` Christian Couder
  2021-01-21 16:58         ` Phillip Wood
                           ` (2 more replies)
  0 siblings, 3 replies; 66+ messages in thread
From: Christian Couder @ 2021-01-21 15:21 UTC (permalink / raw)
  To: Charvi Mendiratta
  Cc: Junio C Hamano, git, Christian Couder, Phillip Wood, Taylor Blau

On Thu, Jan 21, 2021 at 3:02 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
>
> Hi Junio,
>
> On Thu, 21 Jan 2021 at 07:08, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Charvi Mendiratta <charvi077@gmail.com> writes:
> >
> > > +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;
> > > +}
> >
> > I cannot quite tell what this loop is trying to compute at the first
> > glance.
> >
>
> Oops, I think Phillip and Christian also pointed in the last revision
> to look for alternatives to make it easy. I mistook that point and
> forgot to look at it.

Yes, please take a look at find_commit_subject() in "commit.c".

> > > +     grep "extra para" commit >actual &&
> >
> > I can tell that you want the "extra para" to still remain, but how
> > does the grep that is not anchored guarantee that?
>
> .. but now I think to remove this `grep -v "squash" commit` as also
> discussed with Phillip earlier that in this test script we are not
> checking for the commented commit message.
>
> > Perhaps look for
> >
> >         grep "^extra para" commit
> >
> > to ensure that you are not seeing a commented out but somehow failed
> > to get stripspaced out?
> >
> I am not sure, what does failing to get stripspaced mean?

I think this refers to:

https://git-scm.com/docs/git-stripspace

Best,
Christian.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21 15:21       ` Christian Couder
@ 2021-01-21 16:58         ` Phillip Wood
  2021-01-21 20:56         ` Junio C Hamano
  2021-01-22 19:41         ` Charvi Mendiratta
  2 siblings, 0 replies; 66+ messages in thread
From: Phillip Wood @ 2021-01-21 16:58 UTC (permalink / raw)
  To: Christian Couder, Charvi Mendiratta
  Cc: Junio C Hamano, git, Christian Couder, Phillip Wood, Taylor Blau

Hi Christian and Charvi

On 21/01/2021 15:21, Christian Couder wrote:
> On Thu, Jan 21, 2021 at 3:02 PM Charvi Mendiratta <charvi077@gmail.com> wrote:
>>
>> Hi Junio,
>>
>> On Thu, 21 Jan 2021 at 07:08, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Charvi Mendiratta <charvi077@gmail.com> writes:
>>>
>>>> +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;
>>>> +}
>>>
>>> I cannot quite tell what this loop is trying to compute at the first
>>> glance.
>>>
>>
>> Oops, I think Phillip and Christian also pointed in the last revision
>> to look for alternatives to make it easy. I mistook that point and
>> forgot to look at it.
> 
> Yes, please take a look at find_commit_subject() in "commit.c".

That looks like it is taking the commit header and finding the start of 
the message. We have just the message, I think we probably want to use 
format_subject() in pretty.c which does what my hard to follow code does 
with the option to replace newlines in the subject with another character.

Best Wishes

Phillip

>>>> +     grep "extra para" commit >actual &&
>>>
>>> I can tell that you want the "extra para" to still remain, but how
>>> does the grep that is not anchored guarantee that?
>>
>> .. but now I think to remove this `grep -v "squash" commit` as also
>> discussed with Phillip earlier that in this test script we are not
>> checking for the commented commit message.
>>
>>> Perhaps look for
>>>
>>>          grep "^extra para" commit
>>>
>>> to ensure that you are not seeing a commented out but somehow failed
>>> to get stripspaced out?
>>>
>> I am not sure, what does failing to get stripspaced mean?
> 
> I think this refers to:
> 
> https://git-scm.com/docs/git-stripspace
> 
> Best,
> Christian.
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21 15:21       ` Christian Couder
  2021-01-21 16:58         ` Phillip Wood
@ 2021-01-21 20:56         ` Junio C Hamano
  2021-01-22 19:41           ` Charvi Mendiratta
  2021-01-22 19:41         ` Charvi Mendiratta
  2 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2021-01-21 20:56 UTC (permalink / raw)
  To: Christian Couder
  Cc: Charvi Mendiratta, git, Christian Couder, Phillip Wood, Taylor Blau

Christian Couder <christian.couder@gmail.com> writes:

>> Oops, I think Phillip and Christian also pointed in the last revision
>> to look for alternatives to make it easy. I mistook that point and
>> forgot to look at it.
>
> Yes, please take a look at find_commit_subject() in "commit.c".

Yeah, it uses pretty.c::skip_blank_lines(), which is easy to use.
so something like a loop that calls skip_blank_lines() to see if it
returns a differnt result (which means the argument we fed it was at
the beginning of a blank line, which is what this helper wants to
return), and otherwise we advance by one line with strchrnul() and
retry, perhaps.

    while (*body) {
	char *next = skip_blank_lines(body);
	if (next != body)
	    break; /* found a blank line */
	body = strchrnul(body, '\n');
	if (*body)
	    body++;
    }
    /* body has the answer */

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21 15:21       ` Christian Couder
  2021-01-21 16:58         ` Phillip Wood
  2021-01-21 20:56         ` Junio C Hamano
@ 2021-01-22 19:41         ` Charvi Mendiratta
  2 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-22 19:41 UTC (permalink / raw)
  To: Christian Couder
  Cc: Junio C Hamano, git, Christian Couder, Phillip Wood, Taylor Blau

Hi,

On Thu, 21 Jan 2021 at 20:51, Christian Couder
<christian.couder@gmail.com> wrote:
[...]
> > > > +     grep "extra para" commit >actual &&
> > >
> > > I can tell that you want the "extra para" to still remain, but how
> > > does the grep that is not anchored guarantee that?
> >
> > .. but now I think to remove this `grep -v "squash" commit` as also
> > discussed with Phillip earlier that in this test script we are not
> > checking for the commented commit message.
> >
> > > Perhaps look for
> > >
> > >         grep "^extra para" commit
> > >
> > > to ensure that you are not seeing a commented out but somehow failed
> > > to get stripspaced out?
> > >
> > I am not sure, what does failing to get stripspaced mean?
>
> I think this refers to:
>
> https://git-scm.com/docs/git-stripspace
>

Okay, thanks for referring and also here in this test script - so that
the test does not fail
due to stripspace, it is using the `test_line_count` function. Also I
agree with above
so maybe it is right to just replace with:

grep "^extra para" commit &&
grep "extra para" commit >actual &&

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-21 20:56         ` Junio C Hamano
@ 2021-01-22 19:41           ` Charvi Mendiratta
  0 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-22 19:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Christian Couder, git, Christian Couder, Phillip Wood, Taylor Blau

On Fri, 22 Jan 2021 at 02:26, Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> >> Oops, I think Phillip and Christian also pointed in the last revision
> >> to look for alternatives to make it easy. I mistook that point and
> >> forgot to look at it.
> >
> > Yes, please take a look at find_commit_subject() in "commit.c".
>
> Yeah, it uses pretty.c::skip_blank_lines(), which is easy to use.
> so something like a loop that calls skip_blank_lines() to see if it
> returns a differnt result (which means the argument we fed it was at
> the beginning of a blank line, which is what this helper wants to
> return), and otherwise we advance by one line with strchrnul() and
> retry, perhaps.
>
>     while (*body) {
>         char *next = skip_blank_lines(body);
>         if (next != body)
>             break; /* found a blank line */
>         body = strchrnul(body, '\n');
>         if (*body)
>             body++;
>     }
>     /* body has the answer */

Thanks for all the pointers. I took time in looking into it, but now I got more
cleared its actually looks for all the spaces in complete line which
is considered
as a blank line and above function explains it more clearly. I will
update the "subject
length" function as the above way and send in the next revision.

Thanks and Regards,
Charvi

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 0/9][Outreachy] rebase -i: add options to fixup command
  2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
@ 2021-01-24 17:03   ` Charvi Mendiratta
  2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
                       ` (8 more replies)
  0 siblings, 9 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:03 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123, Charvi Mendiratta

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3328 bytes --]

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

Changes from v2 :

(Thanks to Junio C Hamano, Marc Branchaud, Christian Couder, Phillip Wood
and Taylor Blau for the reviews and suggestions)

* Updated the Documentation/git-rebase.txt
* Added tests(t3437-rebase-fixup-options.sh) to check working of multiple
  fixup -c in rebase -i and related changes in sequencer.c
* Updated subject_length() function in sequencer.c and some corrections in
  t3415-rebase-autosquash.sh

[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      |  14 +-
 rebase-interactive.c              |   4 +-
 sequencer.c                       | 295 ++++++++++++++++++++++++++----
 t/lib-rebase.sh                   |   4 +
 t/t3415-rebase-autosquash.sh      |  30 +--
 t/t3437-rebase-fixup-options.sh   | 225 +++++++++++++++++++++++
 t/t3437/expected-combined-message |  21 +++
 t/t3437/expected-squash-message   |  51 ++++++
 t/t3900-i18n-commit.sh            |   4 -
 9 files changed, 587 insertions(+), 61 deletions(-)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-combined-message
 create mode 100644 t/t3437/expected-squash-message

--
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
@ 2021-01-24 17:03     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
                       ` (7 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:03 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Phillip Wood, Taylor Blau, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a46770..a59e0c84af 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1757,11 +1757,10 @@ 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)) {
+		if (command == TODO_FIXUP && write_message(body, strlen(body),
+							rebase_path_fixup_msg(), 0) < 0) {
 			unuse_commit_buffer(head_commit, head_message);
-			return error(_("cannot write '%s'"),
-				     rebase_path_fixup_msg());
+			return error(_("cannot write '%s'"), rebase_path_fixup_msg());
 		}
 
 		strbuf_addf(&buf, "%c ", comment_line_char);
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 2/9] sequencer: factor out code to append squash message
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
  2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
                       ` (6 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Phillip Wood, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a59e0c84af..08cce40834 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,
@@ -1779,12 +1790,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
  2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
                       ` (5 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Phillip Wood, Taylor Blau, Charvi Mendiratta

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c                  | 21 ++++++++++++++++++++-
 t/t3415-rebase-autosquash.sh | 30 ++++++++++++++++--------------
 t/t3900-i18n-commit.sh       |  4 ----
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 08cce40834..034149f24d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1718,15 +1718,34 @@ static int is_pick_or_similar(enum todo_command command)
 	}
 }
 
+static size_t subject_length(const char *body)
+{
+	const char *p = body;
+	while (*p) {
+		const char *next = skip_blank_lines(p);
+		if (next != p)
+			break;
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	}
+	return p - body;
+}
+
 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..a512335527 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,8 @@ 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 -v "squash" commit &&
+	grep "^extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 
@@ -213,7 +213,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 +224,8 @@ 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 -v "squash" commit &&
+	grep "^extra para" commit >actual &&
 	test_line_count = 1 actual
 '
 
@@ -233,7 +234,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 +262,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 +327,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 +343,9 @@ 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 -v "squash" commit &&
+	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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit()
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (2 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
                       ` (4 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Christian Couder, Phillip Wood

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

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 034149f24d..09cbb17f87 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1877,8 +1877,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)
 {
@@ -1891,6 +1890,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) {
 		/*
@@ -4140,8 +4141,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);
@@ -4603,11 +4604,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 5/9] sequencer: use const variable for commit message comments
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (3 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
                       ` (3 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Christian Couder, Phillip Wood, Taylor Blau

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 09cbb17f87..6d9a10afcf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1732,6 +1732,11 @@ static size_t subject_length(const char *body)
 	return p - body;
 }
 
+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_fmt[] = N_("This is a combination of %d commits.");
+
 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)
 {
@@ -1741,7 +1746,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);
@@ -1770,7 +1775,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_fmt),
 			    opts->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
@@ -1794,9 +1799,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_fmt), 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);
 
@@ -1812,7 +1817,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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (4 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
                       ` (2 subsequent siblings)
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Phillip Wood, Christian Couder

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 <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 rebase-interactive.c |   4 +-
 sequencer.c          | 213 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 197 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 6d9a10afcf..46e11d20e8 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)
 {
 	const char *p = body;
@@ -1734,32 +1740,176 @@ static size_t subject_length(const char *body)
 
 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_fmt[] = 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)
+{
+	return starts_with(opts->current_fixups.buf, "squash") ||
+		strstr(opts->current_fixups.buf, "\nsquash");
+}
+
+static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
 {
-	size_t commented_len = 0;
+	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');
+}
 
-	unlink(rebase_path_fixup_msg());
-	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
+/*
+ * 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) &&
+		    (file_exists(rebase_path_fixup_msg()) ||
+		     !file_exists(rebase_path_squash_msg()))) {
+			fixup_msg = skip_blank_lines(buf->buf + fixup_off);
+			if (write_message(fixup_msg, strlen(fixup_msg),
+					rebase_path_fixup_msg(), 0) < 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();
 
@@ -1779,6 +1929,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;
@@ -1792,18 +1944,22 @@ 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 && write_message(body, strlen(body),
+		if (command == TODO_FIXUP && !flag && write_message(body, strlen(body),
 							rebase_path_fixup_msg(), 0) < 0) {
 			unuse_commit_buffer(head_commit, head_message);
 			return error(_("cannot write '%s'"), rebase_path_fixup_msg());
 		}
-
 		strbuf_addf(&buf, "%c ", comment_line_char);
 		strbuf_addf(&buf, _(combined_commit_msg_fmt), 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);
 	}
@@ -1813,8 +1969,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),
@@ -1825,7 +1981,9 @@ static int update_squash_messages(struct repository *r,
 		return error(_("unknown command: %d"), command);
 	unuse_commit_buffer(commit, message);
 
-	res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
+	if (!res)
+		res = write_message(buf.buf, buf.len, rebase_path_squash_msg(),
+				    0);
 	strbuf_release(&buf);
 
 	if (!res) {
@@ -2026,7 +2184,8 @@ static int do_pick_commit(struct repository *r,
 	if (command == TODO_REWORD)
 		reword = 1;
 	else if (is_fixup(command)) {
-		if (update_squash_messages(r, command, commit, opts))
+		if (update_squash_messages(r, command, commit,
+					   opts, item->flags))
 			return -1;
 		flags |= AMEND_MSG;
 		if (!final_fixup)
@@ -2191,10 +2350,6 @@ static int read_and_refresh_cache(struct repository *r,
 	return 0;
 }
 
-enum todo_item_flags {
-	TODO_EDIT_MERGE_MSG = 1
-};
-
 void todo_list_release(struct todo_list *todo_list)
 {
 	strbuf_release(&todo_list->buf);
@@ -2281,6 +2436,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 		return 0;
 	}
 
+	if (item->command == TODO_FIXUP) {
+		if (skip_prefix(bol, "-C", &bol) &&
+		   (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_REPLACE_FIXUP_MSG;
+		} else if (skip_prefix(bol, "-c", &bol) &&
+				  (*bol == ' ' || *bol == '\t')) {
+			bol += strspn(bol, " \t");
+			item->flags |= TODO_EDIT_FIXUP_MSG;
+		}
+	}
+
 	if (item->command == TODO_MERGE) {
 		if (skip_prefix(bol, "-C", &bol))
 			bol += strspn(bol, " \t");
@@ -5287,6 +5454,14 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 					  short_commit_name(item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
+			if (item->command == TODO_FIXUP) {
+				if (item->flags & TODO_EDIT_FIXUP_MSG)
+					strbuf_addstr(buf, " -c");
+				else if (item->flags & TODO_REPLACE_FIXUP_MSG) {
+					strbuf_addstr(buf, " -C");
+				}
+			}
+
 			if (item->command == TODO_MERGE) {
 				if (item->flags & TODO_EDIT_MERGE_MSG)
 					strbuf_addstr(buf, " -c");
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (5 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Christian Couder, Phillip Wood

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 t/lib-rebase.sh                   |   4 +
 t/t3437-rebase-fixup-options.sh   | 213 ++++++++++++++++++++++++++++++
 t/t3437/expected-combined-message |  21 +++
 t/t3437/expected-squash-message   |  51 +++++++
 4 files changed, 289 insertions(+)
 create mode 100755 t/t3437-rebase-fixup-options.sh
 create mode 100644 t/t3437/expected-combined-message
 create mode 100644 t/t3437/expected-squash-message

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index b72c051f47..e10e38060b 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -4,6 +4,7 @@
 #
 # - override the commit message with $FAKE_COMMIT_MESSAGE
 # - amend the commit message with $FAKE_COMMIT_AMEND
+# - copy the original commit message to a file with $FAKE_MESSAGE_COPY
 # - check that non-commit messages have a certain line count with $EXPECT_COUNT
 # - check the commit count in the commit message header with $EXPECT_HEADER_COUNT
 # - rewrite a rebase -i script as directed by $FAKE_LINES.
@@ -33,6 +34,7 @@ set_fake_editor () {
 			exit
 		test -z "$FAKE_COMMIT_MESSAGE" || echo "$FAKE_COMMIT_MESSAGE" > "$1"
 		test -z "$FAKE_COMMIT_AMEND" || echo "$FAKE_COMMIT_AMEND" >> "$1"
+		test -z "$FAKE_MESSAGE_COPY" || cat "$1" >"$FAKE_MESSAGE_COPY"
 		exit
 		;;
 	esac
@@ -51,6 +53,8 @@ set_fake_editor () {
 			action="$line";;
 		exec_*|x_*|break|b)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
+		merge_*|fixup_*)
+			action=$(echo "$line" | sed 's/_/ /g');;
 		"#")
 			echo '# comment' >> "$1";;
 		">")
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
new file mode 100755
index 0000000000..832971ffdb
--- /dev/null
+++ b/t/t3437-rebase-fixup-options.sh
@@ -0,0 +1,213 @@
+#!/bin/sh
+#
+# Copyright (c) 2018 Phillip Wood
+#
+
+test_description='git rebase interactive fixup options
+
+This test checks the "fixup [-C|-c]" command of rebase interactive.
+In addition to amending the contents of the commit, "fixup -C"
+replaces the original commit message with the message of the fixup
+commit. "fixup -c" also replaces the original message, but opens the
+editor to allow the user to edit the message before committing.
+'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+EMPTY=""
+
+test_commit_message () {
+	rev="$1" && # commit or tag we want to test
+	file="$2" && # test against the content of a file
+	git show --no-patch --pretty=format:%B "$rev" >actual-message &&
+	if test "$2" = -m
+	then
+		str="$3" && # test against a string
+		printf "%s\n" "$str" >tmp-expected-message &&
+		file="tmp-expected-message"
+	fi
+	test_cmp "$file" actual-message
+}
+
+get_author () {
+	rev="$1" &&
+	git log -1 --pretty=format:"%an %ae" "$rev"
+}
+
+test_expect_success 'setup' '
+	cat >message <<-EOF &&
+		amend! B
+		${EMPTY}
+		new subject
+		${EMPTY}
+		new
+		body
+		EOF
+
+	sed "1,2d" message >expected-message &&
+
+	test_commit A A &&
+	test_commit B B &&
+	get_author HEAD >expected-author &&
+	ORIG_AUTHOR_NAME="$GIT_AUTHOR_NAME" &&
+	ORIG_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" &&
+	GIT_AUTHOR_NAME="Amend Author" &&
+	GIT_AUTHOR_EMAIL="amend@example.com" &&
+	test_commit "$(cat message)" A A1 A1 &&
+	test_commit A2 A &&
+	test_commit A3 A &&
+	GIT_AUTHOR_NAME="$ORIG_AUTHOR_NAME" &&
+	GIT_AUTHOR_EMAIL="$ORIG_AUTHOR_EMAIL" &&
+	git checkout -b conflicts-branch A &&
+	test_commit conflicts A &&
+
+	set_fake_editor &&
+	git checkout -b branch B &&
+	echo B1 >B &&
+	test_tick &&
+	git commit --fixup=HEAD -a &&
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		EOF
+	test_tick &&
+	git commit --allow-empty -F - <<-EOF &&
+		amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		EOF
+	echo B2 >B &&
+	test_tick &&
+	FAKE_COMMIT_AMEND="edited squash" git commit --squash=HEAD -a &&
+	echo B3 >B &&
+	test_tick &&
+	git commit -a -F - <<-EOF &&
+		amend! amend! amend! B
+		${EMPTY}
+		B
+		${EMPTY}
+		edited 1
+		${EMPTY}
+		edited 2
+		${EMPTY}
+		edited 3
+		EOF
+
+	GIT_AUTHOR_NAME="Rebase Author" &&
+	GIT_AUTHOR_EMAIL="rebase.author@example.com" &&
+	GIT_COMMITTER_NAME="Rebase Committer" &&
+	GIT_COMMITTER_EMAIL="rebase.committer@example.com"
+'
+
+test_expect_success 'simple fixup -C works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD -m "A2"
+'
+
+test_expect_success 'simple fixup -c works' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A2 &&
+	git log -1 --pretty=format:%B >expected-fixup-message &&
+	test_write_lines "" "Modified A2" >>expected-fixup-message &&
+	FAKE_LINES="1 fixup_-c 2" \
+		FAKE_COMMIT_AMEND="Modified A2" \
+		git rebase -i B &&
+	test_cmp_rev HEAD^ B &&
+	test_cmp_rev HEAD^{tree} A2^{tree} &&
+	test_commit_message HEAD expected-fixup-message
+'
+
+test_expect_success 'fixup -C removes amend! from message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	FAKE_LINES="1 fixup_-C 2" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'fixup -C with conflicts gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A1 &&
+	test_must_fail env FAKE_LINES="1 fixup_-C 2" git rebase -i conflicts &&
+	git checkout --theirs -- A &&
+	git add A &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_cmp_rev HEAD^ conflicts &&
+	test_cmp_rev HEAD^{tree} A1^{tree} &&
+	test_write_lines "" edited >>expected-message &&
+	test_commit_message HEAD expected-message &&
+	get_author HEAD >actual-author &&
+	test_cmp expected-author actual-author
+'
+
+test_expect_success 'skipping fixup -C after fixup gives correct message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	git reset --hard &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_commit_message HEAD -m "B"
+'
+
+test_expect_success 'sequence of fixup, fixup -C & squash --signoff works' '
+	git checkout --detach branch &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
+		FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
+test_expect_success 'first fixup -C commented out in sequence fixup fixup -C fixup -C' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout branch && git checkout --detach branch~2 &&
+	git log -1 --pretty=format:%b >expected-message &&
+	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4" git rebase -i A &&
+	test_cmp_rev HEAD^ A &&
+	test_commit_message HEAD expected-message
+'
+
+test_expect_success 'multiple fixup -c opens editor once' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_MESSAGE="Modified-A3" \
+		FAKE_LINES="1 fixup_-C 2 fixup_-c 3 fixup_-c 4" \
+		EXPECT_HEADER_COUNT=4 \
+		git rebase -i $base &&
+	test_cmp_rev $base HEAD^ &&
+	test 1 = $(git show | grep Modified-A3 | wc -l)
+'
+
+test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	FAKE_LINES="1 squash 2 fixup 3 fixup_-c 4" \
+		FAKE_MESSAGE_COPY=actual-combined-message \
+		git -c commit.status=false rebase -i A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-combined-message" \
+		actual-combined-message &&
+	test_cmp_rev HEAD^ A
+'
+
+test_done
diff --git a/t/t3437/expected-combined-message b/t/t3437/expected-combined-message
new file mode 100644
index 0000000000..a26cfb2fa9
--- /dev/null
+++ b/t/t3437/expected-combined-message
@@ -0,0 +1,21 @@
+# This is a combination of 4 commits.
+# This is the 1st commit message:
+
+B
+
+# This is the commit message #2:
+
+# amend! B
+
+new subject
+
+new
+body
+
+# The commit message #3 will be skipped:
+
+# A2
+
+# This is the commit message #4:
+
+A3
diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message
new file mode 100644
index 0000000000..ab2434f90e
--- /dev/null
+++ b/t/t3437/expected-squash-message
@@ -0,0 +1,51 @@
+# This is a combination of 6 commits.
+# The 1st commit message will be skipped:
+
+# B
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# The commit message #2 will be skipped:
+
+# fixup! B
+
+# The commit message #3 will be skipped:
+
+# amend! B
+#
+# B
+#
+# edited 1
+#
+# Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #4:
+
+# amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+Signed-off-by: Rebase Committer <rebase.committer@example.com>
+
+# This is the commit message #5:
+
+# squash! amend! amend! B
+
+edited squash
+
+# This is the commit message #6:
+
+# amend! amend! amend! B
+
+B
+
+edited 1
+
+edited 2
+
+edited 3
+squashed
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend!
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (6 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  2021-01-24 17:04     ` [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Phillip Wood, Christian Couder

If the commit subject starts with "amend!" then rearrange it like a
"fixup!" commit and replace `pick` command with `fixup -C` command,
which is used to fixup up the content if any and replaces the original
commit message with amend! commit's message.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 sequencer.c                     | 23 ++++++++++++++++-------
 t/t3437-rebase-fixup-options.sh | 12 ++++++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 46e11d20e8..d09ce446b6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5662,6 +5662,12 @@ static int subject2item_cmp(const void *fndata,
 
 define_commit_slab(commit_todo_item, struct todo_item *);
 
+static inline int skip_fixup_amend_squash(const char *subject, const char **p) {
+	return skip_prefix(subject, "fixup! ", p) ||
+	       skip_prefix(subject, "amend! ", p) ||
+	       skip_prefix(subject, "squash! ", p);
+}
+
 /*
  * Rearrange the todo list that has both "pick commit-id msg" and "pick
  * commit-id fixup!/squash! msg" in it so that the latter is put immediately
@@ -5720,15 +5726,13 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		format_subject(&buf, subject, " ");
 		subject = subjects[i] = strbuf_detach(&buf, &subject_len);
 		unuse_commit_buffer(item->commit, commit_buffer);
-		if ((skip_prefix(subject, "fixup! ", &p) ||
-		     skip_prefix(subject, "squash! ", &p))) {
+		if (skip_fixup_amend_squash(subject, &p)) {
 			struct commit *commit2;
 
 			for (;;) {
 				while (isspace(*p))
 					p++;
-				if (!skip_prefix(p, "fixup! ", &p) &&
-				    !skip_prefix(p, "squash! ", &p))
+				if (!skip_fixup_amend_squash(p, &p))
 					break;
 			}
 
@@ -5758,9 +5762,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 		}
 		if (i2 >= 0) {
 			rearranged = 1;
-			todo_list->items[i].command =
-				starts_with(subject, "fixup!") ?
-				TODO_FIXUP : TODO_SQUASH;
+			if (starts_with(subject, "fixup!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+			} else if (starts_with(subject, "amend!")) {
+				todo_list->items[i].command = TODO_FIXUP;
+				todo_list->items[i].flags = TODO_REPLACE_FIXUP_MSG;
+			} else {
+				todo_list->items[i].command = TODO_SQUASH;
+			}
 			if (tail[i2] < 0) {
 				next[i] = next[i2];
 				next[i2] = i;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 832971ffdb..945df2555b 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -210,4 +210,16 @@ test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
 	test_cmp_rev HEAD^ A
 '
 
+test_expect_success 'fixup -C works upon --autosquash with amend!' '
+	git checkout --detach branch &&
+	FAKE_COMMIT_AMEND=squashed \
+		FAKE_MESSAGE_COPY=actual-squash-message \
+		git -c commit.status=false rebase -ik --autosquash \
+						--signoff A &&
+	git diff-tree --exit-code --patch HEAD branch -- &&
+	test_cmp_rev HEAD^ A &&
+	test_i18ncmp "$TEST_DIRECTORY/t3437/expected-squash-message" \
+		actual-squash-message
+'
+
 test_done
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options
  2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
                       ` (7 preceding siblings ...)
  2021-01-24 17:04     ` [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
@ 2021-01-24 17:04     ` Charvi Mendiratta
  8 siblings, 0 replies; 66+ messages in thread
From: Charvi Mendiratta @ 2021-01-24 17:04 UTC (permalink / raw)
  To: git
  Cc: gitster, christian.couder, marcnarc, phillip.wood123,
	Charvi Mendiratta, Christian Couder, Phillip Wood

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Reviewed-by: Marc Branchaud <marcnarc@xiplink.com>
Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
---
 Documentation/git-rebase.txt | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index a0487b5cc5..a6903419c4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -887,9 +887,17 @@ If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be
 attributed to the author of the first commit.  The suggested commit
-message for the folded commit is the concatenation of the commit
-messages of the first commit and of those with the "squash" command,
-but omits the commit messages of commits with the "fixup" command.
+message for the folded commit is the concatenation of the first
+commit's message with those identified by "squash" commands, omitting the
+messages of commits identified by "fixup" commands, unless "fixup -c"
+is used.  In that case the suggested commit message is only the message
+of the "fixup -c" commit, and an editor is opened allowing you to edit
+the message.  The contents (patch) of the "fixup -c" commit are still
+incorporated into the folded commit. If there is more than one "fixup -c"
+commit, the message from the last last one is used.  You can also use
+"fixup -C" to get the same behavior as "fixup -c" except without opening
+an editor.
+
 
 'git rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
-- 
2.29.0.rc1


^ permalink raw reply	[flat|nested] 66+ messages in thread

end of thread, other threads:[~2021-01-24 17:09 UTC | newest]

Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-13 18:43   ` Taylor Blau
2021-01-14  8:12     ` Charvi Mendiratta
2021-01-14 10:46       ` Phillip Wood
2021-01-15  8:38         ` Charvi Mendiratta
2021-01-15 17:22           ` Junio C Hamano
2021-01-16  4:49             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-13 19:01   ` Taylor Blau
2021-01-14  8:27     ` Charvi Mendiratta
2021-01-14 10:29       ` Phillip Wood
2021-01-15  8:35         ` Charvi Mendiratta
2021-01-15  8:44           ` Christian Couder
2021-01-15 11:12             ` Charvi Mendiratta
2021-01-17  3:39         ` Charvi Mendiratta
2021-01-18 18:29           ` Phillip Wood
2021-01-19  4:08             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-13 19:14   ` Taylor Blau
2021-01-13 20:37     ` Junio C Hamano
2021-01-14  7:40       ` Christian Couder
2021-01-14  8:57     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-14  9:23   ` Christian Couder
2021-01-14  9:45     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-21  1:38   ` Junio C Hamano
2021-01-21 14:02     ` Charvi Mendiratta
2021-01-21 15:21       ` Christian Couder
2021-01-21 16:58         ` Phillip Wood
2021-01-21 20:56         ` Junio C Hamano
2021-01-22 19:41           ` Charvi Mendiratta
2021-01-22 19:41         ` Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19 14:37   ` Marc Branchaud
2021-01-19 17:13     ` Charvi Mendiratta
2021-01-19 22:05       ` Marc Branchaud
2021-01-20  7:10         ` Charvi Mendiratta
2021-01-20 11:04       ` Phillip Wood
2021-01-20 12:31         ` Charvi Mendiratta
2021-01-20 14:29           ` Phillip Wood
2021-01-20 16:09             ` Charvi Mendiratta

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git