Re: [PATCH] add a pre-merge hook

Paolo Bonzini [off-list ref] writes:

+pre-merge
+---------
+
+This hook is invoked before a merge is attempted.  The command
+line passed to the hook can have multiple parameters.  The
+first parameter is the type of merge, which can be one of
+"squash" (if --squash was given on the command line), "ff"
+(fast-forward), "trivial" (trivial in-index merge), "merge"
+(non-trivial 2-head merge), "octopus" (any other merge).  After this
+there is a "--" argument, followed by the commit SHA1 values for the
+heads being merged.

I think we should have done something like this for post_merge, not just
"1/0 to show if it is a squash" which was an ill-thought-out hack.  Is
there a clean way to fix the interface without breaking existing
post_merge hooks in people's repositories?

+The hook can interrupt the merge by returning a non-zero
+status.  The default hook checks for a boolean configuration
+key `branch.<branch-name>.allowmerges`, where `<branch-name>`
+is the current branch.  If the key is false, only squash or
+fast-forward merges are allowed.

Please call whatever you install from template/hooks--* a "sample", not
"default".

+static int run_hook(const char *name, struct commit_list *heads, ...)
 {
+ ...
+	if (heads) {
+		argv[i] = "--";
+		first_remote_head = i + 1;
+		for (; heads; heads = heads->next)
+			argv[++i] = xstrdup(sha1_to_hex(heads->item->object.sha1));
+		argv[++i] = NULL;
+	} else
+		first_remote_head = i;

Making this function varargs is fine, but this heads==NULL condition needs
some explanation.

If it is primarily a hack to support calling post-merge with an non-object
name "0" or "1", probably it will be _much_ cleaner to make this a
separate function that does not know anything about post-merge calling
convention (it can share the boilerplate parts such as "hook.no_stdin = 1"
with the function that calls post-merge, of course).

+static int run_pre_merge_hook(const char *kind)
+{
+	return run_hook("pre-merge", remoteheads,
+			squash ? "squash" : kind, "--", NULL);
 }

remoteheads is a commit_list, you have the name of the hook and kind, so
this extra "--" is only for run_hook() hackery to distinguish between the
callers to post-merge and the callers to new pre-merge?

The general idea to add "pre-merge" hook may be sound, but the run_hook()
implementation looks quite unclean.

I think we should have done something like this for post_merge, not just
"1/0 to show if it is a squash" which was an ill-thought-out hack.  Is
there a clean way to fix the interface without breaking existing
post_merge hooks in people's repositories?

Well, you can add this argument as a second argument to post-merge.
Shall I do it?

Please call whatever you install from template/hooks--* a "sample", not
"default".

Ok.

If it is primarily a hack to support calling post-merge with an non-object
name "0" or "1"

It is to support not appending the commit list to the arguments of
post-merge.

quoted

+static int run_pre_merge_hook(const char *kind)
+{
+	return run_hook("pre-merge", remoteheads,
+			squash ? "squash" : kind, "--", NULL);
 }

remoteheads is a commit_list, you have the name of the hook and kind, so
this extra "--" is only for run_hook() hackery to distinguish between the
callers to post-merge and the callers to new pre-merge?

No, absolutely.  The "--" separator is for future extension, in case we
want to add other arguments before the head list.

run_hook does not care about what it receives in its varargs part.  It
is copied verbatim until the NULL.  Then it adds a "--" and the heads
argument if requested (which is not the case for post-merge).  The fact
that I had to specify a "--" here though is fishy.  Maybe I have a
off-by-one somewhere that, if fixed, can simplify the code somehow.

I'll take a look tomorrow, thanks for the review.

Paolo