[PATCH v3 0/4] count-objects improvements

[PATCH v3 0/4] count-objects improvements

[Nguyễn Thái Ngọc Duy]

Compared to v2 [1], this version
 - fixes sparse warning
 - restructures 2/3 (now 3/4) to make it easier to read
 - report "path too long" instead of "garbage found" in
   .git/path/too/long/pack-xxx.pack case
 - changes output prefix "error:" to "warning:"

[1] http://thread.gmane.org/gmane.comp.version-control.git/215378/focus=215744

Nguyễn Thái Ngọc Duy (4):
  git-count-objects.txt: describe each line in -v output
  sha1_file: reorder code in prepare_packed_git_one()
  count-objects: report garbage files in pack directory too
  count-objects: report how much disk space taken by garbage files

 Documentation/git-count-objects.txt |  22 ++++++--
 builtin/count-objects.c             |  43 +++++++++++---
 cache.h                             |   3 +
 sha1_file.c                         | 108 +++++++++++++++++++++++++++++++-----
 4 files changed, 148 insertions(+), 28 deletions(-)

-- 
1.8.1.2.536.gf441e6d

[PATCH v3 1/4] git-count-objects.txt: describe each line in -v output

[Nguyễn Thái Ngọc Duy]

The current description requires a bit of guessing (what clause
corresponds to what printed line?) and lacks information, such as
the unit of size and size-pack.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 23c80ce..e816823 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -20,11 +20,21 @@ OPTIONS
 -------
 -v::
 --verbose::
-	In addition to the number of loose objects and disk
-	space consumed, it reports the number of in-pack
-	objects, number of packs, disk space consumed by those packs,
-	and number of objects that can be removed by running
-	`git prune-packed`.
+	Report in more detail:
++
+count: the number of loose objects
++
+size: disk space consumed by loose objects, in KiB
++
+in-pack: the number of in-pack objects
++
+size-pack: disk space consumed by the packs, in KiB
++
+prune-packable: the number of loose objects that are also present in
+the packs. These objects could be pruned using `git prune-packed`.
++
+garbage: the number of files in loose object database that are not
+valid loose objects
 
 GIT
 ---
-- 
1.8.1.2.536.gf441e6d

[PATCH v3 2/4] sha1_file: reorder code in prepare_packed_git_one()

[Nguyễn Thái Ngọc Duy]

The current loop does

	while (...) {
		if (!not .idx file)
			continue;
		process .idx file;
	}

and is reordered to

	while (...) {
		if (!.idx file) {
			process .idx file;
		}
	}

This makes it easier to add new extension file processing.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sha1_file.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..8d7da1d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1024,27 +1024,28 @@ static void prepare_packed_git_one(char *objdir, int local)
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (!has_extension(de->d_name, ".idx"))
-			continue;
-
 		if (len + namelen + 1 > sizeof(path))
 			continue;
 
-		/* Don't reopen a pack we already have. */
 		strcpy(path + len, de->d_name);
-		for (p = packed_git; p; p = p->next) {
-			if (!memcmp(path, p->pack_name, len + namelen - 4))
-				break;
+
+		if (has_extension(de->d_name, ".idx")) {
+			/* Don't reopen a pack we already have. */
+			for (p = packed_git; p; p = p->next) {
+				if (!memcmp(path, p->pack_name, len + namelen - 4))
+					break;
+			}
+			if (p)
+				continue;
+			/*
+			 * See if it really is a valid .idx file with
+			 * corresponding .pack file that we can map.
+			 */
+			p = add_packed_git(path, len + namelen, local);
+			if (!p)
+				continue;
+			install_packed_git(p);
 		}
-		if (p)
-			continue;
-		/* See if it really is a valid .idx file with corresponding
-		 * .pack file that we can map.
-		 */
-		p = add_packed_git(path, len + namelen, local);
-		if (!p)
-			continue;
-		install_packed_git(p);
 	}
 	closedir(dir);
 }
-- 
1.8.1.2.536.gf441e6d

[PATCH v3 3/4] count-objects: report garbage files in pack directory too

[Nguyễn Thái Ngọc Duy]

prepare_packed_git_one() is modified to allow count-objects to hook a
report function to so we don't need to duplicate the pack searching
logic in count-objects.c. When report_pack_garbage is NULL, the
overhead is insignificant.

The garbage is reported with warning() instead of error() in packed
garbage case because it's not an error to have garbage. Loose garbage
is still reported as errors and will be converted to warnings later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  4 +-
 builtin/count-objects.c             | 22 +++++++++-
 cache.h                             |  3 ++
 sha1_file.c                         | 83 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index e816823..1611d7c 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
 +
-garbage: the number of files in loose object database that are not
-valid loose objects
+garbage: the number of files in object database that are not valid
+loose objects nor valid packs
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..639c9a5 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -9,6 +9,24 @@
 #include "builtin.h"
 #include "parse-options.h"
 
+static unsigned long garbage;
+
+static void real_report_garbage(const char *desc,
+				const char *path, int len,
+				const char *name)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (len && name)
+		strbuf_addf(&sb, "%.*s/%s", len, path, name);
+	else if (!len && name)
+		strbuf_addf(&sb, "%s%s", path, name);
+	else
+		strbuf_addf(&sb, "%s", path);
+	warning("%s: %s", desc, sb.buf);
+	garbage++;
+	strbuf_release(&sb);
+}
+
 static void count_objects(DIR *d, char *path, int len, int verbose,
 			  unsigned long *loose,
 			  off_t *loose_size,
@@ -76,7 +94,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
-	unsigned long loose = 0, packed = 0, packed_loose = 0, garbage = 0;
+	unsigned long loose = 0, packed = 0, packed_loose = 0;
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -87,6 +105,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	/* we do not take arguments other than flags for now */
 	if (argc)
 		usage_with_options(count_objects_usage, opts);
+	if (verbose)
+		report_garbage = real_report_garbage;
 	memcpy(path, objdir, len);
 	if (len && objdir[len-1] != '/')
 		path[len++] = '/';
diff --git a/cache.h b/cache.h
index 7339f21..e486499 100644
--- a/cache.h
+++ b/cache.h
@@ -1051,6 +1051,9 @@ extern const char *parse_feature_value(const char *feature_list, const char *fea
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_garbage)(const char *desc, const char *path, int len, const char *name);
+
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
 extern void install_packed_git(struct packed_git *pack);
diff --git a/sha1_file.c b/sha1_file.c
index 8d7da1d..290e348 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -21,6 +21,7 @@
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
 #include "streaming.h"
+#include "dir.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1000,6 +1001,52 @@ void install_packed_git(struct packed_git *pack)
 	packed_git = pack;
 }
 
