Hi Julian,
Thanks for the review below. I can't help feeling some of your
comments come from infamiliarity with the working copy code, but I'll
address your points.
Anyway, to start, I'd like to point out that I didn't add
svn_wc_translated_file2 in this commit, so if you mean this commit
should be (partially) reverted, then that doesn't mean
svn_wc_translated_file2 should go. I added translated_file2 as an
enhanced version of the non-2 version to allow the use of our new
remove-temp-file-on-pool-cleanup code. I still think that should stay,
as it greatly simplifies some code which used to manually track errors
(instead of using SVN_ERR).
On 11/16/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> 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.
Ok :-) I changed the order of you e-mail to make the big thought come
at the beginning.
> 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.
Hmm. Isn't that what svn_wc_translated_file does too? Never mind that
though. What would expect callers of this function to want to do more
than it does now? They can choose the direction to translate in,
translate efficiently (ie. don't when there's no need) or to guarantee
a new file creation even if translation would not require it (ie,
there would be no translation).
> 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.
Sure. There never even was a (good) reason to provide
svn_wc_translated_file anyway, because all callers could call
svn_subst_copy_and_translate directly anyway. But, I think it's a good
idea not to keep certain routines which provide certain oft-used
behaviours for ourselves.
> Eventually we could make
> the function public if and when it becomes stable and generally useful and
> seems worthwhile.
I'd prefer reverting and re-introducing later than making private and
maybe-sometime-making-public. When would that 'sometime' be?
> 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.
Well, I addressed this above. It was there before this commit in - I
believe - a usefull form.
> > /** 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).
If we decide to do that: ofcourse.
> 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.
Thanks for taking the time to do so. I'll do the same and be verbose
below. If I sound petronising, please tell me so (I don't mean to be).
> 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?).
Here, I think to see your knowledge (or lack thereof) showing.
Symlinks are versioned since 1.1. How does that work? Well, we
introduced a property (svn:special) to signal something odd was up
with the versioned target. The contents of the NF in case of a symlink
is one line: 'link <path to which the symlink points>'. Detranslating
a special file, means creating the symlink somewhere and make sure to
point it to the argument read from the NF.
Detranslating means exactly the oposite: read the symlink to see where
it points and write a NF file with the contents as explained above.
The whole point of creating this function though is that the author of
the software using the function doesn't need to know all this. He just
needs to know if he wants the NF or WC versions of the file and which
version he knows the path to. The routine will do the rest.
In line with the thought above, I think I should kill the SPECIAL_ONLY
flag, since other translations aren't relevant to this kind of file. I
only realised this when I was at work today (not in a position to
write this mail at the time).
> 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?
FORCE_COPY is there to disable an optimization svn_wc_translated_file
does: return the original file path if no translation is applicable.
Sometimes (quite often - measured in code lines) however we need a
copy of the file, be it a translated or non-translated one. This
happens when translating a text base to get a file for installation in
the working copy: we can't just move the text base into the wc if no
translation is required; we need a second file to be in the wc (or
we'd be left with no text base).
> 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"?
Ok. It means what it says: Do no other translations than the one
associated with the svn:special flag. But, as pointed out above all
that may be moot.
> 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?
I think: Remove the option, as it only saves 2 calls to get properties.
> >> + /** 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.
Yes. And in libsvn_wc/log.c.
> However, it isn't sufficient for
> the purpose: what that merge wanted was a copy in a particular place.
Yes, but log.c doesn't. Also, what I did here was introduce the
function. There are a lot of uses of svn_subst_copy_and_translate3 in
libsvn_wc (see http://hix.nu/subversion/lxr/ident?i=svn_subst_copy_and_translate3).
I'll be checking each of them and most certainly replacing them by
calls to this new function. I expect to need the flag on a number of
occasions.
> (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.
The flag itself not, but I believe the docstring for
svn_wc_tranlated_file2 does:
"
* Temporary files are created in the temp file area belonging to
* @a versioned_file.
"
> 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.)
I thought about that: xlated_path could point to a valid path when a
certain flag is specified I didn't do that for 3 reasons: 1) merge.c
is the only user of that functionality (atm); 2) I couldn't decide on
the name of the flag; 3) it has an ugly amount of overlap with the
current FORCE_COPY, which doesn't require a pathname as input (which I
expect to use a lot).
> * 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.)
The reason you'd want a copy is somewhere higher up in this lengthy
reply, but: There are many occasions you want to have a file which you
want to (atomically) rename into place later. Oftentimes you can't do
that with the original, either because it's already used as text base,
or if we're talking about adding files, because it's already used as
working copy file (and we're creating a text base).
> > @@ -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.)
Point taken. You're absolutely right.
> 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?
Some eol-styles need their EOLs repaired upon conversion. We use this
parameter and always set it to true for those styles. I introduced a
function elsewhere which has this knowledge coded in. So, in no other
places in the code it should (have to) be hard coded when to repair
EOLs.
But here, too, you are right. svn_wc_translated_file should keep
offering this option. I'll have to re-introduce it. Although I hope
-but doubt- it's used as it should.
> 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.
If you read notes/line-endings-and-keywords.txt, *we* don't offer any
user repair on fixed line endings; nor do we ever offer repair on
broken files with 'native' line endings. That's exactly the algorithm
I coded into the 'what arguments to pass to
svn_subst_copy_and_translate' function svn_subst_eol_style_normalize.
Hope to have been a bit clearer than before.
bye,
Erik.
PS: I may have sounded a bit defensive, but don't intend to. Please
know I value the input and just try to find out how this commit could
have been acceptable to you; because I believe that the basic idea is
still valuable.
Received on Wed Nov 16 22:03:36 2005