Re: Really slow 'git gc'

11 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: Really slow 'git gc'

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:14

Linus Torvalds [off-list ref] writes:
The real reason _seems_ to be the "--unpacked=pack-....pack" arguments. I 
literally had 232 pack-files, and it looks like a lot of the time was 
spent in that silly loop oer 'ignore_packed' in find_pack_entry(), when 
revision.c does that "has_sha1_pack()" thing. You get a O(n**2) effect in 
number of pack-files: for each commit we look over every pack-file, and 
for every pack-file we look at, we look over each ignore_pack entry.
I think we can add a single bit to "struct packed_git" and in the middle
of setup_revisions() perform the O(N**2) once, so that find_pack_entry()
can check the bit without looping.

Re: Really slow 'git gc'

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:46:14


On Thu, 19 Feb 2009, Junio C Hamano wrote:
I think we can add a single bit to "struct packed_git" and in the middle
of setup_revisions() perform the O(N**2) once, so that find_pack_entry()
can check the bit without looping.
Yes. However, most users call find_pack_entry() with a NULL ignore_packed, 
so we'd still have to pass in that flag to say whether we should look at 
the bit or not.

However, the real issue (?) I think is that the whole "ignore_packed" 
logic is crazy. It's the wrong way around. The whole thing is broken.

Rather than marking which ones are "unpacked" and should be ignored, it 
should just look at the ones to keep. That's how the filesystem layout 
works and that's what "git repack" does anyway.

So I think we should just remove the whole "--unpacked=" thing, and 
instead replace it with a "--keep" flag - and then only finding things in 
the keep packs if we have a list of them.

That would (a) make the logic a whole lot easier to follow and (b) get rid 
of the scalability issue, since you're not really supposed to have more 
than one or two .keep files anyway (if that).

Nobody uses "--unpacked=xyzzy" by hand anyway. The only thing that 
generates those things is git-repack.sh, so this is not a compatibility 
issue, I suspect.

			Linus

[PATCH 6/6] is_kept_pack(): final clean-up

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

Now is_kept_pack() is just a member lookup into a structure, we can write
it as such.

Also rewrite the sole caller of has_sha1_kept_pack() to switch on the
criteria the callee uses (namely, revs->kept_pack_only) between calling
has_sha1_kept_pack() and has_sha1_pack(), so that these two callees do not
have to take a pointer to struct rev_info as an argument.

This removes the header file dependency issue temporarily introduced by
the earlier commit, so we revert changes associated to that as well.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-pack-objects.c |    4 ++--
 cache.h                |    1 +
 revision.c             |    4 +++-
 revision.h             |    3 ---
 sha1_file.c            |   22 +++++++---------------
 5 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 150258b..b2e4626 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1915,7 +1915,7 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)
 		const unsigned char *sha1;
 		struct object *o;
 
-		if (is_kept_pack(p))
+		if (p->pack_keep)
 			continue;
 		if (open_pack_index(p))
 			die("cannot open pack index");
@@ -1951,7 +1951,7 @@ static void loosen_unused_packed_objects(struct rev_info *revs)
 	const unsigned char *sha1;
 
 	for (p = packed_git; p; p = p->next) {
-		if (is_kept_pack(p))
+		if (p->pack_keep)
 			continue;
 
 		if (open_pack_index(p))
diff --git a/cache.h b/cache.h
index 23c16d0..0a3d523 100644
--- a/cache.h
+++ b/cache.h
@@ -566,6 +566,7 @@ extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned l
 extern int move_temp_to_file(const char *tmpfile, const char *filename);
 
 extern int has_sha1_pack(const unsigned char *sha1);
+extern int has_sha1_kept_pack(const unsigned char *sha1);
 extern int has_sha1_file(const unsigned char *sha1);
 extern int has_loose_object_nonlocal(const unsigned char *sha1);
 
diff --git a/revision.c b/revision.c
index 3cfd653..6d8ac46 100644
--- a/revision.c
+++ b/revision.c
@@ -1476,7 +1476,9 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 	if (commit->object.flags & SHOWN)
 		return commit_ignore;
 	if (revs->unpacked &&
-	    has_sha1_kept_pack(commit->object.sha1, revs))
+	    (revs->kept_pack_only
+	     ? has_sha1_kept_pack(commit->object.sha1)
+	     : has_sha1_pack(commit->object.sha1)))
 		return commit_ignore;
 	if (revs->show_all)
 		return commit_show;
