Re: Bug report - sparse-checkout ignores prefix when run in subdirectories

[Elijah Newren <newren@gmail.com>]

On Thu, Jan 27, 2022 at 8:43 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/5/2022 6:19 PM, Elijah Newren wrote:
> > On Wed, Jan 5, 2022 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Lessley Dennington <lessleydennington@gmail.com> writes:
> >>
> >>> Hello everyone! See the following bug report pertaining to sparse-checkout
> >>> when run outside top-level directories.
> >>
> >> In a bug report it is fine, but "outside top-level" usually means
> >> above the top-level of the working tree.  Here, I think you meant
> >> running in a subdirectory of the top-level.
> >>
> >> Perhaps something along this line?
> >>
> >>  builtin/sparse-checkout.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c
> >> index 45e838d8f8..4e5efbb85e 100644
> >> --- c/builtin/sparse-checkout.c
> >> +++ w/builtin/sparse-checkout.c
> >> @@ -753,6 +753,16 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
> >>         if (!core_sparse_checkout_cone && argc == 0) {
> >>                 argv = default_patterns;
> >>                 argc = default_patterns_nr;
> >> +       } else if (argc && prefix && *prefix) {
> >> +               /*
> >> +                * The args are not pathspecs, so unfortunately we
> >> +                * cannot imitate how cmd_add() uses parse_pathspec().
> >> +                */
> >> +               int i;
> >> +               int prefix_len = strlen(prefix);
> >> +
> >> +               for (i = 0; i < argc; i++)
> >> +                       argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> >>         }
> >
> > This looks good (sparse_checkout_add() needs a similar fix), at least
> > for cone mode.  There might be a small pickle here that I didn't think
> > about before.  --cone mode always uses directories, so we expect
> > people to provide directory names.  Because of that, I think it's fair
> > to expect the arguments passed to `set` or `add` to be paths relative
> > to the current working directory.  In contrast, for non-cone mode we
> > do not expect pathnames but gitignore-style globs.  And when we get
> > gitignore-style globs, we don't know if they were intended relative to
> > the current working directory or the toplevel, because we only have
> > one $GIT_DIR/info/sparse-checkout file versus many .gitignore files.
> > So, should "**.py" go directly into the sparse-checkout file as-is, or
> > be translated to "my/current/subdir/**.py" first?
> >
> > Maybe translating is always fine, or maybe we want to throw an error
> > when: (not using cone mode and prefix is non-empty and any patterns
> > are provided).
> >
> > Thoughts?
>
> You seem to have worked it out in the other threads, but I came here
> to agree: we should not do this transformation unless we are in
> cone mode. We should also do this when "--cone" is supplied.

Actually, I didn't figure it out.  See also the last four paragraphs
of https://lore.kernel.org/git/CABPp-BEwMAPHGt5xD9jDU58grbrAqCdqNY9Nh8UJGLKuLbArXQ@mail.gmail.com/
for related confounding factors.

Anyway, Junio's insistence elsewhere (earlier in the above linked
thread) that bash-completion include both files and directories in
non-cone mode seemed to only make sense if we did this transformation
in non-cone mode.  So, I implemented the transformation for both
modes, though I called out how it causes some possibly nasty surprises
in non-cone mode.  It's not yet submitted (wanted other series to
merge to next first), but you can see it at
https://github.com/gitgitgadget/git/pull/1118; it's the commit
entitled, "Pay attention to prefix for {set, add}".  The final patch
in that series also adds a "NON-CONE PROBLEMS" section to the manual
detailing six different problems with non-cone mode (all of which are
fundamental and unfixable), one of which is this one.  The fourth
patch also possibly mixes in with this topic.

Anyway, whether or not there is a clean answer here, that other series
at least carefully points out how non-cone mode is fundamentally
problematic and deprecates it.

> The prefix_path() collapses "../" entries, right? Just making sure
> that we test that scenario when writing a full fix here.
>
> For example, if we added a case to t1092, we should be able to
> do the following within any of the example repos:
>
>         git sparse-checkout disable &&
>         cd folder1 &&
>         git sparse-checkout set --cone . ../folder2
>         git sparse-checkout list >actual &&
>         cat >expect <<-EOF &&
>         folder1
>         folder2
>         EOF
>         test_cmp expect actual

Yes, "../" is handled; I added testcases to t1091 to verify that; they
look pretty much exactly like what you suggest here, so we seem to be
on the same wavelength.