[PATCH 0/3] rebase -i (reword): call the commit-msg hook again

[PATCH 0/3] rebase -i (reword): call the commit-msg hook again

[Johannes Schindelin]

It is actually not only the commit-msg, but also the prepare-commit-msg
hook...


Johannes Schindelin (3):
  t7504: document regression: reword no longer calls commit-msg
  sequencer: make commit options more extensible
  sequencer: allow the commit-msg hooks to run during a `reword`

 sequencer.c                | 58 +++++++++++++++++++++++++++-------------------
 t/t7504-commit-msg-hook.sh | 17 ++++++++++++++
 2 files changed, 51 insertions(+), 24 deletions(-)


base-commit: afd6726309f57f532b4b989a75c1392359c611cc
Published-As: https://github.com/dscho/git/releases/tag/reword-commit-msg-hook-v1
Fetch-It-Via: git fetch https://github.com/dscho/git reword-commit-msg-hook-v1

-- 
2.12.1.windows.1

[PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

The `reword` command of an interactive rebase used to call the
commit-msg hooks, but that regressed when we switched to the
rebase--helper backed by the sequencer.

Noticed by Sebastian Schuberth.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7504-commit-msg-hook.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 8728db61d38..c3d9ab02a3b 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -220,4 +220,21 @@ test_expect_success "hook doesn't edit commit message (editor)" '
 
 '
 
+# set up fake editor to replace `pick` by `reword`
+cat > reword-editor <<'EOF'
+#!/bin/sh
+mv "$1" "$1".bup &&
+sed 's/^pick/reword/' <"$1".bup >"$1"
+EOF
+chmod +x reword-editor
+REWORD_EDITOR="$(pwd)/reword-editor"
+export REWORD_EDITOR
+
+test_expect_failure 'hook is called for reword during `rebase -i`' '
+
+	GIT_SEQUENCE_EDITOR="\"$REWORD_EDITOR\"" git rebase -i HEAD^ &&
+	commit_msg_is "new message"
+
+'
+
 test_done
-- 
2.12.1.windows.1

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

[Johannes Schindelin]

It was pointed out during review of the sequencer-i patch series (which
taught the sequencer to execute an interactive rebase) that it may be
cumbersome to keep extending the signature of the run_git_commit()
function whenever a new commit option is needed.

While that concern had merit, back then I was reluctant to change even
more than was already asked for (which typically introduces regressions,
this late in the review process, which is no fun for nobody).

Now, with fresh eyes, and with an actual need, is a good time to change
the strategy from adding individual flag parameters to coalescing them
into a single flags parameter.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8183a83c1fa..1abe559fe86 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -602,6 +602,11 @@ N_("you have staged changes in your working tree\n"
 "\n"
 "  git rebase --continue\n");
 
+#define ALLOW_EMPTY (1<<0)
+#define EDIT_MSG    (1<<1)
+#define AMEND_MSG   (1<<2)
+#define CLEANUP_MSG (1<<3)
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
-			  int allow_empty, int edit, int amend,
-			  int cleanup_commit_message)
+			  int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts)) {
-		if (!edit) {
+		if (!(flags & EDIT_MSG)) {
 			cmd.stdout_to_stderr = 1;
 			cmd.err = -1;
 		}
@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_push(&cmd.args, "commit");
 	argv_array_push(&cmd.args, "-n");
 
-	if (amend)
+	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
 		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 		argv_array_push(&cmd.args, "-s");
 	if (defmsg)
 		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
-	if (cleanup_commit_message)
+	if ((flags & CLEANUP_MSG))
 		argv_array_push(&cmd.args, "--cleanup=strip");
-	if (edit)
+	if ((flags & EDIT_MSG))
 		argv_array_push(&cmd.args, "-e");
-	else if (!cleanup_commit_message &&
+	else if (!(flags & CLEANUP_MSG) &&
 		 !opts->signoff && !opts->record_origin &&
 		 git_config_get_value("commit.cleanup", &value))
 		argv_array_push(&cmd.args, "--cleanup=verbatim");
 
-	if (allow_empty)
+	if ((flags & ALLOW_EMPTY))
 		argv_array_push(&cmd.args, "--allow-empty");
 
 	if (opts->allow_empty_message)
@@ -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();
+	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;
 
 	if (opts->no_commit) {
 		/*
@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			opts);
 		if (res || command != TODO_REWORD)
 			goto leave;
-		edit = amend = 1;
+		flags |= EDIT_MSG | AMEND_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		edit = 1;
+		flags |= EDIT_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
-		amend = 1;
+		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			cleanup_commit_message = 1;
+			flags |= CLEANUP_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path("SQUASH_MSG");
@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 					     rebase_path_squash_msg(), dest);
 			unlink(git_path("MERGE_MSG"));
 			msg_file = dest;
-			edit = 1;
+			flags |= EDIT_MSG;
 		}
 	}
 
@@ -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;
 	if (!opts->no_commit)
 fast_forward_edit:
-		res = run_git_commit(msg_file, opts, allow, edit, amend,
-				     cleanup_commit_message);
+		res = run_git_commit(msg_file, opts, flags);
 
 	if (!res && final_fixup) {
 		unlink(rebase_path_fixup_msg());
@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)
 
 static int commit_staged_changes(struct replay_opts *opts)
 {
-	int amend = 0;
+	int flags = ALLOW_EMPTY | EDIT_MSG;
 
 	if (has_unstaged_changes(1))
 		return error(_("cannot rebase: You have unstaged changes."));
@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)
 				       "--continue' again."));
 
 		strbuf_release(&rev);
-		amend = 1;
+		flags |= AMEND_MSG;
 	}
 
-	if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+	if (run_git_commit(rebase_path_message(), opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
 	return 0;
-- 
2.12.1.windows.1

[PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Johannes Schindelin]

The `reword` command used to call `git commit` in a manner that asks for
the prepare-commit-msg and commit-msg hooks to do their thing.

Converting that part of the interactive rebase to C code introduced the
regression where those hooks were no longer run.

Let's fix this.

Note: the flag is called `VERIFY_MSG` instead of the more intuitive
`RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the
`--no-verify` flag (which may do other things in the future in addition
to suppressing the commit message hooks, too).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c                | 12 +++++++++---
 t/t7504-commit-msg-hook.sh |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1abe559fe86..377af91c475 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
 #define EDIT_MSG    (1<<1)
 #define AMEND_MSG   (1<<2)
 #define CLEANUP_MSG (1<<3)
+#define VERIFY_MSG  (1<<4)
 
 /*
  * If we are cherry-pick, and if the merge did not result in
@@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	}
 
 	argv_array_push(&cmd.args, "commit");
-	argv_array_push(&cmd.args, "-n");
 
+	if (!(flags & VERIFY_MSG))
+		argv_array_push(&cmd.args, "-n");
 	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
@@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			write_author_script(msg.message);
 		res = fast_forward_to(commit->object.oid.hash, head, unborn,
 			opts);
-		if (res || command != TODO_REWORD)
+		if (res)
+			goto leave;
+		else if (command == TODO_REWORD)
+			flags |= VERIFY_MSG;
+		else
 			goto leave;
 		flags |= EDIT_MSG | AMEND_MSG;
 		msg_file = NULL;
@@ -1050,7 +1056,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		flags |= EDIT_MSG;
+		flags |= EDIT_MSG | VERIFY_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index c3d9ab02a3b..88d4cda2992 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -230,7 +230,7 @@ chmod +x reword-editor
 REWORD_EDITOR="$(pwd)/reword-editor"
 export REWORD_EDITOR
 
-test_expect_failure 'hook is called for reword during `rebase -i`' '
+test_expect_success 'hook is called for reword during `rebase -i`' '
 
 	GIT_SEQUENCE_EDITOR="\"$REWORD_EDITOR\"" git rebase -i HEAD^ &&
 	commit_msg_is "new message"
-- 
2.12.1.windows.1

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Sebastian Schuberth]

On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:

> Noticed by Sebastian Schuberth.

Thanks for working on the fix.

> +# set up fake editor to replace `pick` by `reword`
> +cat > reword-editor <<'EOF'
> +#!/bin/sh
> +mv "$1" "$1".bup &&
> +sed 's/^pick/reword/' <"$1".bup >"$1"
> +EOF

Maybe use

sed -i 's/^pick/reword/' "$1"

here to avoid renaming the input file? Not sure how portable -i for
sed is, though. Otherwise, maybe remove the file "$1".bup afterwards
to be clean?

-- 
Sebastian Schuberth

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

Hi Sebastian,

On Wed, 22 Mar 2017, Sebastian Schuberth wrote:

> On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> 
> > +# set up fake editor to replace `pick` by `reword`
> > +cat > reword-editor <<'EOF'
> > +#!/bin/sh
> > +mv "$1" "$1".bup &&
> > +sed 's/^pick/reword/' <"$1".bup >"$1"
> > +EOF
> 
> Maybe use
> 
> sed -i 's/^pick/reword/' "$1"

It's not portable, otherwise I would have used it. GNU sed expects an
optional extension to be used for a backup file, and that optional
extension needs to be specified without whitespace between it and the -i
option. BSD sed *requires* an extension to be specified, and it has to be
separated using white space (or is that orange space now?).

So even if we could make that call portable somehow (and remember, BSD sed
is what OSX uses), there *would be a backup file*.

> here to avoid renaming the input file? Not sure how portable -i for
> sed is, though. Otherwise, maybe remove the file "$1".bup afterwards
> to be clean?

No. The file in question is written to the .git/rebase-merge/
subdirectory, therefore cleaned up after the rebase.

In any case, this is *test* code. So I'd prefer to have the changes to the
C code scrutinized a bit more, not the test code as long as it is obvious
what it does.

Ciao,
Johannes

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>
>> Noticed by Sebastian Schuberth.
>
> Thanks for working on the fix.
>
>> +# set up fake editor to replace `pick` by `reword`
>> +cat > reword-editor <<'EOF'
>> +#!/bin/sh
>> +mv "$1" "$1".bup &&
>> +sed 's/^pick/reword/' <"$1".bup >"$1"
>> +EOF
>
> Maybe use
>
> sed -i 's/^pick/reword/' "$1"
>
> here to avoid renaming the input file? Not sure how portable -i for
> sed is, though. Otherwise, maybe remove the file "$1".bup afterwards
> to be clean?

"-i" is GNUism and it is a good idea to avoid it, but cleaning after
itself may be worth doing to avoid contaminating the working tree.

Thanks.

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In any case, this is *test* code. So I'd prefer to have the changes to the
> C code scrutinized a bit more, not the test code as long as it is obvious
> what it does.

I do not think the tone of this comment serves any productive
purpose.  People have strengths in different areas, and those who
can spot issues in tests better than the C code should not be
discouraged from suggesting improvements by getting scolded like
this.

Also remember what is "obvious" to you may not be obvious to others.

Thanks.

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> +#define ALLOW_EMPTY (1<<0)
> +#define EDIT_MSG    (1<<1)
> +#define AMEND_MSG   (1<<2)
> +#define CLEANUP_MSG (1<<3)

These being bits makes it clear that they can be independently set
and unset.

> @@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
>   * author metadata.
>   */
>  static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit, int amend,
> -			  int cleanup_commit_message)
> +			  int flags)

Use "unsigned" not signed integer for a collection of bits (see
e.g. GSoC microproject ideas page).

> @@ -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;

;-)  Much more descriptive than just "allow".

>  static int commit_staged_changes(struct replay_opts *opts)
>  {
> -	int amend = 0;
> +	int flags = ALLOW_EMPTY | EDIT_MSG;
>  
>  	if (has_unstaged_changes(1))
>  		return error(_("cannot rebase: You have unstaged changes."));
> @@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)
>  				       "--continue' again."));
>  
>  		strbuf_release(&rev);
> -		amend = 1;
> +		flags |= AMEND_MSG;
>  	}
>  
> -	if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
> +	if (run_git_commit(rebase_path_message(), opts, flags))

