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

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

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jul 6, 2015, 15:53 UTC
Message-ID
<559AA490.3080605@alum.mit.edu>
In-Reply-To
<1435609076-8592-2-git-send-email-dturner@twopensource.com>
On 06/29/2015 10:17 PM, David Turner wrote:
Show 11 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.
> 
> Update of a patch by Ronnie Sahlberg.

First a general comment: we have a convention, not yet very well adhered to, that error messages should start with lower-case letters. I know that in many cases you are just carrying along pre-existing error messages that are capitalized. But at a minimum, please avoid adding new error messages that are capitalized. And if you are feeling ambitious, feel free to lower-case some existing ones :-)

Show 26 quoted lines
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.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
> [...]
> diff --git a/refs.c b/refs.c
> index fb568d7..c97ca02 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -3216,26 +3217,25 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
>  	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
>  				  git_committer_info(0), msg);
>  	if (result) {
> -		int save_errno = errno;
>  		close(logfd);
> -		error("Unable to append to %s", log_file);
> -		errno = save_errno;
> +		strbuf_addf(err, "Unable to append to %s. %s", log_file,
> +			    strerror(errno));
>  		return -1;

Above, the call to close(logfd) could clobber errno. It would be better to exchange the calls.

Show 43 quoted lines
>  	}
>  	if (close(logfd)) {
> -		int save_errno = errno;
> -		error("Unable to append to %s", log_file);
> -		errno = save_errno;
> +		strbuf_addf(err, "Unable to append to %s. %s", log_file,
> +			    strerror(errno));
>  		return -1;
>  	}
>  	return 0;
>  }
>  
>  static int log_ref_write(const char *refname, const unsigned char *old_sha1,
> -			 const unsigned char *new_sha1, const char *msg)
> +			 const unsigned char *new_sha1, const char *msg,
> +			 struct strbuf *err)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb);
> +	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, err);
>  	strbuf_release(&sb);
>  	return ret;
>  }
> [...]
> @@ -3288,12 +3290,15 @@ 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)
>  {
>  	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)) {
> +	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {
> +		strbuf_addf(err, "Cannot update the ref '%s'.",
> +			    lock->ref_name);
>  		unlock_ref(lock);
>  		return -1;
>  	}

Since you are passing err into log_ref_write(), I assume that it would already have an error message written to it by the time you enter into this block. Yet in the block you append more text to it.

It seems to me that you either want to clear err and replace it with your own message, or create a new message that looks like

    Cannot update the ref '%s': %s
where the second "%s" is replaced with the error from log_ref_write().
Show 7 quoted lines
> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
>  					      head_sha1, &head_flag);
>  		if (head_ref && (head_flag & REF_ISSYMREF) &&
>  		    !strcmp(head_ref, lock->ref_name))
> -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
> +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
> +				      err);

Here you don't check for errors from log_ref_write(). So it might or might not have written something to err. If it has, is that error handled correctly?

