Re: [PATCH v2 2/3] sequencer: make commit options more extensible

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 23 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > @@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,
> >  static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  		struct replay_opts *opts, int final_fixup)
> >  {
> > -	int edit = opts->edit, cleanup_commit_message = 0;
> > -	const char *msg_file = edit ? NULL : git_path_merge_msg();
> > +	unsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;
> > +	const char *msg_file = opts->edit ? NULL : git_path_merge_msg();
> >  	unsigned char head[20];
> >  	struct commit *base, *next, *parent;
> >  	const char *base_label, *next_label;
> >  	struct commit_message msg = { NULL, NULL, NULL, NULL };
> >  	struct strbuf msgbuf = STRBUF_INIT;
> > -	int res, unborn = 0, amend = 0, allow = 0;
> > +	int res, unborn = 0;
> > ... 
> > @@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  	if (allow < 0) {
> >  		res = allow;
> >  		goto leave;
> > -	}
> > +	} else if (allow)
> > +		flags |= ALLOW_EMPTY;
> 
> Making "flags" unsigned was a correct change, but this is now wrong,
> as "allow" is made unsigned by accident.

Right. Bummer.

> Perhaps something like this needs to be squashed in?
> 
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index bc2fe48e65..6c423566e6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -932,14 +932,14 @@ static void record_in_rewritten(struct object_id *oid,
>  static int do_pick_commit(enum todo_command command, struct commit *commit,
>  		struct replay_opts *opts, int final_fixup)
>  {
> -	unsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;
> +	unsigned int flags = opts->edit ? EDIT_MSG : 0;
>  	const char *msg_file = opts->edit ? NULL : git_path_merge_msg();
>  	unsigned char head[20];
>  	struct commit *base, *next, *parent;
>  	const char *base_label, *next_label;
>  	struct commit_message msg = { NULL, NULL, NULL, NULL };
>  	struct strbuf msgbuf = STRBUF_INIT;
> -	int res, unborn = 0;
> +	int allow, res, unborn = 0;

I had originally moved it from there, to stay with the related flags, but
I should just have left it there.

Your patch looks good, you could do even better by reverting that move
(IIRC it was at the end of the line, and it was set to 0 by default).

Ciao,
Dscho