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

[Danilo Krummrich <dakr@redhat.com>]

On Mon, May 06, 2024 at 11:33:31PM -0300, Wedson Almeida Filho 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.
> 
> > If you are in the position as a code maintainer that you say "do
> > whatever, I'll just fix it up myself later", it comes across as
> > passive aggressive, whereas you could just have said that in the
> > initial review,  "change this to an if for consistent style" or
> > because of maintainer preference it leaves out any ambiguity that you
> > want to be see the change in order to merge it. In my experience most
> > contributors will just make changes and move on, and are happy with
> > less ambiguity.
> 
> The latest exchange didn't lead me to believe this contributor "would

While I also don't agree with some other points in this context, I only want to
focus on the following two aspects.

I don't appreciate being referred to as "this contributor". If you want to refer
to me, please do so by using my name. Doing otherwise feels like a gesture of
disrespect to me and hence doesn't provide a foundation for working together.

> 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.
> 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

I also don't appreciate spreading false information here.

Please note that I was only sharing that my experience has been that people
typically do care whether their own patch or the code of their patch squashed
into an existing one is landing.

I did not say that I personally insist on the former. I even explicitly said the
opposite:

"Even though I wouldn't mind personally, my experience has been that people do
care about the difference." [1]

[1] https://lore.kernel.org/rust-for-linux/ZjEfW6QpErDDnntk@pollux/

> 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? :)
> 
> Cheers,
> -Wedson
>