On Saturday 09 October 2010, Jonathan Nieder wrote:
> Johan Herland wrote:
> > Jonathan Nieder wrote:
> >> 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. :)
I see. I still find the OUTPUT() macro more readable, but I've amended
the patch to eliminate the extraneous show() function. Hence, you'll
find this in the next iteration:
#define OUTPUT(o, v, ...) \
do { \
if ((o)->verbosity >= (v)) { \
printf(__VA_ARGS__); \
puts(""); \
} \
} while (0)
> >>> +
> >>> + 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.
I see. I didn't take these into account when I wrote the code.
> Especially if the caller tries
>
> git notes merge 'afjkdlsa^{gobbledeegook'
>
> I would not like the merge to succeed.
Agreed, but I believe that command currently returns:
fatal: Could not parse commit 'refs/notes/afjkdlsa^{gobbledeegook'.
(or using afjkdlsa^{gobbledeegook as "our" side of the merge):
fatal: Cannot lock the ref 'refs/notes/afjkdlsa^{gobbledeegook'.
> 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?
My tests indicate that case 4 does fail (correctly). However I also
agree that this is not at all clear from the current code, so I've
rewritten this code to something that is hopefully more straightforward
(this is the next iteration):
/* Dereference o->local_ref into local_sha1 */
if (!resolve_ref(o->local_ref, local_sha1, 0, NULL))
die("Failed to resolve local notes ref '%s'", o->local_ref);
else if (!check_ref_format(o->local_ref) && is_null_sha1(local_sha1))
local = NULL; /* local_sha1 == null_sha1 indicates unborn ref */
else if (!(local = lookup_commit_reference(local_sha1)))
die("Could not parse local commit %s (%s)",
sha1_to_hex(local_sha1), o->local_ref);
trace_printf("\tlocal commit: %.7s\n", sha1_to_hex(local_sha1));
/* Dereference o->remote_ref into remote_sha1 */
if (get_sha1(o->remote_ref, remote_sha1)) {
/*
* Failed to get remote_sha1. If o->remote_ref looks like an
* unborn ref, perform the merge using an empty notes tree.
*/
if (!check_ref_format(o->remote_ref)) {
hashclr(remote_sha1);
remote = NULL;
}
else
die("Failed to resolve remote notes ref '%s'",
o->remote_ref);
}
else if (!(remote = lookup_commit_reference(remote_sha1)))
die("Could not parse remote commit %s (%s)",
sha1_to_hex(remote_sha1), o->remote_ref);
trace_printf("\tremote commit: %.7s\n", sha1_to_hex(remote_sha1));
I've also added more testcases for expected failures.
> > 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.
My thoughts exactly.
> > 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)?
Any caller who cares about the merge at all, I'd imagine
If 1 (or 0) the merge is complete, and there's nothing more to be done
(expect updating the notes ref, of course).
If -1, there are conflicts checked out in .git/NOTES_MERGE_WORKTREE,
and the user must be told to fix them and commit the conflict
resolution.
See the last part of builtin/notes.c:merge() at the end of this series
for an example.
...Johan
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html