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

From: Stefan Beller <sbeller@google.com>
Date: Jan 5, 2015, 18:02 UTC
Message-ID: <CAGZ79ka8TMvF1s=ZL=4Lj1EaDrLVn8HRA2PR4JLAHWasHmvkFA@mail.gmail.com>
In-Reply-To: <CACsJy8DGS2KN5Vd=UY2nUBH1Nc9rMiTt84HNB9K=8z2H0m4zVg@mail.gmail.com>

On Sat, Jan 3, 2015 at 1:53 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 13 quoted lines

> On Sat, Jan 3, 2015 at 9:20 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> -     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))

The intention of that patch is to move the code unrelated to executing commands out of execute_commands. And I feel this error checking is not the core task of executing commands, hence it should be moved out completely. Having one part in warn_if_skipped_connectivity_check and then the other error part triggered by an unsuccessful return doesn't improve the situation IMHO.

I think about moving the check for shallow_update inside that function and to have a

        if (!shallow_update)
                return;

and additionally renaming the function to be more precise:

        assure_shallow_connectivity_checked();

These changes I can put into this patch.

Replacing error by a die command will go in an extra patch on top. So I am understanding your answers such that we definitely want to prevent a push if this future bug happen. Instead of incorporating that into later patches of the series to abort in case of this possible bug, we could just go with s/error/die/ and the problem is solved.

Show 8 quoted lines

>>                 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?
>
> That code is to prevent bugs in future. The whole operation is spread
> out in many functions/steps and people may overlook.
>

Then this patch also helps with introducing a dedicated function to assure the connectivity which we can reuse maybe.

Thanks, Stefan