Re: [PATCH 3/2] Enhance --early-output format

Linus Torvalds [off-list ref] writes:

It wasn't totally trivial, but it doesn't seem to be excessively subtle 
either. About half the patch is moving around some code to look at whether 
the commit is interesting or not and rewriting the parents, so that it can 
be shared with the revision walker.

Very nicely done.

+	while (list) {
+		struct commit *commit = list->item;
+		unsigned int flags = commit->object.flags;
+
+		list = list->next;
+		if (flags & UNINTERESTING)
+			continue;
+		if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
+			if (commit->parents && !commit->parents->next)
+				continue;
+		}

When looking at:

	if (A && B && C) {
        	if (D && E)
                	continue;
	}

an uninitiated might say "Huh?  Why use nested 'if'?", but to
somebody who knows how revision traversal works, the above split
is a more logical way to test this condition.  Maybe one liner
comment is in order?

+static void show_early_header(struct rev_info *rev, const char *stage, int nr)
+{
+	if (rev->shown_one) {
+		rev->shown_one = 0;
+		if (rev->commit_format != CMIT_FMT_ONELINE)
+			putchar(rev->diffopt.line_termination);
+	}
+	printf("Final output: %d %s\n", nr, stage);
+}

As you noted, this is more like "Partial output" now.
How about painting the bikeshed pink by saying:

	Partial output: 20
        Partial output: 70
        Final output: 70000

+	/* Did we already get enough commits for the early output? */
+	if (!i)
+		return;
+
+	/*
+	 * ..if no, then repeat it twice a second until we
+	 * do.
+	 *
+	 * NOTE! We don't use "it_interval", because if the
+	 * reader isn't listening, we want our output to be
+	 * throttled by the writing, and not have the timer
+	 * trigger every second even if we're blocked on a
+	 * reader!
+	 */

A comment like this is very much appreciated.

+	early_output_timer.it_value.tv_sec = 0;
+	early_output_timer.it_value.tv_usec = 500000;
+	setitimer(ITIMER_REAL, &early_output_timer, NULL);
 }

On Mon, 5 Nov 2007, Junio C Hamano wrote:

quoted

+		if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
+			if (commit->parents && !commit->parents->next)
+				continue;
+		}

When looking at:

	if (A && B && C) {
        	if (D && E)
                	continue;
	}

an uninitiated might say "Huh?  Why use nested 'if'?", but to
somebody who knows how revision traversal works, the above split
is a more logical way to test this condition.  Maybe one liner
comment is in order?

I'd almost prefer not to.

If people feel the code is subtle enough that a comment is in order to 
explain *what* something does, I would rather introduce helper functions 
than add comments. I'm not a big believer in comments in the middle of 
code to explain the code, but I *am* a big believer in trying to make the 
code easy to read without them.

(I don't dislike comments per se, but I'd *much* rather have the comments 
explain what is going on at a higher level. Comments that talk about the 
details of the code itself is likely bogus).

