Re: [PATCH 1/6] refs.c: add an err argument to log_ref_setup

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

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

I think I saw Michael did something similar to other codepaths in
the ref API recently, and going in the same direction is generally a
good thing to do for consistency ;-)

 write_ref_to_lockfile is a private function that calls
 log_ref_setup. Update this function to also take an err argument and
 fill it in.  This again eliminates the need to manage errno in this
 function.

This particular patch is not just about log_ref_setup() and its
private friend write_ref_to_lockfile(), right?  It seems to also
touch commit_ref_update(), log_ref_write(), etc.  The overall
theme of this change is to teach parts of the ref API that deal
with writing reflogs to report errors the same way (i.e. as the
remainder of the ref API does by using err), and that may be a
good single-line summary of the change (aka Subject).

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

Original is underindented but this makes it even worse.  Push
continuation line further to the right (or restructure the code so
that it does not have to indent too deeply in the first place).

Other than that, this step looks sensible.

Thanks.
--
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 1/6] refs.c: add an err argument to log_ref_setup

[David Turner]

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

write_ref_to_lockfile is a private function that calls
log_ref_setup. Update this function to also take an err argument and
fill it in.  This again eliminates the need to manage errno in this
function.

Update of a patch by Ronnie Sahlberg.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index c018ab3..ac4d10a 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..b34a54a 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)
 starts_with(refname, refs/notes/) ||