+void (*report_garbage)(const char *desc, const char *path,
+		       int len, const char *name);
+
+static void report_pack_garbage(struct string_list *list)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct packed_git *p;
+	int i;
+
+	if (!report_garbage)
+		return;
+
+	sort_string_list(list);
+
+	for (p = packed_git; p; p = p->next) {
+		struct string_list_item *item;
+		if (!p->pack_local)
+			continue;
+		strbuf_reset(&sb);
+		strbuf_add(&sb, p->pack_name,
+			   strlen(p->pack_name) - 5); /* ".pack" */
+		item = string_list_lookup(list, sb.buf);
+		if (!item)
+			continue;
+		/*
+		 * string_list_lookup does not guarantee to return the
+		 * first matched string if it's duplicated.
+		 */
+		while (item - list->items &&
+		       !strcmp(item[-1].string, item->string))
+			item--;
+		while (item - list->items < list->nr &&
+		       !strcmp(item->string, sb.buf)) {
+			item->util = NULL; /* non-garbage mark */
+			item++;
+		}
+	}
+	for (i = 0; i < list->nr; i++) {
+		struct string_list_item *item = list->items + i;
+		if (!item->util)
+			continue;
+		report_garbage("garbage found", item->string, 0, item->util);
+	}
+	strbuf_release(&sb);
+}
+
 static void prepare_packed_git_one(char *objdir, int local)
 {
 	/* Ensure that this buffer is large enough so that we can
@@ -1009,6 +1056,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 	int len;
 	DIR *dir;
 	struct dirent *de;
+	struct string_list garbage = STRING_LIST_INIT_DUP;
 
 	sprintf(path, "%s/pack", objdir);
 	len = strlen(path);
@@ -1024,7 +1072,14 @@ static void prepare_packed_git_one(char *objdir, int local)
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (len + namelen + 1 > sizeof(path))
+		if (len + namelen + 1 > sizeof(path)) {
+			if (report_garbage)
+				report_garbage("path too long",
+					       path, len - 1, de->d_name);
+			continue;
+		}
+
+		if (is_dot_or_dotdot(de->d_name))
 			continue;
 
 		strcpy(path + len, de->d_name);
@@ -1045,9 +1100,33 @@ static void prepare_packed_git_one(char *objdir, int local)
 			if (!p)
 				continue;
 			install_packed_git(p);
-		}
+		} else if (!report_garbage) {
+			/*
+			 * the rest of this if-chain requires
+			 * report_garbage != NULL. Stop the chain if
+			 * report_garbage is NULL.
+			 */
+			;
+		} else if (has_extension(de->d_name, ".pack")) {
+			struct string_list_item *item;
+			int n = strlen(path) - 5;
+			item = string_list_append_nodup(&garbage,
+							 xstrndup(path, n));
+			item->util = ".pack";
+			continue;
+		} else if (has_extension(de->d_name, ".idx")) {
+			struct string_list_item *item;
+			int n = strlen(path) - 4;
+			item = string_list_append_nodup(&garbage,
+							xstrndup(path, n));
+			item->util = ".idx";
+			continue;
+		} else
+			report_garbage("garbage found", path, 0, NULL);
 	}
 	closedir(dir);
+	report_pack_garbage(&garbage);
+	string_list_clear(&garbage, 0);
 }
 
 static int sort_pack(const void *a_, const void *b_)
-- 
1.8.1.2.536.gf441e6d

[PATCH v3 4/4] count-objects: report how much disk space taken by garbage files

[Nguyễn Thái Ngọc Duy]

Also issue warnings on loose garbages instead of errors as a result of
using report_garbage() function in count_objects()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  2 ++
 builtin/count-objects.c             | 21 +++++++++++++--------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 1611d7c..da6e72e 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -35,6 +35,8 @@ the packs. These objects could be pruned using `git prune-packed`.
 +
 garbage: the number of files in object database that are not valid
 loose objects nor valid packs
++
+size-garbage: disk space consumed by garbage files, in KiB
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 639c9a5..75feee5 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -10,12 +10,15 @@
 #include "parse-options.h"
 
 static unsigned long garbage;
