From: Victoria Dye <vdye@github.com>
To: Junio C Hamano <gitster@pobox.com>,
Phillip Wood <phillip.wood123@gmail.com>
Cc: Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, derrickstolee@github.com
Subject: Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed
Date: Fri, 18 Mar 2022 12:19:08 -0700 [thread overview]
Message-ID: <80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com> (raw)
In-Reply-To: <xmqqy2179o3c.fsf@gitster.g>
Junio C Hamano wrote:
> 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.
> * 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!
next prev parent reply other threads:[~2022-03-18 19:20 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-12 0:08 [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye via GitGitGadget
2022-03-12 0:08 ` [PATCH 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-12 0:08 ` [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 15:05 ` Derrick Stolee
2022-03-14 15:13 ` Derrick Stolee
2022-03-14 15:55 ` Victoria Dye
2022-03-12 0:08 ` [PATCH 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-12 0:08 ` [PATCH 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-12 0:08 ` [PATCH 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 15:10 ` Derrick Stolee
2022-03-14 15:56 ` Victoria Dye
2022-03-12 17:12 ` [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye
2022-03-14 6:22 ` Junio C Hamano
2022-03-14 15:13 ` Derrick Stolee
2022-03-14 16:10 ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' " Victoria Dye via GitGitGadget
2022-03-14 16:10 ` [PATCH v2 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-14 16:10 ` [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 19:27 ` Junio C Hamano
2022-03-14 23:48 ` Victoria Dye
2022-03-14 16:10 ` [PATCH v2 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-14 16:10 ` [PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-14 19:38 ` Junio C Hamano
2022-03-14 16:10 ` [PATCH v2 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 19:42 ` Junio C Hamano
2022-03-14 23:54 ` Victoria Dye
2022-03-14 16:30 ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' Derrick Stolee
2022-03-14 23:17 ` Junio C Hamano
2022-03-15 1:49 ` [PATCH v3 " Victoria Dye via GitGitGadget
2022-03-15 1:49 ` [PATCH v3 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-15 1:49 ` [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-18 11:08 ` Phillip Wood
2022-03-18 17:17 ` Junio C Hamano
2022-03-18 19:19 ` Victoria Dye [this message]
2022-03-15 1:49 ` [PATCH v3 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-15 1:49 ` [PATCH v3 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-15 1:49 ` [PATCH v3 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-15 10:23 ` Junio C Hamano
2022-03-16 20:07 ` Victoria Dye
2022-03-16 20:55 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com \
--to=vdye@github.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).