[PATCH 00/24] Yet another pre-refs-backend series

[PATCH 00/24] Yet another pre-refs-backend series

[David Turner]

We now have quite a large number of patches before we even get into
the meat of the pluggable refs backend series.  So it's worth breaking
those out and getting them in before we get into the main series
(which Michael Haggerty swants to redesign a bit anyway).

This set of patches should be applied on top of
jk/check-repository-format.

Michael Haggerty has reviewed those of my patches which are in here
except maybe:
  refs: on symref reflog expire, lock symref not referrent
This was the one from later in the series that was straightforward to
move to before the vtable; the other two were going to be harder to
move and can wait until after the vtable.

I have reviewed all of Michael's patches.


David Turner (5):
  refs: move head_ref{,_submodule} to the common code
  refs: move for_each_*ref* functions into common code
  files-backend: break out ref reading
  refs: move resolve_ref_unsafe into common code
  refs: on symref reflog expire, lock symref not referrent

Michael Haggerty (19):
  t1430: test the output and error of some commands more carefully
  t1430: clean up broken refs/tags/shadow
  t1430: don't rely on symbolic-ref for creating broken symrefs
  t1430: test for-each-ref in the presence of badly-named refs
  t1430: improve test coverage of deletion of badly-named refs
  resolve_missing_loose_ref(): simplify semantics
  resolve_ref_unsafe(): use for loop to count up to MAXDEPTH
  resolve_ref_unsafe(): ensure flags is always set
  resolve_ref_1(): eliminate local variable
  resolve_ref_1(): reorder code
  resolve_ref_1(): eliminate local variable "bad_name"
  read_raw_ref(): manage own scratch space
  Inline resolve_ref_1() into resolve_ref_unsafe()
  read_raw_ref(): change flags parameter to unsigned int
  fsck_head_link(): remove unneeded flag variable
  cmd_merge(): remove unneeded flag variable
  checkout_paths(): remove unneeded flag variable
  check_aliased_update(): check that dst_name is non-NULL
  show_head_ref(): check the result of resolve_ref_namespace()

 builtin/checkout.c      |   3 +-
 builtin/fsck.c          |   3 +-
 builtin/merge.c         |   4 +-
 builtin/receive-pack.c  |   2 +-
 http-backend.c          |   4 +-
 refs.c                  | 149 ++++++++++++++++++
 refs/files-backend.c    | 406 ++++++++++++++++--------------------------------
 refs/refs-internal.h    |  15 ++
 t/t1410-reflog.sh       |  10 ++
 t/t1430-bad-ref-name.sh | 132 ++++++++++++++--
 10 files changed, 440 insertions(+), 288 deletions(-)

-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 01/24] refs: move head_ref{,_submodule} to the common code

[David Turner]

These don't use any backend-specific functions.  These were previously
defined in terms of the do_head_ref helper function, but since they
are otherwise identical, we don't need that function.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c               | 23 +++++++++++++++++++++++
 refs/files-backend.c | 28 ----------------------------
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/refs.c b/refs.c
index b0e6ece..6b8c16c 100644
--- a/refs.c
+++ b/refs.c
@@ -1080,3 +1080,26 @@ int rename_ref_available(const char *oldname, const char *newname)
 	strbuf_release(&err);
 	return ret;
 }
+
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+	struct object_id oid;
+	int flag;
+
+	if (submodule) {
+		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
+			return fn("HEAD", &oid, 0, cb_data);
+
+		return 0;
+	}
+
+	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
+		return fn("HEAD", &oid, flag, cb_data);
+
+	return 0;
+}
+
+int head_ref(each_ref_fn fn, void *cb_data)
+{
+	return head_ref_submodule(NULL, fn, cb_data);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81f68f8..c07dc41 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1745,34 +1745,6 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
-static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	struct object_id oid;
-	int flag;
-
-	if (submodule) {
-		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
-			return fn("HEAD", &oid, 0, cb_data);
-
-		return 0;
-	}
-
-	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
-		return fn("HEAD", &oid, flag, cb_data);
-
-	return 0;
-}
-
-int head_ref(each_ref_fn fn, void *cb_data)
-{
-	return do_head_ref(NULL, fn, cb_data);
-}
-
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return do_head_ref(submodule, fn, cb_data);
-}
-
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_ref(&ref_cache, "", fn, 0, 0, cb_data);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 02/24] refs: move for_each_*ref* functions into common code

[David Turner]

Make do_for_each_ref take a submodule as an argument instead of a
ref_cache.  Since all for_each_*ref* functions are defined in terms of
do_for_each_ref, we can then move them into the common code.

Later, we can simply make do_for_each_ref into a backend function.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c               | 52 +++++++++++++++++++++++++++++++++++++++++++
 refs/files-backend.c | 62 +++++-----------------------------------------------
 refs/refs-internal.h |  9 ++++++++
 3 files changed, 66 insertions(+), 57 deletions(-)

diff --git a/refs.c b/refs.c
index 6b8c16c..f0feff7 100644
--- a/refs.c
+++ b/refs.c
@@ -1103,3 +1103,55 @@ int head_ref(each_ref_fn fn, void *cb_data)
 {
 	return head_ref_submodule(NULL, fn, cb_data);
 }
