git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations
@ 2010-08-26  6:21 Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 1/8] Add testcases showing how pathspecs are ignored with rev-list --objects Elijah Newren
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

This series enables rev-list to produce a list of objects that is path
limited, when the user requests it.  It also provides a few small code
cleanups and some small optimizations.

Changes since last round:
  * Patch 1 has two new testcase; one that triggered a bug I found,
    and an interesting case Junio mentioned needed to work.
  * Patch 2 will have fewer mallocs/frees in some cases (a suggestion
    from Nguyen Thai Ngoc Duy), and corrects an issue from the first
    series with how it calls tree_entry_interesting.
  * Patch 3 is new; it adds a simple code comment that would have
    alerted me to avoiding the bug in the first round of the series.
  * Patches 4-8 are unchanged.

As before, the last two patches are weatherballons.  They can be
silently dropped.

Updated timings, for the curious:
 A git rev-list --quiet HEAD
 B git rev-list --quiet HEAD -- Documentation/
 C git rev-list --quiet HEAD -- t/
 D git rev-list --objects HEAD > /dev/null
 E git rev-list --objects HEAD -- Documentation/ > /dev/null
 F git rev-list --objects HEAD -- t/t9350-fast-export.sh > /dev/null
Results:
             A      B      C      D      E      F
maint       0.34   0.68   1.33   1.90   1.39   0.65
Patch-1     0.34   0.68   1.33   1.90   1.38   0.65
Patch-2     0.34   0.68   1.33   1.88   0.97   0.66
Patch-3     0.34   0.68   1.33   1.88   0.97   0.65
Patch-4     0.34   0.67   1.33   1.87   0.97   0.65
Patch-5     0.34   0.68   1.33   1.88   0.97   0.65
Patch-6     0.34   0.65   1.28   1.87   0.93   0.65
Patch-7     0.34   0.65   1.29   1.86   0.94   0.65
Patch-8     0.34   0.65   1.28   1.85   0.93   0.65



Elijah Newren (8):
  Add testcases showing how pathspecs are ignored with rev-list
    --objects
  Make rev-list --objects work together with pathspecs
  Document pre-condition for tree_entry_interesting
  tree-walk: Correct bitrotted comment about tree_entry()
  tree_entry_interesting(): Make return value more specific
  diff_tree(): Skip skip_uninteresting() when all remaining paths
    interesting
  list-objects.c: Avoid recomputing interesting-ness for subtrees when
    possible
  tree-diff.c: Avoid recomputing interesting-ness for subtrees when
    possible

 diff.h                   |    1 +
 list-objects.c           |   34 +++++++++++++++++++++++---
 revision.c               |    8 ++++-
 revision.h               |    3 +-
 t/t6000-rev-list-misc.sh |   51 +++++++++++++++++++++++++++++++++++++++
 tree-diff.c              |   60 +++++++++++++++++++++++----------------------
 tree-walk.h              |    4 ++-
 7 files changed, 124 insertions(+), 37 deletions(-)
 create mode 100755 t/t6000-rev-list-misc.sh

-- 
1.7.2.2.45.ga60f

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCHv2 1/8] Add testcases showing how pathspecs are ignored with rev-list --objects
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 2/8] Make rev-list --objects work together with pathspecs Elijah Newren
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren


Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6000-rev-list-misc.sh |   51 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100755 t/t6000-rev-list-misc.sh

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
new file mode 100755
index 0000000..a298f0d
--- /dev/null
+++ b/t/t6000-rev-list-misc.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+
+test_description='miscellaneous rev-list tests'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo content1 > wanted_file &&
+	echo content2 > unwanted_file &&
+	git add wanted_file unwanted_file &&
+	git commit -mone
+'
+
+test_expect_failure 'rev-list --objects heeds pathspecs' '
+	git rev-list --objects HEAD -- wanted_file >output &&
+	grep wanted_file output &&
+	! grep unwanted_file output
+'
+
+test_expect_failure 'rev-list --objects with pathspecs and deeper paths' '
+	mkdir foo &&
+	> foo/file &&
+	git add foo/file &&
+	git commit -mtwo &&
+
+	git rev-list --objects HEAD -- foo >output &&
+	grep foo/file output &&
+
+	git rev-list --objects HEAD -- foo/file >output &&
+	grep foo/file output &&
+	! grep unwanted_file output
+'
+
+test_expect_failure 'rev-list --objects with pathspecs and copied files' '
+	git checkout --orphan junio-testcase &&
+	git rm -rf . &&
+
+	mkdir two &&
+	echo frotz >one &&
+	cp one two/three &&
+	git add one two/three &&
+	test_tick &&
+	git commit -m that &&
+
+	ONE=$(git rev-parse HEAD:one)
+	git rev-list --objects HEAD two >output &&
+	grep "$ONE two/three" output &&
+	! grep one output
+'
+
+test_done
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 2/8] Make rev-list --objects work together with pathspecs
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 1/8] Add testcases showing how pathspecs are ignored with rev-list --objects Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 3/8] Document pre-condition for tree_entry_interesting Elijah Newren
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

