Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2018, 06:07 UTC
Message-ID: <xmqq8t1xwlw6.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqq1s7pzqzl.fsf@gitster-ct.c.googlers.com>

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

Show 19 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> On the other hand, for a file-scope static that is likely to stay as a
>>> non-API function but a local helper, it may not be worth it.
>>
>> Oh, do you think that `reset_head()` is likely to stay as non-API
>> function? I found myself in the need of repeating this tedious
>> unpack_trees() dance quite a few times over the past two years, and it is
>> *always* daunting because the API is *that* unintuitive.
>>
>> So I *do* hope that `reset_head()` will actually be moved to reset.[ch]
>> eventually, and callsites e.g. in `sequencer.c` will be converted from
>> calling `unpack_trees()` to calling `reset_head()` instead.
>
> As long as the public API function is well thought out, of course it
> is OK.  Ideally, builtin/reset.c can detect when it is making a hard
> reset to HEAD and call that same function.  Which may or may not
> mean you would also want to tell it to record ORIG_HEAD and remove
> MERGE_HEAD and others), perhaps with another bit in that flag word.

Nah, forget about that one. It sometimes does branch switching and sometimes does hard reset, which looks like a strange chimera. We shouldn't burden it further by adding "while at it remove cruft, as 'reset --hard' needs to do that" to it.