On 11/15/05, Malcolm Rowe <malcolm@farside.org.uk> wrote:
> Couple of comments on the documentation changes (only), inline:
>
> On Tue, Nov 15, 2005 at 01:36:57PM -0600, dionisos@tigris.org wrote:
> > Author: dionisos
> > Date: Tue Nov 15 13:36:57 2005
> > New Revision: 17362
> >
>
> > * subversion/libsvn_wc/update_editor.c
> > Update callers to svn_wc_translated_file and
> > svn_wc_translated_file2, removing superseeded static functions.
> "superseded"
>
> > --- trunk/subversion/include/svn_wc.h (original)
> > +++ trunk/subversion/include/svn_wc.h Tue Nov 15 13:36:57 2005
> > + /** Translate from Normal Format; excludes SVN_WC_TRANSLATE_TO_NF */
> > +#define SVN_WC_TRANSLATE_FROM_NF 0x00000000
> > +
> > + /** Translate to Normal Format; excludes SVN_WC_TRANSLATE_FROM_NF */
> > +#define SVN_WC_TRANSLATE_TO_NF 0x00000001
>
> What is 'Normal Form(at)'?
Well, the essence of my changes are actually to keep that an
implementation detail. The user of these constants should not have to
know what it is.
> I guess (from later) that it's the format
> that the text-base is stored in: LF line termination and unexpanded
> keywords.
Ah! See? This is exactly why it should be a (hidden) implementation
fact: you have it only partly right. Yes, some files have LF
termination. Others have CRLF termination as their normal form.
Keywords are always stored unexpanded. But, the normal form for a
symlink is entirely different from the actual versioned file (ie not a
symlink).
> But it'd be good to document it here, since we don't
> use the phrase elsewhere.
Then maybe the README.txt in libsvn_wc should be expanded, or some
document in notes/ should be added explaining what the normal form is
defined as. All a programmer should need to know (IMO) is that we
have such a thing and that you need to translate a WC file to it
before sending it to the repository.
> (Hmm, would something even clearer like
> SVN_WC_TRANSLATE_TEXTBASE_TO_WORKING be better? Probably too long.)
But: it's not only the text base, it's also the contents as stored in
the repository.
> > +
> > + /** Only do translation associated with the svn:special property only */
> > +#define SVN_WC_TRANSLATE_SPECIAL_ONLY 0x00000002
>
> Offhand, I don't know what kind of translation we'd do with respect to the
> svn:special property at all. Could we have some more information here?
Again, I agree that we may need more information on what libsvn_wc
does and how, but I'd rather put that in some design document than in
the docstrings.
> > +
> > + /** Translate the special property only */
> > +#define SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP 0x00000004
>
> Comment is wrong.
Oops! Thanks!
> > +/** Set @a *xlated_path to a path to translated copy of @src
> > + * or to @a src itself if no translation is necessary.
> "(unless SVN_WC_TRANSLATE_FORCE_COPY is set)"? I know, you mention it
> further down, maybe it's unimportant.
>
> > + * If @c SVN_WC_TRANSLATE_DEL_TEMP_ON_POOL_CLEANUP is specified,
> > + * a pool cleanup handler is registered on *xlated_p if it points to a
> > + * temporary file.
> > *
> > * If an error is returned, the effect on @a *xlated_p is undefined.
>
> "xlated_path", both times.
Thanks for the review! What do you think about my comments?
bye,
Erik.
Received on Thu Nov 17 11:17:07 2005