When traversing commits, the selection of commits would heed the list of
pathspecs passed, but subsequent walking of the trees of those commits
would not.  This resulted in 'rev-list --objects HEAD -- <paths>'
displaying objects at unwanted paths.

Have process_tree() call tree_entry_interesting() to determine which paths
are interesting and should be walked.

Naturally, this change can provide a large speedup when paths are specified
together with --objects, since many tree entries are now correctly ignored.
Interestingly, though, this change also gives me a small (~1%) but
repeatable speedup even when no paths are specified with --objects.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diff.h                   |    1 +
 list-objects.c           |   27 ++++++++++++++++++++++++++-
 revision.c               |    8 ++++++--
 revision.h               |    3 ++-
 t/t6000-rev-list-misc.sh |    6 +++---
 tree-diff.c              |    2 +-
 6 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/diff.h b/diff.h
index 6fff024..35130b8 100644
--- a/diff.h
+++ b/diff.h
@@ -169,6 +169,7 @@ extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
 			  const char *base, struct diff_options *opt);
 extern int diff_root_tree_sha1(const unsigned char *new, const char *base,
                                struct diff_options *opt);
+extern int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt);
 
 struct combine_diff_path {
 	struct combine_diff_path *next;
diff --git a/list-objects.c b/list-objects.c
index 8953548..bb95962 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -67,6 +67,9 @@ static void process_tree(struct rev_info *revs,
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct name_path me;
+	int all_interesting = (revs->diffopt.nr_paths == 0);
+	char *full_prefix = NULL;
+	int full_prefix_len = 0;
 
 	if (!revs->tree_objects)
 		return;
@@ -82,9 +85,30 @@ static void process_tree(struct rev_info *revs,
 	me.elem = name;
 	me.elem_len = strlen(name);
 
+	if (!all_interesting) {
+		full_prefix = path_name_impl(path, name, 1);
+		full_prefix_len = strlen(full_prefix);
+	}
+
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
-	while (tree_entry(&desc, &entry)) {
+	for (; desc.size; update_tree_entry(&desc)) {
+		entry = desc.entry;
+
+		if (!all_interesting) {
+			int showit = tree_entry_interesting(&desc,
+							    full_prefix,
+							    full_prefix_len,
+							    &revs->diffopt);
+
+			if (showit < 0)
+				break;
+			else if (!showit)
+				continue;
+			else if (showit == 2)
+				all_interesting = 1;
+		}
+
 		if (S_ISDIR(entry.mode))
 			process_tree(revs,
 				     lookup_tree(entry.sha1),
@@ -97,6 +121,7 @@ static void process_tree(struct rev_info *revs,
 				     lookup_blob(entry.sha1),
 				     show, &me, entry.path);
 	}
+	free(full_prefix);
 	free(tree->buffer);
 	tree->buffer = NULL;
 }
diff --git a/revision.c b/revision.c
index 7e82efd..64d8316 100644
--- a/revision.c
+++ b/revision.c
@@ -16,7 +16,7 @@
 
 volatile show_early_output_fn_t show_early_output;
 
-char *path_name(const struct name_path *path, const char *name)
+char *path_name_impl(const struct name_path *path, const char *name, int isdir)
 {
 	const struct name_path *p;
 	char *n, *m;
@@ -27,7 +27,7 @@ char *path_name(const struct name_path *path, const char *name)
 		if (p->elem_len)
 			len += p->elem_len + 1;
 	}
-	n = xmalloc(len);
+	n = xmalloc(len + !!isdir);
 	m = n + len - (nlen + 1);
 	strcpy(m, name);
 	for (p = path; p; p = p->up) {
@@ -37,6 +37,10 @@ char *path_name(const struct name_path *path, const char *name)
 			m[p->elem_len] = '/';
 		}
 	}
+	if (isdir && len > 1) {
+		n[len-1] = '/';
+		n[len] = '\0';
+	}
 	return n;
 }
 
diff --git a/revision.h b/revision.h
index 36fdf22..5ecbeb0 100644
--- a/revision.h
+++ b/revision.h
@@ -172,7 +172,8 @@ struct name_path {
 	const char *elem;
 };
 
-char *path_name(const struct name_path *path, const char *name);
+char *path_name_impl(const struct name_path *path, const char *name, int isdir);
+#define path_name(path, name) path_name_impl(path, name, 0)
 
 extern void add_object(struct object *obj,
 		       struct object_array *p,
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index a298f0d..b6a7c9c 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -11,13 +11,13 @@ test_expect_success setup '
 	git commit -mone
 '
 
-test_expect_failure 'rev-list --objects heeds pathspecs' '
+test_expect_success 'rev-list --objects heeds pathspecs' '
 	git rev-list --objects HEAD -- wanted_file >output &&
 	grep wanted_file output &&
 	! grep unwanted_file output
 '
 
-test_expect_failure 'rev-list --objects with pathspecs and deeper paths' '
+test_expect_success 'rev-list --objects with pathspecs and deeper paths' '
 	mkdir foo &&
 	> foo/file &&
 	git add foo/file &&
@@ -31,7 +31,7 @@ test_expect_failure 'rev-list --objects with pathspecs and deeper paths' '
 	! grep unwanted_file output
 '
 
-test_expect_failure 'rev-list --objects with pathspecs and copied files' '
+test_expect_success 'rev-list --objects with pathspecs and copied files' '
 	git checkout --orphan junio-testcase &&
 	git rm -rf . &&
 
diff --git a/tree-diff.c b/tree-diff.c
index cd659c6..2fb670b 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -91,7 +91,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
  *  - zero for no
  *  - negative for "no, and no subsequent entries will be either"
  */
-static int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt)
+int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen, struct diff_options *opt)
 {
 	const char *path;
 	const unsigned char *sha1;
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 3/8] Document pre-condition for tree_entry_interesting
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 1/8] Add testcases showing how pathspecs are ignored with rev-list --objects Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 2/8] Make rev-list --objects work together with pathspecs Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 4/8] tree-walk: Correct bitrotted comment about tree_entry() Elijah Newren
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

