On Tue, Dec 30, 2014 at 3:33 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html