Hi Erik. Thanks for your earlier reply to some of the points I made. I see
you've now committed a version of this in r17362. I'm afraid it needs a bit
more work. Please don't get stuck on my first paragraph below before you have
read through to the Big Thought near the end.
Julian Foad wrote:
> Erik Huelsmann wrote:
>
>> Index: subversion/include/svn_wc.h
>> ===================================================================
[...]
>> + /** Only do translation associated with the svn:special property only */
>> +#define SVN_WC_TRANSLATE_SPECIAL_ONLY 0x00000002
I'll repeat and expand some of what I asked before. It's not clear to me what
this means. OK, I think I can guess what it does to translate a symlink from
NF: it would give a path to (possibly a copy of) the symlink destination file
(or directory?). What does it do in reverse: can it translate a file or dir to
a symlink? What does it do to a non-special file? Does it still obey
"FORCE_COPY" for such a file? You see, "Only do translation associated
with..." is still ambiguous. (Partly, it's not clear whether "Translation"
refers to the general idea of converting a file to a different form, or to the
specific set of EOL, keyword and "special" conversions.) Does it mean "Do the
translation associated..., and nothing else, so no obeying of any (or some) of
the other options." Or does it mean "Do everything except translating keywords
and EOL style"?
It would be nice to have orthogonal options as far as possible. This one
sounds like a candidate for a separate function altogether because it does not
seem to be orthogonal to the other options and actions Or at least it isn't
yet documented in a way that makes clear that it is orthogonal. What do you think?
>> + /** Guarantee a new file is created on successful return.
>> + * The default shortcuts translation by returning the path
>> + * of the untranslated file when no translation is required.
>> + */
>> +#define SVN_WC_TRANSLATE_FORCE_COPY 0x00000008
This is used in subversion/libsvn_wc/merge.c. However, it isn't sufficient for
the purpose: what that merge wanted was a copy in a particular place.
(Actually, not an explicitly specified place, but the place chosen by a
particular function.) This flag doesn't say anything about where the new file
will be. We need to do one of:
* Allow the caller to specify where to put it. (We don't want yet another path
argument, but maybe the semantics could be changed a bit so that one of the
existing arguments optionally specifies the desired location.)
* Document that "This creates a file where the function ...() decides". (No; I
don't think that would be a good API design.)
* Change the "merge" caller so that it doesn't have this requirement. (This
would probably be best, and it might make this option redundant since I can't
imagine any other reason for insisting on a copy.)
> @@ -99,8 +111,10 @@
> svn_boolean_t force_repair,
> apr_pool_t *pool)
> {
> - return svn_wc_translated_file2 (xlated_p, vfile, adm_access,
> - force_repair, FALSE, pool);
> + /* we discard the force_repair flag,
> + because we know what to do ourselves now... but is it acceptable?! */
> + return svn_wc_translated_file2 (xlated_p, vfile, vfile, adm_access,
> + SVN_WC_TRANSLATE_TO_NF, pool);
> }
Everybody: Please don't put "Is this right?" comments in the code where they
might go unnoticed for months; instead, ask the question on the mailing list.
(If the list can't answer, or decides that it's too difficult and not important
enough to fix immediately, mark the comment that indicates a problem with "###"
so that we can easily find all such comments.)
In this case - this is a public API; I don't see how it can be right to discard
the flag. What does "we know what to do ourselves" mean?
I believe the purpose of the flag was (when not set) to allow inconsistent EOL
style in a working file to be detected so the user can be alerted to the
problem, and (when set) to convert all EOLs to normal form so that a textual
diff can ignore any line ending changes, for convenience.
Perhaps the latter ability ("repairing" for the convenience of "diff") should
be delegated to the "diff" code, but it should also be supported by this
deprecated API.
A Big Thought
Does this new function really want to be public (yet)? It seems like (your
version of) this function is designed to do exactly what the existing callers
in libsvn_wc want, and not for general use. I think it is a bit rash to make
such a thing part of our public API immediately.
Can we make a private one first? Then we would have plenty of freedom to
change it, have it incompletely documented, and so on. This might well avoid
the need to fix some of the problems mentioned above. Eventually we could make
the function public if and when it becomes stable and generally useful and
seems worthwhile.
I would suggest leaving the existing svn_wc_translated_file() in place in the
public API (not deprecated), and removing svn_wc_translated_file2() from the
public API.
> /** Same as svn_wc_translated_file2, but will never clean up
> * temporary files.
> *
> * @deprecated Provided for compatibility with the 1.3 API
> */
> svn_error_t *svn_wc_translated_file (const char **xlated_p,
> const char *vfile,
> svn_wc_adm_access_t *adm_access,
> svn_boolean_t force_repair,
> apr_pool_t *pool);
This doc string needs to be updated (or reverted).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 16 13:42:51 2005