{"thread":{"id":"19806","subject":"[PATCH] fix show_entry() in tree-diff.c for TREE_IN_RECURSIVE","startedAt":"2009-06-13T20:32:33Z","lastAt":"2009-06-14T12:01:20Z","messageCount":4,"participants":["Nick Edelen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"116254","messageId":"op.uvhcoii8tdk399@sirnot.private","threadId":"19806","inReplyTo":null,"subject":"[PATCH] fix show_entry() in tree-diff.c for TREE_IN_RECURSIVE","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-13T20:32:33Z","receivedAt":"2009-06-13T20:32:33Z","isPatch":true,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"fix show_entry() in tree_diff.c to display tree entries on TREE_IN_RECURSIVE\n\nSigned-off-by: Nick Edelen \n\n---\nthis seems like how it should act: trees are shown under this option for changes, so it's logical they should also be shown for adds and removals. at any rate rev-cache needs it, so I sure hope it's supposed to do this...\n tree-diff.c | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex edd8394..189a298 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -239,6 +239,12 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t\tif (!tree || type != OBJ_TREE)\n \t\t\tdie(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n \n+\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n+\t\t\tnewbase[baselen + pathlen] = 0;\n+\t\t\topt->add_remove(opt, prefix[0], mode, sha1, newbase);\n+\t\t\tnewbase[baselen + pathlen] = '/';\n+\t\t}\n+\n \t\tinit_tree_desc(&inner, tree, size);\n \t\tshow_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen);\n \n-- \n"},{"id":"116258","messageId":"7v63ezvgyo.fsf@alter.siamese.dyndns.org","threadId":"19806","inReplyTo":"op.uvhcoii8tdk399@sirnot.private","subject":"Re: [PATCH] fix show_entry() in tree-diff.c for TREE_IN_RECURSIVE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-13T23:42:23Z","receivedAt":"2009-06-13T23:42:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nick Edelen\" writes:\n\n> fix show_entry() in tree_diff.c to display tree entries on TREE_IN_RECURSIVE\n\ns/fix/Fix/; s/$/./;\n\n> Signed-off-by: Nick Edelen \n\n> this seems like how it should act: trees are shown under this option for\n> changes,...\n\nPlease show a sample input, the output you expect and the output from the\ncurrent code, to illustrate the alleged breakage better.\n\nFor example. if you used to have files dc/1, dr/2, dt/3, fc, fr and ft and\nthen removed dr/2, dt/3, fr, ft and added da/5, fa, dt and ft/4, you\nexpect this output.\n\n$ git diff -t -r --raw HEAD^ HEAD\n:000000 040000 0000000... a13e5ad... A\tda\n:000000 100644 0000000... ce01362... A\tda/5\n:040000 040000 8dc877a... 40b5137... M\tdc\n:100644 100644 e69de29... ce01362... M\tdc/1\n:040000 000000 f84fc27... 0000000... D\tdr\n:100644 000000 e69de29... 0000000... D\tdr/2\n:000000 100644 0000000... ce01362... A\tdt\n:040000 000000 6e36c7d... 0000000... D\tdt\n:100644 000000 e69de29... 0000000... D\tdt/3\n:000000 100644 0000000... ce01362... A\tfa\n:100644 100644 e69de29... ce01362... M\tfc\n:100644 000000 e69de29... 0000000... D\tfr\n:100644 000000 e69de29... 0000000... D\tft\n:000000 040000 0000000... 9a1efba... A\tft\n:000000 100644 0000000... ce01362... A\tft/4\n\nBut because we show 040000 entries only for changed and typechange cases, \nwe currently get this.\n\n$ git diff -t -r --raw HEAD^ HEAD\n:000000 100644 0000000... ce01362... A\tda/5\n:040000 040000 8dc877a... 40b5137... M\tdc\n:100644 100644 e69de29... ce01362... M\tdc/1\n:100644 000000 e69de29... 0000000... D\tdr/2\n:000000 100644 0000000... ce01362... A\tdt\n:100644 000000 e69de29... 0000000... D\tdt/3\n:000000 100644 0000000... ce01362... A\tfa\n:100644 100644 e69de29... ce01362... M\tfc\n:100644 000000 e69de29... 0000000... D\tfr\n:100644 000000 e69de29... 0000000... D\tft\n:000000 100644 0000000... ce01362... A\tft/4\n\nI think the output from the code after your change is more consistent, but\nI somehow suspect that this might break people's script, like gitweb,\nrather badly, if they depended on the existing behaviour.\n"},{"id":"116261","messageId":"7vvdmzu1c8.fsf@alter.siamese.dyndns.org","threadId":"19806","inReplyTo":"7v63ezvgyo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix show_entry() in tree-diff.c for TREE_IN_RECURSIVE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-14T00:05:11Z","receivedAt":"2009-06-14T00:05:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Nick Edelen\" writes:\n> ...\n>> this seems like how it should act: trees are shown under this option for\n>> changes,...\n>\n> Please show a sample input, the output you expect and the output from the\n> current code, to illustrate the alleged breakage better.\n> ...\n> I think the output from the code after your change is more consistent, but\n> I somehow suspect that this might break people's script, like gitweb,\n> rather badly, if they depended on the existing behaviour.\n\nThat is, something like the attached patch.\n\nI cannot afford to do this to everybody, but you seem to be new to the\ncommunity, and every once in a while teaching by showing example is the\nbest way to do things, so...\n\n-- >8 --\nFrom: Nick Edelen \nSubject: [PATCH] diff-tree -r -t: include added/removed directories in the output\n\nWe used to include only the modified and typechanged directories\nin the ouptut, but for consistency's sake, we should also include\nadded and removed ones as well.\n\nThis makes the output more consistent, but it may break existing scripts\nthat expect to see the current output which has long been the established\nbehaviour.\n\nSigned-off-by: Nick Edelen \nSigned-off-by: Junio C Hamano \n---\n\n * This should apply at least down to 1.6.0 series, if not earlier.\n\n t/t4037-diff-r-t-dirs.sh | 53 ++++++++++++++++++++++++++++++++++++++++++++++\n tree-diff.c | 6 +++++\n 2 files changed, 59 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4037-diff-r-t-dirs.sh b/t/t4037-diff-r-t-dirs.sh\nnew file mode 100755\nindex 0000000..f5ce3b2\n--- /dev/null\n+++ b/t/t4037-diff-r-t-dirs.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description='diff -r -t shows directory additions and deletions'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tmkdir dc dr dt &&\n+\t>dc/1 &&\n+\t>dr/2 &&\n+\t>dt/3 &&\n+\t>fc &&\n+\t>fr &&\n+\t>ft &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\n+\trm -fr dt dr ft fr &&\n+\tmkdir da ft &&\n+\tfor p in dc/1 da/4 dt ft/5 fc\n+\tdo\n+\t\techo hello >$p || exit\n+\tdone &&\n+\tgit add -u &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m second\n+'\n+\n+cat >expect <<\\EOF\n+A\tda\n+A\tda/4\n+M\tdc\n+M\tdc/1\n+D\tdr\n+D\tdr/2\n+A\tdt\n+D\tdt\n+D\tdt/3\n+M\tfc\n+D\tfr\n+D\tft\n+A\tft\n+A\tft/5\n+EOF\n+\n+test_expect_success verify '\n+\tgit diff-tree -r -t --name-status HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 9f67af6..c83a8da 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -233,6 +233,12 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n \t\tif (!tree || type != OBJ_TREE)\n \t\t\tdie(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n \n+\t\tif (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n+\t\t\tnewbase[baselen + pathlen] = 0;\n+\t\t\topt->add_remove(opt, *prefix, mode, sha1, newbase);\n+\t\t\tnewbase[baselen + pathlen] = '/';\n+\t\t}\n+\n \t\tinit_tree_desc(&inner, tree, size);\n \t\tshow_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen);\n \n"},{"id":"116280","messageId":"c77435a80906140501m1b63512fif5cc5e57f5d114c5@mail.gmail.com","threadId":"19806","inReplyTo":"7vvdmzu1c8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix show_entry() in tree-diff.c for TREE_IN_RECURSIVE","fromName":"Nick Edelen","fromEmail":"sirnot@gmail.com","sentAt":"2009-06-14T12:01:20Z","receivedAt":"2009-06-14T12:01:20Z","isPatch":true,"sender":{"key":"sirnot@gmail.com","avatar":null},"body":"ah, right. so next time I should add a test to demonstrate and verify\nthe external effect? admittedly part of the reason I hadn't showed\nany example output was because I changed this from an internal\nperspective. I'll definitely do that next time though. thanks for\nthe example! (btw, what is the numerical naming convention for\ntests?)\n\nyou know if no one really liked it as an output we could keep this\nchange for internal consistency's sake, and filter our added/removed\ndirs in the outputing, or even diff_addremove. seems a bit hackish\nthough...\n\n(also, do you want me to resubmit? because you seem to have taken care of it)\n\n - Nick\n\nOn Sun, Jun 14, 2009 at 2:05 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> \"Nick Edelen\" writes:\n>> ...\n>>> this seems like how it should act: trees are shown under this option for\n>>> changes,...\n>>\n>> Please show a sample input, the output you expect and the output from the\n>> current code, to illustrate the alleged breakage better.\n>> ...\n>> I think the output from the code after your change is more consistent, but\n>> I somehow suspect that this might break people's script, like gitweb,\n>> rather badly, if they depended on the existing behaviour.\n>\n> That is, something like the attached patch.\n>\n> I cannot afford to do this to everybody, but you seem to be new to the\n> community, and every once in a while teaching by showing example is the\n> best way to do things, so...\n>\n> -- >8 --\n> From: Nick Edelen \n> Subject: [PATCH] diff-tree -r -t: include added/removed directories in the output\n>\n> We used to include only the modified and typechanged directories\n> in the ouptut, but for consistency's sake, we should also include\n> added and removed ones as well.\n>\n> This makes the output more consistent, but it may break existing scripts\n> that expect to see the current output which has long been the established\n> behaviour.\n>\n> Signed-off-by: Nick Edelen \n> Signed-off-by: Junio C Hamano \n> ---\n>\n>  * This should apply at least down to 1.6.0 series, if not earlier.\n>\n>  t/t4037-diff-r-t-dirs.sh |   53 ++++++++++++++++++++++++++++++++++++++++++++++\n>  tree-diff.c              |    6 +++++\n>  2 files changed, 59 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t4037-diff-r-t-dirs.sh b/t/t4037-diff-r-t-dirs.sh\n> new file mode 100755\n> index 0000000..f5ce3b2\n> --- /dev/null\n> +++ b/t/t4037-diff-r-t-dirs.sh\n> @@ -0,0 +1,53 @@\n> +#!/bin/sh\n> +\n> +test_description='diff -r -t shows directory additions and deletions'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +       mkdir dc dr dt &&\n> +       >dc/1 &&\n> +       >dr/2 &&\n> +       >dt/3 &&\n> +       >fc &&\n> +       >fr &&\n> +       >ft &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m initial &&\n> +\n> +       rm -fr dt dr ft fr &&\n> +       mkdir da ft &&\n> +       for p in dc/1 da/4 dt ft/5 fc\n> +       do\n> +               echo hello >$p || exit\n> +       done &&\n> +       git add -u &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m second\n> +'\n> +\n> +cat >expect <<\\EOF\n> +A      da\n> +A      da/4\n> +M      dc\n> +M      dc/1\n> +D      dr\n> +D      dr/2\n> +A      dt\n> +D      dt\n> +D      dt/3\n> +M      fc\n> +D      fr\n> +D      ft\n> +A      ft\n> +A      ft/5\n> +EOF\n> +\n> +test_expect_success verify '\n> +       git diff-tree -r -t --name-status HEAD^ HEAD >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n> +test_done\n> diff --git a/tree-diff.c b/tree-diff.c\n> index 9f67af6..c83a8da 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -233,6 +233,12 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree\n>                if (!tree || type != OBJ_TREE)\n>                        die(\"corrupt tree sha %s\", sha1_to_hex(sha1));\n>\n> +               if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {\n> +                       newbase[baselen + pathlen] = 0;\n> +                       opt->add_remove(opt, *prefix, mode, sha1, newbase);\n> +                       newbase[baselen + pathlen] = '/';\n> +               }\n> +\n>                init_tree_desc(&inner, tree, size);\n>                show_tree(opt, prefix, &inner, newbase, baselen + 1 + pathlen);\n>\n>\n"}]}