On Mon, Aug 9, 2010 at 12:11 PM, Hyrum K. Wright <hyrum_wri...@mail.utexas.edu> wrote: > On Mon, Aug 9, 2010 at 11:51 AM, Stefan Küng <tortoise...@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.f...@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)
