{"thread":{"id":"46303","subject":"[PATCH v3 00/30] Create a reference backend for packed refs","startedAt":"2017-07-01T18:31:25Z","lastAt":"2021-06-05T01:07:47Z","messageCount":52,"participants":["Michael Haggerty","Jeff King","Stefan Beller","Jonathan Nieder","Junio C Hamano","Ævar Arnfjörð Bjarmason","Felipe Contreras"],"isPatch":true,"patchVersion":3,"patchTotal":30},"messages":[{"id":"323694","messageId":"cover.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":null,"subject":"[PATCH v3 00/30] Create a reference backend for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:38Z","receivedAt":"2017-07-01T18:31:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v3 of a patch series creating a `packed_ref_store` reference\nbackend. Thanks to Peff and Junio for their comments about v2 [1].\n\nChanges since v2:\n\n* Delete some debugging `cat` commands in t1408.\n\n* Add some tests of reading packed-refs files with bogus contents.\n\n* When reporting corruption in packed-refs files, distinguish between\n unterminated lines and other corruption.\n\n* Fixed a typo in a commit message.\n\nThis patch series can also be obtained from my GitHub fork [2] as\nbranch packed-ref-store.\n\nMichael\n\n[1] v1: http://public-inbox.org/git/cover.1497534157.git.mhagger@alum.mit.edu/\n v2: http://public-inbox.org/git/cover.1498200513.git.mhagger@alum.mit.edu/\n[2] https://github.com/mhagger/git\n\nJunio C Hamano (1):\n t1408: add a test of stale packed refs covered by loose refs\n\nMichael Haggerty (29):\n add_packed_ref(): teach function to overwrite existing refs\n packed_ref_store: new struct\n packed_ref_store: move `packed_refs_path` here\n packed_ref_store: move `packed_refs_lock` member here\n clear_packed_ref_cache(): take a `packed_ref_store *` parameter\n validate_packed_ref_cache(): take a `packed_ref_store *` parameter\n get_packed_ref_cache(): take a `packed_ref_store *` parameter\n get_packed_refs(): take a `packed_ref_store *` parameter\n add_packed_ref(): take a `packed_ref_store *` parameter\n lock_packed_refs(): take a `packed_ref_store *` parameter\n commit_packed_refs(): take a `packed_ref_store *` parameter\n rollback_packed_refs(): take a `packed_ref_store *` parameter\n get_packed_ref(): take a `packed_ref_store *` parameter\n repack_without_refs(): take a `packed_ref_store *` parameter\n packed_peel_ref(): new function, extracted from `files_peel_ref()`\n packed_ref_store: support iteration\n packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`\n packed-backend: new module for handling packed references\n packed_ref_store: make class into a subclass of `ref_store`\n commit_packed_refs(): report errors rather than dying\n commit_packed_refs(): use a staging file separate from the lockfile\n packed_refs_lock(): function renamed from lock_packed_refs()\n packed_refs_lock(): report errors via a `struct strbuf *err`\n packed_refs_unlock(), packed_refs_is_locked(): new functions\n clear_packed_ref_cache(): don't protest if the lock is held\n commit_packed_refs(): remove call to `packed_refs_unlock()`\n repack_without_refs(): don't lock or unlock the packed refs\n t3210: add some tests of bogus packed-refs file contents\n read_packed_refs(): die if `packed-refs` contains bogus data\n\n Makefile | 1 +\n refs.c | 18 +\n refs/files-backend.c | 631 ++++-------------------------------\n refs/packed-backend.c | 872 +++++++++++++++++++++++++++++++++++++++++++++++++\n refs/packed-backend.h | 25 ++\n refs/refs-internal.h | 10 +\n t/t1408-packed-refs.sh | 42 +++\n t/t3210-pack-refs.sh | 27 ++\n 8 files changed, 1066 insertions(+), 560 deletions(-)\n create mode 100644 refs/packed-backend.c\n create mode 100644 refs/packed-backend.h\n create mode 100755 t/t1408-packed-refs.sh\n\n-- \n2.11.0\n\n"},{"id":"323695","messageId":"4b60931d1aa472b0e25450eb6e9ef70d5b11ff6a.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 01/30] t1408: add a test of stale packed refs covered by loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:39Z","receivedAt":"2017-07-01T18:31:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Junio C Hamano \n\nIt is OK for the packed-refs file to contain old reference definitions\nthat might even refer to objects that have since been\ngarbage-collected, as long as there is a corresponding loose reference\ndefinition that overrides it. Add a test that such references don't\ncause problems.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Michael Haggerty \n---\n t/t1408-packed-refs.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n create mode 100755 t/t1408-packed-refs.sh\n\ndiff --git a/t/t1408-packed-refs.sh b/t/t1408-packed-refs.sh\nnew file mode 100755\nindex 0000000000..1e44a17eea\n--- /dev/null\n+++ b/t/t1408-packed-refs.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description='packed-refs entries are covered by loose refs'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m one &&\n+\tone=$(git rev-parse HEAD) &&\n+\tgit for-each-ref >actual &&\n+\techo \"$one commit\trefs/heads/master\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit pack-refs --all &&\n+\tgit for-each-ref >actual &&\n+\techo \"$one commit\trefs/heads/master\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit checkout --orphan another &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m two &&\n+\ttwo=$(git rev-parse HEAD) &&\n+\tgit checkout -B master &&\n+\tgit branch -D another &&\n+\n+\tgit for-each-ref >actual &&\n+\techo \"$two commit\trefs/heads/master\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit reflog expire --expire=now --all &&\n+\tgit prune &&\n+\tgit tag -m v1.0 v1.0 master\n+'\n+\n+test_expect_success 'no error from stale entry in packed-refs' '\n+\tgit describe master >actual 2>&1 &&\n+\techo \"v1.0\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.11.0\n\n"},{"id":"323696","messageId":"ae5f4e2818ca8b184500bef5044900a4250bec3a.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 03/30] packed_ref_store: new struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:41Z","receivedAt":"2017-07-01T18:31:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Start extracting the packed-refs-related data structures into a new\nclass, `packed_ref_store`. It doesn't yet implement `ref_store`, but\nit will.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 42 +++++++++++++++++++++++++++++++++---------\n 1 file changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 87cecde231..2efb71cee9 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -47,6 +47,28 @@ struct packed_ref_cache {\n \tstruct stat_validity validity;\n };\n \n+/*\n+ * A container for `packed-refs`-related data. It is not (yet) a\n+ * `ref_store`.\n+ */\n+struct packed_ref_store {\n+\tunsigned int store_flags;\n+\n+\t/*\n+\t * A cache of the values read from the `packed-refs` file, if\n+\t * it might still be current; otherwise, NULL.\n+\t */\n+\tstruct packed_ref_cache *cache;\n+};\n+\n+static struct packed_ref_store *packed_ref_store_create(unsigned int store_flags)\n+{\n+\tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n+\n+\trefs->store_flags = store_flags;\n+\treturn refs;\n+}\n+\n /*\n * Future: need to be in \"struct repository\"\n * when doing a full libification.\n@@ -60,13 +82,14 @@ struct files_ref_store {\n \tchar *packed_refs_path;\n \n \tstruct ref_cache *loose;\n-\tstruct packed_ref_cache *packed;\n \n \t/*\n \t * Lock used for the \"packed-refs\" file. Note that this (and\n \t * thus the enclosing `files_ref_store`) must not be freed.\n \t */\n \tstruct lock_file packed_refs_lock;\n+\n+\tstruct packed_ref_store *packed_ref_store;\n };\n \n /*\n@@ -95,12 +118,12 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n \n static void clear_packed_ref_cache(struct files_ref_store *refs)\n {\n-\tif (refs->packed) {\n-\t\tstruct packed_ref_cache *packed_refs = refs->packed;\n+\tif (refs->packed_ref_store->cache) {\n+\t\tstruct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;\n \n \t\tif (is_lock_file_locked(&refs->packed_refs_lock))\n \t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n-\t\trefs->packed = NULL;\n+\t\trefs->packed_ref_store->cache = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n \t}\n }\n@@ -132,6 +155,7 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n \tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n \trefs->packed_refs_path = strbuf_detach(&sb, NULL);\n+\trefs->packed_ref_store = packed_ref_store_create(flags);\n \n \treturn ref_store;\n }\n@@ -375,8 +399,8 @@ static void files_ref_path(struct files_ref_store *refs,\n */\n static void validate_packed_ref_cache(struct files_ref_store *refs)\n {\n-\tif (refs->packed &&\n-\t !stat_validity_check(&refs->packed->validity,\n+\tif (refs->packed_ref_store->cache &&\n+\t !stat_validity_check(&refs->packed_ref_store->cache->validity,\n \t\t\t\t files_packed_refs_path(refs)))\n \t\tclear_packed_ref_cache(refs);\n }\n@@ -396,10 +420,10 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n \tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tvalidate_packed_ref_cache(refs);\n \n-\tif (!refs->packed)\n-\t\trefs->packed = read_packed_refs(packed_refs_file);\n+\tif (!refs->packed_ref_store->cache)\n+\t\trefs->packed_ref_store->cache = read_packed_refs(packed_refs_file);\n \n-\treturn refs->packed;\n+\treturn refs->packed_ref_store->cache;\n }\n \n static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)\n-- \n2.11.0\n\n"},{"id":"323697","messageId":"aaaee3174089ccefc6e3d628163edfda5de16a4b.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 04/30] packed_ref_store: move `packed_refs_path` here","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:42Z","receivedAt":"2017-07-01T18:31:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move `packed_refs_path` from `files_ref_store` to `packed_ref_store`,\nand rename it to `path` since its meaning is clear from its new\ncontext.\n\nInline `files_packed_refs_path()`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 25 ++++++++++++-------------\n 1 file changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2efb71cee9..c4b8e2f63b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -54,6 +54,9 @@ struct packed_ref_cache {\n struct packed_ref_store {\n \tunsigned int store_flags;\n \n+\t/* The path of the \"packed-refs\" file: */\n+\tchar *path;\n+\n \t/*\n \t * A cache of the values read from the `packed-refs` file, if\n \t * it might still be current; otherwise, NULL.\n@@ -61,11 +64,13 @@ struct packed_ref_store {\n \tstruct packed_ref_cache *cache;\n };\n \n-static struct packed_ref_store *packed_ref_store_create(unsigned int store_flags)\n+static struct packed_ref_store *packed_ref_store_create(\n+\t\tconst char *path, unsigned int store_flags)\n {\n \tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n \n \trefs->store_flags = store_flags;\n+\trefs->path = xstrdup(path);\n \treturn refs;\n }\n \n@@ -79,7 +84,6 @@ struct files_ref_store {\n \n \tchar *gitdir;\n \tchar *gitcommondir;\n-\tchar *packed_refs_path;\n \n \tstruct ref_cache *loose;\n \n@@ -154,8 +158,8 @@ static struct ref_store *files_ref_store_create(const char *gitdir,\n \tget_common_dir_noenv(&sb, gitdir);\n \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n \tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n-\trefs->packed_refs_path = strbuf_detach(&sb, NULL);\n-\trefs->packed_ref_store = packed_ref_store_create(flags);\n+\trefs->packed_ref_store = packed_ref_store_create(sb.buf, flags);\n+\tstrbuf_release(&sb);\n \n \treturn ref_store;\n }\n@@ -343,11 +347,6 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \treturn packed_refs;\n }\n \n-static const char *files_packed_refs_path(struct files_ref_store *refs)\n-{\n-\treturn refs->packed_refs_path;\n-}\n-\n static void files_reflog_path(struct files_ref_store *refs,\n \t\t\t struct strbuf *sb,\n \t\t\t const char *refname)\n@@ -401,7 +400,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)\n {\n \tif (refs->packed_ref_store->cache &&\n \t !stat_validity_check(&refs->packed_ref_store->cache->validity,\n-\t\t\t\t files_packed_refs_path(refs)))\n+\t\t\t\t refs->packed_ref_store->path))\n \t\tclear_packed_ref_cache(refs);\n }\n \n@@ -415,7 +414,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)\n */\n static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)\n {\n-\tconst char *packed_refs_file = files_packed_refs_path(refs);\n+\tconst char *packed_refs_file = refs->packed_ref_store->path;\n \n \tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tvalidate_packed_ref_cache(refs);\n@@ -1352,7 +1351,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t}\n \n \tif (hold_lock_file_for_update_timeout(\n-\t\t\t &refs->packed_refs_lock, files_packed_refs_path(refs),\n+\t\t\t &refs->packed_refs_lock, refs->packed_ref_store->path,\n \t\t\t flags, timeout_value) < 0)\n \t\treturn -1;\n \n@@ -1633,7 +1632,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \t\treturn 0; /* no refname exists in packed refs */\n \n \tif (lock_packed_refs(refs, 0)) {\n-\t\tunable_to_lock_message(files_packed_refs_path(refs), errno, err);\n+\t\tunable_to_lock_message(refs->packed_ref_store->path, errno, err);\n \t\treturn -1;\n \t}\n \tpacked = get_packed_refs(refs);\n-- \n2.11.0\n\n"},{"id":"323698","messageId":"7f88f3ecbbaccc6d5f517494a1c33c063967b8ba.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 02/30] add_packed_ref(): teach function to overwrite existing refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:40Z","receivedAt":"2017-07-01T18:31:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Teach `add_packed_ref()` to overwrite an existing entry if one already\nexists for the specified `refname`. This means that we can call it\nfrom `files_pack_refs()`, thereby reducing the amount that the latter\nfunction needs to know about the internals of packed-reference\nhandling.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 40 ++++++++++++++++++----------------------\n 1 file changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b040bb3b0a..87cecde231 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -413,15 +413,16 @@ static struct ref_dir *get_packed_refs(struct files_ref_store *refs)\n }\n \n /*\n- * Add a reference to the in-memory packed reference cache. This may\n- * only be called while the packed-refs file is locked (see\n- * lock_packed_refs()). To actually write the packed-refs file, call\n- * commit_packed_refs().\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 lock_packed_refs()). To actually write the packed-refs file,\n+ * call commit_packed_refs().\n */\n static void add_packed_ref(struct files_ref_store *refs,\n \t\t\t const char *refname, const struct object_id *oid)\n {\n-\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n+\tstruct ref_dir *packed_refs;\n+\tstruct ref_entry *packed_entry;\n \n \tif (!is_lock_file_locked(&refs->packed_refs_lock))\n \t\tdie(\"BUG: packed refs not locked\");\n@@ -429,8 +430,17 @@ static void add_packed_ref(struct files_ref_store *refs,\n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n \t\tdie(\"Reference has invalid format: '%s'\", refname);\n \n-\tadd_ref_entry(get_packed_ref_dir(packed_ref_cache),\n-\t\t create_ref_entry(refname, oid, REF_ISPACKED));\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@@ -1526,12 +1536,10 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\tfiles_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,\n \t\t\t \"pack_refs\");\n \tstruct ref_iterator *iter;\n-\tstruct ref_dir *packed_refs;\n \tint ok;\n \tstruct ref_to_prune *refs_to_prune = NULL;\n \n \tlock_packed_refs(refs, LOCK_DIE_ON_ERROR);\n-\tpacked_refs = get_packed_refs(refs);\n \n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n@@ -1540,8 +1548,6 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * in the packed ref cache. If the reference should be\n \t\t * pruned, also add it to refs_to_prune.\n \t\t */\n-\t\tstruct ref_entry *packed_entry;\n-\n \t\tif (!should_pack_ref(iter->refname, iter->oid, iter->flags,\n \t\t\t\t flags))\n \t\t\tcontinue;\n@@ -1552,17 +1558,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * we don't copy the peeled status, because we want it\n \t\t * to be re-peeled.\n \t\t */\n-\t\tpacked_entry = find_ref_entry(packed_refs, iter->refname);\n-\t\tif (packed_entry) {\n-\t\t\t/* Overwrite existing packed entry with info from loose entry */\n-\t\t\tpacked_entry->flag = REF_ISPACKED;\n-\t\t\toidcpy(&packed_entry->u.value.oid, iter->oid);\n-\t\t} else {\n-\t\t\tpacked_entry = create_ref_entry(iter->refname, iter->oid,\n-\t\t\t\t\t\t\tREF_ISPACKED);\n-\t\t\tadd_ref_entry(packed_refs, packed_entry);\n-\t\t}\n-\t\toidclr(&packed_entry->u.value.peeled);\n+\t\tadd_packed_ref(refs, iter->refname, iter->oid);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n \t\tif ((flags & PACK_REFS_PRUNE)) {\n-- \n2.11.0\n\n"},{"id":"323699","messageId":"8f8c54b55ecbdea360263f5820dc54852bb96266.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 06/30] clear_packed_ref_cache(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:44Z","receivedAt":"2017-07-01T18:31:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex de8293493f..2b0db60b2b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -120,15 +120,15 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n \t}\n }\n \n-static void clear_packed_ref_cache(struct files_ref_store *refs)\n+static void clear_packed_ref_cache(struct packed_ref_store *refs)\n {\n-\tif (refs->packed_ref_store->cache) {\n-\t\tstruct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;\n+\tif (refs->cache) {\n+\t\tstruct packed_ref_cache *cache = refs->cache;\n \n-\t\tif (is_lock_file_locked(&refs->packed_ref_store->lock))\n+\t\tif (is_lock_file_locked(&refs->lock))\n \t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n-\t\trefs->packed_ref_store->cache = NULL;\n-\t\trelease_packed_ref_cache(packed_refs);\n+\t\trefs->cache = NULL;\n+\t\trelease_packed_ref_cache(cache);\n \t}\n }\n \n@@ -401,7 +401,7 @@ static void validate_packed_ref_cache(struct files_ref_store *refs)\n \tif (refs->packed_ref_store->cache &&\n \t !stat_validity_check(&refs->packed_ref_store->cache->validity,\n \t\t\t\t refs->packed_ref_store->path))\n-\t\tclear_packed_ref_cache(refs);\n+\t\tclear_packed_ref_cache(refs->packed_ref_store);\n }\n \n /*\n@@ -1435,7 +1435,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \t\tdie(\"BUG: packed-refs not locked\");\n \trollback_lock_file(&refs->packed_ref_store->lock);\n \trelease_packed_ref_cache(packed_ref_cache);\n-\tclear_packed_ref_cache(refs);\n+\tclear_packed_ref_cache(refs->packed_ref_store);\n }\n \n struct ref_to_prune {\n-- \n2.11.0\n\n"},{"id":"323700","messageId":"ffe9dd89adb2b342ea379f2644ed965aa8d89ffb.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 05/30] packed_ref_store: move `packed_refs_lock` member here","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:43Z","receivedAt":"2017-07-01T18:31:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Move the `packed_refs_lock` member from `files_ref_store` to\n`packed_ref_store`, and rename it to `lock` since it's now more\nobvious what it is locking.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c4b8e2f63b..de8293493f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -62,6 +62,12 @@ struct packed_ref_store {\n \t * it might still be current; otherwise, NULL.\n \t */\n \tstruct packed_ref_cache *cache;\n+\n+\t/*\n+\t * Lock used for the \"packed-refs\" file. Note that this (and\n+\t * thus the enclosing `packed_ref_store`) must not be freed.\n+\t */\n+\tstruct lock_file lock;\n };\n \n static struct packed_ref_store *packed_ref_store_create(\n@@ -87,12 +93,6 @@ struct files_ref_store {\n \n \tstruct ref_cache *loose;\n \n-\t/*\n-\t * Lock used for the \"packed-refs\" file. Note that this (and\n-\t * thus the enclosing `files_ref_store`) must not be freed.\n-\t */\n-\tstruct lock_file packed_refs_lock;\n-\n \tstruct packed_ref_store *packed_ref_store;\n };\n \n@@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)\n \tif (refs->packed_ref_store->cache) {\n \t\tstruct packed_ref_cache *packed_refs = refs->packed_ref_store->cache;\n \n-\t\tif (is_lock_file_locked(&refs->packed_refs_lock))\n+\t\tif (is_lock_file_locked(&refs->packed_ref_store->lock))\n \t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n \t\trefs->packed_ref_store->cache = NULL;\n \t\trelease_packed_ref_cache(packed_refs);\n@@ -416,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n {\n \tconst char *packed_refs_file = refs->packed_ref_store->path;\n \n-\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n+\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n \t\tvalidate_packed_ref_cache(refs);\n \n \tif (!refs->packed_ref_store->cache)\n@@ -447,7 +447,7 @@ static void add_packed_ref(struct files_ref_store *refs,\n \tstruct ref_dir *packed_refs;\n \tstruct ref_entry *packed_entry;\n \n-\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n+\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n \t\tdie(\"BUG: packed refs not locked\");\n \n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))\n@@ -1351,7 +1351,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t}\n \n \tif (hold_lock_file_for_update_timeout(\n-\t\t\t &refs->packed_refs_lock, refs->packed_ref_store->path,\n+\t\t\t &refs->packed_ref_store->lock,\n+\t\t\t refs->packed_ref_store->path,\n \t\t\t flags, timeout_value) < 0)\n \t\treturn -1;\n \n@@ -1388,10 +1389,10 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n \n-\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n+\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(&refs->packed_refs_lock, \"w\");\n+\tout = fdopen_lock_file(&refs->packed_ref_store->lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1409,7 +1410,7 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error while iterating over references\");\n \n-\tif (commit_lock_file(&refs->packed_refs_lock)) {\n+\tif (commit_lock_file(&refs->packed_ref_store->lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n@@ -1430,9 +1431,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)\n \n \tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n-\tif (!is_lock_file_locked(&refs->packed_refs_lock))\n+\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(&refs->packed_refs_lock);\n+\trollback_lock_file(&refs->packed_ref_store->lock);\n \trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(refs);\n }\n-- \n2.11.0\n\n"},{"id":"323701","messageId":"e70fdafcf5d2d8b46985c7251ad1509423725a60.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 09/30] get_packed_refs(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:47Z","receivedAt":"2017-07-01T18:31:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b2ef7b3bb9..bc5c0de84e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -427,9 +427,9 @@ static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_ca\n \treturn get_ref_dir(packed_ref_cache->cache->root);\n }\n \n-static struct ref_dir *get_packed_refs(struct files_ref_store *refs)\n+static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n {\n-\treturn get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));\n+\treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n /*\n@@ -450,7 +450,7 @@ static void add_packed_ref(struct files_ref_store *refs,\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_refs = get_packed_refs(refs->packed_ref_store);\n \tpacked_entry = find_ref_entry(packed_refs, refname);\n \tif (packed_entry) {\n \t\t/* Overwrite the existing entry: */\n@@ -592,7 +592,7 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n static struct ref_entry *get_packed_ref(struct files_ref_store *refs,\n \t\t\t\t\tconst char *refname)\n {\n-\treturn find_ref_entry(get_packed_refs(refs), refname);\n+\treturn find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);\n }\n \n /*\n@@ -1633,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \t\tunable_to_lock_message(refs->packed_ref_store->path, errno, err);\n \t\treturn -1;\n \t}\n-\tpacked = get_packed_refs(refs);\n+\tpacked = get_packed_refs(refs->packed_ref_store);\n \n \t/* Remove refnames from the cache */\n \tfor_each_string_list_item(refname, refnames)\n-- \n2.11.0\n\n"},{"id":"323702","messageId":"3b93dc4d38ce71a367ad9c28b7ca55bb4c274eb7.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 07/30] validate_packed_ref_cache(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:45Z","receivedAt":"2017-07-01T18:31:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2b0db60b2b..f061506bf0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -396,12 +396,11 @@ static void files_ref_path(struct files_ref_store *refs,\n * Check that the packed refs cache (if any) still reflects the\n * contents of the file. If not, clear the cache.\n */\n-static void validate_packed_ref_cache(struct files_ref_store *refs)\n+static void validate_packed_ref_cache(struct packed_ref_store *refs)\n {\n-\tif (refs->packed_ref_store->cache &&\n-\t !stat_validity_check(&refs->packed_ref_store->cache->validity,\n-\t\t\t\t refs->packed_ref_store->path))\n-\t\tclear_packed_ref_cache(refs->packed_ref_store);\n+\tif (refs->cache &&\n+\t !stat_validity_check(&refs->cache->validity, refs->path))\n+\t\tclear_packed_ref_cache(refs);\n }\n \n /*\n@@ -417,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref\n \tconst char *packed_refs_file = refs->packed_ref_store->path;\n \n \tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n-\t\tvalidate_packed_ref_cache(refs);\n+\t\tvalidate_packed_ref_cache(refs->packed_ref_store);\n \n \tif (!refs->packed_ref_store->cache)\n \t\trefs->packed_ref_store->cache = read_packed_refs(packed_refs_file);\n@@ -1364,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t * cache is still valid. We've just locked the file, but it\n \t * might have changed the moment *before* we locked it.\n \t */\n-\tvalidate_packed_ref_cache(refs);\n+\tvalidate_packed_ref_cache(refs->packed_ref_store);\n \n \tpacked_ref_cache = get_packed_ref_cache(refs);\n \t/* Increment the reference count to prevent it from being freed: */\n-- \n2.11.0\n\n"},{"id":"323703","messageId":"20d0b63c77d17b864a645d6c02f16b1045cbb062.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 12/30] commit_packed_refs(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:50Z","receivedAt":"2017-07-01T18:31:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0d8f39089f..5d159620f0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1388,21 +1388,21 @@ static int lock_packed_refs(struct packed_ref_store *refs, int flags)\n * lock_packed_refs()). Return zero on success. On errors, set errno\n * and return a nonzero value\n */\n-static int commit_packed_refs(struct files_ref_store *refs)\n+static int commit_packed_refs(struct packed_ref_store *refs)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs->packed_ref_store);\n+\t\tget_packed_ref_cache(refs);\n \tint ok, error = 0;\n \tint save_errno = 0;\n \tFILE *out;\n \tstruct ref_iterator *iter;\n \n-\tfiles_assert_main_repository(refs, \"commit_packed_refs\");\n+\tpacked_assert_main_repository(refs, \"commit_packed_refs\");\n \n-\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n+\tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n \n-\tout = fdopen_lock_file(&refs->packed_ref_store->lock, \"w\");\n+\tout = fdopen_lock_file(&refs->lock, \"w\");\n \tif (!out)\n \t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n \n@@ -1420,7 +1420,7 @@ static int commit_packed_refs(struct files_ref_store *refs)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error while iterating over references\");\n \n-\tif (commit_lock_file(&refs->packed_ref_store->lock)) {\n+\tif (commit_lock_file(&refs->lock)) {\n \t\tsave_errno = errno;\n \t\terror = -1;\n \t}\n@@ -1606,7 +1606,7 @@ 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))\n+\tif (commit_packed_refs(refs->packed_ref_store))\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \n \tprune_refs(refs, refs_to_prune);\n@@ -1662,7 +1662,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \t}\n \n \t/* Write what remains */\n-\tret = commit_packed_refs(refs);\n+\tret = commit_packed_refs(refs->packed_ref_store);\n \tif (ret)\n \t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n \t\t\t strerror(errno));\n@@ -3227,7 +3227,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &update->new_oid);\n \t}\n \n-\tif (commit_packed_refs(refs)) {\n+\tif (commit_packed_refs(refs->packed_ref_store)) {\n \t\tstrbuf_addf(err, \"unable to commit packed-refs file: %s\",\n \t\t\t strerror(errno));\n \t\tret = TRANSACTION_GENERIC_ERROR;\n-- \n2.11.0\n\n"},{"id":"323704","messageId":"34ec9fea5819575908a5e622b0ef79465f234112.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 08/30] get_packed_ref_cache(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:46Z","receivedAt":"2017-07-01T18:31:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 26 ++++++++++++--------------\n 1 file changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f061506bf0..b2ef7b3bb9 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -404,24 +404,22 @@ static void validate_packed_ref_cache(struct packed_ref_store *refs)\n }\n \n /*\n- * Get the packed_ref_cache for the specified files_ref_store,\n+ * Get the packed_ref_cache for the specified packed_ref_store,\n * creating and populating it if it hasn't been read before or if the\n * file has been changed (according to its `validity` field) since it\n * was last read. On the other hand, if we hold the lock, then assume\n * that the file hasn't been changed out from under us, so skip the\n * extra `stat()` call in `stat_validity_check()`.\n */\n-static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)\n+static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *refs)\n {\n-\tconst char *packed_refs_file = refs->packed_ref_store->path;\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tvalidate_packed_ref_cache(refs);\n \n-\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n-\t\tvalidate_packed_ref_cache(refs->packed_ref_store);\n-\n-\tif (!refs->packed_ref_store->cache)\n-\t\trefs->packed_ref_store->cache = read_packed_refs(packed_refs_file);\n+\tif (!refs->cache)\n+\t\trefs->cache = read_packed_refs(refs->path);\n \n-\treturn refs->packed_ref_store->cache;\n+\treturn refs->cache;\n }\n \n static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)\n@@ -431,7 +429,7 @@ static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_ca\n \n static struct ref_dir *get_packed_refs(struct files_ref_store *refs)\n {\n-\treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n+\treturn get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));\n }\n \n /*\n@@ -1151,7 +1149,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \tloose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),\n \t\t\t\t\t prefix, 1);\n \n-\titer->packed_ref_cache = get_packed_ref_cache(refs);\n+\titer->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);\n \tacquire_packed_ref_cache(iter->packed_ref_cache);\n \tpacked_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,\n \t\t\t\t\t prefix, 0);\n@@ -1365,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t */\n \tvalidate_packed_ref_cache(refs->packed_ref_store);\n \n-\tpacked_ref_cache = get_packed_ref_cache(refs);\n+\tpacked_ref_cache = get_packed_ref_cache(refs->packed_ref_store);\n \t/* Increment the reference count to prevent it from being freed: */\n \tacquire_packed_ref_cache(packed_ref_cache);\n \treturn 0;\n@@ -1380,7 +1378,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n static int commit_packed_refs(struct files_ref_store *refs)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs);\n+\t\tget_packed_ref_cache(refs->packed_ref_store);\n \tint ok, error = 0;\n \tint save_errno = 0;\n \tFILE *out;\n@@ -1426,7 +1424,7 @@ static int commit_packed_refs(struct files_ref_store *refs)\n static void rollback_packed_refs(struct files_ref_store *refs)\n {\n \tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs);\n+\t\tget_packed_ref_cache(refs->packed_ref_store);\n \n \tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n \n-- \n2.11.0\n\n"},{"id":"323705","messageId":"7a56a1e4b8dd02d4b92170f705260d832f1673a7.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 10/30] add_packed_ref(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:48Z","receivedAt":"2017-07-01T18:31:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bc5c0de84e..4943207098 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -438,19 +438,19 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n * (see lock_packed_refs()). To actually write the packed-refs file,\n * call commit_packed_refs().\n */\n-static void add_packed_ref(struct files_ref_store *refs,\n+static void add_packed_ref(struct packed_ref_store *refs,\n \t\t\t const char *refname, const struct object_id *oid)\n {\n \tstruct ref_dir *packed_refs;\n \tstruct ref_entry *packed_entry;\n \n-\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\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->packed_ref_store);\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@@ -1579,7 +1579,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \t\t * we don't copy the peeled status, because we want it\n \t\t * to be re-peeled.\n \t\t */\n-\t\tadd_packed_ref(refs, iter->refname, iter->oid);\n+\t\tadd_packed_ref(refs->packed_ref_store, iter->refname, iter->oid);\n \n \t\t/* Schedule the loose reference for pruning if requested. */\n \t\tif ((flags & PACK_REFS_PRUNE)) {\n@@ -3210,7 +3210,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\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, update->refname,\n+\t\t\tadd_packed_ref(refs->packed_ref_store, update->refname,\n \t\t\t\t &update->new_oid);\n \t}\n \n-- \n2.11.0\n\n"},{"id":"323706","messageId":"f459f865a71462d9ddac20c9d01fbb80470c24e7.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 11/30] lock_packed_refs(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:49Z","receivedAt":"2017-07-01T18:31:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 31 ++++++++++++++++++++++---------\n 1 file changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4943207098..0d8f39089f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -80,6 +80,19 @@ static struct packed_ref_store *packed_ref_store_create(\n \treturn refs;\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 * Future: need to be in \"struct repository\"\n * when doing a full libification.\n@@ -1334,13 +1347,13 @@ static void write_packed_entry(FILE *fh, const char *refname,\n * hold_lock_file_for_update(). Return 0 on success. On errors, set\n * errno appropriately and return a nonzero value.\n */\n-static int lock_packed_refs(struct files_ref_store *refs, int flags)\n+static int lock_packed_refs(struct packed_ref_store *refs, int flags)\n {\n \tstatic int timeout_configured = 0;\n \tstatic int timeout_value = 1000;\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tfiles_assert_main_repository(refs, \"lock_packed_refs\");\n+\tpacked_assert_main_repository(refs, \"lock_packed_refs\");\n \n \tif (!timeout_configured) {\n \t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n@@ -1348,8 +1361,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t}\n \n \tif (hold_lock_file_for_update_timeout(\n-\t\t\t &refs->packed_ref_store->lock,\n-\t\t\t refs->packed_ref_store->path,\n+\t\t\t &refs->lock,\n+\t\t\t refs->path,\n \t\t\t flags, timeout_value) < 0)\n \t\treturn -1;\n \n@@ -1361,9 +1374,9 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)\n \t * cache is still valid. We've just locked the file, but it\n \t * might have changed the moment *before* we locked it.\n \t */\n-\tvalidate_packed_ref_cache(refs->packed_ref_store);\n+\tvalidate_packed_ref_cache(refs);\n \n-\tpacked_ref_cache = get_packed_ref_cache(refs->packed_ref_store);\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 \treturn 0;\n@@ -1560,7 +1573,7 @@ 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 \n-\tlock_packed_refs(refs, LOCK_DIE_ON_ERROR);\n+\tlock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);\n \n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n@@ -1629,7 +1642,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (lock_packed_refs(refs, 0)) {\n+\tif (lock_packed_refs(refs->packed_ref_store, 0)) {\n \t\tunable_to_lock_message(refs->packed_ref_store->path, errno, err);\n \t\treturn -1;\n \t}\n@@ -3198,7 +3211,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t}\n \t}\n \n-\tif (lock_packed_refs(refs, 0)) {\n+\tif (lock_packed_refs(refs->packed_ref_store, 0)) {\n \t\tstrbuf_addf(err, \"unable to lock packed-refs file: %s\",\n \t\t\t strerror(errno));\n \t\tret = TRANSACTION_GENERIC_ERROR;\n-- \n2.11.0\n\n"},{"id":"323707","messageId":"6e52008a0556f80e48ab17a6db1cc0d328c4f539.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 16/30] packed_peel_ref(): new function, extracted from `files_peel_ref()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:54Z","receivedAt":"2017-07-01T18:31:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This will later become a method of `packed_ref_store`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c206791b91..185d05e1d6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1013,6 +1013,18 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \treturn ret;\n }\n \n+static int packed_peel_ref(struct packed_ref_store *refs,\n+\t\t\t const char *refname, unsigned char *sha1)\n+{\n+\tstruct ref_entry *r = get_packed_ref(refs, refname);\n+\n+\tif (!r || peel_entry(r, 0))\n+\t\treturn -1;\n+\n+\thashcpy(sha1, r->u.value.peeled.hash);\n+\treturn 0;\n+}\n+\n static int files_peel_ref(struct ref_store *ref_store,\n \t\t\t const char *refname, unsigned char *sha1)\n {\n@@ -1043,17 +1055,9 @@ static int files_peel_ref(struct ref_store *ref_store,\n \t * be expensive and (b) loose references anyway usually do not\n \t * have REF_KNOWS_PEELED.\n \t */\n-\tif (flag & REF_ISPACKED) {\n-\t\tstruct ref_entry *r =\n-\t\t\tget_packed_ref(refs->packed_ref_store, refname);\n-\n-\t\tif (r) {\n-\t\t\tif (peel_entry(r, 0))\n-\t\t\t\treturn -1;\n-\t\t\thashcpy(sha1, r->u.value.peeled.hash);\n-\t\t\treturn 0;\n-\t\t}\n-\t}\n+\tif (flag & REF_ISPACKED &&\n+\t !packed_peel_ref(refs->packed_ref_store, refname, sha1))\n+\t\treturn 0;\n \n \treturn peel_object(base, sha1);\n }\n-- \n2.11.0\n\n"},{"id":"323708","messageId":"bd8b6c8658c7f4a5a0739cc7900843d6b73164d0.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 15/30] repack_without_refs(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:53Z","receivedAt":"2017-07-01T18:32:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2b9d93d3b6..c206791b91 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1621,19 +1621,19 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n *\n * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n */\n-static int repack_without_refs(struct files_ref_store *refs,\n+static int repack_without_refs(struct packed_ref_store *refs,\n \t\t\t struct string_list *refnames, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list_item *refname;\n \tint ret, needs_repacking = 0, removed = 0;\n \n-\tfiles_assert_main_repository(refs, \"repack_without_refs\");\n+\tpacked_assert_main_repository(refs, \"repack_without_refs\");\n \tassert(err);\n \n \t/* Look for a packed ref */\n \tfor_each_string_list_item(refname, refnames) {\n-\t\tif (get_packed_ref(refs->packed_ref_store, refname->string)) {\n+\t\tif (get_packed_ref(refs, refname->string)) {\n \t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n \t\t}\n@@ -1643,11 +1643,11 @@ static int repack_without_refs(struct files_ref_store *refs,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (lock_packed_refs(refs->packed_ref_store, 0)) {\n-\t\tunable_to_lock_message(refs->packed_ref_store->path, errno, err);\n+\tif (lock_packed_refs(refs, 0)) {\n+\t\tunable_to_lock_message(refs->path, errno, err);\n \t\treturn -1;\n \t}\n-\tpacked = get_packed_refs(refs->packed_ref_store);\n+\tpacked = get_packed_refs(refs);\n \n \t/* Remove refnames from the cache */\n \tfor_each_string_list_item(refname, refnames)\n@@ -1658,12 +1658,12 @@ static int repack_without_refs(struct files_ref_store *refs,\n \t\t * All packed entries disappeared while we were\n \t\t * acquiring the lock.\n \t\t */\n-\t\trollback_packed_refs(refs->packed_ref_store);\n+\t\trollback_packed_refs(refs);\n \t\treturn 0;\n \t}\n \n \t/* Write what remains */\n-\tret = commit_packed_refs(refs->packed_ref_store);\n+\tret = commit_packed_refs(refs);\n \tif (ret)\n \t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n \t\t\t strerror(errno));\n@@ -1681,7 +1681,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!refnames->nr)\n \t\treturn 0;\n \n-\tresult = repack_without_refs(refs, refnames, &err);\n+\tresult = repack_without_refs(refs->packed_ref_store, refnames, &err);\n \tif (result) {\n \t\t/*\n \t\t * If we failed to rewrite the packed-refs file, then\n@@ -3101,7 +3101,7 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t}\n \t}\n \n-\tif (repack_without_refs(refs, &refs_to_delete, err)) {\n+\tif (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-- \n2.11.0\n\n"},{"id":"323709","messageId":"8519a233a8055b510b950da7763c2dcfdb31fa54.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 14/30] get_packed_ref(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:52Z","receivedAt":"2017-07-01T18:32:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a08d3fbadf..2b9d93d3b6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -602,10 +602,10 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n * Return the ref_entry for the given refname from the packed\n * references. If it does not exist, return NULL.\n */\n-static struct ref_entry *get_packed_ref(struct files_ref_store *refs,\n+static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n \t\t\t\t\tconst char *refname)\n {\n-\treturn find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);\n+\treturn find_ref_entry(get_packed_refs(refs), refname);\n }\n \n /*\n@@ -621,7 +621,7 @@ static int resolve_packed_ref(struct files_ref_store *refs,\n \t * The loose reference file does not exist; check for a packed\n \t * reference.\n \t */\n-\tentry = get_packed_ref(refs, refname);\n+\tentry = get_packed_ref(refs->packed_ref_store, refname);\n \tif (entry) {\n \t\thashcpy(sha1, entry->u.value.oid.hash);\n \t\t*flags |= REF_ISPACKED;\n@@ -1044,7 +1044,9 @@ static int files_peel_ref(struct ref_store *ref_store,\n \t * have REF_KNOWS_PEELED.\n \t */\n \tif (flag & REF_ISPACKED) {\n-\t\tstruct ref_entry *r = get_packed_ref(refs, refname);\n+\t\tstruct ref_entry *r =\n+\t\t\tget_packed_ref(refs->packed_ref_store, refname);\n+\n \t\tif (r) {\n \t\t\tif (peel_entry(r, 0))\n \t\t\t\treturn -1;\n@@ -1631,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store *refs,\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\tif (get_packed_ref(refs->packed_ref_store, refname->string)) {\n \t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n \t\t}\n-- \n2.11.0\n\n"},{"id":"323710","messageId":"34acfb1986a79d847ed1a978e495b91ac4bb2273.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 17/30] packed_ref_store: support iteration","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:55Z","receivedAt":"2017-07-01T18:32:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add the infrastructure to iterate over a `packed_ref_store`. It's a\nlot of boilerplate, but it's all part of a campaign to make\n`packed_ref_store` implement `ref_store`. In the future, this iterator\nwill work much differently.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 110 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 185d05e1d6..0490cc087e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1062,10 +1062,102 @@ static int files_peel_ref(struct ref_store *ref_store,\n \treturn peel_object(base, sha1);\n }\n \n+struct packed_ref_iterator {\n+\tstruct ref_iterator base;\n+\n+\tstruct packed_ref_cache *cache;\n+\tstruct ref_iterator *iter0;\n+\tunsigned int flags;\n+};\n+\n+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\tint ok;\n+\n+\twhile ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {\n+\t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n+\t\t ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n+\t\t\tcontinue;\n+\n+\t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n+\t\t !ref_resolves_to_object(iter->iter0->refname,\n+\t\t\t\t\t iter->iter0->oid,\n+\t\t\t\t\t iter->iter0->flags))\n+\t\t\tcontinue;\n+\n+\t\titer->base.refname = iter->iter0->refname;\n+\t\titer->base.oid = iter->iter0->oid;\n+\t\titer->base.flags = iter->iter0->flags;\n+\t\treturn ITER_OK;\n+\t}\n+\n+\titer->iter0 = NULL;\n+\tif (ref_iterator_abort(ref_iterator) != ITER_DONE)\n+\t\tok = ITER_ERROR;\n+\n+\treturn ok;\n+}\n+\n+static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t\t\t struct object_id *peeled)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\n+\treturn ref_iterator_peel(iter->iter0, peeled);\n+}\n+\n+static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\tint ok = ITER_DONE;\n+\n+\tif (iter->iter0)\n+\t\tok = ref_iterator_abort(iter->iter0);\n+\n+\trelease_packed_ref_cache(iter->cache);\n+\tbase_ref_iterator_free(ref_iterator);\n+\treturn ok;\n+}\n+\n+static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n+\tpacked_ref_iterator_advance,\n+\tpacked_ref_iterator_peel,\n+\tpacked_ref_iterator_abort\n+};\n+\n+static struct ref_iterator *packed_ref_iterator_begin(\n+\t\tstruct packed_ref_store *refs,\n+\t\tconst char *prefix, unsigned int flags)\n+{\n+\tstruct packed_ref_iterator *iter;\n+\tstruct ref_iterator *ref_iterator;\n+\n+\titer = xcalloc(1, sizeof(*iter));\n+\tref_iterator = &iter->base;\n+\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n+\n+\t/*\n+\t * Note that get_packed_ref_cache() internally checks whether\n+\t * the packed-ref cache is up to date with what is on disk,\n+\t * and re-reads it if not.\n+\t */\n+\n+\titer->cache = get_packed_ref_cache(refs);\n+\tacquire_packed_ref_cache(iter->cache);\n+\titer->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);\n+\n+\titer->flags = flags;\n+\n+\treturn ref_iterator;\n+}\n+\n struct files_ref_iterator {\n \tstruct ref_iterator base;\n \n-\tstruct packed_ref_cache *packed_ref_cache;\n \tstruct ref_iterator *iter0;\n \tunsigned int flags;\n };\n@@ -1118,7 +1210,6 @@ static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \tif (iter->iter0)\n \t\tok = ref_iterator_abort(iter->iter0);\n \n-\trelease_packed_ref_cache(iter->packed_ref_cache);\n \tbase_ref_iterator_free(ref_iterator);\n \treturn ok;\n }\n@@ -1160,18 +1251,28 @@ static struct ref_iterator *files_ref_iterator_begin(\n \t * (If they've already been read, that's OK; we only need to\n \t * guarantee that they're read before the packed refs, not\n \t * *how much* before.) After that, we call\n-\t * get_packed_ref_cache(), which internally checks whether the\n-\t * packed-ref cache is up to date with what is on disk, and\n-\t * re-reads it if not.\n+\t * packed_ref_iterator_begin(), which internally checks\n+\t * whether the packed-ref cache is up to date with what is on\n+\t * disk, and re-reads it if not.\n \t */\n \n \tloose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),\n \t\t\t\t\t prefix, 1);\n \n-\titer->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);\n-\tacquire_packed_ref_cache(iter->packed_ref_cache);\n-\tpacked_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,\n-\t\t\t\t\t prefix, 0);\n+\t/*\n+\t * The packed-refs file might contain broken references, for\n+\t * example an old version of a reference that points at an\n+\t * object that has since been garbage-collected. This is OK as\n+\t * long as there is a corresponding loose reference that\n+\t * overrides it, and we don't want to emit an error message in\n+\t * this case. So ask the packed_ref_store for all of its\n+\t * references, and (if needed) do our own check for broken\n+\t * ones in files_ref_iterator_advance(), after we have merged\n+\t * the packed and loose references.\n+\t */\n+\tpacked_iter = packed_ref_iterator_begin(\n+\t\t\trefs->packed_ref_store, prefix,\n+\t\t\tDO_FOR_EACH_INCLUDE_BROKEN);\n \n \titer->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);\n \titer->flags = flags;\n-- \n2.11.0\n\n"},{"id":"323711","messageId":"eda8defbbdd76562350a9b115d01c6dcf2cb85d0.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 19/30] packed-backend: new module for handling packed references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:57Z","receivedAt":"2017-07-01T18:32:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that the interface between `files_ref_store` and\n`packed_ref_store` is relatively narrow, move the latter into a new\nmodule, \"refs/packed-backend.h\" and \"refs/packed-backend.c\". It still\ndoesn't quite implement the `ref_store` interface, but it will soon.\n\nThis commit moves code around and adjusts its visibility, but doesn't\nchange anything.\n\nSigned-off-by: Michael Haggerty \n---\n Makefile | 1 +\n refs.c | 18 ++\n refs/files-backend.c | 640 +-------------------------------------------------\n refs/packed-backend.c | 624 ++++++++++++++++++++++++++++++++++++++++++++++++\n refs/packed-backend.h | 33 +++\n refs/refs-internal.h | 9 +\n 6 files changed, 686 insertions(+), 639 deletions(-)\n create mode 100644 refs/packed-backend.c\n create mode 100644 refs/packed-backend.h\n\ndiff --git a/Makefile b/Makefile\nindex e35542e631..aff9c6d7e2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -817,6 +817,7 @@ LIB_OBJS += reflog-walk.o\n LIB_OBJS += refs.o\n LIB_OBJS += refs/files-backend.o\n LIB_OBJS += refs/iterator.o\n+LIB_OBJS += refs/packed-backend.o\n LIB_OBJS += refs/ref-cache.o\n LIB_OBJS += ref-filter.o\n LIB_OBJS += remote.o\ndiff --git a/refs.c b/refs.c\nindex 32177969f0..5880c12372 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -173,6 +173,24 @@ int refname_is_safe(const char *refname)\n \treturn 1;\n }\n \n+/*\n+ * Return true if refname, which has the specified oid and flags, can\n+ * be resolved to an object in the database. If the referred-to object\n+ * does not exist, emit a warning and return false.\n+ */\n+int ref_resolves_to_object(const char *refname,\n+\t\t\t const struct object_id *oid,\n+\t\t\t unsigned int flags)\n+{\n+\tif (flags & REF_ISBROKEN)\n+\t\treturn 0;\n+\tif (!has_sha1_file(oid->hash)) {\n+\t\terror(\"%s does not point to a valid object!\", refname);\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n char *refs_resolve_refdup(struct ref_store *refs,\n \t\t\t const char *refname, int resolve_flags,\n \t\t\t unsigned char *sha1, int *flags)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 346794cf7c..7df9747798 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2,6 +2,7 @@\n #include \"../refs.h\"\n #include \"refs-internal.h\"\n #include \"ref-cache.h\"\n+#include \"packed-backend.h\"\n #include \"../iterator.h\"\n #include \"../dir-iterator.h\"\n #include \"../lockfile.h\"\n@@ -14,85 +15,6 @@ struct ref_lock {\n \tstruct object_id old_oid;\n };\n \n-/*\n- * Return true if refname, which has the specified oid and flags, can\n- * be resolved to an object in the database. If the referred-to object\n- * does not exist, emit a warning and return false.\n- */\n-static int ref_resolves_to_object(const char *refname,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t unsigned int flags)\n-{\n-\tif (flags & REF_ISBROKEN)\n-\t\treturn 0;\n-\tif (!has_sha1_file(oid->hash)) {\n-\t\terror(\"%s does not point to a valid object!\", refname);\n-\t\treturn 0;\n-\t}\n-\treturn 1;\n-}\n-\n-struct packed_ref_cache {\n-\tstruct ref_cache *cache;\n-\n-\t/*\n-\t * Count of references to the data structure in this instance,\n-\t * including the pointer from files_ref_store::packed if any.\n-\t * The data will not be freed as long as the reference count\n-\t * is nonzero.\n-\t */\n-\tunsigned int referrers;\n-\n-\t/* The metadata from when this packed-refs cache was read */\n-\tstruct stat_validity validity;\n-};\n-\n-/*\n- * A container for `packed-refs`-related data. It is not (yet) a\n- * `ref_store`.\n- */\n-struct packed_ref_store {\n-\tunsigned int store_flags;\n-\n-\t/* The path of the \"packed-refs\" file: */\n-\tchar *path;\n-\n-\t/*\n-\t * A cache of the values read from the `packed-refs` file, if\n-\t * it might still be current; otherwise, NULL.\n-\t */\n-\tstruct packed_ref_cache *cache;\n-\n-\t/*\n-\t * Lock used for the \"packed-refs\" file. Note that this (and\n-\t * thus the enclosing `packed_ref_store`) must not be freed.\n-\t */\n-\tstruct lock_file lock;\n-};\n-\n-static struct packed_ref_store *packed_ref_store_create(\n-\t\tconst char *path, unsigned int store_flags)\n-{\n-\tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n-\n-\trefs->store_flags = store_flags;\n-\trefs->path = xstrdup(path);\n-\treturn refs;\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 * Future: need to be in \"struct repository\"\n * when doing a full libification.\n@@ -109,42 +31,6 @@ struct files_ref_store {\n \tstruct packed_ref_store *packed_ref_store;\n };\n \n-/*\n- * Increment the reference count of *packed_refs.\n- */\n-static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n-{\n-\tpacked_refs->referrers++;\n-}\n-\n-/*\n- * Decrease the reference count of *packed_refs. If it goes to zero,\n- * free *packed_refs and return true; otherwise return false.\n- */\n-static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n-{\n-\tif (!--packed_refs->referrers) {\n-\t\tfree_ref_cache(packed_refs->cache);\n-\t\tstat_validity_clear(&packed_refs->validity);\n-\t\tfree(packed_refs);\n-\t\treturn 1;\n-\t} else {\n-\t\treturn 0;\n-\t}\n-}\n-\n-static void clear_packed_ref_cache(struct packed_ref_store *refs)\n-{\n-\tif (refs->cache) {\n-\t\tstruct packed_ref_cache *cache = refs->cache;\n-\n-\t\tif (is_lock_file_locked(&refs->lock))\n-\t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n-\t\trefs->cache = NULL;\n-\t\trelease_packed_ref_cache(cache);\n-\t}\n-}\n-\n static void clear_loose_ref_cache(struct files_ref_store *refs)\n {\n \tif (refs->loose) {\n@@ -215,151 +101,6 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store,\n \treturn refs;\n }\n \n-/* The length of a peeled reference line in packed-refs, including EOL: */\n-#define PEELED_LINE_LENGTH 42\n-\n-/*\n- * The packed-refs header line that we write out. Perhaps other\n- * traits will be added later. The trailing space is required.\n- */\n-static const char PACKED_REFS_HEADER[] =\n-\t\"# pack-refs with: peeled fully-peeled \\n\";\n-\n-/*\n- * Parse one line from a packed-refs file. Write the SHA1 to sha1.\n- * Return a pointer to the refname within the line (null-terminated),\n- * or NULL if there was a problem.\n- */\n-static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n-{\n-\tconst char *ref;\n-\n-\tif (parse_oid_hex(line->buf, oid, &ref) < 0)\n-\t\treturn NULL;\n-\tif (!isspace(*ref++))\n-\t\treturn NULL;\n-\n-\tif (isspace(*ref))\n-\t\treturn NULL;\n-\n-\tif (line->buf[line->len - 1] != '\\n')\n-\t\treturn NULL;\n-\tline->buf[--line->len] = 0;\n-\n-\treturn ref;\n-}\n-\n-/*\n- * Read from `packed_refs_file` into a newly-allocated\n- * `packed_ref_cache` and return it. The return value will already\n- * have its reference count incremented.\n- *\n- * A comment line of the form \"# pack-refs with: \" may contain zero or\n- * more traits. We interpret the traits as follows:\n- *\n- * No traits:\n- *\n- * Probably no references are peeled. But if the file contains a\n- * peeled value for a reference, we will use it.\n- *\n- * peeled:\n- *\n- * References under \"refs/tags/\", if they *can* be peeled, *are*\n- * peeled in this file. References outside of \"refs/tags/\" are\n- * probably not peeled even if they could have been, but if we find\n- * a peeled value for such a reference we will use it.\n- *\n- * fully-peeled:\n- *\n- * All references in the file that can be peeled are peeled.\n- * Inversely (and this is more important), any references in the\n- * file for which no peeled value is recorded is not peelable. This\n- * trait should typically be written alongside \"peeled\" for\n- * compatibility with older clients, but we do not require it\n- * (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n- */\n-static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n-{\n-\tFILE *f;\n-\tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n-\tstruct ref_entry *last = NULL;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n-\tstruct ref_dir *dir;\n-\n-\tacquire_packed_ref_cache(packed_refs);\n-\tpacked_refs->cache = create_ref_cache(NULL, NULL);\n-\tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n-\n-\tf = fopen(packed_refs_file, \"r\");\n-\tif (!f) {\n-\t\tif (errno == ENOENT) {\n-\t\t\t/*\n-\t\t\t * This is OK; it just means that no\n-\t\t\t * \"packed-refs\" file has been written yet,\n-\t\t\t * which is equivalent to it being empty.\n-\t\t\t */\n-\t\t\treturn packed_refs;\n-\t\t} else {\n-\t\t\tdie_errno(\"couldn't read %s\", packed_refs_file);\n-\t\t}\n-\t}\n-\n-\tstat_validity_update(&packed_refs->validity, fileno(f));\n-\n-\tdir = get_ref_dir(packed_refs->cache->root);\n-\twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n-\t\tstruct object_id oid;\n-\t\tconst char *refname;\n-\t\tconst char *traits;\n-\n-\t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n-\t\t\tif (strstr(traits, \" fully-peeled \"))\n-\t\t\t\tpeeled = PEELED_FULLY;\n-\t\t\telse if (strstr(traits, \" peeled \"))\n-\t\t\t\tpeeled = PEELED_TAGS;\n-\t\t\t/* perhaps other traits later as well */\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\trefname = parse_ref_line(&line, &oid);\n-\t\tif (refname) {\n-\t\t\tint flag = REF_ISPACKED;\n-\n-\t\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\t\tif (!refname_is_safe(refname))\n-\t\t\t\t\tdie(\"packed refname is dangerous: %s\", refname);\n-\t\t\t\toidclr(&oid);\n-\t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n-\t\t\t}\n-\t\t\tlast = create_ref_entry(refname, &oid, flag);\n-\t\t\tif (peeled == PEELED_FULLY ||\n-\t\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n-\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n-\t\t\tadd_ref_entry(dir, last);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (last &&\n-\t\t line.buf[0] == '^' &&\n-\t\t line.len == PEELED_LINE_LENGTH &&\n-\t\t line.buf[PEELED_LINE_LENGTH - 1] == '\\n' &&\n-\t\t !get_oid_hex(line.buf + 1, &oid)) {\n-\t\t\toidcpy(&last->u.value.peeled, &oid);\n-\t\t\t/*\n-\t\t\t * Regardless of what the file header said,\n-\t\t\t * we definitely know the value of *this*\n-\t\t\t * reference:\n-\t\t\t */\n-\t\t\tlast->flag |= REF_KNOWS_PEELED;\n-\t\t}\n-\t}\n-\n-\tfclose(f);\n-\tstrbuf_release(&line);\n-\n-\treturn packed_refs;\n-}\n-\n static void files_reflog_path(struct files_ref_store *refs,\n \t\t\t struct strbuf *sb,\n \t\t\t const char *refname)\n@@ -405,77 +146,6 @@ static void files_ref_path(struct files_ref_store *refs,\n \t}\n }\n \n-/*\n- * Check that the packed refs cache (if any) still reflects the\n- * contents of the file. If not, clear the cache.\n- */\n-static void validate_packed_ref_cache(struct packed_ref_store *refs)\n-{\n-\tif (refs->cache &&\n-\t !stat_validity_check(&refs->cache->validity, refs->path))\n-\t\tclear_packed_ref_cache(refs);\n-}\n-\n-/*\n- * Get the packed_ref_cache for the specified packed_ref_store,\n- * creating and populating it if it hasn't been read before or if the\n- * file has been changed (according to its `validity` field) since it\n- * was last read. On the other hand, if we hold the lock, then assume\n- * that the file hasn't been changed out from under us, so skip the\n- * extra `stat()` call in `stat_validity_check()`.\n- */\n-static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *refs)\n-{\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tvalidate_packed_ref_cache(refs);\n-\n-\tif (!refs->cache)\n-\t\trefs->cache = read_packed_refs(refs->path);\n-\n-\treturn refs->cache;\n-}\n-\n-static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)\n-{\n-\treturn get_ref_dir(packed_ref_cache->cache->root);\n-}\n-\n-static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n-{\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 lock_packed_refs()). To actually write the packed-refs file,\n- * call commit_packed_refs().\n- */\n-static void add_packed_ref(struct packed_ref_store *refs,\n-\t\t\t const char *refname, const struct object_id *oid)\n-{\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 * Read the loose references from the namespace dirname into dir\n * (without recursing). dirname must end with '/'. dir must be the\n@@ -598,35 +268,6 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n \treturn refs->loose;\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- */\n-static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n-\t\t\t\t\tconst char *refname)\n-{\n-\treturn find_ref_entry(get_packed_refs(refs), refname);\n-}\n-\n-static int packed_read_raw_ref(struct packed_ref_store *refs,\n-\t\t\t const char *refname, unsigned char *sha1,\n-\t\t\t struct strbuf *referent, unsigned int *type)\n-{\n-\tstruct ref_entry *entry;\n-\n-\t*type = 0;\n-\n-\tentry = get_packed_ref(refs, refname);\n-\tif (!entry) {\n-\t\terrno = ENOENT;\n-\t\treturn -1;\n-\t}\n-\n-\thashcpy(sha1, entry->u.value.oid.hash);\n-\t*type = REF_ISPACKED;\n-\treturn 0;\n-}\n-\n static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t\t const char *refname, unsigned char *sha1,\n \t\t\t struct strbuf *referent, unsigned int *type)\n@@ -1011,18 +652,6 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \treturn ret;\n }\n \n-static int packed_peel_ref(struct packed_ref_store *refs,\n-\t\t\t const char *refname, unsigned char *sha1)\n-{\n-\tstruct ref_entry *r = get_packed_ref(refs, refname);\n-\n-\tif (!r || peel_entry(r, 0))\n-\t\treturn -1;\n-\n-\thashcpy(sha1, r->u.value.peeled.hash);\n-\treturn 0;\n-}\n-\n static int files_peel_ref(struct ref_store *ref_store,\n \t\t\t const char *refname, unsigned char *sha1)\n {\n@@ -1060,99 +689,6 @@ static int files_peel_ref(struct ref_store *ref_store,\n \treturn peel_object(base, sha1);\n }\n \n-struct packed_ref_iterator {\n-\tstruct ref_iterator base;\n-\n-\tstruct packed_ref_cache *cache;\n-\tstruct ref_iterator *iter0;\n-\tunsigned int flags;\n-};\n-\n-static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n-{\n-\tstruct packed_ref_iterator *iter =\n-\t\t(struct packed_ref_iterator *)ref_iterator;\n-\tint ok;\n-\n-\twhile ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {\n-\t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n-\t\t ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n-\t\t\tcontinue;\n-\n-\t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n-\t\t !ref_resolves_to_object(iter->iter0->refname,\n-\t\t\t\t\t iter->iter0->oid,\n-\t\t\t\t\t iter->iter0->flags))\n-\t\t\tcontinue;\n-\n-\t\titer->base.refname = iter->iter0->refname;\n-\t\titer->base.oid = iter->iter0->oid;\n-\t\titer->base.flags = iter->iter0->flags;\n-\t\treturn ITER_OK;\n-\t}\n-\n-\titer->iter0 = NULL;\n-\tif (ref_iterator_abort(ref_iterator) != ITER_DONE)\n-\t\tok = ITER_ERROR;\n-\n-\treturn ok;\n-}\n-\n-static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t struct object_id *peeled)\n-{\n-\tstruct packed_ref_iterator *iter =\n-\t\t(struct packed_ref_iterator *)ref_iterator;\n-\n-\treturn ref_iterator_peel(iter->iter0, peeled);\n-}\n-\n-static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n-{\n-\tstruct packed_ref_iterator *iter =\n-\t\t(struct packed_ref_iterator *)ref_iterator;\n-\tint ok = ITER_DONE;\n-\n-\tif (iter->iter0)\n-\t\tok = ref_iterator_abort(iter->iter0);\n-\n-\trelease_packed_ref_cache(iter->cache);\n-\tbase_ref_iterator_free(ref_iterator);\n-\treturn ok;\n-}\n-\n-static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n-\tpacked_ref_iterator_advance,\n-\tpacked_ref_iterator_peel,\n-\tpacked_ref_iterator_abort\n-};\n-\n-static struct ref_iterator *packed_ref_iterator_begin(\n-\t\tstruct packed_ref_store *refs,\n-\t\tconst char *prefix, unsigned int flags)\n-{\n-\tstruct packed_ref_iterator *iter;\n-\tstruct ref_iterator *ref_iterator;\n-\n-\titer = xcalloc(1, sizeof(*iter));\n-\tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n-\n-\t/*\n-\t * Note that get_packed_ref_cache() internally checks whether\n-\t * the packed-ref cache is up to date with what is on disk,\n-\t * and re-reads it if not.\n-\t */\n-\n-\titer->cache = get_packed_ref_cache(refs);\n-\tacquire_packed_ref_cache(iter->cache);\n-\titer->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);\n-\n-\titer->flags = flags;\n-\n-\treturn ref_iterator;\n-}\n-\n struct files_ref_iterator {\n \tstruct ref_iterator base;\n \n@@ -1434,124 +970,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \treturn lock;\n }\n \n-/*\n- * Write an entry to the packed-refs file for the specified refname.\n- * If peeled is non-NULL, write it as the entry's peeled value.\n- */\n-static void write_packed_entry(FILE *fh, const char *refname,\n-\t\t\t const unsigned char *sha1,\n-\t\t\t const unsigned char *peeled)\n-{\n-\tfprintf_or_die(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n-\tif (peeled)\n-\t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n-}\n-\n-/*\n- * Lock the packed-refs file for writing. Flags is passed to\n- * hold_lock_file_for_update(). Return 0 on success. On errors, set\n- * errno appropriately and return a nonzero value.\n- */\n-static int lock_packed_refs(struct packed_ref_store *refs, int flags)\n-{\n-\tstatic int timeout_configured = 0;\n-\tstatic int timeout_value = 1000;\n-\tstruct packed_ref_cache *packed_ref_cache;\n-\n-\tpacked_assert_main_repository(refs, \"lock_packed_refs\");\n-\n-\tif (!timeout_configured) {\n-\t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n-\t\ttimeout_configured = 1;\n-\t}\n-\n-\tif (hold_lock_file_for_update_timeout(\n-\t\t\t &refs->lock,\n-\t\t\t refs->path,\n-\t\t\t flags, timeout_value) < 0)\n-\t\treturn -1;\n-\n-\t/*\n-\t * Now that we hold the `packed-refs` lock, make sure that our\n-\t * cache matches the current version of the file. Normally\n-\t * `get_packed_ref_cache()` does that for us, but that\n-\t * function assumes that when the file is locked, any existing\n-\t * cache is still valid. We've just locked the file, but it\n-\t * might have changed the moment *before* we locked it.\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-\treturn 0;\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- * lock_packed_refs()). Return zero on success. On errors, set errno\n- * and return a nonzero value\n- */\n-static int commit_packed_refs(struct packed_ref_store *refs)\n-{\n-\tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs);\n-\tint ok, error = 0;\n-\tint save_errno = 0;\n-\tFILE *out;\n-\tstruct ref_iterator *iter;\n-\n-\tpacked_assert_main_repository(refs, \"commit_packed_refs\");\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: packed-refs not locked\");\n-\n-\tout = fdopen_lock_file(&refs->lock, \"w\");\n-\tif (!out)\n-\t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n-\n-\tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\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\twrite_packed_entry(out, iter->refname, iter->oid->hash,\n-\t\t\t\t peel_error ? NULL : peeled.hash);\n-\t}\n-\n-\tif (ok != ITER_DONE)\n-\t\tdie(\"error while iterating over references\");\n-\n-\tif (commit_lock_file(&refs->lock)) {\n-\t\tsave_errno = errno;\n-\t\terror = -1;\n-\t}\n-\trelease_packed_ref_cache(packed_ref_cache);\n-\terrno = save_errno;\n-\treturn error;\n-}\n-\n-/*\n- * Rollback the lockfile for the packed-refs file, and discard the\n- * in-memory packed reference cache. (The packed-refs file will be\n- * read anew if it is needed again after this function is called.)\n- */\n-static void rollback_packed_refs(struct packed_ref_store *refs)\n-{\n-\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n-\n-\tpacked_assert_main_repository(refs, \"rollback_packed_refs\");\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(&refs->lock);\n-\trelease_packed_ref_cache(packed_ref_cache);\n-\tclear_packed_ref_cache(refs);\n-}\n-\n struct ref_to_prune {\n \tstruct ref_to_prune *next;\n \tunsigned char sha1[20];\n@@ -1717,62 +1135,6 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \treturn 0;\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.\n- *\n- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n- */\n-static int repack_without_refs(struct packed_ref_store *refs,\n-\t\t\t struct string_list *refnames, struct strbuf *err)\n-{\n-\tstruct ref_dir *packed;\n-\tstruct string_list_item *refname;\n-\tint ret, needs_repacking = 0, removed = 0;\n-\n-\tpacked_assert_main_repository(refs, \"repack_without_refs\");\n-\tassert(err);\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-\tif (lock_packed_refs(refs, 0)) {\n-\t\tunable_to_lock_message(refs->path, errno, err);\n-\t\treturn -1;\n-\t}\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\trollback_packed_refs(refs);\n-\t\treturn 0;\n-\t}\n-\n-\t/* Write what remains */\n-\tret = commit_packed_refs(refs);\n-\tif (ret)\n-\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n-\t\t\t strerror(errno));\n-\treturn ret;\n-}\n-\n static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nnew file mode 100644\nindex 0000000000..6fa988c953\n--- /dev/null\n+++ b/refs/packed-backend.c\n@@ -0,0 +1,624 @@\n+#include \"../cache.h\"\n+#include \"../refs.h\"\n+#include \"refs-internal.h\"\n+#include \"ref-cache.h\"\n+#include \"packed-backend.h\"\n+#include \"../iterator.h\"\n+#include \"../lockfile.h\"\n+\n+struct packed_ref_cache {\n+\tstruct ref_cache *cache;\n+\n+\t/*\n+\t * Count of references to the data structure in this instance,\n+\t * including the pointer from files_ref_store::packed if any.\n+\t * The data will not be freed as long as the reference count\n+\t * is nonzero.\n+\t */\n+\tunsigned int referrers;\n+\n+\t/* The metadata from when this packed-refs cache was read */\n+\tstruct stat_validity validity;\n+};\n+\n+/*\n+ * Increment the reference count of *packed_refs.\n+ */\n+static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+{\n+\tpacked_refs->referrers++;\n+}\n+\n+/*\n+ * Decrease the reference count of *packed_refs. If it goes to zero,\n+ * free *packed_refs and return true; otherwise return false.\n+ */\n+static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+{\n+\tif (!--packed_refs->referrers) {\n+\t\tfree_ref_cache(packed_refs->cache);\n+\t\tstat_validity_clear(&packed_refs->validity);\n+\t\tfree(packed_refs);\n+\t\treturn 1;\n+\t} else {\n+\t\treturn 0;\n+\t}\n+}\n+\n+/*\n+ * A container for `packed-refs`-related data. It is not (yet) a\n+ * `ref_store`.\n+ */\n+struct packed_ref_store {\n+\tunsigned int store_flags;\n+\n+\t/* The path of the \"packed-refs\" file: */\n+\tchar *path;\n+\n+\t/*\n+\t * A cache of the values read from the `packed-refs` file, if\n+\t * it might still be current; otherwise, NULL.\n+\t */\n+\tstruct packed_ref_cache *cache;\n+\n+\t/*\n+\t * Lock used for the \"packed-refs\" file. Note that this (and\n+\t * thus the enclosing `packed_ref_store`) must not be freed.\n+\t */\n+\tstruct lock_file lock;\n+};\n+\n+struct packed_ref_store *packed_ref_store_create(\n+\t\tconst char *path, unsigned int store_flags)\n+{\n+\tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n+\n+\trefs->store_flags = store_flags;\n+\trefs->path = xstrdup(path);\n+\treturn refs;\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+static void clear_packed_ref_cache(struct packed_ref_store *refs)\n+{\n+\tif (refs->cache) {\n+\t\tstruct packed_ref_cache *cache = refs->cache;\n+\n+\t\tif (is_lock_file_locked(&refs->lock))\n+\t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n+\t\trefs->cache = NULL;\n+\t\trelease_packed_ref_cache(cache);\n+\t}\n+}\n+\n+/* The length of a peeled reference line in packed-refs, including EOL: */\n+#define PEELED_LINE_LENGTH 42\n+\n+/*\n+ * Parse one line from a packed-refs file. Write the SHA1 to sha1.\n+ * Return a pointer to the refname within the line (null-terminated),\n+ * or NULL if there was a problem.\n+ */\n+static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n+{\n+\tconst char *ref;\n+\n+\tif (parse_oid_hex(line->buf, oid, &ref) < 0)\n+\t\treturn NULL;\n+\tif (!isspace(*ref++))\n+\t\treturn NULL;\n+\n+\tif (isspace(*ref))\n+\t\treturn NULL;\n+\n+\tif (line->buf[line->len - 1] != '\\n')\n+\t\treturn NULL;\n+\tline->buf[--line->len] = 0;\n+\n+\treturn ref;\n+}\n+\n+/*\n+ * Read from `packed_refs_file` into a newly-allocated\n+ * `packed_ref_cache` and return it. The return value will already\n+ * have its reference count incremented.\n+ *\n+ * A comment line of the form \"# pack-refs with: \" may contain zero or\n+ * more traits. We interpret the traits as follows:\n+ *\n+ * No traits:\n+ *\n+ * Probably no references are peeled. But if the file contains a\n+ * peeled value for a reference, we will use it.\n+ *\n+ * peeled:\n+ *\n+ * References under \"refs/tags/\", if they *can* be peeled, *are*\n+ * peeled in this file. References outside of \"refs/tags/\" are\n+ * probably not peeled even if they could have been, but if we find\n+ * a peeled value for such a reference we will use it.\n+ *\n+ * fully-peeled:\n+ *\n+ * All references in the file that can be peeled are peeled.\n+ * Inversely (and this is more important), any references in the\n+ * file for which no peeled value is recorded is not peelable. This\n+ * trait should typically be written alongside \"peeled\" for\n+ * compatibility with older clients, but we do not require it\n+ * (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n+ */\n+static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n+{\n+\tFILE *f;\n+\tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n+\tstruct ref_entry *last = NULL;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n+\tstruct ref_dir *dir;\n+\n+\tacquire_packed_ref_cache(packed_refs);\n+\tpacked_refs->cache = create_ref_cache(NULL, NULL);\n+\tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n+\n+\tf = fopen(packed_refs_file, \"r\");\n+\tif (!f) {\n+\t\tif (errno == ENOENT) {\n+\t\t\t/*\n+\t\t\t * This is OK; it just means that no\n+\t\t\t * \"packed-refs\" file has been written yet,\n+\t\t\t * which is equivalent to it being empty.\n+\t\t\t */\n+\t\t\treturn packed_refs;\n+\t\t} else {\n+\t\t\tdie_errno(\"couldn't read %s\", packed_refs_file);\n+\t\t}\n+\t}\n+\n+\tstat_validity_update(&packed_refs->validity, fileno(f));\n+\n+\tdir = get_ref_dir(packed_refs->cache->root);\n+\twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n+\t\tstruct object_id oid;\n+\t\tconst char *refname;\n+\t\tconst char *traits;\n+\n+\t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n+\t\t\tif (strstr(traits, \" fully-peeled \"))\n+\t\t\t\tpeeled = PEELED_FULLY;\n+\t\t\telse if (strstr(traits, \" peeled \"))\n+\t\t\t\tpeeled = PEELED_TAGS;\n+\t\t\t/* perhaps other traits later as well */\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\trefname = parse_ref_line(&line, &oid);\n+\t\tif (refname) {\n+\t\t\tint flag = REF_ISPACKED;\n+\n+\t\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\t\t\tif (!refname_is_safe(refname))\n+\t\t\t\t\tdie(\"packed refname is dangerous: %s\", refname);\n+\t\t\t\toidclr(&oid);\n+\t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n+\t\t\t}\n+\t\t\tlast = create_ref_entry(refname, &oid, flag);\n+\t\t\tif (peeled == PEELED_FULLY ||\n+\t\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n+\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t\tadd_ref_entry(dir, last);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (last &&\n+\t\t line.buf[0] == '^' &&\n+\t\t line.len == PEELED_LINE_LENGTH &&\n+\t\t line.buf[PEELED_LINE_LENGTH - 1] == '\\n' &&\n+\t\t !get_oid_hex(line.buf + 1, &oid)) {\n+\t\t\toidcpy(&last->u.value.peeled, &oid);\n+\t\t\t/*\n+\t\t\t * Regardless of what the file header said,\n+\t\t\t * we definitely know the value of *this*\n+\t\t\t * reference:\n+\t\t\t */\n+\t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t}\n+\t}\n+\n+\tfclose(f);\n+\tstrbuf_release(&line);\n+\n+\treturn packed_refs;\n+}\n+\n+/*\n+ * Check that the packed refs cache (if any) still reflects the\n+ * contents of the file. If not, clear the cache.\n+ */\n+static void validate_packed_ref_cache(struct packed_ref_store *refs)\n+{\n+\tif (refs->cache &&\n+\t !stat_validity_check(&refs->cache->validity, refs->path))\n+\t\tclear_packed_ref_cache(refs);\n+}\n+\n+/*\n+ * Get the packed_ref_cache for the specified packed_ref_store,\n+ * creating and populating it if it hasn't been read before or if the\n+ * file has been changed (according to its `validity` field) since it\n+ * was last read. On the other hand, if we hold the lock, then assume\n+ * that the file hasn't been changed out from under us, so skip the\n+ * extra `stat()` call in `stat_validity_check()`.\n+ */\n+static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *refs)\n+{\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tvalidate_packed_ref_cache(refs);\n+\n+\tif (!refs->cache)\n+\t\trefs->cache = read_packed_refs(refs->path);\n+\n+\treturn refs->cache;\n+}\n+\n+static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)\n+{\n+\treturn get_ref_dir(packed_ref_cache->cache->root);\n+}\n+\n+static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n+{\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 lock_packed_refs()). To actually write the packed-refs file,\n+ * call commit_packed_refs().\n+ */\n+void add_packed_ref(struct packed_ref_store *refs,\n+\t\t const char *refname, const struct object_id *oid)\n+{\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+ */\n+static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n+\t\t\t\t\tconst char *refname)\n+{\n+\treturn find_ref_entry(get_packed_refs(refs), refname);\n+}\n+\n+int packed_read_raw_ref(struct packed_ref_store *refs,\n+\t\t\tconst char *refname, unsigned char *sha1,\n+\t\t\tstruct strbuf *referent, unsigned int *type)\n+{\n+\tstruct ref_entry *entry;\n+\n+\t*type = 0;\n+\n+\tentry = get_packed_ref(refs, refname);\n+\tif (!entry) {\n+\t\terrno = ENOENT;\n+\t\treturn -1;\n+\t}\n+\n+\thashcpy(sha1, entry->u.value.oid.hash);\n+\t*type = REF_ISPACKED;\n+\treturn 0;\n+}\n+\n+int packed_peel_ref(struct packed_ref_store *refs,\n+\t\t const char *refname, unsigned char *sha1)\n+{\n+\tstruct ref_entry *r = get_packed_ref(refs, refname);\n+\n+\tif (!r || peel_entry(r, 0))\n+\t\treturn -1;\n+\n+\thashcpy(sha1, r->u.value.peeled.hash);\n+\treturn 0;\n+}\n+\n+struct packed_ref_iterator {\n+\tstruct ref_iterator base;\n+\n+\tstruct packed_ref_cache *cache;\n+\tstruct ref_iterator *iter0;\n+\tunsigned int flags;\n+};\n+\n+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\tint ok;\n+\n+\twhile ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {\n+\t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n+\t\t ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n+\t\t\tcontinue;\n+\n+\t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n+\t\t !ref_resolves_to_object(iter->iter0->refname,\n+\t\t\t\t\t iter->iter0->oid,\n+\t\t\t\t\t iter->iter0->flags))\n+\t\t\tcontinue;\n+\n+\t\titer->base.refname = iter->iter0->refname;\n+\t\titer->base.oid = iter->iter0->oid;\n+\t\titer->base.flags = iter->iter0->flags;\n+\t\treturn ITER_OK;\n+\t}\n+\n+\titer->iter0 = NULL;\n+\tif (ref_iterator_abort(ref_iterator) != ITER_DONE)\n+\t\tok = ITER_ERROR;\n+\n+\treturn ok;\n+}\n+\n+static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t\t\t struct object_id *peeled)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\n+\treturn ref_iterator_peel(iter->iter0, peeled);\n+}\n+\n+static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n+{\n+\tstruct packed_ref_iterator *iter =\n+\t\t(struct packed_ref_iterator *)ref_iterator;\n+\tint ok = ITER_DONE;\n+\n+\tif (iter->iter0)\n+\t\tok = ref_iterator_abort(iter->iter0);\n+\n+\trelease_packed_ref_cache(iter->cache);\n+\tbase_ref_iterator_free(ref_iterator);\n+\treturn ok;\n+}\n+\n+static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n+\tpacked_ref_iterator_advance,\n+\tpacked_ref_iterator_peel,\n+\tpacked_ref_iterator_abort\n+};\n+\n+struct ref_iterator *packed_ref_iterator_begin(\n+\t\tstruct packed_ref_store *refs,\n+\t\tconst char *prefix, unsigned int flags)\n+{\n+\tstruct packed_ref_iterator *iter;\n+\tstruct ref_iterator *ref_iterator;\n+\n+\titer = xcalloc(1, sizeof(*iter));\n+\tref_iterator = &iter->base;\n+\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n+\n+\t/*\n+\t * Note that get_packed_ref_cache() internally checks whether\n+\t * the packed-ref cache is up to date with what is on disk,\n+\t * and re-reads it if not.\n+\t */\n+\n+\titer->cache = get_packed_ref_cache(refs);\n+\tacquire_packed_ref_cache(iter->cache);\n+\titer->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);\n+\n+\titer->flags = flags;\n+\n+\treturn ref_iterator;\n+}\n+\n+/*\n+ * Write an entry to the packed-refs file for the specified refname.\n+ * If peeled is non-NULL, write it as the entry's peeled value.\n+ */\n+static void write_packed_entry(FILE *fh, const char *refname,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t const unsigned char *peeled)\n+{\n+\tfprintf_or_die(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n+\tif (peeled)\n+\t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n+}\n+\n+int lock_packed_refs(struct packed_ref_store *refs, int flags)\n+{\n+\tstatic int timeout_configured = 0;\n+\tstatic int timeout_value = 1000;\n+\tstruct packed_ref_cache *packed_ref_cache;\n+\n+\tpacked_assert_main_repository(refs, \"lock_packed_refs\");\n+\n+\tif (!timeout_configured) {\n+\t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n+\t\ttimeout_configured = 1;\n+\t}\n+\n+\tif (hold_lock_file_for_update_timeout(\n+\t\t\t &refs->lock,\n+\t\t\t refs->path,\n+\t\t\t flags, timeout_value) < 0)\n+\t\treturn -1;\n+\n+\t/*\n+\t * Now that we hold the `packed-refs` lock, make sure that our\n+\t * cache matches the current version of the file. Normally\n+\t * `get_packed_ref_cache()` does that for us, but that\n+\t * function assumes that when the file is locked, any existing\n+\t * cache is still valid. We've just locked the file, but it\n+\t * might have changed the moment *before* we locked it.\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+\treturn 0;\n+}\n+\n+/*\n+ * The packed-refs header line that we write out. Perhaps other\n+ * traits will be added later. The trailing space is required.\n+ */\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+ * lock_packed_refs()). Return zero on success. On errors, set errno\n+ * and return a nonzero value.\n+ */\n+int commit_packed_refs(struct packed_ref_store *refs)\n+{\n+\tstruct packed_ref_cache *packed_ref_cache =\n+\t\tget_packed_ref_cache(refs);\n+\tint ok, error = 0;\n+\tint save_errno = 0;\n+\tFILE *out;\n+\tstruct ref_iterator *iter;\n+\n+\tpacked_assert_main_repository(refs, \"commit_packed_refs\");\n+\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tdie(\"BUG: packed-refs not locked\");\n+\n+\tout = fdopen_lock_file(&refs->lock, \"w\");\n+\tif (!out)\n+\t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\n+\n+\tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\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\twrite_packed_entry(out, iter->refname, iter->oid->hash,\n+\t\t\t\t peel_error ? NULL : peeled.hash);\n+\t}\n+\n+\tif (ok != ITER_DONE)\n+\t\tdie(\"error while iterating over references\");\n+\n+\tif (commit_lock_file(&refs->lock)) {\n+\t\tsave_errno = errno;\n+\t\terror = -1;\n+\t}\n+\trelease_packed_ref_cache(packed_ref_cache);\n+\terrno = save_errno;\n+\treturn error;\n+}\n+\n+/*\n+ * Rollback the lockfile for the packed-refs file, and discard the\n+ * in-memory packed reference cache. (The packed-refs file will be\n+ * read anew if it is needed again after this function is called.)\n+ */\n+static void rollback_packed_refs(struct packed_ref_store *refs)\n+{\n+\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n+\n+\tpacked_assert_main_repository(refs, \"rollback_packed_refs\");\n+\n+\tif (!is_lock_file_locked(&refs->lock))\n+\t\tdie(\"BUG: packed-refs not locked\");\n+\trollback_lock_file(&refs->lock);\n+\trelease_packed_ref_cache(packed_ref_cache);\n+\tclear_packed_ref_cache(refs);\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.\n+ *\n+ * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n+ */\n+int repack_without_refs(struct packed_ref_store *refs,\n+\t\t\tstruct string_list *refnames, struct strbuf *err)\n+{\n+\tstruct ref_dir *packed;\n+\tstruct string_list_item *refname;\n+\tint ret, needs_repacking = 0, removed = 0;\n+\n+\tpacked_assert_main_repository(refs, \"repack_without_refs\");\n+\tassert(err);\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+\tif (lock_packed_refs(refs, 0)) {\n+\t\tunable_to_lock_message(refs->path, errno, err);\n+\t\treturn -1;\n+\t}\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\trollback_packed_refs(refs);\n+\t\treturn 0;\n+\t}\n+\n+\t/* Write what remains */\n+\tret = commit_packed_refs(refs);\n+\tif (ret)\n+\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n+\t\t\t strerror(errno));\n+\treturn ret;\n+}\n+\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nnew file mode 100644\nindex 0000000000..22e8817ac4\n--- /dev/null\n+++ b/refs/packed-backend.h\n@@ -0,0 +1,33 @@\n+#ifndef REFS_PACKED_BACKEND_H\n+#define REFS_PACKED_BACKEND_H\n+\n+struct packed_ref_store *packed_ref_store_create(\n+\t\tconst char *path, unsigned int store_flags);\n+\n+int packed_read_raw_ref(struct packed_ref_store *refs,\n+\t\t\tconst char *refname, unsigned char *sha1,\n+\t\t\tstruct strbuf *referent, unsigned int *type);\n+\n+int packed_peel_ref(struct packed_ref_store *refs,\n+\t\t const char *refname, unsigned char *sha1);\n+\n+struct ref_iterator *packed_ref_iterator_begin(\n+\t\tstruct packed_ref_store *refs,\n+\t\tconst char *prefix, unsigned int flags);\n+\n+/*\n+ * Lock the packed-refs file for writing. Flags is passed to\n+ * hold_lock_file_for_update(). Return 0 on success. On errors, set\n+ * errno appropriately and return a nonzero value.\n+ */\n+int lock_packed_refs(struct packed_ref_store *refs, int flags);\n+\n+void add_packed_ref(struct packed_ref_store *refs,\n+\t\t const char *refname, const struct object_id *oid);\n+\n+int commit_packed_refs(struct packed_ref_store *refs);\n+\n+int repack_without_refs(struct packed_ref_store *refs,\n+\t\t\tstruct string_list *refnames, struct strbuf *err);\n+\n+#endif /* REFS_PACKED_BACKEND_H */\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 192f9f85c9..6f8f9f5619 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -77,6 +77,15 @@\n */\n int refname_is_safe(const char *refname);\n \n+/*\n+ * Helper function: return true if refname, which has the specified\n+ * oid and flags, can be resolved to an object in the database. If the\n+ * referred-to object does not exist, emit a warning and return false.\n+ */\n+int ref_resolves_to_object(const char *refname,\n+\t\t\t const struct object_id *oid,\n+\t\t\t unsigned int flags);\n+\n enum peel_status {\n \t/* object was peeled successfully: */\n \tPEEL_PEELED = 0,\n-- \n2.11.0\n\n"},{"id":"323712","messageId":"1c34d58b9f371e7a57f27cbf947eeef3fea31146.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 18/30] packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:56Z","receivedAt":"2017-07-01T18:32:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new function, `packed_read_raw_ref()`, which is nearly a\n`read_raw_ref_fn`. Use it in place of `resolve_packed_ref()`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 36 +++++++++++++++++-------------------\n 1 file changed, 17 insertions(+), 19 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0490cc087e..346794cf7c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -608,27 +608,23 @@ static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n \treturn find_ref_entry(get_packed_refs(refs), refname);\n }\n \n-/*\n- * A loose ref file doesn't exist; check for a packed ref.\n- */\n-static int resolve_packed_ref(struct files_ref_store *refs,\n-\t\t\t const char *refname,\n-\t\t\t unsigned char *sha1, unsigned int *flags)\n+static int packed_read_raw_ref(struct packed_ref_store *refs,\n+\t\t\t const char *refname, unsigned char *sha1,\n+\t\t\t struct strbuf *referent, unsigned int *type)\n {\n \tstruct ref_entry *entry;\n \n-\t/*\n-\t * The loose reference file does not exist; check for a packed\n-\t * reference.\n-\t */\n-\tentry = get_packed_ref(refs->packed_ref_store, refname);\n-\tif (entry) {\n-\t\thashcpy(sha1, entry->u.value.oid.hash);\n-\t\t*flags |= REF_ISPACKED;\n-\t\treturn 0;\n+\t*type = 0;\n+\n+\tentry = get_packed_ref(refs, refname);\n+\tif (!entry) {\n+\t\terrno = ENOENT;\n+\t\treturn -1;\n \t}\n-\t/* refname is not a packed reference. */\n-\treturn -1;\n+\n+\thashcpy(sha1, entry->u.value.oid.hash);\n+\t*type = REF_ISPACKED;\n+\treturn 0;\n }\n \n static int files_read_raw_ref(struct ref_store *ref_store,\n@@ -674,7 +670,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tif (lstat(path, &st) < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-\t\tif (resolve_packed_ref(refs, refname, sha1, type)) {\n+\t\tif (packed_read_raw_ref(refs->packed_ref_store, refname,\n+\t\t\t\t\tsha1, referent, type)) {\n \t\t\terrno = ENOENT;\n \t\t\tgoto out;\n \t\t}\n@@ -713,7 +710,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (resolve_packed_ref(refs, refname, sha1, type)) {\n+\t\tif (packed_read_raw_ref(refs->packed_ref_store, refname,\n+\t\t\t\t\tsha1, referent, type)) {\n \t\t\terrno = EISDIR;\n \t\t\tgoto out;\n \t\t}\n-- \n2.11.0\n\n"},{"id":"323713","messageId":"9b920206192a0dfb65c5aec03015332ab561bace.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 22/30] commit_packed_refs(): use a staging file separate from the lockfile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:00Z","receivedAt":"2017-07-01T18:32:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We will want to be able to hold the lockfile for `packed-refs` even\nafter we have activated the new values. So use a separate tempfile,\n`packed-refs.new`, as a place to stage the new contents of the\n`packed-refs` file. For now this is all done within\n`commit_packed_refs()`, but that will change shortly.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 40 ++++++++++++++++++++++++++++++++--------\n 1 file changed, 32 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 18ce47fcb7..71f92ed6f0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -68,6 +68,13 @@ struct packed_ref_store {\n \t * thus the enclosing `packed_ref_store`) must not be freed.\n \t */\n \tstruct lock_file lock;\n+\n+\t/*\n+\t * Temporary file used when rewriting new contents to the\n+\t * \"packed-refs\" file. Note that this (and thus the enclosing\n+\t * `packed_ref_store`) must not be freed.\n+\t */\n+\tstruct tempfile tempfile;\n };\n \n struct ref_store *packed_ref_store_create(const char *path,\n@@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int flags)\n \t\ttimeout_configured = 1;\n \t}\n \n+\t/*\n+\t * Note that we close the lockfile immediately because we\n+\t * don't write new content to it, but rather to a separate\n+\t * tempfile.\n+\t */\n \tif (hold_lock_file_for_update_timeout(\n \t\t\t &refs->lock,\n \t\t\t refs->path,\n-\t\t\t flags, timeout_value) < 0)\n+\t\t\t flags, timeout_value) < 0 ||\n+\t close_lock_file(&refs->lock))\n \t\treturn -1;\n \n \t/*\n@@ -567,13 +580,23 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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 \n \tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: commit_packed_refs() called when unlocked\");\n \n-\tout = fdopen_lock_file(&refs->lock, \"w\");\n+\tstrbuf_addf(&sb, \"%s.new\", 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@@ -582,7 +605,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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_lock_file_path(&refs->lock), strerror(errno));\n+\t\t\t get_tempfile_path(&refs->tempfile), strerror(errno));\n \t\tgoto error;\n \t}\n \n@@ -594,7 +617,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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_lock_file_path(&refs->lock),\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@@ -602,13 +625,13 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \t}\n \n \tif (ok != ITER_DONE) {\n-\t\tstrbuf_addf(err, \"unable to write packed-refs file: \"\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 (commit_lock_file(&refs->lock)) {\n-\t\tstrbuf_addf(err, \"error overwriting %s: %s\",\n+\tif (rename_tempfile(&refs->tempfile, 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@@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \tgoto out;\n \n error:\n-\trollback_lock_file(&refs->lock);\n+\tdelete_tempfile(&refs->tempfile);\n \n out:\n+\trollback_lock_file(&refs->lock);\n \trelease_packed_ref_cache(packed_ref_cache);\n \treturn ret;\n }\n-- \n2.11.0\n\n"},{"id":"323714","messageId":"769e494a5f36da82034c9c27dc8dd876fd167540.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 20/30] packed_ref_store: make class into a subclass of `ref_store`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:58Z","receivedAt":"2017-07-01T18:32:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add the infrastructure to make `packed_ref_store` implement\n`ref_store`, at least formally (few of the methods are actually\nimplemented yet). Change the functions in its interface to take\n`ref_store *` arguments. Change `files_ref_store` to store a pointer\nto `ref_store *` and to call functions via the virtual `ref_store`\ninterface where possible. This also means that a few\n`packed_ref_store` functions can become static.\n\nThis is a work in progress. Some more `ref_store` methods will soon be\nimplemented (e.g., those having to do with reference transactions).\nBut some of them will never be implemented (e.g., those having to do\nwith symrefs or reflogs).\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 16 ++--\n refs/packed-backend.c | 231 +++++++++++++++++++++++++++++++++++++++++++++-----\n refs/packed-backend.h | 23 ++---\n refs/refs-internal.h | 1 +\n 4 files changed, 226 insertions(+), 45 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7df9747798..60f4fa5e7a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -28,7 +28,7 @@ struct files_ref_store {\n \n \tstruct ref_cache *loose;\n \n-\tstruct packed_ref_store *packed_ref_store;\n+\tstruct ref_store *packed_ref_store;\n };\n \n static void clear_loose_ref_cache(struct files_ref_store *refs)\n@@ -311,8 +311,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tif (lstat(path, &st) < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-\t\tif (packed_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t\tsha1, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n+\t\t\t\t sha1, referent, type)) {\n \t\t\terrno = ENOENT;\n \t\t\tgoto out;\n \t\t}\n@@ -351,8 +351,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (packed_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t\tsha1, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n+\t\t\t\t sha1, referent, type)) {\n \t\t\terrno = EISDIR;\n \t\t\tgoto out;\n \t\t}\n@@ -683,7 +683,7 @@ static int files_peel_ref(struct ref_store *ref_store,\n \t * have REF_KNOWS_PEELED.\n \t */\n \tif (flag & REF_ISPACKED &&\n-\t !packed_peel_ref(refs->packed_ref_store, refname, sha1))\n+\t !refs_peel_ref(refs->packed_ref_store, refname, sha1))\n \t\treturn 0;\n \n \treturn peel_object(base, sha1);\n@@ -804,8 +804,8 @@ static struct ref_iterator *files_ref_iterator_begin(\n \t * ones in files_ref_iterator_advance(), after we have merged\n \t * the packed and loose references.\n \t */\n-\tpacked_iter = packed_ref_iterator_begin(\n-\t\t\trefs->packed_ref_store, prefix,\n+\tpacked_iter = refs_ref_iterator_begin(\n+\t\t\trefs->packed_ref_store, prefix, 0,\n \t\t\tDO_FOR_EACH_INCLUDE_BROKEN);\n \n \titer->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 6fa988c953..4676dc3959 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -50,6 +50,8 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n * `ref_store`.\n */\n struct packed_ref_store {\n+\tstruct ref_store base;\n+\n \tunsigned int store_flags;\n \n \t/* The path of the \"packed-refs\" file: */\n@@ -68,14 +70,17 @@ struct packed_ref_store {\n \tstruct lock_file lock;\n };\n \n-struct packed_ref_store *packed_ref_store_create(\n-\t\tconst char *path, unsigned int store_flags)\n+struct ref_store *packed_ref_store_create(const char *path,\n+\t\t\t\t\t unsigned int store_flags)\n {\n \tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n+\tstruct ref_store *ref_store = (struct ref_store *)refs;\n \n+\tbase_ref_store_init(ref_store, &refs_be_packed);\n \trefs->store_flags = store_flags;\n+\n \trefs->path = xstrdup(path);\n-\treturn refs;\n+\treturn ref_store;\n }\n \n /*\n@@ -91,6 +96,31 @@ static void packed_assert_main_repository(struct packed_ref_store *refs,\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+ * support at least the flags specified in `required_flags`. `caller`\n+ * is used in any necessary error messages.\n+ */\n+static struct packed_ref_store *packed_downcast(struct ref_store *ref_store,\n+\t\t\t\t\t\tunsigned int required_flags,\n+\t\t\t\t\t\tconst char *caller)\n+{\n+\tstruct packed_ref_store *refs;\n+\n+\tif (ref_store->be != &refs_be_packed)\n+\t\tdie(\"BUG: ref_store is type \\\"%s\\\" not \\\"packed\\\" in %s\",\n+\t\t ref_store->be->name, caller);\n+\n+\trefs = (struct packed_ref_store *)ref_store;\n+\n+\tif ((refs->store_flags & required_flags) != required_flags)\n+\t\tdie(\"BUG: unallowed operation (%s), requires %x, has %x\\n\",\n+\t\t caller, required_flags, refs->store_flags);\n+\n+\treturn refs;\n+}\n+\n static void clear_packed_ref_cache(struct packed_ref_store *refs)\n {\n \tif (refs->cache) {\n@@ -287,9 +317,12 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n * (see lock_packed_refs()). To actually write the packed-refs file,\n * call commit_packed_refs().\n */\n-void add_packed_ref(struct packed_ref_store *refs,\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@@ -322,10 +355,13 @@ static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n \treturn find_ref_entry(get_packed_refs(refs), refname);\n }\n \n-int packed_read_raw_ref(struct packed_ref_store *refs,\n-\t\t\tconst char *refname, unsigned char *sha1,\n-\t\t\tstruct strbuf *referent, unsigned int *type)\n+static int packed_read_raw_ref(struct ref_store *ref_store,\n+\t\t\t const char *refname, unsigned char *sha1,\n+\t\t\t struct strbuf *referent, unsigned int *type)\n {\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n+\n \tstruct ref_entry *entry;\n \n \t*type = 0;\n@@ -341,9 +377,12 @@ int packed_read_raw_ref(struct packed_ref_store *refs,\n \treturn 0;\n }\n \n-int packed_peel_ref(struct packed_ref_store *refs,\n-\t\t const char *refname, unsigned char *sha1)\n+static int packed_peel_ref(struct ref_store *ref_store,\n+\t\t\t const char *refname, unsigned char *sha1)\n {\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,\n+\t\t\t\t\"peel_ref\");\n \tstruct ref_entry *r = get_packed_ref(refs, refname);\n \n \tif (!r || peel_entry(r, 0))\n@@ -420,12 +459,18 @@ static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n \tpacked_ref_iterator_abort\n };\n \n-struct ref_iterator *packed_ref_iterator_begin(\n-\t\tstruct packed_ref_store *refs,\n+static struct ref_iterator *packed_ref_iterator_begin(\n+\t\tstruct ref_store *ref_store,\n \t\tconst char *prefix, unsigned int flags)\n {\n+\tstruct packed_ref_store *refs;\n \tstruct packed_ref_iterator *iter;\n \tstruct ref_iterator *ref_iterator;\n+\tunsigned int required_flags = REF_STORE_READ;\n+\n+\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))\n+\t\trequired_flags |= REF_STORE_ODB;\n+\trefs = packed_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n@@ -459,14 +504,15 @@ static void write_packed_entry(FILE *fh, const char *refname,\n \t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n }\n \n-int lock_packed_refs(struct packed_ref_store *refs, int flags)\n+int lock_packed_refs(struct ref_store *ref_store, int flags)\n {\n+\tstruct packed_ref_store *refs =\n+\t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n+\t\t\t\t\"lock_packed_refs\");\n \tstatic int timeout_configured = 0;\n \tstatic int timeout_value = 1000;\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tpacked_assert_main_repository(refs, \"lock_packed_refs\");\n-\n \tif (!timeout_configured) {\n \t\tgit_config_get_int(\"core.packedrefstimeout\", &timeout_value);\n \t\ttimeout_configured = 1;\n@@ -507,8 +553,11 @@ static const char PACKED_REFS_HEADER[] =\n * lock_packed_refs()). Return zero on success. On errors, set errno\n * and return a nonzero value.\n */\n-int commit_packed_refs(struct packed_ref_store *refs)\n+int commit_packed_refs(struct ref_store *ref_store)\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, error = 0;\n@@ -516,8 +565,6 @@ int commit_packed_refs(struct packed_ref_store *refs)\n \tFILE *out;\n \tstruct ref_iterator *iter;\n \n-\tpacked_assert_main_repository(refs, \"commit_packed_refs\");\n-\n \tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n \n@@ -573,9 +620,12 @@ static void rollback_packed_refs(struct packed_ref_store *refs)\n *\n * The refs in 'refnames' needn't be sorted. `err` must not be NULL.\n */\n-int repack_without_refs(struct packed_ref_store *refs,\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 ret, needs_repacking = 0, removed = 0;\n@@ -595,7 +645,7 @@ int repack_without_refs(struct packed_ref_store *refs,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (lock_packed_refs(refs, 0)) {\n+\tif (lock_packed_refs(&refs->base, 0)) {\n \t\tunable_to_lock_message(refs->path, errno, err);\n \t\treturn -1;\n \t}\n@@ -615,10 +665,151 @@ int repack_without_refs(struct packed_ref_store *refs,\n \t}\n \n \t/* Write what remains */\n-\tret = commit_packed_refs(refs);\n+\tret = commit_packed_refs(&refs->base);\n \tif (ret)\n \t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n \t\t\t strerror(errno));\n \treturn ret;\n }\n \n+static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n+{\n+\t/* Nothing to do. */\n+\treturn 0;\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+}\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+}\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+}\n+\n+static int packed_initial_transaction_commit(struct ref_store *ref_store,\n+\t\t\t\t\t struct ref_transaction *transaction,\n+\t\t\t\t\t struct strbuf *err)\n+{\n+\treturn ref_transaction_commit(transaction, err);\n+}\n+\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+}\n+\n+static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)\n+{\n+\t/*\n+\t * Packed refs are already packed. It might be that loose refs\n+\t * are packed *into* a packed refs store, but that is done by\n+\t * updating the packed references via a transaction.\n+\t */\n+\treturn 0;\n+}\n+\n+static int packed_create_symref(struct ref_store *ref_store,\n+\t\t\t const char *refname, const char *target,\n+\t\t\t const char *logmsg)\n+{\n+\tdie(\"BUG: packed reference store does not support symrefs\");\n+}\n+\n+static int packed_rename_ref(struct ref_store *ref_store,\n+\t\t\t const char *oldrefname, const char *newrefname,\n+\t\t\t const char *logmsg)\n+{\n+\tdie(\"BUG: packed reference store does not support renaming references\");\n+}\n+\n+static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store)\n+{\n+\treturn empty_ref_iterator_begin();\n+}\n+\n+static int packed_for_each_reflog_ent(struct ref_store *ref_store,\n+\t\t\t\t const char *refname,\n+\t\t\t\t each_reflog_ent_fn fn, void *cb_data)\n+{\n+\treturn 0;\n+}\n+\n+static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t each_reflog_ent_fn fn,\n+\t\t\t\t\t void *cb_data)\n+{\n+\treturn 0;\n+}\n+\n+static int packed_reflog_exists(struct ref_store *ref_store,\n+\t\t\t const char *refname)\n+{\n+\treturn 0;\n+}\n+\n+static int packed_create_reflog(struct ref_store *ref_store,\n+\t\t\t const char *refname, int force_create,\n+\t\t\t struct strbuf *err)\n+{\n+\tdie(\"BUG: packed reference store does not support reflogs\");\n+}\n+\n+static int packed_delete_reflog(struct ref_store *ref_store,\n+\t\t\t const char *refname)\n+{\n+\treturn 0;\n+}\n+\n+static int packed_reflog_expire(struct ref_store *ref_store,\n+\t\t\t\tconst char *refname, const unsigned char *sha1,\n+\t\t\t\tunsigned int flags,\n+\t\t\t\treflog_expiry_prepare_fn prepare_fn,\n+\t\t\t\treflog_expiry_should_prune_fn should_prune_fn,\n+\t\t\t\treflog_expiry_cleanup_fn cleanup_fn,\n+\t\t\t\tvoid *policy_cb_data)\n+{\n+\treturn 0;\n+}\n+\n+struct ref_storage_be refs_be_packed = {\n+\tNULL,\n+\t\"packed\",\n+\tpacked_ref_store_create,\n+\tpacked_init_db,\n+\tpacked_transaction_prepare,\n+\tpacked_transaction_finish,\n+\tpacked_transaction_abort,\n+\tpacked_initial_transaction_commit,\n+\n+\tpacked_pack_refs,\n+\tpacked_peel_ref,\n+\tpacked_create_symref,\n+\tpacked_delete_refs,\n+\tpacked_rename_ref,\n+\n+\tpacked_ref_iterator_begin,\n+\tpacked_read_raw_ref,\n+\n+\tpacked_reflog_iterator_begin,\n+\tpacked_for_each_reflog_ent,\n+\tpacked_for_each_reflog_ent_reverse,\n+\tpacked_reflog_exists,\n+\tpacked_create_reflog,\n+\tpacked_delete_reflog,\n+\tpacked_reflog_expire\n+};\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 22e8817ac4..beea9c14b5 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -1,33 +1,22 @@\n #ifndef REFS_PACKED_BACKEND_H\n #define REFS_PACKED_BACKEND_H\n \n-struct packed_ref_store *packed_ref_store_create(\n-\t\tconst char *path, unsigned int store_flags);\n-\n-int packed_read_raw_ref(struct packed_ref_store *refs,\n-\t\t\tconst char *refname, unsigned char *sha1,\n-\t\t\tstruct strbuf *referent, unsigned int *type);\n-\n-int packed_peel_ref(struct packed_ref_store *refs,\n-\t\t const char *refname, unsigned char *sha1);\n-\n-struct ref_iterator *packed_ref_iterator_begin(\n-\t\tstruct packed_ref_store *refs,\n-\t\tconst char *prefix, unsigned int flags);\n+struct ref_store *packed_ref_store_create(const char *path,\n+\t\t\t\t\t unsigned int store_flags);\n \n /*\n * Lock the packed-refs file for writing. Flags is passed to\n * hold_lock_file_for_update(). Return 0 on success. On errors, set\n * errno appropriately and return a nonzero value.\n */\n-int lock_packed_refs(struct packed_ref_store *refs, int flags);\n+int lock_packed_refs(struct ref_store *ref_store, int flags);\n \n-void add_packed_ref(struct packed_ref_store *refs,\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 packed_ref_store *refs);\n+int commit_packed_refs(struct ref_store *ref_store);\n \n-int repack_without_refs(struct packed_ref_store *refs,\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 */\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 6f8f9f5619..4789106fc0 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -664,6 +664,7 @@ struct ref_storage_be {\n };\n \n extern struct ref_storage_be refs_be_files;\n+extern struct ref_storage_be refs_be_packed;\n \n /*\n * A representation of the reference store for the main repository or\n-- \n2.11.0\n\n"},{"id":"323715","messageId":"f920fc693ffd464120e8925d6e7faeae8b8a8dab.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 13/30] rollback_packed_refs(): take a `packed_ref_store *` parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:51Z","receivedAt":"2017-07-01T18:32:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only cares about the packed-refs part of the reference store.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5d159620f0..a08d3fbadf 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1434,18 +1434,17 @@ static int commit_packed_refs(struct packed_ref_store *refs)\n * in-memory packed reference cache. (The packed-refs file will be\n * read anew if it is needed again after this function is called.)\n */\n-static void rollback_packed_refs(struct files_ref_store *refs)\n+static void rollback_packed_refs(struct packed_ref_store *refs)\n {\n-\tstruct packed_ref_cache *packed_ref_cache =\n-\t\tget_packed_ref_cache(refs->packed_ref_store);\n+\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n \n-\tfiles_assert_main_repository(refs, \"rollback_packed_refs\");\n+\tpacked_assert_main_repository(refs, \"rollback_packed_refs\");\n \n-\tif (!is_lock_file_locked(&refs->packed_ref_store->lock))\n+\tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(&refs->packed_ref_store->lock);\n+\trollback_lock_file(&refs->lock);\n \trelease_packed_ref_cache(packed_ref_cache);\n-\tclear_packed_ref_cache(refs->packed_ref_store);\n+\tclear_packed_ref_cache(refs);\n }\n \n struct ref_to_prune {\n@@ -1657,7 +1656,7 @@ static int repack_without_refs(struct files_ref_store *refs,\n \t\t * All packed entries disappeared while we were\n \t\t * acquiring the lock.\n \t\t */\n-\t\trollback_packed_refs(refs);\n+\t\trollback_packed_refs(refs->packed_ref_store);\n \t\treturn 0;\n \t}\n \n-- \n2.11.0\n\n"},{"id":"323716","messageId":"4d04ba165888245f384df461f981a664d2c5ffce.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 23/30] packed_refs_lock(): function renamed from lock_packed_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:01Z","receivedAt":"2017-07-01T18:32:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rename `lock_packed_refs()` to `packed_refs_lock()` for consistency\nwith how other methods are named. Also, it's about to get some\ncompanions.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 4 ++--\n refs/packed-backend.c | 10 +++++-----\n refs/packed-backend.h | 2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2810785efc..88de907148 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct ref_to_prune *refs_to_prune = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tlock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);\n+\tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);\n \n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n@@ -2679,7 +2679,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t}\n \t}\n \n-\tif (lock_packed_refs(refs->packed_ref_store, 0)) {\n+\tif (packed_refs_lock(refs->packed_ref_store, 0)) {\n \t\tstrbuf_addf(err, \"unable to lock packed-refs file: %s\",\n \t\t\t strerror(errno));\n \t\tret = TRANSACTION_GENERIC_ERROR;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 71f92ed6f0..cd214e07a1 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -321,7 +321,7 @@ static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\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 lock_packed_refs()). To actually write the packed-refs file,\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@@ -515,11 +515,11 @@ static int write_packed_entry(FILE *fh, const char *refname,\n \treturn 0;\n }\n \n-int lock_packed_refs(struct ref_store *ref_store, int flags)\n+int packed_refs_lock(struct ref_store *ref_store, int flags)\n {\n \tstruct packed_ref_store *refs =\n \t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n-\t\t\t\t\"lock_packed_refs\");\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@@ -567,7 +567,7 @@ static const char PACKED_REFS_HEADER[] =\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- * lock_packed_refs()). Return zero on success. On errors, rollback\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@@ -698,7 +698,7 @@ int repack_without_refs(struct ref_store *ref_store,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (lock_packed_refs(&refs->base, 0)) {\n+\tif (packed_refs_lock(&refs->base, 0)) {\n \t\tunable_to_lock_message(refs->path, errno, err);\n \t\treturn -1;\n \t}\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 3d4057b65b..dbc00d3396 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -9,7 +9,7 @@ struct ref_store *packed_ref_store_create(const char *path,\n * hold_lock_file_for_update(). Return 0 on success. On errors, set\n * errno appropriately and return a nonzero value.\n */\n-int lock_packed_refs(struct ref_store *ref_store, int flags);\n+int packed_refs_lock(struct ref_store *ref_store, int flags);\n \n void add_packed_ref(struct ref_store *ref_store,\n \t\t const char *refname, const struct object_id *oid);\n-- \n2.11.0\n\n"},{"id":"323717","messageId":"5354fb6b0a1ac30061ecf30852ee877b599ae1ac.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 28/30] repack_without_refs(): don't lock or unlock the packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:06Z","receivedAt":"2017-07-01T18:32:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change `repack_without_refs()` to expect the packed-refs lock to be\nheld already, and not to release the lock before returning. Change the\ncallers to deal with lock management.\n\nThis change makes it possible for callers to hold the packed-refs lock\nfor a longer span of time, a possibility that will eventually make it\npossible to fix some longstanding races.\n\nThe only semantic change here is that `repack_without_refs()` used to\nforget to release the lock in the `if (!removed)` exit path. That\nomission is now fixed.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 47 +++++++++++++++++++++++++++++++----------------\n refs/packed-backend.c | 32 ++++++++------------------------\n 2 files changed, 39 insertions(+), 40 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 93bdc8f0c8..e9b95592b6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1149,24 +1149,16 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!refnames->nr)\n \t\treturn 0;\n \n-\tresult = repack_without_refs(refs->packed_ref_store, refnames, &err);\n-\tif (result) {\n-\t\t/*\n-\t\t * If we failed to rewrite the packed-refs file, then\n-\t\t * it is unsafe to try to remove loose refs, because\n-\t\t * doing so might expose an obsolete packed value for\n-\t\t * a reference that might even point at an object that\n-\t\t * has been garbage collected.\n-\t\t */\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+\tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n+\t\tgoto error;\n \n-\t\tgoto out;\n+\tif (repack_without_refs(refs->packed_ref_store, refnames, &err)) {\n+\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\t\tgoto error;\n \t}\n \n+\tpacked_refs_unlock(refs->packed_ref_store);\n+\n \tfor (i = 0; i < refnames->nr; i++) {\n \t\tconst char *refname = refnames->items[i].string;\n \n@@ -1174,9 +1166,24 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n \t}\n \n-out:\n \tstrbuf_release(&err);\n \treturn result;\n+\n+error:\n+\t/*\n+\t * If we failed to rewrite the packed-refs file, then it is\n+\t * unsafe to try to remove loose refs, because doing so might\n+\t * expose an obsolete packed value for a reference that might\n+\t * even point at an object that has been garbage collected.\n+\t */\n+\tif (refnames->nr == 1)\n+\t\terror(_(\"could not delete reference %s: %s\"),\n+\t\t refnames->items[0].string, err.buf);\n+\telse\n+\t\terror(_(\"could not delete references: %s\"), err.buf);\n+\n+\tstrbuf_release(&err);\n+\treturn -1;\n }\n \n /*\n@@ -2569,11 +2576,19 @@ static int files_transaction_finish(struct ref_store *ref_store,\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);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 5cf6b3d40e..377c775adb 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -669,25 +669,12 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \treturn -1;\n }\n \n-/*\n- * Rollback the lockfile for the packed-refs file, and discard the\n- * in-memory packed reference cache. (The packed-refs file will be\n- * read anew if it is needed again after this function is called.)\n- */\n-static void rollback_packed_refs(struct packed_ref_store *refs)\n-{\n-\tpacked_assert_main_repository(refs, \"rollback_packed_refs\");\n-\n-\tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: packed-refs not locked\");\n-\tpacked_refs_unlock(&refs->base);\n-\tclear_packed_ref_cache(refs);\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.\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@@ -700,11 +687,13 @@ int repack_without_refs(struct ref_store *ref_store,\n \tstruct ref_dir *packed;\n \tstruct string_list_item *refname;\n \tint needs_repacking = 0, removed = 0;\n-\tint ret;\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@@ -717,9 +706,6 @@ int repack_without_refs(struct ref_store *ref_store,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (packed_refs_lock(&refs->base, 0, err))\n-\t\treturn -1;\n-\n \tpacked = get_packed_refs(refs);\n \n \t/* Remove refnames from the cache */\n@@ -731,14 +717,12 @@ int repack_without_refs(struct ref_store *ref_store,\n \t\t * All packed entries disappeared while we were\n \t\t * acquiring the lock.\n \t\t */\n-\t\trollback_packed_refs(refs);\n+\t\tclear_packed_ref_cache(refs);\n \t\treturn 0;\n \t}\n \n \t/* Write what remains */\n-\tret = commit_packed_refs(&refs->base, err);\n-\tpacked_refs_unlock(ref_store);\n-\treturn ret;\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-- \n2.11.0\n\n"},{"id":"323718","messageId":"70b7e80537793270c4e3a01358f3922090b8b3be.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 29/30] t3210: add some tests of bogus packed-refs file contents","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:07Z","receivedAt":"2017-07-01T18:32:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If `packed-refs` contains indecipherable lines, we should emit an\nerror and quit rather than just skipping the lines. Unfortunately, we\ncurrently do the latter. Add some failing tests demonstrating the\nproblem.\n\nThis will be fixed in the next commit.\n\nSigned-off-by: Michael Haggerty \n---\n t/t3210-pack-refs.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 9b182a0c32..4b65836283 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -194,6 +194,33 @@ test_expect_success 'notice d/f conflict with existing ref' '\n \ttest_must_fail git branch foo/bar/baz/lots/of/extra/components\n '\n \n+test_expect_failure 'reject packed-refs with unterminated line' '\n+\tcp .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tprintf \"%s\" \"$HEAD refs/zzzzz\" >>.git/packed-refs &&\n+\techo \"fatal: unterminated line in .git/packed-refs: $HEAD refs/zzzzz\" >expected_err &&\n+\ttest_must_fail git for-each-ref >out 2>err &&\n+\ttest_cmp expected_err err\n+'\n+\n+test_expect_failure 'reject packed-refs containing junk' '\n+\tcp .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tprintf \"%s\\n\" \"bogus content\" >>.git/packed-refs &&\n+\techo \"fatal: unexpected line in .git/packed-refs: bogus content\" >expected_err &&\n+\ttest_must_fail git for-each-ref >out 2>err &&\n+\ttest_cmp expected_err err\n+'\n+\n+test_expect_failure 'reject packed-refs with a short SHA-1' '\n+\tcp .git/packed-refs .git/packed-refs.bak &&\n+\ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n+\tprintf \"%.7s %s\\n\" $HEAD refs/zzzzz >>.git/packed-refs &&\n+\tprintf \"fatal: unexpected line in .git/packed-refs: %.7s %s\\n\" $HEAD refs/zzzzz >expected_err &&\n+\ttest_must_fail git for-each-ref >out 2>err &&\n+\ttest_cmp expected_err err\n+'\n+\n test_expect_success 'timeout if packed-refs.lock exists' '\n \tLOCK=.git/packed-refs.lock &&\n \t>\"$LOCK\" &&\n-- \n2.11.0\n\n"},{"id":"323719","messageId":"8de06c76181aff06d7236d3a5f7305500f728bde.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 26/30] clear_packed_ref_cache(): don't protest if the lock is held","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:04Z","receivedAt":"2017-07-01T18:32:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The existing callers already check that the lock isn't held just\nbefore calling `clear_packed_ref_cache()`, and in the near future we\nwant to be able to call this function when the lock is held.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f27943f9a1..96d92a5eea 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -133,8 +133,6 @@ static void clear_packed_ref_cache(struct packed_ref_store *refs)\n \tif (refs->cache) {\n \t\tstruct packed_ref_cache *cache = refs->cache;\n \n-\t\tif (is_lock_file_locked(&refs->lock))\n-\t\t\tdie(\"BUG: packed-ref cache cleared while locked\");\n \t\trefs->cache = NULL;\n \t\trelease_packed_ref_cache(cache);\n \t}\n-- \n2.11.0\n\n"},{"id":"323720","messageId":"8b0ddf2433044fa5c5c3ec59633fc59b4f8faf8e.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 30/30] read_packed_refs(): die if `packed-refs` contains bogus data","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:08Z","receivedAt":"2017-07-01T18:32:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code ignored any lines that it didn't understand, including\nunterminated lines. This is dangerous. Instead, `die()` if the\n`packed-refs` file contains any unterminated lines or lines that we\ndon't know how to handle.\n\nThis fixes the tests added in the last commit.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 10 +++++++---\n t/t3210-pack-refs.sh | 6 +++---\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 377c775adb..a28befbfa3 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -229,6 +229,9 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\tconst char *refname;\n \t\tconst char *traits;\n \n+\t\tif (!line.len || line.buf[line.len - 1] != '\\n')\n+\t\t\tdie(\"unterminated line in %s: %s\", packed_refs_file, line.buf);\n+\n \t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n \t\t\t\tpeeled = PEELED_FULLY;\n@@ -253,9 +256,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n \t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t\tadd_ref_entry(dir, last);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (last &&\n+\t\t} else if (last &&\n \t\t line.buf[0] == '^' &&\n \t\t line.len == PEELED_LINE_LENGTH &&\n \t\t line.buf[PEELED_LINE_LENGTH - 1] == '\\n' &&\n@@ -267,6 +268,9 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\t * reference:\n \t\t\t */\n \t\t\tlast->flag |= REF_KNOWS_PEELED;\n+\t\t} else {\n+\t\t\tstrbuf_setlen(&line, line.len - 1);\n+\t\t\tdie(\"unexpected line in %s: %s\", packed_refs_file, line.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 4b65836283..2bb4b25ed9 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -194,7 +194,7 @@ test_expect_success 'notice d/f conflict with existing ref' '\n \ttest_must_fail git branch foo/bar/baz/lots/of/extra/components\n '\n \n-test_expect_failure 'reject packed-refs with unterminated line' '\n+test_expect_success 'reject packed-refs with unterminated line' '\n \tcp .git/packed-refs .git/packed-refs.bak &&\n \ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n \tprintf \"%s\" \"$HEAD refs/zzzzz\" >>.git/packed-refs &&\n@@ -203,7 +203,7 @@ test_expect_failure 'reject packed-refs with unterminated line' '\n \ttest_cmp expected_err err\n '\n \n-test_expect_failure 'reject packed-refs containing junk' '\n+test_expect_success 'reject packed-refs containing junk' '\n \tcp .git/packed-refs .git/packed-refs.bak &&\n \ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n \tprintf \"%s\\n\" \"bogus content\" >>.git/packed-refs &&\n@@ -212,7 +212,7 @@ test_expect_failure 'reject packed-refs containing junk' '\n \ttest_cmp expected_err err\n '\n \n-test_expect_failure 'reject packed-refs with a short SHA-1' '\n+test_expect_success 'reject packed-refs with a short SHA-1' '\n \tcp .git/packed-refs .git/packed-refs.bak &&\n \ttest_when_finished \"mv .git/packed-refs.bak .git/packed-refs\" &&\n \tprintf \"%.7s %s\\n\" $HEAD refs/zzzzz >>.git/packed-refs &&\n-- \n2.11.0\n\n"},{"id":"323721","messageId":"99cb6a5eb837f50f7ef39deca98e4794316b3ca2.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 27/30] commit_packed_refs(): remove call to `packed_refs_unlock()`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:05Z","receivedAt":"2017-07-01T18:32:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, change the callers of `commit_packed_refs()` to call\n`packed_refs_unlock()`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 2 ++\n refs/packed-backend.c | 18 ++++++++----------\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8ea4e9ab05..93bdc8f0c8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1131,6 +1131,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\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+\tpacked_refs_unlock(refs->packed_ref_store);\n \n \tprune_refs(refs, refs_to_prune);\n \tstrbuf_release(&err);\n@@ -2699,6 +2700,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t}\n \n cleanup:\n+\tpacked_refs_unlock(refs->packed_ref_store);\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \tstring_list_clear(&affected_refnames, 0);\n \treturn ret;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 96d92a5eea..5cf6b3d40e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -606,7 +606,6 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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@@ -619,7 +618,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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\treturn -1;\n \t}\n \tstrbuf_release(&sb);\n \n@@ -660,18 +659,14 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \tif (rename_tempfile(&refs->tempfile, 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\treturn -1;\n \t}\n \n-\tret = 0;\n-\tgoto out;\n+\treturn 0;\n \n error:\n \tdelete_tempfile(&refs->tempfile);\n-\n-out:\n-\tpacked_refs_unlock(ref_store);\n-\treturn ret;\n+\treturn -1;\n }\n \n /*\n@@ -705,6 +700,7 @@ int repack_without_refs(struct ref_store *ref_store,\n \tstruct ref_dir *packed;\n \tstruct string_list_item *refname;\n \tint needs_repacking = 0, removed = 0;\n+\tint ret;\n \n \tpacked_assert_main_repository(refs, \"repack_without_refs\");\n \tassert(err);\n@@ -740,7 +736,9 @@ int repack_without_refs(struct ref_store *ref_store,\n \t}\n \n \t/* Write what remains */\n-\treturn commit_packed_refs(&refs->base, err);\n+\tret = commit_packed_refs(&refs->base, err);\n+\tpacked_refs_unlock(ref_store);\n+\treturn ret;\n }\n \n static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n-- \n2.11.0\n\n"},{"id":"323722","messageId":"6adc870c99c80bffcadf34637b98126b698e27e4.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 21/30] commit_packed_refs(): report errors rather than dying","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:30:59Z","receivedAt":"2017-07-01T18:32:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Report errors via a `struct strbuf *err` rather than by calling\n`die()`. To enable this goal, change `write_packed_entry()` to report\nerrors via a return value and `errno` rather than dying.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 10 +++---\n refs/packed-backend.c | 85 +++++++++++++++++++++++++++++++++------------------\n refs/packed-backend.h | 2 +-\n 3 files changed, 61 insertions(+), 36 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 60f4fa5e7a..2810785efc 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1094,6 +1094,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct ref_iterator *iter;\n \tint ok;\n \tstruct ref_to_prune *refs_to_prune = NULL;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tlock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);\n \n@@ -1128,10 +1129,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))\n-\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\tif (commit_packed_refs(refs->packed_ref_store, &err))\n+\t\tdie(\"unable to overwrite old ref-pack file: %s\", err.buf);\n \n \tprune_refs(refs, refs_to_prune);\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \n@@ -2693,9 +2695,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t &update->new_oid);\n \t}\n \n-\tif (commit_packed_refs(refs->packed_ref_store)) {\n-\t\tstrbuf_addf(err, \"unable to commit packed-refs file: %s\",\n-\t\t\t strerror(errno));\n+\tif (commit_packed_refs(refs->packed_ref_store, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 4676dc3959..18ce47fcb7 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -493,15 +493,19 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n /*\n * Write an entry to the packed-refs file for the specified refname.\n- * If peeled is non-NULL, write it as the entry's peeled value.\n+ * If peeled is non-NULL, write it as the entry's peeled value. On\n+ * error, return a nonzero value and leave errno set at the value left\n+ * by the failing call to `fprintf()`.\n */\n-static void write_packed_entry(FILE *fh, const char *refname,\n-\t\t\t const unsigned char *sha1,\n-\t\t\t const unsigned char *peeled)\n+static int write_packed_entry(FILE *fh, const char *refname,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t const unsigned char *peeled)\n {\n-\tfprintf_or_die(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n-\tif (peeled)\n-\t\tfprintf_or_die(fh, \"^%s\\n\", sha1_to_hex(peeled));\n+\tif (fprintf(fh, \"%s %s\\n\", sha1_to_hex(sha1), refname) < 0 ||\n+\t (peeled && fprintf(fh, \"^%s\\n\", sha1_to_hex(peeled)) < 0))\n+\t\treturn -1;\n+\n+\treturn 0;\n }\n \n int lock_packed_refs(struct ref_store *ref_store, int flags)\n@@ -550,49 +554,74 @@ static const char PACKED_REFS_HEADER[] =\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- * lock_packed_refs()). Return zero on success. On errors, set errno\n- * and return a nonzero value.\n+ * lock_packed_refs()). 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)\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, error = 0;\n-\tint save_errno = 0;\n+\tint ok;\n+\tint ret = -1;\n \tFILE *out;\n \tstruct ref_iterator *iter;\n \n \tif (!is_lock_file_locked(&refs->lock))\n-\t\tdie(\"BUG: packed-refs not locked\");\n+\t\tdie(\"BUG: commit_packed_refs() called when unlocked\");\n \n \tout = fdopen_lock_file(&refs->lock, \"w\");\n-\tif (!out)\n-\t\tdie_errno(\"unable to fdopen packed-refs descriptor\");\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-\tfprintf_or_die(out, \"%s\", PACKED_REFS_HEADER);\n+\tif (fprintf(out, \"%s\", PACKED_REFS_HEADER) < 0) {\n+\t\tstrbuf_addf(err, \"error writing to %s: %s\",\n+\t\t\t get_lock_file_path(&refs->lock), 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\twrite_packed_entry(out, iter->refname, iter->oid->hash,\n-\t\t\t\t peel_error ? NULL : peeled.hash);\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_lock_file_path(&refs->lock),\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\tdie(\"error while iterating over references\");\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 (commit_lock_file(&refs->lock)) {\n-\t\tsave_errno = errno;\n-\t\terror = -1;\n+\t\tstrbuf_addf(err, \"error overwriting %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+\trollback_lock_file(&refs->lock);\n+\n+out:\n \trelease_packed_ref_cache(packed_ref_cache);\n-\terrno = save_errno;\n-\treturn error;\n+\treturn ret;\n }\n \n /*\n@@ -628,7 +657,7 @@ int repack_without_refs(struct ref_store *ref_store,\n \t\t\t\t\"repack_without_refs\");\n \tstruct ref_dir *packed;\n \tstruct string_list_item *refname;\n-\tint ret, needs_repacking = 0, removed = 0;\n+\tint needs_repacking = 0, removed = 0;\n \n \tpacked_assert_main_repository(refs, \"repack_without_refs\");\n \tassert(err);\n@@ -665,11 +694,7 @@ int repack_without_refs(struct ref_store *ref_store,\n \t}\n \n \t/* Write what remains */\n-\tret = commit_packed_refs(&refs->base);\n-\tif (ret)\n-\t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n-\t\t\t strerror(errno));\n-\treturn ret;\n+\treturn commit_packed_refs(&refs->base, err);\n }\n \n static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex beea9c14b5..3d4057b65b 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -14,7 +14,7 @@ int lock_packed_refs(struct ref_store *ref_store, int flags);\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);\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-- \n2.11.0\n\n"},{"id":"323723","messageId":"ade8d87db881b5406a68363f4bed5201a3958ac3.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 25/30] packed_refs_unlock(), packed_refs_is_locked(): new functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:03Z","receivedAt":"2017-07-01T18:32:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add two new public functions, `packed_refs_unlock()` and\n`packed_refs_is_locked()`, with which callers can manage and query the\n`packed-refs` lock externally.\n\nCall `packed_refs_unlock()` from `commit_packed_refs()` and\n`rollback_packed_refs()`.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 31 +++++++++++++++++++++++++------\n refs/packed-backend.h | 3 +++\n 2 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 78e877a9e3..f27943f9a1 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -563,6 +563,29 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \treturn 0;\n }\n \n+void packed_refs_unlock(struct ref_store *ref_store)\n+{\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ | REF_STORE_WRITE,\n+\t\t\t\"packed_refs_unlock\");\n+\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+{\n+\tstruct packed_ref_store *refs = packed_downcast(\n+\t\t\tref_store,\n+\t\t\tREF_STORE_READ | REF_STORE_WRITE,\n+\t\t\t\"packed_refs_is_locked\");\n+\n+\treturn is_lock_file_locked(&refs->lock);\n+}\n+\n /*\n * The packed-refs header line that we write out. Perhaps other\n * traits will be added later. The trailing space is required.\n@@ -649,8 +672,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \tdelete_tempfile(&refs->tempfile);\n \n out:\n-\trollback_lock_file(&refs->lock);\n-\trelease_packed_ref_cache(packed_ref_cache);\n+\tpacked_refs_unlock(ref_store);\n \treturn ret;\n }\n \n@@ -661,14 +683,11 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n */\n static void rollback_packed_refs(struct packed_ref_store *refs)\n {\n-\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n-\n \tpacked_assert_main_repository(refs, \"rollback_packed_refs\");\n \n \tif (!is_lock_file_locked(&refs->lock))\n \t\tdie(\"BUG: packed-refs not locked\");\n-\trollback_lock_file(&refs->lock);\n-\trelease_packed_ref_cache(packed_ref_cache);\n+\tpacked_refs_unlock(&refs->base);\n \tclear_packed_ref_cache(refs);\n }\n \ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex 210e3f35ce..03b7c1de95 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -11,6 +11,9 @@ struct ref_store *packed_ref_store_create(const char *path,\n */\n int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);\n \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-- \n2.11.0\n\n"},{"id":"323724","messageId":"f76dd91617965cdd2d72821de60bfcd1a70ab9bb.1498933362.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"[PATCH v3 24/30] packed_refs_lock(): report errors via a `struct strbuf *err`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-01T18:31:02Z","receivedAt":"2017-07-01T18:32:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"That way the callers don't have to come up with error messages\nthemselves.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 6 ++----\n refs/packed-backend.c | 17 +++++++++++------\n refs/packed-backend.h | 6 +++---\n 3 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 88de907148..8ea4e9ab05 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)\n \tstruct ref_to_prune *refs_to_prune = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \n-\tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);\n+\tpacked_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);\n \n \titer = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n@@ -2679,9 +2679,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t}\n \t}\n \n-\tif (packed_refs_lock(refs->packed_ref_store, 0)) {\n-\t\tstrbuf_addf(err, \"unable to lock packed-refs file: %s\",\n-\t\t\t strerror(errno));\n+\tif (packed_refs_lock(refs->packed_ref_store, 0, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex cd214e07a1..78e877a9e3 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -515,7 +515,7 @@ static int write_packed_entry(FILE *fh, const char *refname,\n \treturn 0;\n }\n \n-int packed_refs_lock(struct ref_store *ref_store, int flags)\n+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n {\n \tstruct packed_ref_store *refs =\n \t\tpacked_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,\n@@ -537,9 +537,15 @@ int packed_refs_lock(struct ref_store *ref_store, int flags)\n \tif (hold_lock_file_for_update_timeout(\n \t\t\t &refs->lock,\n \t\t\t refs->path,\n-\t\t\t flags, timeout_value) < 0 ||\n-\t close_lock_file(&refs->lock))\n+\t\t\t flags, timeout_value) < 0) {\n+\t\tunable_to_lock_message(refs->path, errno, err);\n+\t\treturn -1;\n+\t}\n+\n+\tif (close_lock_file(&refs->lock)) {\n+\t\tstrbuf_addf(err, \"unable to close %s: %s\", refs->path, strerror(errno));\n \t\treturn -1;\n+\t}\n \n \t/*\n \t * Now that we hold the `packed-refs` lock, make sure that our\n@@ -698,10 +704,9 @@ int repack_without_refs(struct ref_store *ref_store,\n \tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (packed_refs_lock(&refs->base, 0)) {\n-\t\tunable_to_lock_message(refs->path, errno, err);\n+\tif (packed_refs_lock(&refs->base, 0, err))\n \t\treturn -1;\n-\t}\n+\n \tpacked = get_packed_refs(refs);\n \n \t/* Remove refnames from the cache */\ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex dbc00d3396..210e3f35ce 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -6,10 +6,10 @@ struct ref_store *packed_ref_store_create(const char *path,\n \n /*\n * Lock the packed-refs file for writing. Flags is passed to\n- * hold_lock_file_for_update(). Return 0 on success. On errors, set\n- * errno appropriately and return a nonzero value.\n+ * hold_lock_file_for_update(). Return 0 on success. On errors, write\n+ * an error message to `err` and return a nonzero value.\n */\n-int packed_refs_lock(struct ref_store *ref_store, int flags);\n+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err);\n \n void add_packed_ref(struct ref_store *ref_store,\n \t\t const char *refname, const struct object_id *oid);\n-- \n2.11.0\n\n"},{"id":"323833","messageId":"20170705091216.ij4uzp3wgsalb56f@sigill.intra.peff.net","threadId":"46303","inReplyTo":"cover.1498933362.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/30] Create a reference backend for packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-05T09:12:16Z","receivedAt":"2017-07-05T09:12:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 01, 2017 at 08:30:38PM +0200, Michael Haggerty wrote:\n\n> This is v3 of a patch series creating a `packed_ref_store` reference\n> backend. Thanks to Peff and Junio for their comments about v2 [1].\n> \n> Changes since v2:\n> \n> * Delete some debugging `cat` commands in t1408.\n> \n> * Add some tests of reading packed-refs files with bogus contents.\n> \n> * When reporting corruption in packed-refs files, distinguish between\n> unterminated lines and other corruption.\n> \n> * Fixed a typo in a commit message.\n\nThanks. I just quickly re-reviewed based on the diff from v2, and it\nlooks good to me.\n\n-Peff\n"},{"id":"324848","messageId":"CAGZ79kY=N5H2q1SB9ZEtt1EvuAQg+bqVBZNoXE6nLgtoUd2txA@mail.gmail.com","threadId":"46303","inReplyTo":"20170705091216.ij4uzp3wgsalb56f@sigill.intra.peff.net","subject":"Re: [PATCH v3 00/30] Create a reference backend for packed refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-20T23:05:11Z","receivedAt":"2017-07-20T23:05:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 5, 2017 at 2:12 AM, Jeff King wrote:\n> On Sat, Jul 01, 2017 at 08:30:38PM +0200, Michael Haggerty wrote:\n>\n>> This is v3 of a patch series creating a `packed_ref_store` reference\n>> backend. Thanks to Peff and Junio for their comments about v2 [1].\n>>\n>> Changes since v2:\n>>\n>> * Delete some debugging `cat` commands in t1408.\n>>\n>> * Add some tests of reading packed-refs files with bogus contents.\n>>\n>> * When reporting corruption in packed-refs files, distinguish between\n>> unterminated lines and other corruption.\n>>\n>> * Fixed a typo in a commit message.\n>\n> Thanks. I just quickly re-reviewed based on the diff from v2, and it\n> looks good to me.\n>\n> -Peff\n\nWe have a user that reports:\n\n The issue is for users who have a mirrored repository, \"git pack-refs\"\n now overwrites the .git/packed-refs symlink instead of following it and\n replacing the file it points to.\n\nI suspect this series to be at fault, as the bug report came in a day after\nwe deployed next containing these changes.\n\nDo symlinks and packed-refs ring a bell for this series?\n\nThanks,\nStefan\n"},{"id":"324850","messageId":"20170720232040.GA159617@aiede.mtv.corp.google.com","threadId":"46303","inReplyTo":"CAGZ79kY=N5H2q1SB9ZEtt1EvuAQg+bqVBZNoXE6nLgtoUd2txA@mail.gmail.com","subject":"Re: [PATCH v3 00/30] Create a reference backend for packed refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-20T23:20:40Z","receivedAt":"2017-07-20T23:21:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"+cc: dawalker, who reported the bug\nStefan Beller wrote:\n\n> We have a user that reports:\n>\n> The issue is for users who have a mirrored repository, \"git pack-refs\"\n> now overwrites the .git/packed-refs symlink instead of following it and\n> replacing the file it points to.\n>\n> I suspect this series to be at fault, as the bug report came in a day after\n> we deployed next containing these changes.\n>\n> Do symlinks and packed-refs ring a bell for this series?\n\ncontrib/workdir/git-new-workdir installs packed-refs as a symlink.\nThe reported scenario was with another tool that does something\nsimilar for similar reasons.\n\nDave Walker wrote:\n\n> In the meantime, since this is linked to \"git gc\", it can crop up\n> nearly at any time you modify things from a mirror. I'd recommend\n> extreme care until this is sorted out, and it's probably safest to\n> avoid using the mirror for branch-modifying operations.\n[...]\n> The change at fault is this one:\n> https://github.com/gitster/git/commit/42dfa7ecef22191b004862fb56074b408c94fc97\n\nThat's \"commit_packed_refs(): use a staging file separate from the\nlockfile\", 2017-06-23, which would indeed appear to explain the\nsymptoms.\n\nI'll try to make a reproduction recipe.\n\nThanks,\nJonathan\n"},{"id":"325168","messageId":"d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu","threadId":"46303","inReplyTo":"20170720232040.GA159617@aiede.mtv.corp.google.com","subject":"[PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-26T23:39:42Z","receivedAt":"2017-07-26T23:39:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"One of the tricks that `contrib/workdir/git-new-workdir` plays is to\nmaking `packed-refs` in the new workdir a symlink to the `packed-refs`\nfile in the original repository. Before\n42dfa7ecef (\"commit_packed_refs(): use a staging file separate from\nthe lockfile\", 2017-06-23), a lockfile was used as the staging file,\nand because the `LOCK_NO_DEREF` was not used, the pointed-to file was\nlocked and modified.\n\nBut after that commit, the staging file was created using a tempfile,\nwith the end result that rewriting the `packed-refs` file in the\nworkdir overwrote the symlink rather than the original `packed-refs`\nfile.\n\nChange `commit_packed_refs()` to use `get_locked_file_path()` to find\nthe path of the file that it should overwrite. Since that path was\nproperly resolved when the lockfile was created, this restores the\npre-42dfa7ecef behavior.\n\nAlso add a test case to document this use case and prevent a\nregression like this from recurring.\n\nSigned-off-by: Michael Haggerty \n---\nSorry for the slow response; I've been traveling and very busy.\n\nI didn't even know that a symlinked `packed-refs` file is a thing. The\nattached patch should fix the problem. It applies on top of v3 of\nmh/packed-ref-store [1] (which is already in next).\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1498933362.git.mhagger@alum.mit.edu/\n\n refs/packed-backend.c | 24 ++++++++++++++++++------\n t/t3210-pack-refs.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a28befbfa3..59e7d1a509 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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-\tstrbuf_addf(&sb, \"%s.new\", refs->path);\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\treturn -1;\n+\t\tgoto out;\n \t}\n \tstrbuf_release(&sb);\n \n@@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n \t\tgoto error;\n \t}\n \n-\tif (rename_tempfile(&refs->tempfile, refs->path)) {\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\treturn -1;\n+\t\tgoto out;\n \t}\n \n-\treturn 0;\n+\tret = 0;\n+\tgoto out;\n \n error:\n \tdelete_tempfile(&refs->tempfile);\n-\treturn -1;\n+\n+out:\n+\tfree(packed_refs_path);\n+\treturn ret;\n }\n \n /*\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 2bb4b25ed9..0d8a03e2a9 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '\n \tgit -c core.packedrefstimeout=3000 pack-refs --all --prune\n '\n \n+test_expect_success 'pack symlinked packed-refs' '\n+\t# First make sure that symlinking works when reading:\n+\tgit update-ref refs/heads/loosy refs/heads/master &&\n+\tgit for-each-ref >all-refs-before &&\n+\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n+\tln -s my-deviant-packed-refs .git/packed-refs &&\n+\tgit for-each-ref >all-refs-linked &&\n+\ttest_cmp all-refs-before all-refs-linked &&\n+\tgit pack-refs --all --prune &&\n+\tgit for-each-ref >all-refs-packed &&\n+\ttest_cmp all-refs-before all-refs-packed &&\n+\ttest -h .git/packed-refs &&\n+\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"325172","messageId":"CAGZ79ka+BDqSpQtMPhFjuJrGb+eT=oPNeMEhFavg43TbPVerbw@mail.gmail.com","threadId":"46303","inReplyTo":"d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-27T00:15:41Z","receivedAt":"2017-07-27T00:15:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 26, 2017 at 4:39 PM, Michael Haggerty wrote:\n> One of the tricks that `contrib/workdir/git-new-workdir` plays is to\n> making `packed-refs` in the new workdir a symlink to the `packed-refs`\n> file in the original repository. Before\n> 42dfa7ecef (\"commit_packed_refs(): use a staging file separate from\n> the lockfile\", 2017-06-23), a lockfile was used as the staging file,\n> and because the `LOCK_NO_DEREF` was not used, the pointed-to file was\n> locked and modified.\n>\n> But after that commit, the staging file was created using a tempfile,\n> with the end result that rewriting the `packed-refs` file in the\n> workdir overwrote the symlink rather than the original `packed-refs`\n> file.\n>\n> Change `commit_packed_refs()` to use `get_locked_file_path()` to find\n> the path of the file that it should overwrite. Since that path was\n> properly resolved when the lockfile was created, this restores the\n> pre-42dfa7ecef behavior.\n>\n> Also add a test case to document this use case and prevent a\n> regression like this from recurring.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> Sorry for the slow response; I've been traveling and very busy.\n>\n> I didn't even know that a symlinked `packed-refs` file is a thing. The\n> attached patch should fix the problem. It applies on top of v3 of\n> mh/packed-ref-store [1] (which is already in next).\n\nThanks for providing a fix.\nCode looks good to me, I wonder if the test needs SYMLINKS\nspecial treatment though?\n\nStefan\n"},{"id":"325173","messageId":"20170727001812.GI13924@aiede.mtv.corp.google.com","threadId":"46303","inReplyTo":"d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-27T00:18:12Z","receivedAt":"2017-07-27T00:18:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael Haggerty wrote:\n\n> One of the tricks that `contrib/workdir/git-new-workdir` plays is to\n> making `packed-refs` in the new workdir a symlink to the `packed-refs`\n> file in the original repository. Before\n> 42dfa7ecef (\"commit_packed_refs(): use a staging file separate from\n> the lockfile\", 2017-06-23), a lockfile was used as the staging file,\n> and because the `LOCK_NO_DEREF` was not used, the pointed-to file was\n> locked and modified.\n>\n> But after that commit, the staging file was created using a tempfile,\n> with the end result that rewriting the `packed-refs` file in the\n> workdir overwrote the symlink rather than the original `packed-refs`\n> file.\n>\n> Change `commit_packed_refs()` to use `get_locked_file_path()` to find\n> the path of the file that it should overwrite. Since that path was\n> properly resolved when the lockfile was created, this restores the\n> pre-42dfa7ecef behavior.\n>\n> Also add a test case to document this use case and prevent a\n> regression like this from recurring.\n>\n> Signed-off-by: Michael Haggerty \n\nReported-by: Dave Walker \n\n[...]\n> refs/packed-backend.c | 24 ++++++++++++++++++------\n> t/t3210-pack-refs.sh | 15 +++++++++++++++\n> 2 files changed, 33 insertions(+), 6 deletions(-)\n\nThe patch looks good, except for one nit marked below (*). I'll apply\nit locally and ask people to test it, probably tomorrow morning.\n\nReviewed-by: Jonathan Nieder \n\nThanks for writing it. Patch left unsnipped for reference.\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index a28befbfa3..59e7d1a509 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\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> -\tstrbuf_addf(&sb, \"%s.new\", refs->path);\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\treturn -1;\n> +\t\tgoto out;\n> \t}\n> \tstrbuf_release(&sb);\n> \n> @@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)\n> \t\tgoto error;\n> \t}\n> \n> -\tif (rename_tempfile(&refs->tempfile, refs->path)) {\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\treturn -1;\n> +\t\tgoto out;\n> \t}\n> \n> -\treturn 0;\n> +\tret = 0;\n> +\tgoto out;\n> \n> error:\n> \tdelete_tempfile(&refs->tempfile);\n> -\treturn -1;\n> +\n> +out:\n> +\tfree(packed_refs_path);\n> +\treturn ret;\n> }\n> \n> /*\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index 2bb4b25ed9..0d8a03e2a9 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '\n> \tgit -c core.packedrefstimeout=3000 pack-refs --all --prune\n> '\n> \n> +test_expect_success 'pack symlinked packed-refs' '\n\n(*) Does this need a SYMLINKS prereq to avoid trouble on Windows?\n\n> +\t# First make sure that symlinking works when reading:\n> +\tgit update-ref refs/heads/loosy refs/heads/master &&\n> +\tgit for-each-ref >all-refs-before &&\n> +\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n> +\tln -s my-deviant-packed-refs .git/packed-refs &&\n> +\tgit for-each-ref >all-refs-linked &&\n> +\ttest_cmp all-refs-before all-refs-linked &&\n> +\tgit pack-refs --all --prune &&\n> +\tgit for-each-ref >all-refs-packed &&\n> +\ttest_cmp all-refs-before all-refs-packed &&\n> +\ttest -h .git/packed-refs &&\n> +\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> +'\n> +\n> test_done\n"},{"id":"325181","messageId":"CAMy9T_HJ9LV2P6DYbdDpYXPjB9dAxG8EYopuMtiKgbu0-ar3Mg@mail.gmail.com","threadId":"46303","inReplyTo":"20170727001812.GI13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-27T11:12:42Z","receivedAt":"2017-07-27T11:12:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Stefan, Jonathan: you're both right, the new test needs the SYMLINKS\nprerequisite. Junio, would you mind adding that flag?\n\nThanks,\nMichael\n"},{"id":"325194","messageId":"xmqq8tj9okzv.fsf@gitster.mtv.corp.google.com","threadId":"46303","inReplyTo":"d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-27T17:19:48Z","receivedAt":"2017-07-27T17:20:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Change `commit_packed_refs()` to use `get_locked_file_path()` to find\n> the path of the file that it should overwrite. Since that path was\n> properly resolved when the lockfile was created, this restores the\n> pre-42dfa7ecef behavior.\n\nBecause when we take a lock hold_lock_file() eventually calls into\nlock_file() which by default takes the lock on the target of the\nsymbolic link (which is the sensible default, which is triggered in\nthis codepath), so this change to use the name of that file is all\nthat is needed.\n\nMakes sense. Makes me wonder why we need a separate .new file\n(instead of writing into the .lock instead), but that is a different\nissue.\n\nThanks. I'll do the SYMLINKS thing while queuing.\n\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index 2bb4b25ed9..0d8a03e2a9 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '\n> \tgit -c core.packedrefstimeout=3000 pack-refs --all --prune\n> '\n> \n> +test_expect_success 'pack symlinked packed-refs' '\n> +\t# First make sure that symlinking works when reading:\n> +\tgit update-ref refs/heads/loosy refs/heads/master &&\n> +\tgit for-each-ref >all-refs-before &&\n> +\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n> +\tln -s my-deviant-packed-refs .git/packed-refs &&\n> +\tgit for-each-ref >all-refs-linked &&\n> +\ttest_cmp all-refs-before all-refs-linked &&\n> +\tgit pack-refs --all --prune &&\n> +\tgit for-each-ref >all-refs-packed &&\n> +\ttest_cmp all-refs-before all-refs-packed &&\n> +\ttest -h .git/packed-refs &&\n> +\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> +'\n> +\n> test_done\n"},{"id":"325201","messageId":"20170727182854.wxjvjs2x746n3x2t@sigill.intra.peff.net","threadId":"46303","inReplyTo":"xmqq8tj9okzv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-27T18:28:54Z","receivedAt":"2017-07-27T18:29:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 27, 2017 at 10:19:48AM -0700, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > Change `commit_packed_refs()` to use `get_locked_file_path()` to find\n> > the path of the file that it should overwrite. Since that path was\n> > properly resolved when the lockfile was created, this restores the\n> > pre-42dfa7ecef behavior.\n> \n> Because when we take a lock hold_lock_file() eventually calls into\n> lock_file() which by default takes the lock on the target of the\n> symbolic link (which is the sensible default, which is triggered in\n> this codepath), so this change to use the name of that file is all\n> that is needed.\n> \n> Makes sense. Makes me wonder why we need a separate .new file\n> (instead of writing into the .lock instead), but that is a different\n> issue.\n\nIt comes from 42dfa7ece (commit_packed_refs(): use a staging file\nseparate from the lockfile, 2017-06-23). That commit explains that we\nwant to be able to put the new contents into service before we release\nthe lock. But it doesn't say why that's useful.\n\nI recall from past discussions that this will help close some races,\nand e5cc7d7d2 (repack_without_refs(): don't lock or unlock the packed\nrefs, 2017-07-01) alludes to this. I think the races in question have to\ndo with holding the packed-refs lock while pruning the just-packed\nfiles, but I'm having trouble digging up specifics in the archive.\n\n-Peff\n"},{"id":"325207","messageId":"xmqqlgn9mzww.fsf@gitster.mtv.corp.google.com","threadId":"46303","inReplyTo":"20170727182854.wxjvjs2x746n3x2t@sigill.intra.peff.net","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-27T19:40:31Z","receivedAt":"2017-07-27T19:40:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jul 27, 2017 at 10:19:48AM -0700, Junio C Hamano wrote:\n>\n>> Makes sense. Makes me wonder why we need a separate .new file\n>> (instead of writing into the .lock instead), but that is a different\n>> issue.\n>\n> It comes from 42dfa7ece (commit_packed_refs(): use a staging file\n> separate from the lockfile, 2017-06-23). That commit explains that we\n> want to be able to put the new contents into service before we release\n> the lock. But it doesn't say why that's useful.\n\nBy being able to hold the lock on packed-refs longer, I guess\nsomething like this becomes possible:\n\n * hold the lock on packed-refs\n * hold the lock on loose ref A, B, C, ...\n * update packed-refs to include the freshest values of these refs\n * start serving packed-refs without releasing the lock\n * for X in A, B, C...: delete the loose ref X and unlock X\n * unlock the packed-refs\n\nOther people racing with the sequence to recreate a loose ref that\nis even fresher than the resulting packed-refs file, while we still\nhold the lock on packed-refs, is perfectly OK.\n\nBut we must make sure our packed-refs is visible to others before\nstarting to delete and unlock the loose refs.\n\nHmph, but that is not a sufficient explanation. I am not seeing\nanything bad to happen if we unlock the packed-refs before deleting\nloose refs that we have locks on, so there must be something else\nthat needs \"new packed-refs is made visible way before we unlock it\".\n\n> I recall from past discussions that this will help close some races,\n> and e5cc7d7d2 (repack_without_refs(): don't lock or unlock the packed\n> refs, 2017-07-01) alludes to this. I think the races in question have to\n> do with holding the packed-refs lock while pruning the just-packed\n> files, but I'm having trouble digging up specifics in the archive.\n>\n> -Peff\n"},{"id":"325215","messageId":"CAMy9T_GP1SRigZHonC8PTPn4RoKGwa31ZTCT04C+d0ks6k8jxA@mail.gmail.com","threadId":"46303","inReplyTo":"xmqqlgn9mzww.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-07-28T06:07:22Z","receivedAt":"2017-07-28T06:07:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Thu, Jul 27, 2017 at 12:40 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Thu, Jul 27, 2017 at 10:19:48AM -0700, Junio C Hamano wrote:\n>>\n>>> Makes sense. Makes me wonder why we need a separate .new file\n>>> (instead of writing into the .lock instead), but that is a different\n>>> issue.\n>>\n>> It comes from 42dfa7ece (commit_packed_refs(): use a staging file\n>> separate from the lockfile, 2017-06-23). That commit explains that we\n>> want to be able to put the new contents into service before we release\n>> the lock. But it doesn't say why that's useful.\n>\n> By being able to hold the lock on packed-refs longer, I guess\n> something like this becomes possible:\n>\n> * hold the lock on packed-refs\n> * hold the lock on loose ref A, B, C, ...\n> * update packed-refs to include the freshest values of these refs\n> * start serving packed-refs without releasing the lock\n> * for X in A, B, C...: delete the loose ref X and unlock X\n> * unlock the packed-refs\n\nHmmm, I thought that I explained somewhere why this is useful, but I\ncan't find it now.\n\nThe code even after this patch series is\n\nPrepare:\n1. lock loose refs\nFinish:\n2. perform creates and updates of loose refs, releasing their locks as we go\n3. perform deletes of loose refs\n4. lock packed-refs file\n5. repack_without_refs() (delete packed versions of refs to be deleted)\n6. commit new packed-refs file (which also unlocks it)\n7. release locks on loose refs that were deleted\n\nThis is problematic because after a loose reference is deleted, it\nbriefly (between steps 3 and 5) reveals any packed version of the\nreference, which might even point at a commit that has been GCed. It\nis even worse if the attempt to acquire the packed refs lock in step 4\nfails, because then the repo could be left in that broken state.\n\nClearly we can avoid the second problem by acquiring the packed-refs\nlock during the prepare phase. You might also attempt to fix the first\nproblem by deleting the references from the packed-refs file before we\ndelete the corresponding loose references:\n\nPrepare:\n1. lock loose refs\n2. lock packed-refs file\nFinish:\n3. perform creates and updates of loose refs, releasing their locks as we go\n4. repack_without_refs() (delete packed versions of refs to be deleted)\n5. commit new packed-refs file (which also unlocks it)\n6. perform deletes of loose refs\n7. release locks on loose refs that were deleted\n\nBut now we have a different problem. pack-refs does the following:\n\nA. lock packed-refs file\nB. read loose refs\nC. commit new packed-refs file (which also unlocks it)\nThen, for each loose reference that should be pruned, delete it in a\ntransaction with REF_ISPRUNING, which means:\nD. lock the loose reference\nE. lock packed-refs file\nF. check that the reference's value is the same as the value just\nwritten to packed-refs\nG. delete the loose ref file if it still exists\nH. unlock the loose reference\n\nIf a reference deletion is running at the same time as a pack-refs,\nyou could have a race where steps A and B of a pack-refs run between\nsteps 5 and 6 of a reference deletion. The pack-refs would see the\nloose reference and would pack it into the packed-refs file, leaving\nthe reference at its old value even though the user was told that the\nreference was deleted.\n\nIf a new packed-refs file can be activated while retaining the lock on\nthe file, then a reference update could keep the packed-refs file\nlocked during step 6 of the second algorithm, which would block step A\nof pack-refs from beginning.\n\nI have the feeling that there might be other, more improbably races\nlurking here, too.\n\nThe eternal problems with races in the files backend is a main reason\nthat I'm enthusiastic about the reftable proposal.\n\nMichael\n"},{"id":"426010","messageId":"87y2bv0yvl.fsf@evledraar.gmail.com","threadId":"46303","inReplyTo":"d0da02a8b6f0272fa70ae3b1dc80fee6c6ee8d18.1501111803.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-31T14:18:46Z","receivedAt":"2021-05-31T15:29:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 26 2017, Michael Haggerty wrote:\n\n> [...]\n> +test_expect_success 'pack symlinked packed-refs' '\n> +\t# First make sure that symlinking works when reading:\n> +\tgit update-ref refs/heads/loosy refs/heads/master &&\n> +\tgit for-each-ref >all-refs-before &&\n> +\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n> +\tln -s my-deviant-packed-refs .git/packed-refs &&\n> +\tgit for-each-ref >all-refs-linked &&\n> +\ttest_cmp all-refs-before all-refs-linked &&\n> +\tgit pack-refs --all --prune &&\n> +\tgit for-each-ref >all-refs-packed &&\n> +\ttest_cmp all-refs-before all-refs-packed &&\n> +\ttest -h .git/packed-refs &&\n> +\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> +'\n\nFWIW this broke tests on AIX because we can't assume readlink(1) exists\nat all. See d2addc3b96 (t7800: readlink may not be available,\n2016-05-31) for a workaround.\n"},{"id":"426352","messageId":"YLkwCTcRT/9s8+5R@coredump.intra.peff.net","threadId":"46303","inReplyTo":"87y2bv0yvl.fsf@evledraar.gmail.com","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T19:39:53Z","receivedAt":"2021-06-03T19:39:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 31, 2021 at 04:18:46PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Wed, Jul 26 2017, Michael Haggerty wrote:\n> \n> > [...]\n> > +test_expect_success 'pack symlinked packed-refs' '\n> > +\t# First make sure that symlinking works when reading:\n> > +\tgit update-ref refs/heads/loosy refs/heads/master &&\n> > +\tgit for-each-ref >all-refs-before &&\n> > +\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n> > +\tln -s my-deviant-packed-refs .git/packed-refs &&\n> > +\tgit for-each-ref >all-refs-linked &&\n> > +\ttest_cmp all-refs-before all-refs-linked &&\n> > +\tgit pack-refs --all --prune &&\n> > +\tgit for-each-ref >all-refs-packed &&\n> > +\ttest_cmp all-refs-before all-refs-packed &&\n> > +\ttest -h .git/packed-refs &&\n> > +\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> > +'\n> \n> FWIW this broke tests on AIX because we can't assume readlink(1) exists\n> at all. See d2addc3b96 (t7800: readlink may not be available,\n> 2016-05-31) for a workaround.\n\nHmm. So obviously we can use a fix similar to the one in t7800 (though\nit's sufficiently complicated that I'd be tempted to wrap it in a helper\nfunction). There are a few other calls that could be changed, too.\n\nBut it's interesting to me that it sounds like the tests have been\nbroken on AIX for 4 years, and nobody noticed. I assume you ran into\nthis on the gcc build-farm machines. Our traditional approach for\nportability has been: if somebody is using the platform and cares enough\nto submit patches, then we'll support it. But testing on the build-farm\nmeans preemptively finding these problems, whether anyone actually cares\nabout AIX or not. :)\n\nI'm not really arguing either way here, just thinking out loud.\n\nPreemptively finding portability problems may save work in the long\nterm. And people may even be using Git on AIX and just ignoring test\nfailures, or they have GNU coreutils installed anyway, etc. But it would\nalso save work if we can ignore platforms that nobody uses.\n\n-Peff\n"},{"id":"426353","messageId":"YLk0Zm2J6VOA/lks@coredump.intra.peff.net","threadId":"46303","inReplyTo":"YLkwCTcRT/9s8+5R@coredump.intra.peff.net","subject":"[PATCH] t: use portable wrapper for readlink(1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T19:58:30Z","receivedAt":"2021-06-03T19:58:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:\n\n> > FWIW this broke tests on AIX because we can't assume readlink(1) exists\n> > at all. See d2addc3b96 (t7800: readlink may not be available,\n> > 2016-05-31) for a workaround.\n> \n> Hmm. So obviously we can use a fix similar to the one in t7800 (though\n> it's sufficiently complicated that I'd be tempted to wrap it in a helper\n> function). There are a few other calls that could be changed, too.\n\nHere's a patch to do that. Can you confirm that it fixes your test\nfailure?\n\n-- >8 --\nSubject: [PATCH] t: use portable wrapper for readlink(1)\n\nNot all systems have a readlink program available for use by the shell.\nThis causes t3210 to fail on at least AIX. Let's provide a perl\none-liner to do the same thing, and use it there.\n\nI also updated calls in t9802. Nobody reported failure there, but it's\nthe same issue. Presumably nobody actually tests with p4 on AIX in the\nfirst place (if it is even available there).\n\nI left the use of readlink in the \"--valgrind\" setup in test-lib.sh, as\nvalgrind isn't available on exotic platforms anyway (and I didn't want\nto increase dependencies between test-lib.sh and test-lib-functions.sh).\n\nThere's one other curious case. Commit d2addc3b96 (t7800: readlink may\nnot be available, 2016-05-31) fixed a similar case. We can't use our\nwrapper function there, though, as it's inside a sub-script triggered by\nGit. It uses a slightly different technique (\"ls\" piped to \"sed\"). I\nchose not to use that here as it gives confusing \"ls -l\" output if the\nfile is unexpectedly not a symlink (which is OK for its limited use, but\npotentially confusing for general use within the test suite). The perl\nversion emits the empty string.\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Jeff King \n---\n t/t3210-pack-refs.sh | 2 +-\n t/t9802-git-p4-filetype.sh | 4 ++--\n t/test-lib-functions.sh | 6 ++++++\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 3b7cdc56ec..577f32dc71 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '\n \tgit for-each-ref >all-refs-packed &&\n \ttest_cmp all-refs-before all-refs-packed &&\n \ttest -h .git/packed-refs &&\n-\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n+\ttest \"$(test_readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n '\n \n test_done\ndiff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh\nindex 94edebe272..19073c6e9f 100755\n--- a/t/t9802-git-p4-filetype.sh\n+++ b/t/t9802-git-p4-filetype.sh\n@@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '\n \t(\n \t\tcd \"$git\" &&\n \t\ttest -L symlink &&\n-\t\ttest $(readlink symlink) = symlink-target\n+\t\ttest $(test_readlink symlink) = symlink-target\n \t)\n '\n \n@@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '\n \tgit p4 clone --dest=\"$git\" //depot@all &&\n \t(\n \t\tcd \"$git\" &&\n-\t\ttest $(readlink empty-symlink) = target2\n+\t\ttest $(test_readlink empty-symlink) = target2\n \t)\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b823c14027..661f376077 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1692,3 +1692,9 @@ test_region () {\n \n \treturn 0\n }\n+\n+# Print the destination of symlink(s) provided as arguments. Basically\n+# the same as the readlink command, but it's not available everywhere.\n+test_readlink () {\n+\tperl -le 'print readlink($_) for @ARGV' \"$@\"\n+}\n-- \n2.32.0.rc3.525.gfa939c0632\n\n"},{"id":"426357","messageId":"60b93a264cb7_39da0420855@natae.notmuch","threadId":"46303","inReplyTo":"YLkwCTcRT/9s8+5R@coredump.intra.peff.net","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-03T20:23:02Z","receivedAt":"2021-06-03T20:24:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> Preemptively finding portability problems may save work in the long\n> term. And people may even be using Git on AIX and just ignoring test\n> failures, or they have GNU coreutils installed anyway, etc. But it would\n> also save work if we can ignore platforms that nobody uses.\n\nI agree, but the Git project is overly preoccupied (IMO) with\nhypothetical issues some hypothetical users might have in some\nhypothetical situations, and that is used as a rationale to block changes\nthat would improve the experience of the vast majority of users.\n\nThis is not a hypothetical issue, and yet you are suggesting to\ndiscount it?\n\nI don't disagree, but this is not consistent.\n\nEither we preoccupy ourselves with obscure issues that will probably not\naffect anyone in the real world, or we don't.\n\nIn my opinion it's perfectly sensible to let a few potential issues\nslide by, and only worry seriously about them when a real user reports a\nreal problem.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"426359","messageId":"YLlEx5qlN7rgehaP@coredump.intra.peff.net","threadId":"46303","inReplyTo":"60b93a264cb7_39da0420855@natae.notmuch","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-03T21:08:23Z","receivedAt":"2021-06-03T21:08:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 03, 2021 at 03:23:02PM -0500, Felipe Contreras wrote:\n\n> Jeff King wrote:\n> > Preemptively finding portability problems may save work in the long\n> > term. And people may even be using Git on AIX and just ignoring test\n> > failures, or they have GNU coreutils installed anyway, etc. But it would\n> > also save work if we can ignore platforms that nobody uses.\n> \n> I agree, but the Git project is overly preoccupied (IMO) with\n> hypothetical issues some hypothetical users might have in some\n> hypothetical situations, and that is used as a rationale to block changes\n> that would improve the experience of the vast majority of users.\n> \n> This is not a hypothetical issue, and yet you are suggesting to\n> discount it?\n> \n> I don't disagree, but this is not consistent.\n\nI don't think they're the same issue at all. One is: we have millions of\nusers, and this change may affect some of them negatively, so we may\nwant to err on the side of caution. The other is: this has been\naccidentally broken for four years and nobody complained, so perhaps\nnobody is actually using it.\n\nYou may also note that I did in fact produce a patch.\n\n-Peff\n"},{"id":"426367","messageId":"60b956da36dad_39f8372088d@natae.notmuch","threadId":"46303","inReplyTo":"YLlEx5qlN7rgehaP@coredump.intra.peff.net","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-03T22:25:30Z","receivedAt":"2021-06-03T22:25:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Thu, Jun 03, 2021 at 03:23:02PM -0500, Felipe Contreras wrote:\n> \n> > Jeff King wrote:\n> > > Preemptively finding portability problems may save work in the long\n> > > term. And people may even be using Git on AIX and just ignoring test\n> > > failures, or they have GNU coreutils installed anyway, etc. But it would\n> > > also save work if we can ignore platforms that nobody uses.\n> > \n> > I agree, but the Git project is overly preoccupied (IMO) with\n> > hypothetical issues some hypothetical users might have in some\n> > hypothetical situations, and that is used as a rationale to block changes\n> > that would improve the experience of the vast majority of users.\n> > \n> > This is not a hypothetical issue, and yet you are suggesting to\n> > discount it?\n> > \n> > I don't disagree, but this is not consistent.\n> \n> I don't think they're the same issue at all.\n\n> One is: we have millions of users, and this change may affect some of\n> them negatively, so we may want to err on the side of caution.\n\nThis language may sound nice, but it doesn't match the behavior, which is\nmuch more akin to \"this change may affect some of them negatively,\ntherefore we must tune it forever so it never actually happens\".\n\nFew people would object to erring on the side of caution.\n\n> The other is: this has been accidentally broken for four years and\n> nobody complained, so perhaps nobody is actually using it.\n\nYes, maybe. Or maybe they haven't reported it yet.\n\nEither way I was confusing this issue with another one. This one does\nseem much less serious than I was thinking.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"426442","messageId":"87r1hh5o97.fsf@evledraar.gmail.com","threadId":"46303","inReplyTo":"YLk0Zm2J6VOA/lks@coredump.intra.peff.net","subject":"Re: [PATCH] t: use portable wrapper for readlink(1)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-04T21:09:28Z","receivedAt":"2021-06-04T21:12:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 03 2021, Jeff King wrote:\n\n> On Thu, Jun 03, 2021 at 03:39:53PM -0400, Jeff King wrote:\n>\n>> > FWIW this broke tests on AIX because we can't assume readlink(1) exists\n>> > at all. See d2addc3b96 (t7800: readlink may not be available,\n>> > 2016-05-31) for a workaround.\n>> \n>> Hmm. So obviously we can use a fix similar to the one in t7800 (though\n>> it's sufficiently complicated that I'd be tempted to wrap it in a helper\n>> function). There are a few other calls that could be changed, too.\n>\n> Here's a patch to do that. Can you confirm that it fixes your test\n> failure?\n\nIt does, thanks, t3210*.sh is broken on current master on AIX, fixed\nwith this patch. I don't have p4 on that machine (or any box I hack on),\nso I can't test t9802*.sh.\n\n> -- >8 --\n> Subject: [PATCH] t: use portable wrapper for readlink(1)\n>\n> Not all systems have a readlink program available for use by the shell.\n> This causes t3210 to fail on at least AIX. Let's provide a perl\n> one-liner to do the same thing, and use it there.\n>\n> I also updated calls in t9802. Nobody reported failure there, but it's\n> the same issue. Presumably nobody actually tests with p4 on AIX in the\n> first place (if it is even available there).\n>\n> I left the use of readlink in the \"--valgrind\" setup in test-lib.sh, as\n> valgrind isn't available on exotic platforms anyway (and I didn't want\n> to increase dependencies between test-lib.sh and test-lib-functions.sh).\n>\n> There's one other curious case. Commit d2addc3b96 (t7800: readlink may\n> not be available, 2016-05-31) fixed a similar case. We can't use our\n> wrapper function there, though, as it's inside a sub-script triggered by\n> Git. It uses a slightly different technique (\"ls\" piped to \"sed\"). I\n> chose not to use that here as it gives confusing \"ls -l\" output if the\n> file is unexpectedly not a symlink (which is OK for its limited use, but\n> potentially confusing for general use within the test suite). The perl\n> version emits the empty string.\n>\n> Reported-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Jeff King \n> ---\n> t/t3210-pack-refs.sh | 2 +-\n> t/t9802-git-p4-filetype.sh | 4 ++--\n> t/test-lib-functions.sh | 6 ++++++\n> 3 files changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index 3b7cdc56ec..577f32dc71 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -253,7 +253,7 @@ test_expect_success SYMLINKS 'pack symlinked packed-refs' '\n> \tgit for-each-ref >all-refs-packed &&\n> \ttest_cmp all-refs-before all-refs-packed &&\n> \ttest -h .git/packed-refs &&\n> -\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> +\ttest \"$(test_readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n> '\n> \n> test_done\n> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh\n> index 94edebe272..19073c6e9f 100755\n> --- a/t/t9802-git-p4-filetype.sh\n> +++ b/t/t9802-git-p4-filetype.sh\n> @@ -263,7 +263,7 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '\n> \t(\n> \t\tcd \"$git\" &&\n> \t\ttest -L symlink &&\n> -\t\ttest $(readlink symlink) = symlink-target\n> +\t\ttest $(test_readlink symlink) = symlink-target\n> \t)\n> '\n> \n> @@ -329,7 +329,7 @@ test_expect_success SYMLINKS 'empty symlink target' '\n> \tgit p4 clone --dest=\"$git\" //depot@all &&\n> \t(\n> \t\tcd \"$git\" &&\n> -\t\ttest $(readlink empty-symlink) = target2\n> +\t\ttest $(test_readlink empty-symlink) = target2\n> \t)\n> '\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b823c14027..661f376077 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1692,3 +1692,9 @@ test_region () {\n> \n> \treturn 0\n> }\n> +\n> +# Print the destination of symlink(s) provided as arguments. Basically\n> +# the same as the readlink command, but it's not available everywhere.\n> +test_readlink () {\n> +\tperl -le 'print readlink($_) for @ARGV' \"$@\"\n> +}\n\n"},{"id":"426445","messageId":"87o8cl5n31.fsf@evledraar.gmail.com","threadId":"46303","inReplyTo":"YLkwCTcRT/9s8+5R@coredump.intra.peff.net","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-04T21:12:06Z","receivedAt":"2021-06-04T21:36:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 03 2021, Jeff King wrote:\n\n> On Mon, May 31, 2021 at 04:18:46PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> \n>> On Wed, Jul 26 2017, Michael Haggerty wrote:\n>> \n>> > [...]\n>> > +test_expect_success 'pack symlinked packed-refs' '\n>> > +\t# First make sure that symlinking works when reading:\n>> > +\tgit update-ref refs/heads/loosy refs/heads/master &&\n>> > +\tgit for-each-ref >all-refs-before &&\n>> > +\tmv .git/packed-refs .git/my-deviant-packed-refs &&\n>> > +\tln -s my-deviant-packed-refs .git/packed-refs &&\n>> > +\tgit for-each-ref >all-refs-linked &&\n>> > +\ttest_cmp all-refs-before all-refs-linked &&\n>> > +\tgit pack-refs --all --prune &&\n>> > +\tgit for-each-ref >all-refs-packed &&\n>> > +\ttest_cmp all-refs-before all-refs-packed &&\n>> > +\ttest -h .git/packed-refs &&\n>> > +\ttest \"$(readlink .git/packed-refs)\" = \"my-deviant-packed-refs\"\n>> > +'\n>> \n>> FWIW this broke tests on AIX because we can't assume readlink(1) exists\n>> at all. See d2addc3b96 (t7800: readlink may not be available,\n>> 2016-05-31) for a workaround.\n>\n> Hmm. So obviously we can use a fix similar to the one in t7800 (though\n> it's sufficiently complicated that I'd be tempted to wrap it in a helper\n> function). There are a few other calls that could be changed, too.\n>\n> But it's interesting to me that it sounds like the tests have been\n> broken on AIX for 4 years, and nobody noticed. I assume you ran into\n> this on the gcc build-farm machines. Our traditional approach for\n> portability has been: if somebody is using the platform and cares enough\n> to submit patches, then we'll support it. But testing on the build-farm\n> means preemptively finding these problems, whether anyone actually cares\n> about AIX or not. :)\n>\n> I'm not really arguing either way here, just thinking out loud.\n>\n> Preemptively finding portability problems may save work in the long\n> term. And people may even be using Git on AIX and just ignoring test\n> failures, or they have GNU coreutils installed anyway, etc. But it would\n> also save work if we can ignore platforms that nobody uses.\n\nI believe that the main packager for AIX is Michael Felt (CC'd). He's\noccasionally posted here on-list, e.g. at [1], the package is at [2].\n\nI think in the grand scheme of things if we break things completely for\nIBM xlc and/or Oracle's C compiler git won't be that much worse\noff. Plenty of third party packages for those platforms simply use the\nfull GNU toolchain.\n\nFrom memory the most commonly used package for Solaris is built with the\nfull GNU toolchain, so in that sense portability to that platform isn't\na practical issue.\n\nI just send patches for these as part of doing rc-phase testing on the\nGCC farm to see if anything's broken since the last release. Issues we\nhave on OSX, Linux, Windows etc. tend to be resolved already, which\nleaves more obscure issues on these platforms.\n\nI've said before that I do think that porting to \"obscure\" platforms\nlike those has some value in itself, even now there's e.g. C compiler\nwarnings on those proprietary compilers that cover blind spots that\nneither gcc nor clang have. Often they're false alarms, but there's the\noccasional gold nuggets in there.\n\nE.g. I have some post-release patches queued up for fixing various cmd_*\nfunctions of ours to use \"return\" instead of \"exit(code)\". Even though\nthey return \"int\" neither GCC nor Clang complain about the early exit,\nbut SunCC does. As a result we don't run the usual teardown in git.c for\nsome built-ins.\n\nI was trying to get AIX in particular to the point of passing 100% a\nwhile ago, but sort of lost steam with Junio not wanting to take my\npatches at [3].\n\nWithout those re-running the tests usually takes manual intervention on\nthe box, so and since my usual test setup is more of a fire-and-forget\naffair I mostly stopped bothering with the AIX box as a result...\n\n1. https://lore.kernel.org/git/1200106e-b75d-5b15-0608-427cd923578a@felt.demon.nl/\n2. http://www.aixtools.net/index.php/git\n3. https://lore.kernel.org/git/patch-1.6-3330cdbccc0-20210329T161723Z-avarab@gmail.com/\n"},{"id":"426447","messageId":"87lf7p5msj.fsf@evledraar.gmail.com","threadId":"46303","inReplyTo":"YLlEx5qlN7rgehaP@coredump.intra.peff.net","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-04T21:37:22Z","receivedAt":"2021-06-04T21:44:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 03 2021, Jeff King wrote:\n\n> On Thu, Jun 03, 2021 at 03:23:02PM -0500, Felipe Contreras wrote:\n>\n>> Jeff King wrote:\n>> > Preemptively finding portability problems may save work in the long\n>> > term. And people may even be using Git on AIX and just ignoring test\n>> > failures, or they have GNU coreutils installed anyway, etc. But it would\n>> > also save work if we can ignore platforms that nobody uses.\n>> \n>> I agree, but the Git project is overly preoccupied (IMO) with\n>> hypothetical issues some hypothetical users might have in some\n>> hypothetical situations, and that is used as a rationale to block changes\n>> that would improve the experience of the vast majority of users.\n>> \n>> This is not a hypothetical issue, and yet you are suggesting to\n>> discount it?\n>> \n>> I don't disagree, but this is not consistent.\n>\n> I don't think they're the same issue at all. One is: we have millions of\n> users, and this change may affect some of them negatively, so we may\n> want to err on the side of caution. The other is: this has been\n> accidentally broken for four years and nobody complained, so perhaps\n> nobody is actually using it.\n\nAside from AIX I think you're assuming less of a cowboy attitude among\npackagers of these platforms than is the reality in the wild :)\n\nI mean I don't blame them, git's just one thing they're packaging, and\ne.g. on the BSD's or whatever this is just the Nth\nLinux-toolchain-specific smelling test failure or issue they have that\nday.\n\nI have some incomplete work somewhere to slurp up all the package\nsources I could find in the wild (SRPM's, Debian recipies etc.) and\ntheir patches, the aim was to submit it into contrib/ so we could see\nwhat monkeypatches to git.git existed in the wild.\n\nLast time I looked at those (and this is from memory, and it was a while\nago) many of those patches / build recipies were simply blindly skipping\nor otherwise working around test failures.\n\nSo we can't assume that failures in the wild are reported to us, and I\nthink many packagers are not running any of our tests at all. If it\ncompiles and seems to work they're probably just shipping it.\n"},{"id":"426450","messageId":"CAMP44s2FrZkbPbi+vFxD1dLfZPtKeJ2QZk5Gk0HgQjMgE_3Rbg@mail.gmail.com","threadId":"46303","inReplyTo":"87lf7p5msj.fsf@evledraar.gmail.com","subject":"Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-05T01:07:22Z","receivedAt":"2021-06-05T01:07:47Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Jun 4, 2021 at 4:42 PM Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Jun 03 2021, Jeff King wrote:\n\n> I have some incomplete work somewhere to slurp up all the package\n> sources I could find in the wild (SRPM's, Debian recipies etc.) and\n> their patches, the aim was to submit it into contrib/ so we could see\n> what monkeypatches to git.git existed in the wild.\n>\n> Last time I looked at those (and this is from memory, and it was a while\n> ago) many of those patches / build recipies were simply blindly skipping\n> or otherwise working around test failures.\n>\n> So we can't assume that failures in the wild are reported to us, and I\n> think many packagers are not running any of our tests at all. If it\n> compiles and seems to work they're probably just shipping it.\n\nArch Linux boasts of following upstream as closely as possible, I\nlooked at their build instructions for the umpteenth time to see if I\ncould find anything interesting. While they don't have patches, they\ndo have NO_SVN_TESTS=y (plus a bunch of `cp contrib/*`).\n\nSo I don't think any packager can just blindly trust the Git project\nto do the sensible thing here. Who uses subversion anymore? And who\nthat still uses subversion wants or needs git-svn? And why would\nfailing any of these break the release?\n\nMaybe we need some \"test-packager\" target that limits the tests to\nsomething sensible. Ideally it should be the other way around:\npackagers run `make test`, and developers `make test-developer`. But\nthat's a bigger discussion.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}