Re: Bug report - sparse-checkout ignores prefix when run in subdirectories
On Thu, Jan 27, 2022 at 8:43 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 60 quoted lines
>
> 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.
Show 15 quoted lines
> 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.
Lessley Dennington