+static off_t size_garbage;
 
 static void real_report_garbage(const char *desc,
 				const char *path, int len,
 				const char *name)
 {
 	struct strbuf sb = STRBUF_INIT;
+	struct stat st;
+
 	if (len && name)
 		strbuf_addf(&sb, "%.*s/%s", len, path, name);
 	else if (!len && name)
@@ -23,6 +26,10 @@ static void real_report_garbage(const char *desc,
 	else
 		strbuf_addf(&sb, "%s", path);
 	warning("%s: %s", desc, sb.buf);
+
+	if (!stat(sb.buf, &st))
+		size_garbage += st.st_size;
+
 	garbage++;
 	strbuf_release(&sb);
 }
@@ -30,8 +37,7 @@ static void real_report_garbage(const char *desc,
 static void count_objects(DIR *d, char *path, int len, int verbose,
 			  unsigned long *loose,
 			  off_t *loose_size,
-			  unsigned long *packed_loose,
-			  unsigned long *garbage)
+			  unsigned long *packed_loose)
 {
 	struct dirent *ent;
 	while ((ent = readdir(d)) != NULL) {
@@ -63,11 +69,9 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 				(*loose_size) += xsize_t(on_disk_bytes(st));
 		}
 		if (bad) {
-			if (verbose) {
-				error("garbage found: %.*s/%s",
-				      len + 2, path, ent->d_name);
-				(*garbage)++;
-			}
+			if (verbose)
+				report_garbage("garbage found",
+					       path, len + 2, ent->d_name);
 			continue;
 		}
 		(*loose)++;
@@ -117,7 +121,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		if (!d)
 			continue;
 		count_objects(d, path, len, verbose,
-			      &loose, &loose_size, &packed_loose, &garbage);
+			      &loose, &loose_size, &packed_loose);
 		closedir(d);
 	}
 	if (verbose) {
@@ -142,6 +146,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		printf("size-pack: %lu\n", (unsigned long) (size_pack / 1024));
 		printf("prune-packable: %lu\n", packed_loose);
 		printf("garbage: %lu\n", garbage);
+		printf("size-garbage: %lu\n", (unsigned long) (size_garbage / 1024));
 	}
 	else
 		printf("%lu objects, %lu kilobytes\n",
-- 
1.8.1.2.536.gf441e6d

Re: [PATCH v3 3/4] count-objects: report garbage files in pack directory too

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> +/* A hook for count-objects to report invalid files in pack directory */
> +extern void (*report_garbage)(const char *desc, const char *path, int len, const char *name);

We may want to document the strange way the last three parameters
are used somewhere.  e.g.

	shows "path" (if "name" is NULL), or prepends "path" in
	front of name (otherwise); only for the latter, "path" can
	be a string that is not NUL-terminated but its length
	specified with "len" and in that case a slash is inserted
	between the path and the "name".

When described clearly, it sounds somewhat ugly and incoherent API,
even though it covers the immediate need X-<.

> +	sort_string_list(list);
> +
> +	for (p = packed_git; p; p = p->next) {
> +		struct string_list_item *item;
> +		if (!p->pack_local)
> +			continue;
> +		strbuf_reset(&sb);
> +		strbuf_add(&sb, p->pack_name,
> +			   strlen(p->pack_name) - 5); /* ".pack" */
> +		item = string_list_lookup(list, sb.buf);
> +		if (!item)
> +			continue;
> +		/*
> +		 * string_list_lookup does not guarantee to return the
> +		 * first matched string if it's duplicated.
> +		 */

Do we need to even allow duplication?  Why does prepare_packed_git_one()
below have to split the pathname into the base and extension in the first
place?

If you collect all pathnames that end with known extensions (".idx",
".pack" and ".keep") in the sorted "garbage" list as separate entries in
prepare_packed_git_one(), report_pack_garbage() can do the right thing
without trusting or iterating over packed_git list at all, I think.  As
the entries are sorted, .pack, .idx and all other valid .ext are grouped
together for the same basename.  If you see a group that have both .pack
and .idx, the group is good.  Otherwise, everybody in the group is bad
(e.g. a lonely .pack file without .idx is an unusable garbage).

How about doing it something along this line, perhaps?

	int i;
	int beginning_of_this_name = -1;
        int seen_bits = 0; /* 01 for .idx, 02 for .pack */
	for (i = 0; i < list->nr; i++) {
        	if (beginning_of_this_name < 0)
                	beginning_of_this_name = i;
		else if (list->items[i] and list->items[beginning_of_this_name]
                	 share the same basename)
			; /* keep scanning */
		else {
                	/* one name ended at (i-1) */
			if (seen_bits == 3)
				; /* both .idx and .pack exist; good */
			else
			        report_garbage_for_one_name(list, beginning_of_this_name, i,
                                		seen_bits);
			seen_bits = 0;
                        beginning_of_this_name = i;
		}
                if (list->items[i] is ".idx")
			seen_bits |= 1;
                if (list->items[i] is ".pack")
			seen_bits |= 2;

	}
	if (0 <= beginning_of_this_name && seen_bits != 3)
	        report_garbages_for_one_name(list, beginning_of_this_name, list->nr, seen_bits);

with a helper function report_garbage_for_one_name() that would look like this:

	report_garbage_for_one_name(...) {
                int j;
                const char *msg;
                switch (seen_bits) {
                case 0: msg = "no corresponding .idx nor .pack"; break;
                case 1: msg = "no corresponding .pack"; break;
                case 2: msg = "no corresponding .idx; break;
                }
                for (j = beginning_of_this_name; j < i; j++)
                        report_garbage(msg, list->items[j]);
	}

For the above to work, prepare_packed_git_one() needs to retain only the
paths with known extensions in garbage list. "pack-deadbeef.unk" can and
should be reported as a garbage immediately when it is seen without being
placed in the list.

> @@ -1045,9 +1100,33 @@ static void prepare_packed_git_one(char *objdir, int local)
>  			if (!p)
>  				continue;
>  			install_packed_git(p);
> -		}
> +		} else if (!report_garbage) {
> +			/*
> +			 * the rest of this if-chain requires
> +			 * report_garbage != NULL. Stop the chain if
> +			 * report_garbage is NULL.
> +			 */
> +			;
> +		} else if (has_extension(de->d_name, ".pack")) {
> +			struct string_list_item *item;
> +			int n = strlen(path) - 5;
> +			item = string_list_append_nodup(&garbage,
> +							 xstrndup(path, n));
> +			item->util = ".pack";
> +			continue;
> +		} else if (has_extension(de->d_name, ".idx")) {
> +			struct string_list_item *item;
> +			int n = strlen(path) - 4;
> +			item = string_list_append_nodup(&garbage,
> +							xstrndup(path, n));
> +			item->util = ".idx";
> +			continue;
> +		} else
> +			report_garbage("garbage found", path, 0, NULL);

Hmm, where is a ".keep" file handled in this flow?

The structure of the if/else cascade is much nicer than the earlier
iterations, but wouldn't it be even more clear to do this?

	if (is .idx file) {
		... do that .idx thing ...
	}

	if (!report_garbage)
        	continue; /* it does not matter what the file is */

	if (is .pack) {
		... remember that we saw this .pack ...
	} else if (is .idx) {
		... remember that we saw this .idx ...
	} else if (is .keep) {
		... remember that we saw this .keep ...
        } else {
        	... all else --- report as garbage immediately ...
	}

[PATCH v4 0/4] count-objects improvements

[Nguyễn Thái Ngọc Duy]

On Wed, Feb 13, 2013 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> +/* A hook for count-objects to report invalid files in pack directory */
>> +extern void (*report_garbage)(const char *desc, const char *path, int len, const char *name);
>
> We may want to document the strange way the last three parameters
> are used somewhere.  e.g.
>
>         shows "path" (if "name" is NULL), or prepends "path" in
>         front of name (otherwise); only for the latter, "path" can
>         be a string that is not NUL-terminated but its length
>         specified with "len" and in that case a slash is inserted
>         between the path and the "name".
>
> When described clearly, it sounds somewhat ugly and incoherent API,
> even though it covers the immediate need X-<.

One of the reasons why I did not export it explicitly. Changed it to

void (*report_garbage)(const char *desc, const char *path);

and pushed the ugly part back to callers.

> How about doing it something along this line, perhaps?
>
>         int i;
>         int beginning_of_this_name = -1;
>         int seen_bits = 0; /* 01 for .idx, 02 for .pack */
>         for (i = 0; i < list->nr; i++) {
>                 if (beginning_of_this_name < 0)
>                         beginning_of_this_name = i;
>                 else if (list->items[i] and list->items[beginning_of_this_name]
>                          share the same basename)
>                         ; /* keep scanning */
>                 else {
>                         /* one name ended at (i-1) */
>                         if (seen_bits == 3)
>                                 ; /* both .idx and .pack exist; good */
>                         else
>                                 report_garbage_for_one_name(list, beginning_of_this_name, i,
>                                                 seen_bits);
>                         seen_bits = 0;
>                         beginning_of_this_name = i;
>                 }
>                 if (list->items[i] is ".idx")
>                         seen_bits |= 1;
>                 if (list->items[i] is ".pack")
>                         seen_bits |= 2;
>
>         }
>         if (0 <= beginning_of_this_name && seen_bits != 3)
>                 report_garbages_for_one_name(list, beginning_of_this_name, list->nr, seen_bits);
>
> with a helper function report_garbage_for_one_name() that would look like this:
>
>         report_garbage_for_one_name(...) {
>                 int j;
>                 const char *msg;
>                 switch (seen_bits) {
>                 case 0: msg = "no corresponding .idx nor .pack"; break;
>                 case 1: msg = "no corresponding .pack"; break;
>                 case 2: msg = "no corresponding .idx; break;
>                 }
>                 for (j = beginning_of_this_name; j < i; j++)
>                         report_garbage(msg, list->items[j]);
>         }
>
> For the above to work, prepare_packed_git_one() needs to retain only the
> paths with known extensions in garbage list. "pack-deadbeef.unk" can and
> should be reported as a garbage immediately when it is seen without being
> placed in the list.

Yup. Looks good.

>> +             } else if (has_extension(de->d_name, ".idx")) {
>> +                     struct string_list_item *item;
>> +                     int n = strlen(path) - 4;
>> +                     item = string_list_append_nodup(&garbage,
>> +                                                     xstrndup(path, n));
>> +                     item->util = ".idx";
>> +                     continue;
>> +             } else
>> +                     report_garbage("garbage found", path, 0, NULL);
>
> Hmm, where is a ".keep" file handled in this flow?

Apparently I smoked/drank while coding or something. .idx is supposed
to be .keep. This calls for a test to guard my code (part of this v4).

> The structure of the if/else cascade is much nicer than the earlier
> iterations, but wouldn't it be even more clear to do this?
>
>         if (is .idx file) {
>                 ... do that .idx thing ...
>         }
>
>         if (!report_garbage)
>                 continue; /* it does not matter what the file is */
>
>         if (is .pack) {
>                 ... remember that we saw this .pack ...
>         } else if (is .idx) {
>                 ... remember that we saw this .idx ...
>         } else if (is .keep) {
>                 ... remember that we saw this .keep ...
>         } else {
>                 ... all else --- report as garbage immediately ...
>         }

Done. 2/4 is updated to make sure the "if (is .idx file)" block does
not shortcut the loop with "continue;" so that we always get .idx
file in the end of the loop.

Nguyễn Thái Ngọc Duy (4):
  git-count-objects.txt: describe each line in -v output
  sha1_file: reorder code in prepare_packed_git_one()
  count-objects: report garbage files in pack directory too
  count-objects: report how much disk space taken by garbage files

 Documentation/git-count-objects.txt |  22 ++++++--
 builtin/count-objects.c             |  30 ++++++++---
 cache.h                             |   3 ++
 sha1_file.c                         | 101 +++++++++++++++++++++++++++++++-----
 t/t5304-prune.sh                    |  26 ++++++++++
 5 files changed, 156 insertions(+), 26 deletions(-)

-- 
1.8.1.2.536.gf441e6d

[PATCH v4 1/4] git-count-objects.txt: describe each line in -v output

[Nguyễn Thái Ngọc Duy]

The current description requires a bit of guessing (what clause
corresponds to what printed line?) and lacks information, such as
the unit of size and size-pack.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 23c80ce..e816823 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -20,11 +20,21 @@ OPTIONS
 -------
 -v::
 --verbose::
-	In addition to the number of loose objects and disk
-	space consumed, it reports the number of in-pack
-	objects, number of packs, disk space consumed by those packs,
-	and number of objects that can be removed by running
-	`git prune-packed`.
+	Report in more detail:
++
+count: the number of loose objects
++
+size: disk space consumed by loose objects, in KiB
++
+in-pack: the number of in-pack objects
++
+size-pack: disk space consumed by the packs, in KiB
++
+prune-packable: the number of loose objects that are also present in
+the packs. These objects could be pruned using `git prune-packed`.
++
+garbage: the number of files in loose object database that are not
+valid loose objects
 
 GIT
 ---
-- 
1.8.1.2.536.gf441e6d

[PATCH v4 2/4] sha1_file: reorder code in prepare_packed_git_one()

[Nguyễn Thái Ngọc Duy]

The current loop does

	while (...) {
		if (!not .idx file)
			continue;
		process .idx file;
	}

and is reordered to

	while (...) {
		if (!.idx file) {
			process .idx file;
		}
	}

This makes it easier to add new extension file processing.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sha1_file.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..239bee7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1024,27 +1024,25 @@ static void prepare_packed_git_one(char *objdir, int local)
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (!has_extension(de->d_name, ".idx"))
-			continue;
-
 		if (len + namelen + 1 > sizeof(path))
 			continue;
 
-		/* Don't reopen a pack we already have. */
 		strcpy(path + len, de->d_name);
-		for (p = packed_git; p; p = p->next) {
-			if (!memcmp(path, p->pack_name, len + namelen - 4))
-				break;
+
+		if (has_extension(de->d_name, ".idx")) {
+			/* Don't reopen a pack we already have. */
+			for (p = packed_git; p; p = p->next) {
+				if (!memcmp(path, p->pack_name, len + namelen - 4))
+					break;
+			}
+			if (p == NULL &&
+			    /*
+			     * See if it really is a valid .idx file with
+			     * corresponding .pack file that we can map.
+			     */
+			    (p = add_packed_git(path, len + namelen, local)) != NULL)
+				install_packed_git(p);
 		}
-		if (p)
-			continue;
-		/* See if it really is a valid .idx file with corresponding
-		 * .pack file that we can map.
-		 */
-		p = add_packed_git(path, len + namelen, local);
-		if (!p)
-			continue;
-		install_packed_git(p);
 	}
 	closedir(dir);
 }
-- 
1.8.1.2.536.gf441e6d

[PATCH v4 3/4] count-objects: report garbage files in pack directory too

[Nguyễn Thái Ngọc Duy]

prepare_packed_git_one() is modified to allow count-objects to hook a
report function to so we don't need to duplicate the pack searching
logic in count-objects.c. When report_pack_garbage is NULL, the
overhead is insignificant.

