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

[Jonathan Nieder]

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.

 Add a new function unable_to_lock_message that takes a strbuf argument and
 fills in the reason for the failure.

 In commit_packed_refs, make sure that we propagate any errno that
 commit_lock_file might have set back to our caller.

 Make sure both commit_packed_refs and lock_file has errno set to a meaningful
 value on error. Update a whole bunch of other places to keep errno from
 beeing clobbered.

This patch is doing several (all nice) things.  Are they connected?
Would splitting some of them out into separate patches make sense or
would it be too much fuss to be worth the trouble?

[...]
 --- 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_message(const char *path, int err,
 +struct strbuf *buf);

Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.

[...]
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
   return p;
  }
  
 -
 +/* Make sure errno contains a meaningful value on error */
  static int lock_file(struct lock_file *lk, const char *path, int flags)

Making errno when returning from lock_file() meaningful, which should
fix

 * an existing almost-bug in lock_ref_sha1_basic where it assumes
   errno==ENOENT is meaningful and could waste some work on retries

 * an existing bug in repack_without_refs where it prints
   strerror(errno) and picks advice based on errno, despite errno
   potentially being zero and potentially having been clobbered by
   that point

To make sure we don't lose these fixes, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:

 * hold_lock_file_for_update (declared in cache.h)
 * lock_packed_refs (declared in refs.h)

[...]
 --- a/refs.c
 +++ b/refs.c
 @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, 
 unsigned char *sha1, int rea

Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix

 * a bug in lock_ref_sha1_basic, where it assumes EISDIR
   means it failed due to a directory being in the way

To make sure we don't lose this fix, it would be helpful to change
Michael's understated comment (why wasn't it marked with NEEDSWORK,
btw?)

 * errno is sometimes set on errors, but not always.

to describe the new guarantee.

[...]
 @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock 
 *lock,

Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix

 * a bug in git fetch's s_update_ref, which trusts the result of an
   errno == ENOTDIR check to detect D/F conflicts

To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:

 * lock_any_ref_for_update (declared in refs.h), after handling the
   check_refname_format case
 * lock_ref_sha1_basic

ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create.  Should git fetch also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?

 @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)

Making errno when returning from remove_empty_directories() more
obviously meaningful, which should provide some peace of mind for
people auditing lock_ref_sha1_basic.

[...]
 @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
[...]
  int commit_packed_refs(void)

Making errno when returning from commit_packed_refs() meaningful,
which should fix

 * a bug in git clone where it prints strerror(errno) based on
   errno, despite errno possibly being zero and potentially having
   been clobbered by that point
 * the same kind of bug in git pack-refs

and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.

To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for
commit_packed_refs in refs.h.

[...]
 @@ -2427,12 +2450,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)

Adding an 'err' argument to repack_without_refs and writing to it on
error.

[...]
 @@ -2723,9 +2755,12 @@ int log_ref_setup(const char 

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

[Ronnie Sahlberg]

Thanks.

On Tue, Jun 10, 2014 at 1:10 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 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.

 Add a new function unable_to_lock_message that takes a strbuf argument and
 fills in the reason for the failure.

 In commit_packed_refs, make sure that we propagate any errno that
 commit_lock_file might have set back to our caller.

 Make sure both commit_packed_refs and lock_file has errno set to a meaningful
 value on error. Update a whole bunch of other places to keep errno from
 beeing clobbered.

 This patch is doing several (all nice) things.  Are they connected?
 Would splitting some of them out into separate patches make sense or
 would it be too much fuss to be worth the trouble?

 [...]
 --- 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_message(const char *path, int err,
 +struct strbuf *buf);

 Introducing a new unable_to_lock_message helper, which has nicer
 semantics than unable_to_lock_error and cleans up lockfile.c a little.


Done.

 [...]
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
   return p;
  }

 -
 +/* Make sure errno contains a meaningful value on error */
  static int lock_file(struct lock_file *lk, const char *path, int flags)

 Making errno when returning from lock_file() meaningful, which should
 fix

  * an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries

  * an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point

 To make sure we don't lose these fixes, it would be helpful to also
 mention that errno is set in the API documentation for the relevant
 functions:

  * hold_lock_file_for_update (declared in cache.h)
  * lock_packed_refs (declared in refs.h)

