Hi Malcolm!
Thanks for your continued review! (Reactions and explanations below)
On 11/19/05, Malcolm Rowe <malcolm-svn-dev@farside.org.uk> wrote:
> On Fri, Nov 18, 2005 at 03:42:10PM +0100, Erik Huelsmann wrote:
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 17435)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -64,23 +64,42 @@
> > * @{
> > */
> >
> > - /** Translate from Normal Format; excludes SVN_WC_TRANSLATE_TO_NF */
> > + /** Translate from Normal Format.
> > + *
> > + * The working copy text bases and repository files are stored
> > + * in this form if they have any svn: properties set which
> > + * require translation.
>
> Actually, they're always stored in this format, aren't they?
That depends what you call normal form: files without any of the
translation-causing svn: properties are stored untouched, meaning that
they may even be stored without consistent EOLs (and thereby violating
our normal format).
> Minor nit, since it's probably determinable by context, but: it's not
> entirely clear what "this form" above refers to (also, nit: the 'form'
> vs. 'format' thing I mentioned before).
> This is in the documentation
> for a flag that translates _from_ Normal Format; it then says "are
> stored in this form" which makes me slightly confused: is it referring
> to the form I'm using this flag to translate to, or the 'Normal Format'
> mentioned in the flag's summary?
>
> > + *
> > + * This flag excludes SVN_WC_TRANSLATE_FROM_NF
>
> How about "cannot be used with" rather than "excludes"?
> > +#define SVN_WC_TRANSLATE_NO_EOL_REPAIR 0x00000002
> > +#define SVN_WC_TRANSLATE_FORCE_EOL_REPAIR 0x00000004
>
> Again, "cannot be used with" might be simpler.
Yup. same comment as julian. Addressed in next version.
> I'm confused about these
> two options. From the descriptions, it seems like there must be some
> default functionality to do with 'repairing eol styles' (which is what,
> exactly? It's not been mentioned anywhere so far), one that's used when
> neither flag is passed.
Right. But it is in the function description. Hmm. Maybe it's better
to duplicate it to these docstrings? OTOH, duplicate docs tend to get
out of sync :-(
> But I can't think what it would be. You've got 'do no repair', and
> 'force repair'. Is the default option 'do some repair'?
Exactly: On some formats (the fixed ones) we repair as a default, on
others (native) we only check consistency and error out if
inconsistencies are found.
> I assume that these options are only intended to be used with
> _TRANSLATE_TO_NF? Probably worth documenting that, if so, in addition
> to whatever repair we do by default.
Well, we'd only use them when translating TO_NF, but, if a file would
be stored incorrectly in the repository, they wouldn't be without
effect, so: they're not restricted to TO_NF. (Though logically, yes,
our client doesn't use them FROM_NF.)
> > + /** Don't register a pool cleanup on the output file */
> > +#define SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP 0x00000008
> > +
> > /** Guarantee a new file is created on successful return.
> > * The default shortcuts translation by returning the path
>
> nit: I keep reading 'default' as an adjective, and 'shortcuts' as a noun.
> (The default shortcuts... what?). Perhaps 'default implementation
> shortcuts...'.
Ok.
> > -/** 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.
> > * 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.
> > + * as requested by @a flags. The default eol translation can be
> > + * overridden by specifying either @c SVN_WC_TRANSLATE_NO_EOL_REPAIR or
> > + * @c SVN_WC_TRANSLATE_FORCE_EOL_REPAIR.
>
> Okay, so here's what those options do. I grok FORCE_EOL_REPAIR, but
> not NO_EOL_REPAIR...
>
> [delete three paragraphs where I try to guess what it does.]
>
> No, sorry, I can't guess.
Ok: Not the default (which is: repair sometimes), but repair *never*
(as opposed to repair always which is force_eol_repair).
> Also, you updated all the callers, but there don't appear to _be_ any
> callers that pass these flags: do we really need them? Perhaps better
> as a separate patch?
Yes, we need them. What you found is a flaw in my patch. I should have
updated svn_wc_translated_file (). Julian commented that I discarded
an argument which I shouldn't have. It needs this patch:
@@ -114,7 +114,10 @@
/* we discard the force_repair flag,
because we know what to do ourselves now... but is it acceptable?! */
return svn_wc_translated_file2 (xlated_p, vfile, vfile, adm_access,
- SVN_WC_TRANSLATE_TO_NF, pool);
+ SVN_WC_TRANSLATE_TO_NF
+ | (force_repair
+ ? SVN_WC_TRANSLATE_FORCE_EOL_REPAIR
+ : SVN_WC_TRANSLATE_NO_EOL_REPAIR), pool);
}
> > - * Temporary files are created in the temp file area belonging to
> > + * Output files are created in the temp file area belonging to
> > * @a versioned_file.
>
> Add something like 'By default, they will be deleted at pool cleanup
> time.'?
Good idea. Thanks again!
bye,
Erik.
Received on Sat Nov 19 13:06:03 2005