The garbage is reported with warning() instead of error() in packed
garbage case because it's not an error to have garbage. Loose garbage
is still reported as errors and will be converted to warnings later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  4 +-
 builtin/count-objects.c             | 12 +++++-
 cache.h                             |  3 ++
 sha1_file.c                         | 77 ++++++++++++++++++++++++++++++++++++-
 t/t5304-prune.sh                    | 26 +++++++++++++
 5 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index e816823..1611d7c 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
 +
-garbage: the number of files in loose object database that are not
-valid loose objects
+garbage: the number of files in object database that are not valid
+loose objects nor valid packs
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..1706c8b 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -9,6 +9,14 @@
 #include "builtin.h"
 #include "parse-options.h"
 
+static unsigned long garbage;
+
+static void real_report_garbage(const char *desc, const char *path)
+{
+	warning("%s: %s", desc, path);
+	garbage++;
+}
+
 static void count_objects(DIR *d, char *path, int len, int verbose,
 			  unsigned long *loose,
 			  off_t *loose_size,
@@ -76,7 +84,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
-	unsigned long loose = 0, packed = 0, packed_loose = 0, garbage = 0;
+	unsigned long loose = 0, packed = 0, packed_loose = 0;
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -87,6 +95,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	/* we do not take arguments other than flags for now */
 	if (argc)
 		usage_with_options(count_objects_usage, opts);
+	if (verbose)
+		report_garbage = real_report_garbage;
 	memcpy(path, objdir, len);
 	if (len && objdir[len-1] != '/')
 		path[len++] = '/';
diff --git a/cache.h b/cache.h
index 7339f21..73de68c 100644
--- a/cache.h
+++ b/cache.h
@@ -1051,6 +1051,9 @@ extern const char *parse_feature_value(const char *feature_list, const char *fea
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_garbage)(const char *desc, const char *path);
+
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
 extern void install_packed_git(struct packed_git *pack);
diff --git a/sha1_file.c b/sha1_file.c
index 239bee7..5bedf78 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -21,6 +21,7 @@
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
 #include "streaming.h"
+#include "dir.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1000,6 +1001,57 @@ void install_packed_git(struct packed_git *pack)
 	packed_git = pack;
 }
 
