Re: [PATCH v8 1/7] refs.c: add err arguments to reflog functions

2015-07-21 Thread Michael Haggerty 
On 07/09/2015 03:50 PM, David Turner wrote:
 Add an err argument to log_ref_setup that can explain the reason
 for a failure. This then eliminates the need to manage errno through
 this function since we can just add strerror(errno) to the err string
 when meaningful. No callers relied on errno from this function for
 anything else than the error message.
 
 Also add err arguments to private functions write_ref_to_lockfile,
 log_ref_write_1, commit_ref_update. This again eliminates the need to
 manage errno in these functions.
 
 Some error messages are slightly reordered.
 
 Update of a patch by Ronnie Sahlberg.
 
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 Signed-off-by: David Turner dtur...@twopensource.com
 ---
  builtin/checkout.c |   8 ++--
  refs.c | 127 
 +++--
  refs.h |   4 +-
  3 files changed, 81 insertions(+), 58 deletions(-)
 
 diff --git a/builtin/checkout.c b/builtin/checkout.c
 index c018ab3..93f63d3 100644
 --- a/builtin/checkout.c
 +++ b/builtin/checkout.c
 @@ -624,16 +624,18 @@ static void update_refs_for_switch(const struct 
 checkout_opts *opts,
   struct strbuf log_file = STRBUF_INIT;
   int ret;
   const char *ref_name;
 + struct strbuf err = STRBUF_INIT;
  
   ref_name = mkpath(refs/heads/%s, 
 opts-new_orphan_branch);
   temp = log_all_ref_updates;
   log_all_ref_updates = 1;
 - ret = log_ref_setup(ref_name, log_file);
 + ret = log_ref_setup(ref_name, log_file, err);
   log_all_ref_updates = temp;
   strbuf_release(log_file);
   if (ret) {
 - fprintf(stderr, _(Can not do reflog 
 for '%s'\n),
 - opts-new_orphan_branch);
 + fprintf(stderr, _(Can not do reflog 
 for '%s'. %s\n),
 + opts-new_orphan_branch, 
 err.buf);

Our usual pattern for chaining error messages is

$problem: $reason

In this patch (and maybe later patches too? I haven't checked yet) I see

$problem. $reason

and also

$problem. '$reason'

I think it would be good to use the first pattern consistently.

 + strbuf_release(err);
   return;
   }
   }
 diff --git a/refs.c b/refs.c
 index fb568d7..03e7505 100644
 --- a/refs.c
 +++ b/refs.c
 [...]
 @@ -3247,25 +3247,28 @@ int is_branch(const char *refname)
  
  /*
   * Write sha1 into the open lockfile, then close the lockfile. On
 - * errors, rollback the lockfile and set errno to reflect the problem.
 + * errors, rollback the lockfile, fill in *err and
 + * return -1.
   */
  static int write_ref_to_lockfile(struct ref_lock *lock,
 -  const unsigned char *sha1)
 +  const unsigned char *sha1, struct strbuf *err)
  {
   static char term = '\n';
   struct object *o;
  
   o = parse_object(sha1);
   if (!o) {
 - error(Trying to write ref %s with nonexistent object %s,
 - lock-ref_name, sha1_to_hex(sha1));
 + strbuf_addf(err,
 + Trying to write ref %s with nonexistent object %s,
 + lock-ref_name, sha1_to_hex(sha1));
   unlock_ref(lock);
   errno = EINVAL;

Is it intentional that this function is still setting errno (here and a
few lines farther down)? I'm guessing that this is no longer needed,
though I haven't audited the callers.

   return -1;
   }
   if (o-type != OBJ_COMMIT  is_branch(lock-ref_name)) {
 - error(Trying to write non-commit object %s to branch %s,
 - sha1_to_hex(sha1), lock-ref_name);
 + strbuf_addf(err,
 + Trying to write non-commit object %s to branch %s,
 + sha1_to_hex(sha1), lock-ref_name);
   unlock_ref(lock);
   errno = EINVAL;
   return -1;
 [...]

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

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

[PATCH v8 1/7] refs.c: add err arguments to reflog functions

2015-07-09 Thread David Turner 
Add an err argument to log_ref_setup that can explain the reason
for a failure. This then eliminates the need to manage errno through
this function since we can just add strerror(errno) to the err string
when meaningful. No callers relied on errno from this function for
anything else than the error message.

Also add err arguments to private functions write_ref_to_lockfile,
log_ref_write_1, commit_ref_update. This again eliminates the need to
manage errno in these functions.

Some error messages are slightly reordered.

Update of a patch by Ronnie Sahlberg.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: David Turner dtur...@twopensource.com
---
 builtin/checkout.c |   8 ++--
 refs.c | 127 +++--
 refs.h |   4 +-
 3 files changed, 81 insertions(+), 58 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c018ab3..93f63d3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -624,16 +624,18 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
struct strbuf log_file = STRBUF_INIT;
int ret;
const char *ref_name;
+   struct strbuf err = STRBUF_INIT;
 
ref_name = mkpath(refs/heads/%s, 
opts-new_orphan_branch);
temp = log_all_ref_updates;
log_all_ref_updates = 1;
-   ret = log_ref_setup(ref_name, log_file);
+   ret = log_ref_setup(ref_name, log_file, err);
log_all_ref_updates = temp;
strbuf_release(log_file);
if (ret) {
-   fprintf(stderr, _(Can not do reflog 
for '%s'\n),
-   opts-new_orphan_branch);
+   fprintf(stderr, _(Can not do reflog 
for '%s'. %s\n),
+   opts-new_orphan_branch, 
err.buf);
+   strbuf_release(err);
return;
}
}
diff --git a/refs.c b/refs.c
index fb568d7..03e7505 100644
--- a/refs.c
+++ b/refs.c
@@ -2975,9 +2975,11 @@ static int rename_ref_available(const char *oldname, 
const char *newname)
return ret;
 }
 
