[PATCH 1/4] Accept commit in some places when tree is needed.

[PATCH 1/4] Accept commit in some places when tree is needed.

[Junio C Hamano]

Similar to the diff-cache command, we should accept commit ID
when tree ID is required but the end-user intent is unambiguous.

This patch lifts the tree-from-tree-or-commit logic from
diff-cache.c and moves it to sha1_file.c, which is a common
library source for the SHA1 storage part.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 cache.h      |    1 +
 diff-cache.c |   18 ++----------------
 sha1_file.c  |   28 ++++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 16 deletions(-)

Makefile: needs update
--- a/cache.h
+++ b/cache.h
@@ -124,5 +124,6 @@ extern void die(const char *err, ...);
 extern int error(const char *err, ...);
 
 extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
+extern void *tree_from_tree_or_commit(unsigned char *sha1, char *type, unsigned long *size);
 
 #endif /* CACHE_H */
--- a/diff-cache.c
+++ b/diff-cache.c
@@ -245,23 +245,9 @@ int main(int argc, char **argv)
 	if (argc != 2 || get_sha1_hex(argv[1], tree_sha1))
 		usage("diff-cache [-r] [-z] <tree sha1>");
 
+	tree = tree_from_tree_or_commit(tree_sha1, type, &size);
 	tree = read_sha1_file(tree_sha1, type, &size);
 	if (!tree)
-		die("bad tree object %s", argv[1]);
-
-	/* We allow people to feed us a commit object, just because we're nice */
-	if (!strcmp(type, "commit")) {
-		/* tree sha1 is always at offset 5 ("tree ") */
-		if (get_sha1_hex(tree + 5, tree_sha1))
-			die("bad commit object %s", argv[1]);
-		free(tree);
-		tree = read_sha1_file(tree_sha1, type, &size);       
-		if (!tree)
-			die("unable to read tree object %s", sha1_to_hex(tree_sha1));
-	}
-
-	if (strcmp(type, "tree"))
-		die("bad tree object %s (%s)", sha1_to_hex(tree_sha1), type);
-
+		die("cannot get tree object from %s", argv[1]);
 	return diff_cache(tree, size, active_cache, active_nr, "");
 }
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -245,3 +245,31 @@ int write_sha1_buffer(const unsigned cha
 	close(fd);
 	return 0;
 }
+
+void *tree_from_tree_or_commit(unsigned char *sha1, char *type,
+			       unsigned long *size)
+{
+	void *tree = read_sha1_file(sha1, type, size);
+	if (!tree)
+		return tree;
+
+	/* We allow people to feed us a commit object,
+	 * just because we're nice.
+	 */
+	if (!strcmp(type, "commit")) {
+		/* tree sha1 is always at offset 5 ("tree ") */
+		if (get_sha1_hex(tree + 5, sha1)) {
+			free(tree);
+			return NULL;
+		}
+		free(tree);
+		tree = read_sha1_file(sha1, type, size);
+		if (!tree)
+			return NULL;
+	}
+	if (strcmp(type , "tree")) {
+		free(tree);
+		return NULL;
+	}
+	return tree;
+}

(fixed) [PATCH 1/4] Accept commit in some places when tree is needed.

[Junio C Hamano]

<cover-paragraph>
_BLUSH_  

The 1/4 in the series was a buggy one I sent by mistake.  Please
replace it with this fixed one.  The other three are OK.

BTW, do you have a preferred patch-mail convention to mark the
cover paragraph like this to be excluded from the commit log,
like the three-dash one you mentioned to exclude the tail of
the message?
</cover-paragraph>

Similar to diff-cache which was introduced recently, when the
intent is obvious we should accept commit ID when tree ID is
required.  This patch lifts the tree-from-tree-or-commit logic
from diff-cache.c and moves it to sha1_file.c, which is a common
library source for the SHA1 storage part.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 cache.h      |    1 +
 diff-cache.c |   19 ++-----------------
 sha1_file.c  |   29 +++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 17 deletions(-)

--- a/cache.h
+++ b/cache.h
@@ -124,5 +124,6 @@ extern void die(const char *err, ...);
 extern int error(const char *err, ...);
 
 extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
