{"thread":{"id":"11747","subject":"[PATCH] Add builtin dump command, to query a repository using a pipe.","startedAt":"2008-01-26T00:24:34Z","lastAt":"2008-01-26T09:46:28Z","messageCount":3,"participants":["Han-Wen Nienhuys","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"66617","messageId":"fnduk3$d0c$1@ger.gmane.org","threadId":"11747","inReplyTo":null,"subject":"[PATCH] Add builtin dump command, to query a repository using a pipe.","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@xs4all.nl","sentAt":"2008-01-26T00:24:34Z","receivedAt":"2008-01-26T00:24:34Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"\nThis command prints either contents or metadata on stdout for SHA1s or\nsymbolic object names supplied to its stdin, so you can extract many\npieces of data of the repository using a single subprocess.\n\nDocumentation will follow once the code is approved.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n Makefile       |    1 +\n builtin-dump.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n builtin.h      |    1 +\n git.c          |    1 +\n 4 files changed, 103 insertions(+), 0 deletions(-)\n create mode 100644 builtin-dump.c\n\ndiff --git a/Makefile b/Makefile\nindex 21c80e6..63a26a7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -339,6 +339,7 @@ BUILTIN_OBJS = \\\n \tbuiltin-diff-files.o \\\n \tbuiltin-diff-index.o \\\n \tbuiltin-diff-tree.o \\\n+\tbuiltin-dump.o \\\n \tbuiltin-fast-export.o \\\n \tbuiltin-fetch.o \\\n \tbuiltin-fetch-pack.o \\\ndiff --git a/builtin-dump.c b/builtin-dump.c\nnew file mode 100644\nindex 0000000..813c7ab\n--- /dev/null\n+++ b/builtin-dump.c\n@@ -0,0 +1,100 @@\n+/*\n+ * Copyright (C) 2008 Google Inc.\n+ * Author: hanwen@google.com\n+ * \n+ *  This program is free software; you can redistribute it and/or modify\n+ *  it under the terms of the GNU General Public License as published by\n+ *  the Free Software Foundation; either version 2 of the License, or\n+ *  (at your option) any later version.\n+ *\n+ *  This program is distributed in the hope that it will be useful,\n+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of\n+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the\n+ *  GNU General Public License for more details.\n+ *\n+ *  You should have received a copy of the GNU General Public License\n+ *  along with this program; if not, write to the Free Software\n+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA\n+ */\n+\n+#include \"cache.h\"\n+#include \"exec_cmd.h\"\n+#include \"object.h\"\n+#include \"builtin.h\"\n+\n+/*\n+  format:\n+\n+  'show' SP type SP objname LF\n+  =>\n+  'len' number LF\n+  contents LF\n+    \n+  'metadata' SP objname LF\n+  =>\n+  type SP size SP sha1 LF\n+  \n+  for non-existent objects, 'metadata' prints\n+\n+  'none' SP 0 SP null-hash LF\n+  \n+*/\n+\n+typedef unsigned char sha1_t[20];\n+\n+void print_metadata(struct strbuf command_buf) {\n+\tchar *obj_name = strchr(command_buf.buf, ' ') + 1;\n+\tsha1_t sha1 = {0};\n+\tconst char *type_name = \"none\";\n+\tunsigned long size = 0;\n+\tchar header[200];\n+\n+\tif (!get_sha1(obj_name, sha1)) {\n+\t\tenum object_type type = sha1_object_info(sha1, &size);\n+\t\ttype_name = type ? typename(type) : \"none\";\n+\t}\n+\tsprintf(header, \"%s %lu %s\\n\", type_name, size, sha1_to_hex(sha1));\n+\twrite_or_die(1, header, strlen(header));  \n+}\n+\n+\n+void print_show(struct strbuf command_buf) {\n+\tchar *type = strchr(command_buf.buf, ' ') + 1;\n+\tchar *obj_name;\n+\tsha1_t sha1;\n+\tunsigned long size;\n+\tchar header[200];\n+\tvoid *buf;\n+\t\n+\tif (!type)\n+\t\tdie(\"commmand format error: %s\\n\", command_buf.buf);\n+  \n+\tobj_name = strchr(type, ' ');\n+\tif (!obj_name)\n+\t\tdie(\"commmand format error: %s\\n\", command_buf.buf);\n+\t*obj_name = '\\0';\n+\tobj_name++;\n+  \n+\tif (get_sha1(obj_name, sha1))\n+\t\tdie(\"not a valid object name: %s\", obj_name);\n+  \n+\tbuf = read_object_with_reference(sha1, type, &size, NULL);\n+\tsprintf(header, \"len %lu\\n\", size);\n+\twrite_or_die(1, header, strlen(header));\n+\twrite_or_die(1, buf, size);\n+\twrite_or_die(1, \"\\n\", 1);\n+}\n+\n+\n+int cmd_dump(int argc, const char **argv, const char *prefix) {\n+\tstruct strbuf command_buf = STRBUF_INIT;\n+\twhile (strbuf_getline(&command_buf, stdin, '\\n') != EOF) {\n+\t\tif (!prefixcmp(command_buf.buf, \"metadata \"))\n+\t\t\tprint_metadata(command_buf);\n+\t\telse if (!prefixcmp(command_buf.buf, \"print \"))\n+\t\t\tprint_show(command_buf);\n+\t\telse\n+\t\t\tdie(\"unknown command %s\", command_buf.buf);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/builtin.h b/builtin.h\nindex cb675c4..df873f9 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -33,6 +33,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);\n extern int cmd_diff_index(int argc, const char **argv, const char *prefix);\n extern int cmd_diff(int argc, const char **argv, const char *prefix);\n extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);\n+extern int cmd_dump(int argc, const char **argv, const char *prefix);\n extern int cmd_fast_export(int argc, const char **argv, const char *prefix);\n extern int cmd_fetch(int argc, const char **argv, const char *prefix);\n extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex 15fec89..853dbc1 100644\n--- a/git.c\n+++ b/git.c\n@@ -303,6 +303,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"diff-files\", cmd_diff_files },\n \t\t{ \"diff-index\", cmd_diff_index, RUN_SETUP },\n \t\t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP },\n+\t\t{ \"dump\", cmd_dump, RUN_SETUP },\n \t\t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t\t{ \"fetch\", cmd_fetch, RUN_SETUP },\n \t\t{ \"fetch-pack\", cmd_fetch_pack, RUN_SETUP },\n-- \n1.5.3.5.643.g40e25\n"},{"id":"66625","messageId":"7vve5hyln3.fsf@gitster.siamese.dyndns.org","threadId":"11747","inReplyTo":"fnduk3$d0c$1@ger.gmane.org","subject":"Re: [PATCH] Add builtin dump command, to query a repository using a pipe.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-26T09:35:44Z","receivedAt":"2008-01-26T09:35:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@xs4all.nl> writes:\n\n> This command prints either contents or metadata on stdout for SHA1s or\n> symbolic object names supplied to its stdin, so you can extract many\n> pieces of data of the repository using a single subprocess.\n>\n> Documentation will follow once the code is approved.\n\nI will comment on this part at the end.\n\n> +typedef unsigned char sha1_t[20];\n\nHmph.  We do not seem to do typedefs like this elsewhere.  Would\nit be worth doing so everywhere?  I generally stay away from a\nnew type that is \"an array of some type\".\n\n> +void print_metadata(struct strbuf command_buf) {\n\nStyle.  \"{\" at the beginning of a function goes to the beginning\nof a line.\n\nAre you sure you want to pass \"struct strbuf\" by value?  As you\ndo not use anything other than its .buf, you may instead want to\nmake the parameter \"const char *\" and have the caller do buf.buf\nbusiness.\n\n> +\tchar *obj_name = strchr(command_buf.buf, ' ') + 1;\n\nWasts reader's time by making him wonder if this needs to check\nfor malformed command buffer.  The answer happens to be no only\nbecause the caller does something stupid ;-)\n\n\tif (!prefixcmp(command_buf.buf, \"metadata \"))\n\t\tprint_metadata(command_buf);\n\t\nand the strchr is guaranteed to find that ' ' after metadata.\n\n\tif (!prefixcmp(command_buf.buf, \"metadata \"))\n        \tprint_metadata(command_buf.buf + 9);\n\nwould give you the beginning of obj_name in the parameter\nwithout any need for the strchr().\n\n> +\tsha1_t sha1 = {0};\n> +\tconst char *type_name = \"none\";\n> +\tunsigned long size = 0;\n> +\tchar header[200];\n> +\n> +\tif (!get_sha1(obj_name, sha1)) {\n> +\t\tenum object_type type = sha1_object_info(sha1, &size);\n> +\t\ttype_name = type ? typename(type) : \"none\";\n> +\t}\n> +\tsprintf(header, \"%s %lu %s\\n\", type_name, size, sha1_to_hex(sha1));\n> +\twrite_or_die(1, header, strlen(header));  \n> +}\n> +\n> +\n> +void print_show(struct strbuf command_buf) {\n> +\tchar *type = strchr(command_buf.buf, ' ') + 1;\n\nLikewise.\n\n> +\tchar *obj_name;\n> +\tsha1_t sha1;\n> +\tunsigned long size;\n> +\tchar header[200];\n> +\tvoid *buf;\n> +\t\n> +\tif (!type)\n> +\t\tdie(\"commmand format error: %s\\n\", command_buf.buf);\n\nEspecially, this will never trigger, although \"if (!*type)\" would.\n\n> +  \n> +\tobj_name = strchr(type, ' ');\n> +\tif (!obj_name)\n> +\t\tdie(\"commmand format error: %s\\n\", command_buf.buf);\n\nI am not sure if this interface that \"die()\"s on an obviously\nmalformed input is nice to the user (which is the process at the\nother end of the pipe).  It only gets abrupt disconnection\nwithout any diagnosis.  But the user may deserve it, as it is\nabout \"obviously malformed\" input.  I do not have strong opinion\nabout this.\n\n> +\t*obj_name = '\\0';\n> +\tobj_name++;\n> +  \n> +\tif (get_sha1(obj_name, sha1))\n> +\t\tdie(\"not a valid object name: %s\", obj_name);\n> +  \n> +\tbuf = read_object_with_reference(sha1, type, &size, NULL);\n\nThis is not nice and definitely needs to be tightened for a case\nwhere the object does not exist in this repository.  Otherwise\nsafe use of this dump \"server\" will require one \"metadata\" query\nto protect \"print\" query from barfing for each object.\n\n> +\tsprintf(header, \"len %lu\\n\", size);\n> +\twrite_or_die(1, header, strlen(header));\n> +\twrite_or_die(1, buf, size);\n> +\twrite_or_die(1, \"\\n\", 1);\n> +}\n\nIOW, this needs an if() statement that triggers upon (buf == NULL)\nto return an error message back to the other end.\n\nNow, back to the proposed commit log message.\n\n> Documentation will follow once the code is approved.\n\nThis sends a wrong message to the list audiences.  It roughly\nsays:\n\n    I wrote something new.  It is used to read many pieces of\n    data without having to spawn a subprocesses per request.\n    Isn't it nice?\n\n    Oh, by the way, I am not giving you more than the absolute\n    minimum information right now, the code, to let you become\n    qualified to say \"Ah, yes, that new feature makes sense, it\n    meshes well with the way how the rest of the system works,\n    and I think it is a good addition to the git.git project\".\n    If you really want to know how it interfaces to the outside\n    world (i.e. how the input is structured is parsed, and how\n    the output is structured and is designed to be parsed, how\n    errors are reported), go read it.\n\n    But trust me, it's a good code, and you will have\n    documentation once it is accepted anyway.\n\nBut the external interface matters.  It often matters more than\nthe details of how the interface is implemented (which is the\ncode).  If the interface is wrong, time spent on reading and\ncommenting on the code is often wasted.  I had to read all the\ncode to notice that I may not necessarily agree with the\nresponse of the command to a malformed input, or missing\nobjects.  If it was documented, it would have been more obvious\nand many other people would have responded, including the ones\nwho may not be inclined to work on or review dump \"server\"\nimplementation side written in C, but are _very much_ interested\nin using such a server as a client, perhaps from their\nhigher-level language scripts.\n\nIf the patch was about a completely new command and sizeable, it\nis very likely that I might have felt defeated and said \"Ah, Ok,\nI choose to stay unqualified to judge this patch to say 'yes,\nthis is a good idea', if I have to wade through that amount of\ncode\".  Most likely many others would do the same.\n\nPlease don't do that again.  It also gives a false impression\nthat this kind of submission is an appropriate and acceptable\napproach to people new to the list.\n"},{"id":"66627","messageId":"7vr6g4zzpn.fsf@gitster.siamese.dyndns.org","threadId":"11747","inReplyTo":"7vve5hyln3.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add builtin dump command, to query a repository using a pipe.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-26T09:46:28Z","receivedAt":"2008-01-26T09:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Another thing.\n\nAs currently written, the \"dump\" server is strictly one-request, \none-response interface.\n\nI can easily see the command set to be extended to allow\ntransfer of richer set of data (e.g. whether a ref is a symref\nand if so where it points at, querying config variables).  Also,\nif you want to write a repository browser using this \"dump\"\nserver, you would first throw a commit at it, learn its parents\nand its tree, and then you suddenly have _many_ objects that you\ncan simultaneously ask about without doing one-request,\none-response exchange.\n\nSo it might make sense to base this not on line-per-line message\nformat using strbuf_getline() interface, but base it instead on\npacket_read_line()/packet_write() interface, and explicitly mark\nthe end of a batch of request with packet_flush().  That would\nallow the \"protocol\" stream, which would matter even more when\nyou run this over the network, perhaps via git-daemon and/or\nfrom connect.c\n\n\n  \n"}]}