{"thread":{"id":"14896","subject":"[PATCH 1/3] Refactoring: Split up diff_tree_stdin","startedAt":"2008-08-08T20:48:05Z","lastAt":"2008-08-11T22:28:36Z","messageCount":14,"participants":["Karl Hasselström","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"86544","messageId":"20080808204348.7744.46006.stgit@yoghurt","threadId":"14896","inReplyTo":"7vbq06rwhq.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 0/3] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-08T20:48:05Z","receivedAt":"2008-08-08T20:48:05Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Here's two patches that implements diffing of trees: first a\nrefactoring, then the actual functionality.\n\nI'm not familiar with git's API, so I might have made mistakes in\nchoosing or using functions. Extra eyeballs appreciated. But the test\nsuite passes, it does what I want, and it does make StGit faster, so\n_I'm_ happy ...\n\n---\n\nKarl Hasselström (3):\n      Add test for diff-tree --stdin with two trees\n      Teach git diff-tree --stdin to diff trees\n      Refactoring: Split up diff_tree_stdin\n\n\n Documentation/git-diff-tree.txt |   14 ++++++---\n builtin-diff-tree.c             |   58 +++++++++++++++++++++++++++++++--------\n t/t4002-diff-basic.sh           |   14 +++++++++\n 3 files changed, 69 insertions(+), 17 deletions(-)\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"86543","messageId":"20080808204823.7744.57203.stgit@yoghurt","threadId":"14896","inReplyTo":"20080808204348.7744.46006.stgit@yoghurt","subject":"[PATCH 1/3] Refactoring: Split up diff_tree_stdin","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-08T20:48:23Z","receivedAt":"2008-08-08T20:48:23Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Into a first half that determines what operation to do, and a second\nhalf that does it.\n\nCurrently the only operation is diffing one or more commits, but a\nlater patch will add diffing of trees, at which point this refactoring\nwill pay off.\n\nSigned-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n builtin-diff-tree.c |   31 +++++++++++++++++++------------\n 1 files changed, 19 insertions(+), 12 deletions(-)\n\n\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex 415cb16..ebbd631 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -14,20 +14,10 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)\n \treturn log_tree_commit(&log_tree_opt, commit);\n }\n \n-static int diff_tree_stdin(char *line)\n+/* Diff one or more commits. */\n+static int stdin_diff_commit(struct commit *commit, char *line, int len)\n {\n-\tint len = strlen(line);\n \tunsigned char sha1[20];\n-\tstruct commit *commit;\n-\n-\tif (!len || line[len-1] != '\\n')\n-\t\treturn -1;\n-\tline[len-1] = 0;\n-\tif (get_sha1_hex(line, sha1))\n-\t\treturn -1;\n-\tcommit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n-\t\treturn -1;\n \tif (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {\n \t\t/* Graft the fake parents locally to the commit */\n \t\tint pos = 41;\n@@ -52,6 +42,23 @@ static int diff_tree_stdin(char *line)\n \treturn log_tree_commit(&log_tree_opt, commit);\n }\n \n+static int diff_tree_stdin(char *line)\n+{\n+\tint len = strlen(line);\n+\tunsigned char sha1[20];\n+\tstruct commit *commit;\n+\n+\tif (!len || line[len-1] != '\\n')\n+\t\treturn -1;\n+\tline[len-1] = 0;\n+\tif (get_sha1_hex(line, sha1))\n+\t\treturn -1;\n+\tcommit = lookup_commit(sha1);\n+\tif (!commit || parse_commit(commit))\n+\t\treturn -1;\n+\treturn stdin_diff_commit(commit, line, len);\n+}\n+\n static const char diff_tree_usage[] =\n \"git diff-tree [--stdin] [-m] [-c] [--cc] [-s] [-v] [--pretty] [-t] [-r] [--root] \"\n \"[<common diff options>] <tree-ish> [<tree-ish>] [<path>...]\\n\"\n"},{"id":"86546","messageId":"20080808204829.7744.11661.stgit@yoghurt","threadId":"14896","inReplyTo":"20080808204348.7744.46006.stgit@yoghurt","subject":"[PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-08T20:48:29Z","receivedAt":"2008-08-08T20:48:29Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"In addition to accepting lines with one or more commits, it now\naccepts lines with precisely two trees.\n\nWhen diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,\n--encoding, --no-commit-id, -c, --cc, and --always options are\nignored, since they do not apply to trees. This is the same behavior\nyou get when specifying two trees on the command line instead of with\n--stdin.\n\nSigned-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n Documentation/git-diff-tree.txt |   14 +++++++++-----\n builtin-diff-tree.c             |   35 +++++++++++++++++++++++++++++++----\n 2 files changed, 40 insertions(+), 9 deletions(-)\n\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\nindex 1fdf20d..0b1ade8 100644\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -49,13 +49,17 @@ include::diff-options.txt[]\n --stdin::\n \tWhen '--stdin' is specified, the command does not take\n \t<tree-ish> arguments from the command line.  Instead, it\n-\treads either one <commit> or a list of <commit>\n-\tseparated with a single space from its standard input.\n+\treads lines containing either two <tree>, one <commit>, or a\n+\tlist of <commit> from its standard input.  (Use a single space\n+\tas separator.)\n +\n-When a single commit is given on one line of such input, it compares\n-the commit with its parents.  The following flags further affects its\n-behavior.  The remaining commits, when given, are used as if they are\n+When two trees are given, it compares the first tree with the second.\n+When a single commit is given, it compares the commit with its\n+parents.  The remaining commits, when given, are used as if they are\n parents of the first commit.\n++\n+The following flags further affects the behavior when comparing\n+commits (but not trees).\n \n -m::\n \tBy default, 'git-diff-tree --stdin' does not show\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex ebbd631..0bdb1cf 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -42,21 +42,48 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)\n \treturn log_tree_commit(&log_tree_opt, commit);\n }\n \n+/* Diff two trees. */\n+static int stdin_diff_trees(struct tree *tree1, char *line, int len)\n+{\n+\tunsigned char sha1[20];\n+\tstruct tree *tree2;\n+\tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {\n+\t\terror(\"Need precisely two trees, separated by one space\");\n+\t\treturn -1;\n+\t}\n+\ttree2 = lookup_tree(sha1);\n+\tif (!tree2 || parse_tree(tree2))\n+\t\treturn -1;\n+\tprintf(\"%s %s\\n\", sha1_to_hex(tree1->object.sha1),\n+\t\t\t  sha1_to_hex(tree2->object.sha1));\n+\tdiff_tree_sha1(tree1->object.sha1, tree2->object.sha1,\n+\t\t       \"\", &log_tree_opt.diffopt);\n+\tlog_tree_diff_flush(&log_tree_opt);\n+\treturn 0;\n+}\n+\n static int diff_tree_stdin(char *line)\n {\n \tint len = strlen(line);\n \tunsigned char sha1[20];\n-\tstruct commit *commit;\n+\tstruct object *obj;\n \n \tif (!len || line[len-1] != '\\n')\n \t\treturn -1;\n \tline[len-1] = 0;\n \tif (get_sha1_hex(line, sha1))\n \t\treturn -1;\n-\tcommit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n+\tobj = lookup_object(sha1);\n+\tobj = obj ? obj : parse_object(sha1);\n+\tif (!obj)\n \t\treturn -1;\n-\treturn stdin_diff_commit(commit, line, len);\n+\tif (obj->type == OBJ_COMMIT)\n+\t\treturn stdin_diff_commit((struct commit *)obj, line, len);\n+\tif (obj->type == OBJ_TREE)\n+\t\treturn stdin_diff_trees((struct tree *)obj, line, len);\n+\terror(\"Object %s is a %s, not a commit or tree\",\n+\t      sha1_to_hex(sha1), typename(obj->type));\n+\treturn -1;\n }\n \n static const char diff_tree_usage[] =\n"},{"id":"86545","messageId":"20080808204834.7744.10469.stgit@yoghurt","threadId":"14896","inReplyTo":"20080808204348.7744.46006.stgit@yoghurt","subject":"[PATCH 3/3] Add test for diff-tree --stdin with two trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-08T20:48:35Z","receivedAt":"2008-08-08T20:48:35Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"Signed-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\n t/t4002-diff-basic.sh |   14 ++++++++++++++\n 1 files changed, 14 insertions(+), 0 deletions(-)\n\n\ndiff --git a/t/t4002-diff-basic.sh b/t/t4002-diff-basic.sh\nindex a4cfde6..27743c4 100755\n--- a/t/t4002-diff-basic.sh\n+++ b/t/t4002-diff-basic.sh\n@@ -169,6 +169,20 @@ test_expect_success \\\n      cmp -s .test-a .test-recursive-AB'\n \n test_expect_success \\\n+    'diff-tree --stdin of known trees.' \\\n+    'echo $tree_A $tree_B | git diff-tree --stdin > .test-a &&\n+     echo $tree_A $tree_B > .test-plain-ABx &&\n+     cat .test-plain-AB >> .test-plain-ABx &&\n+     cmp -s .test-a .test-plain-ABx'\n+\n+test_expect_success \\\n+    'diff-tree --stdin of known trees.' \\\n+    'echo $tree_A $tree_B | git diff-tree -r --stdin > .test-a &&\n+     echo $tree_A $tree_B > .test-recursive-ABx &&\n+     cat .test-recursive-AB >> .test-recursive-ABx &&\n+     cmp -s .test-a .test-recursive-ABx'\n+\n+test_expect_success \\\n     'diff-cache O with A in cache' \\\n     'git read-tree $tree_A &&\n      git diff-index --cached $tree_O >.test-a &&\n"},{"id":"86550","messageId":"7vk5erb3ru.fsf@gitster.siamese.dyndns.org","threadId":"14896","inReplyTo":"20080808204829.7744.11661.stgit@yoghurt","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T21:22:45Z","receivedAt":"2008-08-08T21:22:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström <kha@treskal.com> writes:\n\n> In addition to accepting lines with one or more commits, it now\n> accepts lines with precisely two trees.\n\nHmm, slightly dissapointed (I actually was hoping you would also handle\nmore than two trees and run -m or -c or --cc on them).\n\n\"On the command line, you can give exactly two trees, not three nor one;\nthis two-tree form is now also supported in --stdin mode.\" --- that\njustfication sounds like a good one (and that is why my dissapointment is\nonly \"slight\").\n\nBut the following two sentences are a bit confusing, especially it is\nunclear what \"This\" refers to in the last sentence.\n\n> When diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,\n> --encoding, --no-commit-id, -c, --cc, and --always options are\n> ignored, since they do not apply to trees. This is the same behavior\n> you get when specifying two trees on the command line instead of with\n> --stdin.\n\nPerhaps swap the sentences in the log message like this?\n\n  When feeding trees on the command line, you can give exactly two trees,\n  not three nor one; --stdin now supports this \"two tree\" form on its\n  input, in addition to accepting lines with one or more commits.\n\n  When diffing trees (either specified on the command line or from the\n  standard input), the -m, -s, -v, --pretty, --abbrev-commit, --encoding,\n  --no-commit-id, -c, --cc, and --always options are ignored, since they\n  do not apply to trees.\n\nThanks, now we can update that documentation change.\n"},{"id":"86551","messageId":"20080808214523.GA31424@sigill.intra.peff.net","threadId":"14896","inReplyTo":"20080808204829.7744.11661.stgit@yoghurt","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-08T21:45:23Z","receivedAt":"2008-08-08T21:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 08, 2008 at 10:48:29PM +0200, Karl Hasselström wrote:\n\n>  --stdin::\n>  \tWhen '--stdin' is specified, the command does not take\n>  \t<tree-ish> arguments from the command line.  Instead, it\n> -\treads either one <commit> or a list of <commit>\n> -\tseparated with a single space from its standard input.\n> +\treads lines containing either two <tree>, one <commit>, or a\n> +\tlist of <commit> from its standard input.  (Use a single space\n> +\tas separator.)\n\nHmm. Just looking at this as a git user, I would have expected it to\ntake one or more hashes, separated by spaces. If only one, then it must\nbe a commit, and it is diffed against its parents. If more than one,\nthen each must be a tree-ish. So you could diff a commit against a tree\n(or a tag against a commit, or...).\n\nAnd I think it might even be easier to code. ;)\n\n-Peff\n"},{"id":"86590","messageId":"20080809095605.GA10804@diana.vm.bytemark.co.uk","threadId":"14896","inReplyTo":"7vk5erb3ru.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-09T09:56:05Z","receivedAt":"2008-08-09T09:56:05Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-08-08 14:22:45 -0700, Junio C Hamano wrote:\n\n> Karl Hasselström <kha@treskal.com> writes:\n>\n> > In addition to accepting lines with one or more commits, it now\n> > accepts lines with precisely two trees.\n>\n> Hmm, slightly dissapointed (I actually was hoping you would also\n> handle more than two trees and run -m or -c or --cc on them).\n\nI decided to not attempt that -- I'm unsure enough of my git\nprogramming skills that I decided the smaller jump would provide\nsufficient material for constructive criticism. And, perhaps more\nimportantly, I personally don't have a need for anything beyond what I\nimplemented. (And, I'm not burning any bridges -- the multiple-tree\nforms can be added in the future. Along with handling them on the\ncommand line too, hopefully.)\n\n> But the following two sentences are a bit confusing, especially it\n> is unclear what \"This\" refers to in the last sentence.\n>\n> > When diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,\n> > --encoding, --no-commit-id, -c, --cc, and --always options are\n> > ignored, since they do not apply to trees. This is the same\n> > behavior you get when specifying two trees on the command line\n> > instead of with --stdin.\n>\n> Perhaps swap the sentences in the log message like this?\n>\n>   When feeding trees on the command line, you can give exactly two\n>   trees, not three nor one; --stdin now supports this \"two tree\"\n>   form on its input, in addition to accepting lines with one or more\n>   commits.\n>\n>   When diffing trees (either specified on the command line or from\n>   the standard input), the -m, -s, -v, --pretty, --abbrev-commit,\n>   --encoding, --no-commit-id, -c, --cc, and --always options are\n>   ignored, since they do not apply to trees.\n\nWill do. Thanks.\n\n> Thanks, now we can update that documentation change.\n\nI think your doc patch is obsoleted by my patch. I'll make sure it's\nall taken care of in the resend.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"86592","messageId":"20080809100049.GB10804@diana.vm.bytemark.co.uk","threadId":"14896","inReplyTo":"20080808214523.GA31424@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-09T10:00:49Z","receivedAt":"2008-08-09T10:00:49Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-08-08 17:45:23 -0400, Jeff King wrote:\n\n> Hmm. Just looking at this as a git user, I would have expected it to\n> take one or more hashes, separated by spaces. If only one, then it\n> must be a commit, and it is diffed against its parents. If more than\n> one, then each must be a tree-ish. So you could diff a commit\n> against a tree (or a tag against a commit, or...).\n\nI agree.\n\n> And I think it might even be easier to code. ;)\n\nNot for someone who's almost entirely unfamiliar with the git API.\nFinding the right functions to call takes a lot of time ... which is\nwhy I decided to chicken out and implement only the subset I actually\nneeded. But it can be added later -- perhaps by me.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"86599","messageId":"20080809120816.11085.66578.stgit@yoghurt","threadId":"14896","inReplyTo":"20080809095605.GA10804@diana.vm.bytemark.co.uk","subject":"[PATCH 2/3 v2] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-09T12:11:18Z","receivedAt":"2008-08-09T12:11:18Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"When feeding trees on the command line, you can give exactly two\ntrees, not three nor one; --stdin now supports this \"two tree\" form on\nits input, in addition to accepting lines with one or more commits.\n\nWhen diffing trees (either specified on the command line or from the\nstandard input), the -m, -s, -v, --pretty, --abbrev-commit,\n--encoding, --no-commit-id, -c, --cc, and --always options are\nignored, since they do not apply to trees.\n\nSigned-off-by: Karl Hasselström <kha@treskal.com>\n\n---\n\nWith updated commit message. (And I checked -- your earlier doc patch\n_is_ obsoleted by this patch, so I saw no need to change anything\nelse.)\n\n Documentation/git-diff-tree.txt |   14 +++++++++-----\n builtin-diff-tree.c             |   35 +++++++++++++++++++++++++++++++----\n 2 files changed, 40 insertions(+), 9 deletions(-)\n\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\nindex 1fdf20d..0b1ade8 100644\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -49,13 +49,17 @@ include::diff-options.txt[]\n --stdin::\n \tWhen '--stdin' is specified, the command does not take\n \t<tree-ish> arguments from the command line.  Instead, it\n-\treads either one <commit> or a list of <commit>\n-\tseparated with a single space from its standard input.\n+\treads lines containing either two <tree>, one <commit>, or a\n+\tlist of <commit> from its standard input.  (Use a single space\n+\tas separator.)\n +\n-When a single commit is given on one line of such input, it compares\n-the commit with its parents.  The following flags further affects its\n-behavior.  The remaining commits, when given, are used as if they are\n+When two trees are given, it compares the first tree with the second.\n+When a single commit is given, it compares the commit with its\n+parents.  The remaining commits, when given, are used as if they are\n parents of the first commit.\n++\n+The following flags further affects the behavior when comparing\n+commits (but not trees).\n \n -m::\n \tBy default, 'git-diff-tree --stdin' does not show\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex ebbd631..0bdb1cf 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -42,21 +42,48 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)\n \treturn log_tree_commit(&log_tree_opt, commit);\n }\n \n+/* Diff two trees. */\n+static int stdin_diff_trees(struct tree *tree1, char *line, int len)\n+{\n+\tunsigned char sha1[20];\n+\tstruct tree *tree2;\n+\tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {\n+\t\terror(\"Need precisely two trees, separated by one space\");\n+\t\treturn -1;\n+\t}\n+\ttree2 = lookup_tree(sha1);\n+\tif (!tree2 || parse_tree(tree2))\n+\t\treturn -1;\n+\tprintf(\"%s %s\\n\", sha1_to_hex(tree1->object.sha1),\n+\t\t\t  sha1_to_hex(tree2->object.sha1));\n+\tdiff_tree_sha1(tree1->object.sha1, tree2->object.sha1,\n+\t\t       \"\", &log_tree_opt.diffopt);\n+\tlog_tree_diff_flush(&log_tree_opt);\n+\treturn 0;\n+}\n+\n static int diff_tree_stdin(char *line)\n {\n \tint len = strlen(line);\n \tunsigned char sha1[20];\n-\tstruct commit *commit;\n+\tstruct object *obj;\n \n \tif (!len || line[len-1] != '\\n')\n \t\treturn -1;\n \tline[len-1] = 0;\n \tif (get_sha1_hex(line, sha1))\n \t\treturn -1;\n-\tcommit = lookup_commit(sha1);\n-\tif (!commit || parse_commit(commit))\n+\tobj = lookup_object(sha1);\n+\tobj = obj ? obj : parse_object(sha1);\n+\tif (!obj)\n \t\treturn -1;\n-\treturn stdin_diff_commit(commit, line, len);\n+\tif (obj->type == OBJ_COMMIT)\n+\t\treturn stdin_diff_commit((struct commit *)obj, line, len);\n+\tif (obj->type == OBJ_TREE)\n+\t\treturn stdin_diff_trees((struct tree *)obj, line, len);\n+\terror(\"Object %s is a %s, not a commit or tree\",\n+\t      sha1_to_hex(sha1), typename(obj->type));\n+\treturn -1;\n }\n \n static const char diff_tree_usage[] =\n"},{"id":"86610","messageId":"7viqua54vd.fsf@gitster.siamese.dyndns.org","threadId":"14896","inReplyTo":"20080809095605.GA10804@diana.vm.bytemark.co.uk","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-09T20:07:50Z","receivedAt":"2008-08-09T20:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström <kha@treskal.com> writes:\n\n> On 2008-08-08 14:22:45 -0700, Junio C Hamano wrote:\n> ...\n>> But the following two sentences are a bit confusing, especially it\n>> is unclear what \"This\" refers to in the last sentence.\n>>\n>> > When diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,\n>> > --encoding, --no-commit-id, -c, --cc, and --always options are\n>> > ignored, since they do not apply to trees. This is the same\n>> > behavior you get when specifying two trees on the command line\n>> > instead of with --stdin.\n>>\n>> Perhaps swap the sentences in the log message like this?\n>>\n>>   When feeding trees on the command line, you can give exactly two\n>>   trees, not three nor one; --stdin now supports this \"two tree\"\n>>   form on its input, in addition to accepting lines with one or more\n>>   commits.\n>>\n>>   When diffing trees (either specified on the command line or from\n>>   the standard input), the -m, -s, -v, --pretty, --abbrev-commit,\n>>   --encoding, --no-commit-id, -c, --cc, and --always options are\n>>   ignored, since they do not apply to trees.\n>\n> Will do. Thanks.\n\nThinking about it a bit more, -m, -c and --cc are not about commits at\nall.  Your excuse not to support them is because these three are about\ndiffing more than two trees (and I'd say that is still a good rationale).\n"},{"id":"86611","messageId":"20080809203651.GA18968@diana.vm.bytemark.co.uk","threadId":"14896","inReplyTo":"7viqua54vd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-09T20:36:51Z","receivedAt":"2008-08-09T20:36:51Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-08-09 13:07:50 -0700, Junio C Hamano wrote:\n\n> Thinking about it a bit more, -m, -c and --cc are not about commits\n> at all. Your excuse not to support them is because these three are\n> about diffing more than two trees (and I'd say that is still a good\n> rationale).\n\nIndeed (but only because that mode is not supported on the command\nline). Would you like a commit message with a revised excuse?\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"86612","messageId":"7vwsip53bg.fsf@gitster.siamese.dyndns.org","threadId":"14896","inReplyTo":"20080809120816.11085.66578.stgit@yoghurt","subject":"Re: [PATCH 2/3 v2] Teach git diff-tree --stdin to diff trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-09T20:41:23Z","receivedAt":"2008-08-09T20:41:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Hasselström <kha@treskal.com> writes:\n\n> When feeding trees on the command line, you can give exactly two\n> trees, not three nor one; --stdin now supports this \"two tree\" form on\n> its input, in addition to accepting lines with one or more commits.\n>\n> When diffing trees (either specified on the command line or from the\n> standard input), the -m, -s, -v, --pretty, --abbrev-commit,\n> --encoding, --no-commit-id, -c, --cc, and --always options are\n> ignored, since they do not apply to trees.\n\nI've commented on this part already; -m, -c, --cc are excluded because\nthey make sense only when you are dealing with three or more trees.\n\n> diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\n> index ebbd631..0bdb1cf 100644\n> --- a/builtin-diff-tree.c\n> +++ b/builtin-diff-tree.c\n> @@ -42,21 +42,48 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)\n>  \treturn log_tree_commit(&log_tree_opt, commit);\n>  }\n>  \n> +/* Diff two trees. */\n> +static int stdin_diff_trees(struct tree *tree1, char *line, int len)\n> +{\n> +\tunsigned char sha1[20];\n> +\tstruct tree *tree2;\n> +\tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {\n> +\t\terror(\"Need precisely two trees, separated by one space\");\n> +\t\treturn -1;\n> +\t}\n\nerror() returns -1, so:\n\n\tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))\n\t\treturn error(\"Need two trees, separated by one space\");\n\n> +\ttree2 = lookup_tree(sha1);\n> +\tif (!tree2 || parse_tree(tree2))\n> +\t\treturn -1;\n\nDon't you want to make error() say something here as well?\n\n> +\tprintf(\"%s %s\\n\", sha1_to_hex(tree1->object.sha1),\n> +\t\t\t  sha1_to_hex(tree2->object.sha1));\n\nSince this is strictly for Porcelain's use, you may want to document this\noutput format.\n\nTwo-tree form from the command line does not have anything like this, and\ntwo-commit form from --stdin have either a single object name, the log\nmessage under -v or --pretty options.  I notice that these are not\ndocumented but we may want to document it while at it.\n\nOther than that, the patch looks good.  Thanks.\n"},{"id":"86675","messageId":"20080810153848.GA28032@diana.vm.bytemark.co.uk","threadId":"14896","inReplyTo":"7vwsip53bg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3 v2] Teach git diff-tree --stdin to diff trees","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-08-10T15:38:48Z","receivedAt":"2008-08-10T15:38:48Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-08-09 13:41:23 -0700, Junio C Hamano wrote:\n\n> Karl Hasselström <kha@treskal.com> writes:\n>\n> > When diffing trees (either specified on the command line or from\n> > the standard input), the -m, -s, -v, --pretty, --abbrev-commit,\n> > --encoding, --no-commit-id, -c, --cc, and --always options are\n> > ignored, since they do not apply to trees.\n>\n> I've commented on this part already; -m, -c, --cc are excluded\n> because they make sense only when you are dealing with three or more\n> trees.\n\nFixed.\n\n> > +\tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {\n> > +\t\terror(\"Need precisely two trees, separated by one space\");\n> > +\t\treturn -1;\n> > +\t}\n>\n> error() returns -1, so:\n>\n> \tif (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))\n> \t\treturn error(\"Need two trees, separated by one space\");\n\nFixed.\n\n> > +\ttree2 = lookup_tree(sha1);\n> > +\tif (!tree2 || parse_tree(tree2))\n> > +\t\treturn -1;\n>\n> Don't you want to make error() say something here as well?\n\nLooking at lookup_tree() and parse_tree(), I got the impression that\nthey take care of that themselves. Do they miss some case that I need\nto cover?\n\n> > +\tprintf(\"%s %s\\n\", sha1_to_hex(tree1->object.sha1),\n> > +\t\t\t  sha1_to_hex(tree2->object.sha1));\n>\n> Since this is strictly for Porcelain's use, you may want to document\n> this output format.\n\nYes. Fixed.\n\n> Two-tree form from the command line does not have anything like\n> this, and two-commit form from --stdin have either a single object\n> name, the log message under -v or --pretty options. I notice that\n> these are not documented but we may want to document it while at it.\n\nI'll whip something up and send it out as a separate patch.\n\n> Other than that, the patch looks good. Thanks.\n\nThanks for the feedback.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"86843","messageId":"20080811222836.GA12301@sigill.intra.peff.net","threadId":"14896","inReplyTo":"20080809100049.GB10804@diana.vm.bytemark.co.uk","subject":"Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-11T22:28:36Z","receivedAt":"2008-08-11T22:28:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 09, 2008 at 12:00:49PM +0200, Karl Hasselström wrote:\n\n> > And I think it might even be easier to code. ;)\n> \n> Not for someone who's almost entirely unfamiliar with the git API.\n> Finding the right functions to call takes a lot of time ... which is\n> why I decided to chicken out and implement only the subset I actually\n> needed. But it can be added later -- perhaps by me.\n\n:) I took a quick look, and I don't think it would be too hard to reuse\nthe logic from the command-line codepath. However, your patches are\nalready in 'next', and I don't see much point doing it the other way\nunless there is actually some demand for mixed commit/tree input.\n\n-Peff\n"}]}