[ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

[Samuel Ghinet]

Hello guys,

I'm sorry I could not reply earlier.
What has been pushed in the mean time looks pretty ok.

Regarding my "Not acked-by": Generally speaking, I am not very fixed on details. And even if some tiny detail I do not like, if the others like it, then it's ok by me. I simply prefer that someone else does not say the ok for me, e.g. when I might have not seen the last version.

> I have talked to ben and he is fine with the approach of handling the review comment in another patch in the same series
I personally don't think it's a "one size fits all" thing.

For specific cases, IMHO: 
a) I personally see a bloating of patches if we put a series of patches in which the last rewrite stuff on the former as refactoring or documentation, when they could have been put in the first patch and reduce the number of patches sent out.

b) A split of patches seems a good idea to me when it would help code review, by splitting the kinds of modification done in code (e.g. the modification would increase size of existing functions, while a new patch does the refactor).

c) If, later in the future, some refactor code or documentation is added, such a thing can go very well with "review comment in another patch" aforementioned.

Regards,
Sam

________________________________________
From: Ankur Sharma [ankursharma at vmware.com]
Sent: Saturday, August 30, 2014 2:04 AM
To: Ben Pfaff
Cc: Eitan Eliahu; Samuel Ghinet; dev at openvswitch.org; Nithin Raju
Subject: RE: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

Hi,

I have sent a v4 for review.
All the code is from nithin only. I have only merged the patches and rebased it.

a. Since i am NOT the author and did review the changes, hence i have kept my name in Acked-by.
b. As per the CONTRIBUTING guidelines i have added my name to Signed-off-by (because i am submitting on nithin's behalf).


Hi Sam,
As per your request i have removed your name from Acked-by.

As i understand following are the review comments which you have mentioned are not addressed in this patch:

1. "my suggestion was to use two separate variables, instead of an array, each to have a specific name."
2. "I had suggested you specify the #define-s in Datapath.c as comments"
3. "when I suggest you add some doc comments, it is possible I do not find the documentation clear enough, or complete"
4. "I had given some suggestions, which you agreed upon, but you did not apply them to this patch" <-- can you please let me know the suggestion which were missed out.

kindly let us know If you think that patch should not be applied unless above review comments (all or some) are addressed. If you are ok for patch to be applied then i'll leave the discussion on above comments b/w you and nithin (once he is back).


Thanks.


Regards,
Ankur
________________________________________
From: Ankur Sharma
Sent: Friday, August 29, 2014 1:38 PM
To: Ben Pfaff
Cc: Eitan Eliahu; Samuel Ghinet; dev at openvswitch.org; Nithin Raju
Subject: RE: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

Hi Ben,

Thank you so much for the reply.
No problem @ all, i can merge the review comments in original series of 4 patches.

I'll send a v4 soon.

Regards,
Ankur
________________________________________
From: Ben Pfaff <blp at nicira.com>
Sent: Friday, August 29, 2014 1:30 PM
To: Ankur Sharma
Cc: Eitan Eliahu; Samuel Ghinet; dev at openvswitch.org; Nithin Raju
Subject: Re: [ovs-dev] [PATCH 9/9 v2] datapath-windows: refactor code to setup dump start state

On Fri, Aug 29, 2014 at 07:11:41PM +0000, Ankur Sharma wrote:
> I have talked to ben and he is fine with the approach of handling
> the review comment in another patch in the same series. But yes
> ideally we should try to keep the review comment fix in the same
> patch.

Is this just a matter of squashing some patches together?  i.e. can
you just run "git rebase -i origin/master" to fix up the issues?  If
so then I'd also prefer to see the comments handled as part of the
original patch.

Basically, is it for some reason *difficult* to fix up the original
patch?  I doubt it; many of the OVS contributors do this routinely.