Re: [PATCH v8 1/7] refs.c: add err arguments to reflog functions

[Michael Haggerty <mhagger@alum.mit.edu>]

On 07/09/2015 03:50 PM, David Turner wrote:
> Add an err argument to log_ref_setup that can explain the reason
> for a failure. This then eliminates the need to manage errno through
> this function since we can just add strerror(errno) to the err string
> when meaningful. No callers relied on errno from this function for
> anything else than the error message.
> 
> Also add err arguments to private functions write_ref_to_lockfile,
> log_ref_write_1, commit_ref_update. This again eliminates the need to
> manage errno in these functions.
> 
> Some error messages are slightly reordered.
> 
> Update of a patch by Ronnie Sahlberg.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  builtin/checkout.c |   8 ++--
>  refs.c             | 127 +++++++++++++++++++++++++++++++----------------------
>  refs.h             |   4 +-
>  3 files changed, 81 insertions(+), 58 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index c018ab3..93f63d3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -624,16 +624,18 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  				struct strbuf log_file = STRBUF_INIT;
>  				int ret;
>  				const char *ref_name;
> +				struct strbuf err = STRBUF_INIT;
>  
>  				ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
>  				temp = log_all_ref_updates;
>  				log_all_ref_updates = 1;
> -				ret = log_ref_setup(ref_name, &log_file);
> +				ret = log_ref_setup(ref_name, &log_file, &err);
>  				log_all_ref_updates = temp;
>  				strbuf_release(&log_file);
>  				if (ret) {
> -					fprintf(stderr, _("Can not do reflog for '%s'\n"),
> -					    opts->new_orphan_branch);
> +					fprintf(stderr, _("Can not do reflog for '%s'. %s\n"),
> +						opts->new_orphan_branch, err.buf);

Our usual pattern for chaining error messages is

    $problem: $reason

In this patch (and maybe later patches too? I haven't checked yet) I see

    $problem. $reason

and also

    $problem. '$reason'

I think it would be good to use the first pattern consistently.

> +					strbuf_release(&err);
>  					return;
>  				}
>  			}
> diff --git a/refs.c b/refs.c
> index fb568d7..03e7505 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -3247,25 +3247,28 @@ int is_branch(const char *refname)
>  
>  /*
>   * Write sha1 into the open lockfile, then close the lockfile. On
> - * errors, rollback the lockfile and set errno to reflect the problem.
> + * errors, rollback the lockfile, fill in *err and
> + * return -1.
>   */
>  static int write_ref_to_lockfile(struct ref_lock *lock,
> -				 const unsigned char *sha1)
> +				 const unsigned char *sha1, struct strbuf *err)
>  {
>  	static char term = '\n';
>  	struct object *o;
>  
>  	o = parse_object(sha1);
>  	if (!o) {
> -		error("Trying to write ref %s with nonexistent object %s",
> -			lock->ref_name, sha1_to_hex(sha1));
> +		strbuf_addf(err,
> +			    "Trying to write ref %s with nonexistent object %s",
> +			    lock->ref_name, sha1_to_hex(sha1));
>  		unlock_ref(lock);
>  		errno = EINVAL;

Is it intentional that this function is still setting errno (here and a
few lines farther down)? I'm guessing that this is no longer needed,
though I haven't audited the callers.

>  		return -1;
>  	}
>  	if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
> -		error("Trying to write non-commit object %s to branch %s",
> -			sha1_to_hex(sha1), lock->ref_name);
> +		strbuf_addf(err,
> +			    "Trying to write non-commit object %s to branch %s",
> +			    sha1_to_hex(sha1), lock->ref_name);
>  		unlock_ref(lock);
>  		errno = EINVAL;
>  		return -1;
> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu