[BUG] git diff-tree --stdin doesn't accept two trees

[BUG] git diff-tree --stdin doesn't accept two trees

[Karl Hasselström]

I'm trying to use diff-tree --stdin to diff several trees in one go.
But I just get error messages when I feed it two space-separated trees
(one commit works fine):

  $ echo $(git rev-parse HEAD^{tree}) $(git rev-parse HEAD^^{tree}) | git diff-tree -p --stdin
  error: Object 7bfd9971f77438858e412be0219ec78afb3ca46f not a commit

This is at odds with the documentation:

  --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 pair of <tree-ish>
        separated with a single space from its standard input.

I tried reading the code to figure out what's wrong, and as far as I
can tell the code to do this is there, but seems to be protected by
logic that aborts everything unless the whole input line is a valid
commit. Or maybe I'm just confused ...

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

Re: [BUG] git diff-tree --stdin doesn't accept two trees

[Junio C Hamano]

Karl Hasselström <kha@treskal.com> writes:

> I'm trying to use diff-tree --stdin to diff several trees in one go.
> But I just get error messages when I feed it two space-separated trees
> (one commit works fine):
>
>   $ echo $(git rev-parse HEAD^{tree}) $(git rev-parse HEAD^^{tree}) | git diff-tree -p --stdin
>   error: Object 7bfd9971f77438858e412be0219ec78afb3ca46f not a commit
>
> This is at odds with the documentation:
>
>   --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 pair of <tree-ish>
>         separated with a single space from its standard input.
>
> I tried reading the code to figure out what's wrong, and as far as I
> can tell the code to do this is there, but seems to be protected by
> logic that aborts everything unless the whole input line is a valid
> commit. Or maybe I'm just confused ...

No, the documentation was made wrong during 1.2.0 timeperiod.

The feature of --stdin to take a commit and its parents on one line was
broken before that to support the common

	rev-list --parents $commits... -- $paths... |
        diff-tree --stdin -v -p

usage pattern by Porcelains.  For diff-tree to talk sensibly about
commits, it needs to see commits, not just trees.


 

[PATCH] fix diff-tree --stdin documentation

[Junio C Hamano]

Long time ago, the feature of "diff-tree --stdin" to take a commit and its
parents on one line was broken, and did not support the common:

    git rev-list --parents $commits... -- $paths... |
    git diff-tree --stdin -v -p

usage pattern by Porcelains properly.  For diff-tree to talk sensibly
about commits, it needs to see commits, not just trees; the code was fixed
to take list of commits on the standard input in 1.2.0.

However we left the documentation stale for a long time, until Karl
Hasselström finally noticed it very recently.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 8c8f35b..1fdf20d 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -49,13 +49,13 @@ 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 pair of <tree-ish>
+	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.  This does not apply to the case where two <tree-ish>
-separated with a single space are given.
+behavior.  The remaining commits, when given, are used as if they are
+parents of the first commit.
 
 -m::
 	By default, 'git-diff-tree --stdin' does not show

Re: [PATCH] fix diff-tree --stdin documentation

[Karl Hasselström]

On 2008-08-05 22:32:28 -0700, Junio C Hamano wrote:

> -	reads either one <commit> or a pair of <tree-ish>
> +	reads either one <commit> or a list of <commit>

Thanks. Didn't quite solve my problem though, since diffing trees was
what I wanted to use diff-tree for. :-/ But I think I can rephrase my
problem so that I give it a commit instead.

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

Re: [BUG] git diff-tree --stdin doesn't accept two trees

[Karl Hasselström]

On 2008-08-05 13:07:17 -0700, Junio C Hamano wrote:

> Karl Hasselström <kha@treskal.com> writes:
>
> > I'm trying to use diff-tree --stdin to diff several trees in one
> > go. But I just get error messages when I feed it two
> > space-separated trees (one commit works fine):
>
> No, the documentation was made wrong during 1.2.0 timeperiod.
>
> The feature of --stdin to take a commit and its parents on one line was
> broken before that to support the common
>
>         rev-list --parents $commits... -- $paths... |
>                 diff-tree --stdin -v -p
>
> usage pattern by Porcelains.  For diff-tree to talk sensibly about
> commits, it needs to see commits, not just trees.

But is there any fundamental reason why it couldn't accept tree-ishes
as well? It only talks about commits if asked to do so with
command-line options.

Or would that break existing users somehow?

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

Re: [BUG] git diff-tree --stdin doesn't accept two trees

[Junio C Hamano]

Karl Hasselström <kha@treskal.com> writes:

>> The feature of --stdin to take a commit and its parents on one line was
>> broken before that to support the common
>>
>>         rev-list --parents $commits... -- $paths... |
>>                 diff-tree --stdin -v -p
>>
>> usage pattern by Porcelains.  For diff-tree to talk sensibly about
>> commits, it needs to see commits, not just trees.
>
> But is there any fundamental reason why it couldn't accept tree-ishes
> as well?

