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:
>> No functional changes intended.
>
> This is useful to know but is of secondary importance, thus should be
> placed after the explanation and justification of the change.
>
>> Subject: receive-pack.c: simplify execute_commands
>> This commit shortens execute_commands by moving some parts of the code
>> to extra functions.
>
> The _real_ reason for moving these bits of code into their own
> functions is that you intend to introduce additional callers in
> subsequent patches. That is what the commit message (including
> subject) should be conveying to the reader.
>
> The claimed simplification is questionable and not of particular
> importance; and it's misleading to paint it as a goal of the patch.
> Consequently, you could drop mention of it altogether.
>
> More below.
>
>> Suggested-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 4e8eaf7..06eb287 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
>> }
>> }
>>
>> +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.
>
>> +}
>> +
>> +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.
>
> Also, since nothing happens between them, the two conditionals inside
> the loop can be coalesced:
>
> if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
>
>> + error("BUG: run 'git fsck' for safety.\n"
>> + "If there are errors, try to remove "
>> + "the reported refs above");
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.
>
> In v6, you considered this a fatal error in the atomic case, which
> caused the entire transaction to be rolled back. However, in this
> version, this error has no effect whatsoever on the atomic
> transaction, which is a rather significant behavioral departure. Which
> is correct? (This is a genuine question; not at all rhetorical.) If
> failing the entire transaction is the correct thing to do, then this
> is going to need some more work.
>
>> +}
>> +
>> static void execute_commands(struct command *commands,
>> const char *unpacker_error,
>> struct shallow_info *si)
>> {
>> - int checked_connectivity;
>> struct command *cmd;
>> unsigned char sha1[20];
>> struct iterate_data data;
>> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
>> free(head_name_to_free);
>> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>>
>> - checked_connectivity = 1;
>> for (cmd = commands; cmd; cmd = cmd->next) {
>> - if (cmd->error_string)
>> - continue;
>> -
>> - if (cmd->skip_update)
>> + if (!should_process_cmd(cmd))
>> continue;
>>
>> cmd->error_string = update(cmd, si);
>> - if (shallow_update && !cmd->error_string &&
>> - si->shallow_ref[cmd->index]) {
>> - error("BUG: connectivity check has not been run on ref %s",
>> - cmd->ref_name);
>> - checked_connectivity = 0;
>> - }
>> }
>> -
>> - if (shallow_update && !checked_connectivity)
>> - error("BUG: run 'git fsck' for safety.\n"
>> - "If there are errors, try to remove "
>> - "the reported refs above");
>> + check_shallow_bugs(commands, si);
>> }
>>
>> static struct command **queue_command(struct command **tail,
>> --
>> 2.2.1.62.g3f15098
--
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