{"thread":{"id":"32823","subject":"[PATCH 1/2] git-count-objects.txt: describe each line in -v output","startedAt":"2013-02-04T12:49:04Z","lastAt":"2013-02-09T01:58:23Z","messageCount":14,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"208605","messageId":"1359982145-10792-1-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":null,"subject":"[PATCH 1/2] git-count-objects.txt: describe each line in -v output","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-04T12:49:04Z","receivedAt":"2013-02-04T12:49:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The current description requires a bit of guessing (what clause\ncorresponds to what printed line?) and lacks information, such as\nthe unit of size and size-pack.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-count-objects.txt | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex 23c80ce..e816823 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -20,11 +20,21 @@ OPTIONS\n -------\n -v::\n --verbose::\n-\tIn addition to the number of loose objects and disk\n-\tspace consumed, it reports the number of in-pack\n-\tobjects, number of packs, disk space consumed by those packs,\n-\tand number of objects that can be removed by running\n-\t`git prune-packed`.\n+\tReport in more detail:\n++\n+count: the number of loose objects\n++\n+size: disk space consumed by loose objects, in KiB\n++\n+in-pack: the number of in-pack objects\n++\n+size-pack: disk space consumed by the packs, in KiB\n++\n+prune-packable: the number of loose objects that are also present in\n+the packs. These objects could be pruned using `git prune-packed`.\n++\n+garbage: the number of files in loose object database that are not\n+valid loose objects\n \n GIT\n ---\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"208606","messageId":"1359982145-10792-2-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":"1359982145-10792-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-04T12:49:05Z","receivedAt":"2013-02-04T12:49:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"While it's unusual to have strange files in loose object database,\n.git/objects/pack/tmp_* is normal after a broken fetch and they\ncan eat up a lot of disk space if the user does not pay attention.\nReport them.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n The hook in prepare_packed_git_one is ugly, but I don't want to\n duplicate the search file logic there in count-objects. Maybe I'm\n wrong.\n\n Interestingly it reports .commits file in my repo too. A nice\n reminder to myself to remind Jeff about count-objects improvements\n for his commit-cache work :)\n\n Way may also need a more friendly format than this one, which I\n assume is plumbing. Something that average git user can understand\n without looking up the document. If \"git stats\" is too much for this\n purpose, perhaps \"git gc --stats\"?\n\n Documentation/git-count-objects.txt |  4 ++--\n builtin/count-objects.c             | 27 ++++++++++++++++++++++++++-\n sha1_file.c                         | 23 ++++++++++++++++++++---\n 3 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex e816823..1611d7c 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB\n prune-packable: the number of loose objects that are also present in\n the packs. These objects could be pruned using `git prune-packed`.\n +\n-garbage: the number of files in loose object database that are not\n-valid loose objects\n+garbage: the number of files in object database that are not valid\n+loose objects nor valid packs\n \n GIT\n ---\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 9afaa88..e8fabcf 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -9,6 +9,8 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n \n+static unsigned long garbage;\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@@ -65,6 +67,27 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n \t}\n }\n \n+extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n+static void real_report_pack_garbage(const char *path, int len, const char *name)\n+{\n+\tif (is_dot_or_dotdot(name))\n+\t\treturn;\n+\tif (has_extension(name, \".pack\")) {\n+\t\tstruct strbuf idx_file = STRBUF_INIT;\n+\t\tstruct stat st;\n+\n+\t\tstrbuf_addf(&idx_file, \"%.*s/%.*s.idx\", len, path,\n+\t\t\t    (int)strlen(name) - 5, name);\n+\t\tif (!stat(idx_file.buf, &st) && S_ISREG(st.st_mode)) {\n+\t\t\tstrbuf_release(&idx_file);\n+\t\t\treturn;\n+\t\t}\n+\t\tstrbuf_release(&idx_file);\n+\t}\n+\terror(\"garbage found: %.*s/%s\", len, path, name);\n+\tgarbage++;\n+}\n+\n static char const * const count_objects_usage[] = {\n \tN_(\"git count-objects [-v]\"),\n \tNULL\n@@ -76,7 +99,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, garbage = 0;\n+\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n \toff_t loose_size = 0;\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n@@ -87,6 +110,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t/* we do not take arguments other than flags for now */\n \tif (argc)\n \t\tusage_with_options(count_objects_usage, opts);\n+\tif (verbose)\n+\t\treport_pack_garbage = real_report_pack_garbage;\n \tmemcpy(path, objdir, len);\n \tif (len && objdir[len-1] != '/')\n \t\tpath[len++] = '/';\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..6045946 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1000,6 +1000,17 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n+static void dummy_report_pack_garbage(const char *path,\n+\t\t\t\t      int len,\n+\t\t\t\t      const char *name)\n+{\n+}\n+\n+void (*report_pack_garbage)(const char *path,\n+\t\t\t    int len,\n+\t\t\t    const char *name) =\n+\tdummy_report_pack_garbage;\n+\n static void prepare_packed_git_one(char *objdir, int local)\n {\n \t/* Ensure that this buffer is large enough so that we can\n@@ -1024,11 +1035,15 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (!has_extension(de->d_name, \".idx\"))\n+\t\tif (!has_extension(de->d_name, \".idx\")) {\n+\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n \t\t\tcontinue;\n+\t\t}\n \n-\t\tif (len + namelen + 1 > sizeof(path))\n+\t\tif (len + namelen + 1 > sizeof(path)) {\n+\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n \t\t\tcontinue;\n+\t\t}\n \n \t\t/* Don't reopen a pack we already have. */\n \t\tstrcpy(path + len, de->d_name);\n@@ -1042,8 +1057,10 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t * .pack file that we can map.\n \t\t */\n \t\tp = add_packed_git(path, len + namelen, local);\n-\t\tif (!p)\n+\t\tif (!p) {\n+\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n \t\t\tcontinue;\n+\t\t}\n \t\tinstall_packed_git(p);\n \t}\n \tclosedir(dir);\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"208625","messageId":"7vy5f42elc.fsf@alter.siamese.dyndns.org","threadId":"32823","inReplyTo":"1359982145-10792-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T17:06:39Z","receivedAt":"2013-02-04T17:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n>  The hook in prepare_packed_git_one is ugly, but I don't want to\n>  duplicate the search file logic there in count-objects. Maybe I'm\n>  wrong.\n\nIn this particular case I do not think you are completely wrong;\nyou are probably only two thirds wrong ;-)\n\nThe idea to use a customizable function pointer to allow it do extra\nwork is probably fine, but adding three identical calls and continue\nis a bad taste.  Just have one callsite for the error path and do\nnot hesitate to jump to it with 'goto'.\n\nI do not think failure to add_packed_git() should be treated the\nsame way as other cases where the files readdir() found are truly\ngarbage, by the way.\n\nI also wonder, especially because you are enumerating the temporary\npack files in .git/objects/pack/, if it make more sense to account\nfor their sizes as well.  After all, you would end up doing stat()\nfor a common case of files with \".pack\" suffix---doing so for all of\nthem shouldn't be too bad.\n"},{"id":"208630","messageId":"7v1ucw2bd4.fsf@alter.siamese.dyndns.org","threadId":"32823","inReplyTo":"1359982145-10792-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T18:16:23Z","receivedAt":"2013-02-04T18:16:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> @@ -1024,11 +1035,15 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\tint namelen = strlen(de->d_name);\n>  \t\tstruct packed_git *p;\n>  \n> -\t\tif (!has_extension(de->d_name, \".idx\"))\n> +\t\tif (!has_extension(de->d_name, \".idx\")) {\n> +\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n>  \t\t\tcontinue;\n> +\t\t}\n>  \n> -\t\tif (len + namelen + 1 > sizeof(path))\n> +\t\tif (len + namelen + 1 > sizeof(path)) {\n> +\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n>  \t\t\tcontinue;\n> +\t\t}\n>  \n>  \t\t/* Don't reopen a pack we already have. */\n>  \t\tstrcpy(path + len, de->d_name);\n> @@ -1042,8 +1057,10 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\t * .pack file that we can map.\n>  \t\t */\n>  \t\tp = add_packed_git(path, len + namelen, local);\n> -\t\tif (!p)\n> +\t\tif (!p) {\n> +\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n>  \t\t\tcontinue;\n> +\t\t}\n>  \t\tinstall_packed_git(p);\n>  \t}\n>  \tclosedir(dir);\n\nI forgot to mention one more thing.  Your report_pack_garbage()\nspecial cases \".pack\" to see if it is a regular file, but this loop\nstructure causes a regular file whose name ends with \".pack\" but\nwithout corresponding \".idx\" file to go unreported.\n\nI think the loop should be restructured to iterate over all known\nfile types and report unknown ones, if you want to repurpose it for\nthe reporting, something along this line, perhaps:\n\n\tfor (each name) {\n\t\tif (does it end with \".idx\") {\n\t\t\tif (is it unusable \".idx\") {\n\t\t\t\treport garbage;\n\t\t\t}\n                        continue;\n\t\t}\n\t\tif (! we are in report mode)\n\t\t\tcontinue;\n\t\tif (does it end with \".pack\") {\n\t\t\tif (!have we seen corresponding \".idx\")\n\t\t\t\tremember it;\n\t\t\tcontinue;\n\t\t}\n\t\treport garbage;\n\t}\n\tfor (remembered pack) {\n\t\tif (does it have corresponding \".idx\" &&\n\t\t\tis it really usable \".pack\")\n\t\t\tcontinue;\n\t\treport garbage;\n\t}\n"},{"id":"208899","messageId":"20130207073751.GA20672@duynguyen-vnpc.dek-tpc.internal","threadId":"32823","inReplyTo":"7v1ucw2bd4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-07T07:37:52Z","receivedAt":"2013-02-07T07:37:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 04, 2013 at 10:16:23AM -0800, Junio C Hamano wrote:\n> I forgot to mention one more thing.  Your report_pack_garbage()\n> special cases \".pack\" to see if it is a regular file, but this loop\n> structure causes a regular file whose name ends with \".pack\" but\n> without corresponding \".idx\" file to go unreported.\n> \n> I think the loop should be restructured to iterate over all known\n> file types and report unknown ones, if you want to repurpose it for\n> the reporting, something along this line, perhaps:\n> \n> \tfor (each name) {\n> \t\tif (does it end with \".idx\") {\n> \t\t\tif (is it unusable \".idx\") {\n> \t\t\t\treport garbage;\n> \t\t\t}\n>                         continue;\n> \t\t}\n> \t\tif (! we are in report mode)\n> \t\t\tcontinue;\n> \t\tif (does it end with \".pack\") {\n> \t\t\tif (!have we seen corresponding \".idx\")\n> \t\t\t\tremember it;\n> \t\t\tcontinue;\n> \t\t}\n> \t\treport garbage;\n> \t}\n> \tfor (remembered pack) {\n> \t\tif (does it have corresponding \".idx\" &&\n> \t\t\tis it really usable \".pack\")\n> \t\t\tcontinue;\n> \t\treport garbage;\n> \t}\n> \n\nHow about the below patch? Ignoring good .commits files will be just a\ncouple more lines in the \"for (remembered_pack)\" loop.\n\nAlso in another mail in this thread:\n\n> I also wonder, especially because you are enumerating the temporary\n> pack files in .git/objects/pack/, if it make more sense to account\n> for their sizes as well.  After all, you would end up doing stat()\n> for a common case of files with \".pack\" suffix---doing so for all of\n> them shouldn't be too bad.\n\nI thought about that, but we may need to do extra stat() for loose\ngarbage as well. As it is now, garbage is complained loudly, which\ngives me enough motivation to clean up, even without looking at how\nmuch disk space it uses.\n\n-- 8< --\nSubject: count-objects: report garbage pack directory too\n\nprepare_packed_git_one() is modified to allow count-objects to hook a\nreport function to so we don't need to duplicate the pack searching\nlogic in count-objects.c. When report_pack_garbage is NULL, the\noverhead is insignificant.\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex e816823..1611d7c 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB\n prune-packable: the number of loose objects that are also present in\n the packs. These objects could be pruned using `git prune-packed`.\n +\n-garbage: the number of files in loose object database that are not\n-valid loose objects\n+garbage: the number of files in object database that are not valid\n+loose objects nor valid packs\n \n GIT\n ---\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 9afaa88..7fdd508 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -9,6 +9,8 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n \n+static unsigned long garbage;\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@@ -65,6 +67,16 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n \t}\n }\n \n+extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n+static void real_report_pack_garbage(const char *path, int len, const char *name)\n+{\n+\tif (len)\n+\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n+\telse\n+\t\terror(\"garbage found: %s\", path);\n+\tgarbage++;\n+}\n+\n static char const * const count_objects_usage[] = {\n \tN_(\"git count-objects [-v]\"),\n \tNULL\n@@ -76,7 +88,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, garbage = 0;\n+\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n \toff_t loose_size = 0;\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n@@ -87,6 +99,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t/* we do not take arguments other than flags for now */\n \tif (argc)\n \t\tusage_with_options(count_objects_usage, opts);\n+\tif (verbose)\n+\t\treport_pack_garbage = real_report_pack_garbage;\n \tmemcpy(path, objdir, len);\n \tif (len && objdir[len-1] != '/')\n \t\tpath[len++] = '/';\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..5b70e55 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -21,6 +21,7 @@\n #include \"sha1-lookup.h\"\n #include \"bulk-checkin.h\"\n #include \"streaming.h\"\n+#include \"dir.h\"\n \n #ifndef O_NOATIME\n #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n@@ -1000,15 +1001,19 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n+void (*report_pack_garbage)(const char *path, int len, const char *name);\n+\n static void prepare_packed_git_one(char *objdir, int local)\n {\n \t/* Ensure that this buffer is large enough so that we can\n \t   append \"/pack/\" without clobbering the stack even if\n \t   strlen(objdir) were PATH_MAX.  */\n \tchar path[PATH_MAX + 1 + 4 + 1 + 1];\n-\tint len;\n+\tint i, len;\n \tDIR *dir;\n \tstruct dirent *de;\n+\tstruct packed_git *p;\n+\tstruct string_list garbage = STRING_LIST_INIT_DUP;\n \n \tsprintf(path, \"%s/pack\", objdir);\n \tlen = strlen(path);\n@@ -1024,14 +1029,33 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (!has_extension(de->d_name, \".idx\"))\n+\t\tif (len + namelen + 1 > sizeof(path)) {\n+\t\t\tif (report_pack_garbage)\n+\t\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n \t\t\tcontinue;\n+\t\t}\n \n-\t\tif (len + namelen + 1 > sizeof(path))\n+\t\tstrcpy(path + len, de->d_name);\n+\n+\t\tif (!has_extension(de->d_name, \".idx\")) {\n+\t\t\tif (!report_pack_garbage)\n+\t\t\t\tcontinue;\n+\t\t\tif (is_dot_or_dotdot(de->d_name))\n+\t\t\t\tcontinue;\n+\t\t\tif (!has_extension(de->d_name, \".pack\")) {\n+\t\t\t\treport_pack_garbage(path, 0, NULL);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\t/*\n+\t\t\t * we can't decide right know if this .pack is\n+\t\t\t * garbage. Delay until we identify all good\n+\t\t\t * packs.\n+\t\t\t */\n+\t\t\tstring_list_append(&garbage, path);\n \t\t\tcontinue;\n+\t\t}\n \n \t\t/* Don't reopen a pack we already have. */\n-\t\tstrcpy(path + len, de->d_name);\n \t\tfor (p = packed_git; p; p = p->next) {\n \t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n \t\t\t\tbreak;\n@@ -1047,6 +1071,25 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tinstall_packed_git(p);\n \t}\n \tclosedir(dir);\n+\n+\tif (!report_pack_garbage)\n+\t\treturn;\n+\n+\tsort_string_list(&garbage);\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tstruct string_list_item *item;\n+\t\tif (!p->pack_local)\n+\t\t\tcontinue;\n+\t\titem = string_list_lookup(&garbage, p->pack_name);\n+\t\tif (item)\n+\t\t\titem->util = &garbage; /* anything but NULL */\n+\t}\n+\tfor (i = 0; i < garbage.nr; i++) {\n+\t\tstruct string_list_item *item = garbage.items + i;\n+\t\tif (!item->util)\n+\t\t\treport_pack_garbage(item->string, 0, NULL);\n+\t}\n+\tstring_list_clear(&garbage, 0);\n }\n \n static int sort_pack(const void *a_, const void *b_)\n-- 8< --\n"},{"id":"208926","messageId":"7vzjzg802d.fsf@alter.siamese.dyndns.org","threadId":"32823","inReplyTo":"20130207073751.GA20672@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-07T18:12:58Z","receivedAt":"2013-02-07T18:12:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I thought about that, but we may need to do extra stat() for loose\n> garbage as well. As it is now, garbage is complained loudly, which\n> gives me enough motivation to clean up, even without looking at how\n> much disk space it uses.\n\nI wouldn't call a single line \"garbage: 4\" exactly *loud*.  I also\nthink that this is not about *motivating* you, but about giving\nmore information to the users to help them assess the health of\ntheir repository themselves.\n\nBy the way, I wonder if we also want to notice .git/objects/garbage\nor .git/objects/ga/rbage if we are to do this?\n\n> -- 8< --\n> Subject: count-objects: report garbage pack directory too\n>\n> prepare_packed_git_one() is modified to allow count-objects to hook a\n> report function to so we don't need to duplicate the pack searching\n> logic in count-objects.c. When report_pack_garbage is NULL, the\n> overhead is insignificant.\n>\n> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\n> index e816823..1611d7c 100644\n> --- a/Documentation/git-count-objects.txt\n> +++ b/Documentation/git-count-objects.txt\n> @@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB\n>  prune-packable: the number of loose objects that are also present in\n>  the packs. These objects could be pruned using `git prune-packed`.\n>  +\n> -garbage: the number of files in loose object database that are not\n> -valid loose objects\n> +garbage: the number of files in object database that are not valid\n> +loose objects nor valid packs\n>  \n>  GIT\n>  ---\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index 9afaa88..7fdd508 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -9,6 +9,8 @@\n>  #include \"builtin.h\"\n>  #include \"parse-options.h\"\n>  \n> +static unsigned long garbage;\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> @@ -65,6 +67,16 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n>  \t}\n>  }\n>  \n> +extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n> +static void real_report_pack_garbage(const char *path, int len, const char *name)\n> +{\n> +\tif (len)\n> +\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n> +\telse\n> +\t\terror(\"garbage found: %s\", path);\n> +\tgarbage++;\n> +}\n> +\n>  static char const * const count_objects_usage[] = {\n>  \tN_(\"git count-objects [-v]\"),\n>  \tNULL\n> @@ -76,7 +88,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, garbage = 0;\n> +\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n>  \toff_t loose_size = 0;\n>  \tstruct option opts[] = {\n>  \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n> @@ -87,6 +99,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>  \t/* we do not take arguments other than flags for now */\n>  \tif (argc)\n>  \t\tusage_with_options(count_objects_usage, opts);\n> +\tif (verbose)\n> +\t\treport_pack_garbage = real_report_pack_garbage;\n>  \tmemcpy(path, objdir, len);\n>  \tif (len && objdir[len-1] != '/')\n>  \t\tpath[len++] = '/';\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 40b2329..5b70e55 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -21,6 +21,7 @@\n>  #include \"sha1-lookup.h\"\n>  #include \"bulk-checkin.h\"\n>  #include \"streaming.h\"\n> +#include \"dir.h\"\n>  \n>  #ifndef O_NOATIME\n>  #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n> @@ -1000,15 +1001,19 @@ void install_packed_git(struct packed_git *pack)\n>  \tpacked_git = pack;\n>  }\n>  \n> +void (*report_pack_garbage)(const char *path, int len, const char *name);\n> +\n>  static void prepare_packed_git_one(char *objdir, int local)\n>  {\n>  \t/* Ensure that this buffer is large enough so that we can\n>  \t   append \"/pack/\" without clobbering the stack even if\n>  \t   strlen(objdir) were PATH_MAX.  */\n>  \tchar path[PATH_MAX + 1 + 4 + 1 + 1];\n> -\tint len;\n> +\tint i, len;\n>  \tDIR *dir;\n>  \tstruct dirent *de;\n> +\tstruct packed_git *p;\n> +\tstruct string_list garbage = STRING_LIST_INIT_DUP;\n>  \n>  \tsprintf(path, \"%s/pack\", objdir);\n>  \tlen = strlen(path);\n> @@ -1024,14 +1029,33 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\tint namelen = strlen(de->d_name);\n>  \t\tstruct packed_git *p;\n>  \n> -\t\tif (!has_extension(de->d_name, \".idx\"))\n> +\t\tif (len + namelen + 1 > sizeof(path)) {\n> +\t\t\tif (report_pack_garbage)\n> +\t\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n>  \t\t\tcontinue;\n> +\t\t}\n>  \n> -\t\tif (len + namelen + 1 > sizeof(path))\n> +\t\tstrcpy(path + len, de->d_name);\n> +\n> +\t\tif (!has_extension(de->d_name, \".idx\")) {\n> +\t\t\tif (!report_pack_garbage)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!has_extension(de->d_name, \".pack\")) {\n> +\t\t\t\treport_pack_garbage(path, 0, NULL);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nDidn't I already say the logic should be inverted to whitelist the\nknown ones?  Saying \"Anything that is not '.pack' is bad\" here is a\ndirect opposite, I think.  \n\nAdd \"A '.keep' file is OK\" to this codeflow and see how it goes.\n\n> +\t\t\t/*\n> +\t\t\t * we can't decide right know if this .pack is\n> +\t\t\t * garbage. Delay until we identify all good\n> +\t\t\t * packs.\n> +\t\t\t */\n> +\t\t\tstring_list_append(&garbage, path);\n>  \t\t\tcontinue;\n> +\t\t}\n>  \n>  \t\t/* Don't reopen a pack we already have. */\n> -\t\tstrcpy(path + len, de->d_name);\n>  \t\tfor (p = packed_git; p; p = p->next) {\n>  \t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n>  \t\t\t\tbreak;\n> @@ -1047,6 +1071,25 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\tinstall_packed_git(p);\n>  \t}\n>  \tclosedir(dir);\n> +\n> +\tif (!report_pack_garbage)\n> +\t\treturn;\n> +\n> +\tsort_string_list(&garbage);\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\tstruct string_list_item *item;\n> +\t\tif (!p->pack_local)\n> +\t\t\tcontinue;\n> +\t\titem = string_list_lookup(&garbage, p->pack_name);\n> +\t\tif (item)\n> +\t\t\titem->util = &garbage; /* anything but NULL */\n> +\t}\n> +\tfor (i = 0; i < garbage.nr; i++) {\n> +\t\tstruct string_list_item *item = garbage.items + i;\n> +\t\tif (!item->util)\n> +\t\t\treport_pack_garbage(item->string, 0, NULL);\n> +\t}\n> +\tstring_list_clear(&garbage, 0);\n>  }\n>  \n>  static int sort_pack(const void *a_, const void *b_)\n> -- 8< --\n"},{"id":"208959","messageId":"CACsJy8DaHanuPxeHv5GNqomE4=XtnyyJDepoVP8PmY88t4Au=Q@mail.gmail.com","threadId":"32823","inReplyTo":"7vzjzg802d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] count-objects: report garbage files in .git/objects/pack directory too","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-07T23:58:24Z","receivedAt":"2013-02-07T23:58:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 8, 2013 at 1:12 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> I thought about that, but we may need to do extra stat() for loose\n>> garbage as well. As it is now, garbage is complained loudly, which\n>> gives me enough motivation to clean up, even without looking at how\n>> much disk space it uses.\n>\n> I wouldn't call a single line \"garbage: 4\" exactly *loud*.  I also\n> think that this is not about *motivating* you, but about giving\n> more information to the users to help them assess the health of\n> their repository themselves.\n\nThat's not the only line it prints:\n\nerror: garbage found:\n.git/objects/pack/pack-8074dfd2b01f494a30f02d0374baa57632d26fea.commits\nerror: garbage found:\n.git/objects/pack/pack-834c9dccca7634c2b225db1b5050a980cb2c2de0.commits\nerror: garbage found: .git/objects/pack/tmp_pack_G235da\nerror: garbage found:\n.git/objects/pack/pack-8bdf298e9252573289cd4f1e83e80c9f261882a2.commits\ncount: 604\nsize: 5576\nin-pack: 172307\npacks: 4\nsize-pack: 50421\nprune-packable: 4\ngarbage: 4\n\n> By the way, I wonder if we also want to notice .git/objects/garbage\n> or .git/objects/ga/rbage if we are to do this?\n\nI prefer not (for code simplicity) because we may need to catch\n.git/objects/pack/info/garbage too if we do that.\n\n>> +                     if (!has_extension(de->d_name, \".pack\")) {\n>> +                             report_pack_garbage(path, 0, NULL);\n>> +                             continue;\n>> +                     }\n>\n> Didn't I already say the logic should be inverted to whitelist the\n> known ones?  Saying \"Anything that is not '.pack' is bad\" here is a\n> direct opposite, I think.\n\nI must have missed it. Will do.\n\n>\n> Add \"A '.keep' file is OK\" to this codeflow and see how it goes.\n\nOK\n-- \nDuy\n"},{"id":"208965","messageId":"1360295307-5469-1-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":"1359982145-10792-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 0/3] count-objects improvements","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-08T03:48:24Z","receivedAt":"2013-02-08T03:48:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This series:\n - updates count-objects -v documentation, describe each line in detail\n - counts garbage files in pack directory in addition to loose odb\n - shows how much disk space consumed by garbage files\n\nNguyễn Thái Ngọc Duy (3):\n  git-count-objects.txt: describe each line in -v output\n  count-objects: report garbage files in pack directory too\n  count-objects: report how much disk space taken by garbage files\n\n Documentation/git-count-objects.txt | 22 +++++++---\n builtin/count-objects.c             | 41 ++++++++++++++-----\n sha1_file.c                         | 81 +++++++++++++++++++++++++++++++++++--\n 3 files changed, 127 insertions(+), 17 deletions(-)\n\n-- \n1.8.1.2.495.g3fdf5d5.dirty\n"},{"id":"208966","messageId":"1360295307-5469-2-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":"1360295307-5469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] git-count-objects.txt: describe each line in -v output","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-08T03:48:25Z","receivedAt":"2013-02-08T03:48:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The current description requires a bit of guessing (what clause\ncorresponds to what printed line?) and lacks information, such as\nthe unit of size and size-pack.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-count-objects.txt | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex 23c80ce..e816823 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -20,11 +20,21 @@ OPTIONS\n -------\n -v::\n --verbose::\n-\tIn addition to the number of loose objects and disk\n-\tspace consumed, it reports the number of in-pack\n-\tobjects, number of packs, disk space consumed by those packs,\n-\tand number of objects that can be removed by running\n-\t`git prune-packed`.\n+\tReport in more detail:\n++\n+count: the number of loose objects\n++\n+size: disk space consumed by loose objects, in KiB\n++\n+in-pack: the number of in-pack objects\n++\n+size-pack: disk space consumed by the packs, in KiB\n++\n+prune-packable: the number of loose objects that are also present in\n+the packs. These objects could be pruned using `git prune-packed`.\n++\n+garbage: the number of files in loose object database that are not\n+valid loose objects\n \n GIT\n ---\n-- \n1.8.1.2.495.g3fdf5d5.dirty\n"},{"id":"208967","messageId":"1360295307-5469-3-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":"1360295307-5469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] count-objects: report garbage files in pack directory too","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-08T03:48:26Z","receivedAt":"2013-02-08T03:48:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"prepare_packed_git_one() is modified to allow count-objects to hook a\nreport function to so we don't need to duplicate the pack searching\nlogic in count-objects.c. When report_pack_garbage is NULL, the\noverhead is insignificant.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/git-count-objects.txt |  4 +-\n builtin/count-objects.c             | 18 ++++++++-\n sha1_file.c                         | 81 +++++++++++++++++++++++++++++++++++--\n 3 files changed, 97 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex e816823..1611d7c 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB\n prune-packable: the number of loose objects that are also present in\n the packs. These objects could be pruned using `git prune-packed`.\n +\n-garbage: the number of files in loose object database that are not\n-valid loose objects\n+garbage: the number of files in object database that are not valid\n+loose objects nor valid packs\n \n GIT\n ---\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 9afaa88..118b2ae 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -9,6 +9,20 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n \n+static unsigned long garbage;\n+\n+extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n+static void real_report_pack_garbage(const char *path, int len, const char *name)\n+{\n+\tif (len && name)\n+\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n+\telse if (!len && name)\n+\t\terror(\"garbage found: %s%s\", path, name);\n+\telse\n+\t\terror(\"garbage found: %s\", 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@@ -76,7 +90,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, garbage = 0;\n+\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n \toff_t loose_size = 0;\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n@@ -87,6 +101,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t/* we do not take arguments other than flags for now */\n \tif (argc)\n \t\tusage_with_options(count_objects_usage, opts);\n+\tif (verbose)\n+\t\treport_pack_garbage = real_report_pack_garbage;\n \tmemcpy(path, objdir, len);\n \tif (len && objdir[len-1] != '/')\n \t\tpath[len++] = '/';\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 40b2329..cc6ef03 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -21,6 +21,7 @@\n #include \"sha1-lookup.h\"\n #include \"bulk-checkin.h\"\n #include \"streaming.h\"\n+#include \"dir.h\"\n \n #ifndef O_NOATIME\n #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n@@ -1000,6 +1001,54 @@ void install_packed_git(struct packed_git *pack)\n \tpacked_git = pack;\n }\n \n+/* A hook for count-objects to report invalid files in pack directory */\n+void (*report_pack_garbage)(const char *path, int len, const char *name);\n+\n+static const char *known_pack_extensions[] = { \".pack\", \".keep\", NULL };\n+\n+static void report_garbage(struct string_list *list)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct packed_git *p;\n+\tint i;\n+\n+\tif (!report_pack_garbage)\n+\t\treturn;\n+\n+\tsort_string_list(list);\n+\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tstruct string_list_item *item;\n+\t\tif (!p->pack_local)\n+\t\t\tcontinue;\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_add(&sb, p->pack_name,\n+\t\t\t   strlen(p->pack_name) - strlen(\".pack\"));\n+\t\titem = string_list_lookup(list, sb.buf);\n+\t\tif (!item)\n+\t\t\tcontinue;\n+\t\t/*\n+\t\t * string_list_lookup does not guarantee to return the\n+\t\t * first matched string if it's duplicated.\n+\t\t */\n+\t\twhile (item - list->items &&\n+\t\t       !strcmp(item[-1].string, item->string))\n+\t\t\titem--;\n+\t\twhile (item - list->items < list->nr &&\n+\t\t       !strcmp(item->string, sb.buf)) {\n+\t\t\titem->util = NULL; /* non-garbage mark */\n+\t\t\titem++;\n+\t\t}\n+\t}\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tstruct string_list_item *item = list->items + i;\n+\t\tif (!item->util)\n+\t\t\tcontinue;\n+\t\treport_pack_garbage(item->string, 0, item->util);\n+\t}\n+\tstrbuf_release(&sb);\n+}\n+\n static void prepare_packed_git_one(char *objdir, int local)\n {\n \t/* Ensure that this buffer is large enough so that we can\n@@ -1009,6 +1058,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tint len;\n \tDIR *dir;\n \tstruct dirent *de;\n+\tstruct string_list garbage = STRING_LIST_INIT_DUP;\n \n \tsprintf(path, \"%s/pack\", objdir);\n \tlen = strlen(path);\n@@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (!has_extension(de->d_name, \".idx\"))\n+\t\tif (len + namelen + 1 > sizeof(path)) {\n+\t\t\tif (report_pack_garbage)\n+\t\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n \t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrcpy(path + len, de->d_name);\n \n-\t\tif (len + namelen + 1 > sizeof(path))\n+\t\tif (!has_extension(de->d_name, \".idx\")) {\n+\t\t\tstruct string_list_item *item;\n+\t\t\tint i, n;\n+\t\t\tif (!report_pack_garbage)\n+\t\t\t\tcontinue;\n+\t\t\tif (is_dot_or_dotdot(de->d_name))\n+\t\t\t\tcontinue;\n+\t\t\tfor (i = 0; known_pack_extensions[i]; i++)\n+\t\t\t\tif (has_extension(de->d_name,\n+\t\t\t\t\t\t  known_pack_extensions[i]))\n+\t\t\t\t\tbreak;\n+\t\t\tif (!known_pack_extensions[i]) {\n+\t\t\t\treport_pack_garbage(path, 0, NULL);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tn = strlen(path) - strlen(known_pack_extensions[i]);\n+\t\t\titem = string_list_append_nodup(&garbage,\n+\t\t\t\t\t\t\txstrndup(path, n));\n+\t\t\titem->util = (void*)known_pack_extensions[i];\n \t\t\tcontinue;\n+\t\t}\n \n \t\t/* Don't reopen a pack we already have. */\n-\t\tstrcpy(path + len, de->d_name);\n \t\tfor (p = packed_git; p; p = p->next) {\n \t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n \t\t\t\tbreak;\n@@ -1047,6 +1120,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tinstall_packed_git(p);\n \t}\n \tclosedir(dir);\n+\treport_garbage(&garbage);\n+\tstring_list_clear(&garbage, 0);\n }\n \n static int sort_pack(const void *a_, const void *b_)\n-- \n1.8.1.2.495.g3fdf5d5.dirty\n"},{"id":"208968","messageId":"1360295307-5469-4-git-send-email-pclouds@gmail.com","threadId":"32823","inReplyTo":"1360295307-5469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] count-objects: report how much disk space taken by garbage files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-08T03:48:27Z","receivedAt":"2013-02-08T03:48:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n We may do some redundant stat() here, but I don't think it can slow\n count-objects down much to worry about.\n\n Documentation/git-count-objects.txt |  2 ++\n builtin/count-objects.c             | 29 ++++++++++++++++++-----------\n 2 files changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\nindex 1611d7c..da6e72e 100644\n--- a/Documentation/git-count-objects.txt\n+++ b/Documentation/git-count-objects.txt\n@@ -35,6 +35,8 @@ the packs. These objects could be pruned using `git prune-packed`.\n +\n garbage: the number of files in object database that are not valid\n loose objects nor valid packs\n++\n+size-garbage: disk space consumed by garbage files, in KiB\n \n GIT\n ---\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 118b2ae..90d476d 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -10,24 +10,33 @@\n #include \"parse-options.h\"\n \n static unsigned long garbage;\n+static off_t size_garbage;\n \n extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n static void real_report_pack_garbage(const char *path, int len, const char *name)\n {\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct stat st;\n+\n \tif (len && name)\n-\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n+\t\tstrbuf_addf(&sb, \"%.*s/%s\", len, path, name);\n \telse if (!len && name)\n-\t\terror(\"garbage found: %s%s\", path, name);\n+\t\tstrbuf_addf(&sb, \"%s%s\", path, name);\n \telse\n-\t\terror(\"garbage found: %s\", path);\n+\t\tstrbuf_addf(&sb, \"%s\", path);\n+\terror(_(\"garbage found: %s\"), sb.buf);\n+\n+\tif (!stat(sb.buf, &st))\n+\t\tsize_garbage += st.st_size;\n+\n \tgarbage++;\n+\tstrbuf_release(&sb);\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-\t\t\t  unsigned long *garbage)\n+\t\t\t  unsigned long *packed_loose)\n {\n \tstruct dirent *ent;\n \twhile ((ent = readdir(d)) != NULL) {\n@@ -59,11 +68,8 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n \t\t\t\t(*loose_size) += xsize_t(on_disk_bytes(st));\n \t\t}\n \t\tif (bad) {\n-\t\t\tif (verbose) {\n-\t\t\t\terror(\"garbage found: %.*s/%s\",\n-\t\t\t\t      len + 2, path, ent->d_name);\n-\t\t\t\t(*garbage)++;\n-\t\t\t}\n+\t\t\tif (verbose)\n+\t\t\t\treport_pack_garbage(path, len + 2, ent->d_name);\n \t\t\tcontinue;\n \t\t}\n \t\t(*loose)++;\n@@ -113,7 +119,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, &garbage);\n+\t\t\t      &loose, &loose_size, &packed_loose);\n \t\tclosedir(d);\n \t}\n \tif (verbose) {\n@@ -138,6 +144,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tprintf(\"size-pack: %lu\\n\", (unsigned long) (size_pack / 1024));\n \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n \t\tprintf(\"garbage: %lu\\n\", garbage);\n+\t\tprintf(\"size-garbage: %lu\\n\", (unsigned long) (size_garbage / 1024));\n \t}\n \telse\n \t\tprintf(\"%lu objects, %lu kilobytes\\n\",\n-- \n1.8.1.2.495.g3fdf5d5.dirty\n"},{"id":"209021","messageId":"7vwqui1w84.fsf@alter.siamese.dyndns.org","threadId":"32823","inReplyTo":"1360295307-5469-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/3] count-objects: report garbage files in pack directory too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-08T18:44:43Z","receivedAt":"2013-02-08T18:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> prepare_packed_git_one() is modified to allow count-objects to hook a\n> report function to so we don't need to duplicate the pack searching\n> logic in count-objects.c. When report_pack_garbage is NULL, the\n> overhead is insignificant.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Documentation/git-count-objects.txt |  4 +-\n>  builtin/count-objects.c             | 18 ++++++++-\n>  sha1_file.c                         | 81 +++++++++++++++++++++++++++++++++++--\n>  3 files changed, 97 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\n> index e816823..1611d7c 100644\n> --- a/Documentation/git-count-objects.txt\n> +++ b/Documentation/git-count-objects.txt\n> @@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB\n>  prune-packable: the number of loose objects that are also present in\n>  the packs. These objects could be pruned using `git prune-packed`.\n>  +\n> -garbage: the number of files in loose object database that are not\n> -valid loose objects\n> +garbage: the number of files in object database that are not valid\n> +loose objects nor valid packs\n>  \n>  GIT\n>  ---\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index 9afaa88..118b2ae 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -9,6 +9,20 @@\n>  #include \"builtin.h\"\n>  #include \"parse-options.h\"\n>  \n> +static unsigned long garbage;\n> +\n> +extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n> +static void real_report_pack_garbage(const char *path, int len, const char *name)\n> +{\n\nDon't some callers call this on paths outside objects/pack/\ndirectory?  Is it still report-pack-garbage?\n\n> +\tif (len && name)\n> +\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n> +\telse if (!len && name)\n> +\t\terror(\"garbage found: %s%s\", path, name);\n> +\telse\n> +\t\terror(\"garbage found: %s\", 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> @@ -76,7 +90,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, garbage = 0;\n> +\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n>  \toff_t loose_size = 0;\n>  \tstruct option opts[] = {\n>  \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n> @@ -87,6 +101,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>  \t/* we do not take arguments other than flags for now */\n>  \tif (argc)\n>  \t\tusage_with_options(count_objects_usage, opts);\n> +\tif (verbose)\n> +\t\treport_pack_garbage = real_report_pack_garbage;\n>  \tmemcpy(path, objdir, len);\n>  \tif (len && objdir[len-1] != '/')\n>  \t\tpath[len++] = '/';\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 40b2329..cc6ef03 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -21,6 +21,7 @@\n>  #include \"sha1-lookup.h\"\n>  #include \"bulk-checkin.h\"\n>  #include \"streaming.h\"\n> +#include \"dir.h\"\n>  \n>  #ifndef O_NOATIME\n>  #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n> @@ -1000,6 +1001,54 @@ void install_packed_git(struct packed_git *pack)\n>  \tpacked_git = pack;\n>  }\n>  \n> +/* A hook for count-objects to report invalid files in pack directory */\n> +void (*report_pack_garbage)(const char *path, int len, const char *name);\n> +\n> +static const char *known_pack_extensions[] = { \".pack\", \".keep\", NULL };\n\nThis sounds wrong.  Isn't \".idx\" also known?\n\n> +static void report_garbage(struct string_list *list)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct packed_git *p;\n> +\tint i;\n> +\n> +\tif (!report_pack_garbage)\n> +\t\treturn;\n> +\n> +\tsort_string_list(list);\n> +\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\tstruct string_list_item *item;\n> +\t\tif (!p->pack_local)\n> +\t\t\tcontinue;\n> +\t\tstrbuf_reset(&sb);\n> +\t\tstrbuf_add(&sb, p->pack_name,\n> +\t\t\t   strlen(p->pack_name) - strlen(\".pack\"));\n> +\t\titem = string_list_lookup(list, sb.buf);\n> +\t\tif (!item)\n> +\t\t\tcontinue;\n> +\t\t/*\n> +\t\t * string_list_lookup does not guarantee to return the\n> +\t\t * first matched string if it's duplicated.\n> +\t\t */\n> +\t\twhile (item - list->items &&\n> +\t\t       !strcmp(item[-1].string, item->string))\n> +\t\t\titem--;\n> +\t\twhile (item - list->items < list->nr &&\n> +\t\t       !strcmp(item->string, sb.buf)) {\n> +\t\t\titem->util = NULL; /* non-garbage mark */\n> +\t\t\titem++;\n> +\t\t}\n> +\t}\n> +\tfor (i = 0; i < list->nr; i++) {\n> +\t\tstruct string_list_item *item = list->items + i;\n> +\t\tif (!item->util)\n> +\t\t\tcontinue;\n> +\t\treport_pack_garbage(item->string, 0, item->util);\n> +\t}\n> +\tstrbuf_release(&sb);\n> +}\n> +\n>  static void prepare_packed_git_one(char *objdir, int local)\n>  {\n>  \t/* Ensure that this buffer is large enough so that we can\n> @@ -1009,6 +1058,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \tint len;\n>  \tDIR *dir;\n>  \tstruct dirent *de;\n> +\tstruct string_list garbage = STRING_LIST_INIT_DUP;\n>  \n>  \tsprintf(path, \"%s/pack\", objdir);\n>  \tlen = strlen(path);\n> @@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\tint namelen = strlen(de->d_name);\n>  \t\tstruct packed_git *p;\n>  \n> -\t\tif (!has_extension(de->d_name, \".idx\"))\n> +\t\tif (len + namelen + 1 > sizeof(path)) {\n> +\t\t\tif (report_pack_garbage)\n> +\t\t\t\treport_pack_garbage(path, len - 1, de->d_name);\n\nA pack/in/a/very/long/path/pack-0000000000000000000000000000000000000000.pack\nmay pass when fed to \"git verify-pack\", but this will report it as \"garbage\",\nwithout reporting what is wrong with it.  Wouldn't that confuse users?\n\n>  \t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tstrcpy(path + len, de->d_name);\n>  \n> -\t\tif (len + namelen + 1 > sizeof(path))\n> +\t\tif (!has_extension(de->d_name, \".idx\")) {\n> +\t\t\tstruct string_list_item *item;\n> +\t\t\tint i, n;\n> +\t\t\tif (!report_pack_garbage)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> +\t\t\t\tcontinue;\n> +\t\t\tfor (i = 0; known_pack_extensions[i]; i++)\n> +\t\t\t\tif (has_extension(de->d_name,\n> +\t\t\t\t\t\t  known_pack_extensions[i]))\n> +\t\t\t\t\tbreak;\n> +\t\t\tif (!known_pack_extensions[i]) {\n> +\t\t\t\treport_pack_garbage(path, 0, NULL);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tn = strlen(path) - strlen(known_pack_extensions[i]);\n> +\t\t\titem = string_list_append_nodup(&garbage,\n> +\t\t\t\t\t\t\txstrndup(path, n));\n> +\t\t\titem->util = (void*)known_pack_extensions[i];\n>  \t\t\tcontinue;\n> +\t\t}\n\nWhy isn't this part more like this?\n\n\t\tif (dot-or-dotdot) {\n\t\t\tcontinue;\n\t\t} else if (has_extension(de->d_name, \".idx\")) {\n\t\t\tdo things for the .idx file;\n\t\t} else if (has_extension(de->d_name, \".pack\") {\n\t\t\tdo things for the .pack file, including\n                        queuing the name if we haven't seen\n\t\t\tcorresponding .idx for later examination;\n\t\t} else if (has_extension(de->d_name, \".keep\") {\n                \tnothing special for now but we may\n                        want to add some other checks later\n\t\t} else {\n                \teverything else is a garbage\n                        report_pack_garbage();\n\t\t}\n                        \n\n>  \n>  \t\t/* Don't reopen a pack we already have. */\n> -\t\tstrcpy(path + len, de->d_name);\n>  \t\tfor (p = packed_git; p; p = p->next) {\n>  \t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n>  \t\t\t\tbreak;\n> @@ -1047,6 +1120,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n>  \t\tinstall_packed_git(p);\n>  \t}\n>  \tclosedir(dir);\n> +\treport_garbage(&garbage);\n> +\tstring_list_clear(&garbage, 0);\n>  }\n>  \n>  static int sort_pack(const void *a_, const void *b_)\n"},{"id":"209022","messageId":"7vsj561w3g.fsf@alter.siamese.dyndns.org","threadId":"32823","inReplyTo":"1360295307-5469-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] count-objects: report how much disk space taken by garbage files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-08T18:47:31Z","receivedAt":"2013-02-08T18:47:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  We may do some redundant stat() here, but I don't think it can slow\n>  count-objects down much to worry about.\n\nI don't either.  Looks like a good change to me.\n\nIt appears that the sb.buf refactoring is better done to the\nprevious patch, but that is minor.\n\n>  Documentation/git-count-objects.txt |  2 ++\n>  builtin/count-objects.c             | 29 ++++++++++++++++++-----------\n>  2 files changed, 20 insertions(+), 11 deletions(-)\n>\n> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt\n> index 1611d7c..da6e72e 100644\n> --- a/Documentation/git-count-objects.txt\n> +++ b/Documentation/git-count-objects.txt\n> @@ -35,6 +35,8 @@ the packs. These objects could be pruned using `git prune-packed`.\n>  +\n>  garbage: the number of files in object database that are not valid\n>  loose objects nor valid packs\n> ++\n> +size-garbage: disk space consumed by garbage files, in KiB\n>  \n>  GIT\n>  ---\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index 118b2ae..90d476d 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -10,24 +10,33 @@\n>  #include \"parse-options.h\"\n>  \n>  static unsigned long garbage;\n> +static off_t size_garbage;\n>  \n>  extern void (*report_pack_garbage)(const char *path, int len, const char *name);\n>  static void real_report_pack_garbage(const char *path, int len, const char *name)\n>  {\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct stat st;\n> +\n>  \tif (len && name)\n> -\t\terror(\"garbage found: %.*s/%s\", len, path, name);\n> +\t\tstrbuf_addf(&sb, \"%.*s/%s\", len, path, name);\n>  \telse if (!len && name)\n> -\t\terror(\"garbage found: %s%s\", path, name);\n> +\t\tstrbuf_addf(&sb, \"%s%s\", path, name);\n>  \telse\n> -\t\terror(\"garbage found: %s\", path);\n> +\t\tstrbuf_addf(&sb, \"%s\", path);\n> +\terror(_(\"garbage found: %s\"), sb.buf);\n> +\n> +\tif (!stat(sb.buf, &st))\n> +\t\tsize_garbage += st.st_size;\n> +\n>  \tgarbage++;\n> +\tstrbuf_release(&sb);\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> -\t\t\t  unsigned long *garbage)\n> +\t\t\t  unsigned long *packed_loose)\n>  {\n>  \tstruct dirent *ent;\n>  \twhile ((ent = readdir(d)) != NULL) {\n> @@ -59,11 +68,8 @@ static void count_objects(DIR *d, char *path, int len, int verbose,\n>  \t\t\t\t(*loose_size) += xsize_t(on_disk_bytes(st));\n>  \t\t}\n>  \t\tif (bad) {\n> -\t\t\tif (verbose) {\n> -\t\t\t\terror(\"garbage found: %.*s/%s\",\n> -\t\t\t\t      len + 2, path, ent->d_name);\n> -\t\t\t\t(*garbage)++;\n> -\t\t\t}\n> +\t\t\tif (verbose)\n> +\t\t\t\treport_pack_garbage(path, len + 2, ent->d_name);\n>  \t\t\tcontinue;\n>  \t\t}\n>  \t\t(*loose)++;\n> @@ -113,7 +119,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\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, &garbage);\n> +\t\t\t      &loose, &loose_size, &packed_loose);\n>  \t\tclosedir(d);\n>  \t}\n>  \tif (verbose) {\n> @@ -138,6 +144,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n>  \t\tprintf(\"size-pack: %lu\\n\", (unsigned long) (size_pack / 1024));\n>  \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n>  \t\tprintf(\"garbage: %lu\\n\", garbage);\n> +\t\tprintf(\"size-garbage: %lu\\n\", (unsigned long) (size_garbage / 1024));\n>  \t}\n>  \telse\n>  \t\tprintf(\"%lu objects, %lu kilobytes\\n\",\n"},{"id":"209063","messageId":"CACsJy8D1qMpLRS0nyayRc=sq2A4X-dbiXhdVdfP6OOTCkxms4Q@mail.gmail.com","threadId":"32823","inReplyTo":"7vwqui1w84.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] count-objects: report garbage files in pack directory too","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-09T01:58:23Z","receivedAt":"2013-02-09T01:58:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 9, 2013 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +static void real_report_pack_garbage(const char *path, int len, const char *name)\n>> +{\n>\n> Don't some callers call this on paths outside objects/pack/\n> directory?  Is it still report-pack-garbage?\n\nIn fact 3/3 uses it to report loose garbage. Will rename.\n\n>> +static const char *known_pack_extensions[] = { \".pack\", \".keep\", NULL };\n>\n> This sounds wrong.  Isn't \".idx\" also known?\n\nI had a comment saying \"all known extensions related to a pack, except\n.idx\" but I dropped it. .idx being used as the pointer file needs to\nbe handled separately.\n\n>> @@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)\n>>               int namelen = strlen(de->d_name);\n>>               struct packed_git *p;\n>>\n>> -             if (!has_extension(de->d_name, \".idx\"))\n>> +             if (len + namelen + 1 > sizeof(path)) {\n>> +                     if (report_pack_garbage)\n>> +                             report_pack_garbage(path, len - 1, de->d_name);\n>\n> A pack/in/a/very/long/path/pack-0000000000000000000000000000000000000000.pack\n> may pass when fed to \"git verify-pack\", but this will report it as \"garbage\",\n> without reporting what is wrong with it.  Wouldn't that confuse users?\n\nIf all other git commands do not recognize the pack, then I think it's\nstill considered garbage. We could either\n\n - make prepare_packed_git_one accept pack/in/a/very/long/path-...\n - show the reason why we consider it garbage\n\nWhich option do we do? Option 1 may involve chdir in, stat stat stat\nand chdir out to get around short PATH_MAX limit on some system.\n\n>> -             if (len + namelen + 1 > sizeof(path))\n>> +             if (!has_extension(de->d_name, \".idx\")) {\n>> +                     struct string_list_item *item;\n>> +                     int i, n;\n>> +                     if (!report_pack_garbage)\n>> +                             continue;\n>> +                     if (is_dot_or_dotdot(de->d_name))\n>> +                             continue;\n>> +                     for (i = 0; known_pack_extensions[i]; i++)\n>> +                             if (has_extension(de->d_name,\n>> +                                               known_pack_extensions[i]))\n>> +                                     break;\n>> +                     if (!known_pack_extensions[i]) {\n>> +                             report_pack_garbage(path, 0, NULL);\n>> +                             continue;\n>> +                     }\n>> +                     n = strlen(path) - strlen(known_pack_extensions[i]);\n>> +                     item = string_list_append_nodup(&garbage,\n>> +                                                     xstrndup(path, n));\n>> +                     item->util = (void*)known_pack_extensions[i];\n>>                       continue;\n>> +             }\n>\n> Why isn't this part more like this?\n>\n>                 if (dot-or-dotdot) {\n>                         continue;\n>                 } else if (has_extension(de->d_name, \".idx\")) {\n>                         do things for the .idx file;\n>                 } else if (has_extension(de->d_name, \".pack\") {\n>                         do things for the .pack file, including\n>                         queuing the name if we haven't seen\n>                         corresponding .idx for later examination;\n>                 } else if (has_extension(de->d_name, \".keep\") {\n>                         nothing special for now but we may\n>                         want to add some other checks later\n>                 } else {\n>                         everything else is a garbage\n>                         report_pack_garbage();\n>                 }\n\nOriginally I checked known_extensions again in report_pack_garbage()\nbut after restructuring, yeah we can drop known_extensions and do it\nyour way. Looks much clearer.\n-- \nDuy\n"}]}