Hi, On Mon, Aug 17, 2015 at 5:54 AM, Johan Herland <jo...@herland.net> wrote: > On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.kel...@intel.com> > wrote: >> From: Jacob Keller <jacob.kel...@gmail.com> >> >> notes-merge.c already re-uses the same functions for the automatic merge >> strategies used by the rewrite functionality. Teach the -s/--strategy >> option how to interpret the equivalent rewrite terminology for >> consistency. > > I'm somewhat negative to this patch. IMHO, adding the rewrite modes as > merge strategy synonyms adds no benefit - only potential confusion - > to the existing merge strategies. Words that have a sensible meaning > in the context of rewrite, do not necessarily have the same sensible > meaning in the context of merge (and vice versa). I'd rather have the > rewrite code map ignore/overwrite/concatenate to ours/theirs/union, > without teaching the notes-merge code about these words. Or maybe even > drop this patch (and the next?) entirely, and let the future author > (who implements notes rewrite in terms of notes merge) decide how to > deal with this? By committing to these synonyms now, you might > actually be making things harder for the future author: once the > synonyms are part of the user-visible and documented interface, they > cannot easily be removed/changed again. > > ...Johan >
I am ok dropping these, I really only added them after Junio brought it up. I think that documenting union/concatenate is fine, but I think that ours/theirs is very confusing. However, I think you're right that a future author can deal with this when working on rewrite -> merge. I don't think we need to do this now. I'll drop these patches, but I will leave the parse_notes_merge_strategy in it's location here. We could document concatenate as a synonym of union but I don't think that is a big deal. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
