Subject: Re: [PATCH 11/20] sparse-index: convert from full to sparse

From: Derrick Stolee <[email-masked]>
Date: Tue, 9 Mar 2021 16:13:48 -0500

On 2/25/2021 2:33 AM, Elijah Newren wrote:
> On Tue, Feb 23, 2021 at 12:14 PM Derrick Stolee via GitGitGadget
> <[email-masked]> wrote:

>> +                       /*
>> +                        * allow terminating directory separators for
>> +                        * sparse directory enries.
> 
> enries -> entries

Thanks.

>> +                        */
>> +                       if (c == '\0')
>> +                               return S_ISDIR(mode);
> 
> Yaay, much simpler (than the RFC version).

>> +       /*
>> +        * Is the current path outside of the sparse cone?
>> +        * Then check if the region can be replaced by a sparse
>> +        * directory entry (everything is sparse and merged).
>> +        */
>> +       match = path_matches_pattern_list(ct_path, ct_pathlen,
>> +                                         NULL, &dtype, pl, istate);
>> +       if (match != NOT_MATCHED)
>> +               can_convert = 0;
> 
> Not sure if you saw my comments on the flow control at
> https://lore.kernel.org/git/CABPp-BE9wPwmC0=[email-masked]/
> (the typos elsewhere seem to still be present).  If you saw it and
> decided against it, that's fine, just wanted the idea to at least be
> floated.

Sorry for dropping this one. I _did_ decide against it, and
primarily because the "if (can_convert)" condition contains
a return statement. I like to use 'gotos' for blocks that
will eventually be entered by all paths through the code,
such as "goto cleanup;" but here I find the "can_convert"
check to be clearer.

>> +               /*
>> +                * Detect if this is a normal entry oustide of any subtree
> 
> s/oustide/outside/

Got it.

>> +test_expect_success 'sparse-index contents' '
>> +       init_repos &&
>> +
>> +       test-tool -C sparse-index read-cache --table >cache &&
>> +       for dir in folder1 folder2 x
>> +       do
>> +               TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
>> +               grep "040000 tree $TREE $dir/" cache \
>> +                       || return 1
>> +       done &&
> 
> Thanks for making the output look more like ls-tree output; it's
> easier to parse that way, at least for me.

Excellent.
 
> I mostly read over the range-diff since it was much shorter.  You've
> addressed a number of questions/comments I had on the RFC version, but
> there's still some I didn't see a response to so I reposted them here.
 
Thanks for being diligent!
-Stolee