On Tue, Jun 01 2021, Emily Shaffer wrote:
> On Fri, May 28, 2021 at 02:11:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> After suggesting[1] an another round that the config-based-hook
>> topic[2] should take a more incremental approach to reach its end goal
>> I thought I'd try hacking that up.
>>
>> So this is a proposed restart of that topic which if the consensus
>> favors it should replace it, and the config-based hooks topic should
>> be rebased on top of this.
>
> I'm not entirely sure what you're trying to achieve by sending this
> series.
I'm trying to convince you and others that an approach where we split up
the refactoring part of your large series from the behavior changes +
new behaviors it introduces is a better approach in getting us to your
currently proposed (or some small variation thereof) desired end-state.
I think the opening line of your reply doesn't bode for the "convince
you" part of that, maybe I'll do better on the "and others" front :)
I'm a bit surprised at what seems to be some hostility or annoyance that
I submitted this as a set of patches. That's ultimately something that
saves everyone involved time (well, except me by coming up with said
patches). To borrow some words:
"Talk is cheap. Show me the code." ― Linus Torvalds.
If I give you feedback suggesting that maybe we should reorganize this
thing to split out refactorings from behavior changes I'm asking you to
do extra work. Ultimately neither I, you nor anyone else can really know
if such a proposed effort is going to be better until it happens.
When you get feedback in the form of restructured patches we skip past
all that. We know it compiles, passes tests, and we can more concretely
evaluate the proposal with diff, range-diff etc.
> It was my impression that the existing config-based-hooks topic
> was close to being ready to submit anyway (since Junio mentioned
> submitting it a couple revisions ago); rather than churning by reviewing
> a different 31-patch topic, and then re-rolling and re-reviewing a
> (reduced) config hook topic, wouldn't it be easier on everyone's time to
> do a final incremental review on the existing topic and then start in on
> bugfixes/feature patches afterwards?
I think it's fair to say that nobody's read your code in its current
state more thoroughly than I have at this point, but still, going back
to it and paging through it even now a couple of days after reading the
whole thing line-by-line I find myself getting lost again.
That's because the whole structure of it is to conflate changing
existing behavior with the introduction of new behavior. This makes
things *much harder* for reviewers, because they need to be on toes
about regressions *and* evaluating the function/viability/sanity of new
proposed semantics.
I very much would like to see some approximation (sans my outstanding
comments, more on that below) of your topic land sooner than later.
I think this approach gets us there faster, not slower. How long it
takes to review something isn't about the number of patches, it can be
harder to review a 10 patch series than a 100 patch series. It's mainly
about structuring things in such a way as to make it readable to other
people.
If you've followed any of my own topics you'll probably correctly form
the opinion that that the last few paragraphs at best amount to throwing
some rather large pebbles from a glass house, which you'd be right about
:)
It's can be really hard to see how/where to split things when you're the
author of the code. It's really hard in the "theory of mind" sense of
things to explain an idea to someone who doesn't have the information
you have.
Still, I do think there's a near-universal rules of thumb that the
structure of your series thoroughly runs afoul of, i.e. changing
existing behavior at the same time as introducing new behavior. We
should split such changes up whenever possible. This alternate approach
shows that's it's possible.
> their would have been nice to see a more clear discussion of patch
> organization sometime much sooner in the past year and a half since the
> project was proposed[3], like maybe in the few iterations of the design
> doc which included a rollout plan in July of last year[4]. To me, it
> seems late to be overhauling the direction like this, especially after I
> asked for opinions and approval on the direction before I started work
> in earnest.
We've had some version of this series going back to at least May last
year, and none of it has landed on "master" or "next" yet. To me that's
more reason, not less, for it benefiting from a more incremental
approach.
But yes, I also wish I'd submitted this much earlier. Sorry I didn't
have time to review in this detail until now. I think Felipe's
downthread "never too late" comments apply here though[Æ.1]
I've had outstanding significant feedback on the
"function/viability/sanity of new proposed semantics" part of this
almost 3 months ago that you still haven't addressed in any way[Æ.2]
except this comment[Æ.3] around a month ago on v8 (which I took to mean
that you would). So the lateness of us discussing this is at least
partially a two-way street.
There was also the "why do we need strbuf here?" feedback[Æ.4] around the
same time from me, and as shown in the diff between our two versions
things like your run_hooks_opt_init_async() being better done as a
"struct ... = *_INIT" idiom.
Small issues, but especially the latter to me suggests that your v9 is
too big a chunk for reviewers to consider. I daresay if this was a
smaller series there's no way that wouldn't have been pointed out
already, ditto your "git hook run" introducing an "--env" which never
gets used etc.
> Anyway, I'd personally rather spend effort getting the existing series
> the last few yards to the finish line than to head most of the way back
> to the start.
I think I've made a good argument above for why this takes you a step
forward, not backwards, I'm hoping despite this initial reply that
you'll come to agree on that.
In any case, writing code is hard, but splitting it up like I've done
here is rather easily done. It took me about a day with waiting for
"rebase -i --exec='make test'" equivalent, and that's from being mostly
unfamiliar with the code in question beforehand.
If this alternate series were to go in ahead of yours that doesn't land
you back on square one, you still have a version you could relatively
easily rebase on top. The patches here are mostly smaller versions (no
hook config changes) of corresponding patches of yours.
If anything I don't see how it doesn't make things much easier for you,
if you do get around to replying to my outstanding feedback in [Æ.1]
that'll involve rebasing/commit message rewording of patches that now
conflate significant refactoring and behavior changes.
You'll instead cleanly have just the behavior changes, which at that
point will be both easier to justify in a briefer commit message, and
easier for others to review.
>> 1. https://lore.kernel.org/git/87lf80l1m6.fsf@xxxxxxxxxxxxxxxxxxx/
>> 2. https://lore.kernel.org/git/20210527000856.695702-1-emilyshaffer@xxxxxxxxxx/
> 3. https://lore.kernel.org/git/20191116011125.GG22855@xxxxxxxxxx/
> 4. https://lore.kernel.org/git/20200728222455.3023400-1-emilyshaffer@xxxxxxxxxx/
Æ.1. https://lore.kernel.org/git/60b71788c0e6d_67d0208d4@natae.notmuch/
Æ.2. https://lore.kernel.org/git/87mtv8fww3.fsf@xxxxxxxxxxxxxxxxxxx/
Æ.3. https://lore.kernel.org/git/YJBdbi50Hz+ekOtt@xxxxxxxxxx/
Æ.4. https://lore.kernel.org/git/87pn04g0r1.fsf@xxxxxxxxxxxxxxxxxxx/
