{"thread":{"id":"42157","subject":"[PATCH 03/29] refname_is_safe(): use skip_prefix()","startedAt":"2016-04-27T16:57:17Z","lastAt":"2016-05-02T18:06:56Z","messageCount":70,"participants":["Michael Haggerty","Junio C Hamano","David Turner","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":29},"messages":[{"id":"284694","messageId":"cover.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":null,"subject":"[PATCH 00/29] Yet more preparation for reference backends","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:17Z","receivedAt":"2016-04-27T16:57:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This started as a modest attempt to move the last couple of patches\nmentioned here [1] to before the vtable patches. I wanted to do that\nbecause having real changes mixed with the vtable refactoring was\nmaking rebasing and stuff awkward.\n\nBut then it snowballed. A lot of what's new is pretty trivial (though\nin some cases fixes minor bugs). But a few of the later patches are\npretty major.\n\nThe two main problems addressed by this series are:\n\n1. Reference transactions will, in the future, sometimes span two\n completely different reference backends. For example, we already\n have worktree-specific refs like `HEAD` and `refs/bisect/*`, which\n are stored within the worktree, plus shared refs, which live in the\n main repository. It is even possible for a symref in one reference\n backend to point at a reference in another reference backend. Thus\n we need to be able to split one main transaction into one\n transaction per backend.\n\n2. Currently, reference transactions that involve symbolic refs are\n not atomic: the symref is not locked at all, even when its reflog\n is being updated. This is a no-no. It also means that its referent\n can change between the time that the symref is resolved to find out\n its old_oid and the time that the referent is locked. These aren't\n super serious races because symrefs are usually not modified very\n often (especially on Git servers, which is mostly where concurrent\n modifications are an issue). But still...\n\nThe approach taken to solve these problems is inspired by David\nTurner's patch [2], whose approach was first discussed here [3]. David\nwrote a separate function, dereference_symrefs(transaction, ...),\nwhich scanned through the whole transaction and split up any symref\nupdates into one symref update with the REF_LOG_ONLY option (to update\nthe symrefs reflog), plus a second update that changes the underlying\nreference. This approach solves problem 1 but not problem 2.\n\nThis patch series takes a slightly different approach. For each\nreference update during the \"lock references\" part of\nref_transaction_commit(), it\n\n1. Locks the named reference. (If it is a symref, lock *the symref*,\n not its referent!)\n\n2. Reads the reference non-recursively\n\n3. If it is a symref, change the update to REF_LOG_ONLY and add a\n second update to the transaction for the underlying reference.\n\n4. If it is not a symref but was derived from a symref update, record\n the reference's old_oid in the ref_update structure for the\n original symref.\n\nIn this way, each reference in a symref chain is locked down *before*\nwe read it and the lock is held throughout the transaction. As a\nbonus, we don't have to use resolve_ref_full() to find old_oid for the\nreferences; it is enough to read the references *once*, because we do\nit under lock.\n\nThe three patches starting with \"refs: resolve symbolic refs first\"\ninvolve a lot of new code in an area that is pretty intricate. I've\nreviewed them a few times, but it's quite possible I've made some\nmistakes (especially in the error-handling code). Careful review here\nwould be much appreciated.\n\nIt's possible that people will think this is too much new code for\nfixing a relatively unlikely race. I'm not absolutely convinced myself\nthat it's not overengineered. But splitting ref_updates is definitely\nneeded for supporting multiple backends, and I think the approach of\nlocking then following one reference at a time is more or less a\nprerequisite for making symref locking work correctly with multiple\nreference backends. So (I think) our choices are to accept this code\nor something like it, or to give up the hope of correct atomicity of\ntransactions that span backends.\n\nThis patch series is also available from my GitHub repository [4] as\nbranch \"split-under-lock\". It applies to Junio's current master.\n\nIncidentally, a draft of the *next* patch series, which adds a\nref_store vtable abstraction for managing reference backends, is\navailable as branch \"ref-store\" in my GitHub repo. That branch passes\nall tests but is not yet carefully reviewed.\n\nFollowing is a table of contents for this patch series...\n\nChapter 1: Prologue\n\n This chapter consists of small cleanups that I noticed while working\n on the code.\n\n * safe_create_leading_directories(): improve docstring\n * remove_dir_recursively(): add docstring\n * refname_is_safe(): use skip_prefix()\n\n This patch fixes a bug in refname_is_safe():\n\n * refname_is_safe(): don't allow the empty string\n\n This one makes refname_is_safe() a little stricter...it shouldn't\n allow refnames like \"refs/foo/../bar///baz\" because the downstream\n code isn't smart enough to handle them anyway. (At the latest if the\n reference has to be sought in packed-refs, things would fail):\n\n * refname_is_safe(): insist that the refname already be normalized\n\nChapter 2: Character development\n\n Make sure error output goes the right place:\n\n * commit_ref_update(): write error message to *err, not stderr\n\n Tighten up some code, rename some parameters:\n\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 * lock_ref_sha1_basic(): remove unneeded local variable\n\nChapter 3: Small adventures\n\n Change error messages to adhere to our new policy of starting with\n lower-case letters:\n\n * refs: make error messages more consistent\n\n Require REF_NODEREF if REF_ISPRUNING is set:\n\n * ref_transaction_create(): disallow recursive pruning\n\n Correctly handle an (unlikely) error path:\n\n * ref_transaction_commit(): correctly report close_ref() failure\n\n Oops, here's a problem that could have bit us later. (In fact, it\n did bite me when I implemented one of the later patches):\n\n * delete_branches(): use resolve_refdup()\n\nChapter 4: Flashback\n\n The following two patches are split out of one of David Turner's\n patches mentioned above:\n\n * refs: allow log-only updates\n * refs: don't dereference on rename\n\nChapter 5: The tension builds\n\n The protagonist clearly has something planned...but what?\n\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\n Here we force a little more refname safety while building up\n ref_transactions:\n\n * ref_transaction_update(): check refname_is_safe() at a minimum\n\nChapter 6: The climax\n\n This is the guts of the patch series. Everything up to this point\n should be pretty uncontroversial. These three patches are the main\n subject of the discussion above. They are pretty large patches, but\n I don't see a reasonable way to break them up more:\n\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\nChapter 7: Denouement\n\n Remove some stuff that's not needed anymore.\n\n * commit_ref_update(): remove the flags parameter\n * lock_ref_sha1_basic(): only handle REF_NODEREF mode\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/289994\n[2] http://thread.gmane.org/gmane.comp.version-control.git/287949/focus=287958\n[3] http://article.gmane.org/gmane.comp.version-control.git/282927\n[4] 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 (27):\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 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 | 67 ++--\n refs/files-backend.c | 882 +++++++++++++++++++++++++++++++++++++-----------\n refs/refs-internal.h | 55 ++-\n t/t1400-update-ref.sh | 41 ++-\n t/t1430-bad-ref-name.sh | 2 +-\n t/t3200-branch.sh | 9 +\n 9 files changed, 869 insertions(+), 234 deletions(-)\n\n-- \n2.8.1\n"},{"id":"284693","messageId":"4e04c27d057a757f82d039d4875a95229e68ac4e.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 01/29] safe_create_leading_directories(): improve docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:18Z","receivedAt":"2016-04-27T16:57:18Z","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":"284692","messageId":"275274daa6126a8896b76ab3fca55d6e4a684948.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 02/29] remove_dir_recursively(): add docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:19Z","receivedAt":"2016-04-27T16:57:19Z","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":"284666","messageId":"b1ad00ad0962210fe1746012640312b40b22fa99.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 03/29] refname_is_safe(): use skip_prefix()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:20Z","receivedAt":"2016-04-27T16:57:20Z","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":"284667","messageId":"51574acf932c4650110d9f7be0601532879f624c.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 04/29] refname_is_safe(): don't allow the empty string","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:21Z","receivedAt":"2016-04-27T16:57:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\nThis fixes a coding error from the original implementation.\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":"284671","messageId":"a8e1e1a9e6a48fbb20fab2144279b93a48db584a.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:22Z","receivedAt":"2016-04-27T16:57:22Z","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---\nDoes anybody have a use case for allowing un-normalized reference\nnames like \"refs/foo/../bar///baz\"? I'm pretty certain they would not\nbe handled correctly anyway, especially if they are not stored as\nloose references.\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":"284668","messageId":"88629e7803570ef31459656184d01c73d3dc8b73.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 06/29] commit_ref_update(): write error message to *err, not stderr","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:23Z","receivedAt":"2016-04-27T16:57:23Z","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 1f38076..de38517 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2686,7 +2686,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":"284691","messageId":"518b8429d7b47dfdb5a74d22c7a72fc2b095c862.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 07/29] rename_ref(): remove unneeded local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:24Z","receivedAt":"2016-04-27T16:57:24Z","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 de38517..0ade681 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2351,20 +2351,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":"284669","messageId":"9c31a016aa893e4a3748e31ee3fd48562f63a7a7.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 08/29] ref_transaction_commit(): remove local variable n","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:25Z","receivedAt":"2016-04-27T16:57:25Z","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 0ade681..05797cb 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3043,7 +3043,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@@ -3054,13 +3053,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@@ -3074,7 +3073,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@@ -3145,7 +3144,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@@ -3164,7 +3163,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@@ -3190,7 +3189,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@@ -3210,7 +3209,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@@ -3220,7 +3218,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@@ -3243,7 +3241,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@@ -3264,7 +3262,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":"284675","messageId":"9ff68ec3e90555304f7bec4970dee769bbe0e668.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 09/29] read_raw_ref(): rename flags argument to type","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:26Z","receivedAt":"2016-04-27T16:57:26Z","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 | 16 ++++++++--------\n refs/refs-internal.h | 2 +-\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 05797cb..303c43b 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@@ -1510,7 +1510,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@@ -1521,7 +1521,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":"284674","messageId":"46e7314c13cfab5cb98f1435f596063826487421.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 10/29] read_raw_ref(): clear *type at start of function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:27Z","receivedAt":"2016-04-27T16:57:27Z","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 303c43b..f10c80f 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":"284670","messageId":"db498e789a50d870c42ebdff7b668cd979e466a0.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 11/29] read_raw_ref(): rename symref argument to referent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:28Z","receivedAt":"2016-04-27T16:57:28Z","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 f10c80f..364425c 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@@ -1509,8 +1510,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":"284672","messageId":"5b73d10ec47a68535725d77ebbc5c4f0512ac5f3.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 12/29] read_raw_ref(): improve docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:29Z","receivedAt":"2016-04-27T16:57:29Z","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 364425c..1d2ef2a 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":"284676","messageId":"6d5914605a939efe1bbe294ad121a002d69d329a.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 13/29] lock_ref_sha1_basic(): remove unneeded local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:30Z","receivedAt":"2016-04-27T16:57:30Z","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 1d2ef2a..b8c1779 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1767,7 +1767,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@@ -1775,7 +1775,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@@ -1795,7 +1794,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@@ -1813,10 +1812,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":"284690","messageId":"48602160ce11b6173c01769cac142a47b7f364b4.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 14/29] refs: make error messages more consistent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:31Z","receivedAt":"2016-04-27T16:57:31Z","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---\nThis change is not strictly needed, but I wanted to fix the old error\nmessages before I started adding new ones (otherwise, should the new\nerror messages be made to look like the old ones or should they be\ncorrected?) By the way, should these be internationalized?\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 b8c1779..9faf17c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1730,7 +1730,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@@ -1739,7 +1739,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@@ -1819,7 +1819,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@@ -1857,7 +1857,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@@ -2478,7 +2478,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@@ -2492,7 +2492,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@@ -2500,7 +2500,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@@ -2569,13 +2569,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@@ -2616,14 +2616,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@@ -2633,7 +2633,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@@ -2654,7 +2654,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@@ -2689,7 +2689,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@@ -3038,7 +3038,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@@ -3142,7 +3142,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":"284689","messageId":"615204c877610855b02b21ce14efa5b7342182bc.1461768689.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:32Z","receivedAt":"2016-04-27T16:57:32Z","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---\nThis also makes later patches a bit clearer.\n\n refs.c | 3 +++\n refs/files-backend.c | 2 +-\n 2 files changed, 4 insertions(+), 1 deletion(-)\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 9faf17c..8fcbd7d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2116,7 +2116,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);\n-- \n2.8.1\n"},{"id":"284678","messageId":"18490be7195bac46a955b385b6848a2383417188.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 16/29] ref_transaction_commit(): correctly report close_ref() failure","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:33Z","receivedAt":"2016-04-27T16:57:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\nclose() rarely fails, but it is possible.\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 8fcbd7d..e86e3de 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3144,6 +3144,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":"284688","messageId":"6fd1a07674b903e005a25184d2fef44dcc3f1d2f.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 17/29] delete_branches(): use resolve_refdup()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:34Z","receivedAt":"2016-04-27T16:57:34Z","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":"284673","messageId":"37887529301c11100ae7bc71495f72ed071a66ca.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 18/29] refs: allow log-only updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:35Z","receivedAt":"2016-04-27T16:57:35Z","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 e86e3de..91416db 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2688,7 +2688,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@@ -3106,7 +3106,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@@ -3138,8 +3139,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@@ -3154,7 +3156,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@@ -3173,7 +3176,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 37a1a37..6b53ba1 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":"284685","messageId":"27f8b223e42dcf1cf3c010833e0aff7baa4559c2.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 19/29] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:36Z","receivedAt":"2016-04-27T16:57:36Z","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 | 13 ++++++++-----\n t/t3200-branch.sh | 9 +++++++++\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 91416db..e4cdd5a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2358,11 +2358,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n \tstruct strbuf err = STRBUF_INIT;\n+\tconst int resolve_flags = RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE;\n \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_flags, orig_sha1, &flag))\n \t\treturn error(\"refname %s not found\", oldrefname);\n \n \tif (flag & REF_ISSYMREF)\n@@ -2380,8 +2381,8 @@ 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+\tif (!read_ref_full(newrefname, resolve_flags, 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@@ -2405,7 +2406,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@@ -2423,7 +2425,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":"284677","messageId":"2757ee65c6c9c9c389a6955adba9cea6d3fc5332.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 20/29] verify_refname_available(): adjust constness in declaration","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:37Z","receivedAt":"2016-04-27T16:57:37Z","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---\nThis is needed because I want to add a new caller that has const\nversions of these lists in hand.\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 e4cdd5a..e0d9fa3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2328,8 +2328,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 6b53ba1..aaf56ea 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":"284681","messageId":"a1a444f7863a6d4457095912793337b8ba18f6d7.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 21/29] add_update(): initialize the whole ref_update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:38Z","receivedAt":"2016-04-27T16:57:38Z","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---\nIn particular, it's nice that it returns the \"struct ref_update *\"\nthat it creates, which is useful for refs-internal code that might\nwant to tweak other field in the ref_update. But we don't want\nnon-refs code to get its hands on a ref_update.\n\n refs.c | 33 +++++++++++++++++----------------\n refs/refs-internal.h | 14 ++++++++++++++\n 2 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5dc2473..858b6d7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -766,13 +766,24 @@ 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 \tFLEX_ALLOC_STR(update, refname, refname);\n \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n \ttransaction->updates[transaction->nr++] = update;\n+\tupdate->flags = flags;\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,8 +794,6 @@ 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@@ -800,18 +809,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 aaf56ea..c46fe67 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":"284695","messageId":"1c34010ac77af9d3f7818d01ca267ff7bd0bf621.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 22/29] lock_ref_for_update(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:39Z","receivedAt":"2016-04-27T16:57:39Z","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 e0d9fa3..546656a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3048,6 +3048,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@@ -3085,74 +3167,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":"284680","messageId":"81ffd15cf36487d744e5565c49a5b105b1d06da9.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 23/29] unlock_ref(): move definition higher in the file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:40Z","receivedAt":"2016-04-27T16:57:40Z","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 546656a..8f2a795 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1545,6 +1545,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@@ -1703,16 +1713,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":"284686","messageId":"a67a1b745d0a14111c774f13a5776d3756cbf2f2.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:41Z","receivedAt":"2016-04-27T16:57:41Z","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---\nThere are remaining problems in this area of the code. For example,\ncheck_refname_format() is *less* strict than refname_is_safe(). It\nallows almost arbitrary top-level reference names like \"foo\" and\n\"refs\". (The latter is especially fun because if you manage to create\na reference called \"refs\", Git stops recognizing the directory as a\nGit repository.) On the other hand, some callers call\ncheck_refname_format() with incomplete reference names (e.g., branch\nnames like \"master\"), so the functions can't be made stricter until\nthose callers are changed.\n\nI'll address these problems separately if I find the time.\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 858b6d7..41eb9e2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\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+\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":"284687","messageId":"a33272db057ac6327cc0c87c002473c89ffcca6b.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 25/29] refs: resolve symbolic refs first","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:42Z","receivedAt":"2016-04-27T16:57:42Z","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 8f2a795..d0e932f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1556,6 +1556,226 @@ static void unlock_ref(struct ref_lock *lock)\n }\n \n /*\n+ * Lock refname, without following symrefs, and set lock to point at a\n+ * newly-allocated lock object. Fill in lock->old_oid, referent, and\n+ * 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 deleting, 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\tif (mustexist) {\n+\t\t\t\t/*\n+\t\t\t\t * Garden variety missing reference.\n+\t\t\t\t */\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 * There is a directory in the way,\n+\t\t\t\t\t * but we don't know of any references\n+\t\t\t\t\t * that it should contain. This might\n+\t\t\t\t\t * be a directory that used to contain\n+\t\t\t\t\t * references but is now empty. Try to\n+\t\t\t\t\t * remove it; otherwise it might cause\n+\t\t\t\t\t * trouble when we try to rename the\n+\t\t\t\t\t * lockfile into place.\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@@ -3049,55 +3269,203 @@ 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,\n+\t\t\t update->flags & REF_DELETING,\n+\t\t\t 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@@ -3121,7 +3489,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@@ -3138,6 +3506,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@@ -3149,9 +3520,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@@ -3159,6 +3546,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@@ -3167,7 +3580,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@@ -3176,23 +3589,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@@ -3225,7 +3650,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 c46fe67..9839b07 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":"284679","messageId":"d49fab134766bd2e2d2592f5cfd7cd06d1182536.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 26/29] lock_ref_for_update(): don't re-read non-symbolic references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:43Z","receivedAt":"2016-04-27T16:57:43Z","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 d0e932f..66ceb83 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3427,33 +3427,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":"284682","messageId":"dc9cd5747320dbbfc328f79b60ba8d6d78764fff.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 27/29] lock_ref_for_update(): don't resolve symrefs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:44Z","receivedAt":"2016-04-27T16:57:44Z","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---\nThere is one remaining race that I know of: the value of HEAD is read\nat the start of the transaction to determine whether its referent is\nchanged in the transaction. If so, the update is also logged in HEAD's\nreflog.\n\nThe problem is that HEAD is read without acquiring its lock. So in\nprinciple HEAD could change during the time that the transaction is\nbeing processed, leading to inconsistencies in the reflogs.\n\nThis problem could be fixed, but it would require HEAD to be locked\nfor every reference transaction, probably in some kind of an\nadditional fake ref_update. I don't know that it's worth the effort,\nbut if it is it can be done within the same framework that I\nimplemented here.\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 66ceb83..40ed157 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3366,8 +3366,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@@ -3375,6 +3382,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@@ -3428,44 +3446,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 9839b07..2355735 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":"284684","messageId":"9516d8e2b9b5853e3d28d0508eba372a1b860605.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 28/29] commit_ref_update(): remove the flags parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:45Z","receivedAt":"2016-04-27T16:57:45Z","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 40ed157..938871b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2568,7 +2568,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@@ -2636,7 +2636,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@@ -2656,7 +2656,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@@ -2870,12 +2870,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@@ -2911,7 +2911,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":"284683","messageId":"a3d853510719a37c1e4bbb52261169c25b19c148.1461768690.git.mhagger@alum.mit.edu","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"[PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-27T16:57:46Z","receivedAt":"2016-04-27T16:57:46Z","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* 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, 21 insertions(+), 33 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 938871b..5dca352 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@@ -1551,7 +1550,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@@ -1605,7 +1603,6 @@ static int lock_raw_ref(const char *refname, int deleting, 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@@ -1991,14 +1988,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@@ -2008,46 +2004,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+\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\tstrbuf_git_path(&ref_file, \"%s\", refname);\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@@ -2064,7 +2053,7 @@ 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_reset(&ref_file);\n \tstrbuf_git_path(&ref_file, \"%s\", refname);\n \n retry:\n@@ -2108,7 +2097,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@@ -2873,9 +2861,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@@ -2883,7 +2869,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@@ -2899,6 +2886,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@@ -2911,6 +2899,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@@ -3016,7 +3005,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":"284699","messageId":"xmqqinz39bl2.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"a8e1e1a9e6a48fbb20fab2144279b93a48db584a.1461768689.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T17:59:53Z","receivedAt":"2016-04-27T17:59:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Does anybody have a use case for allowing un-normalized reference\n> names like \"refs/foo/../bar///baz\"? I'm pretty certain they would not\n> be handled correctly anyway, especially if they are not stored as\n> loose references.\n\nI wondered what codepath this follows:\n\n $ git rev-parse mhagger/wip//split-under-lock\n\nget_sha1_basic() calls to dwim_ref() to learn real_ref and its\nvalue.\n\ndwim_ref() repeatedly gives the string to a known dwim rule, and the\n\"refs/remotes/%.*s\" rule is expected to find that the user meant to\nname \"refs/remotes/mhagger/wip//split-under-lock\".\n\nThis, still with doubled slash, is passed to resolve_ref_unsafe(),\nwhich has a call to !refname_is_safe(refname) to reject the request.\n\nSo I think this will move the rejection early in the codepath than\nhow we reject the ref with doubled slash in the current code, but\nthe end result would be the same for this example. The same is true\nfor heads//master or refs/heads//master.\n\nThere is another call to refname_is_safe() in resolve_ref_unsafe(),\nwhich applies the sanity check to the string from a symref. We seem\nto allow\n\n $ git symbolic-ref refs/heads/SSS refs/heads//master\n\nand we end up storing \"ref: refs/heads//master\" (or make a symbolic\nlink with doubled slashes), but the current code considers the\nresulting symbolic link as \"dangling\". Again, this change moves the\nrejection a bit earlier in the codepath, without changing the end\nresult, I'd think.\n"},{"id":"284704","messageId":"xmqqeg9qape3.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"9c31a016aa893e4a3748e31ee3fd48562f63a7a7.1461768689.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T18:16:20Z","receivedAt":"2016-04-27T18:16:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This microoptimization doesn't make a significant difference in speed.\n> And it causes problems if somebody ever wants to modify the function to\n> add updates to a transaction as part of processing it, as will happen\n> shortly.\n>\n> Make the same change in initial_ref_transaction_commit().\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nThis particular change also makes the end result easier to read. I\nhave no objection to officially declaring that we do support\n\"adding\" new transaction update while we are committing (and we do\nnot support other futzing like \"removing\" or \"reordering\"), either.\n\nI expect that somewhere in this series transaction->nr will not stay\nconstant even if the client code of ref-transaction API makes no\ndirect call that adds a new update[] element, though, even if it is\nnot done in this patch.\n\nThanks.\n"},{"id":"284706","messageId":"xmqqa8keaoof.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"5b73d10ec47a68535725d77ebbc5c4f0512ac5f3.1461768689.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 12/29] read_raw_ref(): improve docstring","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T18:31:44Z","receivedAt":"2016-04-27T18:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\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\nAll made sense.\n\nA welcome bonus is the removal of this stale comment that 42a38cf7\n(read_raw_ref(): manage own scratch space, 2016-04-07) forgot to\nremove.\n"},{"id":"284707","messageId":"xmqq60v2anyo.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"615204c877610855b02b21ce14efa5b7342182bc.1461768689.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T18:47:11Z","receivedAt":"2016-04-27T18:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING\n> without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING\n> caller to pass REF_NODEREF too.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> This also makes later patches a bit clearer.\n\nI wonder if it is more future-proof to solve this by doing\n\n -#define REF_ISPRUNING\t0x04\n +#define REF_ISPRUNING\t(0x04 | REF_NODEREF)\n\ninstead. It makes the intention clear that pruning is always about\nthe single level (i.e. no-deref).\n\n\n> refs.c | 3 +++\n> refs/files-backend.c | 2 +-\n> 2 files changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 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'\",\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 9faf17c..8fcbd7d 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2116,7 +2116,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);\n"},{"id":"284709","messageId":"xmqqy47y98zx.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"27f8b223e42dcf1cf3c010833e0aff7baa4559c2.1461768690.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T18:55:46Z","receivedAt":"2016-04-27T18:55:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> @@ -2380,8 +2381,8 @@ 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> +\tif (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&\n> +\t delete_ref(newrefname, NULL, REF_NODEREF)) {\n\nCould you explain s/sha1/NULL/ here in the proposed log message?\n\n> \t\tif (errno==EISDIR) {\n> \t\t\tstruct strbuf path = STRBUF_INIT;\n> \t\t\tint result;\n"},{"id":"284732","messageId":"1461787832.11504.1.camel@twopensource.com","threadId":"42157","inReplyTo":"xmqqinz39bl2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-27T20:10:32Z","receivedAt":"2016-04-27T20:10:32Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:\n\n> There is another call to refname_is_safe() in resolve_ref_unsafe(),\n> which applies the sanity check to the string from a symref. We seem\n> to allow\n> \n> $ git symbolic-ref refs/heads/SSS refs/heads//master\n> \n> and we end up storing \"ref: refs/heads//master\" (or make a symbolic\n> link with doubled slashes), but the current code considers the\n> resulting symbolic link as \"dangling\". Again, this change moves the\n> rejection a bit earlier in the codepath, without changing the end\n> result, I'd think.\n\nI think we should disallow that -- refname_is_safe should probably call\n(or be replaced with calls to) check_refname_format. \n"},{"id":"284734","messageId":"xmqqtwim95cm.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"a67a1b745d0a14111c774f13a5776d3756cbf2f2.1461768690.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T20:14:33Z","receivedAt":"2016-04-27T20:14:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> If the user has asked that a new value be set for a reference, we use\n> check_refname_format() to verify that the reference name satisfies all\n> of the rules. But in other cases, at least check that refname_is_safe().\n\nIt isn't clear to me what \"in other cases\" exactly refers to. A\nrequest to delete a ref would obviously one of those that do not\n\"ask that a new value be set\", but are there other cases?\n\n> Signed-off-by: Michael Haggerty \n> ---\n> There are remaining problems in this area of the code. For example,\n> check_refname_format() is *less* strict than refname_is_safe(). It\n> allows almost arbitrary top-level reference names like \"foo\" and\n> \"refs\". (The latter is especially fun because if you manage to create\n> a reference called \"refs\", Git stops recognizing the directory as a\n> Git repository.) On the other hand, some callers call\n> check_refname_format() with incomplete reference names (e.g., branch\n> names like \"master\"), so the functions can't be made stricter until\n> those callers are changed.\n>\n> I'll address these problems separately if I find the time.\n\nThanks.\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>\n> diff --git a/refs.c b/refs.c\n> index 858b6d7..41eb9e2 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction *transaction,\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> +\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;\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 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\" \\\n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> index 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"},{"id":"284735","messageId":"20160427201512.GA8073@sigill.intra.peff.net","threadId":"42157","inReplyTo":"1461787832.11504.1.camel@twopensource.com","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-27T20:15:13Z","receivedAt":"2016-04-27T20:15:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:\n\n> On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:\n> \n> > There is another call to refname_is_safe() in resolve_ref_unsafe(),\n> > which applies the sanity check to the string from a symref. We seem\n> > to allow\n> > \n> > $ git symbolic-ref refs/heads/SSS refs/heads//master\n> > \n> > and we end up storing \"ref: refs/heads//master\" (or make a symbolic\n> > link with doubled slashes), but the current code considers the\n> > resulting symbolic link as \"dangling\". Again, this change moves the\n> > rejection a bit earlier in the codepath, without changing the end\n> > result, I'd think.\n> \n> I think we should disallow that -- refname_is_safe should probably call\n> (or be replaced with calls to) check_refname_format. \n\nI thought the point is that one is a lesser check than the other, and we\nneed different rules for different situations. So we might allow\ndeletion on a refname that does not pass check_refname_format(), but we\nmust make sure it is not going to cause any mischief (e.g., escaping\n\".git\" and deleting random files).\n\nBut anything writing a _new_ refname (whether the actual ref, or\nreferencing it via a symref) should be using check_refname_format()\nbefore writing.\n\n-Peff\n"},{"id":"284736","messageId":"1461788637.11504.3.camel@twopensource.com","threadId":"42157","inReplyTo":"xmqq60v2anyo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-27T20:23:57Z","receivedAt":"2016-04-27T20:23:57Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING\n> > without REF_NODEREF. Forbid it explicitly. Change the one\n> > REF_ISPRUNING\n> > caller to pass REF_NODEREF too.\n> > \n> > Signed-off-by: Michael Haggerty \n> > ---\n> > This also makes later patches a bit clearer.\n> \n> I wonder if it is more future-proof to solve this by doing\n> \n> -#define REF_ISPRUNING\t0x04\n> +#define REF_ISPRUNING\t(0x04 | REF_NODEREF)\n> \n> instead. It makes the intention clear that pruning is always about\n> the single level (i.e. no-deref).\n\nI think the approach in Michael's patch might be clearer than yours,\nsince someone reading the code doesn't have to look at the definition\nof REF_ISPRUNING to understand what is going on.\n"},{"id":"284740","messageId":"1461789293.11504.5.camel@twopensource.com","threadId":"42157","inReplyTo":"20160427201512.GA8073@sigill.intra.peff.net","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-27T20:34:53Z","receivedAt":"2016-04-27T20:34:53Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 16:15 -0400, Jeff King wrote:\n> On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:\n> \n> > On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:\n> > \n> > > There is another call to refname_is_safe() in\n> > > resolve_ref_unsafe(),\n> > > which applies the sanity check to the string from a symref. We\n> > > seem\n> > > to allow\n> > > \n> > > $ git symbolic-ref refs/heads/SSS refs/heads//master\n> > > \n> > > and we end up storing \"ref: refs/heads//master\" (or make a\n> > > symbolic\n> > > link with doubled slashes), but the current code considers the\n> > > resulting symbolic link as \"dangling\". Again, this change moves\n> > > the\n> > > rejection a bit earlier in the codepath, without changing the end\n> > > result, I'd think.\n> > \n> > I think we should disallow that -- refname_is_safe should probably\n> > call\n> > (or be replaced with calls to) check_refname_format. \n> \n> I thought the point is that one is a lesser check than the other, and\n> we\n> need different rules for different situations. So we might allow\n> deletion on a refname that does not pass check_refname_format(), but\n> we\n> must make sure it is not going to cause any mischief (e.g., escaping\n> \".git\" and deleting random files).\n> \n> But anything writing a _new_ refname (whether the actual ref, or\n> referencing it via a symref) should be using check_refname_format()\n> before writing.\n\nUnfortunately, neither check is lesser -- refname_is_safe allows\nrefs/heads//foo but not a/b while check_refname_format allows a/b but\nnot refs/heads//foo. So sometimes we need both, while other times we\njust need one :(\n"},{"id":"284741","messageId":"20160427203728.GA8364@sigill.intra.peff.net","threadId":"42157","inReplyTo":"1461789293.11504.5.camel@twopensource.com","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-27T20:37:29Z","receivedAt":"2016-04-27T20:37:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:\n\n> > I thought the point is that one is a lesser check than the other, and\n> > we\n> > need different rules for different situations. So we might allow\n> > deletion on a refname that does not pass check_refname_format(), but\n> > we\n> > must make sure it is not going to cause any mischief (e.g., escaping\n> > \".git\" and deleting random files).\n> > \n> > But anything writing a _new_ refname (whether the actual ref, or\n> > referencing it via a symref) should be using check_refname_format()\n> > before writing.\n> \n> Unfortunately, neither check is lesser -- refname_is_safe allows\n> refs/heads//foo but not a/b while check_refname_format allows a/b but\n> not refs/heads//foo. So sometimes we need both, while other times we\n> just need one :(\n\nIMHO, that sounds like a bug. check_refname_format() should\nconceptually[1] be a superset of refname_is_safe(). Is there a case\nwhere we would want to _allow_ a refname that is not safe to look at on\ndisk?\n\n-Peff\n\n[1] The implementation can be a direct call, or can simply implement a\n superset of the rules, if that's more efficient.\n"},{"id":"284742","messageId":"xmqqh9em93xo.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"1461788637.11504.3.camel@twopensource.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T20:45:07Z","receivedAt":"2016-04-27T20:45:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING\n>> > without REF_NODEREF. Forbid it explicitly. Change the one\n>> > REF_ISPRUNING\n>> > caller to pass REF_NODEREF too.\n>> > \n>> > Signed-off-by: Michael Haggerty \n>> > ---\n>> > This also makes later patches a bit clearer.\n>> \n>> I wonder if it is more future-proof to solve this by doing\n>> \n>> -#define REF_ISPRUNING\t0x04\n>> +#define REF_ISPRUNING\t(0x04 | REF_NODEREF)\n>> \n>> instead. It makes the intention clear that pruning is always about\n>> the single level (i.e. no-deref).\n>\n> I think the approach in Michael's patch might be clearer than yours,\n> since someone reading the code doesn't have to look at the definition\n> of REF_ISPRUNING to understand what is going on.\n\nI have to strongly disagree, assuming that my understanding of the\npoint of this patch correctly.\n\nIf a casual reader sees this code:\n\n ref_transaction_delete(transaction, r->name, r->sha1,\n\t\t\t REF_ISPRUNING | REF_NODEREF, NULL, &err)\n\nit gives an incorrect impression that there may also be a valid case\nto make a \"delete\" call with ISPRUNING alone without NODEREF, in\nother codepaths and under certain conditions, and write an incorrect\n\n ref_transaction_delete(transaction, refname, sha1,\n\t\t\t REF_ISPRUNING, NULL, &err)\n\nin her new code. Or a careless programmer and reviewer may not even\nmemorize and remember what the new world order is when they see such\na code and let it pass.\n\nAs I understand that we declare that \"to prune a ref from set of\nloose refs is to prune the named one, never following a symbolic\nref\" is the new world order with this patch, making sure that\nISPRUNING automatically and always mean NODEREF will eliminate the\npossibility that any new code makes an incorrect call to \"delete\",\nwhich I think is much better.\n"},{"id":"284744","messageId":"xmqqd1pa93we.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"xmqqeg9qape3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T20:45:53Z","receivedAt":"2016-04-27T20:45:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I expect that somewhere in this series transaction->nr will not stay\n\ns/series/& it is documented that/\n\n> constant even if the client code of ref-transaction API makes no\n> direct call that adds a new update[] element, though, even if it is\n> not done in this patch.\n>\n> Thanks.\n"},{"id":"284749","messageId":"xmqqvb327nyz.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"xmqqh9em93xo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T21:15:16Z","receivedAt":"2016-04-27T21:15:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If a casual reader sees this code:\n>\n> ref_transaction_delete(transaction, r->name, r->sha1,\n> \t\t\t REF_ISPRUNING | REF_NODEREF, NULL, &err)\n>\n> it gives an incorrect impression that there may also be a valid case\n> to make a \"delete\" call with ISPRUNING alone without NODEREF, in\n> other codepaths and under certain conditions, and write an incorrect\n>\n> ref_transaction_delete(transaction, refname, sha1,\n> \t\t\t REF_ISPRUNING, NULL, &err)\n>\n> in her new code. Or a careless programmer and reviewer may not even\n> memorize and remember what the new world order is when they see such\n> a code and let it pass.\n>\n> As I understand that we declare that \"to prune a ref from set of\n> loose refs is to prune the named one, never following a symbolic\n> ref\" is the new world order with this patch, making sure that\n> ISPRUNING automatically and always mean NODEREF will eliminate the\n> possibility that any new code makes an incorrect call to \"delete\",\n> which I think is much better.\n\n... but my understanding of the point of this patch may be flawed,\nin which case I of course am willing to be enlightened ;-)\n"},{"id":"284756","messageId":"20160427221906.GA9707@sigill.intra.peff.net","threadId":"42157","inReplyTo":"20160427203728.GA8364@sigill.intra.peff.net","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-27T22:19:06Z","receivedAt":"2016-04-27T22:19:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 27, 2016 at 04:37:28PM -0400, Jeff King wrote:\n\n> > > But anything writing a _new_ refname (whether the actual ref, or\n> > > referencing it via a symref) should be using check_refname_format()\n> > > before writing.\n> > \n> > Unfortunately, neither check is lesser -- refname_is_safe allows\n> > refs/heads//foo but not a/b while check_refname_format allows a/b but\n> > not refs/heads//foo. So sometimes we need both, while other times we\n> > just need one :(\n> \n> IMHO, that sounds like a bug. check_refname_format() should\n> conceptually[1] be a superset of refname_is_safe(). Is there a case\n> where we would want to _allow_ a refname that is not safe to look at on\n> disk?\n\nBTW, if there isn't a superset relationship here, then I suspect I may\nhave introduced some loosening or inconsistencies in 03afcbe\n(read_packed_refs: avoid double-checking sane refs, 2015-04-16). So any\ntightening now may simply be fixing that (which doesn't change anything\nwith respect to correctness, but may give people more confidence that\nthe tightening is not breaking something people have been depending on).\n\n-Peff\n\nPS Reading over that commit message, I think it mis-states\n \"refname_is_safe() can only be true if check_refname_format() also\n failed\". It should be \"!refname_is_safe() ...\". I.e., the condition can\n only trigger if...\n"},{"id":"284850","messageId":"1461865449.4123.2.camel@twopensource.com","threadId":"42157","inReplyTo":"20160427203728.GA8364@sigill.intra.peff.net","subject":"Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-28T17:44:09Z","receivedAt":"2016-04-28T17:44:09Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 16:37 -0400, Jeff King wrote:\n> On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote:\n> \n> > > I thought the point is that one is a lesser check than the other,\n> > > and\n> > > we\n> > > need different rules for different situations. So we might allow\n> > > deletion on a refname that does not pass check_refname_format(),\n> > > but\n> > > we\n> > > must make sure it is not going to cause any mischief (e.g.,\n> > > escaping\n> > > \".git\" and deleting random files).\n> > > \n> > > But anything writing a _new_ refname (whether the actual ref, or\n> > > referencing it via a symref) should be using\n> > > check_refname_format()\n> > > before writing.\n> > \n> > Unfortunately, neither check is lesser -- refname_is_safe allows\n> > refs/heads//foo but not a/b while check_refname_format allows a/b\n> > but\n> > not refs/heads//foo. So sometimes we need both, while other times\n> > we\n> > just need one :(\n> \n> IMHO, that sounds like a bug. check_refname_format() should\n> conceptually[1] be a superset of refname_is_safe(). Is there a case\n> where we would want to _allow_ a refname that is not safe to look at\n> on\n> disk?\n\nThe only such case I can think of is the case where there is a symref\nto such a bad refname, and we want to delete said symref.\n"},{"id":"284853","messageId":"1461865718.4123.4.camel@twopensource.com","threadId":"42157","inReplyTo":"xmqqvb327nyz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-28T17:48:38Z","receivedAt":"2016-04-28T17:48:38Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 14:15 -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > If a casual reader sees this code:\n> > \n> > ref_transaction_delete(transaction, r->name, r->sha1,\n> > \t\t\t REF_ISPRUNING | REF_NODEREF, NULL, &err)\n> > \n> > it gives an incorrect impression that there may also be a valid\n> > case\n> > to make a \"delete\" call with ISPRUNING alone without NODEREF, in\n> > other codepaths and under certain conditions, and write an\n> > incorrect\n> > \n> > ref_transaction_delete(transaction, refname, sha1,\n> > \t\t\t REF_ISPRUNING, NULL, &err)\n> > \n> > in her new code. Or a careless programmer and reviewer may not\n> > even\n> > memorize and remember what the new world order is when they see\n> > such\n> > a code and let it pass.\n> > \n> > As I understand that we declare that \"to prune a ref from set of\n> > loose refs is to prune the named one, never following a symbolic\n> > ref\" is the new world order with this patch, making sure that\n> > ISPRUNING automatically and always mean NODEREF will eliminate the\n> > possibility that any new code makes an incorrect call to \"delete\",\n> > which I think is much better.\n> \n> ... but my understanding of the point of this patch may be flawed,\n> in which case I of course am willing to be enlightened ;-)\n\nSince there is a manual check for that case, the code will fail at test\ntime.\n\nBut I don't have strong feelings and am happy to go either way on this.\n"},{"id":"284871","messageId":"xmqq60v14hhx.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"1461865718.4123.4.camel@twopensource.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-28T20:15:38Z","receivedAt":"2016-04-28T20:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> Since there is a manual check for that case, the code will fail at test\n> time.\n\nI see a difference between \"We make it hard to write incorrect code\"\nand \"We keep it easy to write incorrect code but a runtime check\nwill catch mistakes anyway\", and I tend to prefer the former.\n\nI do think the \"manual check\" needs to be kept in an adjusted form\neven if we decide to take the suggested update. A caller can pass\n0x04 instead of REF_ISPRUNING after all---but that is exactly the\ncase where you have to work hard to write incorrect code.\n\nOne existing check that is not touched with 15/29 also needs to be\nadjusted. An incremental update relative to 15/29 would look like\nthis:\n\n refs.c | 4 ++--\n refs/files-backend.c | 4 ++--\n refs/refs-internal.h | 6 ++++--\n 3 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5dc2473..bb81dfd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -790,8 +790,8 @@ 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+\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)) {\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8fcbd7d..012e3d0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2116,7 +2116,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 | REF_NODEREF, NULL, &err) ||\n+\t\t\t\t REF_ISPRUNING, NULL, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n@@ -3178,7 +3178,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \n-\t\t\tif (!(update->flags & REF_ISPRUNING))\n+\t\t\tif (!(update->flags & REF_ISPRUNING_))\n \t\t\t\tstring_list_append(&refs_to_delete,\n \t\t\t\t\t\t update->lock->ref_name);\n \t\t}\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 37a1a37..0a2bf1f 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -15,9 +15,11 @@\n \n /*\n * Used as a flag in ref_update::flags when a loose ref is being\n- * pruned.\n+ * pruned. As this must always be done with REF_NODEREF, we make\n+ * the public version REF_ISPRUNING to contain REF_NODEREF.\n */\n-#define REF_ISPRUNING\t0x04\n+#define REF_ISPRUNING_\t0x04\n+#define REF_ISPRUNING\t(REF_ISPRUNING_ | REF_NODEREF)\n \n /*\n * Used as a flag in ref_update::flags when the reference should be\n"},{"id":"284883","messageId":"1461886803.4123.14.camel@twopensource.com","threadId":"42157","inReplyTo":"a33272db057ac6327cc0c87c002473c89ffcca6b.1461768690.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 25/29] refs: resolve symbolic refs first","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-28T23:40:03Z","receivedAt":"2016-04-28T23:40:03Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:\n+retry:\n...\n> +\t\tif (--attempts_remaining > 0)\n> +\t\t\tgoto retry;\n\ncould this be a loop instead of using gotos?\n\n> +\t\t\t/*\n> +\t\t\t * There is a directory in the way,\n> +\t\t\t * but we\t don't know of any references\n> +\t\t\t * that it should contain. This might\n> +\t\t\t * be a directory that used to contain\n> +\t\t\t * references but is now empty. Try to\n> +\t\t\t * remove it; otherwise it might cause\n> +\t\t\t * trouble when we try to rename the\n> +\t\t\t * lockfile into place.\n> +\t\t\t */\n> +\t\t\tstrbuf_addf(err, \"there is a non-empty directory '%s' \"\n> +\t\t\t\t \"blocking reference '%s'\",\n> +\t\t\t\t\t ref_file.buf,refname);\n> +\t\t\tgoto error_return;\n\nWe don't actually try to remove anything here, so that comment seems\nwrong?\n"},{"id":"284887","messageId":"1461892474.4123.27.camel@twopensource.com","threadId":"42157","inReplyTo":"cover.1461768689.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/29] Yet more preparation for reference backends","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-29T01:14:34Z","receivedAt":"2016-04-29T01:14:34Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:\n> This started as a modest attempt to move the last couple of patches\n> mentioned here [1] to before the vtable patches. I wanted to do that\n> because having real changes mixed with the vtable refactoring was\n> making rebasing and stuff awkward.\n> \n> But then it snowballed. A lot of what's new is pretty trivial (though\n> in some cases fixes minor bugs). But a few of the later patches are\n> pretty major.\n\nApart from the comments on one patch that I sent, this looks good to\nme. I'm a bit worried about the impact on lmdb (which doesn't do\n\"locking\" the same way), but I think it will probably work out. Since\nMichael has been rebasing the pluggable-refs-backend code on top of\nthis, he's probably got a good sense that it doesn't do too much\nviolence to the plugin infrastructure.\n\n\n\n> \n"},{"id":"284894","messageId":"5723059C.5080406@alum.mit.edu","threadId":"42157","inReplyTo":"xmqqvb327nyz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T06:56:28Z","receivedAt":"2016-04-29T06:56:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/27/2016 11:15 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> If a casual reader sees this code:\n>>\n>> ref_transaction_delete(transaction, r->name, r->sha1,\n>> \t\t\t REF_ISPRUNING | REF_NODEREF, NULL, &err)\n>>\n>> it gives an incorrect impression that there may also be a valid case\n>> to make a \"delete\" call with ISPRUNING alone without NODEREF, in\n>> other codepaths and under certain conditions, and write an incorrect\n>>\n>> ref_transaction_delete(transaction, refname, sha1,\n>> \t\t\t REF_ISPRUNING, NULL, &err)\n>>\n>> in her new code. Or a careless programmer and reviewer may not even\n>> memorize and remember what the new world order is when they see such\n>> a code and let it pass.\n>>\n>> As I understand that we declare that \"to prune a ref from set of\n>> loose refs is to prune the named one, never following a symbolic\n>> ref\" is the new world order with this patch, making sure that\n>> ISPRUNING automatically and always mean NODEREF will eliminate the\n>> possibility that any new code makes an incorrect call to \"delete\",\n>> which I think is much better.\n> \n> ... but my understanding of the point of this patch may be flawed,\n> in which case I of course am willing to be enlightened ;-)\n\nI was thinking of this patch as documenting and enforcing a limitation\nin the current implementation of pruning. But to be honest I can't think\nof a reason that we would ever want to remove this limitation, so I am\nOK with changing the policy to \"REF_ISPRUNING always implies\nREF_NODEREF\" as you have suggested.\n\nBut I think it would be cleaner to achieve that goal with the following\nchange:\n\ndiff --git a/refs.c b/refs.c\nindex 5dc2473..1d4c12a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction\n*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+\tif (flags & REF_ISPRUNING) {\n+\t\t/* Pruning is always non-recursive */\n+\t\tflags |= REF_NODEREF;\n+\t}\n\n \tif (new_sha1 && !is_null_sha1(new_sha1) &&\n \t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8fcbd7d..9faf17c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2116,7 +2116,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 | REF_NODEREF, NULL, &err) ||\n+\t\t\t\t REF_ISPRUNING, 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 37a1a37..704eea7 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 implies REF_NODEREF.\n */\n #define REF_ISPRUNING\t0x04\n\n\nNote that patch \"add_update(): initialize the whole ref_update\" should\nthen be adjusted to do the flag-tweak in the add_update() function.\n\nIf there are no objections, I will implement these changes in v2.\n\nMichael\n"},{"id":"284898","messageId":"57230F71.2020401@alum.mit.edu","threadId":"42157","inReplyTo":"xmqqy47y98zx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T07:38:25Z","receivedAt":"2016-04-29T07:38:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/27/2016 08:55 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> @@ -2380,8 +2381,8 @@ 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>> +\tif (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&\n>> +\t delete_ref(newrefname, NULL, REF_NODEREF)) {\n> \n> Could you explain s/sha1/NULL/ here in the proposed log message?\n\nGood question.\n\nPassing sha1 to delete_ref() doesn't add any safety, because the same\nsha1 was just read a moment before, and it is not used for anything\nelse. So the check only protects us from a concurrent update to\nnewrefname between the call to read_ref_full() and the call to\ndelete_ref(). But such a race is indistinguishable from the case that a\nmodification to newrefname happens just before the call to\nread_ref_full(), which would have the same outcome as the new code. So\nthe \"sha1\" check only adds ways for the rename() to fail in situations\nwhere nothing harmful would have happened anyway.\n\nThat being said, this is a very unlikely race, and I don't think it\nmatters much either way. In any case, the change s/sha1/NULL/ here seems\northogonal to the rest of the patch.\n\nDavid, you wrote the original version of this patch. Am I overlooking\nsomething?\n\nMichael\n"},{"id":"284900","messageId":"57231082.50806@alum.mit.edu","threadId":"42157","inReplyTo":"xmqqtwim95cm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T07:42:58Z","receivedAt":"2016-04-29T07:42:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/27/2016 10:14 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> If the user has asked that a new value be set for a reference, we use\n>> check_refname_format() to verify that the reference name satisfies all\n>> of the rules. But in other cases, at least check that refname_is_safe().\n> \n> It isn't clear to me what \"in other cases\" exactly refers to. A\n> request to delete a ref would obviously one of those that do not\n> \"ask that a new value be set\", but are there other cases?\n\nThe other case is `verify`, which can be used to check the old value of\na reference without modifying it. `verify` is exposed via `git\nupdate-ref --stdin`\n\nMichael\n"},{"id":"284901","messageId":"xmqq4mak3k08.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"5723059C.5080406@alum.mit.edu","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T08:19:03Z","receivedAt":"2016-04-29T08:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> But I think it would be cleaner to achieve that goal with the following\n> change:\n>\n> diff --git a/refs.c b/refs.c\n> index 5dc2473..1d4c12a 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -790,8 +790,10 @@ int ref_transaction_update(struct ref_transaction\n> *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> +\tif (flags & REF_ISPRUNING) {\n> +\t\t/* Pruning is always non-recursive */\n> +\t\tflags |= REF_NODEREF;\n> +\t}\n>\n> \tif (new_sha1 && !is_null_sha1(new_sha1) &&\n> \t check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 8fcbd7d..9faf17c 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2116,7 +2116,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 | REF_NODEREF, NULL, &err) ||\n> +\t\t\t\t REF_ISPRUNING, NULL, &err) ||\n> \t ref_transaction_commit(transaction, &err)) {\n> \t\tref_transaction_free(transaction);\n> \t\terror(\"%s\", err.buf);\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index 37a1a37..704eea7 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 implies REF_NODEREF.\n> */\n> #define REF_ISPRUNING\t0x04\n>\n>\n> Note that patch \"add_update(): initialize the whole ref_update\" should\n> then be adjusted to do the flag-tweak in the add_update() function.\n> ...\n> If there are no objections, I will implement these changes in v2.\n\nOne worrysome point the above approach leaves is that nothing\nguarantees that nobody in the codepath from the callsite of\nref-transaction-delete to ref-transaction-update looks at the flag\nand acts differently depending on REF_NODEREF bit. During that\ntime, REF_ISPRUNING call would not trigger (flag & REF_NODEREF)\ncheck, because \"pruning implies no-deref\" is done only after you\nreach transaction-update (i.e. the code you added in the first\nhunk), but any code after that \"implie no-deref\" happens will see\nREF_NODEREF bit in the flag word.\n\nAs long as you can add some mechanism to make that a non-issue, I am\nfine with that approach.\n\nThanks.\n"},{"id":"284902","messageId":"xmqqzisc24en.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"5723059C.5080406@alum.mit.edu","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T08:41:20Z","receivedAt":"2016-04-29T08:41:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Two things I forgot to say.\n\nMichael Haggerty writes:\n\n> I was thinking of this patch as documenting and enforcing a limitation\n> in the current implementation of pruning.\n\nThis actually is an excellent point, and is the reason why I\nrepeatedly made my suggestion conditional on \"If my understanding of\nthe purpose of this patch correctly\" ;-) And I personally am not\nconvinced that this limitation is fundamental and will not be\nlifted. It may be better to treat this as a limitation of the\ncurrent implementation of pruning, and in that case I do agree that\nthe original patch that started this thread is the best way to\nexpress it.\n\nI said that it makes harder to write incorrect client code of this\nAPI to make REF_ISPRUNING contain both 0x04 and REF_NODEREF, and I\nstill think that is true, but at the same time, it makes it easier\nto write incorrect code in the API implementation. We no longer can\ncheck if we are in the pruning codepath with\n\n if (flag & REF_ISPRUNING)\n\nbut have to write\n\n if ((flag & REF_ISPRUNING) == REF_ISPRUNING)\n\ninstead. This \"& with the constant and compare the result with that\nsame constant\" pattern could also be used for a single-bit constant,\nso if there were short-and-sweet syntax to express that pattern in\nthe language, consistently using that for all bit checks would make\nit less likely for us to write incorrect code, but but there is no\nshort-and-sweet syntax to do so in C, and spelling it out like the\nabove is too cumbersome to be practical. The suggested squash did\n\n if (flag & REF_ISPRUNING_)\n\nto check only for 0x04 bit, but that is also error prone; it is too\neasy to forget the difference between the two and drop the trailing\nunderscore by mistake.\n\nEven though it is generally a good trade-off to make it harder to\nmake mistakes in the \"user of the API\" code even if it makes it\neasier to make mistakes in the \"implementation of the API\" code,\nbecause the \"user of the API\" in this case is actually still inside\nthe ref API implementation, I do not think either way makes too much\nof a difference. So perhaps your original might be the best version\namong those that have been discussed in this thread.\n"},{"id":"284903","messageId":"xmqqvb3023v0.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"57230F71.2020401@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T08:53:07Z","receivedAt":"2016-04-29T08:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> Could you explain s/sha1/NULL/ here in the proposed log message?\n>\n> Good question.\n>\n> Passing sha1 to delete_ref() doesn't add any safety, because the same\n> sha1 was just read a moment before, and it is not used for anything\n> else.\n\n\"... and it is guaranteed that no other process in the meantime\nwanted to update the ref we are trying to delete because it wants to\nsee the ref with its updated value.\" is something I expected to see\nat the end.\n\n> So the check only protects us from a concurrent update to\n> newrefname between the call to read_ref_full() and the call to\n> delete_ref(). But such a race is indistinguishable from the case that a\n> modification to newrefname happens just before the call to\n> read_ref_full(), which would have the same outcome as the new code.\n\nIn other words, when a transaction that contains a deletion of a ref\nraces with another one that updates the ref, the latter transaction\nmay come after the former one, but the ref may not survive in the\nend result and can be left deleted?\n\nPuzzled...\n"},{"id":"284904","messageId":"xmqqr3do23u5.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"57231082.50806@alum.mit.edu","subject":"Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T08:53:38Z","receivedAt":"2016-04-29T08:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 04/27/2016 10:14 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>>> If the user has asked that a new value be set for a reference, we use\n>>> check_refname_format() to verify that the reference name satisfies all\n>>> of the rules. But in other cases, at least check that refname_is_safe().\n>> \n>> It isn't clear to me what \"in other cases\" exactly refers to. A\n>> request to delete a ref would obviously one of those that do not\n>> \"ask that a new value be set\", but are there other cases?\n>\n> The other case is `verify`, which can be used to check the old value of\n> a reference without modifying it. `verify` is exposed via `git\n> update-ref --stdin`\n\nThanks.\n"},{"id":"284906","messageId":"57232EAB.3080406@alum.mit.edu","threadId":"42157","inReplyTo":"1461886803.4123.14.camel@twopensource.com","subject":"Re: [PATCH 25/29] refs: resolve symbolic refs first","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T09:51:39Z","receivedAt":"2016-04-29T09:51:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2016 01:40 AM, David Turner wrote:\n> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:\n> +retry:\n> ...\n>> +\t\tif (--attempts_remaining > 0)\n>> +\t\t\tgoto retry;\n> \n> could this be a loop instead of using gotos?\n\nIt certainly could. The goto-vs-loop question was debated on the mailing\nlist when I first added very similar code elsewhere (unfortunately I'm\nunable to find a link to that conversation). I was persuaded to change\nmy loop into gotos, the argument being that the \"retry\" case is\nexceptional and shouldn't be such a dominant part of the function\nstructure. Plus the goto code is briefer and feels less awkward to me in\nthis case (that's subjective, of course).\n\n>> +\t\t\t/*\n>> +\t\t\t * There is a directory in the way,\n>> +\t\t\t * but we\t don't know of any references\n>> +\t\t\t * that it should contain. This might\n>> +\t\t\t * be a directory that used to contain\n>> +\t\t\t * references but is now empty. Try to\n>> +\t\t\t * remove it; otherwise it might cause\n>> +\t\t\t * trouble when we try to rename the\n>> +\t\t\t * lockfile into place.\n>> +\t\t\t */\n>> +\t\t\tstrbuf_addf(err, \"there is a non-empty directory '%s' \"\n>> +\t\t\t\t \"blocking reference '%s'\",\n>> +\t\t\t\t\t ref_file.buf,refname);\n>> +\t\t\tgoto error_return;\n> \n> We don't actually try to remove anything here, so that comment seems\n> wrong?\n\nThanks, will fix.\n\nMichael\n"},{"id":"284907","messageId":"57233E19.4000200@alum.mit.edu","threadId":"42157","inReplyTo":"xmqqvb3023v0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T10:57:29Z","receivedAt":"2016-04-29T10:57:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2016 10:53 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>>> Could you explain s/sha1/NULL/ here in the proposed log message?\n>>\n>> Good question.\n>>\n>> Passing sha1 to delete_ref() doesn't add any safety, because the same\n>> sha1 was just read a moment before, and it is not used for anything\n>> else.\n> \n> \"... and it is guaranteed that no other process in the meantime\n> wanted to update the ref we are trying to delete because it wants to\n> see the ref with its updated value.\" is something I expected to see\n> at the end.\n> \n>> So the check only protects us from a concurrent update to\n>> newrefname between the call to read_ref_full() and the call to\n>> delete_ref(). But such a race is indistinguishable from the case that a\n>> modification to newrefname happens just before the call to\n>> read_ref_full(), which would have the same outcome as the new code.\n> \n> In other words, when a transaction that contains a deletion of a ref\n> races with another one that updates the ref, the latter transaction\n> may come after the former one, but the ref may not survive in the\n> end result and can be left deleted?\n> \n> Puzzled...\n\nRemember, we're talking about rename_ref() only, not reference deletion\nin general. rename_ref() is not very robust anyway--it doesn't happen in\na single transaction, and it is vulnerable to being defeated by\nsimultaneous reference updates by other processes. It *does* wrap the\ndeletion of newrefname in a transaction; the only question is whether an\nold_sha1 is supplied to that transaction.\n\nSo, suppose that newrefname starts at value A, and there are two updates\nrunning simultaneously:\n\n1. An update of reference newrefname from A -> B\n\n2. A rename of reference oldrefname to newrefname, which includes\n a. read_ref_full(\"newrefname\") and\n b. delete_ref(\"newrefname\").\n\nIt is not possible for (1) to happen after (2b) because the former's\ncheck of the old value of newrefname would fail. So there are two\npossible interleavings:\n\n* 1, 2a, 2b\n* 2a, 1, 2b\n\nWith the new code, both of these interleavings end up with newrefname\ndeleted.\n\nWith the old code, the second interleaving would fail.\n\nBut the only difference is the relative order of the read-only operation\n(2a), whose SHA-1 result is never used. So neither process actually\ncares which of those two interleavings occurred, and it is legitimate to\ntreat them the same.\n\nNote that the first transaction *did* successfully set newrefname to\nvalue B in both cases and indeed knows for sure that the update was\nsuccessful. It's just that newrefname was deleted immediately afterwards.\n\nMichael\n"},{"id":"284908","messageId":"20160429121228.GA27952@sigill.intra.peff.net","threadId":"42157","inReplyTo":"57233E19.4000200@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-29T12:12:28Z","receivedAt":"2016-04-29T12:12:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:\n\n> Remember, we're talking about rename_ref() only, not reference deletion\n> in general. rename_ref() is not very robust anyway--it doesn't happen in\n> a single transaction, and it is vulnerable to being defeated by\n> simultaneous reference updates by other processes. It *does* wrap the\n> deletion of newrefname in a transaction; the only question is whether an\n> old_sha1 is supplied to that transaction.\n> \n> So, suppose that newrefname starts at value A, and there are two updates\n> running simultaneously:\n> \n> 1. An update of reference newrefname from A -> B\n> \n> 2. A rename of reference oldrefname to newrefname, which includes\n> a. read_ref_full(\"newrefname\") and\n> b. delete_ref(\"newrefname\").\n\nI wondered at first if you meant \"oldrefname\" in 2b. That is, a rename\nwould involve writing to \"newrefname\" and then deleting \"oldrefname\",\nand not doing the old_sha1 and normal locking on the deletion of\noldrefname would be bad (in case somebody else updated it while we were\noperating).\n\nBut reading the patch again, that's not what's going on. You're talking\njust about the case where we force-overwrite an existing newrefname, and\ndelete it first to do so. But what I don't understand is:\n\n 1. Why do we read_ref_full() at all? Since it is not done under lock\n anyway, it is useless for helping with race conditions, and I think\n that is what you are arguing (that we should just be deleting\n regardless). But then why not just call delete_ref(), and handle\n the ENOENT case as a noop (which closes another race if somebody\n deletes it between your 2a and 2b).\n\n 2. Why delete it at all? The point is to overwrite, so I guess it is\n trying to make room. But we could just rename the loose ref file\n and reflog overtop the old, couldn't we?\n\nOr am I totally misreading the purpose of this code?\n\n-Peff\n"},{"id":"284920","messageId":"572367B4.4050207@alum.mit.edu","threadId":"42157","inReplyTo":"20160429121228.GA27952@sigill.intra.peff.net","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T13:55:00Z","receivedAt":"2016-04-29T13:55:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2016 02:12 PM, Jeff King wrote:\n> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:\n> \n>> Remember, we're talking about rename_ref() only, not reference deletion\n>> in general. rename_ref() is not very robust anyway--it doesn't happen in\n>> a single transaction, and it is vulnerable to being defeated by\n>> simultaneous reference updates by other processes. It *does* wrap the\n>> deletion of newrefname in a transaction; the only question is whether an\n>> old_sha1 is supplied to that transaction.\n>>\n>> So, suppose that newrefname starts at value A, and there are two updates\n>> running simultaneously:\n>>\n>> 1. An update of reference newrefname from A -> B\n>>\n>> 2. A rename of reference oldrefname to newrefname, which includes\n>> a. read_ref_full(\"newrefname\") and\n>> b. delete_ref(\"newrefname\").\n> \n> I wondered at first if you meant \"oldrefname\" in 2b. That is, a rename\n> would involve writing to \"newrefname\" and then deleting \"oldrefname\",\n> and not doing the old_sha1 and normal locking on the deletion of\n> oldrefname would be bad (in case somebody else updated it while we were\n> operating).\n> \n> But reading the patch again, that's not what's going on. You're talking\n> just about the case where we force-overwrite an existing newrefname, and\n> delete it first to do so. But what I don't understand is:\n\nIt's beyond the ambition of this patch to fix this old rename_ref()\ncode, but...\n\n> 1. Why do we read_ref_full() at all? Since it is not done under lock\n> anyway, it is useless for helping with race conditions, and I think\n> that is what you are arguing (that we should just be deleting\n> regardless). But then why not just call delete_ref(), and handle\n> the ENOENT case as a noop (which closes another race if somebody\n> deletes it between your 2a and 2b).\n\nI thought about that too, and agree it would be an improvement. But it's\nnot quite so trivial. The ENOENT doesn't make it out of delete_ref()\n(which does a full-fledged ref_transaction internally). The error is\nonly reported via \"strbuf *err\".\n\n> 2. Why delete it at all? The point is to overwrite, so I guess it is\n> trying to make room. But we could just rename the loose ref file\n> and reflog overtop the old, couldn't we?\n>\n> Or am I totally misreading the purpose of this code?\n\nI wouldn't want to just rename the files, because (1) I think it is\nbetter to use the existing ref_transaction code, and (2) that wouldn't\nwork if there is a D/F conflict between the old and new reference names,\nwhich the existing code handles (though honestly I'm skeptical that it\ncomes up a lot).\n\nIt would be more plausible to use ref_transaction_update() to write\noldrefname's *value* on top of newrefname without actually moving the\nfile. oldrefname could even be deleted in the same transaction, if you\nwere willing to stop supporting old/new refnames that D/F conflict with\neach other. But we also want to move the reflog, and that should be done\nwhile the newrefname lock and (contrary to the current code) also the\noldrefname lock are held. There's currently no way to slip an arbitrary\naction like that into the middle of a transaction.\n\nIf I had lots of time and interest to work on this, I think the best\napproach would be to add a ref_transaction_rename(), which takes an old\nand a new reference name as arguments, and adds a whole rename operation\n(including of the reflog) to a transaction. This could probably be\nimplemented by adding one ref_update() and one ref_delete(), and using\nthe parent_update pointer that is introduced later to link the two so\nthat ref_transaction_commit() knows to move the reflog too.\n\nIf you were willing to punt on D/F conflicts, you would be done. If not,\nthen it would be better to teach ref_transaction_commit() to deal with\nD/F conflicts in the general case rather than using special-purpose code\nin rename_ref().\n\nThen rename_ref() could be omitted from the vtable entirely.\n\nMichael\n"},{"id":"284921","messageId":"20160429140817.GA26643@sigill.intra.peff.net","threadId":"42157","inReplyTo":"572367B4.4050207@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-29T14:08:17Z","receivedAt":"2016-04-29T14:08:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 29, 2016 at 03:55:00PM +0200, Michael Haggerty wrote:\n\n> It's beyond the ambition of this patch to fix this old rename_ref()\n> code, but...\n> [...]\n\nThanks for the explanation. That all makes sense to me, and I can\ndefinitely live with \"historical warts that aren't worth touching in\nthis series\" as the verdict.\n\n-Peff\n"},{"id":"284927","messageId":"57236FAF.7050201@alum.mit.edu","threadId":"42157","inReplyTo":"xmqqzisc24en.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-29T14:29:03Z","receivedAt":"2016-04-29T14:29:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2016 10:41 AM, Junio C Hamano wrote:\n> [...Long, thoughtful comments omitted...]\n> So perhaps your original might be the best version\n> among those that have been discussed in this thread.\n\nVery well. I'll also add a comment near the definition of REF_ISPRUNING\nthat it must only be used together with REF_NODEREF.\n\nMichael\n"},{"id":"284940","messageId":"xmqqmvoc1lix.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"572367B4.4050207@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T15:29:10Z","receivedAt":"2016-04-29T15:29:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 04/29/2016 02:12 PM, Jeff King wrote:\n>> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:\n>> \n>>> Remember, we're talking about rename_ref() only, not reference deletion\n>>> in general. rename_ref() is not very robust anyway--it doesn't happen in\n>>> a single transaction, and it is vulnerable to being defeated by\n>>> simultaneous reference updates by other processes. It *does* wrap the\n>>> deletion of newrefname in a transaction; the only question is whether an\n>>> old_sha1 is supplied to that transaction.\n>>>\n>>> So, suppose that newrefname starts at value A, and there are two updates\n>>> running simultaneously:\n>>>\n>>> 1. An update of reference newrefname from A -> B\n>>>\n>>> 2. A rename of reference oldrefname to newrefname, which includes\n>>> a. read_ref_full(\"newrefname\") and\n>>> b. delete_ref(\"newrefname\").\n>> \n>> I wondered at first if you meant \"oldrefname\" in 2b. That is, a rename\n>> would involve writing to \"newrefname\" and then deleting \"oldrefname\",\n>> and not doing the old_sha1 and normal locking on the deletion of\n>> oldrefname would be bad (in case somebody else updated it while we were\n>> operating).\n>> \n>> But reading the patch again, that's not what's going on. You're talking\n>> just about the case where we force-overwrite an existing newrefname, and\n>> delete it first to do so. But what I don't understand is:\n>\n> It's beyond the ambition of this patch to fix this old rename_ref()\n> code, but...\n\nThanks for a clear explanation. The NULL might want to be explained\nin in-code comment (e.g. \"here we do not care about verifying old\nvalue because...\"), then.\n"},{"id":"284941","messageId":"xmqqinz01kul.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"a3d853510719a37c1e4bbb52261169c25b19c148.1461768690.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-29T15:43:46Z","receivedAt":"2016-04-29T15:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we\n> don't have to handle other cases anymore.\n>\n> This enables several simplifications, the most interesting of which come\n> from the fact that ref_lock::orig_ref_name is now always the same as\n> ref_lock::ref_name:\n>\n> * Remove ref_lock::orig_ref_name\n> * Remove local variable orig_refname from lock_ref_sha1_basic()\n> * commit_ref_update() never has to write to the reflog for\n> lock->orig_ref_name\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------\n> 1 file changed, 21 insertions(+), 33 deletions(-)\n\nFinished reading the whole thing and it made quite a lot of sense.\n\nThanks.\n"},{"id":"284990","messageId":"1461971692.4123.41.camel@twopensource.com","threadId":"42157","inReplyTo":"57232EAB.3080406@alum.mit.edu","subject":"Re: [PATCH 25/29] refs: resolve symbolic refs first","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-29T23:14:52Z","receivedAt":"2016-04-29T23:14:52Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 11:51 +0200, Michael Haggerty wrote:\n> On 04/29/2016 01:40 AM, David Turner wrote:\n> > On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:\n> > +retry:\n> > ...\n> > > +\t\tif (--attempts_remaining > 0)\n> > > +\t\t\tgoto retry;\n> > \n> > could this be a loop instead of using gotos?\n> \n> It certainly could. The goto-vs-loop question was debated on the\n> mailing\n> list when I first added very similar code elsewhere (unfortunately\n> I'm\n> unable to find a link to that conversation). I was persuaded to\n> change\n> my loop into gotos, the argument being that the \"retry\" case is\n> exceptional and shouldn't be such a dominant part of the function\n> structure. Plus the goto code is briefer and feels less awkward to me\n> in\n> this case (that's subjective, of course).\n\nIt's part of the structure anyway; it's just implicit rather than\nexplicit. I won't block consensus tho.\n"},{"id":"284991","messageId":"1461972108.4123.43.camel@twopensource.com","threadId":"42157","inReplyTo":"57230F71.2020401@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2016-04-29T23:21:48Z","receivedAt":"2016-04-29T23:21:48Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:\n> On 04/27/2016 08:55 PM, Junio C Hamano wrote:\n> > Michael Haggerty writes:\n> > \n> > > @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,\n> > > const char *newrefname, const char *logms\n> > > \t\tgoto rollback;\n> > > \t}\n> > > \n> > > -\tif (!read_ref_full(newrefname, RESOLVE_REF_READING,\n> > > sha1, NULL) &&\n> > > -\t delete_ref(newrefname, sha1, REF_NODEREF)) {\n> > > +\tif (!read_ref_full(newrefname, resolve_flags, sha1,\n> > > NULL) &&\n> > > +\t delete_ref(newrefname, NULL, REF_NODEREF)) {\n> > \n> > Could you explain s/sha1/NULL/ here in the proposed log message?\n> \n> Good question.\n> \n> Passing sha1 to delete_ref() doesn't add any safety, because the same\n> sha1 was just read a moment before, and it is not used for anything\n> else. So the check only protects us from a concurrent update to\n> newrefname between the call to read_ref_full() and the call to\n> delete_ref(). But such a race is indistinguishable from the case that\n> a\n> modification to newrefname happens just before the call to\n> read_ref_full(), which would have the same outcome as the new code.\n> So\n> the \"sha1\" check only adds ways for the rename() to fail in\n> situations\n> where nothing harmful would have happened anyway.\n> \n> That being said, this is a very unlikely race, and I don't think it\n> matters much either way. In any case, the change s/sha1/NULL/ here\n> seems\n> orthogonal to the rest of the patch.\n> \n> David, you wrote the original version of this patch. Am I overlooking\n> something?\n\nI think I might have been handling some weird case related to symbolic\nrefs, but I don't recall the details. Your argument seems right to me.\n"},{"id":"285043","messageId":"57242B22.4050202@alum.mit.edu","threadId":"42157","inReplyTo":"1461972108.4123.43.camel@twopensource.com","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-30T03:48:50Z","receivedAt":"2016-04-30T03:48:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/30/2016 01:21 AM, David Turner wrote:\n> On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:\n>> On 04/27/2016 08:55 PM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>>\n>>>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,\n>>>> const char *newrefname, const char *logms\n>>>> \t\tgoto rollback;\n>>>> \t}\n>>>> \n>>>> -\tif (!read_ref_full(newrefname, RESOLVE_REF_READING,\n>>>> sha1, NULL) &&\n>>>> -\t delete_ref(newrefname, sha1, REF_NODEREF)) {\n>>>> +\tif (!read_ref_full(newrefname, resolve_flags, sha1,\n>>>> NULL) &&\n>>>> +\t delete_ref(newrefname, NULL, REF_NODEREF)) {\n>>>\n>>> Could you explain s/sha1/NULL/ here in the proposed log message?\n>>\n>> Good question.\n>>\n>> Passing sha1 to delete_ref() doesn't add any safety, because the same\n>> sha1 was just read a moment before, and it is not used for anything\n>> else. So the check only protects us from a concurrent update to\n>> newrefname between the call to read_ref_full() and the call to\n>> delete_ref(). But such a race is indistinguishable from the case that\n>> a\n>> modification to newrefname happens just before the call to\n>> read_ref_full(), which would have the same outcome as the new code.\n>> So\n>> the \"sha1\" check only adds ways for the rename() to fail in\n>> situations\n>> where nothing harmful would have happened anyway.\n>>\n>> That being said, this is a very unlikely race, and I don't think it\n>> matters much either way. In any case, the change s/sha1/NULL/ here\n>> seems\n>> orthogonal to the rest of the patch.\n>>\n>> David, you wrote the original version of this patch. Am I overlooking\n>> something?\n> \n> I think I might have been handling some weird case related to symbolic\n> refs, but I don't recall the details. Your argument seems right to me.\n\nDoh, of course. I should have just changed it back to `sha1` and run the\ntest suite, then I would have seen the failure...\n\nThe point is that `read_ref_full()` is now called with\n`RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic\nreference, then `read_ref_full()` sets `sha1` to zeros. But the\npre-check for `delete_ref()` compares `old_sha1` to the recursively\nresolved value of the reference, so that check would fail. (In fact,\n`ref_transaction_delete()` refuses even to add the deletion to the\ntransaction if `old_sha1` is zeros, so it doesn't even get that far.)\n\nSo for shallow technical reasons we can't pass `sha1` to `delete_ref()`\nanymore, and for the deeper reasons discussed in this thread that's not\na problem.\n\nI'll document this in v2 of this patch.\n\nMichael\n"},{"id":"285174","messageId":"xmqq7ffcuyxd.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"57242B22.4050202@alum.mit.edu","subject":"Re: [PATCH 19/29] refs: don't dereference on rename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T17:55:58Z","receivedAt":"2016-05-02T17:55:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The point is that `read_ref_full()` is now called with\n> `RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic\n> reference, then `read_ref_full()` sets `sha1` to zeros.\n\nYes, that was an obvious rationale in the patch that was not\nexplained in the proposed log message (and made me ask you to\nexplain it). I was wondering why this was not loosened\nconditionally (i.e. only pass null_sha1 when symbolic ref is\ninvolved, in which case you _must_ pass null_sha1 because we no\nlonger have anything to compare with).\n\nYour explanation on the \"in all possible interleaving, deletion of\nwhat might have been updated in the middle by other people does not\nmatter\" was a sufficient explanation why it does not have to be\nconditional.\n\n> I'll document this in v2 of this patch.\n\nThanks.\n"},{"id":"285177","messageId":"xmqq37q0uyf3.fsf@gitster.mtv.corp.google.com","threadId":"42157","inReplyTo":"1461886803.4123.14.camel@twopensource.com","subject":"Re: [PATCH 25/29] refs: resolve symbolic refs first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T18:06:56Z","receivedAt":"2016-05-02T18:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:\n> +retry:\n> ...\n>> +\t\tif (--attempts_remaining > 0)\n>> +\t\t\tgoto retry;\n>\n> could this be a loop instead of using gotos?\n\nIt could be, but given that there are many such exit points, I do\nnot think an added level of indentation with while() loop is\nparticularly a good way to go.\n\nUnless that big body of code that is looped around by implicit \"goto\nretry\" is made into a helper function to perform one round of attempt,\nwhich may result in one of success, temporary failure, final failure.\nThen we could do:\n\n\tint attempts_remaining = N;\n int failed_hard = 0;\n while (!failed_hard && attempts_remaining--)\n \tfailed_hard = attempt();\n\t\nand the end result _may_ become easier to read (even though I have\nto think for a second what the correct code should look like that\ncomes after the loop).\n"}]}