Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 2, 2015, 08:01 UTC
Message-ID: <20150802080123.GA17440@flurp.local>
In-Reply-To: <CA+P7+xpTu6eKZEBTbzR9mg4gV3zAvTOc-3PTJP6QamEO_sA1=A@mail.gmail.com>

On Sun, Aug 02, 2015 at 12:41:08AM -0700, Jacob Keller wrote:

Show 31 quoted lines

> On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >>  If conflicts arise and a strategy for automatically resolving
> >> -conflicting notes (see the -s/--strategy option) is not given,
> >> -the "manual" resolver is used. This resolver checks out the
> >> -conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
> >> -and instructs the user to manually resolve the conflicts there.
> >> -When done, the user can either finalize the merge with
> >> -'git notes merge --commit', or abort the merge with
> >> -'git notes merge --abort'.
> >> +conflicting notes (see the -s/--strategy option or notes.merge
> >> +config option) is not given, the "manual" resolver is used.
> >> +This resolver checks out the conflicting notes in a special
> >> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user
> >> +to manually resolve the conflicts there. When done, the user
> >> +can either finalize the merge with 'git notes merge --commit',
> >> +or abort the merge with 'git notes merge --abort'.
> >
> > When you re-wrap the text like this, it's difficult to spot your one
> > little change in all the diff noise. For the sake of review, it's
> > often better to minimize the change, even if it leaves the text more
> > jagged than you'd like.
> 
> This results in something incredibly jagged. I can't find a good way
> to split which minimizes the change. Would a 3rd patch which just
> re-flows this be an acceptable alternative
> 
> ie: add the words in one patch then re-flow afterwards in a second
> patch with no changes?
> 
> There is no good alternative as other re-flows I tried end up looking
> way too jagged, as compared to surrounding documentation.

Don't worry too much about it. Consider it something to keep in mind for future patches. I reviewed the change and it seemed okay. I mentioned it because one of the goals of patch submission, in addition to making an actual change, is to ease the review process. If Junio is okay with accepting it as is, then it's probably not worth spending more time trying to refine it.

Having said that, I came up with the following for those two paragraphs, which gives a much less noisy diff and doesn't look too jagged:

--- 8< ---
 If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
-the "manual" resolver is used. This resolver checks out the
+conflicting notes (see -s/--strategy or notes.merge configuration) is
+not given, the "manual" resolver is used. This resolver checks out the
 conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
 and instructs the user to manually resolve the conflicts there.
 When done, the user can either finalize the merge with
--- 8< ---

...and...

--- 8< ---
 	When merging notes, resolve notes conflicts using the given
-	strategy. The following strategies are recognized: "manual"
+	strategy. Overrides the "notes.merge" configuration variable.
+	The following strategies are recognized: "manual"
 	(default), "ours", "theirs", "union" and "cat_sort_uniq".
 	See the "NOTES MERGE STRATEGIES" section below for more
 	information on each notes merge strategy.
--- 8< ---