Re: [PATCHv10 02/10] receive-pack.c: die instead of error in assure_connectivity_checked

From: Stefan Beller <sbeller@google.com>
Date: Jan 5, 2015, 21:15 UTC
Message-ID: <CAGZ79kZUiwEbcSPk3Td60umixvh_Q7jXTGoLemKLYsvX1ty39w@mail.gmail.com>
In-Reply-To: <20150105201728.GK29365@google.com>

On Mon, Jan 5, 2015 at 12:17 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 17 quoted lines

> Stefan Beller wrote:
>
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1055,15 +1055,15 @@ static void assure_connectivity_checked(struct command *commands,
>>
>>       for (cmd = commands; cmd; cmd = cmd->next) {
>>               if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
>> -                     error("BUG: connectivity check has not been run on ref %s",
>> -                           cmd->ref_name);
>> +                     die("BUG: connectivity check has not been run on ref %s",
>> +                         cmd->ref_name);
>
> This could stay as error() so that the caller gets to see the list of
> refs that didn't experience a connectivity check.  Or if that list
> isn't important, this error/die call could be dropped and the
> 'checked_connectivity = 0' setting would be enough.

Right. I was once again writing patches without brain activity. So we'd keep this as an error.

Show 18 quoted lines

>
>>                       checked_connectivity = 0;
>>               }
>>       }
>>       if (!checked_connectivity)
>> -             error("BUG: run 'git fsck' for safety.\n"
>> -                   "If there are errors, try to remove "
>> -                   "the reported refs above");
>> +             die("BUG: run 'git fsck' for safety.\n"
>> +                 "If there are errors, try to remove "
>> +                 "the reported refs above");
>
> I find this error message misleading and confusing.  It makes it seem
> like this is an expected error that we are trying to help the user
> recover from.  Instead, something impossible has happened.  "Try to
> remove the reported refs" is actively harmful advice --- that would be
> a way for the user to work around a serious bug instead of figuring
> out what went wrong and getting git fixed.

Maybe we should do both?

    die ("BUG: Some refs have not been checked for connectivity."
          "Please contact the git developers (git@vger.kernel.org) and"
          "report the problem. As a workaround run 'git fsck'. If there"
          "are errors, try to remove the reported refs above. (This "
          "may lead to data loss, backup first.)"

Just thinking out loud: We are using die(...) for two kinds of problems. Either because of some bad condition given to us by the user (wrong/meaningless arguments) which we then to refuse to accept.

The other case is usually die("BUG: Git is broken in some way") as we're discussing here. Would it make sense to have an extra die_bug function, which amends the reported error string by something like

    "Please contact the git developers (git@vger.kernel.org) and
report the problem."

as above?

Thanks, Stefan

>
> Jonathan