On Wed, Nov 16 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> On Mon, Nov 14 2022, Glen Choo wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >>> >>>> It's also proposing to replace Glen's one-patch[6], which is working >>>> around the problem shown in the test added in 1/10 here. Per >>>> downthread of [7] I think Glen was aiming for getting a more narrow >>>> fix in case we split off 9/10 here into some later fix. >>>> >>>> As we're fixing an edge case in something that's always been broken >>>> (and thus wouldn't backport) I think it's better to just fix the >>>> problem directly, rather than introducing new "--super-prefix" use, >>>> just to take it away later. >>> >>> I still prefer that we take the one-patch to unbreak new releases, >>> because partial clone + submodules is absolutely broken (e.g. it's >>> already causing quite a lot of headaches at $DAYJOB) and the patch is >>> obviously harmless. >>> >>> And more importantly, it lets us take our time with this series and get >>> it right without time pressure. It's not as pressing as, e.g. a >>> regression fix, but it does render certain Git setups unusable. >>> >>> With regards to urgency and when to choose "small and harmless fixes vs >>> bigger and better fixes", I think Junio has generally made those calls >>> in the past. @Taylor if you have an opinion, I'd love to hear it. >> >> I feel like I'm missing something here. What's the regression? The test >> you're adding here didn't work at all until 0f5e8851737 (Merge branch >> 'rc/fetch-refetch', 2022-04-04), as the command didn't exist yet. That >> commit went out with v2.36.0. >> >> If it never worked there's no regression, and we wouldn't be merging >> down a fix for older point-releases. >> >> Or is there some case I've missed here which did work before, doesn't >> now, but just isn't captured in this test? If so what case is that, and >> when did it break? > > Right, this wasn't meant to be a regression fix at all. There's good > reason to believe that this was always broken, so I never went digging > to see if it ever worked. > > Even so, it doesn't change the fact that it's a use case that we've > expected to work, but doesn't due to some internal silliness, and that > we could fix it without invoking questions of the "--super-prefix" > design and dragging out the process (which is admittedly what I should > have done in the first place). Since we have such an easy fix in front > of us, I don't feel good about not fixing this before the next release. > > At any rate, I'm wiling to accept that I'm being overly cautious, > because it's quite likely that this series will make it into the next > release. (We could technically we unbreak 'next', though I don't know > who distributes that other than internally @ Google.) I'm ok to drop my > patch for now, but I'll revive it if it starts to look like this series > won't make it into the next release. Understood, I'm re-rolling this v2, will send it out soon. I'll keep that patch out for now, but if we're starting to run up against the next release how about we split it out? So it'll depend on how fast we can review & test this, and if/when Taylor is OK with merging it down. I really don't care if the fix comes first, but just thought I was missing something and it didn't seem urgent, as it was in the "never worked" and not "a regression" category AFAICT. But if you'd still like it shout, and I'll just stack it at the start...
