On Tue, Jul 13, 2021 at 5:57 AM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>
> On 7/12/2021 3:38 PM, Elijah Newren wrote:
> > On Mon, Jul 12, 2021 at 10:55 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxxx/
>
> 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.
Looks good to me. Re-roll with my Reviewed-by and let's get this
series merged down to next. :-)
>
> Thanks,
> -Stolee
>
>
> -- >8 --
>
> From 8e69def90f5844c117cc1e9efd673c92b85c9238 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> 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 &&
>
> --
