On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/ > 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?
