Re: [PATCH] t5562: do not reuse output files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 24, 2018, 07:47 UTC
- Message-ID
- <xmqq7eh23ojc.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqbm6f2ajn.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Max Kirillov <max@max630.net> writes:
>
>> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
>> index 90d890d02f..bb53f82c0c 100755
>> --- a/t/t5562-http-backend-content-length.sh
>> +++ b/t/t5562-http-backend-content-length.sh
>> @@ -25,6 +25,8 @@ test_http_env() {
>> handler_type="$1"
>> request_body="$2"
>> shift
>> + (rm -f act.out || true) &&
>> + (rm -f act.err || true) &&
>
> Why "||true"? If the named file doesn't exist, "rm -f" would
> succeed, and if it does exist but somehow we fail to remove, then
> these added lines are not preveting the next part from reusing,
> i.e. they are not doing what they are supposed to be doing,...Another thing. The analysis in your log message talks about a stray process holding open filehandles to these files. An attempt to remove them in such a stat would fail on some systems, so "||true" would not help, would it? It just hides the failure to remove, and when ||true is useful in hiding th failure _is_ when such a stray process is still there, waiting to corrupt the output of the next request and breaking the test, no?
I do agree that forcing the parent to wait, like you described in the comment, would be far more preferrable, but until that happens, a better workaround might be to write into unique output filenames (act1.out, act2.out, etc.); that way, you do not have to worry about the output file for the next request getting clobbered by a stale process handling the previous request. But at the same time, wouldn't this suggest that the test or the previous request may see an incomplete output, as the analysed problem is that such a process is writing to the output file very late, while we are preparing to test the enxt request, which meas we have already checked the output file for the previous request, right? So even without ||true, I am not sure how true a "solution" this change is to the issue.