[PATCH 0/6] refs backend preamble

[PATCH 0/6] refs backend preamble

[David Turner]

This set of patches is a preamble to pluggable ref backends.  The
intent is to use the ref infrastructure for special refs like
CHERRY_PICK_HEAD where possible, and to use git plumbing commands to
access refs and reflogs instead of directly writing files to the .git
directory.

This series is on top of pu at 9039c98c.

[PATCH 1/6] refs.c: add an err argument to log_ref_setup

[David Turner]

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

write_ref_to_lockfile is a private function that calls
log_ref_setup. Update this function to also take an err argument and
fill it in.  This again eliminates the need to manage errno in this
function.

Update of a patch by Ronnie Sahlberg.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: David Turner <dturner@twitter.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..ac4d10a 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..b34a54a 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.314.gdbf7a51-twtrsrc

[PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD

[David Turner]

Also use refs infrastructure for REVERT_HEAD.  These refs
need to go through the refs backend, since some code
assumes that they can be read as refs.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 branch.c                         |  4 ++--
 builtin/commit.c                 |  7 ++++---
 builtin/merge.c                  |  3 ++-
 contrib/completion/git-prompt.sh |  9 +++++----
 git-gui/lib/commit.tcl           |  2 +-
 refs.c                           | 17 +++++++++--------
 refs.h                           |  5 +++++
 sequencer.c                      | 39 +++++++++++++++++++++------------------
 t/t7509-commit.sh                |  4 ++--
 wt-status.c                      |  6 ++----
 10 files changed, 53 insertions(+), 43 deletions(-)

diff --git a/branch.c b/branch.c
index b002435..8371a16 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 | REF_NO_REFLOG);
+	delete_ref("REVERT_HEAD", NULL, REF_NODEREF | REF_NO_REFLOG);
 	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..0f0b184 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -166,9 +166,10 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 
 static void determine_whence(struct wt_status *s)
 {
+	unsigned char unused[20];
 	if (file_exists(git_path("MERGE_HEAD")))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
+	else if (!read_ref("CHERRY_PICK_HEAD", unused)) {
 		whence = FROM_CHERRY_PICK;
 		if (file_exists(git_path(SEQ_DIR)))
 			sequencer_in_use = 1;
@@ -1777,8 +1778,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 | REF_NO_REFLOG);
+	delete_ref("REVERT_HEAD", NULL, REF_NODEREF | REF_NO_REFLOG);
 	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..a4abf93 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1144,6 +1144,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	unsigned char result_tree[20];
 	unsigned char stash[20];
 	unsigned char head_sha1[20];
+	unsigned char unused[20];
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	int flag, i, ret = 0, head_subsumed;
@@ -1206,7 +1207,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 (!read_ref("CHERRY_PICK_HEAD", unused)) {
 		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..5e27a34 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"
@@ -429,8 +429,9 @@ __git_ps1 ()
 			# symlink symbolic ref
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
-			local head=""
-			if ! __git_eread "$g/HEAD" head; then
+			local head
+			head="ref: $(git symbolic-ref HEAD 2>/dev/null)" || head=$(git rev-parse HEAD)
+			if [ -z "$head" ]; then
 				return $exit
 			fi
 			# is it a symbolic ref?
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/refs.c b/refs.c
index b34a54a..c1a563f 100644
--- a/refs.c
+++ b/refs.c
@@ -2979,7 +2979,7 @@ 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,
-			     struct strbuf *err);
+			     struct strbuf *err, int flags);
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
@@ -3041,7 +3041,7 @@ 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, &err) ||
-	    commit_ref_update(lock, orig_sha1, logmsg, &err)) {
+	    commit_ref_update(lock, orig_sha1, logmsg, &err, 0)) {
 		error("unable to write current sha1 into %s: %s", newrefname, err.buf);
 		strbuf_release(&err);
 		goto rollback;
@@ -3060,7 +3060,7 @@ 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, &err) ||
-	    commit_ref_update(lock, orig_sha1, NULL, &err)) {
+	    commit_ref_update(lock, orig_sha1, NULL, &err, 0)) {
 		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
 		strbuf_release(&err);
 	}
