Re: [PATCH v2 17/25] sequencer: support amending commits

Johannes Schindelin [off-list ref] writes:

Hmm, this is more about a comment on 18/25, but I suspect that
"amend" or any opportunity given to the user to futz with the
contents in the editor invites a wish for the result to be treated
with stripspace.

No existing callers use "amend" to call this function, so there is
no change in behaviour, but at the same time, we do not have enough
information to see if 'amend' should by default toggle cleanup.

Hi Junio,

On Mon, 12 Sep 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

This teaches the sequencer_commit() function to take an argument that
will allow us to implement "todo" commands that need to amend the commit
messages ("fixup", "squash" and "reword").

Signed-off-by: Johannes Schindelin <redacted>
---
 sequencer.c | 6 ++++--
 sequencer.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 6e9732c..60b522e 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -485,7 +485,7 @@ static char **read_author_script(void)
  * author metadata.
  */
 int sequencer_commit(const char *defmsg, struct replay_opts *opts,
-			  int allow_empty, int edit)
+			  int allow_empty, int edit, int amend)
 {
 	char **env = NULL;
 	struct argv_array array;

@@ -514,6 +514,8 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
 
+	if (amend)
+		argv_array_push(&array, "--amend");
 	if (opts->gpg_sign)
 		argv_array_pushf(&array, "-S%s", opts->gpg_sign);
 	if (opts->signoff)

@@ -786,7 +788,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 	if (!opts->no_commit)
 		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
-			opts, allow, opts->edit);
+			opts, allow, opts->edit, 0);
 
 leave:
 	free_message(commit, &msg);

Hmm, this is more about a comment on 18/25, but I suspect that
"amend" or any opportunity given to the user to futz with the
contents in the editor invites a wish for the result to be treated
with stripspace.

The point of separating the cleanup_commit_message from the edit flag is
to allow final fixup commands to stripspace, even without letting the user
edit the message.

So while you are correct that "edit != 0" should imply
"cleanup_commit_message != 0", I would rather keep that explicit.

No existing callers use "amend" to call this function, so there is
no change in behaviour, but at the same time, we do not have enough
information to see if 'amend' should by default toggle cleanup.

It should not. Non-final fixup/squash commands *need* to keep the
comment lines.

Keeping things explicit makes it easier to understand the flow, methinks.

Ciao,
Dscho