Erik Huelsmann wrote:
> Ok, I think I addressed all concerns in the patch below. If we're 99%
> there, I think I can commit and work from there.
> [[[
> Followup to r17362.
>
> Suggested by: julianfoad
> malcolm
>
> * subversion/include/svn_subst.h:
> Doc string expansions, including elimination 'detranslate'-like words.
After making several comments on these doc strings, below, I wonder if you
would like to commit the code changes (except for renumbering the TO/FROM_NF
flags, if you agree), and then address the doc strings separately.
> * subversion/include/svn_wc.h
> (SVN_WC_TRANSLATE_FROM_NF,
> SVN_WC_TRANSLATE_TO_NF): Expand docstrings. Renumber too.
> (SVN_WC_TRANSLATE_SPECIAL_ONLY): Remove. It exposes too
> much internal detail - and adds next to no optimization as soon as
> wc-propcaching is merged.
> (SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP): Replaces (and inverts) the
> SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP flag.
> (svn_wc_translated_file2): Expand doc string.
>
> * subversion/libsvn_wc/translate.c
> (svn_wc_translated_file2): Support new and changed flags.
>
> * subversion/libsvn_wc/merge.c
> * subversion/libsvn_wc/diff.c
> * subversion/libsvn_wc/adm_crawler.c
> * subversion/libsvn_wc/update_editor.c
> * subversion/libsvn_wc/questions.c
> Update callers.
> ]]]
>
> Index: subversion/include/svn_subst.h
> ===================================================================
> --- subversion/include/svn_subst.h (revision 17455)
> +++ subversion/include/svn_subst.h (working copy)
> @@ -260,8 +260,9 @@
>
> /**
> * Convenience routine: a variant of svn_subst_translate_stream3()
> - * which operates on files. In addition, it will create/detranslate a special
> - * file if @a special is @c TRUE.
> + * which operates on files. In addition, it will create a special file
> + * from normal form or translate one toto normal form if @a special
Wasn't Toto a dog? :-)
> + * is @c TRUE.
> *
> * Copy the contents of file-path @a src to file-path @a dst atomically,
> * either creating @a dst (or overwriting @a dst if it exists), possibly
> @@ -356,7 +357,9 @@
> apr_pool_t *pool);
>
> /** Convenience routine (wrapper around svn_subst_copy_and_translate3)
I think we should try to avoid describing our API functions as "convenience
wrappers" except when they very closely match the function they wrap. That is
how they start off being designed and implemented, but it is not really what
the user needs to know first. Far better to start with a regular description
of the function, and then mention, if necessary, that further details are the
same as in svn_subst_copy_and_translate3().
Put "()" after the function name so Doxygen recognises it.
> - * which detranslates the given @a src into @a dst.
> + * which translates a normal form @a src into a working copy form @a dst.
Try to indicate that @src and @dst are files (not just strings to translate):
something like "translates the file @src in normal form to the file @dst in
working copy form."
> + * The parameters specified should be those immediately taken from the files'
> + * properties.
> *
> * The values specified for @a eol_style, @a *eol_str, @a keywords and
> * @a special, should be the ones used to translate the file to its
> @@ -368,6 +371,10 @@
> * returned. By setting @a always_repair_eols to @c TRUE, eols will be
> * made consistent even for those styles which don't have it by default.
> *
> + * @note The API doesn't provide a svn_subst_translate_from_normal_form
> + * because it would look too much like
> + * svn_subst_copy_and_translate3.
> + *
That's a bit of an odd thing to say in documentation. Maybe just "@note To
translate FROM normal form, you can use svn_subst_copy_and_translate3()."
> * @since New in 1.4
> *
> */
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17455)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -64,15 +64,24 @@
> * @{
> */
>
> - /** Translate from Normal Format; excludes SVN_WC_TRANSLATE_TO_NF */
> -#define SVN_WC_TRANSLATE_FROM_NF 0x00000000
> + /** Translate from Normal Form.
> + *
> + * The working copy text bases and repository files are stored
> + * in normal form if they have any svn: properties set which
> + * require translation.
> + *
> + * Either this flag or @c SVN_WC_TRANSLATE_FROM_NF should be specified,
> + * but not both.
> + */
> +#define SVN_WC_TRANSLATE_FROM_NF 0x00000001
>
> - /** Translate to Normal Format; excludes SVN_WC_TRANSLATE_FROM_NF */
> -#define SVN_WC_TRANSLATE_TO_NF 0x00000001
> + /** Translate to Normal Form.
> + *
> + * Either this flag or @c SVN_WC_TRANSLATE_FROM_NF should be specified,
> + * but not both.
> + */
> +#define SVN_WC_TRANSLATE_TO_NF 0x00000002
Hmm... the numbering is now a bit funny... it's as if these are two separate
flags. Having FLAG_A = NUMBER_X and INVERSE_OF_FLAG_A = 0 made sense. (The
only thing that made the sequence of numbers look a bit odd was the fact that
the only flag of value zero happened to be the first item in the list. If it
had been somewhere else in the list it would not have looked so odd.)
Everything else looks fine (except the "assert" relating to that pair of flags).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 20 20:36:05 2005