{"thread":{"id":"46727","subject":"[PATCH v2 01/11] packed-backend: don't adjust the reference count on lock/unlock","startedAt":"2017-09-08T13:52:11Z","lastAt":"2017-09-10T05:07:37Z","messageCount":15,"participants":["Michael Haggerty","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"327771","messageId":"1631f277bb86f653c5c679ca07fbcb2e92410046.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 01/11] packed-backend: don't adjust the reference count on lock/unlock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:43Z","receivedAt":"2017-09-08T13:52:11Z","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:\n\n* Another process cannot change the packed-refs file because it is\n  locked.\n\n* When we ourselves change the packed-refs file, we do so by first\n  modifying the packed ref-cache, and then writing the data from the\n  ref-cache to disk. So the packed ref-cache remains fresh because any\n  changes that we plan to make to the file are made in the cache first\n  anyway.\n\nSo there is no reason for the cache to become stale.\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 <mhagger@alum.mit.edu>\n---\n refs/packed-backend.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 412c85034f..b76f14e5b3 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,11 @@ 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+\t/*\n+\t * Now make sure that the packed-refs file as it exists in the\n+\t * locked state is loaded into the cache:\n+\t */\n+\tget_packed_ref_cache(refs);\n \treturn 0;\n }\n \n@@ -576,7 +577,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":"327772","messageId":"cover.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":null,"subject":"[PATCH v2 00/11] Implement transactions for the packed ref store","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:42Z","receivedAt":"2017-09-08T13:52:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of a patch series to implement reference transactions for\nthe packed refs-store. Thanks to Stefan, Brandon, Junio, and Peff for\nyour review of v1 [1]. I believe I have addressed all of your\ncomments.\n\nChanges since v1:\n\n* Patch [01/11]: justify the change better in the log message. Add a\n  comment explaining why `get_packed_ref_cache()` is being called but\n  the return value discarded.\n\n* Patch [05/11]: Lock the `packed-refs` file *after* successfully\n  creating the (empty) transaction object. This prevents leaving the\n  file locked if `ref_store_transaction_begin()` fails.\n\n* Patch [06/11]: New patch, fixing a leak of the `refs_to_prune`\n  linked list.\n\n* Patch [07/11]: Reimplement test \"no bogus intermediate values during\n  delete\" to work without polling. Also incorporate Junio's change\n  `s/grep/test_i18ngrep/`.\n\nThese changes are also available as branch `packed-ref-transactions`\nfrom my GitHub repo [2].\n\nMichael\n\n[1] https://public-inbox.org/git/cover.1503993268.git.mhagger@alum.mit.edu/\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (11):\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  prune_refs(): also free the linked list\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         | 214 ++++++++++++++------\n refs/packed-backend.c        | 461 +++++++++++++++++++++++++++++--------------\n refs/packed-backend.h        |  17 +-\n refs/refs-internal.h         |   1 +\n t/t1404-update-ref-errors.sh |  73 +++++++\n 5 files changed, 550 insertions(+), 216 deletions(-)\n\n-- \n2.14.1\n\n"},{"id":"327773","messageId":"196665b10584f1865d66d75f0b2d77a8a6416783.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 02/11] struct ref_transaction: add a place for backends to store data","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:44Z","receivedAt":"2017-09-08T13:52:18Z","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 <mhagger@alum.mit.edu>\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":"327774","messageId":"ebdd82025b884bdd632a6c4856d07a5011566571.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 05/11] files_pack_refs(): use a reference transaction to write packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:47Z","receivedAt":"2017-09-08T13:52:19Z","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 <mhagger@alum.mit.edu>\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..3475c6f8a2 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1100,6 +1100,11 @@ 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+\ttransaction = ref_store_transaction_begin(refs->packed_ref_store, &err);\n+\tif (!transaction)\n+\t\treturn -1;\n \n \tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);\n \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":"327775","messageId":"aa14ceccee9bcca73e3163f3626b8339326edad9.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 03/11] packed_ref_store: implement reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:45Z","receivedAt":"2017-09-08T13:52:25Z","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 <mhagger@alum.mit.edu>\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 b76f14e5b3..9ab65c5a0a 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -748,25 +748,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":"327776","messageId":"1107477d7134f9c3114ea564ca0dd740e761b537.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 07/11] files_initial_transaction_commit(): use a transaction for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:49Z","receivedAt":"2017-09-08T13:52:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use 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 <mhagger@alum.mit.edu>\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 60031fe3ae..2700e3b5d5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2669,6 +2669,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@@ -2701,6 +2702,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@@ -2713,6 +2720,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@@ -2720,21 +2736,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":"327777","messageId":"f98f9692ad15091c79ac0bb514a70c3facb65a3b.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 10/11] packed-backend: rip out some now-unused code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:52Z","receivedAt":"2017-09-08T13:52:31Z","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 <mhagger@alum.mit.edu>\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 9d5f76b1dc..0279aeceea 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@@ -596,152 +549,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":"327778","messageId":"1e04a8769227abe850863ba75d10b43396362af4.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 11/11] files_transaction_finish(): delete reflogs before references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:53Z","receivedAt":"2017-09-08T13:52:35Z","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 <mhagger@alum.mit.edu>\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 29eb5e826f..961424a4ea 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2636,6 +2636,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@@ -2672,20 +2693,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":"327779","messageId":"efcd2e74427a4b05566c1afaf81af02f4f9a5672.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 04/11] packed_delete_refs(): implement method","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:46Z","receivedAt":"2017-09-08T13:53:08Z","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 <mhagger@alum.mit.edu>\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 9ab65c5a0a..9d5f76b1dc 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1086,7 +1086,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":"327780","messageId":"3356d2dae41d5b9cff120a88a0b25bf61e959fe0.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 09/11] files_ref_store: use a transaction to update packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:51Z","receivedAt":"2017-09-08T13:53:10Z","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 <mhagger@alum.mit.edu>\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 2700e3b5d5..29eb5e826f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2397,13 +2397,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@@ -2415,6 +2424,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@@ -2431,12 +2451,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@@ -2503,6 +2528,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@@ -2510,7 +2570,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@@ -2525,9 +2585,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@@ -2536,6 +2597,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@@ -2571,7 +2635,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@@ -2589,39 +2669,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@@ -2639,7 +2707,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@@ -2647,7 +2714,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 64a81345a8..100d50e362 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@@ -461,7 +461,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":"327781","messageId":"edcf7a0102ee168b9da080d38a4098774d37635e.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 06/11] prune_refs(): also free the linked list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:48Z","receivedAt":"2017-09-08T13:53:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"At least since v1.7, the elements of the `refs_to_prune` linked list\nhave been leaked. Fix the leak by teaching `prune_refs()` to free the\nlist elements as it processes them.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3475c6f8a2..60031fe3ae 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1057,11 +1057,17 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)\n \tstrbuf_release(&err);\n }\n \n-static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r)\n+/*\n+ * Prune the loose versions of the references in the linked list\n+ * `*refs_to_prune`, freeing the entries in the list as we go.\n+ */\n+static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_to_prune)\n {\n-\twhile (r) {\n+\twhile (*refs_to_prune) {\n+\t\tstruct ref_to_prune *r = *refs_to_prune;\n+\t\t*refs_to_prune = r->next;\n \t\tprune_ref(refs, r);\n-\t\tr = r->next;\n+\t\tfree(r);\n \t}\n }\n \n@@ -1148,7 +1154,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \n \tpacked_refs_unlock(refs->packed_ref_store);\n \n-\tprune_refs(refs, refs_to_prune);\n+\tprune_refs(refs, &refs_to_prune);\n \tstrbuf_release(&err);\n \treturn 0;\n }\n-- \n2.14.1\n\n"},{"id":"327782","messageId":"76d473f62a8c1d6328eb15003c4d0d4dbc8f277d.1504877858.git.mhagger@alum.mit.edu","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"[PATCH v2 08/11] t1404: demonstrate two problems with reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-08T13:51:50Z","receivedAt":"2017-09-08T13:53:17Z","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 <mhagger@alum.mit.edu>\n---\n t/t1404-update-ref-errors.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 73 insertions(+)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex c34ece48f5..64a81345a8 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -404,4 +404,77 @@ 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\t# Note: the following command is intentionally run in the\n+\t\t# background. We increase the timeout so that `update-ref`\n+\t\t# attempts to acquire the `packed-refs` lock for longer than\n+\t\t# it takes for us to do the check then delete it:\n+\t\tgit -c core.packedrefstimeout=3000 update-ref -d $prefix/foo &\n+\t} &&\n+\tpid2=$! &&\n+\t# Give update-ref plenty of time to get to the point where it tries\n+\t# to lock packed-refs:\n+\tsleep 1 &&\n+\t# Make sure that update-ref did not complete despite the lock:\n+\tkill -0 $pid2 &&\n+\t# Verify that the reference still has its old value:\n+\tsha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&\n+\tcase \"$sha1\" in\n+\t$D)\n+\t\t# This is what we hope for; it means that nothing\n+\t\t# user-visible has changed yet.\n+\t\t: ;;\n+\tundefined)\n+\t\t# This is not correct; it means the deletion has happened\n+\t\t# already even though update-ref should not have been\n+\t\t# able to acquire the lock yet.\n+\t\techo \"$prefix/foo deleted prematurely\" &&\n+\t\tbreak\n+\t\t;;\n+\t$C)\n+\t\t# This value should never be seen. Probably the loose\n+\t\t# reference has been deleted but the packed reference\n+\t\t# is still there:\n+\t\techo \"$prefix/foo incorrectly observed to be C\" &&\n+\t\tbreak\n+\t\t;;\n+\t*)\n+\t\t# WTF?\n+\t\techo \"unexpected value observed for $prefix/foo: $sha1\" &&\n+\t\tbreak\n+\t\t;;\n+\tesac >out &&\n+\trm -f .git/packed-refs.lock &&\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+\ttest_i18ngrep \"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":"327815","messageId":"20170909111753.pidf26f5koaewyho@sigill.intra.peff.net","threadId":"46727","inReplyTo":"76d473f62a8c1d6328eb15003c4d0d4dbc8f277d.1504877858.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-09T11:17:53Z","receivedAt":"2017-09-09T11:18:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote:\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\t# Note: the following command is intentionally run in the\n> +\t\t# background. We increase the timeout so that `update-ref`\n> +\t\t# attempts to acquire the `packed-refs` lock for longer than\n> +\t\t# it takes for us to do the check then delete it:\n> +\t\tgit -c core.packedrefstimeout=3000 update-ref -d $prefix/foo &\n> +\t} &&\n> +\tpid2=$! &&\n\nThere's some timing trickiness in this test, so I want to take a close\nlook at possible races.\n\nThe point of this timeout is just to make sure that we end up blocking\n\"long enough\" that the test code can ensure that we're blocked for a\ncertain period, during which we can look at the on-disk state.\n\nSo this timeout really could be as long as we want, and ideally longer\nis better (since we would not want it to exit while we're examining the\nstate). Later we do a `wait` on this process, but only after removing\nthe lockfile, which should cause it to exit. So in theory we could make\nthis something silly like an hour that could not possibly race.\n\nThe only downside, I guess, is that if something goes horribly wrong, it\ncould take an hour to exit (but we put the \"rm\" into a\ntest_when_finished, so I think that would cover the test failing early).\n\n> +\t# Give update-ref plenty of time to get to the point where it tries\n> +\t# to lock packed-refs:\n> +\tsleep 1 &&\n\nYuck. So this is definitely a potential race. On a busy system it could\ntake more than a second to try the lock.\n\nBut:\n\n  1. Since we're looking for the on-disk state _not_ to change, when we\n     lose the race the test still succeeds (it just tests nothing\n     useful). So we shouldn't get false positives.\n\n  2. I don't think we can do better. In the corrected state, the\n     sub-process makes no externally visible change that we could wait\n     on (unless we turned to unportable tools like strace).\n\nSo I think it's OK. I'm never excited about using sleep in our tests,\nbut I don't see a better option.\n\n> +\t# Make sure that update-ref did not complete despite the lock:\n> +\tkill -0 $pid2 &&\n\nI'm not sure if \"kill -0\" is portable to Windows or not. I have no\nspecific knowledge that it _isn't_, but signals have been a problem area\nfor us in the past. I see we use it for some of the p4 tests, but I\nwouldn't be surprised if those are already skipped on Windows.\n\nI guess if it produces false positives then Windows folks can report and\nmark it to be skipped. If it produces false negatives there, then nobody\nwill be the wiser, but there's not much we can do.\n\n> +\t# Verify that the reference still has its old value:\n> +\tsha1=$(git rev-parse --verify --quiet $prefix/foo || echo undefined) &&\n> +\tcase \"$sha1\" in\n> +\t$D)\n> +\t\t# This is what we hope for; it means that nothing\n> +\t\t# user-visible has changed yet.\n> +\t\t: ;;\n\nSo if we get what we want, we execute \":\" which should be a successful\nexit code.\n\n> +\tundefined)\n> +\t\t# This is not correct; it means the deletion has happened\n> +\t\t# already even though update-ref should not have been\n> +\t\t# able to acquire the lock yet.\n> +\t\techo \"$prefix/foo deleted prematurely\" &&\n> +\t\tbreak\n> +\t\t;;\n\nBut if we don't, we hit a \"break\". But we're not in a loop, so the break\ndoes nothing. Is the intent to give a false value to the switch so that\nwe fail the &&-chain? If so, I'd think \"false\" would be the right thing\nto use. It's more to the point, and from a few limited tests, it looks\nlike \"break\" will return \"0\" even outside a loop (bash writes a\ncomplaint to stderr, but dash doesn't).\n\nOr did you just forget that you're not writing C and that \";;\" is the\ncorrect way to spell \"break\" here? :)\n\n> [...]\n> +\tesac >out &&\n> [...]\n> +\ttest_must_be_empty out &&\n\nThe return value of \"break\" _doesn't_ matter, because you end up using\nthe presence of the error message.\n\nI think we could write this as just:\n\n  case \"$sha1\" in\n  $D)\n\t# good\n\t;;\n  undefined)\n        echo >&2 this is bad\n\tfalse\n\t;;\n  esac &&\n\nI'm OK with it either way (testing the exit code or testing the output),\nbut either way the \"break\" calls are doing nothing and can be dropped, I\nthink.\n\n-Peff\n"},{"id":"327816","messageId":"20170909111845.fzgbgzzgcjwcbilw@sigill.intra.peff.net","threadId":"46727","inReplyTo":"cover.1504877858.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/11] Implement transactions for the packed ref store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-09T11:18:45Z","receivedAt":"2017-09-09T11:18:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 08, 2017 at 03:51:42PM +0200, Michael Haggerty wrote:\n\n> This is v2 of a patch series to implement reference transactions for\n> the packed refs-store. Thanks to Stefan, Brandon, Junio, and Peff for\n> your review of v1 [1]. I believe I have addressed all of your\n> comments.\n> \n> Changes since v1:\n> \n> * Patch [01/11]: justify the change better in the log message. Add a\n>   comment explaining why `get_packed_ref_cache()` is being called but\n>   the return value discarded.\n> \n> * Patch [05/11]: Lock the `packed-refs` file *after* successfully\n>   creating the (empty) transaction object. This prevents leaving the\n>   file locked if `ref_store_transaction_begin()` fails.\n> \n> * Patch [06/11]: New patch, fixing a leak of the `refs_to_prune`\n>   linked list.\n\nThese all look good to me, including the new patch.\n\n> * Patch [07/11]: Reimplement test \"no bogus intermediate values during\n>   delete\" to work without polling. Also incorporate Junio's change\n>   `s/grep/test_i18ngrep/`.\n\nI picked a few nits in the test script. Nothing too serious, but a few\nthings that might be worth addressing.\n\n-Peff\n"},{"id":"327820","messageId":"cdfea8a5-fc86-095d-7f5f-89a8f922cac9@alum.mit.edu","threadId":"46727","inReplyTo":"20170909111753.pidf26f5koaewyho@sigill.intra.peff.net","subject":"Re: [PATCH v2 08/11] t1404: demonstrate two problems with reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-10T05:07:27Z","receivedAt":"2017-09-10T05:07:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/09/2017 01:17 PM, Jeff King wrote:\n> On Fri, Sep 08, 2017 at 03:51:50PM +0200, Michael Haggerty wrote:\n> [...]\n> So if we get what we want, we execute \":\" which should be a successful\n> exit code.\n\nI think the `:` is superfluous even if we care about the exit code of\nthe `case`. I'll remove it.\n\n>> +\tundefined)\n>> +\t\t# This is not correct; it means the deletion has happened\n>> +\t\t# already even though update-ref should not have been\n>> +\t\t# able to acquire the lock yet.\n>> +\t\techo \"$prefix/foo deleted prematurely\" &&\n>> +\t\tbreak\n>> +\t\t;;\n> \n> But if we don't, we hit a \"break\". But we're not in a loop, so the break\n> does nothing. Is the intent to give a false value to the switch so that\n> we fail the &&-chain? If so, I'd think \"false\" would be the right thing\n> to use. It's more to the point, and from a few limited tests, it looks\n> like \"break\" will return \"0\" even outside a loop (bash writes a\n> complaint to stderr, but dash doesn't).\n> \n> Or did you just forget that you're not writing C and that \";;\" is the\n> correct way to spell \"break\" here? :)\n\nAn earlier version of the patch used a loop and needed the `break`. But\nwhen I removed the loop, I probably didn't notice the now-unneeded\nbreaks because of what you said. I'll take them out.\n\n>> [...]\n>> +\tesac >out &&\n>> [...]\n>> +\ttest_must_be_empty out &&\n> \n> The return value of \"break\" _doesn't_ matter, because you end up using\n> the presence of the error message.\n> \n> I think we could write this as just:\n> \n>   case \"$sha1\" in\n>   $D)\n> \t# good\n> \t;;\n>   undefined)\n>         echo >&2 this is bad\n> \tfalse\n> \t;;\n>   esac &&\n> \n> I'm OK with it either way (testing the exit code or testing the output),\n> but either way the \"break\" calls are doing nothing and can be dropped, I\n> think.\n\nYes, using the exit code to decide success is simpler. I'll make that\nchange, too.\n\nThanks for your comments.\n\nMichael\n"}]}