Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 3)

> 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.

How about "--source-prop-encoding"?

>> > 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.

Okay.

Note that I will be reverting the renames for now, but the variables
will have the same name in a different patch to rename them.

> 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.

I agree.

>> -      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.

Okay.
Received on 2011-01-07 22:09:59 CET