+
+int for_each_ref(each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
+}
+
+int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(submodule, "", fn, 0, 0, cb_data);
+}
+
+int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(NULL, prefix, fn, strlen(prefix), 0, cb_data);
+}
+
+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)
+{
+	unsigned int flag = 0;
+
+	if (broken)
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(NULL, prefix, fn, 0, flag, cb_data);
+}
+
+int for_each_ref_in_submodule(const char *submodule, const char *prefix,
+		each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(submodule, prefix, fn, strlen(prefix), 0, cb_data);
+}
+
+int for_each_replace_ref(each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(NULL, git_replace_ref_base, fn,
+			       strlen(git_replace_ref_base), 0, cb_data);
+}
+
+int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int ret;
+	strbuf_addf(&buf, "%srefs/", get_git_namespace());
+	ret = do_for_each_ref(NULL, buf.buf, fn, 0, 0, cb_data);
+	strbuf_release(&buf);
+	return ret;
+}
+
+int for_each_rawref(each_ref_fn fn, void *cb_data)
+{
+	return do_for_each_ref(NULL, "", fn, 0,
+			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c07dc41..9676ec2 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -513,9 +513,6 @@ static void sort_ref_dir(struct ref_dir *dir)
 	dir->sorted = dir->nr = i;
 }
 
-/* Include broken references in a do_for_each_ref*() iteration: */
-#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
-
 /*
  * Return true iff the reference described by entry can be resolved to
  * an object in the database.  Emit a warning if the referred-to
@@ -1727,10 +1724,13 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,
  * value, stop the iteration and return that value; otherwise, return
  * 0.
  */
-static int do_for_each_ref(struct ref_cache *refs, const char *base,
-			   each_ref_fn fn, int trim, int flags, void *cb_data)
+int do_for_each_ref(const char *submodule, const char *base,
+		    each_ref_fn fn, int trim, int flags, void *cb_data)
 {
 	struct ref_entry_cb data;
+	struct ref_cache *refs;
+
+	refs = get_ref_cache(submodule);
 	data.base = base;
 	data.trim = trim;
 	data.flags = flags;
@@ -1745,58 +1745,6 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 
-int for_each_ref(each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(&ref_cache, "", fn, 0, 0, cb_data);
-}
-
-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(get_ref_cache(submodule), "", fn, 0, 0, cb_data);
-}
-
-int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);
-}
-
-int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)
-{
-	unsigned int flag = 0;
-
-	if (broken)
-		flag = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(&ref_cache, prefix, fn, 0, flag, cb_data);
-}
-
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-		each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(get_ref_cache(submodule), prefix, fn, strlen(prefix), 0, cb_data);
-}
-
-int for_each_replace_ref(each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(&ref_cache, git_replace_ref_base, fn,
-			       strlen(git_replace_ref_base), 0, cb_data);
-}
-
-int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
-{
-	struct strbuf buf = STRBUF_INIT;
-	int ret;
-	strbuf_addf(&buf, "%srefs/", get_git_namespace());
-	ret = do_for_each_ref(&ref_cache, buf.buf, fn, 0, 0, cb_data);
-	strbuf_release(&buf);
-	return ret;
-}
-
-int for_each_rawref(each_ref_fn fn, void *cb_data)
-{
-	return do_for_each_ref(&ref_cache, "", fn, 0,
-			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
-}
-
 static void unlock_ref(struct ref_lock *lock)
 {
 	/* Do not free lock->lk -- atexit() still looks at them */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c7dded3..92aae80 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -197,4 +197,13 @@ const char *find_descendant_ref(const char *dirname,
 
 int rename_ref_available(const char *oldname, const char *newname);
 
+
+/* Include broken references in a do_for_each_ref*() iteration: */
+#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
+
+/*
+ * The common backend for the for_each_*ref* functions
+ */
+int do_for_each_ref(const char *submodule, const char *base,
+		    each_ref_fn fn, int trim, int flags, void *cb_data);
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 03/24] t1430: test the output and error of some commands more carefully

[David Turner]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index c465abe..005e2b1 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -42,7 +42,7 @@ test_expect_success 'git branch shows badly named ref as warning' '
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
 	git branch >output 2>error &&
-	grep -e "broken\.\.\.ref" error &&
+	test_i18ngrep -e "ignoring ref with broken name refs/heads/broken\.\.\.ref" error &&
 	! grep -e "broken\.\.\.ref" output
 '
 
@@ -152,21 +152,25 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '
 	git rev-parse --verify one >expect &&
 	git rev-parse --verify shadow >actual 2>err &&
 	test_cmp expect actual &&
-	test_i18ngrep "ignoring.*refs/tags/shadow" err
+	test_i18ngrep "ignoring dangling symref refs/tags/shadow" err
 '
 
 test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '
 	git symbolic-ref refs/heads/badname refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	test_path_is_file .git/refs/heads/badname &&
-	git update-ref --no-deref -d refs/heads/badname &&
-	test_path_is_missing .git/refs/heads/badname
+	git update-ref --no-deref -d refs/heads/badname >output 2>error &&
+	test_path_is_missing .git/refs/heads/badname &&
+	test_must_be_empty output &&
+	test_must_be_empty error
 '
 
 test_expect_success 'update-ref -d can delete broken name' '
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
-	git update-ref -d refs/heads/broken...ref &&
+	git update-ref -d refs/heads/broken...ref >output 2>error &&
+	test_must_be_empty output &&
+	test_must_be_empty error &&
 	git branch >output 2>error &&
 	! grep -e "broken\.\.\.ref" error &&
 	! grep -e "broken\.\.\.ref" output
@@ -175,7 +179,9 @@ test_expect_success 'update-ref -d can delete broken name' '
 test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
 	echo precious >.git/my-private-file &&
 	echo precious >expect &&
-	test_must_fail git update-ref -d my-private-file &&
+	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_cmp expect .git/my-private-file
 '
 
-- 
2.4.2.767.g62658d5-twtrsrc

Re: [PATCH 03/24] t1430: test the output and error of some commands more carefully

[Michael Haggerty]

On 04/07/2016 03:02 PM, David Turner wrote:
> From: Michael Haggerty <mhagger@alum.mit.edu>
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> [...]

David, even though I wrote these patches, I believe you need to sign
them off too since you are the one submitting them to the list (it's a
chain-of-custody sort of thing).

It's possible Junio would be willing to add the signoffs to the patches
if you tell him "I sign off all of the patches in this series" but I'm
not sure.

Michael

Re: [PATCH 03/24] t1430: test the output and error of some commands more carefully

[David Turner]

On Fri, 2016-04-08 at 11:14 -0400, Michael Haggerty wrote:
> On 04/07/2016 03:02 PM, David Turner wrote:
> > From: Michael Haggerty <mhagger@alum.mit.edu>
> > 
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > [...]
> 
> David, even though I wrote these patches, I believe you need to sign
> them off too since you are the one submitting them to the list (it's
> a
> chain-of-custody sort of thing).
> 
> It's possible Junio would be willing to add the signoffs to the
> patches
> if you tell him "I sign off all of the patches in this series" but
> I'm
> not sure.

Junio, I approve signing off on all of these patches.  Would you like
me to re-roll with my sign-off, or would you prefer to add it yourself?

Thanks.

Re: [PATCH 03/24] t1430: test the output and error of some commands more carefully

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> Would you like
> me to re-roll with my sign-off, or would you prefer to add it yourself?

I take it that you see no other reason to reroll the series at this
point (otherwise you wouldn't be asking ;-), so I'll check which
ones need amending and add them.

Thanks.

Re: [PATCH 03/24] t1430: test the output and error of some commands more carefully

[David Turner]

On Fri, 2016-04-08 at 13:43 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > Would you like
> > me to re-roll with my sign-off, or would you prefer to add it
> > yourself?
> 
> I take it that you see no other reason to reroll the series at this
> point (otherwise you wouldn't be asking ;-), so I'll check which
> ones need amending and add them.

That is correct.

[PATCH 04/24] t1430: clean up broken refs/tags/shadow

[David Turner]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 005e2b1..cb815ab 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -148,7 +148,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '
 	git branch shadow one &&
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	git symbolic-ref refs/tags/shadow refs/heads/broken...ref &&
-
+	test_when_finished "rm -f .git/refs/tags/shadow" &&
 	git rev-parse --verify one >expect &&
 	git rev-parse --verify shadow >actual 2>err &&
 	test_cmp expect actual &&
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 05/24] t1430: don't rely on symbolic-ref for creating broken symrefs

[David Turner]

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

It's questionable whether it should even work.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index cb815ab..a963951 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -147,7 +147,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
 	git branch shadow one &&
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
-	git symbolic-ref refs/tags/shadow refs/heads/broken...ref &&
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/tags/shadow &&
 	test_when_finished "rm -f .git/refs/tags/shadow" &&
 	git rev-parse --verify one >expect &&
 	git rev-parse --verify shadow >actual 2>err &&
@@ -156,7 +156,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '
 '
 
 test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '
-	git symbolic-ref refs/heads/badname refs/heads/broken...ref &&
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	test_path_is_file .git/refs/heads/badname &&
 	git update-ref --no-deref -d refs/heads/badname >output 2>error &&
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 06/24] t1430: test for-each-ref in the presence of badly-named refs

[David Turner]

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index a963951..612cc32 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -155,6 +155,22 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '
 	test_i18ngrep "ignoring dangling symref refs/tags/shadow" err
 '
 
+test_expect_success 'for-each-ref emits warnings for broken names' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	printf "ref: refs/heads/master\n" >.git/refs/heads/broken...symref &&
+	test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+	git for-each-ref >output 2>error &&
+	! grep -e "broken\.\.\.ref" output &&
+	! grep -e "badname" output &&
+	! grep -e "broken\.\.\.symref" output &&
+	test_i18ngrep "ignoring ref with broken name refs/heads/broken\.\.\.ref" error &&
+	test_i18ngrep "ignoring broken ref refs/heads/badname" error &&
+	test_i18ngrep "ignoring ref with broken name refs/heads/broken\.\.\.symref" error
+'
+
 test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 07/24] t1430: improve test coverage of deletion of badly-named refs

[David Turner]

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

Check "branch -d broken...ref"

Check various combinations of

* Deleting using "update-ref -d"
* Deleting using "update-ref --no-deref -d"
* Deleting using "branch -d"

in the following combinations of symref -> ref:

* badname -> broken...ref
* badname -> broken...ref (dangling)
* broken...symref -> master
* broken...symref -> idonotexist (dangling)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 104 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 96 insertions(+), 8 deletions(-)

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 612cc32..25ddab4 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -171,25 +171,113 @@ test_expect_success 'for-each-ref emits warnings for broken names' '
 	test_i18ngrep "ignoring ref with broken name refs/heads/broken\.\.\.symref" error
 '
 
-test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '
+test_expect_success 'update-ref -d can delete broken name' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git update-ref -d refs/heads/broken...ref >output 2>error &&
+	test_must_be_empty output &&
+	test_must_be_empty error &&
+	git branch >output 2>error &&
+	! grep -e "broken\.\.\.ref" error &&
+	! grep -e "broken\.\.\.ref" output
+'
+
+test_expect_success 'branch -d can delete broken name' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	git branch -d broken...ref >output 2>error &&
+	test_i18ngrep "Deleted branch broken...ref (was broken)" output &&
+	test_must_be_empty error &&
+	git branch >output 2>error &&
+	! grep -e "broken\.\.\.ref" error &&
+	! grep -e "broken\.\.\.ref" output
+'
+
+test_expect_success 'update-ref --no-deref -d can delete symref to broken name' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
-	test_path_is_file .git/refs/heads/badname &&
 	git update-ref --no-deref -d refs/heads/badname >output 2>error &&
 	test_path_is_missing .git/refs/heads/badname &&
 	test_must_be_empty output &&
 	test_must_be_empty error
 '
 
-test_expect_success 'update-ref -d can delete broken name' '
+test_expect_success 'branch -d can delete symref to broken name' '
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
-	git update-ref -d refs/heads/broken...ref >output 2>error &&
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	git branch -d badname >output 2>error &&
+	test_path_is_missing .git/refs/heads/badname &&
+	test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	git update-ref --no-deref -d refs/heads/badname >output 2>error &&
+	test_path_is_missing .git/refs/heads/badname &&
 	test_must_be_empty output &&
-	test_must_be_empty error &&
-	git branch >output 2>error &&
-	! grep -e "broken\.\.\.ref" error &&
-	! grep -e "broken\.\.\.ref" output
+	test_must_be_empty error
+'
+
+test_expect_success 'branch -d can delete dangling symref to broken name' '
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	git branch -d badname >output 2>error &&
+	test_path_is_missing .git/refs/heads/badname &&
+	test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'update-ref -d can delete broken name through symref' '
+	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
+	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
+	test_when_finished "rm -f .git/refs/heads/badname" &&
+	git update-ref -d refs/heads/badname >output 2>error &&
+	test_path_is_missing .git/refs/heads/broken...ref &&
+	test_must_be_empty output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
+	printf "ref: refs/heads/master\n" >.git/refs/heads/broken...symref &&
+	test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+	git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
+	test_path_is_missing .git/refs/heads/broken...symref &&
+	test_must_be_empty output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'branch -d can delete symref with broken name' '
+	printf "ref: refs/heads/master\n" >.git/refs/heads/broken...symref &&
+	test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+	git branch -d broken...symref >output 2>error &&
+	test_path_is_missing .git/refs/heads/broken...symref &&
+	test_i18ngrep "Deleted branch broken...symref (was refs/heads/master)" output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
+	printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
+	test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+	git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
+	test_path_is_missing .git/refs/heads/broken...symref &&
+	test_must_be_empty output &&
+	test_must_be_empty error
+'
+
+test_expect_success 'branch -d can delete dangling symref with broken name' '
+	printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
+	test_when_finished "rm -f .git/refs/heads/broken...symref" &&
+	git branch -d broken...symref >output 2>error &&
+	test_path_is_missing .git/refs/heads/broken...symref &&
+	test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
+	test_must_be_empty error
 '
 
 test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 08/24] resolve_missing_loose_ref(): simplify semantics

