{"thread":{"id":"162","subject":"[PATCH 1/4] Accept commit in some places when tree is needed.","startedAt":"2005-04-20T06:08:51Z","lastAt":"2005-04-21T00:19:03Z","messageCount":4,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"942","messageId":"7vis2ikmj0.fsf@assigned-by-dhcp.cox.net","threadId":"162","inReplyTo":null,"subject":"[PATCH 1/4] Accept commit in some places when tree is needed.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-20T06:08:51Z","receivedAt":"2005-04-20T06:08:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Similar to the diff-cache command, we should accept commit ID\nwhen tree ID is required but the end-user intent is unambiguous.\n\nThis patch lifts the tree-from-tree-or-commit logic from\ndiff-cache.c and moves it to sha1_file.c, which is a common\nlibrary source for the SHA1 storage part.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n cache.h      |    1 +\n diff-cache.c |   18 ++----------------\n sha1_file.c  |   28 ++++++++++++++++++++++++++++\n 3 files changed, 31 insertions(+), 16 deletions(-)\n\nMakefile: needs update\n--- a/cache.h\n+++ b/cache.h\n@@ -124,5 +124,6 @@ extern void die(const char *err, ...);\n extern int error(const char *err, ...);\n \n extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);\n+extern void *tree_from_tree_or_commit(unsigned char *sha1, char *type, unsigned long *size);\n \n #endif /* CACHE_H */\n--- a/diff-cache.c\n+++ b/diff-cache.c\n@@ -245,23 +245,9 @@ int main(int argc, char **argv)\n \tif (argc != 2 || get_sha1_hex(argv[1], tree_sha1))\n \t\tusage(\"diff-cache [-r] [-z] <tree sha1>\");\n \n+\ttree = tree_from_tree_or_commit(tree_sha1, type, &size);\n \ttree = read_sha1_file(tree_sha1, type, &size);\n \tif (!tree)\n-\t\tdie(\"bad tree object %s\", argv[1]);\n-\n-\t/* We allow people to feed us a commit object, just because we're nice */\n-\tif (!strcmp(type, \"commit\")) {\n-\t\t/* tree sha1 is always at offset 5 (\"tree \") */\n-\t\tif (get_sha1_hex(tree + 5, tree_sha1))\n-\t\t\tdie(\"bad commit object %s\", argv[1]);\n-\t\tfree(tree);\n-\t\ttree = read_sha1_file(tree_sha1, type, &size);       \n-\t\tif (!tree)\n-\t\t\tdie(\"unable to read tree object %s\", sha1_to_hex(tree_sha1));\n-\t}\n-\n-\tif (strcmp(type, \"tree\"))\n-\t\tdie(\"bad tree object %s (%s)\", sha1_to_hex(tree_sha1), type);\n-\n+\t\tdie(\"cannot get tree object from %s\", argv[1]);\n \treturn diff_cache(tree, size, active_cache, active_nr, \"\");\n }\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -245,3 +245,31 @@ int write_sha1_buffer(const unsigned cha\n \tclose(fd);\n \treturn 0;\n }\n+\n+void *tree_from_tree_or_commit(unsigned char *sha1, char *type,\n+\t\t\t       unsigned long *size)\n+{\n+\tvoid *tree = read_sha1_file(sha1, type, size);\n+\tif (!tree)\n+\t\treturn tree;\n+\n+\t/* We allow people to feed us a commit object,\n+\t * just because we're nice.\n+\t */\n+\tif (!strcmp(type, \"commit\")) {\n+\t\t/* tree sha1 is always at offset 5 (\"tree \") */\n+\t\tif (get_sha1_hex(tree + 5, sha1)) {\n+\t\t\tfree(tree);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tfree(tree);\n+\t\ttree = read_sha1_file(sha1, type, size);\n+\t\tif (!tree)\n+\t\t\treturn NULL;\n+\t}\n+\tif (strcmp(type , \"tree\")) {\n+\t\tfree(tree);\n+\t\treturn NULL;\n+\t}\n+\treturn tree;\n+}\n\n"},{"id":"946","messageId":"7vy8bej6q3.fsf@assigned-by-dhcp.cox.net","threadId":"162","inReplyTo":"7vis2ikmj0.fsf@assigned-by-dhcp.cox.net","subject":"(fixed) [PATCH 1/4] Accept commit in some places when tree is needed.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-20T06:35:32Z","receivedAt":"2005-04-20T06:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"<cover-paragraph>\n_BLUSH_  \n\nThe 1/4 in the series was a buggy one I sent by mistake.  Please\nreplace it with this fixed one.  The other three are OK.\n\nBTW, do you have a preferred patch-mail convention to mark the\ncover paragraph like this to be excluded from the commit log,\nlike the three-dash one you mentioned to exclude the tail of\nthe message?\n</cover-paragraph>\n\nSimilar to diff-cache which was introduced recently, when the\nintent is obvious we should accept commit ID when tree ID is\nrequired.  This patch lifts the tree-from-tree-or-commit logic\nfrom diff-cache.c and moves it to sha1_file.c, which is a common\nlibrary source for the SHA1 storage part.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n cache.h      |    1 +\n diff-cache.c |   19 ++-----------------\n sha1_file.c  |   29 +++++++++++++++++++++++++++++\n 3 files changed, 32 insertions(+), 17 deletions(-)\n\n--- a/cache.h\n+++ b/cache.h\n@@ -124,5 +124,6 @@ extern void die(const char *err, ...);\n extern int error(const char *err, ...);\n \n extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);\n+extern void *tree_from_tree_or_commit(const unsigned char *sha1, char *type, unsigned long *size);\n \n #endif /* CACHE_H */\n--- a/diff-cache.c\n+++ b/diff-cache.c\n@@ -245,23 +245,8 @@ int main(int argc, char **argv)\n \tif (argc != 2 || get_sha1_hex(argv[1], tree_sha1))\n \t\tusage(\"diff-cache [-r] [-z] <tree sha1>\");\n \n-\ttree = read_sha1_file(tree_sha1, type, &size);\n+\ttree = tree_from_tree_or_commit(tree_sha1, type, &size);\n \tif (!tree)\n-\t\tdie(\"bad tree object %s\", argv[1]);\n-\n-\t/* We allow people to feed us a commit object, just because we're nice */\n-\tif (!strcmp(type, \"commit\")) {\n-\t\t/* tree sha1 is always at offset 5 (\"tree \") */\n-\t\tif (get_sha1_hex(tree + 5, tree_sha1))\n-\t\t\tdie(\"bad commit object %s\", argv[1]);\n-\t\tfree(tree);\n-\t\ttree = read_sha1_file(tree_sha1, type, &size);       \n-\t\tif (!tree)\n-\t\t\tdie(\"unable to read tree object %s\", sha1_to_hex(tree_sha1));\n-\t}\n-\n-\tif (strcmp(type, \"tree\"))\n-\t\tdie(\"bad tree object %s (%s)\", sha1_to_hex(tree_sha1), type);\n-\n+\t\tdie(\"cannot get tree object from %s\", argv[1]);\n \treturn diff_cache(tree, size, active_cache, active_nr, \"\");\n }\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -245,3 +245,32 @@ int write_sha1_buffer(const unsigned cha\n \tclose(fd);\n \treturn 0;\n }\n+\n+void *tree_from_tree_or_commit(const unsigned char *sha1, char *type,\n+\t\t\t       unsigned long *size)\n+{\n+\tvoid *tree = read_sha1_file(sha1, type, size);\n+\tif (!tree)\n+\t\treturn tree;\n+\n+\t/* We allow people to feed us a commit object,\n+\t * just because we're nice.\n+\t */\n+\tif (!strcmp(type, \"commit\")) {\n+\t\t/* tree sha1 is always at offset 5 (\"tree \") */\n+\t\tchar tree_sha1[20];\n+\t\tif (get_sha1_hex(tree + 5, tree_sha1)) {\n+\t\t\tfree(tree);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tfree(tree);\n+\t\ttree = read_sha1_file(tree_sha1, type, size);\n+\t\tif (!tree)\n+\t\t\treturn NULL;\n+\t}\n+\tif (strcmp(type , \"tree\")) {\n+\t\tfree(tree);\n+\t\treturn NULL;\n+\t}\n+\treturn tree;\n+}\n\n"},{"id":"988","messageId":"Pine.LNX.4.58.0504200826360.6467@ppc970.osdl.org","threadId":"162","inReplyTo":"7vis2ikmj0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/4] Accept commit in some places when tree is needed.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-04-20T15:32:13Z","receivedAt":"2005-04-20T15:32:13Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 19 Apr 2005, Junio C Hamano wrote:\n> \n> This patch lifts the tree-from-tree-or-commit logic from\n> diff-cache.c and moves it to sha1_file.c, which is a common\n> library source for the SHA1 storage part.\n\nI don't think that's a good interface. It changes the sha1 passed into it: \nthat may actually be nice, since you may want to know what it changed to, \nbut I think you'd want to have that as an (optional) separate \n\"sha1_result\" parameter. \n\nAlso, the \"type\" or \"size\" things make no sense to have as a parameter \nat all.\n\nIOW, it was fine when it was an internal hacky thing in diff-cache, but \nonce it's promoted to be a real library function it should definitely be \ncleaned up to have sane interfaces that make sense in general, and not \njust within the original context.\n\n\t\tLinus\n"},{"id":"1066","messageId":"7vfyxlhthk.fsf_-_@assigned-by-dhcp.cox.net","threadId":"162","inReplyTo":"Pine.LNX.4.58.0504200826360.6467@ppc970.osdl.org","subject":"(rework) [PATCH 1/5] Accept commit in some places when tree is needed.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-04-21T00:19:03Z","receivedAt":"2005-04-21T00:19:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus,\n\n    sorry for bringing up an issue that is already 8 hours old.\n\nLT> I don't think that's a good interface. It changes the sha1 passed into it: \nLT> that may actually be nice, since you may want to know what it changed to, \nLT> but I think you'd want to have that as an (optional) separate \nLT> \"sha1_result\" parameter. \n\nPoint taken about \"_changing_ _is_ _bad_\" part.  It was a mistake.\n\nLT> Also, the \"type\" or \"size\" things make no sense to have as a parameter \nLT> at all.\n\nWell, the semantics is \"I want to read the raw data of a tree\nand I do not know nor care if this sha1 I got from my user is\nfor a commit or a tree.\"  So type does not matter (if it returns\na non NULL we know it is a tree), but the size matters.\n\nAnd that semantics is not so hacky thing specific to diff-cache.\nRather, it applies in general if you structure the way those\nrecursive walkers do things.  The recursive walkers in ls-tree,\ndiff-cache, and diff-tree all expect the caller to supply the\nbuffer read by sha1_read_buffer, and when it calls itself it\ndoes the same (read-tree's recursing convention is an oddball\nthat needs to be addressed, though).\n\nWhen the recursion is structured this way, the only thing you\nneed to do to allow commit ID from the user when tree ID is\nneeded, without breaking the error checking done by the part\nthat recurses down (i.e. we must error on a commit object ID\nwhen we are expecting a tree object ID stored in objects we read\nfrom the tree downwards), is to change the top-level caller to\nuse \"I want tree with this tree/commit ID\" instead of \"I want a\nbuffer with this ID and I'll make sure it is a tree myself\".\nInstead, you make the recursor \"Give me a buffer and its type,\nI'll barf if it is does not say a tree.\"  When the recursor\ncalls itself, it reads with read_sha1_file and feeds the result\nto itself and have the called do the checking.\n\nThe commit_to_tree() thing you introduced in diff-tree.c is\nsimple to use.  IMHO it is however conceptually a wrong thing to\nuse in these contexts.  When the user supplies a tree ID, you\nfirst read that object only to see if it is not a commit and\nthrow it away, then immediately read it again for your real\nprocessing.  In these particular cases of four tree- related\nfiles, \"I want tree with this tree/commit ID\" semantics is a\n_far_ _better_ match for the problem.\n\nHaving said that, here is a reworked version.  This first one \nintroduces read_tree_with_tree_or_commit_sha1() function.\n\n<end-of-cover-letter>\n\nThis patch implements read_tree_with_tree_or_commit_sha1(),\nwhich can be used when you are interested in reading an unpacked\nraw tree data but you do not know nor care if the SHA1 you\nobtained your user is a tree ID or a commit ID.  Before this\nfunction's introduction, you would have called read_sha1_file(),\nexamined its type, parsed it to call read_sha1_file() again if\nit is a commit, and verified that the resulting object is a\ntree.  Instead, this function does that for you.  It returns\nNULL if the given SHA1 is not either a tree or a commit.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n cache.h     |    4 ++++\n sha1_file.c |   40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+)\n\ncache.h: eab355da5d2f6595053f28f0cca61181ac314ee9\n--- a/cache.h\n+++ b/cache.h\n@@ -124,4 +124,8 @@ extern int error(const char *err, ...);\n \n extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);\n \n+extern void *read_tree_with_tree_or_commit_sha1(const unsigned char *sha1,\n+\t\t\t\t\t\tunsigned long *size,\n+\t\t\t\t\t\tunsigned char *tree_sha1_ret);\n+\n #endif /* CACHE_H */\n\n\nsha1_file.c: eee3598bb75e2199045b823f007e7933c0fb9cfe\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -166,6 +166,46 @@ void * read_sha1_file(const unsigned cha\n \treturn NULL;\n }\n \n+void *read_tree_with_tree_or_commit_sha1(const unsigned char *sha1,\n+\t\t\t\t\t unsigned long *size,\n+\t\t\t\t\t unsigned char *tree_sha1_return)\n+{\n+\tchar type[20];\n+\tvoid *buffer;\n+\tunsigned long isize;\n+\tint was_commit = 0;\n+\tchar tree_sha1[20];\n+\n+\tbuffer = read_sha1_file(sha1, type, &isize);\n+\n+\t/* \n+\t * We might have read a commit instead of a tree, in which case\n+\t * we parse out the tree_sha1 and attempt to read from there.\n+\t * (buffer + 5) is because the tree sha1 is always at offset 5\n+\t * in a commit record (\"tree \").\n+\t */\n+\tif (buffer &&\n+\t    !strcmp(type, \"commit\") &&\n+\t    !get_sha1_hex(buffer + 5, tree_sha1)) {\n+\t\tfree(buffer);\n+\t\tbuffer = read_sha1_file(tree_sha1, type, &isize);\n+\t\twas_commit = 1;\n+\t}\n+\n+\t/*\n+\t * Now do we have something and if so is it a tree?\n+\t */\n+\tif (!buffer || strcmp(type, \"tree\")) {\n+\t\tfree(buffer);\n+\t\treturn;\n+\t}\n+\n+\t*size = isize;\n+\tif (tree_sha1_return)\n+\t\tmemcpy(tree_sha1_return, was_commit ? tree_sha1 : sha1, 20);\n+\treturn buffer;\n+}\n+\n int write_sha1_file(char *buf, unsigned len, unsigned char *returnsha1)\n {\n \tint size;\n\n"}]}