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

8 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:07

Karl Hasselström [off-list ref] writes:
quoted
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 1/3] Refactoring: Split up diff_tree_stdin

From: Karl Hasselström <hidden>
Date: 2016-06-15 22:45:07

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"

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

From: Karl Hasselström <hidden>
Date: 2016-06-15 22:45:07

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 3/3] Add test for diff-tree --stdin with two trees

From: Karl Hasselström <hidden>
Date: 2016-06-15 22:45:07

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

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

From: Karl Hasselström <hidden>
Date: 2016-06-15 22:45:07

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

---

 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

From: Jeff King <hidden>
Date: 2016-06-15 22:45:07

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

From: Karl Hasselström <hidden>
Date: 2016-06-15 22:45:07

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

From: Jeff King <hidden>
Date: 2016-06-15 22:45:08

On Sat, Aug 09, 2008 at 12:00:49PM +0200, Karl Hasselström wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help