Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Wed, May 27, 2015 at 01:45:55PM -0700, Stefan Beller wrote: > > Right, but I think (and please correct me if there's a case I'm > > missing) that the behavior is the same whether it is spelled > > "ping-pong" or "capability:ping-pong". That is, the rule for > > "capability:" is "if you do not understand it, ignore it and do not > > mention it in your capabilities; the server is required to assume > > you were written before that capability was invented". But that is > > _also_ the rule for ping-pong, I think. > > The rules are the same, right. But the allowed characters are limited > (in theory) as the regular expressions given for the capabilities > don't allow for binary data for example, but only well formed ASCII > text, space separated. The "ping-pong" keyword could introduce a > binary stream there including line feeds. (Today it sounds like a > stupid idea though) Do we need that restriction? IOW, as long as we parse from the start of the packet and give up as soon as we realize it is not a thing we understand, I do not think anybody is hurt by the contents of the item. E.g., if an old client sees: 00XXping-pong= It knows: 1. The item starts with ping-pong; we don't know what that is, so we never even bother looking at the binary goo. 2. It's in a pkt-line. We read to the end of the packet line and throw the rest of the data away. Now we're synchronized to read the next item. Of course, "ping-pong" may also introduce another phase to the protocol which is not encapsulated in pkt-lines (e.g., if the data is too big to fit right inside the capability pkt-line, or if it needs a lot of back and forth like ref negotiation). But then we only proceed to that phase if both sides have said "I understand ping-pong". So I think we are capable of boot-strapping just about anything using capability negotiation (with the exception of fixing the capability negotiation itself; but even that, we can bootstrap a second more intensive capability negotiation using a capability in the initial list). -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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Wed, May 27, 2015 at 1:34 PM, Jeff King wrote: > On Wed, May 27, 2015 at 10:40:37AM -0700, Stefan Beller wrote: > >> > If we are upload-pack-2, should we advertise that in the capabilities? I >> > think it may make things easier later if we try to provide some >> > opportunistic out-of-band data. E.g., if see tell git-daemon: >> > >> > git-upload-pack repo\0host=whatever\0\0version=2 >> > >> > how do we know whether version=2 was understood and kicked us into v2 >> > mode, versus an old server that ignored it? >> >> So in my vision we would call git-upload-pack-2 instead of having a >> "version=2". >> and if git-upload-pack-2 doesn't exist, the whole conversation is >> over, the client >> it is left to make up some good error message or retry version 1. > > I'd like for that to be a starting point for us, and then to be able to > add-on "hints" to ease the transition path in whatever way we want. We > may even not do that in the long run, but I want to leave the door open > if we can. > >> But I think advertising both which versions the server could deal >> with, as well as >> the currently expected version is a good thing. >> >> capability: can_speak=1,2 >> capability: speaking_now=2 > > I was thinking just "speaking_now=2", but it probably makes sense to do > both. I do not think using it to "downgrade" will ever be particularly > useful (certainly not from v2 to v1, since to understand the flag both > sides must be v2 in the first place). But advertising that via the v1 > conversation will be a good way to tell the other side that upgrade is > possible. If for some reason we discover a flaw in the current version, which makes it unusable (a buffer overflow?, some stupid abuse which makes the capability list huge), you may want to force downgrading (and in the very distant future when we are current on version 4 and have dropped version 1 already, you can only downgrade to 2 and 3, so I can see value in it. Another idea to make it all more future proof: "capability: speaking_now=2" must be sent as the first line, so then you can adapt on the client side easily for which version you are listening. > >> > Also, do we need the capability noise-word? >> >> I thought it opens up a new possible door in the future. >> As we ignore anything not starting with "capability" for now, you >> could introduce >> your foo and bar ping pong easily and still be version 2 compatible. >> >> S: capability: thin >> S: capability: another-capability >> S: ping-pong foo >> S: done >> C: (not having understood ping-pong) just answering with capability: thin >> C: done, let's proceed to refs advertisement >> >> The alternative client would do: >> >> C: ping-pong: foo-data1a >> S: ping-pong: foo-data1b >> C: ping-pong: foo-data2a >> C: capability: thin >> ... > > Right, but I think (and please correct me if there's a case I'm missing) > that the behavior is the same whether it is spelled "ping-pong" or > "capability:ping-pong". That is, the rule for "capability:" is "if you > do not understand it, ignore it and do not mention it in your > capabilities; the server is required to assume you were written before > that capability was invented". But that is _also_ the rule for > ping-pong, I think. The rules are the same, right. But the allowed characters are limited (in theory) as the regular expressions given for the capabilities don't allow for binary data for example, but only well formed ASCII text, space separated. The "ping-pong" keyword could introduce a binary stream there including line feeds. (Today it sounds like a stupid idea though) > >> > Eric mentioned the underflow problems here, but I wonder even more: >> > what's wrong with the global ends_with() that we already provide? >> >> I was missing knowledge we have that, and apparently I was thinking it's >> faster to come up with my own version than to look for it. :) > > Makes sense. :) > > -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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Wed, May 27, 2015 at 10:40:37AM -0700, Stefan Beller wrote: > > If we are upload-pack-2, should we advertise that in the capabilities? I > > think it may make things easier later if we try to provide some > > opportunistic out-of-band data. E.g., if see tell git-daemon: > > > > git-upload-pack repo\0host=whatever\0\0version=2 > > > > how do we know whether version=2 was understood and kicked us into v2 > > mode, versus an old server that ignored it? > > So in my vision we would call git-upload-pack-2 instead of having a > "version=2". > and if git-upload-pack-2 doesn't exist, the whole conversation is > over, the client > it is left to make up some good error message or retry version 1. I'd like for that to be a starting point for us, and then to be able to add-on "hints" to ease the transition path in whatever way we want. We may even not do that in the long run, but I want to leave the door open if we can. > But I think advertising both which versions the server could deal > with, as well as > the currently expected version is a good thing. > > capability: can_speak=1,2 > capability: speaking_now=2 I was thinking just "speaking_now=2", but it probably makes sense to do both. I do not think using it to "downgrade" will ever be particularly useful (certainly not from v2 to v1, since to understand the flag both sides must be v2 in the first place). But advertising that via the v1 conversation will be a good way to tell the other side that upgrade is possible. > > Also, do we need the capability noise-word? > > I thought it opens up a new possible door in the future. > As we ignore anything not starting with "capability" for now, you > could introduce > your foo and bar ping pong easily and still be version 2 compatible. > > S: capability: thin > S: capability: another-capability > S: ping-pong foo > S: done > C: (not having understood ping-pong) just answering with capability: thin > C: done, let's proceed to refs advertisement > > The alternative client would do: > > C: ping-pong: foo-data1a > S: ping-pong: foo-data1b > C: ping-pong: foo-data2a > C: capability: thin > ... Right, but I think (and please correct me if there's a case I'm missing) that the behavior is the same whether it is spelled "ping-pong" or "capability:ping-pong". That is, the rule for "capability:" is "if you do not understand it, ignore it and do not mention it in your capabilities; the server is required to assume you were written before that capability was invented". But that is _also_ the rule for ping-pong, I think. > > Eric mentioned the underflow problems here, but I wonder even more: > > what's wrong with the global ends_with() that we already provide? > > I was missing knowledge we have that, and apparently I was thinking it's > faster to come up with my own version than to look for it. :) Makes sense. :) -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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Wed, May 27, 2015 at 01:30:28PM -0400, Eric Sunshine wrote:
> > Like Eric, I find the whole next_capability thing a little ugly. His
> > suggestion to pass in the parsing state is an improvement, but I wonder
> > why we need to parse at all. Can we keep the capabilities as:
> >
> > const char *capabilities[] = {
> > "multi_ack",
> > "thin-pack",
> > ... etc ...
> > };
> >
> > and then loop over the array?
>
> Yes, that would be much nicer. I also had this in mind but didn't know
> if there was a strong reason for the capabilities to be shoehorned
> into a single string as they are currently.
I don't think there is a good reason, beyond it being the simplest thing
for the current code to work. But as you can see from the existing
packet_write() in upload-pack, it's already going through some
contortions to handle optional capabilities (i.e., "capabilities" is by
no means the full list anymore).
Doing it item by item will mean we can't use a single packet_write() in
the v1 code, but it's OK to format into a buffer here (we already need
such a buffer for format_symref_info anyway).
-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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Tue, May 26, 2015 at 11:35 PM, Jeff King wrote:
> On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:
>
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf,
>> struct string_list *symref)
>> strbuf_addf(buf, " symref=%s:%s", item->string, (char
>> *)item->util);
>> }
>>
>> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
>> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
>> " side-band-64k ofs-delta shallow no-progress"
>> " include-tag multi_ack_detailed";
>
> If we are upload-pack-2, should we advertise that in the capabilities? I
> think it may make things easier later if we try to provide some
> opportunistic out-of-band data. E.g., if see tell git-daemon:
>
> git-upload-pack repo\0host=whatever\0\0version=2
>
> how do we know whether version=2 was understood and kicked us into v2
> mode, versus an old server that ignored it?
So in my vision we would call git-upload-pack-2 instead of having a "version=2".
and if git-upload-pack-2 doesn't exist, the whole conversation is
over, the client
it is left to make up some good error message or retry version 1.
But I think advertising both which versions the server could deal
with, as well as
the currently expected version is a good thing.
capability: can_speak=1,2
capability: speaking_now=2
>
> It also just makes the protocol more self-describing; from the
> conversation you can see which version is in use.
>
>> +static void send_capabilities(void)
>> +{
>> + char buf[100];
>> +
>> + while (next_capability(buf))
>> + packet_write(1, "capability:%s\n", buf);
>
> Having a static-sized buffer and then passing it to a function which has
> no idea of the buffer size seems like a recipe for a buffer overflow. :)
Yes. All patches I proposed are very brittle. My first goal was to have the
last test passing (an actual fetch with version 2). Now I will start looking at
making things robust, as by now you all seem to not disagree totally.
>
> You are fine here because you are parsing the hard-coded capabilities
> string, and we know 100 is enough there. But it's nice to avoid such
> magic.
>
> Like Eric, I find the whole next_capability thing a little ugly. His
> suggestion to pass in the parsing state is an improvement, but I wonder
> why we need to parse at all. Can we keep the capabilities as:
>
> const char *capabilities[] = {
> "multi_ack",
> "thin-pack",
> ... etc ...
> };
>
> and then loop over the array? The v1 writer will need to be modified,
> but it should be fairly straightforward (loop and add them to the buffer
> rather than dumping the whole string).
Thanks for the design guidance! I'll change that.
>
> Also, do we need the capability noise-word?
I thought it opens up a new possible door in the future.
As we ignore anything not starting with "capability" for now, you
could introduce
your foo and bar ping pong easily and still be version 2 compatible.
S: capability: thin
S: capability: another-capability
S: ping-pong foo
S: done
C: (not having understood ping-pong) just answering with capability: thin
C: done, let's proceed to refs advertisement
The alternative client would do:
C: ping-pong: foo-data1a
S: ping-pong: foo-data1b
C: ping-pong: foo-data2a
C: capability: thin
...
> They all have it, except
> for:
>
>> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
>
> But isn't that basically a capability, too (I agree it is stretching the
> definition of "capability", but I think that ship has sailed; the
> client's response is not "I support this, too" but "I want to enable
> this").
>
> IOW, is there a reason that the initial conversation is not just:
>
> pkt-line("multi_ack");
> pkt-line("thin-pack");
> ...
> pkt-line("agent=v2.5.0");
> pkt-line();
>
> We already have framing for each capability due to the use of pkt-line.
> The "capability:" is just one more thing the client has to parse past.
> The keys are already unique up to any "=", so I don't think there is any
> ambiguity (e.g., we don't care about "capability:agent"; we have already
> assigned a meaning to the term "agent", and will never introduce a
> standalone capability with the same name).
It looks clearer to me (we're not wasting band-width), maybe this ping pong
thing can be part of the capabilities announcement too, so we're good this way.
>
> Likewise, if we introduce new protocol items here, the client should
> either ignore them (if it does not understand them) or know what to do
> with them.
>
>> +static void receive_capabilities(void)
>> +{
>> + int done = 0;
>> + while (1) {
>> + char *line = packet_read_line(0, NULL);
>> + if (!line)
>> + break;
>> + if (starts_with(line, "capability:"))
>> + parse_features(line + strlen("capabilit
Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Wed, May 27, 2015 at 2:35 AM, Jeff King wrote:
> On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:
>> +static void send_capabilities(void)
>> +{
>> + char buf[100];
>> +
>> + while (next_capability(buf))
>> + packet_write(1, "capability:%s\n", buf);
>
> Like Eric, I find the whole next_capability thing a little ugly. His
> suggestion to pass in the parsing state is an improvement, but I wonder
> why we need to parse at all. Can we keep the capabilities as:
>
> const char *capabilities[] = {
> "multi_ack",
> "thin-pack",
> ... etc ...
> };
>
> and then loop over the array?
Yes, that would be much nicer. I also had this in mind but didn't know
if there was a strong reason for the capabilities to be shoehorned
into a single string as they are currently.
--
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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf,
> struct string_list *symref)
> strbuf_addf(buf, " symref=%s:%s", item->string, (char
> *)item->util);
> }
>
> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
> " side-band-64k ofs-delta shallow no-progress"
> " include-tag multi_ack_detailed";
If we are upload-pack-2, should we advertise that in the capabilities? I
think it may make things easier later if we try to provide some
opportunistic out-of-band data. E.g., if see tell git-daemon:
git-upload-pack repo\0host=whatever\0\0version=2
how do we know whether version=2 was understood and kicked us into v2
mode, versus an old server that ignored it?
It also just makes the protocol more self-describing; from the
conversation you can see which version is in use.
> +static void send_capabilities(void)
> +{
> + char buf[100];
> +
> + while (next_capability(buf))
> + packet_write(1, "capability:%s\n", buf);
Having a static-sized buffer and then passing it to a function which has
no idea of the buffer size seems like a recipe for a buffer overflow. :)
You are fine here because you are parsing the hard-coded capabilities
string, and we know 100 is enough there. But it's nice to avoid such
magic.
Like Eric, I find the whole next_capability thing a little ugly. His
suggestion to pass in the parsing state is an improvement, but I wonder
why we need to parse at all. Can we keep the capabilities as:
const char *capabilities[] = {
"multi_ack",
"thin-pack",
... etc ...
};
and then loop over the array? The v1 writer will need to be modified,
but it should be fairly straightforward (loop and add them to the buffer
rather than dumping the whole string).
Also, do we need the capability noise-word? They all have it, except
for:
> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
But isn't that basically a capability, too (I agree it is stretching the
definition of "capability", but I think that ship has sailed; the
client's response is not "I support this, too" but "I want to enable
this").
IOW, is there a reason that the initial conversation is not just:
pkt-line("multi_ack");
pkt-line("thin-pack");
...
pkt-line("agent=v2.5.0");
pkt-line();
We already have framing for each capability due to the use of pkt-line.
The "capability:" is just one more thing the client has to parse past.
The keys are already unique up to any "=", so I don't think there is any
ambiguity (e.g., we don't care about "capability:agent"; we have already
assigned a meaning to the term "agent", and will never introduce a
standalone capability with the same name).
Likewise, if we introduce new protocol items here, the client should
either ignore them (if it does not understand them) or know what to do
with them.
> +static void receive_capabilities(void)
> +{
> + int done = 0;
> + while (1) {
> + char *line = packet_read_line(0, NULL);
> + if (!line)
> + break;
> + if (starts_with(line, "capability:"))
> + parse_features(line + strlen("capability:"));
> + }
Use:
const char *cap;
if (skip_prefix(line, "capability:", &cap))
parse_features(cap);
Or better yet, if you take my suggestion above:
parse_features(line);
:)
> +static int endswith(const char *teststring, const char *ending)
> +{
> + int slen = strlen(teststring);
> + int elen = strlen(ending);
> + return !strcmp(teststring + slen - elen, ending);
> +}
Eric mentioned the underflow problems here, but I wonder even more:
what's wrong with the global ends_with() that we already provide?
-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 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> In upload-pack-2 we send each capability in its own packet.
> By reusing the advertise_capabilities and eventually setting it to
> NULL we will be able to reuse the methods for refs advertisement.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/upload-pack-2.c b/upload-pack-2.c
> new file mode 12
> index 000..e30a871
> --- /dev/null
> +++ b/upload-pack-2.c
> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf,
> struct string_list *symref)
> strbuf_addf(buf, " symref=%s:%s", item->string, (char
> *)item->util);
> }
>
> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
> " side-band-64k ofs-delta shallow no-progress"
> " include-tag multi_ack_detailed";
>
> +/*
> + * Reads the next capability and puts it into dst as a null terminated
> string.
> + * Returns true if more capabilities can be read.
> + * */
> +static int next_capability(char *dst)
> +{
> + int len = 0;
> + if (!*advertise_capabilities) {
> + /* make sure to not advertise capabilities afterwards */
> + advertise_capabilities = NULL;
You set advertise_capabilities to NULL here but then unconditionally
dereference that NULL in the if-statement just above (if someone calls
next_capability() again). Seems fishy (and crashy) to me. Either don't
dereference the NULL or don't bother setting it to NULL (in which
case, you'll dereference and find '\0', which should serve the same
purpose).
> + return 0;
> + }
> +
> + while (advertise_capabilities[len] != '\0' &&
> + advertise_capabilities[len] != ' ')
> + len ++;
Style: len++
> + strncpy(dst, advertise_capabilities, len);
> + dst[len] = '\0';
> +
> + advertise_capabilities += len;
> + if (*advertise_capabilities == ' ')
> + advertise_capabilities++;
If for some reason, someone happens to add an extra space between
capabilities in advertise_capabilities, then the capability returned
by the next invocation of next_capability() be zero-length, which is
probably undesirable, and certainly not expected by the caller.
Skipping whitespace in a loop would be more robust.
> + return 1;
> +}
I realize that this is RFC, but my overall reaction to
next_capability() in its current form is that it's ugly . A somewhat
cleaner approach would be to pass some state into next_capability()
and have it modify that state rather than the global
advertise_capabilities. The passed-in state could be as simple as a
'const char *' which initially points at the start of
advertise_capabilities and then gets advanced; until, finally, it
points at the '\0' at the end of advertise_capabilities.
> +static void send_capabilities(void)
> +{
> + char buf[100];
> +
> + while (next_capability(buf))
> + packet_write(1, "capability:%s\n", buf);
> +
> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
> + packet_flush(1);
> +}
> +
> static int send_ref(const char *refname, const unsigned char *sha1, int
> flag, void *cb_data)
> {
>
> @@ -794,6 +831,28 @@ static void upload_pack(void)
> }
> }
>
> +static void receive_capabilities(void)
> +{
> + int done = 0;
> + while (1) {
> + char *line = packet_read_line(0, NULL);
If you declare this 'const char *', then it becomes much more obvious
that it's not your responsibility to free() the result (and,
tangentially, that you have no intention of modifying the content).
> + if (!line)
> + break;
> + if (starts_with(line, "capability:"))
> + parse_features(line + strlen("capability:"));
See skip_prefix().
> + }
> +}
> +
> +static void upload_pack_version_2(void)
> +{
> + send_capabilities();
> + receive_capabilities();
> +
> + /* The rest of the protocol stays the same, capabilities advertising
> + is disabled though. */
/*
* This is a multi-line
* comment.
*/
> + upload_pack();
> +}
> +
> static int upload_pack_config(const char *var, const char *value, void
> *unused)
> {
> if (!strcmp("uploadpack.allowtipsha1inwant", var))
> @@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const
> char *value, void *unused)
> return parse_hide_refs_config(var, value, "uploadpack");
> }
>
> +static int endswith(const char *teststring, const char *ending)
> +{
> + int slen = strlen(teststring);
> + int elen = strlen(ending);
You may be confident today that no callers are supplying an 'ending'
which is longer than 'teststring', but someone may some day break that
assumption. At the very least, for robustness, add an assert() or
die() if 'elen' is greater than 'slen'.
[RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
In upload-pack-2 we send each capability in its own packet.
By reusing the advertise_capabilities and eventually setting it to
NULL we will be able to reuse the methods for refs advertisement.
Signed-off-by: Stefan Beller
---
.gitignore | 1 +
Makefile| 2 ++
upload-pack-2.c | 1 +
upload-pack.c | 77 ++---
4 files changed, 78 insertions(+), 3 deletions(-)
create mode 12 upload-pack-2.c
diff --git a/.gitignore b/.gitignore
index a052419..a3c8ab9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -165,6 +165,7 @@
/git-update-server-info
/git-upload-archive
/git-upload-pack
+/git-upload-pack-2
/git-var
/git-verify-commit
/git-verify-pack
diff --git a/Makefile b/Makefile
index 25a453b..0f3ee41 100644
--- a/Makefile
+++ b/Makefile
@@ -560,6 +560,7 @@ PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += shell.o
PROGRAM_OBJS += show-index.o
PROGRAM_OBJS += upload-pack.o
+PROGRAM_OBJS += upload-pack-2.o
PROGRAM_OBJS += remote-testsvn.o
# Binary suffix, set to .exe for Windows builds
@@ -625,6 +626,7 @@ OTHER_PROGRAMS = git$X
# what test wrappers are needed and 'install' will install, in bindir
BINDIR_PROGRAMS_NEED_X += git
BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-upload-pack-2
BINDIR_PROGRAMS_NEED_X += git-receive-pack
BINDIR_PROGRAMS_NEED_X += git-upload-archive
BINDIR_PROGRAMS_NEED_X += git-shell
diff --git a/upload-pack-2.c b/upload-pack-2.c
new file mode 12
index 000..e30a871
--- /dev/null
+++ b/upload-pack-2.c
@@ -0,0 +1 @@
+upload-pack.c
\ No newline at end of file
diff --git a/upload-pack.c b/upload-pack.c
index a5f75b7..84f9ee3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct
string_list *symref)
strbuf_addf(buf, " symref=%s:%s", item->string, (char
*)item->util);
}
-static const char *advertise_capabilities = "multi_ack thin-pack side-band"
+static char *advertise_capabilities = "multi_ack thin-pack side-band"
" side-band-64k ofs-delta shallow no-progress"
" include-tag multi_ack_detailed";
+/*
+ * Reads the next capability and puts it into dst as a null terminated string.
+ * Returns true if more capabilities can be read.
+ * */
+static int next_capability(char *dst)
+{
+ int len = 0;
+ if (!*advertise_capabilities) {
+ /* make sure to not advertise capabilities afterwards */
+ advertise_capabilities = NULL;
+ return 0;
+ }
+
+ while (advertise_capabilities[len] != '\0' &&
+ advertise_capabilities[len] != ' ')
+ len ++;
+ strncpy(dst, advertise_capabilities, len);
+ dst[len] = '\0';
+
+ advertise_capabilities += len;
+ if (*advertise_capabilities == ' ')
+ advertise_capabilities++;
+
+ return 1;
+}
+
+static void send_capabilities(void)
+{
+ char buf[100];
+
+ while (next_capability(buf))
+ packet_write(1, "capability:%s\n", buf);
+
+ packet_write(1, "agent:%s\n", git_user_agent_sanitized());
+ packet_flush(1);
+}
+
static int send_ref(const char *refname, const unsigned char *sha1, int flag,
void *cb_data)
{
@@ -794,6 +831,28 @@ static void upload_pack(void)
}
}
+static void receive_capabilities(void)
+{
+ int done = 0;
+ while (1) {
+ char *line = packet_read_line(0, NULL);
+ if (!line)
+ break;
+ if (starts_with(line, "capability:"))
+ parse_features(line + strlen("capability:"));
+ }
+}
+
+static void upload_pack_version_2(void)
+{
+ send_capabilities();
+ receive_capabilities();
+
+ /* The rest of the protocol stays the same, capabilities advertising
+ is disabled though. */
+ upload_pack();
+}
+
static int upload_pack_config(const char *var, const char *value, void *unused)
{
if (!strcmp("uploadpack.allowtipsha1inwant", var))
@@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char
*value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
}
+static int endswith(const char *teststring, const char *ending)
+{
+ int slen = strlen(teststring);
+ int elen = strlen(ending);
+ return !strcmp(teststring + slen - elen, ending);
+}
+
int main(int argc, char **argv)
{
char *dir;
+ const char *cmd;
int i;
int strict = 0;
git_setup_gettext();
packet_trace_identity("upload-pack");
- git_extract_argv0_path(argv[0]);
+ cmd = git_extract_argv0_path(argv[0]);
check_replace_refs = 0;
for (i = 1; i < argc; i++) {
@@ -857,6 +924,10 @@ int main(int argc, char **argv)
die("'%s' does not appear to be a git repository", dir);
git_config(upload_pack_confi
