Re: [PATCH 04/11] reset_head(): remove action parameter

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 4, 2021, 10:00 UTC
Message-ID: <97b8cbdb-60f9-cba5-2db0-6ae77a9afd60@gmail.com>
In-Reply-To: <xmqqpmso5uno.fsf@gitster.g>

Hi Junio

On 01/10/2021 21:58, Junio C Hamano wrote:

Show 13 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The action parameter is passed as the command name to
>> setup_unpack_trees_porcelain(). All but two cases pass either
>> "checkout" or "reset". The case that passes "reset --hard" should be
>> passing "reset" instead.
> 
> Describe how the parameter is meant to be used (presumably "this is
> to record in the reflog", perhaps?); without such explanation, it is
> hard to either agree or disagree with the claim that "reset --hard"
> should be "reset".

How about

The only use of the action parameter is to setup the error messages for unpack_trees(). All but two cases pass either "checkout" or "reset". The case that passes "reset --hard" would be better passing "reset" so that the error messages match the builtin reset command like all the other callers that are doing a reset. The case that passes "Fast-forwarded" is only updating HEAD and so the parameter is unused in that case as it does not call unpack_trees(). The value to pass to setup_unpack_trees_porcelain() can be determined by checking whether flags contains RESET_HEAD_HARD without the caller having to specify it.

> Also state if this change is supposed to have any externally
> observable effect.

It'll change the error message if we cannot clear the stashed changes form the working tree from saying "reset --hard" to "reset".

Best Wishes

Phillip

Show 7 quoted lines

> Perhaps this improves what is shown in an error message by affecting
> what setup_unpack_trees_porcelain() does?  I am just guessing,
> because the proposed log message is not telling.  Please do not make
> me (or other readers of "git log") guess.
> 
> Thanks.
>