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

From: Elijah Newren <newren@gmail.com>
Date: Jul 22, 2021, 04:22 UTC
Message-ID: <CABPp-BGbM_+bznuCCLfctt-XqVdCuROa9+s_jpeQXmTHEyZmjg@mail.gmail.com>
In-Reply-To: <pull.973.v2.git.1626812081.gitgitgadget@gmail.com>

On Tue, Jul 20, 2021 at 1:14 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 34 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.
>
>
> Update in V2
> ============
>
>  * There is no change to the code, but it is presented in a slightly
>    different order.
>  * We've been discussing some complicated directory/file conflict cases, in
>    particular with a staged change inside the directory. These tests are
>    added and described as documenting incorrect behavior that should be
>    changed.
>  * After those tests are in place, we can motivate the change to
>    twoway_merge() as necessary for a more-common situation (still rare) but
>    still incorrect in an already-broken situation. Hopefully that balance is
>    sufficient for now, until we can do the bigger work of fixing the bad
>    behavior.

I read the first five patches previously. The tiny changes there in the range-diff still look good to me.

I very much appreciate the new patch 6.

As noted in 7/7, I'm a little unhappy with the patch to twoway_merge(), BUT you've clearly documented the shortcomings in very good detail and pointed out how git has (likely for decades) messed up in related ways for non-sparse checkouts with D/F conflicts. You've documented it well enough and argued well enough about the relative merits, that I have to agree with you that this is a good step forward. I do hope we circle back and tie up the loose ends at some point.

So, the whole series is:

Reviewed-by: Elijah Newren <newren@gmail.com>

Show 51 quoted lines

>
> Thanks, -Stolee
>
> Derrick Stolee (7):
>   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
>   t1092: document bad 'git checkout' behavior
>   unpack-trees: resolve sparse-directory/file conflicts
>
>  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 | 197 ++++++++++++++++++++++-
>  unpack-trees.c                           |  11 ++
>  7 files changed, 240 insertions(+), 30 deletions(-)
>
>
> base-commit: e5ca291076a8a936283bb2c57433c4393d3f80c2
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-973%2Fderrickstolee%2Fsparse-index%2Fcommit-and-checkout-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-973/derrickstolee/sparse-index/commit-and-checkout-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/973
>
> Range-diff vs v1:
>
>  1:  bb3dd1fdd48 = 1:  6e74958f590 p2000: add 'git checkout -' test and decrease depth
>  2:  eb15bf37685 = 2:  3e1d03c41be p2000: compress repo names
>  3:  413babe6e77 ! 3:  cd94f820052 commit: integrate with sparse-index
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is e
>       + ensure_not_expanded commit --include deep/deeper1/a -m deeper
>        '
>
>      - test_expect_success 'reset mixed and checkout orphan' '
>      + # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>  4:  ffe8473caab = 4:  65e79b8037c sparse-index: recompute cache-tree
>  5:  8710fee36b7 ! 5:  e9a9981477e checkout: stop expanding sparse indexes
>      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
>       + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1
>        '
>
>      - test_expect_success 'reset mixed and checkout orphan' '
>      + # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
>  -:  ----------- > 6:  4b801c854fb t1092: document bad 'git checkout' behavior
>  -:  ----------- > 7:  71e301501c8 unpack-trees: resolve sparse-directory/file conflicts
>
> --
> gitgitgadget