Re: [PATCH 0/5] Sparse index: integrate with commit and checkout

From: Elijah Newren <newren@gmail.com>
Date: Jul 9, 2021, 21:26 UTC
Message-ID: <CABPp-BF_i1QRCXaeKzqoc6Q2=3un-wku7aKUEdBbXfeVfTG8xg@mail.gmail.com>
In-Reply-To: <pull.973.git.1624932786.gitgitgadget@gmail.com>

On Mon, Jun 28, 2021 at 7:13 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 41 quoted lines

>
> This series extends our integration of sparse-index to 'git commit' and 'git
> checkout'.
>
> This is based on ds/status-with-sparse-index (v7) and v2.32.0. The hard work
> was already done in that topic, so these changes are simple.
>
> Recall that we have delayed our integration with 'git add' until we can work
> out the concerns about how to deal with pathspecs outside of the
> sparse-checkout definition. Those concerns might have some overlap with how
> 'git commit' takes a pathspec, but this seems like a rare enough case to
> handle here and we can be more careful with the behavior change in the next
> series which will integrate with git add.
>
> In addition to the tests that already exist in t1092, I have integrated
> these changes in microsoft/git and tested them against the Scalar functional
> tests, which go through quite a few complicated scenarios, verifying that
> things work the same across the full index and sparse-index cases.
>
> Thanks, -Stolee
>
> Derrick Stolee (5):
>   p2000: add 'git checkout -' test and decrease depth
>   p2000: compress repo names
>   commit: integrate with sparse-index
>   sparse-index: recompute cache-tree
>   checkout: stop expanding sparse indexes
>
>  builtin/checkout.c                       |  8 ++--
>  builtin/commit.c                         |  3 ++
>  cache-tree.c                             |  2 -
>  sparse-index.c                           |  2 +
>  t/perf/p2000-sparse-operations.sh        | 47 ++++++++++++--------
>  t/t1092-sparse-checkout-compatibility.sh | 55 ++++++++++++++++++++++--
>  6 files changed, 89 insertions(+), 28 deletions(-)
>
>
> base-commit: 1d744848ee6b58ccaf3a30f20abe9797ed5d2ce7
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-973%2Fderrickstolee%2Fsparse-index%2Fcommit-and-checkout-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-973/derrickstolee/sparse-index/commit-and-checkout-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/973

I've read over these patches and didn't find any problems in them in my reading; however, since it builds upon ds/status-with-sparse-index (v7)...

I decided to retry some of my ideas and testing on Patch 10/16 of v7, over at https://lore.kernel.org/git/CABPp-BHwTAKwFiWQ0-2P=_g+7HLK5FfOAz-uujRjLou1fXT3zw@mail.gmail.com/

It turns out that the block you added there is now triggered by t1092 after this series, and the testcase won't pass without that block. It might be clearer to move that code fragment, or perhaps the whole patch, into this series...though the code fragment as is has introduced a bug. If you take t1092 test 12 ("diff with directory/file conflicts") and modify it so that before the

    git checkout df-conflict

invocation from sparse-index, you first run:

    $ git sparse-checkout disable
    $ echo more stuff >>folder1/edited-content
    $ git add -u
    $ git diff HEAD   # note the changes
    $ git sparse-checkout init --cone --sparse-index
    $ git diff HEAD   # note the changes are still there
    $ git checkout df-conflict  # no error??  What about the
conflicting changes?
    $ git diff HEAD

then the last command will show that the staged changes from before the commit have simply been discarded. In short, this makes the series behave like --force was passed with sparse directory entries, when --force wasn't passed.

So we've still got some directory/file conflict issues.