Re: [PATCH v5 0/7] Sparse Index: diff and blame builtins

[Elijah Newren <newren@gmail.com>]

On Fri, Dec 3, 2021 at 1:16 PM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series is based on vd/sparse-reset. It integrates the sparse index with
> git diff and git blame and includes:
>
>  1. tests added to t1092 and p2000 to establish the baseline functionality
>     of the commands
>  2. repository settings to enable the sparse index
>
> The p2000 tests demonstrate a ~44% execution time reduction for 'git diff'
> and a ~86% execution time reduction for 'git diff --staged' using a sparse
> index. For 'git blame', the reduction time was ~60% for a file two levels
> deep and ~30% for a file three levels deep.
>
> Test                                         before  after
> ----------------------------------------------------------------
> 2000.30: git diff (full-v3)                  0.33    0.34 +3.0%
> 2000.31: git diff (full-v4)                  0.33    0.35 +6.1%
> 2000.32: git diff (sparse-v3)                0.53    0.31 -41.5%
> 2000.33: git diff (sparse-v4)                0.54    0.29 -46.3%
> 2000.34: git diff --cached (full-v3)         0.07    0.07 +0.0%
> 2000.35: git diff --cached (full-v4)         0.07    0.08 +14.3%
> 2000.36: git diff --cached (sparse-v3)       0.28    0.04 -85.7%
> 2000.37: git diff --cached (sparse-v4)       0.23    0.03 -87.0%
> 2000.62: git blame f2/f4/a (full-v3)         0.31    0.32 +3.2%
> 2000.63: git blame f2/f4/a (full-v4)         0.29    0.31 +6.9%
> 2000.64: git blame f2/f4/a (sparse-v3)       0.55    0.23 -58.2%
> 2000.65: git blame f2/f4/a (sparse-v4)       0.57    0.23 -59.6%
> 2000.66: git blame f2/f4/f3/a (full-v3)      0.77    0.85 +10.4%
> 2000.67: git blame f2/f4/f3/a (full-v4)      0.78    0.81 +3.8%
> 2000.68: git blame f2/f4/f3/a (sparse-v3)    1.07    0.72 -32.7%
> 2000.99: git blame f2/f4/f3/a (sparse-v4)    1.05    0.73 -30.5%
>
>
>
> Changes since V1
> ================
>
>  * Fix failing diff partially-staged test in
>    t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
>
>
> Changes since V2
> ================
>
>  * Update diff commit description to include patches that make the checkout
>    and status commands work with the sparse index for readers to reference.
>  * Add new test case to verify diff behaves as expected when run against
>    files outside the sparse checkout cone.
>  * Indent error message in blame commit
>  * Check error message in blame with pathspec outside sparse definition test
>    matches expectations.
>  * Loop blame tests (instead of running the same command multiple time
>    against different files).
>
>
> Changes since V3
> ================
>
>  * Update diff p2000 tests to use --cached instead of --staged. Execute new
>    run and update results in commit description and cover letter.
>  * Update comment on blame with pathspec outside sparse definition test in
>    t1092-sparse-checkout-compatibility.sh to clarify that it tests the
>    current state and could be improved in the future.
>  * Ensure sparse index is only activated when diff is running against files
>    in a Git repo.
>  * BUG if prepare_repo_settings() is called outside a repository.
>  * Ensure sparse index is not activated for calls to blame, checkout, or
>    pack-object with -h.
>  * Ensure commit-graph is only loaded if a git directory exists.
>
>
> Changes since V4
> ================
>
>  * Remove startup_info->have_repository check from checkout, pack-objects,
>    and blame. Update git.c to no longer bypass setup when -h is passed
>    instead.
>  * Move commit-graph, test-read-cache, and repo-settings changes into their
>    own patches with details in commit description of why the changes are
>    being made.
>  * Update t1092-sparse-checkout-compatibility.sh tests to use --cached
>    instead of --staged.
>  * Use 10-character hash abbreviations for commits referenced in diff commit
>    message.
>  * Clarify that being unable to blame files outside the working directory is
>    not supported in either sparse or non-sparse checkouts both in comment on
>    blame with pathspec outside sparse definition test in
>    t1092-sparse-checkout-compatibility.sh and blame commit message.

This round addresses all my concerns from previous rounds.  There's a
trivial typo in the subject of the new patch 1, but feel free to add
my

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

when you resubmit with that fix.  Nice work!