On Mon, Jan 24, 2022 at 3:31 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > On Sat, Jan 22 2022, Elijah Newren wrote: > > > On Sat, Jan 22, 2022 at 4:08 AM Ævar Arnfjörð Bjarmason > > <avarab@xxxxxxxxx> wrote: > >> > >> On Fri, Jan 21 2022, Elijah Newren wrote: > >> > >> > On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason > >> > <avarab@xxxxxxxxx> wrote: > >> >> ... > > I think it's a totally different kind of thing and wouldn't belong in > > the same commit even with an amended commit message. I'm curious why > > we're so far apart on this and whether I'm missing something. > > I think I'm biased by my initial look at this problem[1], which was to look > at the various "sparse_filename" callers and their non-coverage. In my > mind fixing & testing for that general problem would make for a nice > atomic change. Ah, ok, that link and your other comments below help; I think I'm starting to see now. Sorry for being slow. I was focusing (pigeonholing?) on the likelihood and feasibility of a user entering certain states, you were doing whatever was necessary to force the code into certain states and then testing them. The forcing into certain states was tripping me up. (Perhaps if we had a unit test framework instead of only a regression test framework, then you could have suggested multiple unit tests and I wouldn't have been so confused.) Thanks for explaining. .... > >> 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? > > > > So, this is a slightly different issue than I was getting at before, > > but this feels slightly obstructionist to me. Now, suggesting related > > improvements and ideas sounds totally fine; we should point those out > > -- once. But pushing it again as though it needs to be addressed as > > part of the submission just doesn't feel right. Jonathan's patch > > fixes a real problem and feels complete to me. There are always > > additional things that could also be fixed or addressed for any patch > > or series. Expecting folks submitting a series to address every "next > > step improvement along these lines" that any reviewer can think of > > seems unfriendly, especially as it has a snowball effect. > > > > Granted, if there's a bug with the patch, or it doesn't fully solve > > the stated problem, then it's a different situation, but I don't think > > that's the case here. (Well, modulo the leftover removing of > > "blank-template" which is a real issue you identified with the patch.) > > The first thing I said in this thread is "Thanks. This fix looks good to > me.". I'd be happy to have just this fix in. This patch resolves a > blocked of an earlier series of mine. > > The rest of the feedback here (aside from the trivial "rm -rf" fix) was > an attempt to bridge the gap between this & my earlier look in [1]. It helps to know your intent. The "I'd be happy to have just this fix in" doesn't appear in your emails until here; to me, you communicated the opposite by not only bringing up the additional changes but also following up to the next email by also including a patch when you saw they weren't included. Anyway, I think we're good here; we both agree Jonathan's patch doesn't need the additions, and you've patiently explained to me what I was missing about your view on the tests. Sorry for being slow on that. One other side point, though, since it came up in this thread... > >> > (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? > > > > Yeah, it probably should. > > > > I just did a little more testing and it looks like commands like > > "switch" don't even error out; they just treat the missing > > $GIT_DIR/info/sparse-checkout files the same as all files being > > included. Weird. It seems to come from dir.c:add_patterns(), which > > appears to have perhaps gotten that way due to assuming the code was > > exclusively about .gitignore files rather than the sparse-checkout > > file. ... > FWIW I don't think it's all that important to focus too much on how > users would get into this scenario. Sure, for the "init" case it's a > thing that's broken with --template=, so that's more urgent. Just to check, "this scenario" means core.sparseCheckout=true with $GIT_DIR/info missing (and thus $GIT_DIR/info/sparse-checkout missing), right? > But for the rest we're already carrying code for those edge cases (errno > handling and all), we just don't test for it. > > So just adding tests seems prudent. Okay, but if we're ignoring how users get into this scenario, then there are more affected code paths than just the sparse-checkout subcommand. And it appears that at least some of those do not behave well (they do weird-ish things and pretend success instead of showing a reasonable error message, or even any error message). So there's probably some work involved to identify the relevant subcommands or codepaths, so that we even know what to add tests for.