[David Turner]

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

Make resolve_missing_loose_ref() only responsible for looking up a
packed reference, without worrying about whether we want to read or
write the reference and without setting errno on failure. Move the other
logic to the caller.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9676ec2..c0cf6fd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1368,11 +1368,9 @@ static struct ref_entry *get_packed_ref(const char *refname)
 }
 
 /*
- * A loose ref file doesn't exist; check for a packed ref.  The
- * options are forwarded from resolve_safe_unsafe().
+ * A loose ref file doesn't exist; check for a packed ref.
  */
 static int resolve_missing_loose_ref(const char *refname,
-				     int resolve_flags,
 				     unsigned char *sha1,
 				     int *flags)
 {
@@ -1389,14 +1387,8 @@ static int resolve_missing_loose_ref(const char *refname,
 			*flags |= REF_ISPACKED;
 		return 0;
 	}
-	/* The reference is not a packed reference, either. */
-	if (resolve_flags & RESOLVE_REF_READING) {
-		errno = ENOENT;
-		return -1;
-	} else {
-		hashclr(sha1);
-		return 0;
-	}
+	/* refname is not a packed reference. */
+	return -1;
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1461,9 +1453,13 @@ static const char *resolve_ref_1(const char *refname,
 		if (lstat(path, &st) < 0) {
 			if (errno != ENOENT)
 				return NULL;
-			if (resolve_missing_loose_ref(refname, resolve_flags,
-						      sha1, flags))
-				return NULL;
+			if (resolve_missing_loose_ref(refname, sha1, flags)) {
+				if (resolve_flags & RESOLVE_REF_READING) {
+					errno = ENOENT;
+					return NULL;
+				}
+				hashclr(sha1);
+			}
 			if (bad_name) {
 				hashclr(sha1);
 				if (flags)
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 09/24] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH

[David Turner]

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

The loop's there anyway; we might as well use it.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c0cf6fd..101abba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1400,8 +1400,8 @@ static const char *resolve_ref_1(const char *refname,
 				 struct strbuf *sb_path,
 				 struct strbuf *sb_contents)
 {
-	int depth = MAXDEPTH;
 	int bad_name = 0;
+	int symref_count;
 
 	if (flags)
 		*flags = 0;
@@ -1425,17 +1425,13 @@ static const char *resolve_ref_1(const char *refname,
 		 */
 		bad_name = 1;
 	}
-	for (;;) {
+
+	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
 		const char *path;
 		struct stat st;
 		char *buf;
 		int fd;
 
-		if (--depth < 0) {
-			errno = ELOOP;
-			return NULL;
-		}
-
 		strbuf_reset(sb_path);
 		strbuf_git_path(sb_path, "%s", refname);
 		path = sb_path->buf;
@@ -1566,6 +1562,9 @@ static const char *resolve_ref_1(const char *refname,
 			bad_name = 1;
 		}
 	}
+
+	errno = ELOOP;
+	return NULL;
 }
 
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 10/24] resolve_ref_unsafe(): ensure flags is always set

[David Turner]

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

If the caller passes flags==NULL, then set it to point at a local
scratch variable. This removes the need for a lot of "if (flags)" guards
in resolve_ref_1() and resolve_missing_loose_ref().

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 101abba..067ce1c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1383,8 +1383,7 @@ static int resolve_missing_loose_ref(const char *refname,
 	entry = get_packed_ref(refname);
 	if (entry) {
 		hashcpy(sha1, entry->u.value.oid.hash);
-		if (flags)
-			*flags |= REF_ISPACKED;
+		*flags |= REF_ISPACKED;
 		return 0;
 	}
 	/* refname is not a packed reference. */
@@ -1403,12 +1402,10 @@ static const char *resolve_ref_1(const char *refname,
 	int bad_name = 0;
 	int symref_count;
 
-	if (flags)
-		*flags = 0;
+	*flags = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		if (flags)
-			*flags |= REF_BAD_NAME;
+		*flags |= REF_BAD_NAME;
 
 		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 		    !refname_is_safe(refname)) {
@@ -1458,8 +1455,7 @@ static const char *resolve_ref_1(const char *refname,
 			}
 			if (bad_name) {
 				hashclr(sha1);
-				if (flags)
-					*flags |= REF_ISBROKEN;
+				*flags |= REF_ISBROKEN;
 			}
 			return refname;
 		}
@@ -1478,8 +1474,7 @@ static const char *resolve_ref_1(const char *refname,
 			    !check_refname_format(sb_contents->buf, 0)) {
 				strbuf_swap(sb_refname, sb_contents);
 				refname = sb_refname->buf;
-				if (flags)
-					*flags |= REF_ISSYMREF;
+				*flags |= REF_ISSYMREF;
 				if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
 					hashclr(sha1);
 					return refname;
@@ -1526,20 +1521,17 @@ static const char *resolve_ref_1(const char *refname,
 			 */
 			if (get_sha1_hex(sb_contents->buf, sha1) ||
 			    (sb_contents->buf[40] != '\0' && !isspace(sb_contents->buf[40]))) {
-				if (flags)
-					*flags |= REF_ISBROKEN;
+				*flags |= REF_ISBROKEN;
 				errno = EINVAL;
 				return NULL;
 			}
 			if (bad_name) {
 				hashclr(sha1);
-				if (flags)
-					*flags |= REF_ISBROKEN;
+				*flags |= REF_ISBROKEN;
 			}
 			return refname;
 		}
-		if (flags)
-			*flags |= REF_ISSYMREF;
+		*flags |= REF_ISSYMREF;
 		buf = sb_contents->buf + 4;
 		while (isspace(*buf))
 			buf++;
@@ -1551,8 +1543,7 @@ static const char *resolve_ref_1(const char *refname,
 			return refname;
 		}
 		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-			if (flags)
-				*flags |= REF_ISBROKEN;
+			*flags |= REF_ISBROKEN;
 
 			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 			    !refname_is_safe(buf)) {
@@ -1573,8 +1564,12 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 	static struct strbuf sb_refname = STRBUF_INIT;
 	struct strbuf sb_contents = STRBUF_INIT;
 	struct strbuf sb_path = STRBUF_INIT;
+	int unused_flags;
 	const char *ret;
 
+	if (!flags)
+		flags = &unused_flags;
+
 	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
 			    &sb_refname, &sb_path, &sb_contents);
 	strbuf_release(&sb_path);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 11/24] resolve_ref_1(): eliminate local variable

[David Turner]

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

In place of `buf`, use `refname`, which is anyway a better description
of what is being pointed at.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 067ce1c..69ec903 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1426,7 +1426,6 @@ static const char *resolve_ref_1(const char *refname,
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
 		const char *path;
 		struct stat st;
-		char *buf;
 		int fd;
 
 		strbuf_reset(sb_path);
@@ -1532,21 +1531,21 @@ static const char *resolve_ref_1(const char *refname,
 			return refname;
 		}
 		*flags |= REF_ISSYMREF;
-		buf = sb_contents->buf + 4;
-		while (isspace(*buf))
-			buf++;
+		refname = sb_contents->buf + 4;
+		while (isspace(*refname))
+			refname++;
 		strbuf_reset(sb_refname);
-		strbuf_addstr(sb_refname, buf);
+		strbuf_addstr(sb_refname, refname);
 		refname = sb_refname->buf;
 		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
 			hashclr(sha1);
 			return refname;
 		}
-		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 			*flags |= REF_ISBROKEN;
 
 			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-			    !refname_is_safe(buf)) {
+			    !refname_is_safe(refname)) {
 				errno = EINVAL;
 				return NULL;
 			}
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 12/24] resolve_ref_1(): reorder code