So we could add a commit to say what is going on ("ignore commits that 
have only one parent and didn't change the tree"), but I'd not want to 
explain why that particular layout of code.

That said, I've often found the "TREECHANGE" bit annoying. The fact that 
we always have to test it together with testing "rev->prune_fn" and often 
also the "rev->dense" flag is just annoying. I'd almost like to just 
always set the bit if "rev->prune_fn" isn't set. Alternatively, the code 
could be rewritten to just have a few inline helper functions, and it 
could perhaps be written as

	if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
		continue;

I dunno. I have no really *strong* opinions, although I suspect that 
anybody who looks at that particular piece of code had better understand 
these issues well enough anyway that it doesn't much matter..

As you noted, this is more like "Partial output" now.
How about painting the bikeshed pink by saying:

	Partial output: 20
        Partial output: 70
        Final output: 70000

I'm fine with it. The reason I did it the way I did was that this way I 
didn't need to change "gitk" - I could just use Pauls original patch 
as-is, since that code didn't care *what* came after the "Final output", 
and didn't care how many times it showed up.

			Linus

On Mon, 5 Nov 2007, Linus Torvalds wrote:

That said, I've often found the "TREECHANGE" bit annoying. The fact that 
we always have to test it together with testing "rev->prune_fn" and often 
also the "rev->dense" flag is just annoying. I'd almost like to just 
always set the bit if "rev->prune_fn" isn't set. Alternatively, the code 
could be rewritten to just have a few inline helper functions, and it 
could perhaps be written as

	if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
		continue;

I dunno.

Here's a possible cleanup patch. It's on top of the enhanced 
--early-output format commit, and in fact fixes a stupid bug in that 
commit ("return -1" vs "return NULL"), but that bug-fix is really an 
independent thing.

It basically removes the unnecessary indirection of "revs->prune_fn", 
since that function is always the same one (or NULL), and there is in fact 
not even an abstraction reason to make it a function (ie its not called 
from some other file and doesn't allow us to keep the function itself 
static or anything like that).

It then just replaces it with a bit that says "prune or not", and if not 
pruning, every commit gets TREECHANGE.

That in turn means that

 - if (!revs->prune_fn || (flags & TREECHANGE))
 - if (revs->prune_fn && !(flags & TREECHANGE))

just become

 - if (flags & TREECHANGE)
 - if (!(flags & TREECHANGE))

respectively.

Together with adding the "single_parent()" helper function, the "complex" 
conditional now becomes

	if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
		continue;

Anyway, do with this as you will. I think it's ok, but apart from passing 
the test-suite and looking obvious, this has gotten zero testing.

		Linus

---
 builtin-log.c      |    6 ++----
 builtin-rev-list.c |   14 +++++++-------
 commit.h           |    5 +++++
 revision.c         |   20 ++++++++++++--------
 revision.h         |    5 +----
 5 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 981f388..76c84e2 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -92,10 +92,8 @@ static int estimate_commit_count(struct rev_info *rev, struct commit_list *list)
 		list = list->next;
 		if (flags & UNINTERESTING)
 			continue;
-		if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
-			if (commit->parents && !commit->parents->next)
-				continue;
-		}
+		if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
+			continue;
 		n++;
 	}
 	return n;

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 6970467..2dec887 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c

@@ -142,7 +142,7 @@ static int count_distance(struct commit_list *entry)
 
 		if (commit->object.flags & (UNINTERESTING | COUNTED))
 			break;
-		if (!revs.prune_fn || (commit->object.flags & TREECHANGE))
+		if (commit->object.flags & TREECHANGE)
 			nr++;
 		commit->object.flags |= COUNTED;
 		p = commit->parents;

@@ -198,7 +198,7 @@ static inline int halfway(struct commit_list *p, int nr)
 	/*
 	 * Don't short-cut something we are not going to return!
 	 */
-	if (revs.prune_fn && !(p->item->object.flags & TREECHANGE))
+	if (!(p->item->object.flags & TREECHANGE))
 		return 0;
 	if (DEBUG_BISECT)
 		return 0;

@@ -268,7 +268,7 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
 		int distance;
 		unsigned flags = p->item->object.flags;
 
-		if (revs.prune_fn && !(flags & TREECHANGE))
+		if (!(flags & TREECHANGE))
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)

@@ -308,7 +308,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
 		int distance;
 		unsigned flags = p->item->object.flags;
 
-		if (revs.prune_fn && !(flags & TREECHANGE))
+		if (!(flags & TREECHANGE))
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)

