Re: [PATCH v7 09/16] unpack-trees: unpack sparse directory entries

On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

-       else
+
+       /*
+        * 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.

You added the qualifier "at least" to this comment since v5.  I think
it's slightly misleading because it sounds like -2 is the end of the
special handling of the "at least" one overshoot.  Perhaps if you
ended with... "hence '-2' instead of '-1' here, and we also need to
check below if we overshot more than one".

+        */
+       pos = -pos - 2;
+
+       if (pos < 0 || pos >= o->src_index->cache_nr)
                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.

It might be helpful to add a quick comment about the scenario where
this comes up.  e.g.

    This arises due to lexicographic sort ordering and sparse
directory entries coming with a trailing slash, causing there to be
multiple entries between "subdir" and "subdir/" (such as anything
beginning with "subdir." or "subdir-").  We are trying to walk back
from "subdir/" to "subdir" here.

+        */
+       while (pos >= 0) {
+               ce = o->src_index->cache[pos];
+
+               if (strncmp(ce->name, p->path, p->pathlen))
+                       return NULL;
+
+               if (S_ISSPARSEDIR(ce->ce_mode) &&
+                   sparse_dir_matches_path(ce, info, p))
+                       return ce;
+
+               pos--;
+       }
+
+       return NULL;
 }