+extern void *tree_from_tree_or_commit(const unsigned char *sha1, char *type, unsigned long *size);
 
 #endif /* CACHE_H */
--- a/diff-cache.c
+++ b/diff-cache.c
@@ -245,23 +245,8 @@ int main(int argc, char **argv)
 	if (argc != 2 || get_sha1_hex(argv[1], tree_sha1))
 		usage("diff-cache [-r] [-z] <tree sha1>");
 
-	tree = read_sha1_file(tree_sha1, type, &size);
+	tree = tree_from_tree_or_commit(tree_sha1, type, &size);
 	if (!tree)
-		die("bad tree object %s", argv[1]);
-
-	/* We allow people to feed us a commit object, just because we're nice */
-	if (!strcmp(type, "commit")) {
-		/* tree sha1 is always at offset 5 ("tree ") */
-		if (get_sha1_hex(tree + 5, tree_sha1))
-			die("bad commit object %s", argv[1]);
-		free(tree);
-		tree = read_sha1_file(tree_sha1, type, &size);       
-		if (!tree)
-			die("unable to read tree object %s", sha1_to_hex(tree_sha1));
-	}
-
-	if (strcmp(type, "tree"))
-		die("bad tree object %s (%s)", sha1_to_hex(tree_sha1), type);
-
+		die("cannot get tree object from %s", argv[1]);
 	return diff_cache(tree, size, active_cache, active_nr, "");
 }
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -245,3 +245,32 @@ int write_sha1_buffer(const unsigned cha
 	close(fd);
 	return 0;
 }
+
+void *tree_from_tree_or_commit(const unsigned char *sha1, char *type,
+			       unsigned long *size)
+{
+	void *tree = read_sha1_file(sha1, type, size);
+	if (!tree)
+		return tree;
+
+	/* We allow people to feed us a commit object,
+	 * just because we're nice.
+	 */
+	if (!strcmp(type, "commit")) {
+		/* tree sha1 is always at offset 5 ("tree ") */
+		char tree_sha1[20];
+		if (get_sha1_hex(tree + 5, tree_sha1)) {
+			free(tree);
+			return NULL;
+		}
+		free(tree);
+		tree = read_sha1_file(tree_sha1, type, size);
+		if (!tree)
+			return NULL;
+	}
+	if (strcmp(type , "tree")) {
+		free(tree);
+		return NULL;
+	}
+	return tree;
+}

Re: [PATCH 1/4] Accept commit in some places when tree is needed.

[Linus Torvalds]

On Tue, 19 Apr 2005, Junio C Hamano wrote:
> 
> This patch lifts the tree-from-tree-or-commit logic from
> diff-cache.c and moves it to sha1_file.c, which is a common
> library source for the SHA1 storage part.

I don't think that's a good interface. It changes the sha1 passed into it: 
that may actually be nice, since you may want to know what it changed to, 
but I think you'd want to have that as an (optional) separate 
"sha1_result" parameter. 

Also, the "type" or "size" things make no sense to have as a parameter 
at all.

IOW, it was fine when it was an internal hacky thing in diff-cache, but 
once it's promoted to be a real library function it should definitely be 
cleaned up to have sane interfaces that make sense in general, and not 
just within the original context.

		Linus

(rework) [PATCH 1/5] Accept commit in some places when tree is needed.

[Junio C Hamano]

Linus,

    sorry for bringing up an issue that is already 8 hours old.

LT> I don't think that's a good interface. It changes the sha1 passed into it: 
LT> that may actually be nice, since you may want to know what it changed to, 
LT> but I think you'd want to have that as an (optional) separate 
LT> "sha1_result" parameter. 

Point taken about "_changing_ _is_ _bad_" part.  It was a mistake.

LT> Also, the "type" or "size" things make no sense to have as a parameter 
LT> at all.

Well, the semantics is "I want to read the raw data of a tree
and I do not know nor care if this sha1 I got from my user is
for a commit or a tree."  So type does not matter (if it returns
a non NULL we know it is a tree), but the size matters.

