On Fri, Jan 21 2022, Elijah Newren wrote: > 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. 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).
