[ovs-dev] [PATCH] Create a NBL for each NB when required

[Saurabh Shah]

> > This function has become a bit too long now, can you please refactor it?
> > One obvious thing you could do is to extract the processing of an
> > single NB into a separate function and the parent function can take
> > care of splitting the NBL (if required) & creating the right forwarding ctx.
> 
> I took it as a "function too big" issue only.
> Also, a code like this (as in OvsStartNBLIngress):
> [CODE]
> if (cond) {
>     line1;
>     line2;
>     line;
> }
> [/CODE]
> refactored to:
> [CODE]
> line1;
> line2;
> line;
> [/CODE]
> In an ordinary git diff in CLI, this becomes difficult to track, because it's seen
> as removal of all lines above and add of new lines (all below). But with a good
> GUI git diff tool, it is seen very clear as "removed indent and associated if()"
> 
> My intent here was to split refactor from actual bug fix, in order to improve
> reviewing - i.e. the bug fix should modify less of code, while refactor should
> keep the same functionality but present it differently. I had thought that
> putting both in one patch would make the code more difficult to review.
> Also, splitting the commits could help by having divided work: "ok, patch1 is
> ok, I'll review patch2 later".

I don't have any problems with dividing work. In fact, it would be a good practice since it keeps the reviews smaller, easier and as a result less error prone.

The point that I was trying to make for this specific review was  -
_do_
refactor_fun0
refactor_fun1
fix_bug 
 
_instead_of_
fix_bug
refactor_fun0 
refactor_fun1

In the first scenario, the refactors are a precursor to the bug fix change, which is the immediate beneficiary of the refactoring. The resulting bug fix is easier to review (IMO) & is there to stay. Doing it the reverse way (second scenario) means that the reviewer needs to review an awkwardly long function call. And to add to the woes, the function itself will immediately be broken up by the refactoring that follows & hence the logic will need to be revisited any ways. All this can be avoided easily.

By the way, thanks for posting a new revision of the patch, I will take a look at it sometime tomorrow.

Thanks,
Saurabh