[David Turner]

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

There is no need to adjust *flags if we're just about to fail.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 69ec903..60f1493 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1542,13 +1542,13 @@ static const char *resolve_ref_1(const char *refname,
 			return refname;
 		}
 		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-			*flags |= REF_ISBROKEN;
-
 			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 			    !refname_is_safe(refname)) {
 				errno = EINVAL;
 				return NULL;
 			}
+
+			*flags |= REF_ISBROKEN;
 			bad_name = 1;
 		}
 	}
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 13/24] resolve_ref_1(): eliminate local variable "bad_name"

[David Turner]

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

We can use (*flags & REF_BAD_NAME) for that purpose.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 60f1493..b865ba5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1399,19 +1399,17 @@ static const char *resolve_ref_1(const char *refname,
 				 struct strbuf *sb_path,
 				 struct strbuf *sb_contents)
 {
-	int bad_name = 0;
 	int symref_count;
 
 	*flags = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		*flags |= REF_BAD_NAME;
-
 		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
 		    !refname_is_safe(refname)) {
 			errno = EINVAL;
 			return NULL;
 		}
+
 		/*
 		 * dwim_ref() uses REF_ISBROKEN to distinguish between
 		 * missing refs and refs that were present but invalid,
@@ -1420,7 +1418,7 @@ static const char *resolve_ref_1(const char *refname,
 		 * We don't know whether the ref exists, so don't set
 		 * REF_ISBROKEN yet.
 		 */
-		bad_name = 1;
+		*flags |= REF_BAD_NAME;
 	}
 
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
@@ -1452,7 +1450,7 @@ static const char *resolve_ref_1(const char *refname,
 				}
 				hashclr(sha1);
 			}
-			if (bad_name) {
+			if (*flags & REF_BAD_NAME) {
 				hashclr(sha1);
 				*flags |= REF_ISBROKEN;
 			}
@@ -1524,7 +1522,7 @@ static const char *resolve_ref_1(const char *refname,
 				errno = EINVAL;
 				return NULL;
 			}
-			if (bad_name) {
+			if (*flags & REF_BAD_NAME) {
 				hashclr(sha1);
 				*flags |= REF_ISBROKEN;
 			}
@@ -1548,8 +1546,7 @@ static const char *resolve_ref_1(const char *refname,
 				return NULL;
 			}
 
-			*flags |= REF_ISBROKEN;
-			bad_name = 1;
+			*flags |= REF_ISBROKEN | REF_BAD_NAME;
 		}
 	}
 
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 14/24] files-backend: break out ref reading

[David Turner]

Refactor resolve_ref_1 in terms of a new function read_raw_ref, which
is responsible for reading ref data from the ref storage.

Later, we will make read_raw_ref a pluggable backend function, and make
resolve_ref_unsafe common.

Signed-off-by: David Turner <dturner@twopensource.com>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 244 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 145 insertions(+), 99 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b865ba5..d51e778 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1390,6 +1390,141 @@ static int resolve_missing_loose_ref(const char *refname,
 	return -1;
 }
 
