On 11/19/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> Erik Huelsmann wrote:
> >
> > Given that svn_wc_translated_file already is public, I think we should
> > provide the increased useability. If that hadn't been public, I could
> > very easily be persuaded not to make it public.
>
> OK. The initial version seemed to have lots of caller-specific knobs on it,
> but now you are making it much more general and "clean" so much better
> suited to being public.
Ok, I hope we can do some further work to make it completely acceptable.
> >>It would also be worth investigating why that single caller (the one in
> >>subversion/libsvn_wc/update_editor.c)
> [is unique in needing a non-temporary file]
> >
> > No, it's not possible to make it clean up: the name of the file
> > generated is used in a log file for later processing, but log files
> > may outlive the pool's life time, for example if the log file
> > processing errors out.
>
> OK. I still wonder why there aren't other callers that use this to create a
> permanent text base WC file, but there's probably a good reason.
Probably because they don't use log files to manipulate them, but
they'll move the files into place in the same function as they're
created in. In that case, it's safe to remove the file at pool
cleanup: either it's renamed into place (thus not existing anymore at
the original path, so that removal has no effect), or the routine
error-ed out and cleanup is appropriate.
I have some doubts the above is as clear as it should be, but I expect
you'll be telling me so if it isn't.
> > (SVN_WC_TRANSLATE_NO_EOL_REPAIR,
> > SVN_WC_TRANSLATE_FORCE_EOL_REPAIR): New. Constants
> > to override the default EOL repair strategy.
>
> Can you explain why you felt it necessary to provide these two options to
> override the default libsvn_wc repair strategy, given that they are not used
> byany present caller? I suppose you will say "To make the function
> general-purpose." But, if you say that, I will say, "Then the caller should
> decide whether it will repair, and there should be a single boolean option,
> not a default and two overrides."
This took its own direction in a thread with Malcolm, but to shortcut
the answer: I deleted SVN_WC_TRANSLATE_NO_EOL_REPAIR, only leaving
_FORCE_EOL_REPAIR, to override the default of 'incorrect EOL detection
on "native" eols'.
> This is closely related to the "force_repair" option in
> svn_subst_eol_style_normalize() which I have just posted about.
Yes, saw that. Taking up that reply next.
> It seems like
> perhaps that function should not be called from within this one, or at least
> not a form of it that obtains a "force_repair" decision.
That's correct. That's why the value-receiving param in
svn_wc_translated_file2 is called 'should_repair'.
> That decision should come from a function in libsvn_wc.
The only reason I need SVN_WC_TRANSLATE_FORCE_EOL_REPAIR is to support
recoding of EOLs in the 'native' scenario. I imagine:
$ svn commit foo
svn: File 'foo' has inconsistent line endings
$ svn commit --force foo
Sending foo
Transmitting .
Committed revision <bar>
Which can both use svn_wc_translated_file2 if it uses the
FORCE_EOL_REPAIR parameter in the second case.
> I'm sorry I'm pointing out what appear to be not the best arrangement of
> interfaces, yet I haven't got a clear enough head to comprehend the whole
> plan and say for sure what the best thing would be. I'm hoping you'll
> understand
> where I'm coming from and use my suggestions with your more thorough
> knowledge of the code area to come up with a cleaner design.
So do I :-) I value your (and Malcom's) input a lot. It keeps me from
just making a different mess.
> (You are doing a great job
> cleaning up libsvn_wc; I'm just concerned that we don't push
> inappropriate bits of knowledge into libsvn_subr.)
Ok. I'll discuss the libsvn_subr change in the other thread.
> > Index: subversion/include/svn_wc.h
> > ===================================================================
>
> >
> > + /** Force repair of eol styles, making sure the output file contains
> > + * one consistent eol style.
>
> Which style? The one specified by its "svn:eol-style" property,
> presumably, if it has one. What if does not have this property?
Check. Addressed in a new version of the patch. BTW: I've also
addressed many of Malcolm's comments and won't go into those in this
mail as well; I just incorporated them.
> > + *
> > + * This flag excludes the use of SVN_WC_NO_EOL_REPAIR.
> > + */
> > +#define SVN_WC_TRANSLATE_FORCE_EOL_REPAIR 0x00000004
>
> Unlike the first pair of options which represent two alternatives, this pair is
> a different kind in which neither need be specified. The "excludes" sentence
> sounds a bit strange, but is probably OK. Maybe "conflicts with"?
No longer applicable as I removed SVN_WC_TRANSLATE_NO_EOL_REPAIR.
> The function's doc string ought to say that, without either of these override,
> the action taken is the default libsvn_wc strategy: repairing certain EOL
> styles when translating to NF, and none when translating from NF (if I got it
> right).
Check.
> > + /** Don't register a pool cleanup on the output file */
>
> For clarity: "on the output file" -> "to delete the output file".
Check.
> > +#define SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP 0x00000008
>
>
> > Index: subversion/libsvn_wc/translate.c
> > ===================================================================
> > @@ -54,19 +54,17 @@
> > apr_hash_t *keywords = NULL;
> > svn_boolean_t special;
> > const char *tmp_dir, *tmp_vfile;
> > - svn_boolean_t special_only = flags & SVN_WC_TRANSLATE_SPECIAL_ONLY;
> > + svn_boolean_t repair_forced = flags & SVN_WC_TRANSLATE_FORCE_EOL_REPAIR;
> > + svn_boolean_t repair_disabled = flags & SVN_WC_TRANSLATE_NO_EOL_REPAIR;
> >
> > svn_path_split (versioned_file, &tmp_dir, &tmp_vfile, pool);
> >
> > tmp_vfile = svn_wc__adm_path (tmp_dir, 1, pool, tmp_vfile, NULL);
> >
> > - if (! special_only)
> > - {
> > - SVN_ERR (svn_wc__get_eol_style (&style, &eol, versioned_file,
> > - adm_access, pool));
> > - SVN_ERR (svn_wc__get_keywords (&keywords, versioned_file,
> > - adm_access, NULL, pool));
> > - }
> > + SVN_ERR (svn_wc__get_eol_style (&style, &eol, versioned_file,
>
> Because this is no longer within an "if", "style" no longer needs to be
> initialised at its declaration.
Same for keywords and eol. Check.
Ok. posting new patch later.
bye,
Erik.
Received on Sat Nov 19 21:08:15 2005