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

2014-05-27 Thread Ronnie Sahlberg 
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),

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

2014-05-16 Thread 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.

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;
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;
+   }
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