Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 30, 2014, 21:15 UTC
Message-ID: <CAPig+cS0RYgmMzhkgMj=G939DADNG29dPQP2BQJQxVYS5d6ThQ@mail.gmail.com>
In-Reply-To: <CAGZ79kYds-oo-Q0=R6ny=CpZ3ja0__6CDm-wmamnSp3TBWcKdQ@mail.gmail.com>

On Tue, Dec 30, 2014 at 3:33 PM, Stefan Beller <sbeller@google.com> wrote:

Show 25 quoted lines

> On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@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.

Show 36 quoted lines

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