diff --git a/revision.h b/revision.h
index f63596f..b9fa9c2 100644
--- a/revision.h
+++ b/revision.h
@@ -156,7 +156,4 @@ enum commit_action {
 
 extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
 
-extern int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *);
-extern int is_kept_pack(const struct packed_git *);
-
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index e8a9517..7ead56c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -16,8 +16,6 @@
 #include "refs.h"
 #include "pack-revindex.h"
 #include "sha1-lookup.h"
-#include "diff.h"
-#include "revision.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1858,13 +1856,8 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	return 0;
 }
 
-int is_kept_pack(const struct packed_git *p)
-{
-	return p->pack_keep;
-}
-
 static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
-			 const struct rev_info *revs)
+			 int kept_pack_only)
 {
 	static struct packed_git *last_found = (void *)1;
 	struct packed_git *p;
@@ -1876,7 +1869,7 @@ static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 	p = (last_found == (void *)1) ? packed_git : last_found;
 
 	do {
-		if (revs->kept_pack_only && !is_kept_pack(p))
+		if (kept_pack_only && !p->pack_keep)
 			goto next;
 		if (p->num_bad_objects) {
 			unsigned i;
@@ -1919,13 +1912,12 @@ static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 
 static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 {
-	return find_pack_ent(sha1, e, NULL);
+	return find_pack_ent(sha1, e, 0);
 }
 
-static int find_kept_pack_entry(const unsigned char *sha1, struct pack_entry *e,
-				const struct rev_info *revs)
+static int find_kept_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 {
-	return find_pack_ent(sha1, e, revs);
+	return find_pack_ent(sha1, e, 1);
 }
 
 struct packed_git *find_sha1_pack(const unsigned char *sha1,
@@ -2395,10 +2387,10 @@ int has_sha1_pack(const unsigned char *sha1)
 	return find_pack_entry(sha1, &e);
 }
 
-int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *revs)
+int has_sha1_kept_pack(const unsigned char *sha1)
 {
 	struct pack_entry e;
-	return find_kept_pack_entry(sha1, &e, revs);
+	return find_kept_pack_entry(sha1, &e);
 }
 
 int has_sha1_file(const unsigned char *sha1)
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 5/6] Simplify is_kept_pack()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

This removes --unpacked=<packfile> parameter from the revision parser, and
rewrites its use in git-repack to pass a single --kept-pack-only option
instead.

The new --kept-pack-only option means just that.  When this option is
given, is_kept_pack() that used to say "not on the --unpacked=<packfile>
list" now says "the packfile has corresponding .keep file".

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-pack-objects.c |    6 +++---
 git-repack.sh          |    5 ++++-
 revision.c             |   20 +++++---------------
 revision.h             |    8 +++-----
 sha1_file.c            |   30 +++---------------------------
 5 files changed, 18 insertions(+), 51 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7e7719b..150258b 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1915,7 +1915,7 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)
 		const unsigned char *sha1;
 		struct object *o;
 
-		if (is_kept_pack(p, revs))
+		if (is_kept_pack(p))
 			continue;
 		if (open_pack_index(p))
 			die("cannot open pack index");
