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

[Jeff King <peff@peff.net>]

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

> >   [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.

> >  	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).