[PATCH] sparse-checkout: create leading directory

[PATCH] sparse-checkout: create leading directory

[Jonathan Tan]

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

Re: [PATCH] sparse-checkout: create leading directory

[Junio C Hamano]

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.

Re: [PATCH] sparse-checkout: create leading directory

[Ævar Arnfjörð Bjarmason]

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
 '
 

[PATCH v2] sparse-checkout: create leading directory

[Jonathan Tan]

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

Re: [PATCH v2] sparse-checkout: create leading directory

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH v2] sparse-checkout: create leading directory

[Elijah Newren]

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>

Re: [PATCH v2] sparse-checkout: create leading directory

[Elijah Newren]

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?

Re: [PATCH] sparse-checkout: create leading directory

[SZEDER Gábor]

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

Re: [PATCH v2] sparse-checkout: create leading directory

[Ævar Arnfjörð Bjarmason]

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).

Re: [PATCH v2] sparse-checkout: create leading directory

[Elijah Newren]

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.

Re: [PATCH v2] sparse-checkout: create leading directory

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH v2] sparse-checkout: create leading directory

[Jonathan Tan]

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!

Re: [PATCH v2] sparse-checkout: create leading directory

[Jonathan Tan]

Æ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.

Re: [PATCH v2] sparse-checkout: create leading directory

[Elijah Newren]

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.