Erik Huelsmann wrote:
> The attached patch disambiguates (in my eyes) file
> translation/detranslation (can *you* tell by the name what should
> happen if a file is translated? does it end up in working copy form or
> in normal form? I have to think hard every time again.)
Good idea. I couldn't tell what it did.
> [[[
> Disambiguate file (de)translation.
Looking at the patch, it seems you've added some significant bits of
functionality to svn_wc_translated_file2() as well. Please mention that.
> * subversion/include/svn_wc.h
> * subversion/libsvn_wc/translate.c
> (svn_wc_translated_file2): Update to support translation both ways.
and ...?
> * subversion/libsvn_wc/questions.c
> * subversion/libsvn_wc/diff.c
> * subversion/libsvn_wc/merge.c
> * subversion/libsvn_wc/log.c
> * subversion/libsvn_wc/adm_crawler.c
> Update callers.
> ]]]
>
> Anybody with comments?
Doesn't this patch make some functions obsolete?
(svn_subst_copy_and_translate3? something_detranslate_something?) Should we
deprecate them? Why, or why not?
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17323)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -57,7 +57,40 @@
> +/** Flags for use with svn_wc_translated_file2
> + *
> + * @defgroup translate_flags Translation flags
> + *
> + * @{
> + */
Good idea. True/false parameters related to non-fundamental behaviours are
ugly because they don't convey any meaning at the call site.
> + /** Translate from Normal Format (the default) */
> +#define SVN_WC_TRANSLATE_FROM_NF 0x00000000
> +
> + /** Translate to Normal Format; overrides _FROM_NF */
> +#define SVN_WC_TRANSLATE_TO_NF 0x00000001
Rather than saying "(the default)" and "overrides _FROM_NF", I'd prefer either
saying "Exactly one of these two options must be specified" or losing the first
option altogether, as it would be ugly to have some callers specifying the
first option explicitly and others leaving it out because it is the default.
> + /** Translate the special property only */
> +#define SVN_WC_TRANSLATE_SPECIAL_ONLY 0x00000002
It's not clear to me what this means. (Which special property is it talking
about? What does it do to it? Which other aspects of this function's possible
behaviour are suppressed by this option?)
It would be nice to have orthogonal options as far as possible. This one
sounds like a candidate for a separate function altogether.
> + /** Force all EOLs to the same type */
> +#define SVN_WC_TRANSLATE_FORCE_REPAIR 0x00000004
Please include "EOL" in the name of that option.
Can any repair of EOLs happen without this option? If not, then the word
"FORCE" is redundant.
> + /** 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
> +
> + /** Create temporary to translate file to in the same directory
> + * as the translation source. The default is to assume the source
> + * is a versioned file in a working copy for which a temporary can
> + * be created in the admin area. */
> +#define SVN_WC_TRANSLATE_IN_SRC_DIR 0x00000010
Do we really need this to be controllable? Can't the function determine this
for itself?
> @@ -3142,12 +3175,14 @@
> *
> * @since New in 1.4
> */
> -svn_error_t *svn_wc_translated_file2 (const char **xlated_p,
> - const char *vfile,
> - svn_wc_adm_access_t *adm_access,
> - svn_boolean_t force_repair,
> - svn_boolean_t del_temp_on_pool_cleanup,
> - apr_pool_t *pool);
> +svn_error_t *
> +svn_wc_translated_file2 (const char **xlated_path,
> + const char *src,
> + const char *path,
Could you make these formal parameter names more meaningful? The name "path"
isn't helpful when it's just one of three paths.
> + svn_wc_adm_access_t *adm_access,
> + apr_uint32_t flags,
> + svn_boolean_t del_temp_on_pool_cleanup,
Why not move that "del_temp" into a bit flag as well?
> + apr_pool_t *pool);
Having changed the arguments I think you need to change the doc string.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 14 16:20:56 2005