Re: [PATCH 3/3] Fix commit-msg hook to allow editing

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

Wincent Colaiuta <win@wincent.com> writes:

> The old git-commit.sh script allowed the commit-msg hook to not only
> prevent a commit from proceding, but also to edit the commit message
> on the fly and allow it to proceed. So here we teach builtin-commit
> to do the same.

This is a bit unfortunate.

> @@ -787,14 +787,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		const char *env[2] = { index, NULL };
>  		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
>  		launch_editor(git_path(commit_editmsg), &sb, env);

Here, launch-editor lets the user edit an external file, and reads it
back into &sb, but ...

>  	}
> +	if (!no_verify) {
> +		if (run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
> +			rollback_index_files();
> +			exit(1);
> +		}
> +		strbuf_setlen(&sb, header_len);
> +	}

we need to discard it (I would prefer setlen to be done before running
the hook, just to make the logic flow more natural), because we need to
read it back again anyway.  On the other hand, if we do not start the
editor, we used to read the contents before running the hook, which was
wrong, and the call to read the file is moved after, like this:

> +	if ((no_edit || !no_verify) &&
> +	    strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
>  		rollback_index_files();
> +		die("could not read commit message");
>  	}

which makes the logic to decide if we read the file back again
unnecessarily convoluted.  The reason why (no_edit || !no_verify) is
there is very closely tied to the fact that you happened to have already
read in the launch_editor() codepath but not yet in no_edit codepath.
This feels very error prone.

	Side note. an unrelated reason where this convolution comes from
	is the unfortunate naming of many options and double-negations
	they bring in.  A normal human being needs to read expressions
	like "if (!no_foo || no_foo)" three times before understanding
	what is being checked.

I would suggest doing the above differently.

 * Allow launch_editor() not to read back the edited result into strbuf
   at all (perhaps pass NULL to signal that), and make "if (!no_edit)"
   codepath use it;

 * The codeflow would become:

	if (!no_edit) {
		launch editor, in "no readback" mode;
	}
	if (!no_verify) {
        	run hook, let it interfere;
	}
	read the file to &sb, no matter what no_edit/no_verify says.

IOW, how about something like this?

---

 builtin-commit.c |    9 +++++----
 builtin-tag.c    |    2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 2032ca3..30a9deb 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -787,16 +787,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		char index[PATH_MAX];
 		const char *env[2] = { index, NULL };
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		launch_editor(git_path(commit_editmsg), &sb, env);
-	} else if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
-		rollback_index_files();
-		die("could not read commit message");
+		launch_editor(git_path(commit_editmsg), NULL, env);
 	}
 	if (!no_verify &&
 	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
 		rollback_index_files();
 		exit(1);
 	}
+	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
+		rollback_index_files();
+		die("could not read commit message");
+	}
 
 	/* Truncate the message just before the diff, if any. */
 	p = strstr(sb.buf, "\ndiff --git a/");
diff --git a/builtin-tag.c b/builtin-tag.c
index 729389b..9f966fc 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -53,6 +53,8 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 			die("There was a problem with the editor %s.", editor);
 	}
 
+	if (!buffer)
+		return;
 	if (strbuf_read_file(buffer, path, 0) < 0)
 		die("could not read message file '%s': %s",
 		    path, strerror(errno));