On Tue, Dec 30, 2014 at 3:33 PM, Stefan Beller <sbel...@google.com> wrote: > On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunsh...@sunshineco.com> > wrote: >> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbel...@google.com> wrote: >>> +static int should_process_cmd(struct command *cmd) >>> +{ >>> + if (cmd->error_string) >>> + return 0; >>> + if (cmd->skip_update) >>> + return 0; >>> + return 1; >> >> Alternately, depending upon the polarity of your brain, you could >> collapse the entire function body to: >> >> return !cmd->error_string && !cmd->skip_update; >> >> or: >> >> return !(cmd->error_string || cmd->skip_update); > > I did not want to change the structure of the code when moving it around as > you noted in another email that helps on reviewing. > And having the exact same conditions in an if and then return instead > of continue > makes me think it is easier to review than if I also introduce that > shortening? > I can do so in a follow up.
It's a judgment call. When composing this review, I actually had a parenthetical comment here saying that collapsing the body of the function (as illustrated above) could be done as a patch separate from the code movement, but I removed the comment just before sending the email. This is such a simple change that the cognitive overhead from combining the code movement and simplification is so slight that it doesn't necessary warrant burdening reviewers with yet another patch. Therefore, although I very slightly favor splitting it into two patches, in this particular case, a single patch is not a great burden, so I don't feel strongly one way or the other. Use your best judgment. >>> +} >>> + >>> +static void check_shallow_bugs(struct command *commands, >>> + struct shallow_info *si) >>> +{ >>> + struct command *cmd; >>> + int checked_connectivity = 1; >>> + for (cmd = commands; cmd; cmd = cmd->next) { >>> + if (!should_process_cmd(cmd)) >>> + continue; >>> + >>> + if (shallow_update && si->shallow_ref[cmd->index]) { >> >> Here, inside the loop, you check 'shallow_update'... >> >>> + error("BUG: connectivity check has not been run on >>> ref %s", >>> + cmd->ref_name); >>> + checked_connectivity = 0; >>> + } >>> + } >>> + if (shallow_update && !checked_connectivity) >> >> And here, after the loop, you check 'shallow_update'. >> >> But, if you examine the overall logic, you will find that this >> function does _nothing_ at all when 'shallow_update' is false (other >> than uselessly looping through 'commands'). Therefore, either check >> 'shallow_update' just once at the beginning of the function and exit >> early if false, or have the caller check 'shallow_update' and only >> invoke check_shallow_bugs() if true. > > That's a good idea. I'll definitely add that. But as said above, I > also feel that this > should rather go in a follow up patch as it twists the logic and this > patch is all > about moving the code. Here, too, when composing the review, I had a parenthetical comment stating that this improvement to the code might warrant a followup patch after the code movement patch. However, this case is a bit different because the combined code in the new function, although not actively wrong, is uselessly iterating a do-nothing loop when 'shallow_update' is false, and even though not actively incorrect, it _feels_ weird to commit such pointless code, only to fix it up later. Due to that "weird" feeling, I didn't feel entirely comfortable recommending splitting the change into two patches, so I removed the comment before sending the email. Hence, this again is judgment call. Try to find a balance which feels right, while taking the review process into consideration. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html