Re: [PATCH] replay: support replaying down from root commit

[Toon Claes <toon@iotcl.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Toon Claes <toon@iotcl.com> writes:
>
>> git-replay(1) doesn't allow replaying commits all the way down to the
>> root commit. Fix that.
>
> OK.
>
>> -	base = pickme->parents->item;
>> -	replayed_base = mapped_commit(replayed_commits, base, onto);
>> +	if (pickme->parents) {
>> +		base = pickme->parents->item;
>> +		replayed_base = mapped_commit(replayed_commits, base, onto);
>> +		base_tree = repo_get_commit_tree(repo, base);
>
> So, if we are replaying a commit with parent(s), we do the same as
> before (base_tree used to be computed a bit later).  But ...
>
>> +	} else {
>> +		base = NULL;
>> +		replayed_base = onto;
>> +		base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
>> +	}
>
> ... if we are replaying the root commit, there is no base (in
> contrast to "the first parent of the original commit" used in the
> other branch of this if-else construct).  We use an empty tree for
> the base_tree, which is the natural thing to use to replay for a
> root commit, of course.
>
> I am not sure why replayed_base is computed differently, though?  Is
> it because mapped_commit() would not work when base==NULL?

That's correct, mapped_commit() dereferences commit->object.oid.

> I have to wonder if the handling of that case should also be
> encapsulated inside mapped_commit() helper, just like the helper
> knows to "fallback" when the commit is not yet mapped

I'm fine either way, so I'll do that in v2.

> After all, if we drive that line of thought to the extreme, we would
> end up making repo_get_commit_tree() to return an empty tree object
> for base==NULL, too, which may be logical but it is probably too much.

Well, it's a bit annoying `struct commit::parents` is a `struct
commit_list`, so we have to first check that pointer before we can
dereference `item`, so that guard do we need anyway. So I agree it's too
much.

One other thing, in v2 I'm also fixing the merge ancestor label. For
root commits it will say "empty tree" instead of "parent of <commit>",
so conflict messages aren't misleading.

-- 
Cheers,
Toon