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

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

[Lessley Dennington]

Hello everyone! See the following bug report pertaining to sparse-checkout
when run outside top-level directories.

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

$ git init -b main tmp
$ cd tmp
$ mkdir -p a/b/c
$ touch a/b/c/d a/b/c/e
$ git add a/
$ git commit -m "initial"
$ cd a/  # Not at the top level anymore
$ git sparse-checkout set --cone b/c
$ git -C .. sparse-checkout list

What did you expect to happen? (Expected behavior)

We expected the output sparsity path to be a/b/c.

What happened instead? (Actual behavior)

The output was b/c. Running $ cat ../.git/info/sparse-checkout
showed that the paths in this file were incorrect:
/*
!/*/
/b/
!/b/*/
/b/c/

What's different between what you expected and what actually happened?

The output path and the paths in the sparse-checkout file are missing the
expected 'a' prefix. This means that the `set` and `add` subcommands of
`sparse-checkout` can only be safely run from a repo's top-level directory.

Anything else you want to add:

This was discovered by newren@gmail.com in my recent tab-completion
series. You can find additional context at [1].

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.34.1.88.gcecf501e07.dirty (local build, based off
en/sparse-checkout-set)
cpu: x86_64
shell-path: /bin/zsh
uname: Darwin
compiler info:
Configured with: --prefix=/Library/Developer/CommandLineTools/usr
--with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk
/usr/include/c++/4.2.1
Apple clang version 13.0.0 (clang-1300.0.29.3)
Target: x86_64-apple-darwin20.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
$SHELL (typically, interactive shell): /bin/zsh

-Lessley

[1]: 
https://lore.kernel.org/git/CABPp-BGdr54XgCXw8k1xRCgkwBtDonyODS3O+_nS_QY3SOEFGQ@mail.gmail.com/

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

[Junio C Hamano]

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]);
 	}
 
 	return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);

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

[Elijah Newren]

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?

>
>         return modify_pattern_list(argc, argv, set_opts.use_stdin, REPLACE);

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> 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?

Perhaps allow things like ":(glob,icase,top)frotz" to be passed and
treating them as pathspec is the most sensible way to go outside the
cone mode?  As far as I understand, the cone mode is a strict subset
that is straight-jacketed into "no patterns allowed, just concrete
directory names and nothing else" for simplicity, so it is perfectly
OK for these two to be taking different things.

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

[Elijah Newren]

On Wed, Jan 5, 2022 at 3:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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?
>
> Perhaps allow things like ":(glob,icase,top)frotz" to be passed and
> treating them as pathspec is the most sensible way to go outside the
> cone mode?  As far as I understand, the cone mode is a strict subset
> that is straight-jacketed into "no patterns allowed, just concrete
> directory names and nothing else" for simplicity, so it is perfectly
> OK for these two to be taking different things.

Are you possibly confusing pathspecs with gitignore patterns here?
(Or am I?)  Or are you suggesting that sparse-checkout be modified to
accept either gitignore-style files or pathspecs and handle both?

I have never liked that gitignore-style patterns were used for
sparse-checkout, but it has always seemed a bit too late to change it.
Ever since Stolee introduced cone-mode, my preferred strategy for
fixing the gitignore-style patterns problem, assuming we are allowed
to do so, is to deprecate if not kill non-cone mode.  (And in the
meantime, I'm just doing due diligence when I mention non-cone mode in
my reviews.)

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Are you possibly confusing pathspecs with gitignore patterns here?
> (Or am I?)  Or are you suggesting that sparse-checkout be modified to
> accept either gitignore-style files or pathspecs and handle both?

Much closer to the latter (actually, removing ignore patterns, and
make the subsystem work with pathspecs).

> I have never liked that gitignore-style patterns were used for
> sparse-checkout, but it has always seemed a bit too late to change it.

Perhaps.

> Ever since Stolee introduced cone-mode, my preferred strategy for
> fixing the gitignore-style patterns problem, assuming we are allowed
> to do so, is to deprecate if not kill non-cone mode.  (And in the
> meantime, I'm just doing due diligence when I mention non-cone mode in
> my reviews.)

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

[Elijah Newren]

On Wed, Jan 5, 2022 at 4:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Are you possibly confusing pathspecs with gitignore patterns here?
> > (Or am I?)  Or are you suggesting that sparse-checkout be modified to
> > accept either gitignore-style files or pathspecs and handle both?
>
> Much closer to the latter (actually, removing ignore patterns, and
> make the subsystem work with pathspecs).

Oh, interesting.  That'd at least suggest you're also happy to get rid
of the ignore patterns in this subsystem if we find a feasible
transition plan to do so.  I'll keep that in mind.  :-)

> > I have never liked that gitignore-style patterns were used for
> > sparse-checkout, but it has always seemed a bit too late to change it.
>
> Perhaps.
>
> > Ever since Stolee introduced cone-mode, my preferred strategy for
> > fixing the gitignore-style patterns problem, assuming we are allowed
> > to do so, is to deprecate if not kill non-cone mode.  (And in the
> > meantime, I'm just doing due diligence when I mention non-cone mode in
> > my reviews.)

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Jan 5, 2022 at 4:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > Are you possibly confusing pathspecs with gitignore patterns here?
>> > (Or am I?)  Or are you suggesting that sparse-checkout be modified to
>> > accept either gitignore-style files or pathspecs and handle both?
>>
>> Much closer to the latter (actually, removing ignore patterns, and
>> make the subsystem work with pathspecs).
>
> Oh, interesting.  That'd at least suggest you're also happy to get rid
> of the ignore patterns in this subsystem if we find a feasible
> transition plan to do so.  I'll keep that in mind.  :-)

It is more like "I've never used it and I'll probably never use it
myself, so I have no data as to how badly we will be hurting
existing users".

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

[Derrick Stolee]

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.

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

Thanks,
-Stolee

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

[Elijah Newren]

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.