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 gav...@thespidernet.com 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 gav...@thespidernet.com 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 d...@daniel.shahaf.name
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)
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 gav...@thespidernet.com 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 gav...@thespidernet.com 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)
On Sun, Sep 19, 2010 at 9:39 PM, Daniel Shahaf d...@daniel.shahaf.name 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-translated_eol = translated_eol;
b-repair = repair;
b-keywords = keywords;
b-expand = expand;
@@ -884,6 +887,8 @@ translate_chunk(svn_stream_t
Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 2)
Daniel Trebbien dtrebbien at 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)
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)
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)
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)
(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 init, sync, and copy-revprops subcommands.
(opt_baton_t, subcommand_baton_t): Added SOURCE_ENCODING field.
(copy_revprops): Added a parameter to receive the subcommand_baton_t*.
Removed the QUIET parameter as this can be found from the subcommand
baton.
(make_subcommand_baton): Set the SOURCE_ENCODING field of the resulting
subcommand_baton_t object to the value of SOURCE_ENCODING from
opt_baton_t.
(do_initialize
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.
