{"thread":{"id":"26834","subject":"[PATCH] builtin/diff.c: remove duplicated call to diff_result_code()","startedAt":"2011-03-22T21:45:43Z","lastAt":"2011-03-25T09:09:18Z","messageCount":24,"participants":["Junio C Hamano","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164092","messageId":"7vk4fqkewo.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":null,"subject":"[PATCH] builtin/diff.c: remove duplicated call to diff_result_code()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-22T21:45:43Z","receivedAt":"2011-03-22T21:45:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The return value from builtin_diff_files() is fed to diff_result_code()\nby the caller, and all other callees like builtin_diff_index() do not\nhave their own call to diff_result_code().  Remove the duplicated one\nfrom builtin_diff_files() and let the caller handle it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * This probably does not make a difference right now, but it will at\n   the end of the rebased 'rename-degrade-cc-to-c' series.\n\n builtin/diff.c |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 4c9deb2..171ac39 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -207,7 +207,6 @@ static void refresh_index_quietly(void)\n \n static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\n {\n-\tint result;\n \tunsigned int options = 0;\n \n \twhile (1 < argc && argv[1][0] == '-') {\n@@ -241,8 +240,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\tperror(\"read_cache_preload\");\n \t\treturn -1;\n \t}\n-\tresult = run_diff_files(revs, options);\n-\treturn diff_result_code(&revs->diffopt, result);\n+\treturn run_diff_files(revs, options);\n }\n \n int cmd_diff(int argc, const char **argv, const char *prefix)\n"},{"id":"164095","messageId":"1300830649-22830-1-git-send-email-gitster@pobox.com","threadId":"26834","inReplyTo":"7vk4fqkewo.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/3] diffcore-rename: refactor \"too many candidates\" logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-22T21:50:47Z","receivedAt":"2011-03-22T21:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Move the logic to a separate function, to be enhanced by later patches in\nthe series.\n\nWhile at it, swap the condition used in the if statement from \"if it is\ntoo big then do this\" to \"if it would fit then do this\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n\n---\n\n Rebased to 'master' as the logic to use the result of this logic was\n updated recently, together with the addition of eye-candy.\n---\n diffcore-rename.c |   47 +++++++++++++++++++++++++++++------------------\n 1 files changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex d40e40a..00f7f84 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -419,6 +419,34 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n \t\tm[worst] = *o;\n }\n \n+static int too_many_rename_candidates(int num_create,\n+\t\t\t\t      struct diff_options *options)\n+{\n+\tint rename_limit = options->rename_limit;\n+\tint num_src = rename_src_nr;\n+\n+\toptions->needed_rename_limit = 0;\n+\n+\t/*\n+\t * This basically does a test for the rename matrix not\n+\t * growing larger than a \"rename_limit\" square matrix, ie:\n+\t *\n+\t *    num_create * num_src > rename_limit * rename_limit\n+\t *\n+\t * but handles the potential overflow case specially (and we\n+\t * assume at least 32-bit integers)\n+\t */\n+\tif (rename_limit <= 0 || rename_limit > 32767)\n+\t\trename_limit = 32767;\n+\tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n+\t    (num_create * num_src <= rename_limit * rename_limit))\n+\t\treturn 0;\n+\n+\toptions->needed_rename_limit =\n+\t\tnum_src > num_create ? num_src : num_create;\n+\treturn 1;\n+}\n+\n static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, int copies)\n {\n \tint count = 0, i;\n@@ -444,7 +472,6 @@ void diffcore_rename(struct diff_options *options)\n {\n \tint detect_rename = options->detect_rename;\n \tint minimum_score = options->rename_score;\n-\tint rename_limit = options->rename_limit;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct diff_queue_struct outq;\n \tstruct diff_score *mx;\n@@ -511,24 +538,8 @@ void diffcore_rename(struct diff_options *options)\n \tif (!num_create)\n \t\tgoto cleanup;\n \n-\t/*\n-\t * This basically does a test for the rename matrix not\n-\t * growing larger than a \"rename_limit\" square matrix, ie:\n-\t *\n-\t *    num_create * num_src > rename_limit * rename_limit\n-\t *\n-\t * but handles the potential overflow case specially (and we\n-\t * assume at least 32-bit integers)\n-\t */\n-\toptions->needed_rename_limit = 0;\n-\tif (rename_limit <= 0 || rename_limit > 32767)\n-\t\trename_limit = 32767;\n-\tif ((num_create > rename_limit && num_src > rename_limit) ||\n-\t    (num_create * num_src > rename_limit * rename_limit)) {\n-\t\toptions->needed_rename_limit =\n-\t\t\tnum_src > num_create ? num_src : num_create;\n+\tif (too_many_rename_candidates(num_create, options))\n \t\tgoto cleanup;\n-\t}\n \n \tif (options->show_rename_progress) {\n \t\tprogress = start_progress_delay(\n-- \n1.7.4.1.554.gfdad8\n"},{"id":"164094","messageId":"1300830649-22830-2-git-send-email-gitster@pobox.com","threadId":"26834","inReplyTo":"1300830649-22830-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] diffcore-rename: record filepair for rename src","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-22T21:50:48Z","receivedAt":"2011-03-22T21:50:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This will allow us to later skip unmodified entries added due to \"-C -C\".\nWe might also want to do something similar to rename_dst side, but that\nwould only be for the sake of symmetry and not necessary for this series.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n diffcore-rename.c |   23 ++++++++++++-----------\n 1 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 00f7f84..a932f76 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -55,22 +55,23 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n \n /* Table of rename/copy src files */\n static struct diff_rename_src {\n-\tstruct diff_filespec *one;\n+\tstruct diff_filepair *p;\n \tunsigned short score; /* to remember the break score */\n } *rename_src;\n static int rename_src_nr, rename_src_alloc;\n \n-static struct diff_rename_src *register_rename_src(struct diff_filespec *one,\n-\t\t\t\t\t\t   unsigned short score)\n+static struct diff_rename_src *register_rename_src(struct diff_filepair *p)\n {\n \tint first, last;\n+\tstruct diff_filespec *one = p->one;\n+\tunsigned short score = p->score;\n \n \tfirst = 0;\n \tlast = rename_src_nr;\n \twhile (last > first) {\n \t\tint next = (last + first) >> 1;\n \t\tstruct diff_rename_src *src = &(rename_src[next]);\n-\t\tint cmp = strcmp(one->path, src->one->path);\n+\t\tint cmp = strcmp(one->path, src->p->one->path);\n \t\tif (!cmp)\n \t\t\treturn src;\n \t\tif (cmp < 0) {\n@@ -90,7 +91,7 @@ static struct diff_rename_src *register_rename_src(struct diff_filespec *one,\n \tif (first < rename_src_nr)\n \t\tmemmove(rename_src + first + 1, rename_src + first,\n \t\t\t(rename_src_nr - first - 1) * sizeof(*rename_src));\n-\trename_src[first].one = one;\n+\trename_src[first].p = p;\n \trename_src[first].score = score;\n \treturn &(rename_src[first]);\n }\n@@ -205,7 +206,7 @@ static void record_rename_pair(int dst_index, int src_index, int score)\n \tif (rename_dst[dst_index].pair)\n \t\tdie(\"internal error: dst already matched.\");\n \n-\tsrc = rename_src[src_index].one;\n+\tsrc = rename_src[src_index].p->one;\n \tsrc->rename_used++;\n \tsrc->count++;\n \n@@ -389,7 +390,7 @@ static int find_exact_renames(struct diff_options *options)\n \n \tinit_hash(&file_table);\n \tfor (i = 0; i < rename_src_nr; i++)\n-\t\tinsert_file_table(&file_table, -1, i, rename_src[i].one);\n+\t\tinsert_file_table(&file_table, -1, i, rename_src[i].p->one);\n \n \tfor (i = 0; i < rename_dst_nr; i++)\n \t\tinsert_file_table(&file_table, 1, i, rename_dst[i].two);\n@@ -460,7 +461,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n \t\tdst = &rename_dst[mx[i].dst];\n \t\tif (dst->pair)\n \t\t\tcontinue; /* already done, either exact or fuzzy. */\n-\t\tif (!copies && rename_src[mx[i].src].one->rename_used)\n+\t\tif (!copies && rename_src[mx[i].src].p->one->rename_used)\n \t\t\tcontinue;\n \t\trecord_rename_pair(mx[i].dst, mx[i].src, mx[i].score);\n \t\tcount++;\n@@ -503,7 +504,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\t */\n \t\t\tif (p->broken_pair && !p->score)\n \t\t\t\tp->one->rename_used++;\n-\t\t\tregister_rename_src(p->one, p->score);\n+\t\t\tregister_rename_src(p);\n \t\t}\n \t\telse if (detect_rename == DIFF_DETECT_COPY) {\n \t\t\t/*\n@@ -511,7 +512,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\t * one, to indicate ourselves as a user.\n \t\t\t */\n \t\t\tp->one->rename_used++;\n-\t\t\tregister_rename_src(p->one, p->score);\n+\t\t\tregister_rename_src(p);\n \t\t}\n \t}\n \tif (rename_dst_nr == 0 || rename_src_nr == 0)\n@@ -560,7 +561,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\tm[j].dst = -1;\n \n \t\tfor (j = 0; j < rename_src_nr; j++) {\n-\t\t\tstruct diff_filespec *one = rename_src[j].one;\n+\t\t\tstruct diff_filespec *one = rename_src[j].p->one;\n \t\t\tstruct diff_score this_src;\n \t\t\tthis_src.score = estimate_similarity(one, two,\n \t\t\t\t\t\t\t     minimum_score);\n-- \n1.7.4.1.554.gfdad8\n"},{"id":"164096","messageId":"1300830649-22830-3-git-send-email-gitster@pobox.com","threadId":"26834","inReplyTo":"1300830649-22830-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-22T21:50:49Z","receivedAt":"2011-03-22T21:50:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When there are too many paths in the project, the number of rename source\ncandidates \"git diff -C -C\" finds will exceed the rename detection limit,\nand no inexact rename detection is performed.  We however could fall back\nto \"git diff -C\" if the number of modified paths is sufficiently small.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/diff-tree.c    |   12 +++++++++++-\n builtin/log.c          |    9 +++++++++\n diff.c                 |   26 ++++++++++++++++++++++++++\n diff.h                 |    2 ++\n diffcore-rename.c      |   38 ++++++++++++++++++++++++++++++++++++--\n merge-recursive.c      |   10 +++-------\n t/t4001-diff-rename.sh |   25 +++++++++++++++++++++++++\n 7 files changed, 112 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 0d2a3e9..be6417d 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -163,6 +163,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_stdin) {\n+\t\tint saved_nrl = 0;\n+\t\tint saved_dcctc = 0;\n+\n \t\tif (opt->diffopt.detect_rename)\n \t\t\topt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |\n \t\t\t\t\t       DIFF_SETUP_USE_CACHE);\n@@ -173,9 +176,16 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t\t\tfputs(line, stdout);\n \t\t\t\tfflush(stdout);\n \t\t\t}\n-\t\t\telse\n+\t\t\telse {\n \t\t\t\tdiff_tree_stdin(line);\n+\t\t\t\tif (saved_nrl < opt->diffopt.needed_rename_limit)\n+\t\t\t\t\tsaved_nrl = opt->diffopt.needed_rename_limit;\n+\t\t\t\tif (opt->diffopt.degraded_cc_to_c)\n+\t\t\t\t\tsaved_dcctc = 1;\n+\t\t\t}\n \t\t}\n+\t\topt->diffopt.degraded_cc_to_c = saved_dcctc;\n+\t\topt->diffopt.needed_rename_limit = saved_nrl;\n \t}\n \n \treturn diff_result_code(&opt->diffopt, 0);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 796e9e5..707fd57 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -247,6 +247,8 @@ static void finish_early_output(struct rev_info *rev)\n static int cmd_log_walk(struct rev_info *rev)\n {\n \tstruct commit *commit;\n+\tint saved_nrl = 0;\n+\tint saved_dcctc = 0;\n \n \tif (rev->early_output)\n \t\tsetup_early_output(rev);\n@@ -277,7 +279,14 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t}\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n+\t\tif (saved_nrl < rev->diffopt.needed_rename_limit)\n+\t\t\tsaved_nrl = rev->diffopt.needed_rename_limit;\n+\t\tif (rev->diffopt.degraded_cc_to_c)\n+\t\t\tsaved_dcctc = 1;\n \t}\n+\trev->diffopt.degraded_cc_to_c = saved_dcctc;\n+\trev->diffopt.needed_rename_limit = saved_nrl;\n+\n \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n \t    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {\n \t\treturn 02;\ndiff --git a/diff.c b/diff.c\nindex 9b3eb99..13f322b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3956,6 +3956,28 @@ static int is_summary_empty(const struct diff_queue_struct *q)\n \treturn 1;\n }\n \n+static const char rename_limit_warning[] =\n+\"inexact rename detection was skipped due to too many files.\";\n+\n+static const char degrade_cc_to_c_warning[] =\n+\"only found copies from modified paths due to too many files.\";\n+\n+static const char rename_limit_advice[] =\n+\"you may want to set your %s variable to at least \"\n+\"%d and retry the command.\";\n+\n+void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)\n+{\n+\tif (degraded_cc)\n+\t\twarning(degrade_cc_to_c_warning);\n+\telse if (needed)\n+\t\twarning(rename_limit_warning);\n+\telse\n+\t\treturn;\n+\tif (0 < needed && needed < 32767)\n+\t\twarning(rename_limit_advice, varname, needed);\n+}\n+\n void diff_flush(struct diff_options *options)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -4237,6 +4259,10 @@ void diffcore_std(struct diff_options *options)\n int diff_result_code(struct diff_options *opt, int status)\n {\n \tint result = 0;\n+\n+\tdiff_warn_rename_limit(\"diff.renamelimit\",\n+\t\t\t       opt->needed_rename_limit,\n+\t\t\t       opt->degraded_cc_to_c);\n \tif (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&\n \t    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))\n \t\treturn status;\ndiff --git a/diff.h b/diff.h\nindex 007a055..b8dde8a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -111,6 +111,7 @@ struct diff_options {\n \tint rename_score;\n \tint rename_limit;\n \tint needed_rename_limit;\n+\tint degraded_cc_to_c;\n \tint show_rename_progress;\n \tint dirstat_percent;\n \tint setup;\n@@ -273,6 +274,7 @@ extern void diffcore_fix_diff_index(struct diff_options *);\n \n extern int diff_queue_is_empty(void);\n extern void diff_flush(struct diff_options*);\n+extern void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n \n /* diff-raw status letters */\n #define DIFF_STATUS_ADDED\t\t'A'\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex a932f76..f62587e 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -420,11 +420,18 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n \t\tm[worst] = *o;\n }\n \n+/*\n+ * Returns:\n+ * 0 if we are under the limit;\n+ * 1 if we need to disable inexact rename detection;\n+ * 2 if we would be under the limit if we were given -C instead of -C -C.\n+ */\n static int too_many_rename_candidates(int num_create,\n \t\t\t\t      struct diff_options *options)\n {\n \tint rename_limit = options->rename_limit;\n \tint num_src = rename_src_nr;\n+\tint i;\n \n \toptions->needed_rename_limit = 0;\n \n@@ -445,6 +452,20 @@ static int too_many_rename_candidates(int num_create,\n \n \toptions->needed_rename_limit =\n \t\tnum_src > num_create ? num_src : num_create;\n+\n+\t/* Are we running under -C -C? */\n+\tif (!DIFF_OPT_TST(options, FIND_COPIES_HARDER))\n+\t\treturn 1;\n+\n+\t/* Would we bust the limit if we were running under -C? */\n+\tfor (num_src = i = 0; i < rename_src_nr; i++) {\n+\t\tif (diff_unmodified_pair(rename_src[i].p))\n+\t\t\tcontinue;\n+\t\tnum_src++;\n+\t}\n+\tif ((num_create <= rename_limit || num_src <= rename_limit) &&\n+\t    (num_create * num_src <= rename_limit * rename_limit))\n+\t\treturn 2;\n \treturn 1;\n }\n \n@@ -476,7 +497,7 @@ void diffcore_rename(struct diff_options *options)\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct diff_queue_struct outq;\n \tstruct diff_score *mx;\n-\tint i, j, rename_count;\n+\tint i, j, rename_count, skip_unmodified = 0;\n \tint num_create, num_src, dst_cnt;\n \tstruct progress *progress = NULL;\n \n@@ -539,8 +560,16 @@ void diffcore_rename(struct diff_options *options)\n \tif (!num_create)\n \t\tgoto cleanup;\n \n-\tif (too_many_rename_candidates(num_create, options))\n+\tswitch (too_many_rename_candidates(num_create, options)) {\n+\tcase 1:\n \t\tgoto cleanup;\n+\tcase 2:\n+\t\toptions->degraded_cc_to_c = 1;\n+\t\tskip_unmodified = 1;\n+\t\tbreak;\n+\tdefault:\n+\t\tbreak;\n+\t}\n \n \tif (options->show_rename_progress) {\n \t\tprogress = start_progress_delay(\n@@ -563,6 +592,11 @@ void diffcore_rename(struct diff_options *options)\n \t\tfor (j = 0; j < rename_src_nr; j++) {\n \t\t\tstruct diff_filespec *one = rename_src[j].p->one;\n \t\t\tstruct diff_score this_src;\n+\n+\t\t\tif (skip_unmodified &&\n+\t\t\t    diff_unmodified_pair(rename_src[j].p))\n+\t\t\t\tcontinue;\n+\n \t\t\tthis_src.score = estimate_similarity(one, two,\n \t\t\t\t\t\t\t     minimum_score);\n \t\t\tthis_src.name_score = basename_same(one, two);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8e82a8b..3ae4d53 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -22,11 +22,6 @@\n #include \"dir.h\"\n #include \"submodule.h\"\n \n-static const char rename_limit_advice[] =\n-\"inexact rename detection was skipped because there were too many\\n\"\n-\"  files. You may want to set your merge.renamelimit variable to at least\\n\"\n-\"  %d and retry this merge.\";\n-\n static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n \t\t\t\t      const char *subtree_shift)\n {\n@@ -1656,8 +1651,9 @@ int merge_recursive(struct merge_options *o,\n \t\tcommit_list_insert(h2, &(*result)->parents->next);\n \t}\n \tflush_output(o);\n-\tif (o->needed_rename_limit)\n-\t\twarning(rename_limit_advice, o->needed_rename_limit);\n+\tif (show(o, 2))\n+\t\tdiff_warn_rename_limit(\"merge.renamelimit\",\n+\t\t\t\t       o->needed_rename_limit, 0);\n \treturn clean;\n }\n \ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex 71bac83..301f3a0 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -77,4 +77,29 @@ test_expect_success  'favour same basenames even with minor differences' '\n \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n \tgit status | grep \"renamed: .*path1 -> subdir/path1\"'\n \n+test_expect_success 'setup for many rename source candidates' '\n+\tgit reset --hard &&\n+\tfor i in 0 1 2 3 4 5 6 7 8 9;\n+\tdo\n+\t\tfor j in 0 1 2 3 4 5 6 7 8 9;\n+\t\tdo\n+\t\t\techo \"$i$j\" >\"path$i$j\"\n+\t\tdone\n+\tdone &&\n+\tgit add \"path??\" &&\n+\ttest_tick &&\n+\tgit commit -m \"hundred\" &&\n+\t(cat path1; echo new) >new-path &&\n+\techo old >>path1 &&\n+\tgit add new-path path1 &&\n+\tgit diff -l 4 -C -C --cached --name-status >actual 2>actual.err &&\n+\tsed -e \"s/^\\([CM]\\)[0-9]*\t/\\1\t/\" actual >actual.munged &&\n+\tcat >expect <<-EOF &&\n+\tC\tpath1\tnew-path\n+\tM\tpath1\n+\tEOF\n+\ttest_cmp expect actual.munged &&\n+\tgrep warning actual.err\n+'\n+\n test_done\n-- \n1.7.4.1.554.gfdad8\n"},{"id":"164158","messageId":"20110323155854.GB30337@sigill.intra.peff.net","threadId":"26834","inReplyTo":"1300830649-22830-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T15:58:54Z","receivedAt":"2011-03-23T15:58:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 22, 2011 at 02:50:49PM -0700, Junio C Hamano wrote:\n\n> When there are too many paths in the project, the number of rename source\n> candidates \"git diff -C -C\" finds will exceed the rename detection limit,\n> and no inexact rename detection is performed.  We however could fall back\n> to \"git diff -C\" if the number of modified paths is sufficiently small.\n\nHmm. Compared to the previous iteration, this also seems to turn on\nwarnings for the diff and log families. It would have been a little\neasier to review as a separate patch, I think. In particular, I'm not\nsure I like the \"just warn once at the end\" strategy when we show\nmultiple diffs.\n\nFor example, in the trash repo created by your new t4001, I did:\n\n  git commit -a -m foo &&\n  git config diff.renamelimit 4 &&\n  git log --raw -C -C\n\nAnd like most users, I read through the commits in the order they're\npresented. So I get to the second one and see that it has a bunch of\nadditions, but no copies. Then I keep reading and finally, after the\nthird commit, I see a warning that we didn't do copy detection.\n\nHere's what I see wrong with that from the user's perspective:\n\n  1. You give me the warning at some unspecified time after I've already\n     read and digested the results of the commit it affects. So at best\n     I've wasted my time looking at results that you later tell me might\n     be bogus.\n\n  2. The warning is at the very end of the potentially long output. With\n     three commits, I'm likely to actually scroll all the way to the end.\n     But how often do you run \"git log\" in git.git and then kill the\n     pager after finding the answer you want, never seeing the bottom of\n     git's output (and potentially before git even generates it!). So I\n     may miss the warning entirely.\n\n  3. Even if I do see the warning, I have no idea which commits it\n     applies to. I have to bump the rename limit and re-run just to find\n     out whether the commits I cared about were affected.\n\nSo I think it would be much more sensible to show something like:\n\n  commit f3ba51f2e2025d15c84873cb24dcf51b77f6b8e1\n  Author: A U Thor <author@example.com>\n  Date:   Thu Apr 7 15:13:13 2005 -0700\n\n      hundred\n\n  warning: only found copies from modified paths due to too many files.\n  warning: you may want to set your diff.renamelimit variable to at\n           least 102 and retry the command.\n  :000000 100644 0000000... 4daddb7... A  path00\n  :000000 100644 0000000... 8a0f05e... A  path01\n  [etc]\n\nOne argument against that is that we may end up printing many such\nwarnings. For the pager case, where stderr and stdout are combined, that\nis probably fine. Any commit which blows the rename limit is going to\nhave a ton of diff output anyway, so a few extra lines doesn't hurt\n(with some careful flushing, those lines are placed in the right spot).\n\nWhen stderr is separated, though, it's going to be annoyingly verbose,\nand multiple warnings wouldn't match up with their respective commits,\nanyway.\n\nSo maybe it is worth detecting the pager case and behaving differently\nbased on whether stderr and stdout are combined.\n\nAnother alternative would be to embed the warning in the stdout stream,\nbut I don't see a clean way of doing that. The best I could come up with\nis something like:\n\n    commit f3ba51f2e2025d15c84873cb24dcf51b77f6b8e1\n    Author: A U Thor <author@example.com>\n    Date:   Thu Apr 7 15:13:13 2005 -0700\n    X-Diff-Warning: only found copies...\n\nwhich just seems a little too hack-ish.\n\nAlso, on a somewhat related note: which commands should have rename\nprogress reporting turned on? It makes sense for \"git diff\" to do it to\nme. And probably even \"git show\". Probably not for \"git log\", though.\nI think we'd also have to look at how that interacts with the\nstderr-to-stdout pager thing. We obviously don't want progress going to\nthe pager.\n\n> +static const char rename_limit_warning[] =\n> +\"inexact rename detection was skipped due to too many files.\";\n> +\n> +static const char degrade_cc_to_c_warning[] =\n> +\"only found copies from modified paths due to too many files.\";\n\nSomehow the phrase \"due to too many files\" seems awkward. The\nmerge-recursive warning this replaces used \"because there were too many\nfiles\". Which is longer, of course, but seems more natural.\n\n> +static const char rename_limit_advice[] =\n> +\"you may want to set your %s variable to at least \"\n> +\"%d and retry the command.\";\n\nAnd this one ends up overflowing reasonably-sized terminals, because it\ngets prefixed with \"warning: \", and because the variable name is\nsomething like \"diff.renamelimit\".\n\n> @@ -445,6 +452,20 @@ static int too_many_rename_candidates(int num_create,\n>  \n>  \toptions->needed_rename_limit =\n>  \t\tnum_src > num_create ? num_src : num_create;\n\nThis is obviously not introduced by your patch, but I noticed once again\nduring testing that this number is conservatively high, isn't it? I\nthink the number we want is actually:\n\n  ceil(sqrt(num_create * num_src))\n\nright? I don't know if it is worth being more accurate.\n\n> @@ -1656,8 +1651,9 @@ int merge_recursive(struct merge_options *o,\n>  \t\tcommit_list_insert(h2, &(*result)->parents->next);\n>  \t}\n>  \tflush_output(o);\n> -\tif (o->needed_rename_limit)\n> -\t\twarning(rename_limit_advice, o->needed_rename_limit);\n> +\tif (show(o, 2))\n> +\t\tdiff_warn_rename_limit(\"merge.renamelimit\",\n> +\t\t\t\t       o->needed_rename_limit, 0);\n\nWith the call to show(), we are showing the warning only for the\noutermost diff. Which is probably way better than generating a bunch of\nwarnings, one for each rename-detect we do. But aren't we now failing to\nmention limits we hit in recursive invocations, even though they can\naffect the results? In other words, isn't this a candidate for the\n\"find the highest limit needed and report once\" strategy you used above?\n\n-Peff\n"},{"id":"164162","messageId":"7vfwqdg574.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":"20110323155854.GB30337@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T16:41:19Z","receivedAt":"2011-03-23T16:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> \"find the highest limit needed and report once\" strategy you used above?\n\nWasn't \"find the highest limit\" your invention in merge-recursive?  The\nonly deliberate fix in merge-recursive codepath on top of what you did is\nto allow squelching the warning when verbosity is set to more quiet than\nthe default, and removal of the \"set var to this value\" advice when the\nvalue we are going to suggest is too high already and will not help.\n\nAs to the warnings in \"log\" output, I actually prefer leaving saved_* out\nand showing the warning per internal diff invocation. My initial iteration\nwas indeed coded that way, and I did the \"find the highest\" only to mimic\nwhat was already in merge-recursive.\n"},{"id":"164164","messageId":"20110323165040.GA4068@sigill.intra.peff.net","threadId":"26834","inReplyTo":"7vfwqdg574.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T16:50:40Z","receivedAt":"2011-03-23T16:50:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2011 at 09:41:19AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > \"find the highest limit needed and report once\" strategy you used above?\n> \n> Wasn't \"find the highest limit\" your invention in merge-recursive?  The\n\nHmm, you're right. I was thinking the code you were modifying was called\nper-diff, but it's not. Though looking at it again, I think we could\ntechnically still warn several times, one for each recursive call to\nmerge_recursive. Your call to show() does fix that (because it checks\no->call_depth).\n\nSo I think the code after your patch does the right thing.\n\n> As to the warnings in \"log\" output, I actually prefer leaving saved_* out\n> and showing the warning per internal diff invocation. My initial iteration\n> was indeed coded that way, and I did the \"find the highest\" only to mimic\n> what was already in merge-recursive.\n\nI think they are two different cases, because the user never gets to see\nthe intermediate results of merge-recursive. That is, at the end of the\nmerge we tell them \"here's the result, and by the way, we limited\nrenames.\" But for something like \"log\" or \"diff-tree --stdin\", it is\nabout doing N independent diffs, and the user gets to see the result of\neach. So if we can match the warnings to the actual diffs in the output,\nthat is much more useful.\n\nBut in the case of something like:\n\n  git rev-list HEAD | git diff-tree --stdin >foo.out\n\nI don't think there is a way to match the warnings to their respective\ncommits. They are on two different streams, and putting the warning to\nstdout would pollute the output.\n\n-Peff\n"},{"id":"164174","messageId":"20110323181756.GA17415@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110323155854.GB30337@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] diffcore-rename: fall back to -C when -C -C busts the rename limit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T18:17:56Z","receivedAt":"2011-03-23T18:17:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2011 at 11:58:54AM -0400, Jeff King wrote:\n\n> Also, on a somewhat related note: which commands should have rename\n> progress reporting turned on? It makes sense for \"git diff\" to do it to\n> me. And probably even \"git show\". Probably not for \"git log\", though.\n> I think we'd also have to look at how that interacts with the\n> stderr-to-stdout pager thing. We obviously don't want progress going to\n> the pager.\n\nThis quick-and-dirty series makes rename progress work properly for \"git\nshow\". Patch 1/3 would also be the building block for having \"git log\"\nshow per-commit warnings when stderr and stdout are mixed, but a single\nwarning otherwise. I built it on top of your patches, but I think there\nis no reason it could not be done in parallel.\n\n  [1/3]: pager: save the original stderr when redirecting to pager\n  [2/3]: progress: use pager's original_stderr if available\n  [3/3]: show: turn on rename progress\n\n-Peff\n"},{"id":"164175","messageId":"20110323181853.GA17533@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110323181756.GA17415@sigill.intra.peff.net","subject":"[PATCH 1/3] pager: save the original stderr when redirecting to pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T18:18:53Z","receivedAt":"2011-03-23T18:18:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we redirect stdout to the pager, we also redirect\nstderr (if it would otherwise go to the terminal) so that\nerror messages do not get overwritten by the pager.\n\nHowever, some stderr output may still want to go to the\nterminal, because they are time-sensitive (like progress\nreports) and should be overwritten by the pager.\n\nThis patch stashes away the original stderr descriptor and\ncreates a new stdio buffer for it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |    2 ++\n pager.c |    7 ++++++-\n 2 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 872bc9b..a6ce524 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1067,6 +1067,8 @@ extern int pager_use_color;\n extern const char *editor_program;\n extern const char *askpass_program;\n extern const char *excludes_file;\n+extern FILE *original_stderr;\n+extern int original_stderr_fd;\n \n /* base85 */\n int decode_85(char *dst, const char *line, int linelen);\ndiff --git a/pager.c b/pager.c\nindex dac358f..701926f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -12,6 +12,8 @@\n  */\n \n static int spawned_pager;\n+FILE *original_stderr;\n+int original_stderr_fd = -1;\n \n #ifndef WIN32\n static void pager_preexec(void)\n@@ -97,8 +99,11 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\toriginal_stderr_fd = dup(2);\n+\t\toriginal_stderr = fdopen(original_stderr_fd, \"w\");\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n1.7.4.39.ge4c30\n"},{"id":"164176","messageId":"20110323181906.GB17533@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110323181756.GA17415@sigill.intra.peff.net","subject":"[PATCH 2/3] progress: use pager's original_stderr if available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T18:19:06Z","receivedAt":"2011-03-23T18:19:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are outputting to a pager, stderr is redirected to the\npager. However, progress messages should not be part of that\nstream, as they are time-sensitive and should end up being\nhidden once we actually have output.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n progress.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 3971f49..bc7d3a3 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -10,6 +10,7 @@\n \n #include \"git-compat-util.h\"\n #include \"progress.h\"\n+#include \"cache.h\"\n \n #define TP_IDX_MAX      8\n \n@@ -71,6 +72,7 @@ static void clear_progress_signal(void)\n \n static int display(struct progress *progress, unsigned n, const char *done)\n {\n+\tFILE *out = original_stderr ? original_stderr : stderr;\n \tconst char *eol, *tp;\n \n \tif (progress->delay) {\n@@ -95,16 +97,16 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n+\t\t\tfprintf(out, \"%s: %3u%% (%u/%u)%s%s\",\n \t\t\t\tprogress->title, percent, n,\n \t\t\t\tprogress->total, tp, eol);\n-\t\t\tfflush(stderr);\n+\t\t\tfflush(out);\n \t\t\tprogress_update = 0;\n \t\t\treturn 1;\n \t\t}\n \t} else if (progress_update) {\n-\t\tfprintf(stderr, \"%s: %u%s%s\", progress->title, n, tp, eol);\n-\t\tfflush(stderr);\n+\t\tfprintf(out, \"%s: %u%s%s\", progress->title, n, tp, eol);\n+\t\tfflush(out);\n \t\tprogress_update = 0;\n \t\treturn 1;\n \t}\n@@ -211,11 +213,12 @@ int display_progress(struct progress *progress, unsigned n)\n struct progress *start_progress_delay(const char *title, unsigned total,\n \t\t\t\t       unsigned percent_treshold, unsigned delay)\n {\n+\tFILE *out = original_stderr ? original_stderr : stderr;\n \tstruct progress *progress = malloc(sizeof(*progress));\n \tif (!progress) {\n \t\t/* unlikely, but here's a good fallback */\n-\t\tfprintf(stderr, \"%s...\\n\", title);\n-\t\tfflush(stderr);\n+\t\tfprintf(out, \"%s...\\n\", title);\n+\t\tfflush(out);\n \t\treturn NULL;\n \t}\n \tprogress->title = title;\n-- \n1.7.4.39.ge4c30\n"},{"id":"164177","messageId":"20110323181915.GC17533@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110323181756.GA17415@sigill.intra.peff.net","subject":"[PATCH 3/3] show: turn on rename progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-23T18:19:15Z","receivedAt":"2011-03-23T18:19:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For large commits, it is nice to have some eye candy for the\nrename detection.\n\nThis makes sense for \"show\", which is showing a single diff,\nsince the progress reporting will be shown before the pager\nshows any output.\n\nWe don't want to do the same for \"log\", though, because the\nprogress would be interspersed with the various commits.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 707fd57..cf68130 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -416,6 +416,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \trev.diff = 1;\n \trev.always_show_header = 1;\n \trev.no_walk = 1;\n+\trev.diffopt.show_rename_progress = 1;\n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = \"HEAD\";\n \topt.tweak = show_rev_tweak_rev;\n-- \n1.7.4.39.ge4c30\n"},{"id":"164183","messageId":"7vvcz9cyxd.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":"20110323181915.GC17533@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-23T21:25:02Z","receivedAt":"2011-03-23T21:25:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> For large commits, it is nice to have some eye candy for the\n> rename detection.\n>\n> This makes sense for \"show\", which is showing a single diff,\n> since the progress reporting will be shown before the pager\n> shows any output.\n>\n> We don't want to do the same for \"log\", though, because the\n> progress would be interspersed with the various commits.\n\nI understand that you said q&d, but these days since f222abd (Make 'git\nshow' more useful, 2009-07-13), show does walk when it is asked to.\n\n\"git show master maint\" will also be affected with this, no?\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/log.c |    1 +\n>  1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 707fd57..cf68130 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -416,6 +416,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n>  \trev.diff = 1;\n>  \trev.always_show_header = 1;\n>  \trev.no_walk = 1;\n> +\trev.diffopt.show_rename_progress = 1;\n>  \tmemset(&opt, 0, sizeof(opt));\n>  \topt.def = \"HEAD\";\n>  \topt.tweak = show_rev_tweak_rev;\n"},{"id":"164233","messageId":"20110324145047.GB16484@sigill.intra.peff.net","threadId":"26834","inReplyTo":"7vvcz9cyxd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T14:50:48Z","receivedAt":"2011-03-24T14:50:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:\n\n> > This makes sense for \"show\", which is showing a single diff,\n> > since the progress reporting will be shown before the pager\n> > shows any output.\n> >\n> > We don't want to do the same for \"log\", though, because the\n> > progress would be interspersed with the various commits.\n> \n> I understand that you said q&d, but these days since f222abd (Make 'git\n> show' more useful, 2009-07-13), show does walk when it is asked to.\n> \n> \"git show master maint\" will also be affected with this, no?\n\nIck, I didn't think of that. So yeah, that patch as-is is definitely no\ngood. I think the most sensible thing to do is to show progress only\nuntil we actually generate some output. That handles both the walking\ncase and the \"git show master maint\" case.\n\nSomething like this:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 707fd57..d652acc 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -265,13 +265,16 @@ static int cmd_log_walk(struct rev_info *rev)\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n-\t\tif (!log_tree_commit(rev, commit) &&\n+\t\tint showed = log_tree_commit(rev, commit);\n+\t\tif (showed &&\n \t\t    rev->max_count >= 0)\n \t\t\t/*\n \t\t\t * We decremented max_count in get_revision,\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n+\t\tif (showed && rev->diffopt.show_rename_progress)\n+\t\t\trev->diffopt.show_rename_progress = 0;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree(commit->buffer);\n@@ -416,6 +419,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \trev.diff = 1;\n \trev.always_show_header = 1;\n \trev.no_walk = 1;\n+\trev.diffopt.show_rename_progress = 1;\n \tmemset(&opt, 0, sizeof(opt));\n \topt.def = \"HEAD\";\n \topt.tweak = show_rev_tweak_rev;\n\nWe could also turn it on for \"git log\" in that case, though it is only\nuseful if the first commit happens to be the one that is slow.\n\nI should also turn it on for \"git diff\". I'll prepare a cleaner series\nwith that in it, too.\n\nWhat about the degrade-cc-to-c warnings? Are you working on another\nrevision, or should I re-roll your changes on top of my series, handling\nthe \"one-warning-per-commit\" behavior I suggested when stdout and stderr\nare combined?\n\n-Peff\n"},{"id":"164234","messageId":"7v39mca7hl.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":"20110324145047.GB16484@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-24T15:00:38Z","receivedAt":"2011-03-24T15:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:\n>\n> We could also turn it on for \"git log\" in that case, though it is only\n> useful if the first commit happens to be the one that is slow.\n>\n> I should also turn it on for \"git diff\". I'll prepare a cleaner series\n> with that in it, too.\n\nSounds good, thanks.\n\n> What about the degrade-cc-to-c warnings? Are you working on another\n> revision, or should I re-roll your changes on top of my series, handling\n> the \"one-warning-per-commit\" behavior I suggested when stdout and stderr\n> are combined?\n\nAlso sounds good, thanks.  No, I am spending far more time on ushering\nstalled topics than looking at my weatherbaloons and no time left for the\nlatter.\n"},{"id":"164240","messageId":"20110324174556.GA30661@sigill.intra.peff.net","threadId":"26834","inReplyTo":"7v39mca7hl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T17:45:56Z","receivedAt":"2011-03-24T17:45:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2011 at 08:00:38AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:\n> >\n> > We could also turn it on for \"git log\" in that case, though it is only\n> > useful if the first commit happens to be the one that is slow.\n> >\n> > I should also turn it on for \"git diff\". I'll prepare a cleaner series\n> > with that in it, too.\n> \n> Sounds good, thanks.\n\nHere it is:\n\n  [1/4]: pager: save the original stderr when redirecting to pager\n  [2/4]: progress: use pager's original_stderr if available\n  [3/4]: show: turn on rename detection progress reporting\n  [4/4]: diff: turn on rename detection progress reporting\n\n> > What about the degrade-cc-to-c warnings? Are you working on another\n> > revision, or should I re-roll your changes on top of my series, handling\n> > the \"one-warning-per-commit\" behavior I suggested when stdout and stderr\n> > are combined?\n> \n> Also sounds good, thanks.  No, I am spending far more time on ushering\n> stalled topics than looking at my weatherbaloons and no time left for the\n> latter.\n\nI thought this would be simple on top of 1/4 above, but it's not. In\nsome cases we want to insert the warning before the diff output, and in\nsome cases we want to know afterwards what happened. Which means we\nprobably need to carry a new \"show the warning\" flag in diffopt and\nrespect it in the rename, but also fill out the needed_rename_limit.\n\n-Peff\n"},{"id":"164241","messageId":"20110324174656.GA30685@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110324174556.GA30661@sigill.intra.peff.net","subject":"[PATCH 1/4] pager: save the original stderr when redirecting to pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T17:46:56Z","receivedAt":"2011-03-24T17:46:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we redirect stdout to the pager, we also redirect\nstderr (if it would otherwise go to the terminal) so that\nerror messages do not get overwritten by the pager.\n\nHowever, some stderr output may still want to go to the\nterminal, because they are time-sensitive (like progress\nreports) and should be overwritten by the pager.\n\nThis patch stashes away the original stderr descriptor and\ncreates a new stdio buffer for it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as the last iteration.\n\n cache.h |    2 ++\n pager.c |    7 ++++++-\n 2 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex f765cf5..73da139 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1068,6 +1068,8 @@ extern int pager_use_color;\n extern const char *editor_program;\n extern const char *askpass_program;\n extern const char *excludes_file;\n+extern FILE *original_stderr;\n+extern int original_stderr_fd;\n \n /* base85 */\n int decode_85(char *dst, const char *line, int linelen);\ndiff --git a/pager.c b/pager.c\nindex dac358f..701926f 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -12,6 +12,8 @@\n  */\n \n static int spawned_pager;\n+FILE *original_stderr;\n+int original_stderr_fd = -1;\n \n #ifndef WIN32\n static void pager_preexec(void)\n@@ -97,8 +99,11 @@ void setup_pager(void)\n \n \t/* original process continues, but writes to the pipe */\n \tdup2(pager_process.in, 1);\n-\tif (isatty(2))\n+\tif (isatty(2)) {\n+\t\toriginal_stderr_fd = dup(2);\n+\t\toriginal_stderr = fdopen(original_stderr_fd, \"w\");\n \t\tdup2(pager_process.in, 2);\n+\t}\n \tclose(pager_process.in);\n \n \t/* this makes sure that the parent terminates after the pager */\n-- \n1.7.4.41.g423da\n"},{"id":"164242","messageId":"20110324174732.GB30685@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110324174556.GA30661@sigill.intra.peff.net","subject":"[PATCH 2/4] progress: use pager's original_stderr if available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T17:47:32Z","receivedAt":"2011-03-24T17:47:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are outputting to a pager, stderr is redirected to the\npager. However, progress messages should not be part of that\nstream, as they are time-sensitive and would end up being\nhidden once we actually have output.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as the last iteration.\n\n progress.c |   15 +++++++++------\n 1 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 3971f49..bc7d3a3 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -10,6 +10,7 @@\n \n #include \"git-compat-util.h\"\n #include \"progress.h\"\n+#include \"cache.h\"\n \n #define TP_IDX_MAX      8\n \n@@ -71,6 +72,7 @@ static void clear_progress_signal(void)\n \n static int display(struct progress *progress, unsigned n, const char *done)\n {\n+\tFILE *out = original_stderr ? original_stderr : stderr;\n \tconst char *eol, *tp;\n \n \tif (progress->delay) {\n@@ -95,16 +97,16 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t\tunsigned percent = n * 100 / progress->total;\n \t\tif (percent != progress->last_percent || progress_update) {\n \t\t\tprogress->last_percent = percent;\n-\t\t\tfprintf(stderr, \"%s: %3u%% (%u/%u)%s%s\",\n+\t\t\tfprintf(out, \"%s: %3u%% (%u/%u)%s%s\",\n \t\t\t\tprogress->title, percent, n,\n \t\t\t\tprogress->total, tp, eol);\n-\t\t\tfflush(stderr);\n+\t\t\tfflush(out);\n \t\t\tprogress_update = 0;\n \t\t\treturn 1;\n \t\t}\n \t} else if (progress_update) {\n-\t\tfprintf(stderr, \"%s: %u%s%s\", progress->title, n, tp, eol);\n-\t\tfflush(stderr);\n+\t\tfprintf(out, \"%s: %u%s%s\", progress->title, n, tp, eol);\n+\t\tfflush(out);\n \t\tprogress_update = 0;\n \t\treturn 1;\n \t}\n@@ -211,11 +213,12 @@ int display_progress(struct progress *progress, unsigned n)\n struct progress *start_progress_delay(const char *title, unsigned total,\n \t\t\t\t       unsigned percent_treshold, unsigned delay)\n {\n+\tFILE *out = original_stderr ? original_stderr : stderr;\n \tstruct progress *progress = malloc(sizeof(*progress));\n \tif (!progress) {\n \t\t/* unlikely, but here's a good fallback */\n-\t\tfprintf(stderr, \"%s...\\n\", title);\n-\t\tfflush(stderr);\n+\t\tfprintf(out, \"%s...\\n\", title);\n+\t\tfflush(out);\n \t\treturn NULL;\n \t}\n \tprogress->title = title;\n-- \n1.7.4.41.g423da\n"},{"id":"164243","messageId":"20110324174916.GC30685@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110324174556.GA30661@sigill.intra.peff.net","subject":"[PATCH 3/4] show: turn on rename detection progress reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T17:49:16Z","receivedAt":"2011-03-24T17:49:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For large commits, it is nice to have some eye candy for the\nrename detection.\n\nHowever, because show can display multiple commits, we have\nto be careful not to clutter existing output. We show the\nprogress report only before we have generated any actual\noutput; once we have sent output to the terminal or pager,\nwe turn off progress reporting.\n\nThis also makes it safe to use with \"git log\", though it\nwill only be useful if the first commit is the slow one.\nSo this patch actually enables it for all of the\nlog/whatchanged/show/reflog family.\n\nWe also handle the usual --{no-}progress option and check\nthat stderr goes to a terminal before turning on progress.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nChanges since the last iteration:\n\n  - only show progress for the first commit\n  - check isatty(2)\n  - --{no-}progress\n\n Documentation/git-log.txt |    7 +++++++\n builtin/log.c             |   17 ++++++++++++++++-\n 2 files changed, 23 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 2c84028..c0f763e 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -73,6 +73,13 @@ produced by --stat etc.\n \tto be prefixed with \"\\-- \" to separate them from options or\n \trefnames.\n \n+--no-progress::\n+--progress::\n+\tDisable or enable progress reporting during long computations;\n+\tthe default is to enable progress reporting when stderr is a\n+\tterminal. Currently the only computation with progress support\n+\tis inexact rename detection.\n+\n include::rev-list-options.txt[]\n \n include::pretty-formats.txt[]\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 796e9e5..4d52e99 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -27,6 +27,7 @@ static int default_show_root = 1;\n static int decoration_style;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n+static int progress = -1;\n \n static const char * const builtin_log_usage =\n \t\"git log [<options>] [<since>..<until>] [[--] <path>...]\\n\"\n@@ -109,10 +110,17 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\t\trev->show_source = 1;\n \t\t} else if (!strcmp(arg, \"-h\")) {\n \t\t\tusage(builtin_log_usage);\n+\t\t} else if (!strcmp(arg, \"--progress\")) {\n+\t\t\tprogress = 1;\n+\t\t} else if (!strcmp(arg, \"--no-progress\")) {\n+\t\t\tprogress = 0;\n \t\t} else\n \t\t\tdie(\"unrecognized argument: %s\", arg);\n \t}\n \n+\tif (progress == -1)\n+\t\tprogress = isatty(2);\n+\n \t/*\n \t * defeat log.decorate configuration interacting with --pretty=raw\n \t * from the command line.\n@@ -257,19 +265,26 @@ static int cmd_log_walk(struct rev_info *rev)\n \tif (rev->early_output)\n \t\tfinish_early_output(rev);\n \n+\tif (progress)\n+\t\trev->diffopt.show_rename_progress = 1;\n+\n \t/*\n \t * For --check and --exit-code, the exit code is based on CHECK_FAILED\n \t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n-\t\tif (!log_tree_commit(rev, commit) &&\n+\t\tint showed = log_tree_commit(rev, commit);\n+\t\tif (showed &&\n \t\t    rev->max_count >= 0)\n \t\t\t/*\n \t\t\t * We decremented max_count in get_revision,\n \t\t\t * but we didn't actually show the commit.\n \t\t\t */\n \t\t\trev->max_count++;\n+\t\t/* Once we have output, progress will clutter the terminal. */\n+\t\tif (showed)\n+\t\t\trev->diffopt.show_rename_progress = 0;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree(commit->buffer);\n-- \n1.7.4.41.g423da\n"},{"id":"164244","messageId":"20110324175124.GD30685@sigill.intra.peff.net","threadId":"26834","inReplyTo":"20110324174556.GA30661@sigill.intra.peff.net","subject":"[PATCH 4/4] diff: turn on rename detection progress reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-24T17:51:24Z","receivedAt":"2011-03-24T17:51:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since all of the progress happens before we generate any\noutput, this looks OK, even when output goes to a pager.\nWe do the usual --progress/--no-progress options and check\nisatty(2) to enable the feature.\n\nThe argument parsing is a little ad-hoc, but we currently\nhave no parse-options infrastructure here at all.  However,\nit should be safe to parse like this, because the prior\ncall to setup_revisions will have removed any options that\ntake an argument, and our parsing removes --progress from\nargv for later parsers. The one exception is diff_no_index,\nwhich may get called early, and needs to learn to ignore\n--progress.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNew since the last iteration.\n\nI'm not happy about the option parsing, but converting the whole of \"git\ndiff\" to a saner parsing structure is more than I want to take on for\nthis series. One thing that would make it easier is if the diff parser\nunderstood --progress and stored the result for the caller to check. I\nwas tempted to do that because the code ends up cleaner, but it feels\nwrong.\n\n Documentation/git-diff.txt |    7 +++++++\n builtin/diff.c             |   28 ++++++++++++++++++++++++++++\n diff-no-index.c            |    2 ++\n 3 files changed, 37 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex f8d0819..dbebc93 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -87,6 +87,13 @@ OPTIONS\n :git-diff: 1\n include::diff-options.txt[]\n \n+--no-progress::\n+--progress::\n+\tDisable or enable progress reporting during long computations;\n+\tthe default is to enable progress reporting when stderr is a\n+\tterminal. Currently the only computation with progress support\n+\tis inexact rename detection.\n+\n <path>...::\n \tThe <paths> parameters, when given, are used to limit\n \tthe diff to the named paths (you can give directory\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 4c9deb2..82ecc1d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -255,6 +255,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct blobinfo blob[2];\n \tint nongit;\n \tint result = 0;\n+\tint progress = -1;\n+\tint unknown_argc, parsed_argc;\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -307,6 +309,32 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"diff_setup_done failed\");\n \t}\n \n+\tparsed_argc = 0;\n+\tfor (unknown_argc = i = 1; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tif (!strcmp(arg, \"--\") || arg[0] != '-') {\n+\t\t\tint j;\n+\t\t\tfor (j = i; j < argc; j++)\n+\t\t\t\targv[unknown_argc++] = argv[j];\n+\t\t\tbreak;\n+\t\t}\n+\t\telse if (!strcmp(argv[i], \"--progress\"))\n+\t\t\tprogress = 1;\n+\t\telse if (!strcmp(argv[i], \"--no-progress\"))\n+\t\t\tprogress = 0;\n+\t\telse {\n+\t\t\targv[unknown_argc++] = argv[i];\n+\t\t\tcontinue;\n+\t\t}\n+\t\tparsed_argc++;\n+\t}\n+\targc -= parsed_argc;\n+\n+\tif (progress == -1)\n+\t\tprogress = isatty(2);\n+\tif (progress)\n+\t\trev.diffopt.show_rename_progress = 1;\n+\n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \n \t/*\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 3a36144..42cb413 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -212,6 +212,8 @@ void diff_no_index(struct rev_info *revs,\n \t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n \t\t\ti++;\n \t\t}\n+\t\telse if (!strcmp(argv[i], \"--progress\"))\n+\t\t\t; /* handled elsewhere */\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n-- \n1.7.4.41.g423da\n"},{"id":"164262","messageId":"7v4o6s86kr.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":"20110324174556.GA30661@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-24T23:03:16Z","receivedAt":"2011-03-24T23:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 24, 2011 at 08:00:38AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > On Wed, Mar 23, 2011 at 02:25:02PM -0700, Junio C Hamano wrote:\n>> >\n>> > We could also turn it on for \"git log\" in that case, though it is only\n>> > useful if the first commit happens to be the one that is slow.\n>> >\n>> > I should also turn it on for \"git diff\". I'll prepare a cleaner series\n>> > with that in it, too.\n>> \n>> Sounds good, thanks.\n>\n> Here it is:\n>\n>   [1/4]: pager: save the original stderr when redirecting to pager\n>   [2/4]: progress: use pager's original_stderr if available\n>   [3/4]: show: turn on rename detection progress reporting\n>   [4/4]: diff: turn on rename detection progress reporting\n\nThanks, but why does it affect t0101 and many others...?\n\n\nt/jk/progress-with-pager$ sh t0101-at-syntax.sh -i -v\nInitialized empty Git repository in /git/git.git/t/trash\ndirectory.t0101-at-syntax/.git/\nexpecting success: \n        test_commit one &&\n        test_commit two\n\n[master (root-commit) d79ce16] one\n Author: A U Thor <author@example.com>\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 one.t\n[master 139b20d] two\n Author: A U Thor <author@example.com>\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 two.t\nok 1 - setup\n\nexpecting success: \n        check_at @{0} two\n\n--- expect      2011-03-24 22:59:09.000000000 +0000\n+++ actual      2011-03-24 22:59:09.000000000 +0000\n@@ -1 +1,2 @@\n two\n+one\nnot ok - 2 @{0} shows current\n#\n#               check_at @{0} two\n#\n"},{"id":"164265","messageId":"7vzkok6qie.fsf@alter.siamese.dyndns.org","threadId":"26834","inReplyTo":"20110324174916.GC30685@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] show: turn on rename detection progress reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-24T23:35:37Z","receivedAt":"2011-03-24T23:35:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  \twhile ((commit = get_revision(rev)) != NULL) {\n> -\t\tif (!log_tree_commit(rev, commit) &&\n> +\t\tint showed = log_tree_commit(rev, commit);\n> +\t\tif (showed &&\n>  \t\t    rev->max_count >= 0)\n>  \t\t\t/*\n>  \t\t\t * We decremented max_count in get_revision,\n>  \t\t\t * but we didn't actually show the commit.\n>  \t\t\t */\n>  \t\t\trev->max_count++;\n> +\t\t/* Once we have output, progress will clutter the terminal. */\n> +\t\tif (showed)\n> +\t\t\trev->diffopt.show_rename_progress = 0;\n\nAfter looking at the implementation of log_tree_commit(), shouldn't this\npart be more like this?\n\n\tint shown = log_tree_commit(rev, commit);\n        if (!shown && rev->max_count >=0)\n        \trev->max_count++;\n\tif (shown)\n        \trev->diffopt.show_rename_progress = 0;\n"},{"id":"164275","messageId":"20110325061744.GA6261@sigill.intra.peff.net","threadId":"26834","inReplyTo":"7vzkok6qie.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] show: turn on rename progress","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T06:17:44Z","receivedAt":"2011-03-25T06:17:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 24, 2011 at 04:03:16PM -0700, Junio C Hamano wrote:\n\n> >   [1/4]: pager: save the original stderr when redirecting to pager\n> >   [2/4]: progress: use pager's original_stderr if available\n> >   [3/4]: show: turn on rename detection progress reporting\n> >   [4/4]: diff: turn on rename detection progress reporting\n> \n> Thanks, but why does it affect t0101 and many others...?\n\nBecause I'm an idiot who, despite manually testing the option-parsing\npart of the code a million times, didn't actually run the full suite.\n\n> >  \twhile ((commit = get_revision(rev)) != NULL) {\n> > -\t\tif (!log_tree_commit(rev, commit) &&\n> > +\t\tint showed = log_tree_commit(rev, commit);\n> > +\t\tif (showed &&\n> >  \t\t    rev->max_count >= 0)\n\nUgh. Of course, it should be:\n\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4d52e99..b19e10d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -275,7 +275,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tint showed = log_tree_commit(rev, commit);\n-\t\tif (showed &&\n+\t\tif (!showed &&\n \t\t    rev->max_count >= 0)\n \t\t\t/*\n \t\t\t * We decremented max_count in get_revision,\n\non top.\n\n> After looking at the implementation of log_tree_commit(), shouldn't this\n> part be more like this?\n> \n> \tint shown = log_tree_commit(rev, commit);\n>         if (!shown && rev->max_count >=0)\n>         \trev->max_count++;\n> \tif (shown)\n>         \trev->diffopt.show_rename_progress = 0;\n\nYes. You shouldn't even need to look at log_tree_commit; you can see in\nthe hunk of my patch that I accidentally inverted the unrelated\nmax_count conditional while factoring out the call.\n\nI'll go find a brown paper bag now.\n\n-Peff\n\nPS I assume you can squash in the above, or take your version (I don't\ncare about the verb tense we use, but re-indenting the max_count\nconditional as you did is good style).\n"},{"id":"164278","messageId":"4D8C53DA.1010300@viscovery.net","threadId":"26834","inReplyTo":"20110324175124.GD30685@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] diff: turn on rename detection progress reporting","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-25T08:35:38Z","receivedAt":"2011-03-25T08:35:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/24/2011 18:51, schrieb Jeff King:\n> Since all of the progress happens before we generate any\n> output, this looks OK, even when output goes to a pager.\n> We do the usual --progress/--no-progress options and check\n> isatty(2) to enable the feature.\n\nWhy does it look good? Because the pager is not spawned, yet? Then this is\nnot so good because on Windows we don't have a facility to wait until\nthere is output, and for this reason we spawn the pager immediately.\n\n-- Hannes\n"},{"id":"164280","messageId":"20110325090918.GA7085@sigill.intra.peff.net","threadId":"26834","inReplyTo":"4D8C53DA.1010300@viscovery.net","subject":"Re: [PATCH 4/4] diff: turn on rename detection progress reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-25T09:09:18Z","receivedAt":"2011-03-25T09:09:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 25, 2011 at 09:35:38AM +0100, Johannes Sixt wrote:\n\n> Am 3/24/2011 18:51, schrieb Jeff King:\n> > Since all of the progress happens before we generate any\n> > output, this looks OK, even when output goes to a pager.\n> > We do the usual --progress/--no-progress options and check\n> > isatty(2) to enable the feature.\n> \n> Why does it look good? Because the pager is not spawned, yet? Then this is\n> not so good because on Windows we don't have a facility to wait until\n> there is output, and for this reason we spawn the pager immediately.\n\nIt depends on your pager. Less is careful not to produce any output\nuntil it has something to show, for exactly this reason. So it looks\nfine even if the pager has been started[1].\n\nBut if you have your pager set to \"tig\" (or you have manually piped to\nit), it looks awful. And something like \"git show $branch | tig\" is ugly\neven on Unix.\n\nI'm not sure what the right solution is. We can add a config option like\npager.progress, but it seems like something the user shouldn't have to\ncare about and set manually. And that doesn't help when the user has\nmanually invoked a pager that takes over the terminal, like \"git log |\ntig\".\n\n-Peff\n\n[1] Actually, there is a slight bug in my patch; if your pager has been\nstarted, our isatty(2) test fails. So you get no progress for \"git show\"\non Windows, anyway, unless you use \"--progress\". \"git diff\", on the\nother hand, starts the pager afterwards, so it does work.\n"}]}