Show 39 quoted lines
>  	}
>  	if (commit_ref(lock)) {
>  		error("Couldn't set %s", lock->ref_name);
> @@ -3336,6 +3342,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
>  	int fd, len, written;
>  	char *git_HEAD = git_pathdup("%s", ref_target);
>  	unsigned char old_sha1[20], new_sha1[20];
> +	struct strbuf err = STRBUF_INIT;
>  
>  	if (logmsg && read_ref(ref_target, old_sha1))
>  		hashclr(old_sha1);
> @@ -3384,8 +3391,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
>  #ifndef NO_SYMLINK_HEAD
>  	done:
>  #endif
> -	if (logmsg && !read_ref(refs_heads_master, new_sha1))
> -		log_ref_write(ref_target, old_sha1, new_sha1, logmsg);
> +	if (logmsg && !read_ref(refs_heads_master, new_sha1) &&
> +		log_ref_write(ref_target, old_sha1, new_sha1, logmsg, &err)) {
> +		error("%s", err.buf);
> +		strbuf_release(&err);
> +	}
>  
>  	free(git_HEAD);
>  	return 0;
> @@ -4021,14 +4031,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				 * value, so we don't need to write it.
>  				 */
>  			} else if (write_ref_to_lockfile(update->lock,
> -							 update->new_sha1)) {
> +							 update->new_sha1,
> +							 err)) {
>  				/*
>  				 * The lock was freed upon failure of
>  				 * write_ref_to_lockfile():
>  				 */
>  				update->lock = NULL;
> -				strbuf_addf(err, "cannot update the ref '%s'.",
> -					    update->refname);

Previously, the error generated here was "cannot update the ref '%s'." But now that you are letting write_ref_to_lockfile() fill err, the error message will be something from that function. This might be OK or it might not. If you think it is, it would be worth a word or two of justification in the commit message.

Show 13 quoted lines
>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
>  			} else {
> @@ -4054,11 +4063,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  		if (update->flags & REF_NEEDS_COMMIT) {
>  			if (commit_ref_update(update->lock,
> -					      update->new_sha1, update->msg)) {
> +					      update->new_sha1, update->msg, err)) {
>  				/* freed by commit_ref_update(): */
>  				update->lock = NULL;
> -				strbuf_addf(err, "Cannot update the ref '%s'.",
> -					    update->refname);

Same story here: you use whatever error message that commit_ref_update() emitted rather than the one that was previously chosen here.

Show 20 quoted lines
>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
>  			} else {
> diff --git a/refs.h b/refs.h
> index e82fca5..debdefc 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -226,9 +226,9 @@ int pack_refs(unsigned int flags);
>  #define REF_NODEREF	0x01
>  
>  /*
> - * Setup reflog before using. Set errno to something meaningful on failure.
> + * Setup reflog before using. Fill in err and return -1 on failure.
>   */
> -int log_ref_setup(const char *refname, struct strbuf *logfile);
> +int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);
>  
>  /** Reads log for the value of ref during at_time. **/
>  extern int read_ref_at(const char *refname, unsigned int flags,
> 
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Previous: David TurnerNext: David Turner
Message 3 of 31 in “refs backend preamble”
  1. 0/7 refs backend preambleDavid Turner, Jun 29, 2015
  2. 1/7 refs.c: add err arguments to reflog functionsDavid Turner, Jun 29, 2015
  3. Michael HaggertyJul 6, 2015
  4. David TurnerJul 7, 2015
  5. Michael HaggertyJul 8, 2015
  6. Junio C HamanoJul 8, 2015
  7. Michael HaggertyJul 9, 2015
  8. 2/7 cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refsDavid Turner, Jun 29, 2015
  9. Michael HaggertyJul 6, 2015
  10. 3/7 bisect: treat BISECT_HEAD as a refDavid Turner, Jun 29, 2015
  11. 4/7 refs: Break out check for reflog autocreationDavid Turner, Jun 29, 2015
  12. 5/7 refs: new public ref function: safe_create_reflogDavid Turner, Jun 29, 2015
  13. Michael HaggertyJul 6, 2015
  14. David TurnerJul 7, 2015
  15. Michael HaggertyJul 8, 2015
  16. 6/7 git-reflog: add create and exists functionsDavid Turner, Jun 29, 2015
  17. Eric SunshineJun 30, 2015
  18. David TurnerJun 30, 2015
  19. Junio C HamanoJun 30, 2015
  20. Eric SunshineJun 30, 2015
  21. Junio C HamanoJun 30, 2015
  22. David TurnerJun 30, 2015
  23. Junio C HamanoJun 30, 2015
  24. Michael HaggertyJul 6, 2015
  25. David TurnerJul 8, 2015
  26. Michael HaggertyJul 8, 2015
  27. David TurnerJul 8, 2015
  28. 7/7 git-stash: use git-reflog instead of creating filesDavid Turner, Jun 29, 2015
  29. Junio C HamanoJun 29, 2015
  30. Junio C HamanoJun 29, 2015
  31. David TurnerJun 29, 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.