@@ -1951,7 +1951,7 @@ static void loosen_unused_packed_objects(struct rev_info *revs)
 	const unsigned char *sha1;
 
 	for (p = packed_git; p; p = p->next) {
-		if (is_kept_pack(p, revs))
+		if (is_kept_pack(p))
 			continue;
 
 		if (open_pack_index(p))
@@ -2149,7 +2149,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp("--unpacked", arg) ||
-		    !prefixcmp(arg, "--unpacked=") ||
+		    !strcmp("--kept-pack-only", arg) ||
 		    !strcmp("--reflog", arg) ||
 		    !strcmp("--all", arg)) {
 			use_internal_rev_list = 1;
diff --git a/git-repack.sh b/git-repack.sh
index 8600015..a736009 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -68,10 +68,13 @@ case ",$all_into_one," in
 			if [ -e "$PACKDIR/$e.keep" ]; then
 				: keep
 			else
-				args="$args --unpacked=$e.pack"
 				existing="$existing $e"
 			fi
 		done
+		if test -n "$existing"
+		then
+			args="--kept-pack-only"
+		fi
 		if test -n "$args" -a -n "$unpack_unreachable" -a \
 			-n "$remove_redundant"
 		then
diff --git a/revision.c b/revision.c
index 795e0c0..3cfd653 100644
--- a/revision.c
+++ b/revision.c
@@ -963,16 +963,6 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
-static void add_ignore_packed(struct rev_info *revs, const char *name)
-{
-	int num = ++revs->num_ignore_packed;
-
-	revs->ignore_packed = xrealloc(revs->ignore_packed,
-				       sizeof(const char *) * (num + 1));
-	revs->ignore_packed[num-1] = name;
-	revs->ignore_packed[num] = NULL;
-}
-
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
 			       int *unkc, const char **unkv)
 {
@@ -1072,12 +1062,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->edge_hint = 1;
 	} else if (!strcmp(arg, "--unpacked")) {
 		revs->unpacked = 1;
-		free(revs->ignore_packed);
-		revs->ignore_packed = NULL;
-		revs->num_ignore_packed = 0;
-	} else if (!prefixcmp(arg, "--unpacked=")) {
+		revs->kept_pack_only = 0;
+	} else if (!strcmp(arg, "--kept-pack-only")) {
 		revs->unpacked = 1;
-		add_ignore_packed(revs, arg+11);
+		revs->kept_pack_only = 1;
+	} else if (!prefixcmp(arg, "--unpacked=")) {
+		die("--unpacked=<packfile> no longer supported.");
 	} else if (!strcmp(arg, "-r")) {
 		revs->diff = 1;
 		DIFF_OPT_SET(&revs->diffopt, RECURSIVE);
diff --git a/revision.h b/revision.h
index af414e5..f63596f 100644
--- a/revision.h
+++ b/revision.h
@@ -47,7 +47,8 @@ struct rev_info {
 			blob_objects:1,
 			edge_hint:1,
 			limited:1,
-			unpacked:1, /* see also ignore_packed below */
+			unpacked:1,
+			kept_pack_only:1,
 			boundary:2,
 			left_right:1,
 			rewrite_parents:1,
@@ -75,9 +76,6 @@ struct rev_info {
 			missing_newline:1;
 	enum date_mode date_mode;
 
-	const char **ignore_packed; /* pretend objects in these are unpacked */
-	int num_ignore_packed;
-
 	unsigned int	abbrev;
 	enum cmit_fmt	commit_format;
 	struct log_info *loginfo;
@@ -159,6 +157,6 @@ enum commit_action {
 extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
 
 extern int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *);
-extern int is_kept_pack(const struct packed_git *, const struct rev_info *);
+extern int is_kept_pack(const struct packed_git *);
 
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index 6e0a462..e8a9517 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1858,33 +1858,9 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	return 0;
 }
 
-static int matches_pack_name(const struct packed_git *p, const char *name)
+int is_kept_pack(const struct packed_git *p)
 {
-	const char *last_c, *c;
-
-	if (!strcmp(p->pack_name, name))
-		return 1;
-
-	for (c = p->pack_name, last_c = c; *c;)
-		if (*c == '/')
-			last_c = ++c;
-		else
-			++c;
-	if (!strcmp(last_c, name))
-		return 1;
-
-	return 0;
-}
-
-int is_kept_pack(const struct packed_git *p, const struct rev_info *revs)
-{
-	int i;
-
-	for (i = 0; i < revs->num_ignore_packed; i++) {
-		if (matches_pack_name(p, revs->ignore_packed[i]))
-			return 0;
-	}
-	return 1;
+	return p->pack_keep;
 }
 
 static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
@@ -1900,7 +1876,7 @@ static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 	p = (last_found == (void *)1) ? packed_git : last_found;
 
 	do {
-		if (revs->ignore_packed && !is_kept_pack(p, revs))
+		if (revs->kept_pack_only && !is_kept_pack(p))
 			goto next;
 		if (p->num_bad_objects) {
 			unsigned i;
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 0/6] "git repack -a -d" improvements

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

This is series removes the linear search in "ignore_packed" list when "git
repack -a -d" is run to consolidate many packfiles that are not marked to
be kept with ".keep".  It is oversplit for easier review; in the final
form, I think it should be two patch series, 1/6 and the rest.

The current mechanism is to pass the name of packfiles that are not marked
to be kept with --unpacked= command line option.  This list is used to a
few internal functions to pretend as if the objects found in named packs
exist in loose form and subject to repacking.  This look-up is linear and
found to be very inefficient.

[1/6] is a preparatory and an unrelated bugfix to git-repack script.

[2/6] refactors public interface has_sha1_pack() that takes an optional
"ignore_packed" list.  Most callers pass NULL, so it introduces a new
function has_sha1_kept_pack() and migrate the minority caller to this
interface while losing the argument from the original function and callers
that currently pass NULL.

[3/6] temporarily makes the the extra argument to has_sha1_kept_pack()
function to pass "ignore_packed" list from a list of char* to a pointer to
struct rev_info, solely to make the refactoring done in [4/6] easier to
follow.  Most of the effects of this patch will be removed at the end.

[4/6] identifies three places that use "ignore_packed" list to tell if a
pack is on the list or not, and introduces a helper function to do so.
The helper is conveniently called is_kept_pack(), even though at this
stage the list does not necessarily mean a list of "unkept" packs yet.

[5/6] removes --unpacked=<packfile> parameter, and adds --kept-pack-only
option.  The sole user of --unpacked=<packfile>, git-repack, is updated to
pass this option instead of listing the "unkept" packfiles on the command
line.

[6/6] reverts most of the effects of [3/6] made solely for reviewability
and removes is_kept_pack() helper function, which now is merely a lookup
of a structure member "p->pack_keep".

I think we probably could get rid of --honor-pack-keep mechanism after
this series, but I didn't look very deeply into it.

Junio C Hamano (6):
  git-repack: resist stray environment variable
  has_sha1_pack(): refactor "pretend these packs do not exist"
    interface
  has_sha1_kept_pack(): take "struct rev_info"
  Consolidate ignore_packed logic more
  Simplify is_kept_pack()
  is_kept_pack(): final clean-up

 builtin-count-objects.c |    2 +-
 builtin-fsck.c          |    2 +-
 builtin-pack-objects.c  |   14 ++--------
 builtin-prune-packed.c  |    2 +-
 cache.h                 |    4 +-
 diff.c                  |    2 +-
 git-repack.sh           |    6 ++++-
 revision.c              |   25 +++++++------------
 revision.h              |    6 +---
 sha1_file.c             |   60 ++++++++++++++++++++--------------------------
 10 files changed, 51 insertions(+), 72 deletions(-)

[PATCH 1/6] git-repack: resist stray environment variable

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

The script used $args and $existing without initializing it to empty.  It
would have been confused by an environment variable the end user had
before running it.

Signed-off-by: Junio C Hamano <redacted>
---
 git-repack.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/git-repack.sh b/git-repack.sh
index 458a497..8600015 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -60,6 +60,7 @@ case ",$all_into_one," in
 	args='--unpacked --incremental'
 	;;
 ,t,)
+	args= existing=
 	if [ -d "$PACKDIR" ]; then
 		for e in `cd "$PACKDIR" && find . -type f -name '*.pack' \
 			| sed -e 's/^\.\///' -e 's/\.pack$//'`
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 2/6] has_sha1_pack(): refactor "pretend these packs do not exist" interface

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

Most of the callers of this function except only one pass NULL to its last
parameter, ignore_packed.

Introduce has_sha1_kept_pack() function that has the function signature
and the semantics of this function, and convert the sole caller that does
not pass NULL to call this new function.

All other callers and has_sha1_pack() lose the ignore_packed parameter.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-count-objects.c |    2 +-
 builtin-fsck.c          |    2 +-
 builtin-prune-packed.c  |    2 +-
 cache.h                 |    3 ++-
 diff.c                  |    2 +-
 revision.c              |    3 ++-
 sha1_file.c             |   32 +++++++++++++++++++++++++-------
 7 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/builtin-count-objects.c b/builtin-count-objects.c
index 91b5487..c095e8d 100644
--- a/builtin-count-objects.c
+++ b/builtin-count-objects.c
@@ -61,7 +61,7 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 		hex[40] = 0;
 		if (get_sha1_hex(hex, sha1))
 			die("internal error");
-		if (has_sha1_pack(sha1, NULL))
+		if (has_sha1_pack(sha1))
 			(*packed_loose)++;
 	}
 }
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 30971ce..491375d 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -158,7 +158,7 @@ static void check_reachable_object(struct object *obj)
 	 * do a full fsck
 	 */
 	if (!obj->parsed) {
-		if (has_sha1_pack(obj->sha1, NULL))
+		if (has_sha1_pack(obj->sha1))
 			return; /* it is in pack - forget about it */
 		printf("missing %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1));
 		errors_found |= ERROR_REACHABLE;
diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index 10cb8df..2d5b2cd 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -23,7 +23,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
 		memcpy(hex+2, de->d_name, 38);
 		if (get_sha1_hex(hex, sha1))
 			continue;
-		if (!has_sha1_pack(sha1, NULL))
+		if (!has_sha1_pack(sha1))
 			continue;
 		memcpy(pathname + len, de->d_name, 38);
 		if (opts & DRY_RUN)
diff --git a/cache.h b/cache.h
index 42f2f27..c1539bf 100644
--- a/cache.h
+++ b/cache.h
@@ -565,7 +565,8 @@ extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned l
 
 extern int move_temp_to_file(const char *tmpfile, const char *filename);
 
-extern int has_sha1_pack(const unsigned char *sha1, const char **ignore);
+extern int has_sha1_pack(const unsigned char *sha1);
+extern int has_sha1_kept_pack(const unsigned char *sha1, const char **ignore);
 extern int has_sha1_file(const unsigned char *sha1);
 extern int has_loose_object_nonlocal(const unsigned char *sha1);
 
diff --git a/diff.c b/diff.c
index f91f256..a34d26c 100644
--- a/diff.c
+++ b/diff.c
@@ -1743,7 +1743,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	 * objects however would tend to be slower as they need
 	 * to be individually opened and inflated.
 	 */
-	if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1, NULL))
+	if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
 		return 0;
 
 	len = strlen(name);
