Subject: Re: [PATCH 19/22] sequencer: support cleaning up commit messages

From: Jakub Narębski <[email-masked]>
Date: Thu, 1 Sep 2016 12:31:18 +0200

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The sequencer_commit() function already knows how to amend commits, and
> with this new option, it can also clean up commit messages (i.e. strip
> out commented lines). This is needed to implement rebase -i's 'fixup'
> and 'squash' commands as sequencer commands.
> 
> Signed-off-by: Johannes Schindelin <[email-masked]>
> ---
>  sequencer.c | 10 +++++++---
>  sequencer.h |  3 ++-
>  2 files changed, 9 insertions(+), 4 deletions(-)

This looks like nice little piece of enhancement, building scaffolding
for sequencer-izing interactive rebase bit by bit.

> 
> diff --git a/sequencer.c b/sequencer.c
> index 20f7590..5ec956f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,8 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit, int amend)
> +			  int allow_empty, int edit, int amend,
> +			  int cleanup_commit_message)

All right, though it slowly begins coming close to the threshold
where using bitfield flags would be sensible.

>  {
>  	char **env = NULL;
>  	struct argv_array array;
> @@ -515,9 +516,12 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  		argv_array_push(&array, "-s");
>  	if (defmsg)
>  		argv_array_pushl(&array, "-F", defmsg, NULL);
> +	if (cleanup_commit_message)
> +		argv_array_push(&array, "--cleanup=strip");

Good.

>  	if (edit)
>  		argv_array_push(&array, "-e");
> -	else if (!opts->signoff && !opts->record_origin &&
> +	else if (!cleanup_commit_message &&

All right, explicit cleanup=strip overrides "commit.cleanup" config,
and turns off passing commit verbatim (incompatible with stripping)...

> +		 !opts->signoff && !opts->record_origin &&

...adding signoff and recording origin requires not passing commit
verbatim,...

>  		 git_config_get_value("commit.cleanup", &value))

..., and in other cases are check the "commit.cleanup"...

>  		argv_array_push(&array, "--cleanup=verbatim");

... and pass commit verbatim if it is not set.

Ah, well, the change you made looks good.

>  
> @@ -781,7 +785,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, 0);
> +			opts, allow, opts->edit, 0, 0);

The calling convention begins to look unwieldy, but we have only
a single such callsite, and there are quite a bit callsites in
Git code that have similar API ("git grep ', 0, 0' -- '*.c'").
So we don't need to think about alternatives.  Yet.

It's a pity that emulation of named parameters in C requires
relying on designated inits from C99

  typedef struct {
    double pressure, moles, temp;
  } ideal_struct;

  #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1,   \
                                        .moles=1, .temp=273.15, __VA_ARGS__})

  double ideal_pressure_base(ideal_struct in)
  {
    return 8.314 * in.moles*in.temp/in.pressure;
  }

  ... ideal_pressure(.moles=2, .temp=373.15) ...

>  
>  leave:
>  	free_message(commit, &msg);
> diff --git a/sequencer.h b/sequencer.h
> index 2106c0d..e272549 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,8 @@ 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 amend);
> +			  int allow_empty, int edit, int amend,
> +			  int cleanup_commit_message);
>  
>  extern const char sign_off_header[];
>  
>