[PATCH v6 0/7] refs backend preamble

[PATCH v6 0/7] refs backend preamble

[David Turner]

Minor formatting fixes from Junio Hamano.

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

[David Turner]

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.

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
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);
+					strbuf_release(&err);
 					return;
 				}
 			}
diff --git a/refs.c b/refs.c
index fb568d7..c97ca02 100644
--- a/refs.c
+++ b/refs.c
@@ -2975,9 +2975,11 @@ static int rename_ref_available(const char *oldname, const char *newname)
 	return ret;
 }
 
-static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1);
+static int write_ref_to_lockfile(struct ref_lock *lock,
+				 const unsigned char *sha1, struct strbuf *err);
 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 rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
@@ -3038,9 +3040,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	}
 	hashcpy(lock->old_oid.hash, orig_sha1);
 
-	if (write_ref_to_lockfile(lock, orig_sha1) ||
-	    commit_ref_update(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newrefname);
+	if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
+	    commit_ref_update(lock, orig_sha1, logmsg, &err)) {
+		error("unable to write current sha1 into %s: %s", newrefname, err.buf);
+		strbuf_release(&err);
 		goto rollback;
 	}
 
@@ -3056,9 +3059,11 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	flag = log_all_ref_updates;
 	log_all_ref_updates = 0;
-	if (write_ref_to_lockfile(lock, orig_sha1) ||
-	    commit_ref_update(lock, orig_sha1, NULL))
-		error("unable to write current sha1 into %s", oldrefname);
+	if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
+	    commit_ref_update(lock, orig_sha1, NULL, &err)) {
+		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
+		strbuf_release(&err);
+	}
 	log_all_ref_updates = flag;
 
  rollbacklog:
@@ -3113,8 +3118,8 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
-/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
+/* This function will fill in *err and return -1 on failure */
+int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
 	char *logfile;
@@ -3129,9 +3134,8 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
 	     starts_with(refname, "refs/notes/") ||
 	     !strcmp(refname, "HEAD"))) {
 		if (safe_create_leading_directories(logfile) < 0) {
-			int save_errno = errno;
-			error("unable to create directory for %s", logfile);
-			errno = save_errno;
+			strbuf_addf(err, "unable to create directory for %s. "
+				    "%s", logfile, strerror(errno));
 			return -1;
 		}
 		oflags |= O_CREAT;
@@ -3144,20 +3148,16 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
 
 		if (errno == EISDIR) {
 			if (remove_empty_directories(logfile)) {
-				int save_errno = errno;
-				error("There are still logs under '%s'",
-				      logfile);
-				errno = save_errno;
+				strbuf_addf(err, "There are still logs under "
+					    "'%s'", logfile);
 				return -1;
 			}
 			logfd = open(logfile, oflags, 0666);
 		}
 
 		if (logfd < 0) {
-			int save_errno = errno;
-			error("Unable to append to %s: %s", logfile,
-			      strerror(errno));
-			errno = save_errno;
+			strbuf_addf(err, "Unable to append to %s: %s",
+				    logfile, strerror(errno));
 			return -1;
 		}
 	}
@@ -3195,7 +3195,7 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 
 static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
-			   struct strbuf *sb_log_file)
+			   struct strbuf *sb_log_file, struct strbuf *err)
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char *log_file;
@@ -3203,7 +3203,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, sb_log_file);
+	result = log_ref_setup(refname, sb_log_file, err);
+
 	if (result)
 		return result;
 	log_file = sb_log_file->buf;
@@ -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;
 	}
 	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;
 }
@@ -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;
 		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;
@@ -3273,10 +3276,9 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 	if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
 	    write_in_full(lock->lk->fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
-		int save_errno = errno;
-		error("Couldn't write %s", lock->lk->filename.buf);
+		strbuf_addf(err,
+			    "Couldn't write %s", lock->lk->filename.buf);
 		unlock_ref(lock);
-		errno = save_errno;
 		return -1;
 	}
 	return 0;
@@ -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;
 	}
@@ -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);
 	}
 	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);
 				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);
 				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,
-- 
2.0.4.315.gad8727a-twtrsrc

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

[Michael Haggerty]

On 06/29/2015 10:17 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.
> 
> 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 :-)

> 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.

>  	}
>  	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().

> @@ -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?

>  	}
>  	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.

>  				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.

>  				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

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

[David Turner]

On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:
> On 06/29/2015 10:17 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.
> > 
> > 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 :-)

I'll save lowercasing messages for other patches, but I'll try to take
care with new messages.

...

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

Done, thanks.

> 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().

Done, thanks.

> > @@ -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?

That was the case before this patch too. But I guess I might as well
check.

> 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.

Will adjust commit message.

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

[Michael Haggerty]

On 07/08/2015 12:41 AM, David Turner wrote:
> On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:
>> On 06/29/2015 10:17 PM, David Turner wrote:
>>> [...]
>>> @@ -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?
> 
> That was the case before this patch too. But I guess I might as well
> check.

This isn't about fixing a pre-existing bug. Your patch introduced a
regression.

Before this patch, commit_ref_update() didn't really need to know
whether log_ref_write() failed, because it didn't take any further
remedial action anyway. log_ref_write() would have already written an
error message to stderr, and that was all the error handling that was
needed.

But now, log_ref_write() *doesn't* write its error message to stderr; it
only stores it to `err`. So now it is the caller's responsibility to
check whether there was an error, and if so write `err` to stderr.
Otherwise the error message will get lost entirely, I think.

I think your v7 of this patch goes too far, by turning a failure to
write to the reflog into a failure of the whole transaction. The problem
is that this failure comes too late, in the commit phase of the
transaction. Aborting at this late stage can leave some references
changed and others rolled back, violating the promise of atomicity.

The old code reported a failure to write the reflog to stderr but didn't
fail the transaction. I think that behavior is more appropriate. The
reflog is of lower importance than the references themselves. Junio, do
you agree?

If so, it might be that adding err arguments to the reflog functions
does not bring any benefits.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I think your v7 of this patch goes too far, by turning a failure to
> write to the reflog into a failure of the whole transaction. The problem
> is that this failure comes too late, in the commit phase of the
> transaction. Aborting at this late stage can leave some references
> changed and others rolled back, violating the promise of atomicity.

Yeah, that sounds problematic.

> The old code reported a failure to write the reflog to stderr but didn't
> fail the transaction. I think that behavior is more appropriate. The
> reflog is of lower importance than the references themselves. Junio, do
> you agree?

That is actually a loaded question.

Do I agree that the current (i.e. before this change) behaviour is
more appropriate given the current choice of representation of refs
and reflogs on the filesystem, treating a failure to update reflog
as lower importance event and accept it as a limitation that it
cannot abort the whole transaction atomically?  Compared to leaving
the repository in a half-updated state where some refs and their
logs are updated already, other remaining proposed updates are
ignored, and the transaction claims to have failed even though some
things have already changed and we cannot rollback, I would say that
is a better compromise to treat reflog update as a lower importance.

Do I agree that reflog writing should stay to be best-effort in the
longer term?  Not really.  If we are moving the ref API in the
direction where we can plug a backend that is different from the
traditional filesystem based one for storing refs, I think the
backend should also be responsible for storing logs for the refs it
stores, and if the backend wants to promise atomicity, then we
should be able to fail the whole transaction when updates to refs
could proceed but updates to the log of one of these updated refs
cannot.  So I do not agree to cast in stone "the reflog is of lower
importance" as a general rule.

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

[Michael Haggerty]

