Re: [PATCH v8 00/15] Sparse-index: integrate with status

>
> On 7/12/2021 3:38 PM, Elijah Newren wrote:
> > On Mon, Jul 12, 2021 at 10:55 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>   9:  237ccf4e43d !  9:  c0b0b58584c unpack-trees: unpack sparse directory entries
> >>      @@ unpack-trees.c: static int find_cache_pos(struct traverse_info *info,
> >>       +  * Check for a sparse-directory entry named "path/".
> >>       +  * Due to the input p->path not having a trailing
> >>       +  * slash, the negative 'pos' value overshoots the
> >>      -+  * expected position by at least one, hence "-2" here.
> >>      ++  * expected position, hence "-2" instead of "-1".
> >>       +  */
> >>       + pos = -pos - 2;
> >>       +
> >>      @@ unpack-trees.c: static int find_cache_pos(struct traverse_info *info,
> >>                 return NULL;
> >>       +
> >>       + /*
> >>      -+  * We might have multiple entries between 'pos' and
> >>      -+  * the actual sparse-directory entry, so start walking
> >>      -+  * back until finding it or passing where it would be.
> >>      ++  * Due to lexicographic sorting and sparse directory
> >>      ++  * entried ending with a trailing slash, our path as a
> >
> > s/entried/entries/ ?
>
> Oops! Yes, that would be a valuable fixup. Thanks for catching it.
>
> >
> >>      ++  * sparse directory (e.g "subdir/") and our path as a
> >>      ++  * file (e.g. "subdir") might be separated by other
> >>      ++  * paths (e.g. "subdir-").
> >>       +  */
> >>       + while (pos >= 0) {
> >>       +         ce = o->src_index->cache[pos];
> > ...
> >>  15:  717a3f49f97 = 14:  dada1b91bdc wt-status: expand added sparse directory entries
> >
> > As I commented over at [1], I would appreciate if we could at least
> > add a comment in the testcase that we know this testcase triggers a
> > bug for both sparse-index and sparse-checkout...and that fixing it
> > might affect the other comments and commands within that testcase in
> > the future...but for now, we're just testing as best we can that the
> > two give the same behavior.
> >
> > [1] https://lore.kernel.org/git/CABPp-BGJ+LTubgS=zvGJjk3kgyfW-7UFEa=qg-0mdyrY32j0pQ@mail.gmail.com/
>
> How do you feel about a new patch that focuses on adding these
> comments, including an older test that had a similar documentation
> of the behavior change? A patch that could be queued on top of
> this series is pasted below the cutline.

>
> Thanks,
> -Stolee
>
>
> -- >8 --
>
> From 8e69def90f5844c117cc1e9efd673c92b85c9238 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <dstolee@microsoft.com>
> Date: Tue, 13 Jul 2021 08:50:24 -0400
> Subject: [PATCH 16/15] t1092: document bad sparse-checkout behavior
>
> There are several situations where a repository with sparse-checkout
> enabled will act differently than a normal repository, and in ways that
> are not intentional. The test t1092-sparse-checkout-compatibility.sh
> documents some of these deviations, but a casual reader might think
> these are intentional behavior changes.
>
> Add comments on these tests that make it clear that these behaviors
> should be updated. Using 'NEEDSWORK' helps contributors find that these
> are potential areas for improvement.
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 2394c36d881..cabbd42e339 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -392,8 +392,8 @@ test_expect_failure 'blame with pathspec outside sparse definition' '
>         test_all_match git blame deep/deeper2/deepest/a
>  '
>
> -# TODO: reset currently does not behave as expected when in a
> -# sparse-checkout.
> +# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> +# in this scenario, but it shouldn't.
>  test_expect_failure 'checkout and reset (mixed)' '
>         init_repos &&
>
> @@ -403,8 +403,8 @@ test_expect_failure 'checkout and reset (mixed)' '
>         test_all_match git reset update-folder2
>  '
>
> -# Ensure that sparse-index behaves identically to
> -# sparse-checkout with a full index.
> +# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> +# in this scenario, but it shouldn't.
>  test_expect_success 'checkout and reset (mixed) [sparse]' '
>         init_repos &&
>
> @@ -524,6 +524,8 @@ test_expect_success 'sparse-index is not expanded' '
>         test_region ! index ensure_full_index trace2.txt
>  '
>
> +# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> +# in this scenario, but it shouldn't.
>  test_expect_success 'reset mixed and checkout orphan' '
>         init_repos &&
>
> --