@@ -362,7 +362,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		p->item->util = &weights[n++];
 		switch (count_interesting_parents(commit)) {
 		case 0:
-			if (!revs.prune_fn || (flags & TREECHANGE)) {
+			if (flags & TREECHANGE) {
 				weight_set(p, 1);
 				counted++;
 				show_list("bisection 2 count one",

@@ -435,7 +435,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			 * add one for p itself if p is to be counted,
 			 * otherwise inherit it from q directly.
 			 */
-			if (!revs.prune_fn || (flags & TREECHANGE)) {
+			if (flags & TREECHANGE) {
 				weight_set(p, weight(q)+1);
 				counted++;
 				show_list("bisection 2 count one",

@@ -482,7 +482,7 @@ static struct commit_list *find_bisection(struct commit_list *list,
 			continue;
 		p->next = last;
 		last = p;
-		if (!revs.prune_fn || (flags & TREECHANGE))
+		if (flags & TREECHANGE)
 			nr++;
 		on_list++;
 	}

diff --git a/commit.h b/commit.h
index 4ed0c1c..aa67986 100644
--- a/commit.h
+++ b/commit.h

@@ -117,4 +117,9 @@ extern int interactive_add(void);
 extern void add_files_to_cache(int verbose, const char *prefix, const char **files);
 extern int rerere(void);
 
+static inline int single_parent(struct commit *commit)
+{
+	return commit->parents && !commit->parents->next;
+}
+
 #endif /* COMMIT_H */

diff --git a/revision.c b/revision.c
index 5d6f208..7a1ecba 100644
--- a/revision.c
+++ b/revision.c

@@ -308,6 +308,14 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 	struct commit_list **pp, *parent;
 	int tree_changed = 0, tree_same = 0;
 
+	/*
+	 * If we don't do pruning, everything is interesting
+	 */
+	if (!revs->prune) {
+		commit->object.flags |= TREECHANGE;
+		return;
+	}
+
 	if (!commit->tree)
 		return;
 

@@ -415,8 +423,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit, str
 	 * simplify the commit history and find the parent
 	 * that has no differences in the path set if one exists.
 	 */
-	if (revs->prune_fn)
-		revs->prune_fn(revs, commit);
+	try_to_simplify_commit(revs, commit);
 
 	if (revs->no_walk)
 		return 0;

@@ -684,9 +691,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->skip_count = -1;
 	revs->max_count = -1;
 
-	revs->prune_fn = NULL;
-	revs->prune_data = NULL;
-
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
 	diff_setup(&revs->diffopt);

@@ -1271,7 +1275,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		diff_tree_setup_paths(revs->prune_data, &revs->pruning);
 		/* Can't prune commits with rename following: the paths change.. */
 		if (!revs->diffopt.follow_renames)
-			revs->prune_fn = try_to_simplify_commit;
+			revs->prune = 1;
 		if (!revs->full_diff)
 			diff_tree_setup_paths(revs->prune_data, &revs->diffopt);
 	}

@@ -1412,7 +1416,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 		return commit_ignore;
 	if (!commit_match(commit, revs))
 		return commit_ignore;
-	if (revs->prune_fn && revs->dense) {
+	if (revs->prune && revs->dense) {
 		/* Commit without changes? */
 		if (!(commit->object.flags & TREECHANGE)) {
 			/* drop merges unless we want parenthood */

@@ -1460,7 +1464,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
 		case commit_ignore:
 			continue;
 		case commit_error:
-			return -1;
+			return NULL;
 		default:
 			return commit;
 		}

diff --git a/revision.h b/revision.h
index 2232247..a798514 100644
--- a/revision.h
+++ b/revision.h

@@ -15,8 +15,6 @@
 struct rev_info;
 struct log_info;
 
-typedef void (prune_fn_t)(struct rev_info *revs, struct commit *commit);
-
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;

@@ -28,12 +26,11 @@ struct rev_info {
 	/* Basic information */
 	const char *prefix;
 	void *prune_data;
-	prune_fn_t *prune_fn;
-
 	unsigned int early_output;
 
 	/* Traversal flags */
 	unsigned int	dense:1,
+			prune:1,
 			no_merges:1,
 			no_walk:1,
 			remove_empty_trees:1,

On Mon, 5 Nov 2007, Linus Torvalds wrote:

Here's a possible cleanup patch. It's on top of the enhanced 
--early-output format commit, and in fact fixes a stupid bug in that 
commit ("return -1" vs "return NULL"), but that bug-fix is really an 
independent thing.

.. and this extends a bit further on the notion.

It basically means that "rev->dense" can now be ignored outside of 
revision.c, because we'll just set TREECHANGE automatically when 
seeing a non-merge regular commit when --sparse is being used.

So it's not just a simplification, it's a performance optimization too! 

Although since nobody sane would ever use --sparse, I guess nobody really 
cares.

		Linus

---
 builtin-log.c |    8 ++------
 revision.c    |    9 +++++++++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 76c84e2..d6845bc 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -88,13 +88,9 @@ static int estimate_commit_count(struct rev_info *rev, struct commit_list *list)
 	while (list) {
 		struct commit *commit = list->item;
 		unsigned int flags = commit->object.flags;
-
 		list = list->next;
-		if (flags & UNINTERESTING)
-			continue;
-		if (!(flags & TREECHANGE) && rev->dense && single_parent(commit))
-			continue;
-		n++;
+		if ((flags & TREECHANGE) && !(flags & UNINTERESTING))
+			n++;
 	}
 	return n;
 }

diff --git a/revision.c b/revision.c
index 7a1ecba..02e9241 100644
--- a/revision.c
+++ b/revision.c

@@ -325,6 +325,15 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 		return;
 	}
 
+	/*
+	 * Normal non-merge commit? If we don't want to make the 
+	 * history dense, we consider it always to be a change..
+	 */
+	if (!revs->dense && !commit->parents->next) {
+		commit->object.flags |= TREECHANGE;
+		return;
+	}
+
 	pp = &commit->parents;
 	while ((parent = *pp) != NULL) {
 		struct commit *p = parent->item;