Message ID | pull.1005.v8.git.1637363024.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Upstreaming the Scalar command | expand |
On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > tl;dr: This series contributes the core part of the Scalar command to the > Git project. This command provides an opinionated way to create and > configure Git repositories with a focus on very large repositories. I thought after https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/ that you'd update merge.renames to true on what is now patch 7. Did you end up changing your mind, or was this overlooked? Other than that, this round looks good to me. (I have no opinion on the build system integration, other than that I like it being optional and not installed by default.)
Hi Elijah, On Sat, 20 Nov 2021, Elijah Newren wrote: > On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > tl;dr: This series contributes the core part of the Scalar command to > > the Git project. This command provides an opinionated way to create > > and configure Git repositories with a focus on very large > > repositories. > > I thought after > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/ > that you'd update merge.renames to true on what is now patch 7. Did > you end up changing your mind, or was this overlooked? Oops! Thank you so much for the reminder. Will fix. I do not plan on sending out a new iteration for a few more days because I do not want to send lots of patches to the list right now, reviewer bandwidth seems to be stretched quite a bit already. > Other than that, this round looks good to me. (I have no opinion on > the build system integration, other than that I like it being optional > and not installed by default.) Yes, I very much wanted to keep this optional and as well-encapsulated as possible for the moment. (Hence the way it integrates with Git's build process.) Thank you for chiming in! Ciao, Dscho
On Mon, Nov 22 2021, Johannes Schindelin wrote: > Hi Elijah, > > On Sat, 20 Nov 2021, Elijah Newren wrote: > >> On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget >> <gitgitgadget@gmail.com> wrote: >> > >> > tl;dr: This series contributes the core part of the Scalar command to >> > the Git project. This command provides an opinionated way to create >> > and configure Git repositories with a focus on very large >> > repositories. >> >> I thought after >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/ >> that you'd update merge.renames to true on what is now patch 7. Did >> you end up changing your mind, or was this overlooked? > > Oops! Thank you so much for the reminder. > > Will fix. I do not plan on sending out a new iteration for a few more days > because I do not want to send lots of patches to the list right now, > reviewer bandwidth seems to be stretched quite a bit already. Bandwidth which is further stretched by continuing to send updates to this topic while ignoring outstanding feedback. I.e. "seen" being broken now due to a merger of this topic and another topic of mine, which as noted in [1] is really just revealing an existing breakage in this topic, which I sent you an unresponded-to patch to fix almost a month ago. >> Other than that, this round looks good to me. (I have no opinion on >> the build system integration, other than that I like it being optional >> and not installed by default.) > > Yes, I very much wanted to keep this optional and as well-encapsulated as > possible for the moment. (Hence the way it integrates with Git's build > process.) > > Thank you for chiming in! Whatever disagreement we have about the particulars of how scalar lands in-tree is one thing, and I'd be the first to admit that some of the stuff I've been suggesting is just my opinion. But I've also been pointing out in reviews of this series (all/most of which you've ignored) that there's specific things that are categorically broken in this series, and clearly not working as you intend them to work. And you're simply ignoring those reports. One of those is that your topic here changes CI in in a way that you clearly didn't intend, and which is an emergent unintended effect of how you're approaching this scalar integration. I.e. the compile-only CI targets now doing tests as a result, which is broken, and the combination of that and my CI topic revealed that breakage. Anyway, as noted in [2] I was hoping to leave this whole scalar thing behind, since I got tired of those reports/suggestions being ignored. I'm only replying here again because it's clear that you don't understand some of the things this scalar topic breaks/changes, and the only reason you wouldn't understand that is because you've been ignoring specific reports/patches-on top that address those breakages. 1. https://lore.kernel.org/git/211122.86ee78yxts.gmgdl@evledraar.gmail.com/ 2. https://lore.kernel.org/git/211110.86czn8hyis.gmgdl@evledraar.gmail.com/
Hi Ævar, On Mon, 22 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > On Mon, Nov 22 2021, Johannes Schindelin wrote: > > > On Sat, 20 Nov 2021, Elijah Newren wrote: > > > >> On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget > >> <gitgitgadget@gmail.com> wrote: > >> > > >> > tl;dr: This series contributes the core part of the Scalar command to > >> > the Git project. This command provides an opinionated way to create > >> > and configure Git repositories with a focus on very large > >> > repositories. > >> > >> I thought after > >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/ > >> that you'd update merge.renames to true on what is now patch 7. Did > >> you end up changing your mind, or was this overlooked? > > > > Oops! Thank you so much for the reminder. > > > > Will fix. I do not plan on sending out a new iteration for a few more days > > because I do not want to send lots of patches to the list right now, > > reviewer bandwidth seems to be stretched quite a bit already. > > Bandwidth which is further stretched by continuing to send updates to > this topic while ignoring outstanding feedback. The feedback you are referring to is probably the repeated demand to integrate Scalar deeply into Git's build process. As I have tired of replying, it is not the time for that yet. Repeating that demand does not make it more sensible, nor does it magically make it the right time. Nor is it credible to call the build "broken" when it does what it is supposed to do, thank you very much. Ciao, Johannes
On Mon, Nov 22 2021, Johannes Schindelin wrote: > On Mon, 22 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > >> On Mon, Nov 22 2021, Johannes Schindelin wrote: >> >> > On Sat, 20 Nov 2021, Elijah Newren wrote: >> > >> >> On Fri, Nov 19, 2021 at 3:03 PM Johannes Schindelin via GitGitGadget >> >> <gitgitgadget@gmail.com> wrote: >> >> > >> >> > tl;dr: This series contributes the core part of the Scalar command to >> >> > the Git project. This command provides an opinionated way to create >> >> > and configure Git repositories with a focus on very large >> >> > repositories. >> >> >> >> I thought after >> >> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110062241150.395@tvgsbejvaqbjf.bet/ >> >> that you'd update merge.renames to true on what is now patch 7. Did >> >> you end up changing your mind, or was this overlooked? >> > >> > Oops! Thank you so much for the reminder. >> > >> > Will fix. I do not plan on sending out a new iteration for a few more days >> > because I do not want to send lots of patches to the list right now, >> > reviewer bandwidth seems to be stretched quite a bit already. >> >> Bandwidth which is further stretched by continuing to send updates to >> this topic while ignoring outstanding feedback. > > The feedback you are referring to is probably the repeated demand to > integrate Scalar deeply into Git's build process. > > As I have tired of replying, it is not the time for that yet. > > Repeating that demand does not make it more sensible, nor does it > magically make it the right time. I'm not repeating that demand. I clearly also think the approach you're insisting picking isn't a good one, but let's leave that aside. What I'm referring to with "I've also been pointing out" in the context you elided is that if you: 1. Check out your topic 2. Apply my proposed patch on top (a newer version than on-list is in avar/scalar-move-build-from-contrib-2 in my GH fork) 3. Push both to CI 4. Diff the two logs of the runs (or just manually click through and inspect them) You'd have seen before sending your version of the CI integration that the difference in behavior that started with your version of the topic was particular to the contrib/scalar/ integration, but not the top-level Makefile integration. I.e. adding the scalar tests to the previously build-only jobs. I've been noting that as clearly as I'm able to in numerous past exchanges. You've either ignored those reports, or like here, selectivtely replied only to parts of what I've told you. I.e. something like "I'm not going 100% for the approach you suggest". Sure, I'm not saying you have to. But I also noted that the patch with that suggested approach can be considered a bug report against your series. The reason that patch isn't split into two things, one fixing all the issues I noticed, and another implementing some "alternate build" approach is that I found that to be impossible to do. Those issues are all particular to emergent effects of the build integration you're choosing to go with. E.g. in the case of "seen" being broken the CI simply runs "make test" as it did before, and scalar integrates into that, and you can run that target without having built anything already. > Nor is it credible to call the build "broken" when it does what it is > supposed to do, thank you very much. Here's specific commands showing that it's broken. On your version (I've got v7 locally, but the same is true of v8): $ make clean; make -C contrib/scalar test [...] CC hook.o CC version.o CC help.o AR libgit.a make[1]: Leaving directory '/home/avar/g/git' SUBDIR ../.. make[1]: Entering directory '/home/avar/g/git' * new link flags CC contrib/scalar/scalar.o LINK contrib/scalar/scalar make[1]: Leaving directory '/home/avar/g/git' make -C t make[1]: Entering directory '/home/avar/g/git/contrib/scalar/t' *** prove *** error: GIT-BUILD-OPTIONS missing (has Git been built?). t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100) No subtests run Test Summary Report ------------------- t9099-scalar.sh (Wstat: 256 Tests: 0 Failed: 0) Non-zero exit status: 1 Parse errors: No plan found in TAP output On the patch I proposed to apply on top: $ make clean; make test T=t9099-scalar.sh [...] CC t/helper/test-xml-encode.o GEN bin-wrappers/git GEN bin-wrappers/git-receive-pack GEN bin-wrappers/git-shell GEN bin-wrappers/git-upload-archive GEN bin-wrappers/git-upload-pack GEN bin-wrappers/scalar GEN bin-wrappers/git-cvsserver GEN bin-wrappers/test-fake-ssh GEN bin-wrappers/test-tool LINK t/helper/test-fake-ssh LINK t/helper/test-tool make -C t/ all make[1]: Entering directory '/home/avar/g/git/t' rm -f -r 'test-results' *** prove *** t9099-scalar.sh .. ok All tests successful. You might correctly note that this doesn't work either on that version (or for any other existing test in t/): $ make clean >/dev/null; make -C t T=t9099-scalar.sh GIT_VERSION = 2.34.GIT-dev make: Entering directory '/home/avar/g/git/t' rm -f -r 'test-results' *** prove *** error: GIT-BUILD-OPTIONS missing (has Git been built?). t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100) No subtests run Test Summary Report ------------------- t9099-scalar.sh (Wstat: 256 Tests: 0 Failed: 0) Non-zero exit status: 1 Parse errors: No plan found in TAP output Which is true, but that's not broken because it's not attempting to build the top-level via some incomplete integration, but your version is.
Hi Ævar, On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > [...] > $ make clean; make -C contrib/scalar test > [...] > CC hook.o > CC version.o > CC help.o > AR libgit.a > make[1]: Leaving directory '/home/avar/g/git' > SUBDIR ../.. > make[1]: Entering directory '/home/avar/g/git' > * new link flags > CC contrib/scalar/scalar.o > LINK contrib/scalar/scalar > make[1]: Leaving directory '/home/avar/g/git' > make -C t > make[1]: Entering directory '/home/avar/g/git/contrib/scalar/t' > *** prove *** > error: GIT-BUILD-OPTIONS missing (has Git been built?). > t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100) > No subtests run That's cute. You seem to have missed that this is `contrib/`? The assumption of pretty much _everything_ in there is that Git was already built. Try this at home: `make clean && make -C contrib/subtree/ test` Yep. It "fails" in the same way. "has Git been built?". So if that was all the evidence in favor of that misinformation "Scalar's build is broken! Broken, broken, BROKEN!", I think we can now let it rest. At last. Ciao, Johannes
On Tue, Nov 23 2021, Johannes Schindelin wrote: > Hi Ævar, > > On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > >> [...] >> $ make clean; make -C contrib/scalar test >> [...] >> CC hook.o >> CC version.o >> CC help.o >> AR libgit.a >> make[1]: Leaving directory '/home/avar/g/git' >> SUBDIR ../.. >> make[1]: Entering directory '/home/avar/g/git' >> * new link flags >> CC contrib/scalar/scalar.o >> LINK contrib/scalar/scalar >> make[1]: Leaving directory '/home/avar/g/git' >> make -C t >> make[1]: Entering directory '/home/avar/g/git/contrib/scalar/t' >> *** prove *** >> error: GIT-BUILD-OPTIONS missing (has Git been built?). >> t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100) >> No subtests run > > That's cute. You seem to have missed that this is `contrib/`? The > assumption of pretty much _everything_ in there is that Git was already > built. > > Try this at home: `make clean && make -C contrib/subtree/ test` > > Yep. It "fails" in the same way. "has Git been built?". > > So if that was all the evidence in favor of that misinformation "Scalar's > build is broken! Broken, broken, BROKEN!", I think we can now let it rest. > At last. No, it doesn't fail in the same way. Really, it seems like you're either not fully reading through E-Mails before replying, or entirely misunderstanding what I'm saying. I highlighted the difference in "[...]that's not broken because[...]" below the context you're quoting. I'm specifically pointing out the difference between how these act: make clean; make -C contrib/subtree/ test make clean; make -C t Which fail right away without trying to build anything, and how your: make clean; make -C contrib/scalar test Won't fail right away, but get most of the way towards building what it needs at the top-level. So yes, I fully agree with your contrib/subtree example, but it's making my argument for me. As I pointed out in the just-sent [1] the main issue is that your latest iteration added "make -C contrib/subtree/ test" in the wrong place, and we thus ended up running those tests in places we didn't intend. The main conflict is that semantic conflict. But that failure also highlighted that contrib/scalar/Makefile will run a build of the top-level C code, only to error out with "GIT-BUILD-OPTIONS". That specifically is what I consider broken. It should either actually work and properly build its dependency, or not even try. Either one would be fine in this context, it's the in-between that's clearly (to me at least) broken. Working software should either attempt a task and succeed, or not attempt the task at all. 1. https://lore.kernel.org/git/211123.86ilwjujmd.gmgdl@evledraar.gmail.com/
Hi Ævar, On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 23 2021, Johannes Schindelin wrote: > > > Hi Ævar, > > > > On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > > > >> [...] > >> $ make clean; make -C contrib/scalar test > >> [...] > >> CC hook.o > >> CC version.o > >> CC help.o > >> AR libgit.a > >> make[1]: Leaving directory '/home/avar/g/git' > >> SUBDIR ../.. > >> make[1]: Entering directory '/home/avar/g/git' > >> * new link flags > >> CC contrib/scalar/scalar.o > >> LINK contrib/scalar/scalar > >> make[1]: Leaving directory '/home/avar/g/git' > >> make -C t > >> make[1]: Entering directory '/home/avar/g/git/contrib/scalar/t' > >> *** prove *** > >> error: GIT-BUILD-OPTIONS missing (has Git been built?). > >> t9099-scalar.sh .. Dubious, test returned 1 (wstat 256, 0x100) > >> No subtests run > > > > That's cute. You seem to have missed that this is `contrib/`? The > > assumption of pretty much _everything_ in there is that Git was already > > built. > > > > Try this at home: `make clean && make -C contrib/subtree/ test` > > > > Yep. It "fails" in the same way. "has Git been built?". > > > > So if that was all the evidence in favor of that misinformation "Scalar's > > build is broken! Broken, broken, BROKEN!", I think we can now let it rest. > > At last. > > No, it doesn't fail in the same way. Really, it seems like you're either > not fully reading through E-Mails before replying, or entirely > misunderstanding what I'm saying. That's really... fresh. I told you half a dozen times that at this point, the build works well enough, and that what you keep insisting is a problem simply isn't. Yes, you have to build Git before building Scalar. Have you actually looked at the design of Scalar? What it does to allow working with large repositories? _That_ is what counts. That you are told to please build Git before running Scalar's tests is maybe a minor annoyance, but not worth the dozens of mails and the weeks of delay that you caused over this. Ciao, Johannes