Re: [PATCH 3/3] Fix commit-msg hook to allow editing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 9, 2007, 07:21 UTC
- Message-ID
- <7vtzmss58h.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1197113889-16243-4-git-send-email-win@wincent.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 ...
Show 8 quoted lines
> }
> + 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:
Show 5 quoted lines
> + 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));