On 07/08/2015 07:11 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I think your v7 of this patch goes too far, by turning a failure to
>> write to the reflog into a failure of the whole transaction. The problem
>> is that this failure comes too late, in the commit phase of the
>> transaction. Aborting at this late stage can leave some references
>> changed and others rolled back, violating the promise of atomicity.
> 
> Yeah, that sounds problematic.
> 
>> The old code reported a failure to write the reflog to stderr but didn't
>> fail the transaction. I think that behavior is more appropriate. The
>> reflog is of lower importance than the references themselves. Junio, do
>> you agree?
> 
> That is actually a loaded question.
> 
> Do I agree that the current (i.e. before this change) behaviour is
> more appropriate given the current choice of representation of refs
> and reflogs on the filesystem, treating a failure to update reflog
> as lower importance event and accept it as a limitation that it
> cannot abort the whole transaction atomically?  Compared to leaving
> the repository in a half-updated state where some refs and their
> logs are updated already, other remaining proposed updates are
> ignored, and the transaction claims to have failed even though some
> things have already changed and we cannot rollback, I would say that
> is a better compromise to treat reflog update as a lower importance.
> 
> Do I agree that reflog writing should stay to be best-effort in the
> longer term?  Not really.  If we are moving the ref API in the
> direction where we can plug a backend that is different from the
> traditional filesystem based one for storing refs, I think the
> backend should also be responsible for storing logs for the refs it
> stores, and if the backend wants to promise atomicity, then we
> should be able to fail the whole transaction when updates to refs
> could proceed but updates to the log of one of these updated refs
> cannot.  So I do not agree to cast in stone "the reflog is of lower
> importance" as a general rule.

Junio,

You make a good distinction. I was describing a compromise that we have
to make now due to the limitations of the current ref/reflog backend.
But I agree 100% that a future storage backend that can do correct
rollback of refs *and* reflogs can fail a transaction if the reflog
updates cannot be committed.

Thanks,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v6 2/7] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

[David Turner]

Instead of directly writing to and reading from files in
$GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
and REVERT_HEAD.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 branch.c                         |  4 ++--
 builtin/commit.c                 |  6 +++---
 builtin/merge.c                  |  2 +-
 contrib/completion/git-prompt.sh |  4 ++--
 git-gui/lib/commit.tcl           |  2 +-
 sequencer.c                      | 39 ++++++++++++++++++++-------------------
 t/t7509-commit.sh                |  4 ++--
 wt-status.c                      |  6 ++----
 8 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/branch.c b/branch.c
index b002435..ec598aa 100644
--- a/branch.c
+++ b/branch.c
@@ -302,8 +302,8 @@ void create_branch(const char *head,
 
 void remove_branch_state(void)
 {
-	unlink(git_path("CHERRY_PICK_HEAD"));
-	unlink(git_path("REVERT_HEAD"));
+	delete_ref("CHERRY_PICK_HEAD", NULL, REF_NODEREF);
+	delete_ref("REVERT_HEAD", NULL, REF_NODEREF);
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_RR"));
 	unlink(git_path("MERGE_MSG"));
diff --git a/builtin/commit.c b/builtin/commit.c
index b5b1158..53c7e90 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -168,7 +168,7 @@ static void determine_whence(struct wt_status *s)
 {
 	if (file_exists(git_path("MERGE_HEAD")))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
+	else if (ref_exists("CHERRY_PICK_HEAD")) {
 		whence = FROM_CHERRY_PICK;
 		if (file_exists(git_path(SEQ_DIR)))
 			sequencer_in_use = 1;
@@ -1777,8 +1777,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 	ref_transaction_free(transaction);
 
-	unlink(git_path("CHERRY_PICK_HEAD"));
-	unlink(git_path("REVERT_HEAD"));
+	delete_ref("CHERRY_PICK_HEAD", NULL, REF_NODEREF);
+	delete_ref("REVERT_HEAD", NULL, REF_NODEREF);
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 	unlink(git_path("MERGE_MODE"));
diff --git a/builtin/merge.c b/builtin/merge.c
index 46aacd6..3e2ae2f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1206,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
-	if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
+	if (ref_exists("CHERRY_PICK_HEAD")) {
 		if (advice_resolve_conflict)
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 366f0bc..e2c5583 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -415,9 +415,9 @@ __git_ps1 ()
 			fi
 		elif [ -f "$g/MERGE_HEAD" ]; then
 			r="|MERGING"
-		elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
+		elif git rev-parse --quiet --verify "CHERRY_PICK_HEAD" >/dev/null; then
 			r="|CHERRY-PICKING"
-		elif [ -f "$g/REVERT_HEAD" ]; then
+		elif git rev-parse --quiet --verify "REVERT_HEAD" >/dev/null; then
 			r="|REVERTING"
 		elif [ -f "$g/BISECT_LOG" ]; then
 			r="|BISECTING"
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 864b687..2b08b13 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -409,7 +409,7 @@ A rescan will be automatically started now.
 	catch {file delete [gitdir MERGE_MSG]}
 	catch {file delete [gitdir SQUASH_MSG]}
 	catch {file delete [gitdir GITGUI_MSG]}
-	catch {file delete [gitdir CHERRY_PICK_HEAD]}
+	catch {git update-ref -d --no-deref CHERRY_PICK_HEAD}
 
 	# -- Let rerere do its thing.
 	#
diff --git a/sequencer.c b/sequencer.c
index f8421a8..44c43e5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,21 +158,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	unuse_commit_buffer(commit, msg->message);
 }
 
-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
+static void write_cherry_pick_head(struct commit *commit, const char *ref)
 {
-	const char *filename;
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf err = STRBUF_INIT;
+	void *transaction;
 
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+	transaction = ref_transaction_begin(&err);
+	if (!transaction)
+		die(_("Could not create transaction: %s"), err.buf);
 
-	filename = git_path("%s", pseudoref);
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), filename);
-	strbuf_release(&buf);
+	if (ref_transaction_update(transaction, ref, commit->object.sha1,
+				   NULL, REF_NODEREF, NULL,
+				   &err))
+		die(_("Could not write ref %s: %s"), ref, err.buf);
+
+	if (ref_transaction_commit(transaction, &err))
+		die(_("Could not commit ref write %s: %s"), ref, err.buf);
 }
 
 static void print_advice(int show_hint, struct replay_opts *opts)
@@ -186,7 +187,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 		 * (typically rebase --interactive) wants to take care
 		 * of the commit itself so remove CHERRY_PICK_HEAD
 		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
+		delete_ref("CHERRY_PICK_HEAD", NULL, REF_NODEREF);
 		return;
 	}
 
@@ -878,8 +879,8 @@ static int rollback_single_pick(void)
 {
 	unsigned char head_sha1[20];
 
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
+	if (!ref_exists("CHERRY_PICK_HEAD") &&
+	    !ref_exists("REVERT_HEAD"))
 		return error(_("no cherry-pick or revert in progress"));
 	if (read_ref_full("HEAD", 0, head_sha1, NULL))
 		return error(_("cannot resolve HEAD"));
@@ -1014,8 +1015,8 @@ static int continue_single_pick(void)
 {
 	const char *argv[] = { "commit", NULL };
 
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
+	if (!ref_exists("CHERRY_PICK_HEAD") &&
+	    !ref_exists("REVERT_HEAD"))
 		return error(_("no cherry-pick or revert in progress"));
 	return run_command_v_opt(argv, RUN_GIT_CMD);
 }
@@ -1030,8 +1031,8 @@ static int sequencer_continue(struct replay_opts *opts)
 	read_populate_todo(&todo_list, opts);
 
 	/* Verify that the conflict has been resolved */
-	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
-	    file_exists(git_path("REVERT_HEAD"))) {
+	if (ref_exists("CHERRY_PICK_HEAD") ||
+	    ref_exists("REVERT_HEAD")) {
 		int ret = continue_single_pick();
 		if (ret)
 			return ret;
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index 9ac7940..f7fd62c 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -163,7 +163,7 @@ test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' '
 	test_tick &&
 	git commit -am "cherry-pick 1" --author="Cherry <cherry@pick.er>" &&
 	git tag cherry-pick-head &&
-	git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD &&
+	git update-ref CHERRY_PICK_HEAD $(git rev-parse cherry-pick-head) &&
 	echo "This is a MERGE_MSG" >.git/MERGE_MSG &&
 	echo "cherry-pick 1b" >>foo &&
 	test_tick &&
@@ -178,7 +178,7 @@ test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' '
 '
 
 test_expect_success '--reset-author with CHERRY_PICK_HEAD' '
-	git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD &&
+	git update-ref CHERRY_PICK_HEAD $(git rev-parse cherry-pick-head) &&
 	echo "cherry-pick 2" >>foo &&
 	test_tick &&
 	git commit -am "cherry-pick 2" --reset-author &&
diff --git a/wt-status.c b/wt-status.c
index 9c686e6..661c1fb 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1336,8 +1336,7 @@ void wt_status_get_state(struct wt_status_state *state,
 			state->rebase_in_progress = 1;
 		state->branch = read_and_strip_branch("rebase-merge/head-name");
 		state->onto = read_and_strip_branch("rebase-merge/onto");
-	} else if (!stat(git_path("CHERRY_PICK_HEAD"), &st) &&
-			!get_sha1("CHERRY_PICK_HEAD", sha1)) {
+	} else if (!read_ref("CHERRY_PICK_HEAD", sha1)) {
 		state->cherry_pick_in_progress = 1;
 		hashcpy(state->cherry_pick_head_sha1, sha1);
 	}
@@ -1345,8 +1344,7 @@ void wt_status_get_state(struct wt_status_state *state,
 		state->bisect_in_progress = 1;
 		state->branch = read_and_strip_branch("BISECT_START");
 	}
-	if (!stat(git_path("REVERT_HEAD"), &st) &&
-	    !get_sha1("REVERT_HEAD", sha1)) {
+	if (!read_ref("REVERT_HEAD", sha1)) {
 		state->revert_in_progress = 1;
 		hashcpy(state->revert_head_sha1, sha1);
 	}
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v6 2/7] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

[Michael Haggerty]

On 06/29/2015 10:17 PM, David Turner wrote:
> Instead of directly writing to and reading from files in
> $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
> and REVERT_HEAD.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  branch.c                         |  4 ++--
>  builtin/commit.c                 |  6 +++---
>  builtin/merge.c                  |  2 +-
>  contrib/completion/git-prompt.sh |  4 ++--
>  git-gui/lib/commit.tcl           |  2 +-
>  sequencer.c                      | 39 ++++++++++++++++++++-------------------
>  t/t7509-commit.sh                |  4 ++--
>  wt-status.c                      |  6 ++----
>  8 files changed, 33 insertions(+), 34 deletions(-)
> 
> [...]
> diff --git a/sequencer.c b/sequencer.c
> index f8421a8..44c43e5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -158,21 +158,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)
>  	unuse_commit_buffer(commit, msg->message);
>  }
>  
> -static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
> +static void write_cherry_pick_head(struct commit *commit, const char *ref)
>  {
> -	const char *filename;
> -	int fd;
> -	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
> +	void *transaction;
>  
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +	transaction = ref_transaction_begin(&err);
> +	if (!transaction)
> +		die(_("Could not create transaction: %s"), err.buf);
>  
> -	filename = git_path("%s", pseudoref);
> -	fd = open(filename, O_WRONLY | O_CREAT, 0666);
> -	if (fd < 0)
> -		die_errno(_("Could not open '%s' for writing"), filename);
> -	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
> -		die_errno(_("Could not write to '%s'"), filename);
> -	strbuf_release(&buf);
> +	if (ref_transaction_update(transaction, ref, commit->object.sha1,
> +				   NULL, REF_NODEREF, NULL,
> +				   &err))
> +		die(_("Could not write ref %s: %s"), ref, err.buf);
> +
> +	if (ref_transaction_commit(transaction, &err))
> +		die(_("Could not commit ref write %s: %s"), ref, err.buf);
>  }

I didn't check all the details, but this code looks a lot like what
update_ref() does. Maybe you can use that function?

>  static void print_advice(int show_hint, struct replay_opts *opts)
> @@ -186,7 +187,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> [...]

Otherwise, looks good to me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v6 3/7] bisect: treat BISECT_HEAD as a ref