The -v option given to diff-tree is the key.  Without it, it could take
trees.

[PATCH 0/3] Teach git diff-tree --stdin to diff trees

[Karl Hasselström]

Here's two patches that implements diffing of trees: first a
refactoring, then the actual functionality.

I'm not familiar with git's API, so I might have made mistakes in
choosing or using functions. Extra eyeballs appreciated. But the test
suite passes, it does what I want, and it does make StGit faster, so
_I'm_ happy ...

---

Karl Hasselström (3):
      Add test for diff-tree --stdin with two trees
      Teach git diff-tree --stdin to diff trees
      Refactoring: Split up diff_tree_stdin


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

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

[PATCH 1/3] Refactoring: Split up diff_tree_stdin

[Karl Hasselström]

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 <kha@treskal.com>

---

 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"

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

[Karl Hasselström]

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

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.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 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[] =

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

[Junio C Hamano]

Karl Hasselström <kha@treskal.com> 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.

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

[Karl Hasselström]

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

> Karl Hasselström <kha@treskal.com> 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).

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.
>
> > 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

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

[Karl Hasselström]

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 <kha@treskal.com>

---

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[] =

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

[Junio C Hamano]

Karl Hasselström <kha@treskal.com> 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.

> 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;
> +	}

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.

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

[Karl Hasselström]

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

> Karl Hasselström <kha@treskal.com> writes:
>
> > 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.

> > +	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.

> > +	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?

> > +	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

[PATCH v3 0/4] Teach git diff-tree --stdin to diff trees

[Karl Hasselström]

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

[PATCH v3 1/4] Refactoring: Split up diff_tree_stdin

[Karl Hasselström]

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 <kha@treskal.com>

---

 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"

[PATCH v3 2/4] diff-tree: Note that the commit ID is printed with --stdin

[Karl Hasselström]

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 <kha@treskal.com>

---

 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

[PATCH v3 3/4] Teach git diff-tree --stdin to diff trees

[Karl Hasselström]

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 <kha@treskal.com>

---

 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[] =

[PATCH v3 4/4] Add test for diff-tree --stdin with two trees

[Karl Hasselström]

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 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 &&

Re: [PATCH v3 0/4] Teach git diff-tree --stdin to diff trees

[Karl Hasselström]

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

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

[Junio C Hamano]

Karl Hasselström <kha@treskal.com> writes:

> On 2008-08-08 14:22:45 -0700, Junio C Hamano wrote:
> ...
>> 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.
>
> Will do. Thanks.

Thinking about it a bit more, -m, -c and --cc are not about commits at
all.  Your excuse not to support them is because these three are about
diffing more than two trees (and I'd say that is still a good rationale).

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

[Karl Hasselström]

On 2008-08-09 13:07:50 -0700, Junio C Hamano wrote:

> Thinking about it a bit more, -m, -c and --cc are not about commits
> at all. Your excuse not to support them is because these three are
> about diffing more than two trees (and I'd say that is still a good
> rationale).

Indeed (but only because that mode is not supported on the command
line). Would you like a commit message with a revised excuse?

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

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

[Jeff King]

On Fri, Aug 08, 2008 at 10:48:29PM +0200, Karl Hasselström wrote:

>  --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.)

Hmm. Just looking at this as a git user, I would have expected it to
take one or more hashes, separated by spaces. If only one, then it must
be a commit, and it is diffed against its parents. If more than one,
then each must be a tree-ish. So you could diff a commit against a tree
(or a tag against a commit, or...).

And I think it might even be easier to code. ;)

-Peff

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

[Karl Hasselström]

On 2008-08-08 17:45:23 -0400, Jeff King wrote:

> Hmm. Just looking at this as a git user, I would have expected it to
> take one or more hashes, separated by spaces. If only one, then it
> must be a commit, and it is diffed against its parents. If more than
> one, then each must be a tree-ish. So you could diff a commit
> against a tree (or a tag against a commit, or...).

I agree.

> And I think it might even be easier to code. ;)

Not for someone who's almost entirely unfamiliar with the git API.
Finding the right functions to call takes a lot of time ... which is
why I decided to chicken out and implement only the subset I actually
needed. But it can be added later -- perhaps by me.

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

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

[Jeff King]

On Sat, Aug 09, 2008 at 12:00:49PM +0200, Karl Hasselström wrote:

> > And I think it might even be easier to code. ;)
> 
> Not for someone who's almost entirely unfamiliar with the git API.
> Finding the right functions to call takes a lot of time ... which is
> why I decided to chicken out and implement only the subset I actually
> needed. But it can be added later -- perhaps by me.

:) I took a quick look, and I don't think it would be too hard to reuse
the logic from the command-line codepath. However, your patches are
already in 'next', and I don't see much point doing it the other way
unless there is actually some demand for mixed commit/tree input.

-Peff

[PATCH 3/3] Add test for diff-tree --stdin with two trees

[Karl Hasselström]

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 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 &&