Re: [PATCH v3 0/3] Updated patch series for default upstream merge

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2011, 00:34 UTC
Message-ID: <7vlj1qoxko.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297211028-14382-1-git-send-email-jaredhance@gmail.com>

Jared Hance <jaredhance@gmail.com> writes:

> Notes/Complications:
> 	- I'm not sure whether the option should be merge.defaultUpstream
> 	  or merge.defaultToUpstream

Probably the latter.

> 	- Should [remotes] be changed to [branches]? I felt like it was
> 	  a completely different change and didn't belong in the patch series.

Perhaps at the beginning (just like your 1/3 refactoring) or at the end as a separate patch?

> 	- I left one of the ifs with unnecessary braces for clarity because
> 	  of a nested if-else: is this the preferred style?

If you are talking about the one at @@ -1017,9 +1023,13 @@, it looks fine to me. The new "else" you added to @@ -525,6 +527,8 @@ may probably want to start on the same line as closing "}" of the "if", though. IOW, like this:

	if (cond) {
        	...
	} else {
        	...
	}

not like this:

	if (cond) {
        	...
	}
        else {
        	...
	}

But other than that looks reasonable.

Thanks.