> > enhanced version of the non-2 version to allow the use of our new
> > remove-temp-file-on-pool-cleanup code. I still think that should stay,
> > as it greatly simplifies some code which used to manually track errors
> > (instead of using SVN_ERR).
>
> OK. I hadn't looked at why the present ...2() version was introduced. If it
> is used by many callers (like more than two) then it sounds worth keeping.
The old ...2() function is used in exactly the same call sites as the
current function.
> >>Can we make a private one first? Then we would have plenty of freedom to
> >>change it, have it incompletely documented, and so on. This might well avoid
> >>the need to fix some of the problems mentioned above.
> >
> > Sure. There never even was a (good) reason to provide
> > svn_wc_translated_file anyway, because all callers could call
> > svn_subst_copy_and_translate directly anyway. But, I think it's a good
> > idea not to keep certain routines which provide certain oft-used
> > behaviours for ourselves.
>
> It's nearly always a good idea to factor out common code. Whether that
> factored-out subroutine is generally useful, or only designed to be used by the
> existing callers, is a completely different question.
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.
> >>Eventually we could make
> >>the function public if and when it becomes stable and generally useful and
> >>seems worthwhile.
> >
> > I'd prefer reverting and re-introducing later than making private and
> > maybe-sometime-making-public. When would that 'sometime' be?
>
> Well, this change appears to reduce the amount of duplicated code and
> spread-out knowledge in the current libsvn_wc, which is definitely a good
> thing. That is why I thought it would be worthwhile to keep the function.
>
> Why don't you like the idea of making a private function? What is the
> disadvantage compared with leaving the code as it is? (The advantage is well
> understood by both of us.)
I don't like to make it private, because we'll still be offering
publicly what we ourselves consider outdated or not usefull anymore...
> >>>/** Same as svn_wc_translated_file2, but will never clean up
> >>> * temporary files.
> >>> *
> >>> * @deprecated Provided for compatibility with the 1.3 API
> >>> */
> >>>svn_error_t *svn_wc_translated_file (const char **xlated_p,
> [...]
> >>This doc string needs to be updated (or reverted).
> >
> > If we decide to do that: ofcourse.
>
> I'm not sure what you are referring to by "do that", so just to be sure you
> know what I mean, I mean that the code currently checked in to Subversion trunk
> has a bad doc string there: it needs to say much more than that before it can
> claim to be "Same as svn_wc_translated_file2".
Ok. I was referring to 'reverting the change' with 'it'. I have
attached a patch however which should address all of your concerns
except the 'not making public' one.
> >> What does it do in reverse: can it translate a file or dir to
> >>a symlink? What does it do to a non-special file? Does it still obey
> >>"FORCE_COPY" for such a file?
> >
> > FORCE_COPY is there to disable an optimization [...]
>
> Yes, I knew what FORCE_COPY does, but I don't mind you explaining it just in
> case I had misunderstood it.
>
> What I didn't know (and believe I know now, from your explanation) is that a
> symlink translation can never be a no-op,
Exactly.
> > The flag itself not, but I believe the docstring for
> > svn_wc_tranlated_file2 does:
> >
> > * Temporary files are created in the temp file area belonging to
> > * @a versioned_file.
>
> I'll admit I didn't notice that, but I don't think it is relevant. I don't
> consider the output of the function to be a "temporary" file in that sense,
> because the function doesn't know what the caller intends to do with the file
> and its doc string doesn't mention anything about the result being temporary.
Ok. I changed it to "Output files are created ..."
> >> We need to do one of:
> >>
> >>* Allow the caller to specify where to put it. (We don't want yet another path
> >>argument, but maybe the semantics could be changed a bit so that one of the
> >>existing arguments optionally specifies the desired location.)
> >
> > I thought about that: xlated_path could point to a valid path when a
> > certain flag is specified I didn't do that for 3 reasons: 1) merge.c
> > is the only user of that functionality (atm); 2) I couldn't decide on
> > the name of the flag; 3) it has an ugly amount of overlap with the
> > current FORCE_COPY, which doesn't require a pathname as input (which I
> > expect to use a lot).
> >
> >>* Document that "This creates a file where the function ...() decides". (No; I
> >>don't think that would be a good API design.)
> >>
> >>* Change the "merge" caller so that it doesn't have this requirement. (This
> >>would probably be best, and it might make this option redundant since I can't
> >>imagine any other reason for insisting on a copy.)
> >
> > The reason you'd want a copy is somewhere higher up in this lengthy
> > reply, but: There are many occasions you want to have a file which you
> > want to (atomically) rename into place later. Oftentimes you can't do
> > that with the original, either because it's already used as text base,
> > or if we're talking about adding files, because it's already used as
> > working copy file (and we're creating a text base).
>
> I wasn't disputing that we need a copy. I meant change "merge" so that it
> doesn't require the copy generated by this function to be in a particular
> directory.
Ah. Ok. noting that for a separate change.
> One last thing: I see that SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP is supplied
> on every call except one. (I don't count the implementation of the deprecated
> svn_wc_translated_file() because that could easily be implemented differently.)
>
> I think this suggests at least that the option should be inverted, so that
> deleting is the normal behaviour, and the one caller that wants to KEEP the TMP
> has to specify something like "SVN_WC_TRANSLATE_KEEP_TMP_ON_POOL_CLEANUP".
I changed this too. See the attached patch.
> It would also be worth investigating why that single caller (the one in
> subversion/libsvn_wc/update_editor.c) is different from the others, and if it
> doesn't really need this behaviour or could easily obtain this behaviour by
> other means then it would be better to remove the option from the interface.
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.
> Also, for naming and describing this option, as I alluded to above, I don't
> like the output file being referred to as a "temporary" file. It may well be
> temporary in many cases, but that's not how it is basically described by the
> doc string.
Changed.
Ok. Here's the patch:
Log:
[[[
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.
(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
===================================================================
--- 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.
+ *
+ * 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 */
+ /** Translate to Normal Format. excludes SVN_WC_TRANSLATE_FROM_NF */
#define SVN_WC_TRANSLATE_TO_NF 0x00000001
- /** Only do translation associated with the svn:special property only */
-#define SVN_WC_TRANSLATE_SPECIAL_ONLY 0x00000002
+ /** 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
- /** Translate the special property only */
-#define SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP 0x00000004
+ /** Force repair of eol styles, making sure the output file contains
+ * one consistent eol style.
+ *
+ * This flag excludes the use of SVN_WC_NO_EOL_REPAIR.
+ */
+#define SVN_WC_TRANSLATE_FORCE_EOL_REPAIR 0x00000004
+ /** 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
* of the untranslated file when no translation is required.
*/
-#define SVN_WC_TRANSLATE_FORCE_COPY 0x00000008
+#define SVN_WC_TRANSLATE_FORCE_COPY 0x00000010
/** @} */
@@ -3161,14 +3180,16 @@
/* EOL conversion and keyword expansion. */
-/** 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.
*
- * Caller can explicitly request a new file to be returned by setting the
+ * The caller can explicitly request a new file to be returned by setting the
* @c SVN_WC_TRANSLATE_FORCE_COPY flag in @a flags.
*
* This function is generally used to get a file that can be compared
@@ -3176,14 +3197,13 @@
* @c SVN_WC_TRANSLATE_TO_NF is specified, against @a versioned_file itself
* if @c SVN_WC_TRANSLATE_FROM_NF is specified.
*
- * 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.
*
- * If @c SVN_WC_TRANSLATE_DEL_TEMP_ON_POOL_CLEANUP is specified,
- * a pool cleanup handler is registered on *xlated_p if it points to a
- * temporary file.
+ * If @c SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP is specified, the default
+ * pool cleanup handler to remove @a *xlated_path is not registered.
*
- * If an error is returned, the effect on @a *xlated_p is undefined.
+ * If an error is returned, the effect on @a *xlated_path is undefined.
*
* @since New in 1.4
*/
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 17435)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -75,7 +75,6 @@
(&tmp_target, merge_target,
merge_target, adm_access,
SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
| SVN_WC_TRANSLATE_FORCE_COPY, pool));
/* Open a second temporary file for writing; this is where diff3
Index: subversion/libsvn_wc/diff.c
===================================================================
--- subversion/libsvn_wc/diff.c (revision 17435)
+++ subversion/libsvn_wc/diff.c (working copy)
@@ -565,8 +565,7 @@
SVN_ERR (svn_wc_translated_file2
(&translated, path, path, adm_access,
- SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_TO_NF,
pool));
SVN_ERR (dir_baton->edit_baton->callbacks->file_added
@@ -594,8 +593,7 @@
SVN_ERR (svn_wc_translated_file2
(&translated, path,
path, adm_access,
- SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_TO_NF,
pool));
}
@@ -1223,8 +1221,7 @@
SVN_ERR (svn_wc_translated_file2
(&localfile, b->path,
b->path, adm_access,
- SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP ,
+ SVN_WC_TRANSLATE_TO_NF,
pool));
temp_file_path = b->temp_file_path;
Index: subversion/libsvn_wc/adm_crawler.c
===================================================================
--- subversion/libsvn_wc/adm_crawler.c (revision 17435)
+++ subversion/libsvn_wc/adm_crawler.c (working copy)
@@ -74,7 +74,6 @@
SVN_ERR (svn_wc_translated_file2 (&tmp_file,
text_base_path, file_path, adm_access,
SVN_WC_TRANSLATE_FROM_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
| SVN_WC_TRANSLATE_FORCE_COPY, pool));
SVN_ERR (svn_io_file_rename (tmp_file, file_path, pool));
@@ -747,8 +746,7 @@
the new text base anyway. */
SVN_ERR (svn_wc_translated_file2 (&tmpf, path, path,
adm_access,
- SVN_WC_TRANSLATE_TO_NF
- |
SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,+
SVN_WC_TRANSLATE_TO_NF,
pool));
/* If the translation didn't create a new file then we need an explicit
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c (revision 17435)
+++ subversion/libsvn_wc/log.c (working copy)
@@ -237,7 +237,7 @@
svn_error_clear (err);
}
break;
-
+
case svn_wc__xfer_cp:
return svn_io_copy_file (full_from_path, full_dest_path, FALSE, pool);
@@ -246,14 +246,11 @@
const char *tmp_file;
err = svn_wc_translated_file2
- (&tmp_file,
- full_from_path,
- full_dest_path, adm_access,
- SVN_WC_TRANSLATE_FROM_NF
- | SVN_WC_TRANSLATE_FORCE_COPY
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
- | (special_only ? SVN_WC_TRANSLATE_SPECIAL_ONLY : 0),
- pool);
+ (&tmp_file,
+ full_from_path, full_dest_path, adm_access,
+ SVN_WC_TRANSLATE_FROM_NF
+ | SVN_WC_TRANSLATE_FORCE_COPY,
+ pool);
if (err)
{
if (! rerun || ! APR_STATUS_IS_ENOENT(err->apr_err))
@@ -280,9 +277,7 @@
full_from_path,
full_from_path, adm_access,
SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_FORCE_COPY
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
- | (special_only ? SVN_WC_TRANSLATE_SPECIAL_ONLY : 0),
+ | SVN_WC_TRANSLATE_FORCE_COPY,
pool));
SVN_ERR (svn_io_file_rename (tmp_file, full_dest_path, pool));
@@ -371,7 +366,6 @@
? tmp_text_base : filepath,
filepath, adm_access,
SVN_WC_TRANSLATE_FROM_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP
| SVN_WC_TRANSLATE_FORCE_COPY,
pool));
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 17435)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -2008,7 +2008,8 @@
to a tmp-text-base. */
SVN_ERR (svn_wc_translated_file2 (&tmptext, file_path, file_path,
adm_access,
- SVN_WC_TRANSLATE_TO_NF,
+ SVN_WC_TRANSLATE_TO_NF
+ | SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP,
pool));
tmptext = svn_path_is_child (parent_dir, tmptext, pool);
Index: subversion/libsvn_wc/questions.c
===================================================================
--- subversion/libsvn_wc/questions.c (revision 17435)
+++ subversion/libsvn_wc/questions.c (working copy)
@@ -230,15 +230,13 @@
SVN_ERR (svn_wc_translated_file2
(&tmp_vfile, versioned_file,
versioned_file, adm_access,
- SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_TO_NF,
pool));
else
SVN_ERR (svn_wc_translated_file2
(&tmp_vfile, base_file,
versioned_file, adm_access,
- SVN_WC_TRANSLATE_FROM_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_FROM_NF,
pool));
SVN_ERR (svn_io_files_contents_same_p (&same, tmp_vfile,
@@ -277,15 +275,13 @@
SVN_ERR (svn_wc_translated_file2
(&tmp_vfile, versioned_file,
versioned_file, adm_access,
- SVN_WC_TRANSLATE_TO_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_TO_NF,
pool));
else
SVN_ERR (svn_wc_translated_file2
(&tmp_vfile, base_file,
versioned_file, adm_access,
- SVN_WC_TRANSLATE_FROM_NF
- | SVN_WC_TRANSLATE_DEL_TMP_ON_POOL_CLEANUP,
+ SVN_WC_TRANSLATE_FROM_NF,
pool));
Index: subversion/libsvn_wc/translate.c
===================================================================
--- subversion/libsvn_wc/translate.c (revision 17435)
+++ subversion/libsvn_wc/translate.c (working copy)
@@ -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,
+ 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_DEL_TMP_ON_POOL_CLEANUP)
- ? svn_io_file_del_on_pool_cleanup : svn_io_file_del_none,
+ (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_ERR (svn_subst_translate_to_normal_form
- (src, tmp_vfile, style, eol, keywords, special, pool));
- else /* translate */
- SVN_ERR (svn_subst_copy_and_translate3 (src,
- tmp_vfile,
- eol, FALSE,
- keywords, TRUE,
- special,
- pool));
+ 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;
}
Received on Fri Nov 18 15:43:18 2005