Re: [PATCH 1/1] sequencer: fix empty commit check when amending

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 22, 2019, 11:01 UTC
Message-ID: <0c393fc0-b9d7-66d3-4562-f5278f8a5b59@gmail.com>
In-Reply-To: <xmqqh82w4do6.fsf@gitster-ct.c.googlers.com>

Hi Junio

On 22/11/2019 06:40, Junio C Hamano wrote:

Show 18 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> This fixes a regression introduced in 356ee4659b ("sequencer: try to
>> commit without forking 'git commit'", 2017-11-24). When amending a
>> commit try_to_commit() was using the wrong parent when checking if the
>> commit would be empty. When amending we need to check against HEAD^ not
>> HEAD.
> 
> Thanks.  That makes sense.
> 
> If you compare with HEAD and find there is no difference, that would
> mean that the "amend" itself is a no-op (i.e. the user said they
> want to make changes, but after all changed their mind)?  It might
> be something worth noticing, but certainly not in the same way as
> "we were told to skip an empty commit---is this one empty?" gets
> treated.

Yes I'm trying to decide what to do about fixups and squashes that become empty. We'd need to do such a check in the sequencer before trying to commit I think.

Another thing to think about for the future is what message we want to display when a fixup makes the amended commit become empty. If there is another fixup following it then the commit may not end up empty by the time we've finished applying all the fixups for that commit. If the user runs 'reset HEAD^' after the first fixup we'll end up fixing up the wrong commit with the later fixups.

Best Wishes

Phillip