threads / patch / 162

patch, 4 partsAccept commit in some places when tree is needed.

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

## tl;dr

4 messages between Apr 20, 2005 and Apr 21, 2005. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Junio C Hamano· Apr 20, 2005, 06:08 UTC · lore

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
Show changes to 3 files +31 −16

cache.h, diff-cache.c, sha1_file.c

--- 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;
+}
Junio C Hamano· Apr 20, 2005, 06:35 UTC · re: Junio C Hamano · lore

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

<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(-)
Show changes to 3 files +32 −17

cache.h, diff-cache.c, sha1_file.c

--- 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;
+}
Linus Torvalds· Apr 20, 2005, 15:32 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Apr 21, 2005, 00:19 UTC · re: Linus Torvalds · lore

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

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
Show changes to 2 files +44 −0

cache.h, sha1_file.c

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

← back to recent threads