+/*
+ * Read a raw ref from the filesystem or packed refs file.
+ *
+ * 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 flags.
+ *
+ * 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
+ * -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
+ * outcome.
+ *
+ * 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
+ *   overwritten and the memory pointed to by refname might be changed
+ *   or even freed.
+ * - in all other cases, symref will be untouched, and therefore
+ *   refname will still be valid and unchanged.
+ */
+static int read_raw_ref(const char *refname, unsigned char *sha1,
+			struct strbuf *symref, struct strbuf *sb_path,
+			struct strbuf *sb_contents, int *flags)
+{
+	const char *path;
+	const char *buf;
+	struct stat st;
+	int fd;
+
+	strbuf_reset(sb_path);
+	strbuf_git_path(sb_path, "%s", refname);
+	path = sb_path->buf;
+
+stat_ref:
+	/*
+	 * We might have to loop back here to avoid a race
+	 * condition: first we lstat() the file, then we try
+	 * to read it as a link or as a file.  But if somebody
+	 * changes the type of the file (file <-> directory
+	 * <-> symlink) between the lstat() and reading, then
+	 * we don't want to report that as an error but rather
+	 * try again starting with the lstat().
+	 */
+
+	if (lstat(path, &st) < 0) {
+		if (errno != ENOENT)
+			return -1;
+		if (resolve_missing_loose_ref(refname, sha1, flags)) {
+			errno = ENOENT;
+			return -1;
+		}
+		return 0;
+	}
+
+	/* Follow "normalized" - ie "refs/.." symlinks by hand */
+	if (S_ISLNK(st.st_mode)) {
+		strbuf_reset(sb_contents);
+		if (strbuf_readlink(sb_contents, path, 0) < 0) {
+			if (errno == ENOENT || errno == EINVAL)
+				/* inconsistent with lstat; retry */
+				goto stat_ref;
+			else
+				return -1;
+		}
+		if (starts_with(sb_contents->buf, "refs/") &&
+		    !check_refname_format(sb_contents->buf, 0)) {
+			strbuf_swap(sb_contents, symref);
+			*flags |= REF_ISSYMREF;
+			return 0;
+		}
+	}
+
+	/* Is it a directory? */
+	if (S_ISDIR(st.st_mode)) {
+		errno = EISDIR;
+		return -1;
+	}
+
+	/*
+	 * Anything else, just open it and try to use it as
+	 * a ref
+	 */
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		if (errno == ENOENT)
+			/* inconsistent with lstat; retry */
+			goto stat_ref;
+		else
+			return -1;
+	}
+	strbuf_reset(sb_contents);
+	if (strbuf_read(sb_contents, fd, 256) < 0) {
+		int save_errno = errno;
+		close(fd);
+		errno = save_errno;
+		return -1;
+	}
+	close(fd);
+	strbuf_rtrim(sb_contents);
+	buf = sb_contents->buf;
+	if (starts_with(buf, "ref:")) {
+		buf += 4;
+		while (isspace(*buf))
+			buf++;
+
+		strbuf_reset(symref);
+		strbuf_addstr(symref, buf);
+		*flags |= REF_ISSYMREF;
+		return 0;
+	}
+
+	/*
+	 * Please note that FETCH_HEAD has additional
+	 * data after the sha.
+	 */
+	if (get_sha1_hex(buf, sha1) ||
+	    (buf[40] != '\0' && !isspace(buf[40]))) {
+		*flags |= REF_ISBROKEN;
+		errno = EINVAL;
+		return -1;
+	}
+
+	return 0;
+}
+
 /* This function needs to return a meaningful errno on failure */
 static const char *resolve_ref_1(const char *refname,
 				 int resolve_flags,
@@ -1422,118 +1557,29 @@ static const char *resolve_ref_1(const char *refname,
 	}
 
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
-		const char *path;
-		struct stat st;
-		int fd;
+		int read_flags = 0;
 
-		strbuf_reset(sb_path);
-		strbuf_git_path(sb_path, "%s", refname);
-		path = sb_path->buf;
-
-		/*
-		 * We might have to loop back here to avoid a race
-		 * condition: first we lstat() the file, then we try
-		 * to read it as a link or as a file.  But if somebody
-		 * changes the type of the file (file <-> directory
-		 * <-> symlink) between the lstat() and reading, then
-		 * we don't want to report that as an error but rather
-		 * try again starting with the lstat().
-		 */
-	stat_ref:
-		if (lstat(path, &st) < 0) {
-			if (errno != ENOENT)
+		if (read_raw_ref(refname, sha1, sb_refname,
+				 sb_path, sb_contents, &read_flags)) {
+			*flags |= read_flags;
+			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
-			if (resolve_missing_loose_ref(refname, sha1, flags)) {
-				if (resolve_flags & RESOLVE_REF_READING) {
-					errno = ENOENT;
-					return NULL;
-				}
-				hashclr(sha1);
-			}
-			if (*flags & REF_BAD_NAME) {
-				hashclr(sha1);
+			hashclr(sha1);
+			if (*flags & REF_BAD_NAME)
 				*flags |= REF_ISBROKEN;
-			}
 			return refname;
 		}
 
-		/* Follow "normalized" - ie "refs/.." symlinks by hand */
-		if (S_ISLNK(st.st_mode)) {
-			strbuf_reset(sb_contents);
-			if (strbuf_readlink(sb_contents, path, 0) < 0) {
-				if (errno == ENOENT || errno == EINVAL)
-					/* inconsistent with lstat; retry */
-					goto stat_ref;
-				else
-					return NULL;
-			}
-			if (starts_with(sb_contents->buf, "refs/") &&
-			    !check_refname_format(sb_contents->buf, 0)) {
-				strbuf_swap(sb_refname, sb_contents);
-				refname = sb_refname->buf;
-				*flags |= REF_ISSYMREF;
-				if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
-					hashclr(sha1);
-					return refname;
-				}
-				continue;
-			}
-		}
-
-		/* Is it a directory? */
-		if (S_ISDIR(st.st_mode)) {
-			errno = EISDIR;
-			return NULL;
-		}
-
-		/*
-		 * Anything else, just open it and try to use it as
-		 * a ref
-		 */
-		fd = open(path, O_RDONLY);
-		if (fd < 0) {
-			if (errno == ENOENT)
-				/* inconsistent with lstat; retry */
-				goto stat_ref;
-			else
-				return NULL;
-		}
-		strbuf_reset(sb_contents);
-		if (strbuf_read(sb_contents, fd, 256) < 0) {
-			int save_errno = errno;
-			close(fd);
-			errno = save_errno;
-			return NULL;
-		}
-		close(fd);
-		strbuf_rtrim(sb_contents);
+		*flags |= read_flags;
 
-		/*
-		 * Is it a symbolic ref?
-		 */
-		if (!starts_with(sb_contents->buf, "ref:")) {
-			/*
-			 * Please note that FETCH_HEAD has a second
-			 * line containing other data.
-			 */
-			if (get_sha1_hex(sb_contents->buf, sha1) ||
-			    (sb_contents->buf[40] != '\0' && !isspace(sb_contents->buf[40]))) {
-				*flags |= REF_ISBROKEN;
-				errno = EINVAL;
-				return NULL;
-			}
+		if (!(read_flags & REF_ISSYMREF)) {
 			if (*flags & REF_BAD_NAME) {
 				hashclr(sha1);
 				*flags |= REF_ISBROKEN;
 			}
 			return refname;
 		}
-		*flags |= REF_ISSYMREF;
-		refname = sb_contents->buf + 4;
-		while (isspace(*refname))
-			refname++;
-		strbuf_reset(sb_refname);
-		strbuf_addstr(sb_refname, refname);
+
 		refname = sb_refname->buf;
 		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
 			hashclr(sha1);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 15/24] read_raw_ref(): manage own scratch space

[David Turner]

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

Instead of creating scratch space in resolve_ref_unsafe() and passing it
down through resolve_ref_1 to read_raw_ref(), teach read_raw_ref() to
manage its own scratch space. This reduces coupling across the functions
at the cost of some extra allocations. Also, when read_raw_ref() is
implemented for different reference backends, the other implementations
might have different scratch space requirements.

Note that we now preserve errno across the calls to strbuf_release(),
which calls free() and can thus theoretically overwrite errno.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d51e778..f752568 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1421,17 +1421,20 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 static int read_raw_ref(const char *refname, unsigned char *sha1,
-			struct strbuf *symref, struct strbuf *sb_path,
-			struct strbuf *sb_contents, int *flags)
+			struct strbuf *symref, int *flags)
 {
+	struct strbuf sb_contents = STRBUF_INIT;
+	struct strbuf sb_path = STRBUF_INIT;
 	const char *path;
 	const char *buf;
 	struct stat st;
 	int fd;
+	int ret = -1;
+	int save_errno;
 
-	strbuf_reset(sb_path);
-	strbuf_git_path(sb_path, "%s", refname);
-	path = sb_path->buf;
+	strbuf_reset(&sb_path);
+	strbuf_git_path(&sb_path, "%s", refname);
+	path = sb_path.buf;
 
 stat_ref:
 	/*
@@ -1446,36 +1449,38 @@ stat_ref:
 
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
-			return -1;
+			goto out;
 		if (resolve_missing_loose_ref(refname, sha1, flags)) {
 			errno = ENOENT;
-			return -1;
+			goto out;
 		}
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	/* Follow "normalized" - ie "refs/.." symlinks by hand */
 	if (S_ISLNK(st.st_mode)) {
-		strbuf_reset(sb_contents);
-		if (strbuf_readlink(sb_contents, path, 0) < 0) {
+		strbuf_reset(&sb_contents);
+		if (strbuf_readlink(&sb_contents, path, 0) < 0) {
 			if (errno == ENOENT || errno == EINVAL)
 				/* inconsistent with lstat; retry */
 				goto stat_ref;
 			else
-				return -1;
+				goto out;
 		}
-		if (starts_with(sb_contents->buf, "refs/") &&
-		    !check_refname_format(sb_contents->buf, 0)) {
-			strbuf_swap(sb_contents, symref);
+		if (starts_with(sb_contents.buf, "refs/") &&
+		    !check_refname_format(sb_contents.buf, 0)) {
+			strbuf_swap(&sb_contents, symref);
 			*flags |= REF_ISSYMREF;
-			return 0;
+			ret = 0;
+			goto out;
 		}
 	}
 
 	/* Is it a directory? */
 	if (S_ISDIR(st.st_mode)) {
 		errno = EISDIR;
-		return -1;
+		goto out;
 	}
 
 	/*
@@ -1488,18 +1493,18 @@ stat_ref:
 			/* inconsistent with lstat; retry */
 			goto stat_ref;
 		else
-			return -1;
+			goto out;
 	}
-	strbuf_reset(sb_contents);
-	if (strbuf_read(sb_contents, fd, 256) < 0) {
+	strbuf_reset(&sb_contents);
+	if (strbuf_read(&sb_contents, fd, 256) < 0) {
 		int save_errno = errno;
 		close(fd);
 		errno = save_errno;
-		return -1;
+		goto out;
 	}
 	close(fd);
-	strbuf_rtrim(sb_contents);
-	buf = sb_contents->buf;
+	strbuf_rtrim(&sb_contents);
+	buf = sb_contents.buf;
 	if (starts_with(buf, "ref:")) {
 		buf += 4;
 		while (isspace(*buf))
@@ -1508,7 +1513,8 @@ stat_ref:
 		strbuf_reset(symref);
 		strbuf_addstr(symref, buf);
 		*flags |= REF_ISSYMREF;
-		return 0;
+		ret = 0;
+		goto out;
 	}
 
 	/*
@@ -1519,10 +1525,17 @@ stat_ref:
 	    (buf[40] != '\0' && !isspace(buf[40]))) {
 		*flags |= REF_ISBROKEN;
 		errno = EINVAL;
-		return -1;
+		goto out;
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	save_errno = errno;
+	strbuf_release(&sb_path);
+	strbuf_release(&sb_contents);
+	errno = save_errno;
+	return ret;
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1530,9 +1543,7 @@ static const char *resolve_ref_1(const char *refname,
 				 int resolve_flags,
 				 unsigned char *sha1,
 				 int *flags,
-				 struct strbuf *sb_refname,
-				 struct strbuf *sb_path,
-				 struct strbuf *sb_contents)
+				 struct strbuf *sb_refname)
 {
 	int symref_count;
 
@@ -1559,8 +1570,7 @@ static const char *resolve_ref_1(const char *refname,
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
 		int read_flags = 0;
 
-		if (read_raw_ref(refname, sha1, sb_refname,
-				 sb_path, sb_contents, &read_flags)) {
+		if (read_raw_ref(refname, sha1, sb_refname, &read_flags)) {
 			*flags |= read_flags;
 			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
@@ -1604,8 +1614,6 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 			       unsigned char *sha1, int *flags)
 {
 	static struct strbuf sb_refname = STRBUF_INIT;
-	struct strbuf sb_contents = STRBUF_INIT;
-	struct strbuf sb_path = STRBUF_INIT;
 	int unused_flags;
 	const char *ret;
 
@@ -1613,9 +1621,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 		flags = &unused_flags;
 
 	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-			    &sb_refname, &sb_path, &sb_contents);
-	strbuf_release(&sb_path);
-	strbuf_release(&sb_contents);
+			    &sb_refname);
 	return ret;
 }
 
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 16/24] Inline resolve_ref_1() into resolve_ref_unsafe()

[David Turner]

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

resolve_ref_unsafe() wasn't doing anything useful anymore.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f752568..120b2dd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1539,14 +1539,16 @@ out:
 }
 
 /* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_1(const char *refname,
-				 int resolve_flags,
-				 unsigned char *sha1,
-				 int *flags,
-				 struct strbuf *sb_refname)
+const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+			       unsigned char *sha1, int *flags)
 {
+	static struct strbuf sb_refname = STRBUF_INIT;
+	int unused_flags;
 	int symref_count;
 
+	if (!flags)
+		flags = &unused_flags;
+
 	*flags = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
@@ -1570,7 +1572,7 @@ static const char *resolve_ref_1(const char *refname,
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
 		int read_flags = 0;
 
-		if (read_raw_ref(refname, sha1, sb_refname, &read_flags)) {
+		if (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {
 			*flags |= read_flags;
 			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
@@ -1590,7 +1592,7 @@ static const char *resolve_ref_1(const char *refname,
 			return refname;
 		}
 
-		refname = sb_refname->buf;
+		refname = sb_refname.buf;
 		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
 			hashclr(sha1);
 			return refname;
@@ -1610,21 +1612,6 @@ static const char *resolve_ref_1(const char *refname,
 	return NULL;
 }
 
-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-			       unsigned char *sha1, int *flags)
-{
-	static struct strbuf sb_refname = STRBUF_INIT;
-	int unused_flags;
-	const char *ret;
-
-	if (!flags)
-		flags = &unused_flags;
-
-	ret = resolve_ref_1(refname, resolve_flags, sha1, flags,
-			    &sb_refname);
-	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
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 17/24] read_raw_ref(): change flags parameter to unsigned int

[David Turner]

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

read_raw_ref() is going to be part of the vtable for reference backends,
so clean up its interface to use "unsigned int flags" rather than "int
flags". Its caller still uses signed int for its flags arguments. But
changing that would touch a lot of code, so leave it for now.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 120b2dd..a15986c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1372,7 +1372,7 @@ static struct ref_entry *get_packed_ref(const char *refname)
  */
 static int resolve_missing_loose_ref(const char *refname,
 				     unsigned char *sha1,
-				     int *flags)
+				     unsigned int *flags)
 {
 	struct ref_entry *entry;
 
@@ -1421,7 +1421,7 @@ static int resolve_missing_loose_ref(const char *refname,
  *   refname will still be valid and unchanged.
  */
 static int read_raw_ref(const char *refname, unsigned char *sha1,
-			struct strbuf *symref, int *flags)
+			struct strbuf *symref, unsigned int *flags)
 {
 	struct strbuf sb_contents = STRBUF_INIT;
 	struct strbuf sb_path = STRBUF_INIT;
@@ -1570,7 +1570,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 	}
 
 	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
-		int read_flags = 0;
+		unsigned int read_flags = 0;
 
 		if (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {
 			*flags |= read_flags;
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 18/24] fsck_head_link(): remove unneeded flag variable

[David Turner]

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

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fsck.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 55eac75..3f27456 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -493,13 +493,12 @@ static void fsck_object_dir(const char *path)
 
 static int fsck_head_link(void)
 {
-	int flag;
 	int null_is_error = 0;
 
 	if (verbose)
 		fprintf(stderr, "Checking HEAD link\n");
 
-	head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid.hash, &flag);
+	head_points_at = resolve_ref_unsafe("HEAD", 0, head_oid.hash, NULL);
 	if (!head_points_at) {
 		errors_found |= ERROR_REFS;
 		return error("Invalid HEAD");
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 19/24] cmd_merge(): remove unneeded flag variable

[David Turner]

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

It is never read, so we can pass NULL to resolve_ref_unsafe().

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

diff --git a/builtin/merge.c b/builtin/merge.c
index 101ffef..c90ee51 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1165,7 +1165,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
-	int flag, i, ret = 0, head_subsumed;
+	int i, ret = 0, head_subsumed;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1179,7 +1179,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = branch_to_free = resolve_refdup("HEAD", 0, head_sha1, &flag);
+	branch = branch_to_free = resolve_refdup("HEAD", 0, head_sha1, NULL);
 	if (branch && starts_with(branch, "refs/heads/"))
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 20/24] checkout_paths(): remove unneeded flag variable

[David Turner]

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

It is never read, so we can pass NULL to resolve_ref_unsafe().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/checkout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index efcbd8f..ea2fe1c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -242,7 +242,6 @@ static int checkout_paths(const struct checkout_opts *opts,
 	struct checkout state;
 	static char *ps_matched;
 	unsigned char rev[20];
-	int flag;
 	struct commit *head;
 	int errs = 0;
 	struct lock_file *lock_file;
@@ -375,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
-	read_ref_full("HEAD", 0, rev, &flag);
+	read_ref_full("HEAD", 0, rev, NULL);
 	head = lookup_commit_reference_gently(rev, 1);
 
 	errs |= post_checkout_hook(head, head, 0);
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 21/24] check_aliased_update(): check that dst_name is non-NULL

[David Turner]

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

If there is an error in resolve_ref_unsafe(), it returns NULL. We check
for this case, but not until after calling strip_namespace(). Instead,
call strip_namespace() *after* the NULL check.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..49cc88d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1081,13 +1081,13 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	if (!(flag & REF_ISSYMREF))
 		return;
 
-	dst_name = strip_namespace(dst_name);
 	if (!dst_name) {
 		rp_error("refusing update to broken symref '%s'", cmd->ref_name);
 		cmd->skip_update = 1;
 		cmd->error_string = "broken symref";
 		return;
 	}
+	dst_name = strip_namespace(dst_name);
 
 	if ((item = string_list_lookup(list, dst_name)) == NULL)
 		return;
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 22/24] show_head_ref(): check the result of resolve_ref_namespace()

[David Turner]

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

Only use the result of resolve_ref_namespace() if it is non-NULL.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 http-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 8870a26..2148814 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -484,9 +484,9 @@ static int show_head_ref(const char *refname, const struct object_id *oid,
 		const char *target = resolve_ref_unsafe(refname,
 							RESOLVE_REF_READING,
 							unused.hash, NULL);
-		const char *target_nons = strip_namespace(target);
 
-		strbuf_addf(buf, "ref: %s\n", target_nons);
+		if (target)
+			strbuf_addf(buf, "ref: %s\n", strip_namespace(target));
 	} else {
 		strbuf_addf(buf, "%s\n", oid_to_hex(oid));
 	}
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 23/24] refs: move resolve_ref_unsafe into common code

[David Turner]

Now that resolve_ref_unsafe's only interaction with the backend is
through read_raw_ref, we can move it into the common code. Later,
we'll replace read_raw_ref with a backend function.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c               | 74 +++++++++++++++++++++++++++++++++++++++++++++++
 refs/files-backend.c | 82 ++--------------------------------------------------
 refs/refs-internal.h |  6 ++++
 3 files changed, 83 insertions(+), 79 deletions(-)

diff --git a/refs.c b/refs.c
index f0feff7..87dc82f 100644
--- a/refs.c
+++ b/refs.c
@@ -1155,3 +1155,77 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return do_for_each_ref(NULL, "", fn, 0,
 			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
+
+/* This function needs to return a meaningful errno on failure */
+const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+			       unsigned char *sha1, int *flags)
+{
+	static struct strbuf sb_refname = STRBUF_INIT;
+	int unused_flags;
+	int symref_count;
+
+	if (!flags)
+		flags = &unused_flags;
+
+	*flags = 0;
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
+		    !refname_is_safe(refname)) {
+			errno = EINVAL;
+			return NULL;
+		}
+
+		/*
+		 * dwim_ref() uses REF_ISBROKEN to distinguish between
+		 * missing refs and refs that were present but invalid,
+		 * to complain about the latter to stderr.
+		 *
+		 * We don't know whether the ref exists, so don't set
+		 * REF_ISBROKEN yet.
+		 */
+		*flags |= REF_BAD_NAME;
+	}
+
+	for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
+		unsigned int read_flags = 0;
+
+		if (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {
+			*flags |= read_flags;
+			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
+				return NULL;
+			hashclr(sha1);
+			if (*flags & REF_BAD_NAME)
+				*flags |= REF_ISBROKEN;
+			return refname;
+		}
+
+		*flags |= read_flags;
+
+		if (!(read_flags & REF_ISSYMREF)) {
+			if (*flags & REF_BAD_NAME) {
+				hashclr(sha1);
+				*flags |= REF_ISBROKEN;
+			}
+			return refname;
+		}
+
+		refname = sb_refname.buf;
+		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
+			hashclr(sha1);
+			return refname;
+		}
+		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
+			    !refname_is_safe(refname)) {
+				errno = EINVAL;
+				return NULL;
+			}
+
+			*flags |= REF_ISBROKEN | REF_BAD_NAME;
+		}
+	}
+
+	errno = ELOOP;
+	return NULL;
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a15986c..71848ab 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1269,8 +1269,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)
 	return get_ref_dir(refs->loose);
 }
 
