Re: [PATCH v8 3/3] http-backend: respect CONTENT_LENGTH for receive-pack

From: Max Kirillov <max@max630.net>
Date: Jul 25, 2018, 14:51 UTC
Message-ID: <20180725145100.GA1959@jessie.local>
In-Reply-To: <20180725121435.20519-1-szeder.dev@gmail.com>

On Wed, Jul 25, 2018 at 02:14:35PM +0200, SZEDER Gábor wrote:

>> +	# sometimes there is fatal error buit the result is still 200
> 
> s/buit/but/

Thanks, will fix

Show 7 quoted lines

>> +	if grep 'fatal:' act.err
>> +	then
>> +		return 1
>> +	fi
> 
> I just happened to stumble upon a failure because of 'fatal: the
> remote end hung up unexpectedly' in the test 'push plain'.

Did it happen once or repeated? It is rather strange, that one shoud not fail. Which OS it was?

There have been doubds that a random incoming signal can trigger such a failure.

> What does that "sometimes" in the above comment mean, and how often
> does such a failure happen?  I see these patches are in 'pu' for over
> a month now, so based on the number of reflog entries since then it
> happened once from about 30-35 builds on Travis CI so far.

"sometimes" here means "for some kinds of fatal error failure", there is nothing random in it.

Show 12 quoted lines

> > +		"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl fetch_body git http-backend >act.out 2>act.err &&
> 
> Don't save the standard error of the whole shell function.
> When running the test with /bin/sh and '-x' tracing, then the trace of
> commands executed in the function will be included in the standard
> error as well, which may interfere with later verification (though in
> this case it doesn't seem like it would cause any issues).
> 
> Please limit the redirections to the relevant command's output.  AFAICT
> all invocations of 'test_http_env' in these tests have their stdout and
> stderr redirected to the same pair of files, so perhaps you could
> simply move all these redirections inside the function.

Thanks, I'll try to fix it

Show 5 quoted lines

>> +	! verify_http_result "200 OK"
> 
> ... this function would return error (because of that 'if grep fatal:
> ...' statement) without even looking at the status, but the test would
> still succeed.  Is that really the desired behavior here?

Yes, it is a desired behavior. A failure is expected here, and the failure does not show up as non-200 status, as described above.