All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
@ 2016-03-23 10:04 Michael Haggerty
  2016-03-23 10:04 ` [PATCH 01/21] t1430: test the output and error of some commands more carefully Michael Haggerty
                   ` (21 more replies)
  0 siblings, 22 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way
too much in a single patch, and in fact got a few minor things wrong.
Instead of that patch, I suggest this patch series, which

* Splits the changes into smaller steps.

* Adds a bunch of tests of deleting references with invalid but safe
  names, including symbolic references and including references
  reached via symbolic references. Two of these tests fail when run
  against David's patch 04 due to changes in output.

* Arranges for the "flags" argument to read_raw_ref() always to be
  non-NULL, which eliminates the need for a lot of "if (flags)"
  guards.

* Eliminates the now-superfluous "bad_name" local variable.

* Move the management of the scratch space sb_path from
  resolve_ref_unsafe() to read_raw_ref().

* Inlines resolve_ref_1() into resolve_ref_unsafe().

* Changes some callers of resolve_ref_unsafe() to pass flags=NULL
  instead of creating a local flags variable that is never used.

* Changes some callers to check for errors before using the return
  value of resolve_ref_unsafe().

I hope that the result is easier to understand and audit, even though
it consists of more patches (indeed, *because* of that).

This patch series applies on top of David's patch 03/33 the same place
David applied it in his repo [2]. It is also available in situ from my
GitHub repo [3] as branch "pluggable-backends-patch4"

