{"thread":{"id":"9936","subject":"[PATCH 2/5] Refactor struct transport_ops inlined into struct transport","startedAt":"2007-09-19T04:49:31Z","lastAt":"2007-09-20T10:59:48Z","messageCount":4,"participants":["Shawn O. Pearce","Johannes Schindelin","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"53546","messageId":"20070919044931.GB17107@spearce.org","threadId":"9936","inReplyTo":null,"subject":"[PATCH 2/5] Refactor struct transport_ops inlined into struct transport","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-19T04:49:31Z","receivedAt":"2007-09-19T04:49:31Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Aside from reducing the code by 20 lines this refactoring removes\na level of indirection when trying to access the operations of a\ngiven transport \"instance\", making the code clearer and easier to\nfollow.\n\nIt also has the nice effect of giving us the benefits of C99 style\nstruct initialization (namely \".fetch = X\") without requiring that\nlevel of language support from our compiler.  We don't need to worry\nabout new operation methods being added as they will now be NULL'd\nout automatically by the xcalloc() we use to create the new struct\ntransport we supply to the caller.\n\nThis pattern already exists in struct walker, so we already have\na precedent for it in Git.  We also don't really need to worry\nabout any sort of performance decreases that may occur as a result\nof filling out 4-8 op pointers when we make a \"struct transport\".\nThe extra few CPU cycles this requires over filling in the \"struct\ntransport_ops\" is killed by the time it will take Git to actually\n*use* one of those functions, as most transport operations are\ngoing over the wire or will be copying object data locally between\ntwo directories.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-fetch.c |    3 +-\n transport.c     |   62 +++++++++++++++++++++---------------------------------\n transport.h     |   16 ++++---------\n 3 files changed, 30 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 997a8ff..2f639cc 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -392,8 +392,7 @@ static int do_fetch(struct transport *transport,\n \tif (transport->remote->fetch_tags == -1)\n \t\tno_tags = 1;\n \n-\tif (!transport->ops || !transport->ops->get_refs_list ||\n-\t    !transport->ops->fetch)\n+\tif (!transport->get_refs_list || !transport->fetch)\n \t\tdie(\"Don't know how to fetch from %s\", transport->url);\n \n \t/* if not appending, truncate FETCH_HEAD */\ndiff --git a/transport.c b/transport.c\nindex cc76e3f..d8458dc 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)\n \treturn 0;\n }\n \n-static const struct transport_ops rsync_transport;\n-\n static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {\n \tconst char **argv;\n \tint argc;\n@@ -199,14 +197,6 @@ static int fetch_objs_via_curl(struct transport *transport,\n \n #endif\n \n-static const struct transport_ops curl_transport = {\n-\t/* set_option */\tNULL,\n-\t/* get_refs_list */\tget_refs_via_curl,\n-\t/* fetch */\t\tfetch_objs_via_curl,\n-\t/* push */\t\tcurl_transport_push,\n-\t/* disconnect */\tdisconnect_walker\n-};\n-\n struct bundle_transport_data {\n \tint fd;\n \tstruct bundle_header header;\n@@ -249,14 +239,6 @@ static int close_bundle(struct transport *transport)\n \treturn 0;\n }\n \n-static const struct transport_ops bundle_transport = {\n-\t/* set_option */\tNULL,\n-\t/* get_refs_list */\tget_refs_from_bundle,\n-\t/* fetch */\t\tfetch_refs_from_bundle,\n-\t/* push */\t\tNULL,\n-\t/* disconnect */\tclose_bundle\n-};\n-\n struct git_transport_data {\n \tunsigned thin : 1;\n \tunsigned keep : 1;\n@@ -401,13 +383,6 @@ static int git_transport_push(struct transport *transport, int refspec_nr, const\n \treturn !!err;\n }\n \n-static const struct transport_ops git_transport = {\n-\t/* set_option */\tset_git_option,\n-\t/* get_refs_list */\tget_refs_via_connect,\n-\t/* fetch */\t\tfetch_refs_via_pack,\n-\t/* push */\t\tgit_transport_push\n-};\n-\n static int is_local(const char *url)\n {\n \tconst char *colon = strchr(url, ':');\n@@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tret->url = url;\n \n \tif (!prefixcmp(url, \"rsync://\")) {\n-\t\tret->ops = &rsync_transport;\n+\t\t/* not supported; don't populate any ops */\n+\n \t} else if (!prefixcmp(url, \"http://\")\n \t        || !prefixcmp(url, \"https://\")\n \t        || !prefixcmp(url, \"ftp://\")) {\n-\t\tret->ops = &curl_transport;\n+\t\tret->get_refs_list = get_refs_via_curl;\n+\t\tret->fetch = fetch_objs_via_curl;\n+\t\tret->push = curl_transport_push;\n+\t\tret->disconnect = disconnect_walker;\n+\n \t} else if (is_local(url) && is_file(url)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n-\t\tret->ops = &bundle_transport;\n+\t\tret->get_refs_list = get_refs_from_bundle;\n+\t\tret->fetch = fetch_refs_from_bundle;\n+\t\tret->disconnect = close_bundle;\n+\n \t} else {\n \t\tstruct git_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n+\t\tret->set_option = set_git_option;\n+\t\tret->get_refs_list = get_refs_via_connect;\n+\t\tret->fetch = fetch_refs_via_pack;\n+\t\tret->push = git_transport_push;\n+\n \t\tdata->thin = 1;\n \t\tdata->uploadpack = \"git-upload-pack\";\n \t\tif (remote && remote->uploadpack)\n@@ -451,7 +439,6 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tif (remote && remote->receivepack)\n \t\t\tdata->receivepack = remote->receivepack;\n \t\tdata->unpacklimit = -1;\n-\t\tret->ops = &git_transport;\n \t}\n \n \treturn ret;\n@@ -460,24 +447,23 @@ struct transport *transport_get(struct remote *remote, const char *url)\n int transport_set_option(struct transport *transport,\n \t\t\t const char *name, const char *value)\n {\n-\tif (transport->ops->set_option)\n-\t\treturn transport->ops->set_option(transport, name, value);\n+\tif (transport->set_option)\n+\t\treturn transport->set_option(transport, name, value);\n \treturn 1;\n }\n \n int transport_push(struct transport *transport,\n \t\t   int refspec_nr, const char **refspec, int flags)\n {\n-\tif (!transport->ops->push)\n+\tif (!transport->push)\n \t\treturn 1;\n-\treturn transport->ops->push(transport, refspec_nr, refspec, flags);\n+\treturn transport->push(transport, refspec_nr, refspec, flags);\n }\n \n struct ref *transport_get_remote_refs(struct transport *transport)\n {\n \tif (!transport->remote_refs)\n-\t\ttransport->remote_refs =\n-\t\t\ttransport->ops->get_refs_list(transport);\n+\t\ttransport->remote_refs = transport->get_refs_list(transport);\n \treturn transport->remote_refs;\n }\n \n@@ -496,7 +482,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \t\theads[nr_heads++] = rm;\n \t}\n \n-\trc = transport->ops->fetch(transport, nr_heads, heads);\n+\trc = transport->fetch(transport, nr_heads, heads);\n \tfree(heads);\n \treturn rc;\n }\n@@ -513,8 +499,8 @@ void transport_unlock_pack(struct transport *transport)\n int transport_disconnect(struct transport *transport)\n {\n \tint ret = 0;\n-\tif (transport->ops->disconnect)\n-\t\tret = transport->ops->disconnect(transport);\n+\tif (transport->disconnect)\n+\t\tret = transport->disconnect(transport);\n \tfree(transport);\n \treturn ret;\n }\ndiff --git a/transport.h b/transport.h\nindex 6a95d66..3e332ff 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -5,22 +5,11 @@\n #include \"remote.h\"\n \n struct transport {\n-\tunsigned verbose : 1;\n \tstruct remote *remote;\n \tconst char *url;\n-\n \tvoid *data;\n-\n \tstruct ref *remote_refs;\n \n-\tconst struct transport_ops *ops;\n-\tchar *pack_lockfile;\n-};\n-\n-#define TRANSPORT_PUSH_ALL 1\n-#define TRANSPORT_PUSH_FORCE 2\n-\n-struct transport_ops {\n \t/**\n \t * Returns 0 if successful, positive if the option is not\n \t * recognized or is inapplicable, and negative if the option\n@@ -34,8 +23,13 @@ struct transport_ops {\n \tint (*push)(struct transport *connection, int refspec_nr, const char **refspec, int flags);\n \n \tint (*disconnect)(struct transport *connection);\n+\tchar *pack_lockfile;\n+\tunsigned verbose : 1;\n };\n \n+#define TRANSPORT_PUSH_ALL 1\n+#define TRANSPORT_PUSH_FORCE 2\n+\n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n \n-- \n1.5.3.1.195.gadd6\n"},{"id":"53581","messageId":"Pine.LNX.4.64.0709191409590.28395@racer.site","threadId":"9936","inReplyTo":"20070919044931.GB17107@spearce.org","subject":"Re: [PATCH 2/5] Refactor struct transport_ops inlined into struct transport","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-19T13:11:33Z","receivedAt":"2007-09-19T13:11:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 19 Sep 2007, Shawn O. Pearce wrote:\n\n> diff --git a/transport.c b/transport.c\n> index cc76e3f..d8458dc 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)\n>  \treturn 0;\n>  }\n>  \n> -static const struct transport_ops rsync_transport;\n> -\n>  static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {\n>  \tconst char **argv;\n>  \tint argc;\n> @@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)\n>  \tret->url = url;\n>  \n>  \tif (!prefixcmp(url, \"rsync://\")) {\n> -\t\tret->ops = &rsync_transport;\n> +\t\t/* not supported; don't populate any ops */\n> +\n\nThat is sneaky.  What are the reasons to remove rsync support?  I know it \nis deprecated, but I'd still like to have it, especially for initial \nclones on small-RAMed machines.\n\nCiao,\nDscho\n"},{"id":"53617","messageId":"Pine.LNX.4.64.0709192226150.21941@iabervon.org","threadId":"9936","inReplyTo":"Pine.LNX.4.64.0709191409590.28395@racer.site","subject":"Re: [PATCH 2/5] Refactor struct transport_ops inlined into struct transport","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-20T02:33:08Z","receivedAt":"2007-09-20T02:33:08Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 19 Sep 2007, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Wed, 19 Sep 2007, Shawn O. Pearce wrote:\n> \n> > diff --git a/transport.c b/transport.c\n> > index cc76e3f..d8458dc 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)\n> >  \treturn 0;\n> >  }\n> >  \n> > -static const struct transport_ops rsync_transport;\n> > -\n> >  static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {\n> >  \tconst char **argv;\n> >  \tint argc;\n> > @@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> >  \tret->url = url;\n> >  \n> >  \tif (!prefixcmp(url, \"rsync://\")) {\n> > -\t\tret->ops = &rsync_transport;\n> > +\t\t/* not supported; don't populate any ops */\n> > +\n> \n> That is sneaky.  What are the reasons to remove rsync support?  I know it \n> is deprecated, but I'd still like to have it, especially for initial \n> clones on small-RAMed machines.\n\nIt never got implemented in a way called from C. This is just removing the \npointer to the empty struct where support would go. If anybody knows \nenough about interfacing with rsync to write the necessary functions, it \ncan be restored.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"53638","messageId":"Pine.LNX.4.64.0709201159260.28395@racer.site","threadId":"9936","inReplyTo":"Pine.LNX.4.64.0709192226150.21941@iabervon.org","subject":"Re: [PATCH 2/5] Refactor struct transport_ops inlined into struct transport","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-20T10:59:48Z","receivedAt":"2007-09-20T10:59:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 19 Sep 2007, Daniel Barkalow wrote:\n\n> On Wed, 19 Sep 2007, Johannes Schindelin wrote:\n> \n> > On Wed, 19 Sep 2007, Shawn O. Pearce wrote:\n> > \n> > > diff --git a/transport.c b/transport.c\n> > > index cc76e3f..d8458dc 100644\n> > > --- a/transport.c\n> > > +++ b/transport.c\n> > > @@ -44,8 +44,6 @@ static int disconnect_walker(struct transport *transport)\n> > >  \treturn 0;\n> > >  }\n> > >  \n> > > -static const struct transport_ops rsync_transport;\n> > > -\n> > >  static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags) {\n> > >  \tconst char **argv;\n> > >  \tint argc;\n> > > @@ -431,18 +406,31 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> > >  \tret->url = url;\n> > >  \n> > >  \tif (!prefixcmp(url, \"rsync://\")) {\n> > > -\t\tret->ops = &rsync_transport;\n> > > +\t\t/* not supported; don't populate any ops */\n> > > +\n> > \n> > That is sneaky.  What are the reasons to remove rsync support?  I know it \n> > is deprecated, but I'd still like to have it, especially for initial \n> > clones on small-RAMed machines.\n> \n> It never got implemented in a way called from C. This is just removing the \n> pointer to the empty struct where support would go. If anybody knows \n> enough about interfacing with rsync to write the necessary functions, it \n> can be restored.\n\nOkay, will do once I find the time.\n\nCiao,\nDscho\n"}]}