Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 10:09:45PM -0700, Junio C Hamano wrote:
Stefan Beller sbel...@google.com writes:
On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano gits...@pobox.com wrote:
if (...-version 2) {
... append -%d ...
}
involved.
Oh! I see here you would count the current one as 1, which has no
number extension, and any further would have a -${version}. That
would transport the intention much better I guess.
Yeah, except that I screwed up my comparison. Obviously, I should
have said If version is 2 or later, then append -%d to the name,
otherwise use the name as-is.
FWIW, I had similar head-scratching over Stefan's original. I think it's
OK to say version 1 is magical, and for historical reasons does not
have its number appended. There's no need for us ever to make
upload-pack-1; it just introduces more headaches.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 03:01:12PM -0700, Stefan Beller wrote:
Signed-off-by: Stefan Beller sbel...@google.com
---
transport.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/transport.c b/transport.c
index 3ef15f6..33644a6 100644
--- a/transport.c
+++ b/transport.c
@@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options
*opts,
static int connect_setup(struct transport *transport, int for_push, int
verbose)
{
struct git_transport_data *data = transport-data;
+ const char *remote_program;
+ char *buf = 0;
Use NULL when you mean a NULL pointer (they're equivalent to the
compiler, but the word is easier to read).
I agree on Eric's naming this to_free (and I consider it idiomatic to
assign them in a chain, like foo = to_free = xmalloc(...), but we
don't always do that).
+ if (transport-smart_options
+ transport-smart_options-transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, %s-%d, remote_program,
+ transport-smart_options-transport_version);
+ remote_program = buf;
+ }
Using xstrfmt can help you avoid magic numbers and repetition,
like:
to_free = xstrfmt(%s-%d,
remote_program,
transport-smart_options-transport_version);
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller sbel...@google.com wrote:
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/transport.c b/transport.c
index 3ef15f6..33644a6 100644
--- a/transport.c
+++ b/transport.c
@@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options
*opts,
static int connect_setup(struct transport *transport, int for_push, int
verbose)
{
struct git_transport_data *data = transport-data;
+ const char *remote_program;
+ char *buf = 0;
Naming this 'to_free' would make its purpose more obvious[1], and be
more consistent with code elsewhere in the project.
[1]: http://article.gmane.org/gmane.comp.version-control.git/256125/
if (data-conn)
return 0;
+ remote_program = (for_push ? data-options.receivepack
+ : data-options.uploadpack);
+
+ if (transport-smart_options
+transport-smart_options-transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, %s-%d, remote_program,
+ transport-smart_options-transport_version);
+ remote_program = buf;
+ }
+
data-conn = git_connect(data-fd, transport-url,
-for_push ? data-options.receivepack :
-data-options.uploadpack,
+remote_program,
verbose ? CONNECT_VERBOSE : 0);
+ free(buf);
+
return 0;
}
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Stefan Beller sbel...@google.com writes:
On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano gits...@pobox.com wrote:
if (...-version 2) {
... append -%d ...
}
involved.
Oh! I see here you would count the current one as 1, which has no
number extension, and any further would have a -${version}. That
would transport the intention much better I guess.
Yeah, except that I screwed up my comparison. Obviously, I should
have said If version is 2 or later, then append -%d to the name,
otherwise use the name as-is.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
+ if (transport-smart_options
+ transport-smart_options-transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, %s-%d, remote_program,
+ transport-smart_options-transport_version);
+ remote_program = buf;
+ }
Bikeshedding: so the versioning scheme is that the current one is
zero, and the next one is two? I would have expected that there
would be something like
I think currently we have version 1. But we don't advertise it, so
I'll call it 0
(the default or unadvertised or however you name it. 0 as in unsure maybe)
I thought about future proofing this version a bit. Say version 2 is bad because
I don't have the experience of 10 years Git nor of maintaining large
projects and
you want to make a version 3 soon. And this would support that just fine.
The meaning being: Any version except 0 should have a dedicated
extension -${version}
The 0 is left out for backwards compatibility.
So in a later patch where we want to introduce force-using of old
versions you could
configure upload-pack to be explicit upload-pack-1. The upload-pack-1
version is not
yet there with this series though.
if (...-version 2) {
... append -%d ...
}
involved.
Oh! I see here you would count the current one as 1, which has no
number extension,
and any further would have a -${version}. That would transport the
intention much better
I guess.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Signed-off-by: Stefan Beller sbel...@google.com
---
transport.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/transport.c b/transport.c
index 3ef15f6..33644a6 100644
--- a/transport.c
+++ b/transport.c
@@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options
*opts,
static int connect_setup(struct transport *transport, int for_push, int
verbose)
{
struct git_transport_data *data = transport-data;
+ const char *remote_program;
+ char *buf = 0;
if (data-conn)
return 0;
+ remote_program = (for_push ? data-options.receivepack
+ : data-options.uploadpack);
+
+ if (transport-smart_options
+transport-smart_options-transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, %s-%d, remote_program,
+ transport-smart_options-transport_version);
+ remote_program = buf;
+ }
+
data-conn = git_connect(data-fd, transport-url,
-for_push ? data-options.receivepack :
-data-options.uploadpack,
+remote_program,
verbose ? CONNECT_VERBOSE : 0);
+ free(buf);
+
return 0;
}
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Stefan Beller sbel...@google.com writes:
+ if (transport-smart_options
+ transport-smart_options-transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, %s-%d, remote_program,
+ transport-smart_options-transport_version);
+ remote_program = buf;
+ }
Bikeshedding: so the versioning scheme is that the current one is
zero, and the next one is two? I would have expected that there
would be something like
if (...-version 2) {
... append -%d ...
}
involved.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