OK, the initialization of "flags" corresponds to these "1, 1" in the
original.

Overall, much easier to understand (and to extend).  Good
maintaintability clean-up.

Thanks.

Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The `reword` command used to call `git commit` in a manner that asks for
> the prepare-commit-msg and commit-msg hooks to do their thing.
>
> Converting that part of the interactive rebase to C code introduced the
> regression where those hooks were no longer run.
>
> Let's fix this.
>
> Note: the flag is called `VERIFY_MSG` instead of the more intuitive
> `RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the
> `--no-verify` flag (which may do other things in the future in addition
> to suppressing the commit message hooks, too).

Yup, this is a sound reasoning.  I would have made it not a "Note"
but just a regular description of the design decision, e.g.

    Call the flag bit "VERIFY_MSG", because this is to suppress the
    "--no-verify" flag (do not call it RUN_COMMIT_MSG_HOOKS, as the
    purpose of the bit does not have to stay only to run the hooks).

or somesuch, but I can go either way.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c                | 12 +++++++++---
>  t/t7504-commit-msg-hook.sh |  2 +-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 1abe559fe86..377af91c475 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
>  #define EDIT_MSG    (1<<1)
>  #define AMEND_MSG   (1<<2)
>  #define CLEANUP_MSG (1<<3)
> +#define VERIFY_MSG  (1<<4)
>  
>  /*
>   * If we are cherry-pick, and if the merge did not result in
> @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	}
>  
>  	argv_array_push(&cmd.args, "commit");
> -	argv_array_push(&cmd.args, "-n");
>  
> +	if (!(flags & VERIFY_MSG))
> +		argv_array_push(&cmd.args, "-n");

OK, so by default we pass "--no-verify" but selected callers can
set the bit in the flags word to disable it.  That sounds sensible,
especially given that the original callers, cherry-pick and revert, 
did want "--no-verify".  "reword" in "rebase -i" is currently the
only one we want to supress "--no-verify" and the place that does so
are all shown in this patch.

Just to see if there are other callers that may want to do the same
suppressing of "--no-verify" as a follow-up, I looked at the whole
file after applying the patch, and I think everything looked good
as-is.

>  	if ((flags & AMEND_MSG))
>  		argv_array_push(&cmd.args, "--amend");
>  	if (opts->gpg_sign)
> @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  			write_author_script(msg.message);
>  		res = fast_forward_to(commit->object.oid.hash, head, unborn,
>  			opts);
> -		if (res || command != TODO_REWORD)
> +		if (res)
> +			goto leave;
> +		else if (command == TODO_REWORD)
> +			flags |= VERIFY_MSG;
> +		else
>  			goto leave;

Both before and after are your code, but wouldn't this:

	if (res || command != TODO_REWORD)
		goto leave;
+	if (command == TODO_REWORD)
+		flags |= VERIFY_MSG

result in smaller changes relative to the original and still be much
easier to understand than the above?

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > @@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
> >   * author metadata.
> >   */
> >  static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> > -			  int allow_empty, int edit, int amend,
> > -			  int cleanup_commit_message)
> > +			  int flags)
> 
> Use "unsigned" not signed integer for a collection of bits (see
> e.g. GSoC microproject ideas page).

True. Fixed in preparation for v2.

Ciao,
Dscho

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

> Sebastian Schuberth <sschuberth@gmail.com> writes:
> 
> > On Wed, Mar 22, 2017 at 4:01 PM, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> >
> >> Noticed by Sebastian Schuberth.
> >
> > Thanks for working on the fix.
> >
> >> +# set up fake editor to replace `pick` by `reword`
> >> +cat > reword-editor <<'EOF'
> >> +#!/bin/sh
> >> +mv "$1" "$1".bup &&
> >> +sed 's/^pick/reword/' <"$1".bup >"$1"
> >> +EOF
> >
> > Maybe use
> >
> > sed -i 's/^pick/reword/' "$1"
> >
> > here to avoid renaming the input file? Not sure how portable -i for
> > sed is, though. Otherwise, maybe remove the file "$1".bup afterwards
> > to be clean?
> 
> "-i" is GNUism and it is a good idea to avoid it, but cleaning after
> itself may be worth doing to avoid contaminating the working tree.

This script is only used as *sequence* editor, i.e. for
.git/merge-rebase/git-rebase-todo. As such, I find it valuable to have the
original version in the same directory, and the file is cleaned up upon
success by the `git rebase -i` command anyway.

In that light, I am sure you agree that the current code is fine.

Ciao,
Dscho

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In any case, this is *test* code. So I'd prefer to have the changes to
> > the C code scrutinized a bit more, not the test code as long as it is
> > obvious what it does.
> 
> [...] People have strengths in different areas, and those who can spot
> issues in tests better than the C code should not be discouraged from
> suggesting improvements by getting scolded like this.

I know Sebastian well, and I would hope that he lends his substantial
competence to making sure that the changes that affect end users are
correct and that I do not introduce another regression.

Besides, he is German, so I tried to spell it out clearly what I wish him
to do in return for my addressing his problem.

That was the entire reasoning behind my "scolding". I am sorry that you
understood it to be meant negatively. It was not.

Ciao,
Johannes

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Sebastian Schuberth]

On Thu, Mar 23, 2017 at 3:43 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> I know Sebastian well, and I would hope that he lends his substantial
> competence to making sure that the changes that affect end users are
> correct and that I do not introduce another regression.

If you'd really know me, you would also know that I only say something
*if* I have to say something. I.e. me not making any remarks regarding
the C code means I did not find any issues.

> Besides, he is German, so I tried to spell it out clearly what I wish him
> to do in return for my addressing his problem.

I fail to see how this is "my" problem just because I happened to
notice it first. While I'm grateful that you've addressed it timely, I
believe this is a naturalness since you've introduced the regression.

-- 
Sebastian Schuberth

Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 1abe559fe86..377af91c475 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
> >  #define EDIT_MSG    (1<<1)
> >  #define AMEND_MSG   (1<<2)
> >  #define CLEANUP_MSG (1<<3)
> > +#define VERIFY_MSG  (1<<4)
> >  
> >  /*
> >   * If we are cherry-pick, and if the merge did not result in
> > @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> >  	}
> >  
> >  	argv_array_push(&cmd.args, "commit");
> > -	argv_array_push(&cmd.args, "-n");
> >  
> > +	if (!(flags & VERIFY_MSG))
> > +		argv_array_push(&cmd.args, "-n");
> 
> OK, so by default we pass "--no-verify" but selected callers can
> set the bit in the flags word to disable it.  That sounds sensible,
> especially given that the original callers, cherry-pick and revert, 
> did want "--no-verify".  "reword" in "rebase -i" is currently the
> only one we want to supress "--no-verify" and the place that does so
> are all shown in this patch.

Indeed, my reasoning was to keep the default to be the previous behavior.

> Just to see if there are other callers that may want to do the same
> suppressing of "--no-verify" as a follow-up, I looked at the whole
> file after applying the patch, and I think everything looked good
> as-is.

Thank you for being thorough; That is exactly the type of review I hoped
for. I did the same research myself, of course, but it is most reassuring
if an independent reviewer comes to the same conclusion.

> > @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  			write_author_script(msg.message);
> >  		res = fast_forward_to(commit->object.oid.hash, head, unborn,
> >  			opts);
> > -		if (res || command != TODO_REWORD)
> > +		if (res)
> > +			goto leave;
> > +		else if (command == TODO_REWORD)
> > +			flags |= VERIFY_MSG;
> > +		else
> >  			goto leave;
> 
> Both before and after are your code, but wouldn't this:
> 
> 	if (res || command != TODO_REWORD)
> 		goto leave;
> +	if (command == TODO_REWORD)
> +		flags |= VERIFY_MSG
> 
> result in smaller changes relative to the original and still be much
> easier to understand than the above?

Yes. I just did not like the repetition.

But thinking about it again, the previous logic was only concerned about
an early exit, and the clause I added is all about the flags. Therefore, I
agree with you that it should be untangled again.

v2 coming,
Dscho

Re: [PATCH 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

Hi Sebastian,

On Thu, 23 Mar 2017, Sebastian Schuberth wrote:

> On Thu, Mar 23, 2017 at 3:43 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> 
> > I know Sebastian well, and I would hope that he lends his substantial
> > competence to making sure that the changes that affect end users are
> > correct and that I do not introduce another regression.
> 
> If you'd really know me, you would also know that I only say something
> *if* I have to say something.

Okay, then, I sit corrected: I do not know you well.

> I.e. me not making any remarks regarding the C code means I did not find
> any issues.

Sadly, this is indistinguishable from you having not bothered to read the
patches to the C code at all.

So an explicit "I have read the patch and found no fault" or even better,
a review with some indication that you were thorough would have been a
nice gesture.

Ciao,
Johannes

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> It was pointed out during review of the sequencer-i patch series (which
> taught the sequencer to execute an interactive rebase) that it may be
> cumbersome to keep extending the signature of the run_git_commit()
> function whenever a new commit option is needed.
>
> While that concern had merit, back then I was reluctant to change even
> more than was already asked for (which typically introduces regressions,
> this late in the review process, which is no fun for nobody).
>
> Now, with fresh eyes, and with an actual need, is a good time to change
> the strategy from adding individual flag parameters to coalescing them
> into a single flags parameter.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I think the first two paragraphs are not about explaining this
commit.  You do not have to justify the past---some suggestions
given during the review may end up not getting followed for
different reasons, which may be good or bad, but what happened has
happened, and the backstory is not useful for understanding why this
is a good change to a reader of "git log".

What belongs to the explanation / justification for this change is
that the interface to run_git_commit() that forces to add one
boolean parameter whenever a new "switch" is required is cumbersome
and it is better to use a flags word that is a collection of bits.
When you reroll this for s/signed int/unsigned int/, please just
describe that in the log. Having the backstory under three-dash
lines may help reviewers who remember the original topic that
introduced the maintenance burden this commit fixes.

Thanks.

[PATCH v2 0/3] rebase -i (reword): call the commit-msg hook again

[Johannes Schindelin]

It is actually not only the commit-msg, but also the prepare-commit-msg
hook...

Changes since v1:

- changed `int flags` to `unsigned int flags`

- uncuddled the if...else if... construct to not mix the "goto leave"
  logic with the one adding the VERIFY_MSG flag


Johannes Schindelin (3):
  t7504: document regression: reword no longer calls commit-msg
  sequencer: make commit options more extensible
  sequencer: allow the commit-msg hooks to run during a `reword`

 sequencer.c                | 54 ++++++++++++++++++++++++++--------------------
 t/t7504-commit-msg-hook.sh | 17 +++++++++++++++
 2 files changed, 48 insertions(+), 23 deletions(-)


base-commit: afd6726309f57f532b4b989a75c1392359c611cc
Published-As: https://github.com/dscho/git/releases/tag/reword-commit-msg-hook-v2
Fetch-It-Via: git fetch https://github.com/dscho/git reword-commit-msg-hook-v2

Interdiff vs v1:

 diff --git a/sequencer.c b/sequencer.c
 index 377af91c475..bc2fe48e65c 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -621,7 +621,7 @@ N_("you have staged changes in your working tree\n"
   * author metadata.
   */
  static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 -			  int flags)
 +			  unsigned int flags)
  {
  	struct child_process cmd = CHILD_PROCESS_INIT;
  	const char *value;
 @@ -932,7 +932,7 @@ 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 flags = opts->edit ? EDIT_MSG : 0, allow = 0;
 +	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;
 @@ -995,13 +995,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
  			write_author_script(msg.message);
  		res = fast_forward_to(commit->object.oid.hash, head, unborn,
  			opts);
 -		if (res)
 -			goto leave;
 -		else if (command == TODO_REWORD)
 -			flags |= VERIFY_MSG;
 -		else
 +		if (res || command != TODO_REWORD)
  			goto leave;
  		flags |= EDIT_MSG | AMEND_MSG;
 +		if (command == TODO_REWORD)
 +			flags |= VERIFY_MSG;
  		msg_file = NULL;
  		goto fast_forward_edit;
  	}
 @@ -2164,7 +2162,7 @@ static int continue_single_pick(void)
  
  static int commit_staged_changes(struct replay_opts *opts)
  {
 -	int flags = ALLOW_EMPTY | EDIT_MSG;
 +	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
  
  	if (has_unstaged_changes(1))
  		return error(_("cannot rebase: You have unstaged changes."));

-- 
2.12.1.windows.1

[PATCH v2 1/3] t7504: document regression: reword no longer calls commit-msg

[Johannes Schindelin]

The `reword` command of an interactive rebase used to call the
commit-msg hooks, but that regressed when we switched to the
rebase--helper backed by the sequencer.

Noticed by Sebastian Schuberth.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7504-commit-msg-hook.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 8728db61d38..c3d9ab02a3b 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -220,4 +220,21 @@ test_expect_success "hook doesn't edit commit message (editor)" '
 
 '
 
+# set up fake editor to replace `pick` by `reword`
+cat > reword-editor <<'EOF'
+#!/bin/sh
+mv "$1" "$1".bup &&
+sed 's/^pick/reword/' <"$1".bup >"$1"
+EOF
+chmod +x reword-editor
+REWORD_EDITOR="$(pwd)/reword-editor"
+export REWORD_EDITOR
+
+test_expect_failure 'hook is called for reword during `rebase -i`' '
+
+	GIT_SEQUENCE_EDITOR="\"$REWORD_EDITOR\"" git rebase -i HEAD^ &&
+	commit_msg_is "new message"
+
+'
+
 test_done
-- 
2.12.1.windows.1

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

[Johannes Schindelin]

It was pointed out during review of the sequencer-i patch series (which
taught the sequencer to execute an interactive rebase) that it may be
cumbersome to keep extending the signature of the run_git_commit()
function whenever a new commit option is needed.

While that concern had merit, back then I was reluctant to change even
more than was already asked for (which typically introduces regressions,
this late in the review process, which is no fun for nobody).

Now, with fresh eyes, and with an actual need, is a good time to change
the strategy from adding individual flag parameters to coalescing them
into a single flags parameter.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8183a83c1fa..ce05d61a2ae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -602,6 +602,11 @@ N_("you have staged changes in your working tree\n"
 "\n"
 "  git rebase --continue\n");
 
+#define ALLOW_EMPTY (1<<0)
+#define EDIT_MSG    (1<<1)
+#define AMEND_MSG   (1<<2)
+#define CLEANUP_MSG (1<<3)
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
-			  int allow_empty, int edit, int amend,
-			  int cleanup_commit_message)
+			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts)) {
-		if (!edit) {
+		if (!(flags & EDIT_MSG)) {
 			cmd.stdout_to_stderr = 1;
 			cmd.err = -1;
 		}
@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_push(&cmd.args, "commit");
 	argv_array_push(&cmd.args, "-n");
 
-	if (amend)
+	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
 		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 		argv_array_push(&cmd.args, "-s");
 	if (defmsg)
 		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
-	if (cleanup_commit_message)
+	if ((flags & CLEANUP_MSG))
 		argv_array_push(&cmd.args, "--cleanup=strip");
-	if (edit)
+	if ((flags & EDIT_MSG))
 		argv_array_push(&cmd.args, "-e");
-	else if (!cleanup_commit_message &&
+	else if (!(flags & CLEANUP_MSG) &&
 		 !opts->signoff && !opts->record_origin &&
 		 git_config_get_value("commit.cleanup", &value))
 		argv_array_push(&cmd.args, "--cleanup=verbatim");
 
-	if (allow_empty)
+	if ((flags & ALLOW_EMPTY))
 		argv_array_push(&cmd.args, "--allow-empty");
 
 	if (opts->allow_empty_message)
@@ -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;
 
 	if (opts->no_commit) {
 		/*
@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			opts);
 		if (res || command != TODO_REWORD)
 			goto leave;
-		edit = amend = 1;
+		flags |= EDIT_MSG | AMEND_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		edit = 1;
+		flags |= EDIT_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
-		amend = 1;
+		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			cleanup_commit_message = 1;
+			flags |= CLEANUP_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path("SQUASH_MSG");
@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 					     rebase_path_squash_msg(), dest);
 			unlink(git_path("MERGE_MSG"));
 			msg_file = dest;
-			edit = 1;
+			flags |= EDIT_MSG;
 		}
 	}
 
@@ -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;
 	if (!opts->no_commit)
 fast_forward_edit:
-		res = run_git_commit(msg_file, opts, allow, edit, amend,
-				     cleanup_commit_message);
+		res = run_git_commit(msg_file, opts, flags);
 
 	if (!res && final_fixup) {
 		unlink(rebase_path_fixup_msg());
@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)
 
 static int commit_staged_changes(struct replay_opts *opts)
 {
-	int amend = 0;
+	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
 
 	if (has_unstaged_changes(1))
 		return error(_("cannot rebase: You have unstaged changes."));
@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)
 				       "--continue' again."));
 
 		strbuf_release(&rev);
-		amend = 1;
+		flags |= AMEND_MSG;
 	}
 
-	if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+	if (run_git_commit(rebase_path_message(), opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
 	return 0;
-- 
2.12.1.windows.1

[PATCH v2 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Johannes Schindelin]

The `reword` command used to call `git commit` in a manner that asks for
the prepare-commit-msg and commit-msg hooks to do their thing.

Converting that part of the interactive rebase to C code introduced the
regression where those hooks were no longer run.

Let's fix this.

Note: the flag is called `VERIFY_MSG` instead of the more intuitive
`RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the
`--no-verify` flag (which may do other things in the future in addition
to suppressing the commit message hooks, too).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c                | 8 ++++++--
 t/t7504-commit-msg-hook.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ce05d61a2ae..bc2fe48e65c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
 #define EDIT_MSG    (1<<1)
 #define AMEND_MSG   (1<<2)
 #define CLEANUP_MSG (1<<3)
