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

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

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