Message-ID: <c4ab4238-1494-4c42-8650-6662eafd1b3b@intel.com>
Date: Thu, 5 Dec 2024 13:26:26 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <vladimir.oltean@....com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>
CC: Andrew Morton <akpm@...ux-foundation.org>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Masahiro
 Yamada" <masahiroy@...nel.org>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and
 unpack_fields()



On 12/5/2024 1:52 AM, Vladimir Oltean wrote:
> On Thu, Dec 05, 2024 at 12:43:35AM +0100, Przemek Kitszel wrote:
>> On 12/4/24 18:12, Vladimir Oltean wrote:
>>> On Tue, Dec 03, 2024 at 03:53:49PM -0800, Jacob Keller wrote:
>>
>> Amazing stuff :), I really like the fact that you both keep striving for
>> the best result, even way past the point of cut off of most other ;)
> 
> It's all Jake. I openly admit I would have given up and not followed
> through with the review feedback to go to modpost and back.
> Additionally, the __builtin_choose_expr() breakthrough was all his.
> 
> Jake's determination, perseverence, discipline and level of skill are
> something to aspire to. It's safe to say that without him, this set
> would have gotten nowhere.
> 

Thanks :) I'm happy to finally arrive at a version I am quite happy
with. It certainly took longer than I had planned, but I believe the end
result is significant improvement and code which hopefully many other
drivers can adopt.

>> prior to the change CHECK_PACKED_FIELD() was called on values smaller
>> than MAX_PACKED_FIELD_SIZE, compare with [j] above, now you call it also
>> for the MAX one
> (...)
>>
>> off by one error? see above
> 
> Yes, indeed, thank you for pointing this out. Jake also replied a few
> minutes prior to your message.
> 
>> PS. incremental diff in a single patch is harder to apply, but easier to
>> review, comment both in a single reply == great idea
> 
> I'm interpreting this as a positive comment :) I got mixed feedback
> about posting diffs in reply to patches, it seems to confuse b4 when
> applying.

Yea, b4 am / b4 shazam don't seem to handle inline diffs like this very
well. However, I've made good use of:

$ b4 mbox --single-message <message-id>

and then editing the file to extract the diffs myself.

I think there may be some possibility to use the scissors lines as a way
to identify the diffs, but I don't know if b4 has this builtin. See the
section from git mailinfo:

>        --scissors
>            Remove everything in body before a scissors line (e.g. "-- >8 --"). The line represents scissors and perforation marks, and is used to request
>            the reader to cut the message at that line. If that line appears in the body of the message before the patch, everything before it (including the
>            scissors line itself) is ignored when this option is used.
> 
>            This is useful if you want to begin your message in a discussion thread with comments and suggestions on the message you are responding to, and
>            to conclude it with a patch submission, separating the discussion and the beginning of the proposed commit log message with a scissors line.
> 
>            This can be enabled by default with the configuration option mailinfo.scissors.
> 

Perhaps some way of marking diffs with the -- >8 -- lines could be used
as convenient markers for where a diff in a message is, vs regular text.

Powered by blists - more mailing lists