Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
On Mon, Oct 18, 2010 at 3:05 AM, Gavin Beau Baumanis wrote: > Hi Daniel, > > This is just a friendly poke. > Are you happy to have me "annoy" you every so often - or would just like me > leave it entirely with you ? > > > Gavin "Beau" Baumanis Hi Gavin, I don't mind at all. Regarding this svnsync patch, I am waiting for "[PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed (v. 2)" (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/123020/) to be committed because this patch depends upon that one. Not to worry, I am still interested in making the previously-discussed enhancements to svnsync. Daniel
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Hi Daniel, This is just a friendly poke. Are you happy to have me "annoy" you every so often - or would just like me leave it entirely with you ? Gavin "Beau" Baumanis On 29/09/2010, at 11:14 AM, Daniel Trebbien wrote: > Hi Gavin, > > There have been some things that I have needed to do recently, so I > haven't been working on the patches as much as I would like. But, I > am still interested in finishing them. > > My apologies for the delay... > > Daniel > > > On Tue, Sep 28, 2010 at 6:07 PM, Gavin Beau Baumanis > wrote: >> Hi Daniel (Trebbien), >> >> Just thought I would ask for an update, please. >> I still have you on my "watch - list" - so there is no rush - just making >> sure for my own sanity that I haven't missed an email / thread about your >> patch submission. >> >> >> Gavin "Beau" Baumanis
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Daniel, Absolutely no apology required. Just about everyone here is a volunteer of the own spare time. I was checking for my own purposes, in case I had missed an alternate message thread. Gavin "Beau" Baumanis On 29/09/2010, at 11:14 AM, Daniel Trebbien wrote: > Hi Gavin, > > There have been some things that I have needed to do recently, so I > haven't been working on the patches as much as I would like. But, I > am still interested in finishing them. > > My apologies for the delay... > > Daniel > > > On Tue, Sep 28, 2010 at 6:07 PM, Gavin Beau Baumanis > wrote: >> Hi Daniel (Trebbien), >> >> Just thought I would ask for an update, please. >> I still have you on my "watch - list" - so there is no rush - just making >> sure for my own sanity that I haven't missed an email / thread about your >> patch submission. >> >> >> Gavin "Beau" Baumanis
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Hi Gavin, There have been some things that I have needed to do recently, so I haven't been working on the patches as much as I would like. But, I am still interested in finishing them. My apologies for the delay... Daniel On Tue, Sep 28, 2010 at 6:07 PM, Gavin Beau Baumanis wrote: > Hi Daniel (Trebbien), > > Just thought I would ask for an update, please. > I still have you on my "watch - list" - so there is no rush - just making > sure for my own sanity that I haven't missed an email / thread about your > patch submission. > > > Gavin "Beau" Baumanis
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Hi Daniel (Trebbien),
Just thought I would ask for an update, please.
I still have you on my "watch - list" - so there is no rush - just making sure
for my own sanity that I haven't missed an email / thread about your patch
submission.
Gavin "Beau" Baumanis
On 22/09/2010, at 2:56 AM, Daniel Shahaf wrote:
> Daniel Trebbien wrote on Tue, Sep 21, 2010 at 08:12:38 -0700:
>> On Sun, Sep 19, 2010 at 9:39 PM, Daniel Shahaf
>> wrote:
>>> Need the word "to" after the semicolon for the English to be correct.
>>
>> Fixed. (When I say "fixed", I mean that I committed a fix in my own
>> fork of Subversion: http://github.com/dtrebbien/subversion )
>>
>
> Fine, but please post patches and log messages to this list as usual.
> It's easier for me/us to process them that way. Thanks.
>
+svn_error_t *
+svn_subst_translate_cstring2(const char *src,
+ const char **dst,
+ const char *eol_str,
+ svn_boolean_t repair,
+ apr_hash_t *keywords,
+ svn_boolean_t expand,
+ apr_pool_t *pool)
+{
+ return translate_cstring2(dst, NULL, src, eol_str, repair, keywords,
expand,
+pool);
+}
>>>
>>> So, this is revved because svn_subst_translate_string2() needs it (and
>>> svn_subst_translate_string() doesn't).
>>
>> I am unsure of what you mean.
>>
>> I took the original implementation of `svn_subst_translate_cstring2`
>> and placed it in the new static utility function `translate_cstring2`.
>> `translate_cstring2` accepts another parameter, and its definition
>> (the old definition on `svn_subst_translate_cstring2`) was
>> appropriately modified to handle this new parameter. This way, I don't
>> have to modify the public API.
>>
>
> Yes, yes. You revved three svn_subst_* functions; one publicly and two
> privately. So I was pointing out (to myself), on each of the latter
> two, where is the call site that needs the additional functionality.
>
>>> I suggest you go ahead (respond to the review, submit an updated patch,
>>> etc) without waiting; the svnsync part will eventually get its share of
>>> reviews. Also, you may want to consider splitting this patch into two
>>> parts (subst work separate from svnsync work).
>>
>> I like the idea of splitting this into two patches.
>
> Okay. Please post an updated patch to this list when you have it ready :-)
>
> Best,
>
> Daniel
>
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Daniel Trebbien wrote on Tue, Sep 21, 2010 at 08:12:38 -0700:
> On Sun, Sep 19, 2010 at 9:39 PM, Daniel Shahaf
> wrote:
> > Need the word "to" after the semicolon for the English to be correct.
>
> Fixed. (When I say "fixed", I mean that I committed a fix in my own
> fork of Subversion: http://github.com/dtrebbien/subversion )
>
Fine, but please post patches and log messages to this list as usual.
It's easier for me/us to process them that way. Thanks.
> >> +svn_error_t *
> >> +svn_subst_translate_cstring2(const char *src,
> >> + const char **dst,
> >> + const char *eol_str,
> >> + svn_boolean_t repair,
> >> + apr_hash_t *keywords,
> >> + svn_boolean_t expand,
> >> + apr_pool_t *pool)
> >> +{
> >> + return translate_cstring2(dst, NULL, src, eol_str, repair, keywords,
> >> expand,
> >> + pool);
> >> +}
> >
> > So, this is revved because svn_subst_translate_string2() needs it (and
> > svn_subst_translate_string() doesn't).
>
> I am unsure of what you mean.
>
> I took the original implementation of `svn_subst_translate_cstring2`
> and placed it in the new static utility function `translate_cstring2`.
> `translate_cstring2` accepts another parameter, and its definition
> (the old definition on `svn_subst_translate_cstring2`) was
> appropriately modified to handle this new parameter. This way, I don't
> have to modify the public API.
>
Yes, yes. You revved three svn_subst_* functions; one publicly and two
privately. So I was pointing out (to myself), on each of the latter
two, where is the call site that needs the additional functionality.
> > I suggest you go ahead (respond to the review, submit an updated patch,
> > etc) without waiting; the svnsync part will eventually get its share of
> > reviews. Also, you may want to consider splitting this patch into two
> > parts (subst work separate from svnsync work).
>
> I like the idea of splitting this into two patches.
Okay. Please post an updated patch to this list when you have it ready :-)
Best,
Daniel
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Daniel Trebbien gmail.com> writes: > Yes, I did copy-and-paste. I see your point, though, and I am going to > correct this by adding another `static` function that both will call. Never mind the talk of a new static function. `svn_subst_translate_string` should of course call `svn_subst_translate_string2` :)
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Daniel Shahaf daniel.shahaf.name> writes: > > Gavin, thanks for the information, but I think Daniel Trebbien already > knows this. > > Gavin Beau Baumanis wrote on Sun, Sep 19, 2010 at 08:52:05 +1000: > > Hi Daniel, > > > > You should always feel comfortable with "pimping" your own patch > > submissions. > > What you have done is completely fine. > > > > But just in case you don't know - the Subversion project does have a Patch Manager. > > The role of the Patch Manager is to ensure that patch submissions (mostly) by people who don;t have commit > rights to the svn repository don't get forgotten. > > > > Normally, I wait about six or seven days prior to "pinging" the mailing list about a patch that has not > received any review. > > > > It can take a little while to get a patch reviewed because of a few reasons that I am sure you can appreciate. > > * The reviewer needs to have a solid understanding of the subject matter. Some people have appropriate > knowledge of only some parts of the code base. > > * People are busy doing their "real" job / family. > > * People are busy working on their own space of the SVN project. > > > > Let me re-iterate that there is absolutely nothing wrong with you reminding us all about your own patch > submission. > > > > The ultimate aim of this email is that any and all patch submissions are ALWAYS gratefully appreciated by > the SVN project / community. > > Sometimes it just takes a little while to get a patch appropriately reviewed / committed. > > > > > > Gavin "Beau" Baumanis I read http://subversion.apache.org/docs/community-guide/general.html#patches , but I think that it is good to know the preferred number of days to wait before reposting. Gavin, I appreciated your response.
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
On Sun, Sep 19, 2010 at 9:39 PM, Daniel Shahaf wrote:
>> * subversion/include/svn_subst.h
>> Added documentation of the new public API function
>> svn_subst_translate_string2().
>>
>
> Should use this syntax:
>
> * subversion/include/svn_subst.h
> (svn_subst_translate_string2): New function.
Okay.
>> * subversion/libsvn_subr/subst.c
>> (translation_baton): Added TRANSLATED_EOL field. It's where
>> translate_chunk()
>> records that it translated a line ending.
>> (create_translation_baton): Added a new parameter TRANSLATED_EOL.
>> Initialize
>> the TRANSLATED_EOL field of the resulting translation_baton to this
>> value.
>> (translate_chunk): After translating a line ending, set the value that is
>> pointed to by TRANSLATED_EOL (from the translation baton) to TRUE.
>
> Could just say "Record that an EOL translation has been done" or similar.
>
> In general, the log message looks good, but to my taste it is borders
> the 'too verbose a log message' line. (I might have written the same
> without the last sentence of each bullet.) Remember that the log
> message must never /repeat/ the code (or in-code comments); rather, it
> should describe them from a high level.
Okay. I will attempt to make the log message less verbose.
>> Index: subversion/include/svn_subst.h
>> ===
>> --- subversion/include/svn_subst.h (revision 996730)
>> +++ subversion/include/svn_subst.h (working copy)
>> @@ -598,6 +598,26 @@ svn_error_t *svn_subst_translate_string(svn_string
>> const char *encoding,
>> apr_pool_t *pool);
>>
>> +/** Translate the data in @a value (assumed to be in encoded in charset
>> + * @a encoding) to UTF8 and LF line-endings. If @a encoding is @c NULL,
>> + * then assume that @a value is in the system-default language encoding.
>> + * If @p translated_to_utf8 is not @c NULL, then
>> + * @code *translated_to_utf8 @endcode is set to @c TRUE if at least one
>
> You use @a, @p, and @code. Existing code uses only @a. Why the
> difference?
Oops. @p should be @a.
I believe that in Doxygen, @c WORD is short for @code WORD @endode.
>> + * character of @p value in the source charset was translated to UTF-8;
>
> Need the word "to" after the semicolon for the English to be correct.
Fixed. (When I say "fixed", I mean that I committed a fix in my own
fork of Subversion: http://github.com/dtrebbien/subversion )
>> + * @c FALSE otherwise. If @p translated_line_endings is not @c NULL, then
>> + * @code *translated_line_endings @endcode is set to @c TRUE if at least one
>> + * line ending was changed to LF; �...@c FALSE otherwise. Return the
>> translated data in @a *new_value,
>> + * allocated in @a pool.
>> + *
>
> Need to rewrap.
Fixed.
>> + * @since New in 1.6
>
> s/1.6/1.7/
Fixed.
>> + */
>> +svn_error_t *svn_subst_translate_string2(svn_string_t **new_value,
>
> I realize that you followed the precedent of svn_subst_translate_string(),
> but please:
>
> Return type on own line, function name on the next line.
Fixed.
>> + svn_boolean_t *translated_to_utf8,
>> + svn_boolean_t
>> *translated_line_endings,
>> + const svn_string_t *value,
>> + const char *encoding,
>> + apr_pool_t *pool);
>> +
>> /** Translate the data in @a value from UTF8 and LF line-endings into
>
> Need to update svn_subst_translate_string()'s docstring and
> implementation and mark it deprecated:
> http://subversion.apache.org/docs/community-guide/community-guide.html#deprecation
> http://mid.gmane.org/20100814132023.089a22388...@eris.apache.org
Okay.
>> * native locale and native line-endings, or to the output locale if
>> * @a for_output is TRUE. Return the translated data in @a
>
>> Index: subversion/libsvn_subr/subst.c
>> ===
>> --- subversion/libsvn_subr/subst.c (revision 996730)
>> +++ subversion/libsvn_subr/subst.c (working copy)
>> @@ -765,6 +765,7 @@ svn_subst_keywords_differ2(apr_hash_t *a,
>> struct translation_baton
>> {
>> const char *eol_str;
>> + svn_boolean_t *translated_eol;
>> svn_boolean_t repair;
>> apr_hash_t *keywords;
>> svn_boolean_t expand;
>> @@ -805,6 +806,7 @@ struct translation_baton
>> */
>> static struct translation_baton *
>> create_translation_baton(const char *eol_str,
>> + svn_boolean_t *translated_eol,
>> svn_boolean_t repair,
>> apr_hash_t *keywords,
>> svn_boolean_t expand,
>> @@ -818,6 +820,7 @@ create_translation_baton(const char *eol_str,
>>
>> b->eol_str = eol_str;
>> b->eol_str_len = eol_str ? strlen(eol_str) : 0;
>> + b
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
(to stsp and other interested parties: I reviewed only the libsvn_subr
part of the patch; see last paragraph for details)
Daniel Trebbien wrote on Mon, Sep 13, 2010 at 19:22:38 -0700:
> Attached are the new patch and log message.
>
> I have already shared one test repository with this mailing list that
> is useful in testing that revprops are re-encoded properly. Since
> then, I have created two more: test2 is useful in testing that
> directory properties (in this case `svn:ignore`) are re-encoded, and
> test3 is useful in testing that line endings are normalized to
> Unix-style in revprops (and that the "# normalized" message is still
> there). Archives of these new repositories have also been attached.
> Also, this shell script makes testing with these repositories *much*
> easier: http://pastebin.com/gz1pmngc
>
>
> #! /usr/bin/env sh
> TEST=test3 # change this line to test with a different repository
> rm -Rf ${TEST}_mirror
> svnadmin create ${TEST}_mirror
> cat <<'EOF' > ${TEST}_mirror/hooks/pre-revprop-change
> #! /usr/bin/env sh
> USER="$3"
>
> if [ "$USER" = "svnsync" ]; then exit 0; fi
>
> echo "Only the svnsync user can change revprops" >&2
> exit 1
> EOF
> chmod +x ${TEST}_mirror/hooks/pre-revprop-change
> svnsync init \
> --username svnsync --source-encoding ISO-8859-1 \
> file://`pwd`/${TEST}_mirror file://`pwd`/${TEST}
> svnsync sync \
> --username svnsync --source-encoding ISO-8859-1 \
> file://`pwd`/${TEST}_mirror
> Subject: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8
> (v. 2)
>
> [[[
> 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.
>
Could you mention in here (at the paragraphs beginning the log message)
that you're adding a new API, too? I think it should be mentioned since
that forms a significant part of the change.
> As discussed at:
> http://thread.gmane.org/gmane.comp.version-control.subversion.user/100020
> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122518
>
> * subversion/include/svn_subst.h
> Added documentation of the new public API function
> svn_subst_translate_string2().
>
Should use this syntax:
* subversion/include/svn_subst.h
(svn_subst_translate_string2): New function.
(and mention svn_subst_translate_string() too, per below review comments)
> * subversion/libsvn_subr/subst.c
> (translation_baton): Added TRANSLATED_EOL field. It's where
> translate_chunk()
> records that it translated a line ending.
> (create_translation_baton): Added a new parameter TRANSLATED_EOL. Initialize
> the TRANSLATED_EOL field of the resulting translation_baton to this value.
> (translate_chunk): After translating a line ending, set the value that is
> pointed to by TRANSLATED_EOL (from the translation baton) to TRUE.
Could just say "Record that an EOL translation has been done" or similar.
In general, the log message looks good, but to my taste it is borders
the 'too verbose a log message' line. (I might have written the same
without the last sentence of each bullet.) Remember that the log
message must never /repeat/ the code (or in-code comments); rather, it
should describe them from a high level.
> (stream_translated): New static function. Its implementation is the old
> implementation of svn_subst_stream_translated(), but accepting another
> parameter, TRANSLATED_EOL, that is passed to the in/out translation batons
> that it creates.
> (svn_subst_stream_translated): Now a wrapper for stream_translated().
> (translate_cstring2): New static function. Its implementation is the old
> implementation of svn_subst_translate_cstring2(), but modified to accept
> another parameter, TRANSLATED_EOL, that is passed to stream_translated().
> (svn_subst_translate_cstring2): Now a wrapper for translate_cstring2().
So, you had to "internally" revv both svn_subst_stream_translated() and
svn_subst_translate_cstring2(). I wonder why...
(self-answered hereinbelow)
> (svn_subst_translate_string2): New function. It does what
> svn_subst_translate_string() does, but also records whether it
> translated a line ending or performed re-encoding.
>
> * subversion/svnsync/main.c
> (svnsync__opt) Added svnsync_opt_source_encoding.
> (svnsync_cmd_table): Added svnsync_opt_source_encoding to the list of
> acceptable options for the in
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
On Sun, Sep 19, 2010 at 11:14:02AM +0100, Daniel Shahaf wrote: > Thanks for the reminder. I've been at Hursley this weekend (and haven't > had much time for coding!), but it's on my list. Same here (at Hursely and interested in the patch). Feel free to prod me about it if you don't hear from anyone else in a while. I'll try to make time to look at it. Stefan
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Gavin, thanks for the information, but I think Daniel Trebbien already knows this. :-) Gavin Beau Baumanis wrote on Sun, Sep 19, 2010 at 08:52:05 +1000: > Hi Daniel, > > You should always feel comfortable with "pimping" your own patch submissions. > What you have done is completely fine. > > But just in case you don't know - the Subversion project does have a Patch > Manager. > The role of the Patch Manager is to ensure that patch submissions (mostly) by > people who don;t have commit rights to the svn repository don't get forgotten. > > Normally, I wait about six or seven days prior to "pinging" the mailing list > about a patch that has not received any review. > > It can take a little while to get a patch reviewed because of a few reasons > that I am sure you can appreciate. > * The reviewer needs to have a solid understanding of the subject matter. > Some people have appropriate knowledge of only some parts of the code base. > * People are busy doing their "real" job / family. > * People are busy working on their own space of the SVN project. > > Let me re-iterate that there is absolutely nothing wrong with you reminding > us all about your own patch submission. > > The ultimate aim of this email is that any and all patch submissions are > ALWAYS gratefully appreciated by the SVN project / community. > Sometimes it just takes a little while to get a patch appropriately reviewed > / committed. > > > Gavin "Beau" Baumanis > > > > On 19/09/2010, at 7:33 AM, Daniel Trebbien wrote: > > > It has been a few days since I submitted my revised patch > > (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122550), > > and I just want to make sure that it hasn't been forgotten. I am waiting for > > the changes to be committed before I work on related enhancements. > > >
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Thanks for the reminder. I've been at Hursley this weekend (and haven't had much time for coding!), but it's on my list. Daniel Trebbien wrote on Sat, Sep 18, 2010 at 21:33:07 +: > It has been a few days since I submitted my revised patch > (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122550), > and I just want to make sure that it hasn't been forgotten. I am waiting for > the changes to be committed before I work on related enhancements. >
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Hi Daniel, You should always feel comfortable with "pimping" your own patch submissions. What you have done is completely fine. But just in case you don't know - the Subversion project does have a Patch Manager. The role of the Patch Manager is to ensure that patch submissions (mostly) by people who don;t have commit rights to the svn repository don't get forgotten. Normally, I wait about six or seven days prior to "pinging" the mailing list about a patch that has not received any review. It can take a little while to get a patch reviewed because of a few reasons that I am sure you can appreciate. * The reviewer needs to have a solid understanding of the subject matter. Some people have appropriate knowledge of only some parts of the code base. * People are busy doing their "real" job / family. * People are busy working on their own space of the SVN project. Let me re-iterate that there is absolutely nothing wrong with you reminding us all about your own patch submission. The ultimate aim of this email is that any and all patch submissions are ALWAYS gratefully appreciated by the SVN project / community. Sometimes it just takes a little while to get a patch appropriately reviewed / committed. Gavin "Beau" Baumanis On 19/09/2010, at 7:33 AM, Daniel Trebbien wrote: > It has been a few days since I submitted my revised patch > (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122550), > and I just want to make sure that it hasn't been forgotten. I am waiting for > the changes to be committed before I work on related enhancements. >
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
It has been a few days since I submitted my revised patch (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122550), and I just want to make sure that it hasn't been forgotten. I am waiting for the changes to be committed before I work on related enhancements.
