[PATCH 00/29] Yet more preparation for reference backends

[PATCH 00/29] Yet more preparation for reference backends

[Michael Haggerty]

This started as a modest attempt to move the last couple of patches
mentioned here [1] to before the vtable patches. I wanted to do that
because having real changes mixed with the vtable refactoring was
making rebasing and stuff awkward.

But then it snowballed. A lot of what's new is pretty trivial (though
in some cases fixes minor bugs). But a few of the later patches are
pretty major.

The two main problems addressed by this series are:

1. Reference transactions will, in the future, sometimes span two
   completely different reference backends. For example, we already
   have worktree-specific refs like `HEAD` and `refs/bisect/*`, which
   are stored within the worktree, plus shared refs, which live in the
   main repository. It is even possible for a symref in one reference
   backend to point at a reference in another reference backend. Thus
   we need to be able to split one main transaction into one
   transaction per backend.

2. Currently, reference transactions that involve symbolic refs are
   not atomic: the symref is not locked at all, even when its reflog
   is being updated. This is a no-no. It also means that its referent
   can change between the time that the symref is resolved to find out
   its old_oid and the time that the referent is locked. These aren't
   super serious races because symrefs are usually not modified very
   often (especially on Git servers, which is mostly where concurrent
   modifications are an issue). But still...

The approach taken to solve these problems is inspired by David
Turner's patch [2], whose approach was first discussed here [3]. David
wrote a separate function, dereference_symrefs(transaction, ...),
which scanned through the whole transaction and split up any symref
updates into one symref update with the REF_LOG_ONLY option (to update
the symrefs reflog), plus a second update that changes the underlying
reference. This approach solves problem 1 but not problem 2.

This patch series takes a slightly different approach. For each
reference update during the "lock references" part of
ref_transaction_commit(), it

1. Locks the named reference. (If it is a symref, lock *the symref*,
   not its referent!)

2. Reads the reference non-recursively

3. If it is a symref, change the update to REF_LOG_ONLY and add a
   second update to the transaction for the underlying reference.

4. If it is not a symref but was derived from a symref update, record
   the reference's old_oid in the ref_update structure for the
   original symref.

In this way, each reference in a symref chain is locked down *before*
we read it and the lock is held throughout the transaction. As a
bonus, we don't have to use resolve_ref_full() to find old_oid for the
references; it is enough to read the references *once*, because we do
it under lock.

The three patches starting with "refs: resolve symbolic refs first"
involve a lot of new code in an area that is pretty intricate. I've
reviewed them a few times, but it's quite possible I've made some
mistakes (especially in the error-handling code). Careful review here
would be much appreciated.

It's possible that people will think this is too much new code for
fixing a relatively unlikely race. I'm not absolutely convinced myself
that it's not overengineered. But splitting ref_updates is definitely
needed for supporting multiple backends, and I think the approach of
locking then following one reference at a time is more or less a
prerequisite for making symref locking work correctly with multiple
reference backends. So (I think) our choices are to accept this code
or something like it, or to give up the hope of correct atomicity of
transactions that span backends.

This patch series is also available from my GitHub repository [4] as
branch "split-under-lock". It applies to Junio's current master.

Incidentally, a draft of the *next* patch series, which adds a
ref_store vtable abstraction for managing reference backends, is
available as branch "ref-store" in my GitHub repo. That branch passes
all tests but is not yet carefully reviewed.

Following is a table of contents for this patch series...

Chapter 1: Prologue

  This chapter consists of small cleanups that I noticed while working
  on the code.

  * safe_create_leading_directories(): improve docstring
  * remove_dir_recursively(): add docstring
  * refname_is_safe(): use skip_prefix()

  This patch fixes a bug in refname_is_safe():

  * refname_is_safe(): don't allow the empty string

  This one makes refname_is_safe() a little stricter...it shouldn't
  allow refnames like "refs/foo/../bar///baz" because the downstream
  code isn't smart enough to handle them anyway. (At the latest if the
  reference has to be sought in packed-refs, things would fail):

  * refname_is_safe(): insist that the refname already be normalized

Chapter 2: Character development

  Make sure error output goes the right place:

  * commit_ref_update(): write error message to *err, not stderr

  Tighten up some code, rename some parameters:

  * rename_ref(): remove unneeded local variable
  * ref_transaction_commit(): remove local variable n
  * read_raw_ref(): rename flags argument to type
  * read_raw_ref(): clear *type at start of function
  * read_raw_ref(): rename symref argument to referent
  * read_raw_ref(): improve docstring
  * lock_ref_sha1_basic(): remove unneeded local variable

Chapter 3: Small adventures

  Change error messages to adhere to our new policy of starting with
  lower-case letters:

  * refs: make error messages more consistent

  Require REF_NODEREF if REF_ISPRUNING is set:

  * ref_transaction_create(): disallow recursive pruning

  Correctly handle an (unlikely) error path:

  * ref_transaction_commit(): correctly report close_ref() failure

  Oops, here's a problem that could have bit us later. (In fact, it
  did bite me when I implemented one of the later patches):

  * delete_branches(): use resolve_refdup()

Chapter 4: Flashback

  The following two patches are split out of one of David Turner's
  patches mentioned above:

  * refs: allow log-only updates
  * refs: don't dereference on rename

Chapter 5: The tension builds

  The protagonist clearly has something planned...but what?

  * verify_refname_available(): adjust constness in declaration
  * add_update(): initialize the whole ref_update
  * lock_ref_for_update(): new function
  * unlock_ref(): move definition higher in the file

  Here we force a little more refname safety while building up
  ref_transactions:

  * ref_transaction_update(): check refname_is_safe() at a minimum

Chapter 6: The climax

  This is the guts of the patch series. Everything up to this point
  should be pretty uncontroversial. These three patches are the main
  subject of the discussion above. They are pretty large patches, but
  I don't see a reasonable way to break them up more:

  * refs: resolve symbolic refs first
  * lock_ref_for_update(): don't re-read non-symbolic references
  * lock_ref_for_update(): don't resolve symrefs

Chapter 7: Denouement

  Remove some stuff that's not needed anymore.

  * commit_ref_update(): remove the flags parameter
  * lock_ref_sha1_basic(): only handle REF_NODEREF mode

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/289994
[2] http://thread.gmane.org/gmane.comp.version-control.git/287949/focus=287958
[3] http://article.gmane.org/gmane.comp.version-control.git/282927
[4] https://github.com/mhagger/git

David Turner (2):
  refs: allow log-only updates
  refs: don't dereference on rename

Michael Haggerty (27):
  safe_create_leading_directories(): improve docstring
  remove_dir_recursively(): add docstring
  refname_is_safe(): use skip_prefix()
  refname_is_safe(): don't allow the empty string
  refname_is_safe(): insist that the refname already be normalized
  commit_ref_update(): write error message to *err, not stderr
  rename_ref(): remove unneeded local variable
  ref_transaction_commit(): remove local variable n
  read_raw_ref(): rename flags argument to type
  read_raw_ref(): clear *type at start of function
  read_raw_ref(): rename symref argument to referent
  read_raw_ref(): improve docstring
  lock_ref_sha1_basic(): remove unneeded local variable
  refs: make error messages more consistent
  ref_transaction_create(): disallow recursive pruning
  ref_transaction_commit(): correctly report close_ref() failure
  delete_branches(): use resolve_refdup()
  verify_refname_available(): adjust constness in declaration
  add_update(): initialize the whole ref_update
  lock_ref_for_update(): new function
  unlock_ref(): move definition higher in the file
  ref_transaction_update(): check refname_is_safe() at a minimum
  refs: resolve symbolic refs first
  lock_ref_for_update(): don't re-read non-symbolic references
  lock_ref_for_update(): don't resolve symrefs
  commit_ref_update(): remove the flags parameter
  lock_ref_sha1_basic(): only handle REF_NODEREF mode

 builtin/branch.c        |  19 +-
 cache.h                 |   5 +
 dir.h                   |  23 ++
 refs.c                  |  67 ++--
 refs/files-backend.c    | 882 +++++++++++++++++++++++++++++++++++++-----------
 refs/refs-internal.h    |  55 ++-
 t/t1400-update-ref.sh   |  41 ++-
 t/t1430-bad-ref-name.sh |   2 +-
 t/t3200-branch.sh       |   9 +
 9 files changed, 869 insertions(+), 234 deletions(-)

-- 
2.8.1

[PATCH 01/29] safe_create_leading_directories(): improve docstring

[Michael Haggerty]

Document the difference between this function and
safe_create_leading_directories_const(), and that the former restores
path before returning.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 cache.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cache.h b/cache.h
index 2711048..4134f64 100644
--- a/cache.h
+++ b/cache.h
@@ -993,6 +993,11 @@ int adjust_shared_perm(const char *path);
  * directory while we were working.  To be robust against this kind of
  * race, callers might want to try invoking the function again when it
  * returns SCLD_VANISHED.
+ *
+ * safe_create_leading_directories() temporarily changes path while it
+ * is working but restores it before returning.
+ * safe_create_leading_directories_const() doesn't modify path, even
+ * temporarily.
  */
 enum scld_error {
 	SCLD_OK = 0,
-- 
2.8.1

[PATCH 02/29] remove_dir_recursively(): add docstring

[Michael Haggerty]

Add a docstring for the remove_dir_recursively() function and the
REMOVE_DIR_* flags that can be passed to it.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 dir.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/dir.h b/dir.h
index 301b737..5f19acc 100644
--- a/dir.h
+++ b/dir.h
@@ -262,9 +262,32 @@ extern int is_empty_dir(const char *dir);
 
 extern void setup_standard_excludes(struct dir_struct *dir);
 
+
+/* Constants for remove_dir_recursively: */
+
+/*
+ * If a non-directory is found within path, stop and return an error.
+ * (In this case some empty directories might already have been
+ * removed.)
+ */
 #define REMOVE_DIR_EMPTY_ONLY 01
+
+/*
+ * If any Git work trees are found within path, skip them without
+ * considering it an error.
+ */
 #define REMOVE_DIR_KEEP_NESTED_GIT 02
+
+/* Remove the contents of path, but leave path itself. */
 #define REMOVE_DIR_KEEP_TOPLEVEL 04
+
+/*
+ * Remove path and its contents, recursively. flags is a combination
+ * of the above REMOVE_DIR_* constants. Return 0 on success.
+ *
+ * This function uses path as temporary scratch space, but restores it
+ * before returning.
+ */
 extern int remove_dir_recursively(struct strbuf *path, int flag);
 
 /* tries to remove the path with empty directories along it, ignores ENOENT */
-- 
2.8.1

[PATCH 03/29] refname_is_safe(): use skip_prefix()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 87dc82f..5789152 100644
--- a/refs.c
+++ b/refs.c
@@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags)
 
 int refname_is_safe(const char *refname)
 {
-	if (starts_with(refname, "refs/")) {
+	const char *rest;
+
+	if (skip_prefix(refname, "refs/", &rest)) {
 		char *buf;
 		int result;
 
-		buf = xmallocz(strlen(refname));
 		/*
 		 * Does the refname try to escape refs/?
 		 * For example: refs/foo/../bar is safe but refs/foo/../../bar
 		 * is not.
 		 */
-		result = !normalize_path_copy(buf, refname + strlen("refs/"));
+		buf = xmallocz(strlen(rest));
+		result = !normalize_path_copy(buf, rest);
 		free(buf);
 		return result;
 	}
-- 
2.8.1

[PATCH 04/29] refname_is_safe(): don't allow the empty string

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This fixes a coding error from the original implementation.

 refs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 5789152..ca0280f 100644
--- a/refs.c
+++ b/refs.c
@@ -136,11 +136,12 @@ int refname_is_safe(const char *refname)
 		free(buf);
 		return result;
 	}
-	while (*refname) {
+
+	do {
 		if (!isupper(*refname) && *refname != '_')
 			return 0;
 		refname++;
-	}
+	} while (*refname);
 	return 1;
 }
 
-- 
2.8.1

[PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[Michael Haggerty]

The reference name is going to be compared to other reference names, so
it should be in its normalized form.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Does anybody have a use case for allowing un-normalized reference
names like "refs/foo/../bar///baz"? I'm pretty certain they would not
be handled correctly anyway, especially if they are not stored as
loose references.

 refs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index ca0280f..b18d995 100644
--- a/refs.c
+++ b/refs.c
@@ -125,14 +125,19 @@ int refname_is_safe(const char *refname)
 	if (skip_prefix(refname, "refs/", &rest)) {
 		char *buf;
 		int result;
+		size_t restlen = strlen(rest);
+
+		/* rest must not be empty, or start or end with "/" */
+		if (!restlen || *rest == '/' || rest[restlen - 1] == '/')
+			return 0;
 
 		/*
 		 * Does the refname try to escape refs/?
 		 * For example: refs/foo/../bar is safe but refs/foo/../../bar
 		 * is not.
 		 */
-		buf = xmallocz(strlen(rest));
-		result = !normalize_path_copy(buf, rest);
+		buf = xmallocz(restlen);
+		result = !normalize_path_copy(buf, rest) && !strcmp(buf, rest);
 		free(buf);
 		return result;
 	}
-- 
2.8.1

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[Junio C Hamano]

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

> Does anybody have a use case for allowing un-normalized reference
> names like "refs/foo/../bar///baz"? I'm pretty certain they would not
> be handled correctly anyway, especially if they are not stored as
> loose references.

I wondered what codepath this follows:

    $ git rev-parse mhagger/wip//split-under-lock

get_sha1_basic() calls to dwim_ref() to learn real_ref and its
value.

dwim_ref() repeatedly gives the string to a known dwim rule, and the
"refs/remotes/%.*s" rule is expected to find that the user meant to
name "refs/remotes/mhagger/wip//split-under-lock".

This, still with doubled slash, is passed to resolve_ref_unsafe(),
which has a call to !refname_is_safe(refname) to reject the request.

So I think this will move the rejection early in the codepath than
how we reject the ref with doubled slash in the current code, but
the end result would be the same for this example.  The same is true
for heads//master or refs/heads//master.

There is another call to refname_is_safe() in resolve_ref_unsafe(),
which applies the sanity check to the string from a symref.  We seem
to allow

    $ git symbolic-ref refs/heads/SSS refs/heads//master

and we end up storing "ref: refs/heads//master" (or make a symbolic
link with doubled slashes), but the current code considers the
resulting symbolic link as "dangling".  Again, this change moves the
rejection a bit earlier in the codepath, without changing the end
result, I'd think.

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[David Turner]

On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:

> There is another call to refname_is_safe() in resolve_ref_unsafe(),
> which applies the sanity check to the string from a symref.  We seem
> to allow
> 
>     $ git symbolic-ref refs/heads/SSS refs/heads//master
> 
> and we end up storing "ref: refs/heads//master" (or make a symbolic
> link with doubled slashes), but the current code considers the
> resulting symbolic link as "dangling".  Again, this change moves the
> rejection a bit earlier in the codepath, without changing the end
> result, I'd think.

I think we should disallow that -- refname_is_safe should probably call
(or be replaced with calls to) check_refname_format.  

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[Jeff King]

On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:

> On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:
> 
> > There is another call to refname_is_safe() in resolve_ref_unsafe(),
> > which applies the sanity check to the string from a symref.  We seem
> > to allow
> > 
> >     $ git symbolic-ref refs/heads/SSS refs/heads//master
> > 
> > and we end up storing "ref: refs/heads//master" (or make a symbolic
> > link with doubled slashes), but the current code considers the
> > resulting symbolic link as "dangling".  Again, this change moves the
> > rejection a bit earlier in the codepath, without changing the end
> > result, I'd think.
> 
> I think we should disallow that -- refname_is_safe should probably call
> (or be replaced with calls to) check_refname_format.  

I thought the point is that one is a lesser check than the other, and we
need different rules for different situations. So we might allow
deletion on a refname that does not pass check_refname_format(), but we
must make sure it is not going to cause any mischief (e.g., escaping
".git" and deleting random files).

But anything writing a _new_ refname (whether the actual ref, or
referencing it via a symref) should be using check_refname_format()
before writing.

-Peff

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[David Turner]

On Wed, 2016-04-27 at 16:15 -0400, Jeff King wrote:
> On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:
> 
> > On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:
> > 
> > > There is another call to refname_is_safe() in
> > > resolve_ref_unsafe(),
> > > which applies the sanity check to the string from a symref.  We
> > > seem
> > > to allow
> > > 
> > >     $ git symbolic-ref refs/heads/SSS refs/heads//master
> > > 
> > > and we end up storing "ref: refs/heads//master" (or make a
> > > symbolic
> > > link with doubled slashes), but the current code considers the
> > > resulting symbolic link as "dangling".  Again, this change moves
> > > the
> > > rejection a bit earlier in the codepath, without changing the end
> > > result, I'd think.
> > 
> > I think we should disallow that -- refname_is_safe should probably
> > call
> > (or be replaced with calls to) check_refname_format.  
> 
> I thought the point is that one is a lesser check than the other, and
> we
> need different rules for different situations. So we might allow
> deletion on a refname that does not pass check_refname_format(), but
> we
> must make sure it is not going to cause any mischief (e.g., escaping
> ".git" and deleting random files).
> 
> But anything writing a _new_ refname (whether the actual ref, or
> referencing it via a symref) should be using check_refname_format()
> before writing.

Unfortunately, neither check is lesser -- refname_is_safe allows
refs/heads//foo but not a/b while check_refname_format allows a/b but
not refs/heads//foo.  So sometimes we need both, while other times we
just need one :(

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[Jeff King]

On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:

> > I thought the point is that one is a lesser check than the other, and
> > we
> > need different rules for different situations. So we might allow
> > deletion on a refname that does not pass check_refname_format(), but
> > we
> > must make sure it is not going to cause any mischief (e.g., escaping
> > ".git" and deleting random files).
> > 
> > But anything writing a _new_ refname (whether the actual ref, or
> > referencing it via a symref) should be using check_refname_format()
> > before writing.
> 
> Unfortunately, neither check is lesser -- refname_is_safe allows
> refs/heads//foo but not a/b while check_refname_format allows a/b but
> not refs/heads//foo.  So sometimes we need both, while other times we
> just need one :(

IMHO, that sounds like a bug. check_refname_format() should
conceptually[1] be a superset of refname_is_safe(). Is there a case
where we would want to _allow_ a refname that is not safe to look at on
disk?

-Peff

[1] The implementation can be a direct call, or can simply implement a
    superset of the rules, if that's more efficient.

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[Jeff King]

On Wed, Apr 27, 2016 at 04:37:28PM -0400, Jeff King wrote:

> > > But anything writing a _new_ refname (whether the actual ref, or
> > > referencing it via a symref) should be using check_refname_format()
> > > before writing.
> > 
> > Unfortunately, neither check is lesser -- refname_is_safe allows
> > refs/heads//foo but not a/b while check_refname_format allows a/b but
> > not refs/heads//foo.  So sometimes we need both, while other times we
> > just need one :(
> 
> IMHO, that sounds like a bug. check_refname_format() should
> conceptually[1] be a superset of refname_is_safe(). Is there a case
> where we would want to _allow_ a refname that is not safe to look at on
> disk?

BTW, if there isn't a superset relationship here, then I suspect I may
have introduced some loosening or inconsistencies in 03afcbe
(read_packed_refs: avoid double-checking sane refs, 2015-04-16). So any
tightening now may simply be fixing that (which doesn't change anything
with respect to correctness, but may give people more confidence that
the tightening is not breaking something people have been depending on).

-Peff

PS Reading over that commit message, I think it mis-states
   "refname_is_safe() can only be true if check_refname_format() also
   failed". It should be "!refname_is_safe() ...". I.e., the condition can
   only trigger if...

Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

[David Turner]

On Wed, 2016-04-27 at 16:37 -0400, Jeff King wrote:
> On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:
> 
> > > I thought the point is that one is a lesser check than the other,
> > > and
> > > we
> > > need different rules for different situations. So we might allow
> > > deletion on a refname that does not pass check_refname_format(),
> > > but
> > > we
> > > must make sure it is not going to cause any mischief (e.g.,
> > > escaping
> > > ".git" and deleting random files).
> > > 
> > > But anything writing a _new_ refname (whether the actual ref, or
> > > referencing it via a symref) should be using
> > > check_refname_format()
> > > before writing.
> > 
> > Unfortunately, neither check is lesser -- refname_is_safe allows
> > refs/heads//foo but not a/b while check_refname_format allows a/b
> > but
> > not refs/heads//foo.  So sometimes we need both, while other times
> > we
> > just need one :(
> 
> IMHO, that sounds like a bug. check_refname_format() should
> conceptually[1] be a superset of refname_is_safe(). Is there a case
> where we would want to _allow_ a refname that is not safe to look at
> on
> disk?

The only such case I can think of is the case where there is a symref
to such a bad refname, and we want to delete said symref.

[PATCH 06/29] commit_ref_update(): write error message to *err, not stderr

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1f38076..de38517 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2686,7 +2686,7 @@ static int commit_ref_update(struct ref_lock *lock,
 		}
 	}
 	if (commit_ref(lock)) {
-		error("Couldn't set %s", lock->ref_name);
+		strbuf_addf(err, "Couldn't set %s", lock->ref_name);
 		unlock_ref(lock);
 		return -1;
 	}
-- 
2.8.1

[PATCH 07/29] rename_ref(): remove unneeded local variable

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index de38517..0ade681 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2351,20 +2351,17 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	struct ref_lock *lock;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
-	const char *symref = NULL;
 	struct strbuf err = STRBUF_INIT;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
-	symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,
-				    orig_sha1, &flag);
+	if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))
+		return error("refname %s not found", oldrefname);
+
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldrefname);
-	if (!symref)
-		return error("refname %s not found", oldrefname);
-
 	if (!rename_ref_available(oldrefname, newrefname))
 		return 1;
 
