{"thread":{"id":"33507","subject":"[PATCH 00/33] Various cleanups around reference packing and peeling","startedAt":"2013-04-14T12:54:15Z","lastAt":"2013-04-25T18:13:49Z","messageCount":64,"participants":["Michael Haggerty","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":33},"messages":[{"id":"214172","messageId":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":null,"subject":"[PATCH 00/33] Various cleanups around reference packing and peeling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:15Z","receivedAt":"2013-04-14T12:54:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The highlight of this patch series are that (1) peeled references are\nnot lost from the packed-refs file when a packed reference is deleted\nand (2) there is more code sharing between the code used by \"git\npacked-refs\" and the code used by repack_without_ref() for deleting a\npacked reference. Along the way I have added a lot of documentation\nand fixed several smaller bugs.\n\nI had to add some sleeps in test t3210 but I hope that somebody can\nfind a way to eliminate them.\n\nThe last patch implements a change that I brushed off a long time ago\nwhen Heiko suggested it. He was right (probably not for performance\nreasons, but because it simplifies the code).\n\nSummary:\n\nPatches 1-4 document existing code.\n\nPatches 5-6 random cleanup.\n\nPatches 7-9 change the semantics of get_packed_ref() to make it more\nreusable and change other locations to use this function.\n\nPatch 10 extracts a function ref_resolves_to_object().\n\nPatches 11-14 unify reference-peeling in two new functions,\npeel_object() and peel_entry() (and fix an inconsistency in\npeel_ref()).\n\nPatch 15 introduces a new internal reference-iteration API and adjusts\nsome internal code to use it. The new API gives the callback function\ndirect access to the ref_entry. This corrects the handling of\ncurrent_ref during an iteration over only packed references.\n\nPatches 16-17 add and fix a test of a spurious error message.\n\nPatches 18-22 change the reference-deleting code to write peeled refs\nto the packed-refs file. (Previously, whenever a packed reference was\ndeleted, all of the peeled values were lost when the packed-refs file\nwas rewritten.)\n\nPatches 23-31 move the code from pack-refs.{c,h} into refs.{c,h},\nadjust it to its new surroundings, and change it to share some code\nwith repack_without_ref().\n\nPatches 32-33 simplify access to the main reference cache.\n\nMichael Haggerty (33):\n refs: document flags constants REF_*\n refs: document the fields of struct ref_value\n refs: document do_for_each_ref() and do_one_ref()\n refs: document how current_ref is used\n refs: define constant PEELED_LINE_LENGTH\n do_for_each_ref_in_dirs(): remove dead code\n get_packed_ref(): return a ref_entry\n peel_ref(): use function get_packed_ref()\n repack_without_ref(): use function get_packed_ref()\n refs: extract a function ref_resolves_to_object()\n refs: extract function peel_object()\n peel_object(): give more specific information in return value\n peel_ref(): fix return value for non-peelable, not-current reference\n refs: extract a function peel_entry()\n refs: change the internal reference-iteration API\n t3210: test for spurious error messages for dangling packed refs\n repack_without_ref(): silence errors for dangling packed refs\n search_ref_dir(): return an index rather than a pointer\n refs: change how packed refs are deleted\n t3211: demonstrate loss of peeled refs if a packed ref is deleted\n repack_without_ref(): write peeled refs in the rewritten file\n refs: extract a function write_packed_entry()\n pack-refs: rename handle_one_ref() to pack_one_ref()\n pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}\n pack_one_ref(): rename \"path\" parameter to \"refname\"\n refs: use same lock_file object for both ref-packing functions\n pack_refs(): change to use do_for_each_entry()\n refs: inline function do_not_prune()\n pack_one_ref(): use function peel_entry()\n pack_one_ref(): use write_packed_entry() to do the writing\n pack_one_ref(): do some cheap tests before a more expensive one\n refs: change do_for_each_*() functions to take ref_cache arguments\n refs: handle the main ref_cache specially\n\n Makefile | 2 -\n builtin/clone.c | 1 -\n builtin/pack-refs.c | 2 +-\n pack-refs.c | 148 -----------\n pack-refs.h | 18 --\n refs.c | 699 ++++++++++++++++++++++++++++++++++++++-------------\n refs.h | 35 +++\n t/t3210-pack-refs.sh | 36 +++\n t/t3211-peel-ref.sh | 9 +\n 9 files changed, 609 insertions(+), 341 deletions(-)\n delete mode 100644 pack-refs.c\n delete mode 100644 pack-refs.h\n\n-- \n1.8.2.1\n"},{"id":"214197","messageId":"1365944088-10588-2-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/33] refs: document flags constants REF_*","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:16Z","receivedAt":"2013-04-14T12:54:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the bits that can appear in the \"flags\" parameter passed to\nan each_ref_function and/or in the ref_entry::flag field.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 +++++++++++-\n refs.h | 13 +++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex de2d8eb..30b4bf7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -158,7 +158,17 @@ struct ref_dir {\n \tstruct ref_entry **entries;\n };\n \n-/* ISSYMREF=0x01, ISPACKED=0x02, and ISBROKEN=0x04 are public interfaces */\n+/*\n+ * Bit values for ref_entry::flag. REF_ISSYMREF=0x01,\n+ * REF_ISPACKED=0x02, and REF_ISBROKEN=0x04 are public values; see\n+ * refs.h.\n+ */\n+\n+/*\n+ * The field ref_entry->u.value.peeled of this value entry contains\n+ * the correct peeled value for the reference, which might be\n+ * null_sha1 if the reference is not a tag or if it is broken.\n+ */\n #define REF_KNOWS_PEELED 0x08\n \n /* ref_entry represents a directory of references */\ndiff --git a/refs.h b/refs.h\nindex a35eafc..17bc1c1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -10,8 +10,21 @@ struct ref_lock {\n \tint force_write;\n };\n \n+/*\n+ * Bit values set in the flags argument passed to each_ref_fn():\n+ */\n+\n+/* Reference is a symbolic reference. */\n #define REF_ISSYMREF 0x01\n+\n+/* Reference is a packed reference. */\n #define REF_ISPACKED 0x02\n+\n+/*\n+ * Reference cannot be resolved to an object name: dangling symbolic\n+ * reference (directly or indirectly), corrupt reference file, or\n+ * symbolic reference refers to ill-formatted reference name.\n+ */\n #define REF_ISBROKEN 0x04\n \n /*\n-- \n1.8.2.1\n"},{"id":"214173","messageId":"1365944088-10588-3-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/33] refs: document the fields of struct ref_value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:17Z","receivedAt":"2013-04-14T12:54:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 30b4bf7..1df1ccd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -109,7 +109,19 @@ struct ref_entry;\n * (ref_entry->flag & REF_DIR) is zero.\n */\n struct ref_value {\n+\t/*\n+\t * The name of the object to which this reference resolves\n+\t * (which may be a tag object). If REF_ISBROKEN, this is\n+\t * null. If REF_ISSYMREF, then this is the name of the object\n+\t * referred to by the last reference in the symlink chain.\n+\t */\n \tunsigned char sha1[20];\n+\n+\t/*\n+\t * If REF_KNOWS_PEELED, then this field holds the peeled value\n+\t * of this reference, or null if the reference is known not to\n+\t * be peelable.\n+\t */\n \tunsigned char peeled[20];\n };\n \n-- \n1.8.2.1\n"},{"id":"214201","messageId":"1365944088-10588-4-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/33] refs: document do_for_each_ref() and do_one_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:18Z","receivedAt":"2013-04-14T12:54:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1df1ccd..f503ec4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -525,10 +525,14 @@ static void sort_ref_dir(struct ref_dir *dir)\n \tdir->sorted = dir->nr = i;\n }\n \n-#define DO_FOR_EACH_INCLUDE_BROKEN 01\n+/* Include broken references in a do_for_each_ref*() iteration: */\n+#define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n \n static struct ref_entry *current_ref;\n \n+/*\n+ * Handle one reference in a do_for_each_ref*()-style iteration.\n+ */\n static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n \t\t int flags, void *cb_data, struct ref_entry *entry)\n {\n@@ -1338,6 +1342,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n \tfor_each_rawref(warn_if_dangling_symref, &data);\n }\n \n+/*\n+ * Call fn for each reference in the specified submodule for which the\n+ * refname begins with base. If trim is non-zero, then trim that many\n+ * characters off the beginning of each refname before passing the\n+ * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n+ * broken references in the iteration.\n+ */\n static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n \t\t\t int trim, int flags, void *cb_data)\n {\n-- \n1.8.2.1\n"},{"id":"214199","messageId":"1365944088-10588-5-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/33] refs: document how current_ref is used","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:19Z","receivedAt":"2013-04-14T12:54:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex f503ec4..88ff153 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -528,6 +528,15 @@ static void sort_ref_dir(struct ref_dir *dir)\n /* Include broken references in a do_for_each_ref*() iteration: */\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n \n+/*\n+ * current_ref is a performance hack: when iterating over references\n+ * using the for_each_ref*() functions, current_ref is set to the\n+ * current reference's entry before calling the callback function. If\n+ * the callback function calls peel_ref(), then peel_ref() first\n+ * checks whether the reference to be peeled is the current reference\n+ * (it usually is) and if so, returns that reference's peeled version\n+ * if it is available. This avoids a refname lookup in a common case.\n+ */\n static struct ref_entry *current_ref;\n \n /*\n-- \n1.8.2.1\n"},{"id":"214198","messageId":"1365944088-10588-6-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/33] refs: define constant PEELED_LINE_LENGTH","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:20Z","receivedAt":"2013-04-14T12:54:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 88ff153..7cbf228 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -805,6 +805,9 @@ void invalidate_ref_cache(const char *submodule)\n \tclear_loose_ref_cache(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 * 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@@ -897,8 +900,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t\t}\n \t\tif (last &&\n \t\t refline[0] == '^' &&\n-\t\t strlen(refline) == 42 &&\n-\t\t refline[41] == '\\n' &&\n+\t\t strlen(refline) == PEELED_LINE_LENGTH &&\n+\t\t refline[PEELED_LINE_LENGTH - 1] == '\\n' &&\n \t\t !get_sha1_hex(refline + 1, sha1)) {\n \t\t\thashcpy(last->u.value.peeled, sha1);\n \t\t\t/*\n-- \n1.8.2.1\n"},{"id":"214196","messageId":"1365944088-10588-7-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/33] do_for_each_ref_in_dirs(): remove dead code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:21Z","receivedAt":"2013-04-14T12:54:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no way to drop out of the while loop. This code has been\ndead since 432ad41e.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7cbf228..9f508dd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -666,13 +666,6 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,\n \t\tif (retval)\n \t\t\treturn retval;\n \t}\n-\tif (i1 < dir1->nr)\n-\t\treturn do_for_each_ref_in_dir(dir1, i1,\n-\t\t\t\t\t base, fn, trim, flags, cb_data);\n-\tif (i2 < dir2->nr)\n-\t\treturn do_for_each_ref_in_dir(dir2, i2,\n-\t\t\t\t\t base, fn, trim, flags, cb_data);\n-\treturn 0;\n }\n \n /*\n-- \n1.8.2.1\n"},{"id":"214192","messageId":"1365944088-10588-8-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/33] get_packed_ref(): return a ref_entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:22Z","receivedAt":"2013-04-14T12:54:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of copying the reference's SHA1 into a caller-supplied\nvariable, just return the ref_entry itself (or NULL if there is no\nsuch entry). This change will allow the function to be used from\nelsewhere.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 20 +++++++++-----------\n 1 file changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9f508dd..de5dc7d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1100,18 +1100,12 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh\n }\n \n /*\n- * Try to read ref from the packed references. On success, set sha1\n- * and return 0; otherwise, return -1.\n+ * Return the ref_entry for the given refname from the packed\n+ * references. If it does not exist, return NULL.\n */\n-static int get_packed_ref(const char *refname, unsigned char *sha1)\n+static struct ref_entry *get_packed_ref(const char *refname)\n {\n-\tstruct ref_dir *packed = get_packed_refs(get_ref_cache(NULL));\n-\tstruct ref_entry *entry = find_ref(packed, refname);\n-\tif (entry) {\n-\t\thashcpy(sha1, entry->u.value.sha1);\n-\t\treturn 0;\n-\t}\n-\treturn -1;\n+\treturn find_ref(get_packed_refs(get_ref_cache(NULL)), refname);\n }\n \n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n@@ -1139,13 +1133,17 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n \t\tif (lstat(path, &st) < 0) {\n+\t\t\tstruct ref_entry *entry;\n+\n \t\t\tif (errno != ENOENT)\n \t\t\t\treturn NULL;\n \t\t\t/*\n \t\t\t * The loose reference file does not exist;\n \t\t\t * check for a packed reference.\n \t\t\t */\n-\t\t\tif (!get_packed_ref(refname, sha1)) {\n+\t\t\tentry = get_packed_ref(refname);\n+\t\t\tif (entry) {\n+\t\t\t\thashcpy(sha1, entry->u.value.sha1);\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISPACKED;\n \t\t\t\treturn refname;\n-- \n1.8.2.1\n"},{"id":"214174","messageId":"1365944088-10588-9-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/33] peel_ref(): use function get_packed_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:23Z","receivedAt":"2013-04-14T12:54:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex de5dc7d..cf017a5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1282,10 +1282,9 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\treturn -1;\n \n \tif ((flag & REF_ISPACKED)) {\n-\t\tstruct ref_dir *dir = get_packed_refs(get_ref_cache(NULL));\n-\t\tstruct ref_entry *r = find_ref(dir, refname);\n+\t\tstruct ref_entry *r = get_packed_ref(refname);\n \n-\t\tif (r != NULL && r->flag & REF_KNOWS_PEELED) {\n+\t\tif (r && (r->flag & REF_KNOWS_PEELED)) {\n \t\t\thashcpy(sha1, r->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n-- \n1.8.2.1\n"},{"id":"214200","messageId":"1365944088-10588-10-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/33] repack_without_ref(): use function get_packed_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:24Z","receivedAt":"2013-04-14T12:54:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cf017a5..c523978 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1818,9 +1818,11 @@ static int repack_without_ref(const char *refname)\n {\n \tstruct repack_without_ref_sb data;\n \tstruct ref_cache *refs = get_ref_cache(NULL);\n-\tstruct ref_dir *packed = get_packed_refs(refs);\n-\tif (find_ref(packed, refname) == NULL)\n-\t\treturn 0;\n+\tstruct ref_dir *packed;\n+\n+\tif (!get_packed_ref(refname))\n+\t\treturn 0; /* refname does not exist in packed refs */\n+\n \tdata.refname = refname;\n \tdata.fd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n \tif (data.fd < 0) {\n-- \n1.8.2.1\n"},{"id":"214202","messageId":"1365944088-10588-11-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/33] refs: extract a function ref_resolves_to_object()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:25Z","receivedAt":"2013-04-14T12:54:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is a nice unit of work and soon will be needed from multiple\nlocations.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 28 ++++++++++++++++++++--------\n 1 file changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c523978..dfc8600 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n \n /*\n+ * Return true iff the reference described by entry can be resolved to\n+ * an object in the database. Emit a warning if the referred-to\n+ * object does not exist.\n+ */\n+static int ref_resolves_to_object(struct ref_entry *entry)\n+{\n+\tif (entry->flag & REF_ISBROKEN)\n+\t\treturn 0;\n+\tif (!has_sha1_file(entry->u.value.sha1)) {\n+\t\terror(\"%s does not point to a valid object!\", entry->name);\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+/*\n * current_ref is a performance hack: when iterating over references\n * using the for_each_ref*() functions, current_ref is set to the\n * current reference's entry before calling the callback function. If\n@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n \tif (prefixcmp(entry->name, base))\n \t\treturn 0;\n \n-\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n-\t\tif (entry->flag & REF_ISBROKEN)\n-\t\t\treturn 0; /* ignore broken refs e.g. dangling symref */\n-\t\tif (!has_sha1_file(entry->u.value.sha1)) {\n-\t\t\terror(\"%s does not point to a valid object!\", entry->name);\n-\t\t\treturn 0;\n-\t\t}\n-\t}\n+\tif (!((flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n+\t ref_resolves_to_object(entry)))\n+\t\treturn 0;\n+\n \tcurrent_ref = entry;\n \tretval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);\n \tcurrent_ref = NULL;\n-- \n1.8.2.1\n"},{"id":"214195","messageId":"1365944088-10588-12-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/33] refs: extract function peel_object()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:26Z","receivedAt":"2013-04-14T12:54:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is a nice, logical unit of work, and putting it in a function\nremoves the need to use a goto in peel_ref(). Soon it will also have\nother uses.\n\nThe algorithm is unchanged.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 50 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 30 insertions(+), 20 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dfc8600..a1fe6b0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1272,11 +1272,38 @@ static int filter_refs(const char *refname, const unsigned char *sha1, int flags\n \treturn filter->fn(refname, sha1, flags, filter->cb_data);\n }\n \n+/*\n+ * Peel the named object; i.e., if the object is a tag, resolve the\n+ * tag recursively until a non-tag is found. Store the result to sha1\n+ * and return 0 iff successful. If the object is not a tag or is not\n+ * valid, return -1 and leave sha1 unchanged.\n+ */\n+static int peel_object(const unsigned char *name, unsigned char *sha1)\n+{\n+\tstruct object *o = lookup_unknown_object(name);\n+\n+\tif (o->type == OBJ_NONE) {\n+\t\tint type = sha1_object_info(name, NULL);\n+\t\tif (type < 0)\n+\t\t\treturn -1;\n+\t\to->type = type;\n+\t}\n+\n+\tif (o->type != OBJ_TAG)\n+\t\treturn -1;\n+\n+\to = deref_tag_noverify(o);\n+\tif (!o)\n+\t\treturn -1;\n+\n+\thashcpy(sha1, o->sha1);\n+\treturn 0;\n+}\n+\n int peel_ref(const char *refname, unsigned char *sha1)\n {\n \tint flag;\n \tunsigned char base[20];\n-\tstruct object *o;\n \n \tif (current_ref && (current_ref->name == refname\n \t\t|| !strcmp(current_ref->name, refname))) {\n@@ -1286,8 +1313,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\t\thashcpy(sha1, current_ref->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n-\t\thashcpy(base, current_ref->u.value.sha1);\n-\t\tgoto fallback;\n+\t\treturn peel_object(current_ref->u.value.sha1, sha1);\n \t}\n \n \tif (read_ref_full(refname, base, 1, &flag))\n@@ -1302,23 +1328,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\t}\n \t}\n \n-fallback:\n-\to = lookup_unknown_object(base);\n-\tif (o->type == OBJ_NONE) {\n-\t\tint type = sha1_object_info(base, NULL);\n-\t\tif (type < 0)\n-\t\t\treturn -1;\n-\t\to->type = type;\n-\t}\n-\n-\tif (o->type == OBJ_TAG) {\n-\t\to = deref_tag_noverify(o);\n-\t\tif (o) {\n-\t\t\thashcpy(sha1, o->sha1);\n-\t\t\treturn 0;\n-\t\t}\n-\t}\n-\treturn -1;\n+\treturn peel_object(base, sha1);\n }\n \n struct warn_if_dangling_data {\n-- \n1.8.2.1\n"},{"id":"214175","messageId":"1365944088-10588-13-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/33] peel_object(): give more specific information in return value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:27Z","receivedAt":"2013-04-14T12:54:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of just returning a success/failure bit, return an enumeration\nvalue that explains the reason for any failure. This will come in\nhandy shortly.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 32 ++++++++++++++++++++++++--------\n 1 file changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a1fe6b0..84c2497 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1272,32 +1272,48 @@ static int filter_refs(const char *refname, const unsigned char *sha1, int flags\n \treturn filter->fn(refname, sha1, flags, filter->cb_data);\n }\n \n+enum peel_status {\n+\t/* object was peeled successfully: */\n+\tPEEL_PEELED = 0,\n+\n+\t/*\n+\t * object cannot be peeled because the named object (or an\n+\t * object referred to by a tag in the peel chain), does not\n+\t * exist.\n+\t */\n+\tPEEL_INVALID = -1,\n+\n+\t/* object cannot be peeled because it is not a tag: */\n+\tPEEL_NON_TAG = -2,\n+};\n+\n /*\n * Peel the named object; i.e., if the object is a tag, resolve the\n- * tag recursively until a non-tag is found. Store the result to sha1\n- * and return 0 iff successful. If the object is not a tag or is not\n- * valid, return -1 and leave sha1 unchanged.\n+ * tag recursively until a non-tag is found. If successful, store the\n+ * result to sha1 and return PEEL_PEELED. If the object is not a tag\n+ * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively,\n+ * and leave sha1 unchanged.\n */\n-static int peel_object(const unsigned char *name, unsigned char *sha1)\n+static enum peel_status peel_object(const unsigned char *name, unsigned char *sha1)\n {\n \tstruct object *o = lookup_unknown_object(name);\n \n \tif (o->type == OBJ_NONE) {\n \t\tint type = sha1_object_info(name, NULL);\n \t\tif (type < 0)\n-\t\t\treturn -1;\n+\t\t\treturn PEEL_INVALID;\n \t\to->type = type;\n \t}\n \n \tif (o->type != OBJ_TAG)\n-\t\treturn -1;\n+\t\treturn PEEL_NON_TAG;\n \n \to = deref_tag_noverify(o);\n \tif (!o)\n-\t\treturn -1;\n+\t\treturn PEEL_INVALID;\n \n \thashcpy(sha1, o->sha1);\n-\treturn 0;\n+\treturn PEEL_PEELED;\n }\n \n int peel_ref(const char *refname, unsigned char *sha1)\n-- \n1.8.2.1\n"},{"id":"214194","messageId":"1365944088-10588-14-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/33] peel_ref(): fix return value for non-peelable, not-current reference","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:28Z","receivedAt":"2013-04-14T12:54:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old version was inconsistent: when a reference was\nREF_KNOWS_PEELED but with a null peeled value, it returned non-zero\nfor the current reference but zero for other references. Change the\nbehavior for non-current references to match that of current_ref,\nwhich is what callers expect. Document the behavior.\n\nCurrent callers did not trigger the previously-buggy behavior.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 ++++-\n refs.h | 8 ++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 84c2497..a0d8e32 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -120,7 +120,8 @@ struct ref_value {\n \t/*\n \t * If REF_KNOWS_PEELED, then this field holds the peeled value\n \t * of this reference, or null if the reference is known not to\n-\t * be peelable.\n+\t * be peelable. See the documentation for peel_ref() for an\n+\t * exact definition of \"peelable\".\n \t */\n \tunsigned char peeled[20];\n };\n@@ -1339,6 +1340,8 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\tstruct ref_entry *r = get_packed_ref(refname);\n \n \t\tif (r && (r->flag & REF_KNOWS_PEELED)) {\n+\t\t\tif (is_null_sha1(r->u.value.peeled))\n+\t\t\t return -1;\n \t\t\thashcpy(sha1, r->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex 17bc1c1..ac0ff92 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -74,6 +74,14 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n \n extern int ref_exists(const char *);\n \n+/*\n+ * If refname is a non-symbolic reference that refers to a tag object,\n+ * and the tag can be (recursively) dereferenced to a non-tag object,\n+ * store the SHA1 of the referred-to object to sha1 and return 0. If\n+ * any of these conditions are not met, return a non-zero value.\n+ * Symbolic references are considered unpeelable, even if they\n+ * ultimately resolve to a peelable tag.\n+ */\n extern int peel_ref(const char *refname, unsigned char *sha1);\n \n /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n-- \n1.8.2.1\n"},{"id":"214177","messageId":"1365944088-10588-15-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/33] refs: extract a function peel_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:29Z","receivedAt":"2013-04-14T12:54:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Peel the entry, and as a side effect store the peeled value in the\nentry. Use this function from two places in peel_ref(); a third\ncaller will be added soon.\n\nPlease note that this change can lead to ref_entries for unpacked refs\nbeing peeled. This has no practical benefit but is harmless.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 48 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a0d8e32..8cc6109 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1286,6 +1286,16 @@ enum peel_status {\n \n \t/* object cannot be peeled because it is not a tag: */\n \tPEEL_NON_TAG = -2,\n+\n+\t/* ref_entry contains no peeled value because it is a symref: */\n+\tPEEL_IS_SYMREF = -3,\n+\n+\t/*\n+\t * ref_entry cannot be peeled because it is broken (i.e., the\n+\t * symbolic reference cannot even be resolved to an object\n+\t * name):\n+\t */\n+\tPEEL_BROKEN = -4,\n };\n \n /*\n@@ -1317,31 +1327,56 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n \treturn PEEL_PEELED;\n }\n \n+/*\n+ * Peel the entry (if possible) and return its new peel_status.\n+ */\n+static enum peel_status peel_entry(struct ref_entry *entry)\n+{\n+\tenum peel_status status;\n+\n+\tif (entry->flag & REF_KNOWS_PEELED)\n+\t\treturn is_null_sha1(entry->u.value.peeled) ?\n+\t\t\tPEEL_NON_TAG : PEEL_PEELED;\n+\tif (entry->flag & REF_ISBROKEN)\n+\t\treturn PEEL_BROKEN;\n+\tif (entry->flag & REF_ISSYMREF)\n+\t\treturn PEEL_IS_SYMREF;\n+\n+\tstatus = peel_object(entry->u.value.sha1, entry->u.value.peeled);\n+\tif (status == PEEL_PEELED || status == PEEL_NON_TAG)\n+\t\tentry->flag |= REF_KNOWS_PEELED;\n+\treturn status;\n+}\n+\n int peel_ref(const char *refname, unsigned char *sha1)\n {\n \tint flag;\n \tunsigned char base[20];\n \n \tif (current_ref && (current_ref->name == refname\n-\t\t|| !strcmp(current_ref->name, refname))) {\n-\t\tif (current_ref->flag & REF_KNOWS_PEELED) {\n-\t\t\tif (is_null_sha1(current_ref->u.value.peeled))\n-\t\t\t return -1;\n-\t\t\thashcpy(sha1, current_ref->u.value.peeled);\n-\t\t\treturn 0;\n-\t\t}\n-\t\treturn peel_object(current_ref->u.value.sha1, sha1);\n+\t\t\t || !strcmp(current_ref->name, refname))) {\n+\t\tif (peel_entry(current_ref))\n+\t\t\treturn -1;\n+\t\thashcpy(sha1, current_ref->u.value.peeled);\n+\t\treturn 0;\n \t}\n \n \tif (read_ref_full(refname, base, 1, &flag))\n \t\treturn -1;\n \n-\tif ((flag & REF_ISPACKED)) {\n+\t/*\n+\t * If the reference is packed, read its ref_entry from the\n+\t * cache in the hope that we already know its peeled value.\n+\t * We only try this optimization on packed references because\n+\t * (a) forcing the filling of the loose reference cache could\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 = get_packed_ref(refname);\n-\n-\t\tif (r && (r->flag & REF_KNOWS_PEELED)) {\n-\t\t\tif (is_null_sha1(r->u.value.peeled))\n-\t\t\t return -1;\n+\t\tif (r) {\n+\t\t\tif (peel_entry(r))\n+\t\t\t\treturn -1;\n \t\t\thashcpy(sha1, r->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n-- \n1.8.2.1\n"},{"id":"214176","messageId":"1365944088-10588-16-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 15/33] refs: change the internal reference-iteration API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:30Z","receivedAt":"2013-04-14T12:54:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Establish an internal API for iterating over references, which gives\nthe callback functions direct access to the ref_entry structure\ndescribing the reference. (Do not change the iteration API that is\nexposed outside of the module.)\n\nDefine a new internal callback signature\n\n int each_ref_entry_fn(struct ref_entry *entry, void *cb_data)\n\nChange do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to\naccept each_ref_entry_fn callbacks, and rename them to\ndo_for_each_entry_in_dir() and do_for_each_entry_in_dirs(),\nrespectively. Adapt their callers accordingly.\n\nAdd a new function do_for_each_entry() analogous to do_for_each_ref()\nbut using the new callback style.\n\nChange do_one_ref() into an each_ref_entry_fn that does some\nbookkeeping and then calls a wrapped each_ref_fn.\n\nReimplement do_for_each_ref() in terms of do_for_each_entry(), using\ndo_one_ref() as an adapter.\n\nPlease note that the responsibility for setting current_ref remains in\ndo_one_ref(), which means that current_ref is *not* set when iterating\nover references via the new internal API. This is not a disadvantage,\nbecause current_ref is not needed by callers of the internal API (they\nreceive a pointer to the current ref_entry anyway). But more\nimportantly, this change prevents peel_ref() from returning invalid\nresults in the following scenario:\n\nWhen iterating via the external API, the iteration always includes\nboth packed and loose references, and in particular never presents a\npacked ref if there is a loose ref with the same name. The internal\nAPI, on the other hand, gives the option to iterate over only the\npacked references. During such an iteration, there is no check\nwhether the packed ref might be hidden by a loose ref of the same\nname. But until now the packed ref was recorded in current_ref during\nthe iteration. So if peel_ref() were called with the reference name\ncorresponding to current ref, it would return the peeled version of\nthe packed ref even though there might be a loose ref that peels to a\ndifferent value. This scenario doesn't currently occur in the code,\nbut fix it to prevent things from breaking in a very confusing way in\nthe future.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 142 +++++++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 81 insertions(+), 61 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8cc6109..51f68d3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -556,22 +556,34 @@ static int ref_resolves_to_object(struct ref_entry *entry)\n */\n static struct ref_entry *current_ref;\n \n+typedef int each_ref_entry_fn(struct ref_entry *entry, void *cb_data);\n+\n+struct ref_entry_cb {\n+\tconst char *base;\n+\tint trim;\n+\tint flags;\n+\teach_ref_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n /*\n- * Handle one reference in a do_for_each_ref*()-style iteration.\n+ * Handle one reference in a do_for_each_ref*()-style iteration,\n+ * calling an each_ref_fn for each entry.\n */\n-static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n-\t\t int flags, void *cb_data, struct ref_entry *entry)\n+static int do_one_ref(struct ref_entry *entry, void *cb_data)\n {\n+\tstruct ref_entry_cb *data = cb_data;\n \tint retval;\n-\tif (prefixcmp(entry->name, base))\n+\tif (prefixcmp(entry->name, data->base))\n \t\treturn 0;\n \n-\tif (!((flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n+\tif (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n \t ref_resolves_to_object(entry)))\n \t\treturn 0;\n \n \tcurrent_ref = entry;\n-\tretval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);\n+\tretval = data->fn(entry->name + data->trim, entry->u.value.sha1,\n+\t\t\t entry->flag, data->cb_data);\n \tcurrent_ref = NULL;\n \treturn retval;\n }\n@@ -580,11 +592,11 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n * Call fn for each reference in dir that has index in the range\n * offset <= index < dir->nr. Recurse into subdirectories that are in\n * that index range, sorting them before iterating. This function\n- * does not sort dir itself; it should be sorted beforehand.\n+ * does not sort dir itself; it should be sorted beforehand. fn is\n+ * called for all references, including broken ones.\n */\n-static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,\n-\t\t\t\t const char *base,\n-\t\t\t\t each_ref_fn fn, int trim, int flags, void *cb_data)\n+static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n \tint i;\n \tassert(dir->sorted == dir->nr);\n@@ -594,10 +606,9 @@ static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,\n \t\tif (entry->flag & REF_DIR) {\n \t\t\tstruct ref_dir *subdir = get_ref_dir(entry);\n \t\t\tsort_ref_dir(subdir);\n-\t\t\tretval = do_for_each_ref_in_dir(subdir, 0,\n-\t\t\t\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\t\tretval = do_for_each_entry_in_dir(subdir, 0, fn, cb_data);\n \t\t} else {\n-\t\t\tretval = do_one_ref(base, fn, trim, flags, cb_data, entry);\n+\t\t\tretval = fn(entry, cb_data);\n \t\t}\n \t\tif (retval)\n \t\t\treturn retval;\n@@ -610,12 +621,12 @@ static int do_for_each_ref_in_dir(struct ref_dir *dir, int offset,\n * by refname. Recurse into subdirectories. If a value entry appears\n * in both dir1 and dir2, then only process the version that is in\n * dir2. The input dirs must already be sorted, but subdirs will be\n- * sorted as needed.\n+ * sorted as needed. fn is called for all references, including\n+ * broken ones.\n */\n-static int do_for_each_ref_in_dirs(struct ref_dir *dir1,\n-\t\t\t\t struct ref_dir *dir2,\n-\t\t\t\t const char *base, each_ref_fn fn, int trim,\n-\t\t\t\t int flags, void *cb_data)\n+static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n+\t\t\t\t struct ref_dir *dir2,\n+\t\t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n \tint retval;\n \tint i1 = 0, i2 = 0;\n@@ -626,12 +637,10 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,\n \t\tstruct ref_entry *e1, *e2;\n \t\tint cmp;\n \t\tif (i1 == dir1->nr) {\n-\t\t\treturn do_for_each_ref_in_dir(dir2, i2,\n-\t\t\t\t\t\t base, fn, trim, flags, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir2, i2, fn, cb_data);\n \t\t}\n \t\tif (i2 == dir2->nr) {\n-\t\t\treturn do_for_each_ref_in_dir(dir1, i1,\n-\t\t\t\t\t\t base, fn, trim, flags, cb_data);\n+\t\t\treturn do_for_each_entry_in_dir(dir1, i1, fn, cb_data);\n \t\t}\n \t\te1 = dir1->entries[i1];\n \t\te2 = dir2->entries[i2];\n@@ -643,14 +652,13 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,\n \t\t\t\tstruct ref_dir *subdir2 = get_ref_dir(e2);\n \t\t\t\tsort_ref_dir(subdir1);\n \t\t\t\tsort_ref_dir(subdir2);\n-\t\t\t\tretval = do_for_each_ref_in_dirs(\n-\t\t\t\t\t\tsubdir1, subdir2,\n-\t\t\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\t\t\tretval = do_for_each_entry_in_dirs(\n+\t\t\t\t\t\tsubdir1, subdir2, fn, cb_data);\n \t\t\t\ti1++;\n \t\t\t\ti2++;\n \t\t\t} else if (!(e1->flag & REF_DIR) && !(e2->flag & REF_DIR)) {\n \t\t\t\t/* Both are references; ignore the one from dir1. */\n-\t\t\t\tretval = do_one_ref(base, fn, trim, flags, cb_data, e2);\n+\t\t\t\tretval = fn(e2, cb_data);\n \t\t\t\ti1++;\n \t\t\t\ti2++;\n \t\t\t} else {\n@@ -669,11 +677,10 @@ static int do_for_each_ref_in_dirs(struct ref_dir *dir1,\n \t\t\tif (e->flag & REF_DIR) {\n \t\t\t\tstruct ref_dir *subdir = get_ref_dir(e);\n \t\t\t\tsort_ref_dir(subdir);\n-\t\t\t\tretval = do_for_each_ref_in_dir(\n-\t\t\t\t\t\tsubdir, 0,\n-\t\t\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\t\t\tretval = do_for_each_entry_in_dir(\n+\t\t\t\t\t\tsubdir, 0, fn, cb_data);\n \t\t\t} else {\n-\t\t\t\tretval = do_one_ref(base, fn, trim, flags, cb_data, e);\n+\t\t\t\tretval = fn(e, cb_data);\n \t\t\t}\n \t\t}\n \t\tif (retval)\n@@ -702,14 +709,13 @@ struct name_conflict_cb {\n \tconst char *conflicting_refname;\n };\n \n-static int name_conflict_fn(const char *existingrefname, const unsigned char *sha1,\n-\t\t\t int flags, void *cb_data)\n+static int name_conflict_fn(struct ref_entry *entry, void *cb_data)\n {\n \tstruct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;\n-\tif (data->oldrefname && !strcmp(data->oldrefname, existingrefname))\n+\tif (data->oldrefname && !strcmp(data->oldrefname, entry->name))\n \t\treturn 0;\n-\tif (names_conflict(data->refname, existingrefname)) {\n-\t\tdata->conflicting_refname = existingrefname;\n+\tif (names_conflict(data->refname, entry->name)) {\n+\t\tdata->conflicting_refname = entry->name;\n \t\treturn 1;\n \t}\n \treturn 0;\n@@ -717,7 +723,7 @@ static int name_conflict_fn(const char *existingrefname, const unsigned char *sh\n \n /*\n * Return true iff a reference named refname could be created without\n- * conflicting with the name of an existing reference in array. If\n+ * conflicting with the name of an existing reference in dir. If\n * oldrefname is non-NULL, ignore potential conflicts with oldrefname\n * (e.g., because oldrefname is scheduled for deletion in the same\n * operation).\n@@ -731,9 +737,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n \tdata.conflicting_refname = NULL;\n \n \tsort_ref_dir(dir);\n-\tif (do_for_each_ref_in_dir(dir, 0, \"\", name_conflict_fn,\n-\t\t\t\t 0, DO_FOR_EACH_INCLUDE_BROKEN,\n-\t\t\t\t &data)) {\n+\tif (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {\n \t\terror(\"'%s' exists; cannot create '%s'\",\n \t\t data.conflicting_refname, refname);\n \t\treturn 0;\n@@ -1421,14 +1425,12 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n }\n \n /*\n- * Call fn for each reference in the specified submodule for which the\n- * refname begins with base. If trim is non-zero, then trim that many\n- * characters off the beginning of each refname before passing the\n- * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n- * broken references in the iteration.\n+ * Call fn for each reference in the specified submodule, omitting\n+ * references not in the containing_dir of base. fn is called for all\n+ * references, including broken ones.\n */\n-static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n-\t\t\t int trim, int flags, void *cb_data)\n+static int do_for_each_entry(const char *submodule, const char *base,\n+\t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n \tstruct ref_cache *refs = get_ref_cache(submodule);\n \tstruct ref_dir *packed_dir = get_packed_refs(refs);\n@@ -1443,24 +1445,41 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn\n \tif (packed_dir && loose_dir) {\n \t\tsort_ref_dir(packed_dir);\n \t\tsort_ref_dir(loose_dir);\n-\t\tretval = do_for_each_ref_in_dirs(\n-\t\t\t\tpacked_dir, loose_dir,\n-\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\tretval = do_for_each_entry_in_dirs(\n+\t\t\t\tpacked_dir, loose_dir, fn, cb_data);\n \t} else if (packed_dir) {\n \t\tsort_ref_dir(packed_dir);\n-\t\tretval = do_for_each_ref_in_dir(\n-\t\t\t\tpacked_dir, 0,\n-\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\tretval = do_for_each_entry_in_dir(\n+\t\t\t\tpacked_dir, 0, fn, cb_data);\n \t} else if (loose_dir) {\n \t\tsort_ref_dir(loose_dir);\n-\t\tretval = do_for_each_ref_in_dir(\n-\t\t\t\tloose_dir, 0,\n-\t\t\t\tbase, fn, trim, flags, cb_data);\n+\t\tretval = do_for_each_entry_in_dir(\n+\t\t\t\tloose_dir, 0, fn, cb_data);\n \t}\n \n \treturn retval;\n }\n \n+/*\n+ * Call fn for each reference in the specified submodule for which the\n+ * refname begins with base. If trim is non-zero, then trim that many\n+ * characters off the beginning of each refname before passing the\n+ * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n+ * broken references in the iteration.\n+ */\n+static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n+\t\t\t int trim, int flags, void *cb_data)\n+{\n+\tstruct ref_entry_cb data;\n+\tdata.base = base;\n+\tdata.trim = trim;\n+\tdata.flags = flags;\n+\tdata.fn = fn;\n+\tdata.cb_data = cb_data;\n+\n+\treturn do_for_each_entry(submodule, base, do_one_ref, &data);\n+}\n+\n static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n {\n \tunsigned char sha1[20];\n@@ -1870,20 +1889,21 @@ struct repack_without_ref_sb {\n \tint fd;\n };\n \n-static int repack_without_ref_fn(const char *refname, const unsigned char *sha1,\n-\t\t\t\t int flags, void *cb_data)\n+static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)\n {\n \tstruct repack_without_ref_sb *data = cb_data;\n \tchar line[PATH_MAX + 100];\n \tint len;\n \n-\tif (!strcmp(data->refname, refname))\n+\tif (!strcmp(data->refname, entry->name))\n \t\treturn 0;\n+\tif (!ref_resolves_to_object(entry))\n+\t\treturn 0; /* Skip broken refs */\n \tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n-\t\t sha1_to_hex(sha1), refname);\n+\t\t sha1_to_hex(entry->u.value.sha1), entry->name);\n \t/* this should not happen but just being defensive */\n \tif (len > sizeof(line))\n-\t\tdie(\"too long a refname '%s'\", refname);\n+\t\tdie(\"too long a refname '%s'\", entry->name);\n \twrite_or_die(data->fd, line, len);\n \treturn 0;\n }\n@@ -1907,7 +1927,7 @@ static int repack_without_ref(const char *refname)\n \t}\n \tclear_packed_ref_cache(refs);\n \tpacked = get_packed_refs(refs);\n-\tdo_for_each_ref_in_dir(packed, 0, \"\", repack_without_ref_fn, 0, 0, &data);\n+\tdo_for_each_entry_in_dir(packed, 0, repack_without_ref_fn, &data);\n \treturn commit_lock_file(&packlock);\n }\n \n-- \n1.8.2.1\n"},{"id":"214204","messageId":"1365944088-10588-17-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:31Z","receivedAt":"2013-04-14T12:54:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"A packed reference can be overridden by a loose reference, in which\ncase the packed reference is obsolete and is never used. The object\npointed to by such a reference can be garbage collected. Since\nd66da478f2, this could lead to the emission of a spurious error\nmessage:\n\n error: refs/heads/master does not point to a valid object!\n\nThe error is generated by repack_without_ref() if there is an obsolete\ndangling packed reference in packed-refs when the packed-refs file has\nto be rewritten due to the deletion of another packed reference. Add\na failing test demonstrating this problem and some passing tests of\nrelated scenarios.\n\nSigned-off-by: Michael Haggerty \n---\n\nHow can I get rid of the sleeps in these tests? I couldn't find\nanother portable way of reliably ensuring that commits get\ngarbage-collected. (Some other tests use similar code without the\nsleeps, but empirically the sleeps are necessary to get the tests to\nwork reliably. Perhaps the other tests are not doing what they\nthink.)\n\n t/t3210-pack-refs.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex cd04361..c032d88 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -118,4 +118,40 @@ test_expect_success 'pack, prune and repack' '\n \ttest_cmp all-of-them again\n '\n \n+test_expect_success 'explicit pack-refs with dangling packed reference' '\n+\tgit commit --allow-empty -m \"soon to be garbage-collected\" &&\n+\tgit pack-refs --all &&\n+\tgit reset --hard HEAD^ &&\n+\tsleep 1 &&\n+\tgit reflog expire --expire=now --all &&\n+\tgit prune --expire=now &&\n+\tgit pack-refs --all 2>result &&\n+\ttest_cmp /dev/null result\n+'\n+\n+test_expect_success 'delete ref with dangling packed version' '\n+\tgit checkout -b lamb &&\n+\tgit commit --allow-empty -m \"future garbage\" &&\n+\tgit pack-refs --all &&\n+\tgit reset --hard HEAD^ &&\n+\tgit checkout master &&\n+\tsleep 1 &&\n+\tgit reflog expire --expire=now --all &&\n+\tgit prune --expire=now &&\n+\tgit branch -d lamb 2>result &&\n+\ttest_cmp /dev/null result\n+'\n+\n+test_expect_failure 'delete ref while another dangling packed ref' '\n+\tgit branch lamb &&\n+\tgit commit --allow-empty -m \"future garbage\" &&\n+\tgit pack-refs --all &&\n+\tgit reset --hard HEAD^ &&\n+\tsleep 1 &&\n+\tgit reflog expire --expire=now --all &&\n+\tgit prune --expire=now &&\n+\tgit branch -d lamb 2>result &&\n+\ttest_cmp /dev/null result\n+'\n+\n test_done\n-- \n1.8.2.1\n"},{"id":"214178","messageId":"1365944088-10588-18-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 17/33] repack_without_ref(): silence errors for dangling packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:32Z","receivedAt":"2013-04-14T12:54:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Stop emitting an error message for dangling packed references found\nwhen deleting another packed reference. See the previous commit for a\nlonger explanation of the issue.\n\nChange repack_without_ref_fn() to silently ignore dangling packed\nreferences.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 17 ++++++++++-------\n t/t3210-pack-refs.sh | 2 +-\n 2 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 51f68d3..eadbc2a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -531,15 +531,17 @@ static void sort_ref_dir(struct ref_dir *dir)\n \n /*\n * Return true iff the reference described by entry can be resolved to\n- * an object in the database. Emit a warning if the referred-to\n- * object does not exist.\n+ * an object in the database. If report_errors is true, emit a\n+ * warning if the referred-to object does not exist.\n */\n-static int ref_resolves_to_object(struct ref_entry *entry)\n+static int ref_resolves_to_object(struct ref_entry *entry, int report_errors)\n {\n \tif (entry->flag & REF_ISBROKEN)\n \t\treturn 0;\n \tif (!has_sha1_file(entry->u.value.sha1)) {\n-\t\terror(\"%s does not point to a valid object!\", entry->name);\n+\t\tif (report_errors)\n+\t\t\terror(\"%s does not point to a valid object!\",\n+\t\t\t entry->name);\n \t\treturn 0;\n \t}\n \treturn 1;\n@@ -578,7 +580,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n \t\treturn 0;\n \n \tif (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n-\t ref_resolves_to_object(entry)))\n+\t ref_resolves_to_object(entry, 1)))\n \t\treturn 0;\n \n \tcurrent_ref = entry;\n@@ -1897,8 +1899,9 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)\n \n \tif (!strcmp(data->refname, entry->name))\n \t\treturn 0;\n-\tif (!ref_resolves_to_object(entry))\n-\t\treturn 0; /* Skip broken refs */\n+\t/* Silently skip broken refs: */\n+\tif (!ref_resolves_to_object(entry, 0))\n+\t\treturn 0;\n \tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n \t\t sha1_to_hex(entry->u.value.sha1), entry->name);\n \t/* this should not happen but just being defensive */\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex c032d88..559f602 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -142,7 +142,7 @@ test_expect_success 'delete ref with dangling packed version' '\n \ttest_cmp /dev/null result\n '\n \n-test_expect_failure 'delete ref while another dangling packed ref' '\n+test_expect_success 'delete ref while another dangling packed ref' '\n \tgit branch lamb &&\n \tgit commit --allow-empty -m \"future garbage\" &&\n \tgit pack-refs --all &&\n-- \n1.8.2.1\n"},{"id":"214193","messageId":"1365944088-10588-19-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 18/33] search_ref_dir(): return an index rather than a pointer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:33Z","receivedAt":"2013-04-14T12:54:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change search_ref_dir() to return the index of the sought entry (or -1\non error) rather than a pointer to the entry. This will make it more\nnatural to use the function for removing an entry from the list.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex eadbc2a..0c0668b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -366,18 +366,17 @@ static int ref_entry_cmp_sslice(const void *key_, const void *ent_)\n }\n \n /*\n- * Return the entry with the given refname from the ref_dir\n- * (non-recursively), sorting dir if necessary. Return NULL if no\n- * such entry is found. dir must already be complete.\n+ * Return the index of the entry with the given refname from the\n+ * ref_dir (non-recursively), sorting dir if necessary. Return -1 if\n+ * no such entry is found. dir must already be complete.\n */\n-static struct ref_entry *search_ref_dir(struct ref_dir *dir,\n-\t\t\t\t\tconst char *refname, size_t len)\n+static int search_ref_dir(struct ref_dir *dir, const char *refname, size_t len)\n {\n \tstruct ref_entry **r;\n \tstruct string_slice key;\n \n \tif (refname == NULL || !dir->nr)\n-\t\treturn NULL;\n+\t\treturn -1;\n \n \tsort_ref_dir(dir);\n \tkey.len = len;\n@@ -386,9 +385,9 @@ static struct ref_entry *search_ref_dir(struct ref_dir *dir,\n \t\t ref_entry_cmp_sslice);\n \n \tif (r == NULL)\n-\t\treturn NULL;\n+\t\treturn -1;\n \n-\treturn *r;\n+\treturn r - dir->entries;\n }\n \n /*\n@@ -402,8 +401,9 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,\n \t\t\t\t\t const char *subdirname, size_t len,\n \t\t\t\t\t int mkdir)\n {\n-\tstruct ref_entry *entry = search_ref_dir(dir, subdirname, len);\n-\tif (!entry) {\n+\tint entry_index = search_ref_dir(dir, subdirname, len);\n+\tstruct ref_entry *entry;\n+\tif (entry_index == -1) {\n \t\tif (!mkdir)\n \t\t\treturn NULL;\n \t\t/*\n@@ -414,6 +414,8 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,\n \t\t */\n \t\tentry = create_dir_entry(dir->ref_cache, subdirname, len, 0);\n \t\tadd_entry_to_dir(dir, entry);\n+\t} else {\n+\t\tentry = dir->entries[entry_index];\n \t}\n \treturn get_ref_dir(entry);\n }\n@@ -452,12 +454,16 @@ static struct ref_dir *find_containing_dir(struct ref_dir *dir,\n */\n static struct ref_entry *find_ref(struct ref_dir *dir, const char *refname)\n {\n+\tint entry_index;\n \tstruct ref_entry *entry;\n \tdir = find_containing_dir(dir, refname, 0);\n \tif (!dir)\n \t\treturn NULL;\n-\tentry = search_ref_dir(dir, refname, strlen(refname));\n-\treturn (entry && !(entry->flag & REF_DIR)) ? entry : NULL;\n+\tentry_index = search_ref_dir(dir, refname, strlen(refname));\n+\tif (entry_index == -1)\n+\t\treturn NULL;\n+\tentry = dir->entries[entry_index];\n+\treturn (entry->flag & REF_DIR) ? NULL : entry;\n }\n \n /*\n-- \n1.8.2.1\n"},{"id":"214179","messageId":"1365944088-10588-20-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 19/33] refs: change how packed refs are deleted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:34Z","receivedAt":"2013-04-14T12:54:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a function remove_ref(), which removes a single entry from a\nreference cache.\n\nUse this function to reimplement repack_without_ref(). The old\nversion iterated over all refs, packing all of them except for the one\nto be deleted, then discarded the entire packed reference cache. The\nnew version deletes the doomed reference from the cache *before*\niterating.\n\nThis has two advantages:\n\n* the code for writing packed-refs becomes simpler, because it doesn't\n have to exclude one of the references.\n\n* it is no longer necessary to discard the packed refs cache after\n deleting a reference: symbolic refs cannot be packed, so packed\n references cannot depend on each other, so the rest of the packed\n refs cache remains valid after a reference is deleted.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 68 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0c0668b..3c20853 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -467,6 +467,57 @@ static struct ref_entry *find_ref(struct ref_dir *dir, const char *refname)\n }\n \n /*\n+ * Remove the entry with the given name from dir, recursing into\n+ * subdirectories as necessary. If refname is the name of a directory\n+ * (i.e., ends with '/'), then remove the directory and its contents.\n+ * If the removal was successful, return the number of entries\n+ * remaining in the directory entry that contained the deleted entry.\n+ * If the name was not found, return -1. Please note that this\n+ * function only deletes the entry from the cache; it does not delete\n+ * it from the filesystem or ensure that other cache entries (which\n+ * might be symbolic references to the removed entry) are updated.\n+ * Nor does it remove any containing dir entries that might be made\n+ * empty by the removal. dir must represent the top-level directory\n+ * and must already be complete.\n+ */\n+static int remove_entry(struct ref_dir *dir, const char *refname)\n+{\n+\tint refname_len = strlen(refname);\n+\tint entry_index;\n+\tstruct ref_entry *entry;\n+\tint is_dir = refname[refname_len - 1] == '/';\n+\tif (is_dir) {\n+\t\t/*\n+\t\t * refname represents a reference directory. Remove\n+\t\t * the trailing slash; otherwise we will get the\n+\t\t * directory *representing* refname rather than the\n+\t\t * one *containing* it.\n+\t\t */\n+\t\tchar *dirname = xmemdupz(refname, refname_len - 1);\n+\t\tdir = find_containing_dir(dir, dirname, 0);\n+\t\tfree(dirname);\n+\t} else {\n+\t\tdir = find_containing_dir(dir, refname, 0);\n+\t}\n+\tif (!dir)\n+\t\treturn -1;\n+\tentry_index = search_ref_dir(dir, refname, refname_len);\n+\tif (entry_index == -1)\n+\t\treturn -1;\n+\tentry = dir->entries[entry_index];\n+\n+\tmemmove(&dir->entries[entry_index],\n+\t\t&dir->entries[entry_index + 1],\n+\t\t(dir->nr - entry_index - 1) * sizeof(*dir->entries)\n+\t\t);\n+\tdir->nr--;\n+\tif (dir->sorted > entry_index)\n+\t\tdir->sorted--;\n+\tfree_ref_entry(entry);\n+\treturn dir->nr;\n+}\n+\n+/*\n * Add a ref_entry to the ref_dir (unsorted), recursing into\n * subdirectories as necessary. dir must represent the top-level\n * directory. Return 0 on success.\n@@ -1892,19 +1943,12 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, NULL);\n }\n \n-struct repack_without_ref_sb {\n-\tconst char *refname;\n-\tint fd;\n-};\n-\n-static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)\n+static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tstruct repack_without_ref_sb *data = cb_data;\n+\tint *fd = cb_data;\n \tchar line[PATH_MAX + 100];\n \tint len;\n \n-\tif (!strcmp(data->refname, entry->name))\n-\t\treturn 0;\n \t/* Silently skip broken refs: */\n \tif (!ref_resolves_to_object(entry, 0))\n \t\treturn 0;\n@@ -1913,7 +1957,7 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)\n \t/* this should not happen but just being defensive */\n \tif (len > sizeof(line))\n \t\tdie(\"too long a refname '%s'\", entry->name);\n-\twrite_or_die(data->fd, line, len);\n+\twrite_or_die(*fd, line, len);\n \treturn 0;\n }\n \n@@ -1921,22 +1965,30 @@ static struct lock_file packlock;\n \n static int repack_without_ref(const char *refname)\n {\n-\tstruct repack_without_ref_sb data;\n+\tint fd;\n \tstruct ref_cache *refs = get_ref_cache(NULL);\n \tstruct ref_dir *packed;\n \n \tif (!get_packed_ref(refname))\n \t\treturn 0; /* refname does not exist in packed refs */\n \n-\tdata.refname = refname;\n-\tdata.fd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n-\tif (data.fd < 0) {\n+\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n+\tif (fd < 0) {\n \t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n \t}\n \tclear_packed_ref_cache(refs);\n \tpacked = get_packed_refs(refs);\n-\tdo_for_each_entry_in_dir(packed, 0, repack_without_ref_fn, &data);\n+\t/* Remove refname from the cache. */\n+\tif (remove_entry(packed, refname) == -1) {\n+\t\t/*\n+\t\t * The packed entry disappeared while we were\n+\t\t * acquiring the lock.\n+\t\t */\n+\t\trollback_lock_file(&packlock);\n+\t\treturn 0;\n+\t}\n+\tdo_for_each_entry_in_dir(packed, 0, repack_ref_fn, &fd);\n \treturn commit_lock_file(&packlock);\n }\n \n@@ -1965,7 +2017,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(lock->ref_name);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_ref_cache(NULL);\n+\tclear_loose_ref_cache(get_ref_cache(NULL));\n \tunlock_ref(lock);\n \treturn ret;\n }\n-- \n1.8.2.1\n"},{"id":"214181","messageId":"1365944088-10588-21-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 20/33] t3211: demonstrate loss of peeled refs if a packed ref is deleted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:35Z","receivedAt":"2013-04-14T12:54:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a test that demonstrates that the peeled values recorded in\npacked-refs are lost if a packed ref is deleted. (The code in\nrepack_without_ref() doesn't even attempt to write peeled refs.) This\nwill be fixed in a moment.\n\nSigned-off-by: Michael Haggerty \n---\n t/t3211-peel-ref.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nindex d4d7792..cca1acb 100755\n--- a/t/t3211-peel-ref.sh\n+++ b/t/t3211-peel-ref.sh\n@@ -61,4 +61,13 @@ test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'peeled refs survive deletion of packed ref' '\n+\tgit pack-refs --all &&\n+\tcp .git/packed-refs fully-peeled &&\n+\tgit branch yadda &&\n+\tgit pack-refs --all &&\n+\tgit branch -d yadda &&\n+\ttest_cmp fully-peeled .git/packed-refs\n+'\n+\n test_done\n-- \n1.8.2.1\n"},{"id":"214182","messageId":"1365944088-10588-22-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 21/33] repack_without_ref(): write peeled refs in the rewritten file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:36Z","receivedAt":"2013-04-14T12:54:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When a reference that existed in the packed-refs file is deleted, the\npacked-refs file must be rewritten. Previously, the file was\nrewritten without any peeled refs, even if the file contained peeled\nrefs when it was read. This was not a bug, because the packed-refs\nfile header didn't claim that the file contained peeled values. But\nit had a performance cost, because the repository would lose the\nbenefit of having precomputed peeled references until pack-refs was\nrun again.\n\nTeach repack_without_ref() to write peeled refs to the packed-refs\nfile (regardless of whether they were present in the old version of\nthe file).\n\nThis means that if the old version of the packed-refs file was not\nfully peeled, then repack_without_ref() will have to peel references.\nTo avoid the expense of reading lots of loose references, we take two\nshortcuts relative to pack-refs:\n\n* If the peeled value of a reference is already known (i.e., because\n it was read from the old version of the packed-refs file), then\n output that peeled value again without any checks. This is the\n usual code path and should avoid any noticeable overhead. (This is\n different than pack-refs, which always re-peels references.)\n\n* We don't verify that the packed ref is still current. It could be\n that a packed references is overridden by a loose reference, in\n which case the packed ref is no longer needed and might even refer\n to an object that has been garbage collected. But we don't check;\n instead, we just try to peel all references. If peeling is\n successful, the peeled value is written out (even though it might\n not be needed any more); if not, then the reference is silently\n omitted from the output.\n\nThe extra overhead of peeling references in repack_without_ref()\nshould only be incurred the first time the packed-refs file is written\nby a version of Git that knows about the \"fully-peeled\" attribute.\n\nSigned-off-by: Michael Haggerty \n---\n\nThis change could cause a noticeable delay on deleting a reference if\nit causes a non-fully-peeled packed-refs file to be fully peeled. But\nafter the file has been fully peeled once, deleting a reference should\nbe as fast as before. In exchange, reference advertising should be\nfaster after this change because peeled values will not be lost\nwhenever a packed reference is deleted.\n\n refs.c | 23 +++++++++++++++++++++++\n t/t3211-peel-ref.sh | 2 +-\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3c20853..140360f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -878,6 +878,13 @@ void invalidate_ref_cache(const char *submodule)\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@@ -1392,6 +1399,12 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n \n /*\n * Peel the entry (if possible) and return its new peel_status.\n+ *\n+ * It is OK to call this function with a packed reference entry that\n+ * might be stale and might even refer to an object that has since\n+ * been garbage-collected. In such a case, if the entry has\n+ * REF_KNOWS_PEELED then leave the status unchanged and return\n+ * PEEL_PEELED or PEEL_NON_TAG; otherwise, return PEEL_INVALID.\n */\n static enum peel_status peel_entry(struct ref_entry *entry)\n {\n@@ -1958,6 +1971,15 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \tif (len > sizeof(line))\n \t\tdie(\"too long a refname '%s'\", entry->name);\n \twrite_or_die(*fd, line, len);\n+\tif (!peel_entry(entry)) {\n+\t\t/* This reference could be peeled; write the peeled value: */\n+\t\tif (snprintf(line, sizeof(line), \"^%s\\n\",\n+\t\t\t sha1_to_hex(entry->u.value.peeled)) !=\n+\t\t PEELED_LINE_LENGTH)\n+\t\t\tdie(\"internal error\");\n+\t\twrite_or_die(*fd, line, PEELED_LINE_LENGTH);\n+\t}\n+\n \treturn 0;\n }\n \n@@ -1988,6 +2010,7 @@ static int repack_without_ref(const char *refname)\n \t\trollback_lock_file(&packlock);\n \t\treturn 0;\n \t}\n+\twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \tdo_for_each_entry_in_dir(packed, 0, repack_ref_fn, &fd);\n \treturn commit_lock_file(&packlock);\n }\ndiff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh\nindex cca1acb..3b7caca 100755\n--- a/t/t3211-peel-ref.sh\n+++ b/t/t3211-peel-ref.sh\n@@ -61,7 +61,7 @@ test_expect_success 'refs are peeled outside of refs/tags (old packed)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'peeled refs survive deletion of packed ref' '\n+test_expect_success 'peeled refs survive deletion of packed ref' '\n \tgit pack-refs --all &&\n \tcp .git/packed-refs fully-peeled &&\n \tgit branch yadda &&\n-- \n1.8.2.1\n"},{"id":"214180","messageId":"1365944088-10588-23-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 22/33] refs: extract a function write_packed_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:37Z","receivedAt":"2013-04-14T12:54:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract the I/O code from the \"business logic\" in repack_ref_fn().\nLater there will be another caller for this function.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 40 +++++++++++++++++++++++++++-------------\n 1 file changed, 27 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 140360f..9e3eff2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1956,29 +1956,43 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,\n \treturn lock_ref_sha1_basic(refname, old_sha1, flags, NULL);\n }\n \n-static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\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(int fd, char *refname, unsigned char *sha1,\n+\t\t\t unsigned char *peeled)\n {\n-\tint *fd = cb_data;\n \tchar line[PATH_MAX + 100];\n \tint len;\n \n-\t/* Silently skip broken refs: */\n-\tif (!ref_resolves_to_object(entry, 0))\n-\t\treturn 0;\n \tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n-\t\t sha1_to_hex(entry->u.value.sha1), entry->name);\n+\t\t sha1_to_hex(sha1), refname);\n \t/* this should not happen but just being defensive */\n \tif (len > sizeof(line))\n-\t\tdie(\"too long a refname '%s'\", entry->name);\n-\twrite_or_die(*fd, line, len);\n-\tif (!peel_entry(entry)) {\n-\t\t/* This reference could be peeled; write the peeled value: */\n+\t\tdie(\"too long a refname '%s'\", refname);\n+\twrite_or_die(fd, line, len);\n+\n+\tif (peeled) {\n \t\tif (snprintf(line, sizeof(line), \"^%s\\n\",\n-\t\t\t sha1_to_hex(entry->u.value.peeled)) !=\n-\t\t PEELED_LINE_LENGTH)\n+\t\t\t sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)\n \t\t\tdie(\"internal error\");\n-\t\twrite_or_die(*fd, line, PEELED_LINE_LENGTH);\n+\t\twrite_or_die(fd, line, PEELED_LINE_LENGTH);\n \t}\n+}\n+\n+static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n+{\n+\tint *fd = cb_data;\n+\tenum peel_status peel_status;\n+\n+\t/* Silently skip broken refs: */\n+\tif (!ref_resolves_to_object(entry, 0))\n+\t\treturn 0;\n+\tpeel_status = peel_entry(entry);\n+\twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n+\t\t\t peel_status == PEEL_PEELED ?\n+\t\t\t entry->u.value.peeled : NULL);\n \n \treturn 0;\n }\n-- \n1.8.2.1\n"},{"id":"214189","messageId":"1365944088-10588-24-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 23/33] pack-refs: rename handle_one_ref() to pack_one_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:38Z","receivedAt":"2013-04-14T12:54:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This code is about to be moved, so name the function more\ndistinctively.\n\nSigned-off-by: Michael Haggerty \n---\n pack-refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 4461f71..d840055 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -23,7 +23,7 @@ static int do_not_prune(int flags)\n \treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n }\n \n-static int handle_one_ref(const char *path, const unsigned char *sha1,\n+static int pack_one_ref(const char *path, const unsigned char *sha1,\n \t\t\t int flags, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n@@ -130,7 +130,7 @@ int pack_refs(unsigned int flags)\n \t/* perhaps other traits later as well */\n \tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n \n-\tfor_each_ref(handle_one_ref, &cbdata);\n+\tfor_each_ref(pack_one_ref, &cbdata);\n \tif (ferror(cbdata.refs_file))\n \t\tdie(\"failed to write ref-pack file\");\n \tif (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))\n-- \n1.8.2.1\n"},{"id":"214203","messageId":"1365944088-10588-25-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 24/33] pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:39Z","receivedAt":"2013-04-14T12:54:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"pack-refs.c doesn't contain much code, and the code it does contain is\nclosely related to reference handling. Moreover, there is some\nduplication between pack_refs() and repack_without_ref(). Therefore,\nmerge pack-refs.c into refs.c and pack-refs.h into refs.h.\n\nThe code duplication will be addressed in future commits.\n\nSigned-off-by: Michael Haggerty \n---\n Makefile | 2 -\n builtin/clone.c | 1 -\n builtin/pack-refs.c | 2 +-\n pack-refs.c | 148 ----------------------------------------------------\n pack-refs.h | 18 -------\n refs.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 14 +++++\n 7 files changed, 159 insertions(+), 170 deletions(-)\n delete mode 100644 pack-refs.c\n delete mode 100644 pack-refs.h\n\ndiff --git a/Makefile b/Makefile\nindex 0f931a2..de38539 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -684,7 +684,6 @@ LIB_H += notes-cache.h\n LIB_H += notes-merge.h\n LIB_H += notes.h\n LIB_H += object.h\n-LIB_H += pack-refs.h\n LIB_H += pack-revindex.h\n LIB_H += pack.h\n LIB_H += parse-options.h\n@@ -817,7 +816,6 @@ LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\n LIB_OBJS += object.o\n LIB_OBJS += pack-check.o\n-LIB_OBJS += pack-refs.o\n LIB_OBJS += pack-revindex.o\n LIB_OBJS += pack-write.o\n LIB_OBJS += pager.o\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex f9c380e..883cf2a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -18,7 +18,6 @@\n #include \"transport.h\"\n #include \"strbuf.h\"\n #include \"dir.h\"\n-#include \"pack-refs.h\"\n #include \"sigchain.h\"\n #include \"branch.h\"\n #include \"remote.h\"\ndiff --git a/builtin/pack-refs.c b/builtin/pack-refs.c\nindex b5a0f88..b20b1ec 100644\n--- a/builtin/pack-refs.c\n+++ b/builtin/pack-refs.c\n@@ -1,6 +1,6 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n-#include \"pack-refs.h\"\n+#include \"refs.h\"\n \n static char const * const pack_refs_usage[] = {\n \tN_(\"git pack-refs [options]\"),\ndiff --git a/pack-refs.c b/pack-refs.c\ndeleted file mode 100644\nindex d840055..0000000\n--- a/pack-refs.c\n+++ /dev/null\n@@ -1,148 +0,0 @@\n-#include \"cache.h\"\n-#include \"refs.h\"\n-#include \"tag.h\"\n-#include \"pack-refs.h\"\n-\n-struct ref_to_prune {\n-\tstruct ref_to_prune *next;\n-\tunsigned char sha1[20];\n-\tchar name[FLEX_ARRAY];\n-};\n-\n-struct pack_refs_cb_data {\n-\tunsigned int flags;\n-\tstruct ref_to_prune *ref_to_prune;\n-\tFILE *refs_file;\n-};\n-\n-static int do_not_prune(int flags)\n-{\n-\t/* If it is already packed or if it is a symref,\n-\t * do not prune it.\n-\t */\n-\treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n-}\n-\n-static int pack_one_ref(const char *path, const unsigned char *sha1,\n-\t\t\t int flags, void *cb_data)\n-{\n-\tstruct pack_refs_cb_data *cb = cb_data;\n-\tstruct object *o;\n-\tint is_tag_ref;\n-\n-\t/* Do not pack the symbolic refs */\n-\tif ((flags & REF_ISSYMREF))\n-\t\treturn 0;\n-\tis_tag_ref = !prefixcmp(path, \"refs/tags/\");\n-\n-\t/* ALWAYS pack refs that were already packed or are tags */\n-\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))\n-\t\treturn 0;\n-\n-\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n-\n-\to = parse_object_or_die(sha1, path);\n-\tif (o->type == OBJ_TAG) {\n-\t\to = deref_tag(o, path, 0);\n-\t\tif (o)\n-\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n-\t\t\t\tsha1_to_hex(o->sha1));\n-\t}\n-\n-\tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\n-\t\tint namelen = strlen(path) + 1;\n-\t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n-\t\thashcpy(n->sha1, sha1);\n-\t\tstrcpy(n->name, path);\n-\t\tn->next = cb->ref_to_prune;\n-\t\tcb->ref_to_prune = n;\n-\t}\n-\treturn 0;\n-}\n-\n-/*\n- * Remove empty parents, but spare refs/ and immediate subdirs.\n- * Note: munges *name.\n- */\n-static void try_remove_empty_parents(char *name)\n-{\n-\tchar *p, *q;\n-\tint i;\n-\tp = name;\n-\tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n-\t\twhile (*p && *p != '/')\n-\t\t\tp++;\n-\t\t/* tolerate duplicate slashes; see check_refname_format() */\n-\t\twhile (*p == '/')\n-\t\t\tp++;\n-\t}\n-\tfor (q = p; *q; q++)\n-\t\t;\n-\twhile (1) {\n-\t\twhile (q > p && *q != '/')\n-\t\t\tq--;\n-\t\twhile (q > p && *(q-1) == '/')\n-\t\t\tq--;\n-\t\tif (q == p)\n-\t\t\tbreak;\n-\t\t*q = '\\0';\n-\t\tif (rmdir(git_path(\"%s\", name)))\n-\t\t\tbreak;\n-\t}\n-}\n-\n-/* make sure nobody touched the ref, and unlink */\n-static void prune_ref(struct ref_to_prune *r)\n-{\n-\tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n-\n-\tif (lock) {\n-\t\tunlink_or_warn(git_path(\"%s\", r->name));\n-\t\tunlock_ref(lock);\n-\t\ttry_remove_empty_parents(r->name);\n-\t}\n-}\n-\n-static void prune_refs(struct ref_to_prune *r)\n-{\n-\twhile (r) {\n-\t\tprune_ref(r);\n-\t\tr = r->next;\n-\t}\n-}\n-\n-static struct lock_file packed;\n-\n-int pack_refs(unsigned int flags)\n-{\n-\tint fd;\n-\tstruct pack_refs_cb_data cbdata;\n-\n-\tmemset(&cbdata, 0, sizeof(cbdata));\n-\tcbdata.flags = flags;\n-\n-\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n-\tcbdata.refs_file = fdopen(fd, \"w\");\n-\tif (!cbdata.refs_file)\n-\t\tdie_errno(\"unable to create ref-pack file structure\");\n-\n-\t/* perhaps other traits later as well */\n-\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n-\n-\tfor_each_ref(pack_one_ref, &cbdata);\n-\tif (ferror(cbdata.refs_file))\n-\t\tdie(\"failed to write ref-pack file\");\n-\tif (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))\n-\t\tdie_errno(\"failed to write ref-pack file\");\n-\t/*\n-\t * Since the lock file was fdopen()'ed and then fclose()'ed above,\n-\t * assign -1 to the lock file descriptor so that commit_lock_file()\n-\t * won't try to close() it.\n-\t */\n-\tpacked.fd = -1;\n-\tif (commit_lock_file(&packed) < 0)\n-\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n-\tprune_refs(cbdata.ref_to_prune);\n-\treturn 0;\n-}\ndiff --git a/pack-refs.h b/pack-refs.h\ndeleted file mode 100644\nindex 518acfb..0000000\n--- a/pack-refs.h\n+++ /dev/null\n@@ -1,18 +0,0 @@\n-#ifndef PACK_REFS_H\n-#define PACK_REFS_H\n-\n-/*\n- * Flags for controlling behaviour of pack_refs()\n- * PACK_REFS_PRUNE: Prune loose refs after packing\n- * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n- */\n-#define PACK_REFS_PRUNE 0x0001\n-#define PACK_REFS_ALL 0x0002\n-\n-/*\n- * Write a packed-refs file for the current repository.\n- * flags: Combination of the above PACK_REFS_* flags.\n- */\n-int pack_refs(unsigned int flags);\n-\n-#endif /* PACK_REFS_H */\ndiff --git a/refs.c b/refs.c\nindex 9e3eff2..6c7f92c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1981,6 +1981,150 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n \t}\n }\n \n+struct ref_to_prune {\n+\tstruct ref_to_prune *next;\n+\tunsigned char sha1[20];\n+\tchar name[FLEX_ARRAY];\n+};\n+\n+struct pack_refs_cb_data {\n+\tunsigned int flags;\n+\tstruct ref_to_prune *ref_to_prune;\n+\tFILE *refs_file;\n+};\n+\n+static int do_not_prune(int flags)\n+{\n+\t/* If it is already packed or if it is a symref,\n+\t * do not prune it.\n+\t */\n+\treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n+}\n+\n+static int pack_one_ref(const char *path, const unsigned char *sha1,\n+\t\t\t int flags, void *cb_data)\n+{\n+\tstruct pack_refs_cb_data *cb = cb_data;\n+\tstruct object *o;\n+\tint is_tag_ref;\n+\n+\t/* Do not pack the symbolic refs */\n+\tif ((flags & REF_ISSYMREF))\n+\t\treturn 0;\n+\tis_tag_ref = !prefixcmp(path, \"refs/tags/\");\n+\n+\t/* ALWAYS pack refs that were already packed or are tags */\n+\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))\n+\t\treturn 0;\n+\n+\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n+\n+\to = parse_object_or_die(sha1, path);\n+\tif (o->type == OBJ_TAG) {\n+\t\to = deref_tag(o, path, 0);\n+\t\tif (o)\n+\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n+\t\t\t\tsha1_to_hex(o->sha1));\n+\t}\n+\n+\tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\n+\t\tint namelen = strlen(path) + 1;\n+\t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n+\t\thashcpy(n->sha1, sha1);\n+\t\tstrcpy(n->name, path);\n+\t\tn->next = cb->ref_to_prune;\n+\t\tcb->ref_to_prune = n;\n+\t}\n+\treturn 0;\n+}\n+\n+/*\n+ * Remove empty parents, but spare refs/ and immediate subdirs.\n+ * Note: munges *name.\n+ */\n+static void try_remove_empty_parents(char *name)\n+{\n+\tchar *p, *q;\n+\tint i;\n+\tp = name;\n+\tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n+\t\twhile (*p && *p != '/')\n+\t\t\tp++;\n+\t\t/* tolerate duplicate slashes; see check_refname_format() */\n+\t\twhile (*p == '/')\n+\t\t\tp++;\n+\t}\n+\tfor (q = p; *q; q++)\n+\t\t;\n+\twhile (1) {\n+\t\twhile (q > p && *q != '/')\n+\t\t\tq--;\n+\t\twhile (q > p && *(q-1) == '/')\n+\t\t\tq--;\n+\t\tif (q == p)\n+\t\t\tbreak;\n+\t\t*q = '\\0';\n+\t\tif (rmdir(git_path(\"%s\", name)))\n+\t\t\tbreak;\n+\t}\n+}\n+\n+/* make sure nobody touched the ref, and unlink */\n+static void prune_ref(struct ref_to_prune *r)\n+{\n+\tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n+\n+\tif (lock) {\n+\t\tunlink_or_warn(git_path(\"%s\", r->name));\n+\t\tunlock_ref(lock);\n+\t\ttry_remove_empty_parents(r->name);\n+\t}\n+}\n+\n+static void prune_refs(struct ref_to_prune *r)\n+{\n+\twhile (r) {\n+\t\tprune_ref(r);\n+\t\tr = r->next;\n+\t}\n+}\n+\n+static struct lock_file packed;\n+\n+int pack_refs(unsigned int flags)\n+{\n+\tint fd;\n+\tstruct pack_refs_cb_data cbdata;\n+\n+\tmemset(&cbdata, 0, sizeof(cbdata));\n+\tcbdata.flags = flags;\n+\n+\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"),\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tcbdata.refs_file = fdopen(fd, \"w\");\n+\tif (!cbdata.refs_file)\n+\t\tdie_errno(\"unable to create ref-pack file structure\");\n+\n+\t/* perhaps other traits later as well */\n+\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n+\n+\tfor_each_ref(pack_one_ref, &cbdata);\n+\tif (ferror(cbdata.refs_file))\n+\t\tdie(\"failed to write ref-pack file\");\n+\tif (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))\n+\t\tdie_errno(\"failed to write ref-pack file\");\n+\t/*\n+\t * Since the lock file was fdopen()'ed and then fclose()'ed above,\n+\t * assign -1 to the lock file descriptor so that commit_lock_file()\n+\t * won't try to close() it.\n+\t */\n+\tpacked.fd = -1;\n+\tif (commit_lock_file(&packed) < 0)\n+\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\tprune_refs(cbdata.ref_to_prune);\n+\treturn 0;\n+}\n+\n static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n {\n \tint *fd = cb_data;\ndiff --git a/refs.h b/refs.h\nindex ac0ff92..8060ed8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -72,6 +72,20 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n */\n extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n \n+/*\n+ * Flags for controlling behaviour of pack_refs()\n+ * PACK_REFS_PRUNE: Prune loose refs after packing\n+ * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n+ */\n+#define PACK_REFS_PRUNE 0x0001\n+#define PACK_REFS_ALL 0x0002\n+\n+/*\n+ * Write a packed-refs file for the current repository.\n+ * flags: Combination of the above PACK_REFS_* flags.\n+ */\n+int pack_refs(unsigned int flags);\n+\n extern int ref_exists(const char *);\n \n /*\n-- \n1.8.2.1\n"},{"id":"214183","messageId":"1365944088-10588-26-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 25/33] pack_one_ref(): rename \"path\" parameter to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:40Z","receivedAt":"2013-04-14T12:54:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make this function conform to the naming convention established in\n65385ef7d4 for the rest of the refs.c file.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6c7f92c..16c237c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2001,7 +2001,7 @@ static int do_not_prune(int flags)\n \treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n }\n \n-static int pack_one_ref(const char *path, const unsigned char *sha1,\n+static int pack_one_ref(const char *refname, const unsigned char *sha1,\n \t\t\t int flags, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n@@ -2011,27 +2011,27 @@ static int pack_one_ref(const char *path, const unsigned char *sha1,\n \t/* Do not pack the symbolic refs */\n \tif ((flags & REF_ISSYMREF))\n \t\treturn 0;\n-\tis_tag_ref = !prefixcmp(path, \"refs/tags/\");\n+\tis_tag_ref = !prefixcmp(refname, \"refs/tags/\");\n \n \t/* ALWAYS pack refs that were already packed or are tags */\n \tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))\n \t\treturn 0;\n \n-\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), path);\n+\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n \n-\to = parse_object_or_die(sha1, path);\n+\to = parse_object_or_die(sha1, refname);\n \tif (o->type == OBJ_TAG) {\n-\t\to = deref_tag(o, path, 0);\n+\t\to = deref_tag(o, refname, 0);\n \t\tif (o)\n \t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n \t\t\t\tsha1_to_hex(o->sha1));\n \t}\n \n \tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\n-\t\tint namelen = strlen(path) + 1;\n+\t\tint namelen = strlen(refname) + 1;\n \t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n \t\thashcpy(n->sha1, sha1);\n-\t\tstrcpy(n->name, path);\n+\t\tstrcpy(n->name, refname);\n \t\tn->next = cb->ref_to_prune;\n \t\tcb->ref_to_prune = n;\n \t}\n-- \n1.8.2.1\n"},{"id":"214184","messageId":"1365944088-10588-27-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 26/33] refs: use same lock_file object for both ref-packing functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:41Z","receivedAt":"2013-04-14T12:54:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use a single struct lock_file for both pack_refs() and\nrepack_without_ref().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 16c237c..e0411c5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2089,7 +2089,7 @@ static void prune_refs(struct ref_to_prune *r)\n \t}\n }\n \n-static struct lock_file packed;\n+static struct lock_file packlock;\n \n int pack_refs(unsigned int flags)\n {\n@@ -2099,7 +2099,7 @@ int pack_refs(unsigned int flags)\n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"),\n+\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcbdata.refs_file = fdopen(fd, \"w\");\n \tif (!cbdata.refs_file)\n@@ -2118,8 +2118,8 @@ int pack_refs(unsigned int flags)\n \t * assign -1 to the lock file descriptor so that commit_lock_file()\n \t * won't try to close() it.\n \t */\n-\tpacked.fd = -1;\n-\tif (commit_lock_file(&packed) < 0)\n+\tpacklock.fd = -1;\n+\tif (commit_lock_file(&packlock) < 0)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \tprune_refs(cbdata.ref_to_prune);\n \treturn 0;\n@@ -2141,8 +2141,6 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-static struct lock_file packlock;\n-\n static int repack_without_ref(const char *refname)\n {\n \tint fd;\n-- \n1.8.2.1\n"},{"id":"214205","messageId":"1365944088-10588-28-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 27/33] pack_refs(): change to use do_for_each_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:42Z","receivedAt":"2013-04-14T12:54:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"pack_refs() was not using any of the extra features of for_each_ref(),\nso change it to use do_for_each_entry(). This also gives it access to\nthe ref_entry and in particular its peeled field, which will be taken\nadvantage of in the next commit.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e0411c5..471d71d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2001,37 +2001,38 @@ static int do_not_prune(int flags)\n \treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n }\n \n-static int pack_one_ref(const char *refname, const unsigned char *sha1,\n-\t\t\t int flags, void *cb_data)\n+static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n \tstruct object *o;\n \tint is_tag_ref;\n \n-\t/* Do not pack the symbolic refs */\n-\tif ((flags & REF_ISSYMREF))\n+\t/* Do not pack symbolic or broken refs: */\n+\tif ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry, 0))\n \t\treturn 0;\n-\tis_tag_ref = !prefixcmp(refname, \"refs/tags/\");\n+\tis_tag_ref = !prefixcmp(entry->name, \"refs/tags/\");\n \n \t/* ALWAYS pack refs that were already packed or are tags */\n-\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref && !(flags & REF_ISPACKED))\n+\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref &&\n+\t !(entry->flag & REF_ISPACKED))\n \t\treturn 0;\n \n-\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n+\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(entry->u.value.sha1),\n+\t\tentry->name);\n \n-\to = parse_object_or_die(sha1, refname);\n+\to = parse_object_or_die(entry->u.value.sha1, entry->name);\n \tif (o->type == OBJ_TAG) {\n-\t\to = deref_tag(o, refname, 0);\n+\t\to = deref_tag(o, entry->name, 0);\n \t\tif (o)\n \t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n \t\t\t\tsha1_to_hex(o->sha1));\n \t}\n \n-\tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {\n-\t\tint namelen = strlen(refname) + 1;\n+\tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(entry->flag)) {\n+\t\tint namelen = strlen(entry->name) + 1;\n \t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n-\t\thashcpy(n->sha1, sha1);\n-\t\tstrcpy(n->name, refname);\n+\t\thashcpy(n->sha1, entry->u.value.sha1);\n+\t\tstrcpy(n->name, entry->name);\n \t\tn->next = cb->ref_to_prune;\n \t\tcb->ref_to_prune = n;\n \t}\n@@ -2108,7 +2109,7 @@ int pack_refs(unsigned int flags)\n \t/* perhaps other traits later as well */\n \tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n \n-\tfor_each_ref(pack_one_ref, &cbdata);\n+\tdo_for_each_entry(NULL, \"\", pack_one_ref, &cbdata);\n \tif (ferror(cbdata.refs_file))\n \t\tdie(\"failed to write ref-pack file\");\n \tif (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))\n-- \n1.8.2.1\n"},{"id":"214185","messageId":"1365944088-10588-29-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 28/33] refs: inline function do_not_prune()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:43Z","receivedAt":"2013-04-14T12:54:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Function do_not_prune() was redundantly checking REF_ISSYMREF, which\nwas already tested at the top of pack_one_ref(), so remove that check.\nAnd the rest was trivial, so inline the function.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 471d71d..e1b8a56 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1993,14 +1993,6 @@ struct pack_refs_cb_data {\n \tFILE *refs_file;\n };\n \n-static int do_not_prune(int flags)\n-{\n-\t/* If it is already packed or if it is a symref,\n-\t * do not prune it.\n-\t */\n-\treturn (flags & (REF_ISSYMREF|REF_ISPACKED));\n-}\n-\n static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n@@ -2028,7 +2020,8 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n \t\t\t\tsha1_to_hex(o->sha1));\n \t}\n \n-\tif ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(entry->flag)) {\n+\t/* If the ref was already packed, there is no need to prune it. */\n+\tif ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {\n \t\tint namelen = strlen(entry->name) + 1;\n \t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n \t\thashcpy(n->sha1, entry->u.value.sha1);\n-- \n1.8.2.1\n"},{"id":"214190","messageId":"1365944088-10588-30-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 29/33] pack_one_ref(): use function peel_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:44Z","receivedAt":"2013-04-14T12:54:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change pack_one_ref() to call peel_entry() rather than using its own\ncode for peeling references. Aside from sharing code, this lets it\ntake advantage of the optimization introduced by 6c4a060d7d.\n\nPlease note that we *could* use any peeled values that happen to\nalready be stored in the ref_entries, which would avoid some object\nlookups for references that were already packed. But doing so would\nalso propagate any peeling errors across runs of \"git pack-refs\" and\ngive no way to recover from such errors. And \"git pack-refs\" isn't\nrun often enough that the performance cost is a problem. So instead,\nadd a new option to peel_entry() to force the entry to be re-peeled,\nand call it with that option set.\n\nSigned-off-by: Michael Haggerty \n---\n\nI am not so familiar with the object DB API so it would be good for\nsomebody to verify that the slightly different method that pack-refs\nnow uses to peel references is equivalent to the old method.\n\nPlease note that this version of the patch series (in contrast to one\nI shared before via github) re-peels all references when pack-refs is\nrun rather than reusing any available peeled values.\n\n refs.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e1b8a56..6397e29 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1398,7 +1398,9 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n }\n \n /*\n- * Peel the entry (if possible) and return its new peel_status.\n+ * Peel the entry (if possible) and return its new peel_status. If\n+ * repeel is true, re-peel the entry even if there is an old peeled\n+ * value that is already stored in it.\n *\n * It is OK to call this function with a packed reference entry that\n * might be stale and might even refer to an object that has since\n@@ -1406,13 +1408,19 @@ static enum peel_status peel_object(const unsigned char *name, unsigned char *sh\n * REF_KNOWS_PEELED then leave the status unchanged and return\n * PEEL_PEELED or PEEL_NON_TAG; otherwise, return PEEL_INVALID.\n */\n-static enum peel_status peel_entry(struct ref_entry *entry)\n+static enum peel_status peel_entry(struct ref_entry *entry, int repeel)\n {\n \tenum peel_status status;\n \n-\tif (entry->flag & REF_KNOWS_PEELED)\n-\t\treturn is_null_sha1(entry->u.value.peeled) ?\n-\t\t\tPEEL_NON_TAG : PEEL_PEELED;\n+\tif (entry->flag & REF_KNOWS_PEELED) {\n+\t\tif (repeel) {\n+\t\t\tentry->flag &= ~REF_KNOWS_PEELED;\n+\t\t\thashclr(entry->u.value.peeled);\n+\t\t} else {\n+\t\t\treturn is_null_sha1(entry->u.value.peeled) ?\n+\t\t\t\tPEEL_NON_TAG : PEEL_PEELED;\n+\t\t}\n+\t}\n \tif (entry->flag & REF_ISBROKEN)\n \t\treturn PEEL_BROKEN;\n \tif (entry->flag & REF_ISSYMREF)\n@@ -1431,7 +1439,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \n \tif (current_ref && (current_ref->name == refname\n \t\t\t || !strcmp(current_ref->name, refname))) {\n-\t\tif (peel_entry(current_ref))\n+\t\tif (peel_entry(current_ref, 0))\n \t\t\treturn -1;\n \t\thashcpy(sha1, current_ref->u.value.peeled);\n \t\treturn 0;\n@@ -1451,7 +1459,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \tif (flag & REF_ISPACKED) {\n \t\tstruct ref_entry *r = get_packed_ref(refname);\n \t\tif (r) {\n-\t\t\tif (peel_entry(r))\n+\t\t\tif (peel_entry(r, 0))\n \t\t\t\treturn -1;\n \t\t\thashcpy(sha1, r->u.value.peeled);\n \t\t\treturn 0;\n@@ -1996,7 +2004,7 @@ struct pack_refs_cb_data {\n static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n-\tstruct object *o;\n+\tenum peel_status peel_status;\n \tint is_tag_ref;\n \n \t/* Do not pack symbolic or broken refs: */\n@@ -2012,13 +2020,12 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n \tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(entry->u.value.sha1),\n \t\tentry->name);\n \n-\to = parse_object_or_die(entry->u.value.sha1, entry->name);\n-\tif (o->type == OBJ_TAG) {\n-\t\to = deref_tag(o, entry->name, 0);\n-\t\tif (o)\n-\t\t\tfprintf(cb->refs_file, \"^%s\\n\",\n-\t\t\t\tsha1_to_hex(o->sha1));\n-\t}\n+\tpeel_status = peel_entry(entry, 1);\n+\tif (peel_status == PEEL_PEELED)\n+\t\tfprintf(cb->refs_file, \"^%s\\n\", sha1_to_hex(entry->u.value.peeled));\n+\telse if (peel_status != PEEL_NON_TAG)\n+\t\tdie(\"internal error peeling reference %s (%s)\",\n+\t\t entry->name, sha1_to_hex(entry->u.value.sha1));\n \n \t/* If the ref was already packed, there is no need to prune it. */\n \tif ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {\n@@ -2127,7 +2134,7 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \t/* Silently skip broken refs: */\n \tif (!ref_resolves_to_object(entry, 0))\n \t\treturn 0;\n-\tpeel_status = peel_entry(entry);\n+\tpeel_status = peel_entry(entry, 0);\n \twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n \t\t\t peel_status == PEEL_PEELED ?\n \t\t\t entry->u.value.peeled : NULL);\n-- \n1.8.2.1\n"},{"id":"214186","messageId":"1365944088-10588-31-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 30/33] pack_one_ref(): use write_packed_entry() to do the writing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:45Z","receivedAt":"2013-04-14T12:54:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change pack_refs() to work with a file descriptor instead of a FILE*\n(making the file-locking code less awkward) and use\nwrite_packed_entry() to do the writing.\n\nSigned-off-by: Michael Haggerty \n---\n\nPlease checked that I am using the file-locking API correctly; I'm not\nthat familiar with it.\n\n refs.c | 33 ++++++++-------------------------\n 1 file changed, 8 insertions(+), 25 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6397e29..923d34f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1998,7 +1998,7 @@ struct ref_to_prune {\n struct pack_refs_cb_data {\n \tunsigned int flags;\n \tstruct ref_to_prune *ref_to_prune;\n-\tFILE *refs_file;\n+\tint fd;\n };\n \n static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n@@ -2017,15 +2017,13 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n \t !(entry->flag & REF_ISPACKED))\n \t\treturn 0;\n \n-\tfprintf(cb->refs_file, \"%s %s\\n\", sha1_to_hex(entry->u.value.sha1),\n-\t\tentry->name);\n-\n \tpeel_status = peel_entry(entry, 1);\n-\tif (peel_status == PEEL_PEELED)\n-\t\tfprintf(cb->refs_file, \"^%s\\n\", sha1_to_hex(entry->u.value.peeled));\n-\telse if (peel_status != PEEL_NON_TAG)\n+\tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n \t\tdie(\"internal error peeling reference %s (%s)\",\n \t\t entry->name, sha1_to_hex(entry->u.value.sha1));\n+\twrite_packed_entry(cb->fd, entry->name, entry->u.value.sha1,\n+\t\t\t peel_status == PEEL_PEELED ?\n+\t\t\t entry->u.value.peeled : NULL);\n \n \t/* If the ref was already packed, there is no need to prune it. */\n \tif ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {\n@@ -2094,32 +2092,17 @@ static struct lock_file packlock;\n \n int pack_refs(unsigned int flags)\n {\n-\tint fd;\n \tstruct pack_refs_cb_data cbdata;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n-\tcbdata.refs_file = fdopen(fd, \"w\");\n-\tif (!cbdata.refs_file)\n-\t\tdie_errno(\"unable to create ref-pack file structure\");\n+\tcbdata.fd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n+\t\t\t\t\t LOCK_DIE_ON_ERROR);\n \n-\t/* perhaps other traits later as well */\n-\tfprintf(cbdata.refs_file, \"# pack-refs with: peeled fully-peeled \\n\");\n+\twrite_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n \tdo_for_each_entry(NULL, \"\", pack_one_ref, &cbdata);\n-\tif (ferror(cbdata.refs_file))\n-\t\tdie(\"failed to write ref-pack file\");\n-\tif (fflush(cbdata.refs_file) || fsync(fd) || fclose(cbdata.refs_file))\n-\t\tdie_errno(\"failed to write ref-pack file\");\n-\t/*\n-\t * Since the lock file was fdopen()'ed and then fclose()'ed above,\n-\t * assign -1 to the lock file descriptor so that commit_lock_file()\n-\t * won't try to close() it.\n-\t */\n-\tpacklock.fd = -1;\n \tif (commit_lock_file(&packlock) < 0)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \tprune_refs(cbdata.ref_to_prune);\n-- \n1.8.2.1\n"},{"id":"214206","messageId":"1365944088-10588-32-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 31/33] pack_one_ref(): do some cheap tests before a more expensive one","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:46Z","receivedAt":"2013-04-14T12:54:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 923d34f..361a28f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2005,16 +2005,15 @@ static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n \tenum peel_status peel_status;\n-\tint is_tag_ref;\n+\tint is_tag_ref = !prefixcmp(entry->name, \"refs/tags/\");\n \n-\t/* Do not pack symbolic or broken refs: */\n-\tif ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry, 0))\n+\t/* ALWAYS pack refs that were already packed or are tags */\n+\tif (!((cb->flags & PACK_REFS_ALL) || is_tag_ref ||\n+\t (entry->flag & REF_ISPACKED)))\n \t\treturn 0;\n-\tis_tag_ref = !prefixcmp(entry->name, \"refs/tags/\");\n \n-\t/* ALWAYS pack refs that were already packed or are tags */\n-\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref &&\n-\t !(entry->flag & REF_ISPACKED))\n+\t/* Do not pack symbolic or broken refs: */\n+\tif ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry, 0))\n \t\treturn 0;\n \n \tpeel_status = peel_entry(entry, 1);\n-- \n1.8.2.1\n"},{"id":"214187","messageId":"1365944088-10588-33-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 32/33] refs: change do_for_each_*() functions to take ref_cache arguments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:47Z","receivedAt":"2013-04-14T12:54:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change the callers convert submodule names into ref_cache pointers.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 361a28f..cfcb42f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1505,14 +1505,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n }\n \n /*\n- * Call fn for each reference in the specified submodule, omitting\n+ * Call fn for each reference in the specified ref_cache, omitting\n * references not in the containing_dir of base. fn is called for all\n * references, including broken ones.\n */\n-static int do_for_each_entry(const char *submodule, const char *base,\n+static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n-\tstruct ref_cache *refs = get_ref_cache(submodule);\n \tstruct ref_dir *packed_dir = get_packed_refs(refs);\n \tstruct ref_dir *loose_dir = get_loose_refs(refs);\n \tint retval = 0;\n@@ -1541,14 +1540,14 @@ static int do_for_each_entry(const char *submodule, const char *base,\n }\n \n /*\n- * Call fn for each reference in the specified submodule for which the\n+ * Call fn for each reference in the specified ref_cache for which the\n * refname begins with base. If trim is non-zero, then trim that many\n * characters off the beginning of each refname before passing the\n * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n * broken references in the iteration.\n */\n-static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n-\t\t\t int trim, int flags, void *cb_data)\n+static int do_for_each_ref(struct ref_cache *refs, const char *base,\n+\t\t\t each_ref_fn fn, int trim, int flags, void *cb_data)\n {\n \tstruct ref_entry_cb data;\n \tdata.base = base;\n@@ -1557,7 +1556,7 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn\n \tdata.fn = fn;\n \tdata.cb_data = cb_data;\n \n-\treturn do_for_each_entry(submodule, base, do_one_ref, &data);\n+\treturn do_for_each_entry(refs, base, do_one_ref, &data);\n }\n \n static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n@@ -1590,23 +1589,23 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n \n int for_each_ref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(NULL, \"\", fn, 0, 0, cb_data);\n+\treturn do_for_each_ref(get_ref_cache(NULL), \"\", fn, 0, 0, cb_data);\n }\n \n int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(submodule, \"\", fn, 0, 0, cb_data);\n+\treturn do_for_each_ref(get_ref_cache(submodule), \"\", fn, 0, 0, cb_data);\n }\n \n int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(NULL, prefix, fn, strlen(prefix), 0, cb_data);\n+\treturn do_for_each_ref(get_ref_cache(NULL), prefix, fn, strlen(prefix), 0, cb_data);\n }\n \n int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n \t\teach_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(submodule, prefix, fn, strlen(prefix), 0, cb_data);\n+\treturn do_for_each_ref(get_ref_cache(submodule), prefix, fn, strlen(prefix), 0, cb_data);\n }\n \n int for_each_tag_ref(each_ref_fn fn, void *cb_data)\n@@ -1641,7 +1640,7 @@ int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *c\n \n int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(NULL, \"refs/replace/\", fn, 13, 0, cb_data);\n+\treturn do_for_each_ref(get_ref_cache(NULL), \"refs/replace/\", fn, 13, 0, cb_data);\n }\n \n int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n@@ -1664,7 +1663,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n-\tret = do_for_each_ref(NULL, buf.buf, fn, 0, 0, cb_data);\n+\tret = do_for_each_ref(get_ref_cache(NULL), buf.buf, fn, 0, 0, cb_data);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n@@ -1706,7 +1705,7 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n \n int for_each_rawref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(NULL, \"\", fn, 0,\n+\treturn do_for_each_ref(get_ref_cache(NULL), \"\", fn, 0,\n \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \n@@ -2101,7 +2100,7 @@ int pack_refs(unsigned int flags)\n \n \twrite_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n-\tdo_for_each_entry(NULL, \"\", pack_one_ref, &cbdata);\n+\tdo_for_each_entry(get_ref_cache(NULL), \"\", pack_one_ref, &cbdata);\n \tif (commit_lock_file(&packlock) < 0)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \tprune_refs(cbdata.ref_to_prune);\n-- \n1.8.2.1\n"},{"id":"214188","messageId":"1365944088-10588-34-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1365944088-10588-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 33/33] refs: handle the main ref_cache specially","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-14T12:54:48Z","receivedAt":"2013-04-14T12:54:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Hold the ref_cache instance for the main repository in a dedicated,\nstatically-allocated instance to avoid the need for a function call\nand a linked-list traversal when it is needed.\n\nSuggested by: Heiko Voigt \n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 60 +++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 31 insertions(+), 29 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cfcb42f..13e5bc8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -812,9 +812,13 @@ static struct ref_cache {\n \tstruct ref_cache *next;\n \tstruct ref_entry *loose;\n \tstruct ref_entry *packed;\n-\t/* The submodule name, or \"\" for the main repo. */\n-\tchar name[FLEX_ARRAY];\n-} *ref_cache;\n+\t/*\n+\t * The submodule name, or \"\" for the main repo. We allocate\n+\t * length 1 rather than FLEX_ARRAY so that the main ref_cache\n+\t * is initialized correctly.\n+\t */\n+\tchar name[1];\n+} ref_cache, *submodule_ref_caches;\n \n static void clear_packed_ref_cache(struct ref_cache *refs)\n {\n@@ -852,18 +856,18 @@ static struct ref_cache *create_ref_cache(const char *submodule)\n */\n static struct ref_cache *get_ref_cache(const char *submodule)\n {\n-\tstruct ref_cache *refs = ref_cache;\n-\tif (!submodule)\n-\t\tsubmodule = \"\";\n-\twhile (refs) {\n+\tstruct ref_cache *refs;\n+\n+\tif (!submodule || !*submodule)\n+\t\treturn &ref_cache;\n+\n+\tfor (refs = submodule_ref_caches; refs; refs = refs->next)\n \t\tif (!strcmp(submodule, refs->name))\n \t\t\treturn refs;\n-\t\trefs = refs->next;\n-\t}\n \n \trefs = create_ref_cache(submodule);\n-\trefs->next = ref_cache;\n-\tref_cache = refs;\n+\trefs->next = submodule_ref_caches;\n+\tsubmodule_ref_caches = refs;\n \treturn refs;\n }\n \n@@ -1012,8 +1016,8 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \n void add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n-\tadd_ref(get_packed_refs(get_ref_cache(NULL)),\n-\t\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n+\tadd_ref(get_packed_refs(&ref_cache),\n+\t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n }\n \n /*\n@@ -1188,7 +1192,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh\n */\n static struct ref_entry *get_packed_ref(const char *refname)\n {\n-\treturn find_ref(get_packed_refs(get_ref_cache(NULL)), refname);\n+\treturn find_ref(get_packed_refs(&ref_cache), refname);\n }\n \n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n@@ -1589,7 +1593,7 @@ int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n \n int for_each_ref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_ref_cache(NULL), \"\", fn, 0, 0, cb_data);\n+\treturn do_for_each_ref(&ref_cache, \"\", fn, 0, 0, cb_data);\n }\n \n int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n@@ -1599,7 +1603,7 @@ int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)\n \n int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_ref_cache(NULL), prefix, fn, strlen(prefix), 0, cb_data);\n+\treturn do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);\n }\n \n int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n@@ -1640,7 +1644,7 @@ int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *c\n \n int for_each_replace_ref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_ref_cache(NULL), \"refs/replace/\", fn, 13, 0, cb_data);\n+\treturn do_for_each_ref(&ref_cache, \"refs/replace/\", fn, 13, 0, cb_data);\n }\n \n int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n@@ -1663,7 +1667,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n-\tret = do_for_each_ref(get_ref_cache(NULL), buf.buf, fn, 0, 0, cb_data);\n+\tret = do_for_each_ref(&ref_cache, buf.buf, fn, 0, 0, cb_data);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n@@ -1705,7 +1709,7 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)\n \n int for_each_rawref(each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(get_ref_cache(NULL), \"\", fn, 0,\n+\treturn do_for_each_ref(&ref_cache, \"\", fn, 0,\n \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \n@@ -1911,7 +1915,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t * name is a proper prefix of our refname.\n \t */\n \tif (missing &&\n-\t !is_refname_available(refname, NULL, get_packed_refs(get_ref_cache(NULL)))) {\n+\t !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {\n \t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n@@ -2100,7 +2104,7 @@ int pack_refs(unsigned int flags)\n \n \twrite_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n-\tdo_for_each_entry(get_ref_cache(NULL), \"\", pack_one_ref, &cbdata);\n+\tdo_for_each_entry(&ref_cache, \"\", pack_one_ref, &cbdata);\n \tif (commit_lock_file(&packlock) < 0)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \tprune_refs(cbdata.ref_to_prune);\n@@ -2126,7 +2130,6 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n static int repack_without_ref(const char *refname)\n {\n \tint fd;\n-\tstruct ref_cache *refs = get_ref_cache(NULL);\n \tstruct ref_dir *packed;\n \n \tif (!get_packed_ref(refname))\n@@ -2137,8 +2140,8 @@ static int repack_without_ref(const char *refname)\n \t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n \t}\n-\tclear_packed_ref_cache(refs);\n-\tpacked = get_packed_refs(refs);\n+\tclear_packed_ref_cache(&ref_cache);\n+\tpacked = get_packed_refs(&ref_cache);\n \t/* Remove refname from the cache. */\n \tif (remove_entry(packed, refname) == -1) {\n \t\t/*\n@@ -2178,7 +2181,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(lock->ref_name);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tclear_loose_ref_cache(get_ref_cache(NULL));\n+\tclear_loose_ref_cache(&ref_cache);\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -2200,7 +2203,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tstruct stat loginfo;\n \tint log = !lstat(git_path(\"logs/%s\", oldrefname), &loginfo);\n \tconst char *symref = NULL;\n-\tstruct ref_cache *refs = get_ref_cache(NULL);\n \n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldrefname);\n@@ -2212,10 +2214,10 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (!symref)\n \t\treturn error(\"refname %s not found\", oldrefname);\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_packed_refs(refs)))\n+\tif (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))\n \t\treturn 1;\n \n-\tif (!is_refname_available(newrefname, oldrefname, get_loose_refs(refs)))\n+\tif (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))\n \t\treturn 1;\n \n \tif (log && rename(git_path(\"logs/%s\", oldrefname), git_path(TMP_RENAMED_LOG)))\n@@ -2471,7 +2473,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tclear_loose_ref_cache(get_ref_cache(NULL));\n+\tclear_loose_ref_cache(&ref_cache);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.8.2.1\n"},{"id":"214325","messageId":"7v7gk3zqhc.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T16:51:59Z","receivedAt":"2013-04-15T16:51:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It is a nice unit of work and soon will be needed from multiple\n> locations.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 28 ++++++++++++++++++++--------\n> 1 file changed, 20 insertions(+), 8 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index c523978..dfc8600 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)\n> #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n> \n> /*\n> + * Return true iff the reference described by entry can be resolved to\n> + * an object in the database. Emit a warning if the referred-to\n> + * object does not exist.\n> + */\n> +static int ref_resolves_to_object(struct ref_entry *entry)\n> +{\n> +\tif (entry->flag & REF_ISBROKEN)\n> +\t\treturn 0;\n> +\tif (!has_sha1_file(entry->u.value.sha1)) {\n> +\t\terror(\"%s does not point to a valid object!\", entry->name);\n> +\t\treturn 0;\n> +\t}\n> +\treturn 1;\n> +}\n> +\n> +/*\n> * current_ref is a performance hack: when iterating over references\n> * using the for_each_ref*() functions, current_ref is set to the\n> * current reference's entry before calling the callback function. If\n> @@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n> \tif (prefixcmp(entry->name, base))\n> \t\treturn 0;\n> \n> -\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n> -\t\tif (entry->flag & REF_ISBROKEN)\n> -\t\t\treturn 0; /* ignore broken refs e.g. dangling symref */\n> -\t\tif (!has_sha1_file(entry->u.value.sha1)) {\n> -\t\t\terror(\"%s does not point to a valid object!\", entry->name);\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t}\n> +\tif (!((flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n> +\t ref_resolves_to_object(entry)))\n> +\t\treturn 0;\n> +\n\nThe original says \"Unless flags tell us to include broken ones,\nreturn 0 for the broken ones and the ones that point at invalid\nobjects\".\n\nThe updated says \"Unless flags tell us to include broken ones or the\nref is a good one, return 0\".\n\nAre they the same? If we have a good one, and if we are not told to\ninclude broken one, the original just passes the control down to the\nremainder of the function. The updated one will return 0.\n\nOh, wait, that is not the case. The OR is inside !( A || B ), so\nwhat it really wants to say is:\n\n\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n !ref_resolves_to_object(entry))\n\t\treturn 0;\n\nthat is, \"When we are told to exclude broken ones and the one we are\nlooking at is broken, return 0\".\n\nAm I the only one who was confused with this, or was the way the\npatch wrote this logic unnecessarily convoluted?\n\n> \tcurrent_ref = entry;\n> \tretval = fn(entry->name + trim, entry->u.value.sha1, entry->flag, cb_data);\n> \tcurrent_ref = NULL;\n"},{"id":"214331","messageId":"7vobdfwv7m.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 03/33] refs: document do_for_each_ref() and do_one_ref()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:38:05Z","receivedAt":"2013-04-15T17:38:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 1df1ccd..f503ec4 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -525,10 +525,14 @@ static void sort_ref_dir(struct ref_dir *dir)\n> \tdir->sorted = dir->nr = i;\n> }\n> \n> -#define DO_FOR_EACH_INCLUDE_BROKEN 01\n> +/* Include broken references in a do_for_each_ref*() iteration: */\n> +#define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n> \n> static struct ref_entry *current_ref;\n> \n> +/*\n> + * Handle one reference in a do_for_each_ref*()-style iteration.\n> + */\n> static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n> \t\t int flags, void *cb_data, struct ref_entry *entry)\n> {\n> @@ -1338,6 +1342,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n> \tfor_each_rawref(warn_if_dangling_symref, &data);\n> }\n> \n> +/*\n> + * Call fn for each reference in the specified submodule for which the\n> + * refname begins with base. If trim is non-zero, then trim that many\n> + * characters off the beginning of each refname before passing the\n> + * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n> + * broken references in the iteration.\n> + */\n\nEarly termination due to \"fn()\" returning non-zero needs to be\ndocumented here, no?\n\n> static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n> \t\t\t int trim, int flags, void *cb_data)\n> {\n"},{"id":"214332","messageId":"7vhaj7wv77.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 12/33] peel_object(): give more specific information in return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:38:20Z","receivedAt":"2013-04-15T17:38:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Instead of just returning a success/failure bit, return an enumeration\n> value that explains the reason for any failure. This will come in\n> handy shortly.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nThis is a valid rewrite because all existing callers check if the\nreturn value is 0 and does not check the error code against -1.\n\nLooking good so far.\n"},{"id":"214333","messageId":"7va9ozwv70.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 13/33] peel_ref(): fix return value for non-peelable, not-current reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:38:27Z","receivedAt":"2013-04-15T17:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The old version was inconsistent: when a reference was\n> REF_KNOWS_PEELED but with a null peeled value, it returned non-zero\n> for the current reference but zero for other references. Change the\n> behavior for non-current references to match that of current_ref,\n> which is what callers expect. Document the behavior.\n>\n> Current callers did not trigger the previously-buggy behavior.\n\nIs that because we were lucky by codeflow, or is it just that we\ndidn't have a testcase to trigger the behaviour?\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 5 ++++-\n> refs.h | 8 ++++++++\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 84c2497..a0d8e32 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -120,7 +120,8 @@ struct ref_value {\n> \t/*\n> \t * If REF_KNOWS_PEELED, then this field holds the peeled value\n> \t * of this reference, or null if the reference is known not to\n> -\t * be peelable.\n> +\t * be peelable. See the documentation for peel_ref() for an\n> +\t * exact definition of \"peelable\".\n> \t */\n> \tunsigned char peeled[20];\n> };\n> @@ -1339,6 +1340,8 @@ int peel_ref(const char *refname, unsigned char *sha1)\n> \t\tstruct ref_entry *r = get_packed_ref(refname);\n> \n> \t\tif (r && (r->flag & REF_KNOWS_PEELED)) {\n> +\t\t\tif (is_null_sha1(r->u.value.peeled))\n> +\t\t\t return -1;\n> \t\t\thashcpy(sha1, r->u.value.peeled);\n> \t\t\treturn 0;\n> \t\t}\n> diff --git a/refs.h b/refs.h\n> index 17bc1c1..ac0ff92 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -74,6 +74,14 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n> \n> extern int ref_exists(const char *);\n> \n> +/*\n> + * If refname is a non-symbolic reference that refers to a tag object,\n> + * and the tag can be (recursively) dereferenced to a non-tag object,\n> + * store the SHA1 of the referred-to object to sha1 and return 0. If\n> + * any of these conditions are not met, return a non-zero value.\n> + * Symbolic references are considered unpeelable, even if they\n> + * ultimately resolve to a peelable tag.\n> + */\n> extern int peel_ref(const char *refname, unsigned char *sha1);\n> \n> /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n"},{"id":"214334","messageId":"7v38urwv6o.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 14/33] refs: extract a function peel_entry()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:38:39Z","receivedAt":"2013-04-15T17:38:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> \tif (read_ref_full(refname, base, 1, &flag))\n> \t\treturn -1;\n> \n> -\tif ((flag & REF_ISPACKED)) {\n> +\t/*\n> +\t * If the reference is packed, read its ref_entry from the\n> +\t * cache in the hope that we already know its peeled value.\n> +\t * We only try this optimization on packed references because\n> +\t * (a) forcing the filling of the loose reference cache could\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 = get_packed_ref(refname);\n\nThis code makes the reader wonder what happens when a new loose ref\nmasks a stale packed ref, but the worry is unfounded because the\nread_ref_full() wouldn't have gave us REF_ISPACKED in the flag in\nsuch a case.\n\nBut somehow the calling sequence looks like such a mistake waiting\nto happen. It would be much more clear if a function that returns a\n\"struct ref_entry *\" is used instead of read_ref_full() above, and\nwe checked (r->flag & REF_ISPACKED) in the conditional, without a\nseparate get_packed_ref(refname).\n\n> -\n> -\t\tif (r && (r->flag & REF_KNOWS_PEELED)) {\n> -\t\t\tif (is_null_sha1(r->u.value.peeled))\n> -\t\t\t return -1;\n> +\t\tif (r) {\n> +\t\t\tif (peel_entry(r))\n> +\t\t\t\treturn -1;\n> \t\t\thashcpy(sha1, r->u.value.peeled);\n> \t\t\treturn 0;\n> \t\t}\n"},{"id":"214335","messageId":"7vvc7nvglw.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 15/33] refs: change the internal reference-iteration API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:38:51Z","receivedAt":"2013-04-15T17:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Establish an internal API for iterating over references, which gives\n> the callback functions direct access to the ref_entry structure\n> describing the reference. (Do not change the iteration API that is\n> exposed outside of the module.)\n>\n> Define a new internal callback signature\n>\n> int each_ref_entry_fn(struct ref_entry *entry, void *cb_data)\n>\n> Change do_for_each_ref_in_dir() and do_for_each_ref_in_dirs() to\n> accept each_ref_entry_fn callbacks, and rename them to\n> do_for_each_entry_in_dir() and do_for_each_entry_in_dirs(),\n> respectively. Adapt their callers accordingly.\n>\n> Add a new function do_for_each_entry() analogous to do_for_each_ref()\n> but using the new callback style.\n\nNicely done.\n\n>\n> Change do_one_ref() into an each_ref_entry_fn that does some\n> bookkeeping and then calls a wrapped each_ref_fn.\n>\n> Reimplement do_for_each_ref() in terms of do_for_each_entry(), using\n> do_one_ref() as an adapter.\n>\n> Please note that the responsibility for setting current_ref remains in\n> do_one_ref(), which means that current_ref is *not* set when iterating\n> over references via the new internal API. This is not a disadvantage,\n> because current_ref is not needed by callers of the internal API (they\n> receive a pointer to the current ref_entry anyway). But more\n> importantly, this change prevents peel_ref() from returning invalid\n> results in the following scenario:\n>\n> When iterating via the external API, the iteration always includes\n> both packed and loose references, and in particular never presents a\n> packed ref if there is a loose ref with the same name. The internal\n> API, on the other hand, gives the option to iterate over only the\n> packed references. During such an iteration, there is no check\n> whether the packed ref might be hidden by a loose ref of the same\n> name. But until now the packed ref was recorded in current_ref during\n> the iteration. So if peel_ref() were called with the reference name\n> corresponding to current ref, it would return the peeled version of\n> the packed ref even though there might be a loose ref that peels to a\n> different value. This scenario doesn't currently occur in the code,\n> but fix it to prevent things from breaking in a very confusing way in\n> the future.\n\nHopefully that means \"in later patches in this series\" ;-)\n"},{"id":"214336","messageId":"7vobdfvglf.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:39:08Z","receivedAt":"2013-04-15T17:39:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> A packed reference can be overridden by a loose reference, in which\n> case the packed reference is obsolete and is never used. The object\n> pointed to by such a reference can be garbage collected. Since\n> d66da478f2, this could lead to the emission of a spurious error\n> message:\n>\n> error: refs/heads/master does not point to a valid object!\n>\n> The error is generated by repack_without_ref() if there is an obsolete\n> dangling packed reference in packed-refs when the packed-refs file has\n> to be rewritten due to the deletion of another packed reference. Add\n> a failing test demonstrating this problem and some passing tests of\n> related scenarios.\n\nThat is one nasty recent bug.\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n>\n> How can I get rid of the sleeps in these tests?\n\nWould test-chmtime help?\n"},{"id":"214337","messageId":"7vhaj7vgk6.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-18-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 17/33] repack_without_ref(): silence errors for dangling packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:39:53Z","receivedAt":"2013-04-15T17:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Stop emitting an error message for dangling packed references found\n> when deleting another packed reference. See the previous commit for a\n> longer explanation of the issue.\n>\n> Change repack_without_ref_fn() to silently ignore dangling packed\n> references.\n>\n> Signed-off-by: Michael Haggerty \n\nSomehow this feels as if it is sweeping the problem under the rug.\n\nIf you ignore a ref for which a loose ref exists when you update a\npacked refs file, whether the stale \"packed\" one points at an object\nthat is still there or an object that has been garbage collected,\nyou would not even have to check if the \"ref\" resolves to object or\nanything like that, no?\n\nAm I missing something?\n\nThis one feels iffy in the otherwise pleasant-to-read series.\n\n> ---\n> refs.c | 17 ++++++++++-------\n> t/t3210-pack-refs.sh | 2 +-\n> 2 files changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 51f68d3..eadbc2a 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -531,15 +531,17 @@ static void sort_ref_dir(struct ref_dir *dir)\n> \n> /*\n> * Return true iff the reference described by entry can be resolved to\n> - * an object in the database. Emit a warning if the referred-to\n> - * object does not exist.\n> + * an object in the database. If report_errors is true, emit a\n> + * warning if the referred-to object does not exist.\n> */\n> -static int ref_resolves_to_object(struct ref_entry *entry)\n> +static int ref_resolves_to_object(struct ref_entry *entry, int report_errors)\n> {\n> \tif (entry->flag & REF_ISBROKEN)\n> \t\treturn 0;\n> \tif (!has_sha1_file(entry->u.value.sha1)) {\n> -\t\terror(\"%s does not point to a valid object!\", entry->name);\n> +\t\tif (report_errors)\n> +\t\t\terror(\"%s does not point to a valid object!\",\n> +\t\t\t entry->name);\n> \t\treturn 0;\n> \t}\n> \treturn 1;\n> @@ -578,7 +580,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data)\n> \t\treturn 0;\n> \n> \tif (!((data->flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n> -\t ref_resolves_to_object(entry)))\n> +\t ref_resolves_to_object(entry, 1)))\n> \t\treturn 0;\n> \n> \tcurrent_ref = entry;\n> @@ -1897,8 +1899,9 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)\n> \n> \tif (!strcmp(data->refname, entry->name))\n> \t\treturn 0;\n> -\tif (!ref_resolves_to_object(entry))\n> -\t\treturn 0; /* Skip broken refs */\n> +\t/* Silently skip broken refs: */\n> +\tif (!ref_resolves_to_object(entry, 0))\n> +\t\treturn 0;\n> \tlen = snprintf(line, sizeof(line), \"%s %s\\n\",\n> \t\t sha1_to_hex(entry->u.value.sha1), entry->name);\n> \t/* this should not happen but just being defensive */\n> diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\n> index c032d88..559f602 100755\n> --- a/t/t3210-pack-refs.sh\n> +++ b/t/t3210-pack-refs.sh\n> @@ -142,7 +142,7 @@ test_expect_success 'delete ref with dangling packed version' '\n> \ttest_cmp /dev/null result\n> '\n> \n> -test_expect_failure 'delete ref while another dangling packed ref' '\n> +test_expect_success 'delete ref while another dangling packed ref' '\n> \tgit branch lamb &&\n> \tgit commit --allow-empty -m \"future garbage\" &&\n> \tgit pack-refs --all &&\n"},{"id":"214338","messageId":"7va9ozvgk0.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-22-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 21/33] repack_without_ref(): write peeled refs in the rewritten file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:39:59Z","receivedAt":"2013-04-15T17:39:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> When a reference that existed in the packed-refs file is deleted, the\n> packed-refs file must be rewritten. Previously, the file was\n> rewritten without any peeled refs, even if the file contained peeled\n> refs when it was read. This was not a bug, because the packed-refs\n> file header didn't claim that the file contained peeled values. But\n> it had a performance cost, because the repository would lose the\n> benefit of having precomputed peeled references until pack-refs was\n> run again.\n\nGood.\n\n> Teach repack_without_ref() to write peeled refs to the packed-refs\n> file (regardless of whether they were present in the old version of\n> the file).\n"},{"id":"214339","messageId":"7v38urvgjv.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"1365944088-10588-33-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 32/33] refs: change do_for_each_*() functions to take ref_cache arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-15T17:40:04Z","receivedAt":"2013-04-15T17:40:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Change the callers convert submodule names into ref_cache pointers.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nA nice cleanup.\n"},{"id":"214467","messageId":"516D15C8.2050500@alum.mit.edu","threadId":"33507","inReplyTo":"7vobdfwv7m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 03/33] refs: document do_for_each_ref() and do_one_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T09:11:36Z","receivedAt":"2013-04-16T09:11:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n>> +/*\n>> + * Call fn for each reference in the specified submodule for which the\n>> + * refname begins with base. If trim is non-zero, then trim that many\n>> + * characters off the beginning of each refname before passing the\n>> + * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include\n>> + * broken references in the iteration.\n>> + */\n> \n> Early termination due to \"fn()\" returning non-zero needs to be\n> documented here, no?\n> \n>> static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n\nCorrect, thanks. Will be fixed in re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214469","messageId":"516D1988.5060501@alum.mit.edu","threadId":"33507","inReplyTo":"7v7gk3zqhc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T09:27:36Z","receivedAt":"2013-04-16T09:27:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 06:51 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> It is a nice unit of work and soon will be needed from multiple\n>> locations.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> refs.c | 28 ++++++++++++++++++++--------\n>> 1 file changed, 20 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index c523978..dfc8600 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)\n>> #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n>> \n>> /*\n>> + * Return true iff the reference described by entry can be resolved to\n>> + * an object in the database. Emit a warning if the referred-to\n>> + * object does not exist.\n>> + */\n>> +static int ref_resolves_to_object(struct ref_entry *entry)\n>> +{\n>> +\tif (entry->flag & REF_ISBROKEN)\n>> +\t\treturn 0;\n>> +\tif (!has_sha1_file(entry->u.value.sha1)) {\n>> +\t\terror(\"%s does not point to a valid object!\", entry->name);\n>> +\t\treturn 0;\n>> +\t}\n>> +\treturn 1;\n>> +}\n>> +\n>> +/*\n>> * current_ref is a performance hack: when iterating over references\n>> * using the for_each_ref*() functions, current_ref is set to the\n>> * current reference's entry before calling the callback function. If\n>> @@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n>> \tif (prefixcmp(entry->name, base))\n>> \t\treturn 0;\n>> \n>> -\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n>> -\t\tif (entry->flag & REF_ISBROKEN)\n>> -\t\t\treturn 0; /* ignore broken refs e.g. dangling symref */\n>> -\t\tif (!has_sha1_file(entry->u.value.sha1)) {\n>> -\t\t\terror(\"%s does not point to a valid object!\", entry->name);\n>> -\t\t\treturn 0;\n>> -\t\t}\n>> -\t}\n>> +\tif (!((flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n>> +\t ref_resolves_to_object(entry)))\n>> +\t\treturn 0;\n>> +\n> \n> The original says \"Unless flags tell us to include broken ones,\n> return 0 for the broken ones and the ones that point at invalid\n> objects\".\n> \n> The updated says \"Unless flags tell us to include broken ones or the\n> ref is a good one, return 0\".\n> \n> Are they the same? If we have a good one, and if we are not told to\n> include broken one, the original just passes the control down to the\n> remainder of the function. The updated one will return 0.\n> \n> Oh, wait, that is not the case. The OR is inside !( A || B ), so\n> what it really wants to say is:\n> \n> \tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n> !ref_resolves_to_object(entry))\n> \t\treturn 0;\n> \n> that is, \"When we are told to exclude broken ones and the one we are\n> looking at is broken, return 0\".\n> \n> Am I the only one who was confused with this, or was the way the\n> patch wrote this logic unnecessarily convoluted?\n\nI find either way of writing it hard to read quickly. I find that the\nconstruct\n\n if (!(situation in which the function should continue))\n return\n\nmakes it easier to pick out the \"situation in which the function should\ncontinue\". But granted, the nesting of multiple parentheses across\nlines compromises the clarity.\n\nIn projects where I can choose the coding standard, I like to use extra\nwhitespace to help the eye pick out the range of parentheses, like\n\n if (!(\n (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n ref_resolves_to_object(entry)\n ))\n return 0;\n\nbut I've never seen this style in the Git project so I haven't used it here.\n\nSince you prefer the other version, I will change it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214471","messageId":"516D1BF8.9030009@alum.mit.edu","threadId":"33507","inReplyTo":"7va9ozwv70.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 13/33] peel_ref(): fix return value for non-peelable, not-current reference","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T09:38:00Z","receivedAt":"2013-04-16T09:38:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> The old version was inconsistent: when a reference was\n>> REF_KNOWS_PEELED but with a null peeled value, it returned non-zero\n>> for the current reference but zero for other references. Change the\n>> behavior for non-current references to match that of current_ref,\n>> which is what callers expect. Document the behavior.\n>>\n>> Current callers did not trigger the previously-buggy behavior.\n> \n> Is that because we were lucky by codeflow, or is it just that we\n> didn't have a testcase to trigger the behaviour?\n\nExisting callers only called peel_ref() from within a for_each_ref-style\niteration and only for the current ref. Therefore the buggy code path\nwas impossible to reach.\n\nI will note that in the commit message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214483","messageId":"516D4D1F.5010207@alum.mit.edu","threadId":"33507","inReplyTo":"7v38urwv6o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 14/33] refs: extract a function peel_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T13:07:43Z","receivedAt":"2013-04-16T13:07:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> \tif (read_ref_full(refname, base, 1, &flag))\n>> \t\treturn -1;\n>> \n>> -\tif ((flag & REF_ISPACKED)) {\n>> +\t/*\n>> +\t * If the reference is packed, read its ref_entry from the\n>> +\t * cache in the hope that we already know its peeled value.\n>> +\t * We only try this optimization on packed references because\n>> +\t * (a) forcing the filling of the loose reference cache could\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 = get_packed_ref(refname);\n> \n> This code makes the reader wonder what happens when a new loose ref\n> masks a stale packed ref, but the worry is unfounded because the\n> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in\n> such a case.\n> \n> But somehow the calling sequence looks like such a mistake waiting\n> to happen. It would be much more clear if a function that returns a\n> \"struct ref_entry *\" is used instead of read_ref_full() above, and\n> we checked (r->flag & REF_ISPACKED) in the conditional, without a\n> separate get_packed_ref(refname).\n\nAs I'm sure you realize, I didn't change the code that you are referring\nto; I just added a comment.\n\nBut yes, I sympathize with your complaint. Additionally, the code has\nthe drawback that get_packed_ref() is called twice: once in\nread_ref_full() and again in the if block here. Unfortunately, this\nisn't so easy to fix because read_ref_full() doesn't use the loose\nreference cache, so the reference that it returns might not even have a\nref_entry associated with it (specifically, unless the returned flag\nvalue has REF_ISPACKED set). So there are a couple options:\n\n* Always read loose references through the cache; that way there would\nalways be a ref_entry in which the return value could be presented.\nThis would not be a good idea at the moment because the loose reference\ncache is populated one directory at a time, and reading a whole\ndirectory of loose references could be expensive. So before\nimplementing this, it would be advisable to change the code to populate\nthe loose reference cache more selectively when single loose references\nare needed. -> This approach would be well beyond the scope of this\npatch series.\n\n* Implement a function like read_ref_full() with an additional (struct\nref_entry **entry) argument that is written to *in the case* that the\nreference that was returned has a ref_entry associated with it, and NULL\notherwise. This would have to be an internal function because we don't\nwant to expose the ref_entry structure outside of refs.c.\nread_ref_full() would be implemented on top of the new function.\n\nEither way, I'd rather put this idea on my TODO list for another time.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214484","messageId":"516D51B2.8090007@alum.mit.edu","threadId":"33507","inReplyTo":"7vvc7nvglw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 15/33] refs: change the internal reference-iteration API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T13:27:14Z","receivedAt":"2013-04-16T13:27:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> [...] But more\n>> importantly, this change prevents peel_ref() from returning invalid\n>> results in the following scenario:\n>>\n>> When iterating via the external API, the iteration always includes\n>> both packed and loose references, and in particular never presents a\n>> packed ref if there is a loose ref with the same name. The internal\n>> API, on the other hand, gives the option to iterate over only the\n>> packed references. During such an iteration, there is no check\n>> whether the packed ref might be hidden by a loose ref of the same\n>> name. But until now the packed ref was recorded in current_ref during\n>> the iteration. So if peel_ref() were called with the reference name\n>> corresponding to current ref, it would return the peeled version of\n>> the packed ref even though there might be a loose ref that peels to a\n>> different value. This scenario doesn't currently occur in the code,\n>> but fix it to prevent things from breaking in a very confusing way in\n>> the future.\n> \n> Hopefully that means \"in later patches in this series\" ;-)\n\nI don't think that the rest of the series would have triggered this\nproblem either. In fact, if I had written repack_without_ref()'s\npeeling functionality using peel_ref(), then it would have *depended* on\nthis bug for its proper operation...otherwise it would have written the\npeeled version of the loose ref to the packed-ref file. Of course, it's\nall pretty academic because the peeled version of a packed ref should\nnever be used when it is overridden by a loose ref, so the incorrect\npeeled values in the packed-ref file shouldn't have any observable effects.\n\nThe real problem is that calling the old peel_ref() function on a packed\nreference was illegitimate because the function only knew how to peel a\nref that was still active. Plus it's kindof silly tucking away the\ncurrent reference in a global variable then looking it up again instead\nof passing the ref_entry around.\n\nCallers outside of refs.c could also not have triggered this bug because\nthey have no way to access overridden packed refs.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214487","messageId":"516D5CC6.10505@alum.mit.edu","threadId":"33507","inReplyTo":"7vobdfvglf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T14:14:30Z","receivedAt":"2013-04-16T14:14:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:39 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> How can I get rid of the sleeps in these tests?\n> \n> Would test-chmtime help?\n\nMaybe I should take a step back and ask why it isn't easier to expire\nthings regardless of age, which is sometimes a reasonable thing to do\neven outside of test suites. In particular, it seem to me that the most\nobvious interpretation of\n\n\tgit reflog expire --expire=now --all\n\nwould be that it expires *everything*. But in fact it seems to only\nexpire things that are at least one second old, which doesn't seem at\nall useful in the real world. \"--expire=all\" is accepted without\ncomplaint but doesn't do what one would hope. Something like\n\"--expire=$(($(date +%s)+3600))\" works, but it is not very convenient\n(is it portable?).\n\nI guess I can use test-chmtime for my particular test, though I will\nhave to pass it the explicit names of the logfile(s), like\n\n find .git/logs -type f -print0 | xargs -0 test-chmtime =-60\n\nI guess that's what I'll do if no better solution comes up.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214503","messageId":"7vli8imkoq.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"516D51B2.8090007@alum.mit.edu","subject":"Re: [PATCH 15/33] refs: change the internal reference-iteration API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T17:47:49Z","receivedAt":"2013-04-16T17:47:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>>> ... This scenario doesn't currently occur in the code,\n>>> but fix it to prevent things from breaking in a very confusing way in\n>>> the future.\n>> \n>> Hopefully that means \"in later patches in this series\" ;-)\n>\n> I don't think that the rest of the series would have triggered this\n> problem either.\n\nYeah, I misread the message when I said \"Hopefully\". I somehow\nthought it was saying \"we will fix it in the future; otherwise\nthings can break in a confusing way\", which is not the case.\n"},{"id":"214505","messageId":"7vhaj6mkcb.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"516D4D1F.5010207@alum.mit.edu","subject":"Re: [PATCH 14/33] refs: extract a function peel_entry()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T17:55:16Z","receivedAt":"2013-04-16T17:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>>> \tif (read_ref_full(refname, base, 1, &flag))\n>>> \t\treturn -1;\n>>> \n>>> -\tif ((flag & REF_ISPACKED)) {\n>>> +\t/*\n>>> +\t * If the reference is packed, read its ref_entry from the\n>>> +\t * cache in the hope that we already know its peeled value.\n>>> +\t * We only try this optimization on packed references because\n>>> +\t * (a) forcing the filling of the loose reference cache could\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 = get_packed_ref(refname);\n>> \n>> This code makes the reader wonder what happens when a new loose ref\n>> masks a stale packed ref, but the worry is unfounded because the\n>> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in\n>> such a case.\n>> \n>> But somehow the calling sequence looks like such a mistake waiting\n>> to happen. It would be much more clear if a function that returns a\n>> \"struct ref_entry *\" is used instead of read_ref_full() above, and\n>> we checked (r->flag & REF_ISPACKED) in the conditional, without a\n>> separate get_packed_ref(refname).\n>\n> As I'm sure you realize, I didn't change the code that you are referring\n> to; I just added a comment.\n\nYes.\n\n> But yes, I sympathize with your complaint. Additionally, the code has\n> the drawback that get_packed_ref() is called twice: once in\n> read_ref_full() and again in the if block here. Unfortunately, this\n> isn't so easy to fix because read_ref_full() doesn't use the loose\n> reference cache, so the reference that it returns might not even have a\n> ref_entry associated with it (specifically, unless the returned flag\n> value has REF_ISPACKED set). So there are a couple options:\n>\n> * Always read loose references through the cache; that way there would\n> always be a ref_entry in which the return value could be presented.\n> This would not be a good idea at the moment because the loose reference\n> cache is populated one directory at a time, and reading a whole\n> directory of loose references could be expensive. So before\n> implementing this, it would be advisable to change the code to populate\n> the loose reference cache more selectively when single loose references\n> are needed. -> This approach would be well beyond the scope of this\n> patch series.\n>\n> * Implement a function like read_ref_full() with an additional (struct\n> ref_entry **entry) argument that is written to *in the case* that the\n> reference that was returned has a ref_entry associated with it, and NULL\n> otherwise. This would have to be an internal function because we don't\n> want to expose the ref_entry structure outside of refs.c.\n> read_ref_full() would be implemented on top of the new function.\n\nIsn't there another?\n\nInstead of using read_ref_full() at this callsite, can it call a\nfunction, given a refname, returns a pointer to \"struct ref_entry\",\nusing the proper \"a loose ref covers the packed ref with the same\nname\" semantics?\n\nI guess that may need the same machinery for your first option to\nimplement it efficiently; right now read_ref_full() goes directly to\nthe single file that would hold the named ref without scanning and\npopulating sibling refs in the in-core loose ref hierarchy, and\nwithout doing so you cannot return a struct ref_entry. Hmm...\n"},{"id":"214508","messageId":"7vd2tumjqj.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"516D1988.5060501@alum.mit.edu","subject":"Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T18:08:20Z","receivedAt":"2013-04-16T18:08:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> In projects where I can choose the coding standard, I like to use extra\n> whitespace to help the eye pick out the range of parentheses, like\n>\n> if (!(\n> (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||\n> ref_resolves_to_object(entry)\n> ))\n> return 0;\n>\n> but I've never seen this style in the Git project so I haven't used it here.\n\nI _think_ at least to me, it is not the textual grouping style but\nthe \"unless X we skip the rest\" logic itself that confused me. It\ntook the same number of minutes to grok the above as the original.\n\nWe skip the rest of this function unless the condition inside !()\nholds, and the condition is.... we are told to include broken ones,\nin which case we know we will do the remainder without checking\nanything else, or we do the checking and find that it is not broken.\n\nI suspect the root cause of the confusion to force the double\nnegation is INCLUDE_BROKEN; we have to express \"when we are told to\nignore broken one and this thing is broken, ignore it\" without\nnegation, i.e.\n\n\tif ( !(flags & INCLUDE_BROKEN) && is_broken(thing) )\n\t\treturn;\n\nwhich would have been much more pleasant to read if it were\n\n\tif ( (flags & IGNORE_BROKEN) && is_broken(thing) )\n\t\treturn;\n\nUnfortunately, we cannot change it to IGNORE_BROKEN and flip the\nmeaning of the bit, because almost all callers do want to ignore\nbroken ones.\n\nEither way is fine by me, even though I find that !(A || B) needs a\nbit more mental exercise to grok than (!A && !B). Perhaps it is\njust me who is not so strong at math ;-)\n"},{"id":"214549","messageId":"516DCA55.8030601@alum.mit.edu","threadId":"33507","inReplyTo":"7vhaj6mkcb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 14/33] refs: extract a function peel_entry()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-16T22:01:57Z","receivedAt":"2013-04-16T22:01:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/16/2013 07:55 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> On 04/15/2013 07:38 PM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>>\n>>>> \tif (read_ref_full(refname, base, 1, &flag))\n>>>> \t\treturn -1;\n>>>> \n>>>> -\tif ((flag & REF_ISPACKED)) {\n>>>> +\t/*\n>>>> +\t * If the reference is packed, read its ref_entry from the\n>>>> +\t * cache in the hope that we already know its peeled value.\n>>>> +\t * We only try this optimization on packed references because\n>>>> +\t * (a) forcing the filling of the loose reference cache could\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 = get_packed_ref(refname);\n>>>\n>>> This code makes the reader wonder what happens when a new loose ref\n>>> masks a stale packed ref, but the worry is unfounded because the\n>>> read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in\n>>> such a case.\n>>>\n>>> But somehow the calling sequence looks like such a mistake waiting\n>>> to happen. It would be much more clear if a function that returns a\n>>> \"struct ref_entry *\" is used instead of read_ref_full() above, and\n>>> we checked (r->flag & REF_ISPACKED) in the conditional, without a\n>>> separate get_packed_ref(refname).\n>>\n>> As I'm sure you realize, I didn't change the code that you are referring\n>> to; I just added a comment.\n> \n> Yes.\n> \n>> But yes, I sympathize with your complaint. Additionally, the code has\n>> the drawback that get_packed_ref() is called twice: once in\n>> read_ref_full() and again in the if block here. Unfortunately, this\n>> isn't so easy to fix because read_ref_full() doesn't use the loose\n>> reference cache, so the reference that it returns might not even have a\n>> ref_entry associated with it (specifically, unless the returned flag\n>> value has REF_ISPACKED set). So there are a couple options:\n>>\n>> * Always read loose references through the cache; that way there would\n>> always be a ref_entry in which the return value could be presented.\n>> This would not be a good idea at the moment because the loose reference\n>> cache is populated one directory at a time, and reading a whole\n>> directory of loose references could be expensive. So before\n>> implementing this, it would be advisable to change the code to populate\n>> the loose reference cache more selectively when single loose references\n>> are needed. -> This approach would be well beyond the scope of this\n>> patch series.\n>>\n>> * Implement a function like read_ref_full() with an additional (struct\n>> ref_entry **entry) argument that is written to *in the case* that the\n>> reference that was returned has a ref_entry associated with it, and NULL\n>> otherwise. This would have to be an internal function because we don't\n>> want to expose the ref_entry structure outside of refs.c.\n>> read_ref_full() would be implemented on top of the new function.\n> \n> Isn't there another?\n> \n> Instead of using read_ref_full() at this callsite, can it call a\n> function, given a refname, returns a pointer to \"struct ref_entry\",\n> using the proper \"a loose ref covers the packed ref with the same\n> name\" semantics?\n> \n> I guess that may need the same machinery for your first option to\n> implement it efficiently; right now read_ref_full() goes directly to\n> the single file that would hold the named ref without scanning and\n> populating sibling refs in the in-core loose ref hierarchy, and\n> without doing so you cannot return a struct ref_entry. Hmm...\n\nYes, that's the crux. Without the ref_cache, there is no persistent\nref_entry that can be returned to the caller.\n\nSomewhere I have a prototype patch series (never submitted) that would\nmake it reasonable to access all references via the cache. For single\nreference accesses, it wouldn't read all of the loose references in the\nwhole directory but rather only create stub REF_INCOMPLETE ref_entries\nfor the refs that are not needed immediately. These entries would be\nfilled when they are accessed. There was also going to be a bulk read\ncommand to be used when it is expected that the whole directory will be\nneeded. With this change we really would have a ref_entry for every\nreference and your suggestion would become plausible. Someday...\n\nBut for the current issue it would be totally sufficient to return an\nadditional pointer to the ref_entry *if it is available*, because for\npacked refs it will always be available.\n\nActually, for the current patch series I don't see the urgency of\nchanging *anything*. The code works as is and is no worse than the old\ncode. I'd rather do this change (if I get to it) in a separate series.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214556","messageId":"7vip3mhxhq.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"516D5CC6.10505@alum.mit.edu","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-16T23:22:25Z","receivedAt":"2013-04-16T23:22:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> would be that it expires *everything*. But in fact it seems to only\n> expire things that are at least one second old, which doesn't seem at\n> all useful in the real world. \"--expire=all\" is accepted without\n> complaint but doesn't do what one would hope.\n\nPerhaps that is worth fixing, independent of this topic.\n\nApproxidate gives the current time for anything it does not\nunderstand, and that is how --expire=all is interpreted as \"anything\nolder than now\". For that matter, even a string \"now\" has long been\ninterpreted as the current time with the same \"I do not understand\nit, so I'll give you the current timestamp\" logic, until we added an\nofficial support for \"now\" at 93cfa7c7a85e (approxidate_careful()\nreports errorneous date string, 2010-01-26) for entirely different\nreasons.\n\nA completely untested patch for your enjoyment...\n\n builtin/reflog.c | 27 ++++++++++++++++++++-------\n 1 file changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 72a0af7..9fa0e41 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -492,15 +492,28 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len)\n \treturn ent;\n }\n \n+static void parse_expire_value(const char *value, unsigned long *expire)\n+{\n+\tif (!strcmp(value, \"never\") || !strcmp(value, \"false\"))\n+\t\t*expire = 0;\n+\telse if (!strcmp(value, \"all\") || !strcmp(value, \"now\"))\n+\t\t/*\n+\t\t * We take over \"now\" here, which usually translates\n+\t\t * to the current timestamp, because the user really\n+\t\t * means everything she has done in the past, and by\n+\t\t * definition reflogs are the record of the past,\n+\t\t * there is nothing from the future to be kept.\n+\t\t */\n+\t\t*expire = ULONG_MAX;\n+\telse\n+\t\t*expire = approxidate(value);\n+}\n+\n static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\tif (!strcmp(value, \"never\") || !strcmp(value, \"false\")) {\n-\t\t*expire = 0;\n-\t\treturn 0;\n-\t}\n-\t*expire = approxidate(value);\n+\tparse_expire_value(value, expire);\n \treturn 0;\n }\n \n@@ -614,11 +627,11 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tcb.dry_run = 1;\n \t\telse if (!prefixcmp(arg, \"--expire=\")) {\n-\t\t\tcb.expire_total = approxidate(arg + 9);\n+\t\t\tparse_expire_value(arg + 9, &cb.expire_total);\n \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n \t\t}\n \t\telse if (!prefixcmp(arg, \"--expire-unreachable=\")) {\n-\t\t\tcb.expire_unreachable = approxidate(arg + 21);\n+\t\t\tparse_expire_value(arg + 21, &cb.expire_unreachable);\n \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n \t\t}\n \t\telse if (!strcmp(arg, \"--stale-fix\"))\n"},{"id":"214559","messageId":"20130416235712.GA20585@sigill.intra.peff.net","threadId":"33507","inReplyTo":"7vip3mhxhq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-16T23:57:12Z","receivedAt":"2013-04-16T23:57:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2013 at 04:22:25PM -0700, Junio C Hamano wrote:\n\n> +static void parse_expire_value(const char *value, unsigned long *expire)\n> +{\n> +\tif (!strcmp(value, \"never\") || !strcmp(value, \"false\"))\n> +\t\t*expire = 0;\n> +\telse if (!strcmp(value, \"all\") || !strcmp(value, \"now\"))\n> +\t\t/*\n> +\t\t * We take over \"now\" here, which usually translates\n> +\t\t * to the current timestamp, because the user really\n> +\t\t * means everything she has done in the past, and by\n> +\t\t * definition reflogs are the record of the past,\n> +\t\t * there is nothing from the future to be kept.\n> +\t\t */\n> +\t\t*expire = ULONG_MAX;\n> +\telse\n> +\t\t*expire = approxidate(value);\n> +}\n\nDo we want to use approxidate_careful here to catch other junk?\n\n-Peff\n"},{"id":"214570","messageId":"7v38upix82.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"20130416235712.GA20585@sigill.intra.peff.net","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-17T04:42:53Z","receivedAt":"2013-04-17T04:42:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Apr 16, 2013 at 04:22:25PM -0700, Junio C Hamano wrote:\n>\n>> +static void parse_expire_value(const char *value, unsigned long *expire)\n>> +{\n>> +\tif (!strcmp(value, \"never\") || !strcmp(value, \"false\"))\n>> +\t\t*expire = 0;\n>> +\telse if (!strcmp(value, \"all\") || !strcmp(value, \"now\"))\n>> +\t\t/*\n>> +\t\t * We take over \"now\" here, which usually translates\n>> +\t\t * to the current timestamp, because the user really\n>> +\t\t * means everything she has done in the past, and by\n>> +\t\t * definition reflogs are the record of the past,\n>> +\t\t * there is nothing from the future to be kept.\n>> +\t\t */\n>> +\t\t*expire = ULONG_MAX;\n>> +\telse\n>> +\t\t*expire = approxidate(value);\n>> +}\n>\n> Do we want to use approxidate_careful here to catch other junk?\n\nWe can catch a misspelt argument or configuration value that way.\nThat would be a good idea.\n"},{"id":"214586","messageId":"516E5938.6040704@alum.mit.edu","threadId":"33507","inReplyTo":"7vip3mhxhq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-17T08:11:36Z","receivedAt":"2013-04-17T08:11:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/17/2013 01:22 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> would be that it expires *everything*. But in fact it seems to only\n>> expire things that are at least one second old, which doesn't seem at\n>> all useful in the real world. \"--expire=all\" is accepted without\n>> complaint but doesn't do what one would hope.\n> \n> Perhaps that is worth fixing, independent of this topic.\n> \n> Approxidate gives the current time for anything it does not\n> understand, and that is how --expire=all is interpreted as \"anything\n> older than now\". For that matter, even a string \"now\" has long been\n> interpreted as the current time with the same \"I do not understand\n> it, so I'll give you the current timestamp\" logic, until we added an\n> official support for \"now\" at 93cfa7c7a85e (approxidate_careful()\n> reports errorneous date string, 2010-01-26) for entirely different\n> reasons.\n> \n> A completely untested patch for your enjoyment...\n\nI enjoy it :-) But it would be better to put the the function in the\ndate module (\"approxidate_expiry_careful()\"?) and also use it from other\nplaces where an expiry date is being parsed, like\n\n prune --expire=\n reflog expire --expire=/--expire-unreachable=\n gc --prune=\n\n(maybe there are others). And the special values \"all\" etc. would need\nto be documented.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214590","messageId":"516E602F.3090703@alum.mit.edu","threadId":"33507","inReplyTo":"7vhaj7vgk6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 17/33] repack_without_ref(): silence errors for dangling packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-17T08:41:19Z","receivedAt":"2013-04-17T08:41:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2013 07:39 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Stop emitting an error message for dangling packed references found\n>> when deleting another packed reference. See the previous commit for a\n>> longer explanation of the issue.\n>>\n>> Change repack_without_ref_fn() to silently ignore dangling packed\n>> references.\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> Somehow this feels as if it is sweeping the problem under the rug.\n> \n> If you ignore a ref for which a loose ref exists when you update a\n> packed refs file, whether the stale \"packed\" one points at an object\n> that is still there or an object that has been garbage collected,\n> you would not even have to check if the \"ref\" resolves to object or\n> anything like that, no?\n> \n> Am I missing something?\n> \n> This one feels iffy in the otherwise pleasant-to-read series.\n\nThe usual situation when this code would be triggered would be that the\npacked reference is overridden by a loose ref and points at an object\nthat has been garbage collected. In that case it is definitely\nincorrect to emit an error message.\n\nBut the fact that we don't explicitly verify that there is an overriding\nloose reference means that it is possible that the failure to resolve\nthe packed ref comes from some kind of repository corruption, and you\nare correct that such a problem would be swept under the rug by my change.\n\nI've been trying to minimize the extra work that repack_without_ref()\nneeds to do to write peeled references, to avoid stretching out the\ndelay that can now occur when deleting a reference. Thus I was trying\nto save a check of loose references during this operation. But I guess\nI agree that a little bit more caution would be prudent.\n\nI can think of a few ways to avoid sweeping possible indications of repo\ncorruption under the rug, in order of increasing run-time:\n\n1. If a packed ref's SHA-1 cannot be resolved, write the packed ref to\nthe new packed-refs file anyway with SHA-1 but without a peeled value.\nThis would avoid having to check the loose references and avoid erasing\npossible evidence of corruption, but would delay an actual check for\ncorruption until a later time. It would be a quick fix, effectively\nkicking the can down the road instead of sweeping it under the rug.\n\nMinor pitfall: a reference that is listed without a peeled value in a\nfully-peeled pack-refs file tells future readers that the corresponding\nSHA-1 *cannot* be peeled. IF the named object would somehow reappear in\nthe repository (e.g., via a fetch) and IF the object is peelable and IF\nthere is in fact no loose ref overriding the packed ref, then the final\nresult would be that one form of corruption (reference points to\nnon-existent object) would be converted to another form (reference\nfalsely believed to be non-peelable). I think this is an acceptable\nrisk because (a) it would only happen in an unlikely series of events in\na repo that was already corrupt, and (b) because falsely believing a\nreference to be non-peelable wouldn't have terrible consequences.\n\n2. Whenever a packed reference cannot be resolved to an object, verify\nthat there is indeed a loose reference overriding it; if not, emit an\nerror and in either case omit the packed ref from the output.\n\n3. Check for an overriding loose reference *before* trying to peel a\npacked reference, and omit any overridden loose references from the\noutput packed-refs file. This would be close to running \"pack-refs\n--no-prune\" without the \"is_tag_ref\" test and with reuse of available\npeeled values. This approach would tidy up the packed-refs file a bit\nmore than (2) because it would cause the deletion of more overridden\npacked refs, but only as part of first peeling them, which should only\nhappen once in a repo, and only if the first peeling occurs within\nrepack_without_ref() as opposed to an explicit pack_refs(). So it's a\nnegligible improvement over (2).\n\n4. Further along the \"correctness\" spectrum, one could check for\noverriding loose references *every* time the packed-refs file is\nrewritten by repack_without_ref(), even for references whose peeled\nvalues are already known. But this would add overhead to every deletion\nof a packed reference, which is probably not justified.\n\nI'm worried that implementing 2-4 would introduce new race conditions of\nthe type that Peff discovered recently, unless we fix the locking policy\nfirst (which is also on my TODO list). So my suggestion is to implement\n1 now and implement 2 sometime in the future.\n\nOpinions?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"214649","messageId":"7vbo9ceqb3.fsf@alter.siamese.dyndns.org","threadId":"33507","inReplyTo":"7v38upix82.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-17T22:38:08Z","receivedAt":"2013-04-17T22:38:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Do we want to use approxidate_careful here to catch other junk?\n>\n> We can catch a misspelt argument or configuration value that way.\n> That would be a good idea.\n\n-- >8 --\nSubject: date.c: add parse_expiry_date()\n\n\"git reflog --expire=all\" tries to expire reflog entries up to the\ncurrent second, because the approxidate() parser gives the current\ntimestamp for anything it does not understand (and it does not know\nwhat time \"all\" means). When the user tells us to expire \"all\" (or\nset the expiration time to \"now\"), the user wants to remove all the\nreflog entries (no reflog entry should record future time).\n\nJust set it to ULONG_MAX and to let everything that is older that\ntimestamp expire.\n\nWhile at it, allow \"now\" to be treated the same way for callers that\nparse expiry date timestamp with this function. Also use an error\nreporting version of approxidate() to report misspelled date. When\nthe user says e.g. \"--expire=mnoday\" to delete entries two days or\nolder on Wednesday, we wouldn't want the \"unknown, default to now\"\nlogic to kick in.\n\nSigned-off-by: Junio C Hamano \n---\n\n * adoption to more users and tests are left as an exercise to the\n reader.\n\n builtin/reflog.c | 14 +++++++-------\n cache.h | 1 +\n date.c | 22 ++++++++++++++++++++++\n 3 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b3c9e27..44700f9 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -496,11 +496,9 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-\tif (!strcmp(value, \"never\") || !strcmp(value, \"false\")) {\n-\t\t*expire = 0;\n-\t\treturn 0;\n-\t}\n-\t*expire = approxidate(value);\n+\tif (parse_expiry_date(value, expire))\n+\t\treturn error(_(\"%s' for '%s' is not a valid timestamp\"),\n+\t\t\t value, var);\n \treturn 0;\n }\n \n@@ -613,11 +611,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tcb.dry_run = 1;\n \t\telse if (!prefixcmp(arg, \"--expire=\")) {\n-\t\t\tcb.expire_total = approxidate(arg + 9);\n+\t\t\tif (parse_expiry_date(arg + 9, &cb.expire_total))\n+\t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n \t\t}\n \t\telse if (!prefixcmp(arg, \"--expire-unreachable=\")) {\n-\t\t\tcb.expire_unreachable = approxidate(arg + 21);\n+\t\t\tif (parse_expiry_date(arg + 21, &cb.expire_unreachable))\n+\t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n \t\t}\n \t\telse if (!strcmp(arg, \"--stale-fix\"))\ndiff --git a/cache.h b/cache.h\nindex 3622e18..f43f6d9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -878,6 +878,7 @@ void show_date_relative(unsigned long time, int tz, const struct timeval *now,\n \t\t\tstruct strbuf *timebuf);\n int parse_date(const char *date, char *buf, int bufsize);\n int parse_date_basic(const char *date, unsigned long *timestamp, int *offset);\n+int parse_expiry_date(const char *date, unsigned long *timestamp);\n void datestamp(char *buf, int bufsize);\n #define approxidate(s) approxidate_careful((s), NULL)\n unsigned long approxidate_careful(const char *, int *);\ndiff --git a/date.c b/date.c\nindex 57331ed..876d679 100644\n--- a/date.c\n+++ b/date.c\n@@ -705,6 +705,28 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset)\n \treturn 0; /* success */\n }\n \n+int parse_expiry_date(const char *date, unsigned long *timestamp)\n+{\n+\tint errors = 0;\n+\n+\tif (!strcmp(date, \"never\") || !strcmp(date, \"false\"))\n+\t\t*timestamp = 0;\n+\telse if (!strcmp(date, \"all\") || !strcmp(date, \"now\"))\n+\t\t/*\n+\t\t * We take over \"now\" here, which usually translates\n+\t\t * to the current timestamp. This is because the user\n+\t\t * really means to expire everything she has done in\n+\t\t * the past, and by definition reflogs are the record\n+\t\t * of the past, and there is nothing from the future\n+\t\t * to be kept.\n+\t\t */\n+\t\t*timestamp = ULONG_MAX;\n+\telse\n+\t\t*timestamp = approxidate_careful(date, &errors);\n+\n+\treturn errors;\n+}\n+\n int parse_date(const char *date, char *result, int maxlen)\n {\n \tunsigned long timestamp;\n"},{"id":"214688","messageId":"1366271195-4276-1-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"7vbo9ceqb3.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] Add documentation for new expiry option values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-18T07:46:33Z","receivedAt":"2013-04-18T07:46:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The first patch changes the manpages for \"git gc\" and \"git reflog\" to\ndocument the new expiry options made possible by Junio's \"date.c: add\nparse_expiry_date()\". Feel free to squash this patch onto yours.\n\nThe second patch changes the parse_options() API documentation to\nmention that a \"no-\" prefix can also be used for non-boolean options.\nIt is related only by the fact that I was confused by this omission\nwhen I was trying to understand your patch.\n\nMichael Haggerty (2):\n git-gc.txt, git-reflog.txt: document new expiry options\n api-parse-options.txt: document \"no-\" for non-boolean options\n\n Documentation/git-gc.txt | 5 +++--\n Documentation/git-reflog.txt | 9 +++++++--\n Documentation/technical/api-parse-options.txt | 2 ++\n 3 files changed, 12 insertions(+), 4 deletions(-)\n\n-- \n1.8.2.1\n"},{"id":"214689","messageId":"1366271195-4276-2-git-send-email-mhagger@alum.mit.edu","threadId":"33507","inReplyTo":"1366271195-4276-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] git-gc.txt, git-reflog.txt: document new expiry options","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-04-18T07:46:34Z","receivedAt":"2013-04-18T07:46:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the new values that can be used for expiry values where their\nuse makes sense:\n\n* git reflog expire --expire=[all|never]\n* git reflog expire --expire-unreachable=[all|never]\n* git gc --prune=all\n\nOther combinations aren't useful and therefore no documentation is\nadded (even though they are allowed):\n\n* git gc --prune=never\n\n is redundant with \"git gc --no-prune\"\n\n* git prune --expire=all\n\n is equivalent to providing no --expire option\n\n* git prune --expire=never\n\n is a NOP\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/git-gc.txt | 5 +++--\n Documentation/git-reflog.txt | 9 +++++++--\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex b370b02..2402ed6 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -62,8 +62,9 @@ automatic consolidation of packs.\n \n --prune=::\n \tPrune loose objects older than date (default is 2 weeks ago,\n-\toverridable by the config variable `gc.pruneExpire`). This\n-\toption is on by default.\n+\toverridable by the config variable `gc.pruneExpire`).\n+\t--prune=all prunes loose objects regardless of their age.\n+\t--prune is on by default.\n \n --no-prune::\n \tDo not prune any loose objects.\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex fb8697e..70791b9 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -67,14 +67,19 @@ them.\n --expire=