-/* We allow "recursive" symbolic refs. Only within reason, though */
-#define MAXDEPTH 5
 #define MAXREFLEN (1024)
 
 /*
@@ -1300,7 +1298,7 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
 	char buffer[128], *p;
 	char *path;
 
-	if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
+	if (recursion > SYMREF_MAXDEPTH || strlen(refname) > MAXREFLEN)
 		return -1;
 	path = *refs->name
 		? git_pathdup_submodule(refs->name, "%s", refname)
@@ -1420,8 +1418,8 @@ static int resolve_missing_loose_ref(const char *refname,
  * - in all other cases, symref will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-static int read_raw_ref(const char *refname, unsigned char *sha1,
-			struct strbuf *symref, unsigned int *flags)
+int read_raw_ref(const char *refname, unsigned char *sha1,
+		 struct strbuf *symref, unsigned int *flags)
 {
 	struct strbuf sb_contents = STRBUF_INIT;
 	struct strbuf sb_path = STRBUF_INIT;
@@ -1538,80 +1536,6 @@ out:
 	return ret;
 }
 
-/* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-			       unsigned char *sha1, int *flags)
-{
-	static struct strbuf sb_refname = STRBUF_INIT;
-	int unused_flags;
-	int symref_count;
-
-	if (!flags)
-		flags = &unused_flags;
-
-	*flags = 0;
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-		    !refname_is_safe(refname)) {
-			errno = EINVAL;
-			return NULL;
-		}
-
-		/*
-		 * dwim_ref() uses REF_ISBROKEN to distinguish between
-		 * missing refs and refs that were present but invalid,
-		 * to complain about the latter to stderr.
-		 *
-		 * We don't know whether the ref exists, so don't set
-		 * REF_ISBROKEN yet.
-		 */
-		*flags |= REF_BAD_NAME;
-	}
-
-	for (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {
-		unsigned int read_flags = 0;
-
-		if (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {
-			*flags |= read_flags;
-			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
-				return NULL;
-			hashclr(sha1);
-			if (*flags & REF_BAD_NAME)
-				*flags |= REF_ISBROKEN;
-			return refname;
-		}
-
-		*flags |= read_flags;
-
-		if (!(read_flags & REF_ISSYMREF)) {
-			if (*flags & REF_BAD_NAME) {
-				hashclr(sha1);
-				*flags |= REF_ISBROKEN;
-			}
-			return refname;
-		}
-
-		refname = sb_refname.buf;
-		if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
-			hashclr(sha1);
-			return refname;
-		}
-		if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-			if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
-			    !refname_is_safe(refname)) {
-				errno = EINVAL;
-				return NULL;
-			}
-
-			*flags |= REF_ISBROKEN | REF_BAD_NAME;
-		}
-	}
-
-	errno = ELOOP;
-	return NULL;
-}
-
 /*
  * 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
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 92aae80..3a4f634 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -197,6 +197,8 @@ const char *find_descendant_ref(const char *dirname,
 
 int rename_ref_available(const char *oldname, const char *newname);
 
+/* We allow "recursive" symbolic refs. Only within reason, though */
+#define SYMREF_MAXDEPTH 5
 
 /* Include broken references in a do_for_each_ref*() iteration: */
 #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