-- 
2.8.1

[PATCH 08/29] ref_transaction_commit(): remove local variable n

[Michael Haggerty]

This microoptimization doesn't make a significant difference in speed.
And it causes problems if somebody ever wants to modify the function to
add updates to a transaction as part of processing it, as will happen
shortly.

Make the same change in initial_ref_transaction_commit().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ade681..05797cb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3043,7 +3043,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
 	int ret = 0, i;
-	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 	struct string_list_item *ref_to_delete;
@@ -3054,13 +3053,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: commit called for transaction that is not open");
 
-	if (!n) {
+	if (!transaction->nr) {
 		transaction->state = REF_TRANSACTION_CLOSED;
 		return 0;
 	}
 
 	/* Fail if a refname appears more than once in the transaction: */
-	for (i = 0; i < n; i++)
+	for (i = 0; i < transaction->nr; i++)
 		string_list_append(&affected_refnames, updates[i]->refname);
 	string_list_sort(&affected_refnames);
 	if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3074,7 +3073,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	 * lockfiles, ready to be activated. Only keep one lockfile
 	 * open at a time to avoid running out of file descriptors.
 	 */
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
 		if ((update->flags & REF_HAVE_NEW) &&
@@ -3145,7 +3144,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	}
 
 	/* Perform updates first so live commits remain referenced */
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
 		if (update->flags & REF_NEEDS_COMMIT) {
@@ -3164,7 +3163,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	}
 
 	/* Perform deletes now that updates are safely completed */
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
 		if (update->flags & REF_DELETING) {
@@ -3190,7 +3189,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 cleanup:
 	transaction->state = REF_TRANSACTION_CLOSED;
 
-	for (i = 0; i < n; i++)
+	for (i = 0; i < transaction->nr; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	string_list_clear(&refs_to_delete, 0);
@@ -3210,7 +3209,6 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 				   struct strbuf *err)
 {
 	int ret = 0, i;
-	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
@@ -3220,7 +3218,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 		die("BUG: commit called for transaction that is not open");
 
 	/* Fail if a refname appears more than once in the transaction: */
-	for (i = 0; i < n; i++)
+	for (i = 0; i < transaction->nr; i++)
 		string_list_append(&affected_refnames, updates[i]->refname);
 	string_list_sort(&affected_refnames);
 	if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3243,7 +3241,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 	if (for_each_rawref(ref_present, &affected_refnames))
 		die("BUG: initial ref transaction called with existing refs");
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
 		if ((update->flags & REF_HAVE_OLD) &&
@@ -3264,7 +3262,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
 		goto cleanup;
 	}
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
 		if ((update->flags & REF_HAVE_NEW) &&
-- 
2.8.1

Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n

[Junio C Hamano]

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

> This microoptimization doesn't make a significant difference in speed.
> And it causes problems if somebody ever wants to modify the function to
> add updates to a transaction as part of processing it, as will happen
> shortly.
>
> Make the same change in initial_ref_transaction_commit().
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

This particular change also makes the end result easier to read.  I
have no objection to officially declaring that we do support
"adding" new transaction update while we are committing (and we do
not support other futzing like "removing" or "reordering"), either.

I expect that somewhere in this series transaction->nr will not stay
constant even if the client code of ref-transaction API makes no
direct call that adds a new update[] element, though, even if it is
not done in this patch.

Thanks.

Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n

[Junio C Hamano]

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

> I expect that somewhere in this series transaction->nr will not stay

s/series/& it is documented that/

> constant even if the client code of ref-transaction API makes no
> direct call that adds a new update[] element, though, even if it is
> not done in this patch.
>
> Thanks.

[PATCH 09/29] read_raw_ref(): rename flags argument to type

[Michael Haggerty]

This will hopefully reduce confusion with the "flags" arguments that are
used in many functions in this module as an input parameter to choose
how the function should operate.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 16 ++++++++--------
 refs/refs-internal.h |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 05797cb..303c43b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1395,18 +1395,18 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * If the ref is symbolic, fill in *symref with the referrent
  * (e.g. "refs/heads/master") and return 0.  The caller is responsible
- * for validating the referrent.  Set REF_ISSYMREF in flags.
+ * for validating the referrent.  Set REF_ISSYMREF in type.
  *
  * If the ref doesn't exist, set errno to ENOENT and return -1.
  *
  * If the ref exists but is neither a symbolic ref nor a sha1, it is
- * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return
+ * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return
  * -1.
  *
  * If there is another error reading the ref, set errno appropriately and
  * return -1.
  *
- * Backend-specific flags might be set in flags as well, regardless of
+ * Backend-specific flags might be set in type as well, regardless of
  * outcome.
  *
  * sb_path is workspace: the caller should allocate and free it.
@@ -1419,7 +1419,7 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 int read_raw_ref(const char *refname, unsigned char *sha1,
-		 struct strbuf *symref, unsigned int *flags)
+		 struct strbuf *symref, unsigned int *type)
 {
 	struct strbuf sb_contents = STRBUF_INIT;
 	struct strbuf sb_path = STRBUF_INIT;
@@ -1448,7 +1448,7 @@ stat_ref:
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-		if (resolve_missing_loose_ref(refname, sha1, flags)) {
+		if (resolve_missing_loose_ref(refname, sha1, type)) {
 			errno = ENOENT;
 			goto out;
 		}
@@ -1469,7 +1469,7 @@ stat_ref:
 		if (starts_with(sb_contents.buf, "refs/") &&
 		    !check_refname_format(sb_contents.buf, 0)) {
 			strbuf_swap(&sb_contents, symref);
-			*flags |= REF_ISSYMREF;
+			*type |= REF_ISSYMREF;
 			ret = 0;
 			goto out;
 		}
@@ -1510,7 +1510,7 @@ stat_ref:
 
 		strbuf_reset(symref);
 		strbuf_addstr(symref, buf);
-		*flags |= REF_ISSYMREF;
+		*type |= REF_ISSYMREF;
 		ret = 0;
 		goto out;
 	}
@@ -1521,7 +1521,7 @@ stat_ref:
 	 */
 	if (get_sha1_hex(buf, sha1) ||
 	    (buf[40] != '\0' && !isspace(buf[40]))) {
-		*flags |= REF_ISBROKEN;
+		*type |= REF_ISBROKEN;
 		errno = EINVAL;
 		goto out;
 	}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3a4f634..0b047f6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
 		    each_ref_fn fn, int trim, int flags, void *cb_data);
 
 int read_raw_ref(const char *refname, unsigned char *sha1,
-		 struct strbuf *symref, unsigned int *flags);
+		 struct strbuf *symref, unsigned int *type);
 
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.8.1

[PATCH 10/29] read_raw_ref(): clear *type at start of function

[Michael Haggerty]

This is more convenient and less error-prone for callers.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 303c43b..f10c80f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1430,6 +1430,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
 	int ret = -1;
 	int save_errno;
 
+	*type = 0;
 	strbuf_reset(&sb_path);
 	strbuf_git_path(&sb_path, "%s", refname);
 	path = sb_path.buf;
-- 
2.8.1

[PATCH 11/29] read_raw_ref(): rename symref argument to referent

[Michael Haggerty]

After all, it doesn't hold the symbolic reference, but rather the
reference referred to.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 21 +++++++++++----------
 refs/refs-internal.h |  2 +-
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f10c80f..364425c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1393,9 +1393,10 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * If the ref is a sha1, fill in sha1 and return 0.
  *
- * If the ref is symbolic, fill in *symref with the referrent
- * (e.g. "refs/heads/master") and return 0.  The caller is responsible
- * for validating the referrent.  Set REF_ISSYMREF in type.
+ * If the ref is symbolic, fill in *referent with the name of the
+ * branch to which it refers (e.g. "refs/heads/master") and return 0.
+ * The caller is responsible for validating the referent. Set
+ * REF_ISSYMREF in type.
  *
  * If the ref doesn't exist, set errno to ENOENT and return -1.
  *
@@ -1411,15 +1412,15 @@ static int resolve_missing_loose_ref(const char *refname,
  *
  * sb_path is workspace: the caller should allocate and free it.
  *
- * It is OK for refname to point into symref. In this case:
- * - if the function succeeds with REF_ISSYMREF, symref will be
+ * It is OK for refname to point into referent. In this case:
+ * - if the function succeeds with REF_ISSYMREF, referent will be
  *   overwritten and the memory pointed to by refname might be changed
  *   or even freed.
- * - in all other cases, symref will be untouched, and therefore
+ * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
 int read_raw_ref(const char *refname, unsigned char *sha1,
-		 struct strbuf *symref, unsigned int *type)
+		 struct strbuf *referent, unsigned int *type)
 {
 	struct strbuf sb_contents = STRBUF_INIT;
 	struct strbuf sb_path = STRBUF_INIT;
@@ -1469,7 +1470,7 @@ stat_ref:
 		}
 		if (starts_with(sb_contents.buf, "refs/") &&
 		    !check_refname_format(sb_contents.buf, 0)) {
-			strbuf_swap(&sb_contents, symref);
+			strbuf_swap(&sb_contents, referent);
 			*type |= REF_ISSYMREF;
 			ret = 0;
 			goto out;
@@ -1509,8 +1510,8 @@ stat_ref:
 		while (isspace(*buf))
 			buf++;
 
-		strbuf_reset(symref);
-		strbuf_addstr(symref, buf);
+		strbuf_reset(referent);
+		strbuf_addstr(referent, buf);
 		*type |= REF_ISSYMREF;
 		ret = 0;
 		goto out;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0b047f6..37a1a37 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
 		    each_ref_fn fn, int trim, int flags, void *cb_data);
 
 int read_raw_ref(const char *refname, unsigned char *sha1,
-		 struct strbuf *symref, unsigned int *type);
+		 struct strbuf *referent, unsigned int *type);
 
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.8.1

[PATCH 12/29] read_raw_ref(): improve docstring

[Michael Haggerty]

Among other things, document the (important!) requirement that input
refname be checked for safety before calling this function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 364425c..1d2ef2a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1389,33 +1389,40 @@ static int resolve_missing_loose_ref(const char *refname,
 }
 
 /*
- * Read a raw ref from the filesystem or packed refs file.
+ * Read the specified reference from the filesystem or packed refs
+ * file, non-recursively. Set type to describe the reference, and:
  *
- * If the ref is a sha1, fill in sha1 and return 0.
+ * - If refname is the name of a normal reference, fill in sha1
+ *   (leaving referent unchanged).
  *
- * If the ref is symbolic, fill in *referent with the name of the
- * branch to which it refers (e.g. "refs/heads/master") and return 0.
- * The caller is responsible for validating the referent. Set
- * REF_ISSYMREF in type.
+ * - If refname is the name of a symbolic reference, write the full
+ *   name of the reference to which it refers (e.g.
+ *   "refs/heads/master") to referent and set the REF_ISSYMREF bit in
+ *   type (leaving sha1 unchanged). The caller is responsible for
+ *   validating that referent is a valid reference name.
  *
- * If the ref doesn't exist, set errno to ENOENT and return -1.
+ * WARNING: refname might be used as part of a filename, so it is
+ * important from a security standpoint that it be safe in the sense
+ * of refname_is_safe(). Moreover, for symrefs this function sets
+ * referent to whatever the repository says, which might not be a
+ * properly-formatted or even safe reference name. NEITHER INPUT NOR
+ * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
  *
- * If the ref exists but is neither a symbolic ref nor a sha1, it is
- * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return
- * -1.
- *
- * If there is another error reading the ref, set errno appropriately and
- * return -1.
+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT
+ * and return -1. If the ref exists but is neither a symbolic ref nor
+ * a sha1, it is broken; set REF_ISBROKEN in type, set errno to
+ * EINVAL, and return -1. If there is another error reading the ref,
+ * set errno appropriately and return -1.
  *
  * Backend-specific flags might be set in type as well, regardless of
  * outcome.
  *
- * sb_path is workspace: the caller should allocate and free it.
+ * It is OK for refname to point into referent. If so:
  *
- * It is OK for refname to point into referent. In this case:
  * - if the function succeeds with REF_ISSYMREF, referent will be
- *   overwritten and the memory pointed to by refname might be changed
- *   or even freed.
+ *   overwritten and the memory formerly pointed to by it might be
+ *   changed or even freed.
+ *
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-- 
2.8.1

Re: [PATCH 12/29] read_raw_ref(): improve docstring

[Junio C Hamano]

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

>   * Backend-specific flags might be set in type as well, regardless of
>   * outcome.
>   *
> - * sb_path is workspace: the caller should allocate and free it.

All made sense.

A welcome bonus is the removal of this stale comment that 42a38cf7
(read_raw_ref(): manage own scratch space, 2016-04-07) forgot to
remove.

[PATCH 13/29] lock_ref_sha1_basic(): remove unneeded local variable

[Michael Haggerty]

resolve_ref_unsafe() can cope with being called with NULL passed to its
flags argument. So lock_ref_sha1_basic() can just hand its own type
parameter through.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1d2ef2a..b8c1779 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1767,7 +1767,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const unsigned char *old_sha1,
 					    const struct string_list *extras,
 					    const struct string_list *skip,
-					    unsigned int flags, int *type_p,
+					    unsigned int flags, int *type,
 					    struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
@@ -1775,7 +1775,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	const char *orig_refname = refname;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int type;
 	int lflags = 0;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 	int resolve_flags = 0;
@@ -1795,7 +1794,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	}
 
 	refname = resolve_ref_unsafe(refname, resolve_flags,
-				     lock->old_oid.hash, &type);
+				     lock->old_oid.hash, type);
 	if (!refname && errno == EISDIR) {
 		/*
 		 * we are trying to lock foo but we used to
@@ -1813,10 +1812,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto error_return;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
-					     lock->old_oid.hash, &type);
+					     lock->old_oid.hash, type);
 	}
-	if (type_p)
-	    *type_p = type;
 	if (!refname) {
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
-- 
2.8.1

[PATCH 14/29] refs: make error messages more consistent

[Michael Haggerty]

* Always start error messages with a lower-case letter.

* Always enclose reference names in single quotes.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This change is not strictly needed, but I wanted to fix the old error
messages before I started adding new ones (otherwise, should the new
error messages be made to look like the old ones or should they be
corrected?) By the way, should these be internationalized?

 refs.c                |  8 ++++----
 refs/files-backend.c  | 32 ++++++++++++++++----------------
 t/t1400-update-ref.sh |  4 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/refs.c b/refs.c
index b18d995..ba14105 100644
--- a/refs.c
+++ b/refs.c
@@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 	filename = git_path("%s", pseudoref);
 	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
 	if (fd < 0) {
-		strbuf_addf(err, "Could not open '%s' for writing: %s",
+		strbuf_addf(err, "could not open '%s' for writing: %s",
 			    filename, strerror(errno));
 		return -1;
 	}
@@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 		if (read_ref(pseudoref, actual_old_sha1))
 			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
-			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+			strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
 			rollback_lock_file(&lock);
 			goto done;
 		}
 	}
 
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
-		strbuf_addf(err, "Could not write to '%s'", filename);
+		strbuf_addf(err, "could not write to '%s'", filename);
 		rollback_lock_file(&lock);
 		goto done;
 	}
@@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 
 	if (new_sha1 && !is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		strbuf_addf(err, "refusing to update ref with bad name %s",
+		strbuf_addf(err, "refusing to update ref with bad name '%s'",
 			    refname);
 		return -1;
 	}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b8c1779..9faf17c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1730,7 +1730,7 @@ static int verify_lock(struct ref_lock *lock,
 			  lock->old_oid.hash, NULL)) {
 		if (old_sha1) {
 			int save_errno = errno;
-			strbuf_addf(err, "can't verify ref %s", lock->ref_name);
+			strbuf_addf(err, "can't verify ref '%s'", lock->ref_name);
 			errno = save_errno;
 			return -1;
 		} else {
@@ -1739,7 +1739,7 @@ static int verify_lock(struct ref_lock *lock,
 		}
 	}
 	if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
-		strbuf_addf(err, "ref %s is at %s but expected %s",
+		strbuf_addf(err, "ref '%s' is at %s but expected %s",
 			    lock->ref_name,
 			    sha1_to_hex(lock->old_oid.hash),
 			    sha1_to_hex(old_sha1));
@@ -1819,7 +1819,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		if (last_errno != ENOTDIR ||
 		    !verify_refname_available_dir(orig_refname, extras, skip,
 						  get_loose_refs(&ref_cache), err))
-			strbuf_addf(err, "unable to resolve reference %s: %s",
+			strbuf_addf(err, "unable to resolve reference '%s': %s",
 				    orig_refname, strerror(last_errno));
 
 		goto error_return;
@@ -1857,7 +1857,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		/* fall through */
 	default:
 		last_errno = errno;
-		strbuf_addf(err, "unable to create directory for %s",
+		strbuf_addf(err, "unable to create directory for '%s'",
 			    ref_file.buf);
 		goto error_return;
 	}
@@ -2478,7 +2478,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 	strbuf_git_path(logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
 		if (safe_create_leading_directories(logfile->buf) < 0) {
-			strbuf_addf(err, "unable to create directory for %s: "
+			strbuf_addf(err, "unable to create directory for '%s': "
 				    "%s", logfile->buf, strerror(errno));
 			return -1;
 		}
@@ -2492,7 +2492,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 
 		if (errno == EISDIR) {
 			if (remove_empty_directories(logfile)) {
-				strbuf_addf(err, "There are still logs under "
+				strbuf_addf(err, "there are still logs under "
 					    "'%s'", logfile->buf);
 				return -1;
 			}
@@ -2500,7 +2500,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 		}
 
 		if (logfd < 0) {
-			strbuf_addf(err, "unable to append to %s: %s",
+			strbuf_addf(err, "unable to append to '%s': %s",
 				    logfile->buf, strerror(errno));
 			return -1;
 		}
@@ -2569,13 +2569,13 @@ 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) {
-		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
+		strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
 			    strerror(errno));
 		close(logfd);
 		return -1;
 	}
 	if (close(logfd)) {
-		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
+		strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
 			    strerror(errno));
 		return -1;
 	}
@@ -2616,14 +2616,14 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 	o = parse_object(sha1);
 	if (!o) {
 		strbuf_addf(err,
-			    "Trying to write ref %s with nonexistent object %s",
+			    "trying to write ref '%s' with nonexistent object %s",
 			    lock->ref_name, sha1_to_hex(sha1));
 		unlock_ref(lock);
 		return -1;
 	}
 	if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
 		strbuf_addf(err,
-			    "Trying to write non-commit object %s to branch %s",
+			    "trying to write non-commit object %s to branch '%s'",
 			    sha1_to_hex(sha1), lock->ref_name);
 		unlock_ref(lock);
 		return -1;
@@ -2633,7 +2633,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 	    write_in_full(fd, &term, 1) != 1 ||
 	    close_ref(lock) < 0) {
 		strbuf_addf(err,
-			    "Couldn't write %s", get_lock_file_path(lock->lk));
+			    "couldn't write '%s'", get_lock_file_path(lock->lk));
 		unlock_ref(lock);
 		return -1;
 	}
