* [PATCH] sparse-checkout: create leading directory @ 2022-01-20 18:55 Jonathan Tan 2022-01-20 20:26 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Jonathan Tan @ 2022-01-20 18:55 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, gitster, stolee, avarab When creating the sparse-checkout file, Git does not create the leading directory, "$GIT_DIR/info", if it does not exist. This causes problems if the repository does not have that directory. Therefore, ensure that the leading directory is created. This is the only "open" in builtin/sparse-checkout.c that does not have a leading directory check. (The other one in write_patterns_and_update() does.) Note that the test needs to explicitly specify a template when running "git init" because the default template used in the tests has the "info/" directory included. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This problem is being discussed in [1], and we also noticed this problem internally at $DAYJOB. Here's a fix. [1] https://lore.kernel.org/git/db6f47a3-0df3-505b-b391-6ca289fd61b5@gmail.com/ --- builtin/sparse-checkout.c | 3 +++ t/t1091-sparse-checkout-builtin.sh | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 679c107036..2b0e1db2d2 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv) FILE *fp; /* assume we are in a fresh repo, but update the sparse-checkout file */ + if (safe_create_leading_directories(sparse_filename)) + die(_("unable to create leading directories of %s"), + sparse_filename); fp = xfopen(sparse_filename, "w"); if (!fp) die(_("failed to open '%s'"), sparse_filename); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 42776984fe..dba0737599 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' ' check_files repo a ' +test_expect_success 'git sparse-checkout init in empty repo' ' + test_when_finished rm -rf empty-repo blank-template && + mkdir blank-template && + git init --template=blank-template empty-repo && + git -C empty-repo sparse-checkout init +' + test_expect_success 'git sparse-checkout list after init' ' git -C repo sparse-checkout list >actual && cat >expect <<-\EOF && -- 2.34.1.703.g22d0c6ccf7-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] sparse-checkout: create leading directory 2022-01-20 18:55 [PATCH] sparse-checkout: create leading directory Jonathan Tan @ 2022-01-20 20:26 ` Junio C Hamano 2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason 2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan 2 siblings, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2022-01-20 20:26 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, stolee, avarab Jonathan Tan <jonathantanmy@google.com> writes: > When creating the sparse-checkout file, Git does not create the leading > directory, "$GIT_DIR/info", if it does not exist. This causes problems > if the repository does not have that directory. Therefore, ensure that > the leading directory is created. > > This is the only "open" in builtin/sparse-checkout.c that does not have > a leading directory check. (The other one in write_patterns_and_update() > does.) > > Note that the test needs to explicitly specify a template when running > "git init" because the default template used in the tests has the > "info/" directory included. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This problem is being discussed in [1], and we also noticed this problem > internally at $DAYJOB. Here's a fix. Interesting, as $GIT_DIR/info is created during "git init". But it is possible to lose the directory if there is no need for any of the files---the user may rmdir or rm -fr it, and it is too harsh to call the repository "corrupted". Will queue. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sparse-checkout: create leading directory 2022-01-20 18:55 [PATCH] sparse-checkout: create leading directory Jonathan Tan 2022-01-20 20:26 ` Junio C Hamano @ 2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason 2022-01-22 7:58 ` SZEDER Gábor 2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan 2 siblings, 1 reply; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-20 21:30 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, gitster, stolee On Thu, Jan 20 2022, Jonathan Tan wrote: > When creating the sparse-checkout file, Git does not create the leading > directory, "$GIT_DIR/info", if it does not exist. This causes problems > if the repository does not have that directory. Therefore, ensure that > the leading directory is created. > > This is the only "open" in builtin/sparse-checkout.c that does not have > a leading directory check. (The other one in write_patterns_and_update() > does.) > > Note that the test needs to explicitly specify a template when running > "git init" because the default template used in the tests has the > "info/" directory included. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This problem is being discussed in [1], and we also noticed this problem > internally at $DAYJOB. Here's a fix. > > [1] https://lore.kernel.org/git/db6f47a3-0df3-505b-b391-6ca289fd61b5@gmail.com/ Thanks. This fix looks good to me. Looking at my [1] my preliminary analysis there wasn't correct, i.e. there are other cases where we get the "sparse_filename" that you don't cover here, but e.g. if you do: <your test> && rm -rf .git/info && git sparse-checkout add foo It looks like it will fail either way, so for those other codepaths we didn't need to ensure the .git/info. Still, it would be nice to have the fix test for those cases too, i.e. how various operations are expected to behave if the user were to rm -rf .git/info. That'll obviously yield a broken repo, but we should still try to handle it gracefully. 1. https://lore.kernel.org/git/211220.86zgovt9bi.gmgdl@evledraar.gmail.com/ > --- > builtin/sparse-checkout.c | 3 +++ > t/t1091-sparse-checkout-builtin.sh | 7 +++++++ > 2 files changed, 10 insertions(+) > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index 679c107036..2b0e1db2d2 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv) > FILE *fp; > > /* assume we are in a fresh repo, but update the sparse-checkout file */ > + if (safe_create_leading_directories(sparse_filename)) > + die(_("unable to create leading directories of %s"), > + sparse_filename); > fp = xfopen(sparse_filename, "w"); > if (!fp) > die(_("failed to open '%s'"), sparse_filename); > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 42776984fe..dba0737599 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' ' > check_files repo a > ' > > +test_expect_success 'git sparse-checkout init in empty repo' ' > + test_when_finished rm -rf empty-repo blank-template && > + mkdir blank-template && > + git init --template=blank-template empty-repo && > + git -C empty-repo sparse-checkout init > +' > + > test_expect_success 'git sparse-checkout list after init' ' > git -C repo sparse-checkout list >actual && > cat >expect <<-\EOF && You're using an overly verbose way to say "no templates, please". You can squash this in, i.e. --template= is an explicitly supported way to do that. diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index dba07375993..1fe741d9cd5 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -80,9 +80,8 @@ test_expect_success 'git sparse-checkout init' ' ' test_expect_success 'git sparse-checkout init in empty repo' ' - test_when_finished rm -rf empty-repo blank-template && - mkdir blank-template && - git init --template=blank-template empty-repo && + test_when_finished rm -rf empty-repo && + git init --template= empty-repo && git -C empty-repo sparse-checkout init ' ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] sparse-checkout: create leading directory 2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason @ 2022-01-22 7:58 ` SZEDER Gábor 0 siblings, 0 replies; 14+ messages in thread From: SZEDER Gábor @ 2022-01-22 7:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Jonathan Tan, git, gitster, stolee On Thu, Jan 20, 2022 at 10:30:03PM +0100, Ævar Arnfjörð Bjarmason wrote: > > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > > index 42776984fe..dba0737599 100755 > > --- a/t/t1091-sparse-checkout-builtin.sh > > +++ b/t/t1091-sparse-checkout-builtin.sh > > @@ -79,6 +79,13 @@ test_expect_success 'git sparse-checkout init' ' > > check_files repo a > > ' > > > > +test_expect_success 'git sparse-checkout init in empty repo' ' > > + test_when_finished rm -rf empty-repo blank-template && > > + mkdir blank-template && > > + git init --template=blank-template empty-repo && > > + git -C empty-repo sparse-checkout init > > +' > > + > > test_expect_success 'git sparse-checkout list after init' ' > > git -C repo sparse-checkout list >actual && > > cat >expect <<-\EOF && > > You're using an overly verbose way to say "no templates, please". You > can squash this in, i.e. --template= is an explicitly supported way to > do that. It would be even more expressive to use 'git init --no-template', but, alas, that doesn't work: $ git init --no-template /tmp/foo Initialized empty Git repository in /tmp/foo/.git/ $ ls /tmp/foo/.git branches config description HEAD hooks info objects refs ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] sparse-checkout: create leading directory 2022-01-20 18:55 [PATCH] sparse-checkout: create leading directory Jonathan Tan 2022-01-20 20:26 ` Junio C Hamano 2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason @ 2022-01-21 17:44 ` Jonathan Tan 2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason 2022-01-22 2:33 ` Elijah Newren 2 siblings, 2 replies; 14+ messages in thread From: Jonathan Tan @ 2022-01-21 17:44 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, gitster, avarab, jabolopes When creating the sparse-checkout file, Git does not create the leading directory, "$GIT_DIR/info", if it does not exist. This causes problems if the repository does not have that directory. Therefore, ensure that the leading directory is created. This is the only "open" in builtin/sparse-checkout.c that does not have a leading directory check. (The other one in write_patterns_and_update() does.) Note that the test needs to explicitly specify a template when running "git init" because the default template used in the tests has the "info/" directory included. Helped-by: Jose Lopes <jabolopes@google.com> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Changes from v1: - made test simpler - added attribution to Jose Lopes for finding and making the first draft of this patch (after confirming with them) Ævar mentioned "git sparse-checkout add" but I think that that is a different problem - in the "git sparse-checkout init" case, we could get into this case with a template that does not have .git/info, but in the "git sparse-checkout add" case, the user would have had to explicitly remove the info directory. So I'll limit this patch to the "init" case, for now. --- builtin/sparse-checkout.c | 3 +++ t/t1091-sparse-checkout-builtin.sh | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 679c107036..2b0e1db2d2 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv) FILE *fp; /* assume we are in a fresh repo, but update the sparse-checkout file */ + if (safe_create_leading_directories(sparse_filename)) + die(_("unable to create leading directories of %s"), + sparse_filename); fp = xfopen(sparse_filename, "w"); if (!fp) die(_("failed to open '%s'"), sparse_filename); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 42776984fe..3189d3da96 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' ' check_files repo a ' +test_expect_success 'git sparse-checkout init in empty repo' ' + test_when_finished rm -rf empty-repo blank-template && + git init --template= empty-repo && + git -C empty-repo sparse-checkout init +' + test_expect_success 'git sparse-checkout list after init' ' git -C repo sparse-checkout list >actual && cat >expect <<-\EOF && -- 2.35.0.rc0.227.g00780c9af4-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan @ 2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason 2022-01-22 5:21 ` Elijah Newren 2022-01-22 2:33 ` Elijah Newren 1 sibling, 1 reply; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-21 18:48 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, gitster, jabolopes On Fri, Jan 21 2022, Jonathan Tan wrote: > When creating the sparse-checkout file, Git does not create the leading > directory, "$GIT_DIR/info", if it does not exist. This causes problems > if the repository does not have that directory. Therefore, ensure that > the leading directory is created. > > This is the only "open" in builtin/sparse-checkout.c that does not have > a leading directory check. (The other one in write_patterns_and_update() > does.) > > Note that the test needs to explicitly specify a template when running > "git init" because the default template used in the tests has the > "info/" directory included. > > Helped-by: Jose Lopes <jabolopes@google.com> > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Changes from v1: > - made test simpler This partial application of the fix-up I suggested in https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/ leaves the now-unused "blank-template" > - added attribution to Jose Lopes for finding and making the first > draft of this patch (after confirming with them) > > Ævar mentioned "git sparse-checkout add" but I think that that is a > different problem - in the "git sparse-checkout init" case, we could get > into this case with a template that does not have .git/info, but in the > "git sparse-checkout add" case, the user would have had to explicitly > remove the info directory. So I'll limit this patch to the "init" case, > for now. > --- > builtin/sparse-checkout.c | 3 +++ > t/t1091-sparse-checkout-builtin.sh | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index 679c107036..2b0e1db2d2 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv) > FILE *fp; > > /* assume we are in a fresh repo, but update the sparse-checkout file */ > + if (safe_create_leading_directories(sparse_filename)) > + die(_("unable to create leading directories of %s"), > + sparse_filename); > fp = xfopen(sparse_filename, "w"); > if (!fp) > die(_("failed to open '%s'"), sparse_filename); > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 42776984fe..3189d3da96 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' ' > check_files repo a > ' > > +test_expect_success 'git sparse-checkout init in empty repo' ' > + test_when_finished rm -rf empty-repo blank-template && > + git init --template= empty-repo && > + git -C empty-repo sparse-checkout init > +' I agree that it's a slightly different problem, but I was just advocating for us testing what happened in these cases. The below fix-up does that. I think we should use warning_errno() there instead of some specutalite "file may not exist", but with/without this patch these tests show that only the "init" case was broken. As a more general issue I don't understand why "add" and "init" need to be conceptually different operations. If what defines a sparse checkout is just that it has that file and the 2 default patterns, which unless I'm missing something is the case. Why isn't "add" merely an "init" that'll optimistically add whatever pattern you asked for, in addition to doing an "init" if you didn't already? Then "add" and "init" will share the same error recovery behavior, and you won't needlessly have to run "init/add x" just to start using sparse-checkout with a pattern of "x". But I've never *actually* used this command, so maybe I'm just missing something obvious... diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 3189d3da965..6b56d9d177f 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' ' ' test_expect_success 'git sparse-checkout init in empty repo' ' - test_when_finished rm -rf empty-repo blank-template && + test_when_finished rm -rf empty-repo && git init --template= empty-repo && git -C empty-repo sparse-checkout init ' +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' ' + test_when_finished "rm -rf empty" && + git init --template= empty && + git -C empty sparse-checkout init && + rm -rf empty/.git/info && + + cat >expect <<-\EOF && + fatal: unable to load existing sparse-checkout patterns + EOF + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual && + test_cmp expect actual +' + +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' ' + test_when_finished "rm -rf empty" && + git init --template= empty && + git -C empty sparse-checkout init && + rm -rf empty/.git/info && + + cat >expect <<-\EOF && + warning: this worktree is not sparse (sparse-checkout file may not exist) + EOF + git -C empty sparse-checkout list 2>actual && + test_cmp expect actual +' + test_expect_success 'git sparse-checkout list after init' ' git -C repo sparse-checkout list >actual && cat >expect <<-\EOF && ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason @ 2022-01-22 5:21 ` Elijah Newren 2022-01-22 12:06 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 14+ messages in thread From: Elijah Newren @ 2022-01-22 5:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jonathan Tan, Git Mailing List, Junio C Hamano, jabolopes On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Fri, Jan 21 2022, Jonathan Tan wrote: > ... > This partial application of the fix-up I suggested in > https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/ > leaves the now-unused "blank-template" Good point; no need to remove something that isn't being created anymore. > > > - added attribution to Jose Lopes for finding and making the first > > draft of this patch (after confirming with them) > > > > Ævar mentioned "git sparse-checkout add" but I think that that is a > > different problem - in the "git sparse-checkout init" case, we could get > > into this case with a template that does not have .git/info, but in the > > "git sparse-checkout add" case, the user would have had to explicitly > > remove the info directory. So I'll limit this patch to the "init" case, > > for now. ... > > I agree that it's a slightly different problem, but I was just > advocating for us testing what happened in these cases. > > The below fix-up does that. Different problem...addressed with a "fix-up"? Why would we squash extra testing of a different problem into the same commit? I think it'd at least deserve its own commit message. > I think we should use warning_errno() there > instead of some specutalite "file may not exist", but with/without this > patch these tests show that only the "init" case was broken. > > As a more general issue I don't understand why "add" and "init" need to > be conceptually different operations. If what defines a sparse checkout > is just that it has that file and the 2 default patterns, which unless > I'm missing something is the case. > > Why isn't "add" merely an "init" > that'll optimistically add whatever pattern you asked for, in addition > to doing an "init" if you didn't already? > > Then "add" and "init" will share the same error recovery behavior, and > you won't needlessly have to run "init/add x" just to start using > sparse-checkout with a pattern of "x". The high level idea you propose (" 'add' can initialize sparsity state if not currently in a sparse checkout") could make sense. It isn't just a straightforward switchover with the various other config settings and such, and would also necessitate adding additional command line flags to the 'add' subcommand, but it could be done. Didn't occur to me before; I'm not sure if such a change in UI would be better or worse. I'm inclined to leave it as it is, though, especially since... The low level idea you propose (sharing code down to error paths) does not make sense in this particular case, for reasons that are rather non-obvious. In particular, we need to be careful to avoid sharing some code with "init". The "init" subcommand is deprecated as of ba2f3f58ac ("git-sparse-checkout.txt: update to document init/set/reapply changes", 2021-12-14). I initially wanted to remove the separate "init" codepath, and just forward "git sparse-checkout init" calls over to the "git sparse-checkout set" codepath (the only difference being that "init" always comes with an empty set of directories/patterns). However, "init" had some problematic behavior that I didn't want to copy to "set". I also didn't want to kill that behavior in "init" for backwards compatibility reasons. And, this xfopen call was tied up in that tangle, which means that it will definitely remain separate, and thus needs to be fixed in isolation. > But I've never *actually* used this command, so maybe I'm just missing > something obvious... > > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 3189d3da965..6b56d9d177f 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' ' > ' > > test_expect_success 'git sparse-checkout init in empty repo' ' > - test_when_finished rm -rf empty-repo blank-template && > + test_when_finished rm -rf empty-repo && This hunk looks like a good fixup to Jonathan's patch. > git init --template= empty-repo && > git -C empty-repo sparse-checkout init > ' > > +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' ' > + test_when_finished "rm -rf empty" && > + git init --template= empty && > + git -C empty sparse-checkout init && > + rm -rf empty/.git/info && > + > + cat >expect <<-\EOF && > + fatal: unable to load existing sparse-checkout patterns > + EOF > + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual && > + test_cmp expect actual > +' > + > +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' ' > + test_when_finished "rm -rf empty" && > + git init --template= empty && > + git -C empty sparse-checkout init && > + rm -rf empty/.git/info && > + > + cat >expect <<-\EOF && > + warning: this worktree is not sparse (sparse-checkout file may not exist) > + EOF > + git -C empty sparse-checkout list 2>actual && > + test_cmp expect actual > +' > + So...you're trying to test what happens when a user intentionally bricks their repository? (Note that `sparse-checkout init` sets core.sparseCheckout=true, as explicitly documented. core.sparseCheckout=true instructs git to pay attention to $GIT_DIR/info/sparse-checkout for every unpack_trees() call that updates the working tree, which basically means nearly any significant Git operation involving a worktree update now needs that file in order to function. So, your commands told Git that this directory is mandatory, and then you nuked the directory.) Now, if you could find a testcase based on `git worktree add ...` (which doesn't create an "info" directory) and then triggers problems somehow without the intentional bricking, then what you'd have would be more in line with what Jonathan is addressing here, but as it stands it's hard to even call your testcases related. There may be some merit to testing deliberately broken repositories, but I'm just not sure if that's what you really intended and were suggesting. Was it? Or am I just missing something here? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-22 5:21 ` Elijah Newren @ 2022-01-22 12:06 ` Ævar Arnfjörð Bjarmason 2022-01-22 17:29 ` Elijah Newren 0 siblings, 1 reply; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-22 12:06 UTC (permalink / raw) To: Elijah Newren; +Cc: Jonathan Tan, Git Mailing List, Junio C Hamano, jabolopes On Fri, Jan 21 2022, Elijah Newren wrote: > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> On Fri, Jan 21 2022, Jonathan Tan wrote: >> > ... >> This partial application of the fix-up I suggested in >> https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/ >> leaves the now-unused "blank-template" > > Good point; no need to remove something that isn't being created anymore. > >> >> > - added attribution to Jose Lopes for finding and making the first >> > draft of this patch (after confirming with them) >> > >> > Ævar mentioned "git sparse-checkout add" but I think that that is a >> > different problem - in the "git sparse-checkout init" case, we could get >> > into this case with a template that does not have .git/info, but in the >> > "git sparse-checkout add" case, the user would have had to explicitly >> > remove the info directory. So I'll limit this patch to the "init" case, >> > for now. > ... >> >> I agree that it's a slightly different problem, but I was just >> advocating for us testing what happened in these cases. >> >> The below fix-up does that. > > Different problem...addressed with a "fix-up"? Why would we squash > extra testing of a different problem into the same commit? I think > it'd at least deserve its own commit message. Sure, or split up, or with an amended commit message etc. >> I think we should use warning_errno() there >> instead of some specutalite "file may not exist", but with/without this >> patch these tests show that only the "init" case was broken. >> >> As a more general issue I don't understand why "add" and "init" need to >> be conceptually different operations. If what defines a sparse checkout >> is just that it has that file and the 2 default patterns, which unless >> I'm missing something is the case. >> >> Why isn't "add" merely an "init" >> that'll optimistically add whatever pattern you asked for, in addition >> to doing an "init" if you didn't already? >> >> Then "add" and "init" will share the same error recovery behavior, and >> you won't needlessly have to run "init/add x" just to start using >> sparse-checkout with a pattern of "x". > > The high level idea you propose (" 'add' can initialize sparsity state > if not currently in a sparse checkout") could make sense. It isn't > just a straightforward switchover with the various other config > settings and such, and would also necessitate adding additional > command line flags to the 'add' subcommand, but it could be done. > Didn't occur to me before; I'm not sure if such a change in UI would > be better or worse. I'm inclined to leave it as it is, though, > especially since... > > The low level idea you propose (sharing code down to error paths) does > not make sense in this particular case, for reasons that are rather > non-obvious. In particular, we need to be careful to avoid sharing > some code with "init". The "init" subcommand is deprecated as of > ba2f3f58ac ("git-sparse-checkout.txt: update to document > init/set/reapply changes", 2021-12-14). I initially wanted to remove > the separate "init" codepath, and just forward "git sparse-checkout > init" calls over to the "git sparse-checkout set" codepath (the only > difference being that "init" always comes with an empty set of > directories/patterns). However, "init" had some problematic behavior > that I didn't want to copy to "set". I also didn't want to kill that > behavior in "init" for backwards compatibility reasons. And, this > xfopen call was tied up in that tangle, which means that it will > definitely remain separate, and thus needs to be fixed in isolation. .... >> But I've never *actually* used this command, so maybe I'm just missing >> something obvious... >> >> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh >> index 3189d3da965..6b56d9d177f 100755 >> --- a/t/t1091-sparse-checkout-builtin.sh >> +++ b/t/t1091-sparse-checkout-builtin.sh >> @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' ' >> ' >> >> test_expect_success 'git sparse-checkout init in empty repo' ' >> - test_when_finished rm -rf empty-repo blank-template && >> + test_when_finished rm -rf empty-repo && > > This hunk looks like a good fixup to Jonathan's patch. > >> git init --template= empty-repo && >> git -C empty-repo sparse-checkout init >> ' >> >> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' ' >> + test_when_finished "rm -rf empty" && >> + git init --template= empty && >> + git -C empty sparse-checkout init && >> + rm -rf empty/.git/info && >> + >> + cat >expect <<-\EOF && >> + fatal: unable to load existing sparse-checkout patterns >> + EOF >> + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual && >> + test_cmp expect actual >> +' >> + >> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' ' >> + test_when_finished "rm -rf empty" && >> + git init --template= empty && >> + git -C empty sparse-checkout init && >> + rm -rf empty/.git/info && >> + >> + cat >expect <<-\EOF && >> + warning: this worktree is not sparse (sparse-checkout file may not exist) >> + EOF >> + git -C empty sparse-checkout list 2>actual && >> + test_cmp expect actual >> +' >> + > > So...you're trying to test what happens when a user intentionally > bricks their repository? I'm just saying that it's cheap to add a regression test for this missing bit of related coverage, so why not add it? We need to deal with the real world, a repo might be in all sorts of odd states, including because of a user mistake. > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as > explicitly documented. core.sparseCheckout=true instructs git to pay > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees() > call that updates the working tree, which basically means nearly any > significant Git operation involving a worktree update now needs that > file in order to function. So, your commands told Git that this > directory is mandatory, and then you nuked the directory.) *nod*. But in that case shouldn't the errors say that you've configured core.sparseCheckout=true but you're missing XYZ file? > Now, if you could find a testcase based on `git worktree add ...` > (which doesn't create an "info" directory) and then triggers problems > somehow without the intentional bricking, then what you'd have would > be more in line with what Jonathan is addressing here, but as it > stands it's hard to even call your testcases related. There may be > some merit to testing deliberately broken repositories, but I'm just > not sure if that's what you really intended and were suggesting. Was > it? Or am I just missing something here? Doesn't the worktree case just use the "main" info/*, e.g. for info/excludes (didn't have time to test it now). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-22 12:06 ` Ævar Arnfjörð Bjarmason @ 2022-01-22 17:29 ` Elijah Newren 2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 14+ messages in thread From: Elijah Newren @ 2022-01-22 17:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jonathan Tan, Git Mailing List, Junio C Hamano, jabolopes On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Fri, Jan 21 2022, Elijah Newren wrote: > > > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> > >> On Fri, Jan 21 2022, Jonathan Tan wrote: > >> ... > >> > Ævar mentioned "git sparse-checkout add" but I think that that is a > >> > different problem - in the "git sparse-checkout init" case, we could get > >> > into this case with a template that does not have .git/info, but in the > >> > "git sparse-checkout add" case, the user would have had to explicitly > >> > remove the info directory. So I'll limit this patch to the "init" case, > >> > for now. > > ... > >> > >> I agree that it's a slightly different problem, but I was just > >> advocating for us testing what happened in these cases. > >> > >> The below fix-up does that. > > > > Different problem...addressed with a "fix-up"? Why would we squash > > extra testing of a different problem into the same commit? I think > > it'd at least deserve its own commit message. > > Sure, or split up, or with an amended commit message etc. I think it's a totally different kind of thing and wouldn't belong in the same commit even with an amended commit message. I'm curious why we're so far apart on this and whether I'm missing something. ... > >> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' ' > >> + test_when_finished "rm -rf empty" && > >> + git init --template= empty && > >> + git -C empty sparse-checkout init && > >> + rm -rf empty/.git/info && > >> + > >> + cat >expect <<-\EOF && > >> + fatal: unable to load existing sparse-checkout patterns > >> + EOF > >> + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual && > >> + test_cmp expect actual > >> +' > >> + > >> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' ' > >> + test_when_finished "rm -rf empty" && > >> + git init --template= empty && > >> + git -C empty sparse-checkout init && > >> + rm -rf empty/.git/info && > >> + > >> + cat >expect <<-\EOF && > >> + warning: this worktree is not sparse (sparse-checkout file may not exist) > >> + EOF > >> + git -C empty sparse-checkout list 2>actual && > >> + test_cmp expect actual > >> +' > >> + > > > > So...you're trying to test what happens when a user intentionally > > bricks their repository? > > I'm just saying that it's cheap to add a regression test for this > missing bit of related coverage, so why not add it? So, this is a slightly different issue than I was getting at before, but this feels slightly obstructionist to me. Now, suggesting related improvements and ideas sounds totally fine; we should point those out -- once. But pushing it again as though it needs to be addressed as part of the submission just doesn't feel right. Jonathan's patch fixes a real problem and feels complete to me. There are always additional things that could also be fixed or addressed for any patch or series. Expecting folks submitting a series to address every "next step improvement along these lines" that any reviewer can think of seems unfriendly, especially as it has a snowball effect. Granted, if there's a bug with the patch, or it doesn't fully solve the stated problem, then it's a different situation, but I don't think that's the case here. (Well, modulo the leftover removing of "blank-template" which is a real issue you identified with the patch.) And I think I'd say the same thing even if I saw your tests as being much more closely related to what Jonathan was checking. That's my $0.02 on "why not?". The story totally changes if you want to submit these tests separate from Jonathan's series. If that's the scenario, then I fully agree with you on "it's cheap to add more test coverage so why not include it?" Anyway...back to my curiosity about why we're so far apart on the relatedness of your tests... > We need to deal with the real world, a repo might be in all sorts of odd > states, including because of a user mistake. So, Jonathan was fixing behavior seen when the user hadn't even made a mistake. Opening up to all possible user mistakes seems to widening scope pretty dramatically and feels like a different kind of thing to me. But even that scope doesn't seem to include the tests you've proposed, at least not to me. Under what circumstances would your tests model a user mistake? A user mistake to me looks like one of the following: * "I hit Ctrl+C while a `git switch` or `git sparse-checkout set ...` just happened to be furiously writing files" * "I ran `git sparse-checkout (add|list|reapply)` without first running `git sparse-checkout (init|set)` as per the docs" Your tests look roughly the same class as the following kinds of things: * echo garbage >.git/refs/heads/master * rm .git/objects/${random_loose_object_or_pack} I know users can attempt surgery on $GIT_DIR, and that perhaps curious ones will do that to see how things break in order to help them understand how things work, but that seems to me to be a different realm. Note that I'm not saying we shouldn't test what happens when the repo is intentionally corrupted; there's probably merit in that. I'm just curious why we're so far apart on this. You view your tests as a "slightly different problem" and feel these tests could be included in Jonathan's commit with an "amended commit message". I think they're not only different classes of problems, but separated by a third class ("user mistake") between the two types of problems. If Jonathan had included your tests in his commit, I think I'd ask that they at _least_ be split into different commits. Am I missing the boat entirely somewhere? > > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as > > explicitly documented. core.sparseCheckout=true instructs git to pay > > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees() > > call that updates the working tree, which basically means nearly any > > significant Git operation involving a worktree update now needs that > > file in order to function. So, your commands told Git that this > > directory is mandatory, and then you nuked the directory.) > > *nod*. But in that case shouldn't the errors say that you've configured > core.sparseCheckout=true but you're missing XYZ file? Yeah, it probably should. I just did a little more testing and it looks like commands like "switch" don't even error out; they just treat the missing $GIT_DIR/info/sparse-checkout files the same as all files being included. Weird. It seems to come from dir.c:add_patterns(), which appears to have perhaps gotten that way due to assuming the code was exclusively about .gitignore files rather than the sparse-checkout file. I think this may be yet another example of why it was a mistake to use gitignore-style patterns for the sparse-checkout feature, though I'm more than a decade too late with my complaints. Ugh. But yeah, you're right to suggest we should have tests for this, and perhaps some fixes as well...in a separate submission from this one. ;-) > > Now, if you could find a testcase based on `git worktree add ...` > > (which doesn't create an "info" directory) and then triggers problems > > somehow without the intentional bricking, then what you'd have would > > be more in line with what Jonathan is addressing here, but as it > > stands it's hard to even call your testcases related. There may be > > some merit to testing deliberately broken repositories, but I'm just > > not sure if that's what you really intended and were suggesting. Was > > it? Or am I just missing something here? > > Doesn't the worktree case just use the "main" info/*, e.g. for > info/excludes (didn't have time to test it now). Maybe for info/excludes, I don't know. But it certainly won't use the main info/sparse-checkout; that'd be _horribly_ broken. Each worktree should be allowed to have its own sparsity rules. And having multiple worktrees where one worktree is not sparse, while others are sparse but with different sparsity patterns from each other (often only slightly different, but sometimes completely unrelated) is actually a common usecase. We do that a lot at $DAYJOB. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-22 17:29 ` Elijah Newren @ 2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason 2022-01-24 18:25 ` Jonathan Tan 2022-01-25 5:37 ` Elijah Newren 0 siblings, 2 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-01-24 11:17 UTC (permalink / raw) To: Elijah Newren; +Cc: Jonathan Tan, Git Mailing List, Junio C Hamano, jabolopes On Sat, Jan 22 2022, Elijah Newren wrote: > On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> On Fri, Jan 21 2022, Elijah Newren wrote: >> >> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason >> > <avarab@gmail.com> wrote: >> >> >> >> On Fri, Jan 21 2022, Jonathan Tan wrote: >> >> > ... >> >> > Ævar mentioned "git sparse-checkout add" but I think that that is a >> >> > different problem - in the "git sparse-checkout init" case, we could get >> >> > into this case with a template that does not have .git/info, but in the >> >> > "git sparse-checkout add" case, the user would have had to explicitly >> >> > remove the info directory. So I'll limit this patch to the "init" case, >> >> > for now. >> > ... >> >> >> >> I agree that it's a slightly different problem, but I was just >> >> advocating for us testing what happened in these cases. >> >> >> >> The below fix-up does that. >> > >> > Different problem...addressed with a "fix-up"? Why would we squash >> > extra testing of a different problem into the same commit? I think >> > it'd at least deserve its own commit message. >> >> Sure, or split up, or with an amended commit message etc. > > I think it's a totally different kind of thing and wouldn't belong in > the same commit even with an amended commit message. I'm curious why > we're so far apart on this and whether I'm missing something. I think I'm biased by my initial look at this problem[1], which was to look at the various "sparse_filename" callers and their non-coverage. In my mind fixing & testing for that general problem would make for a nice atomic change. > ... >> >> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' ' >> >> + test_when_finished "rm -rf empty" && >> >> + git init --template= empty && >> >> + git -C empty sparse-checkout init && >> >> + rm -rf empty/.git/info && >> >> + >> >> + cat >expect <<-\EOF && >> >> + fatal: unable to load existing sparse-checkout patterns >> >> + EOF >> >> + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual && >> >> + test_cmp expect actual >> >> +' >> >> + >> >> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' ' >> >> + test_when_finished "rm -rf empty" && >> >> + git init --template= empty && >> >> + git -C empty sparse-checkout init && >> >> + rm -rf empty/.git/info && >> >> + >> >> + cat >expect <<-\EOF && >> >> + warning: this worktree is not sparse (sparse-checkout file may not exist) >> >> + EOF >> >> + git -C empty sparse-checkout list 2>actual && >> >> + test_cmp expect actual >> >> +' >> >> + >> > >> > So...you're trying to test what happens when a user intentionally >> > bricks their repository? >> >> I'm just saying that it's cheap to add a regression test for this >> missing bit of related coverage, so why not add it? > > So, this is a slightly different issue than I was getting at before, > but this feels slightly obstructionist to me. Now, suggesting related > improvements and ideas sounds totally fine; we should point those out > -- once. But pushing it again as though it needs to be addressed as > part of the submission just doesn't feel right. Jonathan's patch > fixes a real problem and feels complete to me. There are always > additional things that could also be fixed or addressed for any patch > or series. Expecting folks submitting a series to address every "next > step improvement along these lines" that any reviewer can think of > seems unfriendly, especially as it has a snowball effect. > > Granted, if there's a bug with the patch, or it doesn't fully solve > the stated problem, then it's a different situation, but I don't think > that's the case here. (Well, modulo the leftover removing of > "blank-template" which is a real issue you identified with the patch.) The first thing I said in this thread is "Thanks. This fix looks good to me.". I'd be happy to have just this fix in. This patch resolves a blocked of an earlier series of mine. The rest of the feedback here (aside from the trivial "rm -rf" fix) was an attempt to bridge the gap between this & my earlier look in [1]. > And I think I'd say the same thing even if I saw your tests as being > much more closely related to what Jonathan was checking. > > That's my $0.02 on "why not?". The story totally changes if you want > to submit these tests separate from Jonathan's series. If that's the > scenario, then I fully agree with you on "it's cheap to add more test > coverage so why not include it?" Sure, or maybe he'd be interested, or not. I'd rather try to suggest some small proposed changes than submit a patch of my own as an initial approach. > Anyway...back to my curiosity about why we're so far apart on the > relatedness of your tests... > >> We need to deal with the real world, a repo might be in all sorts of odd >> states, including because of a user mistake. > > So, Jonathan was fixing behavior seen when the user hadn't even made a > mistake. Opening up to all possible user mistakes seems to widening > scope pretty dramatically and feels like a different kind of thing to > me. But even that scope doesn't seem to include the tests you've > proposed, at least not to me. Under what circumstances would your > tests model a user mistake? A user mistake to me looks like one of > the following: > > * "I hit Ctrl+C while a `git switch` or `git sparse-checkout set > ...` just happened to be furiously writing files" > * "I ran `git sparse-checkout (add|list|reapply)` without first > running `git sparse-checkout (init|set)` as per the docs" > > Your tests look roughly the same class as the following kinds of things: > > * echo garbage >.git/refs/heads/master > * rm .git/objects/${random_loose_object_or_pack} > > I know users can attempt surgery on $GIT_DIR, and that perhaps curious > ones will do that to see how things break in order to help them > understand how things work, but that seems to me to be a different > realm. > > Note that I'm not saying we shouldn't test what happens when the repo > is intentionally corrupted; there's probably merit in that. I'm just > curious why we're so far apart on this. You view your tests as a > "slightly different problem" and feel these tests could be included in > Jonathan's commit with an "amended commit message". I think they're > not only different classes of problems, but separated by a third class > ("user mistake") between the two types of problems. If Jonathan had > included your tests in his commit, I think I'd ask that they at > _least_ be split into different commits. > > Am I missing the boat entirely somewhere? I was using "fix-up" rather loosely upthread. FWIW I never meant that this needed to be in the same commit, but was going for "this solves some test blind spots in this adjacent area in nearby functions", or something like that. I'm all for splitting extra tests into another commit or whatever, however that eventually lands in tree. [...] >> > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as >> > explicitly documented. core.sparseCheckout=true instructs git to pay >> > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees() >> > call that updates the working tree, which basically means nearly any >> > significant Git operation involving a worktree update now needs that >> > file in order to function. So, your commands told Git that this >> > directory is mandatory, and then you nuked the directory.) >> >> *nod*. But in that case shouldn't the errors say that you've configured >> core.sparseCheckout=true but you're missing XYZ file? > > Yeah, it probably should. > > I just did a little more testing and it looks like commands like > "switch" don't even error out; they just treat the missing > $GIT_DIR/info/sparse-checkout files the same as all files being > included. Weird. It seems to come from dir.c:add_patterns(), which > appears to have perhaps gotten that way due to assuming the code was > exclusively about .gitignore files rather than the sparse-checkout > file. I think this may be yet another example of why it was a mistake > to use gitignore-style patterns for the sparse-checkout feature, > though I'm more than a decade too late with my complaints. > > Ugh. > > But yeah, you're right to suggest we should have tests for this, and > perhaps some fixes as well...in a separate submission from this one. > ;-) FWIW I don't think it's all that important to focus too much on how users would get into this scenario. Sure, for the "init" case it's a thing that's broken with --template=, so that's more urgent. But for the rest we're already carrying code for those edge cases (errno handling and all), we just don't test for it. So just adding tests seems prudent. >> > Now, if you could find a testcase based on `git worktree add ...` >> > (which doesn't create an "info" directory) and then triggers problems >> > somehow without the intentional bricking, then what you'd have would >> > be more in line with what Jonathan is addressing here, but as it >> > stands it's hard to even call your testcases related. There may be >> > some merit to testing deliberately broken repositories, but I'm just >> > not sure if that's what you really intended and were suggesting. Was >> > it? Or am I just missing something here? >> >> Doesn't the worktree case just use the "main" info/*, e.g. for >> info/excludes (didn't have time to test it now). > > Maybe for info/excludes, I don't know. But it certainly won't use the > main info/sparse-checkout; that'd be _horribly_ broken. Each worktree > should be allowed to have its own sparsity rules. And having multiple > worktrees where one worktree is not sparse, while others are sparse > but with different sparsity patterns from each other (often only > slightly different, but sometimes completely unrelated) is actually a > common usecase. We do that a lot at $DAYJOB. 1. https://lore.kernel.org/git/211220.86zgovt9bi.gmgdl@evledraar.gmail.com/ For what it's worth I rebased my earlier --no-template series locally on this, and tried removing the creation of the "info" dir under --no-template. The below diff shows the fixes that were needed for info/sparse-checkout as a result. Now I'm *not* saying this needs to be in this series or whatever, it's just an FYI sanity check, i.e. this didn't reveal any remaining cases where "git sparse-checkout" in whatever mode broke unexpectedly without a .git/info. These cases are just ones where we're manually setting up the sparse checkout. Of course we may have missing coverage etc., but I think this is one more datapoint in favor of this proposed change being a good one. diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh index 24092c09a95..822d753bd95 100755 --- a/t/t1011-read-tree-sparse-checkout.sh +++ b/t/t1011-read-tree-sparse-checkout.sh @@ -53,6 +53,7 @@ test_expect_success 'read-tree without .git/info/sparse-checkout' ' ' test_expect_success 'read-tree with .git/info/sparse-checkout but disabled' ' + mkdir .git/info && echo >.git/info/sparse-checkout && read_tree_u_must_succeed -m -u HEAD && git ls-files -t >result && diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh index 3deb4901874..669b114db03 100755 --- a/t/t1090-sparse-checkout-scope.sh +++ b/t/t1090-sparse-checkout-scope.sh @@ -25,6 +25,7 @@ test_expect_success 'create feature branch' ' test_expect_success 'perform sparse checkout of main' ' git config --local --bool core.sparsecheckout true && + mkdir .git/info && echo "!/*" >.git/info/sparse-checkout && echo "/a" >>.git/info/sparse-checkout && echo "/c" >>.git/info/sparse-checkout && @@ -66,6 +67,7 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs git -C server commit -m message && test_config -C client core.sparsecheckout 1 && + mkdir client/.git/info && echo "!/*" >client/.git/info/sparse-checkout && echo "/a" >>client/.git/info/sparse-checkout && git -C client fetch --filter=blob:none origin && diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh index 3e93506c045..e1d9d0d3c7a 100755 --- a/t/t2018-checkout-branch.sh +++ b/t/t2018-checkout-branch.sh @@ -249,6 +249,7 @@ test_expect_success 'checkout -b to a new branch preserves mergeable changes des test_commit file2 && echo stuff >>file1 && + mkdir .git/info && echo file2 >.git/info/sparse-checkout && git config core.sparseCheckout true && diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 979e843c65a..dbd37147c63 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -558,6 +558,7 @@ test_expect_success 'cherry-pick preserves sparse-checkout' ' echo \"/*\" >.git/info/sparse-checkout git read-tree --reset -u HEAD rm .git/info/sparse-checkout" && + mkdir .git/info && echo /unrelated >.git/info/sparse-checkout && git read-tree --reset -u HEAD && test_must_fail git cherry-pick -Xours picked>actual && diff --git a/t/t6435-merge-sparse.sh b/t/t6435-merge-sparse.sh index 74562e12356..6fd0bdf38ac 100755 --- a/t/t6435-merge-sparse.sh +++ b/t/t6435-merge-sparse.sh @@ -26,6 +26,7 @@ test_expect_success 'setup' ' git rm modify_delete && test_commit_this ours && git config core.sparseCheckout true && + mkdir .git/info && echo "/checked-out" >.git/info/sparse-checkout && git reset --hard && test_must_fail git merge theirs diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh index f87e524d6d4..5cc0b7e700f 100755 --- a/t/t7418-submodule-sparse-gitmodules.sh +++ b/t/t7418-submodule-sparse-gitmodules.sh @@ -33,6 +33,7 @@ test_expect_success 'sparse checkout setup which hides .gitmodules' ' ) && git clone upstream super && (cd super && + mkdir .git/info && cat >.git/info/sparse-checkout <<-\EOF && /* !/.gitmodules ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason @ 2022-01-24 18:25 ` Jonathan Tan 2022-01-25 5:37 ` Elijah Newren 1 sibling, 0 replies; 14+ messages in thread From: Jonathan Tan @ 2022-01-24 18:25 UTC (permalink / raw) To: avarab; +Cc: newren, jonathantanmy, git, gitster, jabolopes Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The first thing I said in this thread is "Thanks. This fix looks good to > me.". I'd be happy to have just this fix in. This patch resolves a > blocked of an earlier series of mine. > > The rest of the feedback here (aside from the trivial "rm -rf" fix) was > an attempt to bridge the gap between this & my earlier look in [1]. > > > And I think I'd say the same thing even if I saw your tests as being > > much more closely related to what Jonathan was checking. > > > > That's my $0.02 on "why not?". The story totally changes if you want > > to submit these tests separate from Jonathan's series. If that's the > > scenario, then I fully agree with you on "it's cheap to add more test > > coverage so why not include it?" > > Sure, or maybe he'd be interested, or not. I'd rather try to suggest > some small proposed changes than submit a patch of my own as an initial > approach. If my patch is OK going in on its own, I'd rather do that, and leave additional changes to other patch sets. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason 2022-01-24 18:25 ` Jonathan Tan @ 2022-01-25 5:37 ` Elijah Newren 1 sibling, 0 replies; 14+ messages in thread From: Elijah Newren @ 2022-01-25 5:37 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Jonathan Tan, Git Mailing List, Junio C Hamano, Jose Lopes On Mon, Jan 24, 2022 at 3:31 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Sat, Jan 22 2022, Elijah Newren wrote: > > > On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> > >> On Fri, Jan 21 2022, Elijah Newren wrote: > >> > >> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason > >> > <avarab@gmail.com> wrote: > >> >> ... > > I think it's a totally different kind of thing and wouldn't belong in > > the same commit even with an amended commit message. I'm curious why > > we're so far apart on this and whether I'm missing something. > > I think I'm biased by my initial look at this problem[1], which was to look > at the various "sparse_filename" callers and their non-coverage. In my > mind fixing & testing for that general problem would make for a nice > atomic change. Ah, ok, that link and your other comments below help; I think I'm starting to see now. Sorry for being slow. I was focusing (pigeonholing?) on the likelihood and feasibility of a user entering certain states, you were doing whatever was necessary to force the code into certain states and then testing them. The forcing into certain states was tripping me up. (Perhaps if we had a unit test framework instead of only a regression test framework, then you could have suggested multiple unit tests and I wouldn't have been so confused.) Thanks for explaining. .... > >> I'm just saying that it's cheap to add a regression test for this > >> missing bit of related coverage, so why not add it? > > > > So, this is a slightly different issue than I was getting at before, > > but this feels slightly obstructionist to me. Now, suggesting related > > improvements and ideas sounds totally fine; we should point those out > > -- once. But pushing it again as though it needs to be addressed as > > part of the submission just doesn't feel right. Jonathan's patch > > fixes a real problem and feels complete to me. There are always > > additional things that could also be fixed or addressed for any patch > > or series. Expecting folks submitting a series to address every "next > > step improvement along these lines" that any reviewer can think of > > seems unfriendly, especially as it has a snowball effect. > > > > Granted, if there's a bug with the patch, or it doesn't fully solve > > the stated problem, then it's a different situation, but I don't think > > that's the case here. (Well, modulo the leftover removing of > > "blank-template" which is a real issue you identified with the patch.) > > The first thing I said in this thread is "Thanks. This fix looks good to > me.". I'd be happy to have just this fix in. This patch resolves a > blocked of an earlier series of mine. > > The rest of the feedback here (aside from the trivial "rm -rf" fix) was > an attempt to bridge the gap between this & my earlier look in [1]. It helps to know your intent. The "I'd be happy to have just this fix in" doesn't appear in your emails until here; to me, you communicated the opposite by not only bringing up the additional changes but also following up to the next email by also including a patch when you saw they weren't included. Anyway, I think we're good here; we both agree Jonathan's patch doesn't need the additions, and you've patiently explained to me what I was missing about your view on the tests. Sorry for being slow on that. One other side point, though, since it came up in this thread... > >> > (Note that `sparse-checkout init` sets core.sparseCheckout=true, as > >> > explicitly documented. core.sparseCheckout=true instructs git to pay > >> > attention to $GIT_DIR/info/sparse-checkout for every unpack_trees() > >> > call that updates the working tree, which basically means nearly any > >> > significant Git operation involving a worktree update now needs that > >> > file in order to function. So, your commands told Git that this > >> > directory is mandatory, and then you nuked the directory.) > >> > >> *nod*. But in that case shouldn't the errors say that you've configured > >> core.sparseCheckout=true but you're missing XYZ file? > > > > Yeah, it probably should. > > > > I just did a little more testing and it looks like commands like > > "switch" don't even error out; they just treat the missing > > $GIT_DIR/info/sparse-checkout files the same as all files being > > included. Weird. It seems to come from dir.c:add_patterns(), which > > appears to have perhaps gotten that way due to assuming the code was > > exclusively about .gitignore files rather than the sparse-checkout > > file. ... > FWIW I don't think it's all that important to focus too much on how > users would get into this scenario. Sure, for the "init" case it's a > thing that's broken with --template=, so that's more urgent. Just to check, "this scenario" means core.sparseCheckout=true with $GIT_DIR/info missing (and thus $GIT_DIR/info/sparse-checkout missing), right? > But for the rest we're already carrying code for those edge cases (errno > handling and all), we just don't test for it. > > So just adding tests seems prudent. Okay, but if we're ignoring how users get into this scenario, then there are more affected code paths than just the sparse-checkout subcommand. And it appears that at least some of those do not behave well (they do weird-ish things and pretend success instead of showing a reasonable error message, or even any error message). So there's probably some work involved to identify the relevant subcommands or codepaths, so that we even know what to add tests for. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan 2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason @ 2022-01-22 2:33 ` Elijah Newren 2022-01-24 18:22 ` Jonathan Tan 1 sibling, 1 reply; 14+ messages in thread From: Elijah Newren @ 2022-01-22 2:33 UTC (permalink / raw) To: Jonathan Tan Cc: Git Mailing List, Junio C Hamano, Ævar Arnfjörð, jabolopes On Fri, Jan 21, 2022 at 6:05 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > When creating the sparse-checkout file, Git does not create the leading > directory, "$GIT_DIR/info", if it does not exist. This causes problems > if the repository does not have that directory. Therefore, ensure that > the leading directory is created. > > This is the only "open" in builtin/sparse-checkout.c that does not have > a leading directory check. (The other one in write_patterns_and_update() > does.) > > Note that the test needs to explicitly specify a template when running > "git init" because the default template used in the tests has the > "info/" directory included. If wanted, you could avoid that by using `git worktree add ...`; git-worktree will create a $GIT_DIR that does not contain an info subdirectory. (No need to resubmit or anything, mostly I'm just mentioning another way folks might have triggered this issue.) > Helped-by: Jose Lopes <jabolopes@google.com> > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Changes from v1: > - made test simpler > - added attribution to Jose Lopes for finding and making the first > draft of this patch (after confirming with them) > > Ævar mentioned "git sparse-checkout add" but I think that that is a > different problem - in the "git sparse-checkout init" case, we could get > into this case with a template that does not have .git/info, but in the > "git sparse-checkout add" case, the user would have had to explicitly > remove the info directory. So I'll limit this patch to the "init" case, > for now. > --- > builtin/sparse-checkout.c | 3 +++ > t/t1091-sparse-checkout-builtin.sh | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index 679c107036..2b0e1db2d2 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -471,6 +471,9 @@ static int sparse_checkout_init(int argc, const char **argv) > FILE *fp; > > /* assume we are in a fresh repo, but update the sparse-checkout file */ > + if (safe_create_leading_directories(sparse_filename)) > + die(_("unable to create leading directories of %s"), > + sparse_filename); > fp = xfopen(sparse_filename, "w"); > if (!fp) > die(_("failed to open '%s'"), sparse_filename); > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 42776984fe..3189d3da96 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -79,6 +79,12 @@ test_expect_success 'git sparse-checkout init' ' > check_files repo a > ' > > +test_expect_success 'git sparse-checkout init in empty repo' ' > + test_when_finished rm -rf empty-repo blank-template && > + git init --template= empty-repo && > + git -C empty-repo sparse-checkout init > +' > + > test_expect_success 'git sparse-checkout list after init' ' > git -C repo sparse-checkout list >actual && > cat >expect <<-\EOF && > -- > 2.35.0.rc0.227.g00780c9af4-goog Patch looks good to me: Reviewed-by: Elijah Newren <newren@gmail.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sparse-checkout: create leading directory 2022-01-22 2:33 ` Elijah Newren @ 2022-01-24 18:22 ` Jonathan Tan 0 siblings, 0 replies; 14+ messages in thread From: Jonathan Tan @ 2022-01-24 18:22 UTC (permalink / raw) To: newren; +Cc: jonathantanmy, git, gitster, avarab, jabolopes Elijah Newren <newren@gmail.com> writes: > On Fri, Jan 21, 2022 at 6:05 PM Jonathan Tan <jonathantanmy@google.com> wro= > te: > > > > When creating the sparse-checkout file, Git does not create the leading > > directory, "$GIT_DIR/info", if it does not exist. This causes problems > > if the repository does not have that directory. Therefore, ensure that > > the leading directory is created. > > > > This is the only "open" in builtin/sparse-checkout.c that does not have > > a leading directory check. (The other one in write_patterns_and_update() > > does.) > > > > Note that the test needs to explicitly specify a template when running > > "git init" because the default template used in the tests has the > > "info/" directory included. > > If wanted, you could avoid that by using `git worktree add ...`; > git-worktree will create a $GIT_DIR that does not contain an info > subdirectory. > > (No need to resubmit or anything, mostly I'm just mentioning another > way folks might have triggered this issue.) Ah, thanks for the information. > Patch looks good to me: > > Reviewed-by: Elijah Newren <newren@gmail.com> Thanks for taking a look! ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-01-25 7:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-20 18:55 [PATCH] sparse-checkout: create leading directory Jonathan Tan 2022-01-20 20:26 ` Junio C Hamano 2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason 2022-01-22 7:58 ` SZEDER Gábor 2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan 2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason 2022-01-22 5:21 ` Elijah Newren 2022-01-22 12:06 ` Ævar Arnfjörð Bjarmason 2022-01-22 17:29 ` Elijah Newren 2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason 2022-01-24 18:25 ` Jonathan Tan 2022-01-25 5:37 ` Elijah Newren 2022-01-22 2:33 ` Elijah Newren 2022-01-24 18:22 ` Jonathan Tan
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).