Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs

On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder [off-list ref] wrote:

Hi,

Comments from http://marc.info/?l=git&m=140079653930751&w=2:

Ronnie Sahlberg wrote:

quoted

--- 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);

"unable_to_lock_strbuf" could be called "unable_to_lock_message" (which
would make its behavior more obvious IMHO).

Renamed.  Thanks.

[...]

quoted

--- 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 = 0;

This is about making errno meaningful when commit_packed_refs returns
an error.  Probably its API documentation should say so to make it
obvious when people modify it in the future that they should preserve
that property or audit callers.

[...]

quoted

@@ -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;
+             }
              unable_to_lock_error(git_path("packed-refs"), errno);

Via the new call to unable_to_lock_..., repack_without_refs cares
about errno after a failed call to lock_packed_refs.  lock_packed_refs
can only fail in hold_lock_file_for_update.  hold_lock_file_for_update
is a thin wrapper around lockfile.c::lock_file.  lock_file can error
out because

        strlen(path) >= max_path_len
        adjust_shared_perm failed [calls error(), clobbers errno]
        open failed

So lock_file needs a similar kind of fix, and it's probably worth
updating API documentation for these calls to make it clear that their
errno is used (though that's not a new problem since
repack_without_refs already called unable_to_lock_error).  Could be a
separate, earlier patch (or a TODO comment in this patch to be
addressed with a later patch) since it's fixing an existing bug.

I will add it to my todo list.
I think passing of errno around is too fragile and that we should
avoid ad-hoc save_errno hacks and implement dedicated return codes to
replace errno.
We should only inspect errno immediately after a syscall has failed.

Hope that helps,
Jonathan