And that semantics is not so hacky thing specific to diff-cache.
Rather, it applies in general if you structure the way those
recursive walkers do things.  The recursive walkers in ls-tree,
diff-cache, and diff-tree all expect the caller to supply the
buffer read by sha1_read_buffer, and when it calls itself it
does the same (read-tree's recursing convention is an oddball
that needs to be addressed, though).

When the recursion is structured this way, the only thing you
need to do to allow commit ID from the user when tree ID is
needed, without breaking the error checking done by the part
that recurses down (i.e. we must error on a commit object ID
when we are expecting a tree object ID stored in objects we read
from the tree downwards), is to change the top-level caller to
use "I want tree with this tree/commit ID" instead of "I want a
buffer with this ID and I'll make sure it is a tree myself".
Instead, you make the recursor "Give me a buffer and its type,
I'll barf if it is does not say a tree."  When the recursor
calls itself, it reads with read_sha1_file and feeds the result
to itself and have the called do the checking.

The commit_to_tree() thing you introduced in diff-tree.c is
simple to use.  IMHO it is however conceptually a wrong thing to
use in these contexts.  When the user supplies a tree ID, you
first read that object only to see if it is not a commit and
throw it away, then immediately read it again for your real
processing.  In these particular cases of four tree- related
files, "I want tree with this tree/commit ID" semantics is a
_far_ _better_ match for the problem.

Having said that, here is a reworked version.  This first one 
introduces read_tree_with_tree_or_commit_sha1() function.

<end-of-cover-letter>

This patch implements read_tree_with_tree_or_commit_sha1(),
which can be used when you are interested in reading an unpacked
raw tree data but you do not know nor care if the SHA1 you
obtained your user is a tree ID or a commit ID.  Before this
function's introduction, you would have called read_sha1_file(),
examined its type, parsed it to call read_sha1_file() again if
it is a commit, and verified that the resulting object is a
tree.  Instead, this function does that for you.  It returns
NULL if the given SHA1 is not either a tree or a commit.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 cache.h     |    4 ++++
 sha1_file.c |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

cache.h: eab355da5d2f6595053f28f0cca61181ac314ee9
--- a/cache.h
+++ b/cache.h
@@ -124,4 +124,8 @@ extern int error(const char *err, ...);
 
 extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
 
+extern void *read_tree_with_tree_or_commit_sha1(const unsigned char *sha1,
+						unsigned long *size,
+						unsigned char *tree_sha1_ret);
+
 #endif /* CACHE_H */


sha1_file.c: eee3598bb75e2199045b823f007e7933c0fb9cfe
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -166,6 +166,46 @@ void * read_sha1_file(const unsigned cha
 	return NULL;
 }
 
+void *read_tree_with_tree_or_commit_sha1(const unsigned char *sha1,
+					 unsigned long *size,
+					 unsigned char *tree_sha1_return)
+{
+	char type[20];
+	void *buffer;
+	unsigned long isize;
+	int was_commit = 0;
+	char tree_sha1[20];
+
+	buffer = read_sha1_file(sha1, type, &isize);
+
+	/* 
+	 * We might have read a commit instead of a tree, in which case
+	 * we parse out the tree_sha1 and attempt to read from there.
+	 * (buffer + 5) is because the tree sha1 is always at offset 5
+	 * in a commit record ("tree ").
+	 */
+	if (buffer &&
+	    !strcmp(type, "commit") &&
+	    !get_sha1_hex(buffer + 5, tree_sha1)) {
+		free(buffer);
+		buffer = read_sha1_file(tree_sha1, type, &isize);
+		was_commit = 1;
+	}
+
+	/*
+	 * Now do we have something and if so is it a tree?
+	 */
+	if (!buffer || strcmp(type, "tree")) {
+		free(buffer);
+		return;
+	}
+
+	*size = isize;
+	if (tree_sha1_return)
+		memcpy(tree_sha1_return, was_commit ? tree_sha1 : sha1, 20);
+	return buffer;
+}
+
 int write_sha1_file(char *buf, unsigned len, unsigned char *returnsha1)
 {
 	int size;