{"thread":{"id":"681","subject":"[PATCH] show changed tree objects with recursive git-diff-tree","startedAt":"2005-05-21T01:40:41Z","lastAt":"2005-05-25T19:18:27Z","messageCount":9,"participants":["Nicolas Pitre","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"3667","messageId":"Pine.LNX.4.62.0505202131520.4397@localhost.localdomain","threadId":"681","inReplyTo":null,"subject":"[PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2005-05-21T01:40:41Z","receivedAt":"2005-05-21T01:40:41Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When -p is not used, git-diff-tree currently shows changed tree objects\nbut only when not recursive. This patch makes the recursive output\nshow tree objects as well.\n\nThis has the immediate benefit of making git-deltafy-script handle\ndeltafication of tree objects.\n\nSigned-off-by: Nicolas Pitre \n\ndiff --git a/diff-tree.c b/diff-tree.c\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -131,7 +131,10 @@ static int compare_tree_entry(void *tree\n \n \tif (recursive && S_ISDIR(mode1)) {\n \t\tint retval;\n-\t\tchar *newbase = malloc_base(base, path1, pathlen1);\n+\t\tchar *newbase;\n+\t\tif (!silent && !generate_patch)\n+\t\t\tdiff_change(mode1, mode2, sha1, sha2, base, path1);\n+\t\tnewbase = malloc_base(base, path1, pathlen1);\n \t\tretval = diff_tree_sha1(sha1, sha2, newbase);\n \t\tfree(newbase);\n \t\treturn retval;\ndiff --git a/git-deltafy-script b/git-deltafy-script\n--- a/git-deltafy-script\n+++ b/git-deltafy-script\n@@ -9,8 +9,6 @@\n # NOTE: the \"best earlier version\" is not implemented in mkdelta yet\n # and therefore only the next eariler version is used at this time.\n #\n-# TODO: deltafy tree objects as well.\n-#\n # The -d argument allows to provide a limit on the delta chain depth.\n # If 0 is passed then everything is undeltafied.\n \n"},{"id":"3671","messageId":"7vsm0hpbub.fsf@assigned-by-dhcp.cox.net","threadId":"681","inReplyTo":"Pine.LNX.4.62.0505202131520.4397@localhost.localdomain","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-21T02:11:24Z","receivedAt":"2005-05-21T02:11:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Although I do not have immediate objections to what it tries to\ndo, I have to think about the intent of the patch and its\nramifications.\n\nHowever, I think the patch operates at the wrong level if you\nare basing on the tip of Linus tree. With the new diff core,\nyou do not filter like this:\n\n-\t\tchar *newbase = malloc_base(base, path1, pathlen1);\n+\t\tchar *newbase;\n+\t\tif (!silent && !generate_patch)\n+\t\t\tdiff_change(mode1, mode2, sha1, sha2, base, path1);\n+\t\tnewbase = malloc_base(base, path1, pathlen1);\n\nI'd just say \"if (!silent)\" there. The updated diff_change\n_should_ do the right thing regardless of generate_patch value\nyou have there, because it is already told about it with\ndiff_setup(), and it knows not to say silly things when we are\ngenerating patch.\n\n\n"},{"id":"3672","messageId":"Pine.LNX.4.58.0505202019330.2206@ppc970.osdl.org","threadId":"681","inReplyTo":"Pine.LNX.4.62.0505202131520.4397@localhost.localdomain","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-21T03:20:49Z","receivedAt":"2005-05-21T03:20:49Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 20 May 2005, Nicolas Pitre wrote:\n>\n> When -p is not used, git-diff-tree currently shows changed tree objects\n> but only when not recursive. This patch makes the recursive output\n> show tree objects as well.\n\nThat sounds wrong. That would seem to make diff-helper confused, no? \n\n\t\tLinus\n"},{"id":"3673","messageId":"Pine.LNX.4.58.0505202025480.2206@ppc970.osdl.org","threadId":"681","inReplyTo":"7vsm0hpbub.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-21T03:34:03Z","receivedAt":"2005-05-21T03:34:03Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 20 May 2005, Junio C Hamano wrote:\n>\n> Although I do not have immediate objections to what it tries to\n> do, I have to think about the intent of the patch and its\n> ramifications.\n\nI really think it should be a totally separate flag to enable showing the\nsub-trees if the tree-blobification wants this.\n\nIn fact, I can pretty much _guarantee_ that the patch as posted is the\nwrong thing to do: it will do horribly wrong things for things like\n\n\tgit-whatchanged arch/i386/kernel/head.S\n\n(but I haven't tried it - try it yourself. The correct output for the \nkernel archive is just a single commit, and a single blob change in that \ncommit).\n\nMy bet is that the patch will end up showing every single changeset that \ntouches anything under \"arch/\", since such _trees_ will be marked as \ninteresting. Which is absolutely the wrong thing to do.\n\nNico, try it, maybe you'll prove me wrong.\n\n\n\t\tLinus\n"},{"id":"3675","messageId":"7vd5rlp7ef.fsf@assigned-by-dhcp.cox.net","threadId":"681","inReplyTo":"Pine.LNX.4.58.0505202019330.2206@ppc970.osdl.org","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-21T03:47:20Z","receivedAt":"2005-05-21T03:47:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Diff heler _should_ not get confused, but maybe it currently\ndoes. If that is the case, I would consider that a bug (my\nbad).\n\n... goes back to the Linus tip for a while and comes back ...\n\nYup. It says a change line containing tree is not something it\nrecognizes. Sorry, there is a bug there (and another bug that\npartially hides that bug).\n\nI'm doing major rewrite of the diff-core right now but even\nafter that, diff helper _should_ just ignore trees.\n\nIn the meantime, this patch should fix it. And this should be\nthe right fix even after the \"major rewrite\" I am doing now.\n\nSigned-off-by: Junio C Hamano \n---\n# - linus: [PATCH] delta creation\n# + (working tree)\ndiff --git a/diff-helper.c b/diff-helper.c\n--- a/diff-helper.c\n+++ b/diff-helper.c\n@@ -20,7 +20,8 @@ static int parse_oneside_change(const ch\n \t\tcp++;\n \t}\n \t*mode = m;\n-\tif (strncmp(cp, \"\\tblob\\t\", 6) && strncmp(cp, \" blob \", 6))\n+\tif (strncmp(cp, \"\\ttree\\t\", 6) && strncmp(cp, \" tree \", 6) &&\n+\t strncmp(cp, \"\\tblob\\t\", 6) && strncmp(cp, \" blob \", 6))\n \t\treturn -1;\n \tcp += 6;\n \tif (get_sha1_hex(cp, sha1))\n@@ -44,11 +45,13 @@ static int parse_diff_raw_output(const c\n \t\tdiff_unmerge(cp + 1);\n \t\tbreak;\n \tcase '+':\n-\t\tparse_oneside_change(cp, &new_mode, new_sha1, path);\n+\t\tif (parse_oneside_change(cp, &new_mode, new_sha1, path))\n+\t\t\treturn -1;\n \t\tdiff_addremove('+', new_mode, new_sha1, path, NULL);\n \t\tbreak;\n \tcase '-':\n-\t\tparse_oneside_change(cp, &old_mode, old_sha1, path);\n+\t\tif (parse_oneside_change(cp, &old_mode, old_sha1, path))\n+\t\t\treturn -1;\n \t\tdiff_addremove('-', old_mode, old_sha1, path, NULL);\n \t\tbreak;\n \tcase '*':\n@@ -64,7 +67,8 @@ static int parse_diff_raw_output(const c\n \t\t\tnew_mode = (new_mode << 3) | (ch - '0');\n \t\t\tcp++;\n \t\t}\n-\t\tif (strncmp(cp, \"\\tblob\\t\", 6) && strncmp(cp, \" blob \", 6))\n+\t\tif (strncmp(cp, \"\\tblob\\t\", 6) && strncmp(cp, \" blob \", 6) &&\n+\t\t (strncmp(cp, \"\\ttree\\t\", 6) && strncmp(cp, \" tree \", 6)))\n \t\t\treturn -1;\n \t\tcp += 6;\n \t\tif (get_sha1_hex(cp, old_sha1))\n\n\n\n"},{"id":"3817","messageId":"Pine.LNX.4.62.0505231724270.16151@localhost.localdomain","threadId":"681","inReplyTo":"Pine.LNX.4.58.0505202025480.2206@ppc970.osdl.org","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2005-05-23T21:49:27Z","receivedAt":"2005-05-23T21:49:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"\n[catching up after a weekind away -- you guys have not been standing still]\n\nOn Fri, 20 May 2005, Linus Torvalds wrote:\n\n> On Fri, 20 May 2005, Junio C Hamano wrote:\n> >\n> > Although I do not have immediate objections to what it tries to\n> > do, I have to think about the intent of the patch and its\n> > ramifications.\n> \n> I really think it should be a totally separate flag to enable showing the\n> sub-trees if the tree-blobification wants this.\n> \n> In fact, I can pretty much _guarantee_ that the patch as posted is the\n> wrong thing to do: it will do horribly wrong things for things like\n> \n> \tgit-whatchanged arch/i386/kernel/head.S\n> \n> (but I haven't tried it - try it yourself. The correct output for the \n> kernel archive is just a single commit, and a single blob change in that \n> commit).\n\nOK. What about the following patch? It outputs changed tree objects \nonly if -p nor -v nor -s is specified, i.e. whenever what is really \nwanted is output of what changed at the object level. This makes it \nmore coherent with the non-recursive output as well. Checked that \ngit-diff-helper doesn't get confused.\n\nIf a separate flag is really needed, then consistency dictates that the \nnon recursive output should provide output for tree objects only when \nthat flag is given as well, which makes the non recursive output rather \nuseless in most cases. And IMHO this is just too much burden for little \nbenefit (the extra flag not the recursive tree object output).\n\n=====\n\nThis patch includes output of modified tree objects to recursive output, \njust like non recursive output already does. When -v, -s or -p is \nspecified then the recursive output supresses modified tree objects \nsince they don't make much sense in that case.\n\nSigned-off-by: Nicolas Pitre \n\ndiff --git a/diff-tree.c b/diff-tree.c\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -127,6 +127,8 @@ static int compare_tree_entry(void *tree\n \tif (recursive && S_ISDIR(mode1)) {\n \t\tint retval;\n \t\tchar *newbase = malloc_base(base, path1, pathlen1);\n+\t\tif (!silent && !verbose_header && !show_root_diff)\n+\t\t\tdiff_change(mode1, mode2, sha1, sha2, base, path1);\n \t\tretval = diff_tree_sha1(sha1, sha2, newbase);\n \t\tfree(newbase);\n \t\treturn retval;\n"},{"id":"3921","messageId":"7vmzqjn7qh.fsf@assigned-by-dhcp.cox.net","threadId":"681","inReplyTo":"Pine.LNX.4.62.0505231724270.16151@localhost.localdomain","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-25T06:24:22Z","receivedAt":"2005-05-25T06:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry, I did not get back to you earlier.\n\nMy only hesitation is that I'd rather want to see outputs from\n\"git-diff-tree -r $O $A\" and \"git-read-tree $A && git-diff-cache\n--cached $O\" exactly match by default, but this is quite minor (I\ncannot even justify why myself).\n\nAs you already know, diff-helper knows it cannot do anything\nabout trees, so that is not a reason to fear your change.\n\nThat said, I have already acquired a habit of running diff-tree\nwith raw output to see what files have changed between two\ntrees, relying on the traditional behaviour of showing only\nblobs and not trees, so turning this \"tree\" output on by default\nis a minor nuisance to adjust to (it is minor --- I just need to\nfilter them out by looking at the first 7 bytes of each line).\n\nHow about this patch instead? Does it do what you need? My\nunderstanding of your needs is that you do not like having to\ncall diff-tree (w/o recursive) only to get tree IDs involved,\nbecause you are indeed interested in the whole tree and prefer\nrecursive behaviour; for that reason I made -t to imply -r.\n\nOnly lightly tested.\n\nSigned-off-by: Junio C Hamano \n---\n\ndiff --git a/diff-tree.c b/diff-tree.c\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -6,6 +6,7 @@ static int show_root_diff = 0;\n static int verbose_header = 0;\n static int ignore_merges = 1;\n static int recursive = 0;\n+static int show_tree_entry_in_recursive = 0;\n static int read_stdin = 0;\n static int diff_output_format = DIFF_FORMAT_HUMAN;\n static int detect_rename = 0;\n@@ -123,6 +124,8 @@ static int compare_tree_entry(void *tree\n \tif (recursive && S_ISDIR(mode1)) {\n \t\tint retval;\n \t\tchar *newbase = malloc_base(base, path1, pathlen1);\n+\t\tif (show_tree_entry_in_recursive)\n+\t\t\tdiff_change(mode1, mode2, sha1, sha2, base, path1);\n \t\tretval = diff_tree_sha1(sha1, sha2, newbase);\n \t\tfree(newbase);\n \t\treturn retval;\n@@ -463,7 +466,7 @@ static int diff_tree_stdin(char *line)\n }\n \n static char *diff_tree_usage =\n-\"git-diff-tree [-p] [-r] [-z] [--stdin] [-M] [-C] [-R] [-S] [-m] [-s] [-v] \";\n+\"git-diff-tree [-p] [-r] [-z] [--stdin] [-M] [-C] [-R] [-S] [-m] [-s] [-v] [-t] \";\n \n int main(int argc, const char **argv)\n {\n@@ -498,6 +501,10 @@ int main(int argc, const char **argv)\n \t\t\trecursive = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"-t\")) {\n+\t\t\trecursive = show_tree_entry_in_recursive = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"-R\")) {\n \t\t\treverse_diff = 1;\n \t\t\tcontinue;\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -9,7 +9,7 @@ git-diff-tree - Compares the content and\n \n SYNOPSIS\n --------\n-'git-diff-tree' [-p] [-r] [-z] [--stdin] [-M] [-R] [-C] [-S] [-m] [-s] [-v] []\\*\n+'git-diff-tree' [-p] [-r] [-z] [--stdin] [-M] [-R] [-C] [-S] [-m] [-s] [-v] [-t] []\\*\n \n DESCRIPTION\n -----------\n@@ -48,6 +48,9 @@ OPTIONS\n -r::\n \trecurse\n \n+-t::\n+\tshow tree entry itself as well as subtrees. Implies -r.\n+\n -z::\n \t\\0 line termination on output\n \n\n\n"},{"id":"3932","messageId":"Pine.LNX.4.62.0505250934040.16151@localhost.localdomain","threadId":"681","inReplyTo":"7vmzqjn7qh.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2005-05-25T13:38:37Z","receivedAt":"2005-05-25T13:38:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 24 May 2005, Junio C Hamano wrote:\n\n> How about this patch instead? Does it do what you need? My\n> understanding of your needs is that you do not like having to\n> call diff-tree (w/o recursive) only to get tree IDs involved,\n> because you are indeed interested in the whole tree and prefer\n> recursive behaviour; for that reason I made -t to imply -r.\n\nYes, that does what I need, thanks.\n\nNow there is only the minor inconsistency that the recursive output \nwithout -t doesn't display tree objects while the non recursive output \ndoes output tree objects regardless. Do we care?\n\n\nNicolas\n"},{"id":"3946","messageId":"7vd5rfktbw.fsf@assigned-by-dhcp.cox.net","threadId":"681","inReplyTo":"Pine.LNX.4.62.0505250934040.16151@localhost.localdomain","subject":"Re: [PATCH] show changed tree objects with recursive git-diff-tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-25T19:18:27Z","receivedAt":"2005-05-25T19:18:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"NP\" == Nicolas Pitre writes:\n\nNP> Now there is only the minor inconsistency that the recursive output \nNP> without -t doesn't display tree objects while the non recursive output \nNP> does output tree objects regardless. Do we care?\n\nHmph. I agree, although I do not care about that much myself\nenough to start coding right now. With luck I may have\nsomething by tomorrow morning US Pacific time though.\n\n"}]}