@@ -206,4 +208,8 @@ int rename_ref_available(const char *oldname, const char *newname);
  */
 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);
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.4.2.767.g62658d5-twtrsrc

[PATCH 24/24] refs: on symref reflog expire, lock symref not referrent

[David Turner]

When locking a symbolic ref to expire a reflog, lock the symbolic
ref (using REF_NODEREF) instead of its referent.

Add a test for this.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs/files-backend.c |  3 ++-
 t/t1410-reflog.sh    | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 71848ab..5e67bfa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3314,7 +3314,8 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 	 * reference itself, plus we might need to update the
 	 * reference if --updateref was specified:
 	 */
-	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, 0, &type, &err);
+	lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, REF_NODEREF,
+				   &type, &err);
 	if (!lock) {
 		error("cannot lock ref '%s': %s", refname, err.buf);
 		strbuf_release(&err);
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index c623824..9cf91dc 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -338,4 +338,14 @@ test_expect_failure 'reflog with non-commit entries displays all entries' '
 	test_line_count = 3 actual
 '
 
+test_expect_success 'reflog expire operates on symref not referrent' '
+	git branch -l the_symref &&
+	git branch -l referrent &&
+	git update-ref referrent HEAD &&
+	git symbolic-ref refs/heads/the_symref refs/heads/referrent &&
+	test_when_finished "rm -f .git/refs/heads/referrent.lock" &&
+	touch .git/refs/heads/referrent.lock &&
+	git reflog expire --expire=all the_symref
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

Re: [PATCH 00/24] Yet another pre-refs-backend series

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> We now have quite a large number of patches before we even get into
> the meat of the pluggable refs backend series.  So it's worth breaking
> those out and getting them in before we get into the main series
> (which Michael Haggerty swants to redesign a bit anyway).
>
> This set of patches should be applied on top of
> jk/check-repository-format.
>
> Michael Haggerty has reviewed those of my patches which are in here
> except maybe:
>   refs: on symref reflog expire, lock symref not referrent
> This was the one from later in the series that was straightforward to
> move to before the vtable; the other two were going to be harder to
> move and can wait until after the vtable.
>
> I have reviewed all of Michael's patches.

Thanks for working together well.
Will queue.

Re: [PATCH 00/24] Yet another pre-refs-backend series

[Michael Haggerty]

On 04/07/2016 03:02 PM, David Turner wrote:
> We now have quite a large number of patches before we even get into
> the meat of the pluggable refs backend series.  So it's worth breaking
> those out and getting them in before we get into the main series
> (which Michael Haggerty swants to redesign a bit anyway).
> 
> This set of patches should be applied on top of
> jk/check-repository-format.
> 
> Michael Haggerty has reviewed those of my patches which are in here
> except maybe:
>   refs: on symref reflog expire, lock symref not referrent
> This was the one from later in the series that was straightforward to
> move to before the vtable; the other two were going to be harder to
> move and can wait until after the vtable.

This last patch deserves a little bit of discussion. Currently, when the
reflog of a symref is expired, the pointed-to ref is locked rather than
the symref. This patch changes the code to lock the symref instead.

This is clearly the right thing to do, and I consider this change a bug
fix. However, it introduces an incompatibility. An old version of `git
reflog expire` and a new version wouldn't agree on the locking protocol,
and could potentially try to overwrite the same reflog at the same time.

I think this risk is acceptable nevertheless, because expiring reflogs
is an uncommon operation and unlikely to be done from two processes at
the same time; moreover, the integrity of reflogs is not a matter of
life or death.

A far more likely conflict would be between a reflog expiration and a
symref update (e.g., `git checkout otherbranch`). This use case is
currently *broken* because `git checkout` locks HEAD. It would be fixed
by this patch.

If somebody is really upset about the risk of a race between an old and
new version of `git reflog expire`, the way to increase the safety would
be to lock *both* the symref and the referent while changing the
symref's reflog. I think that would be overkill.

This whole series is

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

David mentioned that I want to redesign the vtable patches somewhat.
Anybody who is curious can look at the work in progress branch on my
GitHub fork [1], branch wip/ref-storage.

Michael

[1] https://github.com/mhagger/git

Re: [PATCH 00/24] Yet another pre-refs-backend series

[Junio C Hamano]

On Sat, Apr 9, 2016 at 9:19 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> I think this risk is acceptable nevertheless, because expiring reflogs
> is an uncommon operation and unlikely to be done from two processes at
> the same time; moreover, the integrity of reflogs is not a matter of
> life or death.
> ...
> If somebody is really upset about the risk of a race between an old and
> new version of `git reflog expire`, the way to increase the safety would
> be to lock *both* the symref and the referent while changing the
> symref's reflog. I think that would be overkill.

Thanks, I agree with all of the above.

> This whole series is
>
> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Will squeeze it into all of them, then, and merge to 'next'.

Thanks.