On Tue, Apr 13, 2021 at 7:01 AM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> When we have sparse directory entries in the index, we want to compare
> that directory against sparse-checkout patterns. Those pattern matching
> algorithms are built expecting a file path, not a directory path. This
> is especially important in the "cone mode" patterns which will match
> files that exist within the "parent directories" as well as the
> recursive directory matches.
>
> If path_matches_pattern_list() is given a directory, we can add a fake
> filename ("-") to the directory and get the same results as before,
> assuming we are in cone mode. Since sparse index requires cone mode
> patterns, this is an acceptable assumption.
Makes sense; thanks for the good description.
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> dir.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/dir.c b/dir.c
> index 166238e79f52..57e22e605cec 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1378,6 +1378,11 @@ enum pattern_match_result path_matches_pattern_list(
> strbuf_addch(&parent_pathname, '/');
> strbuf_add(&parent_pathname, pathname, pathlen);
>
> + /* Directory requests should be added as if they are a file */
"added" or "matched"? Also, the description seems a bit brief and
likely to surprise; I'd at least want to expand "file" to "file within
their given directory" but it might be nice to get some summarized
version of the commit message or at least state that "-" is just a
random simple name within the given directory.
> + if (parent_pathname.len > 1 &&
Is this line...
> + parent_pathname.buf[parent_pathname.len - 1] == '/')
to prevent an out-of-bounds indexing? If so, shouldn't it be "> 0" or
">= 1" rather than "> 1"? And if so, doesn't the strbuf_addch() call
above ensure the condition is always met?
Or are we trying to avoid adding the "-" when we parent_pathname is
just a plain "/"?
> + strbuf_add(&parent_pathname, "-", 1);
> +
Sorry for all the questions on such a tiny change. It makes sense to
me, I'm just curious whether it'll confuse future code readers.
> if (hashmap_contains_path(&pl->recursive_hashmap,
> &parent_pathname)) {
> result = MATCHED_RECURSIVE;
> --
