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

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Karl Hasselström <[hidden email]> 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.


 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Junio C Hamano
On 2008-08-05 13:07:17 -0700, Junio C Hamano wrote:

> Karl Hasselström <[hidden email]> 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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Karl Hasselström <[hidden email]> 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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] Refactoring: Split up diff_tree_stdin

Karl Wiberg-2
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 <[hidden email]>

---

 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"

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Karl Wiberg-2
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 <[hidden email]>

---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Karl Wiberg-2
Signed-off-by: Karl Hasselström <[hidden email]>

---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Karl Wiberg-2
Karl Hasselström <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
In reply to this post by Karl Wiberg-2
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Junio C Hamano
On 2008-08-08 14:22:45 -0700, Junio C Hamano wrote:

> Karl Hasselström <[hidden email]> 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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Jeff King
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
In reply to this post by Karl Wiberg-2
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 <[hidden email]>

---

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Karl Wiberg-2
Karl Hasselström <[hidden email]> 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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Karl Wiberg-2
Karl Hasselström <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

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

> Karl Hasselström <[hidden email]> 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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Karl Wiberg-2
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, [hidden email]
      www.treskal.com/kalle
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12