{"thread":{"id":"46021","subject":"[PATCH v2 00/25] Prepare to separate out a packed_ref_store","startedAt":"2017-05-22T14:18:15Z","lastAt":"2017-05-24T07:21:18Z","messageCount":31,"participants":["Michael Haggerty","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":25},"messages":[{"id":"320420","messageId":"cover.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":null,"subject":"[PATCH v2 00/25] Prepare to separate out a packed_ref_store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:30Z","receivedAt":"2017-05-22T14:18:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the second iteration of a patch series that prepares the\nground for separating out a `packed_ref_store` and then for changing\n`packed-refs` to be read using `mmap()`. Thanks to Peff, Junio,\nStefan, Brandon, and Johannes for their feedback about v1 [1]. I think\nI have addressed all of your comments.\n\nChanges since v1:\n\n* Since v1, branch `bc/object-id` has been merged to `next`, and it\n  has lots of conflicts with these changes. So I rebased this branch\n  onto a merge of `master` and `bc/object-id`. (I hope this makes\n  Junio's job easier.) This unfortunately causes a bit of tbdiff noise\n  between v1 and v2.\n\n* Patch [01/25]: in t3600, register the `test_when_finished` command\n  before executing `chmod a-w`.\n\n* Patch [04/25] (new patch): convert a few `die(\"internal error: ...\")`\n  to `die(\"BUG: ...\")`.\n\n* Patch [05/25]: Use `strlen()` rather than `memchr()` to check the\n  trim length, and `die()` rather than skipping if it is longer than\n  the reference name.\n\n* Patch [08/25]: Name the log message arguments `msg` for consistency\n  with existing code.\n\n* Patch [10/25]: Rename the new member from `packlock` to\n  `packed_refs_lock`.\n\n* Patch [13/25] (new patch): Move the check for valid\n  `transaction->state` from `files_transaction_commit()` to\n  `ref_transaction_commit()`.\n\n* Patch [14/25]:\n\n  * Add more sanity checks of `transaction->state`.\n\n  * Don't add `ref_transaction_finish()` to the public API. Instead,\n    teach `ref_transaction_commit()` to do the right thing whether or\n    not `ref_transaction_prepare()` has been called.\n\n  * Add and improve docstrings.\n\n  * Allow `ref_transaction_abort()` to be called even before\n    `ref_transaction_prepare()` (in which case it just calls\n    `ref_transaction_free()`).\n\n* Lots of improvements to commit messages and comments, mostly to\n  clarify points that reviewers asked about.\n\nThese changes (along with the merge commit that they are based on) are\nalso available as branch `packed-ref-store-prep` in my GitHub fork\n[2]. If you'd like to see a preview of the rest of the changes (which\nworks but is not yet polished), checkout the `mmap-packed-refs` branch\nfrom the same place.\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1495014840.git.mhagger@alum.mit.edu/\n[2] https://github.com/mhagger/git\n\nJeff King (1):\n  ref-filter: limit traversal to prefix\n\nMichael Haggerty (24):\n  t3600: clean up permissions test properly\n  refs.h: clarify docstring for the ref_transaction_update()-related fns\n  ref_iterator_begin_fn(): fix docstring\n  files-backend: use `die(\"BUG: ...\")`, not `die(\"internal error: ...\")`\n  prefix_ref_iterator: don't trim too much\n  refs_ref_iterator_begin(): don't check prefixes redundantly\n  refs: use `size_t` indexes when iterating over ref transaction updates\n  ref_store: take a `msg` parameter when deleting references\n  lockfile: add a new method, is_lock_file_locked()\n  files-backend: move `lock` member to `files_ref_store`\n  files_ref_store: put the packed files lock directly in this struct\n  files_transaction_cleanup(): new helper function\n  ref_transaction_commit(): check for valid `transaction->state`\n  ref_transaction_prepare(): new optional step for reference updates\n  ref_update_reject_duplicates(): expose function to whole refs module\n  ref_update_reject_duplicates(): use `size_t` rather than `int`\n  ref_update_reject_duplicates(): add a sanity check\n  should_pack_ref(): new function, extracted from `files_pack_refs()`\n  get_packed_ref_cache(): assume \"packed-refs\" won't change while locked\n  read_packed_refs(): do more of the work of reading packed refs\n  read_packed_refs(): report unexpected fopen() failures\n  refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`\n  create_ref_entry(): remove `check_name` option\n  cache_ref_iterator_begin(): avoid priming unneeded directories\n\n builtin/fetch.c                |   2 +-\n builtin/remote.c               |   4 +-\n lockfile.h                     |   8 ++\n ref-filter.c                   |  64 ++++++++-\n refs.c                         | 135 ++++++++++++++++--\n refs.h                         | 149 +++++++++++++++-----\n refs/files-backend.c           | 302 +++++++++++++++++++++++++----------------\n refs/iterator.c                |  18 ++-\n refs/ref-cache.c               | 100 ++++++++++++--\n refs/ref-cache.h               |   6 +-\n refs/refs-internal.h           |  62 +++++++--\n t/helper/test-ref-store.c      |   3 +-\n t/t1405-main-ref-store.sh      |   2 +-\n t/t1406-submodule-ref-store.sh |   2 +-\n t/t3600-rm.sh                  |   4 +-\n 15 files changed, 658 insertions(+), 203 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"320421","messageId":"adea7027b2ff5248100742130fedff4cde961fb9.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 01/25] t3600: clean up permissions test properly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:31Z","receivedAt":"2017-05-22T14:18:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The test of failing `git rm -f` removes the write permissions on the\ntest directory, but fails to restore them if the test fails. This\nmeans that the test temporary directory cannot be cleaned up, which\nmeans that subsequent attempts to run the test fail mysteriously.\n\nInstead, do the cleanup in a `test_when_finished` block so that it\ncan't be skipped.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t3600-rm.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 5f9913ba33..f8568f8841 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -97,9 +97,9 @@ test_expect_success FUNNYNAMES \\\n embedded'\"\n \n test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n+\ttest_when_finished \"chmod 775 .\" &&\n \tchmod a-w . &&\n-\ttest_must_fail git rm -f baz &&\n-\tchmod 775 .\n+\ttest_must_fail git rm -f baz\n '\n \n test_expect_success \\\n-- \n2.11.0\n\n"},{"id":"320422","messageId":"0bfff2359006ff97858da2f86c2b6a8dbdee6e7d.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 02/25] refs.h: clarify docstring for the ref_transaction_update()-related fns","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:32Z","receivedAt":"2017-05-22T14:18:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In particular, make it clear that they make copies of the sha1\narguments.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.h | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/refs.h b/refs.h\nindex 685a979a0e..ec8c6bfbbb 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -427,6 +427,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n  *\n  *     refname -- the name of the reference to be affected.\n  *\n+ *     new_sha1 -- the SHA-1 that should be set to be the new value of\n+ *         the reference. Some functions allow this parameter to be\n+ *         NULL, meaning that the reference is not changed, or\n+ *         null_sha1, meaning that the reference should be deleted. A\n+ *         copy of this value is made in the transaction.\n+ *\n+ *     old_sha1 -- the SHA-1 value that the reference must have before\n+ *         the update. Some functions allow this parameter to be NULL,\n+ *         meaning that the old value of the reference is not checked,\n+ *         or null_sha1, meaning that the reference must not exist\n+ *         before the update. A copy of this value is made in the\n+ *         transaction.\n+ *\n  *     flags -- flags affecting the update, passed to\n  *         update_ref_lock(). Can be REF_NODEREF, which means that\n  *         symbolic references should not be followed.\n-- \n2.11.0\n\n"},{"id":"320423","messageId":"7df9b378009b633e72a98ba6e5f2250718f1ce19.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 03/25] ref_iterator_begin_fn(): fix docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:33Z","receivedAt":"2017-05-22T14:18:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The iterator returned by this function only includes references whose\nnames start with the whole prefix, not all of those in\n`find_containing_dir(prefix)` as the old docstring claimed. This\ndocstring was probably copy-pasted from old ref-cache code, which had\nthe old specification. But now, `cache_ref_iterator_begin()`\n(from which the files reference iterator gets its values)\nautomatically wraps its output using `prefix_ref_iterator_begin()`\nwhen necessary, so it has the stricter behavior.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/refs-internal.h | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex b6b291cf00..7020e51cb7 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -515,9 +515,10 @@ typedef int rename_ref_fn(struct ref_store *ref_store,\n \t\t\t  const char *logmsg);\n \n /*\n- * Iterate over the references in the specified ref_store that are\n- * within find_containing_dir(prefix). If prefix is NULL or the empty\n- * string, iterate over all references in the submodule.\n+ * Iterate over the references in `ref_store` whose names start with\n+ * `prefix`. `prefix` is matched as a literal string, without regard\n+ * for path separators. If prefix is NULL or the empty string, iterate\n+ * over all references in `ref_store`.\n  */\n typedef struct ref_iterator *ref_iterator_begin_fn(\n \t\tstruct ref_store *ref_store,\n-- \n2.11.0\n\n"},{"id":"320424","messageId":"37be27050566a0f5772f2c2e60b435a07fcef263.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 04/25] files-backend: use `die(\"BUG: ...\")`, not `die(\"internal error: ...\")`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:34Z","receivedAt":"2017-05-22T14:18:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The former is by far more common in our codebase.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex cb1f528cbe..fa5d2b6f08 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -105,7 +105,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n \t\tstruct packed_ref_cache *packed_refs = refs->packed;\n \n \t\tif (packed_refs->lock)\n-\t\t\tdie(\"internal error: packed-ref cache cleared while locked\");\n+\t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n \t\trefs->packed = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n \t}\n@@ -397,7 +397,7 @@ static void add_packed_ref(struct files_ref_store *refs,\n \tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n \n \tif (!packed_ref_cache->lock)\n-\t\tdie(\"internal error: packed refs not locked\");\n+\t\tdie(\"BUG: packed refs not locked\");\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n \t\t      create_ref_entry(refname, oid, REF_ISPACKED, 1));\n }\n@@ -1324,7 +1324,7 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n \n \tif (!packed_ref_cache->lock)\n-\t\tdie(\"internal error: packed-refs not locked\");\n+\t\tdie(\"BUG: packed-refs not locked\");\n \n \tout = fdopen_lock_file(packed_ref_cache->lock, \"w\");\n \tif (!out)\n@@ -1367,7 +1367,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n \tif (!packed_ref_cache->lock)\n-\t\tdie(\"internal error: packed-refs not locked\");\n+\t\tdie(\"BUG: packed-refs not locked\");\n \trollback_lock_file(packed_ref_cache->lock);\n \tpacked_ref_cache->lock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n-- \n2.11.0\n\n"},{"id":"320425","messageId":"f2f73adae931939e4335967872a1182bd3c8562b.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 05/25] prefix_ref_iterator: don't trim too much","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:35Z","receivedAt":"2017-05-22T14:18:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The `trim` parameter can be set independently of `prefix`. So if some\ncaller were to set `trim` to be greater than `strlen(prefix)`, we\ncould end up pointing the `refname` field of the iterator past the NUL\nof the actual reference name string.\n\nThat can't happen currently, because `trim` is always set either to\nzero or to `strlen(prefix)`. But even the latter could lead to\nconfusion, if a refname is exactly equal to the prefix, because then\nwe would set the outgoing `refname` to the empty string.\n\nAnd we're about to decouple the `prefix` and `trim` arguments even\nmore, so let's be cautious here. Report a bug if ever asked to trim a\nreference whose name is not longer than `trim`.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/iterator.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex bce1f192f7..4cf449ef66 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -292,7 +292,23 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (!starts_with(iter->iter0->refname, iter->prefix))\n \t\t\tcontinue;\n \n-\t\titer->base.refname = iter->iter0->refname + iter->trim;\n+\t\tif (iter->trim) {\n+\t\t\t/*\n+\t\t\t * It is nonsense to trim off characters that\n+\t\t\t * you haven't already checked for via a\n+\t\t\t * prefix check, whether via this\n+\t\t\t * `prefix_ref_iterator` or upstream in\n+\t\t\t * `iter0`). So if there wouldn't be at least\n+\t\t\t * one character left in the refname after\n+\t\t\t * trimming, report it as a bug:\n+\t\t\t */\n+\t\t\tif (strlen(iter->iter0->refname) <= iter->trim)\n+\t\t\t\tdie(\"BUG: attempt to trim too many characters\");\n+\t\t\titer->base.refname = iter->iter0->refname + iter->trim;\n+\t\t} else {\n+\t\t\titer->base.refname = iter->iter0->refname;\n+\t\t}\n+\n \t\titer->base.oid = iter->iter0->oid;\n \t\titer->base.flags = iter->iter0->flags;\n \t\treturn ITER_OK;\n-- \n2.11.0\n\n"},{"id":"320426","messageId":"c3840954c13e008ad4917a7ccf3faf5ae54b01cc.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 06/25] refs_ref_iterator_begin(): don't check prefixes redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:36Z","receivedAt":"2017-05-22T14:18:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The backend already correctly restricts its output to references whose\nnames start with the prefix. By passing the prefix again to\n`prefix_ref_iterator`, we were forcing that iterator to do redundant\nprefix comparisons. So set it to the empty string.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8af9641aa1..8494b1f20d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1247,7 +1247,13 @@ struct ref_iterator *refs_ref_iterator_begin(\n \tstruct ref_iterator *iter;\n \n \titer = refs->be->iterator_begin(refs, prefix, flags);\n-\titer = prefix_ref_iterator_begin(iter, prefix, trim);\n+\n+\t/*\n+\t * `iterator_begin()` already takes care of prefix, but we\n+\t * might need to do some trimming:\n+\t */\n+\tif (trim)\n+\t\titer = prefix_ref_iterator_begin(iter, \"\", trim);\n \n \treturn iter;\n }\n-- \n2.11.0\n\n"},{"id":"320427","messageId":"affc4f0ef831e70009bdcecbcc1320eb78bb8cf8.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 07/25] refs: use `size_t` indexes when iterating over ref transaction updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:37Z","receivedAt":"2017-05-22T14:18:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Eliminate any chance of integer overflow on platforms where the two\ntypes have different sizes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 2 +-\n refs/files-backend.c | 6 ++++--\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8494b1f20d..71139ba74e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -848,7 +848,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err)\n \n void ref_transaction_free(struct ref_transaction *transaction)\n {\n-\tint i;\n+\tsize_t i;\n \n \tif (!transaction)\n \t\treturn;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fa5d2b6f08..b2559b5585 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2850,7 +2850,8 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n \t\t\t       \"ref_transaction_commit\");\n-\tint ret = 0, i;\n+\tsize_t i;\n+\tint ret = 0;\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@@ -3057,7 +3058,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n \t\t\t       \"initial_ref_transaction_commit\");\n-\tint ret = 0, i;\n+\tsize_t i;\n+\tint ret = 0;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \n \tassert(err);\n-- \n2.11.0\n\n"},{"id":"320428","messageId":"eb1b8016cbf4aff3b595cb176534751520460608.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 08/25] ref_store: take a `msg` parameter when deleting references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:38Z","receivedAt":"2017-05-22T14:18:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Just because the files backend can't retain reflogs for deleted\nreferences is no reason that they shouldn't be supported by the\nvirtual method interface. Also, `delete_ref()` and `refs_delete_ref()`\nhave already gained `msg` parameters. Now let's add them to\n`delete_refs()` and `refs_delete_refs()`.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/fetch.c                |  2 +-\n builtin/remote.c               |  4 ++--\n refs.c                         | 11 ++++++-----\n refs.h                         | 12 +++++++-----\n refs/files-backend.c           |  4 ++--\n refs/refs-internal.h           |  2 +-\n t/helper/test-ref-store.c      |  3 ++-\n t/t1405-main-ref-store.sh      |  2 +-\n t/t1406-submodule-ref-store.sh |  2 +-\n 9 files changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d4d573b985..47708451bc 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -941,7 +941,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \t\tfor (ref = stale_refs; ref; ref = ref->next)\n \t\t\tstring_list_append(&refnames, ref->name);\n \n-\t\tresult = delete_refs(&refnames, 0);\n+\t\tresult = delete_refs(\"fetch: prune\", &refnames, 0);\n \t\tstring_list_clear(&refnames, 0);\n \t}\n \ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex addf97ad29..5f52c5a6d6 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -786,7 +786,7 @@ static int rm(int argc, const char **argv)\n \tstrbuf_release(&buf);\n \n \tif (!result)\n-\t\tresult = delete_refs(&branches, REF_NODEREF);\n+\t\tresult = delete_refs(\"remote: remove\", &branches, REF_NODEREF);\n \tstring_list_clear(&branches, 0);\n \n \tif (skipped.nr) {\n@@ -1301,7 +1301,7 @@ static int prune_remote(const char *remote, int dry_run)\n \tstring_list_sort(&refs_to_prune);\n \n \tif (!dry_run)\n-\t\tresult |= delete_refs(&refs_to_prune, 0);\n+\t\tresult |= delete_refs(\"remote: prune\", &refs_to_prune, 0);\n \n \tfor_each_string_list_item(item, &states.stale) {\n \t\tconst char *refname = item->util;\ndiff --git a/refs.c b/refs.c\nindex 71139ba74e..989462c972 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1902,15 +1902,16 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \treturn refs->be->initial_transaction_commit(refs, transaction, err);\n }\n \n-int refs_delete_refs(struct ref_store *refs, struct string_list *refnames,\n-\t\t     unsigned int flags)\n+int refs_delete_refs(struct ref_store *refs, const char *msg,\n+\t\t     struct string_list *refnames, unsigned int flags)\n {\n-\treturn refs->be->delete_refs(refs, refnames, flags);\n+\treturn refs->be->delete_refs(refs, msg, refnames, flags);\n }\n \n-int delete_refs(struct string_list *refnames, unsigned int flags)\n+int delete_refs(const char *msg, struct string_list *refnames,\n+\t\tunsigned int flags)\n {\n-\treturn refs_delete_refs(get_main_ref_store(), refnames, flags);\n+\treturn refs_delete_refs(get_main_ref_store(), msg, refnames, flags);\n }\n \n int refs_rename_ref(struct ref_store *refs, const char *oldref,\ndiff --git a/refs.h b/refs.h\nindex ec8c6bfbbb..b62722fb81 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -331,7 +331,8 @@ int reflog_exists(const char *refname);\n  * verify that the current value of the reference is old_sha1 before\n  * deleting it. If old_sha1 is NULL, delete the reference if it\n  * exists, regardless of its old value. It is an error for old_sha1 to\n- * be NULL_SHA1. flags is passed through to ref_transaction_delete().\n+ * be NULL_SHA1. msg and flags are passed through to\n+ * ref_transaction_delete().\n  */\n int refs_delete_ref(struct ref_store *refs, const char *msg,\n \t\t    const char *refname,\n@@ -343,12 +344,13 @@ int delete_ref(const char *msg, const char *refname,\n /*\n  * Delete the specified references. If there are any problems, emit\n  * errors but attempt to keep going (i.e., the deletes are not done in\n- * an all-or-nothing transaction). flags is passed through to\n+ * an all-or-nothing transaction). msg and flags are passed through to\n  * ref_transaction_delete().\n  */\n-int refs_delete_refs(struct ref_store *refs, struct string_list *refnames,\n-\t\t     unsigned int flags);\n-int delete_refs(struct string_list *refnames, unsigned int flags);\n+int refs_delete_refs(struct ref_store *refs, const char *msg,\n+\t\t     struct string_list *refnames, unsigned int flags);\n+int delete_refs(const char *msg, struct string_list *refnames,\n+\t\tunsigned int flags);\n \n /** Delete a reflog */\n int refs_delete_reflog(struct ref_store *refs, const char *refname);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b2559b5585..fce8265aa7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1595,7 +1595,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \treturn ret;\n }\n \n-static int files_delete_refs(struct ref_store *ref_store,\n+static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t     struct string_list *refnames, unsigned int flags)\n {\n \tstruct files_ref_store *refs =\n@@ -1627,7 +1627,7 @@ static int files_delete_refs(struct ref_store *ref_store,\n \tfor (i = 0; i < refnames->nr; i++) {\n \t\tconst char *refname = refnames->items[i].string;\n \n-\t\tif (refs_delete_ref(&refs->base, NULL, refname, NULL, flags))\n+\t\tif (refs_delete_ref(&refs->base, msg, refname, NULL, flags))\n \t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n \t}\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 7020e51cb7..95edf6f234 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -508,7 +508,7 @@ typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t     const char *ref_target,\n \t\t\t     const char *refs_heads_master,\n \t\t\t     const char *logmsg);\n-typedef int delete_refs_fn(struct ref_store *ref_store,\n+typedef int delete_refs_fn(struct ref_store *ref_store, const char *msg,\n \t\t\t   struct string_list *refnames, unsigned int flags);\n typedef int rename_ref_fn(struct ref_store *ref_store,\n \t\t\t  const char *oldref, const char *newref,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex fba85e7da5..05d8c4d8af 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -93,12 +93,13 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv)\n static int cmd_delete_refs(struct ref_store *refs, const char **argv)\n {\n \tunsigned int flags = arg_flags(*argv++, \"flags\");\n+\tconst char *msg = *argv++;\n \tstruct string_list refnames = STRING_LIST_INIT_NODUP;\n \n \twhile (*argv)\n \t\tstring_list_append(&refnames, *argv++);\n \n-\treturn refs_delete_refs(refs, &refnames, flags);\n+\treturn refs_delete_refs(refs, msg, &refnames, flags);\n }\n \n static int cmd_rename_ref(struct ref_store *refs, const char **argv)\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 490521f8cb..e8115df5ba 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -31,7 +31,7 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '\n test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '\n \tgit rev-parse FOO -- &&\n \tgit rev-parse refs/tags/new-tag -- &&\n-\t$RUN delete-refs 0 FOO refs/tags/new-tag &&\n+\t$RUN delete-refs 0 nothing FOO refs/tags/new-tag &&\n \ttest_must_fail git rev-parse FOO -- &&\n \ttest_must_fail git rev-parse refs/tags/new-tag --\n '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 13b5454c56..c32d4cc465 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -31,7 +31,7 @@ test_expect_success 'create_symref() not allowed' '\n '\n \n test_expect_success 'delete_refs() not allowed' '\n-\ttest_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag\n+\ttest_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag\n '\n \n test_expect_success 'rename_refs() not allowed' '\n-- \n2.11.0\n\n"},{"id":"320429","messageId":"21939432bc90e0cb58f4b8e1e6a082b9beee93b3.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 10/25] files-backend: move `lock` member to `files_ref_store`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:40Z","receivedAt":"2017-05-22T14:18:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the `lock` member from `packed_ref_cache` to `files_ref_store`,\nsince at most one cache can have a locked \"packed-refs\" file\nassociated with it. Rename it to `packed_refs_lock` to make its\npurpose clearer in its new home. More changes are coming here shortly.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 36 +++++++++++++++++-------------------\n 1 file changed, 17 insertions(+), 19 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fce8265aa7..bfc555a417 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -43,15 +43,6 @@ struct packed_ref_cache {\n \t */\n \tunsigned int referrers;\n \n-\t/*\n-\t * Iff the packed-refs file associated with this instance is\n-\t * currently locked for writing, this points at the associated\n-\t * lock (which is owned by somebody else).  The referrer count\n-\t * is also incremented when the file is locked and decremented\n-\t * when it is unlocked.\n-\t */\n-\tstruct lock_file *lock;\n-\n \t/* The metadata from when this packed-refs cache was read */\n \tstruct stat_validity validity;\n };\n@@ -70,6 +61,13 @@ struct files_ref_store {\n \n \tstruct ref_cache *loose;\n \tstruct packed_ref_cache *packed;\n+\n+\t/*\n+\t * Iff the packed-refs file associated with this instance is\n+\t * currently locked for writing, this points at the associated\n+\t * lock (which is owned by somebody else).\n+\t */\n+\tstruct lock_file *packed_refs_lock;\n };\n \n /* Lock used for the main packed-refs file: */\n@@ -104,7 +102,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n \tif (refs->packed) {\n \t\tstruct packed_ref_cache *packed_refs = refs->packed;\n \n-\t\tif (packed_refs->lock)\n+\t\tif (refs->packed_refs_lock)\n \t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n \t\trefs->packed = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n@@ -396,7 +394,7 @@ static void add_packed_ref(struct files_ref_store *refs,\n {\n \tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n \n-\tif (!packed_ref_cache->lock)\n+\tif (!refs->packed_refs_lock)\n \t\tdie(\"BUG: packed refs not locked\");\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n \t\t      create_ref_entry(refname, oid, REF_ISPACKED, 1));\n@@ -1300,7 +1298,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t * the packed-refs file.\n \t */\n \tpacked_ref_cache = get_packed_ref_cache(refs);\n-\tpacked_ref_cache->lock = &packlock;\n+\trefs->packed_refs_lock = &packlock;\n \t/* Increment the reference count to prevent it from being freed: */\n \tacquire_packed_ref_cache(packed_ref_cache);\n \treturn 0;\n@@ -1323,10 +1321,10 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n \n-\tif (!packed_ref_cache->lock)\n+\tif (!refs->packed_refs_lock)\n \t\tdie(\"BUG: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(packed_ref_cache->lock, \"w\");\n+\tout = fdopen_lock_file(refs->packed_refs_lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1344,11 +1342,11 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error while iterating over references\");\n \n-\tif (commit_lock_file(packed_ref_cache->lock)) {\n+\tif (commit_lock_file(refs->packed_refs_lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n-\tpacked_ref_cache->lock = NULL;\n+\trefs->packed_refs_lock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \terrno = save_errno;\n \treturn error;\n@@ -1366,10 +1364,10 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n-\tif (!packed_ref_cache->lock)\n+\tif (!refs->packed_refs_lock)\n \t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(packed_ref_cache->lock);\n-\tpacked_ref_cache->lock = NULL;\n+\trollback_lock_file(refs->packed_refs_lock);\n+\trefs->packed_refs_lock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(refs);\n }\n-- \n2.11.0\n\n"},{"id":"320430","messageId":"e87859cae9e47cb4d3569111d4604468448637fd.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 12/25] files_transaction_cleanup(): new helper function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:42Z","receivedAt":"2017-05-22T14:18:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract the cleanup functionality from `files_transaction_commit()`\ninto a new function. It will soon have another caller.\n\nUse the common cleanup code even on early exit if the transaction is\nempty, to reduce code duplication.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 33 ++++++++++++++++++++++++---------\n 1 file changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1db40432af..2c70de5209 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2834,6 +2834,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \treturn 0;\n }\n \n+/*\n+ * Unlock any references in `transaction` that are still locked, and\n+ * mark the transaction closed.\n+ */\n+static void files_transaction_cleanup(struct ref_transaction *transaction)\n+{\n+\tsize_t i;\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tstruct ref_lock *lock = update->backend_data;\n+\n+\t\tif (lock) {\n+\t\t\tunlock_ref(lock);\n+\t\t\tupdate->backend_data = NULL;\n+\t\t}\n+\t}\n+\n+\ttransaction->state = REF_TRANSACTION_CLOSED;\n+}\n+\n static int files_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t    struct ref_transaction *transaction,\n \t\t\t\t    struct strbuf *err)\n@@ -2856,10 +2877,8 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: commit called for transaction that is not open\");\n \n-\tif (!transaction->nr) {\n-\t\ttransaction->state = REF_TRANSACTION_CLOSED;\n-\t\treturn 0;\n-\t}\n+\tif (!transaction->nr)\n+\t\tgoto cleanup;\n \n \t/*\n \t * Fail if a refname appears more than once in the\n@@ -3005,15 +3024,11 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \tclear_loose_ref_cache(refs);\n \n cleanup:\n+\tfiles_transaction_cleanup(transaction);\n \tstrbuf_release(&sb);\n-\ttransaction->state = REF_TRANSACTION_CLOSED;\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n-\t\tstruct ref_lock *lock = update->backend_data;\n-\n-\t\tif (lock)\n-\t\t\tunlock_ref(lock);\n \n \t\tif (update->flags & REF_DELETED_LOOSE) {\n \t\t\t/*\n-- \n2.11.0\n\n"},{"id":"320431","messageId":"65189c542226105b83163559192fe647484328d6.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 11/25] files_ref_store: put the packed files lock directly in this struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:41Z","receivedAt":"2017-05-22T14:18:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of using a global `lock_file` instance for the main\n\"packed-refs\" file and using a pointer in `files_ref_store` to keep\ntrack of whether it is locked, embed the `lock_file` instance directly\nin the `files_ref_store` struct and use the new\n`is_lock_file_locked()` function to keep track of whether it is\nlocked. This keeps related data together and makes the main reference\nstore less of a special case.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 29 +++++++++++------------------\n 1 file changed, 11 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bfc555a417..1db40432af 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -63,16 +63,12 @@ struct files_ref_store {\n \tstruct packed_ref_cache *packed;\n \n \t/*\n-\t * Iff the packed-refs file associated with this instance is\n-\t * currently locked for writing, this points at the associated\n-\t * lock (which is owned by somebody else).\n+\t * Lock used for the \"packed-refs\" file. Note that this (and\n+\t * thus the enclosing `files_ref_store`) must not be freed.\n \t */\n-\tstruct lock_file *packed_refs_lock;\n+\tstruct lock_file packed_refs_lock;\n };\n \n-/* Lock used for the main packed-refs file: */\n-static struct lock_file packlock;\n-\n /*\n  * Increment the reference count of *packed_refs.\n  */\n@@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n \tif (refs->packed) {\n \t\tstruct packed_ref_cache *packed_refs = refs->packed;\n \n-\t\tif (refs->packed_refs_lock)\n+\t\tif (is_lock_file_locked(&refs->packed_refs_lock))\n \t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n \t\trefs->packed = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n@@ -394,7 +390,7 @@ static void add_packed_ref(struct files_ref_store *refs,\n {\n \tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n \n-\tif (!refs->packed_refs_lock)\n+\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tdie(\"BUG: packed refs not locked\");\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n \t\t      create_ref_entry(refname, oid, REF_ISPACKED, 1));\n@@ -1288,7 +1284,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t}\n \n \tif (hold_lock_file_for_update_timeout(\n-\t\t\t    &packlock, files_packed_refs_path(refs),\n+\t\t\t    &refs->packed_refs_lock, files_packed_refs_path(refs),\n \t\t\t    flags, timeout_value) < 0)\n \t\treturn -1;\n \t/*\n@@ -1298,7 +1294,6 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t * the packed-refs file.\n \t */\n \tpacked_ref_cache = get_packed_ref_cache(refs);\n-\trefs->packed_refs_lock = &packlock;\n \t/* Increment the reference count to prevent it from being freed: */\n \tacquire_packed_ref_cache(packed_ref_cache);\n \treturn 0;\n@@ -1321,10 +1316,10 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n \n-\tif (!refs->packed_refs_lock)\n+\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(refs->packed_refs_lock, \"w\");\n+\tout = fdopen_lock_file(&refs->packed_refs_lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1342,11 +1337,10 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error while iterating over references\");\n \n-\tif (commit_lock_file(refs->packed_refs_lock)) {\n+\tif (commit_lock_file(&refs->packed_refs_lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n-\trefs->packed_refs_lock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \terrno = save_errno;\n \treturn error;\n@@ -1364,10 +1358,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n-\tif (!refs->packed_refs_lock)\n+\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(refs->packed_refs_lock);\n-\trefs->packed_refs_lock = NULL;\n+\trollback_lock_file(&refs->packed_refs_lock);\n \trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(refs);\n }\n-- \n2.11.0\n\n"},{"id":"320432","messageId":"41938a21a7fe83b197b8b27417b0ad1230f5266a.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 09/25] lockfile: add a new method, is_lock_file_locked()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:39Z","receivedAt":"2017-05-22T14:18:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It will soon prove useful.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n lockfile.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex 7b715f9e77..572064939c 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -175,6 +175,14 @@ static inline int hold_lock_file_for_update(\n \treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n }\n \n+/*\n+ * Return a nonzero value iff `lk` is currently locked.\n+ */\n+static inline int is_lock_file_locked(struct lock_file *lk)\n+{\n+\treturn is_tempfile_active(&lk->tempfile);\n+}\n+\n /*\n  * Append an appropriate error message to `buf` following the failure\n  * of `hold_lock_file_for_update()` to lock `path`. `err` should be the\n-- \n2.11.0\n\n"},{"id":"320433","messageId":"88ca1fc8eafc38050c7efb93dad2c4cb12354271.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 14/25] ref_transaction_prepare(): new optional step for reference updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:44Z","receivedAt":"2017-05-22T14:18:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In the future, compound reference stores will sometimes need to modify\nreferences in two different reference stores at the same time, meaning\nthat a single logical reference transaction might have to be\nimplemented as two internal sub-transactions. They won't want to call\n`ref_transaction_commit()` for the two sub-transactions one after the\nother, because that wouldn't be atomic (the first commit could succeed\nand the second one fail). Instead, they will want to prepare both\nsub-transactions (i.e., obtain any necessary locks and do any\npre-checks), and only if both prepare steps succeed, then commit both\nsub-transactions.\n\nStart preparing for that day by adding a new, optional\n`ref_transaction_prepare()` step to the reference transaction\nsequence, which obtains the locks and does any prechecks, reporting\nany errors that occur. Also add a `ref_transaction_abort()` function\nthat can be used to abort a sub-transaction even if it has already\nbeen prepared.\n\nThat is on the side of the public-facing API. On the side of the\n`ref_store` VTABLE, get rid of `transaction_commit` and instead add\nmethods `transaction_prepare`, `transaction_finish`, and\n`transaction_abort`. A `ref_transaction_commit()` now basically calls\nmethods `transaction_prepare` then `transaction_finish`.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               |  74 ++++++++++++++++++++++++++++--\n refs.h               | 124 ++++++++++++++++++++++++++++++++++++++++-----------\n refs/files-backend.c |  63 ++++++++++++++++++++------\n refs/refs-internal.h |  45 ++++++++++++++-----\n 4 files changed, 253 insertions(+), 53 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f8f41ffb04..b3860a9e33 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -853,6 +853,19 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tif (!transaction)\n \t\treturn;\n \n+\tswitch (transaction->state) {\n+\tcase REF_TRANSACTION_OPEN:\n+\tcase REF_TRANSACTION_CLOSED:\n+\t\t/* OK */\n+\t\tbreak;\n+\tcase REF_TRANSACTION_PREPARED:\n+\t\tdie(\"BUG: free called on a prepared reference transaction\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: unexpected reference transaction state\");\n+\t\tbreak;\n+\t}\n+\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tfree(transaction->updates[i]->msg);\n \t\tfree(transaction->updates[i]);\n@@ -1689,8 +1702,8 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\t\t\t  refs_heads_master, logmsg);\n }\n \n-int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t   struct strbuf *err)\n+int ref_transaction_prepare(struct ref_transaction *transaction,\n+\t\t\t    struct strbuf *err)\n {\n \tstruct ref_store *refs = transaction->ref_store;\n \n@@ -1698,6 +1711,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tcase REF_TRANSACTION_OPEN:\n \t\t/* Good. */\n \t\tbreak;\n+\tcase REF_TRANSACTION_PREPARED:\n+\t\tdie(\"BUG: prepare called twice on reference transaction\");\n+\t\tbreak;\n \tcase REF_TRANSACTION_CLOSED:\n \t\tdie(\"BUG: prepare called on a closed reference transaction\");\n \t\tbreak;\n@@ -1712,7 +1728,59 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n-\treturn refs->be->transaction_commit(refs, transaction, err);\n+\treturn refs->be->transaction_prepare(refs, transaction, err);\n+}\n+\n+int ref_transaction_abort(struct ref_transaction *transaction,\n+\t\t\t  struct strbuf *err)\n+{\n+\tstruct ref_store *refs = transaction->ref_store;\n+\tint ret = 0;\n+\n+\tswitch (transaction->state) {\n+\tcase REF_TRANSACTION_OPEN:\n+\t\t/* No need to abort explicitly. */\n+\t\tbreak;\n+\tcase REF_TRANSACTION_PREPARED:\n+\t\tret = refs->be->transaction_abort(refs, transaction, err);\n+\t\tbreak;\n+\tcase REF_TRANSACTION_CLOSED:\n+\t\tdie(\"BUG: abort called on a closed reference transaction\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: unexpected reference transaction state\");\n+\t\tbreak;\n+\t}\n+\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n+int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t   struct strbuf *err)\n+{\n+\tstruct ref_store *refs = transaction->ref_store;\n+\tint ret;\n+\n+\tswitch (transaction->state) {\n+\tcase REF_TRANSACTION_OPEN:\n+\t\t/* Need to prepare first. */\n+\t\tret = ref_transaction_prepare(transaction, err);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t\tbreak;\n+\tcase REF_TRANSACTION_PREPARED:\n+\t\t/* Fall through to finish. */\n+\t\tbreak;\n+\tcase REF_TRANSACTION_CLOSED:\n+\t\tdie(\"BUG: commit called on a closed reference transaction\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: unexpected reference transaction state\");\n+\t\tbreak;\n+\t}\n+\n+\treturn refs->be->transaction_finish(refs, transaction, err);\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/refs.h b/refs.h\nindex b62722fb81..4be14c4b3c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -143,30 +143,71 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\n \n /*\n- * A ref_transaction represents a collection of ref updates\n- * that should succeed or fail together.\n+ * A ref_transaction represents a collection of reference updates that\n+ * should succeed or fail together.\n  *\n  * Calling sequence\n  * ----------------\n+ *\n  * - Allocate and initialize a `struct ref_transaction` by calling\n  *   `ref_transaction_begin()`.\n  *\n- * - List intended ref updates by calling functions like\n- *   `ref_transaction_update()` and `ref_transaction_create()`.\n- *\n- * - Call `ref_transaction_commit()` to execute the transaction.\n- *   If this succeeds, the ref updates will have taken place and\n- *   the transaction cannot be rolled back.\n- *\n- * - Instead of `ref_transaction_commit`, use\n- *   `initial_ref_transaction_commit()` if the ref database is known\n- *   to be empty (e.g. during clone).  This is likely to be much\n- *   faster.\n- *\n- * - At any time call `ref_transaction_free()` to discard the\n- *   transaction and free associated resources.  In particular,\n- *   this rolls back the transaction if it has not been\n- *   successfully committed.\n+ * - Specify the intended ref updates by calling one or more of the\n+ *   following functions:\n+ *   - `ref_transaction_update()`\n+ *   - `ref_transaction_create()`\n+ *   - `ref_transaction_delete()`\n+ *   - `ref_transaction_verify()`\n+ *\n+ * - Then either:\n+ *\n+ *   - Optionally call `ref_transaction_prepare()` to prepare the\n+ *     transaction. This locks all references, checks preconditions,\n+ *     etc. but doesn't finalize anything. If this step fails, the\n+ *     transaction has been closed and can only be freed. If this step\n+ *     succeeds, then `ref_transaction_commit()` is almost certain to\n+ *     succeed. However, you can still call `ref_transaction_abort()`\n+ *     if you decide not to commit the transaction after all.\n+ *\n+ *   - Call `ref_transaction_commit()` to execute the transaction,\n+ *     make the changes permanent, and release all locks. If you\n+ *     haven't already called `ref_transaction_prepare()`, then\n+ *     `ref_transaction_commit()` calls it for you.\n+ *\n+ *   Or\n+ *\n+ *   - Call `initial_ref_transaction_commit()` if the ref database is\n+ *     known to be empty and have no other writers (e.g. during\n+ *     clone). This is likely to be much faster than\n+ *     `ref_transaction_commit()`. `ref_transaction_prepare()` should\n+ *     *not* be called before `initial_ref_transaction_commit()`.\n+ *\n+ * - Then finally, call `ref_transaction_free()` to free the\n+ *   `ref_transaction` data structure.\n+ *\n+ * At any time before calling `ref_transaction_commit()`, you can call\n+ * `ref_transaction_abort()` to abort the transaction, rollback any\n+ * locks, and free any associated resources (including the\n+ * `ref_transaction` data structure).\n+ *\n+ * Putting it all together, a complete reference update looks like\n+ *\n+ *         struct ref_transaction *transaction;\n+ *         struct strbuf err = STRBUF_INIT;\n+ *         int ret = 0;\n+ *\n+ *         transaction = ref_store_transaction_begin(refs, &err);\n+ *         if (!transaction ||\n+ *             ref_transaction_update(...) ||\n+ *             ref_transaction_create(...) ||\n+ *             ...etc... ||\n+ *             ref_transaction_commit(transaction, &err)) {\n+ *                 error(\"%s\", err.buf);\n+ *                 ret = -1;\n+ *         }\n+ *         ref_transaction_free(transaction);\n+ *         strbuf_release(&err);\n+ *         return ret;\n  *\n  * Error handling\n  * --------------\n@@ -183,8 +224,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\n  * -------\n  *\n  * Note that no locks are taken, and no refs are read, until\n- * `ref_transaction_commit` is called.  So `ref_transaction_verify`\n- * won't report a verification failure until the commit is attempted.\n+ * `ref_transaction_prepare()` or `ref_transaction_commit()` is\n+ * called. So, for example, `ref_transaction_verify()` won't report a\n+ * verification failure until the commit is attempted.\n  */\n struct ref_transaction;\n \n@@ -523,19 +565,47 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags,\n \t\t\t   struct strbuf *err);\n \n-/*\n- * Commit all of the changes that have been queued in transaction, as\n- * atomically as possible.\n- *\n- * Returns 0 for success, or one of the below error codes for errors.\n- */\n /* Naming conflict (for example, the ref names A and A/B conflict). */\n #define TRANSACTION_NAME_CONFLICT -1\n /* All other errors. */\n #define TRANSACTION_GENERIC_ERROR -2\n+\n+/*\n+ * Perform the preparatory stages of commiting `transaction`. Acquire\n+ * any needed locks, check preconditions, etc.; basically, do as much\n+ * as possible to ensure that the transaction will be able to go\n+ * through, stopping just short of making any irrevocable or\n+ * user-visible changes. The updates that this function prepares can\n+ * be finished up by calling `ref_transaction_commit()` or rolled back\n+ * by calling `ref_transaction_abort()`.\n+ *\n+ * On success, return 0 and leave the transaction in \"prepared\" state.\n+ * On failure, abort the transaction, write an error message to `err`,\n+ * and return one of the `TRANSACTION_*` constants.\n+ *\n+ * Callers who don't need such fine-grained control over commiting\n+ * reference transactions should just call `ref_transaction_commit()`.\n+ */\n+int ref_transaction_prepare(struct ref_transaction *transaction,\n+\t\t\t    struct strbuf *err);\n+\n+/*\n+ * Commit all of the changes that have been queued in transaction, as\n+ * atomically as possible. On success, return 0 and leave the\n+ * transaction in \"closed\" state. On failure, roll back the\n+ * transaction, write an error message to `err`, and return one of the\n+ * `TRANSACTION_*` constants\n+ */\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n+/*\n+ * Abort `transaction`, which has been begun and possibly prepared,\n+ * but not yet committed.\n+ */\n+int ref_transaction_abort(struct ref_transaction *transaction,\n+\t\t\t  struct strbuf *err);\n+\n /*\n  * Like ref_transaction_commit(), but optimized for creating\n  * references when originally initializing a repository (e.g., by \"git\n@@ -551,7 +621,7 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t   struct strbuf *err);\n \n /*\n- * Free an existing transaction and all associated data.\n+ * Free `*transaction` and all associated data.\n  */\n void ref_transaction_free(struct ref_transaction *transaction);\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a4261d4683..19842d2e56 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2855,22 +2855,19 @@ static void files_transaction_cleanup(struct ref_transaction *transaction)\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-static int files_transaction_commit(struct ref_store *ref_store,\n-\t\t\t\t    struct ref_transaction *transaction,\n-\t\t\t\t    struct strbuf *err)\n+static int files_transaction_prepare(struct ref_store *ref_store,\n+\t\t\t\t     struct ref_transaction *transaction,\n+\t\t\t\t     struct strbuf *err)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE,\n-\t\t\t       \"ref_transaction_commit\");\n+\t\t\t       \"ref_transaction_prepare\");\n \tsize_t i;\n \tint ret = 0;\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-\tstruct strbuf sb = STRBUF_INIT;\n \n \tassert(err);\n \n@@ -2934,6 +2931,8 @@ static int files_transaction_commit(struct ref_store *ref_store,\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 \t * open at a time to avoid running out of file descriptors.\n+\t * Note that lock_ref_for_update() might append more updates\n+\t * to the transaction.\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -2941,7 +2940,38 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \t\tret = lock_ref_for_update(refs, update, transaction,\n \t\t\t\t\t  head_ref, &affected_refnames, err);\n \t\tif (ret)\n-\t\t\tgoto cleanup;\n+\t\t\tbreak;\n+\t}\n+\n+cleanup:\n+\tfree(head_ref);\n+\tstring_list_clear(&affected_refnames, 0);\n+\n+\tif (ret)\n+\t\tfiles_transaction_cleanup(transaction);\n+\telse\n+\t\ttransaction->state = REF_TRANSACTION_PREPARED;\n+\n+\treturn ret;\n+}\n+\n+static int files_transaction_finish(struct ref_store *ref_store,\n+\t\t\t\t    struct ref_transaction *transaction,\n+\t\t\t\t    struct strbuf *err)\n+{\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, 0, \"ref_transaction_finish\");\n+\tsize_t i;\n+\tint ret = 0;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tassert(err);\n+\n+\tif (!transaction->nr) {\n+\t\ttransaction->state = REF_TRANSACTION_CLOSED;\n+\t\treturn 0;\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n@@ -3022,7 +3052,6 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \n cleanup:\n \tfiles_transaction_cleanup(transaction);\n-\tstrbuf_release(&sb);\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -3039,13 +3068,19 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \t\t}\n \t}\n \n+\tstrbuf_release(&sb);\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 \n+static int files_transaction_abort(struct ref_store *ref_store,\n+\t\t\t\t   struct ref_transaction *transaction,\n+\t\t\t\t   struct strbuf *err)\n+{\n+\tfiles_transaction_cleanup(transaction);\n+\treturn 0;\n+}\n+\n static int ref_present(const char *refname,\n \t\t       const struct object_id *oid, int flags, void *cb_data)\n {\n@@ -3316,7 +3351,9 @@ struct ref_storage_be refs_be_files = {\n \t\"files\",\n \tfiles_ref_store_create,\n \tfiles_init_db,\n-\tfiles_transaction_commit,\n+\tfiles_transaction_prepare,\n+\tfiles_transaction_finish,\n+\tfiles_transaction_abort,\n \tfiles_initial_transaction_commit,\n \n \tfiles_pack_refs,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 95edf6f234..4d3dd17f9f 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -185,17 +185,27 @@ struct ref_update *ref_transaction_add_update(\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- * CLOSED: A closed transaction is no longer active and no other operations\n- *         than free can be used on it in this state.\n- *         A transaction can either become closed by successfully committing\n- *         an active transaction or if there is a failure while building\n- *         the transaction thus rendering it failed/inactive.\n+ *\n+ * OPEN:   The transaction is initialized and new updates can still be\n+ *         added to it. An OPEN transaction can be prepared,\n+ *         committed, freed, or aborted (freeing and aborting an open\n+ *         transaction are equivalent).\n+ *\n+ * PREPARED: ref_transaction_prepare(), which locks all of the\n+ *         references involved in the update and checks that the\n+ *         update has no errors, has been called successfully for the\n+ *         transaction. A PREPARED transaction can be committed or\n+ *         aborted.\n+ *\n+ * CLOSED: The transaction is no longer active. A transaction becomes\n+ *         CLOSED if there is a failure while building the transaction\n+ *         or if a transaction is committed or aborted. A CLOSED\n+ *         transaction can only be freed.\n  */\n enum ref_transaction_state {\n-\tREF_TRANSACTION_OPEN   = 0,\n-\tREF_TRANSACTION_CLOSED = 1\n+\tREF_TRANSACTION_OPEN     = 0,\n+\tREF_TRANSACTION_PREPARED = 1,\n+\tREF_TRANSACTION_CLOSED   = 2\n };\n \n /*\n@@ -497,6 +507,18 @@ typedef struct ref_store *ref_store_init_fn(const char *gitdir,\n \n typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);\n \n+typedef int ref_transaction_prepare_fn(struct ref_store *refs,\n+\t\t\t\t       struct ref_transaction *transaction,\n+\t\t\t\t       struct strbuf *err);\n+\n+typedef int ref_transaction_finish_fn(struct ref_store *refs,\n+\t\t\t\t      struct ref_transaction *transaction,\n+\t\t\t\t      struct strbuf *err);\n+\n+typedef int ref_transaction_abort_fn(struct ref_store *refs,\n+\t\t\t\t     struct ref_transaction *transaction,\n+\t\t\t\t     struct strbuf *err);\n+\n typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t      struct ref_transaction *transaction,\n \t\t\t\t      struct strbuf *err);\n@@ -600,7 +622,10 @@ struct ref_storage_be {\n \tconst char *name;\n \tref_store_init_fn *init;\n \tref_init_db_fn *init_db;\n-\tref_transaction_commit_fn *transaction_commit;\n+\n+\tref_transaction_prepare_fn *transaction_prepare;\n+\tref_transaction_finish_fn *transaction_finish;\n+\tref_transaction_abort_fn *transaction_abort;\n \tref_transaction_commit_fn *initial_transaction_commit;\n \n \tpack_refs_fn *pack_refs;\n-- \n2.11.0\n\n"},{"id":"320434","messageId":"03b1bb62322361cd83016c1d824a7f2d12ad578a.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 16/25] ref_update_reject_duplicates(): use `size_t` rather than `int`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:46Z","receivedAt":"2017-05-22T14:18:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Eliminate a theoretical risk of integer overflow if the two types have\ndifferent sizes.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex beb49fb297..143936a9c3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1705,7 +1705,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n int ref_update_reject_duplicates(struct string_list *refnames,\n \t\t\t\t struct strbuf *err)\n {\n-\tint i, n = refnames->nr;\n+\tsize_t i, n = refnames->nr;\n \n \tassert(err);\n \n-- \n2.11.0\n\n"},{"id":"320435","messageId":"42b26d23a71f932e14727066139b32d905ec53c6.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 15/25] ref_update_reject_duplicates(): expose function to whole refs module","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:45Z","receivedAt":"2017-05-22T14:19:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It will soon have some other users.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 17 +++++++++++++++++\n refs/files-backend.c | 17 -----------------\n refs/refs-internal.h |  8 ++++++++\n 3 files changed, 25 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b3860a9e33..beb49fb297 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1702,6 +1702,23 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\t\t\t  refs_heads_master, logmsg);\n }\n \n+int ref_update_reject_duplicates(struct string_list *refnames,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint i, n = refnames->nr;\n+\n+\tassert(err);\n+\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    refnames->items[i].string);\n+\t\t\treturn 1;\n+\t\t}\n+\treturn 0;\n+}\n+\n int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\t\t    struct strbuf *err)\n {\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 19842d2e56..8d0ce739a6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2512,23 +2512,6 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \treturn ref_iterator;\n }\n \n-static int ref_update_reject_duplicates(struct string_list *refnames,\n-\t\t\t\t\tstruct strbuf *err)\n-{\n-\tint i, n = refnames->nr;\n-\n-\tassert(err);\n-\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    refnames->items[i].string);\n-\t\t\treturn 1;\n-\t\t}\n-\treturn 0;\n-}\n-\n /*\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.\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 4d3dd17f9f..192f9f85c9 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -169,6 +169,14 @@ int refs_read_raw_ref(struct ref_store *ref_store,\n \t\t      const char *refname, unsigned char *sha1,\n \t\t      struct strbuf *referent, unsigned int *type);\n \n+/*\n+ * Write an error to `err` and return a nonzero value iff the same\n+ * refname appears multiple times in `refnames`. `refnames` must be\n+ * sorted on entry to this function.\n+ */\n+int ref_update_reject_duplicates(struct string_list *refnames,\n+\t\t\t\t struct strbuf *err);\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-- \n2.11.0\n\n"},{"id":"320436","messageId":"8383dc8f8e30b61548374a5427da09f98a2175af.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 17/25] ref_update_reject_duplicates(): add a sanity check","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:47Z","receivedAt":"2017-05-22T14:19:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's pretty cheap to make sure that the caller didn't pass us an\nunsorted list by accident, so do so.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 143936a9c3..d1c781d94e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1709,13 +1709,19 @@ int ref_update_reject_duplicates(struct string_list *refnames,\n \n \tassert(err);\n \n-\tfor (i = 1; i < n; i++)\n-\t\tif (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {\n+\tfor (i = 1; i < n; i++) {\n+\t\tint cmp = strcmp(refnames->items[i - 1].string,\n+\t\t\t\t refnames->items[i].string);\n+\n+\t\tif (!cmp) {\n \t\t\tstrbuf_addf(err,\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} else if (cmp > 0) {\n+\t\t\tdie(\"BUG: ref_update_reject_duplicates() received unsorted list\");\n \t\t}\n+\t}\n \treturn 0;\n }\n \n-- \n2.11.0\n\n"},{"id":"320437","messageId":"6f28d8349797120fe344d6ed160df2e80a41c7a2.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 19/25] get_packed_ref_cache(): assume \"packed-refs\" won't change while locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:49Z","receivedAt":"2017-05-22T14:19:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If we've got the \"packed-refs\" file locked, then it can't change;\nthere's no need to keep calling `stat_validity_check()` on it.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 29514392b0..c4bc9550d3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -342,13 +342,18 @@ static void files_ref_path(struct files_ref_store *refs,\n \n /*\n  * Get the packed_ref_cache for the specified files_ref_store,\n- * creating it if necessary.\n+ * creating and populating it if it hasn't been read before or if the\n+ * file has been changed (according to its `validity` field) since it\n+ * was last read. On the other hand, if we hold the lock, then assume\n+ * that the file hasn't been changed out from under us, so skip the\n+ * extra `stat()` call in `stat_validity_check()`.\n  */\n static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)\n {\n \tconst char *packed_refs_file = files_packed_refs_path(refs);\n \n \tif (refs->packed &&\n+\t    !is_lock_file_locked(&refs->packed_refs_lock) &&\n \t    !stat_validity_check(&refs->packed->validity, packed_refs_file))\n \t\tclear_packed_ref_cache(refs);\n \n@@ -1288,10 +1293,11 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t\t\t    flags, timeout_value) < 0)\n \t\treturn -1;\n \t/*\n-\t * Get the current packed-refs while holding the lock.  If the\n-\t * packed-refs file has been modified since we last read it,\n-\t * this will automatically invalidate the cache and re-read\n-\t * the packed-refs file.\n+\t * Get the current packed-refs while holding the lock. It is\n+\t * important that we call `get_packed_ref_cache()` before\n+\t * setting `packed_ref_cache->lock`, because otherwise the\n+\t * former will see that the file is locked and assume that the\n+\t * cache can't be stale.\n \t */\n \tpacked_ref_cache = get_packed_ref_cache(refs);\n \t/* Increment the reference count to prevent it from being freed: */\n-- \n2.11.0\n\n"},{"id":"320438","messageId":"014fce73da7def59605e55e113d05802d4e219ff.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 20/25] read_packed_refs(): do more of the work of reading packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:50Z","receivedAt":"2017-05-22T14:19:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Teach `read_packed_refs()` to also\n\n* Allocate and initialize the new `packed_ref_cache`\n* Open and close the `packed-refs` file\n* Update the `validity` field of the new object\n\nThis decreases the coupling between `packed_refs_cache` and\n`files_ref_store` by a little bit.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 40 ++++++++++++++++++++++++----------------\n refs/ref-cache.h     |  3 ++-\n 2 files changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c4bc9550d3..b4fa745cd7 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -209,7 +209,9 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n }\n \n /*\n- * Read f, which is a packed-refs file, into dir.\n+ * Read from `packed_refs_file` into a newly-allocated\n+ * `packed_ref_cache` and return it. The return value will already\n+ * have its reference count incremented.\n  *\n  * A comment line of the form \"# pack-refs with: \" may contain zero or\n  * more traits. We interpret the traits as follows:\n@@ -235,12 +237,26 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n  *      compatibility with older clients, but we do not require it\n  *      (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n  */\n-static void read_packed_refs(FILE *f, struct ref_dir *dir)\n+static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n {\n+\tFILE *f;\n+\tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n \tstruct ref_entry *last = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n+\tstruct ref_dir *dir;\n \n+\tacquire_packed_ref_cache(packed_refs);\n+\tpacked_refs->cache = create_ref_cache(NULL, NULL);\n+\tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n+\n+\tf = fopen(packed_refs_file, \"r\");\n+\tif (!f)\n+\t\treturn packed_refs;\n+\n+\tstat_validity_update(&packed_refs->validity, fileno(f));\n+\n+\tdir = get_ref_dir(packed_refs->cache->root);\n \twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n \t\tstruct object_id oid;\n \t\tconst char *refname;\n@@ -287,7 +303,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t}\n \t}\n \n+\tfclose(f);\n \tstrbuf_release(&line);\n+\n+\treturn packed_refs;\n }\n \n static const char *files_packed_refs_path(struct files_ref_store *refs)\n@@ -357,20 +376,9 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n \t    !stat_validity_check(&refs->packed->validity, packed_refs_file))\n \t\tclear_packed_ref_cache(refs);\n \n-\tif (!refs->packed) {\n-\t\tFILE *f;\n-\n-\t\trefs->packed = xcalloc(1, sizeof(*refs->packed));\n-\t\tacquire_packed_ref_cache(refs->packed);\n-\t\trefs->packed->cache = create_ref_cache(&refs->base, NULL);\n-\t\trefs->packed->cache->root->flag &= ~REF_INCOMPLETE;\n-\t\tf = fopen(packed_refs_file, \"r\");\n-\t\tif (f) {\n-\t\t\tstat_validity_update(&refs->packed->validity, fileno(f));\n-\t\t\tread_packed_refs(f, get_ref_dir(refs->packed->cache->root));\n-\t\t\tfclose(f);\n-\t\t}\n-\t}\n+\tif (!refs->packed)\n+\t\trefs->packed = read_packed_refs(packed_refs_file);\n+\n \treturn refs->packed;\n }\n \ndiff --git a/refs/ref-cache.h b/refs/ref-cache.h\nindex 1f65e2f9ed..fbfee7ce79 100644\n--- a/refs/ref-cache.h\n+++ b/refs/ref-cache.h\n@@ -194,7 +194,8 @@ struct ref_entry *create_ref_entry(const char *refname,\n  * function called to fill in incomplete directories in the\n  * `ref_cache` when they are accessed. If it is NULL, then the whole\n  * `ref_cache` must be filled (including clearing its directories'\n- * `REF_INCOMPLETE` bits) before it is used.\n+ * `REF_INCOMPLETE` bits) before it is used, and `refs` can be NULL,\n+ * too.\n  */\n struct ref_cache *create_ref_cache(struct ref_store *refs,\n \t\t\t\t   fill_ref_dir_fn *fill_ref_dir);\n-- \n2.11.0\n\n"},{"id":"320439","messageId":"dcd5c44b31a5845b02551ac59ba659eb3d2822c7.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 22/25] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:52Z","receivedAt":"2017-05-22T14:19:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of handling `GIT_REF_PARANOIA` in\n`files_ref_iterator_begin()`, handle it in\n`refs_ref_iterator_begin()`, where it will cover all reference stores.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               |  5 +++++\n refs/files-backend.c | 11 ++++-------\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d1c781d94e..f0685c9251 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1259,6 +1259,11 @@ struct ref_iterator *refs_ref_iterator_begin(\n {\n \tstruct ref_iterator *iter;\n \n+\tif (ref_paranoia < 0)\n+\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n+\tif (ref_paranoia)\n+\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n+\n \titer = refs->be->iterator_begin(refs, prefix, flags);\n \n \t/*\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex dbfd03f989..5de36fc335 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1074,15 +1074,12 @@ static struct ref_iterator *files_ref_iterator_begin(\n \tstruct ref_iterator *loose_iter, *packed_iter;\n \tstruct files_ref_iterator *iter;\n \tstruct ref_iterator *ref_iterator;\n+\tunsigned int required_flags = REF_STORE_READ;\n \n-\tif (ref_paranoia < 0)\n-\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n-\tif (ref_paranoia)\n-\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n+\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))\n+\t\trequired_flags |= REF_STORE_ODB;\n \n-\trefs = files_downcast(ref_store,\n-\t\t\t      REF_STORE_READ | (ref_paranoia ? 0 : REF_STORE_ODB),\n-\t\t\t      \"ref_iterator_begin\");\n+\trefs = files_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n-- \n2.11.0\n\n"},{"id":"320440","messageId":"e7bf8155babba7da1f5c860c3136396972d53c96.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 23/25] create_ref_entry(): remove `check_name` option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:53Z","receivedAt":"2017-05-22T14:19:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Only one caller was using it, so move the check to that caller.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 12 ++++++++----\n refs/ref-cache.c     |  6 +-----\n refs/ref-cache.h     |  3 +--\n 3 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5de36fc335..d8b3f73147 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -291,7 +291,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\t\toidclr(&oid);\n \t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n \t\t\t}\n-\t\t\tlast = create_ref_entry(refname, &oid, flag, 0);\n+\t\t\tlast = create_ref_entry(refname, &oid, flag);\n \t\t\tif (peeled == PEELED_FULLY ||\n \t\t\t    (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n \t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n@@ -415,8 +415,12 @@ static void add_packed_ref(struct files_ref_store *refs,\n \n \tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tdie(\"BUG: packed refs not locked\");\n+\n+\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n+\t\tdie(\"Reference has invalid format: '%s'\", refname);\n+\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n-\t\t      create_ref_entry(refname, oid, REF_ISPACKED, 1));\n+\t\t      create_ref_entry(refname, oid, REF_ISPACKED));\n }\n \n /*\n@@ -493,7 +497,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,\n \t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n \t\t\t}\n \t\t\tadd_entry_to_dir(dir,\n-\t\t\t\t\t create_ref_entry(refname.buf, &oid, flag, 0));\n+\t\t\t\t\t create_ref_entry(refname.buf, &oid, flag));\n \t\t}\n \t\tstrbuf_setlen(&refname, dirnamelen);\n \t\tstrbuf_setlen(&path, path_baselen);\n@@ -1541,7 +1545,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\toidcpy(&packed_entry->u.value.oid, iter->oid);\n \t\t} else {\n \t\t\tpacked_entry = create_ref_entry(iter->refname, iter->oid,\n-\t\t\t\t\t\t\tREF_ISPACKED, 0);\n+\t\t\t\t\t\t\tREF_ISPACKED);\n \t\t\tadd_ref_entry(packed_refs, packed_entry);\n \t\t}\n \t\toidclr(&packed_entry->u.value.peeled);\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 6b11d9cd12..ec97f3a38a 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -32,14 +32,10 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)\n }\n \n struct ref_entry *create_ref_entry(const char *refname,\n-\t\t\t\t   const struct object_id *oid, int flag,\n-\t\t\t\t   int check_name)\n+\t\t\t\t   const struct object_id *oid, int flag)\n {\n \tstruct ref_entry *ref;\n \n-\tif (check_name &&\n-\t    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n-\t\tdie(\"Reference has invalid format: '%s'\", refname);\n \tFLEX_ALLOC_STR(ref, name, refname);\n \toidcpy(&ref->u.value.oid, oid);\n \toidclr(&ref->u.value.peeled);\ndiff --git a/refs/ref-cache.h b/refs/ref-cache.h\nindex fbfee7ce79..794f000fd3 100644\n--- a/refs/ref-cache.h\n+++ b/refs/ref-cache.h\n@@ -185,8 +185,7 @@ struct ref_entry *create_dir_entry(struct ref_cache *cache,\n \t\t\t\t   int incomplete);\n \n struct ref_entry *create_ref_entry(const char *refname,\n-\t\t\t\t   const struct object_id *oid, int flag,\n-\t\t\t\t   int check_name);\n+\t\t\t\t   const struct object_id *oid, int flag);\n \n /*\n  * Return a pointer to a new `ref_cache`. Its top-level starts out\n-- \n2.11.0\n\n"},{"id":"320441","messageId":"e6eab70183ce8016efd9763d6dfe0892b91dc53b.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 24/25] ref-filter: limit traversal to prefix","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:54Z","receivedAt":"2017-05-22T14:19:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nWhen we are matching refnames against a pattern, then we know that the\nbeginning of any refname that can match the pattern has to match the\npart of the pattern up to the first glob character. For example, if\nthe pattern is `refs/heads/foo*bar`, then it can only match a\nreference that has the prefix `refs/heads/foo`.\n\nSo pass that prefix to `for_each_fullref_in()`. This lets the ref code\navoid passing us the full set of refs, and in some cases avoid reading\nthem in the first place.\n\nNote that this applies only when the `match_as_path` flag is set\n(i.e., when `for-each-ref` is the caller), as the matching rules for\ngit-branch and git-tag are subtly different.\n\nThis could be generalized to the case of multiple patterns, but (a) it\nprobably doesn't come up that often, and (b) it is more awkward to\ndeal with multiple patterns (e.g., the patterns might not be\ndisjoint). So, since this is just an optimization, punt on the case of\nmultiple patterns.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n ref-filter.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6cc93dcd9f..25ca56d62f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1665,6 +1665,68 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)\n \treturn match_pattern(filter, refname);\n }\n \n+/*\n+ * Find the longest prefix of pattern we can pass to\n+ * `for_each_fullref_in()`, namely the part of pattern preceding the\n+ * first glob character. (Note that `for_each_fullref_in()` is\n+ * perfectly happy working with a prefix that doesn't end at a\n+ * pathname component boundary.)\n+ */\n+static void find_longest_prefix(struct strbuf *out, const char *pattern)\n+{\n+\tconst char *p;\n+\n+\tfor (p = pattern; *p && !is_glob_special(*p); p++)\n+\t\t;\n+\n+\tstrbuf_add(out, pattern, p - pattern);\n+}\n+\n+/*\n+ * This is the same as for_each_fullref_in(), but it tries to iterate\n+ * only over the patterns we'll care about. Note that it _doesn't_ do a full\n+ * pattern match, so the callback still has to match each ref individually.\n+ */\n+static int for_each_fullref_in_pattern(struct ref_filter *filter,\n+\t\t\t\t       each_ref_fn cb,\n+\t\t\t\t       void *cb_data,\n+\t\t\t\t       int broken)\n+{\n+\tstruct strbuf prefix = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!filter->match_as_path) {\n+\t\t/*\n+\t\t * in this case, the patterns are applied after\n+\t\t * prefixes like \"refs/heads/\" etc. are stripped off,\n+\t\t * so we have to look at everything:\n+\t\t */\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t}\n+\n+\tif (!filter->name_patterns[0]) {\n+\t\t/* no patterns; we have to look at everything */\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t}\n+\n+\tif (filter->name_patterns[1]) {\n+\t\t/*\n+\t\t * multiple patterns; in theory this could still work as long\n+\t\t * as the patterns are disjoint. We'd just make multiple calls\n+\t\t * to for_each_ref(). But if they're not disjoint, we'd end up\n+\t\t * reporting the same ref multiple times. So let's punt on that\n+\t\t * for now.\n+\t\t */\n+\t\treturn for_each_fullref_in(\"\", cb, cb_data, broken);\n+\t}\n+\n+\tfind_longest_prefix(&prefix, filter->name_patterns[0]);\n+\n+\tret = for_each_fullref_in(prefix.buf, cb, cb_data, broken);\n+\tstrbuf_release(&prefix);\n+\treturn ret;\n+}\n+\n /*\n  * Given a ref (sha1, refname), check if the ref belongs to the array\n  * of sha1s. If the given ref is a tag, check if the given tag points\n@@ -1911,7 +1973,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\telse if (filter->kind == FILTER_REFS_TAGS)\n \t\t\tret = for_each_fullref_in(\"refs/tags/\", ref_filter_handler, &ref_cbdata, broken);\n \t\telse if (filter->kind & FILTER_REFS_ALL)\n-\t\t\tret = for_each_fullref_in(\"\", ref_filter_handler, &ref_cbdata, broken);\n+\t\t\tret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken);\n \t\tif (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))\n \t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n \t}\n-- \n2.11.0\n\n"},{"id":"320442","messageId":"0d6a608a62026352c1496239bab02122f123f2e1.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:55Z","receivedAt":"2017-05-22T14:19:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When iterating over references, reference priming is used to make sure\nthat loose references are read into the ref-cache before packed\nreferences, to avoid races. It used to be that the prefix passed to\nreference iterators almost always ended in `/`, for example\n`refs/heads/`. In that case, the priming code would read all loose\nreferences under `find_containing_dir(\"refs/heads/\")`, which is\n\"refs/heads/\". That's just what we want.\n\nBut now that `ref-filter` knows how to pass refname prefixes to\n`for_each_fullref_in()`, the prefix might come from user input; for\nexample,\n\n    git for-each-ref refs/heads\n\nSince the argument doesn't include a trailing slash, the reference\niteration code would prime all of the loose references under\n`find_containing_dir(\"refs/heads\")`, which is \"refs/\". Thus we would\nunnecessarily read tags, remote-tracking references, etc., when the\nuser is only interested in branches.\n\nIt is a bit awkward to get around this problem. We can't just append a\nslash to the argument, because we don't know ab initio whether an\nargument like `refs/tags/release` corresponds to a single tag or to a\ndirectory containing tags.\n\nMoreover, until now a `prefix_ref_iterator` was used to make the final\ndecision about which references fall within the prefix (the\n`cache_ref_iterator` only did a rough cut). This is also inefficient,\nbecause the `prefix_ref_iterator` can't know, for example, that while\nyou are in a subdirectory that is completely within the prefix, you\ndon't have to do the prefix check.\n\nSo:\n\n* Move the responsibility for doing the prefix check directly to\n  `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`\n  never has to wrap its return value in a `prefix_ref_iterator`.\n\n* Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be\n  stricter about what they iterate over and what directories they\n  prime.\n\n* Teach `cache_ref_iterator` to keep track of whether the current\n  `cache_ref_iterator_level` is fully within the prefix. If so, skip\n  the prefix checks entirely.\n\nThe main benefit of these optimizations is for loose references, since\npacked references are always read all at once.\n\nNote that after this change, `prefix_ref_iterator` is only ever used\nfor its trimming feature and not for its \"prefix\" feature. But I'm not\nripping out the latter yet, because it might be useful for another\npatch series that I'm working on.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/ref-cache.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 84 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex ec97f3a38a..fda3942dbe 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -312,11 +312,42 @@ static void sort_ref_dir(struct ref_dir *dir)\n \tdir->sorted = dir->nr = i;\n }\n \n+enum prefix_state {\n+\t/* All refs within the directory would match prefix: */\n+\tPREFIX_CONTAINS_DIR,\n+\n+\t/* Some, but not all, refs within the directory might match prefix: */\n+\tPREFIX_WITHIN_DIR,\n+\n+\t/* No refs within the directory could possibly match prefix: */\n+\tPREFIX_EXCLUDES_DIR\n+};\n+\n /*\n- * Load all of the refs from `dir` (recursively) into our in-memory\n- * cache.\n+ * Return a `prefix_state` constant describing the relationship\n+ * between the directory with the specified `dirname` and `prefix`.\n  */\n-static void prime_ref_dir(struct ref_dir *dir)\n+static enum prefix_state overlaps_prefix(const char *dirname,\n+\t\t\t\t\t const char *prefix)\n+{\n+\twhile (*prefix && *dirname == *prefix) {\n+\t\tdirname++;\n+\t\tprefix++;\n+\t}\n+\tif (!*prefix)\n+\t\treturn PREFIX_CONTAINS_DIR;\n+\telse if (!*dirname)\n+\t\treturn PREFIX_WITHIN_DIR;\n+\telse\n+\t\treturn PREFIX_EXCLUDES_DIR;\n+}\n+\n+/*\n+ * Load all of the refs from `dir` (recursively) that could possibly\n+ * contain references matching `prefix` into our in-memory cache. If\n+ * `prefix` is NULL, prime unconditionally.\n+ */\n+static void prime_ref_dir(struct ref_dir *dir, const char *prefix)\n {\n \t/*\n \t * The hard work of loading loose refs is done by get_ref_dir(), so we\n@@ -327,8 +358,29 @@ static void prime_ref_dir(struct ref_dir *dir)\n \tint i;\n \tfor (i = 0; i < dir->nr; i++) {\n \t\tstruct ref_entry *entry = dir->entries[i];\n-\t\tif (entry->flag & REF_DIR)\n-\t\t\tprime_ref_dir(get_ref_dir(entry));\n+\t\tif (!(entry->flag & REF_DIR)) {\n+\t\t\t/* Not a directory; no need to recurse. */\n+\t\t} else if (!prefix) {\n+\t\t\t/* Recurse in any case: */\n+\t\t\tprime_ref_dir(get_ref_dir(entry), NULL);\n+\t\t} else {\n+\t\t\tswitch (overlaps_prefix(entry->name, prefix)) {\n+\t\t\tcase PREFIX_CONTAINS_DIR:\n+\t\t\t\t/*\n+\t\t\t\t * Recurse, and from here down we\n+\t\t\t\t * don't have to check the prefix\n+\t\t\t\t * anymore:\n+\t\t\t\t */\n+\t\t\t\tprime_ref_dir(get_ref_dir(entry), NULL);\n+\t\t\t\tbreak;\n+\t\t\tcase PREFIX_WITHIN_DIR:\n+\t\t\t\tprime_ref_dir(get_ref_dir(entry), prefix);\n+\t\t\t\tbreak;\n+\t\t\tcase PREFIX_EXCLUDES_DIR:\n+\t\t\t\t/* No need to prime this directory. */\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n }\n \n@@ -343,6 +395,8 @@ struct cache_ref_iterator_level {\n \t */\n \tstruct ref_dir *dir;\n \n+\tenum prefix_state prefix_state;\n+\n \t/*\n \t * The index of the current entry within dir (which might\n \t * itself be a directory). If index == -1, then the iteration\n@@ -369,6 +423,13 @@ struct cache_ref_iterator {\n \t/* The number of levels that have been allocated on the stack */\n \tsize_t levels_alloc;\n \n+\t/*\n+\t * Only include references with this prefix in the iteration.\n+\t * The prefix is matched textually, without regard for path\n+\t * component boundaries.\n+\t */\n+\tconst char *prefix;\n+\n \t/*\n \t * A stack of levels. levels[0] is the uppermost level that is\n \t * being iterated over in this iteration. (This is not\n@@ -390,6 +451,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\t&iter->levels[iter->levels_nr - 1];\n \t\tstruct ref_dir *dir = level->dir;\n \t\tstruct ref_entry *entry;\n+\t\tenum prefix_state entry_prefix_state;\n \n \t\tif (level->index == -1)\n \t\t\tsort_ref_dir(dir);\n@@ -404,6 +466,14 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tentry = dir->entries[level->index];\n \n+\t\tif (level->prefix_state == PREFIX_WITHIN_DIR) {\n+\t\t\tentry_prefix_state = overlaps_prefix(entry->name, iter->prefix);\n+\t\t\tif (entry_prefix_state == PREFIX_EXCLUDES_DIR)\n+\t\t\t\tcontinue;\n+\t\t} else {\n+\t\t\tentry_prefix_state = level->prefix_state;\n+\t\t}\n+\n \t\tif (entry->flag & REF_DIR) {\n \t\t\t/* push down a level */\n \t\t\tALLOC_GROW(iter->levels, iter->levels_nr + 1,\n@@ -411,6 +481,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\t\tlevel = &iter->levels[iter->levels_nr++];\n \t\t\tlevel->dir = get_ref_dir(entry);\n+\t\t\tlevel->prefix_state = entry_prefix_state;\n \t\t\tlevel->index = -1;\n \t\t} else {\n \t\t\titer->base.refname = entry->name;\n@@ -496,10 +567,10 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \t\tdir = find_containing_dir(dir, prefix, 0);\n \tif (!dir)\n \t\t/* There's nothing to iterate over. */\n-\t\treturn  empty_ref_iterator_begin();\n+\t\treturn empty_ref_iterator_begin();\n \n \tif (prime_dir)\n-\t\tprime_ref_dir(dir);\n+\t\tprime_ref_dir(dir, prefix);\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n@@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \tlevel->index = -1;\n \tlevel->dir = dir;\n \n-\tif (prefix && *prefix)\n-\t\tref_iterator = prefix_ref_iterator_begin(ref_iterator,\n-\t\t\t\t\t\t\t prefix, 0);\n+\tif (prefix && *prefix) {\n+\t\titer->prefix = xstrdup(prefix);\n+\t\tlevel->prefix_state = PREFIX_WITHIN_DIR;\n+\t} else {\n+\t\tlevel->prefix_state = PREFIX_CONTAINS_DIR;\n+\t}\n \n \treturn ref_iterator;\n }\n-- \n2.11.0\n\n"},{"id":"320443","messageId":"2b8f4e4bbfaca8f6d3d828bcb0e7ff1779778061.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 21/25] read_packed_refs(): report unexpected fopen() failures","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:51Z","receivedAt":"2017-05-22T14:19:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code ignored any errors encountered when trying to fopen the\n\"packed-refs\" file, treating all such failures as if the file didn't\nexist. But it could be that there is some other error opening the\nfile (e.g., permissions problems), and we don't want to silently\nignore such problems. So report any failures that are not due to\nENOENT.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b4fa745cd7..dbfd03f989 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -251,8 +251,18 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n \n \tf = fopen(packed_refs_file, \"r\");\n-\tif (!f)\n-\t\treturn packed_refs;\n+\tif (!f) {\n+\t\tif (errno == ENOENT) {\n+\t\t\t/*\n+\t\t\t * This is OK; it just means that no\n+\t\t\t * \"packed-refs\" file has been written yet,\n+\t\t\t * which is equivalent to it being empty.\n+\t\t\t */\n+\t\t\treturn packed_refs;\n+\t\t} else {\n+\t\t\tdie_errno(\"couldn't read %s\", packed_refs_file);\n+\t\t}\n+\t}\n \n \tstat_validity_update(&packed_refs->validity, fileno(f));\n \n-- \n2.11.0\n\n"},{"id":"320444","messageId":"5705444f57e334c451f3badd710ee7672b2488a1.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 13/25] ref_transaction_commit(): check for valid `transaction->state`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:43Z","receivedAt":"2017-05-22T14:19:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the check that `transaction->state` is valid from\n`files_transaction_commit()` to `ref_transaction_commit()`, where\nother future reference backends can benefit from it as well.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c               | 12 ++++++++++++\n refs/files-backend.c |  3 ---\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 989462c972..f8f41ffb04 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1694,6 +1694,18 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n {\n \tstruct ref_store *refs = transaction->ref_store;\n \n+\tswitch (transaction->state) {\n+\tcase REF_TRANSACTION_OPEN:\n+\t\t/* Good. */\n+\t\tbreak;\n+\tcase REF_TRANSACTION_CLOSED:\n+\t\tdie(\"BUG: prepare called on a closed reference transaction\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: unexpected reference transaction state\");\n+\t\tbreak;\n+\t}\n+\n \tif (getenv(GIT_QUARANTINE_ENVIRONMENT)) {\n \t\tstrbuf_addstr(err,\n \t\t\t      _(\"ref updates forbidden inside quarantine environment\"));\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2c70de5209..a4261d4683 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2874,9 +2874,6 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \n \tassert(err);\n \n-\tif (transaction->state != REF_TRANSACTION_OPEN)\n-\t\tdie(\"BUG: commit called for transaction that is not open\");\n-\n \tif (!transaction->nr)\n \t\tgoto cleanup;\n \n-- \n2.11.0\n\n"},{"id":"320445","messageId":"b7786f0cc9664fe86580f4296b4916f450b068d0.1495460199.git.mhagger@alum.mit.edu","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"[PATCH v2 18/25] should_pack_ref(): new function, extracted from `files_pack_refs()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-22T14:17:48Z","receivedAt":"2017-05-22T14:19:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract a function for deciding whether a reference should be packed.\nIt is a self-contained bit of logic, so splitting it out improves\nreadability.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 42 ++++++++++++++++++++++++++++--------------\n 1 file changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8d0ce739a6..29514392b0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1455,6 +1455,32 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r)\n \t}\n }\n \n+/*\n+ * Return true if the specified reference should be packed.\n+ */\n+static int should_pack_ref(const char *refname,\n+\t\t\t   const struct object_id *oid, unsigned int ref_flags,\n+\t\t\t   unsigned int pack_flags)\n+{\n+\t/* Do not pack per-worktree refs: */\n+\tif (ref_type(refname) != REF_TYPE_NORMAL)\n+\t\treturn 0;\n+\n+\t/* Do not pack non-tags unless PACK_REFS_ALL is set: */\n+\tif (!(pack_flags & PACK_REFS_ALL) && !starts_with(refname, \"refs/tags/\"))\n+\t\treturn 0;\n+\n+\t/* Do not pack symbolic refs: */\n+\tif (ref_flags & REF_ISSYMREF)\n+\t\treturn 0;\n+\n+\t/* Do not pack broken refs: */\n+\tif (!ref_resolves_to_object(refname, oid, ref_flags))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n {\n \tstruct files_ref_store *refs =\n@@ -1476,21 +1502,9 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n \t\tstruct ref_entry *packed_entry;\n-\t\tint is_tag_ref = starts_with(iter->refname, \"refs/tags/\");\n-\n-\t\t/* Do not pack per-worktree refs: */\n-\t\tif (ref_type(iter->refname) != REF_TYPE_NORMAL)\n-\t\t\tcontinue;\n-\n-\t\t/* ALWAYS pack tags */\n-\t\tif (!(flags & PACK_REFS_ALL) && !is_tag_ref)\n-\t\t\tcontinue;\n-\n-\t\t/* Do not pack symbolic or broken refs: */\n-\t\tif (iter->flags & REF_ISSYMREF)\n-\t\t\tcontinue;\n \n-\t\tif (!ref_resolves_to_object(iter->refname, iter->oid, iter->flags))\n+\t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n+\t\t\t\t     flags))\n \t\t\tcontinue;\n \n \t\t/*\n-- \n2.11.0\n\n"},{"id":"320484","messageId":"CAGZ79kYC-nn9iY+mw6fAdBXKH_WBhJ0dLxg66BnH2pfF3L+9-g@mail.gmail.com","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/25] Prepare to separate out a packed_ref_store","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-22T21:51:33Z","receivedAt":"2017-05-22T21:51:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 22, 2017 at 7:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> This is the second iteration of a patch series that prepares the\n> ground for separating out a `packed_ref_store` and then for changing\n> `packed-refs` to be read using `mmap()`. Thanks to Peff, Junio,\n> Stefan, Brandon, and Johannes for their feedback about v1 [1]. I think\n> I have addressed all of your comments.\n>\n> Changes since v1:\n>\n> * Since v1, branch `bc/object-id` has been merged to `next`, and it\n>   has lots of conflicts with these changes. So I rebased this branch\n>   onto a merge of `master` and `bc/object-id`. (I hope this makes\n>   Junio's job easier.) This unfortunately causes a bit of tbdiff noise\n>   between v1 and v2.\n>\n> * Patch [01/25]: in t3600, register the `test_when_finished` command\n>   before executing `chmod a-w`.\n>\n> * Patch [04/25] (new patch): convert a few `die(\"internal error: ...\")`\n>   to `die(\"BUG: ...\")`.\n>\n> * Patch [05/25]: Use `strlen()` rather than `memchr()` to check the\n>   trim length, and `die()` rather than skipping if it is longer than\n>   the reference name.\n>\n> * Patch [08/25]: Name the log message arguments `msg` for consistency\n>   with existing code.\n>\n> * Patch [10/25]: Rename the new member from `packlock` to\n>   `packed_refs_lock`.\n>\n> * Patch [13/25] (new patch): Move the check for valid\n>   `transaction->state` from `files_transaction_commit()` to\n>   `ref_transaction_commit()`.\n>\n> * Patch [14/25]:\n>\n>   * Add more sanity checks of `transaction->state`.\n>\n>   * Don't add `ref_transaction_finish()` to the public API. Instead,\n>     teach `ref_transaction_commit()` to do the right thing whether or\n>     not `ref_transaction_prepare()` has been called.\n>\n>   * Add and improve docstrings.\n>\n>   * Allow `ref_transaction_abort()` to be called even before\n>     `ref_transaction_prepare()` (in which case it just calls\n>     `ref_transaction_free()`).\n>\n> * Lots of improvements to commit messages and comments, mostly to\n>   clarify points that reviewers asked about.\n>\n> These changes (along with the merge commit that they are based on) are\n> also available as branch `packed-ref-store-prep` in my GitHub fork\n> [2]. If you'd like to see a preview of the rest of the changes (which\n> works but is not yet polished), checkout the `mmap-packed-refs` branch\n> from the same place.\n\nI have read this version and quite like it. I did not have any nits to remark.\n\nThanks,\nStefan\n"},{"id":"320592","messageId":"20170523194519.luhuej3rerzb57t7@sigill.intra.peff.net","threadId":"46021","inReplyTo":"0d6a608a62026352c1496239bab02122f123f2e1.1495460199.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-23T19:45:20Z","receivedAt":"2017-05-23T19:45:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:\n\n> So:\n> \n> * Move the responsibility for doing the prefix check directly to\n>   `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`\n>   never has to wrap its return value in a `prefix_ref_iterator`.\n> \n> * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be\n>   stricter about what they iterate over and what directories they\n>   prime.\n> \n> * Teach `cache_ref_iterator` to keep track of whether the current\n>   `cache_ref_iterator_level` is fully within the prefix. If so, skip\n>   the prefix checks entirely.\n\nAs promised, I came back to this one with a more careful eye. These\nchanges all make sense to me, and the implementation matches.\n\nMy only question is:\n\n> @@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n>  \tlevel->index = -1;\n>  \tlevel->dir = dir;\n>  \n> -\tif (prefix && *prefix)\n> -\t\tref_iterator = prefix_ref_iterator_begin(ref_iterator,\n> -\t\t\t\t\t\t\t prefix, 0);\n> +\tif (prefix && *prefix) {\n> +\t\titer->prefix = xstrdup(prefix);\n> +\t\tlevel->prefix_state = PREFIX_WITHIN_DIR;\n> +\t} else {\n> +\t\tlevel->prefix_state = PREFIX_CONTAINS_DIR;\n> +\t}\n\nWho frees the prefix? Does this need:\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex fda3942db..a3efc5c51 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \tstruct cache_ref_iterator *iter =\n \t\t(struct cache_ref_iterator *)ref_iterator;\n \n+\tfree(iter->prefix);\n \tfree(iter->levels);\n \tbase_ref_iterator_free(ref_iterator);\n \treturn ITER_DONE;\n\n-Peff\n"},{"id":"320612","messageId":"xmqq7f17ht0x.fsf@gitster.mtv.corp.google.com","threadId":"46021","inReplyTo":"cover.1495460199.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/25] Prepare to separate out a packed_ref_store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T02:45:34Z","receivedAt":"2017-05-24T02:45:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> * Since v1, branch `bc/object-id` has been merged to `next`, and it\n>   has lots of conflicts with these changes. So I rebased this branch\n>   onto a merge of `master` and `bc/object-id`. (I hope this makes\n>   Junio's job easier.) This unfortunately causes a bit of tbdiff noise\n>   between v1 and v2.\n\nHeh, that gives me an excellent excuse to procrastinate, as I am\nplanning to merge that topic to 'master' by the end of this week ;-)\n\nJokes aside...\n\n> * Patch [01/25]: in t3600, register the `test_when_finished` command\n>   before executing `chmod a-w`.\n>\n> * Patch [04/25] (new patch): convert a few `die(\"internal error: ...\")`\n>   to `die(\"BUG: ...\")`.\n>\n> * Patch [05/25]: Use `strlen()` rather than `memchr()` to check the\n>   trim length, and `die()` rather than skipping if it is longer than\n>   the reference name.\n>\n> * Patch [08/25]: Name the log message arguments `msg` for consistency\n>   with existing code.\n>\n> * Patch [10/25]: Rename the new member from `packlock` to\n>   `packed_refs_lock`.\n>\n> * Patch [13/25] (new patch): Move the check for valid\n>   `transaction->state` from `files_transaction_commit()` to\n>   `ref_transaction_commit()`.\n\nAll of these feel familiar ;-)\n\n> * Patch [14/25]:\n>\n>   * Add more sanity checks of `transaction->state`.\n>\n>   * Don't add `ref_transaction_finish()` to the public API. Instead,\n>     teach `ref_transaction_commit()` to do the right thing whether or\n>     not `ref_transaction_prepare()` has been called.\n>\n>   * Add and improve docstrings.\n>\n>   * Allow `ref_transaction_abort()` to be called even before\n>     `ref_transaction_prepare()` (in which case it just calls\n>     `ref_transaction_free()`).\n>\n> * Lots of improvements to commit messages and comments, mostly to\n>   clarify points that reviewers asked about.\n\nOverall this looked like quite a quality work to me.\n\n> These changes (along with the merge commit that they are based on) are\n> also available as branch `packed-ref-store-prep` in my GitHub fork\n> [2]. If you'd like to see a preview of the rest of the changes (which\n> works but is not yet polished), checkout the `mmap-packed-refs` branch\n> from the same place.\n\nThanks.\n"},{"id":"320653","messageId":"33ffee8b-22da-ced4-95c6-ab68d4557f1d@alum.mit.edu","threadId":"46021","inReplyTo":"20170523194519.luhuej3rerzb57t7@sigill.intra.peff.net","subject":"Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-24T07:17:20Z","receivedAt":"2017-05-24T07:17:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/23/2017 09:45 PM, Jeff King wrote:\n> On Mon, May 22, 2017 at 04:17:55PM +0200, Michael Haggerty wrote:\n> \n>> So:\n>>\n>> * Move the responsibility for doing the prefix check directly to\n>>   `cache_ref_iterator`. This means that `cache_ref_iterator_begin()`\n>>   never has to wrap its return value in a `prefix_ref_iterator`.\n>>\n>> * Teach `cache_ref_iterator_begin()` (and `prime_ref_dir()`) to be\n>>   stricter about what they iterate over and what directories they\n>>   prime.\n>>\n>> * Teach `cache_ref_iterator` to keep track of whether the current\n>>   `cache_ref_iterator_level` is fully within the prefix. If so, skip\n>>   the prefix checks entirely.\n> \n> As promised, I came back to this one with a more careful eye. These\n> changes all make sense to me, and the implementation matches.\n> \n> My only question is:\n> \n>> @@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n>>  \tlevel->index = -1;\n>>  \tlevel->dir = dir;\n>>  \n>> -\tif (prefix && *prefix)\n>> -\t\tref_iterator = prefix_ref_iterator_begin(ref_iterator,\n>> -\t\t\t\t\t\t\t prefix, 0);\n>> +\tif (prefix && *prefix) {\n>> +\t\titer->prefix = xstrdup(prefix);\n>> +\t\tlevel->prefix_state = PREFIX_WITHIN_DIR;\n>> +\t} else {\n>> +\t\tlevel->prefix_state = PREFIX_CONTAINS_DIR;\n>> +\t}\n> \n> Who frees the prefix? Does this need:\n> \n> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n> index fda3942db..a3efc5c51 100644\n> --- a/refs/ref-cache.c\n> +++ b/refs/ref-cache.c\n> @@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\n>  \tstruct cache_ref_iterator *iter =\n>  \t\t(struct cache_ref_iterator *)ref_iterator;\n>  \n> +\tfree(iter->prefix);\n>  \tfree(iter->levels);\n>  \tbase_ref_iterator_free(ref_iterator);\n>  \treturn ITER_DONE;\n\nYes, you are right. Thanks for catching this.\n\nNote: it has to be\n\n        free((char *)iter->prefix);\n\nbecause `prefix` is const.\n\nJunio, if a reroll is not needed for other reasons, would you mind\nsquashing this into the last patch of my series?\n\nMichael\n\n"},{"id":"320654","messageId":"xmqqwp96g1pa.fsf@gitster.mtv.corp.google.com","threadId":"46021","inReplyTo":"33ffee8b-22da-ced4-95c6-ab68d4557f1d@alum.mit.edu","subject":"Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T07:21:05Z","receivedAt":"2017-05-24T07:21:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Junio, if a reroll is not needed for other reasons, would you mind\n> squashing this into the last patch of my series?\n\nWill do, but won't happen until morning in Tokyo.  I've just\nfinished today's integration cycle.\n\nThanks.\n"}]}