Re: [PATCH] add a pre-merge hook

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 27, 2008, 18:02 UTC
Message-ID: <7v63pmqqtx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <E1KYK3t-0000ZX-3b@fencepost.gnu.org>

Paolo Bonzini <bonzini@gnu.org> writes:

Show 11 quoted lines

> +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?

Show 5 quoted lines

> +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".

Show 11 quoted lines

> +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).

Show 5 quoted lines

> +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.