On Sun, 2011-01-02, Daniel Shahaf wrote:
> Danny Trebbien wrote on Sun, Dec 26, 2010 at 15:39:05 -0800:
> > Attached are version 3 of the patch and corresponding log message.
> > Also attached is a TGZ archive of two Subversion repository dumps that
> > are used by a new test case.
> > [[[
> > Add a command line option (--source-encoding) to the svnsync init, sync, and
> > copy-revprops subcommands that allows the user to specify the character
> > encoding of translatable properties from the source repository. This is needed
> > to allow svnsync to sync some older Subversion repositories that have
> > properties that were not encoded in UTF-8.
[...]
+1 to what danielsh said.
Seems to me it would be good for the name of the command-line option to
include "property", e.g. "--prop-source-encoding". I note that this
option is not intended to be used frequently so I don't imagine it's a
problem to have a long name.
> > Index: subversion/svnsync/sync.h
> > ===================================================================
> > svnsync_normalize_revprops(apr_hash_t *rev_props,
> > - int *normalized_count,
> > + int *normalized_le_count,
> > + const char *encoding,
> > apr_pool_t *pool);
> > svnsync_get_sync_editor(const svn_delta_editor_t *wrapped_editor,
> > void *wrapped_edit_baton,
> > svn_revnum_t base_revision,
> > const char *to_url,
> > + const char *prop_encoding,
> > svn_boolean_t quiet,
> > const svn_delta_editor_t **editor,
> > void **edit_baton,
> > - int *normalized_node_props_counter,
> > + int *le_normalized_node_props_counter,
Can you use the same names for the counter parameters? (I realize one
is an output and the other is an in-out, but that doesn't appear to be
the reason why they differ.) Also we normally use "EOL" for "end of
line"; I haven't noticed the use of "LE" before.
Not directly related to this change, but ...
> - int *normalized_node_props_counter; /* Where to count normalizations? */
> + int *le_normalized_node_props_counter; /* Where to count line ending
> + normalizations? */
> } edit_baton_t;
>
> @@ -407,9 +425,10 @@ change_file_prop(void *file_baton,
> if (svn_prop_needs_translation(name))
> {
> svn_boolean_t was_normalized;
It would be a good idea to rename this variable also, otherwise this is
a bit confusing.
> - SVN_ERR(normalize_string(&value, &was_normalized, pool));
> + SVN_ERR(normalize_string(&value, &was_normalized, eb->prop_encoding,
> + pool, pool));
> if (was_normalized)
> - (*(eb->normalized_node_props_counter))++;
> + (*(eb->le_normalized_node_props_counter))++;
> }
This kind of code can be simplified (removing one level of indirection)
by making the baton hold the actual counter instead of a pointer to it.
- Julian
Received on 2011-01-05 11:43:13 CET