Re: [PATCH 05/12] mkfs: extend opt_params with a value field

[Eric Sandeen <sandeen@sandeen.net>]

On 4/25/17 8:10 PM, Luis R. Rodriguez wrote:
> On Tue, Apr 25, 2017 at 10:04:39AM +0200, Jan Tulak wrote:
>> On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote:
>>>
>>>> and anything you write here now is
>>>> + *     overwritten if user specifies the subopt. (If the user input is a string
>>>> + *     and not a number, this value is set to a positive non-zero number.)
>>>> + *
>>>>   * !!! NOTE ==================================================================
>>>>   *
>>>>   * If you are adding a new option, or changing an existing one,
>>>> @@ -152,6 +158,7 @@ struct opt_params {
>>>>               uint64_t        maxval;
>>>>               uint64_t        flagval;
>>>>               const char      *raw_input;
>>>> +             uint64_t        value;
>>>>       }               subopt_params[MAX_SUBOPTS];
>>>>  } opts[MAX_OPTS] = {
>>>>  #define OPT_B        0
>>>
>>> It would seem rather unfair to define this this but not use it in the patch ?
>>
>> I'm still trying to find out when exactly should I make something two
>> patches and when one, and it seems to shift case by case... I tried to
>> separate the adding of new things and rewriting of the existing code,
>> but do you think, in this case, the new thing is too trivial to have a
>> standalone patch?
> 
> Ah, I see, this was split out to be separate given a slew of subopts in turn
> later use it. The nasty alternative for review would be to have all subopts
> converted in one shot. I suppose a middle ground would be to have this patch
> squashed with just one of the subopt users, and then each other subopt ported
> atomically. That would make the addition of the value and definition in effect
> as soon as its added.
> 
> Just my 0.02 CRC but its up to Eric as this is rather subjective and tree dependent.

I think the way Jan did this is fine.  Ideally the commit would explain a bit more,
i.e. "This patch simply adds the value field.  Subsequent patches will utilize
this new field."

Then the reviewers know what to expect from the start, and we avoid the back and
forth of figuring out why it was done as it was done.  ;)

However, I think 5 & 6 could easily be combined - add the field and the routines
to manipulate it, /then/ start adding users of that new infrastructure.

I haven't really reviewed from here on out yet, but from a "how should I break
up patches" point of view, that's my ... $0.02.  :)

-Eric

> 
>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>