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

[Derrick Stolee <stolee@gmail.com>]

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 &&
 
-- 
2.32.0.15.g108d1b86e21