diff --git a/revision.c b/revision.c
index 45fd7a3..746eeed 100644
--- a/revision.c
+++ b/revision.c
@@ -1485,7 +1485,8 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 {
 	if (commit->object.flags & SHOWN)
 		return commit_ignore;
-	if (revs->unpacked && has_sha1_pack(commit->object.sha1, revs->ignore_packed))
+	if (revs->unpacked &&
+	    has_sha1_kept_pack(commit->object.sha1, revs->ignore_packed))
 		return commit_ignore;
 	if (revs->show_all)
 		return commit_show;
diff --git a/sha1_file.c b/sha1_file.c
index 88035a0..ac4375d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1874,7 +1874,8 @@ int matches_pack_name(struct packed_git *p, const char *name)
 	return 0;
 }
 
-static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, const char **ignore_packed)
+static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
+			 const char **ignore_packed)
 {
 	static struct packed_git *last_found = (void *)1;
 	struct packed_git *p;
@@ -1934,6 +1935,17 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons
 	return 0;
 }
 
+static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
+{
+	return find_pack_ent(sha1, e, NULL);
+}
+
+static int find_kept_pack_entry(const unsigned char *sha1, struct pack_entry *e,
+				const char **ignore_packed)
+{
+	return find_pack_ent(sha1, e, ignore_packed);
+}
+
 struct packed_git *find_sha1_pack(const unsigned char *sha1,
 				  struct packed_git *packs)
 {
@@ -1975,7 +1987,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 	struct pack_entry e;
 	int status;
 
-	if (!find_pack_entry(sha1, &e, NULL)) {
+	if (!find_pack_entry(sha1, &e)) {
 		/* Most likely it's a loose object. */
 		status = sha1_loose_object_info(sha1, sizep);
 		if (status >= 0)
@@ -1983,7 +1995,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 
 		/* Not a loose object; someone else may have just packed it. */
 		reprepare_packed_git();
-		if (!find_pack_entry(sha1, &e, NULL))
+		if (!find_pack_entry(sha1, &e))
 			return status;
 	}
 	return packed_object_info(e.p, e.offset, sizep);
@@ -1995,7 +2007,7 @@ static void *read_packed_sha1(const unsigned char *sha1,
 	struct pack_entry e;
 	void *data;
 
-	if (!find_pack_entry(sha1, &e, NULL))
+	if (!find_pack_entry(sha1, &e))
 		return NULL;
 	data = cache_or_unpack_entry(e.p, e.offset, size, type, 1);
 	if (!data) {
@@ -2395,17 +2407,23 @@ int has_pack_file(const unsigned char *sha1)
 	return 1;
 }
 
-int has_sha1_pack(const unsigned char *sha1, const char **ignore_packed)
+int has_sha1_pack(const unsigned char *sha1)
+{
+	struct pack_entry e;
+	return find_pack_entry(sha1, &e);
+}
+
+int has_sha1_kept_pack(const unsigned char *sha1, const char **ignore_packed)
 {
 	struct pack_entry e;
-	return find_pack_entry(sha1, &e, ignore_packed);
+	return find_kept_pack_entry(sha1, &e, ignore_packed);
 }
 
 int has_sha1_file(const unsigned char *sha1)
 {
 	struct pack_entry e;
 
-	if (find_pack_entry(sha1, &e, NULL))
+	if (find_pack_entry(sha1, &e))
 		return 1;
 	return has_loose_object(sha1);
 }
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 3/6] has_sha1_kept_pack(): take "struct rev_info"

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

Its "ignore_packed" parameter always comes from struct rev_info.  This
patch makes the function take a pointer to the surrounding structure, so
that the refactoring in the next patch becomes easier to review.

There is an unfortunate header file dependency and the easiest workaround
is to temporarily move the function declaration from cache.h to
revision.h; this will be moved back to cache.h once the function loses
this "ignore_packed" parameter altogether in the later part of the
series.

Signed-off-by: Junio C Hamano <redacted>
---
 cache.h     |    1 -
 revision.c  |    2 +-
 revision.h  |    2 ++
 sha1_file.c |   16 +++++++++-------
 4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index c1539bf..8e43f38 100644
--- a/cache.h
+++ b/cache.h
@@ -566,7 +566,6 @@ extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned l
 extern int move_temp_to_file(const char *tmpfile, const char *filename);
 
 extern int has_sha1_pack(const unsigned char *sha1);
-extern int has_sha1_kept_pack(const unsigned char *sha1, const char **ignore);
 extern int has_sha1_file(const unsigned char *sha1);
 extern int has_loose_object_nonlocal(const unsigned char *sha1);
 
diff --git a/revision.c b/revision.c
index 746eeed..795e0c0 100644
--- a/revision.c
+++ b/revision.c
@@ -1486,7 +1486,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 	if (commit->object.flags & SHOWN)
 		return commit_ignore;
 	if (revs->unpacked &&
-	    has_sha1_kept_pack(commit->object.sha1, revs->ignore_packed))
+	    has_sha1_kept_pack(commit->object.sha1, revs))
 		return commit_ignore;
 	if (revs->show_all)
 		return commit_show;
diff --git a/revision.h b/revision.h
index 91f1944..9304296 100644
--- a/revision.h
+++ b/revision.h
@@ -158,4 +158,6 @@ enum commit_action {
 
 extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
 
+extern int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *);
+
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index ac4375d..f963c3c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -16,6 +16,8 @@
 #include "refs.h"
 #include "pack-revindex.h"
 #include "sha1-lookup.h"
+#include "diff.h"
+#include "revision.h"
 
 #ifndef O_NOATIME
 #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))
@@ -1875,7 +1877,7 @@ int matches_pack_name(struct packed_git *p, const char *name)
 }
 
 static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
