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

Tue Sep 9 19:13:47 UTC 2014

hi Guys,
Sorry for all the confusion generated from sending out separate patches for the review comments. The reasons were the following:
1. I wanted to address all the review comments in the limited time I had, and unblock others.
2. I thought it would be easier to see that the review comments were addressed if I created small atomic patches for it. Creating small and atomic patches for a specific concrete item is standard practice.

Anyway, since Ankur has done the due diligence (Thanks Ankur!) of readying the patches and getting them checked in, there may not be much to discuss here. In the future, I'd sincerely request some slack in such cases where there was no correctness issue as such. I had addressed all the review comments.

To address Samuel's comments:
> 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.

Samuel,
I am just quoting the guidelines in the CONTRIBUTING document:

----
 10 Before You Start
 11 ----------------
 12 
 13 Before you send patches at all, make sure that each patch makes sense.
 14 In particular:
 15 
 16         - A given patch should not break anything, even if later
 17           patches fix the problems that it causes.  The source tree
 18           should still build and work after each patch is applied.
 19           (This enables "git bisect" to work best.)
 20 
 21         - A patch should make one logical change.  Don't make
 22           multiple, logically unconnected changes to disparate
 23           subsystems in a single patch.
----

This convention seems to have worked over the years, and we should stick to the same, IMHO. As to what contributes to a individual patches in a series can be figured out as we go. Reviewers will have suggestions too. In general:
1. Make it easier to review (numero uno to get attention in the community!)
2. Need to have to do a lot of refactoring. Eg. you can write a stub function in a commit, and implement the stub later in the series. If you implement version #1 of a function, and completely rewrite the function later in the series, obviously there should be a better way to create the series.

thanks,
Nithin

On Sep 6, 2014, at 5:18 PM, Samuel Ghinet <sghinet at cloudbasesolutions.com>
 wrote:

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