Done.


 [...]
 --- a/refs.c
 +++ b/refs.c
 @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, 
 unsigned char *sha1, int rea

 Making errno when returning from resolve_ref_unsafe() meaningful,
 which should fix

  * a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way

 To make sure we don't lose this fix, it would be helpful to change
 Michael's understated comment (why wasn't it marked with NEEDSWORK,
 btw?)

  * errno is sometimes set on errors, but not always.

 to describe the new guarantee.

Done.


 [...]
 @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock 
 *lock,

 Making errno when returning from verify_lock() meaningful, which
 should almost but not completely fix

  * a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts

 To make sure we don't lose this fix, it would be helpful to also
 mention that errno is set in the API documentation for the relevant
 functions:

  * lock_any_ref_for_update (declared in refs.h), after handling the
check_refname_format case
  * lock_ref_sha1_basic

 ENOTDIR makes sense as a sign that a file was in the way of a
 directory we wanted to create.  Should git fetch also look for
 ENOTEMPTY or EEXIST to catch cases where a directory was in the way
 of a file to be created?


Done.

 @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)

 Making errno when returning from remove_empty_directories() more
 obviously meaningful, which should provide some peace of mind for
 people auditing lock_ref_sha1_basic.


Done.

 [...]
 @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
 [...]
  int commit_packed_refs(void)

 Making errno when returning from commit_packed_refs() meaningful,
 which should fix

  * a bug in git clone where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
  * the same kind of bug in git pack-refs

 and prepares for repack_without_refs() to get a meaningful
 error message when commit_packed_refs() fails without falling into
 the same bug.

 To make sure we don't lose this fix, it would be helpful to also
 mention that errno is set in the API documentation for
 commit_packed_refs in refs.h.


Done.

 [...]
 @@ -2427,12 +2450,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 

[PATCH v14 06/40] refs.c: add an err argument to repack_without_refs

[Ronnie Sahlberg]

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.

Add a new function unable_to_lock_message that takes a strbuf argument and
fills in the reason for the failure.

In commit_packed_refs, make sure that we propagate any errno that
commit_lock_file might have set back to our caller.

Make sure both commit_packed_refs and lock_file has errno set to a meaningful
value on error. Update a whole bunch of other places to keep errno from
beeing clobbered.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 cache.h|   2 ++
 lockfile.c |  37 +
 refs.c | 111 ++---
 3 files changed, 110 insertions(+), 40 deletions(-)

diff --git a/cache.h b/cache.h
index cbe1935..8b12aa8 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_message(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..f5da18c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
return p;
 }
 
-
+/* Make sure errno contains a meaningful value on error */
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
/*
@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
 */
static const size_t max_path_len = sizeof(lk-filename) - 5;
 
-   if (strlen(path) = max_path_len)
+   if (strlen(path) = max_path_len) {
+   errno = ENAMETOOLONG;
return -1;
+   }
strcpy(lk-filename, path);
if (!(flags  LOCK_NODEREF))
resolve_symlink(lk-filename, max_path_len);
@@ -148,42 +150,49 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
-   if (adjust_shared_perm(lk-filename))
-   return error(cannot fix permission bits on %s,
-lk-filename);
+   if (adjust_shared_perm(lk-filename)) {
+   int save_errno = errno;
+   error(cannot fix permission bits on %s,
+ lk-filename);
+   errno = save_errno;
+   return -1;
+   }
}
else
lk-filename[0] = 0;
return lk-fd;
 }
 
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_message(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_message(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_message(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 25c3a93..7ec1f32 100644
--- a/refs.c
+++ b/refs.c
@@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, 
unsigned char *sha1, int rea
if (flag)
*flag = 0;
 
-   if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+