Erik Huelsmann wrote:
> 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.
I admit I am unfamiliar with it and may well have made some wrong assumptions
or bad suggestions or whatever. Sorry for the confusion that that may cause.
I hope you can recognise which parts of my comments are good and which are bad.
I try to make my comments as questions: "Wouldn't it be better if...?" rather
than "You should do so-and-so", and I hope that you (and any reader) will think
about the questions and find the correct answer, and not assume that I am right
because often I am wrong. I'm sure you know this, but at least two committers
recently (not you) have responded to my review questions by saying "OK, I've
done what you wanted" when in fact I didn't say what I wanted - I only asked a
question.
> 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
Yes, I know that. If we decide to keep this stuff private then it so happens
that svn_wc_translated_file2 hasn't been released yet so we don't have to
preserve it. We might want to preserve some version of it for some reason; I
don't mind much about that, I just want us to do the best thing for ongoing
development.
> 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).
OK. I hadn't looked at why the present ...2() version was introduced. If it
is used by many callers (like more than two) then it sounds worth keeping.
> On 11/16/05, Julian Foad <julianfoad@btopenworld.com> wrote:
>
>>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
The same thought may well apply to svn_wc_translated_file(), but that's
history, so never mind, as you say.
> 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).
Well, just to pick one example, given that some existing caller wants to
translate the "svn:special" thing only (so you've made provision for doing
that), perhaps another caller would want to translate EOL style and keywords
only, and not svn:special. Maybe not, but that's an example of the sort of
thing that makes it look like a special-purpose function, not a general-purpose
function.
(Below, you mention that the "only special" flag may not be needed. If so, I
think that's good: the fewer special options the function has, the more
general-purpose and also easier to use it will be.)
>>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.
It's nearly always a good idea to factor out common code. Whether that
factored-out subroutine is generally useful, or only designed to be used by the
existing callers, is a completely different question.
>>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?
Well, this change appears to reduce the amount of duplicated code and
spread-out knowledge in the current libsvn_wc, which is definitely a good
thing. That is why I thought it would be worthwhile to keep the function.
Why don't you like the idea of making a private function? What is the
disadvantage compared with leaving the code as it is? (The advantage is well
understood by both of us.)
>>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.
OK - keep the previous "...2()" version as well if you think it is, in
practice, going to benefit some third-party users.
>>>/** 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,
[...]
>>This doc string needs to be updated (or reverted).
>
> If we decide to do that: ofcourse.
I'm not sure what you are referring to by "do that", so just to be sure you
know what I mean, I mean that the code currently checked in to Subversion trunk
has a bad doc string there: it needs to say much more than that before it can
claim to be "Same as svn_wc_translated_file2".
>>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).
Heh! I'll tell you if you're _very_ patronising. If you're just a little bit
patronising, that's OK, I don't mind: it's natural in an email conversation
because you can't find out how I react to one statement before you say or write
the next one.
>>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.
Ah! Well spotted. Apologies for making you do extra work to explain the truth
to me.
> 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.
OK. Thanks for telling me. I didn't bother to find out how symlinks are
handled, partly because it's not really important for what I was trying to say.
What I was trying to say was: "This doc-string doesn't seem to be precise
enough."
> 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).
Good. That will remove the source of my discomfort.
>> 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 [...]
Yes, I knew what FORCE_COPY does, but I don't mind you explaining it just in
case I had misunderstood it.
What I didn't know (and believe I know now, from your explanation) is that a
symlink translation can never be a no-op, so this "FORCE_COPY" flag is
irrelevant in that case. That's why the description of "only special" didn't
seem to need to mention whether it obeys FORCE_COPY - because it's irrelevant.
However, if there is even a remote possibility that there may be other kinds
of "special" files than symlinks, then maybe the "FORCE_COPY" flag would be
relevant for them, or if there are other flags, I want the doc string to tell
me whether (or which) other flags given in the flags field will be obeyed when
the "ONLY_SPECIAL" flag is set.
>> 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
It does not say one precise thing. The problem is with the English language.
Use of the word "only" is often ambiguous:
"I only drink black coffee."
* "The only coffee I drink is black coffee" ?
* "The only thing I drink is black coffee" ?
Both meanings are equally likely, gramattically. Only the context of a
conversation allows the listener to decide which is meant.
"Only do translation associated with ..."
* "Do no translations except translations associated with ..." ?
* "Do nothing at all except translations associated with ..." ?
In an API doc string, we don't want this ambiguity.
> associated with the svn:special flag. But, as pointed out above all
> that may be moot.
Yes, it will be nice if that goes away.
>>>>+ /** 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.
I'll admit I didn't notice that, but I don't think it is relevant. I don't
consider the output of the function to be a "temporary" file in that sense,
because the function doesn't know what the caller intends to do with the file
and its doc string doesn't mention anything about the result being temporary.
>> 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).
I wasn't disputing that we need a copy. I meant change "merge" so that it
doesn't require the copy generated by this function to be in a particular
directory.
One last thing: I see that SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP is supplied
on every call except one. (I don't count the implementation of the deprecated
svn_wc_translated_file() because that could easily be implemented differently.)
I think this suggests at least that the option should be inverted, so that
deleting is the normal behaviour, and the one caller that wants to KEEP the TMP
has to specify something like "SVN_WC_TRANSLATE_KEEP_TMP_ON_POOL_CLEANUP".
It would also be worth investigating why that single caller (the one in
subversion/libsvn_wc/update_editor.c) is different from the others, and if it
doesn't really need this behaviour or could easily obtain this behaviour by
other means then it would be better to remove the option from the interface.
Also, for naming and describing this option, as I alluded to above, I don't
like the output file being referred to as a "temporary" file. It may well be
temporary in many cases, but that's not how it is basically described by the
doc string.
> 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.
That's fine: your reply was fine, and I agree the basic idea is good. We seem
to be communicating well, or as well as can be expected given how little I know
about how the code works :-)
Thanks for your patience and politeness.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 17 01:12:43 2005