@@ -2654,7 +2654,7 @@ static int commit_ref_update(struct ref_lock *lock,
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {
 		char *old_msg = strbuf_detach(err, NULL);
-		strbuf_addf(err, "Cannot update the ref '%s': %s",
+		strbuf_addf(err, "cannot update the ref '%s': %s",
 			    lock->ref_name, old_msg);
 		free(old_msg);
 		unlock_ref(lock);
@@ -2689,7 +2689,7 @@ static int commit_ref_update(struct ref_lock *lock,
 		}
 	}
 	if (commit_ref(lock)) {
-		strbuf_addf(err, "Couldn't set %s", lock->ref_name);
+		strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
 		unlock_ref(lock);
 		return -1;
 	}
@@ -3038,7 +3038,7 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
 	for (i = 1; i < n; i++)
 		if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
 			strbuf_addf(err,
-				    "Multiple updates for ref '%s' not allowed.",
+				    "multiple updates for ref '%s' not allowed.",
 				    refnames->items[i].string);
 			return 1;
 		}
@@ -3142,7 +3142,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			 * Close it to free up the file descriptor:
 			 */
 			if (close_ref(update->lock)) {
-				strbuf_addf(err, "Couldn't close %s.lock",
+				strbuf_addf(err, "couldn't close '%s.lock'",
 					    update->refname);
 				goto cleanup;
 			}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index af1b20d..40b0cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -479,7 +479,7 @@ test_expect_success 'stdin fails with duplicate refs' '
 	create $a $m
 	EOF
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+	grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin create ref works' '
@@ -880,7 +880,7 @@ test_expect_success 'stdin -z fails option with unknown name' '
 test_expect_success 'stdin -z fails with duplicate refs' '
 	printf $F "create $a" "$m" "create $b" "$m" "create $a" "$m" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+	grep "fatal: multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin -z create ref works' '
-- 
2.8.1

[PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Michael Haggerty]

It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING
caller to pass REF_NODEREF too.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This also makes later patches a bit clearer.

 refs.c               | 3 +++
 refs/files-backend.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index ba14105..5dc2473 100644
--- a/refs.c
+++ b/refs.c
@@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
+	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
+		die("BUG: REF_ISPRUNING set without REF_NODEREF");
+
 	if (new_sha1 && !is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		strbuf_addf(err, "refusing to update ref with bad name '%s'",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9faf17c..8fcbd7d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, NULL, &err) ||
+				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
-- 
2.8.1

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

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

> It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
> without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING
> caller to pass REF_NODEREF too.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> This also makes later patches a bit clearer.

I wonder if it is more future-proof to solve this by doing

    -#define REF_ISPRUNING	0x04
    +#define REF_ISPRUNING	(0x04 | REF_NODEREF)

instead.  It makes the intention clear that pruning is always about
the single level (i.e. no-deref).


>  refs.c               | 3 +++
>  refs/files-backend.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index ba14105..5dc2473 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  	if (transaction->state != REF_TRANSACTION_OPEN)
>  		die("BUG: update called for transaction that is not open");
>  
> +	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> +		die("BUG: REF_ISPRUNING set without REF_NODEREF");
> +
>  	if (new_sha1 && !is_null_sha1(new_sha1) &&
>  	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
>  		strbuf_addf(err, "refusing to update ref with bad name '%s'",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9faf17c..8fcbd7d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
>  	transaction = ref_transaction_begin(&err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, r->name, r->sha1,
> -				   REF_ISPRUNING, NULL, &err) ||
> +				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
>  	    ref_transaction_commit(transaction, &err)) {
>  		ref_transaction_free(transaction);
>  		error("%s", err.buf);

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[David Turner]

On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
> > without REF_NODEREF. Forbid it explicitly. Change the one
> > REF_ISPRUNING
> > caller to pass REF_NODEREF too.
> > 
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> > This also makes later patches a bit clearer.
> 
> I wonder if it is more future-proof to solve this by doing
> 
>     -#define REF_ISPRUNING	0x04
>     +#define REF_ISPRUNING	(0x04 | REF_NODEREF)
> 
> instead.  It makes the intention clear that pruning is always about
> the single level (i.e. no-deref).

I think the approach in Michael's patch might be clearer than yours,
since someone reading the code doesn't have to look at the definition
of REF_ISPRUNING to understand what is going on.

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
>> > without REF_NODEREF. Forbid it explicitly. Change the one
>> > REF_ISPRUNING
>> > caller to pass REF_NODEREF too.
>> > 
>> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> > ---
>> > This also makes later patches a bit clearer.
>> 
>> I wonder if it is more future-proof to solve this by doing
>> 
>>     -#define REF_ISPRUNING	0x04
>>     +#define REF_ISPRUNING	(0x04 | REF_NODEREF)
>> 
>> instead.  It makes the intention clear that pruning is always about
>> the single level (i.e. no-deref).
>
> I think the approach in Michael's patch might be clearer than yours,
> since someone reading the code doesn't have to look at the definition
> of REF_ISPRUNING to understand what is going on.

I have to strongly disagree, assuming that my understanding of the
point of this patch correctly.

If a casual reader sees this code:

    ref_transaction_delete(transaction, r->name, r->sha1,
			   REF_ISPRUNING | REF_NODEREF, NULL, &err)

it gives an incorrect impression that there may also be a valid case
to make a "delete" call with ISPRUNING alone without NODEREF, in
other codepaths and under certain conditions, and write an incorrect

    ref_transaction_delete(transaction, refname, sha1,
			   REF_ISPRUNING, NULL, &err)

in her new code.  Or a careless programmer and reviewer may not even
memorize and remember what the new world order is when they see such
a code and let it pass.

As I understand that we declare that "to prune a ref from set of
loose refs is to prune the named one, never following a symbolic
ref" is the new world order with this patch, making sure that
ISPRUNING automatically and always mean NODEREF will eliminate the
possibility that any new code makes an incorrect call to "delete",
which I think is much better.

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

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

> If a casual reader sees this code:
>
>     ref_transaction_delete(transaction, r->name, r->sha1,
> 			   REF_ISPRUNING | REF_NODEREF, NULL, &err)
>
> it gives an incorrect impression that there may also be a valid case
> to make a "delete" call with ISPRUNING alone without NODEREF, in
> other codepaths and under certain conditions, and write an incorrect
>
>     ref_transaction_delete(transaction, refname, sha1,
> 			   REF_ISPRUNING, NULL, &err)
>
> in her new code.  Or a careless programmer and reviewer may not even
> memorize and remember what the new world order is when they see such
> a code and let it pass.
>
> As I understand that we declare that "to prune a ref from set of
> loose refs is to prune the named one, never following a symbolic
> ref" is the new world order with this patch, making sure that
> ISPRUNING automatically and always mean NODEREF will eliminate the
> possibility that any new code makes an incorrect call to "delete",
> which I think is much better.

... but my understanding of the point of this patch may be flawed,
in which case I of course am willing to be enlightened ;-)

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[David Turner]

On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If a casual reader sees this code:
> > 
> >     ref_transaction_delete(transaction, r->name, r->sha1,
> > 			   REF_ISPRUNING | REF_NODEREF, NULL, &err)
> > 
> > it gives an incorrect impression that there may also be a valid
> > case
> > to make a "delete" call with ISPRUNING alone without NODEREF, in
> > other codepaths and under certain conditions, and write an
> > incorrect
> > 
> >     ref_transaction_delete(transaction, refname, sha1,
> > 			   REF_ISPRUNING, NULL, &err)
> > 
> > in her new code.  Or a careless programmer and reviewer may not
> > even
> > memorize and remember what the new world order is when they see
> > such
> > a code and let it pass.
> > 
> > As I understand that we declare that "to prune a ref from set of
> > loose refs is to prune the named one, never following a symbolic
> > ref" is the new world order with this patch, making sure that
> > ISPRUNING automatically and always mean NODEREF will eliminate the
> > possibility that any new code makes an incorrect call to "delete",
> > which I think is much better.
> 
> ... but my understanding of the point of this patch may be flawed,
> in which case I of course am willing to be enlightened ;-)

Since there is a manual check for that case, the code will fail at test
time.

But I don't have strong feelings and am happy to go either way on this.

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Since there is a manual check for that case, the code will fail at test
> time.

I see a difference between "We make it hard to write incorrect code"
and "We keep it easy to write incorrect code but a runtime check
will catch mistakes anyway", and I tend to prefer the former.

I do think the "manual check" needs to be kept in an adjusted form
even if we decide to take the suggested update.  A caller can pass
0x04 instead of REF_ISPRUNING after all---but that is exactly the
case where you have to work hard to write incorrect code.

One existing check that is not touched with 15/29 also needs to be
adjusted.  An incremental update relative to 15/29 would look like
this:

 refs.c               | 4 ++--
 refs/files-backend.c | 4 ++--
 refs/refs-internal.h | 6 ++++--
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 5dc2473..bb81dfd 100644
--- a/refs.c
+++ b/refs.c
@@ -790,8 +790,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");
 
-	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
-		die("BUG: REF_ISPRUNING set without REF_NODEREF");
+	if ((flags & REF_ISPRUNING_) && !(flags & REF_NODEREF))
+		die("BUG: REF_ISPRUNING_ set without REF_NODEREF");
 
 	if (new_sha1 && !is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..012e3d0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
+				   REF_ISPRUNING, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
@@ -3178,7 +3178,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				goto cleanup;
 			}
 
-			if (!(update->flags & REF_ISPRUNING))
+			if (!(update->flags & REF_ISPRUNING_))
 				string_list_append(&refs_to_delete,
 						   update->lock->ref_name);
 		}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..0a2bf1f 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,9 +15,11 @@
 
 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned.  As this must always be done with REF_NODEREF, we make
+ * the public version REF_ISPRUNING to contain REF_NODEREF.
  */
-#define REF_ISPRUNING	0x04
+#define REF_ISPRUNING_	0x04
+#define REF_ISPRUNING	(REF_ISPRUNING_ | REF_NODEREF)
 
 /*
  * Used as a flag in ref_update::flags when the reference should be

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Michael Haggerty]

On 04/27/2016 11:15 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If a casual reader sees this code:
>>
>>     ref_transaction_delete(transaction, r->name, r->sha1,
>> 			   REF_ISPRUNING | REF_NODEREF, NULL, &err)
>>
>> it gives an incorrect impression that there may also be a valid case
>> to make a "delete" call with ISPRUNING alone without NODEREF, in
>> other codepaths and under certain conditions, and write an incorrect
>>
>>     ref_transaction_delete(transaction, refname, sha1,
>> 			   REF_ISPRUNING, NULL, &err)
>>
>> in her new code.  Or a careless programmer and reviewer may not even
>> memorize and remember what the new world order is when they see such
>> a code and let it pass.
>>
>> As I understand that we declare that "to prune a ref from set of
>> loose refs is to prune the named one, never following a symbolic
>> ref" is the new world order with this patch, making sure that
>> ISPRUNING automatically and always mean NODEREF will eliminate the
>> possibility that any new code makes an incorrect call to "delete",
>> which I think is much better.
> 
> ... but my understanding of the point of this patch may be flawed,
> in which case I of course am willing to be enlightened ;-)

I was thinking of this patch as documenting and enforcing a limitation
in the current implementation of pruning. But to be honest I can't think
of a reason that we would ever want to remove this limitation, so I am
OK with changing the policy to "REF_ISPRUNING always implies
REF_NODEREF" as you have suggested.

But I think it would be cleaner to achieve that goal with the following
change:

diff --git a/refs.c b/refs.c
index 5dc2473..1d4c12a 100644
--- a/refs.c
+++ b/refs.c
@@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction
*transaction,
 	if (transaction->state != REF_TRANSACTION_OPEN)
 		die("BUG: update called for transaction that is not open");

-	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
-		die("BUG: REF_ISPRUNING set without REF_NODEREF");
+	if (flags & REF_ISPRUNING) {
+		/* Pruning is always non-recursive */
+		flags |= REF_NODEREF;
+	}

 	if (new_sha1 && !is_null_sha1(new_sha1) &&
 	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..9faf17c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
+				   REF_ISPRUNING, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..704eea7 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -15,7 +15,7 @@

 /*
  * Used as a flag in ref_update::flags when a loose ref is being
- * pruned.
+ * pruned. This flag implies REF_NODEREF.
  */
 #define REF_ISPRUNING	0x04


Note that patch "add_update(): initialize the whole ref_update" should
then be adjusted to do the flag-tweak in the add_update() function.

If there are no objections, I will implement these changes in v2.

Michael

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

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

> But I think it would be cleaner to achieve that goal with the following
> change:
>
> diff --git a/refs.c b/refs.c
> index 5dc2473..1d4c12a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
>  	if (transaction->state != REF_TRANSACTION_OPEN)
>  		die("BUG: update called for transaction that is not open");
>
> -	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> -		die("BUG: REF_ISPRUNING set without REF_NODEREF");
> +	if (flags & REF_ISPRUNING) {
> +		/* Pruning is always non-recursive */
> +		flags |= REF_NODEREF;
> +	}
>
>  	if (new_sha1 && !is_null_sha1(new_sha1) &&
>  	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8fcbd7d..9faf17c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
>  	transaction = ref_transaction_begin(&err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, r->name, r->sha1,
> -				   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
> +				   REF_ISPRUNING, NULL, &err) ||
>  	    ref_transaction_commit(transaction, &err)) {
>  		ref_transaction_free(transaction);
>  		error("%s", err.buf);
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 37a1a37..704eea7 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -15,7 +15,7 @@
>
>  /*
>   * Used as a flag in ref_update::flags when a loose ref is being
> - * pruned.
> + * pruned. This flag implies REF_NODEREF.
>   */
>  #define REF_ISPRUNING	0x04
>
>
> Note that patch "add_update(): initialize the whole ref_update" should
> then be adjusted to do the flag-tweak in the add_update() function.
> ...
> If there are no objections, I will implement these changes in v2.

One worrysome point the above approach leaves is that nothing
guarantees that nobody in the codepath from the callsite of
ref-transaction-delete to ref-transaction-update looks at the flag
and acts differently depending on REF_NODEREF bit.  During that
time, REF_ISPRUNING call would not trigger (flag & REF_NODEREF)
check, because "pruning implies no-deref" is done only after you
reach transaction-update (i.e. the code you added in the first
hunk), but any code after that "implie no-deref" happens will see
REF_NODEREF bit in the flag word.

As long as you can add some mechanism to make that a non-issue, I am
fine with that approach.

Thanks.

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Junio C Hamano]

Two things I forgot to say.

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

> I was thinking of this patch as documenting and enforcing a limitation
> in the current implementation of pruning.

This actually is an excellent point, and is the reason why I
repeatedly made my suggestion conditional on "If my understanding of
the purpose of this patch correctly" ;-) And I personally am not
convinced that this limitation is fundamental and will not be
lifted.  It may be better to treat this as a limitation of the
current implementation of pruning, and in that case I do agree that
the original patch that started this thread is the best way to
express it.

I said that it makes harder to write incorrect client code of this
API to make REF_ISPRUNING contain both 0x04 and REF_NODEREF, and I
still think that is true, but at the same time, it makes it easier
to write incorrect code in the API implementation.  We no longer can
check if we are in the pruning codepath with

    if (flag & REF_ISPRUNING)

but have to write

    if ((flag & REF_ISPRUNING) == REF_ISPRUNING)

instead.  This "& with the constant and compare the result with that
same constant" pattern could also be used for a single-bit constant,
so if there were short-and-sweet syntax to express that pattern in
the language, consistently using that for all bit checks would make
it less likely for us to write incorrect code, but but there is no
short-and-sweet syntax to do so in C, and spelling it out like the
above is too cumbersome to be practical.  The suggested squash did

    if (flag & REF_ISPRUNING_)

to check only for 0x04 bit, but that is also error prone; it is too
easy to forget the difference between the two and drop the trailing
underscore by mistake.

Even though it is generally a good trade-off to make it harder to
make mistakes in the "user of the API" code even if it makes it
easier to make mistakes in the "implementation of the API" code,
because the "user of the API" in this case is actually still inside
the ref API implementation, I do not think either way makes too much
of a difference.  So perhaps your original might be the best version
among those that have been discussed in this thread.

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

[Michael Haggerty]

On 04/29/2016 10:41 AM, Junio C Hamano wrote:
> [...Long, thoughtful comments omitted...]
> So perhaps your original might be the best version
> among those that have been discussed in this thread.

Very well. I'll also add a comment near the definition of REF_ISPRUNING
that it must only be used together with REF_NODEREF.

Michael

[PATCH 16/29] ref_transaction_commit(): correctly report close_ref() failure

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
close() rarely fails, but it is possible.

 refs/files-backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fcbd7d..e86e3de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3144,6 +3144,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			if (close_ref(update->lock)) {
 				strbuf_addf(err, "couldn't close '%s.lock'",
 					    update->refname);
+				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
 			}
 		}
-- 
2.8.1

[PATCH 17/29] delete_branches(): use resolve_refdup()

[Michael Haggerty]

The return value of resolve_ref_unsafe() is not guaranteed to stay
around as long as we need it, so use resolve_refdup() instead.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/branch.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0adba62..ae55688 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -212,7 +212,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			die(_("Couldn't look up commit object for HEAD"));
 	}
 	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