-			 const char **ignore_packed)
+			 const struct rev_info *revs)
 {
 	static struct packed_git *last_found = (void *)1;
 	struct packed_git *p;
@@ -1887,9 +1889,9 @@ static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 	p = (last_found == (void *)1) ? packed_git : last_found;
 
 	do {
-		if (ignore_packed) {
+		if (revs->ignore_packed) {
 			const char **ig;
-			for (ig = ignore_packed; *ig; ig++)
+			for (ig = revs->ignore_packed; *ig; ig++)
 				if (matches_pack_name(p, *ig))
 					break;
 			if (*ig)
@@ -1941,9 +1943,9 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 }
 
 static int find_kept_pack_entry(const unsigned char *sha1, struct pack_entry *e,
-				const char **ignore_packed)
+				const struct rev_info *revs)
 {
-	return find_pack_ent(sha1, e, ignore_packed);
+	return find_pack_ent(sha1, e, revs);
 }
 
 struct packed_git *find_sha1_pack(const unsigned char *sha1,
@@ -2413,10 +2415,10 @@ int has_sha1_pack(const unsigned char *sha1)
 	return find_pack_entry(sha1, &e);
 }
 
-int has_sha1_kept_pack(const unsigned char *sha1, const char **ignore_packed)
+int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *revs)
 {
 	struct pack_entry e;
-	return find_kept_pack_entry(sha1, &e, ignore_packed);
+	return find_kept_pack_entry(sha1, &e, revs);
 }
 
 int has_sha1_file(const unsigned char *sha1)
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 4/6] Consolidate ignore_packed logic more

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