+void (*report_garbage)(const char *desc, const char *path);
+
+static void report_helper(const struct string_list *list,
+			  int seen_bits, int first, int last)
+{
+	const char *msg;
+	switch (seen_bits) {
+	case 0: msg = "no corresponding .idx nor .pack"; break;
+	case 1: msg = "no corresponding .idx"; break;
+	case 2: msg = "no corresponding .pack"; break;
+	default:
+		return;
+	}
+	for (; first <= last; first++)
+		report_garbage(msg, list->items[first].string);
+}
+
+static void report_pack_garbage(struct string_list *list)
+{
+	int i, baselen = -1, first = 0, seen_bits = 0;
+
+	if (!report_garbage)
+		return;
+
+	sort_string_list(list);
+
+	for (i = 0; i < list->nr; i++) {
+		const char *path = list->items[i].string;
+		if (baselen != -1 &&
+		    strncmp(path, list->items[first].string, baselen)) {
+			report_helper(list, seen_bits, first, i - 1);
+			baselen = -1;
+			seen_bits = 0;
+		}
+		if (baselen == -1) {
+			const char *dot = strrchr(path, '.');
+			if (!dot) {
+				report_garbage("garbage found", path);
+				continue;
+			}
+			baselen = dot - path + 1;
+			first = i;
+		}
+		if (!strcmp(path + baselen, "pack"))
+			seen_bits |= 1;
+		else if (!strcmp(path + baselen, "idx"))
+			seen_bits |= 2;
+	}
+	report_helper(list, seen_bits, first, list->nr - 1);
+}
+
 static void prepare_packed_git_one(char *objdir, int local)
 {
 	/* Ensure that this buffer is large enough so that we can
@@ -1009,6 +1061,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 	int len;
 	DIR *dir;
 	struct dirent *de;
+	struct string_list garbage = STRING_LIST_INIT_DUP;
 
 	sprintf(path, "%s/pack", objdir);
 	len = strlen(path);
@@ -1024,7 +1077,17 @@ static void prepare_packed_git_one(char *objdir, int local)
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (len + namelen + 1 > sizeof(path))
+		if (len + namelen + 1 > sizeof(path)) {
+			if (report_garbage) {
+				struct strbuf sb = STRBUF_INIT;
+				strbuf_addf(&sb, "%.*s/%s", len - 1, path, de->d_name);
+				report_garbage("path too long", sb.buf);
+				strbuf_release(&sb);
+			}
+			continue;
+		}
+
+		if (is_dot_or_dotdot(de->d_name))
 			continue;
 
 		strcpy(path + len, de->d_name);
@@ -1043,8 +1106,20 @@ static void prepare_packed_git_one(char *objdir, int local)
 			    (p = add_packed_git(path, len + namelen, local)) != NULL)
 				install_packed_git(p);
 		}
+
+		if (!report_garbage)
+			continue;
+
+		if (has_extension(de->d_name, ".idx") ||
+		    has_extension(de->d_name, ".pack") ||
+		    has_extension(de->d_name, ".keep"))
+			string_list_append(&garbage, path);
+		else
+			report_garbage("garbage found", path);
 	}
 	closedir(dir);
+	report_pack_garbage(&garbage);
+	string_list_clear(&garbage, 0);
 }
 
 static int sort_pack(const void *a_, const void *b_)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index d645328..e4bb3a1 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -195,4 +195,30 @@ test_expect_success 'gc: prune old objects after local clone' '
 	)
 '
 
+test_expect_success 'garbage report in count-objects -v' '
+	: >.git/objects/pack/foo &&
+	: >.git/objects/pack/foo.bar &&
+	: >.git/objects/pack/foo.keep &&
+	: >.git/objects/pack/foo.pack &&
+	: >.git/objects/pack/fake.bar &&
+	: >.git/objects/pack/fake.keep &&
+	: >.git/objects/pack/fake.pack &&
+	: >.git/objects/pack/fake.idx &&
+	: >.git/objects/pack/fake2.keep &&
+	: >.git/objects/pack/fake3.idx &&
+	git count-objects -v 2>stderr &&
+	grep "index file .git/objects/pack/fake.idx is too small" stderr &&
+	grep "^warning:" stderr | sort >actual &&
+	cat >expected <<\EOF &&
+warning: garbage found: .git/objects/pack/fake.bar
+warning: garbage found: .git/objects/pack/foo
+warning: garbage found: .git/objects/pack/foo.bar
+warning: no corresponding .idx nor .pack: .git/objects/pack/fake2.keep
+warning: no corresponding .idx: .git/objects/pack/foo.keep
+warning: no corresponding .idx: .git/objects/pack/foo.pack
+warning: no corresponding .pack: .git/objects/pack/fake3.idx
+EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.1.2.536.gf441e6d

[PATCH v4 4/4] count-objects: report how much disk space taken by garbage files

[Nguyễn Thái Ngọc Duy]

Also issue warnings on loose garbages instead of errors as a result of
using report_garbage() function in count_objects()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  2 ++
 builtin/count-objects.c             | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 1611d7c..da6e72e 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -35,6 +35,8 @@ the packs. These objects could be pruned using `git prune-packed`.
 +
 garbage: the number of files in object database that are not valid
 loose objects nor valid packs
++
+size-garbage: disk space consumed by garbage files, in KiB
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 1706c8b..3a01a8d 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -10,9 +10,13 @@
 #include "parse-options.h"
 
 static unsigned long garbage;
+static off_t size_garbage;
 
 static void real_report_garbage(const char *desc, const char *path)
 {
+	struct stat st;
+	if (!stat(path, &st))
+		size_garbage += st.st_size;
 	warning("%s: %s", desc, path);
 	garbage++;
 }
@@ -20,8 +24,7 @@ static void real_report_garbage(const char *desc, const char *path)
 static void count_objects(DIR *d, char *path, int len, int verbose,
 			  unsigned long *loose,
 			  off_t *loose_size,
-			  unsigned long *packed_loose,
-			  unsigned long *garbage)
+			  unsigned long *packed_loose)
 {
 	struct dirent *ent;
 	while ((ent = readdir(d)) != NULL) {
@@ -54,9 +57,11 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 		}
 		if (bad) {
 			if (verbose) {
-				error("garbage found: %.*s/%s",
-				      len + 2, path, ent->d_name);
-				(*garbage)++;
+				struct strbuf sb = STRBUF_INIT;
+				strbuf_addf(&sb, "%.*s/%s",
+					    len + 2, path, ent->d_name);
+				report_garbage("garbage found", sb.buf);
+				strbuf_release(&sb);
 			}
 			continue;
 		}
@@ -107,7 +112,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		if (!d)
 			continue;
 		count_objects(d, path, len, verbose,
-			      &loose, &loose_size, &packed_loose, &garbage);
+			      &loose, &loose_size, &packed_loose);
 		closedir(d);
 	}
 	if (verbose) {
@@ -132,6 +137,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		printf("size-pack: %lu\n", (unsigned long) (size_pack / 1024));
 		printf("prune-packable: %lu\n", packed_loose);
 		printf("garbage: %lu\n", garbage);
+		printf("size-garbage: %lu\n", (unsigned long) (size_garbage / 1024));
 	}
 	else
 		printf("%lu objects, %lu kilobytes\n",
-- 
1.8.1.2.536.gf441e6d

Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> prepare_packed_git_one() is modified to allow count-objects to hook a
> report function to so we don't need to duplicate the pack searching
> logic in count-objects.c. When report_pack_garbage is NULL, the
> overhead is insignificant.
>
> The garbage is reported with warning() instead of error() in packed
> garbage case because it's not an error to have garbage. Loose garbage
> is still reported as errors and will be converted to warnings later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks.

Tests look good and the series is getting much closer.

> diff --git a/sha1_file.c b/sha1_file.c
> index 239bee7..5bedf78 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -21,6 +21,7 @@
>  #include "sha1-lookup.h"
>  #include "bulk-checkin.h"
>  #include "streaming.h"
> +#include "dir.h"
>  
>  #ifndef O_NOATIME
>  #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
> @@ -1000,6 +1001,57 @@ void install_packed_git(struct packed_git *pack)
>  	packed_git = pack;
>  }
>  
> +void (*report_garbage)(const char *desc, const char *path);
> +
> +static void report_helper(const struct string_list *list,
> +			  int seen_bits, int first, int last)
> +{
> +	const char *msg;
> +	switch (seen_bits) {
> +	case 0: msg = "no corresponding .idx nor .pack"; break;
> +	case 1: msg = "no corresponding .idx"; break;
> +	case 2: msg = "no corresponding .pack"; break;

That's dense.

> +	default:
> +		return;
> +	}
> +	for (; first <= last; first++)

This looks odd.  If you use the usual last+1 convention between the
caller and callee, you do not have to do this, or call this function
with "i - 1" and "list->nr -1" as the last parameter.

> +static void report_pack_garbage(struct string_list *list)
> +{
> +	int i, baselen = -1, first = 0, seen_bits = 0;
> +
> +	if (!report_garbage)
> +		return;
> +
> +	sort_string_list(list);
> +
> +	for (i = 0; i < list->nr; i++) {
> +		const char *path = list->items[i].string;
> +		if (baselen != -1 &&
> +		    strncmp(path, list->items[first].string, baselen)) {
> +			report_helper(list, seen_bits, first, i - 1);
> +			baselen = -1;
> +			seen_bits = 0;
> +		}
> +		if (baselen == -1) {
> +			const char *dot = strrchr(path, '.');
> +			if (!dot) {
> +				report_garbage("garbage found", path);
> +				continue;
> +			}
> +			baselen = dot - path + 1;
> +			first = i;
> +		}
> +		if (!strcmp(path + baselen, "pack"))
> +			seen_bits |= 1;
> +		else if (!strcmp(path + baselen, "idx"))
> +			seen_bits |= 2;
> +	}
> +	report_helper(list, seen_bits, first, list->nr - 1);
> +}

> @@ -1009,6 +1061,7 @@ static void prepare_packed_git_one(char *objdir, int local)
>  	int len;
>  	DIR *dir;
>  	struct dirent *de;
> +	struct string_list garbage = STRING_LIST_INIT_DUP;
>  
>  	sprintf(path, "%s/pack", objdir);
>  	len = strlen(path);
> ...
> @@ -1043,8 +1106,20 @@ static void prepare_packed_git_one(char *objdir, int local)
>  			    (p = add_packed_git(path, len + namelen, local)) != NULL)
>  				install_packed_git(p);
>  		}
> +
> +		if (!report_garbage)
> +			continue;
> +
> +		if (has_extension(de->d_name, ".idx") ||
> +		    has_extension(de->d_name, ".pack") ||
> +		    has_extension(de->d_name, ".keep"))
> +			string_list_append(&garbage, path);

It might be OK to put .pack and .keep in the same "if (A || B)" as
it may happen to be that they do not need any special treatment
right now, but I do not think this is a good idea in general.

You would want to do things differently for ".idx", e.g.

diff --git a/sha1_file.c b/sha1_file.c
index 5bedf78..450521f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1076,6 +1076,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 	while ((de = readdir(dir)) != NULL) {
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
+		int is_a_bad_idx = 0;
 
 		if (len + namelen + 1 > sizeof(path)) {
 			if (report_garbage) {
@@ -1105,12 +1106,14 @@ static void prepare_packed_git_one(char *objdir, int local)
 			     */
 			    (p = add_packed_git(path, len + namelen, local)) != NULL)
 				install_packed_git(p);
