Re: [PATCH] Don't strip Content-Type in .po files on Windows

[Роман Донченко]

Well, I could, but what would be the point?

Daniel Shahaf  писал в своём письме Mon, 13 Aug  
2012 09:54:14 +0400:



If nothing else, feel free to at least create a feature branch off of
trunk and commit the patch there.

http://subversion.apache.org/docs/community-guide/general.html#lightweight-branches

Gavin Baumanis wrote on Mon, Aug 13, 2012 at 11:06:48 +1000:

Ping.
This message has received no comments.



-Original Message-
From: Роман Донченко [mailto:d...@corrigendum.ru]
Sent: Monday, 6 August 2012 09:19
To: dev@subversion.apache.org
Subject: [PATCH] Don't strip Content-Type in .po files on Windows

Greetings,

I submitted this patch a couple of years ago, but it didn't get much  
attention. I still think it's relevant, so I'll try again.


[[[
On Windows, don't strip the Content-Type field from .po files during  
their compilation.

Re: svn delete fails with "403 Forbidden" if root is not readable

[Julian Foad]

Dmitry Pavlenko wrote:

> I'd like to ping my old report. I'll recollect: if there's no read 
> permission of the repository 
> root, "svn delete" for sub-URL always fails.

Hi Dmitry.

Thanks for the report.

I tried to reproduce this, and I cannot.  The tests I wrote are in the attached 
patch.

I tested with RA-serf (with FSFS and with BDB), on an Ubuntu Linux system.  All 
the tests pass.

Can you try this test, or do you have any idea what I need to do to reproduce 
the problem?

- Julian


> I had the same issue in SVNKit but fixed, and here's the fix:
> 
> svn log --diff -r9286 http://svn.svnkit.com/repos/svnkit/trunk/ 
> 
> I also tried to apply the same changes to Subversion,but
> it's not very comfortable for me to debug because of compilation problems (I 
> have to work with SVN 
> sources under chroot).
> 
> There's a patch with the changes I tried to perform. Important: it DOES NOT 
> work (FAILS with 
> segmentation fault --- maybe I've confused something with pools, sorry I 
> didn't debug) 
> but shows where and how to fix the problem. The main idea:
> 
> if we have several targets to delete: a/b/c/d, a/b/e/f, a/b/c/h, it's better 
> to extract common 
> parent "a/b" and set ra_session URL to    
> original_URL.append("a/b")    and walk using editor 
> through "c/d", "e/f", "c/h" and so on. (Though 
> this maybe won't work for single file URL...)
> 
> Anyway, I believe my report was useful, at least a bit!
> 
> [[[
> DO NOT apply this patch directly, it doesn't work!
> ]]]
> [[[
> Index: subversion/libsvn_client/delete.c
> ===
> --- subversion/libsvn_client/delete.c  (revision 1360990)
> +++ subversion/libsvn_client/delete.c  (working copy)
> @@ -233,6 +233,7 @@ delete_urls_multi_repos(const apr_array_header_t *
>   apr_hash_t *relpaths = apr_hash_make(pool);
>   apr_hash_index_t *hi;
>   int i;
> +  apr_pool_t* iterpool;
> 
>   /* Create a hash of repos_root -> ra_session maps and repos_root -> 
> relpaths
>       maps, used to group the various targets. */
> @@ -303,6 +304,8 @@ delete_urls_multi_repos(const apr_array_header_t *
> 
>       Now we iterate over the collection of sessions and do a commit for each
>       one with the collected relpaths. */
> +
> +  iterpool = svn_pool_create(pool);
>   for (hi = apr_hash_first(pool, sessions); hi; hi = apr_hash_next(hi))
>     {
>       const char *repos_root = svn__apr_hash_index_key(hi);
> @@ -310,10 +313,21 @@ delete_urls_multi_repos(const apr_array_header_t *
>       const apr_array_header_t *relpaths_list =
>         apr_hash_get(relpaths, repos_root, APR_HASH_KEY_STRING);
> 
> -      SVN_ERR(single_repos_delete(ra_session, repos_root, relpaths_list,
> +      const char* common_path;
> +      apr_array_header_t* condensed_targets;
> +      const char* url = repos_root;
> +
> +      svn_pool_clear(iterpool);
> +      SVN_ERR(svn_uri_condense_targets(&common_path, 
> &condensed_targets, relpaths_list, FALSE, 
> iterpool, iterpool));
> +      url = svn_path_url_add_component2(repos_root, common_path, pool);
> +
> +      SVN_ERR(svn_ra_reparent(ra_session, url, pool));
> +
> +      SVN_ERR(single_repos_delete(ra_session, url, condensed_targets,
>                                   revprop_table, commit_callback,
>                                   commit_baton, ctx, pool));
>     }
> +  svn_pool_destroy(iterpool);
> 
>   return SVN_NO_ERROR;
> }
> ]]]
> 
> --
> Dmitry Pavlenko,
> TMate Software,
> http://subgit.com/ - git+svn on the server side
> 
> 
> В сообщении от 18 June 2012 19:20:19 автор Dmitry Pavlenko написал:
>>  Hello again,
>> 
>>  I've fixed the issue in SVNKit: 
> http://svn.svnkit.com/repos/svnkit/trunk/
>>  at r9286 The approach is to use common ancestor instead of repository root
>>  to run editor calls. For example, to remove paths:
>> 
>>  directory/subdirectory1/file1
>>  directory/subdirectory1/file2
>>  directory/subdirectory2/file3
>> 
>>  instead of running
>> 
>>  openRoot();
>>  opedDir("directory")
>> 
>>  opedDir("directory/subdirectory1")
>>  delete("directory/subdirectory1/file1")
>>  delete("directory/subdirectory1/file2")
>>  closeDir()
>> 
>>  opedDir("directory/subdirectory2")
>>  delete("directory/subdirectory1/file3")
>>  closeDir()
>> 
>>  closeDir()
>>  closeDir()
>> 
>>  on http://host/path as on the root one can run
>> 
>>  opedRoot()
>> 
>>  opedDir("subdirectory1")
>>  delete("subdirectory1/file1")
>>  delete("subdirectory1/file2")
>>  closeDir()
>> 
>>  opedDir("subdirectory2")
>>  delete("subdirectory1/file3")
>>  closeDir()
>> 
>>  closeDir()
>> 
>>  http://host/path/directory as on the common ancestor.
>> 
>>  > Hello,
>>  > Suppose you have a repository with authz:
>>  > 
>>  > [/]
>>  > * =
>>  > [/directory]
>>  > * = rw
>>  > 
>>  > And the repository (http://localhost:43714/repos) contains a directory
>>  > (with "rw" access) and a file in it. File deletion fails 
> with the
>>  > following stacktrace (tried with SVN r1350986):
>

Re: [PATCH] Don't strip Content-Type in .po files on Windows

[Julian Foad]

Philip Martin wrote:
> Роман Донченко  writes:
>>  - On Windows, we only support linking with svn-win32-libintl, which
>>  is hacked to disable all encoding conversions.
> 
> I assume it provides bind_textdomain_codeset as a function that does
> nothing.
> 
>>  - Even if someone links with with his own version of libintl, it's
>>  a safe bet that it will be new enough to support
>>  bind_textdomain_codeset, so we  can just call that.
> 
> If somebody managed to use an old version I guess there would be some
> sort of linker failure.  Build time?  Run time?
> 
> If somebody managed to use their own version that did encoding
> conversions I guess that would also be OK since it would be a
> utf8-utf8 no-op.
> 
> The patch looks OK but I don't know enough about the Windows build to
> know whether the above is correct.

I agree, without fully understanding all the details.

I went digging in the archives to try to understand the issue.  Maybe 
this will help others to do so too.  The email threads around the time 
when this 'charset stripping' was originally added are "RFC: Solving 
gettext & UTF-8 brokenness" [1] and "Stripping 'charset=' from po files 
[the sequal]"[2].  Roman's previous submission of this patch resulted 
in a short discussion that petered out [3].

From what I can gather, the stripping of the 'charset=' was originally 
introduced as a hack to work around a mis-feature of the 'gettext' or 
'libintl'/'libiconv' subsystems, that would undesirably change the 
encoding away from UTF-8.  We have two other ways to stop that 
re-coding: we call bind_textdomain_codeset() if available; and/or we 
link to a 'libintl'/'libiconv' implementation that doesn't do 
re-coding.  On Windows we do the latter; we don't currently do the 
former, but Roman's patch does.

It appears that Roman finds that that those other methods are 
sufficient.  Removing this hack makes his life better (eliminates some 
warnings from some translation tools, and simplifies the build system) 
and still provides the desired UTF-8 result.

The worst that could happen after applying this patch is someone 
building Subversion on Windows, and linking an incompatible 
'libintl'/'libiconv', might find that Subversion error messages are 
improperly displayed or are replaced by an 'Invalid UTF-8 sequence' 
message.  That's no big deal -- it's a straightforward issue for the 
builder/maintainer to solve and is not subtle or data-corrupting.

The .po file format is supposed to have this 'charset' field present, 
so in my opinion we shouldn't strip it if we don't have to: I could 
imagine how stripping it could cause extra warnings in translation 
tools and/or extra work for translators operating those tools, even 
if we hadn't actually had this report and this patch.

So it seems to me that we should apply this patch.

Brane was involved in the previous discussions and he's away today 
so I'd like to wait to hear his input.  If he has no particular 
objection then I'll be happy to commit this even though I can't 
easily test it.

- Julian


[1] Email thread "RFC: Solving gettext & UTF-8 brokenness" on dev@ 
in May 2004: 

or .

[2] Email thread "Stripping 'charset=' from po files [the sequal]" on 
dev@ in May 2004; a relevant place to start reading is the message 
from Greg Hudson on 2004-05-13: 

or .

[3] Email thread "Re: [PATCH] Remove po file charset stripping on 
Windows" on dev@ in Aug/Sep 2009; a relevant place to start reading 
is Roman's message on 2009-09-13: 

or .


Roman Donchenko (Роман Донченко) wrote on 6 August 2012:
> I submitted this patch a couple of years ago, but it didn't get much 
> attention. I still think it's relevant, so I'll try again.
> 
> [[[
> On Windows, don't strip the Content-Type field from .po files during 
> their compilation.
> 
> GNU libintl, by default, converts the l10n strings into the locale
> encoding, while Subversion requires UTF-8. This conversion can be
> suppressed by calling bind_textdomain_codeset, but certain old
> versions of libintl don't have that, so the Unix build system checks
> for the existence of that function, and if it's not present, strips
> the Content-Type header from the .po files (which prevents encoding
> conversion, as well, but makes msgfmt complain).
> 
> When building on Windows, this stripping is done unconditionally,
> but is completely unnecessary:
> 
> - On Windows, we only support linking with svn-win32-libintl, which
> is hacked to disable all encoding conversions.
> - Even if someone links with with his own version of libintl, it's a
> safe bet that it will be new enough to 

Re: Is there a way to post a comment to a subversion issue without agreeing to colabnet terms?

[Hyrum K Wright]

On Sat, Aug 11, 2012 at 2:11 AM, Greg Stein  wrote:
> To be fair, Apache Bloodhound (incubating) is just one option. We
> could preserve issue numbers with a migration to Bugzilla, Jira, or
> Google Code. (not sure if other options have been raised as options,
> in the past)
>
> My leanings are towards Google Code's tracker, but I like the idea
> that Apache Bloodhound could integrate with our version control (where
> GC never could).
>
> The migration tool is an important point. We're still on IssueZilla at
> tigris, primarily because we don't have a migration tool.

And the migration tool will probably have to be hand-rolled, iirc.

-Hyrum

Re: [PATCH] Don't strip Content-Type in .po files on Windows

[Philip Martin]

Роман Донченко  writes:

> - On Windows, we only support linking with svn-win32-libintl, which is
> hacked to disable all encoding conversions.

I assume it provides bind_textdomain_codeset as a function that does
nothing.

> - Even if someone links with with his own version of libintl, it's a
> safe bet that it will be new enough to support
> bind_textdomain_codeset, so we  can just call that.

If somebody managed to use an old version I guess there would be some sort of
linker failure.  Build time?  Run time?

If somebody managed to use their own version that did encoding
conversions I guess that would also be OK since it would be a utf8-utf8
no-op.

The patch looks OK but I don't know enough about the Windows build to
know whether the above is correct.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: [PATCH] Don't strip Content-Type in .po files on Windows

[Daniel Shahaf]

Making it visible, and easier to maintain going forward.

Also it's possibly easier to tell persuade someone to "install " than to "".

Роман Донченко wrote on Tue, Aug 14, 2012 at 00:51:28 +0400:
> Well, I could, but what would be the point?
> 
> Daniel Shahaf  писал в своём письме Mon, 13
> Aug 2012 09:54:14 +0400:
> 
> >If nothing else, feel free to at least create a feature branch off of
> >trunk and commit the patch there.
> >
> >http://subversion.apache.org/docs/community-guide/general.html#lightweight-branches
> >
> >Gavin Baumanis wrote on Mon, Aug 13, 2012 at 11:06:48 +1000:
> >>Ping.
> >>This message has received no comments.
> >>
> >>
> >>
> >>-Original Message-
> >>From: Роман Донченко [mailto:d...@corrigendum.ru]
> >>Sent: Monday, 6 August 2012 09:19
> >>To: dev@subversion.apache.org
> >>Subject: [PATCH] Don't strip Content-Type in .po files on Windows
> >>
> >>Greetings,
> >>
> >>I submitted this patch a couple of years ago, but it didn't get
> >>much attention. I still think it's relevant, so I'll try again.
> >>
> >>[[[
> >>On Windows, don't strip the Content-Type field from .po files
> >>during their compilation.

Re: svn commit: r1372665 - /subversion/branches/master-passphrase/subversion/libsvn_auth_kwallet/kwallet.cpp

[Philip Martin]

Blair Zajac  writes:

> I think it's better to use C++ style casts in a C++ body than C style

Agreed, r1372753.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download