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
> ---
> 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 10:09:45PM -0700, Junio C Hamano wrote:
> Stefan Beller writes:
>
> > On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano 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
Stefan Beller writes:
> On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano 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 6:01 PM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> 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
On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:
> Stefan Beller 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
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Stefan Beller 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
