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 <sahlb...@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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
