On 11/14/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> 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.
Heh. yes. I guess so. I stated how I started out.
> > * 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?
No, it doesn't: It collects the a lot of different uses of
svn_subst_copy_and_translate3 and their setups.
svn_wc_subst_copy_and_translateX is merely a wrapper around
svn_subst_translate_streamX ().
> > 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?
From what I can tell? No, because there are 2 paths passed (source and
'path to use for props') passed in and the translated path
(xlated_path) generated as output. Though, while writing this mail, I
start to figure, I probably *can* detect tthat myself.
> > @@ -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.
Definitely.
> > + 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?
I did at first, but felt it was better done this way. Now that some of
the other arguments are gone, I think I feel better about moving it
back into the flags field.
> > + apr_pool_t *pool);
>
> Having changed the arguments I think you need to change the doc string.
Absolutely. I completely forgot about that.
Thanks for your input. It's a great refinement - as usual!
bye,
Erik.
Received on Mon Nov 14 20:01:51 2005