+#define VERIFY_MSG  (1<<4)
 
 /*
  * If we are cherry-pick, and if the merge did not result in
@@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	}
 
 	argv_array_push(&cmd.args, "commit");
-	argv_array_push(&cmd.args, "-n");
 
+	if (!(flags & VERIFY_MSG))
+		argv_array_push(&cmd.args, "-n");
 	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
@@ -996,6 +998,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 		if (res || command != TODO_REWORD)
 			goto leave;
 		flags |= EDIT_MSG | AMEND_MSG;
+		if (command == TODO_REWORD)
+			flags |= VERIFY_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
@@ -1050,7 +1054,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		flags |= EDIT_MSG;
+		flags |= EDIT_MSG | VERIFY_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index c3d9ab02a3b..88d4cda2992 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -230,7 +230,7 @@ chmod +x reword-editor
 REWORD_EDITOR="$(pwd)/reword-editor"
 export REWORD_EDITOR
 
-test_expect_failure 'hook is called for reword during `rebase -i`' '
+test_expect_success 'hook is called for reword during `rebase -i`' '
 
 	GIT_SEQUENCE_EDITOR="\"$REWORD_EDITOR\"" git rebase -i HEAD^ &&
 	commit_msg_is "new message"
-- 
2.12.1.windows.1

Re: [PATCH v2 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> @@ -996,6 +998,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  		if (res || command != TODO_REWORD)
>  			goto leave;
>  		flags |= EDIT_MSG | AMEND_MSG;
> +		if (command == TODO_REWORD)
> +			flags |= VERIFY_MSG;

Good.  This looks even cleaner than what we discussed during the
review of the previous round.  After deciding not to return early,
we set the default set of features in flags and then further add
verify-msg in when needed.  Much easier to understand.

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

[Junio C Hamano]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Making "flags" unsigned was a correct change, but this is now wrong,
>> as "allow" is made unsigned by accident.
> ...
>
> 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).

I do not think the variable needs to be initialized to anything (it
is not looked at until it gets the result from allow_empty() call).

Anyway, the series is not yet in 'next', so you can replace it to
the shape you would have made it into if you noticed that "allow"
shouldn't be unsigned, while updating the log message to explain
what the change is about (instead of only attempting to justify the
past, which is not interesting in the context of understanding what
this change is about).

As this thing is about fixing a regression visible to end users, I
may get around to fixing things up myself, but I have other topics
to attend to, so...

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As this thing is about fixing a regression visible to end users, I
> may get around to fixing things up myself, but I have other topics
> to attend to, so...

So I ended up with this version before merging it to 'next'.  

I moved 'allow' back on the line it is declared, but left it
uninitialized because it is unconditionally assigned to before its
value is looked at.

Also the updated log message stresses more about the reason why
piling new parameters on top is a bad practice, and it is a good
idea to do this clean-up now.  Compared to that, the reason why this
clean-up was not done before and left as maintenance burden is much
less important to the readers of the log.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Thu, 23 Mar 2017 17:07:11 +0100
Subject: [PATCH] sequencer: make commit options more extensible

So far every time we need to tweak the behaviour of run_git_commit()
we have been adding a "int" parameter to it.  As the function gains
parameters and different callsites having different needs, this is
becoming a maintenance burden.  When a new knob needs to be added to
address a specific need for a single callsite, all the other callsites
need to add a "no, I do not want anything special with respect to the
new knob" argument.

Consolidate the existing four parameters into a flag word to make it
more maintainable, as we will be adding a new one to the mix soon.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8183a83c1f..677e6ef764 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -602,6 +602,11 @@ N_("you have staged changes in your working tree\n"
 "\n"
 "  git rebase --continue\n");
 
+#define ALLOW_EMPTY (1<<0)
+#define EDIT_MSG    (1<<1)
+#define AMEND_MSG   (1<<2)
+#define CLEANUP_MSG (1<<3)
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -615,8 +620,7 @@ N_("you have staged changes in your working tree\n"
  * author metadata.
  */
 static int run_git_commit(const char *defmsg, struct replay_opts *opts,
-			  int allow_empty, int edit, int amend,
-			  int cleanup_commit_message)
+			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
@@ -624,7 +628,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts)) {
-		if (!edit) {
+		if (!(flags & EDIT_MSG)) {
 			cmd.stdout_to_stderr = 1;
 			cmd.err = -1;
 		}
@@ -640,7 +644,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_push(&cmd.args, "commit");
 	argv_array_push(&cmd.args, "-n");
 
-	if (amend)
+	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
 	if (opts->gpg_sign)
 		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
@@ -648,16 +652,16 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 		argv_array_push(&cmd.args, "-s");
 	if (defmsg)
 		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
-	if (cleanup_commit_message)
+	if ((flags & CLEANUP_MSG))
 		argv_array_push(&cmd.args, "--cleanup=strip");
-	if (edit)
+	if ((flags & EDIT_MSG))
 		argv_array_push(&cmd.args, "-e");
-	else if (!cleanup_commit_message &&
+	else if (!(flags & CLEANUP_MSG) &&
 		 !opts->signoff && !opts->record_origin &&
 		 git_config_get_value("commit.cleanup", &value))
 		argv_array_push(&cmd.args, "--cleanup=verbatim");
 
-	if (allow_empty)
+	if ((flags & ALLOW_EMPTY))
 		argv_array_push(&cmd.args, "--allow-empty");
 
 	if (opts->allow_empty_message)
@@ -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;
+	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, allow;
 
 	if (opts->no_commit) {
 		/*
@@ -991,7 +995,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			opts);
 		if (res || command != TODO_REWORD)
 			goto leave;
-		edit = amend = 1;
+		flags |= EDIT_MSG | AMEND_MSG;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}
@@ -1046,15 +1050,15 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 	}
 
 	if (command == TODO_REWORD)
-		edit = 1;
+		flags |= EDIT_MSG;
 	else if (is_fixup(command)) {
 		if (update_squash_messages(command, commit, opts))
 			return -1;
-		amend = 1;
+		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			cleanup_commit_message = 1;
+			flags |= CLEANUP_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path("SQUASH_MSG");
@@ -1064,7 +1068,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 					     rebase_path_squash_msg(), dest);
 			unlink(git_path("MERGE_MSG"));
 			msg_file = dest;
-			edit = 1;
+			flags |= EDIT_MSG;
 		}
 	}
 
@@ -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;
 	if (!opts->no_commit)
 fast_forward_edit:
-		res = run_git_commit(msg_file, opts, allow, edit, amend,
-				     cleanup_commit_message);
+		res = run_git_commit(msg_file, opts, flags);
 
 	if (!res && final_fixup) {
 		unlink(rebase_path_fixup_msg());
@@ -2154,7 +2158,7 @@ static int continue_single_pick(void)
 
 static int commit_staged_changes(struct replay_opts *opts)
 {
-	int amend = 0;
+	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
 
 	if (has_unstaged_changes(1))
 		return error(_("cannot rebase: You have unstaged changes."));
@@ -2184,10 +2188,10 @@ static int commit_staged_changes(struct replay_opts *opts)
 				       "--continue' again."));
 
 		strbuf_release(&rev);
-		amend = 1;
+		flags |= AMEND_MSG;
 	}
 
-	if (run_git_commit(rebase_path_message(), opts, 1, 1, amend, 0))
+	if (run_git_commit(rebase_path_message(), opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
 	return 0;
-- 
2.12.2-430-ge0ef7fe78c

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

[Johannes Schindelin]

Hi Junio,

On Mon, 27 Mar 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > As this thing is about fixing a regression visible to end users, I
> > may get around to fixing things up myself, but I have other topics
> > to attend to, so...
> 
> So I ended up with this version before merging it to 'next'.  

Thanks,
Dscho