Re: [PATCH 05/27] add: ensure full index

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 17, 2021, 20:55 UTC
Message-ID: <862197d9-ca71-f682-f03f-62e45ea921ee@gmail.com>
In-Reply-To: <CAHd-oW4DMAjQ_Gjbzkx6csFRAK1iKHsTfuJLniOjWehwrnHdVQ@mail.gmail.com>

On 3/17/2021 4:35 PM, Matheus Tavares Bernardino wrote:

Show 15 quoted lines

> On Wed, Mar 17, 2021 at 2:36 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:>> I'm not so sure about this one; from git-add(1):
>>
>>        --renormalize
>>            Apply the "clean" process freshly to all tracked files to forcibly
>>            add them again to the index. This is useful after changing
>>            core.autocrlf configuration or the text attribute in order to
>>            correct files added with wrong CRLF/LF line endings. This option
>>            implies -u.
>>
>> ... to "all tracked files".  Should that be interpreted as all files
>> within the sparsity paths, especially considering that we're trying to
>> enable folks to work within the set of files of interest to them?

I wrote in reply to another similar comment that I'm being overly cautious in creating these protections. When I can come back later with careful tests, we can ensure that everything behaves properly.

Show 10 quoted lines

> I had the same question when working on the add/rm sparse-checkout
> series. As seen at [1], --renormalize and --chmod are, currently, the
> only options in git-add that do not honor the sparsity patterns and do
> update SKIP_WORKTREE paths too.
> 
> But is this by design or possibly just an oversight? In my series I
> ended up making both options honor the sparsity rules, with a warning
> when requested to update any sparse path. (If we are going with this
> approach, perhaps should we also amend the docs to remove any
> ambiguity as for what "all tracked files" means in this case?)

I'd be happy to see a resolution here, and it can happen in parallel to what I'm doing here.

Show 12 quoted lines

>> The code below already has the following check below:
>>
>>     if (!S_ISREG(ce->ce_mode) && !S_ISLNK(ce->ce_mode))
>>       continue; /* do not touch non blobs */
>>
>> suggesting that it'd correctly skip over the sparse directories, so I
>> think this one just isn't needed.
> 
> But if sparse index is not enabled, it does not skip over the sparse
> files, right? So isn't the ensure_full_index() call here (and in
> chmod_pathspec()) important to be consistent with the case when sparse
> index is not in use?

The tests I am adding in t1092-sparse-checkout-compatibility.sh are focused on ensuring the same behavior across sparse-checkouts with and without the sparse-index, and also a full checkout (when possible).

Since the sparse-index requires cone-mode sparse-checkout (currently), and the sparse files to be within a directory (or no index change happens), the tests you created in t3705-add-sparse-checkout.sh don't seem to apply. I'll need to find some important scenarios to duplicate in t1092 without the full depth of t3705.

> Or maybe Stolee could rebase this on top of my
> series, where both these places already skip the sparse paths?
> (Assuming that's the behavior we are looking for. If not, I can also> fix my series.)

I think they can proceed in parallel and then continue more carefully in the future. The series _after_ this one makes 'git status' and 'git add' work with the sparse-index, so at that point we will be removing these protections at the right time, with tests. In addition, those tests will ensure that we don't expand the sparse-index unless absolutely necessary.

If that work collides with your approach, then I'll rebase onto a merge of that topic and this one.

> [1]: https://lore.kernel.org/git/d93c3f51465a3e409819db63bd81802e7ef20ea9.1615588108.git.matheus.bernardino@usp.br/

 
(I will go take a look over here. I've been ignoring the thread for
too long.)

Thanks, -Stolee