[PATCH] builtin/diff.c: remove duplicated call to diff_result_code()

[PATCH] builtin/diff.c: remove duplicated call to diff_result_code()

[Junio C Hamano]

The return value from builtin_diff_files() is fed to diff_result_code()
by the caller, and all other callees like builtin_diff_index() do not
have their own call to diff_result_code().  Remove the duplicated one
from builtin_diff_files() and let the caller handle it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This probably does not make a difference right now, but it will at
   the end of the rebased 'rename-degrade-cc-to-c' series.

 builtin/diff.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 4c9deb2..171ac39 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -207,7 +207,6 @@ static void refresh_index_quietly(void)
 
 static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
 {
-	int result;
 	unsigned int options = 0;
 
 	while (1 < argc && argv[1][0] == '-') {
@@ -241,8 +240,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 		perror("read_cache_preload");
 		return -1;
 	}
-	result = run_diff_files(revs, options);
-	return diff_result_code(&revs->diffopt, result);
+	return run_diff_files(revs, options);
 }
 
 int cmd_diff(int argc, const char **argv, const char *prefix)

[PATCH 1/3] diffcore-rename: refactor "too many candidates" logic

[Junio C Hamano]

Move the logic to a separate function, to be enhanced by later patches in
the series.

While at it, swap the condition used in the if statement from "if it is
too big then do this" to "if it would fit then do this".

Signed-off-by: Junio C Hamano <gitster@pobox.com>

---

 Rebased to 'master' as the logic to use the result of this logic was
 updated recently, together with the addition of eye-candy.
---
 diffcore-rename.c |   47 +++++++++++++++++++++++++++++------------------
 1 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index d40e40a..00f7f84 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -419,6 +419,34 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)
 		m[worst] = *o;
 }
 
+static int too_many_rename_candidates(int num_create,
+				      struct diff_options *options)
+{
+	int rename_limit = options->rename_limit;
+	int num_src = rename_src_nr;
+
+	options->needed_rename_limit = 0;
+
+	/*
+	 * This basically does a test for the rename matrix not
+	 * growing larger than a "rename_limit" square matrix, ie:
+	 *
+	 *    num_create * num_src > rename_limit * rename_limit
+	 *
+	 * but handles the potential overflow case specially (and we
+	 * assume at least 32-bit integers)
+	 */
+	if (rename_limit <= 0 || rename_limit > 32767)
+		rename_limit = 32767;
+	if ((num_create <= rename_limit || num_src <= rename_limit) &&
+	    (num_create * num_src <= rename_limit * rename_limit))
+		return 0;
+
+	options->needed_rename_limit =
+		num_src > num_create ? num_src : num_create;
+	return 1;
+}
+
 static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, int copies)
 {
 	int count = 0, i;
@@ -444,7 +472,6 @@ void diffcore_rename(struct diff_options *options)
 {
 	int detect_rename = options->detect_rename;
 	int minimum_score = options->rename_score;
-	int rename_limit = options->rename_limit;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
 	struct diff_score *mx;
@@ -511,24 +538,8 @@ void diffcore_rename(struct diff_options *options)
 	if (!num_create)
 		goto cleanup;
 
-	/*
-	 * This basically does a test for the rename matrix not
-	 * growing larger than a "rename_limit" square matrix, ie:
-	 *
-	 *    num_create * num_src > rename_limit * rename_limit
-	 *
-	 * but handles the potential overflow case specially (and we
-	 * assume at least 32-bit integers)
-	 */
-	options->needed_rename_limit = 0;
-	if (rename_limit <= 0 || rename_limit > 32767)
-		rename_limit = 32767;
-	if ((num_create > rename_limit && num_src > rename_limit) ||
-	    (num_create * num_src > rename_limit * rename_limit)) {
-		options->needed_rename_limit =
-			num_src > num_create ? num_src : num_create;
+	if (too_many_rename_candidates(num_create, options))
 		goto cleanup;
-	}
 
 	if (options->show_rename_progress) {
 		progress = start_progress_delay(
-- 
1.7.4.1.554.gfdad8

[PATCH 2/3] diffcore-rename: record filepair for rename src

[Junio C Hamano]

This will allow us to later skip unmodified entries added due to "-C -C".
We might also want to do something similar to rename_dst side, but that
would only be for the sake of symmetry and not necessary for this series.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diffcore-rename.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 00f7f84..a932f76 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -55,22 +55,23 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,
 
 /* Table of rename/copy src files */
 static struct diff_rename_src {
-	struct diff_filespec *one;
+	struct diff_filepair *p;
 	unsigned short score; /* to remember the break score */
 } *rename_src;
 static int rename_src_nr, rename_src_alloc;
 
-static struct diff_rename_src *register_rename_src(struct diff_filespec *one,
-						   unsigned short score)
+static struct diff_rename_src *register_rename_src(struct diff_filepair *p)
 {
 	int first, last;
+	struct diff_filespec *one = p->one;
+	unsigned short score = p->score;
 
 	first = 0;
 	last = rename_src_nr;
 	while (last > first) {
 		int next = (last + first) >> 1;
 		struct diff_rename_src *src = &(rename_src[next]);
-		int cmp = strcmp(one->path, src->one->path);
+		int cmp = strcmp(one->path, src->p->one->path);
 		if (!cmp)
 			return src;
 		if (cmp < 0) {
@@ -90,7 +91,7 @@ static struct diff_rename_src *register_rename_src(struct diff_filespec *one,
 	if (first < rename_src_nr)
 		memmove(rename_src + first + 1, rename_src + first,
 			(rename_src_nr - first - 1) * sizeof(*rename_src));
-	rename_src[first].one = one;
+	rename_src[first].p = p;
 	rename_src[first].score = score;
 	return &(rename_src[first]);
 }
@@ -205,7 +206,7 @@ static void record_rename_pair(int dst_index, int src_index, int score)
 	if (rename_dst[dst_index].pair)
 		die("internal error: dst already matched.");
 
-	src = rename_src[src_index].one;
+	src = rename_src[src_index].p->one;
 	src->rename_used++;
 	src->count++;
 
@@ -389,7 +390,7 @@ static int find_exact_renames(struct diff_options *options)
 
 	init_hash(&file_table);
 	for (i = 0; i < rename_src_nr; i++)
-		insert_file_table(&file_table, -1, i, rename_src[i].one);
+		insert_file_table(&file_table, -1, i, rename_src[i].p->one);
 
 	for (i = 0; i < rename_dst_nr; i++)
 		insert_file_table(&file_table, 1, i, rename_dst[i].two);
@@ -460,7 +461,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i
 		dst = &rename_dst[mx[i].dst];
 		if (dst->pair)
 			continue; /* already done, either exact or fuzzy. */
-		if (!copies && rename_src[mx[i].src].one->rename_used)
+		if (!copies && rename_src[mx[i].src].p->one->rename_used)
 			continue;
 		record_rename_pair(mx[i].dst, mx[i].src, mx[i].score);
 		count++;
@@ -503,7 +504,7 @@ void diffcore_rename(struct diff_options *options)
 			 */
 			if (p->broken_pair && !p->score)
 				p->one->rename_used++;
-			register_rename_src(p->one, p->score);
+			register_rename_src(p);
 		}
 		else if (detect_rename == DIFF_DETECT_COPY) {
 			/*
@@ -511,7 +512,7 @@ void diffcore_rename(struct diff_options *options)
 			 * one, to indicate ourselves as a user.
 			 */
 			p->one->rename_used++;
-			register_rename_src(p->one, p->score);
+			register_rename_src(p);
 		}
 	}
 	if (rename_dst_nr == 0 || rename_src_nr == 0)
@@ -560,7 +561,7 @@ void diffcore_rename(struct diff_options *options)
 			m[j].dst = -1;
 
 		for (j = 0; j < rename_src_nr; j++) {
-			struct diff_filespec *one = rename_src[j].one;
+			struct diff_filespec *one = rename_src[j].p->one;
 			struct diff_score this_src;
 			this_src.score = estimate_similarity(one, two,
 							     minimum_score);
-- 
1.7.4.1.554.gfdad8

[PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Junio C Hamano]

When there are too many paths in the project, the number of rename source
candidates "git diff -C -C" finds will exceed the rename detection limit,
and no inexact rename detection is performed.  We however could fall back
to "git diff -C" if the number of modified paths is sufficiently small.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/diff-tree.c    |   12 +++++++++++-
 builtin/log.c          |    9 +++++++++
 diff.c                 |   26 ++++++++++++++++++++++++++
 diff.h                 |    2 ++
 diffcore-rename.c      |   38 ++++++++++++++++++++++++++++++++++++--
 merge-recursive.c      |   10 +++-------
 t/t4001-diff-rename.sh |   25 +++++++++++++++++++++++++
 7 files changed, 112 insertions(+), 10 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 0d2a3e9..be6417d 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -163,6 +163,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_stdin) {
+		int saved_nrl = 0;
+		int saved_dcctc = 0;
+
 		if (opt->diffopt.detect_rename)
 			opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
 					       DIFF_SETUP_USE_CACHE);
@@ -173,9 +176,16 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 				fputs(line, stdout);
 				fflush(stdout);
 			}
-			else
+			else {
 				diff_tree_stdin(line);
+				if (saved_nrl < opt->diffopt.needed_rename_limit)
+					saved_nrl = opt->diffopt.needed_rename_limit;
+				if (opt->diffopt.degraded_cc_to_c)
+					saved_dcctc = 1;
+			}
 		}
+		opt->diffopt.degraded_cc_to_c = saved_dcctc;
+		opt->diffopt.needed_rename_limit = saved_nrl;
 	}
 
 	return diff_result_code(&opt->diffopt, 0);
diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..707fd57 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -247,6 +247,8 @@ static void finish_early_output(struct rev_info *rev)
 static int cmd_log_walk(struct rev_info *rev)
 {
 	struct commit *commit;
+	int saved_nrl = 0;
+	int saved_dcctc = 0;
 
 	if (rev->early_output)
 		setup_early_output(rev);
@@ -277,7 +279,14 @@ static int cmd_log_walk(struct rev_info *rev)
 		}
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
+		if (saved_nrl < rev->diffopt.needed_rename_limit)
+			saved_nrl = rev->diffopt.needed_rename_limit;
+		if (rev->diffopt.degraded_cc_to_c)
+			saved_dcctc = 1;
 	}
+	rev->diffopt.degraded_cc_to_c = saved_dcctc;
+	rev->diffopt.needed_rename_limit = saved_nrl;
+
 	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
 	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
 		return 02;
diff --git a/diff.c b/diff.c
index 9b3eb99..13f322b 100644
--- a/diff.c
+++ b/diff.c
@@ -3956,6 +3956,28 @@ static int is_summary_empty(const struct diff_queue_struct *q)
 	return 1;
 }
 
