* Bug report - sparse-checkout ignores prefix when run in subdirectories @ 2022-01-05 20:13 Lessley Dennington 2022-01-05 22:38 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Lessley Dennington @ 2022-01-05 20:13 UTC (permalink / raw) To: Git Mailing List; +Cc: Elijah Newren, Derrick Stolee, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-05 20:13 Bug report - sparse-checkout ignores prefix when run in subdirectories Lessley Dennington @ 2022-01-05 22:38 ` Junio C Hamano 2022-01-05 23:19 ` Elijah Newren 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2022-01-05 22:38 UTC (permalink / raw) To: Lessley Dennington; +Cc: Git Mailing List, Elijah Newren, Derrick Stolee 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); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-05 22:38 ` Junio C Hamano @ 2022-01-05 23:19 ` Elijah Newren 2022-01-05 23:29 ` Junio C Hamano 2022-01-27 16:43 ` Derrick Stolee 0 siblings, 2 replies; 10+ messages in thread From: Elijah Newren @ 2022-01-05 23:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-05 23:19 ` Elijah Newren @ 2022-01-05 23:29 ` Junio C Hamano 2022-01-06 0:08 ` Elijah Newren 2022-01-27 16:43 ` Derrick Stolee 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2022-01-05 23:29 UTC (permalink / raw) To: Elijah Newren; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-05 23:29 ` Junio C Hamano @ 2022-01-06 0:08 ` Elijah Newren 2022-01-06 0:17 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Elijah Newren @ 2022-01-06 0:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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.) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-06 0:08 ` Elijah Newren @ 2022-01-06 0:17 ` Junio C Hamano 2022-01-06 1:08 ` Elijah Newren 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2022-01-06 0:17 UTC (permalink / raw) To: Elijah Newren; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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.) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-06 0:17 ` Junio C Hamano @ 2022-01-06 1:08 ` Elijah Newren 2022-01-06 19:25 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Elijah Newren @ 2022-01-06 1:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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.) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-06 1:08 ` Elijah Newren @ 2022-01-06 19:25 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2022-01-06 19:25 UTC (permalink / raw) To: Elijah Newren; +Cc: Lessley Dennington, Git Mailing List, Derrick Stolee 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". ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-05 23:19 ` Elijah Newren 2022-01-05 23:29 ` Junio C Hamano @ 2022-01-27 16:43 ` Derrick Stolee 2022-01-27 17:30 ` Elijah Newren 1 sibling, 1 reply; 10+ messages in thread From: Derrick Stolee @ 2022-01-27 16:43 UTC (permalink / raw) To: Elijah Newren, Junio C Hamano; +Cc: Lessley Dennington, Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Bug report - sparse-checkout ignores prefix when run in subdirectories 2022-01-27 16:43 ` Derrick Stolee @ 2022-01-27 17:30 ` Elijah Newren 0 siblings, 0 replies; 10+ messages in thread From: Elijah Newren @ 2022-01-27 17:30 UTC (permalink / raw) To: Derrick Stolee; +Cc: Junio C Hamano, Lessley Dennington, Git Mailing List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-27 17:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-05 20:13 Bug report - sparse-checkout ignores prefix when run in subdirectories Lessley Dennington 2022-01-05 22:38 ` Junio C Hamano 2022-01-05 23:19 ` Elijah Newren 2022-01-05 23:29 ` Junio C Hamano 2022-01-06 0:08 ` Elijah Newren 2022-01-06 0:17 ` Junio C Hamano 2022-01-06 1:08 ` Elijah Newren 2022-01-06 19:25 ` Junio C Hamano 2022-01-27 16:43 ` Derrick Stolee 2022-01-27 17:30 ` Elijah Newren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).