Re: [PATCH 2/3] Teach git diff-tree --stdin to diff trees

Karl Hasselström [off-list ref] writes:

In addition to accepting lines with one or more commits, it now
accepts lines with precisely two trees.

Hmm, slightly dissapointed (I actually was hoping you would also handle
more than two trees and run -m or -c or --cc on them).

"On the command line, you can give exactly two trees, not three nor one;
this two-tree form is now also supported in --stdin mode." --- that
justfication sounds like a good one (and that is why my dissapointment is
only "slight").

But the following two sentences are a bit confusing, especially it is
unclear what "This" refers to in the last sentence.

When diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,
--encoding, --no-commit-id, -c, --cc, and --always options are
ignored, since they do not apply to trees. This is the same behavior
you get when specifying two trees on the command line instead of with
--stdin.

Perhaps swap the sentences in the log message like this?

  When feeding trees on the command line, you can give exactly two trees,
  not three nor one; --stdin now supports this "two tree" form on its
  input, in addition to accepting lines with one or more commits.

  When diffing trees (either specified on the command line or from the
  standard input), the -m, -s, -v, --pretty, --abbrev-commit, --encoding,
  --no-commit-id, -c, --cc, and --always options are ignored, since they
  do not apply to trees.

Thanks, now we can update that documentation change.

On 2008-08-08 14:22:45 -0700, Junio C Hamano wrote:

Karl Hasselström [off-list ref] writes:

quoted

In addition to accepting lines with one or more commits, it now
accepts lines with precisely two trees.

Hmm, slightly dissapointed (I actually was hoping you would also
handle more than two trees and run -m or -c or --cc on them).

I decided to not attempt that -- I'm unsure enough of my git
programming skills that I decided the smaller jump would provide
sufficient material for constructive criticism. And, perhaps more
importantly, I personally don't have a need for anything beyond what I
implemented. (And, I'm not burning any bridges -- the multiple-tree
forms can be added in the future. Along with handling them on the
command line too, hopefully.)

But the following two sentences are a bit confusing, especially it
is unclear what "This" refers to in the last sentence.

quoted

When diffing trees, the -m, -s, -v, --pretty, --abbrev-commit,
--encoding, --no-commit-id, -c, --cc, and --always options are
ignored, since they do not apply to trees. This is the same
behavior you get when specifying two trees on the command line
instead of with --stdin.

Perhaps swap the sentences in the log message like this?

  When feeding trees on the command line, you can give exactly two
  trees, not three nor one; --stdin now supports this "two tree"
  form on its input, in addition to accepting lines with one or more
  commits.

  When diffing trees (either specified on the command line or from
  the standard input), the -m, -s, -v, --pretty, --abbrev-commit,
  --encoding, --no-commit-id, -c, --cc, and --always options are
  ignored, since they do not apply to trees.

Will do. Thanks.

Thanks, now we can update that documentation change.

I think your doc patch is obsoleted by my patch. I'll make sure it's
all taken care of in the resend.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

When feeding trees on the command line, you can give exactly two
trees, not three nor one; --stdin now supports this "two tree" form on
its input, in addition to accepting lines with one or more commits.

When diffing trees (either specified on the command line or from the
standard input), the -m, -s, -v, --pretty, --abbrev-commit,
--encoding, --no-commit-id, -c, --cc, and --always options are
ignored, since they do not apply to trees.

Signed-off-by: Karl Hasselström <redacted>

---

With updated commit message. (And I checked -- your earlier doc patch
_is_ obsoleted by this patch, so I saw no need to change anything
else.)

 Documentation/git-diff-tree.txt |   14 +++++++++-----
 builtin-diff-tree.c             |   35 +++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 1fdf20d..0b1ade8 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt

@@ -49,13 +49,17 @@ include::diff-options.txt[]
 --stdin::
 	When '--stdin' is specified, the command does not take
 	<tree-ish> arguments from the command line.  Instead, it
-	reads either one <commit> or a list of <commit>
-	separated with a single space from its standard input.
+	reads lines containing either two <tree>, one <commit>, or a
+	list of <commit> from its standard input.  (Use a single space
+	as separator.)
 +
-When a single commit is given on one line of such input, it compares
-the commit with its parents.  The following flags further affects its
-behavior.  The remaining commits, when given, are used as if they are
+When two trees are given, it compares the first tree with the second.
+When a single commit is given, it compares the commit with its
+parents.  The remaining commits, when given, are used as if they are
 parents of the first commit.
