Re: [PATCH v8 04/44] refs.c: add an err argument to repack_without_refs

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:
> --- a/refs.c
> +++ b/refs.c
> @@ -2427,12 +2427,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)

Should this also get an onerr flag to suppress the message to stderr,
or unconditionally suppress the message to stderr when err != NULL?

[...]
> @@ -2445,6 +2445,9 @@ static int repack_without_refs(const char **refnames, int n)
>  
>  	if (lock_packed_refs(0)) {
>  		unable_to_lock_error(git_path("packed-refs"), errno);
> +		if (err)
> +			strbuf_addf(err, "cannot delete '%s' from packed refs",
> +				    refnames[i]);

unable_to_lock_error is able to come up with a message with more
detail (path so the sysadmin can hunt down the problem even if this
was run e.g. from a cronjob where the path is not obvious, errno
hinting at the nature of the problem).

[...]
> @@ -2470,12 +2473,15 @@ static int repack_without_refs(const char **refnames, int n)
>  	}
>  
>  	/* Write what remains */
> -	return commit_packed_refs();
> +	ret = commit_packed_refs();
> +	if (ret && err)
> +		strbuf_addf(err, "unable to overwrite old ref-pack file");

After commit_lock_file sets errno, amazingly no one clobbers it
until we get to this point.  The only calls in between are to
free().

It would be nice to make that more explicit in commit_packed_refs:

	int save_errno;

	...
	if (commit_lock_file(packed_ref_cache->lock)) {
		save_errno = errno;
		error = -1;
	}

	packed_ref_cache->lock = NULL;
	release_packed_ref_cache(packed_ref_cache);

	errno = save_errno;
	return error;

Even without that, this message could include strerror(errno).

> +	return ret;
>  }

Thanks,
Jonathan