This refactors three loops that check if a given packfile is on the
ignore_packed list into a function is_kept_pack().  The function returns
false for a pack on the list, and true for a pack not on the list, because
this list is solely used by "git repack" to pass list of packfiles that do
not have corresponding .keep files, i.e. a packfile not on the list is
"kept".

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-pack-objects.c |   12 ++----------
 cache.h                |    1 -
 revision.h             |    1 +
 sha1_file.c            |   24 ++++++++++++++----------
 4 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index fb5e14d..7e7719b 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1915,11 +1915,7 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)
 		const unsigned char *sha1;
 		struct object *o;
 
-		for (i = 0; i < revs->num_ignore_packed; i++) {
-			if (matches_pack_name(p, revs->ignore_packed[i]))
-				break;
-		}
-		if (revs->num_ignore_packed <= i)
+		if (is_kept_pack(p, revs))
 			continue;
 		if (open_pack_index(p))
 			die("cannot open pack index");
@@ -1955,11 +1951,7 @@ static void loosen_unused_packed_objects(struct rev_info *revs)
 	const unsigned char *sha1;
 
 	for (p = packed_git; p; p = p->next) {
-		for (i = 0; i < revs->num_ignore_packed; i++) {
-			if (matches_pack_name(p, revs->ignore_packed[i]))
-				break;
-		}
-		if (revs->num_ignore_packed <= i)
+		if (is_kept_pack(p, revs))
 			continue;
 
 		if (open_pack_index(p))
diff --git a/cache.h b/cache.h
index 8e43f38..23c16d0 100644
--- a/cache.h
+++ b/cache.h
@@ -747,7 +747,6 @@ extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsign
 extern unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
 extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigned long *, unsigned long *, unsigned int *, unsigned char *);
