{"thread":{"id":"23535","subject":"[PATCH] Make --follow support --find-copies-harder.","startedAt":"2010-04-20T11:27:55Z","lastAt":"2010-04-21T09:24:27Z","messageCount":5,"participants":["Bo Yang","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"139963","messageId":"1271762875-16548-1-git-send-email-struggleyb.nku@gmail.com","threadId":"23535","inReplyTo":null,"subject":"[PATCH] Make --follow support --find-copies-harder.","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-04-20T11:27:55Z","receivedAt":"2010-04-20T11:27:55Z","isPatch":true,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"'git diff --follow ' give users\nthe content difference of between the two commits.\nIt will detect file copies/moves of if there is any.\nBut with '--find-copies-harder', it does not take the\nunmodified files as copy/move source. And this patch fix\nthis bug.\n\nSigned-off-by: Bo Yang \n---\n t/t4042-find-copies-harder.sh | 45 +++++++++++++++++++++++++++++++++++++++++\n tree-diff.c | 2 +\n 2 files changed, 47 insertions(+), 0 deletions(-)\n create mode 100755 t/t4042-find-copies-harder.sh\n\ndiff --git a/t/t4042-find-copies-harder.sh b/t/t4042-find-copies-harder.sh\nnew file mode 100755\nindex 0000000..40d2122\n--- /dev/null\n+++ b/t/t4042-find-copies-harder.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Bo Yang\n+#\n+\n+test_description='Test copy detection with --find-copies-harder in diff engine.\n+\n+'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+echo >path0 'Line 1\n+Line 2\n+Line 3\n+Line 4\n+Line 5\n+Line 6\n+'\n+\n+test_expect_success \\\n+ 'add a file path0 and commit.' \\\n+ 'git add path0 &&\n+ git commit -m \"Add path0\"'\n+\n+cat path1\n+test_expect_success \\\n+ 'copy path0 to path1.' \\\n+ 'git add path1 &&\n+ git commit -m \"Copy path1 from path0\"'\n+\n+test_expect_success \\\n+ 'find the copy path0 -> path1 harder' \\\n+ 'git diff --follow --find-copies-harder HEAD^ HEAD path1 > current'\n+cat >expected <<\\EOF\n+diff --git a/path0 b/path1\n+similarity index 100%\n+copy from path0\n+copy to path1\n+EOF\n+\n+test_expect_success \\\n+ 'validate the output.' \\\n+ 'compare_diff_patch current expected'\n+\n+test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex fe9f52c..0dea53e 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -346,6 +346,8 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co\n \n \tdiff_setup(&diff_opts);\n \tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n+\tif (DIFF_OPT_TST(opt, FIND_COPIES_HARDER))\n+\t\tDIFF_OPT_SET(&diff_opts, FIND_COPIES_HARDER);\n \tdiff_opts.detect_rename = DIFF_DETECT_RENAME;\n \tdiff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tdiff_opts.single_follow = opt->paths[0];\n-- \n1.7.0.2.273.gc2413.dirty\n"},{"id":"140003","messageId":"7vtyr5cxnz.fsf@alter.siamese.dyndns.org","threadId":"23535","inReplyTo":"1271762875-16548-1-git-send-email-struggleyb.nku@gmail.com","subject":"Re: [PATCH] Make --follow support --find-copies-harder.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-21T03:05:52Z","receivedAt":"2010-04-21T03:05:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bo Yang writes:\n\n> 'git diff --follow ' give users\n> the content difference of between the two commits.\n> It will detect file copies/moves of if there is any.\n\nBecause the \"--follow\" hack was done primarily as a \"checkbox\" item, and\nalso because it is not an option for the \"diff\" family (it is an option\nfor the \"log\" family), I would personally think that it is actually a bug\nthat \"git diff\" accepts \"--follow\" and pretends as if it is doing useful\nwork, but does so only some of the time.\n\n $ git diff --follow --name-status maint master -- builtin/log.c\n R089\tbuiltin-log.c\tbuiltin/log.c\n $ git diff --follow --name-status -R maint master -- builtin/log.c\n D\tbuiltin/log.c\n $ git diff --follow --name-status master maint -- builtin/log.c\n D\tbuiltin/log.c\n\nAs we can see, it doesn't quite work, and it is not a fault of 750f7b6\n(Finally implement \"git log --follow\", 2007-06-19) by Linus, exactly\nbecause the feature wasn't designed to work with \"diff\" to begin with.\n\nIf we were to add a support of \"--follow\" to \"diff\" family, I suspect that\nwe need to\n\n (1) make sure we get only one path, just like \"log\" family does;\n\n (2) add a logic to notice the reverse situation as demonstrated above and\n deal with it in a sensible way, without any --find-copies option\n given by the user.\n\namong other things. Also we of course need to document it as a new \"diff\"\noption when we are done.\n"},{"id":"140004","messageId":"h2l41f08ee11004202117nc56510d4y29e39631fdff0923@mail.gmail.com","threadId":"23535","inReplyTo":"7vtyr5cxnz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make --follow support --find-copies-harder.","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-04-21T04:17:22Z","receivedAt":"2010-04-21T04:17:22Z","isPatch":true,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"On Wed, Apr 21, 2010 at 11:05 AM, Junio C Hamano wrote:\n> Because the \"--follow\" hack was done primarily as a \"checkbox\" item, and\n> also because it is not an option for the \"diff\" family (it is an option\n> for the \"log\" family), I would personally think that it is actually a bug\n> that \"git diff\" accepts \"--follow\" and pretends as if it is doing useful\n> work, but does so only some of the time.\n\nAh, sorry for the confusion. I mean, I have found the bug when I use\ngit log. And take a look at:\n\ngit log --follow --find-copies-harder -p t/t4013/diff.show_--first-parent_master\n\nThis will report the file t/t4013/diff.show_--first-parent_master as\na new file but it is copied from t/t4013/diff.show_master indeed.\n'--find-copies-harder' should detect this, but it didn't. With this\npatch it will find such copy and go on following\nt/t4013/diff.show_master history.\n\nAnd I locate the bug in the format of a diff test case and this cause\nthe confusion. What I really try to fix is,\n1. --follow should support --find-copies-harder when using git-log\n2. git-diff should support --find-copies-harder, I mean, diff should\nfind copies in unmodified files.\n\nFor 2, I find the --follow option works for git-diff, so I just take\nconsideration that it is the right way to support the\n--find-copies-harder in git-diff. (and now I don't think so...) ;-)\n\n>    $ git diff --follow --name-status maint master -- builtin/log.c\n>    R089        builtin-log.c   builtin/log.c\n>    $ git diff --follow --name-status -R maint master -- builtin/log.c\n>    D   builtin/log.c\n>    $ git diff --follow --name-status master maint -- builtin/log.c\n>    D   builtin/log.c\n>\n> As we can see, it doesn't quite work, and it is not a fault of 750f7b6\n> (Finally implement \"git log --follow\", 2007-06-19) by Linus, exactly\n> because the feature wasn't designed to work with \"diff\" to begin with.\n\nHmm, really.\n\n> If we were to add a support of \"--follow\" to \"diff\" family, I suspect that\n> we need to\n>\n>  (1) make sure we get only one path, just like \"log\" family does;\n>\n>  (2) add a logic to notice the reverse situation as demonstrated above and\n>     deal with it in a sensible way, without any --find-copies option\n>     given by the user.\n\nEn, as above. I just want to teach git-diff to find copies among\nunmodified files with '--find-copies-harder' option. Maybe, '--follow'\nis the good choice to use for control whether git-diff will detect\nfile move/copy, and '--find-copies-harder' is the option to control\nhow hard we find the copies.\n\nI will try to make this patch into two, one for fixing the git-log\n--follow --find-copies-harder one, and the other try to make a sane\nlogic for '--follow' for git-diff.\n\nThanks for your advice!\n\nRegards!\nBo\n-- \nMy blog: http://blog.morebits.org\n"},{"id":"140020","messageId":"i2r41f08ee11004210202r642aa25dy32b06c33ed98ba4c@mail.gmail.com","threadId":"23535","inReplyTo":"7vtyr5cxnz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make --follow support --find-copies-harder.","fromName":"Bo Yang","fromEmail":"struggleyb.nku@gmail.com","sentAt":"2010-04-21T09:02:24Z","receivedAt":"2010-04-21T09:02:24Z","isPatch":true,"sender":{"key":"struggleyb.nku@gmail.com","avatar":"https://avatars.githubusercontent.com/u/233030?v=4"},"body":"On Wed, Apr 21, 2010 at 11:05 AM, Junio C Hamano wrote:\n>\n> Because the \"--follow\" hack was done primarily as a \"checkbox\" item, and\n> also because it is not an option for the \"diff\" family (it is an option\n> for the \"log\" family), I would personally think that it is actually a bug\n> that \"git diff\" accepts \"--follow\" and pretends as if it is doing useful\n> work, but does so only some of the time.\n>\n>    $ git diff --follow --name-status maint master -- builtin/log.c\n>    R089        builtin-log.c   builtin/log.c\n>    $ git diff --follow --name-status -R maint master -- builtin/log.c\n>    D   builtin/log.c\n>    $ git diff --follow --name-status master maint -- builtin/log.c\n>    D   builtin/log.c\n\nI am really wondering, when -R is used, how the file rename/copy\nshould defined? Now, I can make -R works with --follow, and it produce\nsomething like:\n\nbyang@byang-laptop:~/git/git$ ./git diff --follow --name-status maint\nmaster -- builtin/log.c\nR089 builtin-log.c builtin/log.c\nbyang@byang-laptop:~/git/git$ ./git diff --follow --name-status -R\nmaint master -- builtin/log.c\nR089 builtin/log.c builtin-log.c\nbyang@byang-laptop:~/git/git$ ./git diff --follow --name-status\nmaster maint -- builtin/log.c\nD builtin/log.c\nbyang@byang-laptop:~/git/git$ ./git diff --follow --name-status -R\nmaster maint -- builtin/log.c\nA builtin/log.c\n\n\nThe problem is whether it make sense to say 'builtin/log.c renamed to\nbuiltin-log.c' when -R is given?\n\nRegards!\nBo\n"},{"id":"140022","messageId":"7vpr1tb1kk.fsf@alter.siamese.dyndns.org","threadId":"23535","inReplyTo":"i2r41f08ee11004210202r642aa25dy32b06c33ed98ba4c@mail.gmail.com","subject":"Re: [PATCH] Make --follow support --find-copies-harder.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-21T09:24:27Z","receivedAt":"2010-04-21T09:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bo Yang writes:\n\n> I am really wondering, when -R is used, how the file rename/copy\n> should defined? Now, I can make -R works with --follow, and it produce\n> something like:\n>\n> byang@byang-laptop:~/git/git$ ./git diff --follow --name-status maint\n> master -- builtin/log.c\n> R089 builtin-log.c builtin/log.c\n\nAs I already said, it is a bug that \"diff\" does not diagnose it an error\nwhen you give it the \"--follow\" option. It was designed to be used with\n\"log\" family, and never with \"diff\" family.\n\nWhen a command from the \"log\" family traverses the history, it internally\nruns \"diff-tree\" between the commit C it is currently looking at, and its\nparents C^$n. When you give one path and --follow [*1*], it may notice\nthat C has the named path and C^$n doesn't.\n\nAt that point, it internally runs \"diff -M C^$n C\" to see if there is a\ncorresponding path in C^$n, and switch to follow the path it found in\nthe parent commit.\n\nThe logic only detects the case where \"new\" side has a path that \"old\"\nside doesn't [*3*], and it is not even designed to be used with -R (where\nit needs to be given a path that does not exist anymore on the \"new\" side\nbut used to exist in the \"old\" side).\n\nHeck, it is not even designed to be used with \"diff\" as I already said\ntwice ;-).\n\nEven in the context of \"log\", it is a hack. It globally keeps one single\npath that it follows, which obviously would not work in a history with\nmerges.\n\n\n[Footnote]\n\n*1* \"log\" command line parser enforces this \"only one path\" condition;\n\"diff\" doesn't even bother catching it as an error to give \"--follow\", so\nit lacks the logic to further catch it as an error to give more than one\npaths.\n\n*2* No, I don't think there is an interface to tweak the -M to -C or -C\n-C; see tree-diff.c and look for try_to_follow_renames(). I think it is\nprobably Ok to make this tweakable, and I suspect that is what your patch\nis about, but don't use \"git diff\" as an example nor in any of your tests.\n\n*3* This is exactly why \"diff --follow maint master -- builtin/log.c\"\nappears to do something remotely sensible (notice that the path is what\nexists in the \"new\" side) but \"maint master -- builtin-log.c\" does not.\nThe logic doesn't even care if the named path does not appear in the \"new\"\nside at all, because that is not useful at all in the way \"log\" internally\nuses \"diff-tree\" logic.\n"}]}