Re: [PATCH 08/18] git notes merge: Initial implementation handling trivial merges only

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 9, 2010, 17:29 UTC
Message-ID: <20101009172955.GB17799@burratino>
In-Reply-To: <201010090155.20858.johan@herland.net>

Johan Herland wrote:

> I agree, but I'd like to name it 'git notes get-ref' instead, to stay
> consistent with the current subcommand (instead of option) design.

Sounds good. :)

> Jonathan Nieder wrote:

Show 10 quoted lines

>> I would find it easier to read
>>
>> 	if (o->verbosity >= DEFAULT_VERBOSITY)
>> 		fprintf(stderr, ...)
>>
>> unless there are going to be a huge number of messages.
>
> The current version is modeled on the show() and output() functions in
> merge-recursive.c. I think that works better in this situation.
> Or maybe you have a better solution for merge-recursive.c as well?

Hmm --- isn't the point of output() that it indents to the appropriate level to portray a recursive merge?

Similarly, show() prevents those confusing messages from the internal merge between ancestors from being printed when the user is not interested.

But if you think they are important abstractions to maintain, I won't stop you. :)

>>> +
>>> +	if (!o->local_ref || get_sha1(o->local_ref, local_sha1)) {
>>> +		/* empty notes ref => assume empty notes tree */

[...]

> I'm not sure when you think we should (or shouldn't) error out.

get_sha1() can return -1 in the following cases:

 - starts with :/, regex does not match.
 - starts with :, entry not present in index
 - in form <rev>:<path>, <path> does not exist in <rev>
 - in form <rev>^, <rev> does not exist or that parent does not
   exist
 - tag being used as commit points to a tree instead
 - et c.

Especially if the caller tries

	git notes merge 'afjkdlsa^{gobbledeegook'

I would not like the merge to succeed.

So as I see it, there are four cases:

 - get_sha1() succeeds and returns a commit ==> merge that rev
 - get_sha1() succeeds and returns a non-commit ==> fail
 - get_sha1() fails, but resolve_ref() indicates a ref valid
   for writing ==> merge empty tree
 - get_sha1() fails, invalid refname ==> fail

The current code does not notice case 4, does it?

Show 5 quoted lines

> I guess this becomes a discussion of whether we should model notes
> merges on the 'resolve' merge strategy or the 'recursive' merge
> strategy. Without having studied each strategy in-depth, I don't know
> how much "better" 'recursive' is than 'resolve', especially not from
> the POV of notes merges.

I think 'resolve' should be good enough for now. We can always add 'recursive' later.

The cases where 'recursive' might help are those in which both sides made a lot of changes to the same notes. It helps in two ways: signaling conflicts that might have been otherwise missed and merging cleanly in cases that might otherwise produce spurious conflicts. In theory, it makes the result less "arbitrary"; in practice, it seems to help avoid some conflicts in ugly cases like merging one week's pu with the next week's pu.

[...]

Show 8 quoted lines

> Almost. If you look at the notes_merge() docs in notes-merge.h by the
> end of this series, you'll see the following return values:
> 
> 0: Merge trivially succeeded in an existing commit (e.g. fast-forward).
> 
> 1: Merge successfully completes a merge commit (i.e. no conflicts).
> 
> -1: Merge results is conflicts.

What kind of caller would care about the distinction between 1 and -1 here (just curious)?

Jonathan