Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed

From: Victoria Dye <vdye@github.com>
Date: Mar 18, 2022, 19:19 UTC
Message-ID: <80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com>
In-Reply-To: <xmqqy2179o3c.fsf@gitster.g>

Junio C Hamano wrote:

Show 26 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> This keeps the current behavior of not refreshing when --quiet is
>> given. I wonder how disruptive it would be to take the opportunity to
>> get rid of that hack and go back the the original behavior of
>> refreshing when --quiet is given. There are a couple of assumptions
>> that make me think it might be acceptable
>>
>> 1 - anyone using a sparse index wont notice as refreshing the index
>>     should be fast for them
>>
>> 2 - the large repositories that are affected exist in managed
>>     environments where an admin who reads the release notes could set
>>     reset.refresh in a central config so individual users are not
>>     inconvenienced.
> 
> I would very much prefer to see "--quiet" not making contribution to
> the decision to refresh or not in the longer term.  Many plumbing
> commands expect that the calling scripts refresh the index with an
> explicit use of "update-index --refresh" and leave the index not
> refreshed, but working on unrefreshed index is a trade-off between
> performance and correctness.
> 
>  * Turning "--quiet" not to refresh may incur performance regression
>    for shorter term.  It will not hurt correctness.
>

I tend to agree with you and Phillip on this. I took a more conservative approach with the intention of preserving as much backward compatibility as possible, but having '--quiet' disable refresh (to me) actively hurts its correctness. If backwards-compatibility isn't a huge concern, I'll gladly make that change.

Show 15 quoted lines

>  * Introducing "--no-refresh" to mark "reset --quiet" invocations,
>    where the freshness of the index does not matter for correctness,
>    would help regain performance without breaking scripts.  All
>    "reset --quiet" invocations in scripts written before this series
>    are supposed to be safe (as they lived with their "reset --quiet"
>    that does not refresh), but newly written scripts may start
>    expecting that "reset --quiet" would refresh for correctness.
> 
>  * If we allow reset.refresh to be set to "no", however, that would
>    affect _all_ uses of "reset --quiet", including the ones in newly
>    written scripts that expect "reset --quiet" to refresh.  They
>    would be forced to say "reset --quiet --refresh", just in case
>    the user has such a configuration; otherwise these scripts will
>    be declared "buggy" for not explicitly saying "--refresh".
>

I added the option as a "replacement" for 'reset.quiet' (specifically, its ability to summarily disable refresh), but I can definitely see how it would lead to issues in the future. I'm happy to remove it, but should 'reset.quiet' be removed as well? No other commands have a config option for 'quiet', and it presents a similar issue of potentially suppressing a helpful feature (in this case, informational printouts) across all invocations unless otherwise specified.

> I do not think reset.refresh is a good idea, but I very much like
> the idea to making "reset" (regardless of "--quiet") to refresh by
> default.
>

I was hesitant to go this far because it would force people that were comfortably relying on 'reset.quiet' to need to always use '--no-refresh' to get the same behavior. But, to Phillip's point earlier, there are other options now (like sparse index) that could provide a just-as-substantial (if not greater) performance boost without sacrificing the refresh.

> Thanks.
> 
>

Since this is already in 'next' (and, in its current state, I don't think it does any more damage than the pre-series state), I'll send a new series on top of this that deprecates 'reset.refresh' and 'reset.quiet', and makes '--refresh' the default for all of 'reset'.

Thanks, both!