Re: [PATCHv9 1/9] receive-pack.c: shorten the execute_commands loop over all commands

>> -     if (shallow_update && !checked_connectivity)
>> -             error("BUG: run 'git fsck' for safety.\n"
>> -                   "If there are errors, try to remove "
>> -                   "the reported refs above");
>> +     if (shallow_update)
>> +             check_shallow_bugs(commands, si);
>
> In the same spirit of saving the reader from having to look at the
> body of check_shallow_bugs, would it make sense for the part that reacts
> to an error to be kept in the caller?  E.g.:
>
>         if (shallow_update && warn_if_skipped_connectivity_check(commands, si))
>                 error("BUG: run 'git fsck for safety.\n"
>                       "If there are errors, try removing the refs reported above");
>
> Is this error possible, by the way?

> update() does not return success
> unless it has reached the bottom block in the function.  In the
> !is_null_sha1(new_sha1) case that means it calls update_shallow_ref(),
> which performs the connectivity check.  In the is_null_sha1(new_sha1)
> case, update_shallow_info() does not set cmd->index and
> si->shallow_ref[cmd->index] cannot be set.
>
> Perhaps this error message could be written in a way that makes it
> clearer that we really expect it not to happen, like
>
>                 die("BUG: connectivity check skipped in shallow push???");
>
> (die() instead of error() to prevent refs from updating and pointing
> to a disconnected history).