git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jul 8, 2015, 11:36 UTC
Message-ID
<559D0B42.1040607@alum.mit.edu>
In-Reply-To
<1436316963-25520-1-git-send-email-dturner@twopensource.com>
On 07/08/2015 02:55 AM, David Turner wrote:
Show 13 quoted lines
> 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.)

Show 25 quoted lines
> 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.

Show 15 quoted lines
>  	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
Previous: Michael HaggertyNext: David Turner
Message 29 of 30 in “refs.c: add err arguments to reflog functions”
  1. 1/8 refs.c: add err arguments to reflog functionsDavid Turner, Jul 8, 2015
  2. 2/8 cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refsDavid Turner, Jul 8, 2015
  3. Johannes SixtJul 8, 2015
  4. David TurnerJul 8, 2015
  5. Johannes SixtJul 8, 2015
  6. Junio C HamanoJul 8, 2015
  7. David TurnerJul 8, 2015
  8. Junio C HamanoJul 9, 2015
  9. David TurnerJul 9, 2015
  10. Junio C HamanoJul 9, 2015
  11. Michael HaggertyJul 10, 2015
  12. David TurnerJul 10, 2015
  13. David TurnerJul 14, 2015
  14. Junio C HamanoJul 15, 2015
  15. David TurnerJul 15, 2015
  16. David TurnerJul 8, 2015
  17. 3/8 bisect: treat BISECT_HEAD as a refDavid Turner, Jul 8, 2015
  18. 4/8 refs: Break out check for reflog autocreationDavid Turner, Jul 8, 2015
  19. 5/8 refs: new public ref function: safe_create_reflogDavid Turner, Jul 8, 2015
  20. Michael HaggertyJul 8, 2015
  21. 6/8 git-reflog: add exists commandDavid Turner, Jul 8, 2015
  22. 7/8 update-ref and tag: add --create-reflog argDavid Turner, Jul 8, 2015
  23. Michael HaggertyJul 8, 2015
  24. David TurnerJul 8, 2015
  25. 8/8 git-stash: use update-ref --create-reflog instead of creating filesDavid Turner, Jul 8, 2015
  26. Junio C HamanoJul 8, 2015
  27. Junio C HamanoJul 8, 2015
  28. Michael HaggertyJul 8, 2015
  29. Michael HaggertyJul 8, 2015
  30. David TurnerJul 8, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.