Hi Julian!
Thanks for the continued effort :-)
On 11/19/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> I've just come across this new function while reviewing the latest
> "disambiguate (de)translation" patch.
>
> I found this function very difficult to understand.
With your idea to add a svn_subst_translate_from_normal_form and if
both get an extra argument (repair_eol_style), there wouldn't be a
need to export this function anymore.
> > svn_subst_eol_style_normalize (svn_boolean_t *force_repair,
> > const char **eol,
> > svn_subst_eol_style_t style);
>
> The doc string seems to be written from the point of view of a user of some
> other particular function(s). Please could you define this function in terms
> unrelated to the other function(s). Also mention the "style" parameter. Also
> the function's name doesn't clearly indicate what it does.
Ok. It returns the eol-style values to be passed to
svn_subst_copy_and_translate3. It does that based on the values a user
specifies which he wants to see in his working copy (ie. normally
derived from svn:eol-style on the file or an svn:eol-style autoprop).
> This function is used in two places: in svn_subst_translate_to_normal_form()
> local to its definition, and in the new svn_wc_translated_file2() in libsvn_wc.
Which was actually going to use svn_subst_translate_to_normal_form,
but this function doesn't have the always_repair_eols parameter I
needed to do the 'svn commit --force foo' example from my other mail.
> This function seems to return two bits of information that are only loosely
> related. The "force_repair" decision is the province of libsvn_wc and
> shouldn't be known at this level. It has little to do with determining how to
> translate to Normal Form; it only controls whether the translation will fail if
> the source has mixed EOLs.
Hmm. Maybe: svn_subst_translate_to_normal_form is used by
libsvn_client too but I didn't need the always_repair there. I
actually created the svn_subst_eol_style_normalize to serve the import
functionality in libsvn_client; then realizing I could just use
..._translate_to_normal_form and used that. Maybe it *is* a good idea
to remove this function from the public interface.
> Then, the whole concept of a "Normal Form" is a WC concept - well, and a
> repository concept.
Yes, and because libsvn_client also uses it (hence being system-wide),
I'd say the only right place to add *a* routine for translating to
'normal form' is in libsvn_subr...
> It isn't really inherent to the mechanism of translating
> EOL styles. Does anything apart from the WC need to convert to/from it?
Ah. Well, actually: anything which sets translating-properties,
doesn't use libsvn_wc, but does write to the repository. (svk/custom
clients come to mind.)
> Maybe
> other client-side repository accesses (like "svn cat URL"), so maybe
> knowledge of Normal Form and functions to convert to/from it will have to
> stay in here
> (libsvn_subr) just to share the knowledge between those callers.
Yes. But if we provide the ..._to_normal_form, we can guarantee other
repository users put the right forms into the repository to be useable
with any and all clients (including our own).
> RELATED FUNCTIONS
[ snip the doc string for svn_subst_translation_required to which the
comment below doesn't seem to refer ]
> > /** Convenience routine (wrapper around svn_subst_copy_and_translate3)
> > * which detranslates the given @a src into @a dst. The parameters
> > * specified should be those immediately taken from the files'
> > * properties.
> > *
> > * @since New in 1.4
> > *
> > */
> > svn_error_t *
> > svn_subst_translate_to_normal_form (const char *src,
> > const char *dst,
> > svn_subst_eol_style_t eol_style,
> > const char *eol_str,
> > apr_hash_t *keywords,
> > svn_boolean_t special,
> > apr_pool_t *pool);
>
> Perhaps some of the same arguments apply to these. At least the "disambiguate
> (de)translation" patch should probably remove the word "detranslate" from this
> doc string.
>
> If we're having a "svn_subst_translate_to_normal_form", why not a
> "svn_subst_translate_from_normal_form" to complement it?
Currently we don't, because svn_subst_copy_and_translate3 given the
same arguments this function takes, will do the
..._from_normal_form(). We could define a macro or a wrapper to do it
and make the code look more consistent, but there's no necessity from
an abstraction/encapsulation pov.
> > svn_error_t *
> > svn_subst_eol_style_normalize (svn_boolean_t *force_repair,
> > const char **eol,
> > svn_subst_eol_style_t style)
> > {
> > *force_repair = style == svn_subst_eol_style_fixed;
> > switch (style)
> > {
> > case svn_subst_eol_style_fixed:
> > break;
> >
> > case svn_subst_eol_style_native:
> > *eol = SVN_SUBST__DEFAULT_EOL_STR;
> > break;
> >
> > case svn_subst_eol_style_none:
> > *eol = NULL;
>
> When the style is "none", isn't "*eol" already null?
It should.
> > break;
> >
> > default:
> > return svn_error_create (SVN_ERR_IO_UNKNOWN_EOL, NULL, NULL);
> > }
> >
> > return SVN_NO_ERROR;
> > }
Ok. I propose to:
- rewrite all svn_subst.h doc strings not to mention 'detranslate' anymore;
- remove svn_subst_eol_style_normalize from our public interface;
- add an 'always_repair_eols' boolean to svn_subst_translate_to_normal_form;
- do *not* introduce svn_subst_translate_from_normal_form since it
would be too similar to svn_subst_copy_and_translate3;
- Incorporate your docstring comments as mentioned above.
What do you say to that? (I don't want to do the work before you say
anything, because I had hoped both commits to be 'quick wins', but
they turn out to be more complex than the ones I normally consider
difficult :-(
bye,
Erik.
Received on Sat Nov 19 22:36:48 2005