tree_entry_interesting will fail to find appropriate matches if the base
directory path is not terminated with a slash.  Knowing this earlier would
have saved me some debugging time.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 tree-diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index 2fb670b..75b1480 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -85,6 +85,8 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 /*
  * Is a tree entry interesting given the pathspec we have?
  *
+ * Pre-condition: baselen == 0 || base[baselen-1] == '/'
+ *
  * Return:
  *  - 2 for "yes, and all subsequent entries will be"
  *  - 1 for yes
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 4/8] tree-walk: Correct bitrotted comment about tree_entry()
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (2 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 3/8] Document pre-condition for tree_entry_interesting Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 5/8] tree_entry_interesting(): Make return value more specific Elijah Newren
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

There was a code comment that referred to the "above two functions" but
over time the functions immediately preceding the comment have changed.
Just mention the relevant functions by name.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 tree-walk.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tree-walk.h b/tree-walk.h
index 42110a4..a5175fa 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -28,7 +28,9 @@ static inline int tree_entry_len(const char *name, const unsigned char *sha1)
 void update_tree_entry(struct tree_desc *);
 void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
 
-/* Helper function that does both of the above and returns true for success */
+/* Helper function that does both tree_entry_extract() and update_tree_entry()
+ * and returns true for success
+ */
 int tree_entry(struct tree_desc *, struct name_entry *);
 
 void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 5/8] tree_entry_interesting(): Make return value more specific
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (3 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 4/8] tree-walk: Correct bitrotted comment about tree_entry() Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 6/8] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting Elijah Newren
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

