Erik,
Thanks for being patient. I've kind-of lost track of what's been
merged and what we've commented on before, so I hope I'm not repeating
or contradicting myself below.
On Sun, Nov 20, 2005 at 02:59:20PM +0100, Erik Huelsmann wrote:
> Log:
> [[[
> Followup to r17362.
>
> Suggested by: julianfoad
> malcolm
>
> * subversion/include/svn_subst.h:
> Doc string expansions, including elimination 'detranslate'-like words.
'elimination of'
>
> * subversion/include/svn_wc.h
> (SVN_WC_TRANSLATE_FROM_NF,
> SVN_WC_TRANSLATE_TO_NF): Expand docstrings. Renumber too.
'Renumber constants', perhaps. (You're not renumbering the docstrings).
> 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
s/toto/to/
> + * 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
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17455)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -64,15 +64,24 @@
> /** Force repair of eol styles, making sure the output file consistently
> * contains the one eol style as specified by the svn:eol-style
> * property and the required translation direction.
> @@ -80,8 +89,8 @@
> */
> #define SVN_WC_TRANSLATE_FORCE_EOL_REPAIR 0x00000004
While you're here, you may as well remove the extra blank line in the
above doc-comment (hidden by the diff offset line in your patch).
> /** Guarantee a new file is created on successful return.
> * The default shortcuts translation by returning the path
Weren't you going to change this comment? Doesn't really matter, though.
> @@ -3168,14 +3177,20 @@
> -/** Set @a *xlated_path to a path to translated copy of @src
> +/** Set @a *xlated_path to a path to a translated copy of @src
> * or to @a src itself if no translation is necessary.
I think "Set @a *xlated_path to a translated copy of @a src" would be fine
(ditch 'to a path to', and s/@src/@a src).
> * That is, if @a versioned_file's properties indicate newline conversion or
> * keyword expansion, point @a *xlated_path to a copy of @a src
> * whose newlines and keywords are converted using the translation
> * as requested by @a flags.
> *
> - * Caller can explicitly request a new file to be returned by setting the
> + * When translating to the normal form, inconsistent eol styles will be
> + * repaired when appropriate for the given setting. When translating
> + * from normal form, no EOL repair is performed (consistency is assumed).
Could prefix with 'By default,', or similar, and 'the given setting'
could be clarified slightly ('the given svn:eol-style', perhaps?).
But thanks for explaining this!
> Index: subversion/libsvn_wc/adm_crawler.c
> ===================================================================
> --- subversion/libsvn_wc/adm_crawler.c (revision 17455)
> +++ subversion/libsvn_wc/adm_crawler.c (working copy)
> @@ -720,8 +719,7 @@
> the new text base anyway. */
> SVN_ERR (svn_wc_translated_file2 (&tmpf, path, path,
> adm_access,
> - SVN_WC_TRANSLATE_TO_NF
> - |
> SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,+
> SVN_WC_TRANSLATE_TO_NF,
Patch is word-wrapped, or broken, or something. Either way, it won't
apply. Not that it matters, but was it your mail client or 'svn diff'
that broke it?
> Index: subversion/libsvn_wc/log.c
> ===================================================================
> --- subversion/libsvn_wc/log.c (revision 17455)
> +++ subversion/libsvn_wc/log.c (working copy)
> @@ -246,14 +246,11 @@
> const char *tmp_file;
>
> err = svn_wc_translated_file2
> - (&tmp_file,
> - full_from_path,
> - full_dest_path, adm_access,
> - SVN_WC_TRANSLATE_FROM_NF
> - | SVN_WC_TRANSLATE_FORCE_COPY
> - | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
> - | (special_only ? SVN_WC_TRANSLATE_SPECIAL_ONLY : 0),
> - pool);
> + (&tmp_file,
> + full_from_path, full_dest_path, adm_access,
> + SVN_WC_TRANSLATE_FROM_NF
> + | SVN_WC_TRANSLATE_FORCE_COPY,
> + pool);
Any particular reason you decided to reformat this? Not that it matters,
of course.
> @@ -280,9 +277,7 @@
> full_from_path,
> full_from_path, adm_access,
> SVN_WC_TRANSLATE_TO_NF
> - | SVN_WC_TRANSLATE_FORCE_COPY
> - | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
> - | (special_only ? SVN_WC_TRANSLATE_SPECIAL_ONLY : 0),
> + | SVN_WC_TRANSLATE_FORCE_COPY,
> pool));
> SVN_ERR (svn_io_file_rename (tmp_file, full_dest_path, pool));
>
You've removed all the uses of 'special_only' from the static function
file_xfer_under_path(). You could also remove the argument from the
function, and then you'd see that you could remove a local variable and
another function call from the only caller, log_do_file_xfer().
> Index: subversion/libsvn_wc/translate.c
> ===================================================================
> --- subversion/libsvn_wc/translate.c (revision 17455)
> +++ subversion/libsvn_wc/translate.c (working copy)
> @@ -49,24 +49,17 @@
> apr_uint32_t flags,
> apr_pool_t *pool)
> {
> + assert (flags & (SVN_WC_TRANSLATE_TO_NF | SVN_WC_TRANSLATE_FROM_NF));
I'm not sure whether this assert() is appropriate. This is a
user-callable function, so either the user will get an assert (if they've
built with debugging) or, more likely, the assert() will be ignored.
How about returning an error like SVN_ERR_INCORRECT_PARAMS instead?
> + svn_boolean_t repair_forced = flags & SVN_WC_TRANSLATE_FORCE_EOL_REPAIR;
Just out of interest, why did you decide to create a local variable
for this? You didn't for checking the SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP
flag.
> if (flags & SVN_WC_TRANSLATE_TO_NF)
> + /* to normal form */
> SVN_ERR (svn_subst_translate_to_normal_form
Someone else said, of one of my patches, "I'm not a fan of comments like
these" - and I'd tend to agree.
> - else /* translate */
> + else /* from normal form */
> SVN_ERR (svn_subst_copy_and_translate3
This one's perhaps more useful - especially if you made it a full
sentence: 'translate from normal form'.
Regards,
Malcolm
---------------------------------------------------------------------
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:48 2005