Re: [PATCH] Converted git-merge-ours.sh -> builtin-merge-ours.c

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2007, 08:49 UTC
Message-ID: <7voddluz13.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <4745E45C.7@gmail.com>

Thomas Harning <harningt@gmail.com> writes:

Show 5 quoted lines

> Here's a simple patch to make git-merge-ours.sh into a builtin.
>
> I figure this would be a simple way of getting in the git-development flow.
>
> Signed-off-by: Thomas Harning Jr <harningt@gmail.com>

Have you tried to read this proposed commit log message in the context of "git log", after applying it on top of 'master'? "Here's a simple patch to" and "I figure ..." are of not much use.

The patch looks good except for minor style nits.

Show 9 quoted lines

> diff --git a/builtin-merge-ours.c b/builtin-merge-ours.c
> new file mode 100644
> index 0000000..fbfe183
> --- /dev/null
> +++ b/builtin-merge-ours.c
> @@ -0,0 +1,32 @@
> +/*
> + * Implementation of git-merge-ours.sh as builtin
> + *

Traling whitespace.

Show 13 quoted lines

> +int cmd_merge_ours(int argc, const char **argv, const char *prefix)
> +{
> +	const char *nargv[] = {
> +		"diff-index",
> +		"--quiet",
> +		"--cached",
> +		"HEAD",
> +		NULL
> +	};
> +	int i;
> +
> +	int ret = cmd_diff_index(4, nargv, prefix);
> +	printf("GOT: %i\n", ret);

Unused variable "i".

An unwanted blank line still in the sequence of variable definitions, and a missing blank line after the definitions.

A leftover debug printf() is not very welcomed.

	[Not a nit but a comment]

        The call to cmd_diff_index() here raised my eyebrow a
	bit.  I would have skipped all the parameter parsing and
	arranged it to directly call into run_diff_index()
	instead.

	As the result of literally translating the scripted
	version of git-merge-ours, the code inherits a
	corner-case bug.  Can you spot it?

> +	/* We need to exit with 2 if the index does not match our HEAD tree,
> +	 * because the current index is what we will be committing as the
> +	 * merge result.
> +	 */

We tend to format a multi-line comment block as:

	/*
         * We need to ...
         * ... merge result.
         */

> +	if(ret) ret = 2;

A SP between "if" and "("; put the body of the "if" on a separate line.

Thanks. No need to resend; all these minor nits can be fixed here easily.