Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

From: Jonathan Nieder <jrnieder@gmail.com>
To: Ronnie Sahlberg <sahlberg@google.com>
Cc: git@vger.kernel.org, mhagger@alum.mit.edu
Subject: Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref
Date: Wed, 14 May 2014 15:08:31 -0700	[thread overview]
Message-ID: <20140514220831.GB9218@google.com> (raw)
In-Reply-To: <1398976662-6962-6-git-send-email-sahlberg@google.com>

Hi,

Ronnie Sahlberg wrote:

> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
[...]
> @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
[...]
>  		if (delete || no_deref || argc > 0)
>  			usage_with_options(git_update_ref_usage, options);
>  		if (end_null)
>  			line_termination = '\0';
>  		update_refs_stdin();
> -		ret = ref_transaction_commit(transaction, msg, NULL,
> -					     UPDATE_REFS_DIE_ON_ERR);
> -		return ret;
> +		if (ref_transaction_commit(transaction, msg, &err,
> +					   UPDATE_REFS_QUIET_ON_ERR))
> +			die("%s", err.buf);

Nice.  I like this much more than passing a flag to each function to
tell it how to handle errors. :)

ref_transaction_commit didn't have any stray codepaths that return
some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
so this should be safe as far as the exit code is concerned.

The only danger would be that some codepath leaves 'err' alone and
forgets to write a messages, so we die with

	"fatal: "

Alas, it looks like this patch can do that.

 i. The call to update_ref_write can error out without updating the
    error string.

 ii. delete_ref_loose can print a message and then fail without updating
     the error string so the output looks like

	warning: unable to unlink .git/refs/heads/master.lock: Permission denied
	fatal:
	$

 iii. repack_without_refs can similarly return an error

	error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied
	error: cannot delete 'refs/heads/master' from packed refs
	fatal:
	$

 iv. commit_lock_file in commit_packed_refs is silent on error.
     repack_without_refs probably intends to write a message in that
     case but doesn't :(

I wish there were some way to automatically detect missed spots or
make them stand out (like with the current "return error()" idiom a
bare "return -1" stands out).

(i) is fixed by a later patch.  It would be better to put that before
this one for bisectability.

I don't see fixes to (ii), (iii), and (iv) in the series yet from a
quick glance.

Thanks,
Jonathan