Add builtin dump command, to query a repository using a pipe.
PATCH 1/1 Add builtin dump command, to query a repository using a pipe.
This command prints either contents or metadata on stdout for SHA1s or symbolic object names supplied to its stdin, so you can extract many pieces of data of the repository using a single subprocess.
Documentation will follow once the code is approved.
--- Makefile | 1 + builtin-dump.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git.c | 1 + 4 files changed, 103 insertions(+), 0 deletions(-) create mode 100644 builtin-dump.c
Makefile +1 -0
@@ -339,6 +339,7 @@ BUILTIN_OBJS = \ builtin-diff-files.o \ builtin-diff-index.o \ builtin-diff-tree.o \ + builtin-dump.o \ builtin-fast-export.o \ builtin-fetch.o \ builtin-fetch-pack.o \
builtin-dump.c new file +100 -0
100 lines not on this page · raw message
builtin.h +1 -0
@@ -33,6 +33,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);extern int cmd_diff_index (int argc,const char **argv,const char *prefix);extern int cmd_diff (int argc,const char **argv,const char *prefix);extern int cmd_diff_tree (int argc,const char **argv,const char *prefix); +extern int cmd_dump (int argc,const char **argv,const char *prefix);extern int cmd_fast_export (int argc,const char **argv,const char *prefix);extern int cmd_fetch (int argc,const char **argv,const char *prefix);extern int cmd_fetch_pack (int argc,const char **argv,const char *prefix);
git.c +1 -0
@@ -303,6 +303,7 @@ static void handle_internal_command(int argc, const char **argv) { "diff-files", cmd_diff_files }, { "diff-index", cmd_diff_index, RUN_SETUP }, { "diff-tree", cmd_diff_tree, RUN_SETUP }, + { "dump", cmd_dump, RUN_SETUP }, { "fast-export", cmd_fast_export, RUN_SETUP }, { "fetch", cmd_fetch, RUN_SETUP }, { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
Signature · 1 line
1.5.3.5.643.g40e25
Quoted 5 lines · Han-Wen Nienhuys
This command prints either contents or metadata on stdout for SHA1s or symbolic object names supplied to its stdin, so you can extract many pieces of data of the repository using a single subprocess.
Documentation will follow once the code is approved.
I will comment on this part at the end.
Quoted 1 line
+typedef unsigned char sha1_t[20];
Hmph. We do not seem to do typedefs like this elsewhere. Would it be worth doing so everywhere? I generally stay away from a new type that is "an array of some type".
Quoted 1 line
+void
print_metadata(struct strbuf command_buf){
Style. "{" at the beginning of a function goes to the beginning of a line.
Are you sure you want to pass "struct strbuf" by value? As you do not use anything other than its .buf, you may instead want to make the parameter "const char *" and have the caller do buf.buf business.
builtin-dump.c line 46
Quoted 1 line
builtin-dump.c new file +1
-0+char *obj_name =strchr (command_buf.buf,' ' ) +1 ;
Wasts reader's time by making him wonder if this needs to check for malformed command buffer. The answer happens to be no only because the caller does something stupid ;-)
if (!prefixcmp(command_buf.buf, "metadata ")) print_metadata(command_buf);
and the strchr is guaranteed to find that ' ' after metadata.
if (!prefixcmp(command_buf.buf, "metadata "))
print_metadata(command_buf.buf + 9);would give you the beginning of obj_name in the parameter without any need for the strchr().
Quoted 6 lines
builtin-dump.c new file +6
-0+sha1_t sha1 = {0 }; +const char *type_name ="none" ; +unsigned long size =0 ; +char header[200 ]; + +if (!get_sha1 (obj_name, sha1)) {
builtin-dump.c line 62
Quoted 10 lines
builtin-dump.c new file +10
-0+enum object_type type =sha1_object_info (sha1, &size); + type_name = type ?typename (type) :"none" ; + } +sprintf (header,"%s %lu %s\n" , type_name, size,sha1_to_hex (sha1)); +write_or_die (1 , header,strlen (header)); +} + + +void print_show (struct strbuf command_buf) { +char *type =strchr (command_buf.buf,' ' ) +1 ;
Likewise.
builtin-dump.c line 70
Quoted 8 lines
builtin-dump.c new file +8
-0+char *obj_name; +sha1_t sha1; +unsigned long size; +char header[200 ]; +void *buf; + +if (!type) +die ("commmand format error: %s\n" , command_buf.buf);
Especially, this will never trigger, although "if (!*type)" would.
builtin-dump.c line 74
Quoted 4 lines
builtin-dump.c new file +4
-0+ + obj_name =strchr (type,' ' ); +if (!obj_name) +die ("commmand format error: %s\n" , command_buf.buf);
I am not sure if this interface that "die()"s on an obviously malformed input is nice to the user (which is the process at the other end of the pipe). It only gets abrupt disconnection without any diagnosis. But the user may deserve it, as it is about "obviously malformed" input. I do not have strong opinion about this.
builtin-dump.c line 81
Quoted 7 lines
builtin-dump.c new file +7
-0+ *obj_name ='\0' ; + obj_name++; + +if (get_sha1 (obj_name, sha1)) +die ("not a valid object name: %s" , obj_name); + + buf =read_object_with_reference (sha1, type, &size,NULL );
This is not nice and definitely needs to be tightened for a case where the object does not exist in this repository. Otherwise safe use of this dump "server" will require one "metadata" query to protect "print" query from barfing for each object.
builtin-dump.c line 86
Quoted 5 lines
builtin-dump.c new file +5
-0+sprintf (header,"len %lu\n" , size); +write_or_die (1 , header,strlen (header)); +write_or_die (1 , buf, size); +write_or_die (1 ,"\n" ,1 ); +}
IOW, this needs an if() statement that triggers upon (buf == NULL) to return an error message back to the other end.
Now, back to the proposed commit log message.
Quoted 1 line
Documentation will follow once the code is approved.
This sends a wrong message to the list audiences. It roughly says:
I wrote something new. It is used to read many pieces of data without having to spawn a subprocesses per request. Isn't it nice?
Oh, by the way, I am not giving you more than the absolute minimum information right now, the code, to let you become qualified to say "Ah, yes, that new feature makes sense, it meshes well with the way how the rest of the system works, and I think it is a good addition to the git.git project". If you really want to know how it interfaces to the outside world (i.e. how the input is structured is parsed, and how the output is structured and is designed to be parsed, how errors are reported), go read it.
But trust me, it's a good code, and you will have documentation once it is accepted anyway.
But the external interface matters. It often matters more than the details of how the interface is implemented (which is the code). If the interface is wrong, time spent on reading and commenting on the code is often wasted. I had to read all the code to notice that I may not necessarily agree with the response of the command to a malformed input, or missing objects. If it was documented, it would have been more obvious and many other people would have responded, including the ones who may not be inclined to work on or review dump "server" implementation side written in C, but are _very much_ interested in using such a server as a client, perhaps from their higher-level language scripts.
If the patch was about a completely new command and sizeable, it is very likely that I might have felt defeated and said "Ah, Ok, I choose to stay unqualified to judge this patch to say 'yes, this is a good idea', if I have to wade through that amount of code". Most likely many others would do the same.
Please don't do that again. It also gives a false impression that this kind of submission is an appropriate and acceptable approach to people new to the list.
Another thing.
As currently written, the "dump" server is strictly one-request, one-response interface.
I can easily see the command set to be extended to allow transfer of richer set of data (e.g. whether a ref is a symref and if so where it points at, querying config variables). Also, if you want to write a repository browser using this "dump" server, you would first throw a commit at it, learn its parents and its tree, and then you suddenly have _many_ objects that you can simultaneously ask about without doing one-request, one-response exchange.
So it might make sense to base this not on line-per-line message format using strbuf_getline() interface, but base it instead on packet_read_line()/packet_write() interface, and explicitly mark the end of a batch of request with packet_flush(). That would allow the "protocol" stream, which would matter even more when you run this over the network, perhaps via git-daemon and/or from connect.c