-		const char *target;
+		char *target = NULL;
 		int flags = 0;
 
 		strbuf_branchname(&bname, argv[i]);
@@ -231,11 +231,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			}
 		}
 
-		target = resolve_ref_unsafe(name,
-					    RESOLVE_REF_READING
-					    | RESOLVE_REF_NO_RECURSE
-					    | RESOLVE_REF_ALLOW_BAD_NAME,
-					    sha1, &flags);
+		target = resolve_refdup(name,
+					RESOLVE_REF_READING
+					| RESOLVE_REF_NO_RECURSE
+					| RESOLVE_REF_ALLOW_BAD_NAME,
+					sha1, &flags);
 		if (!target) {
 			error(remote_branch
 			      ? _("remote-tracking branch '%s' not found.")
@@ -248,7 +248,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		    check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
 					force)) {
 			ret = 1;
-			continue;
+			goto next;
 		}
 
 		if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			      : _("Error deleting branch '%s'"),
 			      bname.buf);
 			ret = 1;
-			continue;
+			goto next;
 		}
 		if (!quiet) {
 			printf(remote_branch
@@ -270,6 +270,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			       : find_unique_abbrev(sha1, DEFAULT_ABBREV));
 		}
 		delete_branch_config(bname.buf);
+
+	next:
+		free(target);
 	}
 
 	free(name);
-- 
2.8.1

[PATCH 18/29] refs: allow log-only updates

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

The refs infrastructure learns about log-only ref updates, which only
update the reflog.  Later, we will use this to separate symbolic
reference resolution from ref updating.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 16 ++++++++++------
 refs/refs-internal.h |  7 +++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e86e3de..91416db 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2688,7 +2688,7 @@ static int commit_ref_update(struct ref_lock *lock,
 			}
 		}
 	}
