From: Phillip Wood Date: Thu, 14 Oct 2021 13:34:28 GMT Subject: Re: [PATCH v3] sparse index: fix use-after-free bug in cache_tree_verify() Message-ID: <6dd3ba9f-7054-93f3-7798-d4a4a211899a@gmail.com> In-Reply-To: Hi Junio On 08/10/2021 20:57, Junio C Hamano wrote: > Phillip Wood writes: > >> On 07/10/2021 22:23, Junio C Hamano wrote: >>> "Phillip Wood via GitGitGadget" writes: >>> >>>> * Fixed the spelling of Stolee's name (sorry Stolee) >>>> * Added "-q" to the test to prevent a failure on Microsoft's fork[1] >>>> [1] >>>> https://lore.kernel.org/git/ebbe8616-0863-812b-e112-103680f7298b@gmail.com/ >>> I've seen the exchange, but ... >>> >>>> - for OPERATION in "merge -m merge" cherry-pick rebase >>>> + for OPERATION in "merge -m merge" cherry-pick "rebase --apply -q" "rebase --merge" >>>> do >>> ... it looks too strange that only one of them requires a "--quiet" >>> option. Is it a possibility to get whoever's fork corrected so that >>> it behaves sensibly without requiring the "-q" option only for the >>> particular rebase backend? >> >> The issue is caused by a patch that Microsoft is carrying that stops >> apply from creating paths with the skip-worktree bit set. As they're >> upstreaming their sparse index and checkout work I expect it will show >> up on the list sooner or later. I agree the "-q" is odd and it also >> means the test is weaker but I'm not sure what else we can do. > > Perhaps passing "-q" to the other variant of "rebase" would make it > clear that (1) we do not want to worry about traces involved in the > verbose message generation and (2) there is nothing fishy going on > in only one of the "rebase" backends. I'm not sure about that. There are really three levels of output from rebase - quiet, normal and verbose. I think passing "-q" suppresses virtually all the output - there is no indication of which commits have been picked. As test appears to be comparing the output of the command for the sparse and non-spare case as a proxy for "it behaves the same for sparse and non-sparse checkouts/indexes" passing "-q" to rebase weakens the test considerably. Stolee indicated [1] that he is happy for us to drop the "-q" for the "--apply" case so I'd be inclined to go back to your corrected version of V2. Best Wishes Phillip [1] https://lore.kernel.org/git/e281c2e2-2044-1a11-e2bc-5ab3ee92c300@gmail.com/