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

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

On 05/16/2014 07:36 PM, Ronnie Sahlberg wrote:
> Update repack_without_refs to take an err argument and update it if there
> is a failure. Pass the err variable from ref_transaction_commit to this
> function so that callers can print a meaningful error message if _commit
> fails due to a problem in repack_without_refs.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  cache.h    |  2 ++
>  lockfile.c | 21 ++++++++++++---------
>  refs.c     | 25 +++++++++++++++++++------
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 8c6cdc2..48548d6 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -559,6 +559,8 @@ struct lock_file {
>  #define LOCK_DIE_ON_ERROR 1
>  #define LOCK_NODEREF 2
>  extern int unable_to_lock_error(const char *path, int err);
> +extern void unable_to_lock_strbuf(const char *path, int err,
> +				  struct strbuf *buf);
>  extern NORETURN void unable_to_lock_index_die(const char *path, int err);
>  extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
>  extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
> diff --git a/lockfile.c b/lockfile.c
> index 8fbcb6a..9e04b43 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  	return lk->fd;
>  }
>  
> -static char *unable_to_lock_message(const char *path, int err)
> +void unable_to_lock_strbuf(const char *path, int err, struct strbuf *buf)
>  {
> -	struct strbuf buf = STRBUF_INIT;
>  
>  	if (err == EEXIST) {
> -		strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
>  		    "If no other git process is currently running, this probably means a\n"
>  		    "git process crashed in this repository earlier. Make sure no other git\n"
>  		    "process is running and remove the file manually to continue.",
>  			    absolute_path(path), strerror(err));
>  	} else
> -		strbuf_addf(&buf, "Unable to create '%s.lock': %s",
> +		strbuf_addf(buf, "Unable to create '%s.lock': %s",
>  			    absolute_path(path), strerror(err));
> -	return strbuf_detach(&buf, NULL);
>  }
>  
>  int unable_to_lock_error(const char *path, int err)
>  {
> -	char *msg = unable_to_lock_message(path, err);
> -	error("%s", msg);
> -	free(msg);
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	unable_to_lock_strbuf(path, err, &buf);
> +	error("%s", buf.buf);
> +	strbuf_release(&buf);
>  	return -1;
>  }
>  
>  NORETURN void unable_to_lock_index_die(const char *path, int err)
>  {
> -	die("%s", unable_to_lock_message(path, err));
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	unable_to_lock_strbuf(path, err, &buf);
> +	die("%s", buf.buf);
>  }
>  
>  int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
> diff --git a/refs.c b/refs.c
> index 686b802..a470e51 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void)
>  	struct packed_ref_cache *packed_ref_cache =
>  		get_packed_ref_cache(&ref_cache);
>  	int error = 0;
> +	int save_errno;
>  
>  	if (!packed_ref_cache->lock)
>  		die("internal error: packed-refs not locked");
> @@ -2217,10 +2218,13 @@ int commit_packed_refs(void)
>  	do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
>  				 0, write_packed_entry_fn,
>  				 &packed_ref_cache->lock->fd);
> -	if (commit_lock_file(packed_ref_cache->lock))
> +	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;

This code involving save_errno looks like a bug fix orthogonal to the
rest of the patch.  It should at least be mentioned in the commit
message if not broken into a separate patch.

>  	return error;
>  }
>  
> @@ -2427,12 +2431,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)
>  {
>  	struct ref_dir *packed;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *ref_to_delete;
> -	int i, removed = 0;
> +	int i, ret, removed = 0;
>  
>  	/* Look for a packed ref */
>  	for (i = 0; i < n; i++)
> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n)
>  		return 0; /* no refname exists in packed refs */
>  
>  	if (lock_packed_refs(0)) {
> +		if (err) {
> +			unable_to_lock_strbuf(git_path("packed-refs"), errno,
> +					      err);
> +			return 1;

error() returns -1, but here you have changed the return value to 1.  Is
there a reason for the change?

> +		}
>  		unable_to_lock_error(git_path("packed-refs"), errno);
>  		return error("cannot delete '%s' from packed refs", refnames[i]);
>  	}
> @@ -2470,12 +2479,16 @@ 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: %s",
> +			    strerror(errno));
> +	return ret;
>  }
>  
>  static int repack_without_ref(const char *refname)
>  {
> -	return repack_without_refs(&refname, 1);
> +	return repack_without_refs(&refname, 1, NULL);
>  }
>  
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
> @@ -3481,7 +3494,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		}
>  	}
>  
> -	ret |= repack_without_refs(delnames, delnum);
> +	ret |= repack_without_refs(delnames, delnum, err);
>  	for (i = 0; i < delnum; i++)
>  		unlink_or_warn(git_path("logs/%s", delnames[i]));
>  	clear_loose_ref_cache(&ref_cache);
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/