-	if (commit_ref(lock)) {
+	if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
 		strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
 		unlock_ref(lock);
 		return -1;
@@ -3106,7 +3106,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			goto cleanup;
 		}
 		if ((update->flags & REF_HAVE_NEW) &&
-		    !(update->flags & REF_DELETING)) {
+		    !(update->flags & REF_DELETING) &&
+		    !(update->flags & REF_LOG_ONLY)) {
 			int overwriting_symref = ((update->type & REF_ISSYMREF) &&
 						  (update->flags & REF_NODEREF));
 
@@ -3138,8 +3139,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		}
 		if (!(update->flags & REF_NEEDS_COMMIT)) {
 			/*
-			 * We didn't have to write anything to the lockfile.
-			 * Close it to free up the file descriptor:
+			 * We didn't call write_ref_to_lockfile(), so
+			 * the lockfile is still open. Close it to
+			 * free up the file descriptor:
 			 */
 			if (close_ref(update->lock)) {
 				strbuf_addf(err, "couldn't close '%s.lock'",
@@ -3154,7 +3156,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
-		if (update->flags & REF_NEEDS_COMMIT) {
+		if (update->flags & REF_NEEDS_COMMIT ||
+		    update->flags & REF_LOG_ONLY) {
 			if (commit_ref_update(update->lock,
 					      update->new_sha1, update->msg,
 					      update->flags, err)) {
@@ -3173,7 +3176,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
-		if (update->flags & REF_DELETING) {
+		if (update->flags & REF_DELETING &&
+		    !(update->flags & REF_LOG_ONLY)) {
 			if (delete_ref_loose(update->lock, update->type, err)) {
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 37a1a37..6b53ba1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -43,6 +43,13 @@
  */
 
 /*
+ * Used as a flag in ref_update::flags when we want to log a ref
+ * update but not actually perform it.  This is used when a symbolic
+ * ref update is split up.
+ */
+#define REF_LOG_ONLY 0x80
+
+/*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
  * example by causing a file that is not a reference to be deleted.
-- 
2.8.1

[PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty]

From: David Turner <dturner@twopensource.com>

When renaming refs, don't dereference either the origin or the destination
before renaming.

The origin does not need to be dereferenced because it is presently
forbidden to rename symbolic refs.

Not dereferencing the destination fixes a bug where renaming on top of
a broken symref would use the pointed-to ref name for the moved
reflog.

Add a test for the reflog bug.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 13 ++++++++-----
 t/t3200-branch.sh    |  9 +++++++++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 91416db..e4cdd5a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2358,11 +2358,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	struct strbuf err = STRBUF_INIT;
+	const int resolve_flags = RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE;
 
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
-	if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))
+	if (!resolve_ref_unsafe(oldrefname, resolve_flags, orig_sha1, &flag))
 		return error("refname %s not found", oldrefname);
 
 	if (flag & REF_ISSYMREF)
@@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollback;
 	}
 
-	if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
-	    delete_ref(newrefname, sha1, REF_NODEREF)) {
+	if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
+	    delete_ref(newrefname, NULL, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			struct strbuf path = STRBUF_INIT;
 			int result;
@@ -2405,7 +2406,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
+	lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF,
+				   NULL, &err);
 	if (!lock) {
 		error("unable to rename '%s' to '%s': %s", oldrefname, newrefname, err.buf);
 		strbuf_release(&err);
@@ -2423,7 +2425,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 0;
 
  rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
+	lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF,
+				   NULL, &err);
 	if (!lock) {
 		error("unable to lock %s for rollback: %s", oldrefname, err.buf);
 		strbuf_release(&err);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f3e3b6c..4281160 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -79,6 +79,15 @@ test_expect_success 'git branch -m dumps usage' '
 	test_i18ngrep "branch name required" err
 '
 
+test_expect_success 'git branch -m m broken_symref should work' '
+	test_when_finished "git branch -D broken_symref" &&
+	git branch -l m &&
+	git symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken &&
+	git branch -m m broken_symref &&
+	git reflog exists refs/heads/broken_symref &&
+	test_must_fail git reflog exists refs/heads/i_am_broken
+'
+
 test_expect_success 'git branch -m m m/m should work' '
 	git branch -l m &&
 	git branch -m m m/m &&
-- 
2.8.1

Re: [PATCH 19/29] refs: don't dereference on rename

[Junio C Hamano]

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

> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		goto rollback;
>  	}
>  
> -	if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
> -	    delete_ref(newrefname, sha1, REF_NODEREF)) {
> +	if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
> +	    delete_ref(newrefname, NULL, REF_NODEREF)) {

Could you explain s/sha1/NULL/ here in the proposed log message?

>  		if (errno==EISDIR) {
>  			struct strbuf path = STRBUF_INIT;
>  			int result;

Re: [PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty]

On 04/27/2016 08:55 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>  		goto rollback;
>>  	}
>>  
>> -	if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
>> -	    delete_ref(newrefname, sha1, REF_NODEREF)) {
>> +	if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
>> +	    delete_ref(newrefname, NULL, REF_NODEREF)) {
> 
> Could you explain s/sha1/NULL/ here in the proposed log message?

Good question.

Passing sha1 to delete_ref() doesn't add any safety, because the same
sha1 was just read a moment before, and it is not used for anything
else. So the check only protects us from a concurrent update to
newrefname between the call to read_ref_full() and the call to
delete_ref(). But such a race is indistinguishable from the case that a
modification to newrefname happens just before the call to
read_ref_full(), which would have the same outcome as the new code. So
the "sha1" check only adds ways for the rename() to fail in situations
where nothing harmful would have happened anyway.

That being said, this is a very unlikely race, and I don't think it
matters much either way. In any case, the change s/sha1/NULL/ here seems
orthogonal to the rest of the patch.

David, you wrote the original version of this patch. Am I overlooking
something?

Michael

Re: [PATCH 19/29] refs: don't dereference on rename

[Junio C Hamano]

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

>> Could you explain s/sha1/NULL/ here in the proposed log message?
>
> Good question.
>
> Passing sha1 to delete_ref() doesn't add any safety, because the same
> sha1 was just read a moment before, and it is not used for anything
> else.

"... and it is guaranteed that no other process in the meantime
wanted to update the ref we are trying to delete because it wants to
see the ref with its updated value." is something I expected to see
at the end.

> So the check only protects us from a concurrent update to
> newrefname between the call to read_ref_full() and the call to
> delete_ref(). But such a race is indistinguishable from the case that a
> modification to newrefname happens just before the call to
> read_ref_full(), which would have the same outcome as the new code.

In other words, when a transaction that contains a deletion of a ref
races with another one that updates the ref, the latter transaction
may come after the former one, but the ref may not survive in the
end result and can be left deleted?

Puzzled...

Re: [PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty]

On 04/29/2016 10:53 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else.
> 
> "... and it is guaranteed that no other process in the meantime
> wanted to update the ref we are trying to delete because it wants to
> see the ref with its updated value." is something I expected to see
> at the end.
> 
>> So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
> 
> In other words, when a transaction that contains a deletion of a ref
> races with another one that updates the ref, the latter transaction
> may come after the former one, but the ref may not survive in the
> end result and can be left deleted?
> 
> Puzzled...

Remember, we're talking about rename_ref() only, not reference deletion
in general. rename_ref() is not very robust anyway--it doesn't happen in
a single transaction, and it is vulnerable to being defeated by
simultaneous reference updates by other processes. It *does* wrap the
deletion of newrefname in a transaction; the only question is whether an
old_sha1 is supplied to that transaction.

So, suppose that newrefname starts at value A, and there are two updates
running simultaneously:

1. An update of reference newrefname from A -> B

2. A rename of reference oldrefname to newrefname, which includes
   a. read_ref_full("newrefname") and
   b. delete_ref("newrefname").

It is not possible for (1) to happen after (2b) because the former's
check of the old value of newrefname would fail. So there are two
possible interleavings:

* 1, 2a, 2b
* 2a, 1, 2b

With the new code, both of these interleavings end up with newrefname
deleted.

With the old code, the second interleaving would fail.

But the only difference is the relative order of the read-only operation
(2a), whose SHA-1 result is never used. So neither process actually
cares which of those two interleavings occurred, and it is legitimate to
treat them the same.

Note that the first transaction *did* successfully set newrefname to
value B in both cases and indeed knows for sure that the update was
successful. It's just that newrefname was deleted immediately afterwards.

Michael

Re: [PATCH 19/29] refs: don't dereference on rename

[Jeff King]

On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:

> Remember, we're talking about rename_ref() only, not reference deletion
> in general. rename_ref() is not very robust anyway--it doesn't happen in
> a single transaction, and it is vulnerable to being defeated by
> simultaneous reference updates by other processes. It *does* wrap the
> deletion of newrefname in a transaction; the only question is whether an
> old_sha1 is supplied to that transaction.
> 
> So, suppose that newrefname starts at value A, and there are two updates
> running simultaneously:
> 
> 1. An update of reference newrefname from A -> B
> 
> 2. A rename of reference oldrefname to newrefname, which includes
>    a. read_ref_full("newrefname") and
>    b. delete_ref("newrefname").

I wondered at first if you meant "oldrefname" in 2b. That is, a rename
would involve writing to "newrefname" and then deleting "oldrefname",
and not doing the old_sha1 and normal locking on the deletion of
oldrefname would be bad (in case somebody else updated it while we were
operating).

But reading the patch again, that's not what's going on. You're talking
just about the case where we force-overwrite an existing newrefname, and
delete it first to do so. But what I don't understand is:

  1. Why do we read_ref_full() at all? Since it is not done under lock
     anyway, it is useless for helping with race conditions, and I think
     that is what you are arguing (that we should just be deleting
     regardless). But then why not just call delete_ref(), and handle
     the ENOENT case as a noop (which closes another race if somebody
     deletes it between your 2a and 2b).

  2. Why delete it at all? The point is to overwrite, so I guess it is
     trying to make room. But we could just rename the loose ref file
     and reflog overtop the old, couldn't we?

Or am I totally misreading the purpose of this code?

-Peff

Re: [PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty]

On 04/29/2016 02:12 PM, Jeff King wrote:
> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:
> 
>> Remember, we're talking about rename_ref() only, not reference deletion
>> in general. rename_ref() is not very robust anyway--it doesn't happen in
>> a single transaction, and it is vulnerable to being defeated by
>> simultaneous reference updates by other processes. It *does* wrap the
>> deletion of newrefname in a transaction; the only question is whether an
>> old_sha1 is supplied to that transaction.
>>
>> So, suppose that newrefname starts at value A, and there are two updates
>> running simultaneously:
>>
>> 1. An update of reference newrefname from A -> B
>>
>> 2. A rename of reference oldrefname to newrefname, which includes
>>    a. read_ref_full("newrefname") and
>>    b. delete_ref("newrefname").
> 
> I wondered at first if you meant "oldrefname" in 2b. That is, a rename
> would involve writing to "newrefname" and then deleting "oldrefname",
> and not doing the old_sha1 and normal locking on the deletion of
> oldrefname would be bad (in case somebody else updated it while we were
> operating).
> 
> But reading the patch again, that's not what's going on. You're talking
> just about the case where we force-overwrite an existing newrefname, and
> delete it first to do so. But what I don't understand is:

It's beyond the ambition of this patch to fix this old rename_ref()
code, but...

>   1. Why do we read_ref_full() at all? Since it is not done under lock
>      anyway, it is useless for helping with race conditions, and I think
>      that is what you are arguing (that we should just be deleting
>      regardless). But then why not just call delete_ref(), and handle
>      the ENOENT case as a noop (which closes another race if somebody
>      deletes it between your 2a and 2b).

I thought about that too, and agree it would be an improvement. But it's
not quite so trivial. The ENOENT doesn't make it out of delete_ref()
(which does a full-fledged ref_transaction internally). The error is
only reported via "strbuf *err".

>   2. Why delete it at all? The point is to overwrite, so I guess it is
>      trying to make room. But we could just rename the loose ref file
>      and reflog overtop the old, couldn't we?
>
> Or am I totally misreading the purpose of this code?

I wouldn't want to just rename the files, because (1) I think it is
better to use the existing ref_transaction code, and (2) that wouldn't
work if there is a D/F conflict between the old and new reference names,
which the existing code handles (though honestly I'm skeptical that it
comes up a lot).

It would be more plausible to use ref_transaction_update() to write
oldrefname's *value* on top of newrefname without actually moving the
file. oldrefname could even be deleted in the same transaction, if you
were willing to stop supporting old/new refnames that D/F conflict with
each other. But we also want to move the reflog, and that should be done
while the newrefname lock and (contrary to the current code) also the
oldrefname lock are held. There's currently no way to slip an arbitrary
action like that into the middle of a transaction.

If I had lots of time and interest to work on this, I think the best
approach would be to add a ref_transaction_rename(), which takes an old
and a new reference name as arguments, and adds a whole rename operation
(including of the reflog) to a transaction. This could probably be
implemented by adding one ref_update() and one ref_delete(), and using
the parent_update pointer that is introduced later to link the two so
that ref_transaction_commit() knows to move the reflog too.

If you were willing to punt on D/F conflicts, you would be done. If not,
then it would be better to teach ref_transaction_commit() to deal with
D/F conflicts in the general case rather than using special-purpose code
in rename_ref().

Then rename_ref() could be omitted from the vtable entirely.

Michael

Re: [PATCH 19/29] refs: don't dereference on rename

[Jeff King]

On Fri, Apr 29, 2016 at 03:55:00PM +0200, Michael Haggerty wrote:

> It's beyond the ambition of this patch to fix this old rename_ref()
> code, but...
> [...]

Thanks for the explanation. That all makes sense to me, and I can
definitely live with "historical warts that aren't worth touching in
this series" as the verdict.

-Peff

Re: [PATCH 19/29] refs: don't dereference on rename

[Junio C Hamano]

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

> On 04/29/2016 02:12 PM, Jeff King wrote:
>> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:
>> 
>>> Remember, we're talking about rename_ref() only, not reference deletion
>>> in general. rename_ref() is not very robust anyway--it doesn't happen in
>>> a single transaction, and it is vulnerable to being defeated by
>>> simultaneous reference updates by other processes. It *does* wrap the
>>> deletion of newrefname in a transaction; the only question is whether an
>>> old_sha1 is supplied to that transaction.
>>>
>>> So, suppose that newrefname starts at value A, and there are two updates
>>> running simultaneously:
>>>
>>> 1. An update of reference newrefname from A -> B
>>>
>>> 2. A rename of reference oldrefname to newrefname, which includes
>>>    a. read_ref_full("newrefname") and
>>>    b. delete_ref("newrefname").
>> 
>> I wondered at first if you meant "oldrefname" in 2b. That is, a rename
>> would involve writing to "newrefname" and then deleting "oldrefname",
>> and not doing the old_sha1 and normal locking on the deletion of
>> oldrefname would be bad (in case somebody else updated it while we were
>> operating).
>> 
>> But reading the patch again, that's not what's going on. You're talking
>> just about the case where we force-overwrite an existing newrefname, and
>> delete it first to do so. But what I don't understand is:
>
> It's beyond the ambition of this patch to fix this old rename_ref()
> code, but...

Thanks for a clear explanation.  The NULL might want to be explained
in in-code comment (e.g. "here we do not care about verifying old
value because..."), then.

Re: [PATCH 19/29] refs: don't dereference on rename

[David Turner]

On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:
> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
> > Michael Haggerty <mhagger@alum.mit.edu> writes:
> > 
> > > @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
> > > const char *newrefname, const char *logms
> > >  		goto rollback;
> > >  	}
> > >  
> > > -	if (!read_ref_full(newrefname, RESOLVE_REF_READING,
> > > sha1, NULL) &&
> > > -	    delete_ref(newrefname, sha1, REF_NODEREF)) {
> > > +	if (!read_ref_full(newrefname, resolve_flags, sha1,
> > > NULL) &&
> > > +	    delete_ref(newrefname, NULL, REF_NODEREF)) {
> > 
> > Could you explain s/sha1/NULL/ here in the proposed log message?
> 
> Good question.
> 
> Passing sha1 to delete_ref() doesn't add any safety, because the same
> sha1 was just read a moment before, and it is not used for anything
> else. So the check only protects us from a concurrent update to
> newrefname between the call to read_ref_full() and the call to
> delete_ref(). But such a race is indistinguishable from the case that
> a
> modification to newrefname happens just before the call to
> read_ref_full(), which would have the same outcome as the new code.
> So
> the "sha1" check only adds ways for the rename() to fail in
> situations
> where nothing harmful would have happened anyway.
> 
> That being said, this is a very unlikely race, and I don't think it
> matters much either way. In any case, the change s/sha1/NULL/ here
> seems
> orthogonal to the rest of the patch.
> 
> David, you wrote the original version of this patch. Am I overlooking
> something?

I think I might have been handling some weird case related to symbolic
refs, but I don't recall the details.  Your argument seems right to me.

Re: [PATCH 19/29] refs: don't dereference on rename

[Michael Haggerty]

On 04/30/2016 01:21 AM, David Turner wrote:
> On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:
>> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
>>>> const char *newrefname, const char *logms
>>>>  		goto rollback;
>>>>  	}
>>>>  
>>>> -	if (!read_ref_full(newrefname, RESOLVE_REF_READING,
>>>> sha1, NULL) &&
>>>> -	    delete_ref(newrefname, sha1, REF_NODEREF)) {
>>>> +	if (!read_ref_full(newrefname, resolve_flags, sha1,
>>>> NULL) &&
>>>> +	    delete_ref(newrefname, NULL, REF_NODEREF)) {
>>>
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else. So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that
>> a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
>> So
>> the "sha1" check only adds ways for the rename() to fail in
>> situations
>> where nothing harmful would have happened anyway.
>>
>> That being said, this is a very unlikely race, and I don't think it
>> matters much either way. In any case, the change s/sha1/NULL/ here
>> seems
>> orthogonal to the rest of the patch.
>>
>> David, you wrote the original version of this patch. Am I overlooking
>> something?
> 
> I think I might have been handling some weird case related to symbolic
> refs, but I don't recall the details.  Your argument seems right to me.

Doh, of course. I should have just changed it back to `sha1` and run the
test suite, then I would have seen the failure...

The point is that `read_ref_full()` is now called with
`RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic
reference, then `read_ref_full()` sets `sha1` to zeros. But the
pre-check for `delete_ref()` compares `old_sha1` to the recursively
resolved value of the reference, so that check would fail. (In fact,
`ref_transaction_delete()` refuses even to add the deletion to the
transaction if `old_sha1` is zeros, so it doesn't even get that far.)

So for shallow technical reasons we can't pass `sha1` to `delete_ref()`
anymore, and for the deeper reasons discussed in this thread that's not
a problem.

I'll document this in v2 of this patch.

Michael

Re: [PATCH 19/29] refs: don't dereference on rename

[Junio C Hamano]

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

> The point is that `read_ref_full()` is now called with
> `RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic
> reference, then `read_ref_full()` sets `sha1` to zeros.

Yes, that was an obvious rationale in the patch that was not
explained in the proposed log message (and made me ask you to
explain it).  I was wondering why this was not loosened
conditionally (i.e. only pass null_sha1 when symbolic ref is
involved, in which case you _must_ pass null_sha1 because we no
longer have anything to compare with).

Your explanation on the "in all possible interleaving, deletion of
what might have been updated in the middle by other people does not
matter" was a sufficient explanation why it does not have to be
conditional.

> I'll document this in v2 of this patch.

Thanks.

[PATCH 20/29] verify_refname_available(): adjust constness in declaration

[Michael Haggerty]

The two string_list arguments can be const.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This is needed because I want to add a new caller that has const
versions of these lists in hand.

 refs/files-backend.c | 4 ++--
 refs/refs-internal.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e4cdd5a..e0d9fa3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2328,8 +2328,8 @@ out:
 }
 
 int verify_refname_available(const char *newname,
-			     struct string_list *extras,
-			     struct string_list *skip,
+			     const struct string_list *extras,
+			     const struct string_list *skip,
 			     struct strbuf *err)
 {
 	struct ref_dir *packed_refs = get_packed_refs(&ref_cache);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 6b53ba1..aaf56ea 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -116,8 +116,8 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1);
  * extras and skip must be sorted.
  */
 int verify_refname_available(const char *newname,
-			     struct string_list *extras,
-			     struct string_list *skip,
+			     const struct string_list *extras,
+			     const struct string_list *skip,
 			     struct strbuf *err);
 
 /*
-- 
2.8.1

[PATCH 21/29] add_update(): initialize the whole ref_update

[Michael Haggerty]

Change add_update() to initialize all of the fields in the new
ref_update object. Rename the function to ref_transaction_add_update(),
and increase its visibility to all of the refs-related code.

All of this makes the function more useful for other future callers.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
In particular, it's nice that it returns the "struct ref_update *"
that it creates, which is useful for refs-internal code that might
want to tweak other field in the ref_update. But we don't want
non-refs code to get its hands on a ref_update.

 refs.c               | 33 +++++++++++++++++----------------
 refs/refs-internal.h | 14 ++++++++++++++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 5dc2473..858b6d7 100644
--- a/refs.c
+++ b/refs.c
@@ -766,13 +766,24 @@ void ref_transaction_free(struct ref_transaction *transaction)
 	free(transaction);
 }
 
-static struct ref_update *add_update(struct ref_transaction *transaction,
-				     const char *refname)
+struct ref_update *ref_transaction_add_update(
+		struct ref_transaction *transaction,
+		const char *refname, unsigned int flags,
+		const unsigned char *new_sha1,
+		const unsigned char *old_sha1,
+		const char *msg)
 {
 	struct ref_update *update;
 	FLEX_ALLOC_STR(update, refname, refname);
 	ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
 	transaction->updates[transaction->nr++] = update;
+	update->flags = flags;
+	if (flags & REF_HAVE_NEW)
+		hashcpy(update->new_sha1, new_sha1);
+	if (flags & REF_HAVE_OLD)
+		hashcpy(update->old_sha1, old_sha1);
+	if (msg)
+		update->msg = xstrdup(msg);
 	return update;
 }
 
@@ -783,8 +794,6 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   unsigned int flags, const char *msg,
 			   struct strbuf *err)
 {
-	struct ref_update *update;
-
 	assert(err);
 
 	if (transaction->state != REF_TRANSACTION_OPEN)
@@ -800,18 +809,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	update = add_update(transaction, refname);
-	if (new_sha1) {
-		hashcpy(update->new_sha1, new_sha1);
-		flags |= REF_HAVE_NEW;
-	}
-	if (old_sha1) {
-		hashcpy(update->old_sha1, old_sha1);
-		flags |= REF_HAVE_OLD;
-	}
-	update->flags = flags;
-	if (msg)
-		update->msg = xstrdup(msg);
+	flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);
+
+	ref_transaction_add_update(transaction, refname, flags,
+				   new_sha1, old_sha1, msg);
 	return 0;
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index aaf56ea..c46fe67 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -158,6 +158,20 @@ struct ref_update {
 };
 
 /*
+ * Add a ref_update with the specified properties to transaction, and
+ * return a pointer to the new object. This function does not verify
+ * that refname is well-formed. new_sha1 and old_sha1 are only
+ * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits,
+ * respectively, are set in flags.
+ */
+struct ref_update *ref_transaction_add_update(
+		struct ref_transaction *transaction,
+		const char *refname, unsigned int flags,
+		const unsigned char *new_sha1,
+		const unsigned char *old_sha1,
+		const char *msg);
+
+/*
  * Transaction states.
  * OPEN:   The transaction is in a valid state and can accept new updates.
  *         An OPEN transaction can be committed.
-- 
2.8.1

[PATCH 22/29] lock_ref_for_update(): new function

[Michael Haggerty]

Extract a new function, lock_ref_for_update(), from
ref_transaction_commit().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 152 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 85 insertions(+), 67 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e0d9fa3..546656a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3048,6 +3048,88 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
 	return 0;
 }
 
+/*
+ * Acquire all locks, verify old values if provided, check
+ * that new values are valid, and write new values to the
+ * lockfiles, ready to be activated. Only keep one lockfile
+ * open at a time to avoid running out of file descriptors.
+ */
+static int lock_ref_for_update(struct ref_update *update,
+			       struct ref_transaction *transaction,
+			       struct string_list *affected_refnames,
+			       struct strbuf *err)
+{
+	int ret;
+
+	if ((update->flags & REF_HAVE_NEW) &&
+	    is_null_sha1(update->new_sha1))
+		update->flags |= REF_DELETING;
+	update->lock = lock_ref_sha1_basic(
+			update->refname,
+			((update->flags & REF_HAVE_OLD) ?
+			 update->old_sha1 : NULL),
+			affected_refnames, NULL,
+			update->flags,
+			&update->type,
+			err);
+	if (!update->lock) {
+		char *reason;
+
+		ret = (errno == ENOTDIR)
+			? TRANSACTION_NAME_CONFLICT
+			: TRANSACTION_GENERIC_ERROR;
+		reason = strbuf_detach(err, NULL);
+		strbuf_addf(err, "cannot lock ref '%s': %s",
+			    update->refname, reason);
+		free(reason);
+		return ret;
+	}
+	if ((update->flags & REF_HAVE_NEW) &&
+	    !(update->flags & REF_DELETING) &&
+	    !(update->flags & REF_LOG_ONLY)) {
+		int overwriting_symref = ((update->type & REF_ISSYMREF) &&
+					  (update->flags & REF_NODEREF));
+
+		if (!overwriting_symref &&
+		    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
+			/*
+			 * The reference already has the desired
+			 * value, so we don't need to write it.
+			 */
+		} else if (write_ref_to_lockfile(update->lock,
+						 update->new_sha1,
+						 err)) {
+			char *write_err = strbuf_detach(err, NULL);
+
+			/*
+			 * The lock was freed upon failure of
+			 * write_ref_to_lockfile():
+			 */
+			update->lock = NULL;
+			strbuf_addf(err,
+				    "cannot update the ref '%s': %s",
+				    update->refname, write_err);
+			free(write_err);
+			return TRANSACTION_GENERIC_ERROR;
+		} else {
+			update->flags |= REF_NEEDS_COMMIT;
+		}
+	}
+	if (!(update->flags & REF_NEEDS_COMMIT)) {
+		/*
+		 * We didn't call write_ref_to_lockfile(), so
+		 * the lockfile is still open. Close it to
+		 * free up the file descriptor:
+		 */
+		if (close_ref(update->lock)) {
+			strbuf_addf(err, "couldn't close '%s.lock'",
+				    update->refname);
+			return TRANSACTION_GENERIC_ERROR;
+		}
+	}
+	return 0;
+}
+
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
@@ -3085,74 +3167,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
-		if ((update->flags & REF_HAVE_NEW) &&
-		    is_null_sha1(update->new_sha1))
-			update->flags |= REF_DELETING;
-		update->lock = lock_ref_sha1_basic(
-				update->refname,
-				((update->flags & REF_HAVE_OLD) ?
-				 update->old_sha1 : NULL),
-				&affected_refnames, NULL,
-				update->flags,
-				&update->type,
-				err);
-		if (!update->lock) {
-			char *reason;
-
-			ret = (errno == ENOTDIR)
-				? TRANSACTION_NAME_CONFLICT
-				: TRANSACTION_GENERIC_ERROR;
-			reason = strbuf_detach(err, NULL);
-			strbuf_addf(err, "cannot lock ref '%s': %s",
-				    update->refname, reason);
-			free(reason);
+		ret = lock_ref_for_update(update, transaction,
+					  &affected_refnames, err);
+		if (ret)
 			goto cleanup;
-		}
-		if ((update->flags & REF_HAVE_NEW) &&
-		    !(update->flags & REF_DELETING) &&
-		    !(update->flags & REF_LOG_ONLY)) {
-			int overwriting_symref = ((update->type & REF_ISSYMREF) &&
-						  (update->flags & REF_NODEREF));
-
-			if (!overwriting_symref &&
-			    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
-				/*
-				 * The reference already has the desired
-				 * value, so we don't need to write it.
-				 */
-			} else if (write_ref_to_lockfile(update->lock,
-							 update->new_sha1,
-							 err)) {
-				char *write_err = strbuf_detach(err, NULL);
-
-				/*
-				 * The lock was freed upon failure of
-				 * write_ref_to_lockfile():
-				 */
-				update->lock = NULL;
-				strbuf_addf(err,
-					    "cannot update the ref '%s': %s",
-					    update->refname, write_err);
-				free(write_err);
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto cleanup;
-			} else {
-				update->flags |= REF_NEEDS_COMMIT;
-			}
-		}
-		if (!(update->flags & REF_NEEDS_COMMIT)) {
-			/*
-			 * We didn't call write_ref_to_lockfile(), so
-			 * the lockfile is still open. Close it to
-			 * free up the file descriptor:
-			 */
-			if (close_ref(update->lock)) {
-				strbuf_addf(err, "couldn't close '%s.lock'",
-					    update->refname);
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto cleanup;
-			}
-		}
 	}
 
 	/* Perform updates first so live commits remain referenced */
-- 
2.8.1

[PATCH 23/29] unlock_ref(): move definition higher in the file

[Michael Haggerty]

This avoids the need for a forward declaration in the next patch.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 546656a..8f2a795 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1545,6 +1545,16 @@ out:
 	return ret;
 }
 
+static void unlock_ref(struct ref_lock *lock)
+{
+	/* Do not free lock->lk -- atexit() still looks at them */
+	if (lock->lk)
+		rollback_lock_file(lock->lk);
+	free(lock->ref_name);
+	free(lock->orig_ref_name);
+	free(lock);
+}
+
 /*
  * Peel the entry (if possible) and return its new peel_status.  If
  * repeel is true, re-peel the entry even if there is an old peeled
@@ -1703,16 +1713,6 @@ int do_for_each_ref(const char *submodule, const char *base,
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
-static void unlock_ref(struct ref_lock *lock)
-{
-	/* Do not free lock->lk -- atexit() still looks at them */
-	if (lock->lk)
-		rollback_lock_file(lock->lk);
-	free(lock->ref_name);
-	free(lock->orig_ref_name);
-	free(lock);
-}
-
 /*
  * Verify that the reference locked by lock has the value old_sha1.
  * Fail if the reference doesn't exist and mustexist is set. Return 0
-- 
2.8.1

[PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

[Michael Haggerty]

If the user has asked that a new value be set for a reference, we use
check_refname_format() to verify that the reference name satisfies all
of the rules. But in other cases, at least check that refname_is_safe().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
There are remaining problems in this area of the code. For example,
check_refname_format() is *less* strict than refname_is_safe(). It
allows almost arbitrary top-level reference names like "foo" and
"refs". (The latter is especially fun because if you manage to create
a reference called "refs", Git stops recognizing the directory as a
Git repository.) On the other hand, some callers call
check_refname_format() with incomplete reference names (e.g., branch
names like "master"), so the functions can't be made stricter until
those callers are changed.

I'll address these problems separately if I find the time.

 refs.c                  | 5 +++--
 t/t1400-update-ref.sh   | 2 +-
 t/t1430-bad-ref-name.sh | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 858b6d7..41eb9e2 100644
--- a/refs.c
+++ b/refs.c
@@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
 		die("BUG: REF_ISPRUNING set without REF_NODEREF");
 
-	if (new_sha1 && !is_null_sha1(new_sha1) &&
-	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+	if ((new_sha1 && !is_null_sha1(new_sha1)) ?
+	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+	    !refname_is_safe(refname)) {
 		strbuf_addf(err, "refusing to update ref with bad name '%s'",
 			    refname);
 		return -1;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 40b0cce..08bd8fd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
 m=refs/heads/master
 n_dir=refs/heads/gu
 n=$n_dir/fixes
-outside=foo
+outside=refs/foo
 
 test_expect_success \
 	"create $m" \
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 25ddab4..8937e25 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
 	echo precious >expect &&
 	test_must_fail git update-ref -d my-private-file >output 2>error &&
 	test_must_be_empty output &&
-	test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
+	test_i18ngrep -e "refusing to update ref with bad name" error &&
 	test_cmp expect .git/my-private-file
 '
 
-- 
2.8.1

Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

[Junio C Hamano]

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

> If the user has asked that a new value be set for a reference, we use
> check_refname_format() to verify that the reference name satisfies all
> of the rules. But in other cases, at least check that refname_is_safe().

It isn't clear to me what "in other cases" exactly refers to.  A
request to delete a ref would obviously one of those that do not
"ask that a new value be set", but are there other cases?

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> There are remaining problems in this area of the code. For example,
> check_refname_format() is *less* strict than refname_is_safe(). It
> allows almost arbitrary top-level reference names like "foo" and
> "refs". (The latter is especially fun because if you manage to create
> a reference called "refs", Git stops recognizing the directory as a
> Git repository.) On the other hand, some callers call
> check_refname_format() with incomplete reference names (e.g., branch
> names like "master"), so the functions can't be made stricter until
> those callers are changed.
>
> I'll address these problems separately if I find the time.

Thanks.

>  refs.c                  | 5 +++--
>  t/t1400-update-ref.sh   | 2 +-
>  t/t1430-bad-ref-name.sh | 2 +-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 858b6d7..41eb9e2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  	if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
>  		die("BUG: REF_ISPRUNING set without REF_NODEREF");
>  
> -	if (new_sha1 && !is_null_sha1(new_sha1) &&
> -	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> +	if ((new_sha1 && !is_null_sha1(new_sha1)) ?
> +	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +	    !refname_is_safe(refname)) {
>  		strbuf_addf(err, "refusing to update ref with bad name '%s'",
>  			    refname);
>  		return -1;
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 40b0cce..08bd8fd 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -23,7 +23,7 @@ test_expect_success setup '
>  m=refs/heads/master
>  n_dir=refs/heads/gu
>  n=$n_dir/fixes
> -outside=foo
> +outside=refs/foo
>  
>  test_expect_success \
>  	"create $m" \
> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index 25ddab4..8937e25 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
>  	echo precious >expect &&
>  	test_must_fail git update-ref -d my-private-file >output 2>error &&
>  	test_must_be_empty output &&
> -	test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
> +	test_i18ngrep -e "refusing to update ref with bad name" error &&
>  	test_cmp expect .git/my-private-file
>  '

Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

[Michael Haggerty]

On 04/27/2016 10:14 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If the user has asked that a new value be set for a reference, we use
>> check_refname_format() to verify that the reference name satisfies all
>> of the rules. But in other cases, at least check that refname_is_safe().
> 
> It isn't clear to me what "in other cases" exactly refers to.  A
> request to delete a ref would obviously one of those that do not
> "ask that a new value be set", but are there other cases?

The other case is `verify`, which can be used to check the old value of
a reference without modifying it. `verify` is exposed via `git
update-ref --stdin`

Michael

Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum

[Junio C Hamano]

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

> On 04/27/2016 10:14 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> If the user has asked that a new value be set for a reference, we use
>>> check_refname_format() to verify that the reference name satisfies all
>>> of the rules. But in other cases, at least check that refname_is_safe().
>> 
>> It isn't clear to me what "in other cases" exactly refers to.  A
>> request to delete a ref would obviously one of those that do not
>> "ask that a new value be set", but are there other cases?
>
> The other case is `verify`, which can be used to check the old value of
> a reference without modifying it. `verify` is exposed via `git
> update-ref --stdin`

Thanks.

[PATCH 25/29] refs: resolve symbolic refs first

[Michael Haggerty]

Before committing ref updates, split symbolic ref updates into two
parts: an update to the underlying ref, and a log-only update to the
symbolic ref. This ensures that both references are locked correctly
during the transaction, including while their reflogs are updated.

Similarly, if the reference pointed to by HEAD is modified directly, add
a separate log-only update to HEAD, rather than leaving the job of
updating HEAD's reflog to commit_ref_update(). This change ensures that
HEAD is locked correctly while its reflog is being modified, as well as
being cheaper (HEAD only needs to be resolved once).

This makes use of a new function, lock_ref_raw(), which is analogous to
read_ref_raw(), but acquires a lock on the reference before reading it.

This change still has two problems:

* There are redundant read_ref_full() reference lookups.

* It is still possible to get incorrect reflogs for symbolic references
  if there is a concurrent update by another process, since the old_oid
  of a symref is determined before the lock on the pointed-to ref is
  held.

Both problems will soon be fixed.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c  | 513 +++++++++++++++++++++++++++++++++++++++++++++-----
 refs/refs-internal.h  |  11 +-
 t/t1400-update-ref.sh |  35 ++++
 3 files changed, 514 insertions(+), 45 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8f2a795..d0e932f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1556,6 +1556,226 @@ static void unlock_ref(struct ref_lock *lock)
 }
 
 /*
+ * Lock refname, without following symrefs, and set lock to point at a
+ * newly-allocated lock object. Fill in lock->old_oid, referent, and
+ * type similarly to read_raw_ref().
+ *
+ * The caller must verify that refname is a "safe" reference name (in
+ * the sense of refname_is_safe()) before calling this function.
+ *
+ * If the reference doesn't already exist, verify that refname doesn't
+ * have a D/F conflict with any existing references. extras and skip
+ * are passed to verify_refname_available_dir() for this check.
+ *
+ * If mustexist is not set and the reference is not found or is
+ * broken, lock the reference anyway but clear sha1.
+ *
+ * Return 0 on success. On failure, write an error message to err and
+ * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
+ *
+ * Implementation note: This function is basically
+ *
+ *     lock reference
+ *     read_raw_ref()
+ *
+ * but it includes a lot more code to
+ * - Deal with possible races with other processes
+ * - Avoid calling verify_refname_available_dir() when it can be
+ *   avoided, namely if we were successfully able to read the ref
+ * - Generate informative error messages in the case of failure
+ */
+static int lock_raw_ref(const char *refname, int deleting, int mustexist,
+			const struct string_list *extras,
+			const struct string_list *skip,
+			struct ref_lock **lock_p,
+			struct strbuf *referent,
+			unsigned int *type,
+			struct strbuf *err)
+{
+	struct ref_lock *lock;
+	struct strbuf ref_file = STRBUF_INIT;
+	int attempts_remaining = 3;
+	int ret = TRANSACTION_GENERIC_ERROR;
+
+	assert(err);
+	*type = 0;
+
+	/* First lock the file so it can't change out from under us. */
+
+	*lock_p = lock = xcalloc(1, sizeof(*lock));
+
+	lock->ref_name = xstrdup(refname);
+	lock->orig_ref_name = xstrdup(refname);
+	strbuf_git_path(&ref_file, "%s", refname);
+
+retry:
+	switch (safe_create_leading_directories(ref_file.buf)) {
+	case SCLD_OK:
+		break; /* success */
+	case SCLD_EXISTS:
+		/*
+		 * Suppose refname is "refs/foo/bar". We just failed
+		 * to create the containing directory, "refs/foo",
+		 * because there was a non-directory in the way. This
+		 * indicates a D/F conflict, probably because of
+		 * another reference such as "refs/foo". There is no
+		 * reason to expect this error to be transitory.
+		 */
+		if (verify_refname_available(refname, extras, skip, err)) {
+			if (mustexist) {
+				/*
+				 * To the user the relevant error is
+				 * that the "mustexist" reference is
+				 * missing:
+				 */
+				strbuf_reset(err);
+				strbuf_addf(err, "unable to resolve reference '%s'",
+					    refname);
+			} else {
+				/*
+				 * The error message set by
+				 * verify_refname_available_dir() is OK.
+				 */
+				ret = TRANSACTION_NAME_CONFLICT;
+			}
+		} else {
+			/*
+			 * The file that is in the way isn't a loose
+			 * reference. Report it as a low-level
+			 * failure.
+			 */
+			strbuf_addf(err, "unable to create lock file %s.lock; "
+				    "non-directory in the way",
+				    ref_file.buf);
+		}
+		goto error_return;
+	case SCLD_VANISHED:
+		/* Maybe another process was tidying up. Try again. */
+		if (--attempts_remaining > 0)
+			goto retry;
+		/* fall through */
+	default:
+		strbuf_addf(err, "unable to create directory for %s",
+			    ref_file.buf);
+		goto error_return;
+	}
+
+	if (!lock->lk)
+		lock->lk = xcalloc(1, sizeof(struct lock_file));
+
+	if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {
+		if (errno == ENOENT && --attempts_remaining > 0) {
+			/*
+			 * Maybe somebody just deleted one of the
+			 * directories leading to ref_file.  Try
+			 * again:
+			 */
+			goto retry;
+		} else {
+			unable_to_lock_message(ref_file.buf, errno, err);
+			goto error_return;
+		}
+	}
+
+	/*
+	 * Now we hold the lock and can read the reference without
+	 * fear that its value will change.
+	 */
+
+	if (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {
+		if (errno == ENOENT) {
+			if (mustexist) {
+				/* Garden variety missing reference. */
+				strbuf_addf(err, "unable to resolve reference '%s'",
+					    refname);
+				goto error_return;
+			} else if (verify_refname_available_dir(
+						   refname, extras, skip,
+						   get_loose_refs(&ref_cache),
+						   err)) {
+				/*
+				 * The error message set by
+				 * verify_refname_available() is OK.
+				 */
+				ret = TRANSACTION_NAME_CONFLICT;
+				goto error_return;
+			} else {
+				/* Reference is missing, but that's OK */
+			}
+		} else if (errno == EISDIR) {
+			if (mustexist) {
+				/*
+				 * Garden variety missing reference.
+				 */
+				strbuf_addf(err, "unable to resolve reference '%s'",
+					    refname);
+				goto error_return;
+			} else if (remove_dir_recursively(&ref_file,
+							  REMOVE_DIR_EMPTY_ONLY)) {
+				if (verify_refname_available_dir(
+						    refname, extras, skip,
+						    get_loose_refs(&ref_cache),
+						    err)) {
+					/*
+					 * The error message set by
+					 * verify_refname_available() is OK.
+					 */
+					ret = TRANSACTION_NAME_CONFLICT;
+					goto error_return;
+				} else {
+					/*
+					 * There is a directory in the way,
+					 * but we don't know of any references
+					 * that it should contain. This might
+					 * be a directory that used to contain
+					 * references but is now empty. Try to
+					 * remove it; otherwise it might cause
+					 * trouble when we try to rename the
+					 * lockfile into place.
+					 */
+					strbuf_addf(err, "there is a non-empty directory '%s' "
+						    "blocking reference '%s'",
+						    ref_file.buf, refname);
+					goto error_return;
+				}
+			}
+		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
+			strbuf_addf(err, "unable to resolve reference '%s': "
+				    "reference broken", refname);
+			goto error_return;
+		} else {
+			strbuf_addf(err, "unable to resolve reference '%s': %s",
+				    refname, strerror(errno));
+			goto error_return;
+		}
+
+		/*
+		 * If the ref did not exist and we are creating it,
+		 * make sure there is no existing packed ref whose
+		 * name begins with our refname, nor a packed ref
+		 * whose name is a proper prefix of our refname.
+		 */
+		if (verify_refname_available_dir(
+				    refname, extras, skip,
+				    get_packed_refs(&ref_cache),
+				    err)) {
+			goto error_return;
+		}
+	}
+
+	ret = 0;
+	goto out;
+
+error_return:
+	unlock_ref(lock);
+	*lock_p = NULL;
+
+out:
+	strbuf_release(&ref_file);
+	return ret;
+}
+
+/*
  * Peel the entry (if possible) and return its new peel_status.  If
  * repeel is true, re-peel the entry even if there is an old peeled
  * value that is already stored in it.
@@ -3049,55 +3269,203 @@ static int ref_update_reject_duplicates(struct string_list *refnames,
 }
 
 /*
- * Acquire all locks, verify old values if provided, check
- * that new values are valid, and write new values to the
- * lockfiles, ready to be activated. Only keep one lockfile
- * open at a time to avoid running out of file descriptors.
+ * If update is a direct update of head_ref (the reference pointed to
+ * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
+ */
+static int split_head_update(struct ref_update *update,
+			     struct ref_transaction *transaction,
+			     const char *head_ref,
+			     struct string_list *affected_refnames,
+			     struct strbuf *err)
+{
+	struct string_list_item *item;
+	struct ref_update *new_update;
+
+	if ((update->flags & REF_LOG_ONLY) ||
+	    (update->flags & REF_ISPRUNING) ||
+	    (update->flags & REF_UPDATE_VIA_HEAD))
+		return 0;
+
+	if (strcmp(update->refname, head_ref))
+		return 0;
+
+	/*
+	 * First make sure that HEAD is not already in the
+	 * transaction. This insertion is O(N) in the transaction
+	 * size, but it happens at most once per transaction.
+	 */
+	item = string_list_insert(affected_refnames, "HEAD");
+	if (item->util) {
+		/* An entry already existed */
+		strbuf_addf(err,
+			    "multiple updates for 'HEAD' (including one "
+			    "via its referent '%s') are not allowed",
+			    update->refname);
+		return TRANSACTION_NAME_CONFLICT;
+	}
+
+	new_update = ref_transaction_add_update(
+			transaction, "HEAD",
+			update->flags | REF_LOG_ONLY | REF_NODEREF,
+			update->new_sha1, update->old_sha1,
+			update->msg);
+
+	item->util = new_update;
+
+	return 0;
+}
+
+/*
+ * update is for a symref that points at referent and doesn't have
+ * REF_NODEREF set. Split it into two updates:
+ * - The original update, but with REF_LOG_ONLY and REF_NODEREF set
+ * - A new, separate update for the referent reference
+ * Note that the new update will itself be subject to splitting when
+ * the iteration gets to it.
+ */
+static int split_symref_update(struct ref_update *update,
+			       const char *referent,
+			       struct ref_transaction *transaction,
+			       struct string_list *affected_refnames,
+			       struct strbuf *err)
+{
+	struct string_list_item *item;
+	struct ref_update *new_update;
+	unsigned int new_flags;
+
+	/*
+	 * First make sure that referent is not already in the
+	 * transaction. This insertion is O(N) in the transaction
+	 * size, but it happens at most once per symref in a
+	 * transaction.
+	 */
+	item = string_list_insert(affected_refnames, referent);
+	if (item->util) {
+		/* An entry already existed */
+		strbuf_addf(err,
+			    "multiple updates for '%s' (including one "
+			    "via symref '%s') are not allowed",
+			    referent, update->refname);
+		return TRANSACTION_NAME_CONFLICT;
+	}
+
+	new_flags = update->flags;
+	if (!strcmp(update->refname, "HEAD")) {
+		/*
+		 * Record that the new update came via HEAD, so that
+		 * when we process it, split_head_update() doesn't try
+		 * to add another reflog update for HEAD. Note that
+		 * this bit will be propagated if the new_update
+		 * itself needs to be split.
+		 */
+		new_flags |= REF_UPDATE_VIA_HEAD;
+	}
+
+	new_update = ref_transaction_add_update(
+			transaction, referent, new_flags,
+			update->new_sha1, update->old_sha1,
+			update->msg);
+
+	/* Change the symbolic ref update to log only: */
+	update->flags |= REF_LOG_ONLY | REF_NODEREF;
+
+	item->util = new_update;
+
+	return 0;
+}
+
+/*
+ * Prepare for carrying out update:
+ * - Lock the reference referred to by update.
+ * - Read the reference under lock.
+ * - Check that its old SHA-1 value (if specified) is correct, and in
+ *   any case record it for later use in the reflog.
+ * - If it is a symref update without REF_NODEREF, split it up into a
+ *   REF_LOG_ONLY update of the symref and add a separate update for
+ *   the referent to transaction.
+ * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY
+ *   update of HEAD.
  */
 static int lock_ref_for_update(struct ref_update *update,
 			       struct ref_transaction *transaction,
+			       const char *head_ref,
 			       struct string_list *affected_refnames,
 			       struct strbuf *err)
 {
+	struct strbuf referent = STRBUF_INIT;
+	int mustexist = (update->flags & REF_HAVE_OLD) &&
+		!is_null_sha1(update->old_sha1);
 	int ret;
+	struct ref_lock *lock;
 
-	if ((update->flags & REF_HAVE_NEW) &&
-	    is_null_sha1(update->new_sha1))
+	if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
 		update->flags |= REF_DELETING;
-	update->lock = lock_ref_sha1_basic(
-			update->refname,
-			((update->flags & REF_HAVE_OLD) ?
-			 update->old_sha1 : NULL),
-			affected_refnames, NULL,
-			update->flags,
-			&update->type,
-			err);
-	if (!update->lock) {
+
+	if (head_ref) {
+		ret = split_head_update(update, transaction, head_ref,
+					affected_refnames, err);
+		if (ret)
+			return ret;
+	}
+
+	ret = lock_raw_ref(update->refname,
+			   update->flags & REF_DELETING,
+			   mustexist,
+			   affected_refnames, NULL,
+			   &update->lock, &referent,
+			   &update->type, err);
+
+	if (ret) {
 		char *reason;
 
-		ret = (errno == ENOTDIR)
-			? TRANSACTION_NAME_CONFLICT
-			: TRANSACTION_GENERIC_ERROR;
 		reason = strbuf_detach(err, NULL);
 		strbuf_addf(err, "cannot lock ref '%s': %s",
 			    update->refname, reason);
 		free(reason);
 		return ret;
 	}
+
+	lock = update->lock;
+
+	if (read_ref_full(update->refname,
+			  mustexist ? RESOLVE_REF_READING : 0,
+			  lock->old_oid.hash, NULL)) {
+		if (update->flags & REF_HAVE_OLD) {
+			strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
+				    update->refname);
+			return TRANSACTION_GENERIC_ERROR;
+		} else {
+			hashclr(lock->old_oid.hash);
+		}
+	}
+	if ((update->flags & REF_HAVE_OLD) &&
+	    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+		strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+			    update->refname,
+			    sha1_to_hex(lock->old_oid.hash),
+			    sha1_to_hex(update->old_sha1));
+		return TRANSACTION_GENERIC_ERROR;
+	}
+
+	if (update->type & REF_ISSYMREF) {
+		if (!(update->flags & REF_NODEREF)) {
+			ret = split_symref_update(update, referent.buf, transaction,
+						  affected_refnames, err);
+			if (ret)
+				return ret;
+		}
+	}
+
 	if ((update->flags & REF_HAVE_NEW) &&
 	    !(update->flags & REF_DELETING) &&
 	    !(update->flags & REF_LOG_ONLY)) {
-		int overwriting_symref = ((update->type & REF_ISSYMREF) &&
-					  (update->flags & REF_NODEREF));
-
-		if (!overwriting_symref &&
-		    !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
+		if (!(update->type & REF_ISSYMREF) &&
+		    !hashcmp(lock->old_oid.hash, update->new_sha1)) {
 			/*
 			 * The reference already has the desired
 			 * value, so we don't need to write it.
 			 */
-		} else if (write_ref_to_lockfile(update->lock,
-						 update->new_sha1,
+		} else if (write_ref_to_lockfile(lock, update->new_sha1,
 						 err)) {
 			char *write_err = strbuf_detach(err, NULL);
 
@@ -3121,7 +3489,7 @@ static int lock_ref_for_update(struct ref_update *update,
 		 * the lockfile is still open. Close it to
 		 * free up the file descriptor:
 		 */
-		if (close_ref(update->lock)) {
+		if (close_ref(lock)) {
 			strbuf_addf(err, "couldn't close '%s.lock'",
 				    update->refname);
 			return TRANSACTION_GENERIC_ERROR;
@@ -3138,6 +3506,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
 	struct string_list_item *ref_to_delete;
 	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+	char *head_ref = NULL;
+	int head_type;
+	struct object_id head_oid;
 
 	assert(err);
 
@@ -3149,9 +3520,25 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Fail if a refname appears more than once in the transaction: */
-	for (i = 0; i < transaction->nr; i++)
-		string_list_append(&affected_refnames, updates[i]->refname);
+	/*
+	 * Fail if a refname appears more than once in the
+	 * transaction. (If we end up splitting up any updates using
+	 * split_symref_update() or split_head_update(), those
+	 * functions will check that the new updates don't have the
+	 * same refname as any existing ones.)
+	 */
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *update = updates[i];
+		struct string_list_item *item =
+			string_list_append(&affected_refnames, update->refname);
+
+		/*
+		 * We store a pointer to update in item->util, but at
+		 * the moment we never use the value of this field
+		 * except to check whether it is non-NULL.
+		 */
+		item->util = update;
+	}
 	string_list_sort(&affected_refnames);
 	if (ref_update_reject_duplicates(&affected_refnames, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
@@ -3159,6 +3546,32 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	}
 
 	/*
+	 * Special hack: If a branch is updated directly and HEAD
+	 * points to it (may happen on the remote side of a push
+	 * for example) then logically the HEAD reflog should be
+	 * updated too.
+	 *
+	 * A generic solution would require reverse symref lookups,
+	 * but finding all symrefs pointing to a given branch would be
+	 * rather costly for this rare event (the direct update of a
+	 * branch) to be worth it. So let's cheat and check with HEAD
+	 * only, which should cover 99% of all usage scenarios (even
+	 * 100% of the default ones).
+	 *
+	 * So if HEAD is a symbolic reference, then record the name of
+	 * the reference that it points to. If we see an update of
+	 * head_ref within the transaction, then split_head_update()
+	 * arranges for the reflog of HEAD to be updated, too.
+	 */
+	head_ref = resolve_refdup("HEAD", RESOLVE_REF_NO_RECURSE,
+				  head_oid.hash, &head_type);
+
+	if (head_ref && !(head_type & REF_ISSYMREF)) {
+		free(head_ref);
+		head_ref = NULL;
+	}
+
+	/*
 	 * Acquire all locks, verify old values if provided, check
 	 * that new values are valid, and write new values to the
 	 * lockfiles, ready to be activated. Only keep one lockfile
@@ -3167,7 +3580,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
 
-		ret = lock_ref_for_update(update, transaction,
+		ret = lock_ref_for_update(update, transaction, head_ref,
 					  &affected_refnames, err);
 		if (ret)
 			goto cleanup;
@@ -3176,23 +3589,35 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	/* Perform updates first so live commits remain referenced */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
+		struct ref_lock *lock = update->lock;
 
 		if (update->flags & REF_NEEDS_COMMIT ||
 		    update->flags & REF_LOG_ONLY) {
-			if (commit_ref_update(update->lock,
-					      update->new_sha1, update->msg,
-					      update->flags, err)) {
-				/* freed by commit_ref_update(): */
-				update->lock = NULL;
-				ret = TRANSACTION_GENERIC_ERROR;
-				goto cleanup;
-			} else {
-				/* freed by commit_ref_update(): */
-				update->lock = NULL;
-			}
-		}
-	}
+			if (log_ref_write(lock->ref_name, lock->old_oid.hash,
+					  update->new_sha1,
+					  update->msg, update->flags, err)) {
+				char *old_msg = strbuf_detach(err, NULL);
 
+				strbuf_addf(err, "cannot update the ref '%s': %s",
+					    lock->ref_name, old_msg);
+				free(old_msg);
+				unlock_ref(lock);
+				update->lock = NULL;
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
+		}
+		if (update->flags & REF_NEEDS_COMMIT) {
+			clear_loose_ref_cache(&ref_cache);
+			if (commit_ref(lock)) {
+				strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
+				unlock_ref(lock);
+				update->lock = NULL;
+				ret = TRANSACTION_GENERIC_ERROR;
+				goto cleanup;
+			}
+		}
+	}
 	/* Perform deletes now that updates are safely completed */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = updates[i];
@@ -3225,7 +3650,9 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	string_list_clear(&refs_to_delete, 0);
+	free(head_ref);
 	string_list_clear(&affected_refnames, 0);
+
 	return ret;
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c46fe67..9839b07 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -50,6 +50,12 @@
 #define REF_LOG_ONLY 0x80
 
 /*
+ * Internal flag, meaning that the containing ref_update was via an
+ * update to HEAD.
+ */
+#define REF_UPDATE_VIA_HEAD 0x100
+
+/*
  * Return true iff refname is minimally safe. "Safe" here means that
  * deleting a loose reference by this name will not do any damage, for
  * example by causing a file that is not a reference to be deleted.
@@ -148,11 +154,12 @@ struct ref_update {
 	unsigned char old_sha1[20];
 	/*
 	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-	 * REF_DELETING, and REF_ISPRUNING:
+	 * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
+	 * REF_UPDATE_VIA_HEAD:
 	 */
 	unsigned int flags;
 	struct ref_lock *lock;
-	int type;
+	unsigned int type;
 	char *msg;
 	const char refname[FLEX_ARRAY];
 };
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 08bd8fd..d226930 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1102,6 +1102,41 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '
 	test_must_fail git rev-parse --verify -q $c
 '
 
+test_expect_success 'fails with duplicate HEAD update' '
+	git branch target1 $A &&
+	git checkout target1 &&
+	cat >stdin <<-EOF &&
+	update refs/heads/target1 $C
+	option no-deref
+	update HEAD $B
+	EOF
+	test_must_fail git update-ref --stdin <stdin 2>err &&
+	grep "fatal: multiple updates for '\''HEAD'\'' (including one via its referent .refs/heads/target1.) are not allowed" err &&
+	echo "refs/heads/target1" >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
+	echo "$A" >expect &&
+	git rev-parse refs/heads/target1 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'fails with duplicate ref update via symref' '
+	git branch target2 $A &&
+	git symbolic-ref refs/heads/symref2 refs/heads/target2 &&
+	cat >stdin <<-EOF &&
+	update refs/heads/target2 $C
+	update refs/heads/symref2 $B
+	EOF
+	test_must_fail git update-ref --stdin <stdin 2>err &&
+	grep "fatal: multiple updates for '\''refs/heads/target2'\'' (including one via symref .refs/heads/symref2.) are not allowed" err &&
+	echo "refs/heads/target2" >expect &&
+	git symbolic-ref refs/heads/symref2 >actual &&
+	test_cmp expect actual &&
+	echo "$A" >expect &&
+	git rev-parse refs/heads/target2 >actual &&
+	test_cmp expect actual
+'
+
 run_with_limited_open_files () {
 	(ulimit -n 32 && "$@")
 }
-- 
2.8.1

Re: [PATCH 25/29] refs: resolve symbolic refs first

[David Turner]

On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
+retry:
...
> +		if (--attempts_remaining > 0)
> +			goto retry;

could this be a loop instead of using gotos?

> +			/*
> +			 * There is a directory in the way,
> +			 * but we	 don't know of any references
> +			 * that it should contain. This might
> +			 * be a directory that used to contain
> +			 * references but is now empty. Try to
> +			 * remove it; otherwise it might cause
> +			 * trouble when we try to rename the
> +			 * lockfile into place.
> +			 */
> +			strbuf_addf(err, "there is a non-empty directory '%s' "
> +				          "blocking reference '%s'",
> +					  ref_file.buf,refname);
> +			goto error_return;

We don't actually try to remove anything here, so that comment seems
wrong?

Re: [PATCH 25/29] refs: resolve symbolic refs first

[Michael Haggerty]

On 04/29/2016 01:40 AM, David Turner wrote:
> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> +retry:
> ...
>> +		if (--attempts_remaining > 0)
>> +			goto retry;
> 
> could this be a loop instead of using gotos?

It certainly could. The goto-vs-loop question was debated on the mailing
list when I first added very similar code elsewhere (unfortunately I'm
unable to find a link to that conversation). I was persuaded to change
my loop into gotos, the argument being that the "retry" case is
exceptional and shouldn't be such a dominant part of the function
structure. Plus the goto code is briefer and feels less awkward to me in
this case (that's subjective, of course).

>> +			/*
>> +			 * There is a directory in the way,
>> +			 * but we	 don't know of any references
>> +			 * that it should contain. This might
>> +			 * be a directory that used to contain
>> +			 * references but is now empty. Try to
>> +			 * remove it; otherwise it might cause
>> +			 * trouble when we try to rename the
>> +			 * lockfile into place.
>> +			 */
>> +			strbuf_addf(err, "there is a non-empty directory '%s' "
>> +				          "blocking reference '%s'",
>> +					  ref_file.buf,refname);
>> +			goto error_return;
> 
> We don't actually try to remove anything here, so that comment seems
> wrong?

Thanks, will fix.

Michael

Re: [PATCH 25/29] refs: resolve symbolic refs first

[David Turner]

On Fri, 2016-04-29 at 11:51 +0200, Michael Haggerty wrote:
> On 04/29/2016 01:40 AM, David Turner wrote:
> > On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> > +retry:
> > ...
> > > +		if (--attempts_remaining > 0)
> > > +			goto retry;
> > 
> > could this be a loop instead of using gotos?
> 
> It certainly could. The goto-vs-loop question was debated on the
> mailing
> list when I first added very similar code elsewhere (unfortunately
> I'm
> unable to find a link to that conversation). I was persuaded to
> change
> my loop into gotos, the argument being that the "retry" case is
> exceptional and shouldn't be such a dominant part of the function
> structure. Plus the goto code is briefer and feels less awkward to me
> in
> this case (that's subjective, of course).

It's part of the structure anyway; it's just implicit rather than
explicit. I won't block consensus tho.

Re: [PATCH 25/29] refs: resolve symbolic refs first

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> +retry:
> ...
>> +		if (--attempts_remaining > 0)
>> +			goto retry;
>
> could this be a loop instead of using gotos?

It could be, but given that there are many such exit points, I do
not think an added level of indentation with while() loop is
particularly a good way to go.

Unless that big body of code that is looped around by implicit "goto
retry" is made into a helper function to perform one round of attempt,
which may result in one of success, temporary failure, final failure.
Then we could do:

	int attempts_remaining = N;
        int failed_hard = 0;
        while (!failed_hard && attempts_remaining--)
        	failed_hard = attempt();
	
and the end result _may_ become easier to read (even though I have
to think for a second what the correct code should look like that
comes after the loop).

[PATCH 26/29] lock_ref_for_update(): don't re-read non-symbolic references

[Michael Haggerty]

Before the previous patch, our first read of the reference happened
before the reference was locked, so we couldn't trust its value and had
to read it again. But now that our first read of the reference happens
after acquiring the lock, there is no need to read it a second time. So
move the read_ref_full() call into the (update->type & REF_ISSYMREF)
block.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 52 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d0e932f..66ceb83 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3427,33 +3427,45 @@ static int lock_ref_for_update(struct ref_update *update,
 
 	lock = update->lock;
 
-	if (read_ref_full(update->refname,
-			  mustexist ? RESOLVE_REF_READING : 0,
-			  lock->old_oid.hash, NULL)) {
-		if (update->flags & REF_HAVE_OLD) {
-			strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-				    update->refname);
-			return TRANSACTION_GENERIC_ERROR;
-		} else {
-			hashclr(lock->old_oid.hash);
-		}
-	}
-	if ((update->flags & REF_HAVE_OLD) &&
-	    hashcmp(lock->old_oid.hash, update->old_sha1)) {
-		strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-			    update->refname,
-			    sha1_to_hex(lock->old_oid.hash),
-			    sha1_to_hex(update->old_sha1));
-		return TRANSACTION_GENERIC_ERROR;
-	}
-
 	if (update->type & REF_ISSYMREF) {
+		if (read_ref_full(update->refname,
+				  mustexist ? RESOLVE_REF_READING : 0,
+				  lock->old_oid.hash, NULL)) {
+			if (update->flags & REF_HAVE_OLD) {
+				strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
+					    update->refname);
+				return TRANSACTION_GENERIC_ERROR;
+			} else {
+				hashclr(lock->old_oid.hash);
+			}
+		}
+		if ((update->flags & REF_HAVE_OLD) &&
+		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+				    update->refname,
+				    sha1_to_hex(lock->old_oid.hash),
+				    sha1_to_hex(update->old_sha1));
+			return TRANSACTION_GENERIC_ERROR;
+		}
+
 		if (!(update->flags & REF_NODEREF)) {
 			ret = split_symref_update(update, referent.buf, transaction,
 						  affected_refnames, err);
 			if (ret)
 				return ret;
 		}
+	} else if ((update->flags & REF_HAVE_OLD) &&
+		   hashcmp(lock->old_oid.hash, update->old_sha1)) {
+		if (is_null_sha1(update->old_sha1))
+			strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+				    update->refname);
+		else
+			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+				    update->refname,
+				    sha1_to_hex(lock->old_oid.hash),
+				    sha1_to_hex(update->old_sha1));
+
+		return TRANSACTION_GENERIC_ERROR;
 	}
 
 	if ((update->flags & REF_HAVE_NEW) &&
-- 
2.8.1

[PATCH 27/29] lock_ref_for_update(): don't resolve symrefs

[Michael Haggerty]

If a transaction includes a non-NODEREF update to a symbolic reference,
we don't have to look it up in lock_ref_for_update(). The reference will
be dereferenced anyway when the split-off update is processed.

This change requires that we store a backpointer from the split-off
update to its parent update, for two reasons:

* We still want to report the original reference name in error messages.
  So if an error occurs when checking the split-off update's old_sha1,
  walk the parent_update pointers back to find the original reference
  name, and report that one.

* We still need to write the old_sha1 of the symref to its reflog. So
  after we read the split-off update's reference value, walk the
  parent_update pointers back and fill in their old_sha1 fields.

Aside from eliminating unnecessary reads, this change fixes a
subtle (though not very serious) race condition: in the old code, the
old_sha1 of the symref was resolved before the reference that it pointed
at was locked. So it was possible that the old_sha1 value logged to the
symref's reflog could be wrong if another process changed the downstream
reference before it was locked.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
There is one remaining race that I know of: the value of HEAD is read
at the start of the transaction to determine whether its referent is
changed in the transaction. If so, the update is also logged in HEAD's
reflog.

The problem is that HEAD is read without acquiring its lock. So in
principle HEAD could change during the time that the transaction is
being processed, leading to inconsistencies in the reflogs.

This problem could be fixed, but it would require HEAD to be locked
for every reference transaction, probably in some kind of an
additional fake ref_update. I don't know that it's worth the effort,
but if it is it can be done within the same framework that I
implemented here.

 refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++--------------
 refs/refs-internal.h |  17 ++++++++
 2 files changed, 95 insertions(+), 30 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 66ceb83..40ed157 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3366,8 +3366,15 @@ static int split_symref_update(struct ref_update *update,
 			update->new_sha1, update->old_sha1,
 			update->msg);
 
-	/* Change the symbolic ref update to log only: */
+	new_update->parent_update = update;
+
+	/*
+	 * Change the symbolic ref update to log only. Also, it
+	 * doesn't need to check its old SHA-1 value, as that will be
+	 * done when new_update is processed.
+	 */
 	update->flags |= REF_LOG_ONLY | REF_NODEREF;
+	update->flags &= ~REF_HAVE_OLD;
 
 	item->util = new_update;
 
@@ -3375,6 +3382,17 @@ static int split_symref_update(struct ref_update *update,
 }
 
 /*
+ * Return the refname under which update was originally requested.
+ */
+static const char *original_update_refname(struct ref_update *update)
+{
+	while (update->parent_update)
+		update = update->parent_update;
+
+	return update->refname;
+}
+
+/*
  * Prepare for carrying out update:
  * - Lock the reference referred to by update.
  * - Read the reference under lock.
@@ -3428,44 +3446,74 @@ static int lock_ref_for_update(struct ref_update *update,
 	lock = update->lock;
 
 	if (update->type & REF_ISSYMREF) {
-		if (read_ref_full(update->refname,
-				  mustexist ? RESOLVE_REF_READING : 0,
-				  lock->old_oid.hash, NULL)) {
-			if (update->flags & REF_HAVE_OLD) {
-				strbuf_addf(err, "cannot lock ref '%s': can't resolve old value",
-					    update->refname);
+		if (update->flags & REF_NODEREF) {
+			/*
+			 * We won't be reading the referent as part of
+			 * the transaction, so we have to read it here
+			 * to record and possibly check old_sha1:
+			 */
+			if (read_ref_full(update->refname,
+					  mustexist ? RESOLVE_REF_READING : 0,
+					  lock->old_oid.hash, NULL)) {
+				if (update->flags & REF_HAVE_OLD) {
+					strbuf_addf(err, "cannot lock ref '%s': "
+						    "can't resolve old value",
+						    update->refname);
+					return TRANSACTION_GENERIC_ERROR;
+				} else {
+					hashclr(lock->old_oid.hash);
+				}
+			}
+			if ((update->flags & REF_HAVE_OLD) &&
+			    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+				strbuf_addf(err, "cannot lock ref '%s': "
+					    "is at %s but expected %s",
+					    update->refname,
+					    sha1_to_hex(lock->old_oid.hash),
+					    sha1_to_hex(update->old_sha1));
 				return TRANSACTION_GENERIC_ERROR;
-			} else {
-				hashclr(lock->old_oid.hash);
 			}
-		}
-		if ((update->flags & REF_HAVE_OLD) &&
-		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
-			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-				    update->refname,
-				    sha1_to_hex(lock->old_oid.hash),
-				    sha1_to_hex(update->old_sha1));
-			return TRANSACTION_GENERIC_ERROR;
-		}
 
-		if (!(update->flags & REF_NODEREF)) {
+		} else {
+			/*
+			 * Create a new update for the reference this
+			 * symref is pointing at. Also, we will record
+			 * and verify old_sha1 for this update as part
+			 * of processing the split-off update, so we
+			 * don't have to do it here.
+			 */
 			ret = split_symref_update(update, referent.buf, transaction,
 						  affected_refnames, err);
 			if (ret)
 				return ret;
 		}
-	} else if ((update->flags & REF_HAVE_OLD) &&
-		   hashcmp(lock->old_oid.hash, update->old_sha1)) {
-		if (is_null_sha1(update->old_sha1))
-			strbuf_addf(err, "cannot lock ref '%s': reference already exists",
-				    update->refname);
-		else
-			strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
-				    update->refname,
-				    sha1_to_hex(lock->old_oid.hash),
-				    sha1_to_hex(update->old_sha1));
+	} else {
+		struct ref_update *parent_update;
 
-		return TRANSACTION_GENERIC_ERROR;
+		/*
+		 * If this update is happening indirectly because of a
+		 * symref update, record the old SHA-1 in the parent
+		 * update:
+		 */
+		for (parent_update = update->parent_update;
+		     parent_update;
+		     parent_update = parent_update->parent_update) {
+			oidcpy(&parent_update->lock->old_oid, &lock->old_oid);
+		}
+
+		if ((update->flags & REF_HAVE_OLD) &&
+		    hashcmp(lock->old_oid.hash, update->old_sha1)) {
+			if (is_null_sha1(update->old_sha1))
+				strbuf_addf(err, "cannot lock ref '%s': reference already exists",
+					    original_update_refname(update));
+			else
+				strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s",
+					    original_update_refname(update),
+					    sha1_to_hex(lock->old_oid.hash),
+					    sha1_to_hex(update->old_sha1));
+
+			return TRANSACTION_GENERIC_ERROR;
+		}
 	}
 
 	if ((update->flags & REF_HAVE_NEW) &&
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 9839b07..2355735 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);
  * not exist before update.
  */
 struct ref_update {
+
 	/*
 	 * If (flags & REF_HAVE_NEW), set the reference to this value:
 	 */
 	unsigned char new_sha1[20];
+
 	/*
 	 * If (flags & REF_HAVE_OLD), check that the reference
 	 * previously had this value:
 	 */
 	unsigned char old_sha1[20];
+
 	/*
 	 * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
 	 * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
 	 * REF_UPDATE_VIA_HEAD:
 	 */
 	unsigned int flags;
+
 	struct ref_lock *lock;
 	unsigned int type;
 	char *msg;
+
+	/*
+	 * If this ref_update was split off of a symref update via
+	 * split_symref_update(), then this member points at that
+	 * update. This is used for two purposes:
+	 * 1. When reporting errors, we report the refname under which
+	 *    the update was originally requested.
+	 * 2. When we read the old value of this reference, we
+	 *    propagate it back to its parent update for recording in
+	 *    the latter's reflog.
+	 */
+	struct ref_update *parent_update;
+
 	const char refname[FLEX_ARRAY];
 };
 
-- 
2.8.1

[PATCH 28/29] commit_ref_update(): remove the flags parameter

[Michael Haggerty]

commit_ref_update() is now only called with flags=0. So remove the flags
parameter entirely.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 40ed157..938871b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2568,7 +2568,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,
-			     int flags, struct strbuf *err);
+			     struct strbuf *err);
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
@@ -2636,7 +2636,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, 0, &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;
@@ -2656,7 +2656,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, 0, &err)) {
+	    commit_ref_update(lock, orig_sha1, NULL, &err)) {
 		error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
 		strbuf_release(&err);
 	}
