On Mon, Aug 9, 2010 at 12:11 PM, Hyrum K. Wright
<hyrum_wright_at_mail.utexas.edu> wrote:
> On Mon, Aug 9, 2010 at 11:51 AM, Stefan Küng <tortoisesvn_at_gmail.com> wrote:
>> On 09.08.2010 16:44, Hyrum K. Wright wrote:
>>>
>>> On Mon, Aug 9, 2010 at 7:46 AM, Julian Foad<julian.foad_at_wandisco.com>
>>> wrote:
>>>>
>>>> On Mon, 2010-08-09, Julian Foad wrote:
>>>>>
>>>>> On Mon, 2010-08-09, Bert Huijben wrote:
>>>>>>
>>>>>> Stefan Küng wrote:
>>>>>>>
>>>>>>> This leads me to another request: would it be possible to
>>>>>>> 'un-deprecate'
>>>>>>> the svn_wc_get_pristine_copy_path() API?
>>>>>
>>>>>> One of the ideas of WC-NG (which will most likely not be part of 1.7,
>>>>>> but
>>>>>> can be added later without really updating our current pristine api)
>>>>>> was to
>>>>>> allow compressed and/or (partially) absent pristine storage.
>>>>>>
>>>>>> This specific api function requires that the file is available in the
>>>>>> pristine store as uncompressed file or that we store a copy of the file
>>>>>> in a
>>>>>> tempfolder (see the documentation update from Julian in r983593).
>>>>>
>>>>> Ah, yes - I'd forgotton that. When the pristine file already exists in
>>>>> plain text, it's not a big problem to return its path even though we
>>>>> can't promise how long it will live. But if pristine texts are stored
>>>>> compressed and so we have to create a temp file in order to support that
>>>>> API, when would we delete the temp file?
>>>>
>>>> A good answer could be: let the caller of
>>>> svn_wc_get_pristine_copy_path() specify one of:
>>>>
>>>> - Give me a new copy of the file that I can keep indefinitely and
>>>> delete when I wish.
>>>>
>>>> - Give me a reference to a shared copy of the file; keep it available
>>>> until pool clean-up.
>>
>> Or just 'give me a reference to a file': if it's already available, just
>> return the path to that file. If it's not available yet, get a copy of that
>> file (uncompressed or fetched from a repository if it's not available
>> locally at all) and return the path to that temp file.
>> Maybe with a flag to keep/remove the file on pool cleanup.
>>
>>>>
>>>> - Give me a reference to a shared copy of the file; keep it available
>>>> until I call ... some new "unreference it" API?
>>>>
>>>> Would the second or third of those options work well, Stefan?
>>>>
>>>> We'll need to consider this within Subversion as well. If we start
>>>> supporting compressed bases, we might want to cache non-compressed
>>>> copies of some of them for faster access.
>>>>
>>>> A caller of svn_wc_translated_file2() has some control over its output
>>>> file lifetime with a default behaviour of delete-on-pool-cleanup and a
>>>> SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP flag to avoid that. However that
>>>> only applies when it's making a new copy of the file. The behaviour
>>>> when it returns a reference to the shared file is undefined. I've
>>>> updated the doc string in r983593 to say so. Thus this function also
>>>> needs attention.
>>>
>>> The whole point of the pristine store is that the location and
>>> encoding of pristine contents should be transparent to anybody above
>>> libsvn_wc. While making libsvn_wc much easier to maintain and extend,
>>> going that route apparently decreases performance for clients such as
>>> TortoiseSVN by taking away some of the shortcuts they were exploiting.
>>> We need to decide if exposing these shortcuts (and adding the
>>> associated code complexity) is worth it.
>>>
>>> In the potential scenario where a user has opted for no pristines or
>>> compressed pristines, how would TortoiseSVN maintain its current
>>> functionality without waiting on decompression or network roundtrips?
>>
>> I think users will understand if they decide to have their BASE compressed
>> or not stored locally, that such diffs will take longer.
>>
>> If I may suggest how the API(s) should look and work:
>>
>> * rename svn_wc_translated_file2() to svn_wc_translated_filestream() since
>> it doesn't return a file but a file stream. File streams can only be used by
>> svn clients directly, they can't be passed on to external applications so
>> their use is limited.
>> * new API svn_wc_translated_file3():
>> svn_client_translated_file(const char ** resultfile,
>> svn_boolean_t * copyCreated,
>> const char * wcfile,
>> svn_boolean_t deleteOnPoolCleanup,
>> apr_uint32_t flags,
>> apr_pool_t* pool)
>>
>> the flags would be the same as now: SVN_WC_TRANSLATE_FORCE_EOL_REPAIR and
>> SVN_WC_TRANSLATE_FORCE_COPY.
>> The API would work like this:
>> if SVN_WC_TRANSLATE_FORCE_COPY and SVN_WC_TRANSLATE_FORCE_EOL_REPAIR is not
>> specified and the BASE file is available locally and uncompressed, just
>> return the path to that file.
>> Else create a temp copy of the BASE file and return that path.
>> deleteOnPoolCleanup of course would get ignored if the file is available
>> locally uncompressed and both SVN_WC_TRANSLATE_FORCE_COPY and
>> SVN_WC_TRANSLATE_FORCE_EOL_REPAIR are not specified.
>>
>> The bool copyCreated would get set to true by the API if it created a copy
>> and set to false if it just returned the path to the existing BASE file.
>>
>> That way, I could get the file very fast if it's possible, but still would
>> get the file in a slow way if it's not possible.
>
> Whatever we do, I'm -1 on any solution that uses bitflags.
Daniel suggests some context here. We've been burned in the past by
APIs which use bitflags (see svn_wc__entry_modify()). Not that there
is any guarantee of badness, but bitflags tend to add obfuscation and
control flow difficulties to a function. And it's just plain hard to
read.
I realize that booleans are functionally equivalent, and may take a
bit more typing, but I'd suggest we use them wherever possible,
especially in out public API.
-Hyrum (who offers the following for clarification of his feelings,
not with the intent to start a religious war)
Received on 2010-08-09 19:55:22 CEST