{"thread":{"id":"45990","subject":"[PATCH 00/23] Prepare to separate out a packed_ref_store","startedAt":"2017-05-17T12:06:02Z","lastAt":"2017-05-19T10:02:48Z","messageCount":73,"participants":["Michael Haggerty","Jeff King","Stefan Beller","Brandon Williams","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":23},"messages":[{"id":"320037","messageId":"cover.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":null,"subject":"[PATCH 00/23] Prepare to separate out a packed_ref_store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:23Z","receivedAt":"2017-05-17T12:06:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series is the next leg on a journey towards reading\n`packed-refs` using `mmap()`, the most interesting aspect of which is\nthat we will often be able to avoid having to read the whole\n`packed-refs` file if we only need a subset of references.\n\nThe first leg of the journey was separating out the reference cache\ninto a separate module [1]. That branch is already merged to master.\n\nThis patch series prepares the ground for separating out a\n`packed_ref_store`, but doesn't yet take that step. (As you can see,\nit's a long enough patch series already!) It's kind of a grab bag of\ncleanup patches plus work to decouple the packed-refs handling code\nfrom the rest of `files_ref_store`. Some highlights:\n\n* Patch 07/23 adds a log message parameter to `refs_delete_refs()` and\n `delete_refs()`, for consistency with other reference-changing\n operations. Even though `files_ref_store` is incapable of storing\n reflogs for deleted references, that is no reason that the API\n shouldn't admit the possibility for future backends.\n\n* Patch 12/23 breaks `ref_transaction_commit()` into multiple\n functions:\n\n * `ref_transaction_prepare()`: do pre-checks, obtain locks, etc.; do\n everything possible to make sure that the reference update will be\n successful.\n\n * `ref_transaction_finish()`: actually commit a prepared\n transaction.\n\n * `ref_transaction_abort()`: abort a prepared transaction.\n\n This separation will be useful for supporting a \"compound\" reference\n store composed of multiple reference stores that work together\n (i.e., one for loose refs and one for packed refs).\n\n* Patch 17/23 changes `get_packed_ref_cache()` to skip `lstat()`ing\n the packed-refs file (to check its freshness) if we already have it\n locked.\n\n* Patch 19/23 fixes the error handling in `read_packed_refs()`: if\n `fopen()` fails due to `ENOENT`, then there are no packed refs. But\n if it fails for another reason, that is a problem that should be\n reported to the user.\n\n* Patch 22/23 (by Peff) changes ref-filter to attempt to limit the\n reference traversal to a prefix, if there is a single\n `match_as_path` pattern that starts with non-glob characters. This\n limits the number of loose references that need to be read when\n processing a command like `git for-each-ref refs/heads/`.\n\n* Patch 23/23 makes `cache_ref_iterator` smarter about avoiding\n \"priming\" directories of loose references that won't be needed,\n further reducing the number of loose references that need to be read\n in some cases of iterating over references.\n\nThese changes are also available as branch `packed-ref-store-prep` in\nmy GitHub fork [2]. If you'd like to see a preview of the rest of the\nchanges (which works but is not yet polished), checkout the\n`mmap-packed-refs` branch from the same place.\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1490026594.git.mhagger@alum.mit.edu/\n http://public-inbox.org/git/cover.1490966385.git.mhagger@alum.mit.edu/\n http://public-inbox.org/git/cover.1492323985.git.mhagger@alum.mit.edu/\n\n[2] https://github.com/mhagger/git\n\nJeff King (1):\n ref-filter: limit traversal to prefix\n\nMichael Haggerty (22):\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 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 `logmsg` 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(): break into multiple functions\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 | 62 ++++++++-\n refs.c | 83 ++++++++++--\n refs.h | 62 ++++++++-\n refs/files-backend.c | 300 +++++++++++++++++++++++++----------------\n refs/iterator.c | 14 +-\n refs/ref-cache.c | 99 +++++++++++---\n refs/ref-cache.h | 6 +-\n refs/refs-internal.h | 56 ++++++--\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, 538 insertions(+), 169 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"320038","messageId":"bb244c72ec6e6ab56171d82f7379a75c4eb5481f.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 01/23] t3600: clean up permissions test properly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:24Z","receivedAt":"2017-05-17T12:06:04Z","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_must_fail` block so that it can't\nbe skipped.\n\nSigned-off-by: Michael Haggerty \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..4a35c378c8 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -98,8 +98,8 @@ embedded'\"\n \n test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n \tchmod a-w . &&\n-\ttest_must_fail git rm -f baz &&\n-\tchmod 775 .\n+\ttest_when_finished \"chmod 775 .\" &&\n+\ttest_must_fail git rm -f baz\n '\n \n test_expect_success \\\n-- \n2.11.0\n\n"},{"id":"320039","messageId":"f56871ab01d7a9b07631f143ae902605a1e247f4.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 02/23] refs.h: clarify docstring for the ref_transaction_update()-related fns","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:25Z","receivedAt":"2017-05-17T12:06:07Z","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 \n---\n refs.h | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/refs.h b/refs.h\nindex d18ef47128..a7d7b1afdf 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":"320040","messageId":"da4669a209fa1aafe6a8e3ae8082963a338b1da8.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 03/23] ref_iterator_begin_fn(): fix docstring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:26Z","receivedAt":"2017-05-17T12:06:10Z","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 \n---\n refs/refs-internal.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 12cf4e4718..e5f6bb2047 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -515,8 +515,8 @@ 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+ * Iterate over the references in the specified ref_store whose names\n+ * start with the specified prefix. If prefix is NULL or the empty\n * string, iterate over all references in the submodule.\n */\n typedef struct ref_iterator *ref_iterator_begin_fn(\n-- \n2.11.0\n\n"},{"id":"320041","messageId":"c9e1d561825a63630a09a2815b9b296c9e4bd1f5.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:28Z","receivedAt":"2017-05-17T12:06:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The backend already takes care of the prefix. By passing the prefix\nagain to `prefix_ref_iterator`, we were forcing that iterator to do\nredundant prefix comparisons. So set it to the empty string.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 26d40f9927..f4a485cd8a 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":"320042","messageId":"b26ccc03a7e72dfa30467db81e9210dd621516e2.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 07/23] ref_store: take `logmsg` parameter when deleting references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:30Z","receivedAt":"2017-05-17T12:06:16Z","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. Let's add them now before the interface\nbecomes truly polymorphic and increases the work.\n\nSigned-off-by: Michael Haggerty \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 5f2c2ab23e..f51314f432 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -940,7 +940,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 ea8233c67d..14dec88e7f 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 a7d7b1afdf..b2d9626be6 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 893029f9dc..7708994e82 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1607,7 +1607,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@@ -1639,7 +1639,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 e5f6bb2047..9ed4387c8c 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..3eb78467c4 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 *logmsg = *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, logmsg, &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":"320043","messageId":"eecc015af8d7ed71223b591b13847fdb56ee69f0.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:27Z","receivedAt":"2017-05-17T12:06:17Z","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. Skip over any references whose names\nare not longer than `trim`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/iterator.c | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex bce1f192f7..f33d1b3a39 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -292,7 +292,19 @@ 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 * If there wouldn't be at least one character\n+\t\t\t * left in the refname after trimming, skip\n+\t\t\t * over it:\n+\t\t\t */\n+\t\t\tif (memchr(iter->iter0->refname, '\\0', iter->trim + 1))\n+\t\t\t\tcontinue;\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":"320044","messageId":"9b52c29411fa11ac0d8227d5ae78ba2768485b3f.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 09/23] files-backend: move `lock` member to `files_ref_store`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:32Z","receivedAt":"2017-05-17T12:06:21Z","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 `packlock` to make its purpose\nclearer in its new home. More changes are coming here shortly.\n\nSigned-off-by: Michael Haggerty \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 7708994e82..e7ae5538cc 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 *packlock;\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->packlock)\n \t\t\tdie(\"internal error: packed-ref cache cleared while locked\");\n \t\trefs->packed = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n@@ -408,7 +406,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->packlock)\n \t\tdie(\"internal error: packed refs not locked\");\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n \t\t create_ref_entry(refname, sha1, REF_ISPACKED, 1));\n@@ -1312,7 +1310,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->packlock = &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@@ -1335,10 +1333,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->packlock)\n \t\tdie(\"internal error: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(packed_ref_cache->lock, \"w\");\n+\tout = fdopen_lock_file(refs->packlock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1356,11 +1354,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->packlock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n-\tpacked_ref_cache->lock = NULL;\n+\trefs->packlock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \terrno = save_errno;\n \treturn error;\n@@ -1378,10 +1376,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->packlock)\n \t\tdie(\"internal error: packed-refs not locked\");\n-\trollback_lock_file(packed_ref_cache->lock);\n-\tpacked_ref_cache->lock = NULL;\n+\trollback_lock_file(refs->packlock);\n+\trefs->packlock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(refs);\n }\n-- \n2.11.0\n\n"},{"id":"320045","messageId":"17922578c2fbec4c0b8040cb37f432f913218489.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:29Z","receivedAt":"2017-05-17T12:06:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n refs.c | 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 f4a485cd8a..ea8233c67d 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 4925e698d8..893029f9dc 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2862,7 +2862,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@@ -3069,7 +3070,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":"320046","messageId":"dd7637060bac1a27e03563edc82649812dcf897c.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:33Z","receivedAt":"2017-05-17T12:06:32Z","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 \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 e7ae5538cc..ba0ad0aa44 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 *packlock;\n+\tstruct lock_file packlock;\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->packlock)\n+\t\tif (is_lock_file_locked(&refs->packlock))\n \t\t\tdie(\"internal error: packed-ref cache cleared while locked\");\n \t\trefs->packed = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n@@ -406,7 +402,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->packlock)\n+\tif (!is_lock_file_locked(&refs->packlock))\n \t\tdie(\"internal error: packed refs not locked\");\n \tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n \t\t create_ref_entry(refname, sha1, REF_ISPACKED, 1));\n@@ -1300,7 +1296,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->packlock, files_packed_refs_path(refs),\n \t\t\t flags, timeout_value) < 0)\n \t\treturn -1;\n \t/*\n@@ -1310,7 +1306,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->packlock = &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@@ -1333,10 +1328,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->packlock)\n+\tif (!is_lock_file_locked(&refs->packlock))\n \t\tdie(\"internal error: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(refs->packlock, \"w\");\n+\tout = fdopen_lock_file(&refs->packlock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1354,11 +1349,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->packlock)) {\n+\tif (commit_lock_file(&refs->packlock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n-\trefs->packlock = NULL;\n \trelease_packed_ref_cache(packed_ref_cache);\n \terrno = save_errno;\n \treturn error;\n@@ -1376,10 +1370,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->packlock)\n+\tif (!is_lock_file_locked(&refs->packlock))\n \t\tdie(\"internal error: packed-refs not locked\");\n-\trollback_lock_file(refs->packlock);\n-\trefs->packlock = NULL;\n+\trollback_lock_file(&refs->packlock);\n \trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(refs);\n }\n-- \n2.11.0\n\n"},{"id":"320047","messageId":"5ec5f3f9b4c9fc9f719d116b48dcf7b61e74ae49.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 12/23] ref_transaction_commit(): break into multiple functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:35Z","receivedAt":"2017-05-17T12:06:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Break the function `ref_transaction_commit()` into two functions,\n`ref_transaction_prepare()` and `ref_transaction_finish()`, with a\nthird function, `ref_transaction_abort()`, that can be used to abort\nthe transaction. Break up the `ref_store` methods similarly.\n\nThis split will make it easier to implement compound reference stores,\nwhere a transaction might have to span multiple underlying stores. In\nthat case, we would first want to \"prepare\" the sub-transactions in\neach of the reference stores. If any of the \"prepare\" steps fails, we\nwould \"abort\" all of the sub-transactions. Only if all of the\n\"prepare\" steps succeed would we \"finish\" each of them.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 34 ++++++++++++++++++++++---\n refs.h | 37 ++++++++++++++++++++++++++-\n refs/files-backend.c | 71 +++++++++++++++++++++++++++++++++++++++++-----------\n refs/refs-internal.h | 42 ++++++++++++++++++++++++-------\n 4 files changed, 157 insertions(+), 27 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 14dec88e7f..689362db1e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1689,8 +1689,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@@ -1700,7 +1700,35 @@ 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_finish(struct ref_transaction *transaction,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct ref_store *refs = transaction->ref_store;\n+\n+\treturn refs->be->transaction_finish(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+\n+\treturn refs->be->transaction_abort(refs, transaction, err);\n+}\n+\n+int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t struct strbuf *err)\n+{\n+\tint ret;\n+\n+\tret = ref_transaction_prepare(transaction, err);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn ref_transaction_finish(transaction, err);\n }\n \n int refs_verify_refname_available(struct ref_store *refs,\ndiff --git a/refs.h b/refs.h\nindex b2d9626be6..4139e917f5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -525,7 +525,10 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n \n /*\n * Commit all of the changes that have been queued in transaction, as\n- * atomically as possible.\n+ * atomically as possible. This is equivalent to calling\n+ * `ref_transaction_prepare()` then `ref_transaction_finish()` (see\n+ * below), which you can call yourself if you want finer control over\n+ * reference updates.\n *\n * Returns 0 for success, or one of the below error codes for errors.\n */\n@@ -536,6 +539,38 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err);\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. Anything that this function does should be\n+ * able to be finished up by calling `ref_transaction_finish()` or\n+ * rolled back by calling `ref_transaction_abort()`.\n+ *\n+ * On success, return 0 and leave the transaction in \"prepared\" state.\n+ * On failure, abort the transaction and return one of the\n+ * `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+ * Perform the final commit of `transaction`, which has already been\n+ * prepared.\n+ */\n+int ref_transaction_finish(struct ref_transaction *transaction,\n+\t\t\t struct strbuf *err);\n+\n+/*\n+ * Abort `transaction`, which has already been prepared.\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\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7ddd4f87d5..f48409132d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2867,27 +2867,24 @@ 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 \tif (transaction->state != REF_TRANSACTION_OPEN)\n-\t\tdie(\"BUG: commit called for transaction that is not open\");\n+\t\tdie(\"BUG: prepare called for transaction that is not open\");\n \n \tif (!transaction->nr)\n \t\tgoto cleanup;\n@@ -2949,6 +2946,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@@ -2956,7 +2955,41 @@ 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->state != REF_TRANSACTION_PREPARED)\n+\t\tdie(\"BUG: finish called for transaction that is not prepared\");\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@@ -3037,7 +3070,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@@ -3054,13 +3086,22 @@ 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+\tif (transaction->state != REF_TRANSACTION_PREPARED)\n+\t\tdie(\"BUG: abort called for transaction that is not prepared\");\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@@ -3327,7 +3368,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 9ed4387c8c..2ff65d3ebd 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -185,17 +185,26 @@ 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 in a valid state and new updates can be\n+ * added to it. An OPEN transaction can be prepared or\n+ * committed.\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.\n+ *\n+ * CLOSED: The transaction is no longer active. No other operations\n+ * than free can be used on it in this state. A transaction\n+ * becomes closed if there is a failure while building the\n+ * transaction, if an active transaction is committed, or if a\n+ * prepared transaction is finished.\n */\n enum ref_transaction_state {\n \tREF_TRANSACTION_OPEN = 0,\n-\tREF_TRANSACTION_CLOSED = 1\n+\tREF_TRANSACTION_PREPARED = 1,\n+\tREF_TRANSACTION_CLOSED = 2\n };\n \n /*\n@@ -497,6 +506,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@@ -599,7 +620,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":"320048","messageId":"2e104ca5ff7112bc00e0a482b02a496585eee77d.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 16/23] should_pack_ref(): new function, extracted from `files_pack_refs()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:39Z","receivedAt":"2017-05-17T12:06:40Z","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 \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 d7596b5222..c549a8d956 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1467,6 +1467,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@@ -1488,21 +1514,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":"320049","messageId":"eedc8f85c8237515bc66584649b1e3f3d547049c.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 17/23] get_packed_ref_cache(): assume \"packed-refs\" won't change while locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:40Z","receivedAt":"2017-05-17T12:06:41Z","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 \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 c549a8d956..a910c44f38 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -354,13 +354,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->packlock) &&\n \t !stat_validity_check(&refs->packed->validity, packed_refs_file))\n \t\tclear_packed_ref_cache(refs);\n \n@@ -1300,10 +1305,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":"320050","messageId":"41dba75f36a53c963255922009463e67a8126f21.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 18/23] read_packed_refs(): do more of the work of reading packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:41Z","receivedAt":"2017-05-17T12:06:43Z","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 \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 a910c44f38..6a037e1d61 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -221,7 +221,9 @@ static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)\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@@ -247,12 +249,26 @@ static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)\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\tunsigned char sha1[20];\n \t\tconst char *refname;\n@@ -299,7 +315,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@@ -369,20 +388,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 ffdc54f3f0..83e6c2dd2a 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":"320051","messageId":"5d13d0f0e0a63430a9bf2889e9a9e911382170de.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 19/23] read_packed_refs(): report unexpected fopen() failures","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:42Z","receivedAt":"2017-05-17T12:06:44Z","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 \n---\n refs/files-backend.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6a037e1d61..eb74d1119a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -263,8 +263,19 @@ 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(\"couldn't read %s: %s\",\n+\t\t\t packed_refs_file, strerror(errno));\n+\t\t}\n+\t}\n \n \tstat_validity_update(&packed_refs->validity, fileno(f));\n \n-- \n2.11.0\n\n"},{"id":"320052","messageId":"704a0263164527f09565cda207a9f31939badb60.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 15/23] ref_update_reject_duplicates(): add a sanity check","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:38Z","receivedAt":"2017-05-17T12:06:50Z","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 \n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ffc9bd0be5..68a0872562 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1696,13 +1696,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":"320053","messageId":"14f627170d676bd5c49d91ee0eedea46939e9837.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 20/23] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:43Z","receivedAt":"2017-05-17T12:06:51Z","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 \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 68a0872562..f4b95109af 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1246,6 +1246,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 eb74d1119a..d3716c3a6f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1087,15 +1087,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":"320054","messageId":"1f50142ccbb84a4e5d7a1cd67e6f9d30edc1d73a.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 22/23] ref-filter: limit traversal to prefix","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:45Z","receivedAt":"2017-05-17T12:06:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Jeff King \n\nWhen we are matching refnames \"as path\" against a pattern, then we\nknow that the beginning of any refname that can match the pattern has\nto match the part of the pattern up to the first glob character. For\nexample, if the pattern is `refs/heads/foo*bar`, then it can only\nmatch a reference 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\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 \nSigned-off-by: Michael Haggerty \n---\n ref-filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 61 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1fc5e9970d..54cc6bcc13 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1665,6 +1665,66 @@ 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.\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 +1971,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":"320055","messageId":"42c41e7550ca4d81475780cd02b355a8c20cf4c3.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 14/23] ref_update_reject_duplicates(): use `size_t` rather than `int`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:37Z","receivedAt":"2017-05-17T12:06:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 43d65bc9c6..ffc9bd0be5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1692,7 +1692,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":"320056","messageId":"a20d94b932bc1b53478bb4fc9909357b58a5b37a.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 23/23] cache_ref_iterator_begin(): avoid priming unneeded directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:46Z","receivedAt":"2017-05-17T12:06:56Z","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 \n---\n refs/ref-cache.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 83 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex ab76b34e30..257c3f9cd0 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,28 @@ 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\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n }\n \n@@ -343,6 +394,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 +422,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 +450,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 +465,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 +480,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 +566,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 +581,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":"320057","messageId":"53ee004172d117a1ffc19e36fe1001a2ef62eac5.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 21/23] create_ref_entry(): remove `check_name` option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:44Z","receivedAt":"2017-05-17T12:07:07Z","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 \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 d3716c3a6f..c36194be02 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -304,7 +304,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\t\thashclr(sha1);\n \t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n \t\t\t}\n-\t\t\tlast = create_ref_entry(refname, sha1, flag, 0);\n+\t\t\tlast = create_ref_entry(refname, sha1, 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@@ -428,8 +428,12 @@ static void add_packed_ref(struct files_ref_store *refs,\n \n \tif (!is_lock_file_locked(&refs->packlock))\n \t\tdie(\"internal error: 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, sha1, REF_ISPACKED, 1));\n+\t\t create_ref_entry(refname, sha1, REF_ISPACKED));\n }\n \n /*\n@@ -506,7 +510,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, sha1, flag, 0));\n+\t\t\t\t\t create_ref_entry(refname.buf, sha1, flag));\n \t\t}\n \t\tstrbuf_setlen(&refname, dirnamelen);\n \t\tstrbuf_setlen(&path, path_baselen);\n@@ -1554,7 +1558,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->hash,\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 6059362f1d..ab76b34e30 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 unsigned char *sha1, int flag,\n-\t\t\t\t int check_name)\n+\t\t\t\t const unsigned char *sha1, 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 \thashcpy(ref->u.value.oid.hash, sha1);\n \toidclr(&ref->u.value.peeled);\ndiff --git a/refs/ref-cache.h b/refs/ref-cache.h\nindex 83e6c2dd2a..9a1ceeb7a5 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 unsigned char *sha1, int flag,\n-\t\t\t\t int check_name);\n+\t\t\t\t const unsigned char *sha1, 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":"320058","messageId":"70fbcd573f5c8a78a19a08ffc255437c36e7f49d.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 08/23] lockfile: add a new method, is_lock_file_locked()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:31Z","receivedAt":"2017-05-17T12:07:11Z","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 \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":"320059","messageId":"d6374d948f4424e4e6b4d999460597d226028be0.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 11/23] files_transaction_cleanup(): new helper function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:34Z","receivedAt":"2017-05-17T12:07:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract function from `files_transaction_commit()`. It will soon have\nanother caller.\n\nSigned-off-by: Michael Haggerty \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 ba0ad0aa44..7ddd4f87d5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2846,6 +2846,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@@ -2868,10 +2889,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@@ -3017,15 +3036,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":"320060","messageId":"106ab35ba40da745502da02abac66494208397f6.1495014840.git.mhagger@alum.mit.edu","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"[PATCH 13/23] ref_update_reject_duplicates(): expose function to whole refs module","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T12:05:36Z","receivedAt":"2017-05-17T12:07:36Z","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 \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 689362db1e..43d65bc9c6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1689,6 +1689,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 f48409132d..d7596b5222 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2524,23 +2524,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 2ff65d3ebd..52350c5301 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":"320061","messageId":"20170517124255.62hdqwvcvfy2tdr7@sigill.intra.peff.net","threadId":"45990","inReplyTo":"bb244c72ec6e6ab56171d82f7379a75c4eb5481f.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 01/23] t3600: clean up permissions test properly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T12:42:56Z","receivedAt":"2017-05-17T12:43:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:24PM +0200, Michael Haggerty wrote:\n\n> The test of failing `git rm -f` removes the write permissions on the\n> test directory, but fails to restore them if the test fails. This\n> means that the test temporary directory cannot be cleaned up, which\n> means that subsequent attempts to run the test fail mysteriously.\n> \n> Instead, do the cleanup in a `test_must_fail` block so that it can't\n> be skipped.\n\nThis should be `test_when_finished` in the final paragraph, right?\n\nThe patch itself looks obviously fine.\n\n-Peff\n"},{"id":"320062","messageId":"20170517125510.qmy2qvyxuup25zw6@sigill.intra.peff.net","threadId":"45990","inReplyTo":"eecc015af8d7ed71223b591b13847fdb56ee69f0.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T12:55:10Z","receivedAt":"2017-05-17T12:55:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:27PM +0200, Michael Haggerty wrote:\n\n> diff --git a/refs/iterator.c b/refs/iterator.c\n> index bce1f192f7..f33d1b3a39 100644\n> --- a/refs/iterator.c\n> +++ b/refs/iterator.c\n> @@ -292,7 +292,19 @@ 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 * If there wouldn't be at least one character\n> +\t\t\t * left in the refname after trimming, skip\n> +\t\t\t * over it:\n> +\t\t\t */\n> +\t\t\tif (memchr(iter->iter0->refname, '\\0', iter->trim + 1))\n> +\t\t\t\tcontinue;\n\nIt took me a minute to figure out the logic here. You're looking for the\nend-of-string within the trim boundary, which would be an indication\nthat the string itself is smaller than the boundary.\n\nBut what if it returns true, and the string really is shorter than the\ntrim size? That would mean we pass a size to memchr that is longer than\nthe buffer we pass. Is that legal?\n\nI suspect it's undefined behavior according to the standard, though I'd\nguess in practice it would be fine. But if I'm understanding it\ncorrectly, this is the same check as:\n\n if (strlen(iter->iter0->refname) <= iter->trim)\n\nwhich seems a lot more obvious to me and doesn't fall afoul of weird\nstandard issues. The only downside I see is that it would read to the\nend of string when yours could stop at iter->trim bytes. I have no idea\nif that would be measurable (it might even be faster because strlen()\nonly has one condition to loop on).\n\n-Peff\n"},{"id":"320063","messageId":"20170517125947.npk4x65e2wqwqvkh@sigill.intra.peff.net","threadId":"45990","inReplyTo":"c9e1d561825a63630a09a2815b9b296c9e4bd1f5.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T12:59:48Z","receivedAt":"2017-05-17T13:00:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:28PM +0200, Michael Haggerty wrote:\n\n> The backend already takes care of the prefix. By passing the prefix\n> again to `prefix_ref_iterator`, we were forcing that iterator to do\n> redundant prefix comparisons. So set it to the empty string.\n\nHmm. So givne a refname like \"refs/heads/foo\" and a prefix like\n\"refs/heads/\", would we trim down to \"foo\" and then further try to\nremove \"refs/heads\" again?\n\nThat sounds like it's not just redundant, but a bug (albeit one that is\nvery unlikely to come up, unless you have \"refs/heads/refs/heads/foo\").\n\nOr am I misunderstanding?\n\n-Peff\n"},{"id":"320064","messageId":"20170517131204.m564xy7i3ywietsl@sigill.intra.peff.net","threadId":"45990","inReplyTo":"b26ccc03a7e72dfa30467db81e9210dd621516e2.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:12:04Z","receivedAt":"2017-05-17T13:12:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:\n\n> Just because the files backend can't retain reflogs for deleted\n> references is no reason that they shouldn't be supported by the\n> virtual method interface. Let's add them now before the interface\n> becomes truly polymorphic and increases the work.\n> \n> Signed-off-by: Michael Haggerty \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\nHaving carried a similar patch in GitHub's fork for many years (because\nwe maintain an audit log of all ref updates), I expected this to be\nbigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog\nmessage argument, 2017-02-20) a few months ago, which already hit most\nof the ref-deleting callers. This is just making the plural\ndelete_refs() interface match.\n\nI think your reasoning above is sound by itself, but that gives an added\ninterface: we are making the delete_ref() and delete_refs() interfaces\nconsistent.\n\n-Peff\n"},{"id":"320065","messageId":"20170517131236.kzhsq2jehpxfd27o@sigill.intra.peff.net","threadId":"45990","inReplyTo":"70fbcd573f5c8a78a19a08ffc255437c36e7f49d.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 08/23] lockfile: add a new method, is_lock_file_locked()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:12:37Z","receivedAt":"2017-05-17T13:12:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:31PM +0200, Michael Haggerty wrote:\n\n> It will soon prove useful.\n\nVery mysterious...\n\n-Peff\n"},{"id":"320066","messageId":"20170517131459.uznay3pmzgxijunm@sigill.intra.peff.net","threadId":"45990","inReplyTo":"9b52c29411fa11ac0d8227d5ae78ba2768485b3f.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 09/23] files-backend: move `lock` member to `files_ref_store`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:15:00Z","receivedAt":"2017-05-17T13:15:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:32PM +0200, Michael Haggerty wrote:\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 *packlock;\n\nI'm glad to see you renamed this from just \"lock\", though the short\n\"pack\" makes me think of packed objects. I don't know if\n\"packed_refs_lock\" would be too verbose (I see it's just \"packed\" above,\nso maybe \"packed_lock\").\n\n\n\n-Peff\n"},{"id":"320067","messageId":"20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net","threadId":"45990","inReplyTo":"dd7637060bac1a27e03563edc82649812dcf897c.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:17:53Z","receivedAt":"2017-05-17T13:18:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n\n> 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\n> track of whether it is locked, embed the `lock_file` instance directly\n> in the `files_ref_store` struct and use the new\n> `is_lock_file_locked()` function to keep track of whether it is\n> locked. This keeps related data together and makes the main reference\n> store less of a special case.\n\nThis made me wonder how we handle the locking for ref_stores besides the\nmain one (e.g., for submodules). The lockfile structs have to remain\nvalid for the length of the program. Previously those stores could have\nxcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\nand leak their whole structs.\n\nI suspect the answer is \"we don't ever lock anything except the main ref\nstore because that is the only one we write to\", so it doesn't matter\nanyway.\n\n-Peff\n"},{"id":"320068","messageId":"20170517131958.nxb5ol3wrtwyskss@sigill.intra.peff.net","threadId":"45990","inReplyTo":"d6374d948f4424e4e6b4d999460597d226028be0.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 11/23] files_transaction_cleanup(): new helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:19:58Z","receivedAt":"2017-05-17T13:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:34PM +0200, Michael Haggerty wrote:\n\n> Extract function from `files_transaction_commit()`. It will soon have\n> another caller.\n> [...]\n> @@ -2868,10 +2889,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\nThis is slightly more than it says on the tin. I guess the reason is\nthat the cleanup function is going to learn about more than just\niterating over the transaction, and we'll want to trigger it even for\nempty transactions.\n\n-Peff\n"},{"id":"320069","messageId":"20170517132839.fx6oebgqt5t65tia@sigill.intra.peff.net","threadId":"45990","inReplyTo":"5d13d0f0e0a63430a9bf2889e9a9e911382170de.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:28:39Z","receivedAt":"2017-05-17T13:28:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:\n\n> 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\n> exist. But it could be that there is some other error opening the\n> file (e.g., permissions problems), and we don't want to silently\n> ignore such problems. So report any failures that are not due to\n> ENOENT.\n\nI think there are may be other \"OK\" errno values here, like ENOTDIR.\nThat's probably pretty unlikely in practice, though, as we're opening a\nfile at the root of the $GIT_DIR here (so somebody would had to have\nracily changed our paths). It's probably fine to just err on the side of\nsafety.\n\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(\"couldn't read %s: %s\",\n> +\t\t\t packed_refs_file, strerror(errno));\n> +\t\t}\n\nI think this could be die_errno().\n\n-Peff\n"},{"id":"320070","messageId":"20170517132950.bwi27nx5pxrjznxu@sigill.intra.peff.net","threadId":"45990","inReplyTo":"14f627170d676bd5c49d91ee0eedea46939e9837.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 20/23] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:29:50Z","receivedAt":"2017-05-17T13:30:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:43PM +0200, Michael Haggerty wrote:\n\n> 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\nGood catch. This should definitely go at the outer-most layer of the ref\ncode.\n\n-Peff\n"},{"id":"320071","messageId":"20170517133835.s3m3giccjo3jqstg@sigill.intra.peff.net","threadId":"45990","inReplyTo":"1f50142ccbb84a4e5d7a1cd67e6f9d30edc1d73a.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 22/23] ref-filter: limit traversal to prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:38:36Z","receivedAt":"2017-05-17T13:38:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:\n\n> From: Jeff King \n\nThis patch did originate with me, but I know you had to fix several\nthings to integrate it in your series. So I'll review it anyway, and\ngive you full blame for any bugs. :)\n\n> When we are matching refnames \"as path\" against a pattern, then we\n> know that the beginning of any refname that can match the pattern has\n> to match the part of the pattern up to the first glob character. For\n> example, if the pattern is `refs/heads/foo*bar`, then it can only\n> match a reference that has the prefix `refs/heads/foo`.\n\nThat first sentence confused me as to what \"as path\" meant (I know\nbecause I worked on this code, and even then it took me a minute to\nparse it).\n\nMaybe just \"When we are matching refnames against a pattern\" and then\nlater something like:\n\n Note 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\n git-branch and git-tag are subtly different.\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.\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\nIf I were reviewing this from scratch, I'd probably ask whether it is OK\nin:\n\n refs/heads/m*\n\nto return \"refs/heads/m\" as the prefix, and not stop at the last\nnon-wildcard component (\"refs/heads/\"). But I happen to know we\ndiscussed this off-list and you checked that for_each_ref and friends\nare happy with an arbitrary prefix. But I'm calling it out here for\nother reviewers.\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> [...]\n\nThe rest of it looks good to me.\n\n-Peff\n"},{"id":"320074","messageId":"20170517134241.5zqaqptfgjrrrbju@sigill.intra.peff.net","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/23] Prepare to separate out a packed_ref_store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T13:42:41Z","receivedAt":"2017-05-17T13:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 02:05:23PM +0200, Michael Haggerty wrote:\n\n> This patch series is the next leg on a journey towards reading\n> `packed-refs` using `mmap()`, the most interesting aspect of which is\n> that we will often be able to avoid having to read the whole\n> `packed-refs` file if we only need a subset of references.\n> \n> The first leg of the journey was separating out the reference cache\n> into a separate module [1]. That branch is already merged to master.\n> \n> This patch series prepares the ground for separating out a\n> `packed_ref_store`, but doesn't yet take that step. (As you can see,\n> it's a long enough patch series already!) It's kind of a grab bag of\n> cleanup patches plus work to decouple the packed-refs handling code\n> from the rest of `files_ref_store`. Some highlights:\n\nI dropped a few minor comments in individual patches, but it all looks\ngood to me up through patch 22. Your description in patch 23 makes\nsense, but I was too frazzled by the end to carefully review the code\nitself for that patch. I'll try to come back to it later.\n\n-Peff\n"},{"id":"320076","messageId":"e8ac4b86-12a7-57c0-421b-a3ba90709464@alum.mit.edu","threadId":"45990","inReplyTo":"20170517124255.62hdqwvcvfy2tdr7@sigill.intra.peff.net","subject":"Re: [PATCH 01/23] t3600: clean up permissions test properly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T14:01:57Z","receivedAt":"2017-05-17T14:02:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 02:42 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:24PM +0200, Michael Haggerty wrote:\n> \n>> The test of failing `git rm -f` removes the write permissions on the\n>> test directory, but fails to restore them if the test fails. This\n>> means that the test temporary directory cannot be cleaned up, which\n>> means that subsequent attempts to run the test fail mysteriously.\n>>\n>> Instead, do the cleanup in a `test_must_fail` block so that it can't\n>> be skipped.\n> \n> This should be `test_when_finished` in the final paragraph, right?\n\nYes, thanks. Will fix.\n\nMichael\n\n"},{"id":"320078","messageId":"7cec0285-fe6b-0e7e-83b4-b93c9770b55b@alum.mit.edu","threadId":"45990","inReplyTo":"20170517125510.qmy2qvyxuup25zw6@sigill.intra.peff.net","subject":"Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T14:11:15Z","receivedAt":"2017-05-17T14:11:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 02:55 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:27PM +0200, Michael Haggerty wrote:\n> \n>> diff --git a/refs/iterator.c b/refs/iterator.c\n>> index bce1f192f7..f33d1b3a39 100644\n>> --- a/refs/iterator.c\n>> +++ b/refs/iterator.c\n>> @@ -292,7 +292,19 @@ 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 * If there wouldn't be at least one character\n>> +\t\t\t * left in the refname after trimming, skip\n>> +\t\t\t * over it:\n>> +\t\t\t */\n>> +\t\t\tif (memchr(iter->iter0->refname, '\\0', iter->trim + 1))\n>> +\t\t\t\tcontinue;\n> \n> It took me a minute to figure out the logic here. You're looking for the\n> end-of-string within the trim boundary, which would be an indication\n> that the string itself is smaller than the boundary.\n> \n> But what if it returns true, and the string really is shorter than the\n> trim size? That would mean we pass a size to memchr that is longer than\n> the buffer we pass. Is that legal?\n> \n> I suspect it's undefined behavior according to the standard, though I'd\n> guess in practice it would be fine. But if I'm understanding it\n> correctly, this is the same check as:\n> \n> if (strlen(iter->iter0->refname) <= iter->trim)\n> \n> which seems a lot more obvious to me and doesn't fall afoul of weird\n> standard issues. The only downside I see is that it would read to the\n> end of string when yours could stop at iter->trim bytes. I have no idea\n> if that would be measurable (it might even be faster because strlen()\n> only has one condition to loop on).\n\nYou are correct that I chose `memchr()` over `strlen()` to avoid\nscanning a POTENTIALLY EXTREMELY LARGE NUMBER OF CHARACTERS past the\ntrim length, but of course in real life refnames aren't that long and\n`strlen()` might actually be faster.\n\nI *think* `memchr()` is technically OK:\n\n> Implementations shall behave as if they read the memory byte by byte\nfrom the beginning of the bytes pointed to by s and stop at the first\noccurrence of c (if it is found in the initial n bytes).\n\nBut I agree that the `strlen()` version is also easier to read (I\nactually had that version first). So I'll change it as you have suggested.\n\nThanks,\nMichael\n\n"},{"id":"320080","messageId":"cf706fbc-98aa-ef0a-e2e2-4ecda7fd5366@alum.mit.edu","threadId":"45990","inReplyTo":"20170517125947.npk4x65e2wqwqvkh@sigill.intra.peff.net","subject":"Re: [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T14:21:30Z","receivedAt":"2017-05-17T14:21:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 02:59 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:28PM +0200, Michael Haggerty wrote:\n> \n>> The backend already takes care of the prefix. By passing the prefix\n>> again to `prefix_ref_iterator`, we were forcing that iterator to do\n>> redundant prefix comparisons. So set it to the empty string.\n> \n> Hmm. So givne a refname like \"refs/heads/foo\" and a prefix like\n> \"refs/heads/\", would we trim down to \"foo\" and then further try to\n> remove \"refs/heads\" again?\n\nNo, the prefix is used to choose which references to let through and the\ntrim value is then used to munge the refname. They are orthogonal,\nthough AFAIK the trim value is always set to 0 or to strlen(prefix).\n\n`refs->be->iterator_begin()` only has the `prefix` feature; it doesn't\ntrim anything. So we can rely on it to discard the references that\naren't in `prefix`. All we need the `prefix_ref_iterator` for is the\ntrimming.\n\nMichael\n\n"},{"id":"320081","messageId":"20170517142205.zbhhekogaw5gwqhr@sigill.intra.peff.net","threadId":"45990","inReplyTo":"7cec0285-fe6b-0e7e-83b4-b93c9770b55b@alum.mit.edu","subject":"Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T14:22:06Z","receivedAt":"2017-05-17T14:22:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 04:11:15PM +0200, Michael Haggerty wrote:\n\n> > I suspect it's undefined behavior according to the standard, though I'd\n> > guess in practice it would be fine. But if I'm understanding it\n> > correctly, this is the same check as:\n> > \n> > if (strlen(iter->iter0->refname) <= iter->trim)\n> > \n> > which seems a lot more obvious to me and doesn't fall afoul of weird\n> > standard issues. The only downside I see is that it would read to the\n> > end of string when yours could stop at iter->trim bytes. I have no idea\n> > if that would be measurable (it might even be faster because strlen()\n> > only has one condition to loop on).\n> \n> You are correct that I chose `memchr()` over `strlen()` to avoid\n> scanning a POTENTIALLY EXTREMELY LARGE NUMBER OF CHARACTERS past the\n> trim length, but of course in real life refnames aren't that long and\n> `strlen()` might actually be faster.\n\nHeh. Yeah, I actually did dig up a glibc thread that says that strlen()\nis even faster than rawmemchr(). But they were discussing buffer sizes\nof at least 1K. For refnames I doubt it matters much at all.\n\n> I *think* `memchr()` is technically OK:\n> \n> > Implementations shall behave as if they read the memory byte by byte\n> from the beginning of the bytes pointed to by s and stop at the first\n> occurrence of c (if it is found in the initial n bytes).\n\nInteresting. I don't really see how else you'd implement it, so that\nmakes sense (your quote is from POSIX; I first looked in C99, but\ncouldn't find any similar language there).\n\n-Peff\n"},{"id":"320084","messageId":"4632308e-3f75-0d18-d0f8-c94af0d852c6@alum.mit.edu","threadId":"45990","inReplyTo":"20170517131204.m564xy7i3ywietsl@sigill.intra.peff.net","subject":"Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T15:01:41Z","receivedAt":"2017-05-17T15:01:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:12 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:\n> \n>> Just because the files backend can't retain reflogs for deleted\n>> references is no reason that they shouldn't be supported by the\n>> virtual method interface. Let's add them now before the interface\n>> becomes truly polymorphic and increases the work.\n>>\n>> Signed-off-by: Michael Haggerty \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> \n> Having carried a similar patch in GitHub's fork for many years (because\n> we maintain an audit log of all ref updates), I expected this to be\n> bigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog\n> message argument, 2017-02-20) a few months ago, which already hit most\n> of the ref-deleting callers. This is just making the plural\n> delete_refs() interface match.\n> \n> I think your reasoning above is sound by itself, but that gives an added\n> interface: we are making the delete_ref() and delete_refs() interfaces\n> consistent.\n\nI think you meant s/interface/justification/, in which case I agree with\nyou and I'll mention it in v2. I also noticed that the parameters are\nnamed inconsistently. I'll fix that too.\n\nMichael\n\n"},{"id":"320085","messageId":"20170517150339.4q3p7jlpuxrcwg2i@sigill.intra.peff.net","threadId":"45990","inReplyTo":"4632308e-3f75-0d18-d0f8-c94af0d852c6@alum.mit.edu","subject":"Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T15:03:39Z","receivedAt":"2017-05-17T15:03:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 05:01:41PM +0200, Michael Haggerty wrote:\n\n> On 05/17/2017 03:12 PM, Jeff King wrote:\n> > On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote:\n> > \n> >> Just because the files backend can't retain reflogs for deleted\n> >> references is no reason that they shouldn't be supported by the\n> >> virtual method interface. Let's add them now before the interface\n> >> becomes truly polymorphic and increases the work.\n> >>\n> >> Signed-off-by: Michael Haggerty \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> > \n> > Having carried a similar patch in GitHub's fork for many years (because\n> > we maintain an audit log of all ref updates), I expected this to be\n> > bigger. But I forgot that we did 755b49ae9 (delete_ref: accept a reflog\n> > message argument, 2017-02-20) a few months ago, which already hit most\n> > of the ref-deleting callers. This is just making the plural\n> > delete_refs() interface match.\n> > \n> > I think your reasoning above is sound by itself, but that gives an added\n> > interface: we are making the delete_ref() and delete_refs() interfaces\n> > consistent.\n> \n> I think you meant s/interface/justification/, in which case I agree with\n> you and I'll mention it in v2. I also noticed that the parameters are\n> named inconsistently. I'll fix that too.\n\nOops, I think I meant \"incentive\". But yes, you managed to decipher my\nrambling correctly.\n\n-Peff\n"},{"id":"320086","messageId":"654c485e-f758-22ea-8ed9-884e1e904595@alum.mit.edu","threadId":"45990","inReplyTo":"20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T15:05:49Z","receivedAt":"2017-05-17T15:05:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:17 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n> \n>> 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\n>> track of whether it is locked, embed the `lock_file` instance directly\n>> in the `files_ref_store` struct and use the new\n>> `is_lock_file_locked()` function to keep track of whether it is\n>> locked. This keeps related data together and makes the main reference\n>> store less of a special case.\n> \n> This made me wonder how we handle the locking for ref_stores besides the\n> main one (e.g., for submodules). The lockfile structs have to remain\n> valid for the length of the program. Previously those stores could have\n> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> and leak their whole structs.\n> \n> I suspect the answer is \"we don't ever lock anything except the main ref\n> store because that is the only one we write to\", so it doesn't matter\n> anyway.\n\nCorrect. If that ever changes, we'll be ready!\n\nMichael\n\n"},{"id":"320088","messageId":"488f1c1d-2611-5665-34dd-23ed88769fbc@alum.mit.edu","threadId":"45990","inReplyTo":"20170517132839.fx6oebgqt5t65tia@sigill.intra.peff.net","subject":"Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T15:27:50Z","receivedAt":"2017-05-17T15:28:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:28 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:\n> \n>> 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\n>> exist. But it could be that there is some other error opening the\n>> file (e.g., permissions problems), and we don't want to silently\n>> ignore such problems. So report any failures that are not due to\n>> ENOENT.\n> \n> I think there are may be other \"OK\" errno values here, like ENOTDIR.\n> That's probably pretty unlikely in practice, though, as we're opening a\n> file at the root of the $GIT_DIR here (so somebody would had to have\n> racily changed our paths). It's probably fine to just err on the side of\n> safety.\n\nYikes, I think if somebody swaps $GIT_DIR out from under a working git\ncommand, the user would want to know about it. How would you possibly\nrecover, anyway?\n\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(\"couldn't read %s: %s\",\n>> +\t\t\t packed_refs_file, strerror(errno));\n>> +\t\t}\n> \n> I think this could be die_errno().\n\nAh yes. I'll change it.\n\nThanks,\nMichael\n\n"},{"id":"320089","messageId":"ca2a1d69-8407-fde4-edb8-34cbffcd46ae@alum.mit.edu","threadId":"45990","inReplyTo":"20170517131459.uznay3pmzgxijunm@sigill.intra.peff.net","subject":"Re: [PATCH 09/23] files-backend: move `lock` member to `files_ref_store`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-17T15:49:16Z","receivedAt":"2017-05-17T15:49:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:15 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:32PM +0200, Michael Haggerty wrote:\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 *packlock;\n> \n> I'm glad to see you renamed this from just \"lock\", though the short\n> \"pack\" makes me think of packed objects. I don't know if\n> \"packed_refs_lock\" would be too verbose (I see it's just \"packed\" above,\n> so maybe \"packed_lock\").\n> \n> \n\nI'll rename it to `packed_refs_lock`.\n\nMichael\n\n"},{"id":"320090","messageId":"CAGZ79kYspKC=NZDp8q46KO2pGBfsAfK8A=2PRDb2O1swOLf6MQ@mail.gmail.com","threadId":"45990","inReplyTo":"17922578c2fbec4c0b8040cb37f432f913218489.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T16:59:24Z","receivedAt":"2017-05-17T17:00:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n\nNow this would want to have some selling words for it?\nI do not see an advantage of this patch as-is.\n\nI mean technically we don't need a sign, so we use that extra bit\nto be able to process transactions up to twice the size. But I doubt\nthat is the real reason. I'll read on, maybe a later patch will explain\nwhy we do this here.\n\nStefan\n"},{"id":"320091","messageId":"CAGZ79kaMYVp7SLq3MspC6f45=QUZM=mJMxNDfwVj+1cgEhMQ6Q@mail.gmail.com","threadId":"45990","inReplyTo":"f56871ab01d7a9b07631f143ae902605a1e247f4.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 02/23] refs.h: clarify docstring for the ref_transaction_update()-related fns","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T16:46:20Z","receivedAt":"2017-05-17T17:06:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> In particular, make it clear that they make copies of the sha1\n> arguments.\n\nA couple weeks ago we had plans on getting rid of SHA1 in\n\"the near future\" IIRC. Would it make sense to not go down\nthe SHA1 road further and document this in a more abstract way?\n\n s/SHA1/object name/\n\nessentially, but I guess one of Brians future series' may pick this\nup as well.\n\nI am just hesitant to introduce more sha1-ism at this point.\n\nThanks,\nStefan\n"},{"id":"320092","messageId":"CAGZ79kbdQ-MqYKH-L9unqEAPUtgQxNv6ANJERKSEQCADjgmm1w@mail.gmail.com","threadId":"45990","inReplyTo":"20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T17:18:17Z","receivedAt":"2017-05-17T17:20:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 6:17 AM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n>\n>> 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\n>> track of whether it is locked, embed the `lock_file` instance directly\n>> in the `files_ref_store` struct and use the new\n>> `is_lock_file_locked()` function to keep track of whether it is\n>> locked. This keeps related data together and makes the main reference\n>> store less of a special case.\n>\n> This made me wonder how we handle the locking for ref_stores besides the\n> main one (e.g., for submodules). The lockfile structs have to remain\n> valid for the length of the program. Previously those stores could have\n> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> and leak their whole structs.\n\n+cc Brandon, who is eager to go down that road.\n\n> I suspect the answer is \"we don't ever lock anything except the main ref\n> store because that is the only one we write to\", so it doesn't matter\n> anyway.\n>\n> -Peff\n\n\n> @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n> if (refs->packed) {\n> struct packed_ref_cache *packed_refs = refs->packed;\n>\n> - if (refs->packlock)\n> + if (is_lock_file_locked(&refs->packlock))\n> die(\"internal error: packed-ref cache cleared while locked\");\n\nI think the error message needs adjustment here as well? Maybe\n\n die(\"internal error: packed refs locked in cleanup\");\n\nThanks,\nStefan\n"},{"id":"320093","messageId":"CAGZ79kaRZB=VTe+c-M-qnkQJGQcYb0f-V0OMRuj4o6KAUvZrrA@mail.gmail.com","threadId":"45990","inReplyTo":"d6374d948f4424e4e6b4d999460597d226028be0.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 11/23] files_transaction_cleanup(): new helper function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T17:26:36Z","receivedAt":"2017-05-17T17:26:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> Extract function from `files_transaction_commit()`. It will soon have\n> another caller.\n\nThis sounds odd to me. Maybe it is missing words?\nof s/function/the functionality to cleanup/\n\nCode makes sense to me.\n\nStefan\n"},{"id":"320094","messageId":"CAGZ79kYF=MnJKrQakDbEQoR8r2MyeHro9vtzzmggwm6NXH9T2g@mail.gmail.com","threadId":"45990","inReplyTo":"5ec5f3f9b4c9fc9f719d116b48dcf7b61e74ae49.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T17:44:33Z","receivedAt":"2017-05-17T17:44:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> Break the function `ref_transaction_commit()` into two functions,\n> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a\n> third function, `ref_transaction_abort()`, that can be used to abort\n> the transaction. Break up the `ref_store` methods similarly.\n>\n> This split will make it easier to implement compound reference stores,\n> where a transaction might have to span multiple underlying stores. In\n> that case, we would first want to \"prepare\" the sub-transactions in\n> each of the reference stores. If any of the \"prepare\" steps fails, we\n> would \"abort\" all of the sub-transactions. Only if all of the\n> \"prepare\" steps succeed would we \"finish\" each of them.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nCode looks good to me.\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 in a valid state and new updates can be\n> + * added to it. An OPEN transaction can be prepared or\n> + * committed.\n\nAll of these states are valid, OPEN is not more valid than the others. ;)\nMaybe:\n\n OPEN: initial state. new updates can be added to it. An OPEN\n transaction can be prepared, committed.\n\nIs it possible to also abort/free an open transaction?\n\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.\n\nMaybe add that it cannot be altered any more? Possible ways out are\ncommit and abort IIUC?\n\n\n> + *\n> + * CLOSED: The transaction is no longer active. No other operations\n> + * than free can be used on it in this state. A transaction\n> + * becomes closed if there is a failure while building the\n> + * transaction, if an active transaction is committed, or if a\n\nThe wording here is off as it doesn't use the lingo as introduced before.\n(What is an \"active\" transaction? I think you mean OPEN in this case)\n\n\n\n> + * prepared transaction is finished.\n> */\n\n\nAlso s/prepared/PREPARED/, add \"or aborted\"(?)\nas that is also a viable way IIUC.\n\nThanks,\nStefan\n"},{"id":"320095","messageId":"CAGZ79kb65sv8g6XUQMcGTkZ0ubpY2LYpj7g2wv15knXuv7oKhw@mail.gmail.com","threadId":"45990","inReplyTo":"eedc8f85c8237515bc66584649b1e3f3d547049c.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 17/23] get_packed_ref_cache(): assume \"packed-refs\" won't change while locked","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T17:57:34Z","receivedAt":"2017-05-17T17:57:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> If we've got the \"packed-refs\" file locked, then it can't change;\n> there's no need to keep calling `stat_validity_check()` on it.\n\nThis change will work in a world where all Git implementations\nobey a lock. If there is at least one implementation that doesn't\ncare about the existence of lock files we may introduce a race\nhere.\n\nI am not sure if it is worth to be extra careful in the common case\nthough. But I could imagine some people using a git repo on an\nNFS concurrently with different implementations and one of them\nis an old / careless lock-ignoring implementation.\n\nMy opinion is not strong enough that I'd veto such a patch\njust food for thought.\n\nThanks,\nStefan\n"},{"id":"320096","messageId":"CAGZ79kaNxyh_3eJUZ7VRL673K4_nYD-KosAyv=grzWRCSzYT9Q@mail.gmail.com","threadId":"45990","inReplyTo":"20170517134241.5zqaqptfgjrrrbju@sigill.intra.peff.net","subject":"Re: [PATCH 00/23] Prepare to separate out a packed_ref_store","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-17T18:14:00Z","receivedAt":"2017-05-17T18:14:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 6:42 AM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:23PM +0200, Michael Haggerty wrote:\n>\n>> This patch series is the next leg on a journey towards reading\n>> `packed-refs` using `mmap()`, the most interesting aspect of which is\n>> that we will often be able to avoid having to read the whole\n>> `packed-refs` file if we only need a subset of references.\n>>\n>> The first leg of the journey was separating out the reference cache\n>> into a separate module [1]. That branch is already merged to master.\n>>\n>> This patch series prepares the ground for separating out a\n>> `packed_ref_store`, but doesn't yet take that step. (As you can see,\n>> it's a long enough patch series already!) It's kind of a grab bag of\n>> cleanup patches plus work to decouple the packed-refs handling code\n>> from the rest of `files_ref_store`. Some highlights:\n>\n> I dropped a few minor comments in individual patches, but it all looks\n> good to me up through patch 22. Your description in patch 23 makes\n> sense, but I was too frazzled by the end to carefully review the code\n> itself for that patch. I'll try to come back to it later.\n\nSame here.\n\nThanks,\nStefan\n"},{"id":"320117","messageId":"20170518001717.GF185461@google.com","threadId":"45990","inReplyTo":"20170517131753.rditx62clmkrdmeq@sigill.intra.peff.net","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-18T00:17:17Z","receivedAt":"2017-05-18T00:17:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/17, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n> \n> > 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\n> > track of whether it is locked, embed the `lock_file` instance directly\n> > in the `files_ref_store` struct and use the new\n> > `is_lock_file_locked()` function to keep track of whether it is\n> > locked. This keeps related data together and makes the main reference\n> > store less of a special case.\n> \n> This made me wonder how we handle the locking for ref_stores besides the\n> main one (e.g., for submodules). The lockfile structs have to remain\n> valid for the length of the program. Previously those stores could have\n> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> and leak their whole structs.\n\nWait, why would this be the case? I would assume that you would\nallocate a ref_store (including a lockfile for it) and then once you are\ndone with the ref_store, you free it (and unlock and free the lockfile).\nI'm definitely not versed in ref handling code though so I may be\nmissing something.\n\n> \n> I suspect the answer is \"we don't ever lock anything except the main ref\n> store because that is the only one we write to\", so it doesn't matter\n> anyway.\n\nReally? I can envision a case in the future where we'd want to update\na ref, or create a ref inside a submodule.\n\n-- \nBrandon Williams\n"},{"id":"320118","messageId":"20170518001834.GG185461@google.com","threadId":"45990","inReplyTo":"CAGZ79kbdQ-MqYKH-L9unqEAPUtgQxNv6ANJERKSEQCADjgmm1w@mail.gmail.com","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-18T00:18:34Z","receivedAt":"2017-05-18T00:18:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/17, Stefan Beller wrote:\n> On Wed, May 17, 2017 at 6:17 AM, Jeff King wrote:\n> > On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n> >\n> >> 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\n> >> track of whether it is locked, embed the `lock_file` instance directly\n> >> in the `files_ref_store` struct and use the new\n> >> `is_lock_file_locked()` function to keep track of whether it is\n> >> locked. This keeps related data together and makes the main reference\n> >> store less of a special case.\n> >\n> > This made me wonder how we handle the locking for ref_stores besides the\n> > main one (e.g., for submodules). The lockfile structs have to remain\n> > valid for the length of the program. Previously those stores could have\n> > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> > and leak their whole structs.\n> \n> +cc Brandon, who is eager to go down that road.\n\nI'm probably too eager haha. But I still think its something to slowly\nwork towards.\n\n> \n> > I suspect the answer is \"we don't ever lock anything except the main ref\n> > store because that is the only one we write to\", so it doesn't matter\n> > anyway.\n> >\n> > -Peff\n> \n> \n> > @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n> > if (refs->packed) {\n> > struct packed_ref_cache *packed_refs = refs->packed;\n> >\n> > - if (refs->packlock)\n> > + if (is_lock_file_locked(&refs->packlock))\n> > die(\"internal error: packed-ref cache cleared while locked\");\n> \n> I think the error message needs adjustment here as well? Maybe\n> \n> die(\"internal error: packed refs locked in cleanup\");\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"320119","messageId":"20170518011153.luuacy5jay6vinzv@sigill.intra.peff.net","threadId":"45990","inReplyTo":"20170518001717.GF185461@google.com","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-18T01:11:53Z","receivedAt":"2017-05-18T01:12:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 05:17:17PM -0700, Brandon Williams wrote:\n\n> > This made me wonder how we handle the locking for ref_stores besides the\n> > main one (e.g., for submodules). The lockfile structs have to remain\n> > valid for the length of the program. Previously those stores could have\n> > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> > and leak their whole structs.\n> \n> Wait, why would this be the case? I would assume that you would\n> allocate a ref_store (including a lockfile for it) and then once you are\n> done with the ref_store, you free it (and unlock and free the lockfile).\n> I'm definitely not versed in ref handling code though so I may be\n> missing something.\n\nOne used, you are not allowed to free a lockfile struct (actually these\ndays it's just the \"tempfile\" part of it), because it lives on the\ncleanup-handler's tempfile_list forever.\n\nThis is a holdover from the early days of the lockfile code, but I think\nwe could loosen it (and that's the right solution in the long run).\n\n> > I suspect the answer is \"we don't ever lock anything except the main ref\n> > store because that is the only one we write to\", so it doesn't matter\n> > anyway.\n> \n> Really? I can envision a case in the future where we'd want to update\n> a ref, or create a ref inside a submodule.\n\nOh, I agree it's a probable thing for the future. I was mostly wondering\nabout the immediate change. I think this patch makes things slightly\nworse for that future, but the right fix is to remove the weird tempfile\nlifetime requirement in the first place.\n\n-Peff\n"},{"id":"320120","messageId":"20170518011517.xklxkbmkkx6cppdf@sigill.intra.peff.net","threadId":"45990","inReplyTo":"CAGZ79kb65sv8g6XUQMcGTkZ0ubpY2LYpj7g2wv15knXuv7oKhw@mail.gmail.com","subject":"Re: [PATCH 17/23] get_packed_ref_cache(): assume \"packed-refs\" won't change while locked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-18T01:15:18Z","receivedAt":"2017-05-18T01:15:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 10:57:34AM -0700, Stefan Beller wrote:\n\n> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> > If we've got the \"packed-refs\" file locked, then it can't change;\n> > there's no need to keep calling `stat_validity_check()` on it.\n> \n> This change will work in a world where all Git implementations\n> obey a lock. If there is at least one implementation that doesn't\n> care about the existence of lock files we may introduce a race\n> here.\n> \n> I am not sure if it is worth to be extra careful in the common case\n> though. But I could imagine some people using a git repo on an\n> NFS concurrently with different implementations and one of them\n> is an old / careless lock-ignoring implementation.\n> \n> My opinion is not strong enough that I'd veto such a patch\n> just food for thought.\n\nYou're so unbelievably screwed if somebody is not respecting the lock\nthat I don't think it's worth considering.\n\nThis change just drops the stat_validity_check(), so you may fail to\nrealize that somebody racily (without holding the lock!) changed the\npacked refs, and may omit a ref from your traversal if it moved from\nloose to packed. That _can_ have lasting corruption effects if your\noperation is something like \"git prune\" that is computing full\nreachability.\n\nBut even without this change, somebody writing the packed-refs file\nwithout lock is likely to hose over simultaneous writes and lose ref\nupdates (or even lose refs entirely). So anybody who doesn't respect the\nlocks is broken, period, and needs to be fixed.\n\n-Peff\n"},{"id":"320131","messageId":"xmqqa86avm8j.fsf@gitster.mtv.corp.google.com","threadId":"45990","inReplyTo":"bb244c72ec6e6ab56171d82f7379a75c4eb5481f.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 01/23] t3600: clean up permissions test properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-18T04:10:20Z","receivedAt":"2017-05-18T04:10:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The test of failing `git rm -f` removes the write permissions on the\n> test directory, but fails to restore them if the test fails. This\n> means that the test temporary directory cannot be cleaned up, which\n> means that subsequent attempts to run the test fail mysteriously.\n>\n> Instead, do the cleanup in a `test_must_fail` block so that it can't\n> be skipped.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> t/t3600-rm.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 5f9913ba33..4a35c378c8 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -98,8 +98,8 @@ embedded'\"\n> \n> test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n> \tchmod a-w . &&\n> -\ttest_must_fail git rm -f baz &&\n> -\tchmod 775 .\n> +\ttest_when_finished \"chmod 775 .\" &&\n> +\ttest_must_fail git rm -f baz\n> '\n\nObviously a good idea.\n\nIn this case it would not matter very much, but I think it is a\nbetter style to have when-finished _before_ \"chmod a-w .\" that\nintroduces the state we want to revert out of.\n\n"},{"id":"320132","messageId":"xmqq60gyvm2u.fsf@gitster.mtv.corp.google.com","threadId":"45990","inReplyTo":"CAGZ79kaMYVp7SLq3MspC6f45=QUZM=mJMxNDfwVj+1cgEhMQ6Q@mail.gmail.com","subject":"Re: [PATCH 02/23] refs.h: clarify docstring for the ref_transaction_update()-related fns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-18T04:13:45Z","receivedAt":"2017-05-18T04:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n>> In particular, make it clear that they make copies of the sha1\n>> arguments.\n>\n> A couple weeks ago we had plans on getting rid of SHA1 in\n> \"the near future\" IIRC. Would it make sense to not go down\n> the SHA1 road further and document this in a more abstract way?\n>\n> s/SHA1/object name/\n>\n> essentially, but I guess one of Brians future series' may pick this\n> up as well.\n>\n> I am just hesitant to introduce more sha1-ism at this point.\n\nDon't worry too much about it. These new paragraphs explain\nexisting new_sha1 and old_sha1 parameters, and when they are updated\nto new_oid/old_oid, the comment will get updated at the same time to\nmatch.\n"},{"id":"320133","messageId":"xmqq1srmvlt8.fsf@gitster.mtv.corp.google.com","threadId":"45990","inReplyTo":"eecc015af8d7ed71223b591b13847fdb56ee69f0.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-18T04:19:31Z","receivedAt":"2017-05-18T04:19:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The `trim` parameter can be set independently of `prefix`. So if some\n> caller were to set `trim` to be greater than `strlen(prefix)`, we\n> could end up pointing the `refname` field of the iterator past the NUL\n> of the actual reference name string.\n>\n> That can't happen currently, because `trim` is always set either to\n> zero or to `strlen(prefix)`. But even the latter could lead to\n> confusion, if a refname is exactly equal to the prefix, because then\n> we would set the outgoing `refname` to the empty string.\n>\n> And we're about to decouple the `prefix` and `trim` arguments even\n> more, so let's be cautious here. Skip over any references whose names\n> are not longer than `trim`.\n\nShould we be silently continuing, or should we use die(\"BUG\") here\ninstead, if the motivation is to be cautions? Personally, I do not\nfind this memchr() implementation too bad, especially when our\nobjective is to play cautious, but strlen() based one is fine, too.\n\nIt's not like refname field would point at a run of non-NUL bytes at\nthe end of the last-mapped page and taking strlen() would segfault,\nright?\n\n> +\t\tif (iter->trim) {\n> +\t\t\t/*\n> +\t\t\t * If there wouldn't be at least one character\n> +\t\t\t * left in the refname after trimming, skip\n> +\t\t\t * over it:\n> +\t\t\t */\n> +\t\t\tif (memchr(iter->iter0->refname, '\\0', iter->trim + 1))\n> +\t\t\t\tcontinue;\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"},{"id":"320134","messageId":"e9f987c6-3a02-4121-bd21-e7b78f28f7db@alum.mit.edu","threadId":"45990","inReplyTo":"xmqq1srmvlt8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-18T04:50:10Z","receivedAt":"2017-05-18T04:50:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/18/2017 06:19 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> The `trim` parameter can be set independently of `prefix`. So if some\n>> caller were to set `trim` to be greater than `strlen(prefix)`, we\n>> could end up pointing the `refname` field of the iterator past the NUL\n>> of the actual reference name string.\n>>\n>> That can't happen currently, because `trim` is always set either to\n>> zero or to `strlen(prefix)`. But even the latter could lead to\n>> confusion, if a refname is exactly equal to the prefix, because then\n>> we would set the outgoing `refname` to the empty string.\n>>\n>> And we're about to decouple the `prefix` and `trim` arguments even\n>> more, so let's be cautious here. Skip over any references whose names\n>> are not longer than `trim`.\n> \n> Should we be silently continuing, or should we use die(\"BUG\") here\n> instead, if the motivation is to be cautions? Personally, I do not\n> find this memchr() implementation too bad, especially when our\n> objective is to play cautious, but strlen() based one is fine, too.\n\nTrue; it would not make much sense to trim off more characters than a\nprefix that you have selected for. This also implies that callers should\nonly check-and-trim a prefix that ends with an explicit \"/\". Otherwise,\nsay for example if a caller tries to check-and-trim the prefix\n\"refs/bisect\" and the user has a reference named \"refs/bisect\", the\nresult could be the empty string.\n\nI'll change this to die(\"BUG\") and document the above.\n\n> It's not like refname field would point at a run of non-NUL bytes at\n> the end of the last-mapped page and taking strlen() would segfault,\n> right?\n\nNo, refname should be a legit string.\n\nMichael\n\n"},{"id":"320136","messageId":"d9831944-47e0-e886-60ce-e3db151340dd@alum.mit.edu","threadId":"45990","inReplyTo":"CAGZ79kYspKC=NZDp8q46KO2pGBfsAfK8A=2PRDb2O1swOLf6MQ@mail.gmail.com","subject":"Re: [PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-18T04:55:43Z","receivedAt":"2017-05-18T04:55:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 06:59 PM, Stefan Beller wrote:\n> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n> \n> Now this would want to have some selling words for it?\n> I do not see an advantage of this patch as-is.\n> \n> I mean technically we don't need a sign, so we use that extra bit\n> to be able to process transactions up to twice the size. But I doubt\n> that is the real reason. I'll read on, maybe a later patch will explain\n> why we do this here.\n\nThe reason to use `size_t` is not signedness but rather that it might be\nlarger than `int` (e.g., 64 vs 32 bits), so you could theoretically get\nan integer overflow otherwise. It's unlikely here, because it would be\nhard to initiate an update of more than (2³¹-1) references in a single\nupdate, but it's good hygiene anyway.\n\nI'll mention that in the commit message.\n\nMichael\n\n"},{"id":"320137","messageId":"xmqqwp9eu5h2.fsf@gitster.mtv.corp.google.com","threadId":"45990","inReplyTo":"20170517132839.fx6oebgqt5t65tia@sigill.intra.peff.net","subject":"Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-18T04:57:45Z","receivedAt":"2017-05-18T04:57:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:\n>\n>> 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\n>> exist. But it could be that there is some other error opening the\n>> file (e.g., permissions problems), and we don't want to silently\n>> ignore such problems. So report any failures that are not due to\n>> ENOENT.\n>\n> I think there are may be other \"OK\" errno values here, like ENOTDIR.\n> That's probably pretty unlikely in practice, though, as we're opening a\n> file at the root of the $GIT_DIR here (so somebody would had to have\n> racily changed our paths). It's probably fine to just err on the side of\n> safety.\n>\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(\"couldn't read %s: %s\",\n>> +\t\t\t packed_refs_file, strerror(errno));\n>> +\t\t}\n>\n> I think this could be die_errno().\n\nI wonder what the endgame shape of this code should be, when it and\nnd/fopen-errors topic both graduate. We cannot use fopen_or_warn(),\nas we not just want to warn but want to die, e.g.\n\n\tf = fopen(...);\n\tif (!f) {\n \tif (warn_on_fopen_errors(...))\n \tdie_erno(...);\n\t\treturn packed_refs;\n\t}\n\n"},{"id":"320139","messageId":"20170518050849.kopykuewhfq3psav@sigill.intra.peff.net","threadId":"45990","inReplyTo":"xmqqwp9eu5h2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-18T05:08:49Z","receivedAt":"2017-05-18T05:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 18, 2017 at 01:57:45PM +0900, Junio C Hamano wrote:\n\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(\"couldn't read %s: %s\",\n> >> +\t\t\t packed_refs_file, strerror(errno));\n> >> +\t\t}\n> >\n> > I think this could be die_errno().\n> \n> I wonder what the endgame shape of this code should be, when it and\n> nd/fopen-errors topic both graduate. We cannot use fopen_or_warn(),\n> as we not just want to warn but want to die, e.g.\n> \n> \tf = fopen(...);\n> \tif (!f) {\n> \tif (warn_on_fopen_errors(...))\n> \tdie_erno(...);\n> \t\treturn packed_refs;\n> \t}\n\nIf we go that route I think the die becomes just:\n \n if (warn_on_fopen_errors(...))\n\tdie(\"unable to open packed refs\");\n\nor something. If we want a single die, perhaps the cleanest thing is to\nput the logic into its own function:\n\n static int missing_file_errno(int err)\n {\n\treturn errno == ENOENT || errno == ENOTDIR;\n }\n ...\n if (!missing_file_errno(errno))\n\tdie_errno(\"couldn't read %s\", packed_refs_file);\n\nIt feels a little silly to make such a trivial helper, but the point is\nto encapsulate that logic. And I think it would be used inside\nfopen_or_warn(), and possibly other places. The name might need some\nwork; that was the best I could come up with.\n\n-Peff\n"},{"id":"320165","messageId":"20170518154207.GC112091@google.com","threadId":"45990","inReplyTo":"20170518011153.luuacy5jay6vinzv@sigill.intra.peff.net","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-18T15:42:07Z","receivedAt":"2017-05-18T15:42:33Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/17, Jeff King wrote:\n> On Wed, May 17, 2017 at 05:17:17PM -0700, Brandon Williams wrote:\n> \n> > > This made me wonder how we handle the locking for ref_stores besides the\n> > > main one (e.g., for submodules). The lockfile structs have to remain\n> > > valid for the length of the program. Previously those stores could have\n> > > xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n> > > and leak their whole structs.\n> > \n> > Wait, why would this be the case? I would assume that you would\n> > allocate a ref_store (including a lockfile for it) and then once you are\n> > done with the ref_store, you free it (and unlock and free the lockfile).\n> > I'm definitely not versed in ref handling code though so I may be\n> > missing something.\n> \n> One used, you are not allowed to free a lockfile struct (actually these\n> days it's just the \"tempfile\" part of it), because it lives on the\n> cleanup-handler's tempfile_list forever.\n> \n> This is a holdover from the early days of the lockfile code, but I think\n> we could loosen it (and that's the right solution in the long run).\n> \n\nAh ok, thanks for the info!\n\n> > > I suspect the answer is \"we don't ever lock anything except the main ref\n> > > store because that is the only one we write to\", so it doesn't matter\n> > > anyway.\n> > \n> > Really? I can envision a case in the future where we'd want to update\n> > a ref, or create a ref inside a submodule.\n> \n> Oh, I agree it's a probable thing for the future. I was mostly wondering\n> about the immediate change. I think this patch makes things slightly\n> worse for that future, but the right fix is to remove the weird tempfile\n> lifetime requirement in the first place.\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"320172","messageId":"CAGZ79kZ+R_tm5Kv=t9tdXAJwKfevx1qnnNuK5vaXf=xRu1H4eg@mail.gmail.com","threadId":"45990","inReplyTo":"20170518011517.xklxkbmkkx6cppdf@sigill.intra.peff.net","subject":"Re: [PATCH 17/23] get_packed_ref_cache(): assume \"packed-refs\" won't change while locked","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-18T16:58:53Z","receivedAt":"2017-05-18T16:59:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 17, 2017 at 6:15 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 10:57:34AM -0700, Stefan Beller wrote:\n>\n>> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n>> > If we've got the \"packed-refs\" file locked, then it can't change;\n>> > there's no need to keep calling `stat_validity_check()` on it.\n>>\n>> This change will work in a world where all Git implementations\n>> obey a lock. If there is at least one implementation that doesn't\n>> care about the existence of lock files we may introduce a race\n>> here.\n>>\n>> I am not sure if it is worth to be extra careful in the common case\n>> though. But I could imagine some people using a git repo on an\n>> NFS concurrently with different implementations and one of them\n>> is an old / careless lock-ignoring implementation.\n>>\n>> My opinion is not strong enough that I'd veto such a patch\n>> just food for thought.\n>\n> You're so unbelievably screwed if somebody is not respecting the lock\n> that I don't think it's worth considering.\n>\n> This change just drops the stat_validity_check(), so you may fail to\n> realize that somebody racily (without holding the lock!) changed the\n> packed refs, and may omit a ref from your traversal if it moved from\n> loose to packed. That _can_ have lasting corruption effects if your\n> operation is something like \"git prune\" that is computing full\n> reachability.\n>\n> But even without this change, somebody writing the packed-refs file\n> without lock is likely to hose over simultaneous writes and lose ref\n> updates (or even lose refs entirely). So anybody who doesn't respect the\n> locks is broken, period, and needs to be fixed.\n\nok, in that case I see no objection for this patch.\n\nStefan\n"},{"id":"320174","messageId":"6d84ff31-0282-00ec-83d6-9be2a8b7a776@kdbg.org","threadId":"45990","inReplyTo":"cover.1495014840.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/23] Prepare to separate out a packed_ref_store","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-05-18T17:14:38Z","receivedAt":"2017-05-18T17:14:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.05.2017 um 14:05 schrieb Michael Haggerty:\n> This patch series is the next leg on a journey towards reading\n> `packed-refs` using `mmap()`, the most interesting aspect of which is\n> that we will often be able to avoid having to read the whole\n> `packed-refs` file if we only need a subset of references.\n\nWhich features of mmap() are you going to use? We have only MAP_PRIVATE \non Windows. We do emulate PROT_WRITE (without PROT_READ), but I don't \nthink that code is exercised anywhere. See also compat/win32mmap.c.\n\n-- Hannes\n"},{"id":"320175","messageId":"20170518172220.6caa2dcoc72b5g7z@sigill.intra.peff.net","threadId":"45990","inReplyTo":"6d84ff31-0282-00ec-83d6-9be2a8b7a776@kdbg.org","subject":"Re: [PATCH 00/23] Prepare to separate out a packed_ref_store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-18T17:22:20Z","receivedAt":"2017-05-18T17:22:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 18, 2017 at 07:14:38PM +0200, Johannes Sixt wrote:\n\n> Am 17.05.2017 um 14:05 schrieb Michael Haggerty:\n> > This patch series is the next leg on a journey towards reading\n> > `packed-refs` using `mmap()`, the most interesting aspect of which is\n> > that we will often be able to avoid having to read the whole\n> > `packed-refs` file if we only need a subset of references.\n> \n> Which features of mmap() are you going to use? We have only MAP_PRIVATE on\n> Windows. We do emulate PROT_WRITE (without PROT_READ), but I don't think\n> that code is exercised anywhere. See also compat/win32mmap.c.\n\nIt's just on the reading side, so it should be the same as the mmaps we\ndo for reading packfiles, the index, etc. You can see the actual call in\nthe upcoming series that Michael linked earlier:\n\n https://github.com/mhagger/git/commit/d22420b5f3e967d24aba019a0916a5a93ab00e03\n\n-Peff\n"},{"id":"320245","messageId":"2003333a-4a33-b023-078f-07a481df7b63@alum.mit.edu","threadId":"45990","inReplyTo":"xmqqa86avm8j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 01/23] t3600: clean up permissions test properly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T03:37:57Z","receivedAt":"2017-05-19T03:43:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/18/2017 06:10 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> The test of failing `git rm -f` removes the write permissions on the\n>> test directory, but fails to restore them if the test fails. This\n>> means that the test temporary directory cannot be cleaned up, which\n>> means that subsequent attempts to run the test fail mysteriously.\n>>\n>> Instead, do the cleanup in a `test_must_fail` block so that it can't\n>> be skipped.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> t/t3600-rm.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n>> index 5f9913ba33..4a35c378c8 100755\n>> --- a/t/t3600-rm.sh\n>> +++ b/t/t3600-rm.sh\n>> @@ -98,8 +98,8 @@ embedded'\"\n>> \n>> test_expect_success SANITY 'Test that \"git rm -f\" fails if its rm fails' '\n>> \tchmod a-w . &&\n>> -\ttest_must_fail git rm -f baz &&\n>> -\tchmod 775 .\n>> +\ttest_when_finished \"chmod 775 .\" &&\n>> +\ttest_must_fail git rm -f baz\n>> '\n> \n> Obviously a good idea.\n> \n> In this case it would not matter very much, but I think it is a\n> better style to have when-finished _before_ \"chmod a-w .\" that\n> introduces the state we want to revert out of.\n\nOK, I'll change this in v2.\n\nMichael\n\n"},{"id":"320246","messageId":"d7e3d4ac-6383-d919-9f42-bb278fc26b3f@alum.mit.edu","threadId":"45990","inReplyTo":"CAGZ79kbdQ-MqYKH-L9unqEAPUtgQxNv6ANJERKSEQCADjgmm1w@mail.gmail.com","subject":"Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T04:00:22Z","receivedAt":"2017-05-19T04:00:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 07:18 PM, Stefan Beller wrote:\n> On Wed, May 17, 2017 at 6:17 AM, Jeff King wrote:\n>> On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote:\n>>\n>>> 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\n>>> track of whether it is locked, embed the `lock_file` instance directly\n>>> in the `files_ref_store` struct and use the new\n>>> `is_lock_file_locked()` function to keep track of whether it is\n>>> locked. This keeps related data together and makes the main reference\n>>> store less of a special case.\n>>\n>> This made me wonder how we handle the locking for ref_stores besides the\n>> main one (e.g., for submodules). The lockfile structs have to remain\n>> valid for the length of the program. Previously those stores could have\n>> xcalloc()'d a lockfile and just leaked it. Now they'll need to xcalloc()\n>> and leak their whole structs.\n> \n> +cc Brandon, who is eager to go down that road.\n> \n>> I suspect the answer is \"we don't ever lock anything except the main ref\n>> store because that is the only one we write to\", so it doesn't matter\n>> anyway.\n>>\n>> -Peff\n> \n> \n>> @@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n>> if (refs->packed) {\n>> struct packed_ref_cache *packed_refs = refs->packed;\n>>\n>> - if (refs->packlock)\n>> + if (is_lock_file_locked(&refs->packlock))\n>> die(\"internal error: packed-ref cache cleared while locked\");\n> \n> I think the error message needs adjustment here as well? Maybe\n> \n> die(\"internal error: packed refs locked in cleanup\");\n\nHmmm, I don't see what's wrong with the current error message (except\nfor s/internal error/BUG/). In any case, this is meant to be a sanity\ncheck that users should never see, so I wouldn't worry too much about it\neither way.\n\nMichael\n\n"},{"id":"320247","messageId":"f1f8e0a1-41cc-1a48-2744-87118e39e182@alum.mit.edu","threadId":"45990","inReplyTo":"CAGZ79kaRZB=VTe+c-M-qnkQJGQcYb0f-V0OMRuj4o6KAUvZrrA@mail.gmail.com","subject":"Re: [PATCH 11/23] files_transaction_cleanup(): new helper function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T04:42:28Z","receivedAt":"2017-05-19T04:42:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 07:26 PM, Stefan Beller wrote:\n> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n>> Extract function from `files_transaction_commit()`. It will soon have\n>> another caller.\n> \n> This sounds odd to me. Maybe it is missing words?\n> of s/function/the functionality to cleanup/\n\nThis is basically the same as the standard refactoring step \"extract\nmethod\", except applied to a function, not a method. I thought these\nterms would be standard enough to not need further explanation, but I'll\nadd some more words.\n\nMichael\n\n"},{"id":"320248","messageId":"c8eb0a01-9d7d-a8c6-c1bc-31743d677883@alum.mit.edu","threadId":"45990","inReplyTo":"20170517131958.nxb5ol3wrtwyskss@sigill.intra.peff.net","subject":"Re: [PATCH 11/23] files_transaction_cleanup(): new helper function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T04:49:37Z","receivedAt":"2017-05-19T04:49:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:19 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:34PM +0200, Michael Haggerty wrote:\n> \n>> Extract function from `files_transaction_commit()`. It will soon have\n>> another caller.\n>> [...]\n>> @@ -2868,10 +2889,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> This is slightly more than it says on the tin. I guess the reason is\n> that the cleanup function is going to learn about more than just\n> iterating over the transaction, and we'll want to trigger it even for\n> empty transactions.\n\nMostly this is just DRY. For example, later the cleanup will be changed\nto set the state to `REF_TRANSACTION_PREPARED` rather than\n`REF_TRANSACTION_CLOSED`, and that change will be able to be made in one\nplace rather than two. I'll mention this in the commit message.\n\nMichael\n\n"},{"id":"320250","messageId":"00f2cef4-70fa-9e77-9833-764d4b133733@alum.mit.edu","threadId":"45990","inReplyTo":"CAGZ79kYF=MnJKrQakDbEQoR8r2MyeHro9vtzzmggwm6NXH9T2g@mail.gmail.com","subject":"Re: [PATCH 12/23] ref_transaction_commit(): break into multiple functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T07:58:30Z","receivedAt":"2017-05-19T07:58:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 07:44 PM, Stefan Beller wrote:\n> On Wed, May 17, 2017 at 5:05 AM, Michael Haggerty wrote:\n>> Break the function `ref_transaction_commit()` into two functions,\n>> `ref_transaction_prepare()` and `ref_transaction_finish()`, with a\n>> third function, `ref_transaction_abort()`, that can be used to abort\n>> the transaction. Break up the `ref_store` methods similarly.\n>>\n>> This split will make it easier to implement compound reference stores,\n>> where a transaction might have to span multiple underlying stores. In\n>> that case, we would first want to \"prepare\" the sub-transactions in\n>> each of the reference stores. If any of the \"prepare\" steps fails, we\n>> would \"abort\" all of the sub-transactions. Only if all of the\n>> \"prepare\" steps succeed would we \"finish\" each of them.\n> [...]\n\nThanks for your comments. While I was incorporating them, I realized\nthat other parts of the documentation needed updates, too. And while I\nwas fixing that, I noticed that the interface was unnecessarily\ncomplicated. The old version required either `commit` or `prepare`\nfollowed by `finish`. But there is no reason that the public API has to\nexpose `finish`. So instead, let's call `prepare` an optional step that\nis allowed to precede `commit`, and make `commit` smart enough to call\n`prepare` if it hasn't been called already, and then call `finish`.\n\nFurthermore, let's make it possible to call `abort` when the transaction\nis in OPEN as well as PREPARED state rather than requiring `free` in\nOPEN state and `abort` in PREPARED state.\n\nI will make these changes and revamp the docs for v2.\n\nMichael\n\n"},{"id":"320251","messageId":"c02b630d-1936-aa21-d79c-03e42b103bb7@alum.mit.edu","threadId":"45990","inReplyTo":"20170517133835.s3m3giccjo3jqstg@sigill.intra.peff.net","subject":"Re: [PATCH 22/23] ref-filter: limit traversal to prefix","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-05-19T10:02:38Z","receivedAt":"2017-05-19T10:02:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/17/2017 03:38 PM, Jeff King wrote:\n> On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:\n> \n>> From: Jeff King \n> \n> This patch did originate with me, but I know you had to fix several\n> things to integrate it in your series. So I'll review it anyway, and\n> give you full blame for any bugs. :)\n> \n>> When we are matching refnames \"as path\" against a pattern, then we\n>> know that the beginning of any refname that can match the pattern has\n>> to match the part of the pattern up to the first glob character. For\n>> example, if the pattern is `refs/heads/foo*bar`, then it can only\n>> match a reference that has the prefix `refs/heads/foo`.\n> \n> That first sentence confused me as to what \"as path\" meant (I know\n> because I worked on this code, and even then it took me a minute to\n> parse it).\n> \n> Maybe just \"When we are matching refnames against a pattern\" and then\n> later something like:\n> \n> Note 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\n> git-branch and git-tag are subtly different.\n\nThat is clearer. I'll make the change.\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.\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> If I were reviewing this from scratch, I'd probably ask whether it is OK\n> in:\n> \n> refs/heads/m*\n> \n> to return \"refs/heads/m\" as the prefix, and not stop at the last\n> non-wildcard component (\"refs/heads/\"). But I happen to know we\n> discussed this off-list and you checked that for_each_ref and friends\n> are happy with an arbitrary prefix. But I'm calling it out here for\n> other reviewers.\n\nI'll add a comment about this, too.\n\nThanks,\nMichael\n\n"}]}