{"thread":{"id":"42250","subject":"[PATCH v2 01/33] t1404: demonstrate a bug resolving references","startedAt":"2016-05-06T16:13:41Z","lastAt":"2016-06-16T02:19:53Z","messageCount":51,"participants":["Michael Haggerty","David Turner","Junio C Hamano","Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":2,"patchTotal":33},"messages":[{"id":"285722","messageId":"cover.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":null,"subject":"[PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:41Z","receivedAt":"2016-05-06T16:13:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Thanks to David, Junio, and Peff for their comments on v1 of this\npatch series [1]. I think I have addressed all of the points that were\nbrought up. Plus I fixed a pre-existing bug that I noticed myself\nwhile adding some more tests; see the first bullet point below for\nmore information.\n\nChanges between v1 and v2:\n\n* Prefixed the patch series with three new patches that demonstrate\n and fix some bugs in reference resolution that I noticed while\n inspecting this series:\n\n * \"t1404: demonstrate a bug resolving references\" -- this\n demonstrates a bug that has existed since at least 2.5.0.\n * \"read_raw_ref(): don't get confused by an empty directory\"\n * \"commit_ref(): if there is an empty dir in the way, delete it\"\n\n* Added a patch \"read_raw_ref(): move docstring to header file\".\n\n* \"ref_transaction_create(): disallow recursive pruning\": Add a\n comment that `REF_ISPRUNING` must only be used with `REF_NODEREF`.\n\n* \"refs: don't dereference on rename\": explain why we can't pass an\n `old_sha1` argument to `delete_ref()`. Inline `resolve_flags`\n constant to make the code more transparent.\n\n* \"add_update(): initialize the whole ref_update\": move some more\n checks from `ref_transaction_update()` to `add_update()`.\n\n* \"refs: resolve symbolic refs first\":\n * Remove unused `deleting` parameter from `lock_raw_ref()`\n * Fix a comment, add another.\n\n* \"lock_ref_sha1_basic(): only handle REF_NODEREF mode\": initialize\n `ref_name` only once, as its value can be reused.\n\nThis patch series is also available from my GitHub repo [2] as branch\n\"split-under-lock\".\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/292772\n[2] https://github.com/mhagger/git\n\nDavid Turner (2):\n refs: allow log-only updates\n refs: don't dereference on rename\n\nMichael Haggerty (31):\n t1404: demonstrate a bug resolving references\n commit_ref(): if there is an empty dir in the way, delete it\n read_raw_ref(): don't get confused by an empty directory\n safe_create_leading_directories(): improve docstring\n remove_dir_recursively(): add docstring\n refname_is_safe(): use skip_prefix()\n refname_is_safe(): don't allow the empty string\n refname_is_safe(): insist that the refname already be normalized\n commit_ref_update(): write error message to *err, not stderr\n rename_ref(): remove unneeded local variable\n ref_transaction_commit(): remove local variable n\n read_raw_ref(): rename flags argument to type\n read_raw_ref(): clear *type at start of function\n read_raw_ref(): rename symref argument to referent\n read_raw_ref(): improve docstring\n read_raw_ref(): move docstring to header file\n lock_ref_sha1_basic(): remove unneeded local variable\n refs: make error messages more consistent\n ref_transaction_create(): disallow recursive pruning\n ref_transaction_commit(): correctly report close_ref() failure\n delete_branches(): use resolve_refdup()\n verify_refname_available(): adjust constness in declaration\n add_update(): initialize the whole ref_update\n lock_ref_for_update(): new function\n unlock_ref(): move definition higher in the file\n ref_transaction_update(): check refname_is_safe() at a minimum\n refs: resolve symbolic refs first\n lock_ref_for_update(): don't re-read non-symbolic references\n lock_ref_for_update(): don't resolve symrefs\n commit_ref_update(): remove the flags parameter\n lock_ref_sha1_basic(): only handle REF_NODEREF mode\n\n builtin/branch.c | 19 +-\n cache.h | 5 +\n dir.h | 23 +\n refs.c | 96 ++--\n refs/files-backend.c | 909 ++++++++++++++++++++++++++++---------\n refs/refs-internal.h | 95 +++-\n t/t1400-update-ref.sh | 41 +-\n t/t1404-update-ref-df-conflicts.sh | 76 +++-\n t/t1430-bad-ref-name.sh | 2 +-\n t/t3200-branch.sh | 9 +\n 10 files changed, 1013 insertions(+), 262 deletions(-)\n\n-- \n2.8.1\n"},{"id":"285717","messageId":"19dd7d06e5d2c58895dd101025c013404025e192.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 01/33] t1404: demonstrate a bug resolving references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:42Z","receivedAt":"2016-05-06T16:13:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add some tests checking that it is possible to work with a reference\neven if there is an empty directory where the loose ref would be stored.\n\nOne of the new tests demonstrates a bug that has been with us since at\nleast 2.5.0--single reference lookup gives up when it sees the\ndirectory, even if the reference exists as a packed ref. This probably\nhasn't been reported before because Git usually cleans up empty\ndirectories when packing references.\n\nThis bug will be fixed shortly.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1404-update-ref-df-conflicts.sh | 76 +++++++++++++++++++++++++++++++++++++-\n 1 file changed, 75 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\nindex 66bafb5..16752a9 100755\n--- a/t/t1404-update-ref-df-conflicts.sh\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -28,7 +28,9 @@ Q=\"'\"\n test_expect_success 'setup' '\n \n \tgit commit --allow-empty -m Initial &&\n-\tC=$(git rev-parse HEAD)\n+\tC=$(git rev-parse HEAD) &&\n+\tgit commit --allow-empty -m Second &&\n+\tD=$(git rev-parse HEAD)\n \n '\n \n@@ -104,4 +106,76 @@ test_expect_success 'one new ref is a simple prefix of another' '\n \n '\n \n+test_expect_failure 'empty directory should not fool rev-parse' '\n+\tprefix=refs/e-rev-parse &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\techo \"$C\" >expected &&\n+\tgit rev-parse $prefix/foo >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'empty directory should not fool for-each-ref' '\n+\tprefix=refs/e-for-each-ref &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit for-each-ref $prefix >expected &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tgit for-each-ref $prefix >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'empty directory should not fool create' '\n+\tprefix=refs/e-create &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"create %s $C\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n+test_expect_success 'empty directory should not fool verify' '\n+\tprefix=refs/e-verify &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"verify %s $C\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n+test_expect_success 'empty directory should not fool 1-arg update' '\n+\tprefix=refs/e-update-1 &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"update %s $D\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n+test_expect_success 'empty directory should not fool 2-arg update' '\n+\tprefix=refs/e-update-2 &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"update %s $D $C\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n+test_expect_success 'empty directory should not fool 0-arg delete' '\n+\tprefix=refs/e-delete-0 &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"delete %s\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n+test_expect_success 'empty directory should not fool 1-arg delete' '\n+\tprefix=refs/e-delete-1 &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tmkdir -p .git/$prefix/foo/bar/baz &&\n+\tprintf \"delete %s $C\\n\" $prefix/foo |\n+\tgit update-ref --stdin\n+'\n+\n test_done\n-- \n2.8.1\n"},{"id":"285746","messageId":"5387c0d8839e366c44838c808ccc20eb7f9bd358.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 02/33] commit_ref(): if there is an empty dir in the way, delete it","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:43Z","receivedAt":"2016-05-06T16:13:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Part of the bug revealed in the last commit is that resolve_ref_unsafe()\nincorrectly returns EISDIR if it finds a directory in the place where it\nis looking for a loose reference, even if the corresponding packed\nreference exists. lock_ref_sha1_basic() notices the bogus EISDIR, and\nuse it as an indication that it should call remove_empty_directories()\nand call resolve_ref_unsafe() again.\n\nBut resolve_ref_unsafe() shouldn't report EISDIR in this case. If we\nwould simply make that change, then remove_empty_directories() wouldn't\nget called anymore, and the empty directory would get in the way when\ncommit_ref() calls commit_lock_file() to rename the lockfile into place.\n\nSo instead of relying on lock_ref_sha1_basic() to delete empty\ndirectories, teach commit_ref(), just before calling commit_lock_file(),\nto check whether a directory is in the way, and if so, try to delete it.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1f38076..ad9cd86 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2457,6 +2457,30 @@ static int close_ref(struct ref_lock *lock)\n \n static int commit_ref(struct ref_lock *lock)\n {\n+\tchar *path = get_locked_file_path(lock->lk);\n+\tstruct stat st;\n+\n+\tif (!lstat(path, &st) && S_ISDIR(st.st_mode)) {\n+\t\t/*\n+\t\t * There is a directory at the path we want to rename\n+\t\t * the lockfile to. Hopefully it is empty; try to\n+\t\t * delete it.\n+\t\t */\n+\t\tsize_t len = strlen(path);\n+\t\tstruct strbuf sb_path = STRBUF_INIT;\n+\n+\t\tstrbuf_attach(&sb_path, path, len, len);\n+\n+\t\t/*\n+\t\t * If this fails, commit_lock_file() will also fail\n+\t\t * and will report the problem.\n+\t\t */\n+\t\tremove_empty_directories(&sb_path);\n+\t\tstrbuf_release(&sb_path);\n+\t} else {\n+\t\tfree(path);\n+\t}\n+\n \tif (commit_lock_file(lock->lk))\n \t\treturn -1;\n \treturn 0;\n-- \n2.8.1\n"},{"id":"285718","messageId":"e167a5673e25b960dce118fb967d54da30b69def.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 03/33] read_raw_ref(): don't get confused by an empty directory","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:44Z","receivedAt":"2016-05-06T16:13:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Even if there is an empty directory where we look for the loose version\nof a reference, check for a packed reference before giving up. This\nfixes the failing test that was introduced two commits ago.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 11 ++++++++++-\n t/t1404-update-ref-df-conflicts.sh | 2 +-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ad9cd86..0cc116d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1477,7 +1477,16 @@ stat_ref:\n \n \t/* Is it a directory? */\n \tif (S_ISDIR(st.st_mode)) {\n-\t\terrno = EISDIR;\n+\t\t/*\n+\t\t * Even though there is a directory where the loose\n+\t\t * ref is supposed to be, there could still be a\n+\t\t * packed ref:\n+\t\t */\n+\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n+\t\t\terrno = EISDIR;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tret = 0;\n \t\tgoto out;\n \t}\n \ndiff --git a/t/t1404-update-ref-df-conflicts.sh b/t/t1404-update-ref-df-conflicts.sh\nindex 16752a9..6d869d1 100755\n--- a/t/t1404-update-ref-df-conflicts.sh\n+++ b/t/t1404-update-ref-df-conflicts.sh\n@@ -106,7 +106,7 @@ test_expect_success 'one new ref is a simple prefix of another' '\n \n '\n \n-test_expect_failure 'empty directory should not fool rev-parse' '\n+test_expect_success 'empty directory should not fool rev-parse' '\n \tprefix=refs/e-rev-parse &&\n \tgit update-ref $prefix/foo $C &&\n \tgit pack-refs --all &&\n-- \n2.8.1\n"},{"id":"285721","messageId":"e95792e532bde75fd4a1e91aecfcf9a28ba23955.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 04/33] safe_create_leading_directories(): improve docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:45Z","receivedAt":"2016-05-06T16:13:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the difference between this function and\nsafe_create_leading_directories_const(), and that the former restores\npath before returning.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 2711048..4134f64 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -993,6 +993,11 @@ int adjust_shared_perm(const char *path);\n * directory while we were working. To be robust against this kind of\n * race, callers might want to try invoking the function again when it\n * returns SCLD_VANISHED.\n+ *\n+ * safe_create_leading_directories() temporarily changes path while it\n+ * is working but restores it before returning.\n+ * safe_create_leading_directories_const() doesn't modify path, even\n+ * temporarily.\n */\n enum scld_error {\n \tSCLD_OK = 0,\n-- \n2.8.1\n"},{"id":"285719","messageId":"728af2832c3e58222965521682414adb9a80932b.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 05/33] remove_dir_recursively(): add docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:46Z","receivedAt":"2016-05-06T16:13:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a docstring for the remove_dir_recursively() function and the\nREMOVE_DIR_* flags that can be passed to it.\n\nSigned-off-by: Michael Haggerty \n---\n dir.h | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/dir.h b/dir.h\nindex 301b737..5f19acc 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -262,9 +262,32 @@ extern int is_empty_dir(const char *dir);\n \n extern void setup_standard_excludes(struct dir_struct *dir);\n \n+\n+/* Constants for remove_dir_recursively: */\n+\n+/*\n+ * If a non-directory is found within path, stop and return an error.\n+ * (In this case some empty directories might already have been\n+ * removed.)\n+ */\n #define REMOVE_DIR_EMPTY_ONLY 01\n+\n+/*\n+ * If any Git work trees are found within path, skip them without\n+ * considering it an error.\n+ */\n #define REMOVE_DIR_KEEP_NESTED_GIT 02\n+\n+/* Remove the contents of path, but leave path itself. */\n #define REMOVE_DIR_KEEP_TOPLEVEL 04\n+\n+/*\n+ * Remove path and its contents, recursively. flags is a combination\n+ * of the above REMOVE_DIR_* constants. Return 0 on success.\n+ *\n+ * This function uses path as temporary scratch space, but restores it\n+ * before returning.\n+ */\n extern int remove_dir_recursively(struct strbuf *path, int flag);\n \n /* tries to remove the path with empty directories along it, ignores ENOENT */\n-- \n2.8.1\n"},{"id":"285720","messageId":"39950fef8bb45e944655e48393ee04c0b33211f5.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 06/33] refname_is_safe(): use skip_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:47Z","receivedAt":"2016-05-06T16:13:47Z","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 refs.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 87dc82f..5789152 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags)\n \n int refname_is_safe(const char *refname)\n {\n-\tif (starts_with(refname, \"refs/\")) {\n+\tconst char *rest;\n+\n+\tif (skip_prefix(refname, \"refs/\", &rest)) {\n \t\tchar *buf;\n \t\tint result;\n \n-\t\tbuf = xmallocz(strlen(refname));\n \t\t/*\n \t\t * Does the refname try to escape refs/?\n \t\t * For example: refs/foo/../bar is safe but refs/foo/../../bar\n \t\t * is not.\n \t\t */\n-\t\tresult = !normalize_path_copy(buf, refname + strlen(\"refs/\"));\n+\t\tbuf = xmallocz(strlen(rest));\n+\t\tresult = !normalize_path_copy(buf, rest);\n \t\tfree(buf);\n \t\treturn result;\n \t}\n-- \n2.8.1\n"},{"id":"285725","messageId":"35db25c65f6f77c153ef2b1183ea7821236201c8.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 07/33] refname_is_safe(): don't allow the empty string","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:48Z","receivedAt":"2016-05-06T16:13:48Z","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 refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5789152..ca0280f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -136,11 +136,12 @@ int refname_is_safe(const char *refname)\n \t\tfree(buf);\n \t\treturn result;\n \t}\n-\twhile (*refname) {\n+\n+\tdo {\n \t\tif (!isupper(*refname) && *refname != '_')\n \t\t\treturn 0;\n \t\trefname++;\n-\t}\n+\t} while (*refname);\n \treturn 1;\n }\n \n-- \n2.8.1\n"},{"id":"285747","messageId":"e40f3557f7e767bd2be2a824bc3bc2379aa69931.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 08/33] refname_is_safe(): insist that the refname already be normalized","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:49Z","receivedAt":"2016-05-06T16:13:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The reference name is going to be compared to other reference names, so\nit should be in its normalized form.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ca0280f..b18d995 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -125,14 +125,19 @@ int refname_is_safe(const char *refname)\n \tif (skip_prefix(refname, \"refs/\", &rest)) {\n \t\tchar *buf;\n \t\tint result;\n+\t\tsize_t restlen = strlen(rest);\n+\n+\t\t/* rest must not be empty, or start or end with \"/\" */\n+\t\tif (!restlen || *rest == '/' || rest[restlen - 1] == '/')\n+\t\t\treturn 0;\n \n \t\t/*\n \t\t * Does the refname try to escape refs/?\n \t\t * For example: refs/foo/../bar is safe but refs/foo/../../bar\n \t\t * is not.\n \t\t */\n-\t\tbuf = xmallocz(strlen(rest));\n-\t\tresult = !normalize_path_copy(buf, rest);\n+\t\tbuf = xmallocz(restlen);\n+\t\tresult = !normalize_path_copy(buf, rest) && !strcmp(buf, rest);\n \t\tfree(buf);\n \t\treturn result;\n \t}\n-- \n2.8.1\n"},{"id":"285745","messageId":"76fc394d50efef8f1308a0f0d56087f502dac689.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 09/33] commit_ref_update(): write error message to *err, not stderr","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:50Z","receivedAt":"2016-05-06T16:13:50Z","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 refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0cc116d..2d3a8c6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2719,7 +2719,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t}\n \t}\n \tif (commit_ref(lock)) {\n-\t\terror(\"Couldn't set %s\", lock->ref_name);\n+\t\tstrbuf_addf(err, \"Couldn't set %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-- \n2.8.1\n"},{"id":"285724","messageId":"e711b1af2ead2ffad5c510aadbbc387c7d8aa4c7.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 10/33] rename_ref(): remove unneeded local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:51Z","receivedAt":"2016-05-06T16:13:51Z","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 refs/files-backend.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2d3a8c6..80d346f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2360,20 +2360,17 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tstruct ref_lock *lock;\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n-\tconst char *symref = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n \n-\tsymref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,\n-\t\t\t\t orig_sha1, &flag);\n+\tif (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))\n+\t\treturn error(\"refname %s not found\", oldrefname);\n+\n \tif (flag & REF_ISSYMREF)\n \t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n \t\t\toldrefname);\n-\tif (!symref)\n-\t\treturn error(\"refname %s not found\", oldrefname);\n-\n \tif (!rename_ref_available(oldrefname, newrefname))\n \t\treturn 1;\n \n-- \n2.8.1\n"},{"id":"285723","messageId":"366668e3785a18ca82587ab018ea72eb597e87ba.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 11/33] ref_transaction_commit(): remove local variable n","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:52Z","receivedAt":"2016-05-06T16:13:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This microoptimization doesn't make a significant difference in speed.\nAnd it causes problems if somebody ever wants to modify the function to\nadd updates to a transaction as part of processing it, as will happen\nshortly.\n\nMake the same change in initial_ref_transaction_commit().\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 80d346f..93a94af 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3076,7 +3076,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tint ret = 0, i;\n-\tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *ref_to_delete;\n@@ -3087,13 +3086,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n-\tif (!n) {\n+\tif (!transaction->nr) {\n \t\ttransaction->state = REF_TRANSACTION_CLOSED;\n \t\treturn 0;\n \t}\n \n \t/* Fail if a refname appears more than once in the transaction: */\n-\tfor (i = 0; i < n; i++)\n+\tfor (i = 0; i < transaction->nr; i++)\n \t\tstring_list_append(&affected_refnames, updates[i]->refname);\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n@@ -3107,7 +3106,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t * lockfiles, ready to be activated. Only keep one lockfile\n \t * open at a time to avoid running out of file descriptors.\n \t */\n-\tfor (i = 0; i < n; i++) {\n+\tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif ((update->flags & REF_HAVE_NEW) &&\n@@ -3178,7 +3177,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n-\tfor (i = 0; i < n; i++) {\n+\tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->flags & REF_NEEDS_COMMIT) {\n@@ -3197,7 +3196,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \t/* Perform deletes now that updates are safely completed */\n-\tfor (i = 0; i < n; i++) {\n+\tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif (update->flags & REF_DELETING) {\n@@ -3223,7 +3222,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n cleanup:\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \n-\tfor (i = 0; i < n; i++)\n+\tfor (i = 0; i < transaction->nr; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tstring_list_clear(&refs_to_delete, 0);\n@@ -3243,7 +3242,6 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err)\n {\n \tint ret = 0, i;\n-\tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \n@@ -3253,7 +3251,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n \t/* Fail if a refname appears more than once in the transaction: */\n-\tfor (i = 0; i < n; i++)\n+\tfor (i = 0; i < transaction->nr; i++)\n \t\tstring_list_append(&affected_refnames, updates[i]->refname);\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n@@ -3276,7 +3274,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \tif (for_each_rawref(ref_present, &affected_refnames))\n \t\tdie(\"BUG: initial ref transaction called with existing refs\");\n \n-\tfor (i = 0; i < n; i++) {\n+\tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif ((update->flags & REF_HAVE_OLD) &&\n@@ -3297,7 +3295,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\tgoto cleanup;\n \t}\n \n-\tfor (i = 0; i < n; i++) {\n+\tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n \t\tif ((update->flags & REF_HAVE_NEW) &&\n-- \n2.8.1\n"},{"id":"285731","messageId":"01fdd8bc94d4d207f7043c138809163ad56ea2e4.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 12/33] read_raw_ref(): rename flags argument to type","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:53Z","receivedAt":"2016-05-06T16:13:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This will hopefully reduce confusion with the \"flags\" arguments that are\nused in many functions in this module as an input parameter to choose\nhow the function should operate.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 18 +++++++++---------\n refs/refs-internal.h | 2 +-\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 93a94af..fa8bbd6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1395,18 +1395,18 @@ static int resolve_missing_loose_ref(const char *refname,\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+ * for validating the referrent. Set REF_ISSYMREF in type.\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+ * broken. Set REF_ISBROKEN in type, 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+ * Backend-specific flags might be set in type as well, regardless of\n * outcome.\n *\n * sb_path is workspace: the caller should allocate and free it.\n@@ -1419,7 +1419,7 @@ static int resolve_missing_loose_ref(const char *refname,\n * refname will still be valid and unchanged.\n */\n int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t struct strbuf *symref, unsigned int *flags)\n+\t\t struct strbuf *symref, unsigned int *type)\n {\n \tstruct strbuf sb_contents = STRBUF_INIT;\n \tstruct strbuf sb_path = STRBUF_INIT;\n@@ -1448,7 +1448,7 @@ stat_ref:\n \tif (lstat(path, &st) < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n+\t\tif (resolve_missing_loose_ref(refname, sha1, type)) {\n \t\t\terrno = ENOENT;\n \t\t\tgoto out;\n \t\t}\n@@ -1469,7 +1469,7 @@ stat_ref:\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\t*type |= REF_ISSYMREF;\n \t\t\tret = 0;\n \t\t\tgoto out;\n \t\t}\n@@ -1482,7 +1482,7 @@ stat_ref:\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (resolve_missing_loose_ref(refname, sha1, flags)) {\n+\t\tif (resolve_missing_loose_ref(refname, sha1, type)) {\n \t\t\terrno = EISDIR;\n \t\t\tgoto out;\n \t\t}\n@@ -1519,7 +1519,7 @@ stat_ref:\n \n \t\tstrbuf_reset(symref);\n \t\tstrbuf_addstr(symref, buf);\n-\t\t*flags |= REF_ISSYMREF;\n+\t\t*type |= REF_ISSYMREF;\n \t\tret = 0;\n \t\tgoto out;\n \t}\n@@ -1530,7 +1530,7 @@ stat_ref:\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\t*type |= REF_ISBROKEN;\n \t\terrno = EINVAL;\n \t\tgoto out;\n \t}\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3a4f634..0b047f6 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,\n \t\t each_ref_fn fn, int trim, int flags, void *cb_data);\n \n int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t struct strbuf *symref, unsigned int *flags);\n+\t\t struct strbuf *symref, unsigned int *type);\n \n #endif /* REFS_REFS_INTERNAL_H */\n-- \n2.8.1\n"},{"id":"285743","messageId":"4de20012b2428116751facc0897a3d3b3a2541e8.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 13/33] read_raw_ref(): clear *type at start of function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:54Z","receivedAt":"2016-05-06T16:13:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is more convenient and less error-prone for callers.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fa8bbd6..8ced104 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1430,6 +1430,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,\n \tint ret = -1;\n \tint save_errno;\n \n+\t*type = 0;\n \tstrbuf_reset(&sb_path);\n \tstrbuf_git_path(&sb_path, \"%s\", refname);\n \tpath = sb_path.buf;\n-- \n2.8.1\n"},{"id":"285728","messageId":"a81d595d8d327a37a98cffcdf5dc798969cfd39f.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 14/33] read_raw_ref(): rename symref argument to referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:55Z","receivedAt":"2016-05-06T16:13:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"After all, it doesn't hold the symbolic reference, but rather the\nreference referred to.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 21 +++++++++++----------\n refs/refs-internal.h | 2 +-\n 2 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8ced104..fbbd48f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1393,9 +1393,10 @@ static int resolve_missing_loose_ref(const char *refname,\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 type.\n+ * If the ref is symbolic, fill in *referent with the name of the\n+ * branch to which it refers (e.g. \"refs/heads/master\") and return 0.\n+ * The caller is responsible for validating the referent. Set\n+ * REF_ISSYMREF in type.\n *\n * If the ref doesn't exist, set errno to ENOENT and return -1.\n *\n@@ -1411,15 +1412,15 @@ static int resolve_missing_loose_ref(const char *refname,\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+ * It is OK for refname to point into referent. In this case:\n+ * - if the function succeeds with REF_ISSYMREF, referent 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+ * - in all other cases, referent will be untouched, and therefore\n * refname will still be valid and unchanged.\n */\n int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t struct strbuf *symref, unsigned int *type)\n+\t\t struct strbuf *referent, unsigned int *type)\n {\n \tstruct strbuf sb_contents = STRBUF_INIT;\n \tstruct strbuf sb_path = STRBUF_INIT;\n@@ -1469,7 +1470,7 @@ stat_ref:\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\tstrbuf_swap(&sb_contents, referent);\n \t\t\t*type |= REF_ISSYMREF;\n \t\t\tret = 0;\n \t\t\tgoto out;\n@@ -1518,8 +1519,8 @@ stat_ref:\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \n-\t\tstrbuf_reset(symref);\n-\t\tstrbuf_addstr(symref, buf);\n+\t\tstrbuf_reset(referent);\n+\t\tstrbuf_addstr(referent, buf);\n \t\t*type |= REF_ISSYMREF;\n \t\tret = 0;\n \t\tgoto out;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 0b047f6..37a1a37 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,\n \t\t each_ref_fn fn, int trim, int flags, void *cb_data);\n \n int read_raw_ref(const char *refname, unsigned char *sha1,\n-\t\t struct strbuf *symref, unsigned int *type);\n+\t\t struct strbuf *referent, unsigned int *type);\n \n #endif /* REFS_REFS_INTERNAL_H */\n-- \n2.8.1\n"},{"id":"285727","messageId":"3531b94aa4ba673eafdb92ec43838fbf4e619bc2.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 15/33] read_raw_ref(): improve docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:56Z","receivedAt":"2016-05-06T16:13:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Among other things, document the (important!) requirement that input\nrefname be checked for safety before calling this function.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fbbd48f..73717dd 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1389,33 +1389,40 @@ static int resolve_missing_loose_ref(const char *refname,\n }\n \n /*\n- * Read a raw ref from the filesystem or packed refs file.\n+ * Read the specified reference from the filesystem or packed refs\n+ * file, non-recursively. Set type to describe the reference, and:\n *\n- * If the ref is a sha1, fill in sha1 and return 0.\n+ * - If refname is the name of a normal reference, fill in sha1\n+ * (leaving referent unchanged).\n *\n- * If the ref is symbolic, fill in *referent with the name of the\n- * branch to which it refers (e.g. \"refs/heads/master\") and return 0.\n- * The caller is responsible for validating the referent. Set\n- * REF_ISSYMREF in type.\n+ * - If refname is the name of a symbolic reference, write the full\n+ * name of the reference to which it refers (e.g.\n+ * \"refs/heads/master\") to referent and set the REF_ISSYMREF bit in\n+ * type (leaving sha1 unchanged). The caller is responsible for\n+ * validating that referent is a valid reference name.\n *\n- * If the ref doesn't exist, set errno to ENOENT and return -1.\n+ * WARNING: refname might be used as part of a filename, so it is\n+ * important from a security standpoint that it be safe in the sense\n+ * of refname_is_safe(). Moreover, for symrefs this function sets\n+ * referent to whatever the repository says, which might not be a\n+ * properly-formatted or even safe reference name. NEITHER INPUT NOR\n+ * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n *\n- * If the ref exists but is neither a symbolic ref nor a sha1, it is\n- * broken. Set REF_ISBROKEN in type, 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+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT\n+ * and return -1. If the ref exists but is neither a symbolic ref nor\n+ * a sha1, it is broken; set REF_ISBROKEN in type, set errno to\n+ * EINVAL, and return -1. If there is another error reading the ref,\n+ * set errno appropriately and return -1.\n *\n * Backend-specific flags might be set in type as well, regardless of\n * outcome.\n *\n- * sb_path is workspace: the caller should allocate and free it.\n+ * It is OK for refname to point into referent. If so:\n *\n- * It is OK for refname to point into referent. In this case:\n * - if the function succeeds with REF_ISSYMREF, referent will be\n- * overwritten and the memory pointed to by refname might be changed\n- * or even freed.\n+ * overwritten and the memory formerly pointed to by it might be\n+ * changed or even freed.\n+ *\n * - in all other cases, referent will be untouched, and therefore\n * refname will still be valid and unchanged.\n */\n-- \n2.8.1\n"},{"id":"285726","messageId":"1443ddf162783043699ba2bc5c9cb0995b9b08cd.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 16/33] read_raw_ref(): move docstring to header file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:57Z","receivedAt":"2016-05-06T16:13:57Z","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 refs/files-backend.c | 38 --------------------------------------\n refs/refs-internal.h | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+), 38 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 73717dd..fc0c7c1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1388,44 +1388,6 @@ static int resolve_missing_loose_ref(const char *refname,\n \treturn -1;\n }\n \n-/*\n- * Read the specified reference from the filesystem or packed refs\n- * file, non-recursively. Set type to describe the reference, and:\n- *\n- * - If refname is the name of a normal reference, fill in sha1\n- * (leaving referent unchanged).\n- *\n- * - If refname is the name of a symbolic reference, write the full\n- * name of the reference to which it refers (e.g.\n- * \"refs/heads/master\") to referent and set the REF_ISSYMREF bit in\n- * type (leaving sha1 unchanged). The caller is responsible for\n- * validating that referent is a valid reference name.\n- *\n- * WARNING: refname might be used as part of a filename, so it is\n- * important from a security standpoint that it be safe in the sense\n- * of refname_is_safe(). Moreover, for symrefs this function sets\n- * referent to whatever the repository says, which might not be a\n- * properly-formatted or even safe reference name. NEITHER INPUT NOR\n- * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n- *\n- * Return 0 on success. If the ref doesn't exist, set errno to ENOENT\n- * and return -1. If the ref exists but is neither a symbolic ref nor\n- * a sha1, it is broken; set REF_ISBROKEN in type, set errno to\n- * EINVAL, and return -1. If there is another error reading the ref,\n- * set errno appropriately and return -1.\n- *\n- * Backend-specific flags might be set in type as well, regardless of\n- * outcome.\n- *\n- * It is OK for refname to point into referent. If so:\n- *\n- * - if the function succeeds with REF_ISSYMREF, referent will be\n- * overwritten and the memory formerly pointed to by it might be\n- * changed or even freed.\n- *\n- * - in all other cases, referent will be untouched, and therefore\n- * refname will still be valid and unchanged.\n- */\n int read_raw_ref(const char *refname, unsigned char *sha1,\n \t\t struct strbuf *referent, unsigned int *type)\n {\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 37a1a37..de7722e 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -209,6 +209,44 @@ int rename_ref_available(const char *oldname, const char *newname);\n int do_for_each_ref(const char *submodule, const char *base,\n \t\t each_ref_fn fn, int trim, int flags, void *cb_data);\n \n+/*\n+ * Read the specified reference from the filesystem or packed refs\n+ * file, non-recursively. Set type to describe the reference, and:\n+ *\n+ * - If refname is the name of a normal reference, fill in sha1\n+ * (leaving referent unchanged).\n+ *\n+ * - If refname is the name of a symbolic reference, write the full\n+ * name of the reference to which it refers (e.g.\n+ * \"refs/heads/master\") to referent and set the REF_ISSYMREF bit in\n+ * type (leaving sha1 unchanged). The caller is responsible for\n+ * validating that referent is a valid reference name.\n+ *\n+ * WARNING: refname might be used as part of a filename, so it is\n+ * important from a security standpoint that it be safe in the sense\n+ * of refname_is_safe(). Moreover, for symrefs this function sets\n+ * referent to whatever the repository says, which might not be a\n+ * properly-formatted or even safe reference name. NEITHER INPUT NOR\n+ * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n+ *\n+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT\n+ * and return -1. If the ref exists but is neither a symbolic ref nor\n+ * a sha1, it is broken; set REF_ISBROKEN in type, set errno to\n+ * EINVAL, and return -1. If there is another error reading the ref,\n+ * set errno appropriately and return -1.\n+ *\n+ * Backend-specific flags might be set in type as well, regardless of\n+ * outcome.\n+ *\n+ * It is OK for refname to point into referent. If so:\n+ *\n+ * - if the function succeeds with REF_ISSYMREF, referent will be\n+ * overwritten and the memory formerly pointed to by it might be\n+ * changed or even freed.\n+ *\n+ * - in all other cases, referent will be untouched, and therefore\n+ * refname will still be valid and unchanged.\n+ */\n int read_raw_ref(const char *refname, unsigned char *sha1,\n \t\t struct strbuf *referent, unsigned int *type);\n \n-- \n2.8.1\n"},{"id":"285732","messageId":"3484a284a7e2d495bff9179aef5ecf29d309e8c1.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 17/33] lock_ref_sha1_basic(): remove unneeded local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:58Z","receivedAt":"2016-05-06T16:13:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"resolve_ref_unsafe() can cope with being called with NULL passed to its\nflags argument. So lock_ref_sha1_basic() can just hand its own type\nparameter through.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fc0c7c1..97377c7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1738,7 +1738,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const unsigned char *old_sha1,\n \t\t\t\t\t const struct string_list *extras,\n \t\t\t\t\t const struct string_list *skip,\n-\t\t\t\t\t unsigned int flags, int *type_p,\n+\t\t\t\t\t unsigned int flags, int *type,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct strbuf ref_file = STRBUF_INIT;\n@@ -1746,7 +1746,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n-\tint type;\n \tint lflags = 0;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n@@ -1766,7 +1765,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t}\n \n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n-\t\t\t\t lock->old_oid.hash, &type);\n+\t\t\t\t lock->old_oid.hash, type);\n \tif (!refname && errno == EISDIR) {\n \t\t/*\n \t\t * we are trying to lock foo but we used to\n@@ -1784,10 +1783,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n-\t\t\t\t\t lock->old_oid.hash, &type);\n+\t\t\t\t\t lock->old_oid.hash, type);\n \t}\n-\tif (type_p)\n-\t *type_p = type;\n \tif (!refname) {\n \t\tlast_errno = errno;\n \t\tif (last_errno != ENOTDIR ||\n-- \n2.8.1\n"},{"id":"285729","messageId":"b846ae21daff8fbbeb1b8ed9c8dbfb2a8035a81b.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 18/33] refs: make error messages more consistent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:13:59Z","receivedAt":"2016-05-06T16:13:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* Always start error messages with a lower-case letter.\n\n* Always enclose reference names in single quotes.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 ++++----\n refs/files-backend.c | 32 ++++++++++++++++----------------\n t/t1400-update-ref.sh | 4 ++--\n 3 files changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b18d995..ba14105 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n \tfilename = git_path(\"%s\", pseudoref);\n \tfd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);\n \tif (fd < 0) {\n-\t\tstrbuf_addf(err, \"Could not open '%s' for writing: %s\",\n+\t\tstrbuf_addf(err, \"could not open '%s' for writing: %s\",\n \t\t\t filename, strerror(errno));\n \t\treturn -1;\n \t}\n@@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,\n \t\tif (read_ref(pseudoref, actual_old_sha1))\n \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n \t\tif (hashcmp(actual_old_sha1, old_sha1)) {\n-\t\t\tstrbuf_addf(err, \"Unexpected sha1 when writing %s\", pseudoref);\n+\t\t\tstrbuf_addf(err, \"unexpected sha1 when writing '%s'\", pseudoref);\n \t\t\trollback_lock_file(&lock);\n \t\t\tgoto done;\n \t\t}\n \t}\n \n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len) {\n-\t\tstrbuf_addf(err, \"Could not write to '%s'\", filename);\n+\t\tstrbuf_addf(err, \"could not write to '%s'\", filename);\n \t\trollback_lock_file(&lock);\n \t\tgoto done;\n \t}\n@@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \n \tif (new_sha1 && !is_null_sha1(new_sha1) &&\n \t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\tstrbuf_addf(err, \"refusing to update ref with bad name %s\",\n+\t\tstrbuf_addf(err, \"refusing to update ref with bad name '%s'\",\n \t\t\t refname);\n \t\treturn -1;\n \t}\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 97377c7..5a597bb 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1701,7 +1701,7 @@ static int verify_lock(struct ref_lock *lock,\n \t\t\t lock->old_oid.hash, NULL)) {\n \t\tif (old_sha1) {\n \t\t\tint save_errno = errno;\n-\t\t\tstrbuf_addf(err, \"can't verify ref %s\", lock->ref_name);\n+\t\t\tstrbuf_addf(err, \"can't verify ref '%s'\", lock->ref_name);\n \t\t\terrno = save_errno;\n \t\t\treturn -1;\n \t\t} else {\n@@ -1710,7 +1710,7 @@ static int verify_lock(struct ref_lock *lock,\n \t\t}\n \t}\n \tif (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {\n-\t\tstrbuf_addf(err, \"ref %s is at %s but expected %s\",\n+\t\tstrbuf_addf(err, \"ref '%s' is at %s but expected %s\",\n \t\t\t lock->ref_name,\n \t\t\t sha1_to_hex(lock->old_oid.hash),\n \t\t\t sha1_to_hex(old_sha1));\n@@ -1790,7 +1790,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tif (last_errno != ENOTDIR ||\n \t\t !verify_refname_available_dir(orig_refname, extras, skip,\n \t\t\t\t\t\t get_loose_refs(&ref_cache), err))\n-\t\t\tstrbuf_addf(err, \"unable to resolve reference %s: %s\",\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n \t\t\t\t orig_refname, strerror(last_errno));\n \n \t\tgoto error_return;\n@@ -1828,7 +1828,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t/* fall through */\n \tdefault:\n \t\tlast_errno = errno;\n-\t\tstrbuf_addf(err, \"unable to create directory for %s\",\n+\t\tstrbuf_addf(err, \"unable to create directory for '%s'\",\n \t\t\t ref_file.buf);\n \t\tgoto error_return;\n \t}\n@@ -2473,7 +2473,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \tstrbuf_git_path(logfile, \"logs/%s\", refname);\n \tif (force_create || should_autocreate_reflog(refname)) {\n \t\tif (safe_create_leading_directories(logfile->buf) < 0) {\n-\t\t\tstrbuf_addf(err, \"unable to create directory for %s: \"\n+\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n \t\t\t\t \"%s\", logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n@@ -2487,7 +2487,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \n \t\tif (errno == EISDIR) {\n \t\t\tif (remove_empty_directories(logfile)) {\n-\t\t\t\tstrbuf_addf(err, \"There are still logs under \"\n+\t\t\t\tstrbuf_addf(err, \"there are still logs under \"\n \t\t\t\t\t \"'%s'\", logfile->buf);\n \t\t\t\treturn -1;\n \t\t\t}\n@@ -2495,7 +2495,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \t\t}\n \n \t\tif (logfd < 0) {\n-\t\t\tstrbuf_addf(err, \"unable to append to %s: %s\",\n+\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n \t\t\t\t logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n@@ -2564,13 +2564,13 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n \t\t\t\t git_committer_info(0), msg);\n \tif (result) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", logfile->buf,\n+\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n \t\t\t strerror(errno));\n \t\tclose(logfd);\n \t\treturn -1;\n \t}\n \tif (close(logfd)) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", logfile->buf,\n+\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n \t\t\t strerror(errno));\n \t\treturn -1;\n \t}\n@@ -2611,14 +2611,14 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \to = parse_object(sha1);\n \tif (!o) {\n \t\tstrbuf_addf(err,\n-\t\t\t \"Trying to write ref %s with nonexistent object %s\",\n+\t\t\t \"trying to write ref '%s' with nonexistent object %s\",\n \t\t\t lock->ref_name, sha1_to_hex(sha1));\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n \tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\tstrbuf_addf(err,\n-\t\t\t \"Trying to write non-commit object %s to branch %s\",\n+\t\t\t \"trying to write non-commit object %s to branch '%s'\",\n \t\t\t sha1_to_hex(sha1), lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n@@ -2628,7 +2628,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \t write_in_full(fd, &term, 1) != 1 ||\n \t close_ref(lock) < 0) {\n \t\tstrbuf_addf(err,\n-\t\t\t \"Couldn't write %s\", get_lock_file_path(lock->lk));\n+\t\t\t \"couldn't write '%s'\", get_lock_file_path(lock->lk));\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n@@ -2649,7 +2649,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n-\t\tstrbuf_addf(err, \"Cannot update the ref '%s': %s\",\n+\t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n \t\t\t lock->ref_name, old_msg);\n \t\tfree(old_msg);\n \t\tunlock_ref(lock);\n@@ -2684,7 +2684,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t}\n \t}\n \tif (commit_ref(lock)) {\n-\t\tstrbuf_addf(err, \"Couldn't set %s\", lock->ref_name);\n+\t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n@@ -3033,7 +3033,7 @@ static int ref_update_reject_duplicates(struct string_list *refnames,\n \tfor (i = 1; i < n; i++)\n \t\tif (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {\n \t\t\tstrbuf_addf(err,\n-\t\t\t\t \"Multiple updates for ref '%s' not allowed.\",\n+\t\t\t\t \"multiple updates for ref '%s' not allowed.\",\n \t\t\t\t refnames->items[i].string);\n \t\t\treturn 1;\n \t\t}\n@@ -3137,7 +3137,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t * Close it to free up the file descriptor:\n \t\t\t */\n \t\t\tif (close_ref(update->lock)) {\n-\t\t\t\tstrbuf_addf(err, \"Couldn't close %s.lock\",\n+\t\t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t\t update->refname);\n \t\t\t\tgoto cleanup;\n \t\t\t}\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex af1b20d..40b0cce 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -479,7 +479,7 @@ test_expect_success 'stdin fails with duplicate refs' '\n \tcreate $a $m\n \tEOF\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n+\tgrep \"fatal: multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n '\n \n test_expect_success 'stdin create ref works' '\n@@ -880,7 +880,7 @@ test_expect_success 'stdin -z fails option with unknown name' '\n test_expect_success 'stdin -z fails with duplicate refs' '\n \tprintf $F \"create $a\" \"$m\" \"create $b\" \"$m\" \"create $a\" \"$m\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: Multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n+\tgrep \"fatal: multiple updates for ref '\"'\"'$a'\"'\"' not allowed.\" err\n '\n \n test_expect_success 'stdin -z create ref works' '\n-- \n2.8.1\n"},{"id":"285730","messageId":"e053eafabefce5e91ef00fa69f0ff33270507de2.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 19/33] ref_transaction_create(): disallow recursive pruning","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:00Z","receivedAt":"2016-05-06T16:14:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING\nwithout REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING\ncaller to pass REF_NODEREF too.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 3 +++\n refs/files-backend.c | 2 +-\n refs/refs-internal.h | 2 +-\n 3 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ba14105..5dc2473 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: update called for transaction that is not open\");\n \n+\tif ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))\n+\t\tdie(\"BUG: REF_ISPRUNING set without REF_NODEREF\");\n+\n \tif (new_sha1 && !is_null_sha1(new_sha1) &&\n \t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\tstrbuf_addf(err, \"refusing to update ref with bad name '%s'\",\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5a597bb..7cc680a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2087,7 +2087,7 @@ static void prune_ref(struct ref_to_prune *r)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t ref_transaction_delete(transaction, r->name, r->sha1,\n-\t\t\t\t REF_ISPRUNING, NULL, &err) ||\n+\t\t\t\t REF_ISPRUNING | REF_NODEREF, NULL, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex de7722e..1f94f7a 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -15,7 +15,7 @@\n \n /*\n * Used as a flag in ref_update::flags when a loose ref is being\n- * pruned.\n+ * pruned. This flag must only be used when REF_NODEREF is set.\n */\n #define REF_ISPRUNING\t0x04\n \n-- \n2.8.1\n"},{"id":"285749","messageId":"f5bb5bf956f86d78f321b49ff444469bf274bc4c.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 20/33] ref_transaction_commit(): correctly report close_ref() failure","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:01Z","receivedAt":"2016-05-06T16:14:01Z","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 refs/files-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7cc680a..f4f7953 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3139,6 +3139,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tif (close_ref(update->lock)) {\n \t\t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t\t update->refname);\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \t\t}\n-- \n2.8.1\n"},{"id":"285742","messageId":"8a4c0c705a4694e54399580eb1abe8a61c556d7d.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 21/33] delete_branches(): use resolve_refdup()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:02Z","receivedAt":"2016-05-06T16:14:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The return value of resolve_ref_unsafe() is not guaranteed to stay\naround as long as we need it, so use resolve_refdup() instead.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/branch.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0adba62..ae55688 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -212,7 +212,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n \tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n-\t\tconst char *target;\n+\t\tchar *target = NULL;\n \t\tint flags = 0;\n \n \t\tstrbuf_branchname(&bname, argv[i]);\n@@ -231,11 +231,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t}\n \t\t}\n \n-\t\ttarget = resolve_ref_unsafe(name,\n-\t\t\t\t\t RESOLVE_REF_READING\n-\t\t\t\t\t | RESOLVE_REF_NO_RECURSE\n-\t\t\t\t\t | RESOLVE_REF_ALLOW_BAD_NAME,\n-\t\t\t\t\t sha1, &flags);\n+\t\ttarget = resolve_refdup(name,\n+\t\t\t\t\tRESOLVE_REF_READING\n+\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n+\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n+\t\t\t\t\tsha1, &flags);\n \t\tif (!target) {\n \t\t\terror(remote_branch\n \t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n@@ -248,7 +248,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t check_branch_commit(bname.buf, name, sha1, head_rev, kinds,\n \t\t\t\t\tforce)) {\n \t\t\tret = 1;\n-\t\t\tcontinue;\n+\t\t\tgoto next;\n \t\t}\n \n \t\tif (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,\n@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t : _(\"Error deleting branch '%s'\"),\n \t\t\t bname.buf);\n \t\t\tret = 1;\n-\t\t\tcontinue;\n+\t\t\tgoto next;\n \t\t}\n \t\tif (!quiet) {\n \t\t\tprintf(remote_branch\n@@ -270,6 +270,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t : find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \t\t}\n \t\tdelete_branch_config(bname.buf);\n+\n+\tnext:\n+\t\tfree(target);\n \t}\n \n \tfree(name);\n-- \n2.8.1\n"},{"id":"285750","messageId":"8cc803f8b0b5e3a38d98781095b0093114b23e32.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 22/33] refs: allow log-only updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:03Z","receivedAt":"2016-05-06T16:14:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: David Turner \n\nThe refs infrastructure learns about log-only ref updates, which only\nupdate the reflog. Later, we will use this to separate symbolic\nreference resolution from ref updating.\n\nSigned-off-by: David Turner \nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 16 ++++++++++------\n refs/refs-internal.h | 7 +++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f4f7953..3f546d0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2683,7 +2683,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t\t}\n \t\t}\n \t}\n-\tif (commit_ref(lock)) {\n+\tif (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {\n \t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n@@ -3101,7 +3101,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tgoto cleanup;\n \t\t}\n \t\tif ((update->flags & REF_HAVE_NEW) &&\n-\t\t !(update->flags & REF_DELETING)) {\n+\t\t !(update->flags & REF_DELETING) &&\n+\t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n \t\t\t\t\t\t (update->flags & REF_NODEREF));\n \n@@ -3133,8 +3134,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t}\n \t\tif (!(update->flags & REF_NEEDS_COMMIT)) {\n \t\t\t/*\n-\t\t\t * We didn't have to write anything to the lockfile.\n-\t\t\t * Close it to free up the file descriptor:\n+\t\t\t * We didn't call write_ref_to_lockfile(), so\n+\t\t\t * the lockfile is still open. Close it to\n+\t\t\t * free up the file descriptor:\n \t\t\t */\n \t\t\tif (close_ref(update->lock)) {\n \t\t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n@@ -3149,7 +3151,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif (update->flags & REF_NEEDS_COMMIT) {\n+\t\tif (update->flags & REF_NEEDS_COMMIT ||\n+\t\t update->flags & REF_LOG_ONLY) {\n \t\t\tif (commit_ref_update(update->lock,\n \t\t\t\t\t update->new_sha1, update->msg,\n \t\t\t\t\t update->flags, err)) {\n@@ -3168,7 +3171,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif (update->flags & REF_DELETING) {\n+\t\tif (update->flags & REF_DELETING &&\n+\t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 1f94f7a..85f4650 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -43,6 +43,13 @@\n */\n \n /*\n+ * Used as a flag in ref_update::flags when we want to log a ref\n+ * update but not actually perform it. This is used when a symbolic\n+ * ref update is split up.\n+ */\n+#define REF_LOG_ONLY 0x80\n+\n+/*\n * Return true iff refname is minimally safe. \"Safe\" here means that\n * deleting a loose reference by this name will not do any damage, for\n * example by causing a file that is not a reference to be deleted.\n-- \n2.8.1\n"},{"id":"285744","messageId":"c474a36117cfda48aae539817ad921629533aa16.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 23/33] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:04Z","receivedAt":"2016-05-06T16:14:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: David Turner \n\nWhen renaming refs, don't dereference either the origin or the destination\nbefore renaming.\n\nThe origin does not need to be dereferenced because it is presently\nforbidden to rename symbolic refs.\n\nNot dereferencing the destination fixes a bug where renaming on top of\na broken symref would use the pointed-to ref name for the moved\nreflog.\n\nAdd a test for the reflog bug.\n\nSigned-off-by: David Turner \nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 21 ++++++++++++++++-----\n t/t3200-branch.sh | 9 +++++++++\n 2 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3f546d0..ad53b6e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2333,7 +2333,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n \n-\tif (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1, &flag))\n+\tif (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\torig_sha1, &flag))\n \t\treturn error(\"refname %s not found\", oldrefname);\n \n \tif (flag & REF_ISSYMREF)\n@@ -2351,8 +2352,16 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \t\tgoto rollback;\n \t}\n \n-\tif (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&\n-\t delete_ref(newrefname, sha1, REF_NODEREF)) {\n+\t/*\n+\t * Since we are doing a shallow lookup, sha1 is not the\n+\t * correct value to pass to delete_ref as old_sha1. But that\n+\t * doesn't matter, because an old_sha1 check wouldn't add to\n+\t * the safety anyway; we want to delete the reference whatever\n+\t * its current value.\n+\t */\n+\tif (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t sha1, NULL) &&\n+\t delete_ref(newrefname, NULL, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tstruct strbuf path = STRBUF_INIT;\n \t\t\tint result;\n@@ -2376,7 +2385,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tlogmoved = log;\n \n-\tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);\n+\tlock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF,\n+\t\t\t\t NULL, &err);\n \tif (!lock) {\n \t\terror(\"unable to rename '%s' to '%s': %s\", oldrefname, newrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -2394,7 +2404,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \treturn 0;\n \n rollback:\n-\tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);\n+\tlock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF,\n+\t\t\t\t NULL, &err);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for rollback: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f3e3b6c..4281160 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -79,6 +79,15 @@ test_expect_success 'git branch -m dumps usage' '\n \ttest_i18ngrep \"branch name required\" err\n '\n \n+test_expect_success 'git branch -m m broken_symref should work' '\n+\ttest_when_finished \"git branch -D broken_symref\" &&\n+\tgit branch -l m &&\n+\tgit symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken &&\n+\tgit branch -m m broken_symref &&\n+\tgit reflog exists refs/heads/broken_symref &&\n+\ttest_must_fail git reflog exists refs/heads/i_am_broken\n+'\n+\n test_expect_success 'git branch -m m m/m should work' '\n \tgit branch -l m &&\n \tgit branch -m m m/m &&\n-- \n2.8.1\n"},{"id":"285734","messageId":"b2d414be0f99be4a0f9d7325eb43a1eb85dd7fa9.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 24/33] verify_refname_available(): adjust constness in declaration","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:05Z","receivedAt":"2016-05-06T16:14:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The two string_list arguments can be const.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 4 ++--\n refs/refs-internal.h | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ad53b6e..f0eef9e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2299,8 +2299,8 @@ out:\n }\n \n int verify_refname_available(const char *newname,\n-\t\t\t struct string_list *extras,\n-\t\t\t struct string_list *skip,\n+\t\t\t const struct string_list *extras,\n+\t\t\t const struct string_list *skip,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_dir *packed_refs = get_packed_refs(&ref_cache);\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 85f4650..9686e60 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -116,8 +116,8 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1);\n * extras and skip must be sorted.\n */\n int verify_refname_available(const char *newname,\n-\t\t\t struct string_list *extras,\n-\t\t\t struct string_list *skip,\n+\t\t\t const struct string_list *extras,\n+\t\t\t const struct string_list *skip,\n \t\t\t struct strbuf *err);\n \n /*\n-- \n2.8.1\n"},{"id":"285740","messageId":"64ba0ef8de63267f9625ce8303804a0814d1ecc6.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 25/33] add_update(): initialize the whole ref_update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:06Z","receivedAt":"2016-05-06T16:14:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change add_update() to initialize all of the fields in the new\nref_update object. Rename the function to ref_transaction_add_update(),\nand increase its visibility to all of the refs-related code.\n\nAll of this makes the function more useful for other future callers.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 48 ++++++++++++++++++++++++++----------------------\n refs/refs-internal.h | 14 ++++++++++++++\n 2 files changed, 40 insertions(+), 22 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5dc2473..7c4eeb1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -766,13 +766,33 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n-static struct ref_update *add_update(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname)\n+struct ref_update *ref_transaction_add_update(\n+\t\tstruct ref_transaction *transaction,\n+\t\tconst char *refname, unsigned int flags,\n+\t\tconst unsigned char *new_sha1,\n+\t\tconst unsigned char *old_sha1,\n+\t\tconst char *msg)\n {\n \tstruct ref_update *update;\n+\n+\tif (transaction->state != REF_TRANSACTION_OPEN)\n+\t\tdie(\"BUG: update called for transaction that is not open\");\n+\n+\tif ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))\n+\t\tdie(\"BUG: REF_ISPRUNING set without REF_NODEREF\");\n+\n \tFLEX_ALLOC_STR(update, refname, refname);\n \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n \ttransaction->updates[transaction->nr++] = update;\n+\n+\tupdate->flags = flags;\n+\n+\tif (flags & REF_HAVE_NEW)\n+\t\thashcpy(update->new_sha1, new_sha1);\n+\tif (flags & REF_HAVE_OLD)\n+\t\thashcpy(update->old_sha1, old_sha1);\n+\tif (msg)\n+\t\tupdate->msg = xstrdup(msg);\n \treturn update;\n }\n \n@@ -783,16 +803,8 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n-\tstruct ref_update *update;\n-\n \tassert(err);\n \n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n-\t\tdie(\"BUG: update called for transaction that is not open\");\n-\n-\tif ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))\n-\t\tdie(\"BUG: REF_ISPRUNING set without REF_NODEREF\");\n-\n \tif (new_sha1 && !is_null_sha1(new_sha1) &&\n \t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\tstrbuf_addf(err, \"refusing to update ref with bad name '%s'\",\n@@ -800,18 +812,10 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\tupdate = add_update(transaction, refname);\n-\tif (new_sha1) {\n-\t\thashcpy(update->new_sha1, new_sha1);\n-\t\tflags |= REF_HAVE_NEW;\n-\t}\n-\tif (old_sha1) {\n-\t\thashcpy(update->old_sha1, old_sha1);\n-\t\tflags |= REF_HAVE_OLD;\n-\t}\n-\tupdate->flags = flags;\n-\tif (msg)\n-\t\tupdate->msg = xstrdup(msg);\n+\tflags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);\n+\n+\tref_transaction_add_update(transaction, refname, flags,\n+\t\t\t\t new_sha1, old_sha1, msg);\n \treturn 0;\n }\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 9686e60..babdf27 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -158,6 +158,20 @@ struct ref_update {\n };\n \n /*\n+ * Add a ref_update with the specified properties to transaction, and\n+ * return a pointer to the new object. This function does not verify\n+ * that refname is well-formed. new_sha1 and old_sha1 are only\n+ * dereferenced if the REF_HAVE_NEW and REF_HAVE_OLD bits,\n+ * respectively, are set in flags.\n+ */\n+struct ref_update *ref_transaction_add_update(\n+\t\tstruct ref_transaction *transaction,\n+\t\tconst char *refname, unsigned int flags,\n+\t\tconst unsigned char *new_sha1,\n+\t\tconst unsigned char *old_sha1,\n+\t\tconst char *msg);\n+\n+/*\n * Transaction states.\n * OPEN: The transaction is in a valid state and can accept new updates.\n * An OPEN transaction can be committed.\n-- \n2.8.1\n"},{"id":"285733","messageId":"bfba3b42460c75064ce0538cbd6ad820a7ddc537.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 26/33] lock_ref_for_update(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:07Z","receivedAt":"2016-05-06T16:14:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract a new function, lock_ref_for_update(), from\nref_transaction_commit().\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 152 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 85 insertions(+), 67 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f0eef9e..1c20af5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3051,6 +3051,88 @@ static int ref_update_reject_duplicates(struct string_list *refnames,\n \treturn 0;\n }\n \n+/*\n+ * Acquire all locks, verify old values if provided, check\n+ * that new values are valid, and write new values to the\n+ * lockfiles, ready to be activated. Only keep one lockfile\n+ * open at a time to avoid running out of file descriptors.\n+ */\n+static int lock_ref_for_update(struct ref_update *update,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t struct string_list *affected_refnames,\n+\t\t\t struct strbuf *err)\n+{\n+\tint ret;\n+\n+\tif ((update->flags & REF_HAVE_NEW) &&\n+\t is_null_sha1(update->new_sha1))\n+\t\tupdate->flags |= REF_DELETING;\n+\tupdate->lock = lock_ref_sha1_basic(\n+\t\t\tupdate->refname,\n+\t\t\t((update->flags & REF_HAVE_OLD) ?\n+\t\t\t update->old_sha1 : NULL),\n+\t\t\taffected_refnames, NULL,\n+\t\t\tupdate->flags,\n+\t\t\t&update->type,\n+\t\t\terr);\n+\tif (!update->lock) {\n+\t\tchar *reason;\n+\n+\t\tret = (errno == ENOTDIR)\n+\t\t\t? TRANSACTION_NAME_CONFLICT\n+\t\t\t: TRANSACTION_GENERIC_ERROR;\n+\t\treason = strbuf_detach(err, NULL);\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n+\t\t\t update->refname, reason);\n+\t\tfree(reason);\n+\t\treturn ret;\n+\t}\n+\tif ((update->flags & REF_HAVE_NEW) &&\n+\t !(update->flags & REF_DELETING) &&\n+\t !(update->flags & REF_LOG_ONLY)) {\n+\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n+\t\t\t\t\t (update->flags & REF_NODEREF));\n+\n+\t\tif (!overwriting_symref &&\n+\t\t !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {\n+\t\t\t/*\n+\t\t\t * The reference already has the desired\n+\t\t\t * value, so we don't need to write it.\n+\t\t\t */\n+\t\t} else if (write_ref_to_lockfile(update->lock,\n+\t\t\t\t\t\t update->new_sha1,\n+\t\t\t\t\t\t err)) {\n+\t\t\tchar *write_err = strbuf_detach(err, NULL);\n+\n+\t\t\t/*\n+\t\t\t * The lock was freed upon failure of\n+\t\t\t * write_ref_to_lockfile():\n+\t\t\t */\n+\t\t\tupdate->lock = NULL;\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t \"cannot update the ref '%s': %s\",\n+\t\t\t\t update->refname, write_err);\n+\t\t\tfree(write_err);\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t} else {\n+\t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n+\t\t}\n+\t}\n+\tif (!(update->flags & REF_NEEDS_COMMIT)) {\n+\t\t/*\n+\t\t * We didn't call write_ref_to_lockfile(), so\n+\t\t * the lockfile is still open. Close it to\n+\t\t * free up the file descriptor:\n+\t\t */\n+\t\tif (close_ref(update->lock)) {\n+\t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n+\t\t\t\t update->refname);\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n@@ -3088,74 +3170,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tif ((update->flags & REF_HAVE_NEW) &&\n-\t\t is_null_sha1(update->new_sha1))\n-\t\t\tupdate->flags |= REF_DELETING;\n-\t\tupdate->lock = lock_ref_sha1_basic(\n-\t\t\t\tupdate->refname,\n-\t\t\t\t((update->flags & REF_HAVE_OLD) ?\n-\t\t\t\t update->old_sha1 : NULL),\n-\t\t\t\t&affected_refnames, NULL,\n-\t\t\t\tupdate->flags,\n-\t\t\t\t&update->type,\n-\t\t\t\terr);\n-\t\tif (!update->lock) {\n-\t\t\tchar *reason;\n-\n-\t\t\tret = (errno == ENOTDIR)\n-\t\t\t\t? TRANSACTION_NAME_CONFLICT\n-\t\t\t\t: TRANSACTION_GENERIC_ERROR;\n-\t\t\treason = strbuf_detach(err, NULL);\n-\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n-\t\t\t\t update->refname, reason);\n-\t\t\tfree(reason);\n+\t\tret = lock_ref_for_update(update, transaction,\n+\t\t\t\t\t &affected_refnames, err);\n+\t\tif (ret)\n \t\t\tgoto cleanup;\n-\t\t}\n-\t\tif ((update->flags & REF_HAVE_NEW) &&\n-\t\t !(update->flags & REF_DELETING) &&\n-\t\t !(update->flags & REF_LOG_ONLY)) {\n-\t\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n-\t\t\t\t\t\t (update->flags & REF_NODEREF));\n-\n-\t\t\tif (!overwriting_symref &&\n-\t\t\t !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {\n-\t\t\t\t/*\n-\t\t\t\t * The reference already has the desired\n-\t\t\t\t * value, so we don't need to write it.\n-\t\t\t\t */\n-\t\t\t} else if (write_ref_to_lockfile(update->lock,\n-\t\t\t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t\t\t err)) {\n-\t\t\t\tchar *write_err = strbuf_detach(err, NULL);\n-\n-\t\t\t\t/*\n-\t\t\t\t * The lock was freed upon failure of\n-\t\t\t\t * write_ref_to_lockfile():\n-\t\t\t\t */\n-\t\t\t\tupdate->lock = NULL;\n-\t\t\t\tstrbuf_addf(err,\n-\t\t\t\t\t \"cannot update the ref '%s': %s\",\n-\t\t\t\t\t update->refname, write_err);\n-\t\t\t\tfree(write_err);\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n-\t\t\t} else {\n-\t\t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n-\t\t\t}\n-\t\t}\n-\t\tif (!(update->flags & REF_NEEDS_COMMIT)) {\n-\t\t\t/*\n-\t\t\t * We didn't call write_ref_to_lockfile(), so\n-\t\t\t * the lockfile is still open. Close it to\n-\t\t\t * free up the file descriptor:\n-\t\t\t */\n-\t\t\tif (close_ref(update->lock)) {\n-\t\t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n-\t\t\t\t\t update->refname);\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n-\t\t\t}\n-\t\t}\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n-- \n2.8.1\n"},{"id":"285739","messageId":"e76e0d4119aba05bfce6b4c4d0fe8d7f9d05c132.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 27/33] unlock_ref(): move definition higher in the file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:08Z","receivedAt":"2016-05-06T16:14:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This avoids the need for a forward declaration in the next patch.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1c20af5..8ff76ed 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1516,6 +1516,16 @@ out:\n \treturn ret;\n }\n \n+static void unlock_ref(struct ref_lock *lock)\n+{\n+\t/* Do not free lock->lk -- atexit() still looks at them */\n+\tif (lock->lk)\n+\t\trollback_lock_file(lock->lk);\n+\tfree(lock->ref_name);\n+\tfree(lock->orig_ref_name);\n+\tfree(lock);\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@@ -1674,16 +1684,6 @@ int do_for_each_ref(const char *submodule, const char *base,\n \treturn do_for_each_entry(refs, base, do_one_ref, &data);\n }\n \n-static void unlock_ref(struct ref_lock *lock)\n-{\n-\t/* Do not free lock->lk -- atexit() still looks at them */\n-\tif (lock->lk)\n-\t\trollback_lock_file(lock->lk);\n-\tfree(lock->ref_name);\n-\tfree(lock->orig_ref_name);\n-\tfree(lock);\n-}\n-\n /*\n * Verify that the reference locked by lock has the value old_sha1.\n * Fail if the reference doesn't exist and mustexist is set. Return 0\n-- \n2.8.1\n"},{"id":"285738","messageId":"3da1f37d0476d477d0d640b00c0a1111bf2a8fc7.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 28/33] ref_transaction_update(): check refname_is_safe() at a minimum","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:09Z","receivedAt":"2016-05-06T16:14:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the user has asked that a new value be set for a reference, we use\ncheck_refname_format() to verify that the reference name satisfies all\nof the rules. But in other cases, at least check that refname_is_safe().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 +++--\n t/t1400-update-ref.sh | 2 +-\n t/t1430-bad-ref-name.sh | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7c4eeb1..842c5c7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -805,8 +805,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\n {\n \tassert(err);\n \n-\tif (new_sha1 && !is_null_sha1(new_sha1) &&\n-\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\tif ((new_sha1 && !is_null_sha1(new_sha1)) ?\n+\t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :\n+\t !refname_is_safe(refname)) {\n \t\tstrbuf_addf(err, \"refusing to update ref with bad name '%s'\",\n \t\t\t refname);\n \t\treturn -1;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 40b0cce..08bd8fd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -23,7 +23,7 @@ test_expect_success setup '\n m=refs/heads/master\n n_dir=refs/heads/gu\n n=$n_dir/fixes\n-outside=foo\n+outside=refs/foo\n \n test_expect_success \\\n \t\"create $m\" \\\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex 25ddab4..8937e25 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in .git dir' '\n \techo precious >expect &&\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_i18ngrep -e \"refusing to update ref with bad name\" error &&\n \ttest_cmp expect .git/my-private-file\n '\n \n-- \n2.8.1\n"},{"id":"285741","messageId":"66d33af21bd1e398973414435af43d06f2e2099c.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:10Z","receivedAt":"2016-05-06T16:14:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Before committing ref updates, split symbolic ref updates into two\nparts: an update to the underlying ref, and a log-only update to the\nsymbolic ref. This ensures that both references are locked correctly\nduring the transaction, including while their reflogs are updated.\n\nSimilarly, if the reference pointed to by HEAD is modified directly, add\na separate log-only update to HEAD, rather than leaving the job of\nupdating HEAD's reflog to commit_ref_update(). This change ensures that\nHEAD is locked correctly while its reflog is being modified, as well as\nbeing cheaper (HEAD only needs to be resolved once).\n\nThis makes use of a new function, lock_ref_raw(), which is analogous to\nread_ref_raw(), but acquires a lock on the reference before reading it.\n\nThis change still has two problems:\n\n* There are redundant read_ref_full() reference lookups.\n\n* It is still possible to get incorrect reflogs for symbolic references\n if there is a concurrent update by another process, since the old_oid\n of a symref is determined before the lock on the pointed-to ref is\n held.\n\nBoth problems will soon be fixed.\n\nSigned-off-by: David Turner \nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 513 +++++++++++++++++++++++++++++++++++++++++++++-----\n refs/refs-internal.h | 11 +-\n t/t1400-update-ref.sh | 35 ++++\n 3 files changed, 514 insertions(+), 45 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8ff76ed..799866f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1527,6 +1527,228 @@ static void unlock_ref(struct ref_lock *lock)\n }\n \n /*\n+ * Lock refname, without following symrefs, and set *lock_p to point\n+ * at a newly-allocated lock object. Fill in lock->old_oid, referent,\n+ * and type similarly to read_raw_ref().\n+ *\n+ * The caller must verify that refname is a \"safe\" reference name (in\n+ * the sense of refname_is_safe()) before calling this function.\n+ *\n+ * If the reference doesn't already exist, verify that refname doesn't\n+ * have a D/F conflict with any existing references. extras and skip\n+ * are passed to verify_refname_available_dir() for this check.\n+ *\n+ * If mustexist is not set and the reference is not found or is\n+ * broken, lock the reference anyway but clear sha1.\n+ *\n+ * Return 0 on success. On failure, write an error message to err and\n+ * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.\n+ *\n+ * Implementation note: This function is basically\n+ *\n+ * lock reference\n+ * read_raw_ref()\n+ *\n+ * but it includes a lot more code to\n+ * - Deal with possible races with other processes\n+ * - Avoid calling verify_refname_available_dir() when it can be\n+ * avoided, namely if we were successfully able to read the ref\n+ * - Generate informative error messages in the case of failure\n+ */\n+static int lock_raw_ref(const char *refname, int mustexist,\n+\t\t\tconst struct string_list *extras,\n+\t\t\tconst struct string_list *skip,\n+\t\t\tstruct ref_lock **lock_p,\n+\t\t\tstruct strbuf *referent,\n+\t\t\tunsigned int *type,\n+\t\t\tstruct strbuf *err)\n+{\n+\tstruct ref_lock *lock;\n+\tstruct strbuf ref_file = STRBUF_INIT;\n+\tint attempts_remaining = 3;\n+\tint ret = TRANSACTION_GENERIC_ERROR;\n+\n+\tassert(err);\n+\t*type = 0;\n+\n+\t/* First lock the file so it can't change out from under us. */\n+\n+\t*lock_p = lock = xcalloc(1, sizeof(*lock));\n+\n+\tlock->ref_name = xstrdup(refname);\n+\tlock->orig_ref_name = xstrdup(refname);\n+\tstrbuf_git_path(&ref_file, \"%s\", refname);\n+\n+retry:\n+\tswitch (safe_create_leading_directories(ref_file.buf)) {\n+\tcase SCLD_OK:\n+\t\tbreak; /* success */\n+\tcase SCLD_EXISTS:\n+\t\t/*\n+\t\t * Suppose refname is \"refs/foo/bar\". We just failed\n+\t\t * to create the containing directory, \"refs/foo\",\n+\t\t * because there was a non-directory in the way. This\n+\t\t * indicates a D/F conflict, probably because of\n+\t\t * another reference such as \"refs/foo\". There is no\n+\t\t * reason to expect this error to be transitory.\n+\t\t */\n+\t\tif (verify_refname_available(refname, extras, skip, err)) {\n+\t\t\tif (mustexist) {\n+\t\t\t\t/*\n+\t\t\t\t * To the user the relevant error is\n+\t\t\t\t * that the \"mustexist\" reference is\n+\t\t\t\t * missing:\n+\t\t\t\t */\n+\t\t\t\tstrbuf_reset(err);\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * The error message set by\n+\t\t\t\t * verify_refname_available_dir() is OK.\n+\t\t\t\t */\n+\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t}\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The file that is in the way isn't a loose\n+\t\t\t * reference. Report it as a low-level\n+\t\t\t * failure.\n+\t\t\t */\n+\t\t\tstrbuf_addf(err, \"unable to create lock file %s.lock; \"\n+\t\t\t\t \"non-directory in the way\",\n+\t\t\t\t ref_file.buf);\n+\t\t}\n+\t\tgoto error_return;\n+\tcase SCLD_VANISHED:\n+\t\t/* Maybe another process was tidying up. Try again. */\n+\t\tif (--attempts_remaining > 0)\n+\t\t\tgoto retry;\n+\t\t/* fall through */\n+\tdefault:\n+\t\tstrbuf_addf(err, \"unable to create directory for %s\",\n+\t\t\t ref_file.buf);\n+\t\tgoto error_return;\n+\t}\n+\n+\tif (!lock->lk)\n+\t\tlock->lk = xcalloc(1, sizeof(struct lock_file));\n+\n+\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {\n+\t\tif (errno == ENOENT && --attempts_remaining > 0) {\n+\t\t\t/*\n+\t\t\t * Maybe somebody just deleted one of the\n+\t\t\t * directories leading to ref_file. Try\n+\t\t\t * again:\n+\t\t\t */\n+\t\t\tgoto retry;\n+\t\t} else {\n+\t\t\tunable_to_lock_message(ref_file.buf, errno, err);\n+\t\t\tgoto error_return;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Now we hold the lock and can read the reference without\n+\t * fear that its value will change.\n+\t */\n+\n+\tif (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {\n+\t\tif (errno == ENOENT) {\n+\t\t\tif (mustexist) {\n+\t\t\t\t/* Garden variety missing reference. */\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t\tgoto error_return;\n+\t\t\t} else if (verify_refname_available_dir(\n+\t\t\t\t\t\t refname, extras, skip,\n+\t\t\t\t\t\t get_loose_refs(&ref_cache),\n+\t\t\t\t\t\t err)) {\n+\t\t\t\t/*\n+\t\t\t\t * The error message set by\n+\t\t\t\t * verify_refname_available() is OK.\n+\t\t\t\t */\n+\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t\tgoto error_return;\n+\t\t\t} else {\n+\t\t\t\t/* Reference is missing, but that's OK */\n+\t\t\t}\n+\t\t} else if (errno == EISDIR) {\n+\t\t\t/*\n+\t\t\t * There is a directory in the way. It might have\n+\t\t\t * contained references that have been deleted. If\n+\t\t\t * we don't require that the reference already\n+\t\t\t * exists, try to remove the directory so that it\n+\t\t\t * doesn't cause trouble when we want to rename the\n+\t\t\t * lockfile into place later.\n+\t\t\t */\n+\t\t\tif (mustexist) {\n+\t\t\t\t/* Garden variety missing reference. */\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t\tgoto error_return;\n+\t\t\t} else if (remove_dir_recursively(&ref_file,\n+\t\t\t\t\t\t\t REMOVE_DIR_EMPTY_ONLY)) {\n+\t\t\t\tif (verify_refname_available_dir(\n+\t\t\t\t\t\t refname, extras, skip,\n+\t\t\t\t\t\t get_loose_refs(&ref_cache),\n+\t\t\t\t\t\t err)) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * The error message set by\n+\t\t\t\t\t * verify_refname_available() is OK.\n+\t\t\t\t\t */\n+\t\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t\t\tgoto error_return;\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * We can't delete the directory,\n+\t\t\t\t\t * but we also don't know of any\n+\t\t\t\t\t * references that it should\n+\t\t\t\t\t * contain.\n+\t\t\t\t\t */\n+\t\t\t\t\tstrbuf_addf(err, \"there is a non-empty directory '%s' \"\n+\t\t\t\t\t\t \"blocking reference '%s'\",\n+\t\t\t\t\t\t ref_file.buf, refname);\n+\t\t\t\t\tgoto error_return;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': \"\n+\t\t\t\t \"reference broken\", refname);\n+\t\t\tgoto error_return;\n+\t\t} else {\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n+\t\t\t\t refname, strerror(errno));\n+\t\t\tgoto error_return;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If the ref did not exist and we are creating it,\n+\t\t * make sure there is no existing packed ref whose\n+\t\t * name begins with our refname, nor a packed ref\n+\t\t * whose name is a proper prefix of our refname.\n+\t\t */\n+\t\tif (verify_refname_available_dir(\n+\t\t\t\t refname, extras, skip,\n+\t\t\t\t get_packed_refs(&ref_cache),\n+\t\t\t\t err)) {\n+\t\t\tgoto error_return;\n+\t\t}\n+\t}\n+\n+\tret = 0;\n+\tgoto out;\n+\n+error_return:\n+\tunlock_ref(lock);\n+\t*lock_p = NULL;\n+\n+out:\n+\tstrbuf_release(&ref_file);\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 * value that is already stored in it.\n@@ -3052,55 +3274,201 @@ static int ref_update_reject_duplicates(struct string_list *refnames,\n }\n \n /*\n- * Acquire all locks, verify old values if provided, check\n- * that new values are valid, and write new values to the\n- * lockfiles, ready to be activated. Only keep one lockfile\n- * open at a time to avoid running out of file descriptors.\n+ * If update is a direct update of head_ref (the reference pointed to\n+ * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.\n+ */\n+static int split_head_update(struct ref_update *update,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *head_ref,\n+\t\t\t struct string_list *affected_refnames,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct string_list_item *item;\n+\tstruct ref_update *new_update;\n+\n+\tif ((update->flags & REF_LOG_ONLY) ||\n+\t (update->flags & REF_ISPRUNING) ||\n+\t (update->flags & REF_UPDATE_VIA_HEAD))\n+\t\treturn 0;\n+\n+\tif (strcmp(update->refname, head_ref))\n+\t\treturn 0;\n+\n+\t/*\n+\t * First make sure that HEAD is not already in the\n+\t * transaction. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per transaction.\n+\t */\n+\titem = string_list_insert(affected_refnames, \"HEAD\");\n+\tif (item->util) {\n+\t\t/* An entry already existed */\n+\t\tstrbuf_addf(err,\n+\t\t\t \"multiple updates for 'HEAD' (including one \"\n+\t\t\t \"via its referent '%s') are not allowed\",\n+\t\t\t update->refname);\n+\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t}\n+\n+\tnew_update = ref_transaction_add_update(\n+\t\t\ttransaction, \"HEAD\",\n+\t\t\tupdate->flags | REF_LOG_ONLY | REF_NODEREF,\n+\t\t\tupdate->new_sha1, update->old_sha1,\n+\t\t\tupdate->msg);\n+\n+\titem->util = new_update;\n+\n+\treturn 0;\n+}\n+\n+/*\n+ * update is for a symref that points at referent and doesn't have\n+ * REF_NODEREF set. Split it into two updates:\n+ * - The original update, but with REF_LOG_ONLY and REF_NODEREF set\n+ * - A new, separate update for the referent reference\n+ * Note that the new update will itself be subject to splitting when\n+ * the iteration gets to it.\n+ */\n+static int split_symref_update(struct ref_update *update,\n+\t\t\t const char *referent,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t struct string_list *affected_refnames,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct string_list_item *item;\n+\tstruct ref_update *new_update;\n+\tunsigned int new_flags;\n+\n+\t/*\n+\t * First make sure that referent is not already in the\n+\t * transaction. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per symref in a\n+\t * transaction.\n+\t */\n+\titem = string_list_insert(affected_refnames, referent);\n+\tif (item->util) {\n+\t\t/* An entry already existed */\n+\t\tstrbuf_addf(err,\n+\t\t\t \"multiple updates for '%s' (including one \"\n+\t\t\t \"via symref '%s') are not allowed\",\n+\t\t\t referent, update->refname);\n+\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t}\n+\n+\tnew_flags = update->flags;\n+\tif (!strcmp(update->refname, \"HEAD\")) {\n+\t\t/*\n+\t\t * Record that the new update came via HEAD, so that\n+\t\t * when we process it, split_head_update() doesn't try\n+\t\t * to add another reflog update for HEAD. Note that\n+\t\t * this bit will be propagated if the new_update\n+\t\t * itself needs to be split.\n+\t\t */\n+\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n+\t}\n+\n+\tnew_update = ref_transaction_add_update(\n+\t\t\ttransaction, referent, new_flags,\n+\t\t\tupdate->new_sha1, update->old_sha1,\n+\t\t\tupdate->msg);\n+\n+\t/* Change the symbolic ref update to log only: */\n+\tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n+\n+\titem->util = new_update;\n+\n+\treturn 0;\n+}\n+\n+/*\n+ * Prepare for carrying out update:\n+ * - Lock the reference referred to by update.\n+ * - Read the reference under lock.\n+ * - Check that its old SHA-1 value (if specified) is correct, and in\n+ * any case record it for later use in the reflog.\n+ * - If it is a symref update without REF_NODEREF, split it up into a\n+ * REF_LOG_ONLY update of the symref and add a separate update for\n+ * the referent to transaction.\n+ * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY\n+ * update of HEAD.\n */\n static int lock_ref_for_update(struct ref_update *update,\n \t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *head_ref,\n \t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n+\tstruct strbuf referent = STRBUF_INIT;\n+\tint mustexist = (update->flags & REF_HAVE_OLD) &&\n+\t\t!is_null_sha1(update->old_sha1);\n \tint ret;\n+\tstruct ref_lock *lock;\n \n-\tif ((update->flags & REF_HAVE_NEW) &&\n-\t is_null_sha1(update->new_sha1))\n+\tif ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n \t\tupdate->flags |= REF_DELETING;\n-\tupdate->lock = lock_ref_sha1_basic(\n-\t\t\tupdate->refname,\n-\t\t\t((update->flags & REF_HAVE_OLD) ?\n-\t\t\t update->old_sha1 : NULL),\n-\t\t\taffected_refnames, NULL,\n-\t\t\tupdate->flags,\n-\t\t\t&update->type,\n-\t\t\terr);\n-\tif (!update->lock) {\n+\n+\tif (head_ref) {\n+\t\tret = split_head_update(update, transaction, head_ref,\n+\t\t\t\t\taffected_refnames, err);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t}\n+\n+\tret = lock_raw_ref(update->refname, mustexist,\n+\t\t\t affected_refnames, NULL,\n+\t\t\t &update->lock, &referent,\n+\t\t\t &update->type, err);\n+\n+\tif (ret) {\n \t\tchar *reason;\n \n-\t\tret = (errno == ENOTDIR)\n-\t\t\t? TRANSACTION_NAME_CONFLICT\n-\t\t\t: TRANSACTION_GENERIC_ERROR;\n \t\treason = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t update->refname, reason);\n \t\tfree(reason);\n \t\treturn ret;\n \t}\n+\n+\tlock = update->lock;\n+\n+\tif (read_ref_full(update->refname,\n+\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n+\t\t\t lock->old_oid.hash, NULL)) {\n+\t\tif (update->flags & REF_HAVE_OLD) {\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': can't resolve old value\",\n+\t\t\t\t update->refname);\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t} else {\n+\t\t\thashclr(lock->old_oid.hash);\n+\t\t}\n+\t}\n+\tif ((update->flags & REF_HAVE_OLD) &&\n+\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t\t update->refname,\n+\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t sha1_to_hex(update->old_sha1));\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t}\n+\n+\tif (update->type & REF_ISSYMREF) {\n+\t\tif (!(update->flags & REF_NODEREF)) {\n+\t\t\tret = split_symref_update(update, referent.buf, transaction,\n+\t\t\t\t\t\t affected_refnames, err);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t}\n+\t}\n+\n \tif ((update->flags & REF_HAVE_NEW) &&\n \t !(update->flags & REF_DELETING) &&\n \t !(update->flags & REF_LOG_ONLY)) {\n-\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n-\t\t\t\t\t (update->flags & REF_NODEREF));\n-\n-\t\tif (!overwriting_symref &&\n-\t\t !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {\n+\t\tif (!(update->type & REF_ISSYMREF) &&\n+\t\t !hashcmp(lock->old_oid.hash, update->new_sha1)) {\n \t\t\t/*\n \t\t\t * The reference already has the desired\n \t\t\t * value, so we don't need to write it.\n \t\t\t */\n-\t\t} else if (write_ref_to_lockfile(update->lock,\n-\t\t\t\t\t\t update->new_sha1,\n+\t\t} else if (write_ref_to_lockfile(lock, update->new_sha1,\n \t\t\t\t\t\t err)) {\n \t\t\tchar *write_err = strbuf_detach(err, NULL);\n \n@@ -3124,7 +3492,7 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t * the lockfile is still open. Close it to\n \t\t * free up the file descriptor:\n \t\t */\n-\t\tif (close_ref(update->lock)) {\n+\t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n \t\t\treturn TRANSACTION_GENERIC_ERROR;\n@@ -3141,6 +3509,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *ref_to_delete;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n+\tchar *head_ref = NULL;\n+\tint head_type;\n+\tstruct object_id head_oid;\n \n \tassert(err);\n \n@@ -3152,9 +3523,25 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Fail if a refname appears more than once in the transaction: */\n-\tfor (i = 0; i < transaction->nr; i++)\n-\t\tstring_list_append(&affected_refnames, updates[i]->refname);\n+\t/*\n+\t * Fail if a refname appears more than once in the\n+\t * transaction. (If we end up splitting up any updates using\n+\t * split_symref_update() or split_head_update(), those\n+\t * functions will check that the new updates don't have the\n+\t * same refname as any existing ones.)\n+\t */\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\t\tstruct string_list_item *item =\n+\t\t\tstring_list_append(&affected_refnames, update->refname);\n+\n+\t\t/*\n+\t\t * We store a pointer to update in item->util, but at\n+\t\t * the moment we never use the value of this field\n+\t\t * except to check whether it is non-NULL.\n+\t\t */\n+\t\titem->util = update;\n+\t}\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n@@ -3162,6 +3549,32 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \t/*\n+\t * Special hack: If a branch is updated directly and HEAD\n+\t * points to it (may happen on the remote side of a push\n+\t * for example) then logically the HEAD reflog should be\n+\t * updated too.\n+\t *\n+\t * A generic solution would require reverse symref lookups,\n+\t * but finding all symrefs pointing to a given branch would be\n+\t * rather costly for this rare event (the direct update of a\n+\t * branch) to be worth it. So let's cheat and check with HEAD\n+\t * only, which should cover 99% of all usage scenarios (even\n+\t * 100% of the default ones).\n+\t *\n+\t * So if HEAD is a symbolic reference, then record the name of\n+\t * the reference that it points to. If we see an update of\n+\t * head_ref within the transaction, then split_head_update()\n+\t * arranges for the reflog of HEAD to be updated, too.\n+\t */\n+\thead_ref = resolve_refdup(\"HEAD\", RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t head_oid.hash, &head_type);\n+\n+\tif (head_ref && !(head_type & REF_ISSYMREF)) {\n+\t\tfree(head_ref);\n+\t\thead_ref = NULL;\n+\t}\n+\n+\t/*\n \t * Acquire all locks, verify old values if provided, check\n \t * that new values are valid, and write new values to the\n \t * lockfiles, ready to be activated. Only keep one lockfile\n@@ -3170,7 +3583,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tret = lock_ref_for_update(update, transaction,\n+\t\tret = lock_ref_for_update(update, transaction, head_ref,\n \t\t\t\t\t &affected_refnames, err);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n@@ -3179,23 +3592,35 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_lock *lock = update->lock;\n \n \t\tif (update->flags & REF_NEEDS_COMMIT ||\n \t\t update->flags & REF_LOG_ONLY) {\n-\t\t\tif (commit_ref_update(update->lock,\n-\t\t\t\t\t update->new_sha1, update->msg,\n-\t\t\t\t\t update->flags, err)) {\n-\t\t\t\t/* freed by commit_ref_update(): */\n-\t\t\t\tupdate->lock = NULL;\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n-\t\t\t} else {\n-\t\t\t\t/* freed by commit_ref_update(): */\n-\t\t\t\tupdate->lock = NULL;\n-\t\t\t}\n-\t\t}\n-\t}\n+\t\t\tif (log_ref_write(lock->ref_name, lock->old_oid.hash,\n+\t\t\t\t\t update->new_sha1,\n+\t\t\t\t\t update->msg, update->flags, err)) {\n+\t\t\t\tchar *old_msg = strbuf_detach(err, NULL);\n \n+\t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n+\t\t\t\t\t lock->ref_name, old_msg);\n+\t\t\t\tfree(old_msg);\n+\t\t\t\tunlock_ref(lock);\n+\t\t\t\tupdate->lock = NULL;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t\tif (update->flags & REF_NEEDS_COMMIT) {\n+\t\t\tclear_loose_ref_cache(&ref_cache);\n+\t\t\tif (commit_ref(lock)) {\n+\t\t\t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n+\t\t\t\tunlock_ref(lock);\n+\t\t\t\tupdate->lock = NULL;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t}\n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n@@ -3228,7 +3653,9 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tstring_list_clear(&refs_to_delete, 0);\n+\tfree(head_ref);\n \tstring_list_clear(&affected_refnames, 0);\n+\n \treturn ret;\n }\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex babdf27..cccd76b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -50,6 +50,12 @@\n #define REF_LOG_ONLY 0x80\n \n /*\n+ * Internal flag, meaning that the containing ref_update was via an\n+ * update to HEAD.\n+ */\n+#define REF_UPDATE_VIA_HEAD 0x100\n+\n+/*\n * Return true iff refname is minimally safe. \"Safe\" here means that\n * deleting a loose reference by this name will not do any damage, for\n * example by causing a file that is not a reference to be deleted.\n@@ -148,11 +154,12 @@ struct ref_update {\n \tunsigned char old_sha1[20];\n \t/*\n \t * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,\n-\t * REF_DELETING, and REF_ISPRUNING:\n+\t * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and\n+\t * REF_UPDATE_VIA_HEAD:\n \t */\n \tunsigned int flags;\n \tstruct ref_lock *lock;\n-\tint type;\n+\tunsigned int type;\n \tchar *msg;\n \tconst char refname[FLEX_ARRAY];\n };\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 08bd8fd..d226930 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1102,6 +1102,41 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n+test_expect_success 'fails with duplicate HEAD update' '\n+\tgit branch target1 $A &&\n+\tgit checkout target1 &&\n+\tcat >stdin <<-EOF &&\n+\tupdate refs/heads/target1 $C\n+\toption no-deref\n+\tupdate HEAD $B\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err &&\n+\tgrep \"fatal: multiple updates for '\\''HEAD'\\'' (including one via its referent .refs/heads/target1.) are not allowed\" err &&\n+\techo \"refs/heads/target1\" >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"$A\" >expect &&\n+\tgit rev-parse refs/heads/target1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fails with duplicate ref update via symref' '\n+\tgit branch target2 $A &&\n+\tgit symbolic-ref refs/heads/symref2 refs/heads/target2 &&\n+\tcat >stdin <<-EOF &&\n+\tupdate refs/heads/target2 $C\n+\tupdate refs/heads/symref2 $B\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err &&\n+\tgrep \"fatal: multiple updates for '\\''refs/heads/target2'\\'' (including one via symref .refs/heads/symref2.) are not allowed\" err &&\n+\techo \"refs/heads/target2\" >expect &&\n+\tgit symbolic-ref refs/heads/symref2 >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"$A\" >expect &&\n+\tgit rev-parse refs/heads/target2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n run_with_limited_open_files () {\n \t(ulimit -n 32 && \"$@\")\n }\n-- \n2.8.1\n"},{"id":"285736","messageId":"bb0e9b5dd81738f584d3e0b11907345721b0ed2e.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 30/33] lock_ref_for_update(): don't re-read non-symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:11Z","receivedAt":"2016-05-06T16:14:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Before the previous patch, our first read of the reference happened\nbefore the reference was locked, so we couldn't trust its value and had\nto read it again. But now that our first read of the reference happens\nafter acquiring the lock, there is no need to read it a second time. So\nmove the read_ref_full() call into the (update->type & REF_ISSYMREF)\nblock.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 52 ++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 32 insertions(+), 20 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 799866f..a9066ba 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3430,33 +3430,45 @@ static int lock_ref_for_update(struct ref_update *update,\n \n \tlock = update->lock;\n \n-\tif (read_ref_full(update->refname,\n-\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n-\t\t\t lock->old_oid.hash, NULL)) {\n-\t\tif (update->flags & REF_HAVE_OLD) {\n-\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': can't resolve old value\",\n-\t\t\t\t update->refname);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t\t} else {\n-\t\t\thashclr(lock->old_oid.hash);\n-\t\t}\n-\t}\n-\tif ((update->flags & REF_HAVE_OLD) &&\n-\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n-\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n-\t\t\t update->refname,\n-\t\t\t sha1_to_hex(lock->old_oid.hash),\n-\t\t\t sha1_to_hex(update->old_sha1));\n-\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t}\n-\n \tif (update->type & REF_ISSYMREF) {\n+\t\tif (read_ref_full(update->refname,\n+\t\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n+\t\t\t\t lock->old_oid.hash, NULL)) {\n+\t\t\tif (update->flags & REF_HAVE_OLD) {\n+\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': can't resolve old value\",\n+\t\t\t\t\t update->refname);\n+\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t} else {\n+\t\t\t\thashclr(lock->old_oid.hash);\n+\t\t\t}\n+\t\t}\n+\t\tif ((update->flags & REF_HAVE_OLD) &&\n+\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t\t\t update->refname,\n+\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t\t sha1_to_hex(update->old_sha1));\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t}\n+\n \t\tif (!(update->flags & REF_NODEREF)) {\n \t\t\tret = split_symref_update(update, referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n+\t} else if ((update->flags & REF_HAVE_OLD) &&\n+\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\tif (is_null_sha1(update->old_sha1))\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': reference already exists\",\n+\t\t\t\t update->refname);\n+\t\telse\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t\t\t update->refname,\n+\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t\t sha1_to_hex(update->old_sha1));\n+\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \t}\n \n \tif ((update->flags & REF_HAVE_NEW) &&\n-- \n2.8.1\n"},{"id":"285737","messageId":"6097b21b368baaacdd887deb086219cb919e00d9.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 31/33] lock_ref_for_update(): don't resolve symrefs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:12Z","receivedAt":"2016-05-06T16:14:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If a transaction includes a non-NODEREF update to a symbolic reference,\nwe don't have to look it up in lock_ref_for_update(). The reference will\nbe dereferenced anyway when the split-off update is processed.\n\nThis change requires that we store a backpointer from the split-off\nupdate to its parent update, for two reasons:\n\n* We still want to report the original reference name in error messages.\n So if an error occurs when checking the split-off update's old_sha1,\n walk the parent_update pointers back to find the original reference\n name, and report that one.\n\n* We still need to write the old_sha1 of the symref to its reflog. So\n after we read the split-off update's reference value, walk the\n parent_update pointers back and fill in their old_sha1 fields.\n\nAside from eliminating unnecessary reads, this change fixes a\nsubtle (though not very serious) race condition: in the old code, the\nold_sha1 of the symref was resolved before the reference that it pointed\nat was locked. So it was possible that the old_sha1 value logged to the\nsymref's reflog could be wrong if another process changed the downstream\nreference before it was locked.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 108 +++++++++++++++++++++++++++++++++++++--------------\n refs/refs-internal.h | 17 ++++++++\n 2 files changed, 95 insertions(+), 30 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a9066ba..08ec293 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3371,8 +3371,15 @@ static int split_symref_update(struct ref_update *update,\n \t\t\tupdate->new_sha1, update->old_sha1,\n \t\t\tupdate->msg);\n \n-\t/* Change the symbolic ref update to log only: */\n+\tnew_update->parent_update = update;\n+\n+\t/*\n+\t * Change the symbolic ref update to log only. Also, it\n+\t * doesn't need to check its old SHA-1 value, as that will be\n+\t * done when new_update is processed.\n+\t */\n \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n+\tupdate->flags &= ~REF_HAVE_OLD;\n \n \titem->util = new_update;\n \n@@ -3380,6 +3387,17 @@ static int split_symref_update(struct ref_update *update,\n }\n \n /*\n+ * Return the refname under which update was originally requested.\n+ */\n+static const char *original_update_refname(struct ref_update *update)\n+{\n+\twhile (update->parent_update)\n+\t\tupdate = update->parent_update;\n+\n+\treturn update->refname;\n+}\n+\n+/*\n * Prepare for carrying out update:\n * - Lock the reference referred to by update.\n * - Read the reference under lock.\n@@ -3431,44 +3449,74 @@ static int lock_ref_for_update(struct ref_update *update,\n \tlock = update->lock;\n \n \tif (update->type & REF_ISSYMREF) {\n-\t\tif (read_ref_full(update->refname,\n-\t\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n-\t\t\t\t lock->old_oid.hash, NULL)) {\n-\t\t\tif (update->flags & REF_HAVE_OLD) {\n-\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': can't resolve old value\",\n-\t\t\t\t\t update->refname);\n+\t\tif (update->flags & REF_NODEREF) {\n+\t\t\t/*\n+\t\t\t * We won't be reading the referent as part of\n+\t\t\t * the transaction, so we have to read it here\n+\t\t\t * to record and possibly check old_sha1:\n+\t\t\t */\n+\t\t\tif (read_ref_full(update->refname,\n+\t\t\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n+\t\t\t\t\t lock->old_oid.hash, NULL)) {\n+\t\t\t\tif (update->flags & REF_HAVE_OLD) {\n+\t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t\t\t\t \"can't resolve old value\",\n+\t\t\t\t\t\t update->refname);\n+\t\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t\t} else {\n+\t\t\t\t\thashclr(lock->old_oid.hash);\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif ((update->flags & REF_HAVE_OLD) &&\n+\t\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n+\t\t\t\t\t \"is at %s but expected %s\",\n+\t\t\t\t\t update->refname,\n+\t\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t\t\t sha1_to_hex(update->old_sha1));\n \t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t\t\t} else {\n-\t\t\t\thashclr(lock->old_oid.hash);\n \t\t\t}\n-\t\t}\n-\t\tif ((update->flags & REF_HAVE_OLD) &&\n-\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n-\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n-\t\t\t\t update->refname,\n-\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n-\t\t\t\t sha1_to_hex(update->old_sha1));\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n-\t\t}\n \n-\t\tif (!(update->flags & REF_NODEREF)) {\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Create a new update for the reference this\n+\t\t\t * symref is pointing at. Also, we will record\n+\t\t\t * and verify old_sha1 for this update as part\n+\t\t\t * of processing the split-off update, so we\n+\t\t\t * don't have to do it here.\n+\t\t\t */\n \t\t\tret = split_symref_update(update, referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n-\t} else if ((update->flags & REF_HAVE_OLD) &&\n-\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n-\t\tif (is_null_sha1(update->old_sha1))\n-\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': reference already exists\",\n-\t\t\t\t update->refname);\n-\t\telse\n-\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n-\t\t\t\t update->refname,\n-\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n-\t\t\t\t sha1_to_hex(update->old_sha1));\n+\t} else {\n+\t\tstruct ref_update *parent_update;\n \n-\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t/*\n+\t\t * If this update is happening indirectly because of a\n+\t\t * symref update, record the old SHA-1 in the parent\n+\t\t * update:\n+\t\t */\n+\t\tfor (parent_update = update->parent_update;\n+\t\t parent_update;\n+\t\t parent_update = parent_update->parent_update) {\n+\t\t\toidcpy(&parent_update->lock->old_oid, &lock->old_oid);\n+\t\t}\n+\n+\t\tif ((update->flags & REF_HAVE_OLD) &&\n+\t\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\t\tif (is_null_sha1(update->old_sha1))\n+\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': reference already exists\",\n+\t\t\t\t\t original_update_refname(update));\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t\t\t\t original_update_refname(update),\n+\t\t\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t\t\t sha1_to_hex(update->old_sha1));\n+\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t}\n \t}\n \n \tif ((update->flags & REF_HAVE_NEW) &&\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex cccd76b..1bb3d87 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -143,24 +143,41 @@ int should_autocreate_reflog(const char *refname);\n * not exist before update.\n */\n struct ref_update {\n+\n \t/*\n \t * If (flags & REF_HAVE_NEW), set the reference to this value:\n \t */\n \tunsigned char new_sha1[20];\n+\n \t/*\n \t * If (flags & REF_HAVE_OLD), check that the reference\n \t * previously had this value:\n \t */\n \tunsigned char old_sha1[20];\n+\n \t/*\n \t * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,\n \t * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and\n \t * REF_UPDATE_VIA_HEAD:\n \t */\n \tunsigned int flags;\n+\n \tstruct ref_lock *lock;\n \tunsigned int type;\n \tchar *msg;\n+\n+\t/*\n+\t * If this ref_update was split off of a symref update via\n+\t * split_symref_update(), then this member points at that\n+\t * update. This is used for two purposes:\n+\t * 1. When reporting errors, we report the refname under which\n+\t * the update was originally requested.\n+\t * 2. When we read the old value of this reference, we\n+\t * propagate it back to its parent update for recording in\n+\t * the latter's reflog.\n+\t */\n+\tstruct ref_update *parent_update;\n+\n \tconst char refname[FLEX_ARRAY];\n };\n \n-- \n2.8.1\n"},{"id":"285735","messageId":"678e70dd4fa33223bb5a870d7196413dbfad2c3e.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 32/33] commit_ref_update(): remove the flags parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:13Z","receivedAt":"2016-05-06T16:14:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"commit_ref_update() is now only called with flags=0. So remove the flags\nparameter entirely.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 08ec293..a180b9e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2541,7 +2541,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n \t\t\t\t const unsigned char *sha1, struct strbuf *err);\n static int commit_ref_update(struct ref_lock *lock,\n \t\t\t const unsigned char *sha1, const char *logmsg,\n-\t\t\t int flags, struct strbuf *err);\n+\t\t\t struct strbuf *err);\n \n int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)\n {\n@@ -2617,7 +2617,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \thashcpy(lock->old_oid.hash, orig_sha1);\n \n \tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n-\t commit_ref_update(lock, orig_sha1, logmsg, 0, &err)) {\n+\t commit_ref_update(lock, orig_sha1, logmsg, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", newrefname, err.buf);\n \t\tstrbuf_release(&err);\n \t\tgoto rollback;\n@@ -2637,7 +2637,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tflag = log_all_ref_updates;\n \tlog_all_ref_updates = 0;\n \tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n-\t commit_ref_update(lock, orig_sha1, NULL, 0, &err)) {\n+\t commit_ref_update(lock, orig_sha1, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\n \t}\n@@ -2875,12 +2875,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,\n */\n static int commit_ref_update(struct ref_lock *lock,\n \t\t\t const unsigned char *sha1, const char *logmsg,\n-\t\t\t int flags, struct strbuf *err)\n+\t\t\t struct strbuf *err)\n {\n \tclear_loose_ref_cache(&ref_cache);\n-\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0 ||\n+\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n-\t log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, flags, err) < 0)) {\n+\t log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n \t\t\t lock->ref_name, old_msg);\n@@ -2916,7 +2916,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t\t}\n \t\t}\n \t}\n-\tif (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {\n+\tif (commit_ref(lock)) {\n \t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n-- \n2.8.1\n"},{"id":"285751","messageId":"1b71052522155c73624dde29ddf1d613b56ff5fa.1462550456.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"[PATCH v2 33/33] lock_ref_sha1_basic(): only handle REF_NODEREF mode","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-06T16:14:14Z","receivedAt":"2016-05-06T16:14:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we\ndon't have to handle other cases anymore.\n\nThis enables several simplifications, the most interesting of which come\nfrom the fact that ref_lock::orig_ref_name is now always the same as\nref_lock::ref_name:\n\n* Remove ref_lock::orig_ref_name\n* Remove local variable orig_refname from lock_ref_sha1_basic()\n* ref_name can be initialize once and its value reused\n* commit_ref_update() never has to write to the reflog for\n lock->orig_ref_name\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 54 +++++++++++++++++++---------------------------------\n 1 file changed, 20 insertions(+), 34 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a180b9e..71e068b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -7,7 +7,6 @@\n \n struct ref_lock {\n \tchar *ref_name;\n-\tchar *orig_ref_name;\n \tstruct lock_file *lk;\n \tstruct object_id old_oid;\n };\n@@ -1522,7 +1521,6 @@ static void unlock_ref(struct ref_lock *lock)\n \tif (lock->lk)\n \t\trollback_lock_file(lock->lk);\n \tfree(lock->ref_name);\n-\tfree(lock->orig_ref_name);\n \tfree(lock);\n }\n \n@@ -1576,7 +1574,6 @@ static int lock_raw_ref(const char *refname, int mustexist,\n \t*lock_p = lock = xcalloc(1, sizeof(*lock));\n \n \tlock->ref_name = xstrdup(refname);\n-\tlock->orig_ref_name = xstrdup(refname);\n \tstrbuf_git_path(&ref_file, \"%s\", refname);\n \n retry:\n@@ -1964,14 +1961,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct strbuf ref_file = STRBUF_INIT;\n-\tstruct strbuf orig_ref_file = STRBUF_INIT;\n-\tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n-\tint lflags = 0;\n+\tint lflags = LOCK_NO_DEREF;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n-\tint resolve_flags = 0;\n+\tint resolve_flags = RESOLVE_REF_NO_RECURSE;\n \tint attempts_remaining = 3;\n+\tint resolved;\n \n \tassert(err);\n \n@@ -1981,46 +1977,39 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tresolve_flags |= RESOLVE_REF_READING;\n \tif (flags & REF_DELETING)\n \t\tresolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;\n-\tif (flags & REF_NODEREF) {\n-\t\tresolve_flags |= RESOLVE_REF_NO_RECURSE;\n-\t\tlflags |= LOCK_NO_DEREF;\n-\t}\n \n-\trefname = resolve_ref_unsafe(refname, resolve_flags,\n-\t\t\t\t lock->old_oid.hash, type);\n-\tif (!refname && errno == EISDIR) {\n+\tstrbuf_git_path(&ref_file, \"%s\", refname);\n+\tresolved = !!resolve_ref_unsafe(refname, resolve_flags,\n+\t\t\t\t\tlock->old_oid.hash, type);\n+\tif (!resolved && errno == EISDIR) {\n \t\t/*\n \t\t * we are trying to lock foo but we used to\n \t\t * have foo/bar which now does not exist;\n \t\t * it is normal for the empty directory 'foo'\n \t\t * to remain.\n \t\t */\n-\t\tstrbuf_git_path(&orig_ref_file, \"%s\", orig_refname);\n-\t\tif (remove_empty_directories(&orig_ref_file)) {\n+\t\tif (remove_empty_directories(&ref_file)) {\n \t\t\tlast_errno = errno;\n-\t\t\tif (!verify_refname_available_dir(orig_refname, extras, skip,\n+\t\t\tif (!verify_refname_available_dir(refname, extras, skip,\n \t\t\t\t\t\t\t get_loose_refs(&ref_cache), err))\n \t\t\t\tstrbuf_addf(err, \"there are still refs under '%s'\",\n-\t\t\t\t\t orig_refname);\n+\t\t\t\t\t refname);\n \t\t\tgoto error_return;\n \t\t}\n-\t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n-\t\t\t\t\t lock->old_oid.hash, type);\n+\t\tresolved = !!resolve_ref_unsafe(refname, resolve_flags,\n+\t\t\t\t\t\tlock->old_oid.hash, type);\n \t}\n-\tif (!refname) {\n+\tif (!resolved) {\n \t\tlast_errno = errno;\n \t\tif (last_errno != ENOTDIR ||\n-\t\t !verify_refname_available_dir(orig_refname, extras, skip,\n+\t\t !verify_refname_available_dir(refname, extras, skip,\n \t\t\t\t\t\t get_loose_refs(&ref_cache), err))\n \t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n-\t\t\t\t orig_refname, strerror(last_errno));\n+\t\t\t\t refname, strerror(last_errno));\n \n \t\tgoto error_return;\n \t}\n \n-\tif (flags & REF_NODEREF)\n-\t\trefname = orig_refname;\n-\n \t/*\n \t * If the ref did not exist and we are creating it, make sure\n \t * there is no existing packed ref whose name begins with our\n@@ -2037,8 +2026,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\n \n \tlock->ref_name = xstrdup(refname);\n-\tlock->orig_ref_name = xstrdup(orig_refname);\n-\tstrbuf_git_path(&ref_file, \"%s\", refname);\n \n retry:\n \tswitch (safe_create_leading_directories_const(ref_file.buf)) {\n@@ -2081,7 +2068,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n out:\n \tstrbuf_release(&ref_file);\n-\tstrbuf_release(&orig_ref_file);\n \terrno = last_errno;\n \treturn lock;\n }\n@@ -2878,9 +2864,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t\t struct strbuf *err)\n {\n \tclear_loose_ref_cache(&ref_cache);\n-\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0 ||\n-\t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n-\t log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, 0, err) < 0)) {\n+\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n \t\t\t lock->ref_name, old_msg);\n@@ -2888,7 +2872,8 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tif (strcmp(lock->orig_ref_name, \"HEAD\") != 0) {\n+\n+\tif (strcmp(lock->ref_name, \"HEAD\") != 0) {\n \t\t/*\n \t\t * Special hack: If a branch is updated directly and HEAD\n \t\t * points to it (may happen on the remote side of a push\n@@ -2904,6 +2889,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\tunsigned char head_sha1[20];\n \t\tint head_flag;\n \t\tconst char *head_ref;\n+\n \t\thead_ref = resolve_ref_unsafe(\"HEAD\", RESOLVE_REF_READING,\n \t\t\t\t\t head_sha1, &head_flag);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n@@ -2916,6 +2902,7 @@ static int commit_ref_update(struct ref_lock *lock,\n \t\t\t}\n \t\t}\n \t}\n+\n \tif (commit_ref(lock)) {\n \t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n \t\tunlock_ref(lock);\n@@ -3021,7 +3008,6 @@ int set_worktree_head_symref(const char *gitdir, const char *target)\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lk = &head_lock;\n \tlock->ref_name = xstrdup(head_rel);\n-\tlock->orig_ref_name = xstrdup(head_rel);\n \n \tret = create_symref_locked(lock, head_rel, target, NULL);\n \n-- \n2.8.1\n"},{"id":"285968","messageId":"1462824758.24478.37.camel@twopensource.com","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-05-09T20:12:38Z","receivedAt":"2016-05-09T20:12:38Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:\n> Thanks to David, Junio, and Peff for their comments on v1 of this\n> patch series [1]. I think I have addressed all of the points that\n> were\n> brought up. Plus I fixed a pre-existing bug that I noticed myself\n> while adding some more tests; see the first bullet point below for\n> more information.\n> \n> Changes between v1 and v2:\n> \n> * Prefixed the patch series with three new patches that demonstrate\n> and fix some bugs in reference resolution that I noticed while\n> inspecting this series:\n> \n> * \"t1404: demonstrate a bug resolving references\" -- this\n> demonstrates a bug that has existed since at least 2.5.0.\n> * \"read_raw_ref(): don't get confused by an empty directory\"\n> * \"commit_ref(): if there is an empty dir in the way, delete it\"\n\nI generally like to put the bug fixes before the tests for those fixes\n(so that bisect on the complete suite works). But maybe the git policy\nis different.\n\nOther than that, these changes look good to me.\n"},{"id":"285992","messageId":"xmqqinynt01r.fsf@gitster.mtv.corp.google.com","threadId":"42250","inReplyTo":"1462824758.24478.37.camel@twopensource.com","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T21:05:04Z","receivedAt":"2016-05-09T21:05:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> On Fri, 2016-05-06 at 18:13 +0200, Michael Haggerty wrote:\n>> Thanks to David, Junio, and Peff for their comments on v1 of this\n>> patch series [1]. I think I have addressed all of the points that\n>> were\n>> brought up. Plus I fixed a pre-existing bug that I noticed myself\n>> while adding some more tests; see the first bullet point below for\n>> more information.\n>> \n>> Changes between v1 and v2:\n>> \n>> * Prefixed the patch series with three new patches that demonstrate\n>> and fix some bugs in reference resolution that I noticed while\n>> inspecting this series:\n>> \n>> * \"t1404: demonstrate a bug resolving references\" -- this\n>> demonstrates a bug that has existed since at least 2.5.0.\n>> * \"read_raw_ref(): don't get confused by an empty directory\"\n>> * \"commit_ref(): if there is an empty dir in the way, delete it\"\n>\n> I generally like to put the bug fixes before the tests for those fixes\n> (so that bisect on the complete suite works). But maybe the git policy\n> is different.\n\nThe Git policy only asks not to break bisection.\n\nAs long as patch that adds a new test that comes before a patch that\nfixes the issue marks the new test with test_expect_failure, and a\nlater patch that fixes the issue turns it into test_expect_success,\nbisection would not break.\n\nThe \"demonstrate an existing breakage first\" order makes it slightly\neasier to review and follow a long series, as it forces the reviewer\nto see the issue first and think about possible avenues to solve it\nfor themselves, before seeing a paticular solution. For a trivial\nsingle-issue fix, it is not necessary (including a fix and a test to\nprotect the fix from future breakage in the same patch is a norm).\n"},{"id":"285998","messageId":"5731062B.6080909@alum.mit.edu","threadId":"42250","inReplyTo":"xmqqinynt01r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-09T21:50:35Z","receivedAt":"2016-05-09T21:50:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/09/2016 11:05 PM, Junio C Hamano wrote:\n> David Turner writes:\n>> [...]\n>> I generally like to put the bug fixes before the tests for those fixes\n>> (so that bisect on the complete suite works). But maybe the git policy\n>> is different.\n> \n> The Git policy only asks not to break bisection.\n> \n> As long as patch that adds a new test that comes before a patch that\n> fixes the issue marks the new test with test_expect_failure, and a\n> later patch that fixes the issue turns it into test_expect_success,\n> bisection would not break.\n> \n> The \"demonstrate an existing breakage first\" order makes it slightly\n> easier to review and follow a long series, as it forces the reviewer\n> to see the issue first and think about possible avenues to solve it\n> for themselves, before seeing a paticular solution. For a trivial\n> single-issue fix, it is not necessary (including a fix and a test to\n> protect the fix from future breakage in the same patch is a norm).\n\nI find it useful to add the broken test in a separate patch, because it\nis then easy to cherry pick that patch to other versions of Git to\ndiscover which ones are also affected by the problem. If the addition of\nthe test is combined with the fix, then the patch would more often fail\nto apply to other versions due to conflicts at the locations of the fix,\nand even if it applied, you wouldn't learn whether that version of git\nwas broken but the breakage was fixed by the same fix, or whether it\nwasn't broken in the first place and the fix was unnecessary.\n\nMichael\n"},{"id":"285999","messageId":"xmqq37pqubup.fsf@gitster.mtv.corp.google.com","threadId":"42250","inReplyTo":"5731062B.6080909@alum.mit.edu","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T22:04:46Z","receivedAt":"2016-05-09T22:04:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> The \"demonstrate an existing breakage first\" order makes it slightly\n>> easier to review and follow a long series, as it forces the reviewer\n>> to see the issue first and think about possible avenues to solve it\n>> for themselves, before seeing a paticular solution. For a trivial\n>> single-issue fix, it is not necessary (including a fix and a test to\n>> protect the fix from future breakage in the same patch is a norm).\n>\n> I find it useful to add the broken test in a separate patch, because it\n> is then easy to cherry pick that patch to other versions of Git to\n> discover which ones are also affected by the problem. If the addition of\n> the test is combined with the fix, then the patch would more often fail\n> to apply to other versions due to conflicts at the locations of the fix,\n> and even if it applied, you wouldn't learn whether that version of git\n> was broken but the breakage was fixed by the same fix, or whether it\n> wasn't broken in the first place and the fix was unnecessary.\n\nNote that I only said \"it is not necessary\", and did not say \"it is\nnot desirable\". I wouldn't automatically reject a two patch series\nwith demonstration followed by fix, only because they are not in a\nsingle patch.\n\nEven when I know the maintenance track a particular fix and test\ntargets at, I'd do the \"only try to test to see how it is broken\ncurrently\" step manually anyway as part of the initial \"acceptance\"\ncheck when applying [*1*], so trying the same procedure for older\nmaintenance tracks is no big burden for me. Having these as two\nseparate patches is easier to split them apart, which unfortunately\nmakes it easier to lose one of them while cherry-picking.\n\nThis is of course subjective.\n\n\n[Footnote]\n\n*1* There is a bit of white-lie here. Instead of applying only t/\n part, I tend to just do \"git am\" the whole thing, and then pipe\n \"git show\" to \"git apply -R\" to in-work-tree revert only the\n code that fixes it. But the result I get is the same.\n\n And the \"cherry-picking\" would also involve \"show only the t/\n part and pipe that to \"git apply\", which is even simpler than\n actually cherry-picking and creating a commit (I do not have to\n be very careful not to leave such a \"test cherry-pick\" commit in\n the real history).\n"},{"id":"286120","messageId":"xmqqlh3hmwes.fsf@gitster.mtv.corp.google.com","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T21:32:27Z","receivedAt":"2016-05-10T21:32:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> ... I think I have addressed all of the points that were\n> brought up. Plus I fixed a pre-existing bug that I noticed myself\n> while adding some more tests; see the first bullet point below for\n> more information.\n>\n> Changes between v1 and v2:\n>\n> * Prefixed the patch series with three new patches that demonstrate\n> and fix some bugs in reference resolution that I noticed while\n> inspecting this series:\n\nI'd propose to wait for further comments a few more days and merge\nthis to 'next' by the end of the week.\n"},{"id":"286319","messageId":"20160512074528.GB10922@sigill.intra.peff.net","threadId":"42250","inReplyTo":"66d33af21bd1e398973414435af43d06f2e2099c.1462550456.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T07:45:28Z","receivedAt":"2016-05-12T07:45:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2016 at 06:14:10PM +0200, Michael Haggerty wrote:\n\n> This makes use of a new function, lock_ref_raw(), which is analogous to\n> read_ref_raw(), but acquires a lock on the reference before reading it.\n\nMinor nit: the new function is actually called lock_raw_ref(). I don't\ncare which is used, just an inconsistency.\n\nBut my much bigger (non-)nit is that this seems to make large ref\nupdates much slower. You can see this by running t5551 with \"--long\".\nIn t5551.26, we fetch 48000 new tags into a repository that already has\n2000 tags. Before this patch, it takes about 2 seconds. After, it chews\nCPU for several minutes (I never actually let it finish).\n\nThe perf output isn't all that instructive. We seem to spend a lot of\ntime reading directory entries. Attaching with gdb shows:\n\n#0 0x00007f35e00c2670 in __open_nocancel () at ../sysdeps/unix/syscall-template.S:84\n#1 0x0000000000533982 in read_raw_ref (\n refname=0x4e899f0 \"refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520\", sha1=0x7fff7c5aff30 \"\\002\\357\\373\\070\\332{\\341\\005\\366츖\\265G\\276\\332\\f\\025\\271\\276\\377\\177\", \n referent=0x836300 , type=0x7fff7c5afe34) at refs/files-backend.c:1468\n#2 0x0000000000530bf3 in resolve_ref_unsafe (\n refname=0x4e899f0 \"refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520\", resolve_flags=1, \n sha1=0x7fff7c5aff30 \"\\002\\357\\373\\070\\332{\\341\\005\\366츖\\265G\\276\\332\\f\\025\\271\\276\\377\\177\", \n flags=0x7fff7c5aff2c) at refs.c:1209\n#3 0x000000000052e56f in read_ref_full (\n refname=0x4e899f0 \"refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-11520\", resolve_flags=1, \n sha1=0x7fff7c5aff30 \"\\002\\357\\373\\070\\332{\\341\\005\\366츖\\265G\\276\\332\\f\\025\\271\\276\\377\\177\", \n flags=0x7fff7c5aff2c) at refs.c:169\n#4 0x000000000053316e in read_loose_refs (dirname=0x4e30f80 \"refs/tags/\", dir=0x4e30f58) at refs/files-backend.c:1216\n#5 0x0000000000531435 in get_ref_dir (entry=0x4e30f50) at refs/files-backend.c:174\n#6 0x000000000053265c in verify_refname_available_dir (\n refname=0x1cd9438 \"refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-12016\", extras=0x7fff7c5b01d0, skip=0x0, dir=0x4dd6e98, err=0x7fff7c5b02a0) at refs/files-backend.c:789\n#7 0x0000000000533e44 in lock_raw_ref (\n refname=0x1cd9438 \"refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-12016\", mustexist=0, extras=0x7fff7c5b01d0, skip=0x0, lock_p=0x1cd9420, referent=0x7fff7c5b0140, type=0x1cd9428, \n err=0x7fff7c5b02a0) at refs/files-backend.c:1663\n#8 0x00000000005379d7 in lock_ref_for_update (update=0x1cd93f0, transaction=0x4db0150, \n head_ref=0x4db0000 \"refs/heads/master\", affected_refnames=0x7fff7c5b01d0, err=0x7fff7c5b02a0)\n at refs/files-backend.c:3416\n[...]\n\nSo I'd expect us to hit that lock_ref_for_update() for each of the new\nrefs. But then we end up in verify_refname_available_dir(), which wants\nto read all of the loose refs again. So we end up with a quadratic\nnumber of calls to read_ref_full().\n\nI haven't found the actual bug yet. It may be something as simple as not\nclearing REF_INCOMPLETE from the loose-ref cache when we ought to. But\nthat's a wild (optimistic) guess.\n\n-Peff\n"},{"id":"286320","messageId":"20160512075555.GC10922@sigill.intra.peff.net","threadId":"42250","inReplyTo":"xmqq37pqubup.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T07:55:55Z","receivedAt":"2016-05-12T07:55:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 09, 2016 at 03:04:46PM -0700, Junio C Hamano wrote:\n\n> Note that I only said \"it is not necessary\", and did not say \"it is\n> not desirable\". I wouldn't automatically reject a two patch series\n> with demonstration followed by fix, only because they are not in a\n> single patch.\n> \n> Even when I know the maintenance track a particular fix and test\n> targets at, I'd do the \"only try to test to see how it is broken\n> currently\" step manually anyway as part of the initial \"acceptance\"\n> check when applying [*1*], so trying the same procedure for older\n> maintenance tracks is no big burden for me. Having these as two\n> separate patches is easier to split them apart, which unfortunately\n> makes it easier to lose one of them while cherry-picking.\n> \n> This is of course subjective.\n\nAs a patch-writer, I often find that splitting the tests from the fix\nmeans that you end up having to explain it twice, and often missing some\nof the context. IOW, it's often hard to explain why a test is checking\nthe right thing without basically explaining the fix. And explaining the\nfix when it's not part of that patch gets awkward.\n\nSo I don't think split tests/fixes are wrong, but I'd urge people to\nlook at how their commit messages turn out. Sometimes the split makes\nthings _easier_ to explain, too.\n\n> *1* There is a bit of white-lie here. Instead of applying only t/\n> part, I tend to just do \"git am\" the whole thing, and then pipe\n> \"git show\" to \"git apply -R\" to in-work-tree revert only the\n> code that fixes it. But the result I get is the same.\n\nMy trick for checking the before/after of a patch is:\n\n 1. Compile git without the patch.\n\n 2. Apply the patch, then run the test (via ./t1234-*, which does not\n want to re-build git), confirm that it fails.\n\n 3. Re-compile and re-run the test, confirming that it passes.\n\nThat also works well with \"rebase -i\" where you stop at the patch before\nto compile.\n\nI like it because it's simple and doesn't affect git's view (so you\ncan't accidentally commit the in-work-tree revert, for example). But\nsince there's nothing telling you what state the compiled git is in, it\ncan be easy to get confused.\n\n-Peff\n"},{"id":"286325","messageId":"20160512082526.GA20817@sigill.intra.peff.net","threadId":"42250","inReplyTo":"20160512074528.GB10922@sigill.intra.peff.net","subject":"Re: [PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-12T08:25:26Z","receivedAt":"2016-05-12T08:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 12, 2016 at 03:45:28AM -0400, Jeff King wrote:\n\n> So I'd expect us to hit that lock_ref_for_update() for each of the new\n> refs. But then we end up in verify_refname_available_dir(), which wants\n> to read all of the loose refs again. So we end up with a quadratic\n> number of calls to read_ref_full().\n> \n> I haven't found the actual bug yet. It may be something as simple as not\n> clearing REF_INCOMPLETE from the loose-ref cache when we ought to. But\n> that's a wild (optimistic) guess.\n\nAh, nope, nothing so simple.\n\nIt looks like we get in a chain of:\n\n 1. we want to update a ref, so we end up in\n verify_refname_available_dir to make sure we can do so.\n\n 2. that has to load all of the loose refs in refs/tags, which is\n expensive.\n\n 3. we actually update the ref, which calls clear_loose_ref_cache().\n\nAnd repeat. Each ref update does the expensive step 2, and it gets\nlarger and larger each time.\n\nI understand why we need to verify_refname_available() on the packed\nrefs. But traditionally we would rely on EISDIR or EEXIST to detect\nconflicts with the loose refs, rather than loading using our own cache.\nSo I guess that's the new thing that is causing a problem.\n\n-Peff\n"},{"id":"286337","messageId":"xmqqfutngsud.fsf@gitster.mtv.corp.google.com","threadId":"42250","inReplyTo":"20160512075555.GC10922@sigill.intra.peff.net","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-12T16:10:34Z","receivedAt":"2016-05-12T16:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My trick for checking the before/after of a patch is:\n>\n> 1. Compile git without the patch.\n>\n> 2. Apply the patch, then run the test (via ./t1234-*, which does not\n> want to re-build git), confirm that it fails.\n>\n> 3. Re-compile and re-run the test, confirming that it passes.\n>\n> That also works well with \"rebase -i\" where you stop at the patch before\n> to compile.\n>\n> I like it because it's simple and doesn't affect git's view (so you\n> can't accidentally commit the in-work-tree revert, for example). But\n> since there's nothing telling you what state the compiled git is in, it\n> can be easy to get confused.\n\nTrue. It also would not work well with debuggers, but it is usually\nrare to need a debugger to verify the claim of a \"fix\" patch, so I\nthink the above is easier to use in practice.\n\nThanks.\n"},{"id":"286449","messageId":"5735C990.8080502@alum.mit.edu","threadId":"42250","inReplyTo":"20160512082526.GA20817@sigill.intra.peff.net","subject":"Re: [PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-13T12:33:20Z","receivedAt":"2016-05-13T12:33:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/12/2016 10:25 AM, Jeff King wrote:\n> On Thu, May 12, 2016 at 03:45:28AM -0400, Jeff King wrote:\n> \n>> So I'd expect us to hit that lock_ref_for_update() for each of the new\n>> refs. But then we end up in verify_refname_available_dir(), which wants\n>> to read all of the loose refs again. So we end up with a quadratic\n>> number of calls to read_ref_full().\n>>\n>> I haven't found the actual bug yet. It may be something as simple as not\n>> clearing REF_INCOMPLETE from the loose-ref cache when we ought to. But\n>> that's a wild (optimistic) guess.\n> \n> Ah, nope, nothing so simple.\n> \n> It looks like we get in a chain of:\n> \n> 1. we want to update a ref, so we end up in\n> verify_refname_available_dir to make sure we can do so.\n> \n> 2. that has to load all of the loose refs in refs/tags, which is\n> expensive.\n> \n> 3. we actually update the ref, which calls clear_loose_ref_cache().\n> \n> And repeat. Each ref update does the expensive step 2, and it gets\n> larger and larger each time.\n> \n> I understand why we need to verify_refname_available() on the packed\n> refs. But traditionally we would rely on EISDIR or EEXIST to detect\n> conflicts with the loose refs, rather than loading using our own cache.\n> So I guess that's the new thing that is causing a problem.\n\nTorsten, thanks for the report. Peff, thanks for the analysis.\n\nThe problem in this case is a misguided call to\nverify_refname_available_dir() in the case that read_raw_ref() fails\nwith ENOENT. In that case it is not possible for there to be a conflict\nwith another loose reference, because (1) we already hold the lock, so\nthe containing directory must exist, and (2) we got ENOENT, so there\ncan't be a loose reference in a subdirectory named after the reference\nthat we are trying to create.\n\nAs Peff explained, the call of verify_refname_available_dir() was\nforcing the loose tags to be loaded, which is expensive in this test\nbecause there are 100000 of them being created one at a time. (If they\nwere created in a single ref_transaction instead, the \"available\" tests\nwould all be done together, before any changes are committed, so the\nloose ref cache would not have to be invalidated each time.)\n\nSo instead of calling verify_refname_available_dir() here, we should\njust consider the reference to be missing but available to be written.\n\nI'll rewrite this patch and submit the new version to the mailing list\nas v3 (also with a fix in the commit message). The rest of the patch\nseries is OK as is, so I won't resend it. The entire series is also\navailable from my GitHub repo [1] as branch \"split-under-lock\".\n\nPlease note that there are still some calls of\nverify_refname_available_dir() against the loose reference cache in this\nfunction. If we wanted to give up a little bit on the quality of our\nerror messages, I think we could make those paths faster, too. But they\nare all in failure paths, so I don't think that they are performance\ncritical, so I won't make that change.\n\nMichael\n\n[1] https://github.com/mhagger/git\n"},{"id":"286450","messageId":"e87d7f0f441a9094d75cae7a39aac1e8760426e9.1463140520.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"5735C990.8080502@alum.mit.edu","subject":"[PATCH v3 29/33] refs: resolve symbolic refs first","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-05-13T12:35:40Z","receivedAt":"2016-05-13T12:35:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Before committing ref updates, split symbolic ref updates into two\nparts: an update to the underlying ref, and a log-only update to the\nsymbolic ref. This ensures that both references are locked correctly\nduring the transaction, including while their reflogs are updated.\n\nSimilarly, if the reference pointed to by HEAD is modified directly, add\na separate log-only update to HEAD, rather than leaving the job of\nupdating HEAD's reflog to commit_ref_update(). This change ensures that\nHEAD is locked correctly while its reflog is being modified, as well as\nbeing cheaper (HEAD only needs to be resolved once).\n\nThis makes use of a new function, lock_raw_ref(), which is analogous to\nread_raw_ref(), but acquires a lock on the reference before reading it.\n\nThis change still has two problems:\n\n* There are redundant read_ref_full() reference lookups.\n\n* It is still possible to get incorrect reflogs for symbolic references\n if there is a concurrent update by another process, since the old_oid\n of a symref is determined before the lock on the pointed-to ref is\n held.\n\nBoth problems will soon be fixed.\n\nSigned-off-by: David Turner \nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\nThe rest of this patch series is unchanged from v2 so I won't bother\nresending it.\n\nThanks to Torsten for reporting a problem in v2, and Peff for the\nbisection and diagnosis.\n\nThe changes to this patch since v2:\n\n* Correct s/read_ref_raw/read_raw_ref/ in the log message.\n\n* If read_raw_ref() fails with ENOENT, instead of calling\n verify_refname_available_dir(), insert a comment explaining why such\n a call is unnecessary.\n\n refs/files-backend.c | 518 +++++++++++++++++++++++++++++++++++++++++++++-----\n refs/refs-internal.h | 11 +-\n t/t1400-update-ref.sh | 35 ++++\n 3 files changed, 519 insertions(+), 45 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8ff76ed..3af0d35 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1527,6 +1527,233 @@ static void unlock_ref(struct ref_lock *lock)\n }\n \n /*\n+ * Lock refname, without following symrefs, and set *lock_p to point\n+ * at a newly-allocated lock object. Fill in lock->old_oid, referent,\n+ * and type similarly to read_raw_ref().\n+ *\n+ * The caller must verify that refname is a \"safe\" reference name (in\n+ * the sense of refname_is_safe()) before calling this function.\n+ *\n+ * If the reference doesn't already exist, verify that refname doesn't\n+ * have a D/F conflict with any existing references. extras and skip\n+ * are passed to verify_refname_available_dir() for this check.\n+ *\n+ * If mustexist is not set and the reference is not found or is\n+ * broken, lock the reference anyway but clear sha1.\n+ *\n+ * Return 0 on success. On failure, write an error message to err and\n+ * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.\n+ *\n+ * Implementation note: This function is basically\n+ *\n+ * lock reference\n+ * read_raw_ref()\n+ *\n+ * but it includes a lot more code to\n+ * - Deal with possible races with other processes\n+ * - Avoid calling verify_refname_available_dir() when it can be\n+ * avoided, namely if we were successfully able to read the ref\n+ * - Generate informative error messages in the case of failure\n+ */\n+static int lock_raw_ref(const char *refname, int mustexist,\n+\t\t\tconst struct string_list *extras,\n+\t\t\tconst struct string_list *skip,\n+\t\t\tstruct ref_lock **lock_p,\n+\t\t\tstruct strbuf *referent,\n+\t\t\tunsigned int *type,\n+\t\t\tstruct strbuf *err)\n+{\n+\tstruct ref_lock *lock;\n+\tstruct strbuf ref_file = STRBUF_INIT;\n+\tint attempts_remaining = 3;\n+\tint ret = TRANSACTION_GENERIC_ERROR;\n+\n+\tassert(err);\n+\t*type = 0;\n+\n+\t/* First lock the file so it can't change out from under us. */\n+\n+\t*lock_p = lock = xcalloc(1, sizeof(*lock));\n+\n+\tlock->ref_name = xstrdup(refname);\n+\tlock->orig_ref_name = xstrdup(refname);\n+\tstrbuf_git_path(&ref_file, \"%s\", refname);\n+\n+retry:\n+\tswitch (safe_create_leading_directories(ref_file.buf)) {\n+\tcase SCLD_OK:\n+\t\tbreak; /* success */\n+\tcase SCLD_EXISTS:\n+\t\t/*\n+\t\t * Suppose refname is \"refs/foo/bar\". We just failed\n+\t\t * to create the containing directory, \"refs/foo\",\n+\t\t * because there was a non-directory in the way. This\n+\t\t * indicates a D/F conflict, probably because of\n+\t\t * another reference such as \"refs/foo\". There is no\n+\t\t * reason to expect this error to be transitory.\n+\t\t */\n+\t\tif (verify_refname_available(refname, extras, skip, err)) {\n+\t\t\tif (mustexist) {\n+\t\t\t\t/*\n+\t\t\t\t * To the user the relevant error is\n+\t\t\t\t * that the \"mustexist\" reference is\n+\t\t\t\t * missing:\n+\t\t\t\t */\n+\t\t\t\tstrbuf_reset(err);\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * The error message set by\n+\t\t\t\t * verify_refname_available_dir() is OK.\n+\t\t\t\t */\n+\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t}\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The file that is in the way isn't a loose\n+\t\t\t * reference. Report it as a low-level\n+\t\t\t * failure.\n+\t\t\t */\n+\t\t\tstrbuf_addf(err, \"unable to create lock file %s.lock; \"\n+\t\t\t\t \"non-directory in the way\",\n+\t\t\t\t ref_file.buf);\n+\t\t}\n+\t\tgoto error_return;\n+\tcase SCLD_VANISHED:\n+\t\t/* Maybe another process was tidying up. Try again. */\n+\t\tif (--attempts_remaining > 0)\n+\t\t\tgoto retry;\n+\t\t/* fall through */\n+\tdefault:\n+\t\tstrbuf_addf(err, \"unable to create directory for %s\",\n+\t\t\t ref_file.buf);\n+\t\tgoto error_return;\n+\t}\n+\n+\tif (!lock->lk)\n+\t\tlock->lk = xcalloc(1, sizeof(struct lock_file));\n+\n+\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {\n+\t\tif (errno == ENOENT && --attempts_remaining > 0) {\n+\t\t\t/*\n+\t\t\t * Maybe somebody just deleted one of the\n+\t\t\t * directories leading to ref_file. Try\n+\t\t\t * again:\n+\t\t\t */\n+\t\t\tgoto retry;\n+\t\t} else {\n+\t\t\tunable_to_lock_message(ref_file.buf, errno, err);\n+\t\t\tgoto error_return;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Now we hold the lock and can read the reference without\n+\t * fear that its value will change.\n+\t */\n+\n+\tif (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {\n+\t\tif (errno == ENOENT) {\n+\t\t\tif (mustexist) {\n+\t\t\t\t/* Garden variety missing reference. */\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t\tgoto error_return;\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Reference is missing, but that's OK. We\n+\t\t\t\t * know that there is not a conflict with\n+\t\t\t\t * another loose reference because\n+\t\t\t\t * (supposing that we are trying to lock\n+\t\t\t\t * reference \"refs/foo/bar\"):\n+\t\t\t\t *\n+\t\t\t\t * - We were successfully able to create\n+\t\t\t\t * the lockfile refs/foo/bar.lock, so we\n+\t\t\t\t * know there cannot be a loose reference\n+\t\t\t\t * named \"refs/foo\".\n+\t\t\t\t *\n+\t\t\t\t * - We got ENOENT and not EISDIR, so we\n+\t\t\t\t * know that there cannot be a loose\n+\t\t\t\t * reference named \"refs/foo/bar/baz\".\n+\t\t\t\t */\n+\t\t\t}\n+\t\t} else if (errno == EISDIR) {\n+\t\t\t/*\n+\t\t\t * There is a directory in the way. It might have\n+\t\t\t * contained references that have been deleted. If\n+\t\t\t * we don't require that the reference already\n+\t\t\t * exists, try to remove the directory so that it\n+\t\t\t * doesn't cause trouble when we want to rename the\n+\t\t\t * lockfile into place later.\n+\t\t\t */\n+\t\t\tif (mustexist) {\n+\t\t\t\t/* Garden variety missing reference. */\n+\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n+\t\t\t\t\t refname);\n+\t\t\t\tgoto error_return;\n+\t\t\t} else if (remove_dir_recursively(&ref_file,\n+\t\t\t\t\t\t\t REMOVE_DIR_EMPTY_ONLY)) {\n+\t\t\t\tif (verify_refname_available_dir(\n+\t\t\t\t\t\t refname, extras, skip,\n+\t\t\t\t\t\t get_loose_refs(&ref_cache),\n+\t\t\t\t\t\t err)) {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * The error message set by\n+\t\t\t\t\t * verify_refname_available() is OK.\n+\t\t\t\t\t */\n+\t\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t\t\tgoto error_return;\n+\t\t\t\t} else {\n+\t\t\t\t\t/*\n+\t\t\t\t\t * We can't delete the directory,\n+\t\t\t\t\t * but we also don't know of any\n+\t\t\t\t\t * references that it should\n+\t\t\t\t\t * contain.\n+\t\t\t\t\t */\n+\t\t\t\t\tstrbuf_addf(err, \"there is a non-empty directory '%s' \"\n+\t\t\t\t\t\t \"blocking reference '%s'\",\n+\t\t\t\t\t\t ref_file.buf, refname);\n+\t\t\t\t\tgoto error_return;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': \"\n+\t\t\t\t \"reference broken\", refname);\n+\t\t\tgoto error_return;\n+\t\t} else {\n+\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n+\t\t\t\t refname, strerror(errno));\n+\t\t\tgoto error_return;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If the ref did not exist and we are creating it,\n+\t\t * make sure there is no existing packed ref whose\n+\t\t * name begins with our refname, nor a packed ref\n+\t\t * whose name is a proper prefix of our refname.\n+\t\t */\n+\t\tif (verify_refname_available_dir(\n+\t\t\t\t refname, extras, skip,\n+\t\t\t\t get_packed_refs(&ref_cache),\n+\t\t\t\t err)) {\n+\t\t\tgoto error_return;\n+\t\t}\n+\t}\n+\n+\tret = 0;\n+\tgoto out;\n+\n+error_return:\n+\tunlock_ref(lock);\n+\t*lock_p = NULL;\n+\n+out:\n+\tstrbuf_release(&ref_file);\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 * value that is already stored in it.\n@@ -3052,55 +3279,201 @@ static int ref_update_reject_duplicates(struct string_list *refnames,\n }\n \n /*\n- * Acquire all locks, verify old values if provided, check\n- * that new values are valid, and write new values to the\n- * lockfiles, ready to be activated. Only keep one lockfile\n- * open at a time to avoid running out of file descriptors.\n+ * If update is a direct update of head_ref (the reference pointed to\n+ * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.\n+ */\n+static int split_head_update(struct ref_update *update,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *head_ref,\n+\t\t\t struct string_list *affected_refnames,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct string_list_item *item;\n+\tstruct ref_update *new_update;\n+\n+\tif ((update->flags & REF_LOG_ONLY) ||\n+\t (update->flags & REF_ISPRUNING) ||\n+\t (update->flags & REF_UPDATE_VIA_HEAD))\n+\t\treturn 0;\n+\n+\tif (strcmp(update->refname, head_ref))\n+\t\treturn 0;\n+\n+\t/*\n+\t * First make sure that HEAD is not already in the\n+\t * transaction. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per transaction.\n+\t */\n+\titem = string_list_insert(affected_refnames, \"HEAD\");\n+\tif (item->util) {\n+\t\t/* An entry already existed */\n+\t\tstrbuf_addf(err,\n+\t\t\t \"multiple updates for 'HEAD' (including one \"\n+\t\t\t \"via its referent '%s') are not allowed\",\n+\t\t\t update->refname);\n+\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t}\n+\n+\tnew_update = ref_transaction_add_update(\n+\t\t\ttransaction, \"HEAD\",\n+\t\t\tupdate->flags | REF_LOG_ONLY | REF_NODEREF,\n+\t\t\tupdate->new_sha1, update->old_sha1,\n+\t\t\tupdate->msg);\n+\n+\titem->util = new_update;\n+\n+\treturn 0;\n+}\n+\n+/*\n+ * update is for a symref that points at referent and doesn't have\n+ * REF_NODEREF set. Split it into two updates:\n+ * - The original update, but with REF_LOG_ONLY and REF_NODEREF set\n+ * - A new, separate update for the referent reference\n+ * Note that the new update will itself be subject to splitting when\n+ * the iteration gets to it.\n+ */\n+static int split_symref_update(struct ref_update *update,\n+\t\t\t const char *referent,\n+\t\t\t struct ref_transaction *transaction,\n+\t\t\t struct string_list *affected_refnames,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct string_list_item *item;\n+\tstruct ref_update *new_update;\n+\tunsigned int new_flags;\n+\n+\t/*\n+\t * First make sure that referent is not already in the\n+\t * transaction. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per symref in a\n+\t * transaction.\n+\t */\n+\titem = string_list_insert(affected_refnames, referent);\n+\tif (item->util) {\n+\t\t/* An entry already existed */\n+\t\tstrbuf_addf(err,\n+\t\t\t \"multiple updates for '%s' (including one \"\n+\t\t\t \"via symref '%s') are not allowed\",\n+\t\t\t referent, update->refname);\n+\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t}\n+\n+\tnew_flags = update->flags;\n+\tif (!strcmp(update->refname, \"HEAD\")) {\n+\t\t/*\n+\t\t * Record that the new update came via HEAD, so that\n+\t\t * when we process it, split_head_update() doesn't try\n+\t\t * to add another reflog update for HEAD. Note that\n+\t\t * this bit will be propagated if the new_update\n+\t\t * itself needs to be split.\n+\t\t */\n+\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n+\t}\n+\n+\tnew_update = ref_transaction_add_update(\n+\t\t\ttransaction, referent, new_flags,\n+\t\t\tupdate->new_sha1, update->old_sha1,\n+\t\t\tupdate->msg);\n+\n+\t/* Change the symbolic ref update to log only: */\n+\tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n+\n+\titem->util = new_update;\n+\n+\treturn 0;\n+}\n+\n+/*\n+ * Prepare for carrying out update:\n+ * - Lock the reference referred to by update.\n+ * - Read the reference under lock.\n+ * - Check that its old SHA-1 value (if specified) is correct, and in\n+ * any case record it for later use in the reflog.\n+ * - If it is a symref update without REF_NODEREF, split it up into a\n+ * REF_LOG_ONLY update of the symref and add a separate update for\n+ * the referent to transaction.\n+ * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY\n+ * update of HEAD.\n */\n static int lock_ref_for_update(struct ref_update *update,\n \t\t\t struct ref_transaction *transaction,\n+\t\t\t const char *head_ref,\n \t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n+\tstruct strbuf referent = STRBUF_INIT;\n+\tint mustexist = (update->flags & REF_HAVE_OLD) &&\n+\t\t!is_null_sha1(update->old_sha1);\n \tint ret;\n+\tstruct ref_lock *lock;\n \n-\tif ((update->flags & REF_HAVE_NEW) &&\n-\t is_null_sha1(update->new_sha1))\n+\tif ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n \t\tupdate->flags |= REF_DELETING;\n-\tupdate->lock = lock_ref_sha1_basic(\n-\t\t\tupdate->refname,\n-\t\t\t((update->flags & REF_HAVE_OLD) ?\n-\t\t\t update->old_sha1 : NULL),\n-\t\t\taffected_refnames, NULL,\n-\t\t\tupdate->flags,\n-\t\t\t&update->type,\n-\t\t\terr);\n-\tif (!update->lock) {\n+\n+\tif (head_ref) {\n+\t\tret = split_head_update(update, transaction, head_ref,\n+\t\t\t\t\taffected_refnames, err);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t}\n+\n+\tret = lock_raw_ref(update->refname, mustexist,\n+\t\t\t affected_refnames, NULL,\n+\t\t\t &update->lock, &referent,\n+\t\t\t &update->type, err);\n+\n+\tif (ret) {\n \t\tchar *reason;\n \n-\t\tret = (errno == ENOTDIR)\n-\t\t\t? TRANSACTION_NAME_CONFLICT\n-\t\t\t: TRANSACTION_GENERIC_ERROR;\n \t\treason = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t update->refname, reason);\n \t\tfree(reason);\n \t\treturn ret;\n \t}\n+\n+\tlock = update->lock;\n+\n+\tif (read_ref_full(update->refname,\n+\t\t\t mustexist ? RESOLVE_REF_READING : 0,\n+\t\t\t lock->old_oid.hash, NULL)) {\n+\t\tif (update->flags & REF_HAVE_OLD) {\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': can't resolve old value\",\n+\t\t\t\t update->refname);\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t} else {\n+\t\t\thashclr(lock->old_oid.hash);\n+\t\t}\n+\t}\n+\tif ((update->flags & REF_HAVE_OLD) &&\n+\t hashcmp(lock->old_oid.hash, update->old_sha1)) {\n+\t\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t\t update->refname,\n+\t\t\t sha1_to_hex(lock->old_oid.hash),\n+\t\t\t sha1_to_hex(update->old_sha1));\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t}\n+\n+\tif (update->type & REF_ISSYMREF) {\n+\t\tif (!(update->flags & REF_NODEREF)) {\n+\t\t\tret = split_symref_update(update, referent.buf, transaction,\n+\t\t\t\t\t\t affected_refnames, err);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t}\n+\t}\n+\n \tif ((update->flags & REF_HAVE_NEW) &&\n \t !(update->flags & REF_DELETING) &&\n \t !(update->flags & REF_LOG_ONLY)) {\n-\t\tint overwriting_symref = ((update->type & REF_ISSYMREF) &&\n-\t\t\t\t\t (update->flags & REF_NODEREF));\n-\n-\t\tif (!overwriting_symref &&\n-\t\t !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {\n+\t\tif (!(update->type & REF_ISSYMREF) &&\n+\t\t !hashcmp(lock->old_oid.hash, update->new_sha1)) {\n \t\t\t/*\n \t\t\t * The reference already has the desired\n \t\t\t * value, so we don't need to write it.\n \t\t\t */\n-\t\t} else if (write_ref_to_lockfile(update->lock,\n-\t\t\t\t\t\t update->new_sha1,\n+\t\t} else if (write_ref_to_lockfile(lock, update->new_sha1,\n \t\t\t\t\t\t err)) {\n \t\t\tchar *write_err = strbuf_detach(err, NULL);\n \n@@ -3124,7 +3497,7 @@ static int lock_ref_for_update(struct ref_update *update,\n \t\t * the lockfile is still open. Close it to\n \t\t * free up the file descriptor:\n \t\t */\n-\t\tif (close_ref(update->lock)) {\n+\t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n \t\t\treturn TRANSACTION_GENERIC_ERROR;\n@@ -3141,6 +3514,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *ref_to_delete;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n+\tchar *head_ref = NULL;\n+\tint head_type;\n+\tstruct object_id head_oid;\n \n \tassert(err);\n \n@@ -3152,9 +3528,25 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Fail if a refname appears more than once in the transaction: */\n-\tfor (i = 0; i < transaction->nr; i++)\n-\t\tstring_list_append(&affected_refnames, updates[i]->refname);\n+\t/*\n+\t * Fail if a refname appears more than once in the\n+\t * transaction. (If we end up splitting up any updates using\n+\t * split_symref_update() or split_head_update(), those\n+\t * functions will check that the new updates don't have the\n+\t * same refname as any existing ones.)\n+\t */\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\t\tstruct string_list_item *item =\n+\t\t\tstring_list_append(&affected_refnames, update->refname);\n+\n+\t\t/*\n+\t\t * We store a pointer to update in item->util, but at\n+\t\t * the moment we never use the value of this field\n+\t\t * except to check whether it is non-NULL.\n+\t\t */\n+\t\titem->util = update;\n+\t}\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n@@ -3162,6 +3554,32 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \t/*\n+\t * Special hack: If a branch is updated directly and HEAD\n+\t * points to it (may happen on the remote side of a push\n+\t * for example) then logically the HEAD reflog should be\n+\t * updated too.\n+\t *\n+\t * A generic solution would require reverse symref lookups,\n+\t * but finding all symrefs pointing to a given branch would be\n+\t * rather costly for this rare event (the direct update of a\n+\t * branch) to be worth it. So let's cheat and check with HEAD\n+\t * only, which should cover 99% of all usage scenarios (even\n+\t * 100% of the default ones).\n+\t *\n+\t * So if HEAD is a symbolic reference, then record the name of\n+\t * the reference that it points to. If we see an update of\n+\t * head_ref within the transaction, then split_head_update()\n+\t * arranges for the reflog of HEAD to be updated, too.\n+\t */\n+\thead_ref = resolve_refdup(\"HEAD\", RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t head_oid.hash, &head_type);\n+\n+\tif (head_ref && !(head_type & REF_ISSYMREF)) {\n+\t\tfree(head_ref);\n+\t\thead_ref = NULL;\n+\t}\n+\n+\t/*\n \t * Acquire all locks, verify old values if provided, check\n \t * that new values are valid, and write new values to the\n \t * lockfiles, ready to be activated. Only keep one lockfile\n@@ -3170,7 +3588,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tret = lock_ref_for_update(update, transaction,\n+\t\tret = lock_ref_for_update(update, transaction, head_ref,\n \t\t\t\t\t &affected_refnames, err);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n@@ -3179,23 +3597,35 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_lock *lock = update->lock;\n \n \t\tif (update->flags & REF_NEEDS_COMMIT ||\n \t\t update->flags & REF_LOG_ONLY) {\n-\t\t\tif (commit_ref_update(update->lock,\n-\t\t\t\t\t update->new_sha1, update->msg,\n-\t\t\t\t\t update->flags, err)) {\n-\t\t\t\t/* freed by commit_ref_update(): */\n-\t\t\t\tupdate->lock = NULL;\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n-\t\t\t} else {\n-\t\t\t\t/* freed by commit_ref_update(): */\n-\t\t\t\tupdate->lock = NULL;\n-\t\t\t}\n-\t\t}\n-\t}\n+\t\t\tif (log_ref_write(lock->ref_name, lock->old_oid.hash,\n+\t\t\t\t\t update->new_sha1,\n+\t\t\t\t\t update->msg, update->flags, err)) {\n+\t\t\t\tchar *old_msg = strbuf_detach(err, NULL);\n \n+\t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n+\t\t\t\t\t lock->ref_name, old_msg);\n+\t\t\t\tfree(old_msg);\n+\t\t\t\tunlock_ref(lock);\n+\t\t\t\tupdate->lock = NULL;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t\tif (update->flags & REF_NEEDS_COMMIT) {\n+\t\t\tclear_loose_ref_cache(&ref_cache);\n+\t\t\tif (commit_ref(lock)) {\n+\t\t\t\tstrbuf_addf(err, \"couldn't set '%s'\", lock->ref_name);\n+\t\t\t\tunlock_ref(lock);\n+\t\t\t\tupdate->lock = NULL;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t}\n+\t}\n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = updates[i];\n@@ -3228,7 +3658,9 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tstring_list_clear(&refs_to_delete, 0);\n+\tfree(head_ref);\n \tstring_list_clear(&affected_refnames, 0);\n+\n \treturn ret;\n }\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex babdf27..cccd76b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -50,6 +50,12 @@\n #define REF_LOG_ONLY 0x80\n \n /*\n+ * Internal flag, meaning that the containing ref_update was via an\n+ * update to HEAD.\n+ */\n+#define REF_UPDATE_VIA_HEAD 0x100\n+\n+/*\n * Return true iff refname is minimally safe. \"Safe\" here means that\n * deleting a loose reference by this name will not do any damage, for\n * example by causing a file that is not a reference to be deleted.\n@@ -148,11 +154,12 @@ struct ref_update {\n \tunsigned char old_sha1[20];\n \t/*\n \t * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,\n-\t * REF_DELETING, and REF_ISPRUNING:\n+\t * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and\n+\t * REF_UPDATE_VIA_HEAD:\n \t */\n \tunsigned int flags;\n \tstruct ref_lock *lock;\n-\tint type;\n+\tunsigned int type;\n \tchar *msg;\n \tconst char refname[FLEX_ARRAY];\n };\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 08bd8fd..d226930 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1102,6 +1102,41 @@ test_expect_success 'stdin -z delete refs works with packed and loose refs' '\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n+test_expect_success 'fails with duplicate HEAD update' '\n+\tgit branch target1 $A &&\n+\tgit checkout target1 &&\n+\tcat >stdin <<-EOF &&\n+\tupdate refs/heads/target1 $C\n+\toption no-deref\n+\tupdate HEAD $B\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err &&\n+\tgrep \"fatal: multiple updates for '\\''HEAD'\\'' (including one via its referent .refs/heads/target1.) are not allowed\" err &&\n+\techo \"refs/heads/target1\" >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"$A\" >expect &&\n+\tgit rev-parse refs/heads/target1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fails with duplicate ref update via symref' '\n+\tgit branch target2 $A &&\n+\tgit symbolic-ref refs/heads/symref2 refs/heads/target2 &&\n+\tcat >stdin <<-EOF &&\n+\tupdate refs/heads/target2 $C\n+\tupdate refs/heads/symref2 $B\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err &&\n+\tgrep \"fatal: multiple updates for '\\''refs/heads/target2'\\'' (including one via symref .refs/heads/symref2.) are not allowed\" err &&\n+\techo \"refs/heads/target2\" >expect &&\n+\tgit symbolic-ref refs/heads/symref2 >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"$A\" >expect &&\n+\tgit rev-parse refs/heads/target2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n run_with_limited_open_files () {\n \t(ulimit -n 32 && \"$@\")\n }\n-- \n2.8.1\n"},{"id":"286451","messageId":"20160513125155.GA24031@sigill.intra.peff.net","threadId":"42250","inReplyTo":"5735C990.8080502@alum.mit.edu","subject":"Re: [PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-13T12:51:56Z","receivedAt":"2016-05-13T12:51:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 13, 2016 at 02:33:20PM +0200, Michael Haggerty wrote:\n\n> The problem in this case is a misguided call to\n> verify_refname_available_dir() in the case that read_raw_ref() fails\n> with ENOENT. In that case it is not possible for there to be a conflict\n> with another loose reference, because (1) we already hold the lock, so\n> the containing directory must exist, and (2) we got ENOENT, so there\n> can't be a loose reference in a subdirectory named after the reference\n> that we are trying to create.\n\nRight, that makes perfect sense.\n\n> As Peff explained, the call of verify_refname_available_dir() was\n> forcing the loose tags to be loaded, which is expensive in this test\n> because there are 100000 of them being created one at a time. (If they\n> were created in a single ref_transaction instead, the \"available\" tests\n> would all be done together, before any changes are committed, so the\n> loose ref cache would not have to be invalidated each time.)\n\nYeah, I noticed that, too, and had to wonder whether we should make\n\"fetch\" do a single ref transaction. I think the thing blocking that\n(besides the obvious refactoring needed in fetch) is that transactions\nare currently all-or-nothing. And fetch right now will do its best to\nupdate whatever refs it can.\n\n> Please note that there are still some calls of\n> verify_refname_available_dir() against the loose reference cache in this\n> function. If we wanted to give up a little bit on the quality of our\n> error messages, I think we could make those paths faster, too. But they\n> are all in failure paths, so I don't think that they are performance\n> critical, so I won't make that change.\n\nThat's probably fine. I think the most pathological case becomes a fetch\na fetch where every other incoming ref is bogus. So, \"refs/tags/2/nope\",\n\"refs/tags/2-ok\". And there are 100K of those (or whatever large number\nyou choose).\n\nThe bogus refs cause us to load the loose ref cache, and the successful\nones cause us to discard it. The result is still quadratic (it's just\nn/2 squared). I have a hard time believing anybody would run into this\nsituation in practice, but I do wonder if somebody could cause\ndenial-of-service mischief. Probably not any more than they could cause\nby other means (like sending a nasty diff case).\n\n-Peff\n"},{"id":"286452","messageId":"20160513125822.GB24031@sigill.intra.peff.net","threadId":"42250","inReplyTo":"e87d7f0f441a9094d75cae7a39aac1e8760426e9.1463140520.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 29/33] refs: resolve symbolic refs first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-13T12:58:22Z","receivedAt":"2016-05-13T12:58:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 13, 2016 at 02:35:40PM +0200, Michael Haggerty wrote:\n\n> +\tif (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {\n> +\t\tif (errno == ENOENT) {\n> +\t\t\tif (mustexist) {\n> +\t\t\t\t/* Garden variety missing reference. */\n> +\t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n> +\t\t\t\t\t refname);\n> +\t\t\t\tgoto error_return;\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t * Reference is missing, but that's OK. We\n> +\t\t\t\t * know that there is not a conflict with\n> +\t\t\t\t * another loose reference because\n> +\t\t\t\t * (supposing that we are trying to lock\n> +\t\t\t\t * reference \"refs/foo/bar\"):\n> +\t\t\t\t *\n> +\t\t\t\t * - We were successfully able to create\n> +\t\t\t\t * the lockfile refs/foo/bar.lock, so we\n> +\t\t\t\t * know there cannot be a loose reference\n> +\t\t\t\t * named \"refs/foo\".\n> +\t\t\t\t *\n> +\t\t\t\t * - We got ENOENT and not EISDIR, so we\n> +\t\t\t\t * know that there cannot be a loose\n> +\t\t\t\t * reference named \"refs/foo/bar/baz\".\n> +\t\t\t\t */\n> +\t\t\t}\n\nThanks for this comment, I think it makes the logic easier to follow.\n\nI re-ran t5551 and confirmed that the timings look good (though I\nimagine you might have already done that, too :) ).\n\n-Peff\n"},{"id":"286527","messageId":"44e99d76-a01f-b97c-65a7-d9b1b5e83eb4@web.de","threadId":"42250","inReplyTo":"5735C990.8080502@alum.mit.edu","subject":"Re: [PATCH v2 29/33] refs: resolve symbolic refs first","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-14T09:02:04Z","receivedAt":"2016-05-14T09:02:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 13.05.16 14:33, Michael Haggerty wrote:\n> Torsten, thanks for the report. Peff, thanks for the analysis.\nI didn't follow all the details.\nThe new \"pu\" passes with no errors on all of my test systems :-)\n \n \n"},{"id":"288923","messageId":"575AB797.809@alum.mit.edu","threadId":"42250","inReplyTo":"cover.1462550456.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-10T12:50:31Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2016 06:13 PM, Michael Haggerty wrote:\n> [...]\n> This patch series is also available from my GitHub repo [2] as branch\n> \"split-under-lock\".\n> \n> [1] http://thread.gmane.org/gmane.comp.version-control.git/292772\n> [2] https://github.com/mhagger/git\n\nI was reading this area of the code again, and I found a problem in this\npatch series. Now that ref_updates can be added to ref_transactions\nwhile they are being processed, it is not correct to store a pointer to\ntransaction->updates in ref_transaction_commit() here [1], because the\narray might be moved by realloc() if it grows during the function. The\nproblem wasn't detected during testing because an added commit would\nhave to cross an ALLOC_GROW boundary to trigger the bug.\n\nThe fix is obvious but it is textually quite a few lines. For good\nmeasure, the same fix should be made in initial_ref_transaction_commit()\nhere [2].\n\nThe most logical place to fix this is by expanding commit\n\"ref_transaction_commit(): remove local variable n\" [3], so I've done\nthat and force pushed the result to my GitHub account [4] as branch\n\"split-under-lock\".\n\nJunio, if you want to incorporate this revised version of the branch in\nyour big rewind of next, then we can pretend that the bug was never\nthere :-) Otherwise, tell me in what form you would like the fix and I\nwill be happy to provide it.\n\nSorry for finding this problem so late in the process.\n\nMichael\n\n[1]\nhttps://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3559\n[2]\nhttps://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3725\n[3] http://article.gmane.org/gmane.comp.version-control.git/293801\n[4] https://github.com/mhagger/git\n"},{"id":"288927","messageId":"xmqq8tydgic2.fsf@gitster.mtv.corp.google.com","threadId":"42250","inReplyTo":"575AB797.809@alum.mit.edu","subject":"Re: [PATCH v2 00/33] Yet more preparation for reference backends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-10T15:43:41Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Junio, if you want to incorporate this revised version of the branch in\n> your big rewind of next, then we can pretend that the bug was never\n> there :-) Otherwise, tell me in what form you would like the fix and I\n> will be happy to provide it.\n\nWe actually can do both ;-). That is, when we rewind 'next' after\nthe upcoming release, we'd queue a series without a bug, but you can\nshow a preview to the list in a form of an incremental patch so that\nothers have a chance to see how the end result would look like.\n\nThanks.\n"},{"id":"289095","messageId":"328d460b1730c0b192961234c06036514498c1b1.1465808670.git.mhagger@alum.mit.edu","threadId":"42250","inReplyTo":"xmqq8tydgic2.fsf@gitster.mtv.corp.google.com","subject":"[ADDENDUM v4] Yet more preparation for reference backends","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-13T09:55:57Z","receivedAt":"2016-06-16T02:19:53Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Below is the delta needed to fixed the bug in the mh/split-under-lock\npatch series that I mentioned in an earlier email [1], plus a little\ntweak to make the docstring for lock_ref_for_update() clearer.\n\nI actually fixed the bug in preparatory commit\n\n ref_transaction_commit(): remove local variable n\n\n, whose subject was accordingly changed to\n\n ref_transaction_commit(): remove local variables n and updates\n\n[2]. See branch \"split-under-lock\" in my GitHub fork [3] for the full\nrevised patch series (including the changes to a later patch that are\nnecessary when it is rebased onto the fix). In the same repo I've also\nrebased dependent branches update-ref-errors, ref-iterators, and\nref-store on top of this one; all of those rebases were trivial.\n\nHere, for the convenience of reviewers, I present the delta between\nthe old and new end states of the split-under-lock patch series. (It\ndidn't seem justified to re-send the whole 33-patch series for this\nlogically small change.)\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/297002\n[2] https://github.com/gitster/git/commit/efe472813d60befd72d6e2797934c90b22a26c93\n[3] https://github.com/mhagger/git\n\n---\n refs/files-backend.c | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1230dfb..bbf96ad 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3393,7 +3393,8 @@ static const char *original_update_refname(struct ref_update *update)\n * - Lock the reference referred to by update.\n * - Read the reference under lock.\n * - Check that its old SHA-1 value (if specified) is correct, and in\n- * any case record it for later use in the reflog.\n+ * any case record it in update->lock->old_oid for later use when\n+ * writing the reflog.\n * - If it is a symref update without REF_NODEREF, split it up into a\n * REF_LOG_ONLY update of the symref and add a separate update for\n * the referent to transaction.\n@@ -3556,7 +3557,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tint ret = 0, i;\n-\tstruct ref_update **updates = transaction->updates;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n \tstruct string_list_item *ref_to_delete;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n@@ -3582,7 +3582,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t * same refname as any existing ones.)\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct string_list_item *item =\n \t\t\tstring_list_append(&affected_refnames, update->refname);\n \n@@ -3632,7 +3632,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t * open at a time to avoid running out of file descriptors.\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \n \t\tret = lock_ref_for_update(update, transaction, head_ref,\n \t\t\t\t\t &affected_refnames, err);\n@@ -3642,7 +3642,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->lock;\n \n \t\tif (update->flags & REF_NEEDS_COMMIT ||\n@@ -3674,7 +3674,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n@@ -3701,8 +3701,8 @@ cleanup:\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \n \tfor (i = 0; i < transaction->nr; i++)\n-\t\tif (updates[i]->lock)\n-\t\t\tunlock_ref(updates[i]->lock);\n+\t\tif (transaction->updates[i]->lock)\n+\t\t\tunlock_ref(transaction->updates[i]->lock);\n \tstring_list_clear(&refs_to_delete, 0);\n \tfree(head_ref);\n \tstring_list_clear(&affected_refnames, 0);\n@@ -3722,7 +3722,6 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err)\n {\n \tint ret = 0, i;\n-\tstruct ref_update **updates = transaction->updates;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \n \tassert(err);\n@@ -3732,7 +3731,8 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Fail if a refname appears more than once in the transaction: */\n \tfor (i = 0; i < transaction->nr; i++)\n-\t\tstring_list_append(&affected_refnames, updates[i]->refname);\n+\t\tstring_list_append(&affected_refnames,\n+\t\t\t\t transaction->updates[i]->refname);\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n@@ -3755,7 +3755,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\tdie(\"BUG: initial ref transaction called with existing refs\");\n \n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \n \t\tif ((update->flags & REF_HAVE_OLD) &&\n \t\t !is_null_sha1(update->old_sha1))\n@@ -3776,7 +3776,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = updates[i];\n+\t\tstruct ref_update *update = transaction->updates[i];\n \n \t\tif ((update->flags & REF_HAVE_NEW) &&\n \t\t !is_null_sha1(update->new_sha1))\n-- \n2.8.1\n"}]}