On Mon, Jan 5, 2015 at 12:17 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
>
>> 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@xxxxxxxxxxxxxxx) 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@xxxxxxxxxxxxxxx) and
report the problem."
as above?
Thanks,
Stefan
>
> Jonathan
--
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