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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  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 
> > ---
> >  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(, "commit");
> > argv_array_push(, "-n");
> >  
> > +   if (amend)
> > +   argv_array_push(, "--amend");
> > if (opts->gpg_sign)
> > argv_array_pushf(, "-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, );
> 
> 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

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

[Junio C Hamano]

Johannes Schindelin  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 
> ---
>  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(, "commit");
>   argv_array_push(, "-n");
>  
> + if (amend)
> + argv_array_push(, "--amend");
>   if (opts->gpg_sign)
>   argv_array_pushf(, "-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, );

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.


> diff --git a/sequencer.h b/sequencer.h
> index 7f5222f..c45f5c4 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -54,7 +54,7 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit);
> +   int allow_empty, int edit, int amend);
>  
>  extern const char sign_off_header[];

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

[Johannes Schindelin]

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 
---
 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(, "commit");
argv_array_push(, "-n");
 
+   if (amend)
+   argv_array_push(, "--amend");
if (opts->gpg_sign)
argv_array_pushf(, "-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, );
diff --git a/sequencer.h b/sequencer.h
index 7f5222f..c45f5c4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -54,7 +54,7 @@ int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
 int sequencer_commit(const char *defmsg, struct replay_opts *opts,
- int allow_empty, int edit);
+ int allow_empty, int edit, int amend);
 
 extern const char sign_off_header[];
 
-- 
2.10.0.windows.1.10.g803177d