On Mon, Jul 12, 2021 at 10:55 AM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> This is the first "payoff" series in the sparse-index work. It makes 'git
> status' very fast when a sparse-index is enabled on a repository with
> cone-mode sparse-checkout (and a small populated set).
>
...
> Update in V8
> ============
>
> * The directory/file conflict patch is removed and delayed to the next
> series where it will be required. (It will also be improved in that
> series.)
>
> * Some comments have been improved, including a new assert() that helps
> document the situation.
>
This one looks really good. Just two minor comments:
> Range-diff vs v7:
>
...
> 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/ ?
> ++ * 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/
If you agree and include the two fixups above, the entire series is:
Reviewed-by: Elijah Newren <newren@xxxxxxxxx>
If you disagree, then all patches other than 9 and 14 can have my
Reviewed-by tag. :-)
Thanks for all the awesome work!
