Re: [PATCH 11/18] git notes merge: Add automatic conflict resolvers (ours, theirs, union)

[Johan Herland <johan@herland.net>]

On Saturday 2. October 2010 11.14.46 Stephen Boyd wrote:
> On 09/28/2010 05:23 PM, Johan Herland wrote:
> > @@ -788,6 +792,21 @@ static int merge(int argc, const char **argv, const
> > char *prefix)
> > 
> >  	expand_notes_ref(&remote_ref);
> >  	o.remote_ref = remote_ref.buf;
> > 
> > +	if (strategy) {
> > +		if (!strcmp(strategy, "manual"))
> > +			o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
> > +		else if (!strcmp(strategy, "ours"))
> > +			o.strategy = NOTES_MERGE_RESOLVE_OURS;
> > +		else if (!strcmp(strategy, "theirs"))
> > +			o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
> > +		else if (!strcmp(strategy, "union"))
> > +			o.strategy = NOTES_MERGE_RESOLVE_UNION;
> > +		else {
> > +			error("Unknown -X/--resolve strategy: %s", strategy);
> 
> Is it -X/--resolve or -s/--strategy? This error confuses me.

No, the message should say "Unknown -s/--strategy: %s". Will be fixed in the 
next iteration. Thanks for noticing.

> > diff --git a/notes-merge.c b/notes-merge.c
> > index f625ebd..6fa59d8 100644
> > --- a/notes-merge.c
> > +++ b/notes-merge.c
> > @@ -262,6 +262,35 @@ static void diff_tree_local(struct
> > notes_merge_options *o,
> > 
> >  	diff_tree_release_paths(&opt);
> >  
> >  }
> > 
> > +static int merge_one_change(struct notes_merge_options *o,
> > +			    struct notes_merge_pair *p, struct notes_tree *t)
> > +{
> > +	/*
> > +	 * Return 0 if change was resolved (and added to notes_tree),
> > +	 * 1 if conflict
> > +	 */
> > +	switch (o->strategy) {
> > +	case NOTES_MERGE_RESOLVE_MANUAL:
> > +		return 1;
> > +	case NOTES_MERGE_RESOLVE_OURS:
> > +		OUTPUT(o, 2, "Using local notes for %s", sha1_to_hex(p->obj));
> > +		/* nothing to do */
> > +		return 0;
> > +	case NOTES_MERGE_RESOLVE_THEIRS:
> > +		OUTPUT(o, 2, "Using remote notes for %s", sha1_to_hex(p-
>obj));
> > +		if (add_note(t, p->obj, p->remote, combine_notes_overwrite))
> > +			die("confused: combine_notes_overwrite failed");
> 
> This will say:
> 
> fatal: confused: combine_notes_overwrite failed
> 
> Do we actually need the "confused" part? Heh, maybe we need a confused()
> function?

Well, combine_notes_overwrite() can only return 0, so if we get a non-zero 
return here I will certainly be confused, since this should be impossible. 
Maybe better to drop the check altogether and simply leave add_note(...)?


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net