Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-12-03 Thread Stefan Beller
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd0..ce9e42d10 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, 
> char **src_buffer,
> return PACKET_READ_EOF;
> }
>
> +   if (starts_with(buffer, "ERR ")) {
> +   die(_("remote error: %s"), buffer + 4);
> +   }
> +

Handling any ERR line in the pkt reader is okay, as
* we do not pkt-ize pack files and
* we do not have any other parts of the protocol where user data is in
  the first four bytes, which could randomly match this pattern and
* the rest of the pkt-ized part of the protocol never sends
  ERR lines.

Makes sense.


Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-11-29 Thread Junio C Hamano
Masaya Suzuki  writes:

> Yes, I did. And it also didn't end up in a build error. Do I have a
> different build option...?

Passig DEVELOPER=Yes to make turns a bit more warnings on (in this
case, I think it was "unused-variable") and also uses -Werror to
turn warnings into errors.


Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-11-29 Thread Masaya Suzuki
On Thu, Nov 29, 2018 at 3:42 AM Junio C Hamano  wrote:
>
> Masaya Suzuki  writes:
>
> > In the Git pack protocol definition, an error packet may appear only in
> > a certain context. However, servers can face a runtime error (e.g. I/O
> > error) at an arbitrary timing. This patch changes the protocol to allow
> > an error packet to be sent instead of any packet.
> >
> > Following this protocol spec change, the error packet handling code is
> > moved to pkt-line.c.
> >
> > Signed-off-by: Masaya Suzuki 
> > ---
>
> Have you run tests with this applied?  I noticed 5703.9 now has
> stale expectations, but there may be others.

Yes, I did. And it also didn't end up in a build error. Do I have a
different build option...?


Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-11-29 Thread Junio C Hamano
Masaya Suzuki  writes:

> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
>
> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c.
>
> Signed-off-by: Masaya Suzuki 
> ---

Have you run tests with this applied?  I noticed 5703.9 now has
stale expectations, but there may be others.


Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-11-28 Thread Junio C Hamano
Junio C Hamano  writes:

>> diff --git a/connect.c b/connect.c
>> index 24281b608..458906e60 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader 
>> *reader,
>>  die_initial_contact(1);
>>  case PACKET_READ_NORMAL:
>>  len = reader->pktlen;
>> -if (len > 4 && skip_prefix(reader->line, "ERR ", ))
>> -die(_("remote error: %s"), arg);
>>  break;
>>  case PACKET_READ_FLUSH:
>>  state = EXPECTING_DONE;

This breaks build by not removing the decl of arg while removing the
last user of that variable in the function.

 connect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/connect.c b/connect.c
index 458906e60d..4813f005ab 100644
--- a/connect.c
+++ b/connect.c
@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
-   const char *arg;
 
*list = NULL;
 
-- 
2.20.0-rc1-10-g7068cbc4ab



Re: [PATCH] pack-protocol.txt: accept error packets in any context

2018-11-26 Thread Junio C Hamano
Masaya Suzuki  writes:

> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.

Makes perfect sense.

> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c.
>
> Signed-off-by: Masaya Suzuki 
> ---
>  Documentation/technical/pack-protocol.txt | 20 +++-
>  builtin/archive.c |  2 --
>  connect.c |  2 --
>  fetch-pack.c  |  2 --
>  pkt-line.c|  4 
>  5 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt 
> b/Documentation/technical/pack-protocol.txt
> index 6ac774d5f..7a2375a55 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate 
> `PKT-LINE(...)`, unless
>  otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
>  include a LF, but the receiver MUST NOT complain if it is not present.
>  
> +An error packet is a special pkt-line that contains an error string.
> +
> +
> +  error-line =  PKT-LINE("ERR" SP explanation-text)
> +
> +
> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet 
> MAY
> +be sent. Once this packet is sent by a client or a server, the data transfer
> +process defined in this protocol is terminated.
> +
>  Transports
>  --
>  There are three transports over which the packfile protocol is
> @@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
>   "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
>   nc -v example.com 9418
>  
> -If the server refuses the request for some reasons, it could abort
> -gracefully with an error message.
> -
> -
> -  error-line =  PKT-LINE("ERR" SP explanation-text)
> -
> -
>  
>  SSH Transport
>  -
> @@ -398,12 +401,11 @@ from the client).
>  Then the server will start sending its packfile data.
>  
>  
> -  server-response = *ack_multi ack / nak / error-line
> +  server-response = *ack_multi ack / nak
>ack_multi   = PKT-LINE("ACK" SP obj-id ack_status)
>ack_status  = "continue" / "common" / "ready"
>ack = PKT-LINE("ACK" SP obj-id)
>nak = PKT-LINE("NAK")
> -  error-line =  PKT-LINE("ERR" SP explanation-text)
>  
>  
>  A simple clone may look like this (with no 'have' lines):
> diff --git a/builtin/archive.c b/builtin/archive.c
> index d2455237c..5d179bbd1 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,
>   if (strcmp(buf, "ACK")) {
>   if (starts_with(buf, "NACK "))
>   die(_("git archive: NACK %s"), buf + 5);
> - if (starts_with(buf, "ERR "))
> - die(_("remote error: %s"), buf + 4);
>   die(_("git archive: protocol error"));
>   }
>  
> diff --git a/connect.c b/connect.c
> index 24281b608..458906e60 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -306,8 +306,6 @@ struct ref **get_remote_heads(struct packet_reader 
> *reader,
>   die_initial_contact(1);
>   case PACKET_READ_NORMAL:
>   len = reader->pktlen;
> - if (len > 4 && skip_prefix(reader->line, "ERR ", ))
> - die(_("remote error: %s"), arg);
>   break;
>   case PACKET_READ_FLUSH:
>   state = EXPECTING_DONE;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 9691046e6..e66cd7b71 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id 
> *result_oid)
>   return ACK;
>   }
>   }
> - if (skip_prefix(line, "ERR ", ))
> - die(_("remote error: %s"), arg);
>   die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
>  }
>  
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd0..ce9e42d10 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, 
> char **src_buffer,
>   return PACKET_READ_EOF;
>   }
>  
> + if (starts_with(buffer, "ERR ")) {
> + die(_("remote error: %s"), buffer + 4);
> + }
> +
>   if ((options & PACKET_READ_CHOMP_NEWLINE) &&
>   len && buffer[len-1] == '\n')
>   len--;