Re: [RFC/PATCH] merge: honor prepare-commit-msg hook

Re: [RFC/PATCH] merge: honor prepare-commit-msg hook

[Jeffrey Middleton]

I'd like to add a voice to the support for calling the
prepare-commit-msg and post-commit hooks during a merge.

Reading the documentation, it seems like surely prepare-commit-msg
would be called: the description of the hook mentions that the second
argument might be "merge". The sample hook (also mentioned there)
targets only merge commits, but happens to work because it's designed
to only have an effect if there is a "Conflicts:" section, which
therefore means the user is committing manually. Another possible
merge-related use case for the hook would be intelligently rewriting
the subject, e.g. to canonicalize remote names in integration-style
merges.  If y'all don't agree, I'd suggest modifying the documentation
to clarify that the hook is only called for manually-committed merges.

My instinct is that post-commit makes sense too - if you want to print
some extra information after a commit is recorded, why should it just
be non-merges?

(and just in case, this is a reply to an old thread, including a
proposed patch:
http://thread.gmane.org/gmane.comp.version-control.git/151297/ )

Jeffrey

Junio C Hamano <gitster <at> pobox.com> writes:
> Jay Soffian <jaysoffian <at> gmail.com> writes:
> > ---
> > I couldn't figure out why my prepare-commit-msg wasn't being honored
> > by git merge.
>
> It has been that way from day one, it appears.
>
> The bypassing of pre-commit hook was and remains to be a conscious design
> decision.  When you are pulling from your contributors who may have
> objectionable contents that you have to merge, the damage is already
> done; you _could_ yell at them to fix their branch and re-pull in theory,
> but that wouldn't work very well in practice.
>
> On the other hand, I think letting people use prepare-commit-msg for
> merges might  make sense.  Indeed, "git commit" is prepared to call
> prepare-commit-msg telling the hook that it is concluding a merge, when
> your "git merge" stopped due to a conflict (or you stopped it from making
> a new commit with --no-commit).
>
> I don't know about the other hooks "git commit" normally calls.  Both
> "commit-msg" and "post-commit" may make sense, but I don't care too deeply
> either way---I don't care too deeply for pre-commit either ;-).

Re: [RFC/PATCH] merge: honor prepare-commit-msg hook

[Jay Soffian]

On Tue, Mar 8, 2011 at 7:16 PM, Jeffrey Middleton <jefromi@gmail.com> wrote:
> I'd like to add a voice to the support for calling the
> prepare-commit-msg and post-commit hooks during a merge.

See 65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14), now in
master. I didn't do anything about the other hooks though.

j.

[RFC/PATCH] merge: honor prepare-commit-msg hook

[Jay Soffian]

---
I couldn't figure out why my prepare-commit-msg wasn't being honored
by git merge. I assumed that the standard commit hooks would be honored,
as after-all, merge is creating a new commit. Alas, this is not the case
since merge calls commit_tree directly.

I wonder whether the low-level commit_tree should be responsible for
invoking this hook instead of the commit porcelain?

In any case, this does what I want, but I'm not sure it's the best approach.

Comments?

 builtin/merge.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 37ce4f5..28a9960 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -768,6 +768,33 @@ static void add_strategies(const char *string, unsigned attr)
 
 }
 
+static void write_merge_msg(void)
+{
+	int fd = open(git_path("MERGE_MSG"), O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno("Could not open '%s' for writing",
+			  git_path("MERGE_MSG"));
+	if (write_in_full(fd, merge_msg.buf, merge_msg.len) !=
+		merge_msg.len)
+		die_errno("Could not write to '%s'", git_path("MERGE_MSG"));
+	close(fd);
+}
+
+static void read_merge_msg(void)
+{
+	strbuf_reset(&merge_msg);
+	if (strbuf_read_file(&merge_msg, git_path("MERGE_MSG"), 0) < 0)
+		die_errno("Could not read from '%s'", git_path("MERGE_MSG"));
+}
+
+static void run_prepare_commit_msg(void)
+{
+	write_merge_msg();
+	run_hook(get_index_file(), "prepare-commit-msg",
+		 git_path("MERGE_MSG"), "merge", NULL, NULL);
+	read_merge_msg();
+}
+
 static int merge_trivial(void)
 {
 	unsigned char result_tree[20], result_commit[20];
@@ -779,6 +806,7 @@ static int merge_trivial(void)
 	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
+	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
 	finish(result_commit, "In-index merge");
 	drop_save();
@@ -808,6 +836,7 @@ static int finish_automerge(struct commit_list *common,
 	}
 	free_commit_list(remoteheads);
 	strbuf_addch(&merge_msg, '\n');
+	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
 	strbuf_addf(&buf, "Merge made by %s.", wt_strategy);
 	finish(result_commit, buf.buf);
@@ -1276,14 +1305,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			die_errno("Could not write to '%s'", git_path("MERGE_HEAD"));
 		close(fd);
 		strbuf_addch(&merge_msg, '\n');
-		fd = open(git_path("MERGE_MSG"), O_WRONLY | O_CREAT, 0666);
-		if (fd < 0)
-			die_errno("Could not open '%s' for writing",
-				  git_path("MERGE_MSG"));
-		if (write_in_full(fd, merge_msg.buf, merge_msg.len) !=
-			merge_msg.len)
-			die_errno("Could not write to '%s'", git_path("MERGE_MSG"));
-		close(fd);
+		write_merge_msg();
 		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
 		if (fd < 0)
 			die_errno("Could not open '%s' for writing",
-- 
1.7.2.rc3.53.g670aa

Re: [RFC/PATCH] merge: honor prepare-commit-msg hook

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> ---
> I couldn't figure out why my prepare-commit-msg wasn't being honored
> by git merge.

It has been that way from day one, it appears.

The bypassing of pre-commit hook was and remains to be a conscious design
decision.  When you are pulling from your contributors who may have
objectionable contents that you have to merge, the damage is already
done; you _could_ yell at them to fix their branch and re-pull in theory,
but that wouldn't work very well in practice.

On the other hand, I think letting people use prepare-commit-msg for
merges might  make sense.  Indeed, "git commit" is prepared to call
prepare-commit-msg telling the hook that it is concluding a merge, when
your "git merge" stopped due to a conflict (or you stopped it from making
a new commit with --no-commit).

I don't know about the other hooks "git commit" normally calls.  Both
"commit-msg" and "post-commit" may make sense, but I don't care too deeply
either way---I don't care too deeply for pre-commit either ;-).