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

From: Jonathan Nieder <[email-masked]>
Date: Mon, 5 Jan 2015 12:17:29 -0800

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.

>  			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.

Jonathan