-extern int matches_pack_name(struct packed_git *p, const char *name);
 
 /* Dumb servers support */
 extern int update_server_info(int);
diff --git a/revision.h b/revision.h
index 9304296..af414e5 100644
--- a/revision.h
+++ b/revision.h
@@ -159,5 +159,6 @@ enum commit_action {
 extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
 
 extern int has_sha1_kept_pack(const unsigned char *sha1, const struct rev_info *);
+extern int is_kept_pack(const struct packed_git *, const struct rev_info *);
 
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index f963c3c..6e0a462 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1858,7 +1858,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	return 0;
 }
 
-int matches_pack_name(struct packed_git *p, const char *name)
+static int matches_pack_name(const struct packed_git *p, const char *name)
 {
 	const char *last_c, *c;
 
@@ -1876,6 +1876,17 @@ int matches_pack_name(struct packed_git *p, const char *name)
 	return 0;
 }
 
+int is_kept_pack(const struct packed_git *p, const struct rev_info *revs)
+{
+	int i;
+
+	for (i = 0; i < revs->num_ignore_packed; i++) {
+		if (matches_pack_name(p, revs->ignore_packed[i]))
+			return 0;
+	}
+	return 1;
+}
+
 static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 			 const struct rev_info *revs)
 {
@@ -1889,15 +1900,8 @@ static int find_pack_ent(const unsigned char *sha1, struct pack_entry *e,
 	p = (last_found == (void *)1) ? packed_git : last_found;
 
 	do {
-		if (revs->ignore_packed) {
-			const char **ig;
-			for (ig = revs->ignore_packed; *ig; ig++)
-				if (matches_pack_name(p, *ig))
-					break;
-			if (*ig)
-				goto next;
-		}
-
+		if (revs->ignore_packed && !is_kept_pack(p, revs))
+			goto next;
 		if (p->num_bad_objects) {
 			unsigned i;
 			for (i = 0; i < p->num_bad_objects; i++)
-- 
1.6.2.rc2.99.g9f3bb

Re: [PATCH 0/6] "git repack -a -d" improvements

From: Kjetil Barvik <hidden>
Date: 2016-06-15 22:46:18

* Junio C Hamano [off-list ref] writes:
| [2/6] refactors public interface has_sha1_pack() that takes an optional
| "ignore_packed" list.  Most callers pass NULL, so it introduces a new
| function has_sha1_kept_pack() and migrate the minority caller to this
| interface while losing the argument from the original function and callers
| that currently pass NULL.
  [...]
| [4/6] identifies three places that use "ignore_packed" list to tell if a
| pack is on the list or not, and introduces a helper function to do so.
| The helper is conveniently called is_kept_pack(), even though at this
| stage the list does not necessarily mean a list of "unkept" packs yet.

  OK, patch 2/6 failed for me when I was doing 'git am' to import the
  patch-series, so sorry if do not see all bits of the patch correctly.

  Would it be an improvment to change the signature of the currently
  find_sha1_pack() function to:

    struct packed_git *
    find_pack_entry(const unsigned char *sha1, off_t *sha1_pack_offset,
                    struct packed_git *packs)

    - The currently existing 'struct pack_entry *e' parameter is only
      used to retrn the offset, so make it more clear.  The struct
      pack_entry can probably be deleted from the sha1_file.c file.

    - When the 'git repack -a -d' command is used, one has to compute
      the list of allowed pack-files to look into, and give this list to
      find_pack_entry().

    - The currently named find_sha1_pack() function can then be deleted.

    - For example, when this function is now used in sha1_object_info()
      it can be called like this:

          found_pack = find_pack_entry(sha1, &offset, packed_git);

  -- kjetil

Re: [PATCH 0/6] "git repack -a -d" improvements

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:18

Kjetil Barvik [off-list ref] writes:
  OK, patch 2/6 failed for me when I was doing 'git am' to import the
  patch-series, so sorry if do not see all bits of the patch correctly.
Ah, I should have mentioned that the series was meant to apply to v1.6.0.6
(and merged upwards).

I am off to dentist so I won't have time to think about below until I get
back.
  Would it be an improvment to change the signature of the currently
  find_sha1_pack() function to:

    struct packed_git *
    find_pack_entry(const unsigned char *sha1, off_t *sha1_pack_offset,
                    struct packed_git *packs)

    - The currently existing 'struct pack_entry *e' parameter is only
      used to retrn the offset, so make it more clear.  The struct
      pack_entry can probably be deleted from the sha1_file.c file.

    - When the 'git repack -a -d' command is used, one has to compute
      the list of allowed pack-files to look into, and give this list to
      find_pack_entry().

    - The currently named find_sha1_pack() function can then be deleted.

    - For example, when this function is now used in sha1_object_info()
      it can be called like this:

          found_pack = find_pack_entry(sha1, &offset, packed_git);

  -- kjetil
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help