On 4/20/2021 7:21 PM, Elijah Newren wrote:
> On Tue, Apr 13, 2021 at 7:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>>
>> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>>
>> When we have sparse directory entries in the index, we want to compare
>> that directory against sparse-checkout patterns. Those pattern matching
>> algorithms are built expecting a file path, not a directory path. This
>> is especially important in the "cone mode" patterns which will match
>> files that exist within the "parent directories" as well as the
>> recursive directory matches.
>>
>> If path_matches_pattern_list() is given a directory, we can add a fake
>> filename ("-") to the directory and get the same results as before,
>> assuming we are in cone mode. Since sparse index requires cone mode
>> patterns, this is an acceptable assumption.
>
> Makes sense; thanks for the good description.
>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> dir.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/dir.c b/dir.c
>> index 166238e79f52..57e22e605cec 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1378,6 +1378,11 @@ enum pattern_match_result path_matches_pattern_list(
>> strbuf_addch(&parent_pathname, '/');
>> strbuf_add(&parent_pathname, pathname, pathlen);
>>
>> + /* Directory requests should be added as if they are a file */
>
> "added" or "matched"? Also, the description seems a bit brief and
> likely to surprise; I'd at least want to expand "file" to "file within
> their given directory" but it might be nice to get some summarized
> version of the commit message or at least state that "-" is just a
> random simple name within the given directory.
I can improve this comment.
>> + if (parent_pathname.len > 1 &&
>
> Is this line...
>
>> + parent_pathname.buf[parent_pathname.len - 1] == '/')
>
> to prevent an out-of-bounds indexing? If so, shouldn't it be "> 0" or
> ">= 1" rather than "> 1"? And if so, doesn't the strbuf_addch() call
> above ensure the condition is always met?
>
> Or are we trying to avoid adding the "-" when we parent_pathname is
> just a plain "/"?
I believe plain "/" is impossible. There needs to be a valid tree entry
before that first slash ("a/", for example). But that isn't super
important to the logic here and just adds confusion.
>
>> + strbuf_add(&parent_pathname, "-", 1);
>> +
>
> Sorry for all the questions on such a tiny change. It makes sense to
> me, I'm just curious whether it'll confuse future code readers.
Yes, let's avoid confusion by doing the simple thing and use "> 0".
Thanks,
-Stolee
