threads / patch / 41792

patch, 21 partst1430: improve test coverage of deletion of badly-named refs

Subject: [PATCH 05/21] t1430: improve test coverage of deletion of badly-named refs

## tl;dr

31 messages between Mar 23, 2016 and Apr 1, 2016. Diffs are folded; open one to read it.

replies: 30people: 3as markdown or json

Michael Haggerty· Mar 23, 2016, 10:04 UTC · lore

[PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
Show changes to t/t1430-bad-ref-name.sh +12 −6
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 02/21] t1430: clean up broken refs/tags/shadow

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t1430-bad-ref-name.sh +1 −1
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to t/t1430-bad-ref-name.sh +2 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

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

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t1430-bad-ref-name.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Show changes to t/t1430-bad-ref-name.sh +16 −0
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore
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(-)
Show changes to t/t1430-bad-ref-name.sh +98 −10
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 06/21] resolve_missing_loose_ref(): simplify semantics

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(-)
Show changes to refs/files-backend.c +10 −14
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH

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(-)
Show changes to refs/files-backend.c +6 −7
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set

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(-)
Show changes to refs/files-backend.c +13 −18
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 09/21] resolve_ref_1(): eliminate local variable

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(-)
Show changes to refs/files-backend.c +6 −7
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 10/21] resolve_ref_1(): reorder code

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(-)
Show changes to refs/files-backend.c +2 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 11/21] resolve_ref_1(): eliminate local variable "bad_name"

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(-)
Show changes to refs/files-backend.c +5 −8
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 12/21] files-backend: break out ref reading

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(-)
Show changes to refs/files-backend.c +145 −99
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 13/21] read_raw_ref(): manage own scratch space

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(-)
Show changes to refs/files-backend.c +41 −35
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe()

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(-)
Show changes to refs/files-backend.c +9 −22
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int

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(-)
Show changes to refs/files-backend.c +3 −3
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 16/21] fsck_head_link(): remove unneeded flag variable

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(-)
Show changes to builtin/fsck.c +1 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 17/21] cmd_merge(): remove unneeded flag variable

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(-)
Show changes to builtin/merge.c +2 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 18/21] get_default_remote(): remove unneeded flag variable

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(-)
Show changes to builtin/submodule--helper.c +1 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

[PATCH 19/21] checkout_paths(): remove unneeded flag variable

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(-)
Show changes to builtin/checkout.c +1 −2
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to builtin/receive-pack.c +1 −1
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
Michael Haggerty· Mar 23, 2016, 10:04 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to http-backend.c +2 −2
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
David Turner· Mar 24, 2016, 06:47 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

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.

Michael Haggerty· Mar 27, 2016, 05:22 UTC · re: David Turner · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On 03/24/2016 07:47 AM, David Turner wrote:
Show 5 quoted lines
> [...]
> 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.
Show 12 quoted lines
> 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
David Turner· Mar 29, 2016, 20:12 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
Show 18 quoted lines
> 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?
Michael Haggerty· Mar 30, 2016, 06:37 UTC · re: David Turner · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On 03/29/2016 10:12 PM, David Turner wrote:
Show 28 quoted lines
> 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
David Turner· Mar 30, 2016, 20:05 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
Show 56 quoted lines
> 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?
Michael Haggerty· Mar 31, 2016, 16:14 UTC · re: David Turner · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On 03/30/2016 10:05 PM, David Turner wrote:
Show 75 quoted lines
> 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
David Turner· Mar 31, 2016, 22:22 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On Thu, 2016-03-31 at 18:14 +0200, Michael Haggerty wrote:
Show 99 quoted lines
> 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

Stefan Beller· Apr 1, 2016, 01:37 UTC · re: David Turner · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On Wed, Mar 30, 2016 at 1:05 PM, David Turner <dturner@twopensource.com> wrote:
Show 73 quoted lines
> 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.

Show 6 quoted lines
>
> 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
David Turner· Apr 1, 2016, 17:55 UTC · re: Stefan Beller · lore

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

On Thu, 2016-03-31 at 18:37 -0700, Stefan Beller wrote:
Show 100 quoted lines
> 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.

← back to recent threads