+			else
+				is_a_bad_idx = 1;
 		}
 
 		if (!report_garbage)
 			continue;
 
-		if (has_extension(de->d_name, ".idx") ||
+		if ((has_extension(de->d_name, ".idx") && !is_a_bad_idx) ||
 		    has_extension(de->d_name, ".pack") ||
 		    has_extension(de->d_name, ".keep"))
 			string_list_append(&garbage, path);


so that you can say something about .pack/.keep files that do not
have a working .idx file.  In the above example, the only special
thing you would do for .idx is just to check if it is a bad one, but
in later patches you may have to do different things in the body
(i.e. something else in addition to string_list_append(&garbage))
not just in the condition.  Collapsing these into a condition to a
single "if (A||B||C)" may be suffering from a lack of foresight.

> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index d645328..e4bb3a1 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -195,4 +195,30 @@ test_expect_success 'gc: prune old objects after local clone' '
>  	)
>  '
>  
> +test_expect_success 'garbage report in count-objects -v' '
> +	: >.git/objects/pack/foo &&
> +	: >.git/objects/pack/foo.bar &&
> +	: >.git/objects/pack/foo.keep &&
> +	: >.git/objects/pack/foo.pack &&
> +	: >.git/objects/pack/fake.bar &&
> +	: >.git/objects/pack/fake.keep &&
> +	: >.git/objects/pack/fake.pack &&
> +	: >.git/objects/pack/fake.idx &&
> +	: >.git/objects/pack/fake2.keep &&
> +	: >.git/objects/pack/fake3.idx &&
> +	git count-objects -v 2>stderr &&
> +	grep "index file .git/objects/pack/fake.idx is too small" stderr &&

The above suggested change will make a difference to
fake.{pack,keep} because of this breakage, I think.

> +	grep "^warning:" stderr | sort >actual &&
> +	cat >expected <<\EOF &&
> +warning: garbage found: .git/objects/pack/fake.bar
> +warning: garbage found: .git/objects/pack/foo
> +warning: garbage found: .git/objects/pack/foo.bar
> +warning: no corresponding .idx nor .pack: .git/objects/pack/fake2.keep
> +warning: no corresponding .idx: .git/objects/pack/foo.keep
> +warning: no corresponding .idx: .git/objects/pack/foo.pack
> +warning: no corresponding .pack: .git/objects/pack/fake3.idx
> +EOF
> +	test_cmp expected actual
> +'
> +
>  test_done

Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +		if (has_extension(de->d_name, ".idx") ||
>> +		    has_extension(de->d_name, ".pack") ||
>> +		    has_extension(de->d_name, ".keep"))
>> +			string_list_append(&garbage, path);
>
> It might be OK to put .pack and .keep in the same "if (A || B)" as
> it may happen to be that they do not need any special treatment
> right now, but I do not think this is a good idea in general.

Actually I take this part back.  I can see that the condition part
grow over time but I do not think the body should.  That is the
whole point of collecting paths that cannot be judged as garbage by
themselves into a list; we shouldn't be doing anything else by
definition in the body.

Everything else I said in the review still stands, though..

Thanks.

Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too

[Duy Nguyen]

On Wed, Feb 13, 2013 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     default:
>> +             return;
>> +     }
>> +     for (; first <= last; first++)
>
> This looks odd.  If you use the usual last+1 convention between the
> caller and callee, you do not have to do this, or call this function
> with "i - 1" and "list->nr -1" as the last parameter.

I know. I just don't know how to name the variable to say "the element
after the last one".
-- 
Duy

Re: [PATCH v4 3/4] count-objects: report garbage files in pack directory too

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Feb 13, 2013 at 10:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +     default:
>>> +             return;
>>> +     }
>>> +     for (; first <= last; first++)
>>
>> This looks odd.  If you use the usual last+1 convention between the
>> caller and callee, you do not have to do this, or call this function
>> with "i - 1" and "list->nr -1" as the last parameter.
>
> I know. I just don't know how to name the variable to say "the element
> after the last one".

In case it was unclear, by the "usual last+1 convention", I meant
that it is perfectly normal to write

	for (i = first; i < last; i++)
	for (i = begin; i < end; i++)

in C.  Saying these as

	for (i = first; i < beyond_last; i++)
	for (i = begin; i < beyond_end; i++)

look non-C.

[PATCH v4+ 3/4] count-objects: report garbage files in pack directory too

[Nguyễn Thái Ngọc Duy]

prepare_packed_git_one() is modified to allow count-objects to hook a
report function to so we don't need to duplicate the pack searching
logic in count-objects.c. When report_pack_garbage is NULL, the
overhead is insignificant.

The garbage is reported with warning() instead of error() in packed
garbage case because it's not an error to have garbage. Loose garbage
is still reported as errors and will be converted to warnings later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-count-objects.txt |  4 +-
 builtin/count-objects.c             | 12 +++++-
 cache.h                             |  3 ++
 sha1_file.c                         | 83 ++++++++++++++++++++++++++++++++++++-
 t/t5304-prune.sh                    | 26 ++++++++++++
 5 files changed, 124 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index e816823..1611d7c 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -33,8 +33,8 @@ size-pack: disk space consumed by the packs, in KiB
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
 +
-garbage: the number of files in loose object database that are not
-valid loose objects
+garbage: the number of files in object database that are not valid
+loose objects nor valid packs
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..1706c8b 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -9,6 +9,14 @@
 #include "builtin.h"
 #include "parse-options.h"
 
