Re: some aside maintainer advice (was Re: [PATCH v3] rust: alloc: fix dangling pointer in VecExt<T>::reserve())

[David Airlie <airlied@redhat.com>]

On Tue, May 7, 2024 at 12:33 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> On Mon, 6 May 2024 at 21:36, David Airlie <airlied@redhat.com> wrote:
> >
> > > On Mon, 6 May 2024 at 12:22, Danilo Krummrich <dakr@redhat.com> wrote:
> > > >
> > > > On 5/6/24 16:11, Wedson Almeida Filho wrote:
> [snip]
> > > > > I still think this should be an `if`.
> > > >
> > > > Is there any benefit using an if here, or is that your personal preference?
> > >
> > > Readability.
> > >
> > > I don't remember ever seeing anyone implement a != 0 comparison with a
> > > match or switch.
> > >
> > > But if you insist on innovating on what seems poor style to me, go
> > > ahead. I'll fix it later.
> >
> > I'd like to just respond with a bit of maintainer experience here,
> > please try and use less suggestive language and more requirements
> > language in reviews if you want to see something change.
>
> Thanks for the advice.
>
> I don't want to be intransigent though: if there's a real need for
> this to be a 'match', I of course wouldn't mind it (though I don't
> believe one exists).
>
> > "I still think this should be an 'if'" allows for disagreement and
> > discussion, but clearly in some cases you want to directly inform the
> > patch submitter of some maintainer requirement, I consider stylistic
> > preference one of those cases.
>
> By any chance, have you seen the thread that precedes this comment (in
> v2)? In case you haven't, check it out here:
> https://lore.kernel.org/rust-for-linux/ZjFq1uVXi4k1jjQc@pollux/
>
> Between sending this response and sending a v3 that completely ignores
> the discussion, he waited a grand total of 16 hours.

I think sending a v4 would not be a problem in the same timeframe
though, even for a one line change like that.

You used nit: prefix and asked a question, answering that question was
the objective of the question? if changing the patch was the objective
"nit: please change this match to an if" would prove a better idea,
asking a submitter a question and them replying while sending a v3
doesn't mean they ignored your questions, it just means they are
wanting more information before sending v4. Review questions on an
older version of the patch can still remain open while a new version
exists.

There is nothing against sending updated patches as quickly as
possible, and adding feedback from open questions in another round,
otherwise patch series could be stalled on "nit" for a long time.

> The latest exchange didn't lead me to believe this contributor "would
> just make changes and move on" (as you suggest above), so I had two
> options: reject the patch or accept it and change the style later.

You can just ask for a v4, turnaround time for patch generation
shouldn't be taking a long time here, if v3 was 16 hours why do you
think a v4 would have stalled out the process for long?

> Since I obviously want the fix and for him to be properly credited (as
> he deserves and insisted that it be by means of his own commit -- see
> the discussion in v1, of which I wasn't part), I chose the second
> option because Miguel and I want to finalize the changes for 6.10 now.
>
> > I'd suggest if we wanted to establish conventions and rules around if
> > vs match we should hash it out on zulip and update some docs
> > somewhere, or we can just leave it as is and have maintainers state
> > their requirements to avoid ambiguity.
>
> Zulip? :)

or in the RFL call, but somewhere outside the flow of a patch merge
that we've admitted has some time critical pieces to it.

Dave.