[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r907414 - /subversion/trunk/subversion/libsvn_wc/questions.c

From: Greg Stein <gstein_at_gmail.com>
Date: Mon, 8 Feb 2010 15:30:41 -0500

Sounds silly. I'd suggest removing that, and doc'ing and explicit
closure. *Any* function that reads a stream to EOF should just close
the darned stream. (I adjusted some svn_io.h functions to do this) ...
it just doesn't make sense to leave the stream open since you can't
really do anything with a stream sitting at EOF. (sure, a few streams
are (now) seekable, but fine... disown those before calling a
close-on-EOF function)

Cheers,
-g

On Mon, Feb 8, 2010 at 15:22, Bert Huijben <bert_at_qqmail.nl> wrote:
> The called function does an explicit disown so i assumed that behavior was by design.
>
>  Bert
>
> ----- Oorspronkelijk bericht -----
> Van: Greg Stein <gstein_at_gmail.com>
> Verzonden: maandag 8 februari 2010 20:58
> Aan: dev_at_subversion.apache.org
> Onderwerp: Re: svn commit: r907414 - /subversion/trunk/subversion/libsvn_wc/questions.c
>
> On Sun, Feb 7, 2010 at 06:50,  <rhuijben_at_apache.org> wrote:
>>...
>> +++ subversion/trunk/subversion/libsvn_wc/questions.c Sun Feb  7 11:50:50 2010
>> @@ -367,12 +367,15 @@
>>       return SVN_NO_ERROR;
>>     }
>>
>> +  SVN_ERR(err);
>> +
>>   /* Check all bytes, and verify checksum if requested. */
>> -  SVN_ERR(compare_and_verify(modified_p, db, local_abspath, pristine_stream,
>> -                             compare_textbases, force_comparison,
>> -                             scratch_pool));
>> +  err = compare_and_verify(modified_p, db, local_abspath, pristine_stream,
>> +                           compare_textbases, force_comparison,
>> +                           scratch_pool);
>>
>> -  return SVN_NO_ERROR;
>> +  return svn_error_compose_create(err,
>> +                                  svn_stream_close(pristine_stream));
>>  }
>
> It would be better to have compare_and_verify() close the stream after
> it has done its work. Invariably, a stream on a file will not be used
> once it hits EOF, so you may as well close it instead of waiting for
> pool-cleanup.
>
> compare_and_verify() is a local function, so it should be easy enough
> to look at all uses, and ensure that closing the stream is acceptable.
> If a (semi)public function passes one of its arguments into
> compare_and_verify(), then you can use svn_stream_disown() before
> passing that stream into compare_and_verify().
>
> If a function that does that is semi-public, then you could even
> examine all of its callers, and adjust the docstrings accordingly, to
> talk about stream closure.
>
> Cheers,
> -g
>
>
Received on 2010-02-08 21:31:16 CET

This is an archived mail posted to the Subversion Dev mailing list.