{"thread":{"id":"24728","subject":"BUG: git log: fatal: internal error in diff-resolve-rename-copy","startedAt":"2010-08-13T11:25:57Z","lastAt":"2010-08-17T14:48:59Z","messageCount":16,"participants":["Constantine Plotnikov","Ævar Arnfjörð Bjarmason","Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"147998","messageId":"AANLkTikPhWgeeLBV3dbLZ5UM3UDmkOmpqrmwqPmGfn7Z@mail.gmail.com","threadId":"24728","inReplyTo":null,"subject":"BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2010-08-13T11:25:57Z","receivedAt":"2010-08-13T11:25:57Z","isPatch":false,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename\ndetection started to fail with fatal error on some files in our\nrepository. The bug could be seen on the public IntelliJ IDEA\nrepository (about 760M in size), but our users have reported it as\nwell.\n\nTo reproduce the error, run the following sequence of the commands:\n\ngit clone git://git.jetbrains.org/idea/community.git idea\ncd idea\ngit log -M --follow --name-only --\nplatform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java\n\nAs result \"fatal: internal error in diff-resolve-rename-copy\" is\nwritten on stderr. This is somewhat unexpected result. Git 1.7.0.2 and\n1.6.5.2 seems to work without visible problems.\n\nRegards,\nConstantine\n"},{"id":"148000","messageId":"AANLkTikGE-m=Tht2tNbP5=Bej78284_ouLBfdk_aEDSO@mail.gmail.com","threadId":"24728","inReplyTo":"AANLkTikPhWgeeLBV3dbLZ5UM3UDmkOmpqrmwqPmGfn7Z@mail.gmail.com","subject":"Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-13T13:38:01Z","receivedAt":"2010-08-13T13:38:01Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Aug 13, 2010 at 11:25, Constantine Plotnikov\n wrote:\n> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename\n> detection started to fail with fatal error on some files in our\n> repository. The bug could be seen on the public IntelliJ IDEA\n> repository (about 760M in size), but our users have reported it as\n> well.\n\n>From a bisect:\n\n 1da6175d438a9849db07a68326ee05f291510074 is the first bad commit\n commit 1da6175d438a9849db07a68326ee05f291510074\n Author: Bo Yang \n Date: Thu May 6 21:52:28 2010 -0700\n\n Make diffcore_std only can run once before a diff_flush\n\n When file renames/copies detection is turned on, the\n second diffcore_std will degrade a 'C' pair to a 'R' pair.\n\n And this may happen when we run 'git log --follow' with\n hard copies finding. That is, the try_to_follow_renames()\n will run diffcore_std to find the copies, and then\n 'git log' will issue another diffcore_std, which will reduce\n 'src->rename_used' and recognize this copy as a rename.\n This is not what we want.\n\n So, I think we really don't need to run diffcore_std more\n than one time.\n\n Signed-off-by: Bo Yang \n Signed-off-by: Junio C Hamano \n\nBisect script:\n\n #!/bin/sh\n\n cd ~/g/git\n make clean\n make -j 10 CC=clang CFLAGS=-O0\n\n ./git --git-dir=/tmp/idea/.git log -M --follow --name-only --\nplatform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java\n> /dev/null\n ret=$?\n test $ret -gt 127 && ret=127\n exit $ret\n"},{"id":"148009","messageId":"7vhbiyl8ji.fsf@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"AANLkTikPhWgeeLBV3dbLZ5UM3UDmkOmpqrmwqPmGfn7Z@mail.gmail.com","subject":"Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-13T17:36:01Z","receivedAt":"2010-08-13T17:36:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Constantine Plotnikov writes:\n\n> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename\n> detection started to fail with fatal error on some files in our\n> repository. The bug could be seen on the public IntelliJ IDEA\n> repository (about 760M in size), but our users have reported it as\n> well.\n>\n> To reproduce the error, run the following sequence of the commands:\n>\n> git clone git://git.jetbrains.org/idea/community.git idea\n> cd idea\n> git log -M --follow --name-only --\n> platform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java\n>\n> As result \"fatal: internal error in diff-resolve-rename-copy\" is\n> written on stderr. This is somewhat unexpected result. Git 1.7.0.2 and\n> 1.6.5.2 seems to work without visible problems.\n\nThis is interesting. I actually see another potential \"funny\" (and that\nis why Linus is CC'ed --- scroll down to \"funny\"), which is unrelated.\n\ndiff_tree_sha1() essentially:\n\n - given two trees, runs straightforward diff-tree without any rename\n detection;\n\n - under --follow mode, if it has only one change that deletes a path,\n runs try_to_follow_renames(), which:\n\n - runs the same diff-tree with deep rename detection but without any\n path limiter;\n\n - if we find a rename or copy that explains why we saw the deletion in\n the first step, replace the deletion record with the rename or copy.\n also set up to follow the old name from now on.\n\n - then return to the caller.\n\nThe diff frontends (diff-tree, diff-files and diff-index) are expected to\nleave vanilla filepairs and let the diffcore backend to find renames and\ncopies via a call to diff_resolve_rename_copy() in diffcore_std().\n\nWe however have a special \"hack\" in \"diff-tree --follow\". If it finds\nonly one diff_queue entry that creates a path, it internally runs itself\nagain with the rename/copy detection on, without any pathspec, to see if\nthe creation is an artifact of the pathspec (iow, if there is a source of\nrename/copy into the created path), and replaces that creation record with\na rename record in the diff_queue. When this is done, we do not want the\nregular resolve_rename_copy() to kick in (i.e. the \"follow\" hack already\nmade a pair).\n\nBut what 1da6175 (Make diffcore_std only can run once before a diff_flush,\n2010-05-06) did is clearly wrong. Not wanting to call resolve-rename-copy\ndoes not mean we do not want to run the rest of what diffcore_std() does\nat all! For example, \"-S\" and \"--diff-filter=\" options are processed in\nthat function; the exit status of the command based on the presense of\ndifference is computed in the function, too.\n\nAnother potential \"funny\" is this (unrelated to the reported issue).\n\nThe \"--follow\" logic is called from diff_tree_sha1() function, but the\ninput trees to diff_tree_sha1() are not necessarily the top-level trees\n(compare_tree_entry() calls it while it recursively descends into\nsubtrees). For example, with the example Constantine gave us, the first\n\"try-to-follow-renames\" call happens with the \"base\" set to \"platform/\"\nbut the rename source is actually \"lang-api/src/com/intellij/...\"\nhierarchy, so it is a wasted call. I think we only want to run the rename\nfollowing at the very top level, i.e. like the attached patch.\n\nLinus, what do you think? Am I missing something obvious?\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 1fb3e94..5b68c08 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -412,7 +412,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha\n \tinit_tree_desc(&t1, tree1, size1);\n \tinit_tree_desc(&t2, tree2, size2);\n \tretval = diff_tree(&t1, &t2, base, opt);\n-\tif (DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {\n+\tif (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {\n \t\tinit_tree_desc(&t1, tree1, size1);\n \t\tinit_tree_desc(&t2, tree2, size2);\n \t\ttry_to_follow_renames(&t1, &t2, base, opt);\n"},{"id":"148011","messageId":"AANLkTinSUy9pDtsJizmQTTOsMOVEQ=7+U5Ri4=-RZmB-@mail.gmail.com","threadId":"24728","inReplyTo":"7vhbiyl8ji.fsf@alter.siamese.dyndns.org","subject":"Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-08-13T17:53:25Z","receivedAt":"2010-08-13T17:53:25Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Aug 13, 2010 at 10:36 AM, Junio C Hamano wrote:\n>\n> Another potential \"funny\" is this (unrelated to the reported issue).\n>\n> The \"--follow\" logic is called from diff_tree_sha1() function, but the\n> input trees to diff_tree_sha1() are not necessarily the top-level trees\n> (compare_tree_entry() calls it while it recursively descends into\n> subtrees).  For example, with the example Constantine gave us, the first\n> \"try-to-follow-renames\" call happens with the \"base\" set to \"platform/\"\n> but the rename source is actually \"lang-api/src/com/intellij/...\"\n> hierarchy, so it is a wasted call.  I think we only want to run the rename\n> following at the very top level, i.e. like the attached patch.\n>\n> Linus, what do you think?  Am I missing something obvious?\n\nI think you're right. You're certainly not missing anythng _obvious_.\nIirc the only reason I put that 'follow' hack in diff_tree_sha1() was\nbecause that way I didn't need to worry about all the callers, but I\ndidn't even think about the whole recursion issue.\n\nSo ack on that.\n\n Linus\n"},{"id":"148014","messageId":"7vpqxmjphl.fsf@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"7vhbiyl8ji.fsf@alter.siamese.dyndns.org","subject":"Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-13T19:12:54Z","receivedAt":"2010-08-13T19:12:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Constantine Plotnikov writes:\n>\n>> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename\n>> detection started to fail with fatal error on some files in our\n>> repository. The bug could be seen on the public IntelliJ IDEA\n>> repository (about 760M in size), but our users have reported it as\n>> well.\n> ...\n> But what 1da6175 (Make diffcore_std only can run once before a diff_flush,\n> 2010-05-06) did is clearly wrong. Not wanting to call resolve-rename-copy\n> does not mean we do not want to run the rest of what diffcore_std() does\n> at all! For example, \"-S\" and \"--diff-filter=\" options are processed in\n> that function; the exit status of the command based on the presense of\n> difference is computed in the function, too.\n\nThis reverts 1da6175 (Make diffcore_std only can run once before a\ndiff_flush, 2010-05-06) and replaces it with an uglier looking but\nhopefully correct fix.\n\nConstantine, does it fix your issue?\n\n diff.c | 27 +++++++++++++--------------\n diff.h | 3 +++\n diffcore.h | 2 --\n tree-diff.c | 12 ++++++++++++\n 4 files changed, 28 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex bf65892..9300492 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4064,25 +4064,24 @@ void diffcore_fix_diff_index(struct diff_options *options)\n \n void diffcore_std(struct diff_options *options)\n {\n-\t/* We never run this function more than one time, because the\n-\t * rename/copy detection logic can only run once.\n-\t */\n-\tif (diff_queued_diff.run)\n-\t\treturn;\n-\n \tif (options->skip_stat_unmatch)\n \t\tdiffcore_skip_stat_unmatch(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_break(options->break_opt);\n-\tif (options->detect_rename)\n-\t\tdiffcore_rename(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_merge_broken();\n+\tif (!options->found_follow) {\n+\t\t/* See try_to_follow_renames() in tree-diff.c */\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_break(options->break_opt);\n+\t\tif (options->detect_rename)\n+\t\t\tdiffcore_rename(options);\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_merge_broken();\n+\t}\n \tif (options->pickaxe)\n \t\tdiffcore_pickaxe(options->pickaxe, options->pickaxe_opts);\n \tif (options->orderfile)\n \t\tdiffcore_order(options->orderfile);\n-\tdiff_resolve_rename_copy();\n+\tif (!options->found_follow)\n+\t\t/* See try_to_follow_renames() in tree-diff.c */\n+\t\tdiff_resolve_rename_copy();\n \tdiffcore_apply_filter(options->filter);\n \n \tif (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n@@ -4090,7 +4089,7 @@ void diffcore_std(struct diff_options *options)\n \telse\n \t\tDIFF_OPT_CLR(options, HAS_CHANGES);\n \n-\tdiff_queued_diff.run = 1;\n+\toptions->found_follow = 0;\n }\n \n int diff_result_code(struct diff_options *opt, int status)\ndiff --git a/diff.h b/diff.h\nindex 063d10a..6fff024 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -126,6 +126,9 @@ struct diff_options {\n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\n \n+\t/* to support internal diff recursion by --follow hack*/\n+\tint found_follow;\n+\n \tFILE *file;\n \tint close_file;\n \ndiff --git a/diffcore.h b/diffcore.h\nindex 05ebc11..8b3241a 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -91,13 +91,11 @@ struct diff_queue_struct {\n \tstruct diff_filepair **queue;\n \tint alloc;\n \tint nr;\n-\tint run;\n };\n #define DIFF_QUEUE_CLEAR(q) \\\n \tdo { \\\n \t\t(q)->queue = NULL; \\\n \t\t(q)->nr = (q)->alloc = 0; \\\n-\t\t(q)->run = 0; \\\n \t} while (0)\n \n extern struct diff_queue_struct diff_queued_diff;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 5b68c08..293cc91 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -350,6 +350,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tdiff_opts.single_follow = opt->paths[0];\n \tdiff_opts.break_opt = opt->break_opt;\n+\n \tpaths[0] = NULL;\n \tdiff_tree_setup_paths(paths, &diff_opts);\n \tif (diff_setup_done(&diff_opts) < 0)\n@@ -359,6 +360,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_tree_release_paths(&diff_opts);\n \n \t/* Go through the new set of filepairing, and see if we find a more interesting one */\n+\topt->found_follow = 0;\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n@@ -376,6 +378,16 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \t\t\tdiff_tree_release_paths(opt);\n \t\t\topt->paths[0] = xstrdup(p->one->path);\n \t\t\tdiff_tree_setup_paths(opt->paths, opt);\n+\n+\t\t\t/*\n+\t\t\t * The caller expects us to return a set of vanilla\n+\t\t\t * filepairs to let a later call to diffcore_std()\n+\t\t\t * it makes to sort the renames out (among other\n+\t\t\t * things), but we already have found renames\n+\t\t\t * ourselves; signal diffcore_std() not to muck with\n+\t\t\t * rename information.\n+\t\t\t */\n+\t\t\topt->found_follow = 1;\n \t\t\tbreak;\n \t\t}\n \t}\n"},{"id":"148016","messageId":"7veie2jnww.fsf_-_@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"7vpqxmjphl.fsf@alter.siamese.dyndns.org","subject":"[PATCH] diff --follow: do call diffcore_std() as necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-13T19:46:55Z","receivedAt":"2010-08-13T19:46:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Usually, diff frontends populate the output queue with filepairs without\nany rename information and call diffcore_std() to sort the renames out.\nWhen --follow is in effect, however, diff-tree family of frontend has a\nhack that looks like this:\n\n diff-tree frontend\n -> diff_tree_sha1()\n . populate diff_queued_diff\n . if --follow is in effect and there is only one change that\n creates the target path, then\n -> try_to_follow_renames()\n\t -> diff_tree_sha1() with no pathspec but with -C\n\t -> diffcore_std() to find renames\n\t . if rename is found, tweak diff_queued_diff and put a\n\t single filepair that records the found rename there\n -> diffcore_std()\n . tweak elements on diff_queued_diff by\n - rename detection\n - path ordering\n - pickaxe filtering\n\nWe need to skip parts of the second call to diffcore_std() that is related\nto rename detection, and do so only when try_to_follow_renames() did find\na rename. Earlier 1da6175 (Make diffcore_std only can run once before a\ndiff_flush, 2010-05-06) tried to deal with this issue incorrectly; it\nunconditionally disabled any second call to diffcore_std().\n\nThis hopefully fixes the breakage.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with a proposed commit message for a change. Still seems to\n pass the test in 0cdca13 (Make git log --follow find copies among\n unmodified files., 2010-05-06), presumably added for 1da6175.\n\n diff.c | 27 +++++++++++++--------------\n diff.h | 3 +++\n diffcore.h | 2 --\n tree-diff.c | 11 +++++++++++\n 4 files changed, 27 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex bf65892..9300492 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4064,25 +4064,24 @@ void diffcore_fix_diff_index(struct diff_options *options)\n \n void diffcore_std(struct diff_options *options)\n {\n-\t/* We never run this function more than one time, because the\n-\t * rename/copy detection logic can only run once.\n-\t */\n-\tif (diff_queued_diff.run)\n-\t\treturn;\n-\n \tif (options->skip_stat_unmatch)\n \t\tdiffcore_skip_stat_unmatch(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_break(options->break_opt);\n-\tif (options->detect_rename)\n-\t\tdiffcore_rename(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_merge_broken();\n+\tif (!options->found_follow) {\n+\t\t/* See try_to_follow_renames() in tree-diff.c */\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_break(options->break_opt);\n+\t\tif (options->detect_rename)\n+\t\t\tdiffcore_rename(options);\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_merge_broken();\n+\t}\n \tif (options->pickaxe)\n \t\tdiffcore_pickaxe(options->pickaxe, options->pickaxe_opts);\n \tif (options->orderfile)\n \t\tdiffcore_order(options->orderfile);\n-\tdiff_resolve_rename_copy();\n+\tif (!options->found_follow)\n+\t\t/* See try_to_follow_renames() in tree-diff.c */\n+\t\tdiff_resolve_rename_copy();\n \tdiffcore_apply_filter(options->filter);\n \n \tif (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n@@ -4090,7 +4089,7 @@ void diffcore_std(struct diff_options *options)\n \telse\n \t\tDIFF_OPT_CLR(options, HAS_CHANGES);\n \n-\tdiff_queued_diff.run = 1;\n+\toptions->found_follow = 0;\n }\n \n int diff_result_code(struct diff_options *opt, int status)\ndiff --git a/diff.h b/diff.h\nindex 063d10a..6fff024 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -126,6 +126,9 @@ struct diff_options {\n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\n \n+\t/* to support internal diff recursion by --follow hack*/\n+\tint found_follow;\n+\n \tFILE *file;\n \tint close_file;\n \ndiff --git a/diffcore.h b/diffcore.h\nindex 05ebc11..8b3241a 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -91,13 +91,11 @@ struct diff_queue_struct {\n \tstruct diff_filepair **queue;\n \tint alloc;\n \tint nr;\n-\tint run;\n };\n #define DIFF_QUEUE_CLEAR(q) \\\n \tdo { \\\n \t\t(q)->queue = NULL; \\\n \t\t(q)->nr = (q)->alloc = 0; \\\n-\t\t(q)->run = 0; \\\n \t} while (0)\n \n extern struct diff_queue_struct diff_queued_diff;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 5b68c08..cd659c6 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -359,6 +359,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \tdiff_tree_release_paths(&diff_opts);\n \n \t/* Go through the new set of filepairing, and see if we find a more interesting one */\n+\topt->found_follow = 0;\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n@@ -376,6 +377,16 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \t\t\tdiff_tree_release_paths(opt);\n \t\t\topt->paths[0] = xstrdup(p->one->path);\n \t\t\tdiff_tree_setup_paths(opt->paths, opt);\n+\n+\t\t\t/*\n+\t\t\t * The caller expects us to return a set of vanilla\n+\t\t\t * filepairs to let a later call to diffcore_std()\n+\t\t\t * it makes to sort the renames out (among other\n+\t\t\t * things), but we already have found renames\n+\t\t\t * ourselves; signal diffcore_std() not to muck with\n+\t\t\t * rename information.\n+\t\t\t */\n+\t\t\topt->found_follow = 1;\n \t\t\tbreak;\n \t\t}\n \t}\n-- \n1.7.2.1.186.gffe84\n"},{"id":"148037","messageId":"AANLkTin-qPe-meW_kV5vtC0BT8QS-sZ90BrckRAbjk9X@mail.gmail.com","threadId":"24728","inReplyTo":"7veie2jnww.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff --follow: do call diffcore_std() as necessary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-13T21:27:38Z","receivedAt":"2010-08-13T21:27:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Aug 13, 2010 at 19:46, Junio C Hamano wrote:\n\n> This hopefully fixes the breakage.\n\nHopefully. It'd also be nice if we had a regression test for this, but\nI don't know how hard that would be to arrange. If it's hard to\nreproduce we might get away with a filter-branch + subdir filter from\nthe idea repository.\n"},{"id":"148051","messageId":"7vzkwqi10w.fsf@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"AANLkTin-qPe-meW_kV5vtC0BT8QS-sZ90BrckRAbjk9X@mail.gmail.com","subject":"Re: [PATCH] diff --follow: do call diffcore_std() as necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-13T22:46:39Z","receivedAt":"2010-08-13T22:46:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Fri, Aug 13, 2010 at 19:46, Junio C Hamano wrote:\n>\n>> This hopefully fixes the breakage.\n>\n> Hopefully. It'd also be nice if we had a regression test for this, but\n> I don't know how hard that would be to arrange. If it's hard to\n> reproduce we might get away with a filter-branch + subdir filter from\n> the idea repository.\n\nAs I wrote, the test added by 0cdca13 breaks with just a reversion of\n1da6175 but with this patch it passes. I didn't run any other test,\nthough ;-).\n"},{"id":"148067","messageId":"1281748247-8180-1-git-send-email-avarab@gmail.com","threadId":"24728","inReplyTo":"7vzkwqi10w.fsf@alter.siamese.dyndns.org","subject":"[PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-14T01:10:47Z","receivedAt":"2010-08-14T01:10:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a regression test for the git log -M --follow --name-only bug\nintroduced in v1.7.2-rc0~103^2~2\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4202-log.sh | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 95ac3f8..ff624f4 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '\n \n '\n \n+test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '\n+\t# Needs an unrelated root commit\n+\ttest_commit README &&\n+\t>Foo.bar &&\n+\tgit add Foo.bar &&\n+\tgit commit --allow-empty-message wrote:\n> Add a regression test for the git log -M --follow --name-only bug\n> introduced in v1.7.2-rc0~103^2~2\n\nAKA \"we didn't have any tests for log's --name-only *at all*\".\n"},{"id":"148128","messageId":"7v39uggs5h.fsf@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"AANLkTi=Na_K=9oXM7iyeKodWXyXuSy-0UL792igTEjEe@mail.gmail.com","subject":"Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-15T09:08:10Z","receivedAt":"2010-08-15T09:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Sat, Aug 14, 2010 at 01:10, Ævar Arnfjörð Bjarmason wrote:\n>> Add a regression test for the git log -M --follow --name-only bug\n>> introduced in v1.7.2-rc0~103^2~2\n>\n> AKA \"we didn't have any tests for log's --name-only *at all*\".\n\nBut this is not related to --name-only at all; anything that is \"diff\"\nrelated, e.g. -p, --stat, --name-status, will share the same issue.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 95ac3f8..ff624f4 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '\n> \n> '\n> \n> +test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '\n\nThis is uninformative and ugly at the same time.\n\n - Can't we describe the nature of the situation where the old bug\n triggers concisely? Perhaps 'show added path under \"--follow -M\"?'\n\n - All others begin with lowercase.\n\n> +\t# Needs an unrelated root commit\n> +\ttest_commit README &&\n\nThis is not a \"root\" commit, is it?\n\n> +\t>Foo.bar &&\n> +\tgit add Foo.bar &&\n> +\tgit commit --allow-empty-message +\tgit log -M --follow --name-only Foo.bar\n> +'\n> +\n> test_done\n"},{"id":"148135","messageId":"AANLkTi=PAW_Owy_-DSQ32sboB28373Gb_aySbpeprwLg@mail.gmail.com","threadId":"24728","inReplyTo":"7v39uggs5h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-15T09:24:16Z","receivedAt":"2010-08-15T09:24:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Aug 15, 2010 at 09:08, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Sat, Aug 14, 2010 at 01:10, Ævar Arnfjörð Bjarmason wrote:\n>>> Add a regression test for the git log -M --follow --name-only bug\n>>> introduced in v1.7.2-rc0~103^2~2\n>>\n>> AKA \"we didn't have any tests for log's --name-only *at all*\".\n>\n> But this is not related to --name-only at all; anything that is \"diff\"\n> related, e.g. -p, --stat, --name-status, will share the same issue.\n\nI meant that as an extra benefit this is the first test for log +\n--name-only.\n\n>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n>> index 95ac3f8..ff624f4 100755\n>> --- a/t/t4202-log.sh\n>> +++ b/t/t4202-log.sh\n>> @@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '\n>>\n>>  '\n>>\n>> +test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '\n>\n> This is uninformative and ugly at the same time.\n>\n>  - Can't we describe the nature of the situation where the old bug\n>   triggers concisely?  Perhaps 'show added path under \"--follow -M\"?'\n\nI didn't grok why this was happening, but yeah, that description is\nbetter.\n\n>> +     # Needs an unrelated root commit\n>> +     test_commit README &&\n>\n> This is not a \"root\" commit, is it?\n\ns/root/first/\n\n>> +     >Foo.bar &&\n>> +     git add Foo.bar &&\n>> +     git commit --allow-empty-message \n> Does emptiness of the message matter?\n\nNo, I was just going for a minimal test case, no commit message is\nmore minimal than having one.\n"},{"id":"148134","messageId":"1281867385-30545-1-git-send-email-avarab@gmail.com","threadId":"24728","inReplyTo":"7v39uggs5h.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] log: test for regression introduced in v1.7.2-rc0~103^2~2","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-15T10:16:25Z","receivedAt":"2010-08-15T10:16:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a regression test for the git log -M --follow $diff_option bug\nintroduced in v1.7.2-rc0~103^2~2, $diff_option being diff related\noptions like -p, --stat, --name-only etc.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nVersion two of this test case, simpler, and takes into account\ncommentary from Junio.\n\n t/t4202-log.sh | 13 +++++++++++++\n 1 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 95ac3f8..a0be122 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -441,5 +441,18 @@ test_expect_success 'log.decorate configuration' '\n \n '\n \n+test_expect_success 'show added path under \"--follow -M\"' '\n+\t# This tests for a regression introduced in v1.7.2-rc0~103^2~2\n+\ttest_create_repo regression &&\n+\t(\n+\t\tcd regression &&\n+\t\ttest_commit needs-another-commit &&\n+\t\ttest_commit Foo.bar &&\n+\t\tgit log -M --follow -p Foo.bar.t &&\n+\t\tgit log -M --follow --stat Foo.bar.t &&\n+\t\tgit log -M --follow --name-only Foo.bar.t\n+\t)\n+'\n+\n test_done\n \n-- \n1.7.2.1.339.gfad93\n"},{"id":"148171","messageId":"7vaaonfhs8.fsf@alter.siamese.dyndns.org","threadId":"24728","inReplyTo":"AANLkTi=PAW_Owy_-DSQ32sboB28373Gb_aySbpeprwLg@mail.gmail.com","subject":"Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-16T01:49:43Z","receivedAt":"2010-08-16T01:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> +     # Needs an unrelated root commit\n>>> +     test_commit README &&\n>>\n>> This is not a \"root\" commit, is it?\n>\n> s/root/first/\n\nIt is not even the first commit, is it? It comes on top of whatever\ncommits that earlier tests left.\n\n>>> +     >Foo.bar &&\n>>> +     git add Foo.bar &&\n>>> +     git commit --allow-empty-message >\n>> Does emptiness of the message matter?\n>\n> No, I was just going for a minimal test case, no commit message is\n> more minimal than having one.\n\nI do not think having to write \"--allow-empty-message wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>>> +     # Needs an unrelated root commit\n>>>> +     test_commit README &&\n>>>\n>>> This is not a \"root\" commit, is it?\n>>\n>> s/root/first/\n>\n> It is not even the first commit, is it?  It comes on top of whatever\n> commits that earlier tests left.\n>\n>>>> +     >Foo.bar &&\n>>>> +     git add Foo.bar &&\n>>>> +     git commit --allow-empty-message >>\n>>> Does emptiness of the message matter?\n>>\n>> No, I was just going for a minimal test case, no commit message is\n>> more minimal than having one.\n>\n> I do not think having to write \"--allow-empty-message aiming for being minimal; it is doing something unusual after all.\n>\n> If you do not remember why you added this test 6 months down the road,\n> wouldn't you be confused to think maybe the commit has to be unusual in\n> that it has to lack the message to trigger the bug?\n\nMy v2 patch should address both of these issues.\n"},{"id":"148279","messageId":"AANLkTi=wWii8ep78G7OuyFQ+W9xsm6O-WVZBGyPJjg-p@mail.gmail.com","threadId":"24728","inReplyTo":"7vpqxmjphl.fsf@alter.siamese.dyndns.org","subject":"Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2010-08-17T14:48:59Z","receivedAt":"2010-08-17T14:48:59Z","isPatch":false,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"On Fri, Aug 13, 2010 at 11:12 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Constantine Plotnikov writes:\n>>\n>>> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename\n>>> detection started to fail with fatal error on some files in our\n>>> repository. The bug could be seen on the public IntelliJ IDEA\n>>> repository (about 760M in size), but our users have reported it as\n>>> well.\n>> ...\n>> But what 1da6175 (Make diffcore_std only can run once before a diff_flush,\n>> 2010-05-06) did is clearly wrong.  Not wanting to call resolve-rename-copy\n>> does not mean we do not want to run the rest of what diffcore_std() does\n>> at all!  For example, \"-S\" and \"--diff-filter=\" options are processed in\n>> that function; the exit status of the command based on the presense of\n>> difference is computed in the function, too.\n>\n> This reverts 1da6175 (Make diffcore_std only can run once before a\n> diff_flush, 2010-05-06) and replaces it with an uglier looking but\n> hopefully correct fix.\n>\n> Constantine, does it fix your issue?\n>\nYes. It fixes the issue on the known problematic files. The patch was\ntested over 3235b7053 in the git.git master. Please apply it to the\nnext update.\n\nThank You,\nConstantine\n"}]}