{"thread":{"id":"47026","subject":"[PATCH 0/2] Avoid rewriting \"packed-refs\" unnecessarily","startedAt":"2017-10-25T09:53:34Z","lastAt":"2017-10-28T04:11:54Z","messageCount":7,"participants":["Michael Haggerty","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"331001","messageId":"cover.1508924577.git.mhagger@alum.mit.edu","threadId":"47026","inReplyTo":null,"subject":"[PATCH 0/2] Avoid rewriting \"packed-refs\" unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-25T09:53:19Z","receivedAt":"2017-10-25T09:53:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since commit dc39e09942 (files_ref_store: use a transaction to update\npacked refs, 2017-09-08), we've been rewriting the `packed-refs` file\nunnecessarily when deleting a loose reference that has no packed\ncounterpart. Add some tests demonstrating this problem, then fix it by\nteaching `files_backend` to see whether any references being deleted\nhave packed versions, and if not, skipping the packed_refs\ntransaction.\n\nThis is a mild regression since 2.14, which avoided rewriting the\n`packed-refs` file in these cases (though it still had to *read* the\nwhole file to determine whether the rewrite could be skipped).\nTherefore, it might be considered for 2.15. On the other hand, it is\nlate in the release cycle, so the risk of accepting this change might\nbe considered too risky.\n\nThese patches apply on top of master and commute with\nmh/ref-locking-fix. They are also available from my GitHub fork [1] as\nbranch `avoid-rewriting-packed-refs`.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (2):\n  t1409: check that `packed-refs` is not rewritten unnecessarily\n  files-backend: don't rewrite the `packed-refs` file unnecessarily\n\n refs/files-backend.c          |  18 ++++++-\n refs/packed-backend.c         |  68 ++++++++++++++++++++++++\n refs/packed-backend.h         |   8 +++\n t/t1409-avoid-packing-refs.sh | 118 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 211 insertions(+), 1 deletion(-)\n create mode 100755 t/t1409-avoid-packing-refs.sh\n\n-- \n2.14.1\n\n"},{"id":"331002","messageId":"4c03fc458f619a5f02dd4cae38da8b9b139b850b.1508924577.git.mhagger@alum.mit.edu","threadId":"47026","inReplyTo":"cover.1508924577.git.mhagger@alum.mit.edu","subject":"[PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-25T09:53:21Z","receivedAt":"2017-10-25T09:53:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Even when we are deleting references, we needn't overwrite the\n`packed-refs` file if the references that we are deleting are not\npresent in that file. Implement this optimization as follows:\n\n* Add a function `is_packed_transaction_noop()`, which checks whether\n  a given packed-refs transaction doesn't actually have to do\n  anything. This function must be called while holding the\n  `packed-refs` lock to avoid races.\n\n* Change `files_transaction_prepare()` to check whether the\n  packed-refs transaction is unneeded. If so, squelch it, but continue\n  holding the `packed-refs` lock until the end of the transaction to\n  avoid races.\n\nThis fixes two tests in t1409.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs/files-backend.c          | 18 +++++++++++-\n refs/packed-backend.c         | 68 +++++++++++++++++++++++++++++++++++++++++++\n refs/packed-backend.h         |  8 +++++\n t/t1409-avoid-packing-refs.sh |  4 +--\n 4 files changed, 95 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 014dabb0bf..5689e3a58d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2605,7 +2605,23 @@ static int files_transaction_prepare(struct ref_store *ref_store,\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+\n+\t\tif (is_packed_transaction_noop(refs->packed_ref_store,\n+\t\t\t\t\t       packed_transaction)) {\n+\t\t\t/*\n+\t\t\t * We can skip rewriting the `packed-refs`\n+\t\t\t * file. But we do need to leave it locked, so\n+\t\t\t * that somebody else doesn't pack a reference\n+\t\t\t * that we are trying to delete.\n+\t\t\t */\n+\t\t\tif (ref_transaction_abort(packed_transaction, err)) {\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto cleanup;\n+\t\t\t}\n+\t\t\tbackend_data->packed_transaction = NULL;\n+\t\t} else {\n+\t\t\tret = ref_transaction_prepare(packed_transaction, err);\n+\t\t}\n \t}\n \n cleanup:\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3279d42c5a..064b1b58a2 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1261,6 +1261,74 @@ static int write_with_updates(struct packed_ref_store *refs,\n \treturn -1;\n }\n \n+int is_packed_transaction_noop(struct ref_store *ref_store,\n+\t\t\t       struct ref_transaction *transaction)\n+{\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ,\n+\t\t\t\"is_packed_transaction_noop\");\n+\tstruct strbuf referent = STRBUF_INIT;\n+\tsize_t i;\n+\tint ret;\n+\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tBUG(\"is_packed_transaction_noop() called while unlocked\");\n+\n+\t/*\n+\t * We're only going to bother returning true for the common,\n+\t * trivial case that references are only being deleted, their\n+\t * old values are not being checked, and the old `packed-refs`\n+\t * file doesn't contain any of those reference(s). More\n+\t * complicated cases (1) are unlikely to be able to be\n+\t * optimized away anyway, and (2) are more expensive to check.\n+\t * Start with cheap checks:\n+\t */\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_OLD)\n+\t\t\t/* Have to check the old value -> not a NOOP. */\n+\t\t\treturn 0;\n+\n+\t\tif ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))\n+\t\t\t/* Have to set a new value -> not a NOOP. */\n+\t\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * The transaction isn't checking any old values nor is it\n+\t * setting any nonzero new values, so it still might be a\n+\t * NOOP. Now do the more expensive check: the update is not a\n+\t * NOOP if one of the updates is a delete, and the old\n+\t * `packed-refs` file contains a value for that reference.\n+\t */\n+\tret = 1;\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tunsigned int type;\n+\t\tstruct object_id oid;\n+\n+\t\tif (!(update->flags & REF_HAVE_NEW))\n+\t\t\t/* This reference isn't being deleted -> NOOP. */\n+\t\t\tcontinue;\n+\n+\t\tif (!refs_read_raw_ref(ref_store, update->refname,\n+\t\t\t\t       oid.hash, &referent, &type) ||\n+\t\t    errno != ENOENT) {\n+\t\t\t/*\n+\t\t\t * We might have to actually delete that\n+\t\t\t * reference -> not a NOOP.\n+\t\t\t */\n+\t\t\tret = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&referent);\n+\treturn ret;\n+}\n+\n struct packed_transaction_backend_data {\n \t/* True iff the transaction owns the packed-refs lock. */\n \tint own_lock;\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 61687e408a..0b8b2b9695 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -23,4 +23,12 @@ 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+/*\n+ * Return true if `transaction` is an obvious NOOP with respect to the\n+ * specified packed_ref_store. `ref_store` must be locked before\n+ * calling this function.\n+ */\n+int is_packed_transaction_noop(struct ref_store *ref_store,\n+\t\t\t       struct ref_transaction *transaction);\n+\n #endif /* REFS_PACKED_BACKEND_H */\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex a2397c7b71..e5cb8a252d 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -26,7 +26,7 @@ test_expect_success 'setup' '\n \tC=$(git rev-parse HEAD)\n '\n \n-test_expect_failure 'do not create packed-refs file gratuitously' '\n+test_expect_success 'do not create packed-refs file gratuitously' '\n \ttest_must_fail test -f .git/packed-refs &&\n \tgit update-ref refs/heads/foo $A &&\n \ttest_must_fail test -f .git/packed-refs &&\n@@ -107,7 +107,7 @@ test_expect_success 'leave packed-refs untouched on verify of loose' '\n \tcheck_packed_refs_marked\n '\n \n-test_expect_failure 'leave packed-refs untouched on delete of loose' '\n+test_expect_success 'leave packed-refs untouched on delete of loose' '\n \tgit pack-refs --all &&\n \tgit update-ref refs/heads/loose-delete $A &&\n \tmark_packed_refs &&\n-- \n2.14.1\n\n"},{"id":"331003","messageId":"7d8bafad7491e4040ad4364bdc22b05d9b1d540d.1508924577.git.mhagger@alum.mit.edu","threadId":"47026","inReplyTo":"cover.1508924577.git.mhagger@alum.mit.edu","subject":"[PATCH 1/2] t1409: check that `packed-refs` is not rewritten unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-25T09:53:20Z","receivedAt":"2017-10-25T09:53:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no need to rewrite the `packed-refs` file except for the case\nthat we are deleting a reference that has a packed version. Verify\nthat `packed-refs` is not rewritten when it shouldn't be.\n\nIn fact, two of these tests fail:\n\n* A new (empty) `packed-refs` file is created when deleting any loose\n  reference and no `packed-refs` file previously existed.\n\n* The `packed-refs` file is rewritten unnecessarily when deleting a\n  loose reference that has no packed counterpart.\n\nBoth problems will be fixed in the next commit.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t1409-avoid-packing-refs.sh | 118 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 118 insertions(+)\n create mode 100755 t/t1409-avoid-packing-refs.sh\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nnew file mode 100755\nindex 0000000000..a2397c7b71\n--- /dev/null\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -0,0 +1,118 @@\n+#!/bin/sh\n+\n+test_description='avoid rewriting packed-refs unnecessarily'\n+\n+. ./test-lib.sh\n+\n+# Add an identifying mark to the packed-refs file header line. This\n+# shouldn't upset readers, and it should be omitted if the file is\n+# ever rewritten.\n+mark_packed_refs () {\n+\tsed -e \"s/^\\(#.*\\)/\\1 t1409 /\" <.git/packed-refs >.git/packed-refs.new &&\n+\tmv .git/packed-refs.new .git/packed-refs\n+}\n+\n+# Verify that the packed-refs file is still marked.\n+check_packed_refs_marked () {\n+\tgrep -q '^#.* t1409 ' .git/packed-refs\n+}\n+\n+test_expect_success 'setup' '\n+\tgit commit --allow-empty -m \"Commit A\" &&\n+\tA=$(git rev-parse HEAD) &&\n+\tgit commit --allow-empty -m \"Commit B\" &&\n+\tB=$(git rev-parse HEAD) &&\n+\tgit commit --allow-empty -m \"Commit C\" &&\n+\tC=$(git rev-parse HEAD)\n+'\n+\n+test_expect_failure 'do not create packed-refs file gratuitously' '\n+\ttest_must_fail test -f .git/packed-refs &&\n+\tgit update-ref refs/heads/foo $A &&\n+\ttest_must_fail test -f .git/packed-refs &&\n+\tgit update-ref refs/heads/foo $B &&\n+\ttest_must_fail test -f .git/packed-refs &&\n+\tgit update-ref refs/heads/foo $C $B &&\n+\ttest_must_fail test -f .git/packed-refs &&\n+\tgit update-ref -d refs/heads/foo &&\n+\ttest_must_fail test -f .git/packed-refs\n+'\n+\n+test_expect_success 'check that marking the packed-refs file works' '\n+\tgit for-each-ref >expected &&\n+\tgit pack-refs --all &&\n+\tmark_packed_refs &&\n+\tcheck_packed_refs_marked &&\n+\tgit for-each-ref >actual &&\n+\ttest_cmp expected actual &&\n+\tgit pack-refs --all &&\n+\ttest_must_fail check_packed_refs_marked &&\n+\tgit for-each-ref >actual2 &&\n+\ttest_cmp expected actual2\n+'\n+\n+test_expect_success 'leave packed-refs untouched on update of packed' '\n+\tgit update-ref refs/heads/packed-update $A &&\n+\tgit pack-refs --all &&\n+\tmark_packed_refs &&\n+\tgit update-ref refs/heads/packed-update $B &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_success 'leave packed-refs untouched on checked update of packed' '\n+\tgit update-ref refs/heads/packed-checked-update $A &&\n+\tgit pack-refs --all &&\n+\tmark_packed_refs &&\n+\tgit update-ref refs/heads/packed-checked-update $B $A &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_success 'leave packed-refs untouched on verify of packed' '\n+\tgit update-ref refs/heads/packed-verify $A &&\n+\tgit pack-refs --all &&\n+\tmark_packed_refs &&\n+\techo \"verify refs/heads/packed-verify $A\" | git update-ref --stdin &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_success 'touch packed-refs on delete of packed' '\n+\tgit update-ref refs/heads/packed-delete $A &&\n+\tgit pack-refs --all &&\n+\tmark_packed_refs &&\n+\tgit update-ref -d refs/heads/packed-delete &&\n+\ttest_must_fail check_packed_refs_marked\n+'\n+\n+test_expect_success 'leave packed-refs untouched on update of loose' '\n+\tgit pack-refs --all &&\n+\tgit update-ref refs/heads/loose-update $A &&\n+\tmark_packed_refs &&\n+\tgit update-ref refs/heads/loose-update $B &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_success 'leave packed-refs untouched on checked update of loose' '\n+\tgit pack-refs --all &&\n+\tgit update-ref refs/heads/loose-checked-update $A &&\n+\tmark_packed_refs &&\n+\tgit update-ref refs/heads/loose-checked-update $B $A &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_success 'leave packed-refs untouched on verify of loose' '\n+\tgit pack-refs --all &&\n+\tgit update-ref refs/heads/loose-verify $A &&\n+\tmark_packed_refs &&\n+\techo \"verify refs/heads/loose-verify $A\" | git update-ref --stdin &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_expect_failure 'leave packed-refs untouched on delete of loose' '\n+\tgit pack-refs --all &&\n+\tgit update-ref refs/heads/loose-delete $A &&\n+\tmark_packed_refs &&\n+\tgit update-ref -d refs/heads/loose-delete &&\n+\tcheck_packed_refs_marked\n+'\n+\n+test_done\n-- \n2.14.1\n\n"},{"id":"331038","messageId":"CAPig+cSrjiyGUm0n5dvowjNYZMSGvb+SZJ=haTsxXi9NcThA7A@mail.gmail.com","threadId":"47026","inReplyTo":"cover.1508924577.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Avoid rewriting \"packed-refs\" unnecessarily","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-25T20:10:08Z","receivedAt":"2017-10-25T20:10:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 25, 2017 at 5:53 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Since commit dc39e09942 (files_ref_store: use a transaction to update\n> packed refs, 2017-09-08), we've been rewriting the `packed-refs` file\n> unnecessarily when deleting a loose reference that has no packed\n> counterpart. Add some tests demonstrating this problem, then fix it by\n> teaching `files_backend` to see whether any references being deleted\n> have packed versions, and if not, skipping the packed_refs\n> transaction.\n>\n> This is a mild regression since 2.14, which avoided rewriting the\n> `packed-refs` file in these cases [...]\n\nShould the above information (that this fixes a regression) be\nmentioned in the commit message of at least one of the patches\n(probably 2/2)? Without such context, it may not be clear to someone\nlater reading those commit message -- someone who wasn't following\nthis email discussion -- that this behavior had been lost and is now\nbeing restored.\n"},{"id":"331069","messageId":"xmqqpo9ae79w.fsf@gitster.mtv.corp.google.com","threadId":"47026","inReplyTo":"4c03fc458f619a5f02dd4cae38da8b9b139b850b.1508924577.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-26T06:46:03Z","receivedAt":"2017-10-26T06:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Even when we are deleting references, we needn't overwrite the\n> `packed-refs` file if the references that we are deleting are not\n> present in that file. Implement this optimization as follows:\n\nThis goal I can understand.  files-transaction-prepare may see a\ndeletion and in order to avoid a deletion of a ref from the\nfile-backend to expose a stale entry in the packed-refs file, it\nprepares a transaction to remove the same ref that might exist in\nit.  If it turns out that there is no such ref in the packed-refs\nfile, then we can leave the packed-refs file as-is without\nrewriting.\n\n> * Add a function `is_packed_transaction_noop()`, which checks whether\n>   a given packed-refs transaction doesn't actually have to do\n>   anything. This function must be called while holding the\n>   `packed-refs` lock to avoid races.\n\nThis checks three things only to cover the most trivial case (I am\nperfectly happy that it punts on more complex cases).  If an update\n\n - checks the old value,\n\n - sets a new value, or\n\n - deletes a ref that is not on the filesystem,\n\nit is not a trivial case.  I can understand the latter two (i.e. We\nare special casing the deletion, and an update with a new value is\nnot.  If removing a file from the filesystem is not sufficient to\ndelete, we may have to rewrite the packed-refs), but not the first\none.  Is it because currently we do not say \"delete this ref only\nwhen its current value is X\"?\n\nAlso it is not immediately obvious how the \"is this noop\" helper is\nchecking the absence of the same-named ref in the current\npacked-refs file, which is what matters for the correctness of the\noptimization.\n\n"},{"id":"331074","messageId":"edd90206-7ad3-739e-b8ab-5aee469a8a48@alum.mit.edu","threadId":"47026","inReplyTo":"xmqqpo9ae79w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] files-backend: don't rewrite the `packed-refs` file unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-26T07:35:30Z","receivedAt":"2017-10-26T07:35:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/26/2017 08:46 AM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Even when we are deleting references, we needn't overwrite the\n>> `packed-refs` file if the references that we are deleting are not\n>> present in that file. Implement this optimization as follows:\n> \n> This goal I can understand.  files-transaction-prepare may see a\n> deletion and in order to avoid a deletion of a ref from the\n> file-backend to expose a stale entry in the packed-refs file, it\n> prepares a transaction to remove the same ref that might exist in\n> it.  If it turns out that there is no such ref in the packed-refs\n> file, then we can leave the packed-refs file as-is without\n> rewriting.\n> \n>> * Add a function `is_packed_transaction_noop()`, which checks whether\n>>   a given packed-refs transaction doesn't actually have to do\n>>   anything. This function must be called while holding the\n>>   `packed-refs` lock to avoid races.\n> \n> This checks three things only to cover the most trivial case (I am\n> perfectly happy that it punts on more complex cases).  If an update\n> \n>  - checks the old value,\n> \n>  - sets a new value, or\n> \n>  - deletes a ref that is not on the filesystem,\n> \n> it is not a trivial case.  I can understand the latter two (i.e. We\n> are special casing the deletion, and an update with a new value is\n> not.  If removing a file from the filesystem is not sufficient to\n> delete, we may have to rewrite the packed-refs), but not the first\n> one.  Is it because currently we do not say \"delete this ref only\n> when its current value is X\"?\n\nIt wouldn't be too hard to allow updates with REF_HAVE_OLD to be\noptimized away too. I didn't do it because\n\n1. We currently only create updates for that packed_refs backend with\nREF_HAVE_OLD turned off, so such could would be unreachable (and\nuntestable).\n\n2. I wanted to keep the patch as simple as possible in case you decide\nto merge it into 2.15.\n\nThere would also be a little bit of a leveling violation (or maybe the\nfunction name is not ideal). `is_packed_transaction_noop()` could check\nthat `old_oid` values are correct, and if they all are, declare the\ntransaction a NOOP. (It wasn't *really* a NOOP, but its OP, namely\nchecking old values, has already been carried out.) But what if it finds\nthat an `old_oid` was incorrect? Right now\n`is_packed_transaction_noop()` has no way to report something like a\nTRANSACTION_GENERIC_ERROR.\n\nIt could be that long-term it makes more sense for this optimization to\nhappen in `packed_transaction_prepare()`. Except that function is\nsometimes called for its side-effects, like sorting an existing file, in\nwhich case overwriting the `packed-refs` file shouldn't be optimized away.\n\nSo overall it seemed easier to punt on this optimization at this point.\n\n> Also it is not immediately obvious how the \"is this noop\" helper is\n> checking the absence of the same-named ref in the current\n> packed-refs file, which is what matters for the correctness of the\n> optimization.\n\nThe check is in the second loop in `is_packed_transaction_noop()`:\n\n\tif (!refs_read_raw_ref(ref_store, update->refname,\n\t\t\t       oid.hash, &referent, &type) ||\n\t    errno != ENOENT) {\n\t\t/*\n\t\t * We might have to actually delete that\n\t\t * reference -> not a NOOP.\n\t\t */\n\t\tret = 0;\n\t\tbreak;\n\t}\n\nIf the refname doesn't exist, then `refs_read_raw_ref()` returns -1 and\nsets errno to ENOENT, and the loop continues. Otherwise we exit with a\nvalue of 0, meaning that the transaction is not a NOOP.\n\nThere are a lot of double-negatives here. It might be easier to follow\nthe logic if the sense of the function were inverted:\n`is_packed_transaction_needed()`. Let me know if you have a strong\nfeeling about it.\n\nMichael\n"},{"id":"331204","messageId":"48b59e2b-61f8-c43a-63a7-d0fb32c9b4b7@alum.mit.edu","threadId":"47026","inReplyTo":"CAPig+cSrjiyGUm0n5dvowjNYZMSGvb+SZJ=haTsxXi9NcThA7A@mail.gmail.com","subject":"Re: [PATCH 0/2] Avoid rewriting \"packed-refs\" unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-28T04:11:45Z","receivedAt":"2017-10-28T04:11:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/25/2017 10:10 PM, Eric Sunshine wrote:\n> On Wed, Oct 25, 2017 at 5:53 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> Since commit dc39e09942 (files_ref_store: use a transaction to update\n>> packed refs, 2017-09-08), we've been rewriting the `packed-refs` file\n>> unnecessarily when deleting a loose reference that has no packed\n>> counterpart. Add some tests demonstrating this problem, then fix it by\n>> teaching `files_backend` to see whether any references being deleted\n>> have packed versions, and if not, skipping the packed_refs\n>> transaction.\n>>\n>> This is a mild regression since 2.14, which avoided rewriting the\n>> `packed-refs` file in these cases [...]\n> \n> Should the above information (that this fixes a regression) be\n> mentioned in the commit message of at least one of the patches\n> (probably 2/2)? Without such context, it may not be clear to someone\n> later reading those commit message -- someone who wasn't following\n> this email discussion -- that this behavior had been lost and is now\n> being restored.\n\nYes, that's a good idea. I'll send a re-roll with that change.\n\nMichael\n"}]}