+static unsigned long garbage;
+
+static void real_report_garbage(const char *desc, const char *path)
+{
+	warning("%s: %s", desc, path);
+	garbage++;
+}
+
 static void count_objects(DIR *d, char *path, int len, int verbose,
 			  unsigned long *loose,
 			  off_t *loose_size,
@@ -76,7 +84,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
-	unsigned long loose = 0, packed = 0, packed_loose = 0, garbage = 0;
+	unsigned long loose = 0, packed = 0, packed_loose = 0;
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -87,6 +95,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	/* we do not take arguments other than flags for now */
 	if (argc)
 		usage_with_options(count_objects_usage, opts);
+	if (verbose)
+		report_garbage = real_report_garbage;
 	memcpy(path, objdir, len);
 	if (len && objdir[len-1] != '/')
 		path[len++] = '/';
diff --git a/cache.h b/cache.h
index 7339f21..73de68c 100644
--- a/cache.h
+++ b/cache.h
@@ -1051,6 +1051,9 @@ extern const char *parse_feature_value(const char *feature_list, const char *fea
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_garbage)(const char *desc, const char *path);
+
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
 extern void install_packed_git(struct packed_git *pack);
diff --git a/sha1_file.c b/sha1_file.c
index 239bee7..16967d3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -21,6 +21,7 @@
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
 #include "streaming.h"
+#include "dir.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1000,6 +1001,63 @@ void install_packed_git(struct packed_git *pack)
 	packed_git = pack;
 }
 
+void (*report_garbage)(const char *desc, const char *path);
+
+static void report_helper(const struct string_list *list,
+			  int seen_bits, int first, int last)
+{
+	const char *msg;
+	switch (seen_bits) {
+	case 0:
+		msg = "no corresponding .idx nor .pack";
+		break;
+	case 1:
+		msg = "no corresponding .idx";
+		break;
+	case 2:
+		msg = "no corresponding .pack";
+		break;
+	default:
+		return;
+	}
+	for (; first < last; first++)
+		report_garbage(msg, list->items[first].string);
+}
+
+static void report_pack_garbage(struct string_list *list)
+{
+	int i, baselen = -1, first = 0, seen_bits = 0;
+
+	if (!report_garbage)
+		return;
+
+	sort_string_list(list);
+
+	for (i = 0; i < list->nr; i++) {
+		const char *path = list->items[i].string;
+		if (baselen != -1 &&
+		    strncmp(path, list->items[first].string, baselen)) {
+			report_helper(list, seen_bits, first, i);
+			baselen = -1;
+			seen_bits = 0;
+		}
+		if (baselen == -1) {
+			const char *dot = strrchr(path, '.');
+			if (!dot) {
+				report_garbage("garbage found", path);
+				continue;
+			}
+			baselen = dot - path + 1;
+			first = i;
+		}
+		if (!strcmp(path + baselen, "pack"))
+			seen_bits |= 1;
+		else if (!strcmp(path + baselen, "idx"))
+			seen_bits |= 2;
+	}
+	report_helper(list, seen_bits, first, list->nr);
+}
+
 static void prepare_packed_git_one(char *objdir, int local)
 {
 	/* Ensure that this buffer is large enough so that we can
@@ -1009,6 +1067,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 	int len;
 	DIR *dir;
 	struct dirent *de;
+	struct string_list garbage = STRING_LIST_INIT_DUP;
 
 	sprintf(path, "%s/pack", objdir);
 	len = strlen(path);
@@ -1024,7 +1083,17 @@ static void prepare_packed_git_one(char *objdir, int local)
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (len + namelen + 1 > sizeof(path))
+		if (len + namelen + 1 > sizeof(path)) {
+			if (report_garbage) {
+				struct strbuf sb = STRBUF_INIT;
+				strbuf_addf(&sb, "%.*s/%s", len - 1, path, de->d_name);
+				report_garbage("path too long", sb.buf);
+				strbuf_release(&sb);
+			}
+			continue;
+		}
+
+		if (is_dot_or_dotdot(de->d_name))
 			continue;
 
 		strcpy(path + len, de->d_name);
@@ -1043,8 +1112,20 @@ static void prepare_packed_git_one(char *objdir, int local)
 			    (p = add_packed_git(path, len + namelen, local)) != NULL)
 				install_packed_git(p);
 		}
+
+		if (!report_garbage)
+			continue;
+
+		if (has_extension(de->d_name, ".idx") ||
+		    has_extension(de->d_name, ".pack") ||
+		    has_extension(de->d_name, ".keep"))
+			string_list_append(&garbage, path);
+		else
+			report_garbage("garbage found", path);
 	}
 	closedir(dir);
+	report_pack_garbage(&garbage);
+	string_list_clear(&garbage, 0);
 }
 
 static int sort_pack(const void *a_, const void *b_)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index d645328..e4bb3a1 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -195,4 +195,30 @@ test_expect_success 'gc: prune old objects after local clone' '
 	)
 '
 
+test_expect_success 'garbage report in count-objects -v' '
+	: >.git/objects/pack/foo &&
+	: >.git/objects/pack/foo.bar &&
+	: >.git/objects/pack/foo.keep &&
+	: >.git/objects/pack/foo.pack &&
+	: >.git/objects/pack/fake.bar &&
+	: >.git/objects/pack/fake.keep &&
+	: >.git/objects/pack/fake.pack &&
+	: >.git/objects/pack/fake.idx &&
+	: >.git/objects/pack/fake2.keep &&
+	: >.git/objects/pack/fake3.idx &&
+	git count-objects -v 2>stderr &&
+	grep "index file .git/objects/pack/fake.idx is too small" stderr &&
+	grep "^warning:" stderr | sort >actual &&
+	cat >expected <<\EOF &&
+warning: garbage found: .git/objects/pack/fake.bar
+warning: garbage found: .git/objects/pack/foo
+warning: garbage found: .git/objects/pack/foo.bar
+warning: no corresponding .idx nor .pack: .git/objects/pack/fake2.keep
+warning: no corresponding .idx: .git/objects/pack/foo.keep
+warning: no corresponding .idx: .git/objects/pack/foo.pack
+warning: no corresponding .pack: .git/objects/pack/fake3.idx
+EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.1.2.536.gf441e6d

Re: [PATCH v4+ 3/4] count-objects: report garbage files in pack directory too

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> prepare_packed_git_one() is modified to allow count-objects to hook a
> report function to so we don't need to duplicate the pack searching
> logic in count-objects.c. When report_pack_garbage is NULL, the
> overhead is insignificant.
>
> The garbage is reported with warning() instead of error() in packed
> garbage case because it's not an error to have garbage. Loose garbage
> is still reported as errors and will be converted to warnings later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Will replace the one from the other day and advance the topic to 'next'.

Thanks.