+static const char rename_limit_warning[] =
+"inexact rename detection was skipped due to too many files.";
+
+static const char degrade_cc_to_c_warning[] =
+"only found copies from modified paths due to too many files.";
+
+static const char rename_limit_advice[] =
+"you may want to set your %s variable to at least "
+"%d and retry the command.";
+
+void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
+{
+	if (degraded_cc)
+		warning(degrade_cc_to_c_warning);
+	else if (needed)
+		warning(rename_limit_warning);
+	else
+		return;
+	if (0 < needed && needed < 32767)
+		warning(rename_limit_advice, varname, needed);
+}
+
 void diff_flush(struct diff_options *options)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
@@ -4237,6 +4259,10 @@ void diffcore_std(struct diff_options *options)
 int diff_result_code(struct diff_options *opt, int status)
 {
 	int result = 0;
+
+	diff_warn_rename_limit("diff.renamelimit",
+			       opt->needed_rename_limit,
+			       opt->degraded_cc_to_c);
 	if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
 	    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
 		return status;
diff --git a/diff.h b/diff.h
index 007a055..b8dde8a 100644
--- a/diff.h
+++ b/diff.h
@@ -111,6 +111,7 @@ struct diff_options {
 	int rename_score;
 	int rename_limit;
 	int needed_rename_limit;
+	int degraded_cc_to_c;
 	int show_rename_progress;
 	int dirstat_percent;
 	int setup;
@@ -273,6 +274,7 @@ extern void diffcore_fix_diff_index(struct diff_options *);
 
 extern int diff_queue_is_empty(void);
 extern void diff_flush(struct diff_options*);
+extern void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
 
 /* diff-raw status letters */
 #define DIFF_STATUS_ADDED		'A'
diff --git a/diffcore-rename.c b/diffcore-rename.c
index a932f76..f62587e 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -420,11 +420,18 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)
 		m[worst] = *o;
 }
 
