Re: [PATCH v3] sparse index: fix use-after-free bug in cache_tree_verify()

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Derrick Stolee" <dstolee@microsoft.com>,
	"René Scharfe" <l.s.r@web.de>, "Elijah Newren" <newren@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3] sparse index: fix use-after-free bug in cache_tree_verify()
Date: Thu, 14 Oct 2021 14:34:28 +0100	[thread overview]
Message-ID: <6dd3ba9f-7054-93f3-7798-d4a4a211899a@gmail.com> (raw)
In-Reply-To: <xmqqwnmnjnn5.fsf@gitster.g>

Hi Junio

On 08/10/2021 20:57, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 07/10/2021 22:23, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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/