++
+The following flags further affects the behavior when comparing
+commits (but not trees).
 
 -m::
 	By default, 'git-diff-tree --stdin' does not show

diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index ebbd631..0bdb1cf 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c

@@ -42,21 +42,48 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
 	return log_tree_commit(&log_tree_opt, commit);
 }
 
+/* Diff two trees. */
+static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+{
+	unsigned char sha1[20];
+	struct tree *tree2;
+	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {
+		error("Need precisely two trees, separated by one space");
+		return -1;
+	}
+	tree2 = lookup_tree(sha1);
+	if (!tree2 || parse_tree(tree2))
+		return -1;
+	printf("%s %s\n", sha1_to_hex(tree1->object.sha1),
+			  sha1_to_hex(tree2->object.sha1));
+	diff_tree_sha1(tree1->object.sha1, tree2->object.sha1,
+		       "", &log_tree_opt.diffopt);
+	log_tree_diff_flush(&log_tree_opt);
+	return 0;
+}
+
 static int diff_tree_stdin(char *line)
 {
 	int len = strlen(line);
 	unsigned char sha1[20];
-	struct commit *commit;
+	struct object *obj;
 
 	if (!len || line[len-1] != '\n')
 		return -1;
 	line[len-1] = 0;
 	if (get_sha1_hex(line, sha1))
 		return -1;
-	commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
+	obj = lookup_object(sha1);
+	obj = obj ? obj : parse_object(sha1);
+	if (!obj)
 		return -1;
-	return stdin_diff_commit(commit, line, len);
+	if (obj->type == OBJ_COMMIT)
+		return stdin_diff_commit((struct commit *)obj, line, len);
+	if (obj->type == OBJ_TREE)
+		return stdin_diff_trees((struct tree *)obj, line, len);
+	error("Object %s is a %s, not a commit or tree",
+	      sha1_to_hex(sha1), typename(obj->type));
+	return -1;
 }
 
 static const char diff_tree_usage[] =

Karl Hasselström [off-list ref] writes:

When feeding trees on the command line, you can give exactly two
trees, not three nor one; --stdin now supports this "two tree" form on
its input, in addition to accepting lines with one or more commits.

When diffing trees (either specified on the command line or from the
standard input), the -m, -s, -v, --pretty, --abbrev-commit,
--encoding, --no-commit-id, -c, --cc, and --always options are
ignored, since they do not apply to trees.

I've commented on this part already; -m, -c, --cc are excluded because
they make sense only when you are dealing with three or more trees.

error() returns -1, so:

	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
		return error("Need two trees, separated by one space");

+	tree2 = lookup_tree(sha1);
+	if (!tree2 || parse_tree(tree2))
+		return -1;

Don't you want to make error() say something here as well?

+	printf("%s %s\n", sha1_to_hex(tree1->object.sha1),
+			  sha1_to_hex(tree2->object.sha1));

Since this is strictly for Porcelain's use, you may want to document this
output format.

Two-tree form from the command line does not have anything like this, and
two-commit form from --stdin have either a single object name, the log
message under -v or --pretty options.  I notice that these are not
documented but we may want to document it while at it.

Other than that, the patch looks good.  Thanks.

On 2008-08-09 13:41:23 -0700, Junio C Hamano wrote:

Karl Hasselström [off-list ref] writes:

quoted

When diffing trees (either specified on the command line or from
the standard input), the -m, -s, -v, --pretty, --abbrev-commit,
--encoding, --no-commit-id, -c, --cc, and --always options are
ignored, since they do not apply to trees.

I've commented on this part already; -m, -c, --cc are excluded
because they make sense only when you are dealing with three or more
trees.

Fixed.

quoted

+	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1)) {
+		error("Need precisely two trees, separated by one space");
+		return -1;
+	}

error() returns -1, so:

	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
		return error("Need two trees, separated by one space");

Fixed.

quoted

+	tree2 = lookup_tree(sha1);
+	if (!tree2 || parse_tree(tree2))
+		return -1;

Don't you want to make error() say something here as well?

Looking at lookup_tree() and parse_tree(), I got the impression that
they take care of that themselves. Do they miss some case that I need
to cover?

quoted

+	printf("%s %s\n", sha1_to_hex(tree1->object.sha1),
+			  sha1_to_hex(tree2->object.sha1));

Since this is strictly for Porcelain's use, you may want to document
this output format.