+/*
+ * Returns:
+ * 0 if we are under the limit;
+ * 1 if we need to disable inexact rename detection;
+ * 2 if we would be under the limit if we were given -C instead of -C -C.
+ */
 static int too_many_rename_candidates(int num_create,
 				      struct diff_options *options)
 {
 	int rename_limit = options->rename_limit;
 	int num_src = rename_src_nr;
+	int i;
 
 	options->needed_rename_limit = 0;
 
@@ -445,6 +452,20 @@ static int too_many_rename_candidates(int num_create,
 
 	options->needed_rename_limit =
 		num_src > num_create ? num_src : num_create;
+
+	/* Are we running under -C -C? */
+	if (!DIFF_OPT_TST(options, FIND_COPIES_HARDER))
+		return 1;
+
+	/* Would we bust the limit if we were running under -C? */
+	for (num_src = i = 0; i < rename_src_nr; i++) {
+		if (diff_unmodified_pair(rename_src[i].p))
+			continue;
+		num_src++;
+	}
+	if ((num_create <= rename_limit || num_src <= rename_limit) &&
+	    (num_create * num_src <= rename_limit * rename_limit))
+		return 2;
 	return 1;
 }
 
@@ -476,7 +497,7 @@ void diffcore_rename(struct diff_options *options)
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
 	struct diff_score *mx;
-	int i, j, rename_count;
+	int i, j, rename_count, skip_unmodified = 0;
 	int num_create, num_src, dst_cnt;
 	struct progress *progress = NULL;
 
@@ -539,8 +560,16 @@ void diffcore_rename(struct diff_options *options)
 	if (!num_create)
 		goto cleanup;
 
-	if (too_many_rename_candidates(num_create, options))
+	switch (too_many_rename_candidates(num_create, options)) {
+	case 1:
 		goto cleanup;
+	case 2:
+		options->degraded_cc_to_c = 1;
+		skip_unmodified = 1;
+		break;
+	default:
+		break;
+	}
 
 	if (options->show_rename_progress) {
 		progress = start_progress_delay(
@@ -563,6 +592,11 @@ void diffcore_rename(struct diff_options *options)
 		for (j = 0; j < rename_src_nr; j++) {
 			struct diff_filespec *one = rename_src[j].p->one;
 			struct diff_score this_src;
+
+			if (skip_unmodified &&
+			    diff_unmodified_pair(rename_src[j].p))
+				continue;
+
 			this_src.score = estimate_similarity(one, two,
 							     minimum_score);
 			this_src.name_score = basename_same(one, two);
diff --git a/merge-recursive.c b/merge-recursive.c
index 8e82a8b..3ae4d53 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -22,11 +22,6 @@
 #include "dir.h"
 #include "submodule.h"
 
-static const char rename_limit_advice[] =
-"inexact rename detection was skipped because there were too many\n"
-"  files. You may want to set your merge.renamelimit variable to at least\n"
-"  %d and retry this merge.";
-
 static struct tree *shift_tree_object(struct tree *one, struct tree *two,
 				      const char *subtree_shift)
 {
@@ -1656,8 +1651,9 @@ int merge_recursive(struct merge_options *o,
 		commit_list_insert(h2, &(*result)->parents->next);
 	}
 	flush_output(o);
-	if (o->needed_rename_limit)
-		warning(rename_limit_advice, o->needed_rename_limit);
+	if (show(o, 2))
+		diff_warn_rename_limit("merge.renamelimit",
+				       o->needed_rename_limit, 0);
 	return clean;
 }
 
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 71bac83..301f3a0 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -77,4 +77,29 @@ test_expect_success  'favour same basenames even with minor differences' '
 	git show HEAD:path1 | sed "s/15/16/" > subdir/path1 &&
 	git status | grep "renamed: .*path1 -> subdir/path1"'
 
+test_expect_success 'setup for many rename source candidates' '
+	git reset --hard &&
+	for i in 0 1 2 3 4 5 6 7 8 9;
+	do
+		for j in 0 1 2 3 4 5 6 7 8 9;
+		do
+			echo "$i$j" >"path$i$j"
+		done
+	done &&
+	git add "path??" &&
+	test_tick &&
+	git commit -m "hundred" &&
+	(cat path1; echo new) >new-path &&
+	echo old >>path1 &&
+	git add new-path path1 &&
+	git diff -l 4 -C -C --cached --name-status >actual 2>actual.err &&
+	sed -e "s/^\([CM]\)[0-9]*	/\1	/" actual >actual.munged &&
+	cat >expect <<-EOF &&
+	C	path1	new-path
+	M	path1
+	EOF
+	test_cmp expect actual.munged &&
+	grep warning actual.err
+'
+
 test_done
-- 
1.7.4.1.554.gfdad8

Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Jeff King]

On Tue, Mar 22, 2011 at 02:50:49PM -0700, Junio C Hamano wrote:

> When there are too many paths in the project, the number of rename source
> candidates "git diff -C -C" finds will exceed the rename detection limit,
> and no inexact rename detection is performed.  We however could fall back
> to "git diff -C" if the number of modified paths is sufficiently small.

Hmm. Compared to the previous iteration, this also seems to turn on
warnings for the diff and log families. It would have been a little
easier to review as a separate patch, I think. In particular, I'm not
sure I like the "just warn once at the end" strategy when we show
multiple diffs.

For example, in the trash repo created by your new t4001, I did:

  git commit -a -m foo &&
  git config diff.renamelimit 4 &&
  git log --raw -C -C

And like most users, I read through the commits in the order they're
presented. So I get to the second one and see that it has a bunch of
additions, but no copies. Then I keep reading and finally, after the
third commit, I see a warning that we didn't do copy detection.

Here's what I see wrong with that from the user's perspective:

  1. You give me the warning at some unspecified time after I've already
     read and digested the results of the commit it affects. So at best
     I've wasted my time looking at results that you later tell me might
     be bogus.

  2. The warning is at the very end of the potentially long output. With
     three commits, I'm likely to actually scroll all the way to the end.
     But how often do you run "git log" in git.git and then kill the
     pager after finding the answer you want, never seeing the bottom of
     git's output (and potentially before git even generates it!). So I
     may miss the warning entirely.

  3. Even if I do see the warning, I have no idea which commits it
     applies to. I have to bump the rename limit and re-run just to find
     out whether the commits I cared about were affected.

So I think it would be much more sensible to show something like:

  commit f3ba51f2e2025d15c84873cb24dcf51b77f6b8e1
  Author: A U Thor <author@example.com>
  Date:   Thu Apr 7 15:13:13 2005 -0700

      hundred

  warning: only found copies from modified paths due to too many files.
  warning: you may want to set your diff.renamelimit variable to at
           least 102 and retry the command.
  :000000 100644 0000000... 4daddb7... A  path00
  :000000 100644 0000000... 8a0f05e... A  path01
  [etc]

One argument against that is that we may end up printing many such
warnings. For the pager case, where stderr and stdout are combined, that
is probably fine. Any commit which blows the rename limit is going to
have a ton of diff output anyway, so a few extra lines doesn't hurt
(with some careful flushing, those lines are placed in the right spot).

When stderr is separated, though, it's going to be annoyingly verbose,
and multiple warnings wouldn't match up with their respective commits,
anyway.

So maybe it is worth detecting the pager case and behaving differently
based on whether stderr and stdout are combined.

Another alternative would be to embed the warning in the stdout stream,
but I don't see a clean way of doing that. The best I could come up with
is something like:

    commit f3ba51f2e2025d15c84873cb24dcf51b77f6b8e1
    Author: A U Thor <author@example.com>
    Date:   Thu Apr 7 15:13:13 2005 -0700
    X-Diff-Warning: only found copies...

which just seems a little too hack-ish.

Also, on a somewhat related note: which commands should have rename
progress reporting turned on? It makes sense for "git diff" to do it to
me. And probably even "git show". Probably not for "git log", though.
I think we'd also have to look at how that interacts with the
stderr-to-stdout pager thing. We obviously don't want progress going to
the pager.

> +static const char rename_limit_warning[] =
> +"inexact rename detection was skipped due to too many files.";
> +
> +static const char degrade_cc_to_c_warning[] =
> +"only found copies from modified paths due to too many files.";

Somehow the phrase "due to too many files" seems awkward. The
merge-recursive warning this replaces used "because there were too many
files". Which is longer, of course, but seems more natural.

> +static const char rename_limit_advice[] =
> +"you may want to set your %s variable to at least "
> +"%d and retry the command.";

And this one ends up overflowing reasonably-sized terminals, because it
gets prefixed with "warning: ", and because the variable name is
something like "diff.renamelimit".

> @@ -445,6 +452,20 @@ static int too_many_rename_candidates(int num_create,
>  
>  	options->needed_rename_limit =
>  		num_src > num_create ? num_src : num_create;

This is obviously not introduced by your patch, but I noticed once again
during testing that this number is conservatively high, isn't it? I
think the number we want is actually:

  ceil(sqrt(num_create * num_src))

right? I don't know if it is worth being more accurate.

> @@ -1656,8 +1651,9 @@ int merge_recursive(struct merge_options *o,
>  		commit_list_insert(h2, &(*result)->parents->next);
>  	}
>  	flush_output(o);
> -	if (o->needed_rename_limit)
> -		warning(rename_limit_advice, o->needed_rename_limit);
> +	if (show(o, 2))
> +		diff_warn_rename_limit("merge.renamelimit",
> +				       o->needed_rename_limit, 0);

With the call to show(), we are showing the warning only for the
outermost diff. Which is probably way better than generating a bunch of
warnings, one for each rename-detect we do. But aren't we now failing to
mention limits we hit in recursive invocations, even though they can
affect the results? In other words, isn't this a candidate for the
"find the highest limit needed and report once" strategy you used above?

-Peff

Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> "find the highest limit needed and report once" strategy you used above?

Wasn't "find the highest limit" your invention in merge-recursive?  The
only deliberate fix in merge-recursive codepath on top of what you did is
to allow squelching the warning when verbosity is set to more quiet than
the default, and removal of the "set var to this value" advice when the
value we are going to suggest is too high already and will not help.

As to the warnings in "log" output, I actually prefer leaving saved_* out
and showing the warning per internal diff invocation. My initial iteration
was indeed coded that way, and I did the "find the highest" only to mimic
what was already in merge-recursive.

Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Jeff King]

On Wed, Mar 23, 2011 at 09:41:19AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > "find the highest limit needed and report once" strategy you used above?
> 
> Wasn't "find the highest limit" your invention in merge-recursive?  The

Hmm, you're right. I was thinking the code you were modifying was called
per-diff, but it's not. Though looking at it again, I think we could
technically still warn several times, one for each recursive call to
merge_recursive. Your call to show() does fix that (because it checks
o->call_depth).

So I think the code after your patch does the right thing.

> As to the warnings in "log" output, I actually prefer leaving saved_* out
> and showing the warning per internal diff invocation. My initial iteration
> was indeed coded that way, and I did the "find the highest" only to mimic
> what was already in merge-recursive.

I think they are two different cases, because the user never gets to see
the intermediate results of merge-recursive. That is, at the end of the
merge we tell them "here's the result, and by the way, we limited
renames." But for something like "log" or "diff-tree --stdin", it is
about doing N independent diffs, and the user gets to see the result of
each. So if we can match the warnings to the actual diffs in the output,
that is much more useful.

But in the case of something like:

  git rev-list HEAD | git diff-tree --stdin >foo.out

I don't think there is a way to match the warnings to their respective
commits. They are on two different streams, and putting the warning to
stdout would pollute the output.

-Peff

Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Jeff King]

On Wed, Mar 23, 2011 at 11:58:54AM -0400, Jeff King wrote:

> Also, on a somewhat related note: which commands should have rename
> progress reporting turned on? It makes sense for "git diff" to do it to
> me. And probably even "git show". Probably not for "git log", though.
> I think we'd also have to look at how that interacts with the
> stderr-to-stdout pager thing. We obviously don't want progress going to
> the pager.

This quick-and-dirty series makes rename progress work properly for "git
show". Patch 1/3 would also be the building block for having "git log"
show per-commit warnings when stderr and stdout are mixed, but a single
warning otherwise. I built it on top of your patches, but I think there
is no reason it could not be done in parallel.

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

-Peff

[PATCH 1/3] pager: save the original stderr when redirecting to pager

[Jeff King]

When we redirect stdout to the pager, we also redirect
stderr (if it would otherwise go to the terminal) so that
error messages do not get overwritten by the pager.

However, some stderr output may still want to go to the
terminal, because they are time-sensitive (like progress
reports) and should be overwritten by the pager.

This patch stashes away the original stderr descriptor and
creates a new stdio buffer for it.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |    2 ++
 pager.c |    7 ++++++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index 872bc9b..a6ce524 100644
--- a/cache.h
+++ b/cache.h
@@ -1067,6 +1067,8 @@ extern int pager_use_color;
 extern const char *editor_program;
 extern const char *askpass_program;
 extern const char *excludes_file;
+extern FILE *original_stderr;
+extern int original_stderr_fd;
 
 /* base85 */
 int decode_85(char *dst, const char *line, int linelen);
diff --git a/pager.c b/pager.c
index dac358f..701926f 100644
--- a/pager.c
+++ b/pager.c
@@ -12,6 +12,8 @@
  */
 
 static int spawned_pager;
+FILE *original_stderr;
+int original_stderr_fd = -1;
 
 #ifndef WIN32
 static void pager_preexec(void)
@@ -97,8 +99,11 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		original_stderr_fd = dup(2);
+		original_stderr = fdopen(original_stderr_fd, "w");
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
1.7.4.39.ge4c30

[PATCH 2/3] progress: use pager's original_stderr if available

[Jeff King]

If we are outputting to a pager, stderr is redirected to the
pager. However, progress messages should not be part of that
stream, as they are time-sensitive and should end up being
hidden once we actually have output.

Signed-off-by: Jeff King <peff@peff.net>
---
 progress.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 3971f49..bc7d3a3 100644
--- a/progress.c
+++ b/progress.c
@@ -10,6 +10,7 @@
 
 #include "git-compat-util.h"
 #include "progress.h"
+#include "cache.h"
 
 #define TP_IDX_MAX      8
 
@@ -71,6 +72,7 @@ static void clear_progress_signal(void)
 
 static int display(struct progress *progress, unsigned n, const char *done)
 {
+	FILE *out = original_stderr ? original_stderr : stderr;
 	const char *eol, *tp;
 
 	if (progress->delay) {
@@ -95,16 +97,16 @@ static int display(struct progress *progress, unsigned n, const char *done)
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
+			fprintf(out, "%s: %3u%% (%u/%u)%s%s",
 				progress->title, percent, n,
 				progress->total, tp, eol);
-			fflush(stderr);
+			fflush(out);
 			progress_update = 0;
 			return 1;
 		}
 	} else if (progress_update) {
-		fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol);
-		fflush(stderr);
+		fprintf(out, "%s: %u%s%s", progress->title, n, tp, eol);
+		fflush(out);
 		progress_update = 0;
 		return 1;
 	}
@@ -211,11 +213,12 @@ int display_progress(struct progress *progress, unsigned n)
 struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay)
 {
+	FILE *out = original_stderr ? original_stderr : stderr;
 	struct progress *progress = malloc(sizeof(*progress));
 	if (!progress) {
 		/* unlikely, but here's a good fallback */
-		fprintf(stderr, "%s...\n", title);
-		fflush(stderr);
+		fprintf(out, "%s...\n", title);
+		fflush(out);
 		return NULL;
 	}
 	progress->title = title;
-- 
1.7.4.39.ge4c30

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

[Jeff King]

For large commits, it is nice to have some eye candy for the
rename detection.

This makes sense for "show", which is showing a single diff,
since the progress reporting will be shown before the pager
shows any output.

We don't want to do the same for "log", though, because the
progress would be interspersed with the various commits.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 707fd57..cf68130 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -416,6 +416,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.always_show_header = 1;
 	rev.no_walk = 1;
+	rev.diffopt.show_rename_progress = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 	opt.tweak = show_rev_tweak_rev;
-- 
1.7.4.39.ge4c30

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For large commits, it is nice to have some eye candy for the
> rename detection.
>
> This makes sense for "show", which is showing a single diff,
> since the progress reporting will be shown before the pager
> shows any output.
>
> We don't want to do the same for "log", though, because the
> progress would be interspersed with the various commits.

I understand that you said q&d, but these days since f222abd (Make 'git
show' more useful, 2009-07-13), show does walk when it is asked to.

"git show master maint" will also be affected with this, no?

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/log.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 707fd57..cf68130 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -416,6 +416,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  	rev.diff = 1;
>  	rev.always_show_header = 1;
>  	rev.no_walk = 1;
> +	rev.diffopt.show_rename_progress = 1;
>  	memset(&opt, 0, sizeof(opt));
>  	opt.def = "HEAD";
>  	opt.tweak = show_rev_tweak_rev;

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

[Jeff King]

On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:

> > This makes sense for "show", which is showing a single diff,
> > since the progress reporting will be shown before the pager
> > shows any output.
> >
> > We don't want to do the same for "log", though, because the
> > progress would be interspersed with the various commits.
> 
> I understand that you said q&d, but these days since f222abd (Make 'git
> show' more useful, 2009-07-13), show does walk when it is asked to.
> 
> "git show master maint" will also be affected with this, no?

Ick, I didn't think of that. So yeah, that patch as-is is definitely no
good. I think the most sensible thing to do is to show progress only
until we actually generate some output. That handles both the walking
case and the "git show master maint" case.

Something like this:

diff --git a/builtin/log.c b/builtin/log.c
index 707fd57..d652acc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -265,13 +265,16 @@ static int cmd_log_walk(struct rev_info *rev)
 	 * retain that state information if replacing rev->diffopt in this loop
 	 */
 	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)
 			/*
 			 * We decremented max_count in get_revision,
 			 * but we didn't actually show the commit.
 			 */
 			rev->max_count++;
+		if (showed && rev->diffopt.show_rename_progress)
+			rev->diffopt.show_rename_progress = 0;
 		if (!rev->reflog_info) {
 			/* we allow cycles in reflog ancestry */
 			free(commit->buffer);
@@ -416,6 +419,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.always_show_header = 1;
 	rev.no_walk = 1;
+	rev.diffopt.show_rename_progress = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 	opt.tweak = show_rev_tweak_rev;

We could also turn it on for "git log" in that case, though it is only
useful if the first commit happens to be the one that is slow.

I should also turn it on for "git diff". I'll prepare a cleaner series
with that in it, too.

What about the degrade-cc-to-c warnings? Are you working on another
revision, or should I re-roll your changes on top of my series, handling
the "one-warning-per-commit" behavior I suggested when stdout and stderr
are combined?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:
>
> We could also turn it on for "git log" in that case, though it is only
> useful if the first commit happens to be the one that is slow.
>
> I should also turn it on for "git diff". I'll prepare a cleaner series
> with that in it, too.

Sounds good, thanks.

> What about the degrade-cc-to-c warnings? Are you working on another
> revision, or should I re-roll your changes on top of my series, handling
> the "one-warning-per-commit" behavior I suggested when stdout and stderr
> are combined?

Also sounds good, thanks.  No, I am spending far more time on ushering
stalled topics than looking at my weatherbaloons and no time left for the
latter.

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

[Jeff King]

On Thu, Mar 24, 2011 at 08:00:38AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:
> >
> > We could also turn it on for "git log" in that case, though it is only
> > useful if the first commit happens to be the one that is slow.
> >
> > I should also turn it on for "git diff". I'll prepare a cleaner series
> > with that in it, too.
> 
> Sounds good, thanks.

Here it is:

  [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

> > What about the degrade-cc-to-c warnings? Are you working on another
> > revision, or should I re-roll your changes on top of my series, handling
> > the "one-warning-per-commit" behavior I suggested when stdout and stderr
> > are combined?
> 
> Also sounds good, thanks.  No, I am spending far more time on ushering
> stalled topics than looking at my weatherbaloons and no time left for the
> latter.

I thought this would be simple on top of 1/4 above, but it's not. In
some cases we want to insert the warning before the diff output, and in
some cases we want to know afterwards what happened. Which means we
probably need to carry a new "show the warning" flag in diffopt and
respect it in the rename, but also fill out the needed_rename_limit.

-Peff

[PATCH 1/4] pager: save the original stderr when redirecting to pager

[Jeff King]

When we redirect stdout to the pager, we also redirect
stderr (if it would otherwise go to the terminal) so that
error messages do not get overwritten by the pager.

However, some stderr output may still want to go to the
terminal, because they are time-sensitive (like progress
reports) and should be overwritten by the pager.

This patch stashes away the original stderr descriptor and
creates a new stdio buffer for it.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as the last iteration.

 cache.h |    2 ++
 pager.c |    7 ++++++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index f765cf5..73da139 100644
--- a/cache.h
+++ b/cache.h
@@ -1068,6 +1068,8 @@ extern int pager_use_color;
 extern const char *editor_program;
 extern const char *askpass_program;
 extern const char *excludes_file;
+extern FILE *original_stderr;
+extern int original_stderr_fd;
 
 /* base85 */
 int decode_85(char *dst, const char *line, int linelen);
diff --git a/pager.c b/pager.c
index dac358f..701926f 100644
--- a/pager.c
+++ b/pager.c
@@ -12,6 +12,8 @@
  */
 
 static int spawned_pager;
+FILE *original_stderr;
+int original_stderr_fd = -1;
 
 #ifndef WIN32
 static void pager_preexec(void)
@@ -97,8 +99,11 @@ void setup_pager(void)
 
 	/* original process continues, but writes to the pipe */
 	dup2(pager_process.in, 1);
-	if (isatty(2))
+	if (isatty(2)) {
+		original_stderr_fd = dup(2);
+		original_stderr = fdopen(original_stderr_fd, "w");
 		dup2(pager_process.in, 2);
+	}
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
1.7.4.41.g423da

[PATCH 2/4] progress: use pager's original_stderr if available

[Jeff King]

If we are outputting to a pager, stderr is redirected to the
pager. However, progress messages should not be part of that
stream, as they are time-sensitive and would end up being
hidden once we actually have output.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as the last iteration.

 progress.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 3971f49..bc7d3a3 100644
--- a/progress.c
+++ b/progress.c
@@ -10,6 +10,7 @@
 
 #include "git-compat-util.h"
 #include "progress.h"
+#include "cache.h"
 
 #define TP_IDX_MAX      8
 
@@ -71,6 +72,7 @@ static void clear_progress_signal(void)
 
 static int display(struct progress *progress, unsigned n, const char *done)
 {
+	FILE *out = original_stderr ? original_stderr : stderr;
 	const char *eol, *tp;
 
 	if (progress->delay) {
@@ -95,16 +97,16 @@ static int display(struct progress *progress, unsigned n, const char *done)
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
 			progress->last_percent = percent;
-			fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
+			fprintf(out, "%s: %3u%% (%u/%u)%s%s",
 				progress->title, percent, n,
 				progress->total, tp, eol);
-			fflush(stderr);
+			fflush(out);
 			progress_update = 0;
 			return 1;
 		}
 	} else if (progress_update) {
-		fprintf(stderr, "%s: %u%s%s", progress->title, n, tp, eol);
-		fflush(stderr);
+		fprintf(out, "%s: %u%s%s", progress->title, n, tp, eol);
+		fflush(out);
 		progress_update = 0;
 		return 1;
 	}
@@ -211,11 +213,12 @@ int display_progress(struct progress *progress, unsigned n)
 struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay)
 {
+	FILE *out = original_stderr ? original_stderr : stderr;
 	struct progress *progress = malloc(sizeof(*progress));
 	if (!progress) {
 		/* unlikely, but here's a good fallback */
-		fprintf(stderr, "%s...\n", title);
-		fflush(stderr);
+		fprintf(out, "%s...\n", title);
+		fflush(out);
 		return NULL;
 	}
 	progress->title = title;
-- 
1.7.4.41.g423da

[PATCH 3/4] show: turn on rename detection progress reporting

[Jeff King]

For large commits, it is nice to have some eye candy for the
rename detection.

However, because show can display multiple commits, we have
to be careful not to clutter existing output. We show the
progress report only before we have generated any actual
output; once we have sent output to the terminal or pager,
we turn off progress reporting.

This also makes it safe to use with "git log", though it
will only be useful if the first commit is the slow one.
So this patch actually enables it for all of the
log/whatchanged/show/reflog family.

We also handle the usual --{no-}progress option and check
that stderr goes to a terminal before turning on progress.

Signed-off-by: Jeff King <peff@peff.net>
---
Changes since the last iteration:

  - only show progress for the first commit
  - check isatty(2)
  - --{no-}progress

 Documentation/git-log.txt |    7 +++++++
 builtin/log.c             |   17 ++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2c84028..c0f763e 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -73,6 +73,13 @@ produced by --stat etc.
 	to be prefixed with "\-- " to separate them from options or
 	refnames.
 
+--no-progress::
+--progress::
+	Disable or enable progress reporting during long computations;
+	the default is to enable progress reporting when stderr is a
+	terminal. Currently the only computation with progress support
+	is inexact rename detection.
+
 include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..4d52e99 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -27,6 +27,7 @@ static int default_show_root = 1;
 static int decoration_style;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
+static int progress = -1;
 
 static const char * const builtin_log_usage =
 	"git log [<options>] [<since>..<until>] [[--] <path>...]\n"
@@ -109,10 +110,17 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 			rev->show_source = 1;
 		} else if (!strcmp(arg, "-h")) {
 			usage(builtin_log_usage);
+		} else if (!strcmp(arg, "--progress")) {
+			progress = 1;
+		} else if (!strcmp(arg, "--no-progress")) {
+			progress = 0;
 		} else
 			die("unrecognized argument: %s", arg);
 	}
 
+	if (progress == -1)
+		progress = isatty(2);
+
 	/*
 	 * defeat log.decorate configuration interacting with --pretty=raw
 	 * from the command line.
@@ -257,19 +265,26 @@ static int cmd_log_walk(struct rev_info *rev)
 	if (rev->early_output)
 		finish_early_output(rev);
 
+	if (progress)
+		rev->diffopt.show_rename_progress = 1;
+
 	/*
 	 * For --check and --exit-code, the exit code is based on CHECK_FAILED
 	 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
 	 * retain that state information if replacing rev->diffopt in this loop
 	 */
 	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)
 			/*
 			 * We decremented max_count in get_revision,
 			 * but we didn't actually show the commit.
 			 */
 			rev->max_count++;
+		/* Once we have output, progress will clutter the terminal. */
+		if (showed)
+			rev->diffopt.show_rename_progress = 0;
 		if (!rev->reflog_info) {
 			/* we allow cycles in reflog ancestry */
 			free(commit->buffer);
-- 
1.7.4.41.g423da

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>  	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)
>  			/*
>  			 * We decremented max_count in get_revision,
>  			 * but we didn't actually show the commit.
>  			 */
>  			rev->max_count++;
> +		/* Once we have output, progress will clutter the terminal. */
> +		if (showed)
> +			rev->diffopt.show_rename_progress = 0;

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;

[PATCH 4/4] diff: turn on rename detection progress reporting

[Jeff King]

Since all of the progress happens before we generate any
output, this looks OK, even when output goes to a pager.
We do the usual --progress/--no-progress options and check
isatty(2) to enable the feature.

The argument parsing is a little ad-hoc, but we currently
have no parse-options infrastructure here at all.  However,
it should be safe to parse like this, because the prior
call to setup_revisions will have removed any options that
take an argument, and our parsing removes --progress from
argv for later parsers. The one exception is diff_no_index,
which may get called early, and needs to learn to ignore
--progress.

Signed-off-by: Jeff King <peff@peff.net>
---
New since the last iteration.

I'm not happy about the option parsing, but converting the whole of "git
diff" to a saner parsing structure is more than I want to take on for
this series. One thing that would make it easier is if the diff parser
understood --progress and stored the result for the caller to check. I
was tempted to do that because the code ends up cleaner, but it feels
wrong.

 Documentation/git-diff.txt |    7 +++++++
 builtin/diff.c             |   28 ++++++++++++++++++++++++++++
 diff-no-index.c            |    2 ++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index f8d0819..dbebc93 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -87,6 +87,13 @@ OPTIONS
 :git-diff: 1
 include::diff-options.txt[]
 
+--no-progress::
+--progress::
+	Disable or enable progress reporting during long computations;
+	the default is to enable progress reporting when stderr is a
+	terminal. Currently the only computation with progress support
+	is inexact rename detection.
+
 <path>...::
 	The <paths> parameters, when given, are used to limit
 	the diff to the named paths (you can give directory
diff --git a/builtin/diff.c b/builtin/diff.c
index 4c9deb2..82ecc1d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -255,6 +255,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	struct blobinfo blob[2];
 	int nongit;
 	int result = 0;
+	int progress = -1;
+	int unknown_argc, parsed_argc;
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -307,6 +309,32 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			die("diff_setup_done failed");
 	}
 
+	parsed_argc = 0;
+	for (unknown_argc = i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--") || arg[0] != '-') {
+			int j;
+			for (j = i; j < argc; j++)
+				argv[unknown_argc++] = argv[j];
+			break;
+		}
+		else if (!strcmp(argv[i], "--progress"))
+			progress = 1;
+		else if (!strcmp(argv[i], "--no-progress"))
+			progress = 0;
+		else {
+			argv[unknown_argc++] = argv[i];
+			continue;
+		}
+		parsed_argc++;
+	}
+	argc -= parsed_argc;
+
+	if (progress == -1)
+		progress = isatty(2);
+	if (progress)
+		rev.diffopt.show_rename_progress = 1;
+
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
 	/*
diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..42cb413 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -212,6 +212,8 @@ void diff_no_index(struct rev_info *revs,
 			options |= DIFF_SILENT_ON_REMOVED;
 			i++;
 		}
+		else if (!strcmp(argv[i], "--progress"))
+			; /* handled elsewhere */
 		else if (!strcmp(argv[i], "--"))
 			i++;
 		else {
-- 
1.7.4.41.g423da

Re: [PATCH 4/4] diff: turn on rename detection progress reporting

[Johannes Sixt]

Am 3/24/2011 18:51, schrieb Jeff King:
> Since all of the progress happens before we generate any
> output, this looks OK, even when output goes to a pager.
> We do the usual --progress/--no-progress options and check
> isatty(2) to enable the feature.

Why does it look good? Because the pager is not spawned, yet? Then this is
not so good because on Windows we don't have a facility to wait until
there is output, and for this reason we spawn the pager immediately.

-- Hannes

Re: [PATCH 4/4] diff: turn on rename detection progress reporting

[Jeff King]

On Fri, Mar 25, 2011 at 09:35:38AM +0100, Johannes Sixt wrote:

> Am 3/24/2011 18:51, schrieb Jeff King:
> > Since all of the progress happens before we generate any
> > output, this looks OK, even when output goes to a pager.
> > We do the usual --progress/--no-progress options and check
> > isatty(2) to enable the feature.
> 
> Why does it look good? Because the pager is not spawned, yet? Then this is
> not so good because on Windows we don't have a facility to wait until
> there is output, and for this reason we spawn the pager immediately.

It depends on your pager. Less is careful not to produce any output
until it has something to show, for exactly this reason. So it looks
fine even if the pager has been started[1].

But if you have your pager set to "tig" (or you have manually piped to
it), it looks awful. And something like "git show $branch | tig" is ugly
even on Unix.

I'm not sure what the right solution is. We can add a config option like
pager.progress, but it seems like something the user shouldn't have to
care about and set manually. And that doesn't help when the user has
manually invoked a pager that takes over the terminal, like "git log |
tig".

-Peff

[1] Actually, there is a slight bug in my patch; if your pager has been
started, our isatty(2) test fails. So you get no progress for "git show"
on Windows, anyway, unless you use "--progress". "git diff", on the
other hand, starts the pager afterwards, so it does work.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 24, 2011 at 08:00:38AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:
>> >
>> > We could also turn it on for "git log" in that case, though it is only
>> > useful if the first commit happens to be the one that is slow.
>> >
>> > I should also turn it on for "git diff". I'll prepare a cleaner series
>> > with that in it, too.
>> 
>> Sounds good, thanks.
>
> Here it is:
>
>   [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...?


t/jk/progress-with-pager$ sh t0101-at-syntax.sh -i -v
Initialized empty Git repository in /git/git.git/t/trash
directory.t0101-at-syntax/.git/
expecting success: 
        test_commit one &&
        test_commit two

[master (root-commit) d79ce16] one
 Author: A U Thor <author@example.com>
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 one.t
[master 139b20d] two
 Author: A U Thor <author@example.com>
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 two.t
ok 1 - setup

expecting success: 
        check_at @{0} two

--- expect      2011-03-24 22:59:09.000000000 +0000
+++ actual      2011-03-24 22:59:09.000000000 +0000
@@ -1 +1,2 @@
 two
+one
not ok - 2 @{0} shows current
#
#               check_at @{0} two
#

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

[Jeff King]

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

[PATCH 0/3] Falling back "diff -C -C" to "diff -C" more gracefully

[Junio C Hamano]

In a project with many paths, "diff -C -C" may have too many rename source
candidates (as it tries to use all existing paths) but the rename source
candidates "diff -C" would use may still fit under the rename detection
limit.  Currently, we punt and disable the inexact rename detection
altogether even in such a case.

This weatherballoon series illustrates how diffcore-rename can be tweaked
to allow "-C -C" to fall back to "-C".  Somebody should write a test, but
not today ;-).

Junio C Hamano (3):
  diffcore-rename: refactor "too many candidates" logic
  diffcore-rename: record filepair for rename src
  diffcore-rename: fall back to -C when -C -C busts the rename limit

 diffcore-rename.c |   97 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 71 insertions(+), 26 deletions(-)

-- 
1.7.4.rc1.214.g2a4f9

[PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit

[Junio C Hamano]

When there are too many paths in the project, the number of rename source
candidates "git diff -C -C" finds will exceed the rename detection limit,
and no inexact rename detection is performed.  We however could fall back
to "git diff -C" if the number of paths modified are sufficiently small.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diffcore-rename.c |   37 +++++++++++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 9ce81b6..4851af3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -415,11 +415,18 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)
 		m[worst] = *o;
 }
 
