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

[Elijah Newren <newren@gmail.com>]

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?