tree_entry_interesting() can signal to its callers not only if the given
entry matches one of the specified paths, but whether all remaining paths
will (or will not) match.  When no paths are specified, all paths are
considered interesting, so intead of returning 1 (this path is interesting)
return 2 (all paths are interesting).

This will allow the caller to avoid calling tree_entry_interesting() again,
which theoretically should speed up tree walking.  I am not able to measure
any actual gains in practice, but it certainly can not hurt and seems to
make the code more readable to me.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 tree-diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index 75b1480..bac5008 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -103,7 +103,7 @@ int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen
 	int never_interesting = -1;
 
 	if (!opt->nr_paths)
-		return 1;
+		return 2;
 
 	sha1 = tree_entry_extract(desc, &path, &mode);
 
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 6/8] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (4 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 5/8] tree_entry_interesting(): Make return value more specific Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 7/8] list-objects.c: Avoid recomputing interesting-ness for subtrees when possible Elijah Newren
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

In 1d848f6 (tree_entry_interesting(): allow it to say "everything is
interesting" 2007-03-21), both show_tree() and skip_uninteresting() were
modified to determine if all remaining tree entries were interesting.
However, the latter returns as soon as it finds the first interesting path,
without any way to signal to its caller (namely, diff_tree()) that all
remaining paths are interesting, making these extra checks useless.

Pass whether all remaining entries are interesting back to diff_tree(), and
whenever they are, have diff_tree() skip subsequent calls to
skip_uninteresting().

With this change, I measure speedups of 3-4% for the commands
  git rev-list --quiet HEAD -- Documentation/
  git rev-list --quiet HEAD -- t/
in git.git.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 tree-diff.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index bac5008..d976bdf 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -259,19 +259,12 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 	}
 }
 
