{"thread":{"id":"20256","subject":"[PATCH 1/3 v2] Add support for external programs for handling native fetches","startedAt":"2009-07-28T06:08:49Z","lastAt":"2009-07-29T22:02:25Z","messageCount":6,"participants":["Daniel Barkalow","Bert Wesarg","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"118923","messageId":"alpine.LNX.2.00.0907280155390.2147@iabervon.org","threadId":"20256","inReplyTo":null,"subject":"[PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-07-28T06:08:49Z","receivedAt":"2009-07-28T06:08:49Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"transport_get() can call transport_shim_init() to have list and\nfetch-ref operations handled by running a separate program as:\n\n git shim-<something> <remote> [<url>]\n\nThis program then accepts, on its stdin, \"list\" and \"fetch <hex>\n<name>\" commands; the former prints out a list of available refs and\neither their hashes or what they are symrefs to, while the latter\nfetches them into the local object database and prints a newline when done.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\nThis version uses the more traditional ordering for listing refs and fixes \na memory leak.\n\n Documentation/git-shim.txt |   37 +++++++++++\n Makefile                   |    1 +\n transport-shim.c           |  142 ++++++++++++++++++++++++++++++++++++++++++++\n transport.h                |    3 +\n 4 files changed, 183 insertions(+), 0 deletions(-)\n create mode 100644 Documentation/git-shim.txt\n create mode 100644 transport-shim.c\n\ndiff --git a/Documentation/git-shim.txt b/Documentation/git-shim.txt\nnew file mode 100644\nindex 0000000..dd80c6d\n--- /dev/null\n+++ b/Documentation/git-shim.txt\n@@ -0,0 +1,37 @@\n+git-shim(1)\n+============\n+\n+NAME\n+----\n+git-shim - Helper programs for interoperation with remote git\n+\n+SYNOPSIS\n+--------\n+'git shim-<transport>' <remote>\n+\n+DESCRIPTION\n+-----------\n+\n+These programs are normally not used directly by end users, but are\n+invoked by various git programs that interact with remote repositories\n+when the repository they would operate on will be accessed using\n+transport code not linked into the main git binary.\n+\n+COMMANDS\n+--------\n+\n+Commands are given by the caller on the helper's standard input, one per line.\n+\n+'list'::\n+\tLists the refs, one per line, if the format \"<value>\n+\t<name>\". The value is either a hex sha1 hash or \"@<dest>\" for\n+\tsymrefs. After the complete list, outputs a blank line.\n+\n+'fetch' ref::\n+\tFetches the given ref, writing the necessary objects to the\n+\tdatabase. Outputs a blank line when the fetch is complete.\n++\n+If a fatal error occurs, the program writes the error message to\n+stderr and exits. The caller should expect that a suitable error\n+message has been printed if the child closes the connection without\n+completing a valid response for the current command.\ndiff --git a/Makefile b/Makefile\nindex bde27ed..01efc73 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -549,6 +549,7 @@ LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += trace.o\n LIB_OBJS += transport.o\n+LIB_OBJS += transport-shim.o\n LIB_OBJS += tree-diff.o\n LIB_OBJS += tree.o\n LIB_OBJS += tree-walk.o\ndiff --git a/transport-shim.c b/transport-shim.c\nnew file mode 100644\nindex 0000000..2518aba\n--- /dev/null\n+++ b/transport-shim.c\n@@ -0,0 +1,142 @@\n+#include \"cache.h\"\n+#include \"transport.h\"\n+\n+#include \"run-command.h\"\n+#include \"commit.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+\n+struct shim_data\n+{\n+\tconst char *name;\n+\tstruct child_process *shim;\n+};\n+\n+static struct child_process *get_shim(struct transport *transport)\n+{\n+\tstruct shim_data *data = transport->data;\n+\tif (!data->shim) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstruct child_process *shim = xcalloc(1, sizeof(*shim));\n+\t\tshim->in = -1;\n+\t\tshim->out = -1;\n+\t\tshim->err = 0;\n+\t\tshim->argv = xcalloc(4, sizeof(*shim->argv));\n+\t\tstrbuf_addf(&buf, \"shim-%s\", data->name);\n+\t\tshim->argv[0] = buf.buf;\n+\t\tshim->argv[1] = transport->remote->name;\n+\t\tshim->argv[2] = transport->url;\n+\t\tshim->git_cmd = 1;\n+\t\tstart_command(shim);\n+\t\tdata->shim = shim;\n+\t}\n+\treturn data->shim;\n+}\n+\n+static int disconnect_shim(struct transport *transport)\n+{\n+\tstruct shim_data *data = transport->data;\n+\tif (data->shim) {\n+\t\twrite(data->shim->in, \"\\n\", 1);\n+\t\tclose(data->shim->in);\n+\t\tfinish_command(data->shim);\n+\t\tfree(data->shim->argv);\n+\t\tfree(data->shim);\n+\t\ttransport->data = NULL;\n+\t}\n+\treturn 0;\n+}\n+\n+static int fetch_refs_via_shim(struct transport *transport,\n+\t\t\t       int nr_heads, const struct ref **to_fetch)\n+{\n+\tstruct child_process *shim;\n+\tconst struct ref *posn;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i, count;\n+\tFILE *file;\n+\n+\tcount = 0;\n+\tfor (i = 0; i < nr_heads; i++) {\n+\t\tposn = to_fetch[i];\n+\t\tif (posn->status & REF_STATUS_UPTODATE)\n+\t\t\tcontinue;\n+\t\tcount++;\n+\t}\n+\n+\tif (count) {\n+\t\tshim = get_shim(transport);\n+\t\tfor (i = 0; i < nr_heads; i++) {\n+\t\t\tposn = to_fetch[i];\n+\t\t\tif (posn->status & REF_STATUS_UPTODATE)\n+\t\t\t\tcontinue;\n+\t\t\twrite(shim->in, \"fetch \", 6);\n+\t\t\twrite(shim->in, sha1_to_hex(posn->old_sha1), 40);\n+\t\t\twrite(shim->in, \" \", 1);\n+\t\t\twrite(shim->in, posn->name, strlen(posn->name));\n+\t\t\twrite(shim->in, \"\\n\", 1);\n+\t\t}\n+\t\tfile = fdopen(shim->out, \"r\");\n+\t\twhile (count) {\n+\t\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n+\t\t\t\texit(128); /* child died, message supplied already */\n+\n+\t\t\tcount--;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static struct ref *get_refs_via_shim(struct transport *transport, int for_push)\n+{\n+\tstruct child_process *shim;\n+\tstruct ref *ret = NULL;\n+\tstruct ref **end = &ret;\n+\tstruct ref *posn;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tFILE *file;\n+\n+\tshim = get_shim(transport);\n+\twrite(shim->in, \"list\\n\", 5);\n+\n+\tfile = fdopen(shim->out, \"r\");\n+\twhile (1) {\n+\t\tchar *eov;\n+\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n+\t\t\texit(128); /* child died, message supplied already */\n+\n+\t\tif (!*buf.buf)\n+\t\t\tbreak;\n+\n+\t\teov = strchr(buf.buf, ' ');\n+\t\tif (!eov)\n+\t\t\tdie(\"Malformed response in ref list: %s\", buf.buf);\n+\t\t*eov = '\\0';\n+\t\t*end = alloc_ref(eov + 1);\n+\t\tif (eov) {\n+\t\t\tif (buf.buf[0] == '@')\n+\t\t\t\t(*end)->symref = xstrdup(buf.buf + 1);\n+\t\t\telse\n+\t\t\t\tget_sha1_hex(buf.buf, (*end)->old_sha1);\n+\t\t}\n+\t\tend = &((*end)->next);\n+\t\tstrbuf_reset(&buf);\n+\t}\n+\tstrbuf_release(&buf);\n+\n+\tfor (posn = ret; posn; posn = posn->next)\n+\t\tresolve_remote_symref(posn, ret);\n+\n+\treturn ret;\n+}\n+\n+void transport_shim_init(struct transport *transport, const char *name)\n+{\n+\tstruct shim_data *data = xmalloc(sizeof(*data));\n+\tdata->shim = NULL;\n+\tdata->name = name;\n+\ttransport->data = data;\n+\ttransport->get_refs_list = get_refs_via_shim;\n+\ttransport->fetch = fetch_refs_via_shim;\n+\ttransport->disconnect = disconnect_shim;\n+}\ndiff --git a/transport.h b/transport.h\nindex 51b5397..01f650d 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -77,4 +77,7 @@ void transport_unlock_pack(struct transport *transport);\n int transport_disconnect(struct transport *transport);\n char *transport_anonymize_url(const char *url);\n \n+/* Transport methods defined outside transport.c */\n+void transport_shim_init(struct transport *transport, const char *name);\n+\n #endif\n-- \n1.6.4.rc2.13.ga9762.dirty\n"},{"id":"118931","messageId":"36ca99e90907280107i7afb7c64tf0c49bdf82e1e7cb@mail.gmail.com","threadId":"20256","inReplyTo":"alpine.LNX.2.00.0907280155390.2147@iabervon.org","subject":"Re: [PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-07-28T08:07:18Z","receivedAt":"2009-07-28T08:07:18Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Jul 28, 2009 at 08:08, Daniel Barkalow<barkalow@iabervon.org> wrote:\n> +static struct child_process *get_shim(struct transport *transport)\n> +{\n> +       struct shim_data *data = transport->data;\n> +       if (!data->shim) {\n> +               struct strbuf buf = STRBUF_INIT;\n> +               struct child_process *shim = xcalloc(1, sizeof(*shim));\n> +               shim->in = -1;\n> +               shim->out = -1;\n> +               shim->err = 0;\n> +               shim->argv = xcalloc(4, sizeof(*shim->argv));\n> +               strbuf_addf(&buf, \"shim-%s\", data->name);\n> +               shim->argv[0] = buf.buf;\n> +               shim->argv[1] = transport->remote->name;\n> +               shim->argv[2] = transport->url;\n> +               shim->git_cmd = 1;\n> +               start_command(shim);\n> +               data->shim = shim;\n> +       }\n> +       return data->shim;\n> +}\n> +\n> +static int disconnect_shim(struct transport *transport)\n> +{\n> +       struct shim_data *data = transport->data;\n> +       if (data->shim) {\n> +               write(data->shim->in, \"\\n\", 1);\n> +               close(data->shim->in);\n> +               finish_command(data->shim);\n> +               free(data->shim->argv);\nDoes this leak data->shim->argv[0] (Ie \"shim-%s\")?\n\nBert\n> +               free(data->shim);\n> +               transport->data = NULL;\n> +       }\n> +       return 0;\n> +}\n"},{"id":"118955","messageId":"alpine.DEB.1.00.0907281444140.8306@pacific.mpi-cbg.de","threadId":"20256","inReplyTo":"alpine.LNX.2.00.0907280155390.2147@iabervon.org","subject":"Re: [PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-28T13:04:44Z","receivedAt":"2009-07-28T13:04:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Jul 2009, Daniel Barkalow wrote:\n\n> transport_get() can call transport_shim_init() to have list and\n> fetch-ref operations handled by running a separate program as:\n\nAs I commented already, \"shim\" is a meaningless word in the context of \nGit.  At _least_ call it something like \"git-remote-<protocol>\".  Even \n\"git-fetch-<protocol>\" would be better than \"shim\".\n\n> diff --git a/Documentation/git-shim.txt b/Documentation/git-shim.txt\n> new file mode 100644\n> index 0000000..dd80c6d\n> --- /dev/null\n> +++ b/Documentation/git-shim.txt\n> @@ -0,0 +1,37 @@\n> +git-shim(1)\n> +============\n> +\n> +NAME\n> +----\n> +git-shim - Helper programs for interoperation with remote git\n\nActually, this is just one helper program, no?  Why can't it be integrated \ninto transport.c?\n\nAt the very least, the text so far is misleading.\n\n> +COMMANDS\n> +--------\n> +\n> +Commands are given by the caller on the helper's standard input, one per line.\n> +\n> +'list'::\n> +\tLists the refs, one per line, if the format \"<value>\n> +\t<name>\". The value is either a hex sha1 hash or \"@<dest>\" for\n> +\tsymrefs. After the complete list, outputs a blank line.\n> +\n> +'fetch' ref::\n> +\tFetches the given ref, writing the necessary objects to the\n> +\tdatabase. Outputs a blank line when the fetch is complete.\n> ++\n\nSo you allow only 'list' and 'fetch'.  That is very limiting, and you do \nnot even foresee a method to ask for the helper's capabilities.  We \nalready saw how much pain that caused in the transport protocol, so please \ndo not repeat the mistake here.\n\n> diff --git a/transport-shim.c b/transport-shim.c\n> new file mode 100644\n> index 0000000..2518aba\n> --- /dev/null\n> +++ b/transport-shim.c\n> @@ -0,0 +1,142 @@\n> +#include \"cache.h\"\n> +#include \"transport.h\"\n> +\n> +#include \"run-command.h\"\n> +#include \"commit.h\"\n> +#include \"diff.h\"\n> +#include \"revision.h\"\n> +\n> +struct shim_data\n> +{\n> +\tconst char *name;\n> +\tstruct child_process *shim;\n> +};\n> +\n> +static struct child_process *get_shim(struct transport *transport)\n> +{\n> +\tstruct shim_data *data = transport->data;\n> +\tif (!data->shim) {\n\nWhy can't the caller check for this?  Would this not make much more sense \nto begin with?\n\n> +static int disconnect_shim(struct transport *transport)\n> +{\n> +\tstruct shim_data *data = transport->data;\n> +\tif (data->shim) {\n> +\t\twrite(data->shim->in, \"\\n\", 1);\n> +\t\tclose(data->shim->in);\n> +\t\tfinish_command(data->shim);\n> +\t\tfree(data->shim->argv);\n> +\t\tfree(data->shim);\n> +\t\ttransport->data = NULL;\n> +\t}\n> +\treturn 0;\n> +}\n\nWhy is this function returning anything?\n\n> +static int fetch_refs_via_shim(struct transport *transport,\n> +\t\t\t       int nr_heads, const struct ref **to_fetch)\n\nDo you fetch only the refs, or also their objects?  If the latter, the \nname needs to be adjusted.\n\n> +{\n> +\tstruct child_process *shim;\n> +\tconst struct ref *posn;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint i, count;\n> +\tFILE *file;\n> +\n> +\tcount = 0;\n> +\tfor (i = 0; i < nr_heads; i++) {\n> +\t\tposn = to_fetch[i];\n> +\t\tif (posn->status & REF_STATUS_UPTODATE)\n> +\t\t\tcontinue;\n> +\t\tcount++;\n> +\t}\n\nThis would be more readable IMO if it read like this:\n\n\tfor (count = i = 0; i < nr_heads; i++)\n\t\tif (!(to_fetch[i]->status & REF_STATUS_UPTODATE))\n\t\t\tcount++;\n\n> +\tif (count) {\n> +\t\tshim = get_shim(transport);\n\nIt would be much better to say \"if (!count) return 0;\" here rather than \nindenting a whole block of code, with no code after that.\n\n> +\t\tfor (i = 0; i < nr_heads; i++) {\n> +\t\t\tposn = to_fetch[i];\n> +\t\t\tif (posn->status & REF_STATUS_UPTODATE)\n> +\t\t\t\tcontinue;\n> +\t\t\twrite(shim->in, \"fetch \", 6);\n> +\t\t\twrite(shim->in, sha1_to_hex(posn->old_sha1), 40);\n> +\t\t\twrite(shim->in, \" \", 1);\n> +\t\t\twrite(shim->in, posn->name, strlen(posn->name));\n> +\t\t\twrite(shim->in, \"\\n\", 1);\n> +\t\t}\n> +\t\tfile = fdopen(shim->out, \"r\");\n> +\t\twhile (count) {\n> +\t\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> +\t\t\t\texit(128); /* child died, message supplied already */\n> +\n> +\t\t\tcount--;\n\nwhile (count--)\n\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static struct ref *get_refs_via_shim(struct transport *transport, int for_push)\n> +{\n> +\tstruct child_process *shim;\n> +\tstruct ref *ret = NULL;\n> +\tstruct ref **end = &ret;\n\nA better name for this is \"tail\", as is used at least in many parts of the \nGit source code already.\n\n> +\tstruct ref *posn;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tFILE *file;\n> +\n> +\tshim = get_shim(transport);\n> +\twrite(shim->in, \"list\\n\", 5);\n\nWhat about the return value of this write()?  It can indicate error or \nshort write.\n\n> +\n> +\tfile = fdopen(shim->out, \"r\");\n\nNo check for file != NULL?\n\n> +\twhile (1) {\n> +\t\tchar *eov;\n> +\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> +\t\t\texit(128); /* child died, message supplied already */\n> +\n> +\t\tif (!*buf.buf)\n> +\t\t\tbreak;\n> +\n> +\t\teov = strchr(buf.buf, ' ');\n> +\t\tif (!eov)\n> +\t\t\tdie(\"Malformed response in ref list: %s\", buf.buf);\n> +\t\t*eov = '\\0';\n> +\t\t*end = alloc_ref(eov + 1);\n> +\t\tif (eov) {\n\nDid we not die 4 lines earlier if eov == NULL?\n\n> +\t\t\tif (buf.buf[0] == '@')\n> +\t\t\t\t(*end)->symref = xstrdup(buf.buf + 1);\n> +\t\t\telse\n> +\t\t\t\tget_sha1_hex(buf.buf, (*end)->old_sha1);\n\nIMHO it is not at all clear what you are doing here.  At least a little \nhint is in order if the code does not explain itself.\n\n> +\t\t}\n> +\t\tend = &((*end)->next);\n> +\t\tstrbuf_reset(&buf);\n> +\t}\n> +\tstrbuf_release(&buf);\n> +\n> +\tfor (posn = ret; posn; posn = posn->next)\n> +\t\tresolve_remote_symref(posn, ret);\n> +\n> +\treturn ret;\n> +}\n\nCiao,\nDscho\n"},{"id":"118967","messageId":"alpine.LNX.2.00.0907281124060.2147@iabervon.org","threadId":"20256","inReplyTo":"36ca99e90907280107i7afb7c64tf0c49bdf82e1e7cb@mail.gmail.com","subject":"Re: [PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-07-28T15:25:57Z","receivedAt":"2009-07-28T15:25:57Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 28 Jul 2009, Bert Wesarg wrote:\n\n> On Tue, Jul 28, 2009 at 08:08, Daniel Barkalow<barkalow@iabervon.org> wrote:\n> > +static struct child_process *get_shim(struct transport *transport)\n> > +{\n> > +       struct shim_data *data = transport->data;\n> > +       if (!data->shim) {\n> > +               struct strbuf buf = STRBUF_INIT;\n> > +               struct child_process *shim = xcalloc(1, sizeof(*shim));\n> > +               shim->in = -1;\n> > +               shim->out = -1;\n> > +               shim->err = 0;\n> > +               shim->argv = xcalloc(4, sizeof(*shim->argv));\n> > +               strbuf_addf(&buf, \"shim-%s\", data->name);\n> > +               shim->argv[0] = buf.buf;\n> > +               shim->argv[1] = transport->remote->name;\n> > +               shim->argv[2] = transport->url;\n> > +               shim->git_cmd = 1;\n> > +               start_command(shim);\n> > +               data->shim = shim;\n> > +       }\n> > +       return data->shim;\n> > +}\n> > +\n> > +static int disconnect_shim(struct transport *transport)\n> > +{\n> > +       struct shim_data *data = transport->data;\n> > +       if (data->shim) {\n> > +               write(data->shim->in, \"\\n\", 1);\n> > +               close(data->shim->in);\n> > +               finish_command(data->shim);\n> > +               free(data->shim->argv);\n> Does this leak data->shim->argv[0] (Ie \"shim-%s\")?\n\nYes, I guess it would. I think it's time to look up how to run tests with \nvalgrind...\n\nThanks.\n\t-Daniel\n*This .sig left intentionally blank*"},{"id":"118974","messageId":"alpine.LNX.2.00.0907281126200.2147@iabervon.org","threadId":"20256","inReplyTo":"alpine.DEB.1.00.0907281444140.8306@pacific.mpi-cbg.de","subject":"Re: [PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-07-28T17:38:48Z","receivedAt":"2009-07-28T17:38:48Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 28 Jul 2009, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Tue, 28 Jul 2009, Daniel Barkalow wrote:\n> \n> > transport_get() can call transport_shim_init() to have list and\n> > fetch-ref operations handled by running a separate program as:\n> \n> As I commented already, \"shim\" is a meaningless word in the context of \n> Git.  At _least_ call it something like \"git-remote-<protocol>\".  Even \n> \"git-fetch-<protocol>\" would be better than \"shim\".\n\nI think git-remote-<protocol> is a good name; I'm not particularly tied to \n\"shim\", although I think this should be able to support push as well as \nfetch (that is, instead of http-push being a separate program that \ntransport.c knows how to run, have that be handled by transport-side code \nthat could support pushing with arbitrary protocols, and have the pushing \ncode built into the curl handler). I do think that I want to form the \ncommand names differently depending on whether this is a helper that \ntransports git objects to a remote git repository using a protocol that \nthe helper is for, or a helper that interacts with a non-git remote \nrepository where the helper is particular to the foreign scm.\n\n> > diff --git a/Documentation/git-shim.txt b/Documentation/git-shim.txt\n> > new file mode 100644\n> > index 0000000..dd80c6d\n> > --- /dev/null\n> > +++ b/Documentation/git-shim.txt\n> > @@ -0,0 +1,37 @@\n> > +git-shim(1)\n> > +============\n> > +\n> > +NAME\n> > +----\n> > +git-shim - Helper programs for interoperation with remote git\n> \n> Actually, this is just one helper program, no?  Why can't it be integrated \n> into transport.c?\n\nNo, this defines a pattern of helper programs, all of which should behave \nas documented by the same man page, so that the same code can run any of \nthem.\n\n> > +COMMANDS\n> > +--------\n> > +\n> > +Commands are given by the caller on the helper's standard input, one per line.\n> > +\n> > +'list'::\n> > +\tLists the refs, one per line, if the format \"<value>\n> > +\t<name>\". The value is either a hex sha1 hash or \"@<dest>\" for\n> > +\tsymrefs. After the complete list, outputs a blank line.\n> > +\n> > +'fetch' ref::\n> > +\tFetches the given ref, writing the necessary objects to the\n> > +\tdatabase. Outputs a blank line when the fetch is complete.\n> > ++\n> \n> So you allow only 'list' and 'fetch'.  That is very limiting, and you do \n> not even foresee a method to ask for the helper's capabilities.  We \n> already saw how much pain that caused in the transport protocol, so please \n> do not repeat the mistake here.\n\nI'll put the \"capabilities\" command back in; I dropped it from this subset \nof the foreign vcs helper protocol because there's not much variation \npossible when the remote repository must be a git repository.\n\n> > diff --git a/transport-shim.c b/transport-shim.c\n> > new file mode 100644\n> > index 0000000..2518aba\n> > --- /dev/null\n> > +++ b/transport-shim.c\n> > @@ -0,0 +1,142 @@\n> > +#include \"cache.h\"\n> > +#include \"transport.h\"\n> > +\n> > +#include \"run-command.h\"\n> > +#include \"commit.h\"\n> > +#include \"diff.h\"\n> > +#include \"revision.h\"\n> > +\n> > +struct shim_data\n> > +{\n> > +\tconst char *name;\n> > +\tstruct child_process *shim;\n> > +};\n> > +\n> > +static struct child_process *get_shim(struct transport *transport)\n> > +{\n> > +\tstruct shim_data *data = transport->data;\n> > +\tif (!data->shim) {\n> \n> Why can't the caller check for this?  Would this not make much more sense \n> to begin with?\n\nDepending on the order that the user of transport.c calls commands, the \nhelper may or may not already be running (and it may or may not have been \nclosed by the previous command, in the future extension of also handling \nimporters which produce a fast-import stream). The caller could check for \nthe connection already being constructed, but all callers would have to \nadd the same check.\n\n> > +static int disconnect_shim(struct transport *transport)\n> > +{\n> > +\tstruct shim_data *data = transport->data;\n> > +\tif (data->shim) {\n> > +\t\twrite(data->shim->in, \"\\n\", 1);\n> > +\t\tclose(data->shim->in);\n> > +\t\tfinish_command(data->shim);\n> > +\t\tfree(data->shim->argv);\n> > +\t\tfree(data->shim);\n> > +\t\ttransport->data = NULL;\n> > +\t}\n> > +\treturn 0;\n> > +}\n> \n> Why is this function returning anything?\n\nGood point; we can't really fail to disconnect, and we've already \ndetermined that things haven't screwed up. It's left over from the pattern \nused by some of the native protocol code, where it may determine that the \noperation didn't work after all when the \"I'm done\" packet fails to \nproduce the \"goodbye\" response.\n\n> > +static int fetch_refs_via_shim(struct transport *transport,\n> > +\t\t\t       int nr_heads, const struct ref **to_fetch)\n> \n> Do you fetch only the refs, or also their objects?  If the latter, the \n> name needs to be adjusted.\n\nThis is inherited from the transport.h naming, which made sense years ago \n(when it contrasted fetching objects by name of ref versus fetching \nobjects by hash); it is fetching objects as specified by struct refs.\n\n> > +{\n> > +\tstruct child_process *shim;\n> > +\tconst struct ref *posn;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tint i, count;\n> > +\tFILE *file;\n> > +\n> > +\tcount = 0;\n> > +\tfor (i = 0; i < nr_heads; i++) {\n> > +\t\tposn = to_fetch[i];\n> > +\t\tif (posn->status & REF_STATUS_UPTODATE)\n> > +\t\t\tcontinue;\n> > +\t\tcount++;\n> > +\t}\n> \n> This would be more readable IMO if it read like this:\n> \n> \tfor (count = i = 0; i < nr_heads; i++)\n> \t\tif (!(to_fetch[i]->status & REF_STATUS_UPTODATE))\n> \t\t\tcount++;\n\nI think it's more readable to match the flow control of the later loop. \nThis is a dry run of the main loop, counting the number of times we \ndon't skip the important part. I think your version is a more readable way \nof counting the number of not-up-to-date items, but it's not nearly so \nreadable a way of calculating how many times we'll reach the interesting \npart of the loop further down.\n\n> > +\tif (count) {\n> > +\t\tshim = get_shim(transport);\n> \n> It would be much better to say \"if (!count) return 0;\" here rather than \n> indenting a whole block of code, with no code after that.\n\nTrue.\n\n> > +\t\tfor (i = 0; i < nr_heads; i++) {\n> > +\t\t\tposn = to_fetch[i];\n> > +\t\t\tif (posn->status & REF_STATUS_UPTODATE)\n> > +\t\t\t\tcontinue;\n> > +\t\t\twrite(shim->in, \"fetch \", 6);\n> > +\t\t\twrite(shim->in, sha1_to_hex(posn->old_sha1), 40);\n> > +\t\t\twrite(shim->in, \" \", 1);\n> > +\t\t\twrite(shim->in, posn->name, strlen(posn->name));\n> > +\t\t\twrite(shim->in, \"\\n\", 1);\n> > +\t\t}\n> > +\t\tfile = fdopen(shim->out, \"r\");\n> > +\t\twhile (count) {\n> > +\t\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> > +\t\t\t\texit(128); /* child died, message supplied already */\n> > +\n> > +\t\t\tcount--;\n> \n> while (count--)\n\nI don't like decrement operators used with values, except for the common \nstring idioms. But I like:\n\n\tfor (; count; count--)\n\t\tif ...\n\n> > +\t\t}\n> > +\t}\n> > +\treturn 0;\n> > +}\n> > +\n> > +static struct ref *get_refs_via_shim(struct transport *transport, int for_push)\n> > +{\n> > +\tstruct child_process *shim;\n> > +\tstruct ref *ret = NULL;\n> > +\tstruct ref **end = &ret;\n> \n> A better name for this is \"tail\", as is used at least in many parts of the \n> Git source code already.\n\nTrue.\n\n> > +\tstruct ref *posn;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tFILE *file;\n> > +\n> > +\tshim = get_shim(transport);\n> > +\twrite(shim->in, \"list\\n\", 5);\n> \n> What about the return value of this write()?  It can indicate error or \n> short write.\n\nYeah, I should use write_in_full(), although we know at this point that \nshim->in is a pipe which has been drained entirely.\n\n> > +\n> > +\tfile = fdopen(shim->out, \"r\");\n> \n> No check for file != NULL?\n\nI don't think this can fail, if setting up the child process didn't fail.\n\n> > +\twhile (1) {\n> > +\t\tchar *eov;\n> > +\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> > +\t\t\texit(128); /* child died, message supplied already */\n> > +\n> > +\t\tif (!*buf.buf)\n> > +\t\t\tbreak;\n> > +\n> > +\t\teov = strchr(buf.buf, ' ');\n> > +\t\tif (!eov)\n> > +\t\t\tdie(\"Malformed response in ref list: %s\", buf.buf);\n> > +\t\t*eov = '\\0';\n> > +\t\t*end = alloc_ref(eov + 1);\n> > +\t\tif (eov) {\n> \n> Did we not die 4 lines earlier if eov == NULL?\n\nYeah, I confused myself while paring down the vcs output parsing code. I \nthink I actually want to future-proof this code to allow the helper to add \nspace-separated flags after the name, and I don't want this test.\n\n> > +\t\t\tif (buf.buf[0] == '@')\n> > +\t\t\t\t(*end)->symref = xstrdup(buf.buf + 1);\n> > +\t\t\telse\n> > +\t\t\t\tget_sha1_hex(buf.buf, (*end)->old_sha1);\n> \n> IMHO it is not at all clear what you are doing here.  At least a little \n> hint is in order if the code does not explain itself.\n\nIt's parsing the output of the \"list\" command, as given in the \ndocumentation. Item starting with a '@' is a symref, otherwise it is a \nsha1.\n\n> > +\t\t}\n> > +\t\tend = &((*end)->next);\n> > +\t\tstrbuf_reset(&buf);\n> > +\t}\n> > +\tstrbuf_release(&buf);\n> > +\n> > +\tfor (posn = ret; posn; posn = posn->next)\n> > +\t\tresolve_remote_symref(posn, ret);\n> > +\n> > +\treturn ret;\n> > +}\n> \n> Ciao,\n> Dscho\n> \n> \n"},{"id":"119104","messageId":"alpine.DEB.1.00.0907292351040.8306@pacific.mpi-cbg.de","threadId":"20256","inReplyTo":"alpine.LNX.2.00.0907281126200.2147@iabervon.org","subject":"Re: [PATCH 1/3 v2] Add support for external programs for handling native fetches","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-29T22:02:25Z","receivedAt":"2009-07-29T22:02:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Jul 2009, Daniel Barkalow wrote:\n\n> On Tue, 28 Jul 2009, Johannes Schindelin wrote:\n> \n> > On Tue, 28 Jul 2009, Daniel Barkalow wrote:\n> > \n> > > transport_get() can call transport_shim_init() to have list and\n> > > fetch-ref operations handled by running a separate program as:\n> > \n> > As I commented already, \"shim\" is a meaningless word in the context of \n> > Git.  At _least_ call it something like \"git-remote-<protocol>\".  Even \n> > \"git-fetch-<protocol>\" would be better than \"shim\".\n> \n> I think git-remote-<protocol> is a good name;\n\nGood.\n\n> I think this should be able to support push as well as fetch\n\nSo you agree that it should be extensible.  Also good.\n\n> I do think that I want to form the command names differently depending \n> on whether this is a helper that transports git objects to a remote git \n> repository using a protocol that the helper is for, or a helper that \n> interacts with a non-git remote repository where the helper is \n> particular to the foreign scm.\n\nI think that is a rather bad decision: you should not _need_ to care on \nthe transport.c side.  If the helper handles fetching and/or pushing: \nfine.  How it accomplishes that: don't care.\n\n> > > diff --git a/Documentation/git-shim.txt b/Documentation/git-shim.txt\n> > > new file mode 100644\n> > > index 0000000..dd80c6d\n> > > --- /dev/null\n> > > +++ b/Documentation/git-shim.txt\n> > > @@ -0,0 +1,37 @@\n> > > +git-shim(1)\n> > > +============\n> > > +\n> > > +NAME\n> > > +----\n> > > +git-shim - Helper programs for interoperation with remote git\n> > \n> > Actually, this is just one helper program, no?  Why can't it be integrated \n> > into transport.c?\n> \n> No, this defines a pattern of helper programs, all of which should \n> behave as documented by the same man page, so that the same code can run \n> any of them.\n\nSee?  That description _is_ misleading.\n\n> > > +COMMANDS\n> > > +--------\n> > > +\n> > > +Commands are given by the caller on the helper's standard input, one per line.\n> > > +\n> > > +'list'::\n> > > +\tLists the refs, one per line, if the format \"<value>\n> > > +\t<name>\". The value is either a hex sha1 hash or \"@<dest>\" for\n> > > +\tsymrefs. After the complete list, outputs a blank line.\n> > > +\n> > > +'fetch' ref::\n> > > +\tFetches the given ref, writing the necessary objects to the\n> > > +\tdatabase. Outputs a blank line when the fetch is complete.\n> > > ++\n> > \n> > So you allow only 'list' and 'fetch'.  That is very limiting, and you do \n> > not even foresee a method to ask for the helper's capabilities.  We \n> > already saw how much pain that caused in the transport protocol, so please \n> > do not repeat the mistake here.\n> \n> I'll put the \"capabilities\" command back in; I dropped it from this \n> subset of the foreign vcs helper protocol because there's not much \n> variation possible when the remote repository must be a git repository.\n\nJust because you do not foresee it now does not mean that it won't happen.\n\nAs I hinted at: it is rather likely that you do not foresee what kind of \ncapability negotiation we'll want in the future.\n\nLet's learn from history.\n\n> > > diff --git a/transport-shim.c b/transport-shim.c\n> > > new file mode 100644\n> > > index 0000000..2518aba\n> > > --- /dev/null\n> > > +++ b/transport-shim.c\n> > > @@ -0,0 +1,142 @@\n> > > +#include \"cache.h\"\n> > > +#include \"transport.h\"\n> > > +\n> > > +#include \"run-command.h\"\n> > > +#include \"commit.h\"\n> > > +#include \"diff.h\"\n> > > +#include \"revision.h\"\n> > > +\n> > > +struct shim_data\n> > > +{\n> > > +\tconst char *name;\n> > > +\tstruct child_process *shim;\n> > > +};\n> > > +\n> > > +static struct child_process *get_shim(struct transport *transport)\n> > > +{\n> > > +\tstruct shim_data *data = transport->data;\n> > > +\tif (!data->shim) {\n> > \n> > Why can't the caller check for this?  Would this not make much more sense \n> > to begin with?\n> \n> Depending on the order that the user of transport.c calls commands, the \n> helper may or may not already be running (and it may or may not have been \n> closed by the previous command, in the future extension of also handling \n> importers which produce a fast-import stream). The caller could check for \n> the connection already being constructed, but all callers would have to \n> add the same check.\n\nFair enough.  That large deeply indented part that makes up the most part \nof the function is very ugly, though.\n\n> > > +static int fetch_refs_via_shim(struct transport *transport,\n> > > +\t\t\t       int nr_heads, const struct ref **to_fetch)\n> > \n> > Do you fetch only the refs, or also their objects?  If the latter, the \n> > name needs to be adjusted.\n> \n> This is inherited from the transport.h naming, which made sense years ago \n> (when it contrasted fetching objects by name of ref versus fetching \n> objects by hash); it is fetching objects as specified by struct refs.\n\nIt is actually not inherited from the naming, as the member you assign \nthis function to does not contain the string \"refs\".\n\n> > > +{\n> > > +\tstruct child_process *shim;\n> > > +\tconst struct ref *posn;\n> > > +\tstruct strbuf buf = STRBUF_INIT;\n> > > +\tint i, count;\n> > > +\tFILE *file;\n> > > +\n> > > +\tcount = 0;\n> > > +\tfor (i = 0; i < nr_heads; i++) {\n> > > +\t\tposn = to_fetch[i];\n> > > +\t\tif (posn->status & REF_STATUS_UPTODATE)\n> > > +\t\t\tcontinue;\n> > > +\t\tcount++;\n> > > +\t}\n> > \n> > This would be more readable IMO if it read like this:\n> > \n> > \tfor (count = i = 0; i < nr_heads; i++)\n> > \t\tif (!(to_fetch[i]->status & REF_STATUS_UPTODATE))\n> > \t\t\tcount++;\n> \n> I think it's more readable to match the flow control of the later loop. \n\nYeah, right.  It was so readable that I bothered to comment.\n \n> > > +\t\tfor (i = 0; i < nr_heads; i++) {\n> > > +\t\t\tposn = to_fetch[i];\n> > > +\t\t\tif (posn->status & REF_STATUS_UPTODATE)\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\twrite(shim->in, \"fetch \", 6);\n> > > +\t\t\twrite(shim->in, sha1_to_hex(posn->old_sha1), 40);\n> > > +\t\t\twrite(shim->in, \" \", 1);\n> > > +\t\t\twrite(shim->in, posn->name, strlen(posn->name));\n> > > +\t\t\twrite(shim->in, \"\\n\", 1);\n> > > +\t\t}\n> > > +\t\tfile = fdopen(shim->out, \"r\");\n> > > +\t\twhile (count) {\n> > > +\t\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> > > +\t\t\t\texit(128); /* child died, message supplied already */\n> > > +\n> > > +\t\t\tcount--;\n> > \n> > while (count--)\n> \n> I don't like decrement operators used with values, except for the common \n> string idioms. But I like:\n> \n> \tfor (; count; count--)\n> \t\tif ...\n\nWhich is even more ugly than your current version.  Just my personal \ntaste, of course.  Oh, and maybe also a little disagreement with the rest \nof Git's source.\n\n> > > +\tstruct ref *posn;\n> > > +\tstruct strbuf buf = STRBUF_INIT;\n> > > +\tFILE *file;\n> > > +\n> > > +\tshim = get_shim(transport);\n> > > +\twrite(shim->in, \"list\\n\", 5);\n> > \n> > What about the return value of this write()?  It can indicate error or \n> > short write.\n> \n> Yeah, I should use write_in_full(), although we know at this point that \n> shim->in is a pipe which has been drained entirely.\n\nI am not at all sure that you can guarantee that at this point.\n\n> > > +\tfile = fdopen(shim->out, \"r\");\n> > \n> > No check for file != NULL?\n> \n> I don't think this can fail, if setting up the child process didn't fail.\n\nUntil somebody reports a segmentation fault because her operating system \nran out of file descriptors.\n\nIn any case, I would rather be safe than sorry.  It is not a good habit to \nget into, calling functions that can return NULL and not checking them \n(even if you try to be super-clever).\n\n> > > +\twhile (1) {\n> > > +\t\tchar *eov;\n> > > +\t\tif (strbuf_getline(&buf, file, '\\n') == EOF)\n> > > +\t\t\texit(128); /* child died, message supplied already */\n> > > +\n> > > +\t\tif (!*buf.buf)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\teov = strchr(buf.buf, ' ');\n> > > +\t\tif (!eov)\n> > > +\t\t\tdie(\"Malformed response in ref list: %s\", buf.buf);\n> > > +\t\t*eov = '\\0';\n> > > +\t\t*end = alloc_ref(eov + 1);\n> > > +\t\tif (eov) {\n> > \n> > Did we not die 4 lines earlier if eov == NULL?\n> \n> Yeah, I confused myself while paring down the vcs output parsing code. I \n> think I actually want to future-proof this code to allow the helper to \n> add space-separated flags after the name, and I don't want this test.\n\nYeah, why not making it hard on the reviewer for the slight possibility \nthat some change will go in in the future, in the exact form you envisaged \nit now?\n\n> > > +\t\t\tif (buf.buf[0] == '@')\n> > > +\t\t\t\t(*end)->symref = xstrdup(buf.buf + 1);\n> > > +\t\t\telse\n> > > +\t\t\t\tget_sha1_hex(buf.buf, (*end)->old_sha1);\n> > \n> > IMHO it is not at all clear what you are doing here.  At least a little \n> > hint is in order if the code does not explain itself.\n> \n> It's parsing the output of the \"list\" command, as given in the \n> documentation. Item starting with a '@' is a symref, otherwise it is a \n> sha1.\n\nThat's nice.\n\nIt would be even nicer if there was a comment in the patch so that future \npuzzled readers do not know that they have to search the mailing list to \nunderstand this code.\n\nCiao,\nDscho\n"}]}