Re: [PATCH v3 2/3] Add function per_branch_config.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 9, 2011, 01:14 UTC
Message-ID: <20110209011441.GA24346@elie>
In-Reply-To: <1297211028-14382-3-git-send-email-jaredhance@gmail.com>

Jared Hance wrote:

> [PATCH v3 2/3] Add function per_branch_config.
>
> Adds a configuration function to be filled up more in the next patch.

Micronit: subject should be of the form

	subsystem: do something really great

with no full stop.  So the series might be, roughly:

 - merge: introduce setup_merge_commit helper function
 - merge: introduce per-branch configuration helper function
 - merge: add support for merging from upstream by default

Commit message bodies tend to be in the imperative mood, just like the patches themselves.

Anyway, I like patch 1.  One tiny nit on this one:

Show 15 quoted lines

> +++ b/builtin/merge.c
> @@ -498,11 +498,15 @@ cleanup:
>  	strbuf_release(&bname);
>  }
>  
> -static int git_merge_config(const char *k, const char *v, void *cb)
> +static int per_branch_config(const char *k, const char *v, void *cb)
>  {
> -	if (branch && !prefixcmp(k, "branch.") &&
> -		!prefixcmp(k + 7, branch) &&
> -		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
> +	const char *variable;
> +	if (!branch || prefixcmp(k, "branch.")
> +	   || prefixcmp(k + 7, branch))
> +		return 1; /* not what I handle */

Alignment: isn't the "||" meant to be one space over?

Based on a quick grep[1], it seems the prevailing style is rather

	if (!branch || prefixcmp(k, "branch.") ||
	    prefixcmp(k + 7, branch))
		return 1;

with the || at the end of the line, though I'm not sure I care much. :)

With or without an extra space before the ||,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for a nice cleanup. Jonathan

[1]
addup () {
	sum=0
	while read term
	do
		: $((sum = sum + term))
	done
	echo $sum
}

 $ git grep --cached -c -e '||$' | cut -d: -f2 | addup
 730
 $ git grep --cached -c -e '  ||' | cut -d: -f2 | addup
 98
 $ git grep --cached -c -e '	||' | cut -d: -f2 | addup
 79