Yes. Fixed.

Two-tree form from the command line does not have anything like
this, and two-commit form from --stdin have either a single object
name, the log message under -v or --pretty options. I notice that
these are not documented but we may want to document it while at it.

I'll whip something up and send it out as a separate patch.

Other than that, the patch looks good. Thanks.

Thanks for the feedback.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Signed-off-by: Karl Hasselström <redacted>

---

 t/t4002-diff-basic.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/t/t4002-diff-basic.sh b/t/t4002-diff-basic.sh
index a4cfde6..27743c4 100755
--- a/t/t4002-diff-basic.sh
+++ b/t/t4002-diff-basic.sh

@@ -169,6 +169,20 @@ test_expect_success \
      cmp -s .test-a .test-recursive-AB'
 
 test_expect_success \
+    'diff-tree --stdin of known trees.' \
+    'echo $tree_A $tree_B | git diff-tree --stdin > .test-a &&
+     echo $tree_A $tree_B > .test-plain-ABx &&
+     cat .test-plain-AB >> .test-plain-ABx &&
+     cmp -s .test-a .test-plain-ABx'
+
+test_expect_success \
+    'diff-tree --stdin of known trees.' \
+    'echo $tree_A $tree_B | git diff-tree -r --stdin > .test-a &&
+     echo $tree_A $tree_B > .test-recursive-ABx &&
+     cat .test-recursive-AB >> .test-recursive-ABx &&
+     cmp -s .test-a .test-recursive-ABx'
+
+test_expect_success \
     'diff-cache O with A in cache' \
     'git read-tree $tree_A &&
      git diff-index --cached $tree_O >.test-a &&

Into a first half that determines what operation to do, and a second
half that does it.

Currently the only operation is diffing one or more commits, but a
later patch will add diffing of trees, at which point this refactoring
will pay off.

Signed-off-by: Karl Hasselström <redacted>

---

 builtin-diff-tree.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 415cb16..ebbd631 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c

@@ -14,20 +14,10 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
 	return log_tree_commit(&log_tree_opt, commit);
 }
 
-static int diff_tree_stdin(char *line)
+/* Diff one or more commits. */
+static int stdin_diff_commit(struct commit *commit, char *line, int len)
 {
-	int len = strlen(line);
 	unsigned char sha1[20];
-	struct commit *commit;
-
-	if (!len || line[len-1] != '\n')
-		return -1;
-	line[len-1] = 0;
-	if (get_sha1_hex(line, sha1))
-		return -1;
-	commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		return -1;
 	if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
 		/* Graft the fake parents locally to the commit */
 		int pos = 41;

@@ -52,6 +42,23 @@ static int diff_tree_stdin(char *line)
 	return log_tree_commit(&log_tree_opt, commit);
 }
 
+static int diff_tree_stdin(char *line)
+{
+	int len = strlen(line);
+	unsigned char sha1[20];
+	struct commit *commit;
+
+	if (!len || line[len-1] != '\n')
+		return -1;
+	line[len-1] = 0;
+	if (get_sha1_hex(line, sha1))
+		return -1;
+	commit = lookup_commit(sha1);
+	if (!commit || parse_commit(commit))
+		return -1;
+	return stdin_diff_commit(commit, line, len);
+}
+
 static const char diff_tree_usage[] =
 "git diff-tree [--stdin] [-m] [-c] [--cc] [-s] [-v] [--pretty] [-t] [-r] [--root] "
 "[<common diff options>] <tree-ish> [<tree-ish>] [<path>...]\n"

It's sort of already documented with the --no-commit-id command-line
flag, but let's not hide important information from the user.

Signed-off-by: Karl Hasselström <redacted>

---

 Documentation/git-diff-tree.txt |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 1fdf20d..1f4b91e 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt

@@ -52,10 +52,14 @@ include::diff-options.txt[]
 	reads either one <commit> or a list of <commit>
 	separated with a single space from its standard input.
 +
-When a single commit is given on one line of such input, it compares
-the commit with its parents.  The following flags further affects its
-behavior.  The remaining commits, when given, are used as if they are
+When a single commit is given, it compares the commit with its
+parents.  The remaining commits, when given, are used as if they are
 parents of the first commit.
++
+The ID of the first (or only) commit, followed by a newline, is
+printed before the differences.
++
+The following flags further affects its behavior.
 
 -m::
 	By default, 'git-diff-tree --stdin' does not show

