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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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