On Mon, 2010-08-09, Stefan Küng 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.
Yes, that's what I meant. By saying "a shared copy" I just meant that
the caller must not modify or delete (or get an exclusive read lock,
etc.) the file, because it *may* be shared.
> Maybe with a flag to keep/remove the file on pool cleanup.
My point is that this "give me a reference" API *must* specify the
lifetime of the reference, otherwise the library implementation doesn't
know when it can delete the temp file. The lifetime can be until pool
clean-up, or until something else, but it must be specified and it must
be finite. Otherwise the temp dir will just grow and grow.
> >> - 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.
Yes, exactly.
> 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.
No, svn_wc_translated_file2() already creates a *file* and returns the
path to that file. Maybe you're thinking of svn_wc_translated_stream()
which also exists and was also deprecated recently.
> * 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
More precisely: "if SVN_WC_TRANSLATE_FORCE_COPY is not specified and no
translation is required ...".
> 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.
More precisely: "... ignored unless it creates a temp copy."
> 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.
Then it's the caller's responsibility to own and delete the file if the
output flag says so. That implies that every call must generate a new,
independent copy of the file. That is not optimal, but is a possible
solution.
- Julian
Received on 2010-08-12 15:26:15 CEST