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

[Junio C Hamano <gitster@pobox.com>]

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.  It still needs to receive
the return value from allow_empty() that can signal an error by
returning a negative value.  It seems you did this deliberately (it
wasn't like "flags" and "allow" were on the same line both "int" and
the change to make "flags" unsigned accidentally made "allow" also
unsigned; "allow" was "int" and didn't have to move when "flags" was
introduced in the first place), but I failed to spot it because I
was only looking at s/int/unsigned/ on the line in the interdiff,
and missed that "allow" was on the same line.  Compilers are better at
finding these bugs.

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;
 
 	if (opts->no_commit) {
 		/*
-- 
2.12.1-432-gf364f02724