git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: David Turner <dturner@twopensource.com>, git@vger.kernel.org
Cc: Ronnie Sahlberg <sahlberg@google.com>
Subject: Re: [PATCH v7 1/8] refs.c: add err arguments to reflog functions
Date: Wed, 08 Jul 2015 13:36:34 +0200	[thread overview]
Message-ID: <559D0B42.1040607@alum.mit.edu> (raw)
In-Reply-To: <1436316963-25520-1-git-send-email-dturner@twopensource.com>

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@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  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@alum.mit.edu

  parent reply	other threads:[~2015-07-08 11:36 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-08  0:55 [PATCH v7 1/8] refs.c: add err arguments to reflog functions David Turner
2015-07-08  0:55 ` [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs David Turner
2015-07-08 17:46   ` Johannes Sixt
2015-07-08 19:16     ` David Turner
2015-07-08 21:14       ` Johannes Sixt
2015-07-08 23:23         ` Junio C Hamano
2015-07-08 23:44           ` David Turner
2015-07-09  5:55             ` Junio C Hamano
2015-07-09 21:53               ` David Turner
2015-07-09 22:06                 ` Junio C Hamano
2015-07-10  4:30                   ` Michael Haggerty
2015-07-10 17:59                     ` David Turner
2015-07-14  4:33                     ` David Turner
2015-07-15 16:24                       ` Junio C Hamano
2015-07-15 18:04                         ` David Turner
2015-07-08 23:41         ` David Turner
2015-07-08  0:55 ` [PATCH v7 3/8] bisect: treat BISECT_HEAD as a ref David Turner
2015-07-08  0:55 ` [PATCH v7 4/8] refs: Break out check for reflog autocreation David Turner
2015-07-08  0:56 ` [PATCH v7 5/8] refs: new public ref function: safe_create_reflog David Turner
2015-07-08 11:49   ` Michael Haggerty
2015-07-08  0:56 ` [PATCH v7 6/8] git-reflog: add exists command David Turner
2015-07-08  0:56 ` [PATCH v7 7/8] update-ref and tag: add --create-reflog arg David Turner
2015-07-08 13:44   ` Michael Haggerty
2015-07-08 20:21     ` David Turner
2015-07-08  0:56 ` [PATCH v7 8/8] git-stash: use update-ref --create-reflog instead of creating files David Turner
2015-07-08  7:28   ` Junio C Hamano
2015-07-08  7:33   ` Junio C Hamano
2015-07-08 13:50   ` Michael Haggerty
2015-07-08 11:36 ` Michael Haggerty [this message]
2015-07-08 20:01   ` [PATCH v7 1/8] refs.c: add err arguments to reflog functions David Turner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=559D0B42.1040607@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=dturner@twopensource.com \
    --cc=git@vger.kernel.org \
    --cc=sahlberg@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).