Re: [PATCH v2 2/6] merge: make sparse-aware with ORT

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 30, 2021, 17:18 UTC
Message-ID: <e8a875aa-181f-e71f-55ee-012edccf7ecb@gmail.com>
In-Reply-To: <CABPp-BEF0htk-Q-+wiF+XgTWRRtDc5FPJKauWZAivLGuMgWeAQ@mail.gmail.com>

On 8/27/2021 6:43 PM, Elijah Newren wrote:

> On Tue, Aug 24, 2021 at 2:52 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:

...

Show 5 quoted lines

>> +       /*
>> +        * We are in a conflicted state. These conflicts might be inside
>> +        * sparse-directory entries, so expand the index preemtively.
> 
> Same typo I pointed out in v1.

Sorry, this comment is edited in a later patch and I fixed it there. Will fix here, too.

Show 10 quoted lines

>> +       ensure_not_expanded checkout -f update-deep &&
>> +       (
>> +               sane_unset GIT_TEST_MERGE_ALGORITHM &&
>> +               git -C sparse-index config pull.twohead ort &&
>> +               ensure_not_expanded merge -m merge update-folder1 &&
>> +               ensure_not_expanded merge -m merge update-folder2
>> +       )
>>  '
> 
> Should you use test_config rather than git config here?

That's a better pattern. It's not technically _required_ for these tests because the repositories are completely rewritten at the start of each new test, but it's best to be a good example.

> More importantly, why the subshell and unsetting of
> GIT_TEST_MERGE_ALGORITHM and the special worrying about pull.twohead?
> Wouldn't it be simpler to just set GIT_TEST_MERGE_ALGORITHM=ort,
> perhaps at the beginning of the file?

 
I don't set GIT_TEST_MERGE_ALGORITHM at the beginning of the file so
the rest of the tests are covered with both 'ort' and 'recursive' in
the CI test suite.

Using the config more carefully matches how I expect the 'ort' strategy to be specified in practice (very temporarily, as it will soon be the default).

Thanks, -Stolee