-static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char 
*sha1);
+static int write_ref_to_lockfile(struct ref_lock *lock,
+const unsigned char *sha1, struct strbuf *err);
 static int commit_ref_update(struct ref_lock *lock,
-const unsigned char *sha1, const char *logmsg);
+const unsigned char *sha1, const char *logmsg,
+struct strbuf *err);
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char 
*logmsg)
 {
@@ -3038,9 +3040,10 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
}
hashcpy(lock-old_oid.hash, orig_sha1);
 
-   if (write_ref_to_lockfile(lock, orig_sha1) ||
-   commit_ref_update(lock, orig_sha1, logmsg)) {
-   error(unable to write current sha1 into %s, newrefname);
+   if (write_ref_to_lockfile(lock, orig_sha1, err) ||
+   commit_ref_update(lock, orig_sha1, logmsg, err)) {
+   error(unable to write current sha1 into %s: %s, newrefname, 
err.buf);
+   strbuf_release(err);
goto rollback;
}
 
@@ -3056,9 +3059,11 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
 
flag = log_all_ref_updates;
log_all_ref_updates = 0;
-   if (write_ref_to_lockfile(lock, orig_sha1) ||
-   commit_ref_update(lock, orig_sha1, NULL))
-   error(unable to write current sha1 into %s, oldrefname);
+   if (write_ref_to_lockfile(lock, orig_sha1, err) ||
+   commit_ref_update(lock, orig_sha1, NULL, err)) {
+   error(unable to write current sha1 into %s: %s, oldrefname, 
err.buf);
+   strbuf_release(err);
+   }
log_all_ref_updates = flag;
 
  rollbacklog:
@@ -3113,8 +3118,8 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
 }
 
-/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
+/* This function will fill in *err and return -1 on failure */
+int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct 
strbuf *err)
 {
int logfd, oflags = O_APPEND | O_WRONLY;
char *logfile;
@@ -3129,9 +3134,8 @@ int log_ref_setup(const char *refname, struct strbuf 
*sb_logfile)