Re: [PATCH v2] replay: drop commits that become empty

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the changes in a commit being replayed are already in the branch
>> that the commits are being replayed onto then "git replay" creates an
>> empty commit. This is confusing because the commit message no longer
>> matches the contents of the commit. Drop the commit instead. Commits
>> that start off empty are not dropped. This matches the behavior of
>> "git rebase --reapply-cherry-pick --empty=drop" and "git cherry-pick
>> --empty-drop".
> 
> OK.  Maybe it is just me but "onto then" -> "onto," would flow the
> sentence better?

>> If a branch points to a commit that is dropped it will be updated to
>> point to the last commit that was not dropped. This can been seen
> 
> If one thinks about it, it is the only natural behaviour to use the
> last surviving commit to point the branch at.  Thanks for spelling
> it out so clearly.
> 
> BTW, "can been seen" -> "can be seen" (will amend locally).
> 
>> in the new test where "topic1" is updated to point to the rebased
>> "C" as "F" is dropped because it is already upstream. While this is
>> a breaking change "git replay" is marked as experimental to allow
>> improvements like this that change the behavior.
> 
> Again maybe it is just me, but I'd prefer to see a comma after "a
> breaking change" to flow the sentence better.
> 
>> Helped-by: Elijah Newren <newren@gmail.com>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> ...
>> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
>> index dcb26e8a8e8..96a3a557bf3 100644
>> --- a/Documentation/git-replay.adoc
>> +++ b/Documentation/git-replay.adoc
>> @@ -59,7 +59,9 @@ The default mode can be configured via the `replay.refAction` configuration vari
>>   	be passed, but in `--advance <branch>` mode, they should have
>>   	a single tip, so that it's clear where <branch> should point
>>   	to. See "Specifying Ranges" in linkgit:git-rev-parse[1] and the
>> -	"Commit Limiting" options below.
>> +	"Commit Limiting" options below. Any commits in the range whose
>> +	changes are already present in the branch the commits are being
>> +	replayed onto will be dropped.
> 
> OK.
> 
>> diff --git a/replay.c b/replay.c
>> index 13983dbc566..2864c213993 100644
>> --- a/replay.c
>> +++ b/replay.c
>> @@ -88,12 +88,12 @@ struct commit *replay_pick_regular_commit(struct repository *repo,
>>   					  struct merge_result *result)
>>   {
>>   	struct commit *base, *replayed_base;
>> -	struct tree *pickme_tree, *base_tree;
>> +	struct tree *pickme_tree, *base_tree, *replayed_base_tree;
>>   
>>   	base = pickme->parents->item;
>>   	replayed_base = mapped_commit(replayed_commits, base, onto);
>>   
>> -	result->tree = repo_get_commit_tree(repo, replayed_base);
>> +	replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
>>   	pickme_tree = repo_get_commit_tree(repo, pickme);
>>   	base_tree = repo_get_commit_tree(repo, base);
>>   
>> @@ -103,13 +103,17 @@ struct commit *replay_pick_regular_commit(struct repository *repo,
>>   
>>   	merge_incore_nonrecursive(merge_opt,
>>   				  base_tree,
>> -				  result->tree,
>> +				  replayed_base_tree,
>>   				  pickme_tree,
>>   				  result);
>>   
>>   	free((char*)merge_opt->ancestor);
>>   	merge_opt->ancestor = NULL;
>>   	if (!result->clean)
>>   		return NULL;
>> +	/* Drop commits that become empty */
>> +	if (oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) &&
>> +	    !oideq(&pickme_tree->object.oid, &base_tree->object.oid))
>> +		return replayed_base;
>>   	return replay_create_commit(repo, result->tree, pickme, replayed_base);
>>   }
> 
> OK, that is straight-forward.  Instead of overriding the
> result->tree upfront, we try the same using a temporary
> replayed_base_tree, and that allows us to see if the resulting tree
> computed by merge_incore matches.  Only when it made a non-empty
> change, we proceed to create a new commit.
> 
>> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
>> index cf3aacf3551..9d4b0dd1a77 100755
>> --- a/t/t3650-replay-basics.sh
>> +++ b/t/t3650-replay-basics.sh
>> @@ -25,6 +25,8 @@ test_expect_success 'setup' '
>>   	git switch -c topic3 &&
>>   	test_commit G &&
>>   	test_commit H &&
>> +	git switch -c empty &&
>> +	git commit --allow-empty --only -m empty &&
> 
> The use of "--only" here is a bit curious.  As there is no change
> between the index and the commit our "empty" branch points at,
> wouldn't it be unnecessary?  The option, together with --allow-empty,
> would only matter if you did
> 
> 	git switch -c empty &&
> 	modify blah &&
> 	git add blah &&
> 	git commit --allow-empty --only -m empty
> 
> because without --only, the changes to blah will be taken.