Re: [PATCH 3/3] show: turn on rename progress

On Thu, Mar 24, 2011 at 04:03:16PM -0700, Junio C Hamano wrote:

quoted

  [1/4]: pager: save the original stderr when redirecting to pager
  [2/4]: progress: use pager's original_stderr if available
  [3/4]: show: turn on rename detection progress reporting
  [4/4]: diff: turn on rename detection progress reporting

Thanks, but why does it affect t0101 and many others...?

Because I'm an idiot who, despite manually testing the option-parsing
part of the code a million times, didn't actually run the full suite.

quoted

 	while ((commit = get_revision(rev)) != NULL) {
-		if (!log_tree_commit(rev, commit) &&
+		int showed = log_tree_commit(rev, commit);
+		if (showed &&
 		    rev->max_count >= 0)

Ugh. Of course, it should be:

diff --git a/builtin/log.c b/builtin/log.c
index 4d52e99..b19e10d 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -275,7 +275,7 @@ static int cmd_log_walk(struct rev_info *rev)
 	 */
 	while ((commit = get_revision(rev)) != NULL) {
 		int showed = log_tree_commit(rev, commit);
-		if (showed &&
+		if (!showed &&
 		    rev->max_count >= 0)
 			/*
 			 * We decremented max_count in get_revision,

on top.

After looking at the implementation of log_tree_commit(), shouldn't this
part be more like this?

	int shown = log_tree_commit(rev, commit);
        if (!shown && rev->max_count >=0)
        	rev->max_count++;
	if (shown)
        	rev->diffopt.show_rename_progress = 0;

Yes. You shouldn't even need to look at log_tree_commit; you can see in
the hunk of my patch that I accidentally inverted the unrelated
max_count conditional while factoring out the call.

I'll go find a brown paper bag now.

-Peff

PS I assume you can squash in the above, or take your version (I don't
care about the verb tense we use, but re-indenting the max_count
conditional as you did is good style).