{"thread":{"id":"10962","subject":"[RFC] use typechange as rename source","startedAt":"2007-11-21T17:12:36Z","lastAt":"2007-12-03T01:20:22Z","messageCount":14,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"60547","messageId":"20071121171235.GA32233@sigill.intra.peff.net","threadId":"10962","inReplyTo":null,"subject":"[RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-21T17:12:36Z","receivedAt":"2007-11-21T17:12:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Today in one of my repositories I did something like this:\n\n  $ mv foo bar\n  $ ln -s bar foo\n  $ git add .\n\nand I expected git-status to claim:\n\n  typechange: foo\n  renamed:    foo -> bar\n\nbut it didn't find the rename (without -C) because the path 'foo' still\nexists. So there is a disconnect in what git and I think of as \"exists\".\nShould typechanges make a file eligible as a rename src?\n\nA quickie patch to implement this is:\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex f9ebea5..5a34e8a 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -417,6 +417,10 @@ void diffcore_rename(struct diff_options *options)\n \t\t\t\tp->one->rename_used++;\n \t\t\tregister_rename_src(p->one, p->score);\n \t\t}\n+\t\telse if (DIFF_PAIR_TYPE_CHANGED(p)) {\n+\t\t\tp->one->rename_used++;\n+\t\t\tregister_rename_src(p->one, p->score);\n+\t\t}\n \t\telse if (detect_rename == DIFF_DETECT_COPY) {\n \t\t\t/*\n \t\t\t * Increment the \"rename_used\" score by\n\nThere are a few add-on questions:\n\n  - should typechanges in both directions be used, or just file ->\n    symlink?\n\n  - this actually produces a 'copied' status rather than a 'renamed'\n    since the 'foo' entry does still exist. Is this reasonable?\n\n-Peff\n"},{"id":"61334","messageId":"7vir3l2a1i.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"20071121171235.GA32233@sigill.intra.peff.net","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-29T00:02:49Z","receivedAt":"2007-11-29T00:02:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are a few add-on questions:\n>\n>   - should typechanges in both directions be used, or just file ->\n>     symlink?\n>\n>   - this actually produces a 'copied' status rather than a 'renamed'\n>     since the 'foo' entry does still exist. Is this reasonable?\n\nI do not think this is a risky change; it won't add too many rename\nsources we did not consider traditionally (typechanges are usually rare\nevent anyway).\n\nYou are copying the source to elsewhere and then completely rewriting it\n(even making it into a different type), so I do not think 'copied' is so\nunreasonable.  An alternative would be to say you renamed it and then\ncreated something totally different, which would also be reasonable.\n"},{"id":"61396","messageId":"20071129141452.GA32670@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7vir3l2a1i.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-29T14:14:53Z","receivedAt":"2007-11-29T14:14:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 28, 2007 at 04:02:49PM -0800, Junio C Hamano wrote:\n\n> I do not think this is a risky change; it won't add too many rename\n> sources we did not consider traditionally (typechanges are usually rare\n> event anyway).\n\nOK. What next? Did the patch I sent make sense? Do you want a cleaned up\nversion with a commit message and signoff, or does it need work?\n\n> You are copying the source to elsewhere and then completely rewriting it\n> (even making it into a different type), so I do not think 'copied' is so\n> unreasonable.  An alternative would be to say you renamed it and then\n> created something totally different, which would also be reasonable.\n\nI am inclined to leave it as 'copied' for now because it makes some\nsense and is the least invasive change (and is independent of adding the\nrename source, so we can always change it later).\n\n-Peff\n"},{"id":"61453","messageId":"7vmyswsfl6.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"20071129141452.GA32670@coredump.intra.peff.net","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-30T01:10:45Z","receivedAt":"2007-11-30T01:10:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> OK. What next? Did the patch I sent make sense? Do you want a cleaned up\n> version with a commit message and signoff, or does it need work?\n\nIt just hit me that breaking (as in diffcore-break) a filepair that is a\ntypechange may yield the same result, and if it works, that would be\nconceptually cleaner.  After all, a typechange is the ultimate form of\ntotal rewriting (the similarity between the preimage and the postimage\nis very low -- even their types are different, let alone contents).\n\nCompared to that, the rename_used++ in that codepath you touched feels\nmore magic to me.\n"},{"id":"61463","messageId":"20071130015716.GA15224@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7vmyswsfl6.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-30T01:57:16Z","receivedAt":"2007-11-30T01:57:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 29, 2007 at 05:10:45PM -0800, Junio C Hamano wrote:\n\n> > OK. What next? Did the patch I sent make sense? Do you want a cleaned up\n> > version with a commit message and signoff, or does it need work?\n> \n> It just hit me that breaking (as in diffcore-break) a filepair that is a\n> typechange may yield the same result, and if it works, that would be\n> conceptually cleaner.  After all, a typechange is the ultimate form of\n> total rewriting (the similarity between the preimage and the postimage\n> is very low -- even their types are different, let alone contents).\n> \n> Compared to that, the rename_used++ in that codepath you touched feels\n> more magic to me.\n\nI have always been a bit confused about diffcore-break, so I am probably\nmisunderstanding what you mean. But are you saying that\ndiffcore-break.c:should_break should return 1 for typechanges? If so,\nthat does not have the desired effect.\n\n-Peff\n"},{"id":"61549","messageId":"7vsl2n87jr.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"20071130015716.GA15224@coredump.intra.peff.net","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-01T02:36:56Z","receivedAt":"2007-12-01T02:36:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I have always been a bit confused about diffcore-break, so I am probably\n> misunderstanding what you mean. But are you saying that\n> diffcore-break.c:should_break should return 1 for typechanges?\n\nWhat I had in mind was to do something like that in spirit, but instead\nbreak such a filepair inside diffcore-rename (iow, even when the user\ndid not say -B) early on.\n\nBut after re-reading your patch and the surrounding code, that is\nmore or less what you are doing (without actually recording the extra\nbroken pair to be merged back later).\n\nIf we did the \"automatic break of typechange\" early, instead of your\npatch, when we come to the register_rename_src() loop, one half of the\nbroken pair (i.e. \"create a new symlink here\") will be processed\nin this part of the loop:\n\n\t\tif (!DIFF_FILE_VALID(p->one)) {\n\t\t\tif (!DIFF_FILE_VALID(p->two))\n\t\t\t\tcontinue; /* unmerged */\n\t\t\telse if (options->single_follow &&\n\t\t\t\t strcmp(options->single_follow, p->two->path))\n\t\t\t\tcontinue; /* not interested */\n\t\t\telse\n\t\t\t\tlocate_rename_dst(p->two, 1);\n\t\t}\n\nand rename_dst is registered here.  The other half (i.e. \"remove the\nregular file\") will be caught by this part in the loop:\n\n\t\telse if (!DIFF_FILE_VALID(p->two)) {\n\t\t\t/*\n\t\t\t * If the source is a broken \"delete\", and\n\t\t\t * they did not really want to get broken,\n\t\t\t * that means the source actually stays.\n\t\t\t * So we increment the \"rename_used\" score\n\t\t\t * by one, to indicate ourselves as a user\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}\n\nto register a source candidate.\n\nInstead your patch does that with a single:\n\n+\t\telse if (DIFF_PAIR_TYPE_CHANGED(p)) {\n+\t\t\tp->one->rename_used++;\n+\t\t\tregister_rename_src(p->one, p->score);\n+\t\t}\n\nwhich is essentially doing the same thing but only for the \"remove the\nregular file\" half.  One has to wonder how the lack of handling the\nother half affects the outcome and still produce a result more intuitive\nthan the current code.\n\nIn your test case, the \"new\" symlink won't have any similar symlink that\nis removed from the preimage, so registering it as a rename destination\nwould not make a difference (it will say \"no match found, so create this\nas usual\"), but I am not convinced if that would work well in general.\n"},{"id":"61559","messageId":"20071201043407.GD30725@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7vsl2n87jr.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-01T04:34:07Z","receivedAt":"2007-12-01T04:34:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 30, 2007 at 06:36:56PM -0800, Junio C Hamano wrote:\n\n> > I have always been a bit confused about diffcore-break, so I am probably\n> > misunderstanding what you mean. But are you saying that\n> > diffcore-break.c:should_break should return 1 for typechanges?\n> \n> What I had in mind was to do something like that in spirit, but instead\n> break such a filepair inside diffcore-rename (iow, even when the user\n> did not say -B) early on.\n\nAh, I see. BTW, I totally screwed up the tests I did earlier. Returning\n1 from should_break _does_ produce the same results for my simple case\n(copy + typechange).\n\n> But after re-reading your patch and the surrounding code, that is\n> more or less what you are doing (without actually recording the extra\n> broken pair to be merged back later).\n\nI don't think we need to, because they are never actually \"broken\"; we\nsimply consider the source a candidate for renaming, but keep the pair\ntogether to note the typechange.\n\n> which is essentially doing the same thing but only for the \"remove the\n> regular file\" half.  One has to wonder how the lack of handling the\n> other half affects the outcome and still produce a result more intuitive\n> than the current code.\n\nAIUI, because we never broke the pair in the first place, we don't need\nto look for a source for that dest (the \"add a new symlink\" half). It's\nalready part of the same filepair.\n\nWhether this is by design or simply a happy accident that we record both\nrenames and typechanges in diff_filepairs, I'm not sure. Or perhaps I'm\ntotally misunderstanding how the breaking works.\n\n> In your test case, the \"new\" symlink won't have any similar symlink that\n> is removed from the preimage, so registering it as a rename destination\n> would not make a difference (it will say \"no match found, so create this\n> as usual\"), but I am not convinced if that would work well in general.\n\nI don't know that it makes a difference. We are impacting only a\n'typechange', which implies that we have a filepair in which both p->one\nand p->two are valid; thus, the current code doesn't use it as a rename\ndst at all.\n\n-Peff\n"},{"id":"61560","messageId":"7vhcj36j6e.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"20071201043407.GD30725@coredump.intra.peff.net","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-01T06:08:41Z","receivedAt":"2007-12-01T06:08:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> In your test case, the \"new\" symlink won't have any similar symlink that\n>> is removed from the preimage, so registering it as a rename destination\n>> would not make a difference (it will say \"no match found, so create this\n>> as usual\"), but I am not convinced if that would work well in general.\n>\n> I don't know that it makes a difference. We are impacting only a\n> 'typechange', which implies that we have a filepair in which both p->one\n> and p->two are valid; thus, the current code doesn't use it as a rename\n> dst at all.\n\nI think it does make a difference, if you have a cross rename that\nswaps:\n\n\t$ ls -F foo bar\n        bar\tfoo@\n        $ mv foo tmp; mv bar foo; mv tmp bar\n        $ ls -F foo bar\n        bar@\tfoo\n\nThe attached patch does not automatically break a filepair that is a\ntypechange, so the new t4023 test asks for diffcore-break with -B\nexplicitly.\n\nI suspect your patch alone would not pass t4023 (with or without -B).\n\nWe may want to do the autobreak in diffcore-rename even when -B is not\nin effect on top of this patch, but that is a separate topic.\n\n-- >8 --\nSubject: [PATCH] rename: Break filepairs with different types.\n\nWhen we consider if a path has been totally rewritten, we did not\ntouch changes from symlinks to files or vice versa.  But a change\nthat modifies even the type of a blob surely should count as a\ncomplete rewrite.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h                           |    7 +++\n diffcore-break.c                  |    7 ++-\n t/t4023-diff-rename-typechange.sh |   86 +++++++++++++++++++++++++++++++++++++\n tree-walk.h                       |    7 ---\n 4 files changed, 97 insertions(+), 10 deletions(-)\n create mode 100755 t/t4023-diff-rename-typechange.sh\n\ndiff --git a/cache.h b/cache.h\nindex aaa135b..d0e7a71 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -192,6 +192,13 @@ enum object_type {\n \tOBJ_MAX,\n };\n \n+static inline enum object_type object_type(unsigned int mode)\n+{\n+\treturn S_ISDIR(mode) ? OBJ_TREE :\n+\t\tS_ISGITLINK(mode) ? OBJ_COMMIT :\n+\t\tOBJ_BLOB;\n+}\n+\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex c71a226..69afc07 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -52,8 +52,8 @@ static int should_break(struct diff_filespec *src,\n \t\t\t     * is the default.\n \t\t\t     */\n \n-\tif (!S_ISREG(src->mode) || !S_ISREG(dst->mode))\n-\t\treturn 0; /* leave symlink rename alone */\n+\tif (object_type(src->mode) != object_type(dst->mode))\n+\t\treturn 1; /* even their types are different */\n \n \tif (src->sha1_valid && dst->sha1_valid &&\n \t    !hashcmp(src->sha1, dst->sha1))\ndiff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh\nnew file mode 100755\nindex 0000000..255604e\n--- /dev/null\n+++ b/t/t4023-diff-rename-typechange.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='typechange rename detection'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tln -s linklink bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Initial &&\n+\tgit tag one &&\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >bar &&\n+\tln -s linklink foo &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Second &&\n+\tgit tag two &&\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tgit add foo &&\n+\tgit commit -a -m Third &&\n+\tgit tag three &&\n+\n+\tmv foo bar &&\n+\tln -s linklink foo &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Fourth &&\n+\tgit tag four &&\n+\n+\t# This is purely for sanity check\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tcat ../../Makefile >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Fifth &&\n+\tgit tag five &&\n+\n+\trm -f foo bar &&\n+\tcat ../../Makefile >foo &&\n+\tcat ../../COPYING >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Sixth &&\n+\tgit tag six\n+\n+'\n+\n+test_expect_success 'cross renames to be detected for regular files' '\n+\n+\tgit diff-tree five six -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"R100\tbar\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_expect_success 'cross renames to be detected for typechange' '\n+\n+\tgit diff-tree one two -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"R100\tbar\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_expect_success 'moves and renames' '\n+\n+\tgit diff-tree three four -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"T100\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_done\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 903a7b0..db0fbdc 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -7,13 +7,6 @@ struct name_entry {\n \tunsigned int mode;\n };\n \n-static inline enum object_type object_type(unsigned int mode)\n-{\n-\treturn S_ISDIR(mode) ? OBJ_TREE :\n-\t\tS_ISGITLINK(mode) ? OBJ_COMMIT :\n-\t\tOBJ_BLOB;\n-}\n-\n struct tree_desc {\n \tconst void *buffer;\n \tstruct name_entry entry;\n-- \n1.5.3.6.2090.g4ece0\n"},{"id":"61562","messageId":"7v8x4f6iyu.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"7vhcj36j6e.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-01T06:13:13Z","receivedAt":"2007-12-01T06:13:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index c71a226..69afc07 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -52,8 +52,8 @@ static int should_break(struct diff_filespec *src,\n>  \t\t\t     * is the default.\n>  \t\t\t     */\n>  \n> -\tif (!S_ISREG(src->mode) || !S_ISREG(dst->mode))\n> -\t\treturn 0; /* leave symlink rename alone */\n> +\tif (object_type(src->mode) != object_type(dst->mode))\n> +\t\treturn 1; /* even their types are different */\n\nOops, this part is wrong.  It should be checking ISREG and stuff.\n"},{"id":"61563","messageId":"20071201061748.GA2606@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7v8x4f6iyu.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-01T06:17:48Z","receivedAt":"2007-12-01T06:17:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 30, 2007 at 10:08:41PM -0800, Junio C Hamano wrote:\n\n> I think it does make a difference, if you have a cross rename that\n> swaps:\n> \n> \t$ ls -F foo bar\n>         bar\tfoo@\n>         $ mv foo tmp; mv bar foo; mv tmp bar\n>         $ ls -F foo bar\n>         bar@\tfoo\n\nOK, I see now what you were saying before. Yes, we do want them actually\nbroken, and my initial patch is not right in that sense.\n\n> > -\tif (!S_ISREG(src->mode) || !S_ISREG(dst->mode))\n> > -\t\treturn 0; /* leave symlink rename alone */\n> > +\tif (object_type(src->mode) != object_type(dst->mode))\n> > +\t\treturn 1; /* even their types are different */\n> \n> Oops, this part is wrong.  It should be checking ISREG and stuff.\n\nOK, good. I was sitting there puzzling over what in the world this\nchange meant.\n\n-Peff\n"},{"id":"61564","messageId":"7vy7ce6hyb.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"7v8x4f6iyu.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-01T06:35:08Z","receivedAt":"2007-12-01T06:35:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry for the earlier broken one.  Here is the correct one I wanted to\nsend out.\n\nBy the way, this breaks some tests in t4008 (break-rewrite test) because\nthe test expects the behaviour you found counterintuitive (which was the\nreason we have this thread).\n\n-- >8 --\nSubject: rename: Break filepairs with different types.\n\nWhen we consider if a path has been totally rewritten, we did not\ntouch changes from symlinks to files or vice versa.  But a change\nthat modifies even the type of a blob surely should count as a\ncomplete rewrite.\n\nWhile we are at it, modernise diffcore-break to be aware of gitlinks (we\ndo not want to touch them).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n cache.h                           |    7 +++\n diffcore-break.c                  |   12 +++--\n t/t4023-diff-rename-typechange.sh |   86 +++++++++++++++++++++++++++++++++++++\n tree-walk.h                       |    7 ---\n 4 files changed, 101 insertions(+), 11 deletions(-)\n create mode 100755 t/t4023-diff-rename-typechange.sh\n\ndiff --git a/cache.h b/cache.h\nindex aaa135b..d0e7a71 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -192,6 +192,13 @@ enum object_type {\n \tOBJ_MAX,\n };\n \n+static inline enum object_type object_type(unsigned int mode)\n+{\n+\treturn S_ISDIR(mode) ? OBJ_TREE :\n+\t\tS_ISGITLINK(mode) ? OBJ_COMMIT :\n+\t\tOBJ_BLOB;\n+}\n+\n #define GIT_DIR_ENVIRONMENT \"GIT_DIR\"\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex c71a226..31cdcfe 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -52,8 +52,10 @@ static int should_break(struct diff_filespec *src,\n \t\t\t     * is the default.\n \t\t\t     */\n \n-\tif (!S_ISREG(src->mode) || !S_ISREG(dst->mode))\n-\t\treturn 0; /* leave symlink rename alone */\n+\tif (S_ISREG(src->mode) != S_ISREG(dst->mode)) {\n+\t\t*merge_score_p = (int)MAX_SCORE;\n+\t\treturn 1; /* even their types are different */\n+\t}\n \n \tif (src->sha1_valid && dst->sha1_valid &&\n \t    !hashcmp(src->sha1, dst->sha1))\n@@ -168,11 +170,13 @@ void diffcore_break(int break_score)\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tint score;\n \n-\t\t/* We deal only with in-place edit of non directory.\n+\t\t/*\n+\t\t * We deal only with in-place edit of blobs.\n \t\t * We do not break anything else.\n \t\t */\n \t\tif (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two) &&\n-\t\t    !S_ISDIR(p->one->mode) && !S_ISDIR(p->two->mode) &&\n+\t\t    object_type(p->one->mode) == OBJ_BLOB &&\n+\t\t    object_type(p->two->mode) == OBJ_BLOB &&\n \t\t    !strcmp(p->one->path, p->two->path)) {\n \t\t\tif (should_break(p->one, p->two,\n \t\t\t\t\t break_score, &score)) {\ndiff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh\nnew file mode 100755\nindex 0000000..255604e\n--- /dev/null\n+++ b/t/t4023-diff-rename-typechange.sh\n@@ -0,0 +1,86 @@\n+#!/bin/sh\n+\n+test_description='typechange rename detection'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tln -s linklink bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Initial &&\n+\tgit tag one &&\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >bar &&\n+\tln -s linklink foo &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Second &&\n+\tgit tag two &&\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tgit add foo &&\n+\tgit commit -a -m Third &&\n+\tgit tag three &&\n+\n+\tmv foo bar &&\n+\tln -s linklink foo &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Fourth &&\n+\tgit tag four &&\n+\n+\t# This is purely for sanity check\n+\n+\trm -f foo bar &&\n+\tcat ../../COPYING >foo &&\n+\tcat ../../Makefile >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Fifth &&\n+\tgit tag five &&\n+\n+\trm -f foo bar &&\n+\tcat ../../Makefile >foo &&\n+\tcat ../../COPYING >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m Sixth &&\n+\tgit tag six\n+\n+'\n+\n+test_expect_success 'cross renames to be detected for regular files' '\n+\n+\tgit diff-tree five six -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"R100\tbar\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_expect_success 'cross renames to be detected for typechange' '\n+\n+\tgit diff-tree one two -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"R100\tbar\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_expect_success 'moves and renames' '\n+\n+\tgit diff-tree three four -r --name-status -B -M | sort >actual &&\n+\t{\n+\t\techo \"R100\tfoo\tbar\"\n+\t\techo \"T100\tfoo\"\n+\t} | sort >expect &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_done\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 903a7b0..db0fbdc 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -7,13 +7,6 @@ struct name_entry {\n \tunsigned int mode;\n };\n \n-static inline enum object_type object_type(unsigned int mode)\n-{\n-\treturn S_ISDIR(mode) ? OBJ_TREE :\n-\t\tS_ISGITLINK(mode) ? OBJ_COMMIT :\n-\t\tOBJ_BLOB;\n-}\n-\n struct tree_desc {\n \tconst void *buffer;\n \tstruct name_entry entry;\n-- \n1.5.3.6.2090.g4ece0\n"},{"id":"61565","messageId":"20071201064916.GA7431@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7vy7ce6hyb.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-01T06:49:16Z","receivedAt":"2007-12-01T06:49:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 30, 2007 at 10:35:08PM -0800, Junio C Hamano wrote:\n\n> Sorry for the earlier broken one.  Here is the correct one I wanted to\n> send out.\n>\n> By the way, this breaks some tests in t4008 (break-rewrite test) because\n> the test expects the behaviour you found counterintuitive (which was the\n> reason we have this thread).\n\nOK, this looks reasonable to me, and it produces diff results (with -B)\nthat make sense for typechanges. Looking at the failing test in t4008, I\nlike the new behavior better.\n\nAnd maybe something like this on top for git-status?\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 0e0439f..e77120d 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)\n \trev.diffopt.format_callback_data = s;\n \trev.diffopt.detect_rename = 1;\n \trev.diffopt.rename_limit = 100;\n+\trev.diffopt.break_opt = 0;\n \twt_read_cache(s);\n \trun_diff_index(&rev, 1);\n }\n"},{"id":"61673","messageId":"7v8x4cykkj.fsf@gitster.siamese.dyndns.org","threadId":"10962","inReplyTo":"20071201064916.GA7431@coredump.intra.peff.net","subject":"Re: [RFC] use typechange as rename source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-02T19:15:40Z","receivedAt":"2007-12-02T19:15:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And maybe something like this on top for git-status?\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index 0e0439f..e77120d 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)\n>  \trev.diffopt.format_callback_data = s;\n>  \trev.diffopt.detect_rename = 1;\n>  \trev.diffopt.rename_limit = 100;\n> +\trev.diffopt.break_opt = 0;\n>  \twt_read_cache(s);\n>  \trun_diff_index(&rev, 1);\n>  }\n\nI have to wonder how much this is going to make things worse in the real\nworld, although I agree in the \"as we already spend cycles for\ndetect_rename why not\" sense.\n\nWith the recent change from Alex not to run status when not interactive,\nit probably does not matter.  If we are going to spawn an editor, we are\ndealing with human interaction and even -B -M should not be too bad.\n"},{"id":"61702","messageId":"20071203012022.GA8322@coredump.intra.peff.net","threadId":"10962","inReplyTo":"7v8x4cykkj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] use typechange as rename source","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-03T01:20:22Z","receivedAt":"2007-12-03T01:20:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 02, 2007 at 11:15:40AM -0800, Junio C Hamano wrote:\n\n> > @@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)\n> > +\trev.diffopt.break_opt = 0;\n> \n> I have to wonder how much this is going to make things worse in the real\n> world, although I agree in the \"as we already spend cycles for\n> detect_rename why not\" sense.\n> \n> With the recent change from Alex not to run status when not interactive,\n> it probably does not matter.  If we are going to spawn an editor, we are\n> dealing with human interaction and even -B -M should not be too bad.\n\nI had more or less the same thinking, but I don't have any real-world\nnumbers. I would be curious to see averages on how diffcore-break\ncompares to diffcore-rename. Just thinking about it, it seems intuitive\nthat breaking would always be cheaper, which means that adding \"-B\" to\n\"-M\" won't have a significant performance impact.\n\n-Peff\n"}]}