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

On Thu, May 15, 2014 at 11:38 AM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

--- 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?

Unconditionally suppress the message I think.  And is what I did.

[...]

quoted

@@ -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).

I changed it so that if err is non-NULL we call a new function that updates the
strbuf with a nice message explaining the failure and then return 1
without writing anything to stderr.

For the NULL case I left it to continue logging two lines of errors
"Unable to create '%s.lock': %s.\n\n"
"cannot delete '%s' from packed refs"

That second line should probably go away since the error at this stage
is related to locking the packed
refs file itself and has nothing really to do with a particular ref
such as refnames[i]

[...]

quoted

@@ -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).

Done and Done.

Thanks.