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

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 15:56, Johannes Schindelin pisze: 
> On Thu, 1 Sep 2016, Jakub Narębski wrote:

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

Forgot to add citation:

[1] Ben Klemens "21st Century C: C Tips from the New School", 2nd Ed. (2014),
O'Reilly Media, chapter 10. "Better Structures", subsection
"Optional and Named Arguments"

> 
> Yeah, that looks unwieldy ;-)
>

Declaration needs some trickery, but use is much, much more readable
(if we cannot use sensibly named variables for passing arguments):

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

It is even better if there are large amount of parameters:

  res = amortization(.amount=20, .inflation=3,
 .show_table=0, .extra_payoff=100)

vs

  double amortize(double amt, double rate, double inflation, int months,
int selloff_month, double extra_payoff, int verbose,
double *interest_pv, double *duration, double *monthly_payment);

 
But we can't use it in Git, anyway
-- 
Jakub Narębski

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

[Johannes Schindelin]

Hi Kuba,

On Thu, 1 Sep 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > @@ -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.

Right.

Please note that it will make much more sense in the end, too, as the 0s
will be replaced by appropriate variables.

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

Yeah, that looks unwieldy ;-)

Thanks for the review,
Dscho

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

[Jakub Narębski]

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 
> ---
>  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(, "-s");
>   if (defmsg)
>   argv_array_pushl(, "-F", defmsg, NULL);
> + if (cleanup_commit_message)
> + argv_array_push(, "--cleanup=strip");

Good.

>   if (edit)
>   argv_array_push(, "-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", ))

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

>   argv_array_push(, "--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, );
> 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[];
>  
> 

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

[Johannes Schindelin]

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 
---
 sequencer.c | 10 +++---
 sequencer.h |  3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)

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)
 {
char **env = NULL;
struct argv_array array;
@@ -515,9 +516,12 @@ int sequencer_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
+   if (cleanup_commit_message)
+   argv_array_push(, "--cleanup=strip");
if (edit)
argv_array_push(, "-e");
-   else if (!opts->signoff && !opts->record_origin &&
+   else if (!cleanup_commit_message &&
+!opts->signoff && !opts->record_origin &&
 git_config_get_value("commit.cleanup", ))
argv_array_push(, "--cleanup=verbatim");
 
@@ -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);
 
 leave:
free_message(commit, );
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[];
 
-- 
2.10.0.rc1.114.g2bd6b38