-static void skip_uninteresting(struct tree_desc *t, const char *base, int baselen, struct diff_options *opt)
+static void skip_uninteresting(struct tree_desc *t, const char *base, int baselen, struct diff_options *opt, int *all_interesting)
 {
-	int all_interesting = 0;
 	while (t->size) {
-		int show;
-
-		if (all_interesting)
-			show = 1;
-		else {
-			show = tree_entry_interesting(t, base, baselen, opt);
-			if (show == 2)
-				all_interesting = 1;
-		}
+		int show = tree_entry_interesting(t, base, baselen, opt);
+		if (show == 2)
+			*all_interesting = 1;
 		if (!show) {
 			update_tree_entry(t);
 			continue;
@@ -286,14 +279,20 @@ static void skip_uninteresting(struct tree_desc *t, const char *base, int basele
 int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt)
 {
 	int baselen = strlen(base);
+	int all_t1_interesting = 0;
+	int all_t2_interesting = 0;
 
 	for (;;) {
 		if (DIFF_OPT_TST(opt, QUICK) &&
 		    DIFF_OPT_TST(opt, HAS_CHANGES))
 			break;
 		if (opt->nr_paths) {
-			skip_uninteresting(t1, base, baselen, opt);
-			skip_uninteresting(t2, base, baselen, opt);
+			if (!all_t1_interesting)
+				skip_uninteresting(t1, base, baselen, opt,
+						   &all_t1_interesting);
+			if (!all_t2_interesting)
+				skip_uninteresting(t2, base, baselen, opt,
+						   &all_t2_interesting);
 		}
 		if (!t1->size) {
 			if (!t2->size)
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 7/8] list-objects.c: Avoid recomputing interesting-ness for subtrees when possible
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (5 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 6/8] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-26  6:21 ` [PATCHv2 8/8] tree-diff.c: " Elijah Newren
  2010-08-27 17:28 ` [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

Weather balloon patch.  Doesn't seem to help much in benchmarks; in fact
I think it sometimes hurts a bit.  Is it worthwhile just in case someone
comes up with a ginormous tree that is really deep with few entries per
tree?  I'm leaning against it, but am sending the patch to at least show
others that it has been considered.

---
No signed-off-by, since I'm not sold on this patch and am somewhat leaning
against it.  It would need a better commit message anyway.  :-)

 list-objects.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index bb95962..488428b 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -61,13 +61,13 @@ static void process_tree(struct rev_info *revs,
 			 struct tree *tree,
 			 show_object_fn show,
 			 struct name_path *path,
-			 const char *name)
+			 const char *name,
+			 int all_interesting)
 {
 	struct object *obj = &tree->object;
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct name_path me;
-	int all_interesting = (revs->diffopt.nr_paths == 0);
 	char *full_prefix = NULL;
 	int full_prefix_len = 0;
 
@@ -112,7 +112,8 @@ static void process_tree(struct rev_info *revs,
 		if (S_ISDIR(entry.mode))
 			process_tree(revs,
 				     lookup_tree(entry.sha1),
-				     show, &me, entry.path);
+				     show, &me, entry.path,
+				     all_interesting);
 		else if (S_ISGITLINK(entry.mode))
 			process_gitlink(revs, entry.sha1,
 					show, &me, entry.path);
@@ -189,7 +190,7 @@ void traverse_commit_list(struct rev_info *revs,
 		}
 		if (obj->type == OBJ_TREE) {
 			process_tree(revs, (struct tree *)obj, show_object,
-				     NULL, name);
+				     NULL, name, revs->diffopt.nr_paths == 0);
 			continue;
 		}
 		if (obj->type == OBJ_BLOB) {
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 8/8] tree-diff.c: Avoid recomputing interesting-ness for subtrees when possible
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (6 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 7/8] list-objects.c: Avoid recomputing interesting-ness for subtrees when possible Elijah Newren
@ 2010-08-26  6:21 ` Elijah Newren
  2010-08-27 17:28 ` [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Elijah Newren @ 2010-08-26  6:21 UTC (permalink / raw)
  To: git; +Cc: gitster, pclouds, Elijah Newren

Weather balloon patch.  Doesn't seem to help much in benchmarks, and after
many runs I think it might occasionally hurt slightly.  It ought to help
in the case of a ginormous tree that is really deep with few entries per
tree, but that case seems kind of contrived.

Kind of makes the code look icky too.

---
No signed-off-by, since I'm not sold on this patch and am somewhat leaning
against it.  It would need a better commit message anyway.  :-)

 tree-diff.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index d976bdf..ecae15e 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -25,9 +25,9 @@ static char *malloc_fullname(const char *base, int baselen, const char *path, in
 }
 
 static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,
-		       const char *base, int baselen);
+		       const char *base, int baselen, int all_interesting);
 
-static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const char *base, int baselen, struct diff_options *opt)
+static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const char *base, int baselen, struct diff_options *opt, int all_t1_interesting, int all_t2_interesting)
 {
 	unsigned mode1, mode2;
 	const char *path1, *path2;
@@ -42,11 +42,11 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 	pathlen2 = tree_entry_len(path2, sha2);
 	cmp = base_name_compare(path1, pathlen1, mode1, path2, pathlen2, mode2);
 	if (cmp < 0) {
-		show_entry(opt, "-", t1, base, baselen);
+		show_entry(opt, "-", t1, base, baselen, all_t1_interesting);
 		return -1;
 	}
 	if (cmp > 0) {
-		show_entry(opt, "+", t2, base, baselen);
+		show_entry(opt, "+", t2, base, baselen, all_t2_interesting);
 		return 1;
 	}
 	if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)
@@ -57,8 +57,8 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 	 * file, we need to consider it a remove and an add.
 	 */
 	if (S_ISDIR(mode1) != S_ISDIR(mode2)) {
-		show_entry(opt, "-", t1, base, baselen);
-		show_entry(opt, "+", t2, base, baselen);
+		show_entry(opt, "-", t1, base, baselen, all_t1_interesting);
+		show_entry(opt, "+", t2, base, baselen, all_t2_interesting);
 		return 0;
 	}
 
