{"thread":{"id":"37739","subject":"[PATCH v2 0/25] prune-safety","startedAt":"2014-10-15T22:32:44Z","lastAt":"2014-10-18T12:31:06Z","messageCount":48,"participants":["Jeff King","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":2,"patchTotal":25},"messages":[{"id":"250712","messageId":"20141015223244.GA25368@peff.net","threadId":"37739","inReplyTo":null,"subject":"[PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:32:44Z","receivedAt":"2014-10-15T22:32:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of the patch series I posted earlier to make \"git\nprune\" keep more contiguous chunks of the object graph. The cleanups to\nt5304 were spun off into their own series, and are dropped here.\nHowever, the other patches seem to have multiplied in number (I must\nhave fed them after midnight).\n\nHere are the changes since the first round (thanks everybody for your\ncomments):\n\n - fix bogus return values from freshen_file, foreach_alt_odb, and\n for_each_packed_object\n\n - make for_each_object_in_pack static\n\n - clarify commit message for \"keep objects reachable from recent\n objects\" patch (this was the one that confused Junio, and I\n elaborated based on our discussion)\n\n - clarify the definition of \"loose object dirs\" in the comment above\n for_each_loose_file_in_object_dir\n\n - in for_each_loose_file, traverse hashed loose object directories in\n numeric order, and pass the number to the subdir callback (this is\n used by prune-packed for its progress updates); as a side effect,\n this fixes the bugs Michael noticed with the subdir callback.\n\n - prune-packed now reuses the for_each_loose_file interface\n\n - use revs->ignore_missing_links so we don't barf on already-missing\n unreferenced objects\n\n - convert reachable.c to use traverse_commit_list instead of its own\n custom walk; this gives support for ignore_missing_links above, and\n saves us a fair bit of code.\n\n - while in the area, I noticed that reachable.c's reflog handling is\n the same as rev-list's --reflog option; it now builds on what's in\n revision.c.\n\nThat takes us up to patch 17. While working in reachable.c, I noticed an\noddity: we consider objects in the index to be reachable during prune\n(which is good), but we do not when dropping them during a repack that\nuses --unpack-unreachable=. The remaining patches fix that,\nwhich needed a fair bit of preparatory cleanup.\n\nI'm really beginning to question whether the \"just drop objects that are\nabout to be pruned\" optimization done in 7e52f56 (gc: do not explode\nobjects which will be immediately pruned, 2012-04-07). It really\ncomplicates things as pack-objects and prune need to have the exact same\nrules (and implementing it naively, by having pack-objects run the same\ncode as prune, is not desirable because pack-objects has _already_ done\na complete expensive traversal to generate the packing list). And I fear\nit will get even worse if we implement some of the race-condition fixes\nthat Michael suggested earlier.\n\nOn the other hand, the loosening behavior without 7e52f56 has some\nsevere pathological cases. A repository which has had a chunk of history\ndeleted can easily increase in size several orders of magnitude due to\nloosening (since we lose the benefit of all deltas in the loosened\nobjects).\n\nFinally, there are a few things that were discussed that I didn't\naddress/fix. I don't think any of them is a critical blocker, but I\ndid want to summarize the state:\n\n - when refreshing, we may update a pack's mtime multiple times. It\n probably wouldn't be too hard to cache this and only update once per\n program run, but I also don't think it's that big a deal in\n practice.\n\n - We will munge mtimes of objects found in alternates. If we don't\n have write access to the alternate, we'll create a local duplicate\n of the object. This is the safer thing, but I'm not sure if there\n are cases where we might try to write out a large number of objects\n which exist in an alternate (OTOH, we will eventually drop them at\n the next repack).\n\n - I didn't implement the \"sort by inode\" trick that fsck does when\n traversing the loose objects. It wouldn't be too hard, but I'm not\n convinced it's actually important.\n\n - I didn't convert fsck to the for_each_loose_file interface (mostly\n because I didn't do the inode-sorting trick, and while I don't think\n it matters, I didn't go to the work to show that it _doesn't_).\n\nHere are the patches:\n\n [01/25]: foreach_alt_odb: propagate return value from callback\n [02/25]: isxdigit: cast input to unsigned char\n [03/25]: object_array: factor out slopbuf-freeing logic\n [04/25]: object_array: add a \"clear\" function\n [05/25]: clean up name allocation in prepare_revision_walk\n [06/25]: reachable: use traverse_commit_list instead of custom walk\n [07/25]: reachable: reuse revision.c \"add all reflogs\" code\n [08/25]: prune: factor out loose-object directory traversal\n [09/25]: reachable: mark index blobs as SEEN\n [10/25]: prune-packed: use for_each_loose_file_in_objdir\n [11/25]: count-objects: do not use xsize_t when counting object size\n [12/25]: count-objects: use for_each_loose_file_in_objdir\n [13/25]: sha1_file: add for_each iterators for loose and packed objects\n [14/25]: prune: keep objects reachable from recent objects\n [15/25]: pack-objects: refactor unpack-unreachable expiration check\n [16/25]: pack-objects: match prune logic for discarding objects\n [17/25]: write_sha1_file: freshen existing objects\n [18/25]: make add_object_array_with_context interface more sane\n [19/25]: traverse_commit_list: support pending blobs/trees with paths\n [20/25]: rev-list: document --reflog option\n [21/25]: rev-list: add --index-objects option\n [22/25]: reachable: use revision machinery's --index-objects code\n [23/25]: pack-objects: use argv_array\n [24/25]: repack: pack objects mentioned by the index\n [25/25]: pack-objects: double-check options before discarding objects\n"},{"id":"250713","messageId":"20141015223312.GA25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 01/25] foreach_alt_odb: propagate return value from callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:33:13Z","receivedAt":"2014-10-15T22:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check the return value of the callback and stop iterating\nif it is non-zero. However, we do not make the non-zero\nreturn value available to the caller, so they have no way of\nknowing whether the operation succeeded or not (technically\nthey can keep their own error flag in the callback data, but\nthat is unlike our other for_each functions).\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n sha1_file.c | 12 ++++++++----\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5b86065..13fadb6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1125,7 +1125,7 @@ extern void prepare_alt_odb(void);\n extern void read_info_alternates(const char * relative_base, int depth);\n extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n-extern void foreach_alt_odb(alt_odb_fn, void*);\n+extern int foreach_alt_odb(alt_odb_fn, void*);\n \n struct pack_window {\n \tstruct pack_window *next;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 83f77f0..fa881bf 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -413,14 +413,18 @@ void add_to_alternates_file(const char *reference)\n \t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n }\n \n-void foreach_alt_odb(alt_odb_fn fn, void *cb)\n+int foreach_alt_odb(alt_odb_fn fn, void *cb)\n {\n \tstruct alternate_object_database *ent;\n+\tint r = 0;\n \n \tprepare_alt_odb();\n-\tfor (ent = alt_odb_list; ent; ent = ent->next)\n-\t\tif (fn(ent, cb))\n-\t\t\treturn;\n+\tfor (ent = alt_odb_list; ent; ent = ent->next) {\n+\t\tr = fn(ent, cb);\n+\t\tif (r)\n+\t\t\tbreak;\n+\t}\n+\treturn r;\n }\n \n void prepare_alt_odb(void)\n-- \n2.1.2.596.g7379948\n"},{"id":"250714","messageId":"20141015223405.GB25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 02/25] isxdigit: cast input to unsigned char","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:34:05Z","receivedAt":"2014-10-15T22:34:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Otherwise, callers must do so or risk triggering warnings\n-Wchar-subscript (and rightfully so; a signed char might\ncause us to use a bogus negative index into the\nhexval_table).\n\nWhile we are dropping the now-unnecessary casts from the\ncaller in urlmatch.c, we can get rid of similar casts in\nactually parsing the hex by using the hexval() helper, which\nimplicitly casts to unsigned (but note that we cannot\nimplement isxdigit in terms of hexval(), as it also casts\nits return value to unsigned).\n\nSigned-off-by: Jeff King \n---\nThe patch that added more calls to isxdigit later in the series actually\ngot reworked. So this is purely a cleanup and can be dropped if need be,\nthough I still think it is an improvement.\n\n git-compat-util.h | 2 +-\n urlmatch.c | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex fb41118..44890d5 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -677,7 +677,7 @@ extern const unsigned char sane_ctype[256];\n #define iscntrl(x) (sane_istest(x,GIT_CNTRL))\n #define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \\\n \t\tGIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)\n-#define isxdigit(x) (hexval_table[x] != -1)\n+#define isxdigit(x) (hexval_table[(unsigned char)(x)] != -1)\n #define tolower(x) sane_case((unsigned char)(x), 0x20)\n #define toupper(x) sane_case((unsigned char)(x), 0)\n #define is_pathspec_magic(x) sane_istest(x,GIT_PATHSPEC_MAGIC)\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 3d4c54b..618d216 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -43,11 +43,11 @@ static int append_normalized_escapes(struct strbuf *buf,\n \t\tfrom_len--;\n \t\tif (ch == '%') {\n \t\t\tif (from_len < 2 ||\n-\t\t\t !isxdigit((unsigned char)from[0]) ||\n-\t\t\t !isxdigit((unsigned char)from[1]))\n+\t\t\t !isxdigit(from[0]) ||\n+\t\t\t !isxdigit(from[1]))\n \t\t\t\treturn 0;\n-\t\t\tch = hexval_table[(unsigned char)*from++] << 4;\n-\t\t\tch |= hexval_table[(unsigned char)*from++];\n+\t\t\tch = hexval(*from++) << 4;\n+\t\t\tch |= hexval(*from++);\n \t\t\tfrom_len -= 2;\n \t\t\twas_esc = 1;\n \t\t}\n-- \n2.1.2.596.g7379948\n"},{"id":"250715","messageId":"20141015223419.GC25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 03/25] object_array: factor out slopbuf-freeing logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:34:19Z","receivedAt":"2014-10-15T22:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is not a lot of code, but it's a logical construct that\nshould not need to be repeated (and we are about to add a\nthird repetition).\n\nSigned-off-by: Jeff King \n---\n object.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex ca9d790..60f4864 100644\n--- a/object.c\n+++ b/object.c\n@@ -355,6 +355,16 @@ void add_object_array_with_context(struct object *obj, const char *name, struct\n \t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n }\n \n+/*\n+ * Free all memory associated with an entry; the result is\n+ * in an unspecified state and should not be examined.\n+ */\n+static void object_array_release_entry(struct object_array_entry *ent)\n+{\n+\tif (ent->name != object_array_slopbuf)\n+\t\tfree(ent->name);\n+}\n+\n void object_array_filter(struct object_array *array,\n \t\t\t object_array_each_func_t want, void *cb_data)\n {\n@@ -367,8 +377,7 @@ void object_array_filter(struct object_array *array,\n \t\t\t\tobjects[dst] = objects[src];\n \t\t\tdst++;\n \t\t} else {\n-\t\t\tif (objects[src].name != object_array_slopbuf)\n-\t\t\t\tfree(objects[src].name);\n+\t\t\tobject_array_release_entry(&objects[src]);\n \t\t}\n \t}\n \tarray->nr = dst;\n@@ -400,8 +409,7 @@ void object_array_remove_duplicates(struct object_array *array)\n \t\t\t\tobjects[array->nr] = objects[src];\n \t\t\tarray->nr++;\n \t\t} else {\n-\t\t\tif (objects[src].name != object_array_slopbuf)\n-\t\t\t\tfree(objects[src].name);\n+\t\t\tobject_array_release_entry(&objects[src]);\n \t\t}\n \t}\n }\n-- \n2.1.2.596.g7379948\n"},{"id":"250716","messageId":"20141015223434.GD25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 04/25] object_array: add a \"clear\" function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:34:34Z","receivedAt":"2014-10-15T22:34:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's currently no easy way to free the memory associated\nwith an object_array (and in most cases, we simply leak the\nmemory in a rev_info's pending array). Let's provide a\nhelper to make this easier to handle.\n\nWe can make use of it in list-objects.c, which does the same\nthing by hand (but fails to free the \"name\" field of each\nentry, potentially leaking memory).\n\nSigned-off-by: Jeff King \n---\n list-objects.c | 7 +------\n object.c | 10 ++++++++++\n object.h | 6 ++++++\n 3 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex 3595ee7..fad6808 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -228,11 +228,6 @@ void traverse_commit_list(struct rev_info *revs,\n \t\tdie(\"unknown pending object %s (%s)\",\n \t\t sha1_to_hex(obj->sha1), name);\n \t}\n-\tif (revs->pending.nr) {\n-\t\tfree(revs->pending.objects);\n-\t\trevs->pending.nr = 0;\n-\t\trevs->pending.alloc = 0;\n-\t\trevs->pending.objects = NULL;\n-\t}\n+\tobject_array_clear(&revs->pending);\n \tstrbuf_release(&base);\n }\ndiff --git a/object.c b/object.c\nindex 60f4864..6aeb1bb 100644\n--- a/object.c\n+++ b/object.c\n@@ -383,6 +383,16 @@ void object_array_filter(struct object_array *array,\n \tarray->nr = dst;\n }\n \n+void object_array_clear(struct object_array *array)\n+{\n+\tint i;\n+\tfor (i = 0; i < array->nr; i++)\n+\t\tobject_array_release_entry(&array->objects[i]);\n+\tfree(array->objects);\n+\tarray->objects = NULL;\n+\tarray->nr = array->alloc = 0;\n+}\n+\n /*\n * Return true iff array already contains an entry with name.\n */\ndiff --git a/object.h b/object.h\nindex e028ced..2a755a2 100644\n--- a/object.h\n+++ b/object.h\n@@ -133,6 +133,12 @@ void object_array_filter(struct object_array *array,\n */\n void object_array_remove_duplicates(struct object_array *array);\n \n+/*\n+ * Remove any objects from the array, freeing all used memory; afterwards\n+ * the array is ready to store more objects with add_object_array().\n+ */\n+void object_array_clear(struct object_array *array);\n+\n void clear_object_flags(unsigned flags);\n \n #endif /* OBJECT_H */\n-- \n2.1.2.596.g7379948\n"},{"id":"250717","messageId":"20141015223512.GE25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 05/25] clean up name allocation in prepare_revision_walk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:35:12Z","receivedAt":"2014-10-15T22:35:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we enter prepare_revision_walk, we have zero or more\nentries in our \"pending\" array. We disconnect that array\nfrom the rev_info, and then process each entry:\n\n 1. If the entry is a commit and the --source option is in\n effect, we keep a pointer to the object name.\n\n 2. Otherwise, we re-add the item to the pending list with\n a blank name.\n\nWe then throw away the old array by freeing the array\nitself, but do not touch the \"name\" field of each entry. For\nany items of type (2), we leak the memory associated with\nthe name. This commit fixes that by calling object_array_clear,\nwhich handles the cleanup for us.\n\nThat breaks (1), though, because it depends on the memory\npointed to by the name to last forever. We can solve that by\nmaking a copy of the name. This is slightly less efficient,\nbut it shouldn't matter in practice, as we do it only for\nthe tip commits of the traversal.\n\nSigned-off-by: Jeff King \n---\n revision.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex e498b7c..01cc276 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -300,7 +300,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\trevs->limited = 1;\n \t\t}\n \t\tif (revs->show_source && !commit->util)\n-\t\t\tcommit->util = (void *) name;\n+\t\t\tcommit->util = xstrdup(name);\n \t\treturn commit;\n \t}\n \n@@ -2656,15 +2656,16 @@ void reset_revision_walk(void)\n \n int prepare_revision_walk(struct rev_info *revs)\n {\n-\tint nr = revs->pending.nr;\n-\tstruct object_array_entry *e, *list;\n+\tint i;\n+\tstruct object_array old_pending;\n \tstruct commit_list **next = &revs->commits;\n \n-\te = list = revs->pending.objects;\n+\tmemcpy(&old_pending, &revs->pending, sizeof(old_pending));\n \trevs->pending.nr = 0;\n \trevs->pending.alloc = 0;\n \trevs->pending.objects = NULL;\n-\twhile (--nr >= 0) {\n+\tfor (i = 0; i < old_pending.nr; i++) {\n+\t\tstruct object_array_entry *e = old_pending.objects + i;\n \t\tstruct commit *commit = handle_commit(revs, e->item, e->name);\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n@@ -2672,10 +2673,9 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\t\t\tnext = commit_list_append(commit, next);\n \t\t\t}\n \t\t}\n-\t\te++;\n \t}\n \tif (!revs->leak_pending)\n-\t\tfree(list);\n+\t\tobject_array_clear(&old_pending);\n \n \t/* Signal whether we need per-parent treesame decoration */\n \tif (revs->simplify_merges ||\n-- \n2.1.2.596.g7379948\n"},{"id":"250718","messageId":"20141015223728.GF25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 06/25] reachable: use traverse_commit_list instead of custom walk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:37:28Z","receivedAt":"2014-10-15T22:37:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To find the set of reachable objects, we add a bunch of\npossible sources to our rev_info, call prepare_revision_walk,\nand then launch into a custom walker that handles each\nobject top. This is a subset of what traverse_commit_list\ndoes, so we can just reuse that code (it can also handle\nmore complex cases like UNINTERESTING commits and pathspecs,\nbut we don't use those features).\n\nSigned-off-by: Jeff King \n---\nI was concerned this would be slower because traverse_commit_list is\nmore featureful. To my surprise, it was consistently about 3-4% faster!\nThe major difference is that traverse_commit_list will hit all of the\ncommits first, and then the trees. For reachability that doesn't matter\neither way, but I suspect the new way has slightly better cache\nlocality, leading to the minor speedup.\n\n reachable.c | 130 ++++++++----------------------------------------------------\n 1 file changed, 17 insertions(+), 113 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 6f6835b..02bf6c2 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -8,6 +8,7 @@\n #include \"reachable.h\"\n #include \"cache-tree.h\"\n #include \"progress.h\"\n+#include \"list-objects.h\"\n \n struct connectivity_progress {\n \tstruct progress *progress;\n@@ -21,118 +22,6 @@ static void update_progress(struct connectivity_progress *cp)\n \t\tdisplay_progress(cp->progress, cp->count);\n }\n \n-static void process_blob(struct blob *blob,\n-\t\t\t struct object_array *p,\n-\t\t\t struct name_path *path,\n-\t\t\t const char *name,\n-\t\t\t struct connectivity_progress *cp)\n-{\n-\tstruct object *obj = &blob->object;\n-\n-\tif (!blob)\n-\t\tdie(\"bad blob object\");\n-\tif (obj->flags & SEEN)\n-\t\treturn;\n-\tobj->flags |= SEEN;\n-\tupdate_progress(cp);\n-\t/* Nothing to do, really .. The blob lookup was the important part */\n-}\n-\n-static void process_gitlink(const unsigned char *sha1,\n-\t\t\t struct object_array *p,\n-\t\t\t struct name_path *path,\n-\t\t\t const char *name)\n-{\n-\t/* I don't think we want to recurse into this, really. */\n-}\n-\n-static void process_tree(struct tree *tree,\n-\t\t\t struct object_array *p,\n-\t\t\t struct name_path *path,\n-\t\t\t const char *name,\n-\t\t\t struct connectivity_progress *cp)\n-{\n-\tstruct object *obj = &tree->object;\n-\tstruct tree_desc desc;\n-\tstruct name_entry entry;\n-\tstruct name_path me;\n-\n-\tif (!tree)\n-\t\tdie(\"bad tree object\");\n-\tif (obj->flags & SEEN)\n-\t\treturn;\n-\tobj->flags |= SEEN;\n-\tupdate_progress(cp);\n-\tif (parse_tree(tree) < 0)\n-\t\tdie(\"bad tree object %s\", sha1_to_hex(obj->sha1));\n-\tadd_object(obj, p, path, name);\n-\tme.up = path;\n-\tme.elem = name;\n-\tme.elem_len = strlen(name);\n-\n-\tinit_tree_desc(&desc, tree->buffer, tree->size);\n-\n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tif (S_ISDIR(entry.mode))\n-\t\t\tprocess_tree(lookup_tree(entry.sha1), p, &me, entry.path, cp);\n-\t\telse if (S_ISGITLINK(entry.mode))\n-\t\t\tprocess_gitlink(entry.sha1, p, &me, entry.path);\n-\t\telse\n-\t\t\tprocess_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);\n-\t}\n-\tfree_tree_buffer(tree);\n-}\n-\n-static void process_tag(struct tag *tag, struct object_array *p,\n-\t\t\tconst char *name, struct connectivity_progress *cp)\n-{\n-\tstruct object *obj = &tag->object;\n-\n-\tif (obj->flags & SEEN)\n-\t\treturn;\n-\tobj->flags |= SEEN;\n-\tupdate_progress(cp);\n-\n-\tif (parse_tag(tag) < 0)\n-\t\tdie(\"bad tag object %s\", sha1_to_hex(obj->sha1));\n-\tif (tag->tagged)\n-\t\tadd_object(tag->tagged, p, NULL, name);\n-}\n-\n-static void walk_commit_list(struct rev_info *revs,\n-\t\t\t struct connectivity_progress *cp)\n-{\n-\tint i;\n-\tstruct commit *commit;\n-\tstruct object_array objects = OBJECT_ARRAY_INIT;\n-\n-\t/* Walk all commits, process their trees */\n-\twhile ((commit = get_revision(revs)) != NULL) {\n-\t\tprocess_tree(commit->tree, &objects, NULL, \"\", cp);\n-\t\tupdate_progress(cp);\n-\t}\n-\n-\t/* Then walk all the pending objects, recursively processing them too */\n-\tfor (i = 0; i < revs->pending.nr; i++) {\n-\t\tstruct object_array_entry *pending = revs->pending.objects + i;\n-\t\tstruct object *obj = pending->item;\n-\t\tconst char *name = pending->name;\n-\t\tif (obj->type == OBJ_TAG) {\n-\t\t\tprocess_tag((struct tag *) obj, &objects, name, cp);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (obj->type == OBJ_TREE) {\n-\t\t\tprocess_tree((struct tree *)obj, &objects, NULL, name, cp);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (obj->type == OBJ_BLOB) {\n-\t\t\tprocess_blob((struct blob *)obj, &objects, NULL, name, cp);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tdie(\"unknown pending object %s (%s)\", sha1_to_hex(obj->sha1), name);\n-\t}\n-}\n-\n static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tconst char *email, unsigned long timestamp, int tz,\n \t\tconst char *message, void *cb_data)\n@@ -210,6 +99,21 @@ static void add_cache_refs(struct rev_info *revs)\n \t\tadd_cache_tree(active_cache_tree, revs);\n }\n \n+/*\n+ * The traversal will have already marked us as SEEN, so we\n+ * only need to handle any progress reporting here.\n+ */\n+static void mark_object(struct object *obj, const struct name_path *path,\n+\t\t\tconst char *name, void *data)\n+{\n+\tupdate_progress(data);\n+}\n+\n+static void mark_commit(struct commit *c, void *data)\n+{\n+\tmark_object(&c->object, NULL, NULL, data);\n+}\n+\n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t struct progress *progress)\n {\n@@ -245,6 +149,6 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t */\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n-\twalk_commit_list(revs, &cp);\n+\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n \tdisplay_progress(cp.progress, cp.count);\n }\n-- \n2.1.2.596.g7379948\n"},{"id":"250719","messageId":"20141015223831.GG25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 07/25] reachable: reuse revision.c \"add all reflogs\" code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:38:31Z","receivedAt":"2014-10-15T22:38:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We want to add all reflog entries as tips for finding\nreachable objects. The revision machinery can already do\nthis (to support \"rev-list --reflog\"); we can reuse that\ncode.\n\nSigned-off-by: Jeff King \n---\nThis one is not strictly necessary, but it seems like a nice cleanup.\nNote that the big difference in the revision.c code is that it will\nprint a warning for broken reflogs, but I think that's fine (and perhaps\neven desirable) here.\n\n reachable.c | 24 +-----------------------\n revision.c | 4 ++--\n revision.h | 1 +\n 3 files changed, 4 insertions(+), 25 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 02bf6c2..4e68cfa 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -22,22 +22,6 @@ static void update_progress(struct connectivity_progress *cp)\n \t\tdisplay_progress(cp->progress, cp->count);\n }\n \n-static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n-{\n-\tstruct object *object;\n-\tstruct rev_info *revs = (struct rev_info *)cb_data;\n-\n-\tobject = parse_object(osha1);\n-\tif (object)\n-\t\tadd_pending_object(revs, object, \"\");\n-\tobject = parse_object(nsha1);\n-\tif (object)\n-\t\tadd_pending_object(revs, object, \"\");\n-\treturn 0;\n-}\n-\n static int add_one_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tstruct object *object = parse_object_or_die(sha1, path);\n@@ -48,12 +32,6 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n-static int add_one_reflog(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\tfor_each_reflog_ent(path, add_one_reflog_ent, cb_data);\n-\treturn 0;\n-}\n-\n static void add_one_tree(const unsigned char *sha1, struct rev_info *revs)\n {\n \tstruct tree *tree = lookup_tree(sha1);\n@@ -138,7 +116,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \n \t/* Add all reflog info */\n \tif (mark_reflog)\n-\t\tfor_each_reflog(add_one_reflog, revs);\n+\t\tadd_reflogs_to_pending(revs, 0);\n \n \tcp.progress = progress;\n \tcp.count = 0;\ndiff --git a/revision.c b/revision.c\nindex 01cc276..b8e02e2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1275,7 +1275,7 @@ static int handle_one_reflog(const char *path, const unsigned char *sha1, int fl\n \treturn 0;\n }\n \n-static void handle_reflog(struct rev_info *revs, unsigned flags)\n+void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)\n {\n \tstruct all_refs_cb cb;\n \tcb.all_revs = revs;\n@@ -2061,7 +2061,7 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n-\t\thandle_reflog(revs, *flags);\n+\t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/revision.h b/revision.h\nindex a620530..e644044 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -276,6 +276,7 @@ extern void add_pending_sha1(struct rev_info *revs,\n \t\t\t unsigned int flags);\n \n extern void add_head_to_pending(struct rev_info *);\n+extern void add_reflogs_to_pending(struct rev_info *, unsigned int flags);\n \n enum commit_action {\n \tcommit_ignore,\n-- \n2.1.2.596.g7379948\n"},{"id":"250720","messageId":"20141015223855.GH25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 08/25] prune: factor out loose-object directory traversal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:38:55Z","receivedAt":"2014-10-15T22:38:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prune has to walk $GIT_DIR/objects/?? in order to find the\nset of loose objects to prune. Other parts of the code\n(e.g., count-objects) want to do the same. Let's factor it\nout into a reusable for_each-style function.\n\nNote that this is not quite a straight code movement. The\noriginal code had strange behavior when it found a file of\nthe form \"[0-9a-f]{2}/.{38}\" that did _not_ contain all hex\ndigits. It executed a \"break\" from the loop, meaning that we\nstopped pruning in that directory (but still pruned other\ndirectories!). This was probably a bug; we do not want to\nprocess the file as an object, but we should keep going\notherwise (and that is how the new code handles it).\n\nWe are also a little more careful with loose object\ndirectories which fail to open. The original code silently\nignored any failures, but the new code will complain about\nany problems besides ENOENT.\n\nSigned-off-by: Jeff King \n---\n builtin/prune.c | 87 +++++++++++++++++----------------------------------------\n cache.h | 33 ++++++++++++++++++++++\n sha1_file.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 143 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 144a3bd..763f53e 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -31,11 +31,23 @@ static int prune_tmp_file(const char *fullpath)\n \treturn 0;\n }\n \n-static int prune_object(const char *fullpath, const unsigned char *sha1)\n+static int prune_object(const unsigned char *sha1, const char *fullpath,\n+\t\t\tvoid *data)\n {\n \tstruct stat st;\n-\tif (lstat(fullpath, &st))\n-\t\treturn error(\"Could not stat '%s'\", fullpath);\n+\n+\t/*\n+\t * Do we know about this object?\n+\t * It must have been reachable\n+\t */\n+\tif (lookup_object(sha1))\n+\t\treturn 0;\n+\n+\tif (lstat(fullpath, &st)) {\n+\t\t/* report errors, but do not stop pruning */\n+\t\terror(\"Could not stat '%s'\", fullpath);\n+\t\treturn 0;\n+\t}\n \tif (st.st_mtime > expire)\n \t\treturn 0;\n \tif (show_only || verbose) {\n@@ -48,68 +60,20 @@ static int prune_object(const char *fullpath, const unsigned char *sha1)\n \treturn 0;\n }\n \n-static int prune_dir(int i, struct strbuf *path)\n+static int prune_cruft(const char *basename, const char *path, void *data)\n {\n-\tsize_t baselen = path->len;\n-\tDIR *dir = opendir(path->buf);\n-\tstruct dirent *de;\n-\n-\tif (!dir)\n-\t\treturn 0;\n-\n-\twhile ((de = readdir(dir)) != NULL) {\n-\t\tchar name[100];\n-\t\tunsigned char sha1[20];\n-\n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n-\t\tif (strlen(de->d_name) == 38) {\n-\t\t\tsprintf(name, \"%02x\", i);\n-\t\t\tmemcpy(name+2, de->d_name, 39);\n-\t\t\tif (get_sha1_hex(name, sha1) < 0)\n-\t\t\t\tbreak;\n-\n-\t\t\t/*\n-\t\t\t * Do we know about this object?\n-\t\t\t * It must have been reachable\n-\t\t\t */\n-\t\t\tif (lookup_object(sha1))\n-\t\t\t\tcontinue;\n-\n-\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n-\t\t\tprune_object(path->buf, sha1);\n-\t\t\tstrbuf_setlen(path, baselen);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(de->d_name, \"tmp_obj_\")) {\n-\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n-\t\t\tprune_tmp_file(path->buf);\n-\t\t\tstrbuf_setlen(path, baselen);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path->buf, de->d_name);\n-\t}\n-\tclosedir(dir);\n-\tif (!show_only)\n-\t\trmdir(path->buf);\n+\tif (starts_with(basename, \"tmp_obj_\"))\n+\t\tprune_tmp_file(path);\n+\telse\n+\t\tfprintf(stderr, \"bad sha1 file: %s\\n\", path);\n \treturn 0;\n }\n \n-static void prune_object_dir(const char *path)\n+static int prune_subdir(int nr, const char *path, void *data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tsize_t baselen;\n-\tint i;\n-\n-\tstrbuf_addstr(&buf, path);\n-\tstrbuf_addch(&buf, '/');\n-\tbaselen = buf.len;\n-\n-\tfor (i = 0; i < 256; i++) {\n-\t\tstrbuf_addf(&buf, \"%02x\", i);\n-\t\tprune_dir(i, &buf);\n-\t\tstrbuf_setlen(&buf, baselen);\n-\t}\n+\tif (!show_only)\n+\t\trmdir(path);\n+\treturn 0;\n }\n \n /*\n@@ -173,7 +137,8 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \n \tmark_reachable_objects(&revs, 1, progress);\n \tstop_progress(&progress);\n-\tprune_object_dir(get_object_directory());\n+\tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n+\t\t\t\t prune_cruft, prune_subdir, NULL);\n \n \tprune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);\n \tremove_temporary_files(get_object_directory());\ndiff --git a/cache.h b/cache.h\nindex 13fadb6..8ffefaa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1221,6 +1221,39 @@ extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsig\n extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n \n+/*\n+ * Iterate over the files in the loose-object parts of the object\n+ * directory \"path\", triggering the following callbacks:\n+ *\n+ * - loose_object is called for each loose object we find.\n+ *\n+ * - loose_cruft is called for any files that do not appear to be\n+ * loose objects. Note that we only look in the loose object\n+ * directories \"objects/[0-9a-f]{2}/\", so we will not report\n+ * \"objects/foobar\" as cruft.\n+ *\n+ * - loose_subdir is called for each top-level hashed subdirectory\n+ * of the object directory (e.g., \"$OBJDIR/f0\"). It is called\n+ * after the objects in the directory are processed.\n+ *\n+ * Any callback that is NULL will be ignored. Callbacks returning non-zero\n+ * will end the iteration.\n+ */\n+typedef int each_loose_object_fn(const unsigned char *sha1,\n+\t\t\t\t const char *path,\n+\t\t\t\t void *data);\n+typedef int each_loose_cruft_fn(const char *basename,\n+\t\t\t\tconst char *path,\n+\t\t\t\tvoid *data);\n+typedef int each_loose_subdir_fn(int nr,\n+\t\t\t\t const char *path,\n+\t\t\t\t void *data);\n+int for_each_loose_file_in_objdir(const char *path,\n+\t\t\t\t each_loose_object_fn obj_cb,\n+\t\t\t\t each_loose_cruft_fn cruft_cb,\n+\t\t\t\t each_loose_subdir_fn subdir_cb,\n+\t\t\t\t void *data);\n+\n struct object_info {\n \t/* Request */\n \tenum object_type *typep;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex fa881bf..a20240b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3265,3 +3265,87 @@ void assert_sha1_type(const unsigned char *sha1, enum object_type expect)\n \t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1),\n \t\t typename(expect));\n }\n+\n+static int for_each_file_in_obj_subdir(int subdir_nr,\n+\t\t\t\t struct strbuf *path,\n+\t\t\t\t each_loose_object_fn obj_cb,\n+\t\t\t\t each_loose_cruft_fn cruft_cb,\n+\t\t\t\t each_loose_subdir_fn subdir_cb,\n+\t\t\t\t void *data)\n+{\n+\tsize_t baselen = path->len;\n+\tDIR *dir = opendir(path->buf);\n+\tstruct dirent *de;\n+\tint r = 0;\n+\n+\tif (!dir) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn error(\"unable to open %s: %s\", path->buf, strerror(errno));\n+\t}\n+\n+\twhile ((de = readdir(dir))) {\n+\t\tif (is_dot_or_dotdot(de->d_name))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_setlen(path, baselen);\n+\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\n+\t\tif (strlen(de->d_name) == 38) {\n+\t\t\tchar hex[41];\n+\t\t\tunsigned char sha1[20];\n+\n+\t\t\tsnprintf(hex, sizeof(hex), \"%02x%s\",\n+\t\t\t\t subdir_nr, de->d_name);\n+\t\t\tif (!get_sha1_hex(hex, sha1)) {\n+\t\t\t\tif (obj_cb) {\n+\t\t\t\t\tr = obj_cb(sha1, path->buf, data);\n+\t\t\t\t\tif (r)\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (cruft_cb) {\n+\t\t\tr = cruft_cb(de->d_name, path->buf, data);\n+\t\t\tif (r)\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\tstrbuf_setlen(path, baselen);\n+\n+\tif (!r && subdir_cb)\n+\t\tr = subdir_cb(subdir_nr, path->buf, data);\n+\n+\tclosedir(dir);\n+\treturn r;\n+}\n+\n+int for_each_loose_file_in_objdir(const char *path,\n+\t\t\t each_loose_object_fn obj_cb,\n+\t\t\t each_loose_cruft_fn cruft_cb,\n+\t\t\t each_loose_subdir_fn subdir_cb,\n+\t\t\t void *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t baselen;\n+\tint r = 0;\n+\tint i;\n+\n+\tstrbuf_addstr(&buf, path);\n+\tstrbuf_addch(&buf, '/');\n+\tbaselen = buf.len;\n+\n+\tfor (i = 0; i < 256; i++) {\n+\t\tstrbuf_addf(&buf, \"%02x\", i);\n+\t\tr = for_each_file_in_obj_subdir(i, &buf, obj_cb, cruft_cb,\n+\t\t\t\t\t\tsubdir_cb, data);\n+\t\tstrbuf_setlen(&buf, baselen);\n+\t\tif (r)\n+\t\t\tbreak;\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\treturn r;\n+}\n-- \n2.1.2.596.g7379948\n"},{"id":"250721","messageId":"20141015224037.GI25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 09/25] reachable: mark index blobs as SEEN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:40:37Z","receivedAt":"2014-10-15T22:40:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we mark all reachable objects for pruning, that\nincludes blobs mentioned by the index. However, we do not\nmark these with the SEEN flag, as we do for objects that we\nfind by traversing (we also do not add them to the pending\nlist, but that is because there is nothing further to\ntraverse with them).\n\nThis doesn't cause any problems with prune, because it\nchecks only that the object exists in the global object\nhash, and not its flags. However, let's mark these objects\nto be consistent and avoid any later surprises.\n\nSigned-off-by: Jeff King \n---\nThis code actually gets dropped later on in the series (when we teach\nthe revision machinery to look at index objects), so this could\ntechnically be omitted. But it also keeps the minor behavior change\nhere by itself, where it is explained, instead of as a side effect of\nthe movement.\n\n reachable.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 4e68cfa..d03f829 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -55,6 +55,8 @@ static void add_cache_refs(struct rev_info *revs)\n \n \tread_cache();\n \tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct blob *blob;\n+\n \t\t/*\n \t\t * The index can contain blobs and GITLINKs, GITLINKs are hashes\n \t\t * that don't actually point to objects in the repository, it's\n@@ -65,7 +67,10 @@ static void add_cache_refs(struct rev_info *revs)\n \t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n \t\t\tcontinue;\n \n-\t\tlookup_blob(active_cache[i]->sha1);\n+\t\tblob = lookup_blob(active_cache[i]->sha1);\n+\t\tif (blob)\n+\t\t\tblob->object.flags |= SEEN;\n+\n \t\t/*\n \t\t * We could add the blobs to the pending list, but quite\n \t\t * frankly, we don't care. Once we've looked them up, and\n-- \n2.1.2.596.g7379948\n"},{"id":"250722","messageId":"20141015224053.GJ25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 10/25] prune-packed: use for_each_loose_file_in_objdir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:40:53Z","receivedAt":"2014-10-15T22:40:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us from manually traversing the directory\nstructure ourselves.\n\nSigned-off-by: Jeff King \n---\n builtin/prune-packed.c | 69 +++++++++++++++++---------------------------------\n 1 file changed, 23 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex d430731..f24a2c2 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -10,65 +10,42 @@ static const char * const prune_packed_usage[] = {\n \n static struct progress *progress;\n \n-static void prune_dir(int i, DIR *dir, struct strbuf *pathname, int opts)\n+static int prune_subdir(int nr, const char *path, void *data)\n {\n-\tstruct dirent *de;\n-\tchar hex[40];\n-\tint top_len = pathname->len;\n+\tint *opts = data;\n+\tdisplay_progress(progress, nr + 1);\n+\tif (!(*opts & PRUNE_PACKED_DRY_RUN))\n+\t\trmdir(path);\n+\treturn 0;\n+}\n+\n+static int prune_object(const unsigned char *sha1, const char *path,\n+\t\t\t void *data)\n+{\n+\tint *opts = data;\n \n-\tsprintf(hex, \"%02x\", i);\n-\twhile ((de = readdir(dir)) != NULL) {\n-\t\tunsigned char sha1[20];\n-\t\tif (strlen(de->d_name) != 38)\n-\t\t\tcontinue;\n-\t\tmemcpy(hex + 2, de->d_name, 38);\n-\t\tif (get_sha1_hex(hex, sha1))\n-\t\t\tcontinue;\n-\t\tif (!has_sha1_pack(sha1))\n-\t\t\tcontinue;\n+\tif (!has_sha1_pack(sha1))\n+\t\treturn 0;\n \n-\t\tstrbuf_add(pathname, de->d_name, 38);\n-\t\tif (opts & PRUNE_PACKED_DRY_RUN)\n-\t\t\tprintf(\"rm -f %s\\n\", pathname->buf);\n-\t\telse\n-\t\t\tunlink_or_warn(pathname->buf);\n-\t\tdisplay_progress(progress, i + 1);\n-\t\tstrbuf_setlen(pathname, top_len);\n-\t}\n+\tif (*opts & PRUNE_PACKED_DRY_RUN)\n+\t\tprintf(\"rm -f %s\\n\", path);\n+\telse\n+\t\tunlink_or_warn(path);\n+\treturn 0;\n }\n \n void prune_packed_objects(int opts)\n {\n-\tint i;\n-\tconst char *dir = get_object_directory();\n-\tstruct strbuf pathname = STRBUF_INIT;\n-\tint top_len;\n-\n-\tstrbuf_addstr(&pathname, dir);\n \tif (opts & PRUNE_PACKED_VERBOSE)\n \t\tprogress = start_progress_delay(_(\"Removing duplicate objects\"),\n \t\t\t256, 95, 2);\n \n-\tif (pathname.len && pathname.buf[pathname.len - 1] != '/')\n-\t\tstrbuf_addch(&pathname, '/');\n-\n-\ttop_len = pathname.len;\n-\tfor (i = 0; i < 256; i++) {\n-\t\tDIR *d;\n+\tfor_each_loose_file_in_objdir(get_object_directory(),\n+\t\t\t\t prune_object, NULL, prune_subdir, &opts);\n \n-\t\tdisplay_progress(progress, i + 1);\n-\t\tstrbuf_setlen(&pathname, top_len);\n-\t\tstrbuf_addf(&pathname, \"%02x/\", i);\n-\t\td = opendir(pathname.buf);\n-\t\tif (!d)\n-\t\t\tcontinue;\n-\t\tprune_dir(i, d, &pathname, opts);\n-\t\tclosedir(d);\n-\t\tstrbuf_setlen(&pathname, top_len + 2);\n-\t\trmdir(pathname.buf);\n-\t}\n+\t/* Ensure we show 100% before finishing progress */\n+\tdisplay_progress(progress, 256);\n \tstop_progress(&progress);\n-\tstrbuf_release(&pathname);\n }\n \n int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n-- \n2.1.2.596.g7379948\n"},{"id":"250723","messageId":"20141015224058.GK25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 11/25] count-objects: do not use xsize_t when counting object size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:40:58Z","receivedAt":"2014-10-15T22:40:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of xsize_t is to safely cast an off_t into a size_t\n(because we are about to mmap). But in count-objects, we are\nsumming the sizes in an off_t. Using xsize_t means that\ncount-objects could fail on a 32-bit system with a 4G\nobject (not likely, as other parts of git would fail, but\nwe should at least be correct here).\n\nSigned-off-by: Jeff King \n---\n builtin/count-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex a7f70cb..316a805 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -53,7 +53,7 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n \t\t\tif (lstat(path, &st) || !S_ISREG(st.st_mode))\n \t\t\t\tbad = 1;\n \t\t\telse\n-\t\t\t\t(*loose_size) += xsize_t(on_disk_bytes(st));\n+\t\t\t\t(*loose_size) += on_disk_bytes(st);\n \t\t}\n \t\tif (bad) {\n \t\t\tif (verbose) {\n-- \n2.1.2.596.g7379948\n"},{"id":"250724","messageId":"20141015224110.GL25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 12/25] count-objects: use for_each_loose_file_in_objdir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:41:11Z","receivedAt":"2014-10-15T22:41:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This drops our line count considerably, and should make\nthings more readable by keeping the counting logic separate\nfrom the traversal.\n\nSigned-off-by: Jeff King \n---\n builtin/count-objects.c | 101 ++++++++++++++----------------------------------\n 1 file changed, 30 insertions(+), 71 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 316a805..e47ef0b 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -11,6 +11,9 @@\n \n static unsigned long garbage;\n static off_t size_garbage;\n+static int verbose;\n+static unsigned long loose, packed, packed_loose;\n+static off_t loose_size;\n \n static void real_report_garbage(const char *desc, const char *path)\n {\n@@ -21,61 +24,31 @@ static void real_report_garbage(const char *desc, const char *path)\n \tgarbage++;\n }\n \n-static void count_objects(DIR *d, char *path, int len, int verbose,\n-\t\t\t unsigned long *loose,\n-\t\t\t off_t *loose_size,\n-\t\t\t unsigned long *packed_loose)\n+static void loose_garbage(const char *path)\n {\n-\tstruct dirent *ent;\n-\twhile ((ent = readdir(d)) != NULL) {\n-\t\tchar hex[41];\n-\t\tunsigned char sha1[20];\n-\t\tconst char *cp;\n-\t\tint bad = 0;\n+\tif (verbose)\n+\t\treport_garbage(\"garbage found\", path);\n+}\n \n-\t\tif (is_dot_or_dotdot(ent->d_name))\n-\t\t\tcontinue;\n-\t\tfor (cp = ent->d_name; *cp; cp++) {\n-\t\t\tint ch = *cp;\n-\t\t\tif (('0' <= ch && ch <= '9') ||\n-\t\t\t ('a' <= ch && ch <= 'f'))\n-\t\t\t\tcontinue;\n-\t\t\tbad = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (cp - ent->d_name != 38)\n-\t\t\tbad = 1;\n-\t\telse {\n-\t\t\tstruct stat st;\n-\t\t\tmemcpy(path + len + 3, ent->d_name, 38);\n-\t\t\tpath[len + 2] = '/';\n-\t\t\tpath[len + 41] = 0;\n-\t\t\tif (lstat(path, &st) || !S_ISREG(st.st_mode))\n-\t\t\t\tbad = 1;\n-\t\t\telse\n-\t\t\t\t(*loose_size) += on_disk_bytes(st);\n-\t\t}\n-\t\tif (bad) {\n-\t\t\tif (verbose) {\n-\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\t\tstrbuf_addf(&sb, \"%.*s/%s\",\n-\t\t\t\t\t len + 2, path, ent->d_name);\n-\t\t\t\treport_garbage(\"garbage found\", sb.buf);\n-\t\t\t\tstrbuf_release(&sb);\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\t\t(*loose)++;\n-\t\tif (!verbose)\n-\t\t\tcontinue;\n-\t\tmemcpy(hex, path+len, 2);\n-\t\tmemcpy(hex+2, ent->d_name, 38);\n-\t\thex[40] = 0;\n-\t\tif (get_sha1_hex(hex, sha1))\n-\t\t\tdie(\"internal error\");\n-\t\tif (has_sha1_pack(sha1))\n-\t\t\t(*packed_loose)++;\n+static int count_loose(const unsigned char *sha1, const char *path, void *data)\n+{\n+\tstruct stat st;\n+\n+\tif (lstat(path, &st) || !S_ISREG(st.st_mode))\n+\t\tloose_garbage(path);\n+\telse {\n+\t\tloose_size += on_disk_bytes(st);\n+\t\tloose++;\n+\t\tif (verbose && has_sha1_pack(sha1))\n+\t\t\tpacked_loose++;\n \t}\n+\treturn 0;\n+}\n+\n+static int count_cruft(const char *basename, const char *path, void *data)\n+{\n+\tloose_garbage(path);\n+\treturn 0;\n }\n \n static char const * const count_objects_usage[] = {\n@@ -85,12 +58,7 @@ static char const * const count_objects_usage[] = {\n \n int cmd_count_objects(int argc, const char **argv, const char *prefix)\n {\n-\tint i, verbose = 0, human_readable = 0;\n-\tconst char *objdir = get_object_directory();\n-\tint len = strlen(objdir);\n-\tchar *path = xmalloc(len + 50);\n-\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n-\toff_t loose_size = 0;\n+\tint human_readable = 0;\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_BOOL('H', \"human-readable\", &human_readable,\n@@ -104,19 +72,10 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(count_objects_usage, opts);\n \tif (verbose)\n \t\treport_garbage = real_report_garbage;\n-\tmemcpy(path, objdir, len);\n-\tif (len && objdir[len-1] != '/')\n-\t\tpath[len++] = '/';\n-\tfor (i = 0; i < 256; i++) {\n-\t\tDIR *d;\n-\t\tsprintf(path + len, \"%02x\", i);\n-\t\td = opendir(path);\n-\t\tif (!d)\n-\t\t\tcontinue;\n-\t\tcount_objects(d, path, len, verbose,\n-\t\t\t &loose, &loose_size, &packed_loose);\n-\t\tclosedir(d);\n-\t}\n+\n+\tfor_each_loose_file_in_objdir(get_object_directory(),\n+\t\t\t\t count_loose, count_cruft, NULL, NULL);\n+\n \tif (verbose) {\n \t\tstruct packed_git *p;\n \t\tunsigned long num_pack = 0;\n-- \n2.1.2.596.g7379948\n"},{"id":"250725","messageId":"20141015224121.GM25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 13/25] sha1_file: add for_each iterators for loose and packed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:41:21Z","receivedAt":"2014-10-15T22:41:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We typically iterate over the reachable objects in a\nrepository by starting at the tips and walking the graph.\nThere's no easy way to iterate over all of the objects,\nincluding unreachable ones. Let's provide a way of doing so.\n\nSigned-off-by: Jeff King \n---\n cache.h | 11 +++++++++++\n sha1_file.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 8ffefaa..d24dd32 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1254,6 +1254,17 @@ int for_each_loose_file_in_objdir(const char *path,\n \t\t\t\t each_loose_subdir_fn subdir_cb,\n \t\t\t\t void *data);\n \n+/*\n+ * Iterate over loose and packed objects in both the local\n+ * repository and any alternates repositories.\n+ */\n+typedef int each_packed_object_fn(const unsigned char *sha1,\n+\t\t\t\t struct packed_git *pack,\n+\t\t\t\t uint32_t pos,\n+\t\t\t\t void *data);\n+extern int for_each_loose_object(each_loose_object_fn, void *);\n+extern int for_each_packed_object(each_packed_object_fn, void *);\n+\n struct object_info {\n \t/* Request */\n \tenum object_type *typep;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a20240b..eb410a2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3349,3 +3349,65 @@ int for_each_loose_file_in_objdir(const char *path,\n \tstrbuf_release(&buf);\n \treturn r;\n }\n+\n+struct loose_alt_odb_data {\n+\teach_loose_object_fn *cb;\n+\tvoid *data;\n+};\n+\n+static int loose_from_alt_odb(struct alternate_object_database *alt,\n+\t\t\t void *vdata)\n+{\n+\tstruct loose_alt_odb_data *data = vdata;\n+\treturn for_each_loose_file_in_objdir(alt->base,\n+\t\t\t\t\t data->cb, NULL, NULL,\n+\t\t\t\t\t data->data);\n+}\n+\n+int for_each_loose_object(each_loose_object_fn cb, void *data)\n+{\n+\tstruct loose_alt_odb_data alt;\n+\tint r;\n+\n+\tr = for_each_loose_file_in_objdir(get_object_directory(),\n+\t\t\t\t\t cb, NULL, NULL, data);\n+\tif (r)\n+\t\treturn r;\n+\n+\talt.cb = cb;\n+\talt.data = data;\n+\treturn foreach_alt_odb(loose_from_alt_odb, &alt);\n+}\n+\n+static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn cb, void *data)\n+{\n+\tuint32_t i;\n+\tint r = 0;\n+\n+\tfor (i = 0; i < p->num_objects; i++) {\n+\t\tconst unsigned char *sha1 = nth_packed_object_sha1(p, i);\n+\n+\t\tif (!sha1)\n+\t\t\treturn error(\"unable to get sha1 of object %u in %s\",\n+\t\t\t\t i, p->pack_name);\n+\n+\t\tr = cb(sha1, p, i, data);\n+\t\tif (r)\n+\t\t\tbreak;\n+\t}\n+\treturn r;\n+}\n+\n+int for_each_packed_object(each_packed_object_fn cb, void *data)\n+{\n+\tstruct packed_git *p;\n+\tint r = 0;\n+\n+\tprepare_packed_git();\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tr = for_each_object_in_pack(p, cb, data);\n+\t\tif (r)\n+\t\t\tbreak;\n+\t}\n+\treturn r;\n+}\n-- \n2.1.2.596.g7379948\n"},{"id":"250726","messageId":"20141015224135.GN25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 14/25] prune: keep objects reachable from recent objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:41:35Z","receivedAt":"2014-10-15T22:41:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our current strategy with prune is that an object falls into\none of three categories:\n\n 1. Reachable (from ref tips, reflogs, index, etc).\n\n 2. Not reachable, but recent (based on the --expire time).\n\n 3. Not reachable and not recent.\n\nWe keep objects from (1) and (2), but prune objects in (3).\nThe point of (2) is that these objects may be part of an\nin-progress operation that has not yet updated any refs.\n\nHowever, it is not always the case that objects for an\nin-progress operation will have a recent mtime. For example,\nthe object database may have an old copy of a blob (from an\nabandoned operation, a branch that was deleted, etc). If we\ncreate a new tree that points to it, a simultaneous prune\nwill leave our tree, but delete the blob. Referencing that\ntree with a commit will then work (we check that the tree is\nin the object database, but not that all of its referred\nobjects are), as will mentioning the commit in a ref. But\nthe resulting repo is corrupt; we are missing the blob\nreachable from a ref.\n\nOne way to solve this is to be more thorough when\nreferencing a sha1: make sure that not only do we have that\nsha1, but that we have objects it refers to, and so forth\nrecursively. The problem is that this is very expensive.\nCreating a parent link would require traversing the entire\nobject graph!\n\nInstead, this patch pushes the extra work onto prune, which\nruns less frequently (and has to look at the whole object\ngraph anyway). It creates a new category of objects: objects\nwhich are not recent, but which are reachable from a recent\nobject. We do not prune these objects, just like the\nreachable and recent ones.\n\nThis lets us avoid the recursive check above, because if we\nhave an object, even if it is unreachable, we should have\nits referent. We can make a simple inductive argument that\nwith this patch, this property holds (that there are no\nobjects with missing referents in the repository):\n\n 0. When we have no objects, we have nothing to refer or be\n referred to, so the property holds.\n\n 1. If we add objects to the repository, their direct\n referents must generally exist (e.g., if you create a\n tree, the blobs it references must exist; if you create\n a commit to point at the tree, the tree must exist).\n This is already the case before this patch. And it is\n not 100% foolproof (you can make bogus objects using\n `git hash-object`, for example), but it should be the\n case for normal usage.\n\n Therefore for any sequence of object additions, the\n property will continue to hold.\n\n 2. If we remove objects from the repository, then we will\n not remove a child object (like a blob) if an object\n that refers to it is being kept. That is the part\n implemented by this patch.\n\n Note, however, that our reachability check and the\n actual pruning are not atomic. So it _is_ still\n possible to violate the property (e.g., an object\n becomes referenced just as we are deleting it). This\n patch is shooting for eliminating problems where the\n mtimes of dependent objects differ by hours or days,\n and one is dropped without the other. It does nothing\n to help with short races.\n\nNaively, the simplest way to implement this would be to add\nall recent objects as tips to the reachability traversal.\nHowever, this does not perform well. In a recently-packed\nrepository, all reachable objects will also be recent, and\ntherefore we have to look at each object twice. This patch\ninstead performs the reachability traversal, then follows up\nwith a second traversal for recent objects, skipping any\nthat have already been marked.\n\nSigned-off-by: Jeff King \n---\n builtin/prune.c | 2 +-\n builtin/reflog.c | 2 +-\n reachable.c | 112 +++++++++++++++++++++++++++++++++++++++++++++\n reachable.h | 3 +-\n t/t6501-freshen-objects.sh | 88 +++++++++++++++++++++++++++++++++++\n 5 files changed, 204 insertions(+), 3 deletions(-)\n create mode 100755 t/t6501-freshen-objects.sh\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 763f53e..04d3b12 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -135,7 +135,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \n-\tmark_reachable_objects(&revs, 1, progress);\n+\tmark_reachable_objects(&revs, 1, expire, progress);\n \tstop_progress(&progress);\n \tfor_each_loose_file_in_objdir(get_object_directory(), prune_object,\n \t\t\t\t prune_cruft, prune_subdir, NULL);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b6388f7..2d85d26 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -649,7 +649,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tinit_revisions(&cb.revs, prefix);\n \t\tif (cb.verbose)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.revs, 0, NULL);\n+\t\tmark_reachable_objects(&cb.revs, 0, 0, NULL);\n \t\tif (cb.verbose)\n \t\t\tputchar('\\n');\n \t}\ndiff --git a/reachable.c b/reachable.c\nindex d03f829..55589a0 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -97,7 +97,109 @@ static void mark_commit(struct commit *c, void *data)\n \tmark_object(&c->object, NULL, NULL, data);\n }\n \n+struct recent_data {\n+\tstruct rev_info *revs;\n+\tunsigned long timestamp;\n+};\n+\n+static void add_recent_object(const unsigned char *sha1,\n+\t\t\t unsigned long mtime,\n+\t\t\t struct recent_data *data)\n+{\n+\tstruct object *obj;\n+\tenum object_type type;\n+\n+\tif (mtime <= data->timestamp)\n+\t\treturn;\n+\n+\t/*\n+\t * We do not want to call parse_object here, because\n+\t * inflating blobs and trees could be very expensive.\n+\t * However, we do need to know the correct type for\n+\t * later processing, and the revision machinery expects\n+\t * commits and tags to have been parsed.\n+\t */\n+\ttype = sha1_object_info(sha1, NULL);\n+\tif (type < 0)\n+\t\tdie(\"unable to get object info for %s\", sha1_to_hex(sha1));\n+\n+\tswitch (type) {\n+\tcase OBJ_TAG:\n+\tcase OBJ_COMMIT:\n+\t\tobj = parse_object_or_die(sha1, NULL);\n+\t\tbreak;\n+\tcase OBJ_TREE:\n+\t\tobj = (struct object *)lookup_tree(sha1);\n+\t\tbreak;\n+\tcase OBJ_BLOB:\n+\t\tobj = (struct object *)lookup_blob(sha1);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"unknown object type for %s: %s\",\n+\t\t sha1_to_hex(sha1), typename(type));\n+\t}\n+\n+\tif (!obj)\n+\t\tdie(\"unable to lookup %s\", sha1_to_hex(sha1));\n+\n+\tadd_pending_object(data->revs, obj, \"\");\n+}\n+\n+static int add_recent_loose(const unsigned char *sha1,\n+\t\t\t const char *path, void *data)\n+{\n+\tstruct stat st;\n+\tstruct object *obj = lookup_object(sha1);\n+\n+\tif (obj && obj->flags & SEEN)\n+\t\treturn 0;\n+\n+\tif (stat(path, &st) < 0) {\n+\t\t/*\n+\t\t * It's OK if an object went away during our iteration; this\n+\t\t * could be due to a simultaneous repack. But anything else\n+\t\t * we should abort, since we might then fail to mark objects\n+\t\t * which should not be pruned.\n+\t\t */\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn error(\"unable to stat %s: %s\",\n+\t\t\t sha1_to_hex(sha1), strerror(errno));\n+\t}\n+\n+\tadd_recent_object(sha1, st.st_mtime, data);\n+\treturn 0;\n+}\n+\n+static int add_recent_packed(const unsigned char *sha1,\n+\t\t\t struct packed_git *p, uint32_t pos,\n+\t\t\t void *data)\n+{\n+\tstruct object *obj = lookup_object(sha1);\n+\n+\tif (obj && obj->flags & SEEN)\n+\t\treturn 0;\n+\tadd_recent_object(sha1, p->mtime, data);\n+\treturn 0;\n+}\n+\n+static int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n+\t\t\t\t\t\t unsigned long timestamp)\n+{\n+\tstruct recent_data data;\n+\tint r;\n+\n+\tdata.revs = revs;\n+\tdata.timestamp = timestamp;\n+\n+\tr = for_each_loose_object(add_recent_loose, &data);\n+\tif (r)\n+\t\treturn r;\n+\treturn for_each_packed_object(add_recent_packed, &data);\n+}\n+\n void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n+\t\t\t unsigned long mark_recent,\n \t\t\t struct progress *progress)\n {\n \tstruct connectivity_progress cp;\n@@ -133,5 +235,15 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n \ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n+\n+\tif (mark_recent) {\n+\t\trevs->ignore_missing_links = 1;\n+\t\tif (add_unseen_recent_objects_to_traversal(revs, mark_recent))\n+\t\t\tdie(\"unable to mark recent objects\");\n+\t\tif (prepare_revision_walk(revs))\n+\t\t\tdie(\"revision walk setup failed\");\n+\t\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n+\t}\n+\n \tdisplay_progress(cp.progress, cp.count);\n }\ndiff --git a/reachable.h b/reachable.h\nindex 5d082ad..141fe30 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -2,6 +2,7 @@\n #define REACHEABLE_H\n \n struct progress;\n-extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog, struct progress *);\n+extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n+\t\t\t\t unsigned long mark_recent, struct progress *);\n \n #endif\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nnew file mode 100755\nindex 0000000..de941c2\n--- /dev/null\n+++ b/t/t6501-freshen-objects.sh\n@@ -0,0 +1,88 @@\n+#!/bin/sh\n+#\n+# This test covers the handling of objects which might have old\n+# mtimes in the filesystem (because they were used previously)\n+# and are just now becoming referenced again.\n+#\n+# We're going to do two things that are a little bit \"fake\" to\n+# help make our simulation easier:\n+#\n+# 1. We'll turn off reflogs. You can still run into\n+# problems with reflogs on, but your objects\n+# don't get pruned until both the reflog expiration\n+# has passed on their references, _and_ they are out\n+# of prune's expiration period. Dropping reflogs\n+# means we only have to deal with one variable in our tests,\n+# but the results generalize.\n+#\n+# 2. We'll use a temporary index file to create our\n+# works-in-progress. Most workflows would mention\n+# referenced objects in the index, which prune takes\n+# into account. However, many operations don't. For\n+# example, a partial commit with \"git commit foo\"\n+# will use a temporary index. Or they may not need\n+# an index at all (e.g., creating a new commit\n+# to refer to an existing tree).\n+\n+test_description='check pruning of dependent objects'\n+. ./test-lib.sh\n+\n+# We care about reachability, so we do not want to use\n+# the normal test_commit, which creates extra tags.\n+add () {\n+\techo \"$1\" >\"$1\" &&\n+\tgit add \"$1\"\n+}\n+commit () {\n+\ttest_tick &&\n+\tadd \"$1\" &&\n+\tgit commit -m \"$1\"\n+}\n+\n+test_expect_success 'disable reflogs' '\n+\tgit config core.logallrefupdates false &&\n+\trm -rf .git/logs\n+'\n+\n+test_expect_success 'setup basic history' '\n+\tcommit base\n+'\n+\n+test_expect_success 'create and abandon some objects' '\n+\tgit checkout -b experiment &&\n+\tcommit abandon &&\n+\tgit checkout master &&\n+\tgit branch -D experiment\n+'\n+\n+test_expect_success 'simulate time passing' '\n+\tfind .git/objects -type f |\n+\txargs test-chmtime -v -86400\n+'\n+\n+test_expect_success 'start writing new commit with old blob' '\n+\ttree=$(\n+\t\tGIT_INDEX_FILE=index.tmp &&\n+\t\texport GIT_INDEX_FILE &&\n+\t\tgit read-tree HEAD &&\n+\t\tadd unrelated &&\n+\t\tadd abandon &&\n+\t\tgit write-tree\n+\t)\n+'\n+\n+test_expect_success 'simultaneous gc' '\n+\tgit gc --prune=12.hours.ago\n+'\n+\n+test_expect_success 'finish writing out commit' '\n+\tcommit=$(echo foo | git commit-tree -p HEAD $tree) &&\n+\tgit update-ref HEAD $commit\n+'\n+\n+# \"abandon\" blob should have been rescued by reference from new tree\n+test_expect_success 'repository passes fsck' '\n+\tgit fsck\n+'\n+\n+test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250727","messageId":"20141015224153.GO25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 15/25] pack-objects: refactor unpack-unreachable expiration check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:41:53Z","receivedAt":"2014-10-15T22:41:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are loosening unreachable packed objects, we do not\nbother to process objects that would simply be pruned\nimmediately anyway. The \"would be pruned\" check is a simple\ncomparison, but is about to get more complicated. Let's pull\nit out into a separate function.\n\nNote that this is slightly less efficient than the original,\nwhich avoided even opening old packs, since no object in\nthem could pass the current check, which cares only about\nthe pack mtime. But the new rules will depend on the exact\nobject, so we need to perform the check even for old packs.\n\nNote also that we fix a minor buglet when the pack mtime is\nexactly the same as the expiration time. The prune code\nconsiders that worth pruning, whereas our check here\nconsidered it worth keeping. This wasn't a big deal. Besides\nbeing unlikely to happen, the result was simply that the\nobject was loosened and then pruned, missing the\noptimization. Still, we can easily fix it while we are here.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d391934..2fe2ab0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2407,6 +2407,16 @@ static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n \treturn 0;\n }\n \n+static int loosened_object_can_be_discarded(const unsigned char *sha1,\n+\t\t\t\t\t unsigned long mtime)\n+{\n+\tif (!unpack_unreachable_expiration)\n+\t\treturn 0;\n+\tif (mtime > unpack_unreachable_expiration)\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n static void loosen_unused_packed_objects(struct rev_info *revs)\n {\n \tstruct packed_git *p;\n@@ -2417,17 +2427,14 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n \t\tif (!p->pack_local || p->pack_keep)\n \t\t\tcontinue;\n \n-\t\tif (unpack_unreachable_expiration &&\n-\t\t p->mtime < unpack_unreachable_expiration)\n-\t\t\tcontinue;\n-\n \t\tif (open_pack_index(p))\n \t\t\tdie(\"cannot open pack index\");\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tsha1 = nth_packed_object_sha1(p, i);\n \t\t\tif (!packlist_find(&to_pack, sha1, NULL) &&\n-\t\t\t\t!has_sha1_pack_kept_or_nonlocal(sha1))\n+\t\t\t !has_sha1_pack_kept_or_nonlocal(sha1) &&\n+\t\t\t !loosened_object_can_be_discarded(sha1, p->mtime))\n \t\t\t\tif (force_object_loose(sha1, p->mtime))\n \t\t\t\t\tdie(\"unable to force loose object\");\n \t\t}\n-- \n2.1.2.596.g7379948\n"},{"id":"250728","messageId":"20141015224209.GP25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 16/25] pack-objects: match prune logic for discarding objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:42:09Z","receivedAt":"2014-10-15T22:42:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A recent commit taught git-prune to keep non-recent objects\nthat are reachable from recent ones. However, pack-objects,\nwhen loosening unreachable objects, tries to optimize out\nthe write in the case that the object will be immediately\npruned. It now gets this wrong, since its rule does not\nreflect the new prune code (and this can be seen by running\nt6501 with a strategically placed repack).\n\nLet's teach pack-objects similar logic.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 39 +++++++++++++++++++\n reachable.c | 4 +-\n reachable.h | 2 +\n t/t6501-freshen-objects.sh | 93 +++++++++++++++++++++++++++-------------------\n 4 files changed, 98 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2fe2ab0..4df9499 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -20,6 +20,8 @@\n #include \"streaming.h\"\n #include \"thread-utils.h\"\n #include \"pack-bitmap.h\"\n+#include \"reachable.h\"\n+#include \"sha1-array.h\"\n \n static const char *pack_usage[] = {\n \tN_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n@@ -2407,6 +2409,15 @@ static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n \treturn 0;\n }\n \n+/*\n+ * Store a list of sha1s that are should not be discarded\n+ * because they are either written too recently, or are\n+ * reachable from another object that was.\n+ *\n+ * This is filled by get_object_list.\n+ */\n+static struct sha1_array recent_objects;\n+\n static int loosened_object_can_be_discarded(const unsigned char *sha1,\n \t\t\t\t\t unsigned long mtime)\n {\n@@ -2414,6 +2425,8 @@ static int loosened_object_can_be_discarded(const unsigned char *sha1,\n \t\treturn 0;\n \tif (mtime > unpack_unreachable_expiration)\n \t\treturn 0;\n+\tif (sha1_array_lookup(&recent_objects, sha1) >= 0)\n+\t\treturn 0;\n \treturn 1;\n }\n \n@@ -2470,6 +2483,19 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n \treturn 0;\n }\n \n+static void record_recent_object(struct object *obj,\n+\t\t\t\t const struct name_path *path,\n+\t\t\t\t const char *last,\n+\t\t\t\t void *data)\n+{\n+\tsha1_array_append(&recent_objects, obj->sha1);\n+}\n+\n+static void record_recent_commit(struct commit *commit, void *data)\n+{\n+\tsha1_array_append(&recent_objects, commit->object.sha1);\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n@@ -2517,10 +2543,23 @@ static void get_object_list(int ac, const char **av)\n \tmark_edges_uninteresting(&revs, show_edge);\n \ttraverse_commit_list(&revs, show_commit, show_object, NULL);\n \n+\tif (unpack_unreachable_expiration) {\n+\t\trevs.ignore_missing_links = 1;\n+\t\tif (add_unseen_recent_objects_to_traversal(&revs,\n+\t\t\t\tunpack_unreachable_expiration))\n+\t\t\tdie(\"unable to add recent objects\");\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(\"revision walk setup failed\");\n+\t\ttraverse_commit_list(&revs, record_recent_commit,\n+\t\t\t\t record_recent_object, NULL);\n+\t}\n+\n \tif (keep_unreachable)\n \t\tadd_objects_in_unpacked_packs(&revs);\n \tif (unpack_unreachable)\n \t\tloosen_unused_packed_objects(&revs);\n+\n+\tsha1_array_clear(&recent_objects);\n }\n \n static int option_parse_index_version(const struct option *opt,\ndiff --git a/reachable.c b/reachable.c\nindex 55589a0..0176a88 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -183,8 +183,8 @@ static int add_recent_packed(const unsigned char *sha1,\n \treturn 0;\n }\n \n-static int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n-\t\t\t\t\t\t unsigned long timestamp)\n+int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n+\t\t\t\t\t unsigned long timestamp)\n {\n \tstruct recent_data data;\n \tint r;\ndiff --git a/reachable.h b/reachable.h\nindex 141fe30..d23efc3 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -2,6 +2,8 @@\n #define REACHEABLE_H\n \n struct progress;\n+extern int add_unseen_recent_objects_to_traversal(struct rev_info *revs,\n+\t\t\t\t\t\t unsigned long timestamp);\n extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\t\t\t unsigned long mark_recent, struct progress *);\n \ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex de941c2..e25c47d 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -39,50 +39,67 @@ commit () {\n \tgit commit -m \"$1\"\n }\n \n-test_expect_success 'disable reflogs' '\n-\tgit config core.logallrefupdates false &&\n-\trm -rf .git/logs\n-'\n+maybe_repack () {\n+\tif test -n \"$repack\"; then\n+\t\tgit repack -ad\n+\tfi\n+}\n+\n+for repack in '' true; do\n+\ttitle=${repack:+repack}\n+\ttitle=${title:-loose}\n+\n+\ttest_expect_success \"make repo completely empty ($title)\" '\n+\t\trm -rf .git &&\n+\t\tgit init\n+\t'\n+\n+\ttest_expect_success \"disable reflogs ($title)\" '\n+\t\tgit config core.logallrefupdates false &&\n+\t\trm -rf .git/logs\n+\t'\n \n-test_expect_success 'setup basic history' '\n-\tcommit base\n-'\n+\ttest_expect_success \"setup basic history ($title)\" '\n+\t\tcommit base\n+\t'\n \n-test_expect_success 'create and abandon some objects' '\n-\tgit checkout -b experiment &&\n-\tcommit abandon &&\n-\tgit checkout master &&\n-\tgit branch -D experiment\n-'\n+\ttest_expect_success \"create and abandon some objects ($title)\" '\n+\t\tgit checkout -b experiment &&\n+\t\tcommit abandon &&\n+\t\tmaybe_repack &&\n+\t\tgit checkout master &&\n+\t\tgit branch -D experiment\n+\t'\n \n-test_expect_success 'simulate time passing' '\n-\tfind .git/objects -type f |\n-\txargs test-chmtime -v -86400\n-'\n+\ttest_expect_success \"simulate time passing ($title)\" '\n+\t\tfind .git/objects -type f |\n+\t\txargs test-chmtime -v -86400\n+\t'\n \n-test_expect_success 'start writing new commit with old blob' '\n-\ttree=$(\n-\t\tGIT_INDEX_FILE=index.tmp &&\n-\t\texport GIT_INDEX_FILE &&\n-\t\tgit read-tree HEAD &&\n-\t\tadd unrelated &&\n-\t\tadd abandon &&\n-\t\tgit write-tree\n-\t)\n-'\n+\ttest_expect_success \"start writing new commit with old blob ($title)\" '\n+\t\ttree=$(\n+\t\t\tGIT_INDEX_FILE=index.tmp &&\n+\t\t\texport GIT_INDEX_FILE &&\n+\t\t\tgit read-tree HEAD &&\n+\t\t\tadd unrelated &&\n+\t\t\tadd abandon &&\n+\t\t\tgit write-tree\n+\t\t)\n+\t'\n \n-test_expect_success 'simultaneous gc' '\n-\tgit gc --prune=12.hours.ago\n-'\n+\ttest_expect_success \"simultaneous gc ($title)\" '\n+\t\tgit gc --prune=12.hours.ago\n+\t'\n \n-test_expect_success 'finish writing out commit' '\n-\tcommit=$(echo foo | git commit-tree -p HEAD $tree) &&\n-\tgit update-ref HEAD $commit\n-'\n+\ttest_expect_success \"finish writing out commit ($title)\" '\n+\t\tcommit=$(echo foo | git commit-tree -p HEAD $tree) &&\n+\t\tgit update-ref HEAD $commit\n+\t'\n \n-# \"abandon\" blob should have been rescued by reference from new tree\n-test_expect_success 'repository passes fsck' '\n-\tgit fsck\n-'\n+\t# \"abandon\" blob should have been rescued by reference from new tree\n+\ttest_expect_success \"repository passes fsck ($title)\" '\n+\t\tgit fsck\n+\t'\n+done\n \n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250729","messageId":"20141015224222.GQ25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 17/25] write_sha1_file: freshen existing objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:42:22Z","receivedAt":"2014-10-15T22:42:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we try to write a loose object file, we first check\nwhether that object already exists. If so, we skip the\nwrite as an optimization. However, this can interfere with\nprune's strategy of using mtimes to mark files in progress.\n\nFor example, if a branch contains a particular tree object\nand is deleted, that tree object may become unreachable, and\nhave an old mtime. If a new operation then tries to write\nthe same tree, this ends up as a noop; we notice we\nalready have the object and do nothing. A prune running\nsimultaneously with this operation will see the object as\nold, and may delete it.\n\nWe can solve this by \"freshening\" objects that we avoid\nwriting by updating their mtime. The algorithm for doing so\nis essentially the same as that of has_sha1_file. Therefore\nwe provide a new (static) interface \"check_and_freshen\",\nwhich finds and optionally freshens the object. It's trivial\nto implement freshening and simple checking by tweaking a\nsingle parameter.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 51 +++++++++++++++++++++++++++++++++++++++-------\n t/t6501-freshen-objects.sh | 27 ++++++++++++++++++++++++\n 2 files changed, 71 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex eb410a2..d7f1838 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -443,27 +443,53 @@ void prepare_alt_odb(void)\n \tread_info_alternates(get_object_directory(), 0);\n }\n \n-static int has_loose_object_local(const unsigned char *sha1)\n+static int freshen_file(const char *fn)\n {\n-\treturn !access(sha1_file_name(sha1), F_OK);\n+\tstruct utimbuf t;\n+\tt.actime = t.modtime = time(NULL);\n+\treturn !utime(fn, &t);\n }\n \n-int has_loose_object_nonlocal(const unsigned char *sha1)\n+static int check_and_freshen_file(const char *fn, int freshen)\n+{\n+\tif (access(fn, F_OK))\n+\t\treturn 0;\n+\tif (freshen && freshen_file(fn))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n+static int check_and_freshen_local(const unsigned char *sha1, int freshen)\n+{\n+\treturn check_and_freshen_file(sha1_file_name(sha1), freshen);\n+}\n+\n+static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen)\n {\n \tstruct alternate_object_database *alt;\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tfill_sha1_path(alt->name, sha1);\n-\t\tif (!access(alt->base, F_OK))\n+\t\tif (check_and_freshen_file(alt->base, freshen))\n \t\t\treturn 1;\n \t}\n \treturn 0;\n }\n \n+static int check_and_freshen(const unsigned char *sha1, int freshen)\n+{\n+\treturn check_and_freshen_local(sha1, freshen) ||\n+\t check_and_freshen_nonlocal(sha1, freshen);\n+}\n+\n+int has_loose_object_nonlocal(const unsigned char *sha1)\n+{\n+\treturn check_and_freshen_nonlocal(sha1, 0);\n+}\n+\n static int has_loose_object(const unsigned char *sha1)\n {\n-\treturn has_loose_object_local(sha1) ||\n-\t has_loose_object_nonlocal(sha1);\n+\treturn check_and_freshen(sha1, 0);\n }\n \n static unsigned int pack_used_ctr;\n@@ -2966,6 +2992,17 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmp_file, filename);\n }\n \n+static int freshen_loose_object(const unsigned char *sha1)\n+{\n+\treturn check_and_freshen(sha1, 1);\n+}\n+\n+static int freshen_packed_object(const unsigned char *sha1)\n+{\n+\tstruct pack_entry e;\n+\treturn find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n+}\n+\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n \tunsigned char sha1[20];\n@@ -2978,7 +3015,7 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n-\tif (has_sha1_file(sha1))\n+\tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\ndiff --git a/t/t6501-freshen-objects.sh b/t/t6501-freshen-objects.sh\nindex e25c47d..157f3f9 100755\n--- a/t/t6501-freshen-objects.sh\n+++ b/t/t6501-freshen-objects.sh\n@@ -100,6 +100,33 @@ for repack in '' true; do\n \ttest_expect_success \"repository passes fsck ($title)\" '\n \t\tgit fsck\n \t'\n+\n+\ttest_expect_success \"abandon objects again ($title)\" '\n+\t\tgit reset --hard HEAD^ &&\n+\t\tfind .git/objects -type f |\n+\t\txargs test-chmtime -v -86400\n+\t'\n+\n+\ttest_expect_success \"start writing new commit with same tree ($title)\" '\n+\t\ttree=$(\n+\t\t\tGIT_INDEX_FILE=index.tmp &&\n+\t\t\texport GIT_INDEX_FILE &&\n+\t\t\tgit read-tree HEAD &&\n+\t\t\tadd abandon &&\n+\t\t\tadd unrelated &&\n+\t\t\tgit write-tree\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"simultaneous gc ($title)\" '\n+\t\tgit gc --prune=12.hours.ago\n+\t'\n+\n+\t# tree should have been refreshed by write-tree\n+\ttest_expect_success \"finish writing out commit ($title)\" '\n+\t\tcommit=$(echo foo | git commit-tree -p HEAD $tree) &&\n+\t\tgit update-ref HEAD $commit\n+\t'\n done\n \n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250730","messageId":"20141015224256.GR25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 18/25] make add_object_array_with_context interface more sane","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:42:57Z","receivedAt":"2014-10-15T22:42:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When you resolve a sha1, you can optionally keep any context\nfound during the resolution, including the path and mode of\na tree entry (e.g., when looking up \"HEAD:subdir/file.c\").\n\nThe add_object_array_with_context function lets you then\nattach that context to an entry in a list. Unfortunately,\nthe interface for doing so is horrible. The object_context\nstructure is large and most object_array users do not use\nit. Therefore we keep a pointer to the structure to avoid\nburdening other users too much. But that means when we do\nuse it that we must allocate the struct ourselves. And the\nstruct contains a fixed PATH_MAX-sized buffer, which makes\nthis wholly unsuitable for any large arrays.\n\nWe can observe that there is only a single user of the\n\"with_context\" variant: builtin/grep.c. And in that use\ncase, the only element we care about is the path. We can\ntherefore store only the path as a pointer (the context's\nmode field was redundant with the object_array_entry itself,\nand nobody actually cared about the surrounding tree). This\nstill requires a strdup of the pathname, but at least we are\nonly consuming the minimum amount of memory for each string.\n\nWe can also handle the copying ourselves in\nadd_object_array_*, and free it as appropriate in\nobject_array_release_entry.\n\nSigned-off-by: Jeff King \n---\n builtin/grep.c | 8 ++++----\n object.c | 23 +++++++++--------------\n object.h | 4 ++--\n 3 files changed, 15 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex c86a142..4063882 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -456,10 +456,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n }\n \n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n-\t\t struct object *obj, const char *name, struct object_context *oc)\n+\t\t struct object *obj, const char *name, const char *path)\n {\n \tif (obj->type == OBJ_BLOB)\n-\t\treturn grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL);\n+\t\treturn grep_sha1(opt, obj->sha1, name, 0, path);\n \tif (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) {\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n@@ -501,7 +501,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \t\treal_obj = deref_tag(list->objects[i].item, NULL, 0);\n-\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].context)) {\n+\t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path)) {\n \t\t\thit = 1;\n \t\t\tif (opt->status_only)\n \t\t\t\tbreak;\n@@ -821,7 +821,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tstruct object *object = parse_object_or_die(sha1, arg);\n \t\t\tif (!seen_dashdash)\n \t\t\t\tverify_non_filename(prefix, arg);\n-\t\t\tadd_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));\n+\t\t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\ndiff --git a/object.c b/object.c\nindex 6aeb1bb..df86bdd 100644\n--- a/object.c\n+++ b/object.c\n@@ -307,10 +307,9 @@ int object_list_contains(struct object_list *list, struct object *obj)\n */\n static char object_array_slopbuf[1];\n \n-static void add_object_array_with_mode_context(struct object *obj, const char *name,\n-\t\t\t\t\t struct object_array *array,\n-\t\t\t\t\t unsigned mode,\n-\t\t\t\t\t struct object_context *context)\n+void add_object_array_with_path(struct object *obj, const char *name,\n+\t\t\t\tstruct object_array *array,\n+\t\t\t\tunsigned mode, const char *path)\n {\n \tunsigned nr = array->nr;\n \tunsigned alloc = array->alloc;\n@@ -333,7 +332,10 @@ static void add_object_array_with_mode_context(struct object *obj, const char *n\n \telse\n \t\tentry->name = xstrdup(name);\n \tentry->mode = mode;\n-\tentry->context = context;\n+\tif (path)\n+\t\tentry->path = xstrdup(path);\n+\telse\n+\t\tentry->path = NULL;\n \tarray->nr = ++nr;\n }\n \n@@ -344,15 +346,7 @@ void add_object_array(struct object *obj, const char *name, struct object_array\n \n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)\n {\n-\tadd_object_array_with_mode_context(obj, name, array, mode, NULL);\n-}\n-\n-void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context)\n-{\n-\tif (context)\n-\t\tadd_object_array_with_mode_context(obj, name, array, context->mode, context);\n-\telse\n-\t\tadd_object_array_with_mode_context(obj, name, array, S_IFINVALID, context);\n+\tadd_object_array_with_path(obj, name, array, mode, NULL);\n }\n \n /*\n@@ -363,6 +357,7 @@ static void object_array_release_entry(struct object_array_entry *ent)\n {\n \tif (ent->name != object_array_slopbuf)\n \t\tfree(ent->name);\n+\tfree(ent->path);\n }\n \n void object_array_filter(struct object_array *array,\ndiff --git a/object.h b/object.h\nindex 2a755a2..e5178a5 100644\n--- a/object.h\n+++ b/object.h\n@@ -18,8 +18,8 @@ struct object_array {\n \t\t * empty string.\n \t\t */\n \t\tchar *name;\n+\t\tchar *path;\n \t\tunsigned mode;\n-\t\tstruct object_context *context;\n \t} *objects;\n };\n \n@@ -115,7 +115,7 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);\n-void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context);\n+void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n \n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n \n-- \n2.1.2.596.g7379948\n"},{"id":"250731","messageId":"20141015224318.GS25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 19/25] traverse_commit_list: support pending blobs/trees with paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:43:19Z","receivedAt":"2014-10-15T22:43:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call traverse_commit_list, we may have trees and\nblobs in the pending array. As we process these, we pass the\n\"name\" field from the pending entry as the path of the\nobject within the tree (which then becomes the root path if\nwe recurse into subtrees).\n\nWhen we set up the traversal in prepare_revision_walk,\nthough, the \"name\" field of any pending trees and blobs is\nlikely to be the ref at which we found the object. We would\nnot want to make this part of the path (e.g., doing so would\nmake \"git rev-list --objects v2.6.11-tree\" in linux.git show\npaths like \"v2.6.11-tree/Makefile\", which is nonsensical).\nTherefore prepare_revision_walk sets the name field of each\npending tree and blobs to the empty string.\n\nHowever, this leaves no room for a caller who does know the\ncorrect path of a pending object to propagate that\ninformation to the revision walker. We can fix this by\nmaking two related changes:\n\n 1. Use the \"path\" field as the path instead of the \"name\"\n field in traverse_commit_list. If the path is not set,\n default to \"\" (which is what we always ended up with in\n the current code, because of prepare_revision_walk).\n\n 2. In prepare_revision_walk, make a complete copy of the\n entry. This makes the path field available to the\n walker (if there is one), solving our problem.\n Leaving the name field intact is now OK, as we do not\n use it as a path due to point (1) above (and we can use\n it to make more meaningful error messages if we want).\n We also make the original \"mode\" field available to the\n walker, though it does not actually use it.\n\nNote that we still re-add the pending objects and free the\nold ones (so we may strdup the path and name only to free\nthe old ones). This could be made more efficient by simply\ncopying the object_array entries that we are keeping.\nHowever, that would require more restructuring of the code,\nand is not done here.\n\nSigned-off-by: Jeff King \n---\n list-objects.c | 7 +++++--\n revision.c | 30 +++++++++++++++++++++++-------\n 2 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex fad6808..2910bec 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -208,6 +208,7 @@ void traverse_commit_list(struct rev_info *revs,\n \t\tstruct object_array_entry *pending = revs->pending.objects + i;\n \t\tstruct object *obj = pending->item;\n \t\tconst char *name = pending->name;\n+\t\tconst char *path = pending->path;\n \t\tif (obj->flags & (UNINTERESTING | SEEN))\n \t\t\tcontinue;\n \t\tif (obj->type == OBJ_TAG) {\n@@ -215,14 +216,16 @@ void traverse_commit_list(struct rev_info *revs,\n \t\t\tshow_object(obj, NULL, name, data);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!path)\n+\t\t\tpath = \"\";\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tprocess_tree(revs, (struct tree *)obj, show_object,\n-\t\t\t\t NULL, &base, name, data);\n+\t\t\t\t NULL, &base, path, data);\n \t\t\tcontinue;\n \t\t}\n \t\tif (obj->type == OBJ_BLOB) {\n \t\t\tprocess_blob(revs, (struct blob *)obj, show_object,\n-\t\t\t\t NULL, name, data);\n+\t\t\t\t NULL, path, data);\n \t\t\tcontinue;\n \t\t}\n \t\tdie(\"unknown pending object %s (%s)\",\ndiff --git a/revision.c b/revision.c\nindex b8e02e2..9a0f99a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -198,9 +198,10 @@ void mark_parents_uninteresting(struct commit *commit)\n \t}\n }\n \n-static void add_pending_object_with_mode(struct rev_info *revs,\n+static void add_pending_object_with_path(struct rev_info *revs,\n \t\t\t\t\t struct object *obj,\n-\t\t\t\t\t const char *name, unsigned mode)\n+\t\t\t\t\t const char *name, unsigned mode,\n+\t\t\t\t\t const char *path)\n {\n \tif (!obj)\n \t\treturn;\n@@ -220,7 +221,20 @@ static void add_pending_object_with_mode(struct rev_info *revs,\n \t\tif (st)\n \t\t\treturn;\n \t}\n-\tadd_object_array_with_mode(obj, name, &revs->pending, mode);\n+\tadd_object_array_with_path(obj, name, &revs->pending, mode, path);\n+}\n+\n+static void add_pending_object_with_mode(struct rev_info *revs,\n+\t\t\t\t\t struct object *obj,\n+\t\t\t\t\t const char *name, unsigned mode)\n+{\n+\tadd_pending_object_with_path(revs, obj, name, mode, NULL);\n+}\n+\n+static void add_pending_object_from_entry(struct rev_info *revs,\n+\t\t\t\t\t struct object_array_entry *e)\n+{\n+\tadd_pending_object_with_path(revs, e->item, e->name, e->mode, e->path);\n }\n \n void add_pending_object(struct rev_info *revs,\n@@ -265,8 +279,10 @@ void add_pending_sha1(struct rev_info *revs, const char *name,\n }\n \n static struct commit *handle_commit(struct rev_info *revs,\n-\t\t\t\t struct object *object, const char *name)\n+\t\t\t\t struct object_array_entry *entry)\n {\n+\tstruct object *object = entry->item;\n+\tconst char *name = entry->name;\n \tunsigned long flags = object->flags;\n \n \t/*\n@@ -316,7 +332,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tmark_tree_contents_uninteresting(tree);\n \t\t\treturn NULL;\n \t\t}\n-\t\tadd_pending_object(revs, object, \"\");\n+\t\tadd_pending_object_from_entry(revs, entry);\n \t\treturn NULL;\n \t}\n \n@@ -328,7 +344,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING)\n \t\t\treturn NULL;\n-\t\tadd_pending_object(revs, object, \"\");\n+\t\tadd_pending_object_from_entry(revs, entry);\n \t\treturn NULL;\n \t}\n \tdie(\"%s is unknown object\", name);\n@@ -2666,7 +2682,7 @@ int prepare_revision_walk(struct rev_info *revs)\n \trevs->pending.objects = NULL;\n \tfor (i = 0; i < old_pending.nr; i++) {\n \t\tstruct object_array_entry *e = old_pending.objects + i;\n-\t\tstruct commit *commit = handle_commit(revs, e->item, e->name);\n+\t\tstruct commit *commit = handle_commit(revs, e);\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n \t\t\t\tcommit->object.flags |= SEEN;\n-- \n2.1.2.596.g7379948\n"},{"id":"250732","messageId":"20141015224327.GT25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 20/25] rev-list: document --reflog option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:43:28Z","receivedAt":"2014-10-15T22:43:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is mostly used internally, but it does not hurt to\nexplain it.\n\nSigned-off-by: Jeff King \n---\n Documentation/rev-list-options.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5d311b8..4cf94c6 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -168,6 +168,10 @@ respectively, and they must begin with `refs/` when applied to `--glob`\n or `--all`. If a trailing '/{asterisk}' is intended, it must be given\n explicitly.\n \n+--reflog::\n+\tPretend as if all objects mentioned by reflogs are listed on the\n+\tcommand line as ``.\n+\n --ignore-missing::\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\n-- \n2.1.2.596.g7379948\n"},{"id":"250733","messageId":"20141015224430.GU25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 21/25] rev-list: add --index-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:44:30Z","receivedAt":"2014-10-15T22:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is currently no easy way to ask the revision traversal\nmachinery to include objects reachable from the index (e.g.,\nblobs and trees that have not yet been committed). This\npatch adds an option to do so.\n\nSigned-off-by: Jeff King \n---\nI was tempted to call this just \"--index\", because I could not think of\nwhat else \"--index\" would mean in the context of rev-list. But I also\nworried about weird interactions with other commands that take revision\narguments. And since this is mostly for internal use anyway, I figured\nthe more verbose name is not too bad. I could be convinced otherwise,\nthough.\n\n Documentation/rev-list-options.txt | 7 ++++++\n revision.c | 51 ++++++++++++++++++++++++++++++++++++++\n revision.h | 1 +\n t/t6000-rev-list-misc.sh | 23 +++++++++++++++++\n 4 files changed, 82 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4cf94c6..03ab343 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -172,6 +172,13 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as ``.\n \n+--index-objects::\n+\tPretend as if all objects used by the index (any blobs, and any\n+\ttrees which are mentioned by the index's cache-tree extension)\n+\tad listed on the command line. Note that you probably want to\n+\tuse `--objects`, too, as there are by definition no commits in\n+\tthe index.\n+\n --ignore-missing::\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\ndiff --git a/revision.c b/revision.c\nindex 9a0f99a..caabaf1 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -17,6 +17,7 @@\n #include \"mailmap.h\"\n #include \"commit-slab.h\"\n #include \"dir.h\"\n+#include \"cache-tree.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1299,6 +1300,53 @@ void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)\n \tfor_each_reflog(handle_one_reflog, &cb);\n }\n \n+static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,\n+\t\t\t struct strbuf *path)\n+{\n+\tsize_t baselen = path->len;\n+\tint i;\n+\n+\tif (it->entry_count >= 0) {\n+\t\tstruct tree *tree = lookup_tree(it->sha1);\n+\t\tadd_pending_object_with_path(revs, &tree->object, \"\",\n+\t\t\t\t\t 040000, path->buf);\n+\t}\n+\n+\tfor (i = 0; i < it->subtree_nr; i++) {\n+\t\tstruct cache_tree_sub *sub = it->down[i];\n+\t\tstrbuf_addf(path, \"%s%s\", baselen ? \"/\" : \"\", sub->name);\n+\t\tadd_cache_tree(sub->cache_tree, revs, path);\n+\t\tstrbuf_setlen(path, baselen);\n+\t}\n+\n+}\n+\n+void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n+{\n+\tint i;\n+\n+\tread_cache();\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tstruct blob *blob;\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n+\n+\t\tblob = lookup_blob(ce->sha1);\n+\t\tif (!blob)\n+\t\t\tdie(\"unable to add index blob to traversal\");\n+\t\tadd_pending_object_with_path(revs, &blob->object, \"\",\n+\t\t\t\t\t ce->ce_mode, ce->name);\n+\t}\n+\n+\tif (active_cache_tree) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\t\tadd_cache_tree(active_cache_tree, revs, &path);\n+\t\tstrbuf_release(&path);\n+\t}\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n {\n \tunsigned char sha1[20];\n@@ -1649,6 +1697,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--reflog\") || !strcmp(arg, \"--not\") ||\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\") ||\n \t !strcmp(arg, \"--bisect\") || starts_with(arg, \"--glob=\") ||\n+\t !strcmp(arg, \"--index-objects\") ||\n \t starts_with(arg, \"--exclude=\") ||\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n@@ -2078,6 +2127,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n+\t} else if (!strcmp(arg, \"--index-objects\")) {\n+\t\tadd_index_objects_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/revision.h b/revision.h\nindex e644044..e6dcd5d 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -277,6 +277,7 @@ extern void add_pending_sha1(struct rev_info *revs,\n \n extern void add_head_to_pending(struct rev_info *);\n extern void add_reflogs_to_pending(struct rev_info *, unsigned int flags);\n+extern void add_index_objects_to_pending(struct rev_info *, unsigned int flags);\n \n enum commit_action {\n \tcommit_ignore,\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 3794e4c..c2fb09c 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -73,4 +73,27 @@ test_expect_success 'symleft flag bit is propagated down from tag' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list can show index objects' '\n+\t# Of the blobs and trees in the index, note:\n+\t#\n+\t# - we do not show two/three, because it is the\n+\t# same blob as \"one\", and we show objects only once\n+\t#\n+\t# - we do show the tree \"two\", because it has a valid cache tree\n+\t# from the last commit\n+\t#\n+\t# - we do not show the root tree; since we updated the index, it\n+\t# does not have a valid cache tree\n+\t#\n+\tcat >expect <<-\\EOF\n+\t8e4020bb5a8d8c873b25de15933e75cc0fc275df one\n+\td9d3a7417b9605cfd88ee6306b28dadc29e6ab08 only-in-index\n+\t9200b628cf9dc883a85a7abc8d6e6730baee589c two\n+\tEOF\n+\techo only-in-index >only-in-index &&\n+\tgit add only-in-index &&\n+\tgit rev-list --objects --index-objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250734","messageId":"20141015224451.GV25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 22/25] reachable: use revision machinery's --index-objects code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:44:51Z","receivedAt":"2014-10-15T22:44:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This does the same thing as our custom code, so let's not\nrepeat ourselves.\n\nSigned-off-by: Jeff King \n---\n reachable.c | 52 +---------------------------------------------------\n 1 file changed, 1 insertion(+), 51 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 0176a88..a647267 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -32,56 +32,6 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n-static void add_one_tree(const unsigned char *sha1, struct rev_info *revs)\n-{\n-\tstruct tree *tree = lookup_tree(sha1);\n-\tif (tree)\n-\t\tadd_pending_object(revs, &tree->object, \"\");\n-}\n-\n-static void add_cache_tree(struct cache_tree *it, struct rev_info *revs)\n-{\n-\tint i;\n-\n-\tif (it->entry_count >= 0)\n-\t\tadd_one_tree(it->sha1, revs);\n-\tfor (i = 0; i < it->subtree_nr; i++)\n-\t\tadd_cache_tree(it->down[i]->cache_tree, revs);\n-}\n-\n-static void add_cache_refs(struct rev_info *revs)\n-{\n-\tint i;\n-\n-\tread_cache();\n-\tfor (i = 0; i < active_nr; i++) {\n-\t\tstruct blob *blob;\n-\n-\t\t/*\n-\t\t * The index can contain blobs and GITLINKs, GITLINKs are hashes\n-\t\t * that don't actually point to objects in the repository, it's\n-\t\t * almost guaranteed that they are NOT blobs, so we don't call\n-\t\t * lookup_blob() on them, to avoid populating the hash table\n-\t\t * with invalid information\n-\t\t */\n-\t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n-\t\t\tcontinue;\n-\n-\t\tblob = lookup_blob(active_cache[i]->sha1);\n-\t\tif (blob)\n-\t\t\tblob->object.flags |= SEEN;\n-\n-\t\t/*\n-\t\t * We could add the blobs to the pending list, but quite\n-\t\t * frankly, we don't care. Once we've looked them up, and\n-\t\t * added them as objects, we've really done everything\n-\t\t * there is to do for a blob\n-\t\t */\n-\t}\n-\tif (active_cache_tree)\n-\t\tadd_cache_tree(active_cache_tree, revs);\n-}\n-\n /*\n * The traversal will have already marked us as SEEN, so we\n * only need to handle any progress reporting here.\n@@ -213,7 +163,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \trevs->tree_objects = 1;\n \n \t/* Add all refs from the index file */\n-\tadd_cache_refs(revs);\n+\tadd_index_objects_to_pending(revs, 0);\n \n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n-- \n2.1.2.596.g7379948\n"},{"id":"250735","messageId":"20141015224516.GW25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 23/25] pack-objects: use argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:45:16Z","receivedAt":"2014-10-15T22:45:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us from having to bump the rp_av count when we\nadd new traversal options.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4df9499..b26276b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -22,6 +22,7 @@\n #include \"pack-bitmap.h\"\n #include \"reachable.h\"\n #include \"sha1-array.h\"\n+#include \"argv-array.h\"\n \n static const char *pack_usage[] = {\n \tN_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n@@ -2614,8 +2615,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint use_internal_rev_list = 0;\n \tint thin = 0;\n \tint all_progress_implied = 0;\n-\tconst char *rp_av[6];\n-\tint rp_ac = 0;\n+\tstruct argv_array rp = ARGV_ARRAY_INIT;\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -2705,24 +2705,24 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (pack_to_stdout != !base_name || argc)\n \t\tusage_with_options(pack_usage, pack_objects_options);\n \n-\trp_av[rp_ac++] = \"pack-objects\";\n+\targv_array_push(&rp, \"pack-objects\");\n \tif (thin) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--objects-edge\";\n+\t\targv_array_push(&rp, \"--objects-edge\");\n \t} else\n-\t\trp_av[rp_ac++] = \"--objects\";\n+\t\targv_array_push(&rp, \"--objects\");\n \n \tif (rev_list_all) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--all\";\n+\t\targv_array_push(&rp, \"--all\");\n \t}\n \tif (rev_list_reflog) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--reflog\";\n+\t\targv_array_push(&rp, \"--reflog\");\n \t}\n \tif (rev_list_unpacked) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--unpacked\";\n+\t\targv_array_push(&rp, \"--unpacked\");\n \t}\n \n \tif (!reuse_object)\n@@ -2766,8 +2766,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!use_internal_rev_list)\n \t\tread_object_list_from_stdin();\n \telse {\n-\t\trp_av[rp_ac] = NULL;\n-\t\tget_object_list(rp_ac, rp_av);\n+\t\tget_object_list(rp.argc, rp.argv);\n+\t\targv_array_clear(&rp);\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n-- \n2.1.2.596.g7379948\n"},{"id":"250736","messageId":"20141015224632.GX25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 24/25] repack: pack objects mentioned by the index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:46:32Z","receivedAt":"2014-10-15T22:46:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pack all objects, we use only the objects reachable\nfrom references and reflogs. This misses any objects which\nare reachable from the index, but not yet referenced.\n\nBy itself this isn't a big deal; the objects can remain\nloose until they are actually used in a commit. However, it\ndoes create a problem when we drop packed but unreachable\nobjects. We try to optimize out the writing of objects that\nwe will immediately prune, which means we must follow the\nsame rules as prune in determining what is reachable. And\nprune uses the index for this purpose.\n\nThis is rather uncommon in practice, as objects in the index\nwould not usually have been packed in the first place. But\nit could happen in a sequence like:\n\n 1. You make a commit on a branch that references blob X.\n\n 2. You repack, moving X into the pack.\n\n 3. You delete the branch (and its reflog), so that X is\n unreferenced.\n\n 4. You \"git add\" blob X so that it is now referenced only\n by the index.\n\n 5. You repack again with git-gc. The pack-objects we\n invoke will see that X is neither referenced nor\n recent and not bother loosening it.\n\nSigned-off-by: Jeff King \n---\nIn addition to fixing the safety, this is actually _packing_ those index\nobjects. I don't see anything wrong with that, but if would prefer not\nto, it should be possible to do a separate traversal over the index\nobjects (and mark them as \"to keep\", but not \"to pack\").\n\n builtin/pack-objects.c | 8 ++++++++\n builtin/repack.c | 1 +\n t/t7701-repack-unpack-unreachable.sh | 13 +++++++++++++\n 3 files changed, 22 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b26276b..3dc9108 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2617,6 +2617,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint all_progress_implied = 0;\n \tstruct argv_array rp = ARGV_ARRAY_INIT;\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n+\tint rev_list_index = 0;\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -2663,6 +2664,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_SET_INT, 0, \"reflog\", &rev_list_reflog, NULL,\n \t\t N_(\"include objects referred by reflog entries\"),\n \t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n+\t\t{ OPTION_SET_INT, 0, \"index-objects\", &rev_list_index, NULL,\n+\t\t N_(\"include objects referred to by the index\"),\n+\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n \t\tOPT_BOOL(0, \"stdout\", &pack_to_stdout,\n \t\t\t N_(\"output pack to stdout\")),\n \t\tOPT_BOOL(0, \"include-tag\", &include_tag,\n@@ -2720,6 +2724,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tuse_internal_rev_list = 1;\n \t\targv_array_push(&rp, \"--reflog\");\n \t}\n+\tif (rev_list_index) {\n+\t\tuse_internal_rev_list = 1;\n+\t\targv_array_push(&rp, \"--index-objects\");\n+\t}\n \tif (rev_list_unpacked) {\n \t\tuse_internal_rev_list = 1;\n \t\targv_array_push(&rp, \"--unpacked\");\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2aae05d..80b6021 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -209,6 +209,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\n+\targv_array_push(&cmd_args, \"--index-objects\");\n \tif (window)\n \t\targv_array_pushf(&cmd_args, \"--window=%s\", window);\n \tif (window_memory)\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex b8d4cde..aad8a9c 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -109,4 +109,17 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'keep packed objects found only in index' '\n+\techo my-unique-content >file &&\n+\tgit add file &&\n+\tgit commit -m \"make it reachable\" &&\n+\tgit gc &&\n+\tgit reset HEAD^ &&\n+\tgit reflog expire --expire=now --all &&\n+\tgit add file &&\n+\ttest-chmtime =-86400 .git/objects/pack/* &&\n+\tgit gc --prune=1.hour.ago &&\n+\tgit cat-file blob :file\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250737","messageId":"20141015224815.GY25630@peff.net","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"[PATCH v2 25/25] pack-objects: double-check options before discarding objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-15T22:48:15Z","receivedAt":"2014-10-15T22:48:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are given an expiration time like\n--unpack-unreachable=2.weeks.ago, we avoid writing out old,\nunreachable loose objects entirely, under the assumption\nthat running \"prune\" would simply delete them immediately\nanyway. However, this is only valid if we computed the same\nset of reachable objects as prune would.\n\nIn practice, this is the case, because only git-repack uses\nthe --unpack-unreachable option with an expiration, and it\nalways feeds as many objects into the pack as possible. But\nwe can double-check at runtime just to be sure.\n\nSigned-off-by: Jeff King \n---\nNo test here, because the potential breakage cannot be seen by running\n\"git repack\" (because it gives sane options), nor by just running \"git\npack-objects\" (you can convince it not explode a loose object, but then\nyou would have to reimplement the part of \"git repack\" where it deletes\nall the packs except the one we just created). So really this would only\nbe protecting in practice against somebody who tried to reimplement\ngit-repack themselves (and I do not know of anybody who has done that).\n\n builtin/pack-objects.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3dc9108..72589ed 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2757,6 +2757,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (keep_unreachable && unpack_unreachable)\n \t\tdie(\"--keep-unreachable and --unpack-unreachable are incompatible.\");\n+\tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n+\t\tunpack_unreachable_expiration = 0;\n \n \tif (!use_internal_rev_list || !pack_to_stdout || is_repository_shallow())\n \t\tuse_bitmap_index = 0;\n-- \n2.1.2.596.g7379948\n"},{"id":"250758","messageId":"xmqqwq80szol.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141015223405.GB25630@peff.net","subject":"Re: [PATCH v2 02/25] isxdigit: cast input to unsigned char","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T17:16:10Z","receivedAt":"2014-10-16T17:16:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Otherwise, callers must do so or risk triggering warnings\n> -Wchar-subscript (and rightfully so; a signed char might\n> cause us to use a bogus negative index into the\n> hexval_table).\n>\n> While we are dropping the now-unnecessary casts from the\n> caller in urlmatch.c, we can get rid of similar casts in\n> actually parsing the hex by using the hexval() helper, which\n> implicitly casts to unsigned (but note that we cannot\n> implement isxdigit in terms of hexval(), as it also casts\n> its return value to unsigned).\n>\n> Signed-off-by: Jeff King \n> ---\n> The patch that added more calls to isxdigit later in the series actually\n> got reworked. So this is purely a cleanup and can be dropped if need be,\n> though I still think it is an improvement.\n\nYes, thanks.\n\n>\n> git-compat-util.h | 2 +-\n> urlmatch.c | 8 ++++----\n> 2 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index fb41118..44890d5 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -677,7 +677,7 @@ extern const unsigned char sane_ctype[256];\n> #define iscntrl(x) (sane_istest(x,GIT_CNTRL))\n> #define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \\\n> \t\tGIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)\n> -#define isxdigit(x) (hexval_table[x] != -1)\n> +#define isxdigit(x) (hexval_table[(unsigned char)(x)] != -1)\n> #define tolower(x) sane_case((unsigned char)(x), 0x20)\n> #define toupper(x) sane_case((unsigned char)(x), 0)\n> #define is_pathspec_magic(x) sane_istest(x,GIT_PATHSPEC_MAGIC)\n> diff --git a/urlmatch.c b/urlmatch.c\n> index 3d4c54b..618d216 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -43,11 +43,11 @@ static int append_normalized_escapes(struct strbuf *buf,\n> \t\tfrom_len--;\n> \t\tif (ch == '%') {\n> \t\t\tif (from_len < 2 ||\n> -\t\t\t !isxdigit((unsigned char)from[0]) ||\n> -\t\t\t !isxdigit((unsigned char)from[1]))\n> +\t\t\t !isxdigit(from[0]) ||\n> +\t\t\t !isxdigit(from[1]))\n> \t\t\t\treturn 0;\n> -\t\t\tch = hexval_table[(unsigned char)*from++] << 4;\n> -\t\t\tch |= hexval_table[(unsigned char)*from++];\n> +\t\t\tch = hexval(*from++) << 4;\n> +\t\t\tch |= hexval(*from++);\n> \t\t\tfrom_len -= 2;\n> \t\t\twas_esc = 1;\n> \t\t}\n"},{"id":"250759","messageId":"xmqqr3y7ud5h.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141015223419.GC25630@peff.net","subject":"Re: [PATCH v2 03/25] object_array: factor out slopbuf-freeing logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T17:39:54Z","receivedAt":"2014-10-16T17:39:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is not a lot of code, but it's a logical construct that\n> should not need to be repeated (and we are about to add a\n> third repetition).\n\nGood, but I have two and a half tangential comments about the\ncontext that appears in this patch ;-)\n\n> void object_array_filter(struct object_array *array,\n> \t\t\t object_array_each_func_t want, void *cb_data)\n> {\n> @@ -367,8 +377,7 @@ void object_array_filter(struct object_array *array,\n> \t\t\t\tobjects[dst] = objects[src];\n> \t\t\tdst++;\n> \t\t} else {\n> -\t\t\tif (objects[src].name != object_array_slopbuf)\n> -\t\t\t\tfree(objects[src].name);\n> +\t\t\tobject_array_release_entry(&objects[src]);\n> \t\t}\n> \t}\n> \tarray->nr = dst;\n> @@ -400,8 +409,7 @@ void object_array_remove_duplicates(struct object_array *array)\n> \t\t\t\tobjects[array->nr] = objects[src];\n> \t\t\tarray->nr++;\n> \t\t} else {\n> -\t\t\tif (objects[src].name != object_array_slopbuf)\n> -\t\t\t\tfree(objects[src].name);\n> +\t\t\tobject_array_release_entry(&objects[src]);\n> \t\t}\n> \t}\n> }\n\n 1. These two functions both remove elements from a given array\n in-place, the former being in a more generic form that takes a\n caller-specified criterion while the latter uses a hardcoded\n condition to decide what to filter. aeb4a51e (object_array:\n add function object_array_filter(), 2013-05-25) and later\n 1506510c (object_array_remove_duplicates(): rewrite to reduce\n copying, 2013-05-25) should have refactored the latter further\n to implement it in terms of the former, perhaps?\n\n 1.5 I would have expected a function to \"remove duplicates from an\n array\" to remove duplicates from the array by comparing the objects\n contained in the array, not entries that may (or may not) point at\n different objects but happens to share the same name; I think this\n function is misnamed.\n\n 2. We use object_array_remove_duplicates() to de-dup \"git bundle\n create x master master\", which came from b2a6d1c6 (bundle:\n allow the same ref to be given more than once, 2009-01-17),\n which is still the sole caller of the function, and I think\n this is bogus. Comparing .name would not de-dup \"git bundle\n create x master refs/heads/master\".\n\nI think the right way to fix these two and a half problems is to do\nthe following:\n\n - object_array_remove_duplicates() (and contains_name() helper it\n uses) should be removed from object.c;\n\n - create_bundle() in bundle.c should implement a helper that is\n similar to contains_name() but knows about ref dwimming and use\n it to call object_array_filter() to replace its call to\n object_array_remove_duplicates().\n\nI am not doing this myself, and I do not expect either you or\nMichael to do so, either. I am just writing this down to point out\na low hanging fruit to aspiring new contributors (hint, hint).\n\nThanks.\n"},{"id":"250760","messageId":"xmqqmw8vuci7.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141015223728.GF25630@peff.net","subject":"Re: [PATCH v2 06/25] reachable: use traverse_commit_list instead of custom walk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T17:53:52Z","receivedAt":"2014-10-16T17:53:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> To find the set of reachable objects, we add a bunch of\n> possible sources to our rev_info, call prepare_revision_walk,\n> and then launch into a custom walker that handles each\n> object top. This is a subset of what traverse_commit_list\n> does, so we can just reuse that code (it can also handle\n> more complex cases like UNINTERESTING commits and pathspecs,\n> but we don't use those features).\n>\n> Signed-off-by: Jeff King \n> ---\n> I was concerned this would be slower because traverse_commit_list is\n> more featureful. To my surprise, it was consistently about 3-4% faster!\n> The major difference is that traverse_commit_list will hit all of the\n> commits first, and then the trees. For reachability that doesn't matter\n> either way, but I suspect the new way has slightly better cache\n> locality, leading to the minor speedup.\n\nI am not very surprised, as \"custom walk\" hasn't changed much ever\nsince it was done in ba84a797 (builtin \"git prune\", 2006-07-06),\nwhile the generic traversal code has been worked heavily while it\nwas still in builtin-rev-list.c and then later moved to\nlist-objects.c.\n\n> reachable.c | 130 ++++++++----------------------------------------------------\n> 1 file changed, 17 insertions(+), 113 deletions(-)\n\n;-) ;-) ;-) ;-) ;-)\n"},{"id":"250762","messageId":"xmqqiojjuaao.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141015224430.GU25630@peff.net","subject":"Re: [PATCH v2 21/25] rev-list: add --index-objects option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T18:41:35Z","receivedAt":"2014-10-16T18:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There is currently no easy way to ask the revision traversal\n> machinery to include objects reachable from the index (e.g.,\n> blobs and trees that have not yet been committed). This\n> patch adds an option to do so.\n>\n> Signed-off-by: Jeff King \n> ---\n> I was tempted to call this just \"--index\", because I could not think of\n> what else \"--index\" would mean in the context of rev-list. But I also\n> worried about weird interactions with other commands that take revision\n> arguments. And since this is mostly for internal use anyway, I figured\n> the more verbose name is not too bad. I could be convinced otherwise,\n> though.\n\nI agree that \"--index\" is a bad name as it usually is used in a\nparticular context: the command can work on various combination of\nworking tree and the index, and I am asking it to work on both\n(e.g. \"apply --index\" as opposed to \"apply --cached\").\n\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4cf94c6..03ab343 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -172,6 +172,13 @@ explicitly.\n> \tPretend as if all objects mentioned by reflogs are listed on the\n> \tcommand line as ``.\n> \n> +--index-objects::\n\nThis risks \"index\" getting misunderstood as a verb, e.g. \"please\nenumerate the objects and assign labels to later refer to them\",\ndoesn't it?\n\n\"--indexed-objects\" (short for \"--show-objects-in-the-index\") or\nsomething?\n\n> +\tPretend as if all objects used by the index (any blobs, and any\n> +\ttrees which are mentioned by the index's cache-tree extension)\n> +\tad listed on the command line. Note that you probably want to\n\ns/ad/are/, probably?\n\n> +\tuse `--objects`, too, as there are by definition no commits in\n> +\tthe index.\n\nFor gitlinks/submodules, the index records names of the commit\nobjects, they are not listed, and that is the right behaviour, but\nthis description invites some confusion.\n"},{"id":"250773","messageId":"xmqqsiinsoyk.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141015223244.GA25368@peff.net","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T21:07:47Z","receivedAt":"2014-10-16T21:07:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Somewhere in this series the object enumeration seems to get\nbroken. \"git clone --no-local\" of git.git repository died\nwith\n\n Cloning into 'victim-7'...\n remote: Counting objects: 173727, done.\n remote: Compressing objects: 100% (43697/43697), done.\n remote: Total 173727 (delta 130908), reused 170009 (delta 128151)\n Receiving objects: 100% (173727/173727), 33.45 MiB | 13.69 MiB/s,\n done.\n Resolving deltas: 100% (130908/130908), done.\n fatal: did not receive expected object a74380c3117994efba501df1707418eb6feb9284\n fatal: index-pack failed\n\nwhere a74380c... is such an object.\n\nIf you have a clone of https://github.com/git/git.git\n\n$ git rev-list --parents --objects --all | grep a74380c3117994\n\nwould be an easy way to reproduce.\n"},{"id":"250774","messageId":"xmqqoatbsotb.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"xmqqsiinsoyk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T21:10:56Z","receivedAt":"2014-10-16T21:10:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Somewhere in this series the object enumeration seems to get\n> broken. \"git clone --no-local\" of git.git repository died\n> with\n>\n> Cloning into 'victim-7'...\n> remote: Counting objects: 173727, done.\n> remote: Compressing objects: 100% (43697/43697), done.\n> remote: Total 173727 (delta 130908), reused 170009 (delta 128151)\n> Receiving objects: 100% (173727/173727), 33.45 MiB | 13.69 MiB/s,\n> done.\n> Resolving deltas: 100% (130908/130908), done.\n> fatal: did not receive expected object a74380c3117994efba501df1707418eb6feb9284\n> fatal: index-pack failed\n>\n> where a74380c... is such an object.\n\nEhh, \"such\" is a nonsense. It is a blob that directly is pointed by\na tag that is in refs/tags/*.\n\n> If you have a clone of https://github.com/git/git.git\n>\n> $ git rev-list --parents --objects --all | grep a74380c3117994\n>\n> would be an easy way to reproduce.\n"},{"id":"250775","messageId":"20141016212112.GA16054@peff.net","threadId":"37739","inReplyTo":"xmqqsiinsoyk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-16T21:21:13Z","receivedAt":"2014-10-16T21:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 02:07:47PM -0700, Junio C Hamano wrote:\n\n> Somewhere in this series the object enumeration seems to get\n> broken. \"git clone --no-local\" of git.git repository died\n> with\n> \n> Cloning into 'victim-7'...\n> remote: Counting objects: 173727, done.\n> remote: Compressing objects: 100% (43697/43697), done.\n> remote: Total 173727 (delta 130908), reused 170009 (delta 128151)\n> Receiving objects: 100% (173727/173727), 33.45 MiB | 13.69 MiB/s,\n> done.\n> Resolving deltas: 100% (130908/130908), done.\n> fatal: did not receive expected object a74380c3117994efba501df1707418eb6feb9284\n> fatal: index-pack failed\n> \n> where a74380c... is such an object.\n> \n> If you have a clone of https://github.com/git/git.git\n\nHrm. I cannot reproduce the clone failure...\n\n> $ git rev-list --parents --objects --all | grep a74380c3117994\n> \n> would be an easy way to reproduce.\n\nBut this I can. Digging into it...\n\n-Peff\n"},{"id":"250776","messageId":"20141016213918.GA29397@peff.net","threadId":"37739","inReplyTo":"20141016212112.GA16054@peff.net","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-16T21:39:18Z","receivedAt":"2014-10-16T21:39:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 05:21:12PM -0400, Jeff King wrote:\n\n> Hrm. I cannot reproduce the clone failure...\n\nOh, because I have bitmaps turned on, and we do not run the list-objects\ncode at all in that case.\n\nThe bug unsurprisingly bisects to \"traverse_commit_list: support\npending blobs/trees with paths\". The problem is that I didn't notice\nthat handle_commit actually rewrites the \"object\" pointer when\nunwrapping the tags, and that commit reuses the original pointer from\nthe entry. So we end up putting two copies of the tag object into the\npending list, rather than the tag and the blob.\n\nThe fix is:\n\ndiff --git a/revision.c b/revision.c\nindex 9a0f99a..8030fc8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -231,12 +231,6 @@ static void add_pending_object_with_mode(struct rev_info *revs,\n \tadd_pending_object_with_path(revs, obj, name, mode, NULL);\n }\n \n-static void add_pending_object_from_entry(struct rev_info *revs,\n-\t\t\t\t\t struct object_array_entry *e)\n-{\n-\tadd_pending_object_with_path(revs, e->item, e->name, e->mode, e->path);\n-}\n-\n void add_pending_object(struct rev_info *revs,\n \t\t\tstruct object *obj, const char *name)\n {\n@@ -283,6 +277,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n {\n \tstruct object *object = entry->item;\n \tconst char *name = entry->name;\n+\tconst char *path = entry->path;\n+\tunsigned int mode = entry->mode;\n \tunsigned long flags = object->flags;\n \n \t/*\n@@ -301,6 +297,14 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n \t\t}\n \t\tobject->flags |= flags;\n+\t\t/*\n+\t\t * We'll handle the tagged object by looping or dropping\n+\t\t * through to the non-tag handlers below. Do not\n+\t\t * propagate data from the tag's pending entry.\n+\t\t */\n+\t\tname = NULL;\n+\t\tpath = NULL;\n+\t\tmode = 0;\n \t}\n \n \t/*\n@@ -332,7 +336,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tmark_tree_contents_uninteresting(tree);\n \t\t\treturn NULL;\n \t\t}\n-\t\tadd_pending_object_from_entry(revs, entry);\n+\t\tadd_pending_object_with_path(revs, object, name, mode, path);\n \t\treturn NULL;\n \t}\n \n@@ -344,7 +348,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING)\n \t\t\treturn NULL;\n-\t\tadd_pending_object_from_entry(revs, entry);\n+\t\tadd_pending_object_with_path(revs, object, name, mode, path);\n \t\treturn NULL;\n \t}\n \tdie(\"%s is unknown object\", name);\n\nWe should probably add a test for cloning a tag of an otherwise\nunreferenced object, too.\n\n-Peff\n"},{"id":"250779","messageId":"xmqq8ukfslol.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"20141016213918.GA29397@peff.net","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T22:18:34Z","receivedAt":"2014-10-16T22:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 16, 2014 at 05:21:12PM -0400, Jeff King wrote:\n>\n>> Hrm. I cannot reproduce the clone failure...\n>\n> Oh, because I have bitmaps turned on, and we do not run the list-objects\n> code at all in that case.\n\n;-)\n\n> We should probably add a test for cloning a tag of an otherwise\n> unreferenced object, too.\n\nYeah, that too ;-)\n\nThanks for a quick diag.\n"},{"id":"250784","messageId":"20141017000341.GA7848@peff.net","threadId":"37739","inReplyTo":"xmqq8ukfslol.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:03:41Z","receivedAt":"2014-10-17T00:03:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 03:18:34PM -0700, Junio C Hamano wrote:\n\n> > We should probably add a test for cloning a tag of an otherwise\n> > unreferenced object, too.\n> \n> Yeah, that too ;-)\n\nHere's that test (after the scissors below). It can be applied totally\nseparately, though I stuck it in as patch \"18.5/25\" in the existing\nseries to confirm that my original series does cause the test to fail,\nand that it passes with the patch I posted earlier.\n\nDo you want to just squash the fix I posted earlier (into patch 19, the\n\"traverse_commit_list: ...\" one)? I'm happy to repost the revised patch,\nbut my impression of your workflow is that squashing is just as easy\nthan replacing a patch (i.e., you're running \"rebase -i\" either way).\n\nOr I'm happy to re-post the whole series, but it's rather long. :)\n\n> Thanks for a quick diag.\n\nI'm impressed that you found the bug so quickly. :) My biggest fear with\nthe whole series is that it's disrupting and refactoring some\nfundamental parts of the code that might cause regressions. I put a lot\nof my faith in the test suite, but that did not work out here (I do\nstill feel pretty good about the series overall, though I think I'd cook\nit longer than usual given the complexity and the areas it touches).\n\n-- >8 --\nSubject: t5516: test pushing a tag of an otherwise unreferenced blob\n\nIt's not unreasonable to have a tag that points to a blob\nthat is not part of the normal history. We do this in\ngit.git to distribute gpg keys. However, we never explicitly\nchecked in our test suite that this actually works (i.e.,\nthat pack-objects actually sends the blob because of the tag\nmentioning it).\n\nIt does in fact work fine, but a recent patch under\ndiscussion broke this, and the test suite didn't notice.\nLet's make the test suite more complete.\n\nSigned-off-by: Jeff King \n---\nThe \"should have\" below is belt-and-suspenders. The test actually fails\nwith my series without the cat-file check, but I'm concerned a bug\nthat affects pack-objects could also affect the connectivity check on\nthe receiving side.\n\n t/t5516-fetch-push.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 67e0ab3..7c8a769 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1277,4 +1277,17 @@ EOF\n \tgit push --no-thin --receive-pack=\"$rcvpck\" no-thin/.git refs/heads/master:refs/heads/foo\n '\n \n+test_expect_success 'pushing a tag pushes the tagged object' '\n+\trm -rf dst.git &&\n+\tblob=$(echo unreferenced | git hash-object -w --stdin) &&\n+\tgit tag -m foo tag-of-blob $blob &&\n+\tgit init --bare dst.git &&\n+\tgit push dst.git tag-of-blob &&\n+\t# the receiving index-pack should have noticed\n+\t# any problems, but we double check\n+\techo unreferenced >expect &&\n+\tgit --git-dir=dst.git cat-file blob tag-of-blob >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250785","messageId":"20141017001230.GB7848@peff.net","threadId":"37739","inReplyTo":"xmqqiojjuaao.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 21/25] rev-list: add --index-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:12:31Z","receivedAt":"2014-10-17T00:12:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 11:41:35AM -0700, Junio C Hamano wrote:\n\n> I agree that \"--index\" is a bad name as it usually is used in a\n> particular context: the command can work on various combination of\n> working tree and the index, and I am asking it to work on both\n> (e.g. \"apply --index\" as opposed to \"apply --cached\").\n\nThanks. I wasn't sure if I was just being paranoid or not, but now there\nare at least two of us.\n\n> > +--index-objects::\n> \n> This risks \"index\" getting misunderstood as a verb, e.g. \"please\n> enumerate the objects and assign labels to later refer to them\",\n> doesn't it?\n> \n> \"--indexed-objects\" (short for \"--show-objects-in-the-index\") or\n> something?\n\nThat sounds reasonable. We could technically do `--indexed` as that is\ndifferent from `--index`, but maybe they are still confusingly close.\n\n> > +\tPretend as if all objects used by the index (any blobs, and any\n> > +\ttrees which are mentioned by the index's cache-tree extension)\n> > +\tad listed on the command line. Note that you probably want to\n> \n> s/ad/are/, probably?\n\nYeah, sorry, vi cruft I think (at least I didn't accidentally insert\n\"C-a C-k\" ;) ).\n\n> > +\tuse `--objects`, too, as there are by definition no commits in\n> > +\tthe index.\n> \n> For gitlinks/submodules, the index records names of the commit\n> objects, they are not listed, and that is the right behaviour, but\n> this description invites some confusion.\n\nGood point. How about this:\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 03ab343..3301fde 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -172,12 +172,10 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as ``.\n \n---index-objects::\n-\tPretend as if all objects used by the index (any blobs, and any\n-\ttrees which are mentioned by the index's cache-tree extension)\n-\tad listed on the command line. Note that you probably want to\n-\tuse `--objects`, too, as there are by definition no commits in\n-\tthe index.\n+--indexed-objects::\n+\tPretend as if all trees and blobs used by the index are listed\n+\ton the command line. Note that you probably want to use\n+\t`--objects`, too.\n \n --ignore-missing::\n \tUpon seeing an invalid object name in the input, pretend as if\n\nI was tempted to not document this at all (nor to add documentation for\n--reflog), as I think these are really only going to be used internally.\nBut it's nice to have documentation even for this internal stuff (if\nanything, we should probably be making sure they are just limited to\nrev-list plumbing, and not included in the log manpage).\n\n-Peff\n"},{"id":"250786","messageId":"20141017003356.GC7848@peff.net","threadId":"37739","inReplyTo":"xmqqr3y7ud5h.fsf@gitster.dls.corp.google.com","subject":"git-bundle rev handling and de-duping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:33:56Z","receivedAt":"2014-10-17T00:33:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[subject tweaked as we have veered quite far off the original, and\n this might get more attention from interested people]\n\nOn Thu, Oct 16, 2014 at 10:39:54AM -0700, Junio C Hamano wrote:\n\n> 2. We use object_array_remove_duplicates() to de-dup \"git bundle\n> create x master master\", which came from b2a6d1c6 (bundle:\n> allow the same ref to be given more than once, 2009-01-17),\n> which is still the sole caller of the function, and I think\n> this is bogus. Comparing .name would not de-dup \"git bundle\n> create x master refs/heads/master\".\n\nHmm. We also doesn't respect the UNINTERESTING flag, so it will dedup\n\"foo\" and \"^foo\" into a single entry. I _think_ this doesn't have any\nbad side effects, because they are equivalent (i.e., the flag is carried\non the object, not the pending entry). I would expect this:\n\n git bundle create ... foo ^foo\n\nto barf (and it does) because the bundle is empty. But with this:\n\n git bundle create ... another-ref foo ^foo\n\nI would expect the resulting bundle to contain the objects from\nanother-ref, but still mention \"foo\" as an update (we didn't need to\nsend any objects, since presumably the other side already had them). It\ndoesn't, but that is not because of the dedup. It is because we generate\nthe list of refs to send based on the pending list, and we skip all\nUNINTERESTING objects (we must, because otherwise \"^foo\" would make an\nentry). I.e., it is the same \"the flag is on the object, not the pending\nentry\" that saved us above now causing its own bug. Obviously the\nexample above is sort of silly, but if you have:\n\n # two branches point at the same object\n git branch foo master\n\n # the other side already has master. Let's send them foo.\n # this will fail because the bundle is empty. That's mildly\n # annoying because we really want to tell them \"hey, update\n # your foo branch\". But at least we get an error.\n git bundle create tmp.bundle foo ^master\n\n # now the same thing, but we're sending them some other objects.\n # This one succeeds, but silently omits foo from the bundle!\n git bundle create tmp.bundle foo another-ref ^master\n\nI have a feeling that the list needs to be generated from revs.cmdline,\nnot revs.pending, and the de-duplication needs to happen there (with the\nref resolution that you mention).\n\nI also have the feeling that fast-export had to deal with this exact\nsame issue. It looks like we use revs.cmdline there. I seem to recall\nthere was some ongoing work in that area, but I stopped paying close\nattention due to some personality conflicts, and I don't know if all of\nthe issues were ever resolved.\n\n> I think the right way to fix these two and a half problems is to do\n> the following:\n> \n> - object_array_remove_duplicates() (and contains_name() helper it\n> uses) should be removed from object.c;\n> \n> - create_bundle() in bundle.c should implement a helper that is\n> similar to contains_name() but knows about ref dwimming and use\n> it to call object_array_filter() to replace its call to\n> object_array_remove_duplicates().\n\nAgreed. The loop in create_bundle right after the de-dup does the\ndwimming. Probably it would be simple to just skip duplicates there\nusing a hashmap or sorted list.\n\n> I am not doing this myself, and I do not expect either you or\n> Michael to do so, either. I am just writing this down to point out\n> a low hanging fruit to aspiring new contributors (hint, hint).\n\nI am also not planning on working on it soon, but now we have hopefully\nfed plenty of possibilities to anybody who wants to. :)\n\n-Peff\n"},{"id":"250787","messageId":"20141017004346.GD7848@peff.net","threadId":"37739","inReplyTo":"20141017001230.GB7848@peff.net","subject":"Re: [PATCH v2 21/25] rev-list: add --index-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:43:46Z","receivedAt":"2014-10-17T00:43:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 08:12:31PM -0400, Jeff King wrote:\n\n> > \"--indexed-objects\" (short for \"--show-objects-in-the-index\") or\n> > something?\n> \n> That sounds reasonable. We could technically do `--indexed` as that is\n> different from `--index`, but maybe they are still confusingly close.\n\nHere's a re-roll of the final 5 patches of the series with the updated\nname (`--indexed-objects`). The name change cascades from patch 22 to\npatches 23 and 25 (and I renamed the matching pack-objects option as\nwell). 24 and 26 are unchanged, but I figured it is easier on you to\nreplace the whole block of patches at once.\n\n [22/26]: rev-list: add --indexed-objects option\n [23/26]: reachable: use revision machinery's --indexed-objects code\n [24/26]: pack-objects: use argv_array\n [25/26]: repack: pack objects mentioned by the index\n [26/26]: pack-objects: double-check options before discarding objects\n"},{"id":"250788","messageId":"20141017004423.GA2919@peff.net","threadId":"37739","inReplyTo":"20141017004346.GD7848@peff.net","subject":"[PATCH v3 22/26] rev-list: add --indexed-objects option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:44:23Z","receivedAt":"2014-10-17T00:44:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is currently no easy way to ask the revision traversal\nmachinery to include objects reachable from the index (e.g.,\nblobs and trees that have not yet been committed). This\npatch adds an option to do so.\n\nSigned-off-by: Jeff King \n---\n Documentation/rev-list-options.txt | 5 ++++\n revision.c | 51 ++++++++++++++++++++++++++++++++++++++\n revision.h | 1 +\n t/t6000-rev-list-misc.sh | 23 +++++++++++++++++\n 4 files changed, 80 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4cf94c6..3301fde 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -172,6 +172,11 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as ``.\n \n+--indexed-objects::\n+\tPretend as if all trees and blobs used by the index are listed\n+\ton the command line. Note that you probably want to use\n+\t`--objects`, too.\n+\n --ignore-missing::\n \tUpon seeing an invalid object name in the input, pretend as if\n \tthe bad input was not given.\ndiff --git a/revision.c b/revision.c\nindex 8030fc8..a6c5dc3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -17,6 +17,7 @@\n #include \"mailmap.h\"\n #include \"commit-slab.h\"\n #include \"dir.h\"\n+#include \"cache-tree.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1303,6 +1304,53 @@ void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)\n \tfor_each_reflog(handle_one_reflog, &cb);\n }\n \n+static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,\n+\t\t\t struct strbuf *path)\n+{\n+\tsize_t baselen = path->len;\n+\tint i;\n+\n+\tif (it->entry_count >= 0) {\n+\t\tstruct tree *tree = lookup_tree(it->sha1);\n+\t\tadd_pending_object_with_path(revs, &tree->object, \"\",\n+\t\t\t\t\t 040000, path->buf);\n+\t}\n+\n+\tfor (i = 0; i < it->subtree_nr; i++) {\n+\t\tstruct cache_tree_sub *sub = it->down[i];\n+\t\tstrbuf_addf(path, \"%s%s\", baselen ? \"/\" : \"\", sub->name);\n+\t\tadd_cache_tree(sub->cache_tree, revs, path);\n+\t\tstrbuf_setlen(path, baselen);\n+\t}\n+\n+}\n+\n+void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)\n+{\n+\tint i;\n+\n+\tread_cache();\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tstruct blob *blob;\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n+\n+\t\tblob = lookup_blob(ce->sha1);\n+\t\tif (!blob)\n+\t\t\tdie(\"unable to add index blob to traversal\");\n+\t\tadd_pending_object_with_path(revs, &blob->object, \"\",\n+\t\t\t\t\t ce->ce_mode, ce->name);\n+\t}\n+\n+\tif (active_cache_tree) {\n+\t\tstruct strbuf path = STRBUF_INIT;\n+\t\tadd_cache_tree(active_cache_tree, revs, &path);\n+\t\tstrbuf_release(&path);\n+\t}\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)\n {\n \tunsigned char sha1[20];\n@@ -1653,6 +1701,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--reflog\") || !strcmp(arg, \"--not\") ||\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\") ||\n \t !strcmp(arg, \"--bisect\") || starts_with(arg, \"--glob=\") ||\n+\t !strcmp(arg, \"--indexed-objects\") ||\n \t starts_with(arg, \"--exclude=\") ||\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n@@ -2082,6 +2131,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n+\t} else if (!strcmp(arg, \"--indexed-objects\")) {\n+\t\tadd_index_objects_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/revision.h b/revision.h\nindex e644044..e6dcd5d 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -277,6 +277,7 @@ extern void add_pending_sha1(struct rev_info *revs,\n \n extern void add_head_to_pending(struct rev_info *);\n extern void add_reflogs_to_pending(struct rev_info *, unsigned int flags);\n+extern void add_index_objects_to_pending(struct rev_info *, unsigned int flags);\n \n enum commit_action {\n \tcommit_ignore,\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 3794e4c..2602086 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -73,4 +73,27 @@ test_expect_success 'symleft flag bit is propagated down from tag' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list can show index objects' '\n+\t# Of the blobs and trees in the index, note:\n+\t#\n+\t# - we do not show two/three, because it is the\n+\t# same blob as \"one\", and we show objects only once\n+\t#\n+\t# - we do show the tree \"two\", because it has a valid cache tree\n+\t# from the last commit\n+\t#\n+\t# - we do not show the root tree; since we updated the index, it\n+\t# does not have a valid cache tree\n+\t#\n+\tcat >expect <<-\\EOF\n+\t8e4020bb5a8d8c873b25de15933e75cc0fc275df one\n+\td9d3a7417b9605cfd88ee6306b28dadc29e6ab08 only-in-index\n+\t9200b628cf9dc883a85a7abc8d6e6730baee589c two\n+\tEOF\n+\techo only-in-index >only-in-index &&\n+\tgit add only-in-index &&\n+\tgit rev-list --objects --indexed-objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250789","messageId":"20141017004429.GB2919@peff.net","threadId":"37739","inReplyTo":"20141017004346.GD7848@peff.net","subject":"[PATCH v3 23/26] reachable: use revision machinery's --indexed-objects code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:44:30Z","receivedAt":"2014-10-17T00:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This does the same thing as our custom code, so let's not\nrepeat ourselves.\n\nSigned-off-by: Jeff King \n---\n reachable.c | 52 +---------------------------------------------------\n 1 file changed, 1 insertion(+), 51 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex 0176a88..a647267 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -32,56 +32,6 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n-static void add_one_tree(const unsigned char *sha1, struct rev_info *revs)\n-{\n-\tstruct tree *tree = lookup_tree(sha1);\n-\tif (tree)\n-\t\tadd_pending_object(revs, &tree->object, \"\");\n-}\n-\n-static void add_cache_tree(struct cache_tree *it, struct rev_info *revs)\n-{\n-\tint i;\n-\n-\tif (it->entry_count >= 0)\n-\t\tadd_one_tree(it->sha1, revs);\n-\tfor (i = 0; i < it->subtree_nr; i++)\n-\t\tadd_cache_tree(it->down[i]->cache_tree, revs);\n-}\n-\n-static void add_cache_refs(struct rev_info *revs)\n-{\n-\tint i;\n-\n-\tread_cache();\n-\tfor (i = 0; i < active_nr; i++) {\n-\t\tstruct blob *blob;\n-\n-\t\t/*\n-\t\t * The index can contain blobs and GITLINKs, GITLINKs are hashes\n-\t\t * that don't actually point to objects in the repository, it's\n-\t\t * almost guaranteed that they are NOT blobs, so we don't call\n-\t\t * lookup_blob() on them, to avoid populating the hash table\n-\t\t * with invalid information\n-\t\t */\n-\t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n-\t\t\tcontinue;\n-\n-\t\tblob = lookup_blob(active_cache[i]->sha1);\n-\t\tif (blob)\n-\t\t\tblob->object.flags |= SEEN;\n-\n-\t\t/*\n-\t\t * We could add the blobs to the pending list, but quite\n-\t\t * frankly, we don't care. Once we've looked them up, and\n-\t\t * added them as objects, we've really done everything\n-\t\t * there is to do for a blob\n-\t\t */\n-\t}\n-\tif (active_cache_tree)\n-\t\tadd_cache_tree(active_cache_tree, revs);\n-}\n-\n /*\n * The traversal will have already marked us as SEEN, so we\n * only need to handle any progress reporting here.\n@@ -213,7 +163,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \trevs->tree_objects = 1;\n \n \t/* Add all refs from the index file */\n-\tadd_cache_refs(revs);\n+\tadd_index_objects_to_pending(revs, 0);\n \n \t/* Add all external refs */\n \tfor_each_ref(add_one_ref, revs);\n-- \n2.1.2.596.g7379948\n"},{"id":"250790","messageId":"20141017004434.GC2919@peff.net","threadId":"37739","inReplyTo":"20141017004346.GD7848@peff.net","subject":"[PATCH v3 24/26] pack-objects: use argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:44:35Z","receivedAt":"2014-10-17T00:44:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us from having to bump the rp_av count when we\nadd new traversal options.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4df9499..b26276b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -22,6 +22,7 @@\n #include \"pack-bitmap.h\"\n #include \"reachable.h\"\n #include \"sha1-array.h\"\n+#include \"argv-array.h\"\n \n static const char *pack_usage[] = {\n \tN_(\"git pack-objects --stdout [options...] [< ref-list | < object-list]\"),\n@@ -2614,8 +2615,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint use_internal_rev_list = 0;\n \tint thin = 0;\n \tint all_progress_implied = 0;\n-\tconst char *rp_av[6];\n-\tint rp_ac = 0;\n+\tstruct argv_array rp = ARGV_ARRAY_INIT;\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n@@ -2705,24 +2705,24 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (pack_to_stdout != !base_name || argc)\n \t\tusage_with_options(pack_usage, pack_objects_options);\n \n-\trp_av[rp_ac++] = \"pack-objects\";\n+\targv_array_push(&rp, \"pack-objects\");\n \tif (thin) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--objects-edge\";\n+\t\targv_array_push(&rp, \"--objects-edge\");\n \t} else\n-\t\trp_av[rp_ac++] = \"--objects\";\n+\t\targv_array_push(&rp, \"--objects\");\n \n \tif (rev_list_all) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--all\";\n+\t\targv_array_push(&rp, \"--all\");\n \t}\n \tif (rev_list_reflog) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--reflog\";\n+\t\targv_array_push(&rp, \"--reflog\");\n \t}\n \tif (rev_list_unpacked) {\n \t\tuse_internal_rev_list = 1;\n-\t\trp_av[rp_ac++] = \"--unpacked\";\n+\t\targv_array_push(&rp, \"--unpacked\");\n \t}\n \n \tif (!reuse_object)\n@@ -2766,8 +2766,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!use_internal_rev_list)\n \t\tread_object_list_from_stdin();\n \telse {\n-\t\trp_av[rp_ac] = NULL;\n-\t\tget_object_list(rp_ac, rp_av);\n+\t\tget_object_list(rp.argc, rp.argv);\n+\t\targv_array_clear(&rp);\n \t}\n \tcleanup_preferred_base();\n \tif (include_tag && nr_result)\n-- \n2.1.2.596.g7379948\n"},{"id":"250791","messageId":"20141017004449.GD2919@peff.net","threadId":"37739","inReplyTo":"20141017004346.GD7848@peff.net","subject":"[PATCH v3 25/26] repack: pack objects mentioned by the index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:44:49Z","receivedAt":"2014-10-17T00:44:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we pack all objects, we use only the objects reachable\nfrom references and reflogs. This misses any objects which\nare reachable from the index, but not yet referenced.\n\nBy itself this isn't a big deal; the objects can remain\nloose until they are actually used in a commit. However, it\ndoes create a problem when we drop packed but unreachable\nobjects. We try to optimize out the writing of objects that\nwe will immediately prune, which means we must follow the\nsame rules as prune in determining what is reachable. And\nprune uses the index for this purpose.\n\nThis is rather uncommon in practice, as objects in the index\nwould not usually have been packed in the first place. But\nit could happen in a sequence like:\n\n 1. You make a commit on a branch that references blob X.\n\n 2. You repack, moving X into the pack.\n\n 3. You delete the branch (and its reflog), so that X is\n unreferenced.\n\n 4. You \"git add\" blob X so that it is now referenced only\n by the index.\n\n 5. You repack again with git-gc. The pack-objects we\n invoke will see that X is neither referenced nor\n recent and not bother loosening it.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 8 ++++++++\n builtin/repack.c | 1 +\n t/t7701-repack-unpack-unreachable.sh | 13 +++++++++++++\n 3 files changed, 22 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex b26276b..0cf95c9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2617,6 +2617,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint all_progress_implied = 0;\n \tstruct argv_array rp = ARGV_ARRAY_INIT;\n \tint rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;\n+\tint rev_list_index = 0;\n \tstruct option pack_objects_options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -2663,6 +2664,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_SET_INT, 0, \"reflog\", &rev_list_reflog, NULL,\n \t\t N_(\"include objects referred by reflog entries\"),\n \t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n+\t\t{ OPTION_SET_INT, 0, \"indexed-objects\", &rev_list_index, NULL,\n+\t\t N_(\"include objects referred to by the index\"),\n+\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1 },\n \t\tOPT_BOOL(0, \"stdout\", &pack_to_stdout,\n \t\t\t N_(\"output pack to stdout\")),\n \t\tOPT_BOOL(0, \"include-tag\", &include_tag,\n@@ -2720,6 +2724,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tuse_internal_rev_list = 1;\n \t\targv_array_push(&rp, \"--reflog\");\n \t}\n+\tif (rev_list_index) {\n+\t\tuse_internal_rev_list = 1;\n+\t\targv_array_push(&rp, \"--indexed-objects\");\n+\t}\n \tif (rev_list_unpacked) {\n \t\tuse_internal_rev_list = 1;\n \t\targv_array_push(&rp, \"--unpacked\");\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2aae05d..2845620 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -209,6 +209,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\n+\targv_array_push(&cmd_args, \"--indexed-objects\");\n \tif (window)\n \t\targv_array_pushf(&cmd_args, \"--window=%s\", window);\n \tif (window_memory)\ndiff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh\nindex b8d4cde..aad8a9c 100755\n--- a/t/t7701-repack-unpack-unreachable.sh\n+++ b/t/t7701-repack-unpack-unreachable.sh\n@@ -109,4 +109,17 @@ test_expect_success 'do not bother loosening old objects' '\n \ttest_must_fail git cat-file -p $obj2\n '\n \n+test_expect_success 'keep packed objects found only in index' '\n+\techo my-unique-content >file &&\n+\tgit add file &&\n+\tgit commit -m \"make it reachable\" &&\n+\tgit gc &&\n+\tgit reset HEAD^ &&\n+\tgit reflog expire --expire=now --all &&\n+\tgit add file &&\n+\ttest-chmtime =-86400 .git/objects/pack/* &&\n+\tgit gc --prune=1.hour.ago &&\n+\tgit cat-file blob :file\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"250792","messageId":"20141017004453.GE2919@peff.net","threadId":"37739","inReplyTo":"20141017004346.GD7848@peff.net","subject":"[PATCH v3 26/26] pack-objects: double-check options before discarding objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T00:44:54Z","receivedAt":"2014-10-17T00:44:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are given an expiration time like\n--unpack-unreachable=2.weeks.ago, we avoid writing out old,\nunreachable loose objects entirely, under the assumption\nthat running \"prune\" would simply delete them immediately\nanyway. However, this is only valid if we computed the same\nset of reachable objects as prune would.\n\nIn practice, this is the case, because only git-repack uses\nthe --unpack-unreachable option with an expiration, and it\nalways feeds as many objects into the pack as possible. But\nwe can double-check at runtime just to be sure.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0cf95c9..64123d4 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2757,6 +2757,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (keep_unreachable && unpack_unreachable)\n \t\tdie(\"--keep-unreachable and --unpack-unreachable are incompatible.\");\n+\tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n+\t\tunpack_unreachable_expiration = 0;\n \n \tif (!use_internal_rev_list || !pack_to_stdout || is_repository_shallow())\n \t\tuse_bitmap_index = 0;\n-- \n2.1.2.596.g7379948\n"},{"id":"250794","messageId":"20141017044910.GA12624@peff.net","threadId":"37739","inReplyTo":"CAPc5daX0AFv9jDrFyd_OnupW5AfZW9Je_rgzaViX_xxs3SG5zg@mail.gmail.com","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-17T04:49:11Z","receivedAt":"2014-10-17T04:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 09:13:39PM -0700, Junio C Hamano wrote:\n\n> Just being curious, but would the same bug, if allowed to be triggered\n> cutting repacking of your repository, have corrupted the resulting bitmap?\n\nI didn't test, but yes, almost certainly. The bug was in list-objects.c,\nwhich is used by pack-objects to generate the list of objects to pack,\nas well as to build the bitmaps. So not only would it have corrupted the\nbitmaps, a `git repack -ad`[1] would have dropped the object completely,\ncorrupting the repository!\n\n-Peff\n\n[1] git-gc uses `repack -Ad`, of course. So assuming you had packed more\n recently than 2 weeks ago, it would have just been ejected to a\n loose object. Small comfort. :)\n\n-Peff\n"},{"id":"250800","messageId":"ECD50FDF3714423E99E6CC6B4DE04FBF@PhilipOakley","threadId":"37739","inReplyTo":"20141017003356.GC7848@peff.net","subject":"Re: git-bundle rev handling and de-duping","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2014-10-17T21:03:14Z","receivedAt":"2014-10-17T21:03:14Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \n> [subject tweaked as we have veered quite far off the original, and\n> this might get more attention from interested people]\n>\n> On Thu, Oct 16, 2014 at 10:39:54AM -0700, Junio C Hamano wrote:\n>\n>> 2. We use object_array_remove_duplicates() to de-dup \"git bundle\n>> create x master master\", which came from b2a6d1c6 (bundle:\n>> allow the same ref to be given more than once, 2009-01-17),\n>> which is still the sole caller of the function, and I think\n>> this is bogus. Comparing .name would not de-dup \"git bundle\n>> create x master refs/heads/master\".\n>\n> Hmm. We also doesn't respect the UNINTERESTING flag, so it will dedup\n> \"foo\" and \"^foo\" into a single entry. I _think_ this doesn't have any\n> bad side effects, because they are equivalent (i.e., the flag is\n> carried\n> on the object, not the pending entry). I would expect this:\n>\n> git bundle create ... foo ^foo\n>\n> to barf (and it does) because the bundle is empty. But with this:\n>\n> git bundle create ... another-ref foo ^foo\n>\n> I would expect the resulting bundle to contain the objects from\n> another-ref, but still mention \"foo\" as an update (we didn't need to\n> send any objects, since presumably the other side already had them).\n> It\n> doesn't, but that is not because of the dedup. It is because we\n> generate\n> the list of refs to send based on the pending list, and we skip all\n> UNINTERESTING objects (we must, because otherwise \"^foo\" would make an\n> entry). I.e., it is the same \"the flag is on the object, not the\n> pending\n> entry\" that saved us above now causing its own bug. Obviously the\n> example above is sort of silly, but if you have:\n>\n> # two branches point at the same object\n> git branch foo master\n>\n> # the other side already has master. Let's send them foo.\n> # this will fail because the bundle is empty. That's mildly\n> # annoying because we really want to tell them \"hey, update\n> # your foo branch\". But at least we get an error.\n> git bundle create tmp.bundle foo ^master\n\nIsn't this kindof what happens as an issue when we want the right HEAD \nto be included explicitly in a bundle. Though \nttp://thread.gmane.org/gmane.comp.version-control.git/234053 suggests \nits more complicated than that.\n\n>\n> # now the same thing, but we're sending them some other objects.\n> # This one succeeds, but silently omits foo from the bundle!\n> git bundle create tmp.bundle foo another-ref ^master\n>\n> I have a feeling that the list needs to be generated from\n> revs.cmdline,\n> not revs.pending, and the de-duplication needs to happen there (with\n> the\n> ref resolution that you mention).\n>\n> I also have the feeling that fast-export had to deal with this exact\n> same issue. It looks like we use revs.cmdline there. I seem to recall\n> there was some ongoing work in that area, but I stopped paying close\n> attention due to some personality conflicts, and I don't know if all\n> of\n> the issues were ever resolved.\n>\n>> I think the right way to fix these two and a half problems is to do\n>> the following:\n>>\n>> - object_array_remove_duplicates() (and contains_name() helper it\n>> uses) should be removed from object.c;\n>>\n>> - create_bundle() in bundle.c should implement a helper that is\n>> similar to contains_name() but knows about ref dwimming and use\n>> it to call object_array_filter() to replace its call to\n>> object_array_remove_duplicates().\n>\n> Agreed. The loop in create_bundle right after the de-dup does the\n> dwimming. Probably it would be simple to just skip duplicates there\n> using a hashmap or sorted list.\n>\n>> I am not doing this myself, and I do not expect either you or\n>> Michael to do so, either. I am just writing this down to point out\n>> a low hanging fruit to aspiring new contributors (hint, hint).\n>\n> I am also not planning on working on it soon, but now we have\n> hopefully\n> fed plenty of possibilities to anybody who wants to. :)\n>\n--\nPhilip \n"},{"id":"250805","messageId":"xmqq1tq6s4j1.fsf@gitster.dls.corp.google.com","threadId":"37739","inReplyTo":"ECD50FDF3714423E99E6CC6B4DE04FBF@PhilipOakley","subject":"Re: git-bundle rev handling and de-duping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-17T22:41:22Z","receivedAt":"2014-10-17T22:41:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n>> # two branches point at the same object\n>> git branch foo master\n>>\n>> # the other side already has master. Let's send them foo.\n>> # this will fail because the bundle is empty. That's mildly\n>> # annoying because we really want to tell them \"hey, update\n>> # your foo branch\". But at least we get an error.\n>> git bundle create tmp.bundle foo ^master\n>\n> Isn't this kindof what happens as an issue when we want the right HEAD\n> to be included explicitly in a bundle. Though\n\n\nWhat we are discussing here is \"we tell from the command line where\nthe histories end, but do we correctly record all these end points\nas fetchable refs in the resulting bundle?\"\n\nIt does not have anything to do with \"bundle that does not record\nits HEAD cannot be cloned\", which happens when you do not mention\nHEAD when creating the bundle in the first place, which is a totally\ndifferent thing.\n\n> http://thread.gmane.org/gmane.comp.version-control.git/234053 suggests\n> its more complicated than that.\n\nThe main topic of discussion does not have much to what bundle\nrecords and what a reader of a bundle guesses. It is about what\ngoes on the wire and mention of bundle was just a tangent brought up\nby those who do not know what was being discussed, I think.\n\nI think the right fix to the \"git bundle\" issue is to make it easier\non the \"git bundle create\" side to have the resulting bundle record\nits HEAD, even when the user did not mention HEAD on the command\nline. For example, when there is only one end point, e.g. \"git\nbundle create x next\", record refs/heads/next _and_ HEAD pointing at\nthe same commit, because there is no other seneible choice. \n\n\"git bundle create y master next\" may record master, next and HEAD\nwhile HEAD is likely pointing at the same commit as master (because\n'master' is special). Or we could give a warning and even go\ninteractive to ask which ref to record as HEAD.\n\nBut the above three paragraphs are tangent so I'd stop here.\n"},{"id":"250806","messageId":"20141018123106.GA5494@peff.net","threadId":"37739","inReplyTo":"20141016213918.GA29397@peff.net","subject":"Re: [PATCH v2 0/25] prune-safety","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-18T12:31:06Z","receivedAt":"2014-10-18T12:31:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2014 at 05:39:18PM -0400, Jeff King wrote:\n\n> @@ -301,6 +297,14 @@ static struct commit *handle_commit(struct rev_info *revs,\n> \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n> \t\t}\n> \t\tobject->flags |= flags;\n> +\t\t/*\n> +\t\t * We'll handle the tagged object by looping or dropping\n> +\t\t * through to the non-tag handlers below. Do not\n> +\t\t * propagate data from the tag's pending entry.\n> +\t\t */\n> +\t\tname = NULL;\n> +\t\tpath = NULL;\n> +\t\tmode = 0;\n\nHmm. On second thought (and after seeing a warning from Coverity), this\nshould be:\n\ndiff --git a/revision.c b/revision.c\nindex 8030fc8..ebe3e93 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -302,7 +302,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t * through to the non-tag handlers below. Do not\n \t\t * propagate data from the tag's pending entry.\n \t\t */\n-\t\tname = NULL;\n+\t\tname = \"\";\n \t\tpath = NULL;\n \t\tmode = 0;\n \t}\n\nThe rest of the function assumes that name is not NULL (which I'm not\nsure is entirely safe, as add_pending_object can take a NULL; presumably\nevery \"add\" uses the empty string instead of NULL. But either way,\nsetting it to NULL here is definite wrong).\n\nThe \"path\" field is explicitly OK to be NULL.\n\n-Peff\n"}]}