Re: [PATCH v3 00/15] [RFC] Upstreaming the Scalar command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 14, 2021, 10:59 UTC
Message-ID: <87mtofnzv1.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2109131914080.55@tvgsbejvaqbjf.bet>

On Mon, Sep 13 2021, Johannes Schindelin wrote:

Show 47 quoted lines

> Hi Ævar,
>
> On Mon, 13 Sep 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Thu, Sep 09 2021, Ævar Arnfjörð Bjarmason wrote:
>>
>> > On Wed, Sep 08 2021, Johannes Schindelin via GitGitGadget wrote:
>> >
>> >> 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
>> >
>> > In the summary I had on v1->v2 points 1-3 are for v2->v3, respectively,
>> > outstanding, addressed, outstanding:
>> >
>> >     https://lore.kernel.org/git/877dfupl7o.fsf@evledraar.gmail.com/
>> >
>> > In addition the discussion ending here:
>> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109082112270.55@tvgsbejvaqbjf.bet/
>> >
>> > For that point: I think it's fair enough not to properly handle the
>> > cleanup case in "scalar clone", but perhaps add a note in the commit
>> > message that unlike "git clone" this is known not to clean after itself
>> > properly on ctrl+c?
>>
>> Seeing [1] about the planned re-roll I have the above a shot a few days
>> ago, see the original discussion at [2] (indirectly linked above).
>
> There is a good reason why I did not engage in that tangent about
> deviating from the established `contrib/*/Makefile` paradigm: I find it
> particularly unrelated to what this here patch series is trying to
> accomplish, and I cannot bring myself to be interested in the proposed
> build system changes, either, because I do not see any benefit in the
> changes, only downsides.
>
> I find the distraction unnecessary.

Perhaps I'm reading too much between the lines here, so forgive any undue knee-jerk reaction.

But aside from any technical disagreement we may have I find this way of handling reviews quite disrespectful of other people's time.

Sure, maybe I'm wrong, and maybe you either don't see any value in the proposed changes or maybe they're just bad suggestions.

I still took the time to review and comment on a series you're submitting. I think the least you can do is to include some comment in a re-roll like:

    Skimmed Ævar's proposal about an alternate build system
    implementation, sorry, I just don't think it's worth it, going to
    not change anything there.

Which would be fair enough, and would leave the ball in my court in terms of either dropping it, submitting any patch on top etc.

As opposed to just ignoring that whole thread, leaving both me wondering if it's even been seen (and sending a few reminders like the linked upthread), as well as others trying to track the state of the series.

Show 7 quoted lines

> Besides, the way I designed it, the code in `contrib/scalar/` intrudes as
> little as possible on the core Git build system. The impact on the
> top-level `Makefile` is quite minimal, which is just the way I firmly
> believe it should be.
>
> In short: I do not want those intrusive changes to the top-level
> `Makefile`, not in this patch series, and not as a follow-up, either.

Communication grievances aside:

 * Saying that we have an "established `contrib/*/Makefile` paradigm"
   doesn't really follow in this case. Most of that isn't C code, and
   the bits that are C code are not using libgit.a.

   And as I've argued elsewhere I think that whole pattern was a mistake
   in the first place, it makes inter-Makefile dependencies a pain to
   manage, has resulted in bitrot of things like git-subtree.sh and
   mw-to-git, all because we conflate whether we want to build/test
   things with what we'd like in a default installation.

 * Because of that any number of targets / workflows in the Makefile
   aren't going to work by default, e.g. try checking it out and doing
   "make TAGS".

   That specific one happens to because we exclude contrib explicitly,
   that could be fixed, but there's going to be any number of things
   like that, but current and future ones.

 * One target that seems missing (maybe I've somehow missed it) is any
   support for installing the build command, its docs etc.

 * I hacked a bit more on this today and came up with a not-quite-ready
   change that both for the Makefile and Documentation/Makefile will
   build scalar.c like any other top-level command, and we'll always get
   a bin-wrapper/scalar, we'll only do something different at "install"
   time.

   This means that just like the "all:: $(FUZZ_OBJS)" we'll always build
   a scalar.o, so that'll prevent others from e.g. breaking a library
   function you rely on (which'll only be annoying caught in CI, or
   integration or whatever).

 * I think it's fair to say that whatever one thinks of my argument,
   your [1] leaves the reader hanging about *why* you went for this
   "make -C contrib/scalar/Makefile".

   As summarized there you tried to have it completely separate, but
   failed.

   So now there's a bit of integration in the top-level Makefile
   already. But why try in the first place? Just slavish conformance to
   existing "contrib/" convention?

   The dependency back on assets in subdirs there is deep, so surely
   it's not to build this independently somehow.

Show 6 quoted lines

> We have much bigger fries to fry: namely, how to migrate the improvements
> for large-scale operations from Scalar to core Git, so that all Git users
> can benefit. Granted, it will take a lot effort, and it would be easier to
> move around `Makefile` rules instead. But ultimately, the benefit of
> allowing users to handle larger repositories with ease will be worth that
> effort.

It's your implementation that requires mostly moving around / copying Makefile rules, if you piggy-back on the existing Makefile rules you'll get most of the behavior you've duplicated for free without any moving or copying.

But in any case, re the last bullet point above and your "[...]and not as a follow-up, either" comments it's not clear whether you're saying that you don't have time to work on this, or that you wouldn't want it at all in any shape or form, even if someone else did it.

Which is not to say that I'm promising to do so even if that's the case, I do think the onus is on the person proposing the change, and to take productive feedback about things that are introducing unnecessary complexity, and won't saddle others with undue technical debt going forward.

1. https://lore.kernel.org/git/b8c7d3f84508ae0fb300f47c726764f4cbf46be9.1631129086.git.gitgitgadget@gmail.com/