@@ -199,9 +199,8 @@ int tree_entry_interesting(struct tree_desc *desc, const char *base, int baselen
 }
 
 /* A whole sub-tree went away or appeared */
-static void show_tree(struct diff_options *opt, const char *prefix, struct tree_desc *desc, const char *base, int baselen)
+static void show_tree(struct diff_options *opt, const char *prefix, struct tree_desc *desc, const char *base, int baselen, int all_interesting)
 {
-	int all_interesting = 0;
 	while (desc->size) {
 		int show;
 
@@ -216,14 +215,14 @@ static void show_tree(struct diff_options *opt, const char *prefix, struct tree_
 		if (show < 0)
 			break;
 		if (show)
-			show_entry(opt, prefix, desc, base, baselen);
+			show_entry(opt, prefix, desc, base, baselen, all_interesting);
 		update_tree_entry(desc);
 	}
 }
 
 /* A file entry went away or appeared */
 static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,
-		       const char *base, int baselen)
+		       const char *base, int baselen, int all_interesting)
 {
 	unsigned mode;
 	const char *path;
@@ -248,7 +247,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 		}
 
 		init_tree_desc(&inner, tree, size);
-		show_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen);
+		show_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen, all_interesting);
 
 		free(tree);
 		free(newbase);
@@ -297,16 +296,18 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 		if (!t1->size) {
 			if (!t2->size)
 				break;
-			show_entry(opt, "+", t2, base, baselen);
+			show_entry(opt, "+", t2, base, baselen, all_t2_interesting);
 			update_tree_entry(t2);
 			continue;
 		}
 		if (!t2->size) {
-			show_entry(opt, "-", t1, base, baselen);
+			show_entry(opt, "-", t1, base, baselen, all_t1_interesting);
 			update_tree_entry(t1);
 			continue;
 		}
-		switch (compare_tree_entry(t1, t2, base, baselen, opt)) {
+		switch (compare_tree_entry(t1, t2, base, baselen, opt,
+					   all_t1_interesting,
+					   all_t2_interesting)) {
 		case -1:
 			update_tree_entry(t1);
 			continue;
-- 
1.7.2.2.45.ga60f

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations
  2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
                   ` (7 preceding siblings ...)
  2010-08-26  6:21 ` [PATCHv2 8/8] tree-diff.c: " Elijah Newren
@ 2010-08-27 17:28 ` Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2010-08-27 17:28 UTC (permalink / raw)
  To: Elijah Newren; +Cc: git, pclouds

Elijah Newren <newren@gmail.com> writes:

> This series enables rev-list to produce a list of objects that is path
> limited, when the user requests it.  It also provides a few small code
> cleanups and some small optimizations.

A cursory look didn't spot anything glaringly wrong ;-)

I'd split 3-6 into a separate "tree-walk-optim" topic and keep the first
two as "object-list-with-pathspec" topic.  They really look independent.

Thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2010-08-27 17:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-26  6:21 [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Elijah Newren
2010-08-26  6:21 ` [PATCHv2 1/8] Add testcases showing how pathspecs are ignored with rev-list --objects Elijah Newren
2010-08-26  6:21 ` [PATCHv2 2/8] Make rev-list --objects work together with pathspecs Elijah Newren
2010-08-26  6:21 ` [PATCHv2 3/8] Document pre-condition for tree_entry_interesting Elijah Newren
2010-08-26  6:21 ` [PATCHv2 4/8] tree-walk: Correct bitrotted comment about tree_entry() Elijah Newren
2010-08-26  6:21 ` [PATCHv2 5/8] tree_entry_interesting(): Make return value more specific Elijah Newren
2010-08-26  6:21 ` [PATCHv2 6/8] diff_tree(): Skip skip_uninteresting() when all remaining paths interesting Elijah Newren
2010-08-26  6:21 ` [PATCHv2 7/8] list-objects.c: Avoid recomputing interesting-ness for subtrees when possible Elijah Newren
2010-08-26  6:21 ` [PATCHv2 8/8] tree-diff.c: " Elijah Newren
2010-08-27 17:28 ` [PATCHv2 0/8] Make rev-list --objects work with pathspecs; minor optimizations Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).