| Message ID | pull.1005.v9.git.1638273289.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Upstreaming the Scalar command | expand |
On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote: > [...] > * The rebase on top of v2.34.0, which changed the default merge strategy to > ORT, should have changed the default for merge.renames to true. This is > now the case. > * Accommodate preemptively for ab/ci-updates which invalidates assumptions > made by this patch series that would still hold true with v2.34.0 but are > no longer valid in seen and would trigger CI build breakages. > [...] > 1: 3a2e28275f1 = 1: 3a2e28275f1 scalar: add a README with a roadmap > 2: 50160d61a41 = 2: 50160d61a41 scalar: create a rudimentary executable > 3: 74cd6410931 = 3: 74cd6410931 scalar: start documenting the command > 4: 37231a4dd07 = 4: 37231a4dd07 scalar: create test infrastructure > 5: a39b9c81214 = 5: a39b9c81214 cmake: optionally build `scalar`, too > 6: 8e3542e43f7 ! 6: 8c6762def30 ci: also run the `scalar` tests > @@ .github/workflows/main.yml: jobs: > - name: upload tracked files and build artifacts > > ## ci/run-build-and-tests.sh ## > -@@ ci/run-build-and-tests.sh: linux-gcc-4.8|pedantic) > - make test > - ;; > - esac > -+make -C contrib/scalar test > +@@ ci/run-build-and-tests.sh: esac > > check_unignored_build_artifacts > > ++make && make -C contrib/scalar test > ++ > + save_good_tree This gets rid of the hard CI failure we saw in "seen" but still carries forward the logic error noted in [1] and of the "pedantic" compilation-only job now running tests, which isn't what that job is supposed to be doing. See the "Don't run the tests" comment in cebead1ebfb (ci: run a pedantic build as part of the GitHub workflow, 2021-08-08). As noted before you can see that at the tail-end of your own CI output[2] on top of "master". There's other seemingly unintended interactions with the ci/ code on "master" here (which I also also noted in previous threads). FWIW the naïve merge with ab/ci-updates happens to fix at least one of those. I.e. the issue here of "linux-gcc" and "linux-clang" only running the scalar tests in one half of their test modes, but running everything else in both. I think it's clear from past exchanges that you vehemently disagree with my proposed direction for solving these and other issues in one fell swoop[3], which is fair enough. But I really don't think it's OK to continue to ignore reports from me of specific issues in this series just because you don't like that larger set of fixes [3]. I honestly don't care if you'd pick that up as-is, just as long as outstanding issues it addressed are either fixed, or commit messages/docs are updated to note that the bugs/behavior changes are intentional. In this case I think it would be fine to keep the patch as-is and have the commit message argue for why the scalar tests should be a special snowflake in being the only tests that are run in "pedantic". Or to just fix the seemingly unintentional behavior change in some smaller way. I think the "added thusly" comment I hade in [4] should be the easiest way to do that (well, [3] is easier, but let's leave that aside...). 1. https://lore.kernel.org/git/211122.86ee78yxts.gmgdl@evledraar.gmail.com/ 2. https://github.com/gitgitgadget/git/runs/4292915519?check_suite_focus=true 3. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/ 4. https://lore.kernel.org/git/211123.86ilwjujmd.gmgdl@evledraar.gmail.com/
Hi Ævar, On Tue, 30 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote: > > > [...] Unfortunately, you clipped the most important part, the part that I put in there mostly for your benefit. So let me repeat it once again: This patch series' focus is entirely on Scalar, on choosing sensible defaults and offering a delightful user experience around working with monorepos, and not about changing any existing paradigms for contrib/. I do see that you want to drag the conversation back to discussing the build process, and the CI integration. And on changing the way things are done in `contrib/`. You've made that point abundantly clear. I just don't see how that could possibly improve Scalar. I mean, if it failed during the past 4 months, why expect any different outcome in the future. Ciao, Johannes
On Tue, Nov 30 2021, Johannes Schindelin wrote: > Hi Ævar, > > On Tue, 30 Nov 2021, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote: >> >> > [...] > > Unfortunately, you clipped the most important part, the part that I put in > there mostly for your benefit. So let me repeat it once again: > > This patch series' focus is entirely on Scalar, on choosing sensible > defaults and offering a delightful user experience around working with > monorepos, and not about changing any existing paradigms for contrib/. > > I do see that you want to drag the conversation back to discussing the > build process, and the CI integration. And on changing the way things are > done in `contrib/`. You've made that point abundantly clear. I just don't > see how that could possibly improve Scalar. I mean, if it failed during > the past 4 months, why expect any different outcome in the future. The seemingly unintentional behavior change in CI jobs that aren't scalar jobs you're introducing started in v7 of this series, submitted on November 17th: https://lore.kernel.org/git/1b0328fa236a35c2427b82f53c32944e513580d3.1637158762.git.gitgitgadget@gmail.com/ So as far as any CI testing is concerned we're talking about just under 2 weeks. I really don't see how that and other unintentional behavior changes in the CI on top of "master" have anything to do with "the build process" in the sense that we've discussed as part of the greater scalar integration topic in the past. I only linked to those thread(s) because some of that behavior changing (i.e. now running tests in a previously compile-only job) is apparent when either running with my patch-on-top of this series, or it was discussed in some threads related to that & the merger with ab/ci-updates. Is it intentional that the previously compile-only "pedantic" job is now running the scalar tests?
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > This patch series' focus is entirely on Scalar, on choosing sensible > defaults and offering a delightful user experience around working with > monorepos, and not about changing any existing paradigms for contrib/. Sorry, but I am confused. The change to add "make &&" before testing scalar is a good change that allows CI to work with "existing paradigm for contrib/" that is "you need to build the top before doing anything in contrib/". But none of the contrib/ stuff is tested in the pedantic job, but if I understand correctly, we start some (namely, scalar) stuff in it tested there, deviating from existing practice. Is that intended?
Hi Junio, On Wed, 1 Dec 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > This patch series' focus is entirely on Scalar, on choosing sensible > > defaults and offering a delightful user experience around working with > > monorepos, and not about changing any existing paradigms for contrib/. > > Sorry, but I am confused. > > The change to add "make &&" before testing scalar is a good change > that allows CI to work with "existing paradigm for contrib/" that is > "you need to build the top before doing anything in contrib/". > > But none of the contrib/ stuff is tested in the pedantic job, but if > I understand correctly, we start some (namely, scalar) stuff in it > tested there, deviating from existing practice. Is that intended? No, it was not intended. It was not even intended to integrate Scalar this tightly with Git's CI, but since you did not move along `js/scalar` into `next` for the past weeks, when no reviewer had anything to add to the actual code in `contrib/scalar/` nor were there any objections to integrate it, I made the mistake of assuming that you agreed with Ævar that such a tight integration into Git's CI was desired. However, I do not want to make the mistake again of assuming what your thoughts are, so let me ask you directly: from your perspective, what is stopping the inclusion of `js/scalar` into `next`? I am patiently and eagerly awaiting your answer, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > No, it was not intended. It was not even intended to integrate Scalar this > tightly with Git's CI, but since you did not move along `js/scalar` into > `next` for the past weeks, when no reviewer had anything to add to the > actual code in `contrib/scalar/` nor were there any objections to > integrate it, I made the mistake of assuming that you agreed with Ævar > that such a tight integration into Git's CI was desired. OK, sorry to hear that we had miscommunication. I took the lack of comments an indication that people are not either interested in it, or viewing it as not-quite-ready-yet and waiting for a "more or less done" version. I think the CI updates from Ævar would be one of the things we'd have early in 'next' in this cycle, so if the topic does not play nice with it, the perception that it is not yet part of the regular CI testing would continue, I am afraid. Thanks.
On Thu, Dec 2, 2021 at 9:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > No, it was not intended. It was not even intended to integrate Scalar this > > tightly with Git's CI, but since you did not move along `js/scalar` into > > `next` for the past weeks, when no reviewer had anything to add to the > > actual code in `contrib/scalar/` nor were there any objections to > > integrate it, I made the mistake of assuming that you agreed with Ævar > > that such a tight integration into Git's CI was desired. > > OK, sorry to hear that we had miscommunication. > > I took the lack of comments an indication that people are not either > interested in it, or viewing it as not-quite-ready-yet and waiting > for a "more or less done" version. Sorry that my work project rendered me unable to respond for over a month. > I think the CI updates from Ævar would be one of the things we'd > have early in 'next' in this cycle, so if the topic does not play > nice with it, the perception that it is not yet part of the regular > CI testing would continue, I am afraid. I think I missed the answer. I believe Johannes was curious if he reverted the recent CI testing of scalar he added in v7 (which would as a side effect make it play nicely with Ævar's CI updates), if the resulting version of js/scalar would be acceptable for next. (If my opinion matters: I'd be in favor. In more detail: Personally, while I think CI testing would be nice once we have a functionally useful scalar, the CI tests of this early version aren't really netting us anything. And they're blocking future scalar series unnecessarily. Johannes already said he had planned CI testing for a future series, so I'd rather just take this version of js/scalar minus the CI integration for next.)
Elijah Newren <newren@gmail.com> writes: > while I think CI testing would be nice once we have a functionally > useful scalar, the CI tests of this early version aren't really > netting us anything. And they're blocking future scalar series > unnecessarily. Johannes already said he had planned CI testing for a > future series, so I'd rather just take this version of js/scalar minus > the CI integration for next.) Yeah, with less stomping on each others' toes, things may flow smoother. As long as people are happy with the core part, that sounds like the best approach forward.
Hi Junio, On Thu, 2 Dec 2021, Junio C Hamano wrote: > Elijah Newren <newren@gmail.com> writes: > > > while I think CI testing would be nice once we have a functionally > > useful scalar, the CI tests of this early version aren't really > > netting us anything. And they're blocking future scalar series > > unnecessarily. Johannes already said he had planned CI testing for a > > future series, so I'd rather just take this version of js/scalar minus > > the CI integration for next.) > > Yeah, with less stomping on each others' toes, things may flow > smoother. I also would find it nice if that was the case. If I remember correctly, you mentioned quite a couple of times that you expect, particularly oldtimers on this list, to be mindful when contributing patch series, and to delay patches that would interfere with other patch series that are already in flight. I saw with sorrow that this rule was ignored a couple of times recently, even with new contributors, and I sincerely hope that we can unignore that rule again. > As long as people are happy with the core part, that sounds like the > best approach forward. Thank you. I have to admit that it was quite frustrating to see so many obstacles being put in my way, only to end up at pretty much the same place as I had been over a month ago. So it is extra gratifying to hear that you move forward with the Scalar patch series. I truly believe that our users will be better off for it. Thanks, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > If I remember correctly, you mentioned quite a couple of times that you > expect, particularly oldtimers on this list, to be mindful when > contributing patch series, and to delay patches that would interfere with > other patch series that are already in flight. > > I saw with sorrow that this rule was ignored a couple of times recently, > even with new contributors, and I sincerely hope that we can unignore that > rule again. Sorry, but I am not sure what you are complaining about. In general, I do try to ask more experienced and competent folks to bear more burden when playing the role of a traffic coordinator, as they are more capable of doing so to help the process. Relative importance and complexity of the topics also play a role, so it is also possible that a more junior contributor may be asked to yield for a more urgent or a less complex topic. I would give strong preference to things that are already in 'next', of course. There has to be an extraordinary reason before we kick something out of 'next', only to yield the way to allow another topic to graduate first. So, I am not sure if there is a need to unignore any rule here. Thanks.
tl;dr: This series contributes the core part of the Scalar command to the Git project. This command provides a convenient way to clone/initialize very large repositories (think: monorepos). Note: This patch series' focus is entirely on Scalar, on choosing sensible defaults and offering a delightful user experience around working with monorepos, and not about changing any existing paradigms for contrib/ (even if catching up on the mail thread is likely to give interested readers that false impression). Changes since v8: * The rebase on top of v2.34.0, which changed the default merge strategy to ORT, should have changed the default for merge.renames to true. This is now the case. * Accommodate preemptively for ab/ci-updates which invalidates assumptions made by this patch series that would still hold true with v2.34.0 but are no longer valid in seen and would trigger CI build breakages. Changes since v7: * Clarified in the commit message why we cannot easily encapsulate the Scalar part of the CMake configuration in contrib/scalar/. * Improved the README.md. Changes since v6: * Rebased on top of v2.34.0. * Inserted a commit that adds contrib/scalar/README.md, containing the roadmap of what I have planned for Scalar. * The Scalar test's definition of GIT_TEST_MAINT_SCHEDULER has been adjusted to accommodate for a change in v2.32.0..v2.34.0. * The config setting defaults now include fetch.showForcedUpdates=false, which has been identified as helping with a performance issue in large repositories. * To avoid mistaking the current patch series for being feature-complete enough to unleash onto end users, I moved the Makefile rules to build HTML/manual pages to a later patch series. * The patch that adds support for -c <key>=<value> and -C <directory> was moved to its own add-on patch series: While it is obvious that those options are valuable to have, an open question is whether there are other "pre-command" options in git that would be useful, too, and I would like to postpone that discussion to that date. * I added two patches that I had planned on keeping in an add-on patch series for later, to build and test Scalar as part of the CI. I am still not 100% certain that it is a good idea to do so already now, but let's see what the reviewers have to say. Changes since v5: * Fixed the commit message talking about make -C contrib/scalar/Makefile. * Fixed the git ls-tree invocation suggested in the manual for scalar clone. * Invoking make -C contrib/scalar, then changing a source file of libgit.a and then immediately invoking make -C contrib/scalar again will now implicitly rebuild libgit.a. Changes since v4: * scalar delete now refuses to delete anything if it was started from within the enlistment. * scalar delete releases any handles to the object store before deleting the enlistment. * The OBJECTS list in the Makefile will now include Scalar. * scalar register now supports secondary worktrees, in addition to the primary worktree. Changes since v3: * Moved the "Changes since" section to the top, to make it easier to see what changed. * Reworded the commit message of the first patch. * Removed the [RFC] prefix because I did not hear any objections against putting this into contrib/. Changes since v2: * Adjusted the description of the list command in the manual page , as suggested by Bagas. * Addressed two style nits in cmd_run(). * The documentation of git reconfigure -a was improved. Changes since v1: * A couple typos were fixed * The code parsing the output of ls-remote was made more readable * The indentation used in scalar.txt now consistently uses tabs * We no longer hard-code core.bare = false when registering with Scalar Background ========== Microsoft invested a lot of effort into scaling Git to the needs of the Windows operating system source code. Based on the experience of the first approach, VFS for Git, the Scalar project was started. Scalar specifically has as its core goal to funnel all improvements into core Git. The present =========== The Scalar project provides a completely functional non-virtual experience for monorepos. But why stop there. The Scalar project was designed to be a self-destructing vehicle to allow those key concepts to be moved into core Git itself for the benefit of all. For example, partial clone, sparse-checkout, and scheduled background maintenance have already been upstreamed and removed from Scalar proper. This patch series provides a C-based implementation of the final remaining portions of the Scalar command. This will make it easier for users to experiment with the Scalar command. It will also make it substantially easier to experiment with moving functionality from Scalar into core Git, while maintaining backwards-compatibility for existing Scalar users. The C-based Scalar has been shipped to Scalar users, and can be tested by any interested reader: https://github.com/microsoft/git/releases/ (it offers a Git for Windows installer, a macOS package and an Ubuntu package, Scalar has been included since v2.33.0.vfs.0.0). Next steps ========== Since there are existing Scalar users, I want to ensure backwards-compatibility with its existing command-line interface. Keeping that in mind, everything in this series is up for discussion. I obviously believe that Scalar brings a huge benefit, and think that it would be ideal for all of Scalar's learnings to end up in git clone/git init/git maintenance eventually. It is also conceivable, however, that the scalar command could graduate to be a core part of Git at some stage in the future (such a decision would probably depend highly on users' feedback). See also the discussion about the architecture of Scalar [https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/], kicked off by Stolee. On top of this patch series, I have lined up a few more: 1. Implement a scalar diagnose command. 2. Use the built-in FSMonitor (that patch series obviously needs to wait for FSMonitor to be integrated). 3. Modify the config machinery to be more generous about concurrent writes, say, to the user-wide config. 4. A few patches to optionally build and install scalar as part of a regular Git install (also teaching git help scalar to find the Scalar documentation These are included in my vfs-with-scalar branch thicket [https://github.com/dscho/git/commits/vfs-with-scalar]. On top of that, this branch thicket also includes patches I do not plan on upstreaming, mainly because they are too specific either to VFS for Git, or they support Azure Repos (which does not offer partial clones but speaks the GVFS protocol, which can be used to emulate partial clones). One other thing is very interesting about that vfs-with-scalar branch thicket: it contains a GitHub workflow which will run Scalar's quite extensive Functional Tests suite. This test suite is quite comprehensive and caught us a lot of bugs in the past, not only in the Scalar code, but also core Git. Epilogue ======== Now, to address some questions that I imagine every reader has who made it this far: * Why not put the Scalar functionality directly into core Git, even a built-in? I wanted to provide an easy way for Git contributors to "play with" Scalar, without forcing a new top-level command into Git. * Why implement the Scalar command in the Git code base? Apart from simplifying Scalar maintenance in the Microsoft port of Git, the tight version coupling between Git and Scalar reduces the maintenance burden even further. Besides, I believe that it will make it much easier to shift functionality from Scalar into core Git, once we took the hurdle of accepting the Scalar code into the code base. * Why contribute Scalar to the Git project? We are biased, of course, yet our data-driven approach provides evidence that Scalar helps handling huge repositories with ease. By contributing it to the core Git project, we are able to share it with more users, especially some users who do not want to install Microsoft's fork of Git. We also hope that a lot of Scalar (maybe all of it) will end up in core Git, to benefit even more users. Derrick Stolee (4): scalar: 'register' sets recommended config and starts maintenance scalar: 'unregister' stops background maintenance scalar: implement 'scalar list' scalar: implement the `run` command Johannes Schindelin (12): scalar: add a README with a roadmap scalar: create a rudimentary executable scalar: start documenting the command scalar: create test infrastructure cmake: optionally build `scalar`, too ci: also run the `scalar` tests scalar: let 'unregister' handle a deleted enlistment directory gracefully scalar: implement the `clone` subcommand scalar: teach 'clone' to support the --single-branch option scalar: allow reconfiguring an existing enlistment scalar: teach 'reconfigure' to optionally handle all registered enlistments scalar: implement the `version` command Matthew John Cheetham (1): scalar: implement the `delete` command .github/workflows/main.yml | 15 + Makefile | 9 + ci/run-build-and-tests.sh | 2 + ci/run-test-slice.sh | 5 + contrib/buildsystems/CMakeLists.txt | 14 + contrib/scalar/.gitignore | 2 + contrib/scalar/Makefile | 45 ++ contrib/scalar/README.md | 82 +++ contrib/scalar/scalar.c | 826 ++++++++++++++++++++++++++++ contrib/scalar/scalar.txt | 145 +++++ contrib/scalar/t/Makefile | 78 +++ contrib/scalar/t/t9099-scalar.sh | 88 +++ 12 files changed, 1311 insertions(+) create mode 100644 contrib/scalar/.gitignore create mode 100644 contrib/scalar/Makefile create mode 100644 contrib/scalar/README.md create mode 100644 contrib/scalar/scalar.c create mode 100644 contrib/scalar/scalar.txt create mode 100644 contrib/scalar/t/Makefile create mode 100755 contrib/scalar/t/t9099-scalar.sh base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1005%2Fdscho%2Fscalar-the-beginning-v9 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1005/dscho/scalar-the-beginning-v9 Pull-Request: https://github.com/gitgitgadget/git/pull/1005 Range-diff vs v8: 1: 3a2e28275f1 = 1: 3a2e28275f1 scalar: add a README with a roadmap 2: 50160d61a41 = 2: 50160d61a41 scalar: create a rudimentary executable 3: 74cd6410931 = 3: 74cd6410931 scalar: start documenting the command 4: 37231a4dd07 = 4: 37231a4dd07 scalar: create test infrastructure 5: a39b9c81214 = 5: a39b9c81214 cmake: optionally build `scalar`, too 6: 8e3542e43f7 ! 6: 8c6762def30 ci: also run the `scalar` tests @@ .github/workflows/main.yml: jobs: - name: upload tracked files and build artifacts ## ci/run-build-and-tests.sh ## -@@ ci/run-build-and-tests.sh: linux-gcc-4.8|pedantic) - make test - ;; - esac -+make -C contrib/scalar test +@@ ci/run-build-and-tests.sh: esac check_unignored_build_artifacts ++make && make -C contrib/scalar test ++ + save_good_tree ## ci/run-test-slice.sh ## @@ ci/run-test-slice.sh: make --quiet -C t T="$(cd t && 7: 385abdb8d8e ! 7: 936ee0475ad scalar: 'register' sets recommended config and starts maintenance @@ contrib/scalar/scalar.c + { "index.threads", "true" }, + { "index.version", "4" }, + { "merge.stat", "false" }, -+ { "merge.renames", "false" }, ++ { "merge.renames", "true" }, + { "pack.useBitmaps", "false" }, + { "pack.useSparse", "true" }, + { "receive.autoGC", "false" }, 8: 64c6a75353e = 8: 09a15f86c3d scalar: 'unregister' stops background maintenance 9: f7fc1958b9e = 9: 42121a5764d scalar: let 'unregister' handle a deleted enlistment directory gracefully 10: fd2680bc945 = 10: 6afb2eb4163 scalar: implement 'scalar list' 11: 4966a43aad9 = 11: dd4e3a4b761 scalar: implement the `clone` subcommand 12: b00d68b37b0 = 12: abd9c8827cd scalar: teach 'clone' to support the --single-branch option 13: 771d826bbb1 = 13: 5601f82dbe1 scalar: implement the `run` command 14: a8b2d26a830 ! 14: 08e4f548aa8 scalar: allow reconfiguring an existing enlistment @@ contrib/scalar/scalar.c: static int set_recommended_config(void) - { "index.threads", "true" }, - { "index.version", "4" }, - { "merge.stat", "false" }, -- { "merge.renames", "false" }, +- { "merge.renames", "true" }, - { "pack.useBitmaps", "false" }, - { "pack.useSparse", "true" }, - { "receive.autoGC", "false" }, @@ contrib/scalar/scalar.c: static int set_recommended_config(void) + { "index.threads", "true", 1 }, + { "index.version", "4", 1 }, + { "merge.stat", "false", 1 }, -+ { "merge.renames", "false", 1 }, ++ { "merge.renames", "true", 1 }, + { "pack.useBitmaps", "false", 1 }, + { "pack.useSparse", "true", 1 }, + { "receive.autoGC", "false", 1 }, 15: ca284ff34a2 = 15: 0cec6dbd2cb scalar: teach 'reconfigure' to optionally handle all registered enlistments 16: 9983eb8912c = 16: 835f1c79792 scalar: implement the `delete` command 17: 889f613ab18 = 17: 4ee1b701c7b scalar: implement the `version` command