If this series is used, later patches from David's series would need
to be rebased on top of it. This is a little bit messy but not
difficult; the result can be seen in branch
"pluggable-backends-rebased" in my GitHub repo [3] (albeit without
adjusting the LMDB-related patches).

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/287971
[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
[3] https://github.com/mhagger/git

David Turner (1):
  files-backend: break out ref reading

Michael Haggerty (20):
  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
  get_default_remote(): 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 +-
 builtin/submodule--helper.c |   3 +-
 http-backend.c              |   4 +-
 refs/files-backend.c        | 341 ++++++++++++++++++++++++--------------------
 t/t1430-bad-ref-name.sh     | 132 +++++++++++++++--
 8 files changed, 312 insertions(+), 180 deletions(-)

-- 
2.8.0.rc3

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 01/21] t1430: test the output and error of some commands more carefully
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 02/21] t1430: clean up broken refs/tags/shadow Michael Haggerty
                   ` (20 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 02/21] t1430: clean up broken refs/tags/shadow
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
  2016-03-23 10:04 ` [PATCH 01/21] t1430: test the output and error of some commands more carefully Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs Michael Haggerty
                   ` (19 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
  2016-03-23 10:04 ` [PATCH 01/21] t1430: test the output and error of some commands more carefully Michael Haggerty
  2016-03-23 10:04 ` [PATCH 02/21] t1430: clean up broken refs/tags/shadow Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 04/21] t1430: test for-each-ref in the presence of badly-named refs Michael Haggerty
                   ` (18 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 04/21] t1430: test for-each-ref in the presence of badly-named refs
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (2 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 05/21] t1430: improve test coverage of deletion " Michael Haggerty
                   ` (17 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 05/21] t1430: improve test coverage of deletion of badly-named refs
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (3 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 04/21] t1430: test for-each-ref in the presence of badly-named refs Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 06/21] resolve_missing_loose_ref(): simplify semantics Michael Haggerty
                   ` (16 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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 | 108 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 98 insertions(+), 10 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,16 +171,6 @@ 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' '
-	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' '
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
@@ -192,6 +182,104 @@ test_expect_success 'update-ref -d can delete broken name' '
 	! 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" &&
+	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 '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" &&
+	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
+'
+
+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' '
 	echo precious >.git/my-private-file &&
 	echo precious >expect &&
-- 
2.8.0.rc3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 06/21] resolve_missing_loose_ref(): simplify semantics
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (4 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 05/21] t1430: improve test coverage of deletion " Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH Michael Haggerty
                   ` (15 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (5 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 06/21] resolve_missing_loose_ref(): simplify semantics Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set Michael Haggerty
                   ` (14 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (6 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 09/21] resolve_ref_1(): eliminate local variable Michael Haggerty
                   ` (13 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 09/21] resolve_ref_1(): eliminate local variable
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (7 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 10/21] resolve_ref_1(): reorder code Michael Haggerty
                   ` (12 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 10/21] resolve_ref_1(): reorder code
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (8 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 09/21] resolve_ref_1(): eliminate local variable Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 11/21] resolve_ref_1(): eliminate local variable "bad_name" Michael Haggerty
                   ` (11 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 11/21] resolve_ref_1(): eliminate local variable "bad_name"
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (9 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 10/21] resolve_ref_1(): reorder code Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 12/21] files-backend: break out ref reading Michael Haggerty
                   ` (10 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 12/21] files-backend: break out ref reading
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (10 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 11/21] resolve_ref_1(): eliminate local variable "bad_name" Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 13/21] read_raw_ref(): manage own scratch space Michael Haggerty
                   ` (9 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, peff, pclouds, Ramsay Jones, David Turner, Michael Haggerty

From: David Turner <dturner@twopensource.com>

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;
-			}
-		}
+		*flags |= read_flags;
 
-		/* 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);
-
-		/*
-		 * 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.8.0.rc3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 13/21] read_raw_ref(): manage own scratch space
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (11 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 12/21] files-backend: break out ref reading Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe() Michael Haggerty
                   ` (8 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe()
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (12 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 13/21] read_raw_ref(): manage own scratch space Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int Michael Haggerty
                   ` (7 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (13 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe() Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 16/21] fsck_head_link(): remove unneeded flag variable Michael Haggerty
                   ` (6 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 16/21] fsck_head_link(): remove unneeded flag variable
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (14 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 17/21] cmd_merge(): " Michael Haggerty
                   ` (5 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 17/21] cmd_merge(): remove unneeded flag variable
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (15 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 16/21] fsck_head_link(): remove unneeded flag variable Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 18/21] get_default_remote(): " Michael Haggerty
                   ` (4 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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 df0afa0..a91ae5b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1146,7 +1146,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	unsigned char head_sha1[20];
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
-	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;
@@ -1160,7 +1160,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.8.0.rc3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 18/21] get_default_remote(): remove unneeded flag variable
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (16 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 17/21] cmd_merge(): " Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 19/21] checkout_paths(): " Michael Haggerty
                   ` (3 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ed4f60b..c72365e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -17,9 +17,8 @@ static char *get_default_remote(void)
 {
 	char *dest = NULL, *ret;
 	unsigned char sha1[20];
-	int flag = 0;
 	struct strbuf sb = STRBUF_INIT;
-	const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
+	const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
 
 	if (!refname)
 		die("No such ref: HEAD");
-- 
2.8.0.rc3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 19/21] checkout_paths(): remove unneeded flag variable
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (17 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 18/21] get_default_remote(): " Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 20/21] check_aliased_update(): check that dst_name is non-NULL Michael Haggerty
                   ` (2 subsequent siblings)
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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 cfa66e2..ef42237 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.8.0.rc3

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 20/21] check_aliased_update(): check that dst_name is non-NULL
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (18 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 19/21] checkout_paths(): " Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-23 10:04 ` [PATCH 21/21] show_head_ref(): check the result of resolve_ref_namespace() Michael Haggerty
  2016-03-24  6:47 ` [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 David Turner
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 21/21] show_head_ref(): check the result of resolve_ref_namespace()
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (19 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 20/21] check_aliased_update(): check that dst_name is non-NULL Michael Haggerty
@ 2016-03-23 10:04 ` Michael Haggerty
  2016-03-24  6:47 ` [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 David Turner
  21 siblings, 0 replies; 31+ messages in thread
From: Michael Haggerty @ 2016-03-23 10:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones, Michael Haggerty

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
                   ` (20 preceding siblings ...)
  2016-03-23 10:04 ` [PATCH 21/21] show_head_ref(): check the result of resolve_ref_namespace() Michael Haggerty
@ 2016-03-24  6:47 ` David Turner
  2016-03-27  5:22   ` Michael Haggerty
  21 siblings, 1 reply; 31+ messages in thread
From: David Turner @ 2016-03-24  6:47 UTC (permalink / raw)
  To: Michael Haggerty, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On Wed, 2016-03-23 at 11:04 +0100, Michael Haggerty wrote:
> Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way
> too much in a single patch, and in fact got a few minor things wrong.
> Instead of that patch, I suggest this patch series, which
> ...

LGTM.  I think I would squash these patches:
  fsck_head_link(): remove unneeded flag variable
  cmd_merge(): remove unneeded flag variable
  get_default_remote(): remove unneeded flag variable
  checkout_paths(): remove unneeded flag variable

But that's up to you.

I incorporated your changes into the lmdb backend.  To make merging
later more convenient, I rebased on top of pu -- I think this mainly
depends on jk/check-repository-format, but I also included some fixes
for a couple of tests that had been changed by other patches.

The current version can be found here:

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

I won't resend the full patchset to the list until I hear back on the
rest of the review.

It seems like maybe we should now split this into two patchsets:
everything up to and including "refs: move resolve_ref_unsafe into
common code" does not depend on the backend structure and could go in
earlier.  If you agree, we could send that first series and get it in,
hopefully reducing later merge conflicts.  

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-24  6:47 ` [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 David Turner
@ 2016-03-27  5:22   ` Michael Haggerty
  2016-03-29 20:12     ` David Turner
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Haggerty @ 2016-03-27  5:22 UTC (permalink / raw)
  To: David Turner, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On 03/24/2016 07:47 AM, David Turner wrote:
> [...]
> I incorporated your changes into the lmdb backend.  To make merging
> later more convenient, I rebased on top of pu -- I think this mainly
> depends on jk/check-repository-format, but I also included some fixes
> for a couple of tests that had been changed by other patches.

I think rebasing changes on top of pu is counterproductive. I believe
that Junio had extra work rebasing your earlier series onto a merge of
the minimum number of topics that it really depended on. There is no way
that he could merge the branch in this form because it would imply
merging all of pu.

See the zeroth section of SubmittingPatches [1] for the guidelines.

> The current version can be found here:
> 
> https://github.com/dturner-tw/git/tree/dturner/pluggable-backends
> 
> I won't resend the full patchset to the list until I hear back on the
> rest of the review.
> 
> It seems like maybe we should now split this into two patchsets:
> everything up to and including "refs: move resolve_ref_unsafe into
> common code" does not depend on the backend structure and could go in
> earlier.  If you agree, we could send that first series and get it in,
> hopefully reducing later merge conflicts.  

That sounds like a good idea. It's always a relief to get work merged
and not have to keep porting it along.

There are three patches later in the series that (I think) also don't
have specifically to do with pluggable backends. These could potentially
also be considered for earlier merge to reduce the size of what remains:

* refs: don't dereference on rename
* refs: on symref reflog expire, lock symref not referrent
* refs: resolve symbolic refs first

But note that I haven't audited those patches yet, so I'm not saying
that they are necessarily ready to be merged.

Michael

[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-27  5:22   ` Michael Haggerty
@ 2016-03-29 20:12     ` David Turner
  2016-03-30  6:37       ` Michael Haggerty
  0 siblings, 1 reply; 31+ messages in thread
From: David Turner @ 2016-03-29 20:12 UTC (permalink / raw)
  To: Michael Haggerty, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> On 03/24/2016 07:47 AM, David Turner wrote:
> > [...]
> > I incorporated your changes into the lmdb backend.  To make merging
> > later more convenient, I rebased on top of pu -- I think this
> > mainly
> > depends on jk/check-repository-format, but I also included some
> > fixes
> > for a couple of tests that had been changed by other patches.
> 
> I think rebasing changes on top of pu is counterproductive. I believe
> that Junio had extra work rebasing your earlier series onto a merge
> of
> the minimum number of topics that it really depended on. There is no
> way
> that he could merge the branch in this form because it would imply
> merging all of pu.
> 
> See the zeroth section of SubmittingPatches [1] for the guidelines.

I'm a bit confused because 
[PATCH 18/21] get_default_remote(): remove unneeded flag variable

doesn't do anything on master -- it depends on some patch in pu.  And
we definitely want to pick up jk/check-repository-format (which doesn't
include whatever 18/21 depends on).

So what do you think our base should be?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-29 20:12     ` David Turner
@ 2016-03-30  6:37       ` Michael Haggerty
  2016-03-30 20:05         ` David Turner
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Haggerty @ 2016-03-30  6:37 UTC (permalink / raw)
  To: David Turner, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On 03/29/2016 10:12 PM, David Turner wrote:
> On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
>> On 03/24/2016 07:47 AM, David Turner wrote:
>>> [...]
>>> I incorporated your changes into the lmdb backend.  To make merging
>>> later more convenient, I rebased on top of pu -- I think this
>>> mainly
>>> depends on jk/check-repository-format, but I also included some
>>> fixes
>>> for a couple of tests that had been changed by other patches.
>>
>> I think rebasing changes on top of pu is counterproductive. I believe
>> that Junio had extra work rebasing your earlier series onto a merge
>> of
>> the minimum number of topics that it really depended on. There is no
>> way
>> that he could merge the branch in this form because it would imply
>> merging all of pu.
>>
>> See the zeroth section of SubmittingPatches [1] for the guidelines.
> 
> I'm a bit confused because 
> [PATCH 18/21] get_default_remote(): remove unneeded flag variable
> 
> doesn't do anything on master -- it depends on some patch in pu.  And
> we definitely want to pick up jk/check-repository-format (which doesn't
> include whatever 18/21 depends on).
> 
> So what do you think our base should be?

I think the preference is to base a patch series on the merge of master
plus the minimum number of topics in pu (ideally, none) that are
"essential" prerequisites of the changes in the patch series. For
example, the version of this patch series that Junio has in his tree was
based on master + sb/submodule-parallel-update. Even if there are minor
conflicts with another in-flight topic, it is easier for Junio to
resolve the conflicts when merging the topics together than to rebase
the patch series over and over as the other patch series evolves. The
goal of this practice is of course to allow patch series to evolve
independently of each other as much as possible.

Of course if you have insights into nontrivial conflicts between your
patch series and others, it would be helpful to discuss these in your
cover letter.

Michael

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-30  6:37       ` Michael Haggerty
@ 2016-03-30 20:05         ` David Turner
  2016-03-31 16:14           ` Michael Haggerty
  2016-04-01  1:37           ` Stefan Beller
  0 siblings, 2 replies; 31+ messages in thread
From: David Turner @ 2016-03-30 20:05 UTC (permalink / raw)
  To: Michael Haggerty, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
> On 03/29/2016 10:12 PM, David Turner wrote:
> > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> > > On 03/24/2016 07:47 AM, David Turner wrote:
> > > > [...]
> > > > I incorporated your changes into the lmdb backend.  To make
> > > > merging
> > > > later more convenient, I rebased on top of pu -- I think this
> > > > mainly
> > > > depends on jk/check-repository-format, but I also included some
> > > > fixes
> > > > for a couple of tests that had been changed by other patches.
> > > 
> > > I think rebasing changes on top of pu is counterproductive. I
> > > believe
> > > that Junio had extra work rebasing your earlier series onto a
> > > merge
> > > of
> > > the minimum number of topics that it really depended on. There is
> > > no
> > > way
> > > that he could merge the branch in this form because it would
> > > imply
> > > merging all of pu.
> > > 
> > > See the zeroth section of SubmittingPatches [1] for the
> > > guidelines.
> > 
> > I'm a bit confused because 
> > [PATCH 18/21] get_default_remote(): remove unneeded flag variable
> > 
> > doesn't do anything on master -- it depends on some patch in pu. 
> >  And
> > we definitely want to pick up jk/check-repository-format (which
> > doesn't
> > include whatever 18/21 depends on).
> > 
> > So what do you think our base should be?
> 
> I think the preference is to base a patch series on the merge of
> master
> plus the minimum number of topics in pu (ideally, none) that are
> "essential" prerequisites of the changes in the patch series. For
> example, the version of this patch series that Junio has in his tree
> was
> based on master + sb/submodule-parallel-update. 
>
> Even if there are minor
> conflicts with another in-flight topic, it is easier for Junio to
> resolve the conflicts when merging the topics together than to rebase
> the patch series over and over as the other patch series evolves. The
> goal of this practice is of course to allow patch series to evolve
> independently of each other as much as possible.
> 
> Of course if you have insights into nontrivial conflicts between your
> patch series and others, it would be helpful to discuss these in your
> cover letter.

If I am reading this correctly, it looks like your series also has a
few more sb submodule patches, e.g. sb/submodule-init, which is
responsible for the code that 18/21 depends on.  

I think jk/check-repository-format is also  good to get in first,
because it changes the startup sequence a bit and it's a bit tricky to
figure out what needs to change in dt/refs-backend-lmdb as a result of
it. 

But I can't just merge jk/check-repository-format on top of 71defe0047 
-- some function signatures have changed in the run-command stuff and
it seems kind of annoying to fix up.  

So I propose instead that we just drop 18/21 for now, and use just
jk/check-repository-format as the base.

Does this seem reasonable to you?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-30 20:05         ` David Turner
@ 2016-03-31 16:14           ` Michael Haggerty
  2016-03-31 22:22             ` David Turner
  2016-04-01  1:37           ` Stefan Beller
  1 sibling, 1 reply; 31+ messages in thread
From: Michael Haggerty @ 2016-03-31 16:14 UTC (permalink / raw)
  To: David Turner, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On 03/30/2016 10:05 PM, David Turner wrote:
> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
>> On 03/29/2016 10:12 PM, David Turner wrote:
>>> On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
>>>> On 03/24/2016 07:47 AM, David Turner wrote:
>>>>> [...]
>>>>> I incorporated your changes into the lmdb backend.  To make
>>>>> merging
>>>>> later more convenient, I rebased on top of pu -- I think this
>>>>> mainly
>>>>> depends on jk/check-repository-format, but I also included some
>>>>> fixes
>>>>> for a couple of tests that had been changed by other patches.
>>>>
>>>> I think rebasing changes on top of pu is counterproductive. I
>>>> believe
>>>> that Junio had extra work rebasing your earlier series onto a
>>>> merge
>>>> of
>>>> the minimum number of topics that it really depended on. There is
>>>> no
>>>> way
>>>> that he could merge the branch in this form because it would
>>>> imply
>>>> merging all of pu.
>>>>
>>>> See the zeroth section of SubmittingPatches [1] for the
>>>> guidelines.
>>>
>>> I'm a bit confused because 
>>> [PATCH 18/21] get_default_remote(): remove unneeded flag variable
>>>
>>> doesn't do anything on master -- it depends on some patch in pu. 
>>>  And
>>> we definitely want to pick up jk/check-repository-format (which
>>> doesn't
>>> include whatever 18/21 depends on).
>>>
>>> So what do you think our base should be?
>>
>> I think the preference is to base a patch series on the merge of
>> master
>> plus the minimum number of topics in pu (ideally, none) that are
>> "essential" prerequisites of the changes in the patch series. For
>> example, the version of this patch series that Junio has in his tree
>> was
>> based on master + sb/submodule-parallel-update. 
>>
>> Even if there are minor
>> conflicts with another in-flight topic, it is easier for Junio to
>> resolve the conflicts when merging the topics together than to rebase
>> the patch series over and over as the other patch series evolves. The
>> goal of this practice is of course to allow patch series to evolve
>> independently of each other as much as possible.
>>
>> Of course if you have insights into nontrivial conflicts between your
>> patch series and others, it would be helpful to discuss these in your
>> cover letter.
> 
> If I am reading this correctly, it looks like your series also has a
> few more sb submodule patches, e.g. sb/submodule-init, which is
> responsible for the code that 18/21 depends on.  
> 
> I think jk/check-repository-format is also  good to get in first,
> because it changes the startup sequence a bit and it's a bit tricky to
> figure out what needs to change in dt/refs-backend-lmdb as a result of
> it. 
> 
> But I can't just merge jk/check-repository-format on top of 71defe0047 
> -- some function signatures have changed in the run-command stuff and
> it seems kind of annoying to fix up.  
> 
> So I propose instead that we just drop 18/21 for now, and use just
> jk/check-repository-format as the base.
> 
> Does this seem reasonable to you?

Yes, that's fine. Patch 18/21 is just a random cleanup that nothing else
depends on. Will you do the rebasing? If so, please let me know where I
can fetch the result from.

Michael

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-31 16:14           ` Michael Haggerty
@ 2016-03-31 22:22             ` David Turner
  0 siblings, 0 replies; 31+ messages in thread
From: David Turner @ 2016-03-31 22:22 UTC (permalink / raw)
  To: Michael Haggerty, Junio C Hamano; +Cc: git, peff, pclouds, Ramsay Jones

On Thu, 2016-03-31 at 18:14 +0200, Michael Haggerty wrote:
> On 03/30/2016 10:05 PM, David Turner wrote:
> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
> > > On 03/29/2016 10:12 PM, David Turner wrote:
> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> > > > > On 03/24/2016 07:47 AM, David Turner wrote:
> > > > > > [...]
> > > > > > I incorporated your changes into the lmdb backend.  To make
> > > > > > merging
> > > > > > later more convenient, I rebased on top of pu -- I think
> > > > > > this
> > > > > > mainly
> > > > > > depends on jk/check-repository-format, but I also included
> > > > > > some
> > > > > > fixes
> > > > > > for a couple of tests that had been changed by other
> > > > > > patches.
> > > > > 
> > > > > I think rebasing changes on top of pu is counterproductive. I
> > > > > believe
> > > > > that Junio had extra work rebasing your earlier series onto a
> > > > > merge
> > > > > of
> > > > > the minimum number of topics that it really depended on.
> > > > > There is
> > > > > no
> > > > > way
> > > > > that he could merge the branch in this form because it would
> > > > > imply
> > > > > merging all of pu.
> > > > > 
> > > > > See the zeroth section of SubmittingPatches [1] for the
> > > > > guidelines.
> > > > 
> > > > I'm a bit confused because 
> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag
> > > > variable
> > > > 
> > > > doesn't do anything on master -- it depends on some patch in
> > > > pu. 
> > > >  And
> > > > we definitely want to pick up jk/check-repository-format (which
> > > > doesn't
> > > > include whatever 18/21 depends on).
> > > > 
> > > > So what do you think our base should be?
> > > 
> > > I think the preference is to base a patch series on the merge of
> > > master
> > > plus the minimum number of topics in pu (ideally, none) that are
> > > "essential" prerequisites of the changes in the patch series. For
> > > example, the version of this patch series that Junio has in his
> > > tree
> > > was
> > > based on master + sb/submodule-parallel-update. 
> > > 
> > > Even if there are minor
> > > conflicts with another in-flight topic, it is easier for Junio to
> > > resolve the conflicts when merging the topics together than to
> > > rebase
> > > the patch series over and over as the other patch series evolves.
> > > The
> > > goal of this practice is of course to allow patch series to
> > > evolve
> > > independently of each other as much as possible.
> > > 
> > > Of course if you have insights into nontrivial conflicts between
> > > your
> > > patch series and others, it would be helpful to discuss these in
> > > your
> > > cover letter.
> > 
> > If I am reading this correctly, it looks like your series also has
> > a
> > few more sb submodule patches, e.g. sb/submodule-init, which is
> > responsible for the code that 18/21 depends on.  
> > 
> > I think jk/check-repository-format is also  good to get in first,
> > because it changes the startup sequence a bit and it's a bit tricky
> > to
> > figure out what needs to change in dt/refs-backend-lmdb as a result
> > of
> > it. 
> > 
> > But I can't just merge jk/check-repository-format on top of
> > 71defe0047 
> > -- some function signatures have changed in the run-command stuff
> > and
> > it seems kind of annoying to fix up.  
> > 
> > So I propose instead that we just drop 18/21 for now, and use just
> > jk/check-repository-format as the base.
> > 
> > Does this seem reasonable to you?
> 
> Yes, that's fine. Patch 18/21 is just a random cleanup that nothing
> else
> depends on. Will you do the rebasing? If so, please let me know where
> I
> can fetch the result from.

Rebased:

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-03-30 20:05         ` David Turner
  2016-03-31 16:14           ` Michael Haggerty
@ 2016-04-01  1:37           ` Stefan Beller
  2016-04-01 17:55             ` David Turner
  1 sibling, 1 reply; 31+ messages in thread
From: Stefan Beller @ 2016-04-01  1:37 UTC (permalink / raw)
  To: David Turner
  Cc: Michael Haggerty, Junio C Hamano, git, Jeff King, Duy Nguyen,
	Ramsay Jones

On Wed, Mar 30, 2016 at 1:05 PM, David Turner <dturner@twopensource.com> wrote:
> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
>> On 03/29/2016 10:12 PM, David Turner wrote:
>> > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
>> > > On 03/24/2016 07:47 AM, David Turner wrote:
>> > > > [...]
>> > > > I incorporated your changes into the lmdb backend.  To make
>> > > > merging
>> > > > later more convenient, I rebased on top of pu -- I think this
>> > > > mainly
>> > > > depends on jk/check-repository-format, but I also included some
>> > > > fixes
>> > > > for a couple of tests that had been changed by other patches.
>> > >
>> > > I think rebasing changes on top of pu is counterproductive. I
>> > > believe
>> > > that Junio had extra work rebasing your earlier series onto a
>> > > merge
>> > > of
>> > > the minimum number of topics that it really depended on. There is
>> > > no
>> > > way
>> > > that he could merge the branch in this form because it would
>> > > imply
>> > > merging all of pu.
>> > >
>> > > See the zeroth section of SubmittingPatches [1] for the
>> > > guidelines.
>> >
>> > I'm a bit confused because
>> > [PATCH 18/21] get_default_remote(): remove unneeded flag variable
>> >
>> > doesn't do anything on master -- it depends on some patch in pu.
>> >  And
>> > we definitely want to pick up jk/check-repository-format (which
>> > doesn't
>> > include whatever 18/21 depends on).
>> >
>> > So what do you think our base should be?
>>
>> I think the preference is to base a patch series on the merge of
>> master
>> plus the minimum number of topics in pu (ideally, none) that are
>> "essential" prerequisites of the changes in the patch series. For
>> example, the version of this patch series that Junio has in his tree
>> was
>> based on master + sb/submodule-parallel-update.
>>
>> Even if there are minor
>> conflicts with another in-flight topic, it is easier for Junio to
>> resolve the conflicts when merging the topics together than to rebase
>> the patch series over and over as the other patch series evolves. The
>> goal of this practice is of course to allow patch series to evolve
>> independently of each other as much as possible.
>>
>> Of course if you have insights into nontrivial conflicts between your
>> patch series and others, it would be helpful to discuss these in your
>> cover letter.
>
> If I am reading this correctly, it looks like your series also has a
> few more sb submodule patches, e.g. sb/submodule-init, which is
> responsible for the code that 18/21 depends on.
>
> I think jk/check-repository-format is also  good to get in first,
> because it changes the startup sequence a bit and it's a bit tricky to
> figure out what needs to change in dt/refs-backend-lmdb as a result of
> it.
>
> But I can't just merge jk/check-repository-format on top of 71defe0047
> -- some function signatures have changed in the run-command stuff and
> it seems kind of annoying to fix up.
>
> So I propose instead that we just drop 18/21 for now, and use just
> jk/check-repository-format as the base.

By 18/21 you mean
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
in builtin/submodule--helper.c?
You could drop that and I'll pick it up in one of the submodule series',
if that is more convenient for you.


>
> Does this seem reasonable to you?
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33
  2016-04-01  1:37           ` Stefan Beller
@ 2016-04-01 17:55             ` David Turner
  0 siblings, 0 replies; 31+ messages in thread
From: David Turner @ 2016-04-01 17:55 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Michael Haggerty, Junio C Hamano, git, Jeff King, Duy Nguyen,
	Ramsay Jones

On Thu, 2016-03-31 at 18:37 -0700, Stefan Beller wrote:
> On Wed, Mar 30, 2016 at 1:05 PM, David Turner <
> dturner@twopensource.com> wrote:
> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
> > > On 03/29/2016 10:12 PM, David Turner wrote:
> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> > > > > On 03/24/2016 07:47 AM, David Turner wrote:
> > > > > > [...]
> > > > > > I incorporated your changes into the lmdb backend.  To make
> > > > > > merging
> > > > > > later more convenient, I rebased on top of pu -- I think
> > > > > > this
> > > > > > mainly
> > > > > > depends on jk/check-repository-format, but I also included
> > > > > > some
> > > > > > fixes
> > > > > > for a couple of tests that had been changed by other
> > > > > > patches.
> > > > > 
> > > > > I think rebasing changes on top of pu is counterproductive. I
> > > > > believe
> > > > > that Junio had extra work rebasing your earlier series onto a
> > > > > merge
> > > > > of
> > > > > the minimum number of topics that it really depended on.
> > > > > There is
> > > > > no
> > > > > way
> > > > > that he could merge the branch in this form because it would
> > > > > imply
> > > > > merging all of pu.
> > > > > 
> > > > > See the zeroth section of SubmittingPatches [1] for the
> > > > > guidelines.
> > > > 
> > > > I'm a bit confused because
> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag
> > > > variable
> > > > 
> > > > doesn't do anything on master -- it depends on some patch in
> > > > pu.
> > > >  And
> > > > we definitely want to pick up jk/check-repository-format (which
> > > > doesn't
> > > > include whatever 18/21 depends on).
> > > > 
> > > > So what do you think our base should be?
> > > 
> > > I think the preference is to base a patch series on the merge of
> > > master
> > > plus the minimum number of topics in pu (ideally, none) that are
> > > "essential" prerequisites of the changes in the patch series. For
> > > example, the version of this patch series that Junio has in his
> > > tree
> > > was
> > > based on master + sb/submodule-parallel-update.
> > > 
> > > Even if there are minor
> > > conflicts with another in-flight topic, it is easier for Junio to
> > > resolve the conflicts when merging the topics together than to
> > > rebase
> > > the patch series over and over as the other patch series evolves.
> > > The
> > > goal of this practice is of course to allow patch series to
> > > evolve
> > > independently of each other as much as possible.
> > > 
> > > Of course if you have insights into nontrivial conflicts between
> > > your
> > > patch series and others, it would be helpful to discuss these in
> > > your
> > > cover letter.
> > 
> > If I am reading this correctly, it looks like your series also has
> > a
> > few more sb submodule patches, e.g. sb/submodule-init, which is
> > responsible for the code that 18/21 depends on.
> > 
> > I think jk/check-repository-format is also  good to get in first,
> > because it changes the startup sequence a bit and it's a bit tricky
> > to
> > figure out what needs to change in dt/refs-backend-lmdb as a result
> > of
> > it.
> > 
> > But I can't just merge jk/check-repository-format on top of
> > 71defe0047
> > -- some function signatures have changed in the run-command stuff
> > and
> > it seems kind of annoying to fix up.
> > 
> > So I propose instead that we just drop 18/21 for now, and use just
> > jk/check-repository-format as the base.
> 
> By 18/21 you mean
> [PATCH 18/21] get_default_remote(): remove unneeded flag variable
> in builtin/submodule--helper.c?
> You could drop that and I'll pick it up in one of the submodule
> series',
> if that is more convenient for you.
> 


Yes, thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2016-04-01 17:55 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-23 10:04 [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 Michael Haggerty
2016-03-23 10:04 ` [PATCH 01/21] t1430: test the output and error of some commands more carefully Michael Haggerty
2016-03-23 10:04 ` [PATCH 02/21] t1430: clean up broken refs/tags/shadow Michael Haggerty
2016-03-23 10:04 ` [PATCH 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs Michael Haggerty
2016-03-23 10:04 ` [PATCH 04/21] t1430: test for-each-ref in the presence of badly-named refs Michael Haggerty
2016-03-23 10:04 ` [PATCH 05/21] t1430: improve test coverage of deletion " Michael Haggerty
2016-03-23 10:04 ` [PATCH 06/21] resolve_missing_loose_ref(): simplify semantics Michael Haggerty
2016-03-23 10:04 ` [PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH Michael Haggerty
2016-03-23 10:04 ` [PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set Michael Haggerty
2016-03-23 10:04 ` [PATCH 09/21] resolve_ref_1(): eliminate local variable Michael Haggerty
2016-03-23 10:04 ` [PATCH 10/21] resolve_ref_1(): reorder code Michael Haggerty
2016-03-23 10:04 ` [PATCH 11/21] resolve_ref_1(): eliminate local variable "bad_name" Michael Haggerty
2016-03-23 10:04 ` [PATCH 12/21] files-backend: break out ref reading Michael Haggerty
2016-03-23 10:04 ` [PATCH 13/21] read_raw_ref(): manage own scratch space Michael Haggerty
2016-03-23 10:04 ` [PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe() Michael Haggerty
2016-03-23 10:04 ` [PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int Michael Haggerty
2016-03-23 10:04 ` [PATCH 16/21] fsck_head_link(): remove unneeded flag variable Michael Haggerty
2016-03-23 10:04 ` [PATCH 17/21] cmd_merge(): " Michael Haggerty
2016-03-23 10:04 ` [PATCH 18/21] get_default_remote(): " Michael Haggerty
2016-03-23 10:04 ` [PATCH 19/21] checkout_paths(): " Michael Haggerty
2016-03-23 10:04 ` [PATCH 20/21] check_aliased_update(): check that dst_name is non-NULL Michael Haggerty
2016-03-23 10:04 ` [PATCH 21/21] show_head_ref(): check the result of resolve_ref_namespace() Michael Haggerty
2016-03-24  6:47 ` [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33 David Turner
2016-03-27  5:22   ` Michael Haggerty
2016-03-29 20:12     ` David Turner
2016-03-30  6:37       ` Michael Haggerty
2016-03-30 20:05         ` David Turner
2016-03-31 16:14           ` Michael Haggerty
2016-03-31 22:22             ` David Turner
2016-04-01  1:37           ` Stefan Beller
2016-04-01 17:55             ` David Turner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.