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.
>>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.
> [[[
> Followup to r17362.
>
> * subversion/include/svn_wc.h
> (SVN_WC_TRANSLATE_FROM_NF,
> SVN_WC_TRANSLATE_TO_NF): Expand docstrings.
> (SVN_WC_TRANSLATE_SPECIAL_ONLY): Remove. It exposes too
> much internal detail - and adds little optimization as soon as
> wc-propcaching is merged.
> (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 by
any 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 is closely related to the "force_repair" option in
svn_subst_eol_style_normalize() which I have just posted about. 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 decision should
come from a function in libsvn_wc.
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. (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.)
> (SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP): New. Disables
> the automatically registered pool cleanup to delete the output file.
> (SVN_WC_TRANSLATE_FORCE_COPY): Renumber.
> (svn_wc_translated_file2): Expand doc string.
>
> * subversion/libsvn_wc/translate.c
> (svn_wc_translated_file2): Support new and changed flags.
>
> * subversion/libsvn_wc/merge.c
> * subversion/libsvn_wc/diff.c
> * subversion/libsvn_wc/adm_crawler.c
> * subversion/libsvn_wc/update_editor.c
> * subversion/libsvn_wc/questions.c
> Update callers.
> ]]]
>
>
> Index: subversion/include/svn_wc.h
> ===================================================================
(I've snipped some of the old ("-") diff lines to concentrate on the new version.)
> + /** 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.
> + *
> + * This flag excludes SVN_WC_TRANSLATE_FROM_NF
> + */
> #define SVN_WC_TRANSLATE_FROM_NF 0x00000000
>
> + /** Translate to Normal Format. excludes SVN_WC_TRANSLATE_FROM_NF */
> #define SVN_WC_TRANSLATE_TO_NF 0x00000001
The "excludes" text is unclear. May I suggest:
Either this or SVN_WC_TRANSLATE_TO_NF must be specified (but not both).
> + /** Prevent any repair of eol styles, erroring out if
> + * the input doesn't use one eol style consistently.
> + * All other appropriate translations will be performed regardless.
> + *
> + * This flag excludes the use of SVN_WC_FORCE_EOL_REPAIR.
> + */
> +#define SVN_WC_TRANSLATE_NO_EOL_REPAIR 0x00000002
>
> + /** 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?
> + *
> + * 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"?
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).
> + /** Don't register a pool cleanup on the output file */
For clarity: "on the output file" -> "to delete the output file".
> +#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.
> + adm_access, pool));
> + SVN_ERR (svn_wc__get_keywords (&keywords, versioned_file,
> + adm_access, NULL, pool));
> SVN_ERR (svn_wc__get_special (&special, versioned_file, adm_access, pool));
>
>
> @@ -79,24 +77,26 @@
> }
> else /* some translation (or copying) is necessary */
> {
> + svn_boolean_t should_repair = FALSE;
> + svn_boolean_t contract_keywords = ! (flags & SVN_WC_TRANSLATE_TO_NF);
> +
> SVN_ERR (svn_io_open_unique_file2
> (NULL, &tmp_vfile,
> tmp_vfile,
> SVN_WC__TMP_EXT,
> + (flags & SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP)
> + ? svn_io_file_del_none : svn_io_file_del_on_pool_cleanup,
> pool));
>
> if (flags & SVN_WC_TRANSLATE_TO_NF)
> + svn_subst_eol_style_normalize (&should_repair, &eol, style);
> +
> + SVN_ERR (svn_subst_copy_and_translate3
> + (src, tmp_vfile,
> + eol, repair_forced || (should_repair && ! repair_disabled),
> + keywords, contract_keywords,
> + special,
> + pool));
> *xlated_path = tmp_vfile;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 19 01:20:47 2005