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 <sahlberg@xxxxxxxxxx>
> Signed-off-by: David Turner <dturner@xxxxxxxxxxxxxxxx>
> ---
> 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
mhagger@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html