{"thread":{"id":"46681","subject":"[PATCH 00/10] Implement transactions for the packed ref store","startedAt":"2017-08-29T08:20:52Z","lastAt":"2017-09-08T12:57:40Z","messageCount":31,"participants":["Michael Haggerty","Brandon Williams","Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"327304","messageId":"cover.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":null,"subject":"[PATCH 00/10] Implement transactions for the packed ref store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:24Z","receivedAt":"2017-08-29T08:20:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This should be the second-to-last patch series in the quest for\nmmapped packed-refs.\n\nBefore this patch series, there is still more coupling than necessary\nbetween `files_ref_store` and `packed_ref_store`:\n\n* `files_ref_store` adds packed references (e.g., during `git clone`\n and `git pack-refs`) by inserting them into the `packed_ref_store`'s\n internal `ref_cache`, then calling `commit_packed_refs()`. But\n `files_ref_store` shouldn't even *know* that `packed_ref_store` uses\n a `ref_cache`, let alone muck about in it.\n\n* `files_ref_store` deletes packed references by calling\n `repack_without_refs()`.\n\nInstead, implement reference transactions and `delete_refs()` for\n`packed_ref_store`, and change `files_ref_store` to use these standard\nmethods rather than internal `packed_ref_store` methods.\n\nThis patch series finishes up the encapsulation of `packed_ref_store`.\nAt the end of the series, the outside world only interacts with it via\nthe refs API plus a couple of locking-related functions. That will\nmake it easy for the next patch series to change the internal workings\nof `packed_ref_store` without affecting `files_ref_store`.\n\nAlong the way, we fix some longstanding problems with reference\nupdates. Quoting from patch [08/10]:\n\n First, the old code didn't obtain the `packed-refs` lock until\n `files_transaction_finish()`. This means that a failure to acquire the\n `packed-refs` lock (e.g., due to contention with another process)\n wasn't detected until it was too late (problems like this are supposed\n to be detected in the \"prepare\" phase). The new code acquires the\n `packed-refs` lock in `files_transaction_prepare()`, the same stage of\n the processing when the loose reference locks are being acquired,\n removing another reason why the \"prepare\" phase might succeed and the\n \"finish\" phase might nevertheless fail.\n\n Second, the old code deleted the loose version of a reference before\n deleting any packed version of the same reference. This left a moment\n when another process might think that the packed version of the\n reference is current, which is incorrect. (Even worse, the packed\n version of the reference can be arbitrarily old, and might even point\n at an object that has since been garbage-collected.)\n\n Third, if a reference deletion fails to acquire the `packed-refs` lock\n altogether, then the old code might leave the repository in the\n incorrect state (possibly corrupt) described in the previous\n paragraph.\n\n Now we activate the new \"packed-refs\" file (sans any references that\n are being deleted) *before* deleting the corresponding loose\n references. But we hold the \"packed-refs\" lock until after the loose\n references have been finalized, thus preventing a simultaneous\n \"pack-refs\" process from packing the loose version of the reference in\n the time gap, which would otherwise defeat our attempt to delete it.\n\nThis patch series is also available as branch\n`packed-ref-transactions` in my fork on GitHub [1]. A draft of the\npatch series to change `packed_ref_store` to use mmap and get rid of\nits `ref_cache` is available as branch `mmap-packed-refs` from the\nsame source.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (10):\n packed-backend: don't adjust the reference count on lock/unlock\n struct ref_transaction: add a place for backends to store data\n packed_ref_store: implement reference transactions\n packed_delete_refs(): implement method\n files_pack_refs(): use a reference transaction to write packed refs\n files_initial_transaction_commit(): use a transaction for packed refs\n t1404: demonstrate two problems with reference transactions\n files_ref_store: use a transaction to update packed refs\n packed-backend: rip out some now-unused code\n files_transaction_finish(): delete reflogs before references\n\n refs/files-backend.c | 200 ++++++++++++++-----\n refs/packed-backend.c | 457 +++++++++++++++++++++++++++++--------------\n refs/packed-backend.h | 17 +-\n refs/refs-internal.h | 1 +\n t/t1404-update-ref-errors.sh | 71 +++++++\n 5 files changed, 534 insertions(+), 212 deletions(-)\n\n-- \n2.14.1\n\n"},{"id":"327305","messageId":"2280d995f8516f296859ee583da112790e764b98.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 05/10] files_pack_refs(): use a reference transaction to write packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:29Z","receivedAt":"2017-08-29T08:21:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that the packed reference store supports transactions, we can use\na transaction to write the packed versions of references that we want\nto pack. This decreases the coupling between `files_ref_store` and\n`packed_ref_store`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 24 +++++++++++++++++-------\n 1 file changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2c78f63494..0fc44eaca5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1100,9 +1100,14 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tint ok;\n \tstruct ref_to_prune *refs_to_prune = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \n \tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);\n \n+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\treturn -1;\n+\n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n \t\t/*\n@@ -1115,12 +1120,14 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t\tcontinue;\n \n \t\t/*\n-\t\t * Create an entry in the packed-refs cache equivalent\n-\t\t * to the one from the loose ref cache, except that\n-\t\t * we don't copy the peeled status, because we want it\n-\t\t * to be re-peeled.\n+\t\t * Add a reference creation for this reference to the\n+\t\t * packed-refs transaction:\n \t\t */\n-\t\tadd_packed_ref(refs->packed_ref_store, iter->refname, iter->oid);\n+\t\tif (ref_transaction_update(transaction, iter->refname,\n+\t\t\t\t\t iter->oid->hash, NULL,\n+\t\t\t\t\t REF_NODEREF, NULL, &err))\n+\t\t\tdie(\"failure preparing to create packed reference %s: %s\",\n+\t\t\t iter->refname, err.buf);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n \t\tif ((flags & PACK_REFS_PRUNE)) {\n@@ -1134,8 +1141,11 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error while iterating over references\");\n \n-\tif (commit_packed_refs(refs->packed_ref_store, &err))\n-\t\tdie(\"unable to overwrite old ref-pack file: %s\", err.buf);\n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(\"unable to write new packed-refs: %s\", err.buf);\n+\n+\tref_transaction_free(transaction);\n+\n \tpacked_refs_unlock(refs->packed_ref_store);\n \n \tprune_refs(refs, refs_to_prune);\n-- \n2.14.1\n\n"},{"id":"327306","messageId":"7c61f04574e32d6172062a6a8ffa8ade5c90bb1d.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 04/10] packed_delete_refs(): implement method","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:28Z","receivedAt":"2017-08-29T08:21:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Implement `packed_delete_refs()` using a reference transaction. This\nmeans that `files_delete_refs()` can use `refs_delete_refs()` instead\nof `repack_without_refs()` to delete any packed references, decreasing\nthe coupling between the classes.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 2 +-\n refs/packed-backend.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 45 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fccbc24ac4..2c78f63494 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n \t\tgoto error;\n \n-\tif (repack_without_refs(refs->packed_ref_store, refnames, &err)) {\n+\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n \t\tpacked_refs_unlock(refs->packed_ref_store);\n \t\tgoto error;\n \t}\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex d19b3bfba5..83a088118f 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n-\tdie(\"BUG: not implemented yet\");\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n+\tstruct string_list_item *item;\n+\tint ret;\n+\n+\t(void)refs; /* We need the check above, but don't use the variable */\n+\n+\tif (!refnames->nr)\n+\t\treturn 0;\n+\n+\t/*\n+\t * Since we don't check the references' old_oids, the\n+\t * individual updates can't fail, so we can pack all of the\n+\t * updates into a single transaction.\n+\t */\n+\n+\ttransaction = ref_store_transaction_begin(ref_store, &err);\n+\tif (!transaction)\n+\t\treturn -1;\n+\n+\tfor_each_string_list_item(item, refnames) {\n+\t\tif (ref_transaction_delete(transaction, item->string, NULL,\n+\t\t\t\t\t flags, msg, &err)) {\n+\t\t\twarning(_(\"could not delete reference %s: %s\"),\n+\t\t\t\titem->string, err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t}\n+\t}\n+\n+\tret = ref_transaction_commit(transaction, &err);\n+\n+\tif (ret) {\n+\t\tif (refnames->nr == 1)\n+\t\t\terror(_(\"could not delete reference %s: %s\"),\n+\t\t\t refnames->items[0].string, err.buf);\n+\t\telse\n+\t\t\terror(_(\"could not delete references: %s\"), err.buf);\n+\t}\n+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n+\treturn ret;\n }\n \n static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)\n-- \n2.14.1\n\n"},{"id":"327307","messageId":"92b29defc3c4ddb65a948b7d142ded941eaccefd.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:30Z","receivedAt":"2017-08-29T08:21:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Used a `packed_ref_store` transaction in the implementation of\n`files_initial_transaction_commit()` rather than using internal\nfeatures of the packed ref store. This further decouples\n`files_ref_store` from `packed_ref_store`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 29 +++++++++++++++++++----------\n 1 file changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0fc44eaca5..29c7c78602 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2663,6 +2663,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \tsize_t i;\n \tint ret = 0;\n \tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n+\tstruct ref_transaction *packed_transaction = NULL;\n \n \tassert(err);\n \n@@ -2695,6 +2696,12 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &affected_refnames))\n \t\tdie(\"BUG: initial ref transaction called with existing refs\");\n \n+\tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);\n+\tif (!packed_transaction) {\n+\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n \n@@ -2707,6 +2714,15 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\tret = TRANSACTION_NAME_CONFLICT;\n \t\t\tgoto cleanup;\n \t\t}\n+\n+\t\t/*\n+\t\t * Add a reference creation for this reference to the\n+\t\t * packed-refs transaction:\n+\t\t */\n+\t\tref_transaction_add_update(packed_transaction, update->refname,\n+\t\t\t\t\t update->flags & ~REF_HAVE_OLD,\n+\t\t\t\t\t update->new_oid.hash, update->old_oid.hash,\n+\t\t\t\t\t NULL);\n \t}\n \n \tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n@@ -2714,21 +2730,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = transaction->updates[i];\n-\n-\t\tif ((update->flags & REF_HAVE_NEW) &&\n-\t\t !is_null_oid(&update->new_oid))\n-\t\t\tadd_packed_ref(refs->packed_ref_store, update->refname,\n-\t\t\t\t &update->new_oid);\n-\t}\n-\n-\tif (commit_packed_refs(refs->packed_ref_store, err)) {\n+\tif (initial_ref_transaction_commit(packed_transaction, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n \n cleanup:\n+\tif (packed_transaction)\n+\t\tref_transaction_free(packed_transaction);\n \tpacked_refs_unlock(refs->packed_ref_store);\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \tstring_list_clear(&affected_refnames, 0);\n-- \n2.14.1\n\n"},{"id":"327308","messageId":"caaa44126f18869158872e5473e53478db780ba9.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:31Z","receivedAt":"2017-08-29T08:21:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Currently, a loose reference is deleted even before locking the\n`packed-refs` file, let alone deleting any packed version of the\nreference. This leads to two problems, demonstrated by two new tests:\n\n* While a reference is being deleted, other processes might see the\n old, packed value of the reference for a moment before the packed\n version is deleted. Normally this would be hard to observe, but we\n can prolong the window by locking the `packed-refs` file externally\n before running `update-ref`, then unlocking it before `update-ref`'s\n attempt to acquire the lock times out.\n\n* If the `packed-refs` file is locked so long that `update-ref` fails\n to lock it, then the reference can be left permanently in the\n incorrect state described in the previous point.\n\nIn a moment, both problems will be fixed.\n\nSigned-off-by: Michael Haggerty \n---\nThe first of the new tests is rather involved; it uses two background\nprocesses plus a foreground process that polls the value of a\nreference. But despite sensitivity to timing, I think it should be\nrobust even in this broken state. Once the functionality being tested\nis fixed, this test should never produce false positives, though\nreally bad timing (e.g., if it takes more than a second for\n`update-ref` to get going) could still lead to false negatives.\n\nEach of the new tests takes about a second to run because they\nsimulate lock contention.\n\nIf anybody has suggestions for better ways to test these things,\nplease speak up :-)\n\n t/t1404-update-ref-errors.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex c34ece48f5..752f83c377 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -404,4 +404,75 @@ test_expect_success 'broken reference blocks indirect create' '\n \ttest_cmp expected output.err\n '\n \n+test_expect_failure 'no bogus intermediate values during delete' '\n+\tprefix=refs/slow-transaction &&\n+\t# Set up a reference with differing loose and packed versions:\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tgit update-ref $prefix/foo $D &&\n+\tgit for-each-ref $prefix >unchanged &&\n+\t# Now try to update the reference, but hold the `packed-refs` lock\n+\t# for a while to see what happens while the process is blocked:\n+\t: >.git/packed-refs.lock &&\n+\ttest_when_finished \"rm -f .git/packed-refs.lock\" &&\n+\t{\n+\t\tsleep 1 &&\n+\t\trm -f .git/packed-refs.lock &\n+\t} &&\n+\tpid1=$! &&\n+\t{\n+\t\t# Note: the following command is intentionally run in the\n+\t\t# background. We extend the timeout so that `update-ref`\n+\t\t# tries to acquire the `packed-refs` lock longer than it\n+\t\t# takes the background process above to delete it:\n+\t\tgit -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &\n+\t} &&\n+\tpid2=$! &&\n+\tok=true &&\n+\twhile kill -0 $pid2 2>/dev/null\n+\tdo\n+\t\tsha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&\n+\t\tcase \"$sha1\" in\n+\t\t$D)\n+\t\t\t# This is OK; it just means that nothing has happened yet.\n+\t\t\t: ;;\n+\t\tundefined)\n+\t\t\t# This is OK; it means the deletion was successful.\n+\t\t\t: ;;\n+\t\t$C)\n+\t\t\t# This value should never be seen. Probably the loose\n+\t\t\t# reference has been deleted but the packed reference\n+\t\t\t# is still there:\n+\t\t\techo \"$prefix/foo incorrectly observed to be C\" &&\n+\t\t\tbreak\n+\t\t\t;;\n+\t\t*)\n+\t\t\t# WTF?\n+\t\t\techo \"$prefix/foo unexpected value observed: $sha1\" &&\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\tdone >out &&\n+\twait $pid1 &&\n+\twait $pid2 &&\n+\ttest_must_be_empty out &&\n+\ttest_must_fail git rev-parse --verify --quiet $prefix/foo\n+'\n+\n+test_expect_failure 'delete fails cleanly if packed-refs file is locked' '\n+\tprefix=refs/locked-packed-refs &&\n+\t# Set up a reference with differing loose and packed versions:\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tgit update-ref $prefix/foo $D &&\n+\tgit for-each-ref $prefix >unchanged &&\n+\t# Now try to delete it while the `packed-refs` lock is held:\n+\t: >.git/packed-refs.lock &&\n+\ttest_when_finished \"rm -f .git/packed-refs.lock\" &&\n+\ttest_must_fail git update-ref -d $prefix/foo >out 2>err &&\n+\tgit for-each-ref $prefix >actual &&\n+\tgrep \"Unable to create $Q.*packed-refs.lock$Q: File exists\" err &&\n+\ttest_cmp unchanged actual\n+'\n+\n test_done\n-- \n2.14.1\n\n"},{"id":"327309","messageId":"824ad441f8531f1bbcc8800a14fe71466721066d.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 09/10] packed-backend: rip out some now-unused code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:33Z","receivedAt":"2017-08-29T08:21:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now the outside world interacts with the packed ref store only via the\ngeneric refs API plus a few lock-related functions. This allows us to\ndelete some functions that are no longer used, thereby completing the\nencapsulation of the packed ref store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 193 --------------------------------------------------\n refs/packed-backend.h | 8 ---\n 2 files changed, 201 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 83a088118f..90f44c1fbb 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -91,19 +91,6 @@ struct ref_store *packed_ref_store_create(const char *path,\n \treturn ref_store;\n }\n \n-/*\n- * Die if refs is not the main ref store. caller is used in any\n- * necessary error messages.\n- */\n-static void packed_assert_main_repository(struct packed_ref_store *refs,\n-\t\t\t\t\t const char *caller)\n-{\n-\tif (refs->store_flags & REF_STORE_MAIN)\n-\t\treturn;\n-\n-\tdie(\"BUG: operation %s only allowed for main ref store\", caller);\n-}\n-\n /*\n * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is\n * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't\n@@ -321,40 +308,6 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n \treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n-/*\n- * Add or overwrite a reference in the in-memory packed reference\n- * cache. This may only be called while the packed-refs file is locked\n- * (see packed_refs_lock()). To actually write the packed-refs file,\n- * call commit_packed_refs().\n- */\n-void add_packed_ref(struct ref_store *ref_store,\n-\t\t const char *refname, const struct object_id *oid)\n-{\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE,\n-\t\t\t\t\"add_packed_ref\");\n-\tstruct ref_dir *packed_refs;\n-\tstruct ref_entry *packed_entry;\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: packed refs not locked\");\n-\n-\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n-\t\tdie(\"Reference has invalid format: '%s'\", refname);\n-\n-\tpacked_refs = get_packed_refs(refs);\n-\tpacked_entry = find_ref_entry(packed_refs, refname);\n-\tif (packed_entry) {\n-\t\t/* Overwrite the existing entry: */\n-\t\toidcpy(&packed_entry->u.value.oid, oid);\n-\t\tpacked_entry->flag = REF_ISPACKED;\n-\t\toidclr(&packed_entry->u.value.peeled);\n-\t} else {\n-\t\tpacked_entry = create_ref_entry(refname, oid, REF_ISPACKED);\n-\t\tadd_ref_entry(packed_refs, packed_entry);\n-\t}\n-}\n-\n /*\n * Return the ref_entry for the given refname from the packed\n * references. If it does not exist, return NULL.\n@@ -592,152 +545,6 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n static const char PACKED_REFS_HEADER[] =\n \t\"# pack-refs with: peeled fully-peeled \\n\";\n \n-/*\n- * Write the current version of the packed refs cache from memory to\n- * disk. The packed-refs file must already be locked for writing (see\n- * packed_refs_lock()). Return zero on success. On errors, rollback\n- * the lockfile, write an error message to `err`, and return a nonzero\n- * value.\n- */\n-int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n-{\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n-\t\t\t\t\"commit_packed_refs\");\n-\tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs);\n-\tint ok;\n-\tint ret = -1;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tFILE *out;\n-\tstruct ref_iterator *iter;\n-\tchar *packed_refs_path;\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: commit_packed_refs() called when unlocked\");\n-\n-\t/*\n-\t * If packed-refs is a symlink, we want to overwrite the\n-\t * symlinked-to file, not the symlink itself. Also, put the\n-\t * staging file next to it:\n-\t */\n-\tpacked_refs_path = get_locked_file_path(&refs->lock);\n-\tstrbuf_addf(&sb, \"%s.new\", packed_refs_path);\n-\tif (create_tempfile(&refs->tempfile, sb.buf) < 0) {\n-\t\tstrbuf_addf(err, \"unable to create file %s: %s\",\n-\t\t\t sb.buf, strerror(errno));\n-\t\tstrbuf_release(&sb);\n-\t\tgoto out;\n-\t}\n-\tstrbuf_release(&sb);\n-\n-\tout = fdopen_tempfile(&refs->tempfile, \"w\");\n-\tif (!out) {\n-\t\tstrbuf_addf(err, \"unable to fdopen packed-refs tempfile: %s\",\n-\t\t\t strerror(errno));\n-\t\tgoto error;\n-\t}\n-\n-\tif (fprintf(out, \"%s\", PACKED_REFS_HEADER) < 0) {\n-\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n-\t\t\t get_tempfile_path(&refs->tempfile), strerror(errno));\n-\t\tgoto error;\n-\t}\n-\n-\titer = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);\n-\twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n-\t\tstruct object_id peeled;\n-\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n-\n-\t\tif (write_packed_entry(out, iter->refname, iter->oid->hash,\n-\t\t\t\t peel_error ? NULL : peeled.hash)) {\n-\t\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n-\t\t\t\t get_tempfile_path(&refs->tempfile),\n-\t\t\t\t strerror(errno));\n-\t\t\tref_iterator_abort(iter);\n-\t\t\tgoto error;\n-\t\t}\n-\t}\n-\n-\tif (ok != ITER_DONE) {\n-\t\tstrbuf_addf(err, \"unable to rewrite packed-refs file: \"\n-\t\t\t \"error iterating over old contents\");\n-\t\tgoto error;\n-\t}\n-\n-\tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n-\t\tstrbuf_addf(err, \"error replacing %s: %s\",\n-\t\t\t refs->path, strerror(errno));\n-\t\tgoto out;\n-\t}\n-\n-\tret = 0;\n-\tgoto out;\n-\n-error:\n-\tdelete_tempfile(&refs->tempfile);\n-\n-out:\n-\tfree(packed_refs_path);\n-\treturn ret;\n-}\n-\n-/*\n- * Rewrite the packed-refs file, omitting any refs listed in\n- * 'refnames'. On error, leave packed-refs unchanged, write an error\n- * message to 'err', and return a nonzero value. The packed refs lock\n- * must be held when calling this function; it will still be held when\n- * the function returns.\n- *\n- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n- */\n-int repack_without_refs(struct ref_store *ref_store,\n-\t\t\tstruct string_list *refnames, struct strbuf *err)\n-{\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n-\t\t\t\t\"repack_without_refs\");\n-\tstruct ref_dir *packed;\n-\tstruct string_list_item *refname;\n-\tint needs_repacking = 0, removed = 0;\n-\n-\tpacked_assert_main_repository(refs, \"repack_without_refs\");\n-\tassert(err);\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: repack_without_refs called without holding lock\");\n-\n-\t/* Look for a packed ref */\n-\tfor_each_string_list_item(refname, refnames) {\n-\t\tif (get_packed_ref(refs, refname->string)) {\n-\t\t\tneeds_repacking = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\t/* Avoid locking if we have nothing to do */\n-\tif (!needs_repacking)\n-\t\treturn 0; /* no refname exists in packed refs */\n-\n-\tpacked = get_packed_refs(refs);\n-\n-\t/* Remove refnames from the cache */\n-\tfor_each_string_list_item(refname, refnames)\n-\t\tif (remove_entry_from_dir(packed, refname->string) != -1)\n-\t\t\tremoved = 1;\n-\tif (!removed) {\n-\t\t/*\n-\t\t * All packed entries disappeared while we were\n-\t\t * acquiring the lock.\n-\t\t */\n-\t\tclear_packed_ref_cache(refs);\n-\t\treturn 0;\n-\t}\n-\n-\t/* Write what remains */\n-\treturn commit_packed_refs(&refs->base, err);\n-}\n-\n static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n {\n \t/* Nothing to do. */\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 7af2897757..61687e408a 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -23,12 +23,4 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n void packed_refs_unlock(struct ref_store *ref_store);\n int packed_refs_is_locked(struct ref_store *ref_store);\n \n-void add_packed_ref(struct ref_store *ref_store,\n-\t\t const char *refname, const struct object_id *oid);\n-\n-int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err);\n-\n-int repack_without_refs(struct ref_store *ref_store,\n-\t\t\tstruct string_list *refnames, struct strbuf *err);\n-\n #endif /* REFS_PACKED_BACKEND_H */\n-- \n2.14.1\n\n"},{"id":"327310","messageId":"25b38730abe97c29d04116b4b2c6067934dfca14.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 08/10] files_ref_store: use a transaction to update packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:32Z","receivedAt":"2017-08-29T08:21:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When processing a `files_ref_store` transaction, it is sometimes\nnecessary to delete some references from the \"packed-refs\" file. Do\nthat using a reference transaction conducted against the\n`packed_ref_store`.\n\nThis change further decouples `files_ref_store` from\n`packed_ref_store`. It also fixes multiple problems, including the two\nrevealed by test cases added in the previous commit.\n\nFirst, the old code didn't obtain the `packed-refs` lock until\n`files_transaction_finish()`. This means that a failure to acquire the\n`packed-refs` lock (e.g., due to contention with another process)\nwasn't detected until it was too late (problems like this are supposed\nto be detected in the \"prepare\" phase). The new code acquires the\n`packed-refs` lock in `files_transaction_prepare()`, the same stage of\nthe processing when the loose reference locks are being acquired,\nremoving another reason why the \"prepare\" phase might succeed and the\n\"finish\" phase might nevertheless fail.\n\nSecond, the old code deleted the loose version of a reference before\ndeleting any packed version of the same reference. This left a moment\nwhen another process might think that the packed version of the\nreference is current, which is incorrect. (Even worse, the packed\nversion of the reference can be arbitrarily old, and might even point\nat an object that has since been garbage-collected.)\n\nThird, if a reference deletion fails to acquire the `packed-refs` lock\naltogether, then the old code might leave the repository in the\nincorrect state (possibly corrupt) described in the previous\nparagraph.\n\nNow we activate the new \"packed-refs\" file (sans any references that\nare being deleted) *before* deleting the corresponding loose\nreferences. But we hold the \"packed-refs\" lock until after the loose\nreferences have been finalized, thus preventing a simultaneous\n\"pack-refs\" process from packing the loose version of the reference in\nthe time gap, which would otherwise defeat our attempt to delete it.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 132 +++++++++++++++++++++++++++++++++----------\n t/t1404-update-ref-errors.sh | 4 +-\n 2 files changed, 103 insertions(+), 33 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 29c7c78602..4f4c47b9db 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2391,13 +2391,22 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \treturn 0;\n }\n \n+struct files_transaction_backend_data {\n+\tstruct ref_transaction *packed_transaction;\n+\tint packed_refs_locked;\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+static void files_transaction_cleanup(struct files_ref_store *refs,\n+\t\t\t\t struct ref_transaction *transaction)\n {\n \tsize_t i;\n+\tstruct files_transaction_backend_data *backend_data =\n+\t\ttransaction->backend_data;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -2409,6 +2418,17 @@ static void files_transaction_cleanup(struct ref_transaction *transaction)\n \t\t}\n \t}\n \n+\tif (backend_data->packed_transaction &&\n+\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n+\t\terror(\"error aborting transaction: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t}\n+\n+\tif (backend_data->packed_refs_locked)\n+\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\n+\tfree(backend_data);\n+\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n@@ -2425,12 +2445,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \tchar *head_ref = NULL;\n \tint head_type;\n \tstruct object_id head_oid;\n+\tstruct files_transaction_backend_data *backend_data;\n+\tstruct ref_transaction *packed_transaction = NULL;\n \n \tassert(err);\n \n \tif (!transaction->nr)\n \t\tgoto cleanup;\n \n+\tbackend_data = xcalloc(1, sizeof(*backend_data));\n+\ttransaction->backend_data = backend_data;\n+\n \t/*\n \t * Fail if a refname appears more than once in the\n \t * transaction. (If we end up splitting up any updates using\n@@ -2497,6 +2522,41 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t head_ref, &affected_refnames, err);\n \t\tif (ret)\n \t\t\tbreak;\n+\n+\t\tif (update->flags & REF_DELETING &&\n+\t\t !(update->flags & REF_LOG_ONLY) &&\n+\t\t !(update->flags & REF_ISPRUNING)) {\n+\t\t\t/*\n+\t\t\t * This reference has to be deleted from\n+\t\t\t * packed-refs if it exists there.\n+\t\t\t */\n+\t\t\tif (!packed_transaction) {\n+\t\t\t\tpacked_transaction = ref_store_transaction_begin(\n+\t\t\t\t\t\trefs->packed_ref_store, err);\n+\t\t\t\tif (!packed_transaction) {\n+\t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\t\tgoto cleanup;\n+\t\t\t\t}\n+\n+\t\t\t\tbackend_data->packed_transaction =\n+\t\t\t\t\tpacked_transaction;\n+\t\t\t}\n+\n+\t\t\tref_transaction_add_update(\n+\t\t\t\t\tpacked_transaction, update->refname,\n+\t\t\t\t\tupdate->flags & ~REF_HAVE_OLD,\n+\t\t\t\t\tupdate->new_oid.hash, update->old_oid.hash,\n+\t\t\t\t\tNULL);\n+\t\t}\n+\t}\n+\n+\tif (packed_transaction) {\n+\t\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tbackend_data->packed_refs_locked = 1;\n+\t\tret = ref_transaction_prepare(packed_transaction, err);\n \t}\n \n cleanup:\n@@ -2504,7 +2564,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \tstring_list_clear(&affected_refnames, 0);\n \n \tif (ret)\n-\t\tfiles_transaction_cleanup(transaction);\n+\t\tfiles_transaction_cleanup(refs, transaction);\n \telse\n \t\ttransaction->state = REF_TRANSACTION_PREPARED;\n \n@@ -2519,9 +2579,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\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+\tstruct files_transaction_backend_data *backend_data;\n+\tstruct ref_transaction *packed_transaction;\n+\n \n \tassert(err);\n \n@@ -2530,6 +2591,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\treturn 0;\n \t}\n \n+\tbackend_data = transaction->backend_data;\n+\tpacked_transaction = backend_data->packed_transaction;\n+\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -2565,7 +2629,23 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t\t}\n \t\t}\n \t}\n-\t/* Perform deletes now that updates are safely completed */\n+\n+\t/*\n+\t * Perform deletes now that updates are safely completed.\n+\t *\n+\t * First delete any packed versions of the references, while\n+\t * retaining the packed-refs lock:\n+\t */\n+\tif (packed_transaction) {\n+\t\tret = ref_transaction_commit(packed_transaction, err);\n+\t\tref_transaction_free(packed_transaction);\n+\t\tpacked_transaction = NULL;\n+\t\tbackend_data->packed_transaction = NULL;\n+\t\tif (ret)\n+\t\t\tgoto cleanup;\n+\t}\n+\n+\t/* Now delete the loose versions of the references: */\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@@ -2583,39 +2663,27 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t\t\t}\n \t\t\t\tupdate->flags |= REF_DELETED_LOOSE;\n \t\t\t}\n-\n-\t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tstring_list_append(&refs_to_delete,\n-\t\t\t\t\t\t lock->ref_name);\n \t\t}\n \t}\n \n-\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n-\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto cleanup;\n-\t}\n-\n-\tif (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {\n-\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n-\t\tgoto cleanup;\n-\t}\n-\n-\tpacked_refs_unlock(refs->packed_ref_store);\n-\n \t/* Delete the reflogs of any references that were deleted: */\n-\tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n-\t\tstrbuf_reset(&sb);\n-\t\tfiles_reflog_path(refs, &sb, ref_to_delete->string);\n-\t\tif (!unlink_or_warn(sb.buf))\n-\t\t\ttry_remove_empty_parents(refs, ref_to_delete->string,\n-\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REFLOG);\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tif (update->flags & REF_DELETING &&\n+\t\t !(update->flags & REF_LOG_ONLY) &&\n+\t\t !(update->flags & REF_ISPRUNING)) {\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tfiles_reflog_path(refs, &sb, update->refname);\n+\t\t\tif (!unlink_or_warn(sb.buf))\n+\t\t\t\ttry_remove_empty_parents(refs, update->refname,\n+\t\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REFLOG);\n+\t\t}\n \t}\n \n \tclear_loose_ref_cache(refs);\n \n cleanup:\n-\tfiles_transaction_cleanup(transaction);\n+\tfiles_transaction_cleanup(refs, transaction);\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -2633,7 +2701,6 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t}\n \n \tstrbuf_release(&sb);\n-\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \n@@ -2641,7 +2708,10 @@ static int files_transaction_abort(struct ref_store *ref_store,\n \t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t struct strbuf *err)\n {\n-\tfiles_transaction_cleanup(transaction);\n+\tstruct files_ref_store *refs =\n+\t\tfiles_downcast(ref_store, 0, \"ref_transaction_abort\");\n+\n+\tfiles_transaction_cleanup(refs, transaction);\n \treturn 0;\n }\n \ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 752f83c377..26e5ff6843 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -404,7 +404,7 @@ test_expect_success 'broken reference blocks indirect create' '\n \ttest_cmp expected output.err\n '\n \n-test_expect_failure 'no bogus intermediate values during delete' '\n+test_expect_success 'no bogus intermediate values during delete' '\n \tprefix=refs/slow-transaction &&\n \t# Set up a reference with differing loose and packed versions:\n \tgit update-ref $prefix/foo $C &&\n@@ -459,7 +459,7 @@ test_expect_failure 'no bogus intermediate values during delete' '\n \ttest_must_fail git rev-parse --verify --quiet $prefix/foo\n '\n \n-test_expect_failure 'delete fails cleanly if packed-refs file is locked' '\n+test_expect_success 'delete fails cleanly if packed-refs file is locked' '\n \tprefix=refs/locked-packed-refs &&\n \t# Set up a reference with differing loose and packed versions:\n \tgit update-ref $prefix/foo $C &&\n-- \n2.14.1\n\n"},{"id":"327311","messageId":"b147389b4abfbd8975fe651d956fded34f5b90cd.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 10/10] files_transaction_finish(): delete reflogs before references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:34Z","receivedAt":"2017-08-29T08:21:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the deletion steps unexpectedly fail, it is less bad to leave a\nreference without its reflog than it is to leave a reflog without its\nreference, since the latter is an invalid repository state.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 35 +++++++++++++++++++++--------------\n 1 file changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4f4c47b9db..36f81b4f28 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2630,6 +2630,27 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t}\n \t}\n \n+\t/*\n+\t * Now that updates are safely completed, we can perform\n+\t * deletes. First delete the reflogs of any references that\n+\t * will be deleted, since (in the unexpected event of an\n+\t * error) leaving a reference without a reflog is less bad\n+\t * than leaving a reflog without a reference (the latter is a\n+\t * mildly invalid repository state):\n+\t */\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tif (update->flags & REF_DELETING &&\n+\t\t !(update->flags & REF_LOG_ONLY) &&\n+\t\t !(update->flags & REF_ISPRUNING)) {\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tfiles_reflog_path(refs, &sb, update->refname);\n+\t\t\tif (!unlink_or_warn(sb.buf))\n+\t\t\t\ttry_remove_empty_parents(refs, update->refname,\n+\t\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REFLOG);\n+\t\t}\n+\t}\n+\n \t/*\n \t * Perform deletes now that updates are safely completed.\n \t *\n@@ -2666,20 +2687,6 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t}\n \t}\n \n-\t/* Delete the reflogs of any references that were deleted: */\n-\tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = transaction->updates[i];\n-\t\tif (update->flags & REF_DELETING &&\n-\t\t !(update->flags & REF_LOG_ONLY) &&\n-\t\t !(update->flags & REF_ISPRUNING)) {\n-\t\t\tstrbuf_reset(&sb);\n-\t\t\tfiles_reflog_path(refs, &sb, update->refname);\n-\t\t\tif (!unlink_or_warn(sb.buf))\n-\t\t\t\ttry_remove_empty_parents(refs, update->refname,\n-\t\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REFLOG);\n-\t\t}\n-\t}\n-\n \tclear_loose_ref_cache(refs);\n \n cleanup:\n-- \n2.14.1\n\n"},{"id":"327312","messageId":"c5d66cadb70efdb5b5ce91fbcc5673621f5d4792.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:25Z","receivedAt":"2017-08-29T08:21:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code incremented the packed ref cache reference count when\nacquiring the packed-refs lock, and decremented the count when\nreleasing the lock. This is unnecessary because a locked packed-refs\nfile cannot be changed, so there is no reason for the cache to become\nstale.\n\nMoreover, the extra reference count causes a problem if we\nintentionally clear the packed refs cache, as we sometimes need to do\nif we change the cache in anticipation of writing a change to disk,\nbut then the write to disk fails. In that case, `packed_refs_unlock()`\nwould have no easy way to find the cache whose reference count it\nneeds to decrement.\n\nThis whole issue will soon become moot due to upcoming changes that\navoid changing the in-memory cache as part of updating the packed-refs\non disk, but this change makes that transition easier.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 412c85034f..7e348feac3 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -525,7 +525,6 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t\t\t\t\"packed_refs_lock\");\n \tstatic int timeout_configured = 0;\n \tstatic int timeout_value = 1000;\n-\tstruct packed_ref_cache *packed_ref_cache;\n \n \tif (!timeout_configured) {\n \t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n@@ -560,9 +559,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \t */\n \tvalidate_packed_ref_cache(refs);\n \n-\tpacked_ref_cache = get_packed_ref_cache(refs);\n-\t/* Increment the reference count to prevent it from being freed: */\n-\tacquire_packed_ref_cache(packed_ref_cache);\n+\tget_packed_ref_cache(refs);\n \treturn 0;\n }\n \n@@ -576,7 +573,6 @@ void packed_refs_unlock(struct ref_store *ref_store)\n \tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: packed_refs_unlock() called when not locked\");\n \trollback_lock_file(&refs->lock);\n-\trelease_packed_ref_cache(refs->cache);\n }\n \n int packed_refs_is_locked(struct ref_store *ref_store)\n-- \n2.14.1\n\n"},{"id":"327313","messageId":"5b8c8f9ab0ccd5f85eb43a7a1535af89b0af85e3.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 02/10] struct ref_transaction: add a place for backends to store data","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:26Z","receivedAt":"2017-08-29T08:22:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"`packed_ref_store` is going to want to store some transaction-wide\ndata, so make a place for it.\n\nSigned-off-by: Michael Haggerty \n---\n refs/refs-internal.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex b02dc5a7e3..d7d344de73 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -242,6 +242,7 @@ struct ref_transaction {\n \tsize_t alloc;\n \tsize_t nr;\n \tenum ref_transaction_state state;\n+\tvoid *backend_data;\n };\n \n /*\n-- \n2.14.1\n\n"},{"id":"327314","messageId":"9e1e41f9bdb9b3e2b274e0f5075bcc5b5fb5390f.1503993268.git.mhagger@alum.mit.edu","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"[PATCH 03/10] packed_ref_store: implement reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:20:27Z","receivedAt":"2017-08-29T08:22:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Implement the methods needed to support reference transactions for\nthe packed-refs backend. The new methods are not yet used.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 313 +++++++++++++++++++++++++++++++++++++++++++++++++-\n refs/packed-backend.h | 9 ++\n 2 files changed, 319 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 7e348feac3..d19b3bfba5 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -744,25 +744,332 @@ static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n \treturn 0;\n }\n \n+/*\n+ * Write the packed-refs from the cache to the packed-refs tempfile,\n+ * incorporating any changes from `updates`. `updates` must be a\n+ * sorted string list whose keys are the refnames and whose util\n+ * values are `struct ref_update *`. On error, rollback the tempfile,\n+ * write an error message to `err`, and return a nonzero value.\n+ *\n+ * The packfile must be locked before calling this function and will\n+ * remain locked when it is done.\n+ */\n+static int write_with_updates(struct packed_ref_store *refs,\n+\t\t\t struct string_list *updates,\n+\t\t\t struct strbuf *err)\n+{\n+\tstruct ref_iterator *iter = NULL;\n+\tsize_t i;\n+\tint ok;\n+\tFILE *out;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *packed_refs_path;\n+\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tdie(\"BUG: write_with_updates() called while unlocked\");\n+\n+\t/*\n+\t * If packed-refs is a symlink, we want to overwrite the\n+\t * symlinked-to file, not the symlink itself. Also, put the\n+\t * staging file next to it:\n+\t */\n+\tpacked_refs_path = get_locked_file_path(&refs->lock);\n+\tstrbuf_addf(&sb, \"%s.new\", packed_refs_path);\n+\tfree(packed_refs_path);\n+\tif (create_tempfile(&refs->tempfile, sb.buf) < 0) {\n+\t\tstrbuf_addf(err, \"unable to create file %s: %s\",\n+\t\t\t sb.buf, strerror(errno));\n+\t\tstrbuf_release(&sb);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&sb);\n+\n+\tout = fdopen_tempfile(&refs->tempfile, \"w\");\n+\tif (!out) {\n+\t\tstrbuf_addf(err, \"unable to fdopen packed-refs tempfile: %s\",\n+\t\t\t strerror(errno));\n+\t\tgoto error;\n+\t}\n+\n+\tif (fprintf(out, \"%s\", PACKED_REFS_HEADER) < 0)\n+\t\tgoto write_error;\n+\n+\t/*\n+\t * We iterate in parallel through the current list of refs and\n+\t * the list of updates, processing an entry from at least one\n+\t * of the lists each time through the loop. When the current\n+\t * list of refs is exhausted, set iter to NULL. When the list\n+\t * of updates is exhausted, leave i set to updates->nr.\n+\t */\n+\titer = packed_ref_iterator_begin(&refs->base, \"\",\n+\t\t\t\t\t DO_FOR_EACH_INCLUDE_BROKEN);\n+\tif ((ok = ref_iterator_advance(iter)) != ITER_OK)\n+\t\titer = NULL;\n+\n+\ti = 0;\n+\n+\twhile (iter || i < updates->nr) {\n+\t\tstruct ref_update *update = NULL;\n+\t\tint cmp;\n+\n+\t\tif (i >= updates->nr) {\n+\t\t\tcmp = -1;\n+\t\t} else {\n+\t\t\tupdate = updates->items[i].util;\n+\n+\t\t\tif (!iter)\n+\t\t\t\tcmp = +1;\n+\t\t\telse\n+\t\t\t\tcmp = strcmp(iter->refname, update->refname);\n+\t\t}\n+\n+\t\tif (!cmp) {\n+\t\t\t/*\n+\t\t\t * There is both an old value and an update\n+\t\t\t * for this reference. Check the old value if\n+\t\t\t * necessary:\n+\t\t\t */\n+\t\t\tif ((update->flags & REF_HAVE_OLD)) {\n+\t\t\t\tif (is_null_oid(&update->old_oid)) {\n+\t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n+\t\t\t\t\t\t \"reference already exists\",\n+\t\t\t\t\t\t update->refname);\n+\t\t\t\t\tgoto error;\n+\t\t\t\t} else if (oidcmp(&update->old_oid, iter->oid)) {\n+\t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n+\t\t\t\t\t\t \"is at %s but expected %s\",\n+\t\t\t\t\t\t update->refname,\n+\t\t\t\t\t\t oid_to_hex(iter->oid),\n+\t\t\t\t\t\t oid_to_hex(&update->old_oid));\n+\t\t\t\t\tgoto error;\n+\t\t\t\t}\n+\t\t\t}\n+\n+\t\t\t/* Now figure out what to use for the new value: */\n+\t\t\tif ((update->flags & REF_HAVE_NEW)) {\n+\t\t\t\t/*\n+\t\t\t\t * The update takes precedence. Skip\n+\t\t\t\t * the iterator over the unneeded\n+\t\t\t\t * value.\n+\t\t\t\t */\n+\t\t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK)\n+\t\t\t\t\titer = NULL;\n+\t\t\t\tcmp = +1;\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * The update doesn't actually want to\n+\t\t\t\t * change anything. We're done with it.\n+\t\t\t\t */\n+\t\t\t\ti++;\n+\t\t\t\tcmp = -1;\n+\t\t\t}\n+\t\t} else if (cmp > 0) {\n+\t\t\t/*\n+\t\t\t * There is no old value but there is an\n+\t\t\t * update for this reference. Make sure that\n+\t\t\t * the update didn't expect an existing value:\n+\t\t\t */\n+\t\t\tif ((update->flags & REF_HAVE_OLD) &&\n+\t\t\t !is_null_oid(&update->old_oid)) {\n+\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n+\t\t\t\t\t \"reference is missing but expected %s\",\n+\t\t\t\t\t update->refname,\n+\t\t\t\t\t oid_to_hex(&update->old_oid));\n+\t\t\t\tgoto error;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (cmp < 0) {\n+\t\t\t/* Pass the old reference through. */\n+\n+\t\t\tstruct object_id peeled;\n+\t\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n+\n+\t\t\tif (write_packed_entry(out, iter->refname,\n+\t\t\t\t\t iter->oid->hash,\n+\t\t\t\t\t peel_error ? NULL : peeled.hash))\n+\t\t\t\tgoto write_error;\n+\n+\t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK)\n+\t\t\t\titer = NULL;\n+\t\t} else if (is_null_oid(&update->new_oid)) {\n+\t\t\t/*\n+\t\t\t * The update wants to delete the reference,\n+\t\t\t * and the reference either didn't exist or we\n+\t\t\t * have already skipped it. So we're done with\n+\t\t\t * the update (and don't have to write\n+\t\t\t * anything).\n+\t\t\t */\n+\t\t\ti++;\n+\t\t} else {\n+\t\t\tstruct object_id peeled;\n+\t\t\tint peel_error = peel_object(update->new_oid.hash,\n+\t\t\t\t\t\t peeled.hash);\n+\n+\t\t\tif (write_packed_entry(out, update->refname,\n+\t\t\t\t\t update->new_oid.hash,\n+\t\t\t\t\t peel_error ? NULL : peeled.hash))\n+\t\t\t\tgoto write_error;\n+\n+\t\t\ti++;\n+\t\t}\n+\t}\n+\n+\tif (ok != ITER_DONE) {\n+\t\tstrbuf_addf(err, \"unable to write packed-refs file: \"\n+\t\t\t \"error iterating over old contents\");\n+\t\tgoto error;\n+\t}\n+\n+\tif (close_tempfile(&refs->tempfile)) {\n+\t\tstrbuf_addf(err, \"error closing file %s: %s\",\n+\t\t\t get_tempfile_path(&refs->tempfile),\n+\t\t\t strerror(errno));\n+\t\tstrbuf_release(&sb);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+\n+write_error:\n+\tstrbuf_addf(err, \"error writing to %s: %s\",\n+\t\t get_tempfile_path(&refs->tempfile), strerror(errno));\n+\n+error:\n+\tif (iter)\n+\t\tref_iterator_abort(iter);\n+\n+\tdelete_tempfile(&refs->tempfile);\n+\treturn -1;\n+}\n+\n+struct packed_transaction_backend_data {\n+\t/* True iff the transaction owns the packed-refs lock. */\n+\tint own_lock;\n+\n+\tstruct string_list updates;\n+};\n+\n+static void packed_transaction_cleanup(struct packed_ref_store *refs,\n+\t\t\t\t struct ref_transaction *transaction)\n+{\n+\tstruct packed_transaction_backend_data *data = transaction->backend_data;\n+\n+\tif (data) {\n+\t\tstring_list_clear(&data->updates, 0);\n+\n+\t\tif (is_tempfile_active(&refs->tempfile))\n+\t\t\tdelete_tempfile(&refs->tempfile);\n+\n+\t\tif (data->own_lock && is_lock_file_locked(&refs->lock)) {\n+\t\t\tpacked_refs_unlock(&refs->base);\n+\t\t\tdata->own_lock = 0;\n+\t\t}\n+\n+\t\tfree(data);\n+\t\ttransaction->backend_data = NULL;\n+\t}\n+\n+\ttransaction->state = REF_TRANSACTION_CLOSED;\n+}\n+\n static int packed_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-\tdie(\"BUG: not implemented yet\");\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,\n+\t\t\t\"ref_transaction_prepare\");\n+\tstruct packed_transaction_backend_data *data;\n+\tsize_t i;\n+\tint ret = TRANSACTION_GENERIC_ERROR;\n+\n+\t/*\n+\t * Note that we *don't* skip transactions with zero updates,\n+\t * because such a transaction might be executed for the side\n+\t * effect of ensuring that all of the references are peeled.\n+\t * If the caller wants to optimize away empty transactions, it\n+\t * should do so itself.\n+\t */\n+\n+\tdata = xcalloc(1, sizeof(*data));\n+\tstring_list_init(&data->updates, 0);\n+\n+\ttransaction->backend_data = data;\n+\n+\t/*\n+\t * Stick the updates in a string list by refname so that we\n+\t * can sort them:\n+\t */\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tstruct string_list_item *item =\n+\t\t\tstring_list_append(&data->updates, update->refname);\n+\n+\t\t/* Store a pointer to update in item->util: */\n+\t\titem->util = update;\n+\t}\n+\tstring_list_sort(&data->updates);\n+\n+\tif (ref_update_reject_duplicates(&data->updates, err))\n+\t\tgoto failure;\n+\n+\tif (!is_lock_file_locked(&refs->lock)) {\n+\t\tif (packed_refs_lock(ref_store, 0, err))\n+\t\t\tgoto failure;\n+\t\tdata->own_lock = 1;\n+\t}\n+\n+\tif (write_with_updates(refs, &data->updates, err))\n+\t\tgoto failure;\n+\n+\ttransaction->state = REF_TRANSACTION_PREPARED;\n+\treturn 0;\n+\n+failure:\n+\tpacked_transaction_cleanup(refs, transaction);\n+\treturn ret;\n }\n \n static int packed_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-\tdie(\"BUG: not implemented yet\");\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,\n+\t\t\t\"ref_transaction_abort\");\n+\n+\tpacked_transaction_cleanup(refs, transaction);\n+\treturn 0;\n }\n \n static int packed_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-\tdie(\"BUG: not implemented yet\");\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,\n+\t\t\t\"ref_transaction_finish\");\n+\tint ret = TRANSACTION_GENERIC_ERROR;\n+\tchar *packed_refs_path;\n+\n+\tpacked_refs_path = get_locked_file_path(&refs->lock);\n+\tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n+\t\tstrbuf_addf(err, \"error replacing %s: %s\",\n+\t\t\t refs->path, strerror(errno));\n+\t\tgoto cleanup;\n+\t}\n+\n+\tclear_packed_ref_cache(refs);\n+\tret = 0;\n+\n+cleanup:\n+\tfree(packed_refs_path);\n+\tpacked_transaction_cleanup(refs, transaction);\n+\treturn ret;\n }\n \n static int packed_initial_transaction_commit(struct ref_store *ref_store,\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 03b7c1de95..7af2897757 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -1,6 +1,15 @@\n #ifndef REFS_PACKED_BACKEND_H\n #define REFS_PACKED_BACKEND_H\n \n+/*\n+ * Support for storing references in a `packed-refs` file.\n+ *\n+ * Note that this backend doesn't check for D/F conflicts, because it\n+ * doesn't care about them. But usually it should be wrapped in a\n+ * `files_ref_store` that prevents D/F conflicts from being created,\n+ * even among packed refs.\n+ */\n+\n struct ref_store *packed_ref_store_create(const char *path,\n \t\t\t\t\t unsigned int store_flags);\n \n-- \n2.14.1\n\n"},{"id":"327330","messageId":"20170829180732.GA131745@google.com","threadId":"46681","inReplyTo":"7c61f04574e32d6172062a6a8ffa8ade5c90bb1d.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 04/10] packed_delete_refs(): implement method","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T18:07:32Z","receivedAt":"2017-08-29T18:07:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Michael Haggerty wrote:\n> Implement `packed_delete_refs()` using a reference transaction. This\n> means that `files_delete_refs()` can use `refs_delete_refs()` instead\n> of `repack_without_refs()` to delete any packed references, decreasing\n> the coupling between the classes.\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 2 +-\n> refs/packed-backend.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-\n> 2 files changed, 45 insertions(+), 2 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index fccbc24ac4..2c78f63494 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n> \tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n> \t\tgoto error;\n> \n> -\tif (repack_without_refs(refs->packed_ref_store, refnames, &err)) {\n> +\tif (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {\n> \t\tpacked_refs_unlock(refs->packed_ref_store);\n> \t\tgoto error;\n> \t}\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index d19b3bfba5..83a088118f 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n> static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n> \t\t\t struct string_list *refnames, unsigned int flags)\n> {\n> -\tdie(\"BUG: not implemented yet\");\n> +\tstruct packed_ref_store *refs =\n> +\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct ref_transaction *transaction;\n> +\tstruct string_list_item *item;\n> +\tint ret;\n> +\n> +\t(void)refs; /* We need the check above, but don't use the variable */\n\nCan't say I've seen a line like this before, Is the intent to just mark\nthat this variable won't be used like you mention in the comment?\n\n> +\n> +\tif (!refnames->nr)\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * Since we don't check the references' old_oids, the\n> +\t * individual updates can't fail, so we can pack all of the\n> +\t * updates into a single transaction.\n> +\t */\n> +\n> +\ttransaction = ref_store_transaction_begin(ref_store, &err);\n> +\tif (!transaction)\n> +\t\treturn -1;\n> +\n> +\tfor_each_string_list_item(item, refnames) {\n> +\t\tif (ref_transaction_delete(transaction, item->string, NULL,\n> +\t\t\t\t\t flags, msg, &err)) {\n> +\t\t\twarning(_(\"could not delete reference %s: %s\"),\n> +\t\t\t\titem->string, err.buf);\n> +\t\t\tstrbuf_reset(&err);\n> +\t\t}\n> +\t}\n> +\n> +\tret = ref_transaction_commit(transaction, &err);\n> +\n> +\tif (ret) {\n> +\t\tif (refnames->nr == 1)\n> +\t\t\terror(_(\"could not delete reference %s: %s\"),\n> +\t\t\t refnames->items[0].string, err.buf);\n> +\t\telse\n> +\t\t\terror(_(\"could not delete references: %s\"), err.buf);\n> +\t}\n> +\n> +\tref_transaction_free(transaction);\n> +\tstrbuf_release(&err);\n> +\treturn ret;\n> }\n> \n> static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)\n> -- \n> 2.14.1\n> \n\n-- \nBrandon Williams\n"},{"id":"327331","messageId":"20170829182428.GB131745@google.com","threadId":"46681","inReplyTo":"824ad441f8531f1bbcc8800a14fe71466721066d.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 09/10] packed-backend: rip out some now-unused code","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T18:24:28Z","receivedAt":"2017-08-29T18:24:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Michael Haggerty wrote:\n> Now the outside world interacts with the packed ref store only via the\n> generic refs API plus a few lock-related functions. This allows us to\n> delete some functions that are no longer used, thereby completing the\n> encapsulation of the packed ref store.\n\nLove seeing patches which only remove old code :)\n\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> refs/packed-backend.c | 193 --------------------------------------------------\n> refs/packed-backend.h | 8 ---\n> 2 files changed, 201 deletions(-)\n> \n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 83a088118f..90f44c1fbb 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -91,19 +91,6 @@ struct ref_store *packed_ref_store_create(const char *path,\n> \treturn ref_store;\n> }\n> \n> -/*\n> - * Die if refs is not the main ref store. caller is used in any\n> - * necessary error messages.\n> - */\n> -static void packed_assert_main_repository(struct packed_ref_store *refs,\n> -\t\t\t\t\t const char *caller)\n> -{\n> -\tif (refs->store_flags & REF_STORE_MAIN)\n> -\t\treturn;\n> -\n> -\tdie(\"BUG: operation %s only allowed for main ref store\", caller);\n> -}\n> -\n> /*\n> * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is\n> * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't\n> @@ -321,40 +308,6 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n> \treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n> }\n> \n> -/*\n> - * Add or overwrite a reference in the in-memory packed reference\n> - * cache. This may only be called while the packed-refs file is locked\n> - * (see packed_refs_lock()). To actually write the packed-refs file,\n> - * call commit_packed_refs().\n> - */\n> -void add_packed_ref(struct ref_store *ref_store,\n> -\t\t const char *refname, const struct object_id *oid)\n> -{\n> -\tstruct packed_ref_store *refs =\n> -\t\tpacked_downcast(ref_store, REF_STORE_WRITE,\n> -\t\t\t\t\"add_packed_ref\");\n> -\tstruct ref_dir *packed_refs;\n> -\tstruct ref_entry *packed_entry;\n> -\n> -\tif (!is_lock_file_locked(&refs->lock))\n> -\t\tdie(\"BUG: packed refs not locked\");\n> -\n> -\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n> -\t\tdie(\"Reference has invalid format: '%s'\", refname);\n> -\n> -\tpacked_refs = get_packed_refs(refs);\n> -\tpacked_entry = find_ref_entry(packed_refs, refname);\n> -\tif (packed_entry) {\n> -\t\t/* Overwrite the existing entry: */\n> -\t\toidcpy(&packed_entry->u.value.oid, oid);\n> -\t\tpacked_entry->flag = REF_ISPACKED;\n> -\t\toidclr(&packed_entry->u.value.peeled);\n> -\t} else {\n> -\t\tpacked_entry = create_ref_entry(refname, oid, REF_ISPACKED);\n> -\t\tadd_ref_entry(packed_refs, packed_entry);\n> -\t}\n> -}\n> -\n> /*\n> * Return the ref_entry for the given refname from the packed\n> * references. If it does not exist, return NULL.\n> @@ -592,152 +545,6 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n> static const char PACKED_REFS_HEADER[] =\n> \t\"# pack-refs with: peeled fully-peeled \\n\";\n> \n> -/*\n> - * Write the current version of the packed refs cache from memory to\n> - * disk. The packed-refs file must already be locked for writing (see\n> - * packed_refs_lock()). Return zero on success. On errors, rollback\n> - * the lockfile, write an error message to `err`, and return a nonzero\n> - * value.\n> - */\n> -int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n> -{\n> -\tstruct packed_ref_store *refs =\n> -\t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n> -\t\t\t\t\"commit_packed_refs\");\n> -\tstruct packed_ref_cache *packed_ref_cache =\n> -\t\tget_packed_ref_cache(refs);\n> -\tint ok;\n> -\tint ret = -1;\n> -\tstruct strbuf sb = STRBUF_INIT;\n> -\tFILE *out;\n> -\tstruct ref_iterator *iter;\n> -\tchar *packed_refs_path;\n> -\n> -\tif (!is_lock_file_locked(&refs->lock))\n> -\t\tdie(\"BUG: commit_packed_refs() called when unlocked\");\n> -\n> -\t/*\n> -\t * If packed-refs is a symlink, we want to overwrite the\n> -\t * symlinked-to file, not the symlink itself. Also, put the\n> -\t * staging file next to it:\n> -\t */\n> -\tpacked_refs_path = get_locked_file_path(&refs->lock);\n> -\tstrbuf_addf(&sb, \"%s.new\", packed_refs_path);\n> -\tif (create_tempfile(&refs->tempfile, sb.buf) < 0) {\n> -\t\tstrbuf_addf(err, \"unable to create file %s: %s\",\n> -\t\t\t sb.buf, strerror(errno));\n> -\t\tstrbuf_release(&sb);\n> -\t\tgoto out;\n> -\t}\n> -\tstrbuf_release(&sb);\n> -\n> -\tout = fdopen_tempfile(&refs->tempfile, \"w\");\n> -\tif (!out) {\n> -\t\tstrbuf_addf(err, \"unable to fdopen packed-refs tempfile: %s\",\n> -\t\t\t strerror(errno));\n> -\t\tgoto error;\n> -\t}\n> -\n> -\tif (fprintf(out, \"%s\", PACKED_REFS_HEADER) < 0) {\n> -\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n> -\t\t\t get_tempfile_path(&refs->tempfile), strerror(errno));\n> -\t\tgoto error;\n> -\t}\n> -\n> -\titer = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);\n> -\twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n> -\t\tstruct object_id peeled;\n> -\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n> -\n> -\t\tif (write_packed_entry(out, iter->refname, iter->oid->hash,\n> -\t\t\t\t peel_error ? NULL : peeled.hash)) {\n> -\t\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n> -\t\t\t\t get_tempfile_path(&refs->tempfile),\n> -\t\t\t\t strerror(errno));\n> -\t\t\tref_iterator_abort(iter);\n> -\t\t\tgoto error;\n> -\t\t}\n> -\t}\n> -\n> -\tif (ok != ITER_DONE) {\n> -\t\tstrbuf_addf(err, \"unable to rewrite packed-refs file: \"\n> -\t\t\t \"error iterating over old contents\");\n> -\t\tgoto error;\n> -\t}\n> -\n> -\tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n> -\t\tstrbuf_addf(err, \"error replacing %s: %s\",\n> -\t\t\t refs->path, strerror(errno));\n> -\t\tgoto out;\n> -\t}\n> -\n> -\tret = 0;\n> -\tgoto out;\n> -\n> -error:\n> -\tdelete_tempfile(&refs->tempfile);\n> -\n> -out:\n> -\tfree(packed_refs_path);\n> -\treturn ret;\n> -}\n> -\n> -/*\n> - * Rewrite the packed-refs file, omitting any refs listed in\n> - * 'refnames'. On error, leave packed-refs unchanged, write an error\n> - * message to 'err', and return a nonzero value. The packed refs lock\n> - * must be held when calling this function; it will still be held when\n> - * the function returns.\n> - *\n> - * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n> - */\n> -int repack_without_refs(struct ref_store *ref_store,\n> -\t\t\tstruct string_list *refnames, struct strbuf *err)\n> -{\n> -\tstruct packed_ref_store *refs =\n> -\t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n> -\t\t\t\t\"repack_without_refs\");\n> -\tstruct ref_dir *packed;\n> -\tstruct string_list_item *refname;\n> -\tint needs_repacking = 0, removed = 0;\n> -\n> -\tpacked_assert_main_repository(refs, \"repack_without_refs\");\n> -\tassert(err);\n> -\n> -\tif (!is_lock_file_locked(&refs->lock))\n> -\t\tdie(\"BUG: repack_without_refs called without holding lock\");\n> -\n> -\t/* Look for a packed ref */\n> -\tfor_each_string_list_item(refname, refnames) {\n> -\t\tif (get_packed_ref(refs, refname->string)) {\n> -\t\t\tneeds_repacking = 1;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> -\n> -\t/* Avoid locking if we have nothing to do */\n> -\tif (!needs_repacking)\n> -\t\treturn 0; /* no refname exists in packed refs */\n> -\n> -\tpacked = get_packed_refs(refs);\n> -\n> -\t/* Remove refnames from the cache */\n> -\tfor_each_string_list_item(refname, refnames)\n> -\t\tif (remove_entry_from_dir(packed, refname->string) != -1)\n> -\t\t\tremoved = 1;\n> -\tif (!removed) {\n> -\t\t/*\n> -\t\t * All packed entries disappeared while we were\n> -\t\t * acquiring the lock.\n> -\t\t */\n> -\t\tclear_packed_ref_cache(refs);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\t/* Write what remains */\n> -\treturn commit_packed_refs(&refs->base, err);\n> -}\n> -\n> static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n> {\n> \t/* Nothing to do. */\n> diff --git a/refs/packed-backend.h b/refs/packed-backend.h\n> index 7af2897757..61687e408a 100644\n> --- a/refs/packed-backend.h\n> +++ b/refs/packed-backend.h\n> @@ -23,12 +23,4 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n> void packed_refs_unlock(struct ref_store *ref_store);\n> int packed_refs_is_locked(struct ref_store *ref_store);\n> \n> -void add_packed_ref(struct ref_store *ref_store,\n> -\t\t const char *refname, const struct object_id *oid);\n> -\n> -int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err);\n> -\n> -int repack_without_refs(struct ref_store *ref_store,\n> -\t\t\tstruct string_list *refnames, struct strbuf *err);\n> -\n> #endif /* REFS_PACKED_BACKEND_H */\n> -- \n> 2.14.1\n> \n\n-- \nBrandon Williams\n"},{"id":"327332","messageId":"20170829183008.GC131745@google.com","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/10] Implement transactions for the packed ref store","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-29T18:30:08Z","receivedAt":"2017-08-29T18:30:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Michael Haggerty wrote:\n> This should be the second-to-last patch series in the quest for\n> mmapped packed-refs.\n> \n> Before this patch series, there is still more coupling than necessary\n> between `files_ref_store` and `packed_ref_store`:\n> \n> * `files_ref_store` adds packed references (e.g., during `git clone`\n> and `git pack-refs`) by inserting them into the `packed_ref_store`'s\n> internal `ref_cache`, then calling `commit_packed_refs()`. But\n> `files_ref_store` shouldn't even *know* that `packed_ref_store` uses\n> a `ref_cache`, let alone muck about in it.\n> \n> * `files_ref_store` deletes packed references by calling\n> `repack_without_refs()`.\n> \n> Instead, implement reference transactions and `delete_refs()` for\n> `packed_ref_store`, and change `files_ref_store` to use these standard\n> methods rather than internal `packed_ref_store` methods.\n> \n> This patch series finishes up the encapsulation of `packed_ref_store`.\n> At the end of the series, the outside world only interacts with it via\n> the refs API plus a couple of locking-related functions. That will\n> make it easy for the next patch series to change the internal workings\n> of `packed_ref_store` without affecting `files_ref_store`.\n> \n> Along the way, we fix some longstanding problems with reference\n> updates. Quoting from patch [08/10]:\n> \n> First, the old code didn't obtain the `packed-refs` lock until\n> `files_transaction_finish()`. This means that a failure to acquire the\n> `packed-refs` lock (e.g., due to contention with another process)\n> wasn't detected until it was too late (problems like this are supposed\n> to be detected in the \"prepare\" phase). The new code acquires the\n> `packed-refs` lock in `files_transaction_prepare()`, the same stage of\n> the processing when the loose reference locks are being acquired,\n> removing another reason why the \"prepare\" phase might succeed and the\n> \"finish\" phase might nevertheless fail.\n> \n> Second, the old code deleted the loose version of a reference before\n> deleting any packed version of the same reference. This left a moment\n> when another process might think that the packed version of the\n> reference is current, which is incorrect. (Even worse, the packed\n> version of the reference can be arbitrarily old, and might even point\n> at an object that has since been garbage-collected.)\n> \n> Third, if a reference deletion fails to acquire the `packed-refs` lock\n> altogether, then the old code might leave the repository in the\n> incorrect state (possibly corrupt) described in the previous\n> paragraph.\n> \n> Now we activate the new \"packed-refs\" file (sans any references that\n> are being deleted) *before* deleting the corresponding loose\n> references. But we hold the \"packed-refs\" lock until after the loose\n> references have been finalized, thus preventing a simultaneous\n> \"pack-refs\" process from packing the loose version of the reference in\n> the time gap, which would otherwise defeat our attempt to delete it.\n> \n> This patch series is also available as branch\n> `packed-ref-transactions` in my fork on GitHub [1]. A draft of the\n> patch series to change `packed_ref_store` to use mmap and get rid of\n> its `ref_cache` is available as branch `mmap-packed-refs` from the\n> same source.\n\nOverall the patches look sane to me, though I don't believe I'm\nqualified in this area to give you a complete thumbs up since I don't\nunderstand the refs code super well yet. I do like reading patch from\nyou as you do a great job of laying out what you are doing in code,\ncomments and commit messages, something I'm trying to get better at :)\n\n-- \nBrandon Williams\n"},{"id":"327343","messageId":"5237dffe-fcb4-37b9-2645-3f0f0dc5bd19@alum.mit.edu","threadId":"46681","inReplyTo":"20170829180732.GA131745@google.com","subject":"Re: [PATCH 04/10] packed_delete_refs(): implement method","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T03:00:01Z","receivedAt":"2017-08-30T03:00:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/29/2017 08:07 PM, Brandon Williams wrote:\n> On 08/29, Michael Haggerty wrote:\n>> [...]\n>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n>> index d19b3bfba5..83a088118f 100644\n>> --- a/refs/packed-backend.c\n>> +++ b/refs/packed-backend.c\n>> @@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,\n>> static int packed_delete_refs(struct ref_store *ref_store, const char *msg,\n>> \t\t\t struct string_list *refnames, unsigned int flags)\n>> {\n>> -\tdie(\"BUG: not implemented yet\");\n>> +\tstruct packed_ref_store *refs =\n>> +\t\tpacked_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n>> +\tstruct strbuf err = STRBUF_INIT;\n>> +\tstruct ref_transaction *transaction;\n>> +\tstruct string_list_item *item;\n>> +\tint ret;\n>> +\n>> +\t(void)refs; /* We need the check above, but don't use the variable */\n> \n> Can't say I've seen a line like this before, Is the intent to just mark\n> that this variable won't be used like you mention in the comment?\n\n\nThe `(void)refs;` part tells the compiler not to complain about a\nvariable that is defined but never used.\n\nSo why define the variable in the first place? I could instead do\n\n (void)packed_downcast(ref_store, REF_STORE_WRITE, \"delete_refs\");\n\nMy reason (which people might disagree with) is that I like to keep this\nboilerplate consistent from method to method. To switch to the second\nvariant, not only would the line look different, but it would also have\nto be moved down several lines, past the other variables' declarations,\nto avoid the dreaded \"ISO C90 forbids mixed declarations and code\".\n\nI'd prefer to be able to mix declarations and code, but the project's\nstyle prohibits it.\n\nMichael\n"},{"id":"327407","messageId":"CAGZ79ka+uaBD_1xk6sx9J83KuP4A_mUWVqtyniNusbDeQiUZyQ@mail.gmail.com","threadId":"46681","inReplyTo":"caaa44126f18869158872e5473e53478db780ba9.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-30T17:21:14Z","receivedAt":"2017-08-30T17:21:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty wrote:\n> Currently, a loose reference is deleted even before locking the\n> `packed-refs` file, let alone deleting any packed version of the\n> reference. This leads to two problems, demonstrated by two new tests:\n>\n> * While a reference is being deleted, other processes might see the\n> old, packed value of the reference for a moment before the packed\n> version is deleted. Normally this would be hard to observe, but we\n> can prolong the window by locking the `packed-refs` file externally\n> before running `update-ref`, then unlocking it before `update-ref`'s\n> attempt to acquire the lock times out.\n>\n> * If the `packed-refs` file is locked so long that `update-ref` fails\n> to lock it, then the reference can be left permanently in the\n> incorrect state described in the previous point.\n>\n> In a moment, both problems will be fixed.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> The first of the new tests is rather involved; it uses two background\n> processes plus a foreground process that polls the value of a\n> reference. But despite sensitivity to timing, I think it should be\n> robust even in this broken state. Once the functionality being tested\n> is fixed, this test should never produce false positives, though\n> really bad timing (e.g., if it takes more than a second for\n> `update-ref` to get going) could still lead to false negatives.\n>\n> Each of the new tests takes about a second to run because they\n> simulate lock contention.\n>\n> If anybody has suggestions for better ways to test these things,\n> please speak up :-)\n>\n> t/t1404-update-ref-errors.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 71 insertions(+)\n>\n> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\n> index c34ece48f5..752f83c377 100755\n> --- a/t/t1404-update-ref-errors.sh\n> +++ b/t/t1404-update-ref-errors.sh\n> @@ -404,4 +404,75 @@ test_expect_success 'broken reference blocks indirect create' '\n> test_cmp expected output.err\n> '\n>\n> +test_expect_failure 'no bogus intermediate values during delete' '\n> + prefix=refs/slow-transaction &&\n> + # Set up a reference with differing loose and packed versions:\n> + git update-ref $prefix/foo $C &&\n> + git pack-refs --all &&\n> + git update-ref $prefix/foo $D &&\n> + git for-each-ref $prefix >unchanged &&\n> + # Now try to update the reference, but hold the `packed-refs` lock\n> + # for a while to see what happens while the process is blocked:\n> + : >.git/packed-refs.lock &&\n> + test_when_finished \"rm -f .git/packed-refs.lock\" &&\n> + {\n> + sleep 1 &&\n> + rm -f .git/packed-refs.lock &\n> + } &&\n> + pid1=$! &&\n> + {\n> + # Note: the following command is intentionally run in the\n> + # background. We extend the timeout so that `update-ref`\n> + # tries to acquire the `packed-refs` lock longer than it\n> + # takes the background process above to delete it:\n> + git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &\n> + } &&\n> + pid2=$! &&\n> + ok=true &&\n> + while kill -0 $pid2 2>/dev/null\n\n If sig is 0, then no signal is sent, but error checking is still\n performed; this can be used to check for the existence of a\n process ID or process group ID.\n\nSo the kill -0 is the idiomatic form of \"while $pid2 is still alive\"?\nignoring errors due to the dev/null redirection?\n\nAnd due to the nature of this test we have to have a busy\nloop, we cannot rate limit the cpu usage inside the loop\nvia some shorter sleeps, as ideally we want to observe\nthe ref at any time.\n\n In an ideal world this test would instruct the kernel to interrupt\n the executing program (update-ref) at certain events such as\n touching/writing/deleting files and in each interrupt we could\n inspect the file system in a read only fashion.\n\n\n> + do\n> + sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&\n> + case \"$sha1\" in\n> + $D)\n> + # This is OK; it just means that nothing has happened yet.\n> + : ;;\n> + undefined)\n> + # This is OK; it means the deletion was successful.\n> + : ;;\n> + $C)\n> + # This value should never be seen. Probably the loose\n> + # reference has been deleted but the packed reference\n> + # is still there:\n> + echo \"$prefix/foo incorrectly observed to be C\" &&\n> + break\n> + ;;\n> + *)\n> + # WTF?\n> + echo \"$prefix/foo unexpected value observed: $sha1\" &&\n> + break\n> + ;;\n> + esac\n> + done >out &&\n> + wait $pid1 &&\n> + wait $pid2 &&\n\noh, you use explicit pids here to check each exit code.\n\n> If anybody has suggestions for better ways to test these things,\n> please speak up :-)\n\nI don't think I'd have a satisfactory answer to that, as the timing is inherent\nto the things we test. In other software projects that are less low level, I\nwould have suggested to use a time/clock mock, which can be stopped\nand then inspection can be performed at defined states.\n"},{"id":"327475","messageId":"810bbfa2-4486-e3dd-8243-ea6116f21b2c@alum.mit.edu","threadId":"46681","inReplyTo":"CAGZ79ka+uaBD_1xk6sx9J83KuP4A_mUWVqtyniNusbDeQiUZyQ@mail.gmail.com","subject":"Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-31T03:42:16Z","receivedAt":"2017-08-31T03:42:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/30/2017 07:21 PM, Stefan Beller wrote:\n> On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty wrote:\n>> [...]\n>> +test_expect_failure 'no bogus intermediate values during delete' '\n>> + prefix=refs/slow-transaction &&\n>> + # Set up a reference with differing loose and packed versions:\n>> + git update-ref $prefix/foo $C &&\n>> + git pack-refs --all &&\n>> + git update-ref $prefix/foo $D &&\n>> + git for-each-ref $prefix >unchanged &&\n>> + # Now try to update the reference, but hold the `packed-refs` lock\n>> + # for a while to see what happens while the process is blocked:\n>> + : >.git/packed-refs.lock &&\n>> + test_when_finished \"rm -f .git/packed-refs.lock\" &&\n>> + {\n>> + sleep 1 &&\n>> + rm -f .git/packed-refs.lock &\n>> + } &&\n>> + pid1=$! &&\n>> + {\n>> + # Note: the following command is intentionally run in the\n>> + # background. We extend the timeout so that `update-ref`\n>> + # tries to acquire the `packed-refs` lock longer than it\n>> + # takes the background process above to delete it:\n>> + git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &\n>> + } &&\n>> + pid2=$! &&\n>> + ok=true &&\n>> + while kill -0 $pid2 2>/dev/null\n> \n> If sig is 0, then no signal is sent, but error checking is still\n> performed; this can be used to check for the existence of a\n> process ID or process group ID.\n> \n> So the kill -0 is the idiomatic form of \"while $pid2 is still alive\"?\n> ignoring errors due to the dev/null redirection?\n> \n> And due to the nature of this test we have to have a busy\n> loop, we cannot rate limit the cpu usage inside the loop\n> via some shorter sleeps, as ideally we want to observe\n> the ref at any time.\n\nCorrect on both counts.\n\nI just noticed that there is a stray line `ok=true &&` from an earlier\ndraft. I'll remove that in v2.\n\n> In an ideal world this test would instruct the kernel to interrupt\n> the executing program (update-ref) at certain events such as\n> touching/writing/deleting files and in each interrupt we could\n> inspect the file system in a read only fashion.\n\nA tool like `strace` could be used for tests like this, but it would be\nterribly non-portable. (But I often use strace manually to check that\nthe ordering of filesystem events is correct.)\n\n>> + do\n>> + sha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&\n>> + case \"$sha1\" in\n>> + $D)\n>> + # This is OK; it just means that nothing has happened yet.\n>> + : ;;\n>> + undefined)\n>> + # This is OK; it means the deletion was successful.\n>> + : ;;\n>> + $C)\n>> + # This value should never be seen. Probably the loose\n>> + # reference has been deleted but the packed reference\n>> + # is still there:\n>> + echo \"$prefix/foo incorrectly observed to be C\" &&\n>> + break\n>> + ;;\n>> + *)\n>> + # WTF?\n>> + echo \"$prefix/foo unexpected value observed: $sha1\" &&\n>> + break\n>> + ;;\n>> + esac\n>> + done >out &&\n>> + wait $pid1 &&\n>> + wait $pid2 &&\n> \n> oh, you use explicit pids here to check each exit code.\n> \n>> If anybody has suggestions for better ways to test these things,\n>> please speak up :-)\n> \n> I don't think I'd have a satisfactory answer to that, as the timing is inherent\n> to the things we test. In other software projects that are less low level, I\n> would have suggested to use a time/clock mock, which can be stopped\n> and then inspection can be performed at defined states.\n\nI just realized that, given that the main goal here is to check the\nvalue of the reference while `update-ref` is waiting on the\n`packed-refs` lock, we can do the test without a busy loop. Instead, we\nroughly\n\n\t: >.git/packed-refs.lock &&\n\t{\n\t\tgit -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &\n\t} &&\n\tpid2=$! &&\n\tsleep 1 &&\n\t# Verify that update-ref is still running:\n\tkill -0 $pid2 &&\n\t# ...verify that the reference still has its old value...\n\trm -f .git/packed-refs.lock &&\n\twait $pid2 &&\n\t# ...verify that the reference is now gone...\n\nIt's true that this version wouldn't discover incorrect transitional\nvalues of the reference that happen at other times, but that was very\nunlikely anyway given the speed disparity between C and shell. I'll make\nthis change in v2.\n\nMichael\n"},{"id":"327749","messageId":"xmqqshfxztn5.fsf@gitster.mtv.corp.google.com","threadId":"46681","inReplyTo":"caaa44126f18869158872e5473e53478db780ba9.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-08T04:44:30Z","receivedAt":"2017-09-08T04:44:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +\tgit for-each-ref $prefix >actual &&\n> +\tgrep \"Unable to create $Q.*packed-refs.lock$Q: File exists\" err &&\n\nI added a squash for doing s/grep/test_i18n&/ here; are there other\nissues in the series, or is the series more or less ready to be\npolished in 'next'?\n\nThanks.\n"},{"id":"327754","messageId":"20170908065223.f7f52hjcx5qsjbch@sigill.intra.peff.net","threadId":"46681","inReplyTo":"c5d66cadb70efdb5b5ce91fbcc5673621f5d4792.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T06:52:24Z","receivedAt":"2017-09-08T06:52:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 10:20:25AM +0200, Michael Haggerty wrote:\n\n> The old code incremented the packed ref cache reference count when\n> acquiring the packed-refs lock, and decremented the count when\n> releasing the lock. This is unnecessary because a locked packed-refs\n> file cannot be changed, so there is no reason for the cache to become\n> stale.\n\nHmm, I thought that after your last series, we might hold the lock but\nupdate the packed-refs from a separate tempfile. I.e., 42dfa7ecef\n(commit_packed_refs(): use a staging file separate from the lockfile,\n2017-06-23).\n\n> Moreover, the extra reference count causes a problem if we\n> intentionally clear the packed refs cache, as we sometimes need to do\n> if we change the cache in anticipation of writing a change to disk,\n> but then the write to disk fails. In that case, `packed_refs_unlock()`\n> would have no easy way to find the cache whose reference count it\n> needs to decrement.\n> \n> This whole issue will soon become moot due to upcoming changes that\n> avoid changing the in-memory cache as part of updating the packed-refs\n> on disk, but this change makes that transition easier.\n\nAll of this makes sense, and I'm happy this complexity is going away in\nthe long run. But I guess what I'm wondering is in the meantime if we\ncan have a sequence like:\n\n 1. We hold packed-refs.lock\n\n 2. We update the file without releasing the lock, via 42dfa7ecef.\n\n 3. Still holding the lock, we try to look at packed-refs. The\n stat_validity code says no, we're not up to date.\n\n 4. We discard the old packed_ref_cache and reload it. Because its\n reference count was not incremented during step 1, we actually\n free() it.\n\n 5. We try to look at at the old freed pointer.\n\nThere are several steps in there that might be implausible. So I'm\nmostly playing devil's advocate here.\n\nI'm wondering if the \"don't validate while we hold the lock\" logic in\nget_packed_refs_cache() means that step 3 is impossible.\n\n> @@ -560,9 +559,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n> \t */\n> \tvalidate_packed_ref_cache(refs);\n> \n> -\tpacked_ref_cache = get_packed_ref_cache(refs);\n> -\t/* Increment the reference count to prevent it from being freed: */\n> -\tacquire_packed_ref_cache(packed_ref_cache);\n> +\tget_packed_ref_cache(refs);\n\nIt seems a bit funny to call a \"get\" function and throw away the return\nvalue. Presumably we care about its side effect of updating refs->cache.\nThat might be worth a comment (though if this is all going away soon, I\ncare a lot less about niceties like that).\n\n-Peff\n"},{"id":"327755","messageId":"20170908070205.3hwa7tjgok7kgknc@sigill.intra.peff.net","threadId":"46681","inReplyTo":"5b8c8f9ab0ccd5f85eb43a7a1535af89b0af85e3.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:02:05Z","receivedAt":"2017-09-08T07:02:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 10:20:26AM +0200, Michael Haggerty wrote:\n\n> `packed_ref_store` is going to want to store some transaction-wide\n> data, so make a place for it.\n\nThat makes sense, although...\n\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index b02dc5a7e3..d7d344de73 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -242,6 +242,7 @@ struct ref_transaction {\n> \tsize_t alloc;\n> \tsize_t nr;\n> \tenum ref_transaction_state state;\n> +\tvoid *backend_data;\n> };\n\nThis is just one pointer. Once we start layering ref backends (and\nalready we're moving towards a \"files\" layer which sits atop loose and\npacked backends, right?), how do we avoid backends stomping on each\nother (or worse, dereferencing somebody else's data as their own\nstruct)?\n\nI don't know that we necessarily need to answer that question right now,\nbut I'm worried that this pattern might need adjustment eventually.\n\nI guess the hand-wavy answer is that whatever is doing the layering\nwould need to manage the pointers. So if you imagine that we had a\n\"union\" backend that took two other arbitrary backends, it would\nprobably have something like:\n\n struct union_backend_data {\n\tvoid *data_a;\n\tvoid *data_b;\n }\n\nand when it forwarded calls to the separate backends, it would give them\na view of the transaction with only their data. Something like:\n\n void union_backend_foo(void *be, struct ref_transaction *transaction)\n {\n\tstruct union_backend *me = be;\n\tstruct union_backend_data *my_data = transaction->backend_data;\n\n /* \"a\" sees only it's data, and we remember any modifications */\n\ttransaction->backend_data = my_data->data_a;\n\tme->backend_a->foo(me->backend_a, transaction);\n\tmy_data->data_a = transaction->backend_data;\n\n /* ditto for \"b\" */\n\ttransaction->backend_data = my_data->data_b;\n\tme->backend_b->foo(me->backend_b, transaction);\n\tmy_data->data_b = transaction->backend_data;\n\n\t/* and then we restore our own view */\n\ttransaction->backend_data = my_data;\n }\n\n-Peff\n"},{"id":"327756","messageId":"20170908072758.d3qvwsrm5t54kc63@sigill.intra.peff.net","threadId":"46681","inReplyTo":"92b29defc3c4ddb65a948b7d142ded941eaccefd.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:27:59Z","receivedAt":"2017-09-08T07:28:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:\n\n> Used a `packed_ref_store` transaction in the implementation of\n> `files_initial_transaction_commit()` rather than using internal\n> features of the packed ref store. This further decouples\n> `files_ref_store` from `packed_ref_store`.\n\nVery nice to see these couplings going away.\n\nMinor nit: s/Used/Use/ in the commit message.\n\n-Peff\n"},{"id":"327757","messageId":"20170908073816.lnlq6lpabtiu7rra@sigill.intra.peff.net","threadId":"46681","inReplyTo":"25b38730abe97c29d04116b4b2c6067934dfca14.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:38:16Z","receivedAt":"2017-09-08T07:38:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 10:20:32AM +0200, Michael Haggerty wrote:\n\n> First, the old code didn't obtain the `packed-refs` lock until\n> `files_transaction_finish()`. This means that a failure to acquire the\n> `packed-refs` lock (e.g., due to contention with another process)\n> wasn't detected until it was too late (problems like this are supposed\n> to be detected in the \"prepare\" phase). The new code acquires the\n> `packed-refs` lock in `files_transaction_prepare()`, the same stage of\n> the processing when the loose reference locks are being acquired,\n> removing another reason why the \"prepare\" phase might succeed and the\n> \"finish\" phase might nevertheless fail.\n\nThat means we're holding the packed-refs lock for a slightly longer\nperiod. I think this could mean worse lock contention between otherwise\nunrelated transactions over the packed-refs file. I wonder if the\nlock-retry timeout might need to be increased to accommodate this. On\nthe other hand, it looks like we take it after getting the individual\nlocks, which I'd think would be the expensive part.\n\nAre there other callers who take the packed-refs and individual locks in\nthe reverse order? I think git-pack-refs might. Could we \"deadlock\" with\npack-refs? There are timeouts so it would resolve itself quickly, but I\nwonder if we'd have a case that would deadlock-until-timeout that\notherwise could succeed.\n\nI guess such a deadlock would exist today, as well, if we take the\npacked-refs lock before giving up the individual loose ref locks.\n\n> Second, the old code deleted the loose version of a reference before\n> deleting any packed version of the same reference. This left a moment\n> when another process might think that the packed version of the\n> reference is current, which is incorrect. (Even worse, the packed\n> version of the reference can be arbitrarily old, and might even point\n> at an object that has since been garbage-collected.)\n> \n> Third, if a reference deletion fails to acquire the `packed-refs` lock\n> altogether, then the old code might leave the repository in the\n> incorrect state (possibly corrupt) described in the previous\n> paragraph.\n\nAnd to think I had the hubris to claim a few weeks ago that we had\nprobably weeded out all of the weird packed-refs inconsistency and\nrace-condition bugs. :)\n\nGood find.\n\n-Peff\n"},{"id":"327758","messageId":"20170908074257.tmdyj4wbij35fylq@sigill.intra.peff.net","threadId":"46681","inReplyTo":"cover.1503993268.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/10] Implement transactions for the packed ref store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:42:57Z","receivedAt":"2017-09-08T07:43:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 10:20:24AM +0200, Michael Haggerty wrote:\n\n> This should be the second-to-last patch series in the quest for\n> mmapped packed-refs.\n\nI just gave this a careful read, and it looks pretty good to me. I had a\nfew questions, but no show-stoppers. I'll admit to glossing a little bit\nover the conversions in patch 3 and 4, after seeing that they're\nbasically adaptations of the code that goes away in patch 9.\n\n-Peff\n"},{"id":"327759","messageId":"20170908074505.r5luqgvi6e45shgg@sigill.intra.peff.net","threadId":"46681","inReplyTo":"xmqqshfxztn5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T07:45:06Z","receivedAt":"2017-09-08T07:45:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 01:44:30PM +0900, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > +\tgit for-each-ref $prefix >actual &&\n> > +\tgrep \"Unable to create $Q.*packed-refs.lock$Q: File exists\" err &&\n> \n> I added a squash for doing s/grep/test_i18n&/ here; are there other\n> issues in the series, or is the series more or less ready to be\n> polished in 'next'?\n\nI think we'd want the test fix to avoid busy-waiting that was discussed\nupthread. I just left a few comments, too. I suspect they will all be\nanswered without code changes, but I think we'd probably want to wait\nfor a v2.\n\n-Peff\n"},{"id":"327761","messageId":"6c4cf1eb-7991-1a39-a298-7839872ed3f5@alum.mit.edu","threadId":"46681","inReplyTo":"20170908070205.3hwa7tjgok7kgknc@sigill.intra.peff.net","subject":"Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T08:19:48Z","receivedAt":"2017-09-08T08:20:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/08/2017 09:02 AM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 10:20:26AM +0200, Michael Haggerty wrote:\n> \n>> `packed_ref_store` is going to want to store some transaction-wide\n>> data, so make a place for it.\n> \n> That makes sense, although...\n> \n>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n>> index b02dc5a7e3..d7d344de73 100644\n>> --- a/refs/refs-internal.h\n>> +++ b/refs/refs-internal.h\n>> @@ -242,6 +242,7 @@ struct ref_transaction {\n>> \tsize_t alloc;\n>> \tsize_t nr;\n>> \tenum ref_transaction_state state;\n>> +\tvoid *backend_data;\n>> };\n> \n> This is just one pointer. Once we start layering ref backends (and\n> already we're moving towards a \"files\" layer which sits atop loose and\n> packed backends, right?), how do we avoid backends stomping on each\n> other (or worse, dereferencing somebody else's data as their own\n> struct)?\n\nMy conception is that layered backends would be separated as much as\npossible, and if the \"top\" layer needs to modify the \"bottom\" layer, it\nwould do so via a separate reference transaction on the bottom layer.\nThat transaction would be owned by the bottom layer, which would be able\nto use the corresponding `backend_data` pointers however it likes.\n\nYou can see an example of this construct in patch 08.\n\nMichael\n"},{"id":"327762","messageId":"20170908083305.hjcsa4ejlujwvj4h@sigill.intra.peff.net","threadId":"46681","inReplyTo":"6c4cf1eb-7991-1a39-a298-7839872ed3f5@alum.mit.edu","subject":"Re: [PATCH 02/10] struct ref_transaction: add a place for backends to store data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T08:33:06Z","receivedAt":"2017-09-08T08:33:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 10:19:48AM +0200, Michael Haggerty wrote:\n\n> > This is just one pointer. Once we start layering ref backends (and\n> > already we're moving towards a \"files\" layer which sits atop loose and\n> > packed backends, right?), how do we avoid backends stomping on each\n> > other (or worse, dereferencing somebody else's data as their own\n> > struct)?\n> \n> My conception is that layered backends would be separated as much as\n> possible, and if the \"top\" layer needs to modify the \"bottom\" layer, it\n> would do so via a separate reference transaction on the bottom layer.\n> That transaction would be owned by the bottom layer, which would be able\n> to use the corresponding `backend_data` pointers however it likes.\n>\n> You can see an example of this construct in patch 08.\n\nOK, that makes sense.\n\n-Peff\n"},{"id":"327764","messageId":"d49a7b75-66d4-7c63-d260-71c7413b3571@alum.mit.edu","threadId":"46681","inReplyTo":"20170908065223.f7f52hjcx5qsjbch@sigill.intra.peff.net","subject":"Re: [PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T10:02:44Z","receivedAt":"2017-09-08T10:02:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/08/2017 08:52 AM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 10:20:25AM +0200, Michael Haggerty wrote:\n> \n>> The old code incremented the packed ref cache reference count when\n>> acquiring the packed-refs lock, and decremented the count when\n>> releasing the lock. This is unnecessary because a locked packed-refs\n>> file cannot be changed, so there is no reason for the cache to become\n>> stale.\n> \n> Hmm, I thought that after your last series, we might hold the lock but\n> update the packed-refs from a separate tempfile. I.e., 42dfa7ecef\n> (commit_packed_refs(): use a staging file separate from the lockfile,\n> 2017-06-23).\n> \n>> Moreover, the extra reference count causes a problem if we\n>> intentionally clear the packed refs cache, as we sometimes need to do\n>> if we change the cache in anticipation of writing a change to disk,\n>> but then the write to disk fails. In that case, `packed_refs_unlock()`\n>> would have no easy way to find the cache whose reference count it\n>> needs to decrement.\n>>\n>> This whole issue will soon become moot due to upcoming changes that\n>> avoid changing the in-memory cache as part of updating the packed-refs\n>> on disk, but this change makes that transition easier.\n> \n> All of this makes sense, and I'm happy this complexity is going away in\n> the long run. But I guess what I'm wondering is in the meantime if we\n> can have a sequence like:\n> \n> 1. We hold packed-refs.lock\n> \n> 2. We update the file without releasing the lock, via 42dfa7ecef.\n> \n> 3. Still holding the lock, we try to look at packed-refs. The\n> stat_validity code says no, we're not up to date.\n> \n> 4. We discard the old packed_ref_cache and reload it. Because its\n> reference count was not incremented during step 1, we actually\n> free() it.\n> \n> 5. We try to look at at the old freed pointer.\n> \n> There are several steps in there that might be implausible. So I'm\n> mostly playing devil's advocate here.\n> \n> I'm wondering if the \"don't validate while we hold the lock\" logic in\n> get_packed_refs_cache() means that step 3 is impossible.\n\nThat's one of the reasons your scenario can't happen, but that just begs\nthe question of whether the \"don't validate while we hold the lock\" code\nis wrongheaded.\n\nIn fact it's OK. The method by which the packed-refs file on disk is\nmodified at this point in the patch series is by modifying the packed\nref-cache and then writing the data from the ref-cache to disk. So the\npacked ref-cache remains fresh because any changes that we plan to make\nto the file are made in the cache first anyway.\n\nI'll explain that a bit better in the log message.\n\nThe next question is whether this change interacts badly with changes\nlater in the patch series, when we cease to modify the ref-cache before\nwriting the new packed-refs file. Here we're OK, too, because\nimmediately after `rename_tempfile()` is used to rename the new\npacked-refs file into place, we clear the packed ref-cache, so no\nsubsequent callers of `get_packed_refs_cache()` should see the stale cache.\n\nWhich in turn is partly because your step 5 is also implausible: code\nshouldn't be holding a pointer to the packed ref-cache across operations\nthat might change the file. (Code that calls `get_packed_refs_cache()`\nis OK because that function would see that `refs->cache` is NULL and\nreload it regardless of whether we hold the lock.)\n\nSo I think everything is OK, but thanks for making me think through and\nexplain it better :-)\n\n>> @@ -560,9 +559,7 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n>> \t */\n>> \tvalidate_packed_ref_cache(refs);\n>> \n>> -\tpacked_ref_cache = get_packed_ref_cache(refs);\n>> -\t/* Increment the reference count to prevent it from being freed: */\n>> -\tacquire_packed_ref_cache(packed_ref_cache);\n>> +\tget_packed_ref_cache(refs);\n> \n> It seems a bit funny to call a \"get\" function and throw away the return\n> value. Presumably we care about its side effect of updating refs->cache.\n> That might be worth a comment (though if this is all going away soon, I\n> care a lot less about niceties like that).\n\nI'm rerolling anyway, so I'll add a comment. Thanks.\n\nMichael\n"},{"id":"327765","messageId":"0b1b1276-5a69-6db3-ab5d-0733bc9781b1@alum.mit.edu","threadId":"46681","inReplyTo":"20170908072758.d3qvwsrm5t54kc63@sigill.intra.peff.net","subject":"Re: [PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T10:04:10Z","receivedAt":"2017-09-08T10:04:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/08/2017 09:27 AM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 10:20:30AM +0200, Michael Haggerty wrote:\n> \n>> Used a `packed_ref_store` transaction in the implementation of\n>> `files_initial_transaction_commit()` rather than using internal\n>> features of the packed ref store. This further decouples\n>> `files_ref_store` from `packed_ref_store`.\n> \n> Very nice to see these couplings going away.\n> \n> Minor nit: s/Used/Use/ in the commit message.\n\nThanks; will fix.\n\nMichael\n"},{"id":"327766","messageId":"c5ff9b3d-6526-44d5-f539-39cac3b05657@alum.mit.edu","threadId":"46681","inReplyTo":"xmqqshfxztn5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T10:06:25Z","receivedAt":"2017-09-08T10:06:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/08/2017 06:44 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> +\tgit for-each-ref $prefix >actual &&\n>> +\tgrep \"Unable to create $Q.*packed-refs.lock$Q: File exists\" err &&\n> \n> I added a squash for doing s/grep/test_i18n&/ here\n\nThanks for the fix. I always forget that gotcha.\n\n> are there other\n> issues in the series, or is the series more or less ready to be\n> polished in 'next'?\n\nI'm working on v2 right now.\n\nMichael\n"},{"id":"327768","messageId":"21ec2708-e8e2-8503-fe90-48a70c802cc6@alum.mit.edu","threadId":"46681","inReplyTo":"20170908073816.lnlq6lpabtiu7rra@sigill.intra.peff.net","subject":"Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T12:44:57Z","receivedAt":"2017-09-08T12:45:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/08/2017 09:38 AM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 10:20:32AM +0200, Michael Haggerty wrote:\n> \n>> First, the old code didn't obtain the `packed-refs` lock until\n>> `files_transaction_finish()`. This means that a failure to acquire the\n>> `packed-refs` lock (e.g., due to contention with another process)\n>> wasn't detected until it was too late (problems like this are supposed\n>> to be detected in the \"prepare\" phase). The new code acquires the\n>> `packed-refs` lock in `files_transaction_prepare()`, the same stage of\n>> the processing when the loose reference locks are being acquired,\n>> removing another reason why the \"prepare\" phase might succeed and the\n>> \"finish\" phase might nevertheless fail.\n> \n> That means we're holding the packed-refs lock for a slightly longer\n> period. I think this could mean worse lock contention between otherwise\n> unrelated transactions over the packed-refs file. I wonder if the\n> lock-retry timeout might need to be increased to accommodate this. On\n> the other hand, it looks like we take it after getting the individual\n> locks, which I'd think would be the expensive part.\n\nThat was my thinking, yes. While the packed-refs lock is held, the\nreferences being created/updated have their reflogs written and are\nrenamed into place. I don't see how that can be shortened without\ncompromising on correctness (in particular, that we want to process\ncreates/updates before deletions to try to preserve reachability as much\nas possible during the transaction). As an added optimization, the\npacked-refs lock is not acquired at all if no references are being deleted.\n\n> Are there other callers who take the packed-refs and individual locks in\n> the reverse order? I think git-pack-refs might. Could we \"deadlock\" with\n> pack-refs? There are timeouts so it would resolve itself quickly, but I\n> wonder if we'd have a case that would deadlock-until-timeout that\n> otherwise could succeed.\n\nThat's not true. `files_pack_refs()` does the following:\n\n1. Lock the `packed-refs` file.\n\n2. Start a packed ref-store transaction.\n\n3. Iterate over the loose ref cache. For each reference that should\n be packed:\n * add it to the packed-refs transaction as an update to set it\n to the loose value (without specifying an old_sha1)\n * if pruning is on, schedule the loose reference to be pruned\n (in an internal data structure, without locking the file).\n\n4. Commit the packed ref-store transaction.\n\n5. Release the packed-refs lock.\n\n6. For each to-prune loose ref:\n * lock the loose reference\n * verify that it still has the value that was just written to\n the `packed-refs` file\n * if so, delete the loose version of the reference\n * unlock the loose reference\n\nThe packed-refs file and loose references are never locked at the same\ntime during pack-refs, so no deadlock is possible.\n\nBut you are right to assume that it *should* be so. The algorithm\nwritten above is a tiny bit unsafe (and has been for years). It is\npossible, though admittedly very unlikely, for the following to happen\nin the gap between steps 5 and 6:\n\n1. One process updates the value of branch \"foo\" from A to B.\n (Remember that A is the value that was just written to the\n `packed-refs` file by step 4.)\n\n2. `pack-refs` is run *again* (while the first `pack-refs` is out\n on its lunch break), writes value B to the `packed-refs` file\n for \"foo\", and deletes the loose version of \"foo\".\n\n3. Yet *another* process changes \"foo\" back from B to A. This\n creates a loose version of \"foo\" with value A, which overwrites\n the packed version with value B.\n\n4. The first `pack-refs` process resumes at step 6, sees that\n \"foo\" \"still\" has the value \"A\", so deletes the loose reference.\n\nThis would leave \"foo\" at the obsolete value \"B\" (i.e., the value\nwritten to the `packed-refs` file for it by the nested `pack-refs` process.\n\nI think that fixing this problem would require the `packed-refs` lock to\nbe held while `pack-refs` is pruning the loose references. But given how\nunlikely that chain of events seems, and that fixing it would increase\ncontention on the `packed-refs` file and allow the deadlock that you\ndescribed, I lean towards leaving it as-is. Though admittedly,\ncontention over a loose reference lock could make the race more likely\nto be hit.\n\nI also just noticed that the `refs_to_prune` linked list is leaked here\n(as has also been the case for many years). I'll fix that separately.\n\n>> Second, the old code deleted the loose version of a reference before\n>> deleting any packed version of the same reference. This left a moment\n>> when another process might think that the packed version of the\n>> reference is current, which is incorrect. (Even worse, the packed\n>> version of the reference can be arbitrarily old, and might even point\n>> at an object that has since been garbage-collected.)\n>>\n>> Third, if a reference deletion fails to acquire the `packed-refs` lock\n>> altogether, then the old code might leave the repository in the\n>> incorrect state (possibly corrupt) described in the previous\n>> paragraph.\n> \n> And to think I had the hubris to claim a few weeks ago that we had\n> probably weeded out all of the weird packed-refs inconsistency and\n> race-condition bugs. :)\n\nHaha, job security.\n\nMichael\n"},{"id":"327769","messageId":"20170908125733.afpo66vbummuaalg@sigill.intra.peff.net","threadId":"46681","inReplyTo":"21ec2708-e8e2-8503-fe90-48a70c802cc6@alum.mit.edu","subject":"Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T12:57:33Z","receivedAt":"2017-09-08T12:57:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 02:44:57PM +0200, Michael Haggerty wrote:\n\n> > That means we're holding the packed-refs lock for a slightly longer\n> > period. I think this could mean worse lock contention between otherwise\n> > unrelated transactions over the packed-refs file. I wonder if the\n> > lock-retry timeout might need to be increased to accommodate this. On\n> > the other hand, it looks like we take it after getting the individual\n> > locks, which I'd think would be the expensive part.\n> \n> That was my thinking, yes. While the packed-refs lock is held, the\n> references being created/updated have their reflogs written and are\n> renamed into place. I don't see how that can be shortened without\n> compromising on correctness (in particular, that we want to process\n> creates/updates before deletions to try to preserve reachability as much\n> as possible during the transaction). As an added optimization, the\n> packed-refs lock is not acquired at all if no references are being deleted.\n\nMakes sense. I guess in theory a process could do significant unrelated\nwork between the \"prepare\" and \"finish\" steps, while holding the lock.\nBut I don't know why it would, and arguably that itself is a bug.\n\n> The packed-refs file and loose references are never locked at the same\n> time during pack-refs, so no deadlock is possible.\n> \n> But you are right to assume that it *should* be so. The algorithm\n> written above is a tiny bit unsafe (and has been for years). It is\n> possible, though admittedly very unlikely, for the following to happen\n> in the gap between steps 5 and 6:\n\nThanks for explaining (and I think that your \"should\" is why I thought\nit was so; we've discussed this race before).\n\n> This would leave \"foo\" at the obsolete value \"B\" (i.e., the value\n> written to the `packed-refs` file for it by the nested `pack-refs` process.\n> \n> I think that fixing this problem would require the `packed-refs` lock to\n> be held while `pack-refs` is pruning the loose references. But given how\n> unlikely that chain of events seems, and that fixing it would increase\n> contention on the `packed-refs` file and allow the deadlock that you\n> described, I lean towards leaving it as-is. Though admittedly,\n> contention over a loose reference lock could make the race more likely\n> to be hit.\n\nAgreed. It's a pretty unlikely sequence of events. And IMHO the real\nsolution is a new storage format that's easier to reason about.\n\n-Peff\n"}]}