When feeding trees on the command line, you can give exactly two
trees, not three nor one; --stdin now supports this "two tree" form on
its input, in addition to accepting lines with one or more commits.

When diffing trees (either specified on the command line or from the
standard input), the -s, -v, --pretty, --abbrev-commit, --encoding,
--no-commit-id, and --always options are ignored, since they do not
apply to trees; and the -m, -c, and --cc options are ignored since
they would require three trees, which is not supported (yet).

Signed-off-by: Karl Hasselström <redacted>

---

 Documentation/git-diff-tree.txt |   15 ++++++++++-----
 builtin-diff-tree.c             |   33 +++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 1f4b91e..5d48664 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt

@@ -49,17 +49,22 @@ include::diff-options.txt[]
 --stdin::
 	When '--stdin' is specified, the command does not take
 	<tree-ish> arguments from the command line.  Instead, it
-	reads either one <commit> or a list of <commit>
-	separated with a single space from its standard input.
+	reads lines containing either two <tree>, one <commit>, or a
+	list of <commit> from its standard input.  (Use a single space
+	as separator.)
 +
+When two trees are given, it compares the first tree with the second.
 When a single commit is given, it compares the commit with its
 parents.  The remaining commits, when given, are used as if they are
 parents of the first commit.
 +
-The ID of the first (or only) commit, followed by a newline, is
-printed before the differences.
+When comparing two trees, the ID of both trees (separated by a space
+and terminated by a newline) is printed before the difference.  When
+comparing commits, the ID of the first (or only) commit, followed by a
+newline, is printed.
 +
-The following flags further affects its behavior.
+The following flags further affects the behavior when comparing
+commits (but not trees).
 
 -m::
 	By default, 'git-diff-tree --stdin' does not show

diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index ebbd631..5a56178 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c

@@ -42,21 +42,46 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
 	return log_tree_commit(&log_tree_opt, commit);
 }
 
+/* Diff two trees. */
+static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+{
+	unsigned char sha1[20];
+	struct tree *tree2;
+	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+		return error("Need precisely two trees, separated by a space");
+	tree2 = lookup_tree(sha1);
+	if (!tree2 || parse_tree(tree2))
+		return -1;
+	printf("%s %s\n", sha1_to_hex(tree1->object.sha1),
+			  sha1_to_hex(tree2->object.sha1));
+	diff_tree_sha1(tree1->object.sha1, tree2->object.sha1,
+		       "", &log_tree_opt.diffopt);
+	log_tree_diff_flush(&log_tree_opt);
+	return 0;
+}
+
 static int diff_tree_stdin(char *line)
 {
 	int len = strlen(line);
 	unsigned char sha1[20];
-	struct commit *commit;
+	struct object *obj;
 
 	if (!len || line[len-1] != '\n')
 		return -1;
 	line[len-1] = 0;
 	if (get_sha1_hex(line, sha1))
 		return -1;
-	commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
+	obj = lookup_object(sha1);
+	obj = obj ? obj : parse_object(sha1);
+	if (!obj)
 		return -1;
-	return stdin_diff_commit(commit, line, len);
+	if (obj->type == OBJ_COMMIT)
+		return stdin_diff_commit((struct commit *)obj, line, len);
+	if (obj->type == OBJ_TREE)
+		return stdin_diff_trees((struct tree *)obj, line, len);
+	error("Object %s is a %s, not a commit or tree",
+	      sha1_to_hex(sha1), typename(obj->type));
+	return -1;
 }
 
 static const char diff_tree_usage[] =

Updated after Junio's comments. (Patch 1/4 and 4/4 have not changed
but I'm resending them anyway for convenience.)

---

Karl Hasselström (4):
      Add test for diff-tree --stdin with two trees
      Teach git diff-tree --stdin to diff trees
      diff-tree: Note that the commit ID is printed with --stdin
      Refactoring: Split up diff_tree_stdin


 Documentation/git-diff-tree.txt |   19 ++++++++++---
 builtin-diff-tree.c             |   56 +++++++++++++++++++++++++++++++--------
 t/t4002-diff-basic.sh           |   14 ++++++++++
 3 files changed, 72 insertions(+), 17 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

On 2008-08-10 18:12:39 +0200, Karl Hasselström wrote:

      diff-tree: Note that the commit ID is printed with --stdin

BTW, I imagine this one might be maint material, if deemed acceptable.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle