Re: [WIP v2 4/5] mv: add check_dir_in_index() and solve general dir check issue
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- May 31, 2022, 15:49 UTC
- Message-ID
- <4e02f672-b246-d32d-b208-8a51b13858c8@github.com>
- In-Reply-To
- <CAJyCBORo-x4jbKhtn+vUE=1TxpM83_3JWj5cvJEJJHHsv2Q0bg@mail.gmail.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:
...
Show 15 quoted lines
>> 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.
Show 12 quoted lines
>>> + } >>> + 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.
Show 14 quoted lines
>>> + 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