@@ -2870,12 +2870,12 @@ 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,
-			     int flags, struct strbuf *err)
+			     struct strbuf *err)
 {
 	clear_loose_ref_cache(&ref_cache);
-	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0 ||
+	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
-	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {
+	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {
 		char *old_msg = strbuf_detach(err, NULL);
 		strbuf_addf(err, "cannot update the ref '%s': %s",
 			    lock->ref_name, old_msg);
@@ -2911,7 +2911,7 @@ static int commit_ref_update(struct ref_lock *lock,
 			}
 		}
 	}
-	if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
+	if (commit_ref(lock)) {
 		strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
 		unlock_ref(lock);
 		return -1;
-- 
2.8.1

[PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode

[Michael Haggerty]

Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we
don't have to handle other cases anymore.

This enables several simplifications, the most interesting of which come
from the fact that ref_lock::orig_ref_name is now always the same as
ref_lock::ref_name:

* Remove ref_lock::orig_ref_name
* Remove local variable orig_refname from lock_ref_sha1_basic()
* commit_ref_update() never has to write to the reflog for
  lock->orig_ref_name

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 938871b..5dca352 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -7,7 +7,6 @@
 
 struct ref_lock {
 	char *ref_name;
-	char *orig_ref_name;
 	struct lock_file *lk;
 	struct object_id old_oid;
 };
@@ -1551,7 +1550,6 @@ static void unlock_ref(struct ref_lock *lock)
 	if (lock->lk)
 		rollback_lock_file(lock->lk);
 	free(lock->ref_name);
-	free(lock->orig_ref_name);
 	free(lock);
 }
 
@@ -1605,7 +1603,6 @@ static int lock_raw_ref(const char *refname, int deleting, int mustexist,
 	*lock_p = lock = xcalloc(1, sizeof(*lock));
 
 	lock->ref_name = xstrdup(refname);
-	lock->orig_ref_name = xstrdup(refname);
 	strbuf_git_path(&ref_file, "%s", refname);
 
 retry:
@@ -1991,14 +1988,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    struct strbuf *err)
 {
 	struct strbuf ref_file = STRBUF_INIT;
-	struct strbuf orig_ref_file = STRBUF_INIT;
-	const char *orig_refname = refname;
 	struct ref_lock *lock;
 	int last_errno = 0;
-	int lflags = 0;
+	int lflags = LOCK_NO_DEREF;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
-	int resolve_flags = 0;
+	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int attempts_remaining = 3;
+	int resolved;
 
 	assert(err);
 
@@ -2008,46 +2004,39 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		resolve_flags |= RESOLVE_REF_READING;
 	if (flags & REF_DELETING)
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
-	if (flags & REF_NODEREF) {
-		resolve_flags |= RESOLVE_REF_NO_RECURSE;
-		lflags |= LOCK_NO_DEREF;
-	}
 
-	refname = resolve_ref_unsafe(refname, resolve_flags,
-				     lock->old_oid.hash, type);
-	if (!refname && errno == EISDIR) {
+	resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+					lock->old_oid.hash, type);
+	if (!resolved && errno == EISDIR) {
 		/*
 		 * we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
 		 * it is normal for the empty directory 'foo'
 		 * to remain.
 		 */
-		strbuf_git_path(&orig_ref_file, "%s", orig_refname);
-		if (remove_empty_directories(&orig_ref_file)) {
+		strbuf_git_path(&ref_file, "%s", refname);
+		if (remove_empty_directories(&ref_file)) {
 			last_errno = errno;
-			if (!verify_refname_available_dir(orig_refname, extras, skip,
+			if (!verify_refname_available_dir(refname, extras, skip,
 							  get_loose_refs(&ref_cache), err))
 				strbuf_addf(err, "there are still refs under '%s'",
-					    orig_refname);
+					    refname);
 			goto error_return;
 		}
-		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
-					     lock->old_oid.hash, type);
+		resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+						lock->old_oid.hash, type);
 	}
-	if (!refname) {
+	if (!resolved) {
 		last_errno = errno;
 		if (last_errno != ENOTDIR ||
-		    !verify_refname_available_dir(orig_refname, extras, skip,
+		    !verify_refname_available_dir(refname, extras, skip,
 						  get_loose_refs(&ref_cache), err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    orig_refname, strerror(last_errno));
+				    refname, strerror(last_errno));
 
 		goto error_return;
 	}
 
-	if (flags & REF_NODEREF)
-		refname = orig_refname;
-
 	/*
 	 * If the ref did not exist and we are creating it, make sure
 	 * there is no existing packed ref whose name begins with our
@@ -2064,7 +2053,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
 
 	lock->ref_name = xstrdup(refname);
-	lock->orig_ref_name = xstrdup(orig_refname);
+	strbuf_reset(&ref_file);
 	strbuf_git_path(&ref_file, "%s", refname);
 
  retry:
@@ -2108,7 +2097,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 
  out:
 	strbuf_release(&ref_file);
-	strbuf_release(&orig_ref_file);
 	errno = last_errno;
 	return lock;
 }
@@ -2873,9 +2861,7 @@ static int commit_ref_update(struct ref_lock *lock,
 			     struct strbuf *err)
 {
 	clear_loose_ref_cache(&ref_cache);
-	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||
-	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
-	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {
+	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
 		char *old_msg = strbuf_detach(err, NULL);
 		strbuf_addf(err, "cannot update the ref '%s': %s",
 			    lock->ref_name, old_msg);
@@ -2883,7 +2869,8 @@ static int commit_ref_update(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
+
+	if (strcmp(lock->ref_name, "HEAD") != 0) {
 		/*
 		 * Special hack: If a branch is updated directly and HEAD
 		 * points to it (may happen on the remote side of a push
@@ -2899,6 +2886,7 @@ static int commit_ref_update(struct ref_lock *lock,
 		unsigned char head_sha1[20];
 		int head_flag;
 		const char *head_ref;
+
 		head_ref = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
 					      head_sha1, &head_flag);
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
@@ -2911,6 +2899,7 @@ static int commit_ref_update(struct ref_lock *lock,
 			}
 		}
 	}
+
 	if (commit_ref(lock)) {
 		strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
 		unlock_ref(lock);
@@ -3016,7 +3005,6 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lk = &head_lock;
 	lock->ref_name = xstrdup(head_rel);
-	lock->orig_ref_name = xstrdup(head_rel);
 
 	ret = create_symref_locked(lock, head_rel, target, NULL);
 
-- 
2.8.1

Re: [PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode

[Junio C Hamano]

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

> Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we
> don't have to handle other cases anymore.
>
> This enables several simplifications, the most interesting of which come
> from the fact that ref_lock::orig_ref_name is now always the same as
> ref_lock::ref_name:
>
> * Remove ref_lock::orig_ref_name
> * Remove local variable orig_refname from lock_ref_sha1_basic()
> * commit_ref_update() never has to write to the reflog for
>   lock->orig_ref_name
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
>  1 file changed, 21 insertions(+), 33 deletions(-)

Finished reading the whole thing and it made quite a lot of sense.

Thanks.

Re: [PATCH 00/29] Yet more preparation for reference backends

[David Turner]

On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> This started as a modest attempt to move the last couple of patches
> mentioned here [1] to before the vtable patches. I wanted to do that
> because having real changes mixed with the vtable refactoring was
> making rebasing and stuff awkward.
> 
> But then it snowballed. A lot of what's new is pretty trivial (though
> in some cases fixes minor bugs). But a few of the later patches are
> pretty major.

Apart from the comments on one patch that I sent, this looks good to
me.  I'm a bit worried about the impact on lmdb (which doesn't do
"locking" the same way), but I think it will probably work out.  Since
Michael has been rebasing the pluggable-refs-backend code on top of
this, he's probably got a good sense that it doesn't do too much
violence to the plugin infrastructure.



>