@@ -3291,12 +3291,13 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
  */
 static int commit_ref_update(struct ref_lock *lock,
 			     const unsigned char *sha1, const char *logmsg,
-			     struct strbuf *err)
+			     struct strbuf *err, int flags)
 {
 	clear_loose_ref_cache(&ref_cache);
-	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, err) < 0)) {
+	if (!(flags & REF_NO_REFLOG) &&
+	    (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, err) < 0))) {
 		strbuf_addf(err, "Cannot update the ref '%s'.",
 			    lock->ref_name);
 		unlock_ref(lock);
@@ -4063,7 +4064,7 @@ 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, err)) {
+					      update->new_sha1, update->msg, err, 0)) {
 				/* freed by commit_ref_update(): */
 				update->lock = NULL;
 				ret = TRANSACTION_GENERIC_ERROR;
diff --git a/refs.h b/refs.h
index debdefc..eee99f6 100644
--- a/refs.h
+++ b/refs.h
@@ -153,6 +153,11 @@ struct ref_transaction;
 #define REF_BAD_NAME 0x08
 
 /*
+ * Used as a flag in ref_update::flags when the reflog should not be updated
+ */
+#define REF_NO_REFLOG	0x40
+
+/*
  * The signature for the callback function for the for_each_*()
  * functions below.  The memory pointed to by the refname and sha1
  * arguments is only guaranteed to be valid for the duration of a
diff --git a/sequencer.c b/sequencer.c
index f8421a8..f66e2be 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -160,19 +160,20 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 
 static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
 {
-	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, pseudoref, commit->object.sha1,
+				   NULL, REF_NODEREF | REF_NO_REFLOG, NULL,
+				   &err))
+		die(_("Could not write ref %s: %s"), pseudoref, err.buf);
+
+	if (ref_transaction_commit(transaction, &err))
+		die(_("Could not commit ref write %s: %s"), pseudoref, 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 (read_ref("CHERRY_PICK_HEAD", head_sha1) &&
+	    read_ref("REVERT_HEAD", head_sha1))
 		return error(_("no cherry-pick or revert in progress"));
 	if (read_ref_full("HEAD", 0, head_sha1, NULL))
 		return error(_("cannot resolve HEAD"));
@@ -1013,9 +1014,10 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 static int continue_single_pick(void)
 {
 	const char *argv[] = { "commit", NULL };
+	unsigned char unused[20];
 
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
+	if (read_ref("CHERRY_PICK_HEAD", unused) &&
+	    read_ref("REVERT_HEAD", unused))
 		return error(_("no cherry-pick or revert in progress"));
 	return run_command_v_opt(argv, RUN_GIT_CMD);
 }
@@ -1023,6 +1025,7 @@ static int continue_single_pick(void)
 static int sequencer_continue(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
+	unsigned char unused[20];
 
 	if (!file_exists(git_path(SEQ_TODO_FILE)))
 		return continue_single_pick();
@@ -1030,8 +1033,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 (!read_ref("CHERRY_PICK_HEAD", unused) ||
+	    !read_ref("REVERT_HEAD", unused)) {
 		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.314.gdbf7a51-twtrsrc

[PATCH 3/6] bisect: use refs infrastructure for BISECT_START

[David Turner]

This ref needs to go through the refs backend, since some code assumes
that it can be written and read as a ref.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 contrib/completion/git-completion.bash | 2 +-
 git-bisect.sh                          | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 93716c4..c4d4d80 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -938,7 +938,7 @@ _git_bisect ()
 	local subcommands="start bad good skip reset visualize replay log run"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
-		if [ -f "$(__gitdir)"/BISECT_START ]; then
+		if [ git rev-parse BISECT_START 2>/dev/null ]; then
 			__gitcomp "$subcommands"
 		else
 			__gitcomp "replay start"
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3fec2..8658772 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -166,7 +166,7 @@ bisect_start() {
 	#
 	echo "$start_head" >"$GIT_DIR/BISECT_START" && {
 		test "z$mode" != "z--no-checkout" ||
-		git update-ref --no-deref BISECT_HEAD "$start_head"
+		git rev-parse "$start_head" > "$GIT_DIR/BISECT_HEAD"
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
@@ -399,7 +399,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_RUN" &&
 	# Cleanup head-name if it got left by an old version of git-bisect
 	rm -f "$GIT_DIR/head-name" &&
-	git update-ref -d --no-deref BISECT_HEAD &&
+	rm -f "$GIT_DIR/BISECT_HEAD" &&
 	# clean up BISECT_START last
 	rm -f "$GIT_DIR/BISECT_START"
 }
-- 
2.0.4.314.gdbf7a51-twtrsrc

[PATCH 4/6] refs: replace log_ref_write with create_reflog

[David Turner]

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 | 2 +-
 refs.c             | 4 ++--
 refs.h             | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ac4d10a..7549ae2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -629,7 +629,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				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);
+				ret = create_reflog(ref_name, &log_file, &err);
 				log_all_ref_updates = temp;
 				strbuf_release(&log_file);
 				if (ret) {
diff --git a/refs.c b/refs.c
index c1a563f..93412ee 100644
--- a/refs.c
+++ b/refs.c
@@ -3119,7 +3119,7 @@ static int copy_msg(char *buf, const char *msg)
 }
 
 /* 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 create_reflog(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
 	char *logfile;
@@ -3203,7 +3203,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 = create_reflog(refname, sb_log_file, err);
 
 	if (result)
 		return result;
diff --git a/refs.h b/refs.h
index eee99f6..d4f75a9 100644
--- a/refs.h
+++ b/refs.h
@@ -233,7 +233,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 create_reflog(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.314.gdbf7a51-twtrsrc

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

[David Turner]

These are necessary because ref backends manage reflogs. 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>
---
 builtin/reflog.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index c2eb8ff..3cf7d3d 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 <refs>...";
 
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
@@ -699,12 +703,81 @@ 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;
+	struct strbuf logfile = 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 (create_reflog(argv[i], &logfile, &err)) {
+			error("could not create reflog %s: %s", argv[i],
+			      err.buf);
+			status = 1;
+			strbuf_release(&err);
+		}
+		strbuf_release(&logfile);
+	}
+	return status;
+}
+
+static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
+{
+	int i, status = 0, 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)
+		return error("Nothing to check?");
+
+	for (i = start ; i < argc; i++) {
+		if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
+			die("invalid ref format: %s", argv[i]);
+		if (!reflog_exists(argv[i]))
+			status = 1;
+	}
+	return status;
+}
+
 /*
  * 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 +797,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);
 }
-- 
2.0.4.314.gdbf7a51-twtrsrc

[PATCH 6/6] 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 93412ee..f3e3da0 100644
--- a/refs.c
+++ b/refs.c
@@ -3132,6 +3132,7 @@ int create_reflog(const char *refname, struct strbuf *sb_logfile, struct strbuf
 	    (starts_with(refname, "refs/heads/") ||
 	     starts_with(refname, "refs/remotes/") ||
 	     starts_with(refname, "refs/notes/") ||
+	     !strcmp(refname, "refs/stash") ||
 	     !strcmp(refname, "HEAD"))) {
 		if (safe_create_leading_directories(logfile) < 0) {
 			strbuf_addf(err, "unable to create directory for %s. "
-- 
2.0.4.314.gdbf7a51-twtrsrc

Re: [PATCH 0/6] refs backend preamble

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> This set of patches is a preamble to pluggable ref backends.  The
> intent is to use the ref infrastructure for special refs like
> CHERRY_PICK_HEAD where possible, and to use git plumbing commands to
> access refs and reflogs instead of directly writing files to the .git
> directory.
>
> This series is on top of pu at 9039c98c.

I won't queue this for now but will read and comment.

Thanks.

Re: [PATCH 1/6] refs.c: add an err argument to log_ref_setup

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

I think I saw Michael did something similar to other codepaths in
the ref API recently, and going in the same direction is generally a
good thing to do for consistency ;-)

> write_ref_to_lockfile is a private function that calls
> log_ref_setup. Update this function to also take an err argument and
> fill it in.  This again eliminates the need to manage errno in this
> function.

This particular patch is not just about log_ref_setup() and its
private friend write_ref_to_lockfile(), right?  It seems to also
touch commit_ref_update(), log_ref_write(), etc.  The overall
"theme" of this change is to teach parts of the ref API that deal
with writing reflogs to report errors the same way (i.e. as the
remainder of the ref API does by using &err), and that may be a
good single-line summary of the change (aka "Subject").

>  				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);

Original is underindented but this makes it even worse.  Push
continuation line further to the right (or restructure the code so
that it does not have to indent too deeply in the first place).

Other than that, this step looks sensible.

Thanks.

Re: [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Subject: Re: [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD
> Also use refs infrastructure for REVERT_HEAD.  These refs
> need to go through the refs backend, since some code
> assumes that they can be read as refs.

	cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

	Instead of directly writing to and reading from files in
        $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
	and REVERT_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 | REF_NO_REFLOG);
> +	delete_ref("REVERT_HEAD", NULL, REF_NODEREF | REF_NO_REFLOG);

Interesting.  There is a separate question about NO_REFLOG I'll
discuss in more detail later.  But no-deref puzzled me.  They should
not be symbolic, but if somebody by mistake made a symbolic one, we
won't accidentally remove another unrelated one through them, so
that bit is a good thing to have.

>  static void determine_whence(struct wt_status *s)
>  {
> +	unsigned char unused[20];
>  	if (file_exists(git_path("MERGE_HEAD")))
>  		whence = FROM_MERGE;
> -	else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
> +	else if (!read_ref("CHERRY_PICK_HEAD", unused)) {

I would have expected that you would use ref_exists() here.

> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 366f0bc..5e27a34 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"

Functionality-wise this may be OK but at some point we might want to
have a native way to produce $r with a single execution of a binary.

> @@ -429,8 +429,9 @@ __git_ps1 ()
>  			# symlink symbolic ref
>  			b="$(git symbolic-ref HEAD 2>/dev/null)"
>  		else
> -			local head=""
> -			if ! __git_eread "$g/HEAD" head; then
> +			local head
> +			head="ref: $(git symbolic-ref HEAD 2>/dev/null)" || head=$(git rev-parse HEAD)
> +			if [ -z "$head" ]; then

This is questionable; before the pre-context of this hunk is a check
for "HEAD" inside $GIT_DIR on the filesystem.  Besides, the "theme"
of this patch is about CHERRY_PICK_HEAD and REVERT_HEAD; it does not
justify to touch things that deal with HEAD in the same patch.

> diff --git a/refs.c b/refs.c
> index b34a54a..c1a563f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2979,7 +2979,7 @@ 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,
> -			     struct strbuf *err);
> +			     struct strbuf *err, int flags);
>  
>  int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
>  {
> @@ -3041,7 +3041,7 @@ 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, &err) ||
> -	    commit_ref_update(lock, orig_sha1, logmsg, &err)) {
> +	    commit_ref_update(lock, orig_sha1, logmsg, &err, 0)) {
>  		error("unable to write current sha1 into %s: %s", newrefname, err.buf);
>  		strbuf_release(&err);
>  		goto rollback;
> @@ -3060,7 +3060,7 @@ 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, &err) ||
> -	    commit_ref_update(lock, orig_sha1, NULL, &err)) {
> +	    commit_ref_update(lock, orig_sha1, NULL, &err, 0)) {
>  		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
>  		strbuf_release(&err);
>  	}
> @@ -3291,12 +3291,13 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
>   */
>  static int commit_ref_update(struct ref_lock *lock,
>  			     const unsigned char *sha1, const char *logmsg,
> -			     struct strbuf *err)
> +			     struct strbuf *err, int flags)
>  {
>  	clear_loose_ref_cache(&ref_cache);
> -	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, err) < 0)) {
> +	if (!(flags & REF_NO_REFLOG) &&
> +	    (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, err) < 0))) {

What is this change about?  I suspect this should be a separate
preparatory commit to allow callers to skip reflog update.

But more importantly, I do not think that it is a good idea in the
first place to allow callers to selectively say "for this update, do
not write a reflog entry".  If you do three ref-updates on the same
ref in a sequence (e.g. imagine you created a single strand of
pearls consisting of three commits) but passed ref-no-reflog only
for the second update, wouldn't that make your reflog entries
inconsistent?

REF_NO_REFLOG functionality that seletively allows updates to be
skipped should *NOT* exist, I think.

In other words, I think "do updates logged" should be a property of
each ref.  If changes to a ref is to be logged, all changes to it
must be logged.  If changes to a ref is not to be logged, no changes
to it must be logged.

The core.logAllRefUpdates configuration may appear to throw a monkey
wrench to that if its name is taken literally, as your creation of
CHECK_HEAD may get logged, which is not what you want.  But we are
under no obligation to obey core.logAllRefUpdates when updating
CHECK_HEAD or REVERT_HEAD.

core.logAllRefUpdates::
	Enable the reflog. Updates to a ref <ref> is logged to the file
	"$GIT_DIR/logs/<ref>", by appending the new and old
	SHA-1, the date/time and the reason of the update, but
	only when the file exists.  If this configuration
	variable is set to true, missing "$GIT_DIR/logs/<ref>"
	file is automatically created for branch heads (i.e. under
	refs/heads/), remote refs (i.e. under refs/remotes/),
	note refs (i.e. under refs/notes/), and the symbolic ref HEAD.

It is fairly clear that we promise not to auto-vivify reflogs for
CHERRY_PICK_HEAD and REVERT_HEAD from this description.

And if some users create $GIT_DIR/logs/CHERRY_PICK_HEAD to enable
reflog for CHERRY_PICK_HEAD, then it is not our business to refuse
logging the change just to speed things up a bit.

Re: [PATCH 3/6] bisect: use refs infrastructure for BISECT_START

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> This ref needs to go through the refs backend, since some code assumes
> that it can be written and read as a ref.

And BISECT_HEAD should no longer be a ref because...?

>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  git-bisect.sh                          | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 93716c4..c4d4d80 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -938,7 +938,7 @@ _git_bisect ()
>  	local subcommands="start bad good skip reset visualize replay log run"
>  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
>  	if [ -z "$subcommand" ]; then
> -		if [ -f "$(__gitdir)"/BISECT_START ]; then
> +		if [ git rev-parse BISECT_START 2>/dev/null ]; then
>  			__gitcomp "$subcommands"
>  		else
>  			__gitcomp "replay start"
> diff --git a/git-bisect.sh b/git-bisect.sh
> index ae3fec2..8658772 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -166,7 +166,7 @@ bisect_start() {
>  	#
>  	echo "$start_head" >"$GIT_DIR/BISECT_START" && {
>  		test "z$mode" != "z--no-checkout" ||
> -		git update-ref --no-deref BISECT_HEAD "$start_head"
> +		git rev-parse "$start_head" > "$GIT_DIR/BISECT_HEAD"
>  	} &&
>  	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
>  	eval "$eval true" &&
> @@ -399,7 +399,7 @@ bisect_clean_state() {
>  	rm -f "$GIT_DIR/BISECT_RUN" &&
>  	# Cleanup head-name if it got left by an old version of git-bisect
>  	rm -f "$GIT_DIR/head-name" &&
> -	git update-ref -d --no-deref BISECT_HEAD &&
> +	rm -f "$GIT_DIR/BISECT_HEAD" &&
>  	# clean up BISECT_START last
>  	rm -f "$GIT_DIR/BISECT_START"
>  }

Re: [PATCH 4/6] refs: replace log_ref_write with create_reflog

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> In a moment, we will use this to add reflog creation commands to
> git-reflog.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---

The title is misleading, if this is merely renaming.  I first
thought with this change the current callers of log_ref_setup()
would get a different/updated behaviour from the API by calling
something else, but that is not the case.

>  builtin/checkout.c | 2 +-
>  refs.c             | 4 ++--
>  refs.h             | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index ac4d10a..7549ae2 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -629,7 +629,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  				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);
> +				ret = create_reflog(ref_name, &log_file, &err);
>  				log_all_ref_updates = temp;
>  				strbuf_release(&log_file);
>  				if (ret) {
> diff --git a/refs.c b/refs.c
> index c1a563f..93412ee 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3119,7 +3119,7 @@ static int copy_msg(char *buf, const char *msg)
>  }
>  
>  /* 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 create_reflog(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
>  {
>  	int logfd, oflags = O_APPEND | O_WRONLY;
>  	char *logfile;
> @@ -3203,7 +3203,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 = create_reflog(refname, sb_log_file, err);
>  
>  	if (result)
>  		return result;
> diff --git a/refs.h b/refs.h
> index eee99f6..d4f75a9 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -233,7 +233,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 create_reflog(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,

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

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> These are necessary because ref backends manage reflogs.

Because?

	Because with core.logAllRefUpdates set to false, creating or
        updating a ref would not log what is done to it, unless a
        reflog already exists for the ref.  There are cases where
        we always want to have a reflog for a ref (e.g. refs/stash)
        regardless of the value of core.logAllRefUpdates, and we
        need a way to ensure that a reflog for a ref exists.
        "reflog create" is the way to do so.

	Also we need to be able to tell if a reflog for a ref
	exists, and "reflog exists" is the way to do so.

Now, going back to 4/6, I think create_reflog() function as an
external API has a few problems.

 * Its name does not tell us what should happen when a reflog
   already exists for the refname the caller asked to "create"
   reflog for.  I understand that this only makes sure it exists and
   does not destroy existing one.  Its old name, log_ref_setup(),
   did not have this problem, but now it does.

 * Its second parameter that is a strbuf is used only internally
   from within log_ref_write_1(); all the external callers do not
   look at it and they are forced to strbuf_init() it before calling
   the function and to strbuf_release() after the function returns.

Oh, also 4/6 incorrectly says that log_ref_write() is renamed to
create_reflog(), but it was log_ref_setup() that was renamed.

I think what 4/6 should have done in order to make the guts of what
log_ref_setup() does available as a more useful external API is to

 * keep log_ref_setup() as-is, make it static to refs.c, and have
   the internal caller that cares about the strbuf (i.e. the path
   to the log file) call that; and

 * Add a thin-wrapper for callers that do not care about the path to
   the log file, e.g.

	int vivify_reflog(const char *refname, struct strbuf *err)
	{
		int ret;
		struct strbuf sb = STRBUF_INIT;

                ret = log_ref_setup(refname, &sb, err);
                strbuf_release(&sb);
                return ret;
	}

Hmm?
    

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Now, going back to 4/6, I think create_reflog() function as an
> external API has a few problems.
>
>  * Its name does not tell us what should happen when a reflog
>    already exists for the refname the caller asked to "create"
>    reflog for.  I understand that this only makes sure it exists and
>    does not destroy existing one.  Its old name, log_ref_setup(),
>    did not have this problem, but now it does.
> ...
>  * Add a thin-wrapper for callers that do not care about the path to
>    the log file, e.g.
>
> 	int vivify_reflog(const char *refname, struct strbuf *err)
> 	{

As usual, I am aware that I am not good at naming things (but I can
tell when somebody uses a bad name), and "vivify" may be a horrible
name for that.  A reroll with a better name is very much welcomed.

Perhaps prepare-reflog?  I dunno.

Re: [PATCH 3/6] bisect: use refs infrastructure for BISECT_START

[David Turner]

On Thu, 2015-06-25 at 09:52 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > This ref needs to go through the refs backend, since some code assumes
> > that it can be written and read as a ref.
> 
> And BISECT_HEAD should no longer be a ref because...?

I can't remember now why I did this.  I guess we'll find out as we work
on the remaining patches.  I'll take it out for now.

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

[David Turner]

On Thu, 2015-06-25 at 11:35 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Now, going back to 4/6, I think create_reflog() function as an
> > external API has a few problems.
> >
> >  * Its name does not tell us what should happen when a reflog
> >    already exists for the refname the caller asked to "create"
> >    reflog for.  I understand that this only makes sure it exists and
> >    does not destroy existing one.  Its old name, log_ref_setup(),
> >    did not have this problem, but now it does.
> > ...
> >  * Add a thin-wrapper for callers that do not care about the path to
> >    the log file, e.g.
> >
> > 	int vivify_reflog(const char *refname, struct strbuf *err)
> > 	{
> 
> As usual, I am aware that I am not good at naming things (but I can
> tell when somebody uses a bad name), and "vivify" may be a horrible
> name for that.  A reroll with a better name is very much welcomed.
> 
> Perhaps prepare-reflog?  I dunno.

I'll try safe_create_reflog, by analogy to safe_create_leading_dirs.