On Sat, May 17, 2014 at 5:40 AM, Michael Haggerty <mhag...@alum.mit.edu> wrote:
> 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.
Text updated.
>
>> 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?
Fixed.
>
>> + }
>> 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
