Hi Junio,
On Mon, 12 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>
> > 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 <johannes.schindelin@xxxxxx>
> > ---
> > 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