Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Jonathan Tan <jonathantanmy@google.com>]

On 01/26/2017 02:33 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
>> new file mode 100644
>> index 000000000..060ec0300
>> --- /dev/null
>> +++ b/t/lib-httpd/one-time-sed.sh
>> @@ -0,0 +1,8 @@
>> +#!/bin/sh
>> +
>> +if [ -e one-time-sed ]; then
>> +	"$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
>> +	rm one-time-sed
>> +else
>> +	"$GIT_EXEC_PATH/git-http-backend"
>> +fi
>
> CodingGuidelines?

Thanks - done locally and will send out in the next reroll.

>> +inconsistency() {
>> +	# Simulate that the server initially reports $2 as the ref
>> +	# corresponding to $1, and after that, $1 as the ref corresponding to
>> +	# $1. This corresponds to the real-life situation where the server's
>> +	# repository appears to change during negotiation, for example, when
>> +	# different servers in a load-balancing arrangement serve (stateless)
>> +	# RPCs during a single negotiation.
>> +	printf "s/%s/%s/" \
>> +	       $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
>> +	       $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
>> +	       >"$HTTPD_ROOT_PATH/one-time-sed"
>
> I'd prefer for the printf'd result to have a final LF (i.e. not
> leaving the resulting one-time-sed with a final incomplete line).
> Also, do you really need the pipe to tr-d?  Doesn't the result of
> $(command substitution) omit the final LF anyway?
>
>     $ printf '1 %s 2 %s 3\n' "$(echo foo)" "$(echo bar)"; echo OK
>     1 foo 2 bar 3
>     OK

Done.

>> diff --git a/upload-pack.c b/upload-pack.c
>> index b88ed8e26..0678c53d6 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -862,9 +862,13 @@ static void receive_needs(struct string_list *wanted_ns_refs)
>>  		} else if (skip_prefix(line, "want ", &arg) &&
>>  			   !get_sha1_hex(arg, sha1_buf)) {
>>  			o = parse_object(sha1_buf);
>> -			if (!o)
>> +			if (!o) {
>> +				packet_write_fmt(1,
>> +						 "ERR upload-pack: not our ref %s",
>> +						 sha1_to_hex(sha1_buf));
>>  				die("git upload-pack: not our ref %s",
>>  				    sha1_to_hex(sha1_buf));
>> +			}
>
> This somehow looks like a good thing to do even in production.  Am I
> mistaken?

Yes, that's true. If this patch set stalls (for whatever reason), I'll 
spin this off into an independent patch.