Re: [WIP v2 4/5] mv: add check_dir_in_index() and solve general dir check issue

[Derrick Stolee <derrickstolee@github.com>]

On 5/31/2022 5:56 AM, Shaoxuan Yuan wrote:
> On Fri, May 27, 2022 at 11:27 PM Derrick Stolee
> <derrickstolee@github.com> wrote:
>>
>> On 5/27/2022 6:08 AM, Shaoxuan Yuan wrote:
...
>> This appears to check if the _first_ entry under the directory
>> is sparse, but not if _all_ entries are sparse. These are not
>> the same thing, even in cone-mode sparse-checkout. The t1092
>> test directory has files like "folder1/0/0/a" but if
>> "folder1/1" is in the sparse-checkout cone, then that first
>> entry has the skip-worktree bit, but "folder1/1/a" and "folder1/a"
>> do not.
> 
> Yes, it is checking the first entry and this would not work without the
> lstat in the front. But I think the "lstat < 0" makes sure that this directory
> cannot be partially sparsified.
> 
> It is either missing both in the worktree and index, or missing in the worktree
> but present in index (with all its content sparsified). And because of that,
> I think only the first entry needs to be checked.

Ah! Good thinking. I hadn't considered that extra detail, so
we get to save some cycles here.

>>> +     }
>>> +     return ret;
>>
>> At the moment, it doesn't seem like we need 'ret' since the
>> only place you set it is in "return ret = 0;" (which could
>> just be "return 0;" while the others are "return 1;"). But,
>> perhaps you intended to create a loop over 'pos' while
>> with_slash is a prefix of the cache entry?
> 
> I agree that this variable is redundant. But I fail to understand
> the logical relation between before "But," and after "But,". Please
> elaborate on that?

I was just thinking that if you intended to write a loop as
I had suggested, then 'ret' could be modified or used in more
places. Feel free to ignore since we resolved that.

>>> +                     else if (!check_dir_in_index(src, length) &&
>>> +                                      !path_in_sparse_checkout(src_w_slash, &the_index)) {
>>
>> style-nit: You'll want to align the different parts of your
>> logical statement to agree with the end of the "else if (",
>>
>>         else if (A &&
>>                  B) {
>>
> 
> This one is interesting because it appears just alright in my VSCode editor.
> Later I found that it is because git-diff is using a tab size of 8 or something,
> but my VSCode uses tab size of 4. After I configured the git-diff tab rendering
> size, it looks alright. Same for another style nit down below.

That'll do it. You can double-check the alignment in your GGG
PR, which should use the correct tab width.

Thanks,
-Stolee