{"thread":{"id":"41792","subject":"[PATCH 05/21] t1430: improve test coverage of deletion of badly-named refs","startedAt":"2016-03-23T10:04:17Z","lastAt":"2016-04-01T17:55:14Z","messageCount":31,"participants":["Michael Haggerty","David Turner","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":21},"messages":[{"id":"281556","messageId":"cover.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":null,"subject":"[PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:17Z","receivedAt":"2016-03-23T10:04:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way\ntoo much in a single patch, and in fact got a few minor things wrong.\nInstead of that patch, I suggest this patch series, which\n\n* Splits the changes into smaller steps.\n\n* Adds a bunch of tests of deleting references with invalid but safe\n names, including symbolic references and including references\n reached via symbolic references. Two of these tests fail when run\n against David's patch 04 due to changes in output.\n\n* Arranges for the \"flags\" argument to read_raw_ref() always to be\n non-NULL, which eliminates the need for a lot of \"if (flags)\"\n guards.\n\n* Eliminates the now-superfluous \"bad_name\" local variable.\n\n* Move the management of the scratch space sb_path from\n resolve_ref_unsafe() to read_raw_ref().\n\n* Inlines resolve_ref_1() into resolve_ref_unsafe().\n\n* Changes some callers of resolve_ref_unsafe() to pass flags=NULL\n instead of creating a local flags variable that is never used.\n\n* Changes some callers to check for errors before using the return\n value of resolve_ref_unsafe().\n\nI hope that the result is easier to understand and audit, even though\nit consists of more patches (indeed, *because* of that).\n\nThis patch series applies on top of David's patch 03/33 the same place\nDavid applied it in his repo [2]. It is also available in situ from my\nGitHub repo [3] as branch \"pluggable-backends-patch4\"\n\nIf this series is used, later patches from David's series would need\nto be rebased on top of it. This is a little bit messy but not\ndifficult; the result can be seen in branch\n\"pluggable-backends-rebased\" in my GitHub repo [3] (albeit without\nadjusting the LMDB-related patches).\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/287971\n[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends\n[3] https://github.com/mhagger/git\n\nDavid Turner (1):\n files-backend: break out ref reading\n\nMichael Haggerty (20):\n t1430: test the output and error of some commands more carefully\n t1430: clean up broken refs/tags/shadow\n t1430: don't rely on symbolic-ref for creating broken symrefs\n t1430: test for-each-ref in the presence of badly-named refs\n t1430: improve test coverage of deletion of badly-named refs\n resolve_missing_loose_ref(): simplify semantics\n resolve_ref_unsafe(): use for loop to count up to MAXDEPTH\n resolve_ref_unsafe(): ensure flags is always set\n resolve_ref_1(): eliminate local variable\n resolve_ref_1(): reorder code\n resolve_ref_1(): eliminate local variable \"bad_name\"\n read_raw_ref(): manage own scratch space\n Inline resolve_ref_1() into resolve_ref_unsafe()\n read_raw_ref(): change flags parameter to unsigned int\n fsck_head_link(): remove unneeded flag variable\n cmd_merge(): remove unneeded flag variable\n get_default_remote(): remove unneeded flag variable\n checkout_paths(): remove unneeded flag variable\n check_aliased_update(): check that dst_name is non-NULL\n show_head_ref(): check the result of resolve_ref_namespace()\n\n builtin/checkout.c | 3 +-\n builtin/fsck.c | 3 +-\n builtin/merge.c | 4 +-\n builtin/receive-pack.c | 2 +-\n builtin/submodule--helper.c | 3 +-\n http-backend.c | 4 +-\n refs/files-backend.c | 341 ++++++++++++++++++++++++--------------------\n t/t1430-bad-ref-name.sh | 132 +++++++++++++++--\n 8 files changed, 312 insertions(+), 180 deletions(-)\n\n-- \n2.8.0.rc3\n"},{"id":"281543","messageId":"b7e3b3bfbd8b4db7c3a76ecf7dcf8466a302c063.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 01/21] t1430: test the output and error of some commands more carefully","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:18Z","receivedAt":"2016-03-23T10:04:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n t/t1430-bad-ref-name.sh | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex c465abe..005e2b1 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -42,7 +42,7 @@ test_expect_success 'git branch shows badly named ref as warning' '\n \tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n \ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n \tgit branch >output 2>error &&\n-\tgrep -e \"broken\\.\\.\\.ref\" error &&\n+\ttest_i18ngrep -e \"ignoring ref with broken name refs/heads/broken\\.\\.\\.ref\" error &&\n \t! grep -e \"broken\\.\\.\\.ref\" output\n '\n \n@@ -152,21 +152,25 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '\n \tgit rev-parse --verify one >expect &&\n \tgit rev-parse --verify shadow >actual 2>err &&\n \ttest_cmp expect actual &&\n-\ttest_i18ngrep \"ignoring.*refs/tags/shadow\" err\n+\ttest_i18ngrep \"ignoring dangling symref refs/tags/shadow\" err\n '\n \n test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '\n \tgit symbolic-ref refs/heads/badname refs/heads/broken...ref &&\n \ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n \ttest_path_is_file .git/refs/heads/badname &&\n-\tgit update-ref --no-deref -d refs/heads/badname &&\n-\ttest_path_is_missing .git/refs/heads/badname\n+\tgit update-ref --no-deref -d refs/heads/badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/badname &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n '\n \n test_expect_success 'update-ref -d can delete broken name' '\n \tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n \ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n-\tgit update-ref -d refs/heads/broken...ref &&\n+\tgit update-ref -d refs/heads/broken...ref >output 2>error &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error &&\n \tgit branch >output 2>error &&\n \t! grep -e \"broken\\.\\.\\.ref\" error &&\n \t! grep -e \"broken\\.\\.\\.ref\" output\n@@ -175,7 +179,9 @@ test_expect_success 'update-ref -d can delete broken name' '\n test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '\n \techo precious >.git/my-private-file &&\n \techo precious >expect &&\n-\ttest_must_fail git update-ref -d my-private-file &&\n+\ttest_must_fail git update-ref -d my-private-file >output 2>error &&\n+\ttest_must_be_empty output &&\n+\ttest_i18ngrep -e \"cannot lock .*: unable to resolve reference\" error &&\n \ttest_cmp expect .git/my-private-file\n '\n \n-- \n2.8.0.rc3\n"},{"id":"281540","messageId":"64ee96b13ea182ef851c809d4abd1fc292bd12b2.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 02/21] t1430: clean up broken refs/tags/shadow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:19Z","receivedAt":"2016-03-23T10:04:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n t/t1430-bad-ref-name.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex 005e2b1..cb815ab 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -148,7 +148,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '\n \tgit branch shadow one &&\n \tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n \tgit symbolic-ref refs/tags/shadow refs/heads/broken...ref &&\n-\n+\ttest_when_finished \"rm -f .git/refs/tags/shadow\" &&\n \tgit rev-parse --verify one >expect &&\n \tgit rev-parse --verify shadow >actual 2>err &&\n \ttest_cmp expect actual &&\n-- \n2.8.0.rc3\n"},{"id":"281551","messageId":"766a2dc4a13b4e2f98a256455d35f1aade548be9.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 03/21] t1430: don't rely on symbolic-ref for creating broken symrefs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:20Z","receivedAt":"2016-03-23T10:04:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's questionable whether it should even work.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1430-bad-ref-name.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex cb815ab..a963951 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -147,7 +147,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '\n \ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n \tgit branch shadow one &&\n \tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n-\tgit symbolic-ref refs/tags/shadow refs/heads/broken...ref &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/tags/shadow &&\n \ttest_when_finished \"rm -f .git/refs/tags/shadow\" &&\n \tgit rev-parse --verify one >expect &&\n \tgit rev-parse --verify shadow >actual 2>err &&\n@@ -156,7 +156,7 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '\n '\n \n test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '\n-\tgit symbolic-ref refs/heads/badname refs/heads/broken...ref &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n \ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n \ttest_path_is_file .git/refs/heads/badname &&\n \tgit update-ref --no-deref -d refs/heads/badname >output 2>error &&\n-- \n2.8.0.rc3\n"},{"id":"281549","messageId":"ec680e35e10657ba1ba17e561ae06f1851b10a0b.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 04/21] t1430: test for-each-ref in the presence of badly-named refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:21Z","receivedAt":"2016-03-23T10:04:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n t/t1430-bad-ref-name.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex a963951..612cc32 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -155,6 +155,22 @@ test_expect_success 'rev-parse skips symref pointing to broken name' '\n \ttest_i18ngrep \"ignoring dangling symref refs/tags/shadow\" err\n '\n \n+test_expect_success 'for-each-ref emits warnings for broken names' '\n+\tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tprintf \"ref: refs/heads/master\\n\" >.git/refs/heads/broken...symref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...symref\" &&\n+\tgit for-each-ref >output 2>error &&\n+\t! grep -e \"broken\\.\\.\\.ref\" output &&\n+\t! grep -e \"badname\" output &&\n+\t! grep -e \"broken\\.\\.\\.symref\" output &&\n+\ttest_i18ngrep \"ignoring ref with broken name refs/heads/broken\\.\\.\\.ref\" error &&\n+\ttest_i18ngrep \"ignoring broken ref refs/heads/badname\" error &&\n+\ttest_i18ngrep \"ignoring ref with broken name refs/heads/broken\\.\\.\\.symref\" error\n+'\n+\n test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '\n \tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n \ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n-- \n2.8.0.rc3\n"},{"id":"281533","messageId":"faabd92da429867a767599e6ce5a9f4832481d42.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 05/21] t1430: improve test coverage of deletion of badly-named refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:22Z","receivedAt":"2016-03-23T10:04:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Check \"branch -d broken...ref\"\n\nCheck various combinations of\n\n* Deleting using \"update-ref -d\"\n* Deleting using \"update-ref --no-deref -d\"\n* Deleting using \"branch -d\"\n\nin the following combinations of symref -> ref:\n\n* badname -> broken...ref\n* badname -> broken...ref (dangling)\n* broken...symref -> master\n* broken...symref -> idonotexist (dangling)\n\nSigned-off-by: Michael Haggerty \n---\n t/t1430-bad-ref-name.sh | 108 +++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 98 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex 612cc32..25ddab4 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -171,16 +171,6 @@ test_expect_success 'for-each-ref emits warnings for broken names' '\n \ttest_i18ngrep \"ignoring ref with broken name refs/heads/broken\\.\\.\\.symref\" error\n '\n \n-test_expect_success 'update-ref --no-deref -d can delete reference to broken name' '\n-\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n-\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n-\ttest_path_is_file .git/refs/heads/badname &&\n-\tgit update-ref --no-deref -d refs/heads/badname >output 2>error &&\n-\ttest_path_is_missing .git/refs/heads/badname &&\n-\ttest_must_be_empty output &&\n-\ttest_must_be_empty error\n-'\n-\n test_expect_success 'update-ref -d can delete broken name' '\n \tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n \ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n@@ -192,6 +182,104 @@ test_expect_success 'update-ref -d can delete broken name' '\n \t! grep -e \"broken\\.\\.\\.ref\" output\n '\n \n+test_expect_success 'branch -d can delete broken name' '\n+\tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n+\tgit branch -d broken...ref >output 2>error &&\n+\ttest_i18ngrep \"Deleted branch broken...ref (was broken)\" output &&\n+\ttest_must_be_empty error &&\n+\tgit branch >output 2>error &&\n+\t! grep -e \"broken\\.\\.\\.ref\" error &&\n+\t! grep -e \"broken\\.\\.\\.ref\" output\n+'\n+\n+test_expect_success 'update-ref --no-deref -d can delete symref to broken name' '\n+\tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tgit update-ref --no-deref -d refs/heads/badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/badname &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'branch -d can delete symref to broken name' '\n+\tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tgit branch -d badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/badname &&\n+\ttest_i18ngrep \"Deleted branch badname (was refs/heads/broken\\.\\.\\.ref)\" output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tgit update-ref --no-deref -d refs/heads/badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/badname &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'branch -d can delete dangling symref to broken name' '\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tgit branch -d badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/badname &&\n+\ttest_i18ngrep \"Deleted branch badname (was refs/heads/broken\\.\\.\\.ref)\" output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'update-ref -d can delete broken name through symref' '\n+\tcp .git/refs/heads/master .git/refs/heads/broken...ref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...ref\" &&\n+\tprintf \"ref: refs/heads/broken...ref\\n\" >.git/refs/heads/badname &&\n+\ttest_when_finished \"rm -f .git/refs/heads/badname\" &&\n+\tgit update-ref -d refs/heads/badname >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/broken...ref &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '\n+\tprintf \"ref: refs/heads/master\\n\" >.git/refs/heads/broken...symref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...symref\" &&\n+\tgit update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/broken...symref &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'branch -d can delete symref with broken name' '\n+\tprintf \"ref: refs/heads/master\\n\" >.git/refs/heads/broken...symref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...symref\" &&\n+\tgit branch -d broken...symref >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/broken...symref &&\n+\ttest_i18ngrep \"Deleted branch broken...symref (was refs/heads/master)\" output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '\n+\tprintf \"ref: refs/heads/idonotexist\\n\" >.git/refs/heads/broken...symref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...symref\" &&\n+\tgit update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/broken...symref &&\n+\ttest_must_be_empty output &&\n+\ttest_must_be_empty error\n+'\n+\n+test_expect_success 'branch -d can delete dangling symref with broken name' '\n+\tprintf \"ref: refs/heads/idonotexist\\n\" >.git/refs/heads/broken...symref &&\n+\ttest_when_finished \"rm -f .git/refs/heads/broken...symref\" &&\n+\tgit branch -d broken...symref >output 2>error &&\n+\ttest_path_is_missing .git/refs/heads/broken...symref &&\n+\ttest_i18ngrep \"Deleted branch broken...symref (was refs/heads/idonotexist)\" output &&\n+\ttest_must_be_empty error\n+'\n+\n test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '\n \techo precious >.git/my-private-file &&\n \techo precious >expect &&\n-- \n2.8.0.rc3\n"},{"id":"281537","messageId":"d40fad11431993f44a53e845f21af3a1add6f7d0.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 06/21] resolve_missing_loose_ref(): simplify semantics","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:23Z","receivedAt":"2016-03-23T10:04:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make resolve_missing_loose_ref() only responsible for looking up a\npacked reference, without worrying about whether we want to read or\nwrite the reference and without setting errno on failure. Move the other\nlogic to the caller.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 24 ++++++++++--------------\n 1 file changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9676ec2..c0cf6fd 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1368,11 +1368,9 @@ static struct ref_entry *get_packed_ref(const char *refname)\n }\n \n /*\n- * A loose ref file doesn't exist; check for a packed ref. The\n- * options are forwarded from resolve_safe_unsafe().\n+ * A loose ref file doesn't exist; check for a packed ref.\n */\n static int resolve_missing_loose_ref(const char *refname,\n-\t\t\t\t int resolve_flags,\n \t\t\t\t unsigned char *sha1,\n \t\t\t\t int *flags)\n {\n@@ -1389,14 +1387,8 @@ static int resolve_missing_loose_ref(const char *refname,\n \t\t\t*flags |= REF_ISPACKED;\n \t\treturn 0;\n \t}\n-\t/* The reference is not a packed reference, either. */\n-\tif (resolve_flags & RESOLVE_REF_READING) {\n-\t\terrno = ENOENT;\n-\t\treturn -1;\n-\t} else {\n-\t\thashclr(sha1);\n-\t\treturn 0;\n-\t}\n+\t/* refname is not a packed reference. */\n+\treturn -1;\n }\n \n /* This function needs to return a meaningful errno on failure */\n@@ -1461,9 +1453,13 @@ static const char *resolve_ref_1(const char *refname,\n \t\tif (lstat(path, &st) < 0) {\n \t\t\tif (errno != ENOENT)\n \t\t\t\treturn NULL;\n-\t\t\tif (resolve_missing_loose_ref(refname, resolve_flags,\n-\t\t\t\t\t\t sha1, flags))\n-\t\t\t\treturn NULL;\n+\t\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n+\t\t\t\tif (resolve_flags & RESOLVE_REF_READING) {\n+\t\t\t\t\terrno = ENOENT;\n+\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n+\t\t\t\thashclr(sha1);\n+\t\t\t}\n \t\t\tif (bad_name) {\n \t\t\t\thashclr(sha1);\n \t\t\t\tif (flags)\n-- \n2.8.0.rc3\n"},{"id":"281547","messageId":"418fbb7db3d1804dd1a4d6abc723dd59bb95ea2f.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 07/21] resolve_ref_unsafe(): use for loop to count up to MAXDEPTH","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:24Z","receivedAt":"2016-03-23T10:04:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The loop's there anyway; we might as well use it.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c0cf6fd..101abba 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1400,8 +1400,8 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\t struct strbuf *sb_path,\n \t\t\t\t struct strbuf *sb_contents)\n {\n-\tint depth = MAXDEPTH;\n \tint bad_name = 0;\n+\tint symref_count;\n \n \tif (flags)\n \t\t*flags = 0;\n@@ -1425,17 +1425,13 @@ static const char *resolve_ref_1(const char *refname,\n \t\t */\n \t\tbad_name = 1;\n \t}\n-\tfor (;;) {\n+\n+\tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n \t\tconst char *path;\n \t\tstruct stat st;\n \t\tchar *buf;\n \t\tint fd;\n \n-\t\tif (--depth < 0) {\n-\t\t\terrno = ELOOP;\n-\t\t\treturn NULL;\n-\t\t}\n-\n \t\tstrbuf_reset(sb_path);\n \t\tstrbuf_git_path(sb_path, \"%s\", refname);\n \t\tpath = sb_path->buf;\n@@ -1566,6 +1562,9 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\tbad_name = 1;\n \t\t}\n \t}\n+\n+\terrno = ELOOP;\n+\treturn NULL;\n }\n \n const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n-- \n2.8.0.rc3\n"},{"id":"281559","messageId":"5e6741bb89307d74231c70079c6fbf1efa1af1be.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 08/21] resolve_ref_unsafe(): ensure flags is always set","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:25Z","receivedAt":"2016-03-23T10:04:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the caller passes flags==NULL, then set it to point at a local\nscratch variable. This removes the need for a lot of \"if (flags)\" guards\nin resolve_ref_1() and resolve_missing_loose_ref().\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 31 +++++++++++++------------------\n 1 file changed, 13 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 101abba..067ce1c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1383,8 +1383,7 @@ static int resolve_missing_loose_ref(const char *refname,\n \tentry = get_packed_ref(refname);\n \tif (entry) {\n \t\thashcpy(sha1, entry->u.value.oid.hash);\n-\t\tif (flags)\n-\t\t\t*flags |= REF_ISPACKED;\n+\t\t*flags |= REF_ISPACKED;\n \t\treturn 0;\n \t}\n \t/* refname is not a packed reference. */\n@@ -1403,12 +1402,10 @@ static const char *resolve_ref_1(const char *refname,\n \tint bad_name = 0;\n \tint symref_count;\n \n-\tif (flags)\n-\t\t*flags = 0;\n+\t*flags = 0;\n \n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\tif (flags)\n-\t\t\t*flags |= REF_BAD_NAME;\n+\t\t*flags |= REF_BAD_NAME;\n \n \t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t !refname_is_safe(refname)) {\n@@ -1458,8 +1455,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t}\n \t\t\tif (bad_name) {\n \t\t\t\thashclr(sha1);\n-\t\t\t\tif (flags)\n-\t\t\t\t\t*flags |= REF_ISBROKEN;\n+\t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t}\n \t\t\treturn refname;\n \t\t}\n@@ -1478,8 +1474,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t !check_refname_format(sb_contents->buf, 0)) {\n \t\t\t\tstrbuf_swap(sb_refname, sb_contents);\n \t\t\t\trefname = sb_refname->buf;\n-\t\t\t\tif (flags)\n-\t\t\t\t\t*flags |= REF_ISSYMREF;\n+\t\t\t\t*flags |= REF_ISSYMREF;\n \t\t\t\tif (resolve_flags & RESOLVE_REF_NO_RECURSE) {\n \t\t\t\t\thashclr(sha1);\n \t\t\t\t\treturn refname;\n@@ -1526,20 +1521,17 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t */\n \t\t\tif (get_sha1_hex(sb_contents->buf, sha1) ||\n \t\t\t (sb_contents->buf[40] != '\\0' && !isspace(sb_contents->buf[40]))) {\n-\t\t\t\tif (flags)\n-\t\t\t\t\t*flags |= REF_ISBROKEN;\n+\t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\tif (bad_name) {\n \t\t\t\thashclr(sha1);\n-\t\t\t\tif (flags)\n-\t\t\t\t\t*flags |= REF_ISBROKEN;\n+\t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t}\n \t\t\treturn refname;\n \t\t}\n-\t\tif (flags)\n-\t\t\t*flags |= REF_ISSYMREF;\n+\t\t*flags |= REF_ISSYMREF;\n \t\tbuf = sb_contents->buf + 4;\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n@@ -1551,8 +1543,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\treturn refname;\n \t\t}\n \t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\tif (flags)\n-\t\t\t\t*flags |= REF_ISBROKEN;\n+\t\t\t*flags |= REF_ISBROKEN;\n \n \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t\t !refname_is_safe(buf)) {\n@@ -1573,8 +1564,12 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \tstatic struct strbuf sb_refname = STRBUF_INIT;\n \tstruct strbuf sb_contents = STRBUF_INIT;\n \tstruct strbuf sb_path = STRBUF_INIT;\n+\tint unused_flags;\n \tconst char *ret;\n \n+\tif (!flags)\n+\t\tflags = &unused_flags;\n+\n \tret = resolve_ref_1(refname, resolve_flags, sha1, flags,\n \t\t\t &sb_refname, &sb_path, &sb_contents);\n \tstrbuf_release(&sb_path);\n-- \n2.8.0.rc3\n"},{"id":"281550","messageId":"e9008ce41529e6692cb01825c3127a12eb959def.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 09/21] resolve_ref_1(): eliminate local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:26Z","receivedAt":"2016-03-23T10:04:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In place of `buf`, use `refname`, which is anyway a better description\nof what is being pointed at.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 067ce1c..69ec903 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1426,7 +1426,6 @@ static const char *resolve_ref_1(const char *refname,\n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n \t\tconst char *path;\n \t\tstruct stat st;\n-\t\tchar *buf;\n \t\tint fd;\n \n \t\tstrbuf_reset(sb_path);\n@@ -1532,21 +1531,21 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\treturn refname;\n \t\t}\n \t\t*flags |= REF_ISSYMREF;\n-\t\tbuf = sb_contents->buf + 4;\n-\t\twhile (isspace(*buf))\n-\t\t\tbuf++;\n+\t\trefname = sb_contents->buf + 4;\n+\t\twhile (isspace(*refname))\n+\t\t\trefname++;\n \t\tstrbuf_reset(sb_refname);\n-\t\tstrbuf_addstr(sb_refname, buf);\n+\t\tstrbuf_addstr(sb_refname, refname);\n \t\trefname = sb_refname->buf;\n \t\tif (resolve_flags & RESOLVE_REF_NO_RECURSE) {\n \t\t\thashclr(sha1);\n \t\t\treturn refname;\n \t\t}\n-\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\t\t*flags |= REF_ISBROKEN;\n \n \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n-\t\t\t !refname_is_safe(buf)) {\n+\t\t\t !refname_is_safe(refname)) {\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n-- \n2.8.0.rc3\n"},{"id":"281557","messageId":"5c86c4c5969701a5b8a098591cc4a9e522beb97c.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 10/21] resolve_ref_1(): reorder code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:27Z","receivedAt":"2016-03-23T10:04:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no need to adjust *flags if we're just about to fail.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 69ec903..60f1493 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1542,13 +1542,13 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\treturn refname;\n \t\t}\n \t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\t*flags |= REF_ISBROKEN;\n-\n \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t\t !refname_is_safe(refname)) {\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n+\n+\t\t\t*flags |= REF_ISBROKEN;\n \t\t\tbad_name = 1;\n \t\t}\n \t}\n-- \n2.8.0.rc3\n"},{"id":"281542","messageId":"2702bd2cb33c454eea064e21a7cedded0580ed63.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 11/21] resolve_ref_1(): eliminate local variable \"bad_name\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:28Z","receivedAt":"2016-03-23T10:04:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We can use (*flags & REF_BAD_NAME) for that purpose.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 60f1493..b865ba5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1399,19 +1399,17 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\t struct strbuf *sb_path,\n \t\t\t\t struct strbuf *sb_contents)\n {\n-\tint bad_name = 0;\n \tint symref_count;\n \n \t*flags = 0;\n \n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t*flags |= REF_BAD_NAME;\n-\n \t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t !refname_is_safe(refname)) {\n \t\t\terrno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n+\n \t\t/*\n \t\t * dwim_ref() uses REF_ISBROKEN to distinguish between\n \t\t * missing refs and refs that were present but invalid,\n@@ -1420,7 +1418,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t * We don't know whether the ref exists, so don't set\n \t\t * REF_ISBROKEN yet.\n \t\t */\n-\t\tbad_name = 1;\n+\t\t*flags |= REF_BAD_NAME;\n \t}\n \n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n@@ -1452,7 +1450,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\t}\n \t\t\t\thashclr(sha1);\n \t\t\t}\n-\t\t\tif (bad_name) {\n+\t\t\tif (*flags & REF_BAD_NAME) {\n \t\t\t\thashclr(sha1);\n \t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t}\n@@ -1524,7 +1522,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tif (bad_name) {\n+\t\t\tif (*flags & REF_BAD_NAME) {\n \t\t\t\thashclr(sha1);\n \t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t}\n@@ -1548,8 +1546,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\treturn NULL;\n \t\t\t}\n \n-\t\t\t*flags |= REF_ISBROKEN;\n-\t\t\tbad_name = 1;\n+\t\t\t*flags |= REF_ISBROKEN | REF_BAD_NAME;\n \t\t}\n \t}\n \n-- \n2.8.0.rc3\n"},{"id":"281534","messageId":"31684011a0fdadf5d4b9bf7cd7d384a103dfb719.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 12/21] files-backend: break out ref reading","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:29Z","receivedAt":"2016-03-23T10:04:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: David Turner \n\nRefactor resolve_ref_1 in terms of a new function read_raw_ref, which\nis responsible for reading ref data from the ref storage.\n\nLater, we will make read_raw_ref a pluggable backend function, and make\nresolve_ref_unsafe common.\n\nSigned-off-by: David Turner \nHelped-by: Duy Nguyen \nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 244 ++++++++++++++++++++++++++++++---------------------\n 1 file changed, 145 insertions(+), 99 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b865ba5..d51e778 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1390,6 +1390,141 @@ static int resolve_missing_loose_ref(const char *refname,\n \treturn -1;\n }\n \n+/*\n+ * Read a raw ref from the filesystem or packed refs file.\n+ *\n+ * If the ref is a sha1, fill in sha1 and return 0.\n+ *\n+ * If the ref is symbolic, fill in *symref with the referrent\n+ * (e.g. \"refs/heads/master\") and return 0. The caller is responsible\n+ * for validating the referrent. Set REF_ISSYMREF in flags.\n+ *\n+ * If the ref doesn't exist, set errno to ENOENT and return -1.\n+ *\n+ * If the ref exists but is neither a symbolic ref nor a sha1, it is\n+ * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return\n+ * -1.\n+ *\n+ * If there is another error reading the ref, set errno appropriately and\n+ * return -1.\n+ *\n+ * Backend-specific flags might be set in flags as well, regardless of\n+ * outcome.\n+ *\n+ * sb_path is workspace: the caller should allocate and free it.\n+ *\n+ * It is OK for refname to point into symref. In this case:\n+ * - if the function succeeds with REF_ISSYMREF, symref will be\n+ * overwritten and the memory pointed to by refname might be changed\n+ * or even freed.\n+ * - in all other cases, symref will be untouched, and therefore\n+ * refname will still be valid and unchanged.\n+ */\n+static int read_raw_ref(const char *refname, unsigned char *sha1,\n+\t\t\tstruct strbuf *symref, struct strbuf *sb_path,\n+\t\t\tstruct strbuf *sb_contents, int *flags)\n+{\n+\tconst char *path;\n+\tconst char *buf;\n+\tstruct stat st;\n+\tint fd;\n+\n+\tstrbuf_reset(sb_path);\n+\tstrbuf_git_path(sb_path, \"%s\", refname);\n+\tpath = sb_path->buf;\n+\n+stat_ref:\n+\t/*\n+\t * We might have to loop back here to avoid a race\n+\t * condition: first we lstat() the file, then we try\n+\t * to read it as a link or as a file. But if somebody\n+\t * changes the type of the file (file <-> directory\n+\t * <-> symlink) between the lstat() and reading, then\n+\t * we don't want to report that as an error but rather\n+\t * try again starting with the lstat().\n+\t */\n+\n+\tif (lstat(path, &st) < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\treturn -1;\n+\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n+\t\t\terrno = ENOENT;\n+\t\t\treturn -1;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n+\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tstrbuf_reset(sb_contents);\n+\t\tif (strbuf_readlink(sb_contents, path, 0) < 0) {\n+\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\tgoto stat_ref;\n+\t\t\telse\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tif (starts_with(sb_contents->buf, \"refs/\") &&\n+\t\t !check_refname_format(sb_contents->buf, 0)) {\n+\t\t\tstrbuf_swap(sb_contents, symref);\n+\t\t\t*flags |= REF_ISSYMREF;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\t/* Is it a directory? */\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terrno = EISDIR;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to use it as\n+\t * a ref\n+\t */\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\t/* inconsistent with lstat; retry */\n+\t\t\tgoto stat_ref;\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n+\tstrbuf_reset(sb_contents);\n+\tif (strbuf_read(sb_contents, fd, 256) < 0) {\n+\t\tint save_errno = errno;\n+\t\tclose(fd);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n+\tclose(fd);\n+\tstrbuf_rtrim(sb_contents);\n+\tbuf = sb_contents->buf;\n+\tif (starts_with(buf, \"ref:\")) {\n+\t\tbuf += 4;\n+\t\twhile (isspace(*buf))\n+\t\t\tbuf++;\n+\n+\t\tstrbuf_reset(symref);\n+\t\tstrbuf_addstr(symref, buf);\n+\t\t*flags |= REF_ISSYMREF;\n+\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Please note that FETCH_HEAD has additional\n+\t * data after the sha.\n+\t */\n+\tif (get_sha1_hex(buf, sha1) ||\n+\t (buf[40] != '\\0' && !isspace(buf[40]))) {\n+\t\t*flags |= REF_ISBROKEN;\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n /* This function needs to return a meaningful errno on failure */\n static const char *resolve_ref_1(const char *refname,\n \t\t\t\t int resolve_flags,\n@@ -1422,118 +1557,29 @@ static const char *resolve_ref_1(const char *refname,\n \t}\n \n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n-\t\tconst char *path;\n-\t\tstruct stat st;\n-\t\tint fd;\n+\t\tint read_flags = 0;\n \n-\t\tstrbuf_reset(sb_path);\n-\t\tstrbuf_git_path(sb_path, \"%s\", refname);\n-\t\tpath = sb_path->buf;\n-\n-\t\t/*\n-\t\t * We might have to loop back here to avoid a race\n-\t\t * condition: first we lstat() the file, then we try\n-\t\t * to read it as a link or as a file. But if somebody\n-\t\t * changes the type of the file (file <-> directory\n-\t\t * <-> symlink) between the lstat() and reading, then\n-\t\t * we don't want to report that as an error but rather\n-\t\t * try again starting with the lstat().\n-\t\t */\n-\tstat_ref:\n-\t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno != ENOENT)\n+\t\tif (read_raw_ref(refname, sha1, sb_refname,\n+\t\t\t\t sb_path, sb_contents, &read_flags)) {\n+\t\t\t*flags |= read_flags;\n+\t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n \t\t\t\treturn NULL;\n-\t\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n-\t\t\t\tif (resolve_flags & RESOLVE_REF_READING) {\n-\t\t\t\t\terrno = ENOENT;\n-\t\t\t\t\treturn NULL;\n-\t\t\t\t}\n-\t\t\t\thashclr(sha1);\n-\t\t\t}\n-\t\t\tif (*flags & REF_BAD_NAME) {\n-\t\t\t\thashclr(sha1);\n+\t\t\thashclr(sha1);\n+\t\t\tif (*flags & REF_BAD_NAME)\n \t\t\t\t*flags |= REF_ISBROKEN;\n-\t\t\t}\n \t\t\treturn refname;\n \t\t}\n \n-\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-\t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tstrbuf_reset(sb_contents);\n-\t\t\tif (strbuf_readlink(sb_contents, path, 0) < 0) {\n-\t\t\t\tif (errno == ENOENT || errno == EINVAL)\n-\t\t\t\t\t/* inconsistent with lstat; retry */\n-\t\t\t\t\tgoto stat_ref;\n-\t\t\t\telse\n-\t\t\t\t\treturn NULL;\n-\t\t\t}\n-\t\t\tif (starts_with(sb_contents->buf, \"refs/\") &&\n-\t\t\t !check_refname_format(sb_contents->buf, 0)) {\n-\t\t\t\tstrbuf_swap(sb_refname, sb_contents);\n-\t\t\t\trefname = sb_refname->buf;\n-\t\t\t\t*flags |= REF_ISSYMREF;\n-\t\t\t\tif (resolve_flags & RESOLVE_REF_NO_RECURSE) {\n-\t\t\t\t\thashclr(sha1);\n-\t\t\t\t\treturn refname;\n-\t\t\t\t}\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n+\t\t*flags |= read_flags;\n \n-\t\t/* Is it a directory? */\n-\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\terrno = EISDIR;\n-\t\t\treturn NULL;\n-\t\t}\n-\n-\t\t/*\n-\t\t * Anything else, just open it and try to use it as\n-\t\t * a ref\n-\t\t */\n-\t\tfd = open(path, O_RDONLY);\n-\t\tif (fd < 0) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\t/* inconsistent with lstat; retry */\n-\t\t\t\tgoto stat_ref;\n-\t\t\telse\n-\t\t\t\treturn NULL;\n-\t\t}\n-\t\tstrbuf_reset(sb_contents);\n-\t\tif (strbuf_read(sb_contents, fd, 256) < 0) {\n-\t\t\tint save_errno = errno;\n-\t\t\tclose(fd);\n-\t\t\terrno = save_errno;\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tclose(fd);\n-\t\tstrbuf_rtrim(sb_contents);\n-\n-\t\t/*\n-\t\t * Is it a symbolic ref?\n-\t\t */\n-\t\tif (!starts_with(sb_contents->buf, \"ref:\")) {\n-\t\t\t/*\n-\t\t\t * Please note that FETCH_HEAD has a second\n-\t\t\t * line containing other data.\n-\t\t\t */\n-\t\t\tif (get_sha1_hex(sb_contents->buf, sha1) ||\n-\t\t\t (sb_contents->buf[40] != '\\0' && !isspace(sb_contents->buf[40]))) {\n-\t\t\t\t*flags |= REF_ISBROKEN;\n-\t\t\t\terrno = EINVAL;\n-\t\t\t\treturn NULL;\n-\t\t\t}\n+\t\tif (!(read_flags & REF_ISSYMREF)) {\n \t\t\tif (*flags & REF_BAD_NAME) {\n \t\t\t\thashclr(sha1);\n \t\t\t\t*flags |= REF_ISBROKEN;\n \t\t\t}\n \t\t\treturn refname;\n \t\t}\n-\t\t*flags |= REF_ISSYMREF;\n-\t\trefname = sb_contents->buf + 4;\n-\t\twhile (isspace(*refname))\n-\t\t\trefname++;\n-\t\tstrbuf_reset(sb_refname);\n-\t\tstrbuf_addstr(sb_refname, refname);\n+\n \t\trefname = sb_refname->buf;\n \t\tif (resolve_flags & RESOLVE_REF_NO_RECURSE) {\n \t\t\thashclr(sha1);\n-- \n2.8.0.rc3\n"},{"id":"281548","messageId":"2bc5bb4589a7a9e313a4f3ce082af765f11a443d.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 13/21] read_raw_ref(): manage own scratch space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:30Z","receivedAt":"2016-03-23T10:04:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of creating scratch space in resolve_ref_unsafe() and passing it\ndown through resolve_ref_1 to read_raw_ref(), teach read_raw_ref() to\nmanage its own scratch space. This reduces coupling across the functions\nat the cost of some extra allocations. Also, when read_raw_ref() is\nimplemented for different reference backends, the other implementations\nmight have different scratch space requirements.\n\nNote that we now preserve errno across the calls to strbuf_release(),\nwhich calls free() and can thus theoretically overwrite errno.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 76 ++++++++++++++++++++++++++++------------------------\n 1 file changed, 41 insertions(+), 35 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d51e778..f752568 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1421,17 +1421,20 @@ static int resolve_missing_loose_ref(const char *refname,\n * refname will still be valid and unchanged.\n */\n static int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t\tstruct strbuf *symref, struct strbuf *sb_path,\n-\t\t\tstruct strbuf *sb_contents, int *flags)\n+\t\t\tstruct strbuf *symref, int *flags)\n {\n+\tstruct strbuf sb_contents = STRBUF_INIT;\n+\tstruct strbuf sb_path = STRBUF_INIT;\n \tconst char *path;\n \tconst char *buf;\n \tstruct stat st;\n \tint fd;\n+\tint ret = -1;\n+\tint save_errno;\n \n-\tstrbuf_reset(sb_path);\n-\tstrbuf_git_path(sb_path, \"%s\", refname);\n-\tpath = sb_path->buf;\n+\tstrbuf_reset(&sb_path);\n+\tstrbuf_git_path(&sb_path, \"%s\", refname);\n+\tpath = sb_path.buf;\n \n stat_ref:\n \t/*\n@@ -1446,36 +1449,38 @@ stat_ref:\n \n \tif (lstat(path, &st) < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n \t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n \t\t\terrno = ENOENT;\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n \t\t}\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tgoto out;\n \t}\n \n \t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \tif (S_ISLNK(st.st_mode)) {\n-\t\tstrbuf_reset(sb_contents);\n-\t\tif (strbuf_readlink(sb_contents, path, 0) < 0) {\n+\t\tstrbuf_reset(&sb_contents);\n+\t\tif (strbuf_readlink(&sb_contents, path, 0) < 0) {\n \t\t\tif (errno == ENOENT || errno == EINVAL)\n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n \t\t\telse\n-\t\t\t\treturn -1;\n+\t\t\t\tgoto out;\n \t\t}\n-\t\tif (starts_with(sb_contents->buf, \"refs/\") &&\n-\t\t !check_refname_format(sb_contents->buf, 0)) {\n-\t\t\tstrbuf_swap(sb_contents, symref);\n+\t\tif (starts_with(sb_contents.buf, \"refs/\") &&\n+\t\t !check_refname_format(sb_contents.buf, 0)) {\n+\t\t\tstrbuf_swap(&sb_contents, symref);\n \t\t\t*flags |= REF_ISSYMREF;\n-\t\t\treturn 0;\n+\t\t\tret = 0;\n+\t\t\tgoto out;\n \t\t}\n \t}\n \n \t/* Is it a directory? */\n \tif (S_ISDIR(st.st_mode)) {\n \t\terrno = EISDIR;\n-\t\treturn -1;\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -1488,18 +1493,18 @@ stat_ref:\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n \t}\n-\tstrbuf_reset(sb_contents);\n-\tif (strbuf_read(sb_contents, fd, 256) < 0) {\n+\tstrbuf_reset(&sb_contents);\n+\tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n \t\tint save_errno = errno;\n \t\tclose(fd);\n \t\terrno = save_errno;\n-\t\treturn -1;\n+\t\tgoto out;\n \t}\n \tclose(fd);\n-\tstrbuf_rtrim(sb_contents);\n-\tbuf = sb_contents->buf;\n+\tstrbuf_rtrim(&sb_contents);\n+\tbuf = sb_contents.buf;\n \tif (starts_with(buf, \"ref:\")) {\n \t\tbuf += 4;\n \t\twhile (isspace(*buf))\n@@ -1508,7 +1513,8 @@ stat_ref:\n \t\tstrbuf_reset(symref);\n \t\tstrbuf_addstr(symref, buf);\n \t\t*flags |= REF_ISSYMREF;\n-\t\treturn 0;\n+\t\tret = 0;\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -1519,10 +1525,17 @@ stat_ref:\n \t (buf[40] != '\\0' && !isspace(buf[40]))) {\n \t\t*flags |= REF_ISBROKEN;\n \t\terrno = EINVAL;\n-\t\treturn -1;\n+\t\tgoto out;\n \t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out:\n+\tsave_errno = errno;\n+\tstrbuf_release(&sb_path);\n+\tstrbuf_release(&sb_contents);\n+\terrno = save_errno;\n+\treturn ret;\n }\n \n /* This function needs to return a meaningful errno on failure */\n@@ -1530,9 +1543,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t unsigned char *sha1,\n \t\t\t\t int *flags,\n-\t\t\t\t struct strbuf *sb_refname,\n-\t\t\t\t struct strbuf *sb_path,\n-\t\t\t\t struct strbuf *sb_contents)\n+\t\t\t\t struct strbuf *sb_refname)\n {\n \tint symref_count;\n \n@@ -1559,8 +1570,7 @@ static const char *resolve_ref_1(const char *refname,\n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n \t\tint read_flags = 0;\n \n-\t\tif (read_raw_ref(refname, sha1, sb_refname,\n-\t\t\t\t sb_path, sb_contents, &read_flags)) {\n+\t\tif (read_raw_ref(refname, sha1, sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n \t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n \t\t\t\treturn NULL;\n@@ -1604,8 +1614,6 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t unsigned char *sha1, int *flags)\n {\n \tstatic struct strbuf sb_refname = STRBUF_INIT;\n-\tstruct strbuf sb_contents = STRBUF_INIT;\n-\tstruct strbuf sb_path = STRBUF_INIT;\n \tint unused_flags;\n \tconst char *ret;\n \n@@ -1613,9 +1621,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\tflags = &unused_flags;\n \n \tret = resolve_ref_1(refname, resolve_flags, sha1, flags,\n-\t\t\t &sb_refname, &sb_path, &sb_contents);\n-\tstrbuf_release(&sb_path);\n-\tstrbuf_release(&sb_contents);\n+\t\t\t &sb_refname);\n \treturn ret;\n }\n \n-- \n2.8.0.rc3\n"},{"id":"281539","messageId":"53338941ec04ba49db1fe6aa117f605d4d2a98f9.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 14/21] Inline resolve_ref_1() into resolve_ref_unsafe()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:31Z","receivedAt":"2016-03-23T10:04:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"resolve_ref_unsafe() wasn't doing anything useful anymore.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 31 +++++++++----------------------\n 1 file changed, 9 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f752568..120b2dd 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1539,14 +1539,16 @@ out:\n }\n \n /* This function needs to return a meaningful errno on failure */\n-static const char *resolve_ref_1(const char *refname,\n-\t\t\t\t int resolve_flags,\n-\t\t\t\t unsigned char *sha1,\n-\t\t\t\t int *flags,\n-\t\t\t\t struct strbuf *sb_refname)\n+const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n+\t\t\t unsigned char *sha1, int *flags)\n {\n+\tstatic struct strbuf sb_refname = STRBUF_INIT;\n+\tint unused_flags;\n \tint symref_count;\n \n+\tif (!flags)\n+\t\tflags = &unused_flags;\n+\n \t*flags = 0;\n \n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n@@ -1570,7 +1572,7 @@ static const char *resolve_ref_1(const char *refname,\n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n \t\tint read_flags = 0;\n \n-\t\tif (read_raw_ref(refname, sha1, sb_refname, &read_flags)) {\n+\t\tif (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n \t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n \t\t\t\treturn NULL;\n@@ -1590,7 +1592,7 @@ static const char *resolve_ref_1(const char *refname,\n \t\t\treturn refname;\n \t\t}\n \n-\t\trefname = sb_refname->buf;\n+\t\trefname = sb_refname.buf;\n \t\tif (resolve_flags & RESOLVE_REF_NO_RECURSE) {\n \t\t\thashclr(sha1);\n \t\t\treturn refname;\n@@ -1610,21 +1612,6 @@ static const char *resolve_ref_1(const char *refname,\n \treturn NULL;\n }\n \n-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n-\t\t\t unsigned char *sha1, int *flags)\n-{\n-\tstatic struct strbuf sb_refname = STRBUF_INIT;\n-\tint unused_flags;\n-\tconst char *ret;\n-\n-\tif (!flags)\n-\t\tflags = &unused_flags;\n-\n-\tret = resolve_ref_1(refname, resolve_flags, sha1, flags,\n-\t\t\t &sb_refname);\n-\treturn ret;\n-}\n-\n /*\n * Peel the entry (if possible) and return its new peel_status. If\n * repeel is true, re-peel the entry even if there is an old peeled\n-- \n2.8.0.rc3\n"},{"id":"281554","messageId":"317cccf286a11f2e4ac92fecacd206494b1bc30f.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 15/21] read_raw_ref(): change flags parameter to unsigned int","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:32Z","receivedAt":"2016-03-23T10:04:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"read_raw_ref() is going to be part of the vtable for reference backends,\nso clean up its interface to use \"unsigned int flags\" rather than \"int\nflags\". Its caller still uses signed int for its flags arguments. But\nchanging that would touch a lot of code, so leave it for now.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 120b2dd..a15986c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1372,7 +1372,7 @@ static struct ref_entry *get_packed_ref(const char *refname)\n */\n static int resolve_missing_loose_ref(const char *refname,\n \t\t\t\t unsigned char *sha1,\n-\t\t\t\t int *flags)\n+\t\t\t\t unsigned int *flags)\n {\n \tstruct ref_entry *entry;\n \n@@ -1421,7 +1421,7 @@ static int resolve_missing_loose_ref(const char *refname,\n * refname will still be valid and unchanged.\n */\n static int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t\tstruct strbuf *symref, int *flags)\n+\t\t\tstruct strbuf *symref, unsigned int *flags)\n {\n \tstruct strbuf sb_contents = STRBUF_INIT;\n \tstruct strbuf sb_path = STRBUF_INIT;\n@@ -1570,7 +1570,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t}\n \n \tfor (symref_count = 0; symref_count < MAXDEPTH; symref_count++) {\n-\t\tint read_flags = 0;\n+\t\tunsigned int read_flags = 0;\n \n \t\tif (read_raw_ref(refname, sha1, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n-- \n2.8.0.rc3\n"},{"id":"281553","messageId":"f569b2b9aa9a2ce2a4422ba55b0ac06753d42915.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 16/21] fsck_head_link(): remove unneeded flag variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:33Z","receivedAt":"2016-03-23T10:04:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is never read, so we can pass NULL to resolve_ref_unsafe().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/fsck.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 55eac75..3f27456 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -493,13 +493,12 @@ static void fsck_object_dir(const char *path)\n \n static int fsck_head_link(void)\n {\n-\tint flag;\n \tint null_is_error = 0;\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking HEAD link\\n\");\n \n-\thead_points_at = resolve_ref_unsafe(\"HEAD\", 0, head_oid.hash, &flag);\n+\thead_points_at = resolve_ref_unsafe(\"HEAD\", 0, head_oid.hash, NULL);\n \tif (!head_points_at) {\n \t\terrors_found |= ERROR_REFS;\n \t\treturn error(\"Invalid HEAD\");\n-- \n2.8.0.rc3\n"},{"id":"281536","messageId":"d9541a71ee169e9214238f839fdb9ec0263a8fb6.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 17/21] cmd_merge(): remove unneeded flag variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:34Z","receivedAt":"2016-03-23T10:04:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is never read, so we can pass NULL to resolve_ref_unsafe().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex df0afa0..a91ae5b 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1146,7 +1146,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tunsigned char head_sha1[20];\n \tstruct commit *head_commit;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint flag, i, ret = 0, head_subsumed;\n+\tint i, ret = 0, head_subsumed;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n@@ -1160,7 +1160,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = branch_to_free = resolve_refdup(\"HEAD\", 0, head_sha1, &flag);\n+\tbranch = branch_to_free = resolve_refdup(\"HEAD\", 0, head_sha1, NULL);\n \tif (branch && starts_with(branch, \"refs/heads/\"))\n \t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\n-- \n2.8.0.rc3\n"},{"id":"281541","messageId":"984403a927046ddd3fec02f32500ee5c73750396.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 18/21] get_default_remote(): remove unneeded flag variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:35Z","receivedAt":"2016-03-23T10:04:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is never read, so we can pass NULL to resolve_ref_unsafe().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/submodule--helper.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed4f60b..c72365e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -17,9 +17,8 @@ static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n \tunsigned char sha1[20];\n-\tint flag = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n+\tconst char *refname = resolve_ref_unsafe(\"HEAD\", 0, sha1, NULL);\n \n \tif (!refname)\n \t\tdie(\"No such ref: HEAD\");\n-- \n2.8.0.rc3\n"},{"id":"281535","messageId":"1b77b4a4bba21cb4f38092b6b36663b38de58a23.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 19/21] checkout_paths(): remove unneeded flag variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:36Z","receivedAt":"2016-03-23T10:04:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is never read, so we can pass NULL to resolve_ref_unsafe().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/checkout.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex cfa66e2..ef42237 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -242,7 +242,6 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstruct checkout state;\n \tstatic char *ps_matched;\n \tunsigned char rev[20];\n-\tint flag;\n \tstruct commit *head;\n \tint errs = 0;\n \tstruct lock_file *lock_file;\n@@ -375,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\tread_ref_full(\"HEAD\", 0, rev, &flag);\n+\tread_ref_full(\"HEAD\", 0, rev, NULL);\n \thead = lookup_commit_reference_gently(rev, 1);\n \n \terrs |= post_checkout_hook(head, head, 0);\n-- \n2.8.0.rc3\n"},{"id":"281544","messageId":"3260ac9599bc9efa6d7355b79d01e1a3e80bcb73.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 20/21] check_aliased_update(): check that dst_name is non-NULL","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:37Z","receivedAt":"2016-03-23T10:04:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If there is an error in resolve_ref_unsafe(), it returns NULL. We check\nfor this case, but not until after calling strip_namespace(). Instead,\ncall strip_namespace() *after* the NULL check.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c8e32b2..49cc88d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1081,13 +1081,13 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tif (!(flag & REF_ISSYMREF))\n \t\treturn;\n \n-\tdst_name = strip_namespace(dst_name);\n \tif (!dst_name) {\n \t\trp_error(\"refusing update to broken symref '%s'\", cmd->ref_name);\n \t\tcmd->skip_update = 1;\n \t\tcmd->error_string = \"broken symref\";\n \t\treturn;\n \t}\n+\tdst_name = strip_namespace(dst_name);\n \n \tif ((item = string_list_lookup(list, dst_name)) == NULL)\n \t\treturn;\n-- \n2.8.0.rc3\n"},{"id":"281538","messageId":"fad78ec03e73aa60ddac12b1f738b7180efcfeff.1458723959.git.mhagger@alum.mit.edu","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"[PATCH 21/21] show_head_ref(): check the result of resolve_ref_namespace()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-23T10:04:38Z","receivedAt":"2016-03-23T10:04:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Only use the result of resolve_ref_namespace() if it is non-NULL.\n\nSigned-off-by: Michael Haggerty \n---\n http-backend.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 8870a26..2148814 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -484,9 +484,9 @@ static int show_head_ref(const char *refname, const struct object_id *oid,\n \t\tconst char *target = resolve_ref_unsafe(refname,\n \t\t\t\t\t\t\tRESOLVE_REF_READING,\n \t\t\t\t\t\t\tunused.hash, NULL);\n-\t\tconst char *target_nons = strip_namespace(target);\n \n-\t\tstrbuf_addf(buf, \"ref: %s\\n\", target_nons);\n+\t\tif (target)\n+\t\t\tstrbuf_addf(buf, \"ref: %s\\n\", strip_namespace(target));\n \t} else {\n \t\tstrbuf_addf(buf, \"%s\\n\", oid_to_hex(oid));\n \t}\n-- \n2.8.0.rc3\n"},{"id":"281638","messageId":"1458802034.28595.8.camel@twopensource.com","threadId":"41792","inReplyTo":"cover.1458723959.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-03-24T06:47:14Z","receivedAt":"2016-03-24T06:47:14Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-03-23 at 11:04 +0100, Michael Haggerty wrote:\n> Patch 04/33 in David Turner's refs-backend-lmdb series v7 [1] did way\n> too much in a single patch, and in fact got a few minor things wrong.\n> Instead of that patch, I suggest this patch series, which\n> ...\n\nLGTM. I think I would squash these patches:\n fsck_head_link(): remove unneeded flag variable\n cmd_merge(): remove unneeded flag variable\n get_default_remote(): remove unneeded flag variable\n checkout_paths(): remove unneeded flag variable\n\nBut that's up to you.\n\nI incorporated your changes into the lmdb backend. To make merging\nlater more convenient, I rebased on top of pu -- I think this mainly\ndepends on jk/check-repository-format, but I also included some fixes\nfor a couple of tests that had been changed by other patches.\n\nThe current version can be found here:\n\nhttps://github.com/dturner-tw/git/tree/dturner/pluggable-backends\n\nI won't resend the full patchset to the list until I hear back on the\nrest of the review.\n\nIt seems like maybe we should now split this into two patchsets:\neverything up to and including \"refs: move resolve_ref_unsafe into\ncommon code\" does not depend on the backend structure and could go in\nearlier. If you agree, we could send that first series and get it in,\nhopefully reducing later merge conflicts. \n"},{"id":"281918","messageId":"56F76E2C.5030700@alum.mit.edu","threadId":"41792","inReplyTo":"1458802034.28595.8.camel@twopensource.com","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-27T05:22:52Z","receivedAt":"2016-03-27T05:22:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/24/2016 07:47 AM, David Turner wrote:\n> [...]\n> I incorporated your changes into the lmdb backend. To make merging\n> later more convenient, I rebased on top of pu -- I think this mainly\n> depends on jk/check-repository-format, but I also included some fixes\n> for a couple of tests that had been changed by other patches.\n\nI think rebasing changes on top of pu is counterproductive. I believe\nthat Junio had extra work rebasing your earlier series onto a merge of\nthe minimum number of topics that it really depended on. There is no way\nthat he could merge the branch in this form because it would imply\nmerging all of pu.\n\nSee the zeroth section of SubmittingPatches [1] for the guidelines.\n\n> The current version can be found here:\n> \n> https://github.com/dturner-tw/git/tree/dturner/pluggable-backends\n> \n> I won't resend the full patchset to the list until I hear back on the\n> rest of the review.\n> \n> It seems like maybe we should now split this into two patchsets:\n> everything up to and including \"refs: move resolve_ref_unsafe into\n> common code\" does not depend on the backend structure and could go in\n> earlier. If you agree, we could send that first series and get it in,\n> hopefully reducing later merge conflicts. \n\nThat sounds like a good idea. It's always a relief to get work merged\nand not have to keep porting it along.\n\nThere are three patches later in the series that (I think) also don't\nhave specifically to do with pluggable backends. These could potentially\nalso be considered for earlier merge to reduce the size of what remains:\n\n* refs: don't dereference on rename\n* refs: on symref reflog expire, lock symref not referrent\n* refs: resolve symbolic refs first\n\nBut note that I haven't audited those patches yet, so I'm not saying\nthat they are necessarily ready to be merged.\n\nMichael\n\n[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n"},{"id":"282107","messageId":"1459282370.2976.7.camel@twopensource.com","threadId":"41792","inReplyTo":"56F76E2C.5030700@alum.mit.edu","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-03-29T20:12:50Z","receivedAt":"2016-03-29T20:12:50Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n> On 03/24/2016 07:47 AM, David Turner wrote:\n> > [...]\n> > I incorporated your changes into the lmdb backend. To make merging\n> > later more convenient, I rebased on top of pu -- I think this\n> > mainly\n> > depends on jk/check-repository-format, but I also included some\n> > fixes\n> > for a couple of tests that had been changed by other patches.\n> \n> I think rebasing changes on top of pu is counterproductive. I believe\n> that Junio had extra work rebasing your earlier series onto a merge\n> of\n> the minimum number of topics that it really depended on. There is no\n> way\n> that he could merge the branch in this form because it would imply\n> merging all of pu.\n> \n> See the zeroth section of SubmittingPatches [1] for the guidelines.\n\nI'm a bit confused because \n[PATCH 18/21] get_default_remote(): remove unneeded flag variable\n\ndoesn't do anything on master -- it depends on some patch in pu. And\nwe definitely want to pick up jk/check-repository-format (which doesn't\ninclude whatever 18/21 depends on).\n\nSo what do you think our base should be?\n"},{"id":"282197","messageId":"56FB743C.3020902@alum.mit.edu","threadId":"41792","inReplyTo":"1459282370.2976.7.camel@twopensource.com","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-30T06:37:48Z","receivedAt":"2016-03-30T06:37:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/29/2016 10:12 PM, David Turner wrote:\n> On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n>> On 03/24/2016 07:47 AM, David Turner wrote:\n>>> [...]\n>>> I incorporated your changes into the lmdb backend. To make merging\n>>> later more convenient, I rebased on top of pu -- I think this\n>>> mainly\n>>> depends on jk/check-repository-format, but I also included some\n>>> fixes\n>>> for a couple of tests that had been changed by other patches.\n>>\n>> I think rebasing changes on top of pu is counterproductive. I believe\n>> that Junio had extra work rebasing your earlier series onto a merge\n>> of\n>> the minimum number of topics that it really depended on. There is no\n>> way\n>> that he could merge the branch in this form because it would imply\n>> merging all of pu.\n>>\n>> See the zeroth section of SubmittingPatches [1] for the guidelines.\n> \n> I'm a bit confused because \n> [PATCH 18/21] get_default_remote(): remove unneeded flag variable\n> \n> doesn't do anything on master -- it depends on some patch in pu. And\n> we definitely want to pick up jk/check-repository-format (which doesn't\n> include whatever 18/21 depends on).\n> \n> So what do you think our base should be?\n\nI think the preference is to base a patch series on the merge of master\nplus the minimum number of topics in pu (ideally, none) that are\n\"essential\" prerequisites of the changes in the patch series. For\nexample, the version of this patch series that Junio has in his tree was\nbased on master + sb/submodule-parallel-update. Even if there are minor\nconflicts with another in-flight topic, it is easier for Junio to\nresolve the conflicts when merging the topics together than to rebase\nthe patch series over and over as the other patch series evolves. The\ngoal of this practice is of course to allow patch series to evolve\nindependently of each other as much as possible.\n\nOf course if you have insights into nontrivial conflicts between your\npatch series and others, it would be helpful to discuss these in your\ncover letter.\n\nMichael\n"},{"id":"282294","messageId":"1459368315.2976.27.camel@twopensource.com","threadId":"41792","inReplyTo":"56FB743C.3020902@alum.mit.edu","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-03-30T20:05:15Z","receivedAt":"2016-03-30T20:05:15Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:\n> On 03/29/2016 10:12 PM, David Turner wrote:\n> > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n> > > On 03/24/2016 07:47 AM, David Turner wrote:\n> > > > [...]\n> > > > I incorporated your changes into the lmdb backend. To make\n> > > > merging\n> > > > later more convenient, I rebased on top of pu -- I think this\n> > > > mainly\n> > > > depends on jk/check-repository-format, but I also included some\n> > > > fixes\n> > > > for a couple of tests that had been changed by other patches.\n> > > \n> > > I think rebasing changes on top of pu is counterproductive. I\n> > > believe\n> > > that Junio had extra work rebasing your earlier series onto a\n> > > merge\n> > > of\n> > > the minimum number of topics that it really depended on. There is\n> > > no\n> > > way\n> > > that he could merge the branch in this form because it would\n> > > imply\n> > > merging all of pu.\n> > > \n> > > See the zeroth section of SubmittingPatches [1] for the\n> > > guidelines.\n> > \n> > I'm a bit confused because \n> > [PATCH 18/21] get_default_remote(): remove unneeded flag variable\n> > \n> > doesn't do anything on master -- it depends on some patch in pu. \n> > And\n> > we definitely want to pick up jk/check-repository-format (which\n> > doesn't\n> > include whatever 18/21 depends on).\n> > \n> > So what do you think our base should be?\n> \n> I think the preference is to base a patch series on the merge of\n> master\n> plus the minimum number of topics in pu (ideally, none) that are\n> \"essential\" prerequisites of the changes in the patch series. For\n> example, the version of this patch series that Junio has in his tree\n> was\n> based on master + sb/submodule-parallel-update. \n>\n> Even if there are minor\n> conflicts with another in-flight topic, it is easier for Junio to\n> resolve the conflicts when merging the topics together than to rebase\n> the patch series over and over as the other patch series evolves. The\n> goal of this practice is of course to allow patch series to evolve\n> independently of each other as much as possible.\n> \n> Of course if you have insights into nontrivial conflicts between your\n> patch series and others, it would be helpful to discuss these in your\n> cover letter.\n\nIf I am reading this correctly, it looks like your series also has a\nfew more sb submodule patches, e.g. sb/submodule-init, which is\nresponsible for the code that 18/21 depends on. \n\nI think jk/check-repository-format is also good to get in first,\nbecause it changes the startup sequence a bit and it's a bit tricky to\nfigure out what needs to change in dt/refs-backend-lmdb as a result of\nit. \n\nBut I can't just merge jk/check-repository-format on top of 71defe0047 \n-- some function signatures have changed in the run-command stuff and\nit seems kind of annoying to fix up. \n\nSo I propose instead that we just drop 18/21 for now, and use just\njk/check-repository-format as the base.\n\nDoes this seem reasonable to you?\n"},{"id":"282333","messageId":"56FD4CCB.1000205@alum.mit.edu","threadId":"41792","inReplyTo":"1459368315.2976.27.camel@twopensource.com","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-03-31T16:14:03Z","receivedAt":"2016-03-31T16:14:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/30/2016 10:05 PM, David Turner wrote:\n> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:\n>> On 03/29/2016 10:12 PM, David Turner wrote:\n>>> On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n>>>> On 03/24/2016 07:47 AM, David Turner wrote:\n>>>>> [...]\n>>>>> I incorporated your changes into the lmdb backend. To make\n>>>>> merging\n>>>>> later more convenient, I rebased on top of pu -- I think this\n>>>>> mainly\n>>>>> depends on jk/check-repository-format, but I also included some\n>>>>> fixes\n>>>>> for a couple of tests that had been changed by other patches.\n>>>>\n>>>> I think rebasing changes on top of pu is counterproductive. I\n>>>> believe\n>>>> that Junio had extra work rebasing your earlier series onto a\n>>>> merge\n>>>> of\n>>>> the minimum number of topics that it really depended on. There is\n>>>> no\n>>>> way\n>>>> that he could merge the branch in this form because it would\n>>>> imply\n>>>> merging all of pu.\n>>>>\n>>>> See the zeroth section of SubmittingPatches [1] for the\n>>>> guidelines.\n>>>\n>>> I'm a bit confused because \n>>> [PATCH 18/21] get_default_remote(): remove unneeded flag variable\n>>>\n>>> doesn't do anything on master -- it depends on some patch in pu. \n>>> And\n>>> we definitely want to pick up jk/check-repository-format (which\n>>> doesn't\n>>> include whatever 18/21 depends on).\n>>>\n>>> So what do you think our base should be?\n>>\n>> I think the preference is to base a patch series on the merge of\n>> master\n>> plus the minimum number of topics in pu (ideally, none) that are\n>> \"essential\" prerequisites of the changes in the patch series. For\n>> example, the version of this patch series that Junio has in his tree\n>> was\n>> based on master + sb/submodule-parallel-update. \n>>\n>> Even if there are minor\n>> conflicts with another in-flight topic, it is easier for Junio to\n>> resolve the conflicts when merging the topics together than to rebase\n>> the patch series over and over as the other patch series evolves. The\n>> goal of this practice is of course to allow patch series to evolve\n>> independently of each other as much as possible.\n>>\n>> Of course if you have insights into nontrivial conflicts between your\n>> patch series and others, it would be helpful to discuss these in your\n>> cover letter.\n> \n> If I am reading this correctly, it looks like your series also has a\n> few more sb submodule patches, e.g. sb/submodule-init, which is\n> responsible for the code that 18/21 depends on. \n> \n> I think jk/check-repository-format is also good to get in first,\n> because it changes the startup sequence a bit and it's a bit tricky to\n> figure out what needs to change in dt/refs-backend-lmdb as a result of\n> it. \n> \n> But I can't just merge jk/check-repository-format on top of 71defe0047 \n> -- some function signatures have changed in the run-command stuff and\n> it seems kind of annoying to fix up. \n> \n> So I propose instead that we just drop 18/21 for now, and use just\n> jk/check-repository-format as the base.\n> \n> Does this seem reasonable to you?\n\nYes, that's fine. Patch 18/21 is just a random cleanup that nothing else\ndepends on. Will you do the rebasing? If so, please let me know where I\ncan fetch the result from.\n\nMichael\n"},{"id":"282401","messageId":"1459462962.2976.38.camel@twopensource.com","threadId":"41792","inReplyTo":"56FD4CCB.1000205@alum.mit.edu","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-03-31T22:22:42Z","receivedAt":"2016-03-31T22:22:42Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2016-03-31 at 18:14 +0200, Michael Haggerty wrote:\n> On 03/30/2016 10:05 PM, David Turner wrote:\n> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:\n> > > On 03/29/2016 10:12 PM, David Turner wrote:\n> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n> > > > > On 03/24/2016 07:47 AM, David Turner wrote:\n> > > > > > [...]\n> > > > > > I incorporated your changes into the lmdb backend. To make\n> > > > > > merging\n> > > > > > later more convenient, I rebased on top of pu -- I think\n> > > > > > this\n> > > > > > mainly\n> > > > > > depends on jk/check-repository-format, but I also included\n> > > > > > some\n> > > > > > fixes\n> > > > > > for a couple of tests that had been changed by other\n> > > > > > patches.\n> > > > > \n> > > > > I think rebasing changes on top of pu is counterproductive. I\n> > > > > believe\n> > > > > that Junio had extra work rebasing your earlier series onto a\n> > > > > merge\n> > > > > of\n> > > > > the minimum number of topics that it really depended on.\n> > > > > There is\n> > > > > no\n> > > > > way\n> > > > > that he could merge the branch in this form because it would\n> > > > > imply\n> > > > > merging all of pu.\n> > > > > \n> > > > > See the zeroth section of SubmittingPatches [1] for the\n> > > > > guidelines.\n> > > > \n> > > > I'm a bit confused because \n> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag\n> > > > variable\n> > > > \n> > > > doesn't do anything on master -- it depends on some patch in\n> > > > pu. \n> > > > And\n> > > > we definitely want to pick up jk/check-repository-format (which\n> > > > doesn't\n> > > > include whatever 18/21 depends on).\n> > > > \n> > > > So what do you think our base should be?\n> > > \n> > > I think the preference is to base a patch series on the merge of\n> > > master\n> > > plus the minimum number of topics in pu (ideally, none) that are\n> > > \"essential\" prerequisites of the changes in the patch series. For\n> > > example, the version of this patch series that Junio has in his\n> > > tree\n> > > was\n> > > based on master + sb/submodule-parallel-update. \n> > > \n> > > Even if there are minor\n> > > conflicts with another in-flight topic, it is easier for Junio to\n> > > resolve the conflicts when merging the topics together than to\n> > > rebase\n> > > the patch series over and over as the other patch series evolves.\n> > > The\n> > > goal of this practice is of course to allow patch series to\n> > > evolve\n> > > independently of each other as much as possible.\n> > > \n> > > Of course if you have insights into nontrivial conflicts between\n> > > your\n> > > patch series and others, it would be helpful to discuss these in\n> > > your\n> > > cover letter.\n> > \n> > If I am reading this correctly, it looks like your series also has\n> > a\n> > few more sb submodule patches, e.g. sb/submodule-init, which is\n> > responsible for the code that 18/21 depends on. \n> > \n> > I think jk/check-repository-format is also good to get in first,\n> > because it changes the startup sequence a bit and it's a bit tricky\n> > to\n> > figure out what needs to change in dt/refs-backend-lmdb as a result\n> > of\n> > it. \n> > \n> > But I can't just merge jk/check-repository-format on top of\n> > 71defe0047 \n> > -- some function signatures have changed in the run-command stuff\n> > and\n> > it seems kind of annoying to fix up. \n> > \n> > So I propose instead that we just drop 18/21 for now, and use just\n> > jk/check-repository-format as the base.\n> > \n> > Does this seem reasonable to you?\n> \n> Yes, that's fine. Patch 18/21 is just a random cleanup that nothing\n> else\n> depends on. Will you do the rebasing? If so, please let me know where\n> I\n> can fetch the result from.\n\nRebased:\n\nhttps://github.com/dturner-tw/git.git on branch\ndturner/pluggable-backends\n"},{"id":"282427","messageId":"CAGZ79katAVbd6mMSu2Kt1SnH2f3Ujn0L4-bH82bwLudzboCyTg@mail.gmail.com","threadId":"41792","inReplyTo":"1459368315.2976.27.camel@twopensource.com","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-01T01:37:17Z","receivedAt":"2016-04-01T01:37:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Mar 30, 2016 at 1:05 PM, David Turner wrote:\n> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:\n>> On 03/29/2016 10:12 PM, David Turner wrote:\n>> > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n>> > > On 03/24/2016 07:47 AM, David Turner wrote:\n>> > > > [...]\n>> > > > I incorporated your changes into the lmdb backend. To make\n>> > > > merging\n>> > > > later more convenient, I rebased on top of pu -- I think this\n>> > > > mainly\n>> > > > depends on jk/check-repository-format, but I also included some\n>> > > > fixes\n>> > > > for a couple of tests that had been changed by other patches.\n>> > >\n>> > > I think rebasing changes on top of pu is counterproductive. I\n>> > > believe\n>> > > that Junio had extra work rebasing your earlier series onto a\n>> > > merge\n>> > > of\n>> > > the minimum number of topics that it really depended on. There is\n>> > > no\n>> > > way\n>> > > that he could merge the branch in this form because it would\n>> > > imply\n>> > > merging all of pu.\n>> > >\n>> > > See the zeroth section of SubmittingPatches [1] for the\n>> > > guidelines.\n>> >\n>> > I'm a bit confused because\n>> > [PATCH 18/21] get_default_remote(): remove unneeded flag variable\n>> >\n>> > doesn't do anything on master -- it depends on some patch in pu.\n>> > And\n>> > we definitely want to pick up jk/check-repository-format (which\n>> > doesn't\n>> > include whatever 18/21 depends on).\n>> >\n>> > So what do you think our base should be?\n>>\n>> I think the preference is to base a patch series on the merge of\n>> master\n>> plus the minimum number of topics in pu (ideally, none) that are\n>> \"essential\" prerequisites of the changes in the patch series. For\n>> example, the version of this patch series that Junio has in his tree\n>> was\n>> based on master + sb/submodule-parallel-update.\n>>\n>> Even if there are minor\n>> conflicts with another in-flight topic, it is easier for Junio to\n>> resolve the conflicts when merging the topics together than to rebase\n>> the patch series over and over as the other patch series evolves. The\n>> goal of this practice is of course to allow patch series to evolve\n>> independently of each other as much as possible.\n>>\n>> Of course if you have insights into nontrivial conflicts between your\n>> patch series and others, it would be helpful to discuss these in your\n>> cover letter.\n>\n> If I am reading this correctly, it looks like your series also has a\n> few more sb submodule patches, e.g. sb/submodule-init, which is\n> responsible for the code that 18/21 depends on.\n>\n> I think jk/check-repository-format is also good to get in first,\n> because it changes the startup sequence a bit and it's a bit tricky to\n> figure out what needs to change in dt/refs-backend-lmdb as a result of\n> it.\n>\n> But I can't just merge jk/check-repository-format on top of 71defe0047\n> -- some function signatures have changed in the run-command stuff and\n> it seems kind of annoying to fix up.\n>\n> So I propose instead that we just drop 18/21 for now, and use just\n> jk/check-repository-format as the base.\n\nBy 18/21 you mean\n[PATCH 18/21] get_default_remote(): remove unneeded flag variable\nin builtin/submodule--helper.c?\nYou could drop that and I'll pick it up in one of the submodule series',\nif that is more convenient for you.\n\n\n>\n> Does this seem reasonable to you?\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"282492","messageId":"1459533314.2976.40.camel@twopensource.com","threadId":"41792","inReplyTo":"CAGZ79katAVbd6mMSu2Kt1SnH2f3Ujn0L4-bH82bwLudzboCyTg@mail.gmail.com","subject":"Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-01T17:55:14Z","receivedAt":"2016-04-01T17:55:14Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2016-03-31 at 18:37 -0700, Stefan Beller wrote:\n> On Wed, Mar 30, 2016 at 1:05 PM, David Turner <\n> dturner@twopensource.com> wrote:\n> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:\n> > > On 03/29/2016 10:12 PM, David Turner wrote:\n> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:\n> > > > > On 03/24/2016 07:47 AM, David Turner wrote:\n> > > > > > [...]\n> > > > > > I incorporated your changes into the lmdb backend. To make\n> > > > > > merging\n> > > > > > later more convenient, I rebased on top of pu -- I think\n> > > > > > this\n> > > > > > mainly\n> > > > > > depends on jk/check-repository-format, but I also included\n> > > > > > some\n> > > > > > fixes\n> > > > > > for a couple of tests that had been changed by other\n> > > > > > patches.\n> > > > > \n> > > > > I think rebasing changes on top of pu is counterproductive. I\n> > > > > believe\n> > > > > that Junio had extra work rebasing your earlier series onto a\n> > > > > merge\n> > > > > of\n> > > > > the minimum number of topics that it really depended on.\n> > > > > There is\n> > > > > no\n> > > > > way\n> > > > > that he could merge the branch in this form because it would\n> > > > > imply\n> > > > > merging all of pu.\n> > > > > \n> > > > > See the zeroth section of SubmittingPatches [1] for the\n> > > > > guidelines.\n> > > > \n> > > > I'm a bit confused because\n> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag\n> > > > variable\n> > > > \n> > > > doesn't do anything on master -- it depends on some patch in\n> > > > pu.\n> > > > And\n> > > > we definitely want to pick up jk/check-repository-format (which\n> > > > doesn't\n> > > > include whatever 18/21 depends on).\n> > > > \n> > > > So what do you think our base should be?\n> > > \n> > > I think the preference is to base a patch series on the merge of\n> > > master\n> > > plus the minimum number of topics in pu (ideally, none) that are\n> > > \"essential\" prerequisites of the changes in the patch series. For\n> > > example, the version of this patch series that Junio has in his\n> > > tree\n> > > was\n> > > based on master + sb/submodule-parallel-update.\n> > > \n> > > Even if there are minor\n> > > conflicts with another in-flight topic, it is easier for Junio to\n> > > resolve the conflicts when merging the topics together than to\n> > > rebase\n> > > the patch series over and over as the other patch series evolves.\n> > > The\n> > > goal of this practice is of course to allow patch series to\n> > > evolve\n> > > independently of each other as much as possible.\n> > > \n> > > Of course if you have insights into nontrivial conflicts between\n> > > your\n> > > patch series and others, it would be helpful to discuss these in\n> > > your\n> > > cover letter.\n> > \n> > If I am reading this correctly, it looks like your series also has\n> > a\n> > few more sb submodule patches, e.g. sb/submodule-init, which is\n> > responsible for the code that 18/21 depends on.\n> > \n> > I think jk/check-repository-format is also good to get in first,\n> > because it changes the startup sequence a bit and it's a bit tricky\n> > to\n> > figure out what needs to change in dt/refs-backend-lmdb as a result\n> > of\n> > it.\n> > \n> > But I can't just merge jk/check-repository-format on top of\n> > 71defe0047\n> > -- some function signatures have changed in the run-command stuff\n> > and\n> > it seems kind of annoying to fix up.\n> > \n> > So I propose instead that we just drop 18/21 for now, and use just\n> > jk/check-repository-format as the base.\n> \n> By 18/21 you mean\n> [PATCH 18/21] get_default_remote(): remove unneeded flag variable\n> in builtin/submodule--helper.c?\n> You could drop that and I'll pick it up in one of the submodule\n> series',\n> if that is more convenient for you.\n> \n\n\nYes, thanks.\n"}]}