Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > 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... That sounds good, thanks :) That's very helpful of you, and I appreciate you making the way forward clear.
