Re: [PATCH 3/3] replay: offer an option to linearize the commit topology

[Toon Claes <toon@iotcl.com>]

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

> Toon Claes <toon@iotcl.com> writes:
>
>> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>>
>> One of the stated goals of git-replay(1) is to allow implementing the
>> git-rebase(1) functionality on the server side.
>>
>> The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
>> was given. This mode drops merge commits instead of replaying them, and
>> linearized the commit history into a sequence of the
>> regular (single-parent) commits.
>
> "linearized" -> "linearizes"?

Thanks.

>>
>> Add option `--linearize` to git-replay(1) do the same.
>
> "do the same" -> "to do the same"?

Ack.

>> Co-authored-by: Toon Claes <toon@iotcl.com>
>
> There is no sign-off by any of the authors?

My bad. I'll add mine.

@Johannes, can I re-add yours? I've removed it because I've made some
changes on top of the patch you wrote, but if you agree, I'll add your
Sign-off back.

>> @@ -430,12 +435,20 @@ int replay_revisions(struct rev_info *revs,
>>  	while ((commit = get_revision(revs))) {
>>  		const struct name_decoration *decoration;
>>  
>> -		if (commit->parents && commit->parents->next)
>> +		if (opts->linearize && (!commit->parents || commit->parents->next))
>> +			; /* map current commit to the same as the previous commit */
>
> This uses the same treatment on either root commits or merge
> commits?  If this were a mistake and this wants to handle merges but
> not roots, shouldn't it be more like
>
> 		if (opts->linearize && (commit->parents && commit->parents->next))
> 			; /* map the merge to the previous */
>
>> +		else if (commit->parents && commit->parents->next)
>>  			die(_("replaying merge commits is not supported yet!"));
>
> And because the next one is also about merges, perhaps the early
> part of this if/else if cascade can be written
>
> 		if (commit->parents && commit->parents->next) {
> 			/* We have a merge */
> 			if (!opts->linearize)
> 				die(_("can't replay a merge (yet)"));
> 			; /* map current to the previous */
> 		} else {
> 			...
>
> wouldn't it?

The way it was written in v1 was maybe a bit too smart and hard to
follow. I agree with your suggestion and will adopt this (with some
tweaks) in the next version.

> If the "map current to prev" is applicable to root, any root are
> mapped to the last_commit in the above, and if we saw a root as the
> first thing in the loop, last_commit is NULL, we do not do anything
> here, and after the if/else if/else cascade, we see last_commit is
> NULL and break out of the loop.

Yes, good observation. I did not test this.

> Perhaps we would want to have a test that replays all the way down
> to the root commit?

I'll add it.

-- 
Cheers,
Toon