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

From: Elijah Newren <newren@gmail.com>
Date: Jul 7, 2021, 22:25 UTC
Message-ID: <CABPp-BE_vGHv7GTESwQbbJ-YSSzkDjapKScEhgYq9QZkVVEg5w@mail.gmail.com>
In-Reply-To: <237ccf4e43dd461250b3e6d609a475c1d675ea86.1624932294.git.gitgitgadget@gmail.com>

On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 7 quoted lines

> -       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".

Show 10 quoted lines

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

Show 16 quoted lines

> +        */
> +       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;
>  }