[David Turner]

Instead of directly writing to and reading from files in
$GIT_DIR, use ref API to interact with BISECT_HEAD.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 git-bisect.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index ae3fec2..dddcc89 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -35,7 +35,7 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 
 bisect_head()
 {
-	if test -f "$GIT_DIR/BISECT_HEAD"
+	if bisect_head_exists
 	then
 		echo BISECT_HEAD
 	else
@@ -209,6 +209,10 @@ check_expected_revs() {
 	done
 }
 
+bisect_head_exists() {
+    git rev-parse --quiet --verify "BISECT_HEAD" >/dev/null
+}
+
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -310,7 +314,7 @@ bisect_next() {
 	bisect_next_check good
 
 	# Perform all bisection computation, display and checkout
-	git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
+	git bisect--helper --next-all $(bisect_head_exists && echo --no-checkout)
 	res=$?
 
 	# Check if we should exit because bisection is finished
@@ -377,7 +381,7 @@ bisect_reset() {
 		usage ;;
 	esac
 
-	if ! test -f "$GIT_DIR/BISECT_HEAD" && ! git checkout "$branch" --
+	if ! bisect_head_exists && ! git checkout "$branch" --
 	then
 		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
 Try 'git bisect reset <commit>'.")"
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v6 4/7] refs: Break out check for reflog autocreation

[David Turner]

This is just for clarity.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index c97ca02..30e81ba 100644
--- a/refs.c
+++ b/refs.c
@@ -3118,6 +3118,16 @@ static int copy_msg(char *buf, const char *msg)
 	return cp - buf;
 }
 
+static int should_autocreate_reflog(const char *refname)
+{
+	if (!log_all_ref_updates)
+		return 0;
+	return starts_with(refname, "refs/heads/") ||
+		starts_with(refname, "refs/remotes/") ||
+		starts_with(refname, "refs/notes/") ||
+		!strcmp(refname, "HEAD");
+}
+
 /* This function will fill in *err and return -1 on failure */
 int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
 {
@@ -3128,11 +3138,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
 	logfile = sb_logfile->buf;
 	/* make sure the rest of the function can't change "logfile" */
 	sb_logfile = NULL;
-	if (log_all_ref_updates &&
-	    (starts_with(refname, "refs/heads/") ||
-	     starts_with(refname, "refs/remotes/") ||
-	     starts_with(refname, "refs/notes/") ||
-	     !strcmp(refname, "HEAD"))) {
+	if (should_autocreate_reflog(refname)) {
 		if (safe_create_leading_directories(logfile) < 0) {
 			strbuf_addf(err, "unable to create directory for %s. "
 				    "%s", logfile, strerror(errno));
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v6 5/7] refs: new public ref function: safe_create_reflog

[David Turner]

The safe_create_reflog function creates a reflog, if it does not
already exist.

The log_ref_setup function becomes private and gains a force_create
parameter to force the creation of a reflog even if log_all_ref_updates
is false or the refname is not one of the special refnames.

The new parameter also reduces the need to store, modify, and restore
the log_all_ref_updates global before reflog creation.

In a moment, we will use this to add reflog creation commands to
git-reflog.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 builtin/checkout.c | 10 +---------
 refs.c             | 25 +++++++++++++++++++++----
 refs.h             |  2 +-
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 93f63d3..9f68399 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -620,19 +620,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
-				int temp;
-				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, &err);
-				log_all_ref_updates = temp;
-				strbuf_release(&log_file);
-				if (ret) {
+				if (safe_create_reflog(ref_name, &err, 1)) {
 					fprintf(stderr, _("Can not do reflog for '%s'. %s\n"),
 						opts->new_orphan_branch, err.buf);
 					strbuf_release(&err);
diff --git a/refs.c b/refs.c
index 30e81ba..1e53ef0 100644
--- a/refs.c
+++ b/refs.c
@@ -3128,8 +3128,14 @@ static int should_autocreate_reflog(const char *refname)
 		!strcmp(refname, "HEAD");
 }
 
-/* This function will fill in *err and return -1 on failure */
-int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
+/*
+ * This function creates a reflog for a ref.  If force_create = 0, the
+ * reflog will only be created for certain refs (those for which
+ * should_autocreate_reflog returns non-zero.  Otherwise, it will be
+ * created regardless of the ref name.  This function will fill in *err
+ * and return -1 on failure
+ */
+static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
 	char *logfile;
@@ -3138,7 +3144,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
 	logfile = sb_logfile->buf;
 	/* make sure the rest of the function can't change "logfile" */
 	sb_logfile = NULL;
-	if (should_autocreate_reflog(refname)) {
+	if (force_create || should_autocreate_reflog(refname)) {
 		if (safe_create_leading_directories(logfile) < 0) {
 			strbuf_addf(err, "unable to create directory for %s. "
 				    "%s", logfile, strerror(errno));
@@ -3173,6 +3179,17 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
 	return 0;
 }
 
+
+int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)
+{
+	int ret;
+	struct strbuf sb = STRBUF_INIT;
+
+	ret = log_ref_setup(refname, &sb, err, force_create);
+	strbuf_release(&sb);
+	return ret;
+}
+
 static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 			    const unsigned char *new_sha1,
 			    const char *committer, const char *msg)
@@ -3209,7 +3226,7 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, sb_log_file, err);
+	result = log_ref_setup(refname, sb_log_file, err, 0);
 
 	if (result)
 		return result;
diff --git a/refs.h b/refs.h
index debdefc..3b90e16 100644
--- a/refs.h
+++ b/refs.h
@@ -228,7 +228,7 @@ int pack_refs(unsigned int flags);
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
  */
-int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);
+int safe_create_reflog(const char *refname, struct strbuf *err, int force_create);
 
 /** Reads log for the value of ref during at_time. **/
 extern int read_ref_at(const char *refname, unsigned int flags,
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog

[Michael Haggerty]

On 06/29/2015 10:17 PM, David Turner wrote:
> The safe_create_reflog function creates a reflog, if it does not
> already exist.
> 
> The log_ref_setup function becomes private and gains a force_create
> parameter to force the creation of a reflog even if log_all_ref_updates
> is false or the refname is not one of the special refnames.
> 
> The new parameter also reduces the need to store, modify, and restore
> the log_all_ref_updates global before reflog creation.
> 
> In a moment, we will use this to add reflog creation commands to
> git-reflog.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  builtin/checkout.c | 10 +---------
>  refs.c             | 25 +++++++++++++++++++++----
>  refs.h             |  2 +-
>  3 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 93f63d3..9f68399 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -620,19 +620,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  	if (opts->new_branch) {
>  		if (opts->new_orphan_branch) {
>  			if (opts->new_branch_log && !log_all_ref_updates) {
> -				int temp;
> -				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, &err);
> -				log_all_ref_updates = temp;
> -				strbuf_release(&log_file);
> -				if (ret) {
> +				if (safe_create_reflog(ref_name, &err, 1)) {
>  					fprintf(stderr, _("Can not do reflog for '%s'. %s\n"),
>  						opts->new_orphan_branch, err.buf);
>  					strbuf_release(&err);

This was probably already dangerous before your change, but...

mkpath() returns a pointer to a static buffer. It is subject to being
overwritten if any of a number of path-related functions is called. So
passing it into a function is dangerous.

Instead, you should store it into memory that you control, for example
by using a strbuf and strbuf_addf().

Also, we usually call variables holding reference names "refname", not
"ref_name". Maybe rename the variable while you are in the area.

> diff --git a/refs.c b/refs.c
> index 30e81ba..1e53ef0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3128,8 +3128,14 @@ static int should_autocreate_reflog(const char *refname)
>  		!strcmp(refname, "HEAD");
>  }
>  
> -/* This function will fill in *err and return -1 on failure */
> -int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
> +/*
> + * This function creates a reflog for a ref.  If force_create = 0, the
> + * reflog will only be created for certain refs (those for which
> + * should_autocreate_reflog returns non-zero.  Otherwise, it will be
> + * created regardless of the ref name.  This function will fill in *err
> + * and return -1 on failure
> + */

It is preferable to write function docstrings in the imperative voice:

    Create a reflog for a ref. If force_create == 0, only create
    the reflog for certain refs...

> +static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
>  {
>  	int logfd, oflags = O_APPEND | O_WRONLY;
>  	char *logfile;
> @@ -3138,7 +3144,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
>  	logfile = sb_logfile->buf;
>  	/* make sure the rest of the function can't change "logfile" */
>  	sb_logfile = NULL;
> -	if (should_autocreate_reflog(refname)) {
> +	if (force_create || should_autocreate_reflog(refname)) {
>  		if (safe_create_leading_directories(logfile) < 0) {
>  			strbuf_addf(err, "unable to create directory for %s. "
>  				    "%s", logfile, strerror(errno));
> @@ -3173,6 +3179,17 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
>  	return 0;
>  }
>  
> +
> +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)
> +{
> +	int ret;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	ret = log_ref_setup(refname, &sb, err, force_create);
> +	strbuf_release(&sb);
> +	return ret;
> +}
> +

Is it really necessary to have two functions, safe_create_reflog() and
log_ref_setup()? I don't see any of the callers doing anything special
with the sb_logfile argument from the latter, so maybe it could be
inlined into safe_create_reflog()? Maybe I'm overlooking something.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog

[David Turner]

On Mon, 2015-07-06 at 18:21 +0200, Michael Haggerty wrote:

<snip> changes applied; will re-roll.

> > +
> > +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)
> > +{
> > +	int ret;
> > +	struct strbuf sb = STRBUF_INIT;
> > +
> > +	ret = log_ref_setup(refname, &sb, err, force_create);
> > +	strbuf_release(&sb);
> > +	return ret;
> > +}
> > +
> 
> Is it really necessary to have two functions, safe_create_reflog() and
> log_ref_setup()? I don't see any of the callers doing anything special
> with the sb_logfile argument from the latter, so maybe it could be
> inlined into safe_create_reflog()? Maybe I'm overlooking something.

log_ref_write_1 does use the sb_logfile argument.

Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog

[Michael Haggerty]

On 07/08/2015 01:18 AM, David Turner wrote:
> On Mon, 2015-07-06 at 18:21 +0200, Michael Haggerty wrote:
> 
> <snip> changes applied; will re-roll.
> 
>>> +
>>> +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)
>>> +{
>>> +	int ret;
>>> +	struct strbuf sb = STRBUF_INIT;
>>> +
>>> +	ret = log_ref_setup(refname, &sb, err, force_create);
>>> +	strbuf_release(&sb);
>>> +	return ret;
>>> +}
>>> +
>>
>> Is it really necessary to have two functions, safe_create_reflog() and
>> log_ref_setup()? I don't see any of the callers doing anything special
>> with the sb_logfile argument from the latter, so maybe it could be
>> inlined into safe_create_reflog()? Maybe I'm overlooking something.
> 
> log_ref_write_1 does use the sb_logfile argument.

Thanks for the clarification. I *did* overlook something.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH v6 6/7] git-reflog: add create and exists functions

[David Turner]

These are necessary because alternate ref backends might store reflogs
somewhere other than .git/logs.  Code that now directly manipulates
.git/logs should instead go through git-reflog.

In a moment, we will use these functions to make git stash work with
alternate ref backends.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 Documentation/git-reflog.txt | 10 ++++++
 builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-
 t/t1411-reflog-show.sh       | 12 +++++++
 3 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 5e7908e..2bf8aa6 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -23,6 +23,8 @@ depending on the subcommand:
 	[--dry-run] [--verbose] [--all | <refs>...]
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run] [--verbose] ref@\{specifier\}...
+'git reflog create' <refs>...
+'git reflog exists' <ref>
 
 Reference logs, or "reflogs", record when the tips of branches and
 other references were updated in the local repository. Reflogs are
@@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. "`git reflog delete
 master@{2}`"). This subcommand is also typically not used directly by
 end users.
 
+The "create" subcommand creates a reflog for one or more refs. Most
+refs (those under refs/heads, refs/remotes, and refs/tags) will
+automatically have reflogs created. Other refs will not. This command
+allows manual ref creation.
+
+The "exists" subcommand checks whether a ref has a reflog.  It exists
+with zero status if the reflog exists, and non-zero status if it does
+not.
 
 OPTIONS
 -------
diff --git a/builtin/reflog.c b/builtin/reflog.c
index c2eb8ff..3080865 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =
 "git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
 static const char reflog_delete_usage[] =
 "git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
+static const char reflog_create_usage[] =
+"git reflog create <refs>...";
+static const char reflog_exists_usage[] =
+"git reflog exists <ref>";
 
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
@@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
+{
+	int i, status = 0, start = 0;
+	struct strbuf err = STRBUF_INIT;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		else if (arg[0] == '-')
+			usage(reflog_create_usage);
+		else
+			break;
+	}
+
+	start = i;
+
+	if (argc - start < 1)
+		return error("Nothing to create?");
+
+	for (i = start; i < argc; i++) {
+		if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
+			die("invalid ref format: %s", argv[i]);
+	}
+	for (i = start; i < argc; i++) {
+		if (safe_create_reflog(argv[i], &err, 1)) {
+			error("could not create reflog %s: %s", argv[i],
+			      err.buf);
+			status = 1;
+			strbuf_release(&err);
+		}
+	}
+	return status;
+}
+
+static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
+{
+	int i, start = 0;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		else if (arg[0] == '-')
+			usage(reflog_exists_usage);
+		else
+			break;
+	}
+
+	start = i;
+
+	if (argc - start != 1)
+		usage(reflog_exists_usage);
+
+	if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
+		die("invalid ref format: %s", argv[start]);
+	return !reflog_exists(argv[start]);
+}
+
 /*
  * main "reflog"
  */
 
 static const char reflog_usage[] =
-"git reflog [ show | expire | delete ]";
+"git reflog [ show | expire | delete | create | exists ]";
 
 int cmd_reflog(int argc, const char **argv, const char *prefix)
 {
@@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[1], "delete"))
 		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
 
+	if (!strcmp(argv[1], "create"))
+		return cmd_reflog_create(argc - 1, argv + 1, prefix);
+
+	if (!strcmp(argv[1], "exists"))
+		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
+
 	return cmd_log_reflog(argc, argv, prefix);
 }
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 6f47c0d..6e1abe7 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '
 	test_cmp expect actual
 '
 
+test_expect_success 'reflog exists works' '
+	git reflog exists refs/heads/master &&
+	! git reflog exists refs/heads/nonexistent
+'
+
+test_expect_success 'reflog create works' '
+	git update-ref non-refs-dir HEAD &&
+	! git reflog exists non-refs-dir &&
+	git reflog create non-refs-dir &&
+	git reflog exists non-refs-dir
+'
+
 test_done
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Eric Sunshine]

On Mon, Jun 29, 2015 at 4:17 PM, David Turner <dturner@twopensource.com> wrote:
> These are necessary because alternate ref backends might store reflogs
> somewhere other than .git/logs.  Code that now directly manipulates
> .git/logs should instead go through git-reflog.
>
> In a moment, we will use these functions to make git stash work with
> alternate ref backends.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index c2eb8ff..3080865 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =
> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
> +{
> +       int i, status = 0, start = 0;
> +       struct strbuf err = STRBUF_INIT;
> +
> +       for (i = 1; i < argc; i++) {
> +               const char *arg = argv[i];
> +               if (!strcmp(arg, "--")) {
> +                       i++;
> +                       break;
> +               }
> +               else if (arg[0] == '-')
> +                       usage(reflog_create_usage);
> +               else
> +                       break;
> +       }
> +
> +       start = i;
> +
> +       if (argc - start < 1)
> +               return error("Nothing to create?");
> +
> +       for (i = start; i < argc; i++) {
> +               if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
> +                       die("invalid ref format: %s", argv[i]);
> +       }
> +       for (i = start; i < argc; i++) {
> +               if (safe_create_reflog(argv[i], &err, 1)) {
> +                       error("could not create reflog %s: %s", argv[i],
> +                             err.buf);
> +                       status = 1;
> +                       strbuf_release(&err);

This feels a bit dirty. While it's true that the current
implementation of strbuf_release() re-initializes the strbuf (so
you're not technically wrong by re-using it), that's an implementation
detail; and indeed, the strbuf_release() documentation says:

    Release a string buffer and the memory it used. You should not
    use the string buffer after using this function, unless you
    initialize it again.

Alternatives would be strbuf_reset() or declaring and releasing the
strbuf within the for-loop scope.

> +               }
> +       }
> +       return status;
> +}

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[David Turner]

On Tue, 2015-06-30 at 03:34 -0400, Eric Sunshine wrote:
> > +                       strbuf_release(&err);
> 
> This feels a bit dirty. While it's true that the current

Thanks.  New patchset pushed to the branch on github:

https://github.com/dturner-tw/git.git
dturner/pluggable-backends-preamble

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       for (i = start; i < argc; i++) {
>> +               if (safe_create_reflog(argv[i], &err, 1)) {
>> +                       error("could not create reflog %s: %s", argv[i],
>> +                             err.buf);
>> +                       status = 1;
>> +                       strbuf_release(&err);
>
> This feels a bit dirty.

Hmm, I do not share that feeling.  I wouldn't be surprised if you
found a lot of existing codepaths that run _init() a strbuf once,
work on it and then _release() once a section of code is done with
it, reuse it for further (unrelated) processing, knowing that
_release() implicitly did _init() and the strbuf is ready to use
after that.  I thought that was a very well established pattern.

> While it's true that the current
> implementation of strbuf_release() re-initializes the strbuf (so
> you're not technically wrong by re-using it), that's an implementation
> detail; and indeed, the strbuf_release() documentation says:
>
>     Release a string buffer and the memory it used. You should not
>     use the string buffer after using this function, unless you
>     initialize it again.

Hmph. Perhaps the doc is wrong? ;-)

> Alternatives would be strbuf_reset() or declaring and releasing the
> strbuf within the for-loop scope.

Because _reset() just rewinds the .len pointer without deallocating,
you would need an extra _release() before it goes out of scope. If
it is expected that the strbuf will be reused for a number of times,
the length of the string each iteration uses is similar, and you
will iterate the loop many times, "_reset() each time and _release()
to clean-up" pattern would save many calls to realloc/free.

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Eric Sunshine]

On Tue, Jun 30, 2015 at 12:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       for (i = start; i < argc; i++) {
>>> +               if (safe_create_reflog(argv[i], &err, 1)) {
>>> +                       error("could not create reflog %s: %s", argv[i],
>>> +                             err.buf);
>>> +                       status = 1;
>>> +                       strbuf_release(&err);
>>
>> This feels a bit dirty.
>
> Hmm, I do not share that feeling.  I wouldn't be surprised if you
> found a lot of existing codepaths that run _init() a strbuf once,
> work on it and then _release() once a section of code is done with
> it, reuse it for further (unrelated) processing, knowing that
> _release() implicitly did _init() and the strbuf is ready to use
> after that.  I thought that was a very well established pattern.

That could the case, and I may not be familiar with code doing that.

I have, however, seen plenty of code which uses strbuf_reset() in the
way you describe.

>> While it's true that the current
>> implementation of strbuf_release() re-initializes the strbuf (so
>> you're not technically wrong by re-using it), that's an implementation
>> detail; and indeed, the strbuf_release() documentation says:
>>
>>     Release a string buffer and the memory it used. You should not
>>     use the string buffer after using this function, unless you
>>     initialize it again.
>
> Hmph. Perhaps the doc is wrong? ;-)

Perhaps. I always interpreted the documentation as meaning that the
project reserved the right to change the underlying implementation.

>> Alternatives would be strbuf_reset() or declaring and releasing the
>> strbuf within the for-loop scope.
>
> Because _reset() just rewinds the .len pointer without deallocating,
> you would need an extra _release() before it goes out of scope. If
> it is expected that the strbuf will be reused for a number of times,
> the length of the string each iteration uses is similar, and you
> will iterate the loop many times, "_reset() each time and _release()
> to clean-up" pattern would save many calls to realloc/free.

Yep, that's why I suggested strbuf_reset() as an alternative (and
likely would have chosen it myself).

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>> Alternatives would be strbuf_reset() or declaring and releasing the
>>> strbuf within the for-loop scope.
>>
>> Because _reset() just rewinds the .len pointer without deallocating,
>> you would need an extra _release() before it goes out of scope. If
>> it is expected that the strbuf will be reused for a number of times,
>> the length of the string each iteration uses is similar, and you
>> will iterate the loop many times, "_reset() each time and _release()
>> to clean-up" pattern would save many calls to realloc/free.
>
> Yep, that's why I suggested strbuf_reset() as an alternative (and
> likely would have chosen it myself).

OK, then let's do that by squashing this in.

 builtin/reflog.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3080865..e9ba600 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -733,10 +733,11 @@ static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
 		if (safe_create_reflog(argv[i], &err, 1)) {
 			error("could not create reflog %s: %s", argv[i],
 			      err.buf);
+			strbuf_reset(&err);
 			status = 1;
-			strbuf_release(&err);
 		}
 	}
+	strbuf_release(&err);
 	return status;
 }
 

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[David Turner]

On Tue, 2015-06-30 at 12:48 -0700, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >>> Alternatives would be strbuf_reset() or declaring and releasing the
> >>> strbuf within the for-loop scope.
> >>
> >> Because _reset() just rewinds the .len pointer without deallocating,
> >> you would need an extra _release() before it goes out of scope. If
> >> it is expected that the strbuf will be reused for a number of times,
> >> the length of the string each iteration uses is similar, and you
> >> will iterate the loop many times, "_reset() each time and _release()
> >> to clean-up" pattern would save many calls to realloc/free.
> >
> > Yep, that's why I suggested strbuf_reset() as an alternative (and
> > likely would have chosen it myself).
> 
> OK, then let's do that by squashing this in.
> 
>  builtin/reflog.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

I squashed that into my repo on github:

https://github.com/dturner-tw/git.git
on the branch dturner/pluggable-backends-preamble

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Tue, 2015-06-30 at 12:48 -0700, Junio C Hamano wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> 
>> >>> Alternatives would be strbuf_reset() or declaring and releasing the
>> >>> strbuf within the for-loop scope.
>> >>
>> >> Because _reset() just rewinds the .len pointer without deallocating,
>> >> you would need an extra _release() before it goes out of scope. If
>> >> it is expected that the strbuf will be reused for a number of times,
>> >> the length of the string each iteration uses is similar, and you
>> >> will iterate the loop many times, "_reset() each time and _release()
>> >> to clean-up" pattern would save many calls to realloc/free.
>> >
>> > Yep, that's why I suggested strbuf_reset() as an alternative (and
>> > likely would have chosen it myself).
>> 
>> OK, then let's do that by squashing this in.
>> 
>>  builtin/reflog.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> I squashed that into my repo on github:

I'm already deep into today's integration cycle, so it is unlikely
I'd pull that before I push the result out.  Please let me know if
the resulting tree looks wrong (I only queued it to be squashed,
haven't done the squashing two into one yet).

Thanks.  The 7-patch series with this fixup looks good to me.

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Michael Haggerty]

On 06/29/2015 10:17 PM, David Turner wrote:
> These are necessary because alternate ref backends might store reflogs
> somewhere other than .git/logs.  Code that now directly manipulates
> .git/logs should instead go through git-reflog.
> 
> In a moment, we will use these functions to make git stash work with
> alternate ref backends.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  Documentation/git-reflog.txt | 10 ++++++
>  builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-
>  t/t1411-reflog-show.sh       | 12 +++++++
>  3 files changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index 5e7908e..2bf8aa6 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -23,6 +23,8 @@ depending on the subcommand:
>  	[--dry-run] [--verbose] [--all | <refs>...]
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run] [--verbose] ref@\{specifier\}...
> +'git reflog create' <refs>...
> +'git reflog exists' <ref>
>  
>  Reference logs, or "reflogs", record when the tips of branches and
>  other references were updated in the local repository. Reflogs are
> @@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. "`git reflog delete
>  master@{2}`"). This subcommand is also typically not used directly by
>  end users.
>  
> +The "create" subcommand creates a reflog for one or more refs. Most
> +refs (those under refs/heads, refs/remotes, and refs/tags) will
> +automatically have reflogs created. Other refs will not. This command
> +allows manual ref creation.
> +
> +The "exists" subcommand checks whether a ref has a reflog.  It exists
> +with zero status if the reflog exists, and non-zero status if it does
> +not.
>  
>  OPTIONS
>  -------
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index c2eb8ff..3080865 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =
>  "git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
>  static const char reflog_delete_usage[] =
>  "git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
> +static const char reflog_create_usage[] =
> +"git reflog create <refs>...";
> +static const char reflog_exists_usage[] =
> +"git reflog exists <ref>";
>  
>  static unsigned long default_reflog_expire;
>  static unsigned long default_reflog_expire_unreachable;
> @@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	return status;
>  }
>  
> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
> +{
> +	int i, status = 0, start = 0;

It looks like start is initialized unconditionally after the first loop,
so the initialization here is a red herring.

> +	struct strbuf err = STRBUF_INIT;
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		else if (arg[0] == '-')
> +			usage(reflog_create_usage);
> +		else
> +			break;
> +	}
> +
> +	start = i;
> +
> +	if (argc - start < 1)

    if (start == argc)

seems clearer, but that might just be me.

> +		return error("Nothing to create?");
> +
> +	for (i = start; i < argc; i++) {
> +		if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
> +			die("invalid ref format: %s", argv[i]);
> +	}
> +	for (i = start; i < argc; i++) {
> +		if (safe_create_reflog(argv[i], &err, 1)) {
> +			error("could not create reflog %s: %s", argv[i],
> +			      err.buf);
> +			status = 1;
> +			strbuf_release(&err);
> +		}
> +	}
> +	return status;
> +}
> +

So, I have a philosophical question here with a practical side...

It appears that this command allows you to create a reflog for a
reference that doesn't yet exist. At first blush, this seems to make
sense. Suppose you want the creation of a reference to be logged. Then
you can first turn on its reflog, then create it.

But I think this is going to create problems. Reflogs are rather
intimately connected to their references. For example, writes to a
reflog are guarded by locking the corresponding reference. And currently
a reflog file is deleted when the corresponding reference is deleted.
Also, there are constraints about which references can coexist; for
example, references "refs/heads/foo" and "refs/heads/foo/bar" cannot
exist at the same time, because (at least when using the filesystem
backend), "refs/heads/foo" would have to be both a file and a directory
at the same time. So if one of these references already exists, it would
be an error to create a reflog for the other one.

Similarly, if there is a reflog file for one of these references that
was created by this command, but there is not yet a corresponding
reference, then any command that later tries to create the other
reference will not be able to create the reflog for *that* reference
because it will conflict with the existing reflog. I doubt that the
reference creation is smart enough to deal with this situation.

So all in all, I think it is unwise to allow a reflog to be created
without its corresponding reference.

This, in turn, suggests one or both of the following alternatives:

1. Allow "git reflog create", but only for references that already exist.

2. If we want to allow a reflog to be created at the same time as the
corresponding reference, the reference-creation commands ("git branch",
"git tag", "git update-ref", and maybe some others) probably need a new
option like "--create-reflog" (and, for symmetry, probably
"--no-create-reflog").

At the API level, it might make sense for the ref-transaction functions
to get a new "REF_FORCE_CREATE_REFLOG" flag or something.

> +static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
> +{
> +	int i, start = 0;
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		else if (arg[0] == '-')
> +			usage(reflog_exists_usage);
> +		else
> +			break;
> +	}
> +
> +	start = i;
> +
> +	if (argc - start != 1)
> +		usage(reflog_exists_usage);
> +
> +	if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
> +		die("invalid ref format: %s", argv[start]);
> +	return !reflog_exists(argv[start]);
> +}
> +
>  /*
>   * main "reflog"
>   */
>  
>  static const char reflog_usage[] =
> -"git reflog [ show | expire | delete ]";
> +"git reflog [ show | expire | delete | create | exists ]";
>  
>  int cmd_reflog(int argc, const char **argv, const char *prefix)
>  {
> @@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[1], "delete"))
>  		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
>  
> +	if (!strcmp(argv[1], "create"))
> +		return cmd_reflog_create(argc - 1, argv + 1, prefix);
> +
> +	if (!strcmp(argv[1], "exists"))
> +		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
> +
>  	return cmd_log_reflog(argc, argv, prefix);
>  }
> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
> index 6f47c0d..6e1abe7 100755
> --- a/t/t1411-reflog-show.sh
> +++ b/t/t1411-reflog-show.sh
> @@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'reflog exists works' '
> +	git reflog exists refs/heads/master &&
> +	! git reflog exists refs/heads/nonexistent
> +'
> +
> +test_expect_success 'reflog create works' '
> +	git update-ref non-refs-dir HEAD &&
> +	! git reflog exists non-refs-dir &&
> +	git reflog create non-refs-dir &&
> +	git reflog exists non-refs-dir
> +'
> +
>  test_done
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[David Turner]

On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:

> > +{
> > +	int i, status = 0, start = 0;
> 
> It looks like start is initialized unconditionally after the first loop,
> so the initialization here is a red herring.

Will fix.

> So, I have a philosophical question here with a practical side...
> 
> It appears that this command allows you to create a reflog for a
> reference that doesn't yet exist. At first blush, this seems to make
> sense. Suppose you want the creation of a reference to be logged. Then
> you can first turn on its reflog, then create it.
> 
> But I think this is going to create problems. Reflogs are rather
> intimately connected to their references. For example, writes to a
> reflog are guarded by locking the corresponding reference. And currently
> a reflog file is deleted when the corresponding reference is deleted.
> Also, there are constraints about which references can coexist; for
> example, references "refs/heads/foo" and "refs/heads/foo/bar" cannot
> exist at the same time, because (at least when using the filesystem
> backend), "refs/heads/foo" would have to be both a file and a directory
> at the same time. So if one of these references already exists, it would
> be an error to create a reflog for the other one.
> 
> Similarly, if there is a reflog file for one of these references that
> was created by this command, but there is not yet a corresponding
> reference, then any command that later tries to create the other
> reference will not be able to create the reflog for *that* reference
> because it will conflict with the existing reflog. I doubt that the
> reference creation is smart enough to deal with this situation.
> 
> So all in all, I think it is unwise to allow a reflog to be created
> without its corresponding reference.
> 
> This, in turn, suggests one or both of the following alternatives:
> 
> 1. Allow "git reflog create", but only for references that already exist.

This turns out not to work for git stash, which wants to create a reflog
for stash creation.

> 2. If we want to allow a reflog to be created at the same time as the
> corresponding reference, the reference-creation commands ("git branch",
> "git tag", "git update-ref", and maybe some others) probably need a new
> option like "--create-reflog" (and, for symmetry, probably
> "--no-create-reflog").

git branch should already autocreate reflogs, since the refs it creates
are under refs/heads.

> At the API level, it might make sense for the ref-transaction functions
> to get a new "REF_FORCE_CREATE_REFLOG" flag or something.

Junio was opposed to the converse flag, so I'm going to just add
manually add code to create reflogs.

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[Michael Haggerty]

On 07/08/2015 02:49 AM, David Turner wrote:
> On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:
>> [...]
>> So all in all, I think it is unwise to allow a reflog to be created
>> without its corresponding reference.
>>
>> This, in turn, suggests one or both of the following alternatives:
>>
>> 1. Allow "git reflog create", but only for references that already exist.
> 
> This turns out not to work for git stash, which wants to create a reflog
> for stash creation.
> 
>> 2. If we want to allow a reflog to be created at the same time as the
>> corresponding reference, the reference-creation commands ("git branch",
>> "git tag", "git update-ref", and maybe some others) probably need a new
>> option like "--create-reflog" (and, for symmetry, probably
>> "--no-create-reflog").
> 
> git branch should already autocreate reflogs, since the refs it creates
> are under refs/heads.

`git branch` only autocreates reflogs if core.logAllRefUpdates is on.
That setting happens to be on by default in a non-bare repository but
the user might turn it off. And it is off by default in a bare repository.

In my opinion it would be nice for the user to be able to ask for a
reflog to be created for a branch regardless of how
core.logAllRefUpdates is set. Though I'm not saying that you have to be
the one to implement that functionality :-)

>> At the API level, it might make sense for the ref-transaction functions
>> to get a new "REF_FORCE_CREATE_REFLOG" flag or something.
> 
> Junio was opposed to the converse flag, so I'm going to just add
> manually add code to create reflogs.

Unfortunately I wasn't keeping up with earlier versions of this patch
series and now I can't find the email from Junio that you are referring
to. If the earlier flag had the opposite ("converse"?) sense, like
REF_INHIBIT_CREATE_REFLOG, then I agree that it wouldn't be an improvement.

But I think this functionality *has to* be implemented within ref
transactions for references that are just being created, because

1. The reflog must *not* be created if the reference creation fails for
some reason. For example, the reflog shouldn't be created if the
reference name has a D/F conflict with an existing one in the "refs/foo"
vs. "refs/foo/bar" sense. (This conflict might not be obvious when
creating the reflog file because the other reference might not have its
reflog turned on.) There are other reasons that a reference creation
might fail, and code outside of the refs API can't be expected to know
all possibilities.

2. On the other hand, the reflog for a newly-created reference *should*
reflect the creation of the reference. So it would be awkward to require
the calling code to create the reference and *then* turn on the reflog.

For references that already exist, I see no problem with a command that
turns on the reflog without adding any entries to it. Though if you
implement this, it would be prudent to check that existing
reflog-handling code doesn't fail when confronted with an empty file; I
think empty reflog files are rare now and might not be well-tested.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v6 6/7] git-reflog: add create and exists functions

[David Turner]

On Wed, 2015-07-08 at 15:16 +0200, Michael Haggerty wrote:
> On 07/08/2015 02:49 AM, David Turner wrote:
> > On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:
> >> [...]
> >> So all in all, I think it is unwise to allow a reflog to be created
> >> without its corresponding reference.
> >>
> >> This, in turn, suggests one or both of the following alternatives:
> >>
> >> 1. Allow "git reflog create", but only for references that already exist.
> > 
> > This turns out not to work for git stash, which wants to create a reflog
> > for stash creation.
> > 
> >> 2. If we want to allow a reflog to be created at the same time as the
> >> corresponding reference, the reference-creation commands ("git branch",
> >> "git tag", "git update-ref", and maybe some others) probably need a new
> >> option like "--create-reflog" (and, for symmetry, probably
> >> "--no-create-reflog").
> > 
> > git branch should already autocreate reflogs, since the refs it creates
> > are under refs/heads.
> 
> `git branch` only autocreates reflogs if core.logAllRefUpdates is on.
> That setting happens to be on by default in a non-bare repository but
> the user might turn it off. And it is off by default in a bare repository.

Oh, right.  Well, we can always add that later, if anyone needs it.

> In my opinion it would be nice for the user to be able to ask for a
> reflog to be created for a branch regardless of how
> core.logAllRefUpdates is set. Though I'm not saying that you have to be
> the one to implement that functionality :-)
> 
> >> At the API level, it might make sense for the ref-transaction functions
> >> to get a new "REF_FORCE_CREATE_REFLOG" flag or something.
> > 
> > Junio was opposed to the converse flag, so I'm going to just add
> > manually add code to create reflogs.
> 
> Unfortunately I wasn't keeping up with earlier versions of this patch
> series and now I can't find the email from Junio that you are referring
> to. If the earlier flag had the opposite ("converse"?) sense, like
> REF_INHIBIT_CREATE_REFLOG, then I agree that it wouldn't be an improvement.
> 
> But I think this functionality *has to* be implemented within ref
> transactions for references that are just being created, because
> 
> 1. The reflog must *not* be created if the reference creation fails for
> some reason. For example, the reflog shouldn't be created if the
> reference name has a D/F conflict with an existing one in the "refs/foo"
> vs. "refs/foo/bar" sense. (This conflict might not be obvious when
> creating the reflog file because the other reference might not have its
> reflog turned on.) There are other reasons that a reference creation
> might fail, and code outside of the refs API can't be expected to know
> all possibilities.
> 
> 2. On the other hand, the reflog for a newly-created reference *should*
> reflect the creation of the reference. So it would be awkward to require
> the calling code to create the reference and *then* turn on the reflog.

Yep, makes sense.

> For references that already exist, I see no problem with a command that
> turns on the reflog without adding any entries to it. Though if you
> implement this, it would be prudent to check that existing
> reflog-handling code doesn't fail when confronted with an empty file; I
> think empty reflog files are rare now and might not be well-tested.

I think empty reflog files are fine; I recall a few tests creating them
(and git stash did so as well).

But I also don't need that command (I think), so I won't implement it
right now.

[PATCH v6 7/7] git-stash: use git-reflog instead of creating files

[David Turner]

This is in support of alternate ref backends which don't necessarily
store reflogs as files.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 git-stash.sh | 4 ++--
 refs.c       | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 8e9e2cd..27155bc 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -184,7 +184,7 @@ store_stash () {
 	fi
 
 	# Make sure the reflog for stash is kept.
-	: >>"$(git rev-parse --git-path logs/$ref_stash)"
+	git reflog create "$ref_stash"
 	git update-ref -m "$stash_msg" $ref_stash $w_commit
 	ret=$?
 	test $ret != 0 && test -z $quiet &&
@@ -262,7 +262,7 @@ save_stash () {
 		say "$(gettext "No local changes to save")"
 		exit 0
 	fi
-	test -f "$(git rev-parse --git-path logs/$ref_stash)" ||
+	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
 	create_stash "$stash_msg" $untracked
diff --git a/refs.c b/refs.c
index 1e53ef0..0f240c9 100644
--- a/refs.c
+++ b/refs.c
@@ -3125,6 +3125,7 @@ static int should_autocreate_reflog(const char *refname)
 	return starts_with(refname, "refs/heads/") ||
 		starts_with(refname, "refs/remotes/") ||
 		starts_with(refname, "refs/notes/") ||
+		!strcmp(refname, "refs/stash") ||
 		!strcmp(refname, "HEAD");
 }
 
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v6 7/7] git-stash: use git-reflog instead of creating files

[Junio C Hamano]

OK, I'll squash in the last bit and restart today's integration run.
Everything looks much nicer than any previous versions ;-)

Thanks.

Re: [PATCH v6 0/7] refs backend preamble

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Minor formatting fixes from Junio Hamano.

There is another.

By the way, "unused variable" is not a formatting fix.


 git-bisect.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index dddcc89..2fd8ea6 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -210,7 +210,7 @@ check_expected_revs() {
 }
 
 bisect_head_exists() {
-    git rev-parse --quiet --verify "BISECT_HEAD" >/dev/null
+	git rev-parse --quiet --verify "BISECT_HEAD" >/dev/null
 }
 
 bisect_skip() {

Re: [PATCH v6 0/7] refs backend preamble

[David Turner]

On Mon, 2015-06-29 at 13:31 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Minor formatting fixes from Junio Hamano.
> 
> There is another.

Would it be simpler for me to push this sort of fixup to github and and
just mention that a new patchset is available?  If so, here it is: 

https://github.com/dturner-tw/git.git
dturner/pluggable-backends-preamble

If not, I'll re-send.

Of course for real reviews, I'll continue sending patches to the mailing
list using git send-email.

> By the way, "unused variable" is not a formatting fix.

Indeed, it is not.