On 07/08/2015 02:55 AM, 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 change slightly. For instance, we sometimes lose
> "cannot update ref" and instead only show the specific cause of ref
> update failure.
Did you check that the new error messages are at least as clear to the
user as the old ones? (Sometimes errors from deep in the call stack
provide details but don't make it clear how the details connect back to
the action that the user was trying to do.)
> 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 | 130 +++++++++++++++++++++++++++++------------------------
> refs.h | 4 +-
> 3 files changed, 79 insertions(+), 63 deletions(-)
>
> [...]
> diff --git a/refs.c b/refs.c
> index fb568d7..e891bed 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -3288,14 +3290,20 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
> * necessary, using the specified lockmsg (which can be NULL).
> */
> 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 result = 0;
Please put a blank line between local variable definitions and the start
of code.
> clear_loose_ref_cache(&ref_cache);
> - if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg) < 0 ||
> + if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||
> (strcmp(lock->ref_name, lock->orig_ref_name) &&
> - log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg) < 0)) {
> - unlock_ref(lock);
> - return -1;
> + log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {
> + char *old_msg = strbuf_detach(err, NULL);
> + strbuf_addf(err, "Cannot update the ref '%s': '%s'",
> + lock->ref_name, old_msg);
> + free(old_msg);
> + result = -1;
> + goto done;
> }
I see this code already did what I complained about in my earlier email
[*]: fail a reference transaction after it is already partly committed.
In my opinion this is incorrect for the reasons stated there. But you
don't have to consider it to be in the scope of your patch series.
[*] http://article.gmane.org/gmane.comp.version-control.git/273668
> if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
> /*
> [...]
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