+/*
+ * Returns:
+ * 0 if we are under the limit;
+ * 1 if we need to disable inexact rename detection;
+ * 2 if we would be under the limit if we were given -C instead of -C -C.
+ */
 static int too_many_rename_candidates(int num_create,
 				      struct diff_options *options)
 {
 	int rename_limit = options->rename_limit;
 	int num_src = rename_src_nr;
+	int i;
 
 	/*
 	 * This basically does a test for the rename matrix not
@@ -436,6 +443,19 @@ static int too_many_rename_candidates(int num_create,
 	    (num_create * num_src <= rename_limit * rename_limit))
 		return 0;
 
+	/* Are we running under -C -C? */
+	if (!DIFF_OPT_TST(options, FIND_COPIES_HARDER))
+		return 1;
+
+	/* Would we bust the limit if we were running under -C? */
+	for (num_src = i = 0; i < rename_src_nr; i++) {
+		if (diff_unmodified_pair(rename_src[i].p))
+			continue;
+		num_src++;
+	}
+	if ((num_create <= rename_limit || num_src <= rename_limit) &&
+	    (num_create * num_src <= rename_limit * rename_limit))
+		return 2;
 	return 1;
 }
 
@@ -446,7 +466,7 @@ void diffcore_rename(struct diff_options *options)
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
 	struct diff_score *mx;
-	int i, j, rename_count;
+	int i, j, rename_count, skip_unmodified = 0;
 	int num_create, num_src, dst_cnt;
 
 	if (!minimum_score)
@@ -508,10 +528,18 @@ void diffcore_rename(struct diff_options *options)
 	if (!num_create)
 		goto cleanup;
 
-	if (too_many_rename_candidates(num_create, options)) {
+	switch (too_many_rename_candidates(num_create, options)) {
+	case 1:
 		if (options->warn_on_too_large_rename)
 			warning("too many files (created: %d deleted: %d), skipping inexact rename detection", num_create, num_src);
 		goto cleanup;
+	case 2:
+		if (options->warn_on_too_large_rename)
+			warning("too many files, falling back to -C");
+		skip_unmodified = 1;
+		break;
+	default:
+		break;
 	}
 
 	mx = xcalloc(num_create * NUM_CANDIDATE_PER_DST, sizeof(*mx));
@@ -529,6 +557,11 @@ void diffcore_rename(struct diff_options *options)
 		for (j = 0; j < rename_src_nr; j++) {
 			struct diff_filespec *one = rename_src[j].p->one;
 			struct diff_score this_src;
+
+			if (skip_unmodified &&
+			    diff_unmodified_pair(rename_src[j].p))
+				continue;
+
 			this_src.score = estimate_similarity(one, two,
 							     minimum_score);
 			this_src.name_score = basename_same(one, two);
-- 
1.7.4.rc1.214.g2a4f9