Re: 1.6.23 up for signing/testing

[Stefan Sperling]

On Wed, May 22, 2013 at 06:08:59PM -0700, Ben Reser wrote:
> The 1.6.22 release artifacts are now available for testing/signing.
> Please get the tarballs from
>   https://dist.apache.org/repos/dist/dev/subversion

Thanks Ben! I did consider rerolling 1.6.22 but didn't have enough
time to do so last night. It's great that you jumped in.

(I'm travelling today, will prepare sigs tomorrow at the earliest.)

Re: svn commit: r1485506 - /subversion/branches/1.6.x/CHANGES

[Stefan Sperling]

On Wed, May 22, 2013 at 11:58:18PM +, Daniel Shahaf wrote:
> r1485501 touches both the 1.6.23 and 1.7.10 sections, so it would conflict if
> merged.  So I didn't merge it, instead, I applied the change by hand.  When I
> tried to record-only merge that revision (to a pristine 1.6.x working copy,
> using an 1.8.0-rc2 client), I got mergeinfo changes on numerous files in the
> tree.  Transcript attached.  I would have expected only ./CHANGES to have
> property modifications, not any other file in the tree.  Is there a bug in the
> behaviour recorded in the transcript?

This is a known issue with --record-only merges.
They always update all subtree mergeinfo.

I guess fixes are very welcome :)

Re: 1.6.22 up for signing/testing

[Ben Reser]

On Wed, May 22, 2013 at 4:10 PM, Ben Reser  wrote:
> I'm just going to reroll a 1.6.23 with the correct fixes and we'll
> just skip 1.6.22.

Done.

Re: 1.6.23 up for signing/testing

[Ben Reser]

On Wed, May 22, 2013 at 6:08 PM, Ben Reser  wrote:
> The 1.6.22 release artifacts are now available for testing/signing.

That should be 1.6.23, changed the subject but missed this in the body.  Sorry.

1.6.23 up for signing/testing

[Ben Reser]

The 1.6.22 release artifacts are now available for testing/signing.
Please get the tarballs from
  https://dist.apache.org/repos/dist/dev/subversion
and add your signatures there. We plan to try and release on May 30th
so please try and get your votes/signatures in place by May 28th EOD.

Thanks!

Please note the following known test failures (perhaps most easily
triggered with APR-1.4.6):

 log_tests.py 30: log -g should ignore cyclic merges
 Can fail because 1.6.x is missing r1293229, which fixed occasionally
 missing merged-via notifications.

 diff_tests.py 32: repos-wc diff showing added entries with props
 Can fail because this test is missing various tweaks to account
 for random output order of 'svn diff' (this is independent of the
 APR 1.4.6 hash order problem).

 ruby swig test 'test_dump'
 Can fail because 1.6.x is missing r966458, which sorted property
 listings in dump files in alphabetical order, and this test does
 not parse dump files and sort properties for comparison.

Side note: I did get a ruby failure I hadn't seen before but it looks
like another case of sorting:
[[[
  2) Failure:
test_relocate(SvnWcTest)
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/test/test_wc.rb:1007:in
`test_relocate'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/svn/wc.rb:117:in
`_open'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/svn/wc.rb:101:in
`probe_open'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/test/test_wc.rb:990:in
`test_relocate'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/svn/ra.rb:33:in
`open'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/test/test_wc.rb:978:in
`test_relocate'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/test/util.rb:181:in
`make_context'
/home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/test/test_wc.rb:975:in
`test_relocate':
<[["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir1",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir1",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir1/hello.txt",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir1/hello.txt",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2/hello2.txt",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2/hello2.txt",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"]]>
expected but was
<[["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2/hello2.txt",
  nil],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2/dir2/hello2.txt",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/dir2"],
 ["0c50d5bb-1bd1-4596-8963-5d86d6b62908",
  
"file:///home/breser/subversion-1.6.23/subversion/bindings/swig/ruby/repos/di

Re: svn commit: r1485499 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c

[Blair Zajac]

On 05/22/2013 04:22 PM, stef...@apache.org wrote:

Author: stefan2
Date: Wed May 22 23:22:22 2013
New Revision: 1485499

URL: http://svn.apache.org/r1485499
Log:
Reduce ra_svn protocol parsing overhead.

* subversion/libsvn_ra_svn/marshal.c
   (vparse_tuple): test for the most frequent item types first

Modified:
 subversion/trunk/subversion/libsvn_ra_svn/marshal.c

Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1485499&r1=1485498&r2=1485499&view=diff
==
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Wed May 22 23:22:22 2013
@@ -1202,14 +1202,15 @@ static svn_error_t *vparse_tuple(const a
if (**fmt == '?')
  (*fmt)++;
elt = &APR_ARRAY_IDX(items, count, svn_ra_svn_item_t);
-  if (**fmt == 'n' && elt->kind == SVN_RA_SVN_NUMBER)
-*va_arg(*ap, apr_uint64_t *) = elt->u.number;
-  else if (**fmt == 'r' && elt->kind == SVN_RA_SVN_NUMBER)
-*va_arg(*ap, svn_revnum_t *) = (svn_revnum_t) elt->u.number;
-  else if (**fmt == 's' && elt->kind == SVN_RA_SVN_STRING)
-*va_arg(*ap, svn_string_t **) = elt->u.string;
+  if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
+{
+  (*fmt)++;
+  SVN_ERR(vparse_tuple(elt->u.list, pool, fmt, ap));
+}


Out of curiosity, how much cost is there in the double dereferencing of 
fmt?  Could you save it in a local variable and do the if/else on that 
instead?  Also, would a switch block be more efficient?


Blair

Re: svn commit: r1485506 - /subversion/branches/1.6.x/CHANGES

[Daniel Shahaf]

On Wed, May 22, 2013 at 11:39:25PM -, danie...@apache.org wrote:
> Author: danielsh
> Date: Wed May 22 23:39:25 2013
> New Revision: 1485506
> 
> URL: http://svn.apache.org/r1485506
> Log:
> * CHANGES (1.6.x): Re-do r1485501 from trunk, minus the 1.7.10 part.
> 
> Modified:
> subversion/branches/1.6.x/CHANGES
> 

r1485501 touches both the 1.6.23 and 1.7.10 sections, so it would conflict if
merged.  So I didn't merge it, instead, I applied the change by hand.  When I
tried to record-only merge that revision (to a pristine 1.6.x working copy,
using an 1.8.0-rc2 client), I got mergeinfo changes on numerous files in the
tree.  Transcript attached.  I would have expected only ./CHANGES to have
property modifications, not any other file in the tree.  Is there a bug in the
behaviour recorded in the transcript?

Thanks

Daniel

> Modified: subversion/branches/1.6.x/CHANGES
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/1.6.x/CHANGES?rev=1485506&r1=1485505&r2=1485506&view=diff
> ==
> --- subversion/branches/1.6.x/CHANGES (original)
> +++ subversion/branches/1.6.x/CHANGES Wed May 22 23:39:25 2013
> @@ -4,7 +4,7 @@ http://svn.apache.org/repos/asf/subversi
>  
>   User-visible changes
>- Server-side bugfixes:
> -* fix repository corruption due to newline in filename (issue #4340)
> +* fix FSFS repository corruption due to newline in filename (issue #4340)
>  * fix svnserve exiting when a client connection is aborted (r1482759)
>  
>- Other tool improvements and bugfixes:
> 
> 
svn-qavm,0:svn/16x% svn --version -q
1.8.0-rc2
svn-qavm,0:svn/16x% svnversion
1485503
svn-qavm,0:svn/16x% svn merge -c r1485501 --record-only ^/subversion/trunk
--- Recording mergeinfo for merge of r1485501 into '.':
 U   .
--- Recording mergeinfo for merge of r1485501 into 'CHANGES':
 U   CHANGES
--- Recording mergeinfo for merge of r1485501 into 
'contrib/client-side/emacs/psvn.el':
 U   contrib/client-side/emacs/psvn.el
--- Recording mergeinfo for merge of r1485501 into 'get-deps.sh':
 U   get-deps.sh
--- Recording mergeinfo for merge of r1485501 into 
'subversion/bindings/ctypes-python/setup.py':
 U   subversion/bindings/ctypes-python/setup.py
--- Recording mergeinfo for merge of r1485501 into 
'subversion/include/private/svn_repos_private.h':
 U   subversion/include/private/svn_repos_private.h
--- Recording mergeinfo for merge of r1485501 into 
'subversion/libsvn_fs_fs/rep-cache.c':
 U   subversion/libsvn_fs_fs/rep-cache.c
--- Recording mergeinfo for merge of r1485501 into 
'subversion/libsvn_repos/repos.c':
 U   subversion/libsvn_repos/repos.c
--- Recording mergeinfo for merge of r1485501 into 'subversion/svn/svn.1':
 U   subversion/svn/svn.1
--- Recording mergeinfo for merge of r1485501 into 
'subversion/svnadmin/svnadmin.1':
 U   subversion/svnadmin/svnadmin.1
--- Recording mergeinfo for merge of r1485501 into 
'subversion/svndumpfilter/svndumpfilter.1':
 U   subversion/svndumpfilter/svndumpfilter.1
--- Recording mergeinfo for merge of r1485501 into 
'subversion/svnlook/svnlook.1':
 U   subversion/svnlook/svnlook.1
--- Recording mergeinfo for merge of r1485501 into 'subversion/svnserve/main.c':
 U   subversion/svnserve/main.c
--- Recording mergeinfo for merge of r1485501 into 
'subversion/svnsync/svnsync.1':
 U   subversion/svnsync/svnsync.1
--- Recording mergeinfo for merge of r1485501 into 
'subversion/svnversion/svnversion.1':
 U   subversion/svnversion/svnversion.1
--- Recording mergeinfo for merge of r1485501 into 
'subversion/tests/cmdline/merge_tests.py':
 U   subversion/tests/cmdline/merge_tests.py
--- Recording mergeinfo for merge of r1485501 into 
'subversion/tests/cmdline/svntest/main.py':
 U   subversion/tests/cmdline/svntest/main.py
svn-qavm,0:svn/16x% svn st -q
 M  .
 M  CHANGES
 M  contrib/client-side/emacs/psvn.el
 M  get-deps.sh 
 M  subversion/bindings/ctypes-python/setup.py
 M  subversion/include/private/svn_repos_private.h
 M  subversion/libsvn_fs_fs/rep-cache.c
 M  subversion/libsvn_repos/repos.c
 M  subversion/svn/svn.1
 M  subversion/svnadmin/svnadmin.1
 M  subversion/svndumpfilter/svndumpfilter.1
 M  subversion/svnlook/svnlook.1
 M  subversion/svnserve/main.c
 M  subversion/svnsync/svnsync.1
 M  subversion/svnversion/svnversion.1
 M  subversion/tests/cmdline/merge_tests.py
 M  subversion/tests/cmdline/svntest/main.py
svn-qavm,0:svn/16x% 

Re: 1.6.22 up for signing/testing

[Ben Reser]

On Wed, May 22, 2013 at 4:36 PM, Stefan Sperling  wrote:
>> > > No we don't.  1.6 are not Apache-branded releases, they are released on 
>> > > tigris.
>> >
>> > We're always voting for 1.6 releases in that directory, and then
>> > we ship the release on trigis. Or has something changed?
>>
>> No.  But you said "wait a day for the mirrors to sync", with tigris that
>> doesn't happen.
>
> Ah, yes, that's right of course.

I can't wait for 1.8.0 to be out and we can stop with these non-Apache releases.

Re: 1.6.22 up for signing/testing

[Stefan Sperling]

On Thu, May 23, 2013 at 02:29:31AM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, May 23, 2013 at 01:25:24 +0200:
> > On Wed, May 22, 2013 at 10:51:38PM +, Daniel Shahaf wrote:
> > > On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> > > > The 1.6.22 release artifacts are now available for testing/signing.
> > > > Please get the tarballs from
> > > >   https://dist.apache.org/repos/dist/dev/subversion
> > > > and add your signatures there. I plan to try and release on May 30th
> > > > so please try and get your votes/signatures in place by May 28th EOD
> > > > (we need to wait for mirrors to sync for one day before release).
> > > 
> > > No we don't.  1.6 are not Apache-branded releases, they are released on 
> > > tigris.
> > 
> > We're always voting for 1.6 releases in that directory, and then
> > we ship the release on trigis. Or has something changed?
> 
> No.  But you said "wait a day for the mirrors to sync", with tigris that
> doesn't happen.

Ah, yes, that's right of course.

Re: 1.6.22 up for signing/testing

[Daniel Shahaf]

Stefan Sperling wrote on Thu, May 23, 2013 at 01:25:24 +0200:
> On Wed, May 22, 2013 at 10:51:38PM +, Daniel Shahaf wrote:
> > On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> > > The 1.6.22 release artifacts are now available for testing/signing.
> > > Please get the tarballs from
> > >   https://dist.apache.org/repos/dist/dev/subversion
> > > and add your signatures there. I plan to try and release on May 30th
> > > so please try and get your votes/signatures in place by May 28th EOD
> > > (we need to wait for mirrors to sync for one day before release).
> > 
> > No we don't.  1.6 are not Apache-branded releases, they are released on 
> > tigris.
> 
> We're always voting for 1.6 releases in that directory, and then
> we ship the release on trigis. Or has something changed?

No.  But you said "wait a day for the mirrors to sync", with tigris that
doesn't happen.

Re: 1.6.22 up for signing/testing

[Stefan Sperling]

On Wed, May 22, 2013 at 04:10:45PM -0700, Ben Reser wrote:
> I'm just going to reroll a 1.6.23 with the correct fixes and we'll
> just skip 1.6.22.

Works for me.

Re: 1.6.22 up for signing/testing

[Stefan Sperling]

On Wed, May 22, 2013 at 10:51:38PM +, Daniel Shahaf wrote:
> On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> > The 1.6.22 release artifacts are now available for testing/signing.
> > Please get the tarballs from
> >   https://dist.apache.org/repos/dist/dev/subversion
> > and add your signatures there. I plan to try and release on May 30th
> > so please try and get your votes/signatures in place by May 28th EOD
> > (we need to wait for mirrors to sync for one day before release).
> 
> No we don't.  1.6 are not Apache-branded releases, they are released on 
> tigris.

We're always voting for 1.6 releases in that directory, and then
we ship the release on trigis. Or has something changed?

Re: 1.6.22 up for signing/testing

[Daniel Shahaf]

On Wed, May 22, 2013 at 04:10:45PM -0700, Ben Reser wrote:
> On Wed, May 22, 2013 at 3:48 PM, Stefan Sperling  wrote:
> > On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> >> The 1.6.22 release artifacts are now available for testing/signing.
> >
> > Please note that due to an oversight (or miscommunication between
> > me and Daniel, if you will), the tarball does not contain the contrib
> > script changes mentioned in the CHANGES file.
> >
> > We won't re-roll because of this. We can mention this in the release
> > announcement and adjust CHANGES on trunk and 1.6.x accordingly, and
> > provide links to the fixed contrib scripts on trunk.
> 
> I hate saying this but I think this is going to be terribly confusing.
>  The CHANGES file in the tarball says it's fixed.  But it's not.  That
> just seems like a terribly confusing situation.  Which leaves me in
> the position of being at least -0 if not -1 on this release.
> 
> I'm just going to reroll a 1.6.23 with the correct fixes and we'll
> just skip 1.6.22.

Thanks Ben!

Re: 1.6.22 up for signing/testing

[Ben Reser]

On Wed, May 22, 2013 at 3:48 PM, Stefan Sperling  wrote:
> On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
>> The 1.6.22 release artifacts are now available for testing/signing.
>
> Please note that due to an oversight (or miscommunication between
> me and Daniel, if you will), the tarball does not contain the contrib
> script changes mentioned in the CHANGES file.
>
> We won't re-roll because of this. We can mention this in the release
> announcement and adjust CHANGES on trunk and 1.6.x accordingly, and
> provide links to the fixed contrib scripts on trunk.

I hate saying this but I think this is going to be terribly confusing.
 The CHANGES file in the tarball says it's fixed.  But it's not.  That
just seems like a terribly confusing situation.  Which leaves me in
the position of being at least -0 if not -1 on this release.

I'm just going to reroll a 1.6.23 with the correct fixes and we'll
just skip 1.6.22.

Re: 1.6.22 up for signing/testing

[Daniel Shahaf]

On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> The 1.6.22 release artifacts are now available for testing/signing.
> Please get the tarballs from
>   https://dist.apache.org/repos/dist/dev/subversion
> and add your signatures there. I plan to try and release on May 30th
> so please try and get your votes/signatures in place by May 28th EOD
> (we need to wait for mirrors to sync for one day before release).

No we don't.  1.6 are not Apache-branded releases, they are released on tigris.

1.7.10 up for signing/testing

[Stefan Sperling]

The 1.7.10 release artifacts are now available for testing/signing.
Please get the tarballs from
  https://dist.apache.org/repos/dist/dev/subversion
and add your signatures there.  I plan to try and release on May
30th so please try and get your votes/signatures in place by May 28th.

Thanks!

Re: 1.6.22 up for signing/testing

[Stefan Sperling]

On Thu, May 23, 2013 at 12:40:30AM +0200, Stefan Sperling wrote:
> The 1.6.22 release artifacts are now available for testing/signing.

Please note that due to an oversight (or miscommunication between
me and Daniel, if you will), the tarball does not contain the contrib
script changes mentioned in the CHANGES file.

We won't re-roll because of this. We can mention this in the release
announcement and adjust CHANGES on trunk and 1.6.x accordingly, and
provide links to the fixed contrib scripts on trunk.

1.6.22 up for signing/testing

[Stefan Sperling]

The 1.6.22 release artifacts are now available for testing/signing.
Please get the tarballs from
  https://dist.apache.org/repos/dist/dev/subversion
and add your signatures there. I plan to try and release on May 30th
so please try and get your votes/signatures in place by May 28th EOD
(we need to wait for mirrors to sync for one day before release).
Thanks!

Please note the following known test failures (perhaps most easily
triggered with APR-1.4.6):

 log_tests.py 30: log -g should ignore cyclic merges
 Can fail because 1.6.x is missing r1293229, which fixed occasionally
 missing merged-via notifications.

 diff_tests.py 32: repos-wc diff showing added entries with props
 Can fail because this test is missing various tweaks to account
 for random output order of 'svn diff' (this is independent of the
 APR 1.4.6 hash order problem).

 ruby swig test 'test_dump'
 Can fail because 1.6.x is missing r966458, which sorted property
 listings in dump files in alphabetical order, and this test does
 not parse dump files and sort properties for comparison.

Re: Create/Apply Patch - UTF-16 and binary support

[Stefan Sperling]

On Wed, May 22, 2013 at 01:12:00PM -0700, Friedrich Brunzema wrote:
> By the sounds of it, this is not really a burning issue for the
> project - ie not on your roadmap.

That seems to be the case, yes. You never know who is working on
what in an open source project. But there is no evidence of any
development activity related to this. As Julian pointed out, you
could help make it happen!

> If it does not work reliably, is it even worth having the create
> patch/apply patch functionality? [Just putting the question out
> there...]

Well, 'svn patch' is intended as the Subversion equivalent of the
UNIX patch program: http://en.wikipedia.org/wiki/Patch_%28Unix%29
That program cannot handle binary files either. Therefor, such
functionality was never within the scope of 'svn patch'.

The intended use case is easier handling of patch submissions made to
open source projects which use Subversion (such as FreeBSD). Patches
submitted to such projects rarely contain binary files.
I believe 'svn patch' has fulfilled that purpose very well, so I wouldn't
consider it incomplete at all. Your desired feature simply wasn't part
of the goal of the 'svn patch' development effort (there were earlier
'svn patch' development efforts some years ago, which tried to cover
your use case, too, but they weren't completed).

TortoiseSVN 1.6 did include a custom 'patch' feature, but that was
even more limited than the Subversion 1.7 patch feature is now.
Since TortoiseSVN 1.7, TortoiseSVN's patch feature uses the same
code under the hood that 'svn patch' is using, so it provides the
same functionality.

Re: Create/Apply Patch - UTF-16 and binary support

[Friedrich Brunzema]

Stefan S,

yes, lack of UTF-16 is one of the issues.  The other is creating / applying 
patches for binary files.


By the sounds of it, this is not really a burning issue for the project - ie 
not on your roadmap.
Question is - how useful would this functionality be to end-users?  Maybe using 
a .zip program to ship changes
to a fellow developer is just as easy - are these non-trival svn changes worth 
the effort is what I'm asking, I guess.
Still, conceptually I'm kind of bothered by the fact that svn has the 
functionality, but that it does not work reliably for all file types.
If it does not work reliably, is it even worth having the create patch/apply 
patch functionality? [Just putting the question out there...]


Stefan K- One could try to tackle this problem on the TortoiseSVN side.  Have a 
new Create Zip patch - functionality looks for changed wc files,
zips all of them using the correct hierarchy from the working copy.  Patch 
could just overwrite in a dumb way, or could actually try to merge
what it can.  Or you don't provide a patch function at all, but leave unzipping 
to the end-user.  Still, this sounds like a bad hack.


If one were to try tackling this problem and fixing it the 'right way' (and I'm 
not saying I necessarily would), where would one start?

Grüße,

Friedrich Brunzema




 From: Stefan Sperling 
To: Friedrich.Brunzema  
Cc: dev@subversion.apache.org; tortoise...@gmail.com 
Sent: Wednesday, May 22, 2013 1:50:45 PM
Subject: Re: Create/Apply Patch - UTF-16 and binary support
 

On Wed, May 22, 2013 at 12:29:16PM -0400, Friedrich.Brunzema wrote:
> I’m kind of surprised/disappointed that a reliable patch mechanism does not
> seem to exist for svn.

Well, before Subversion 1.7, there was no support for applying patches at all!

I think the basic problem you're asking about is that Subversion
does not support UTF-16 as a text format. This problem isn't specific
to the 'svn patch' command. Solving this problem probably isn't trivial.
See http://subversion.tigris.org/issues/show_bug.cgi?id=2194

Tracing the client-side authn subsystem's work.

[C. Michael Pilato]

Playing with my 'auth-notification' branch a few minutes ago, I
enabled the notifications (via an environment variable) and ran an
update command across all the items in my ~/projects directory.  The
resulting notification-filled trace is kinda cool.  Nice to finally
see where my authn subsystem interactions are happening for once!

   $ SVN_AUTH_NOTIFY=yes svn up * --config-dir=/tmp/foo
   Updating 'asf-committers':
   Credentials acquired (Plaintext (SSL server trust))
   Credentials acquired (Gnome Keyring (simple))
   At revision 41815.
   Updating 'cmpilato':
   Credentials acquired (Plaintext (SSL server trust))
   Credentials acquired (Gnome Keyring (simple))
   At revision 23.
   Updating 'dist-subversion':
   Credentials acquired (Plaintext (SSL server trust))
   At revision 2033.
   Updating 'private-subversion':
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   At revision 41815.
   Updating 'serf':
   Credentials acquired (Plaintext (SSL server trust))
   At revision 1869.
   Skipped 'spec.subversion'
   Skipped 'spec.viewvc'
   Updating 'subversion':
   Credentials acquired (Auth Baton Cache)
   At revision 1485359.
   Updating 'svnbook':
   Credentials acquired (Plaintext (SSL server trust))
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)

   Fetching external item into 'svnbook/branches/1.4/ru/nb-bin':
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   External at revision 2339.

   At revision 4500.
   Updating 'svnedge':
   Credentials acquired (Plaintext (SSL server trust))
   Authentication realm:  Authorization R
   ealm
   Password for 'cmpilato':

   Credentials acquired (Plaintext (prompt))
   Credentials stored (Gnome Keyring (simple))
   At revision 3592.
   Updating 'teamforge':
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)

   Fetching external item into 'teamforge/trunk/core/mod_authnz_ctf/source':
   Credentials acquired (Auth Baton Cache)
   Credentials acquired (Auth Baton Cache)
   External at revision 518.

   At revision 59618.
   Updating 'viewvc':
   Authentication realm:  CollabNet Subversion
   Repository
   Password for 'cmpilato':

   Credentials acquired (Plaintext (prompt))
   Credentials stored (Gnome Keyring (simple))
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/tags/1.1.18/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2847.

   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/tags/1.1.19/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2883.

   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/branches/1.0.x/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2904.

   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/branches/1.1.x/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2904.

   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/tags/1.1.10/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2534.

   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))

   Fetching external item into 'viewvc/trunk/templates-contrib':
   Credentials acquired (Auth Baton Cache)
   Credentials stored (Gnome Keyring (simple))
   External at revision 2904.

   At revision 2904.
   Summary of updates:
 Updated 'asf-committers' to r41815.
 Updated 'cmpilato' to r23.
 Updated 'dist-subversion' to r2033.
 Updated 'private-subversion' to r41815.
 Updated 'serf' to r1869.
 Updated 'subversion' to r1485359.
 Updated 'svnbook' to r4500.
 Updated 'svnedge' to r3592.
 Updated 'teamforge' to r59618.
 Updated 'viewvc' to r2904.
   Summary of conflicts:
 Skipped paths: 2

Of particular interest to me, though, is the handling of externals.
My viewvc sparse checkout has quite a few intra-repos externals
defined in it.  As such, all the authn c

Re: Create/Apply Patch - UTF-16 and binary support

[Mark Phippard]

On Wed, May 22, 2013 at 12:29 PM, Friedrich.Brunzema  wrote:

> Stefan told me that someone had implemented such functionality in the past -
> that it did not end up getting used because the diff clients needed
> changing.

I thin Stefan was talking about this Summer of Code project:

http://svn.haxx.se/dev/archive-2007-03/0399.shtml

It was in fact implemented and never released.  My recollection was
that a big problem was the one that Branko mentioned, that binary
deltas cannot simply be applied to any revision.  So you could only
apply these patches to working copies that were the same or similar
revision as the one that created the patch.  And of course they also
could not have been applied with a generic patch program either.

One would need to reach through the archives for the summer and fall
of 2007 to find all the details.

--
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: move-tracking-1 branch (was: Re: svn commit: r1485307 - /subversion/branches/move-tracking-1/)

[Julian Foad]

Stefan Sperling wrote:
>> Author: julianfoad
>> URL: http://svn.apache.org/r1485307
>> Log:
>> Create a branch on which to develop some parts of move-tracking.
> 
> What are your design ideas? :)

Hi Stefan.

I have just added a 'BRANCH-README' file:
.

Other than that, I have been making some notes in
 and
.

I'll try to put some more notes on line.

- Julian

Re: Create/Apply Patch - UTF-16 and binary support

[Julian Foad]

Stefan Sperling wrote:

> On Wed, May 22, 2013 at 12:29:16PM -0400, Friedrich.Brunzema wrote:
>>  I’m kind of surprised/disappointed that a reliable patch mechanism does not
>>  seem to exist for svn.
> 
> Well, before Subversion 1.7, there was no support for applying patches at all!

There was in TortoiseSVN.

> I think the basic problem you're asking about is that Subversion
> does not support UTF-16 as a text format. This problem isn't specific
> to the 'svn patch' command. Solving this problem probably isn't 
> trivial.
> See http://subversion.tigris.org/issues/show_bug.cgi?id=2194

That is one of the two problems that Friedrich listed.  The other is that Svn 
does not support patching of 'binary' files.  (More generally, Subversion 
cannot round-trip an arbitrary change from 'svn diff' to 'svn patch' and back.  
As well as no support for 'binary' files, it has only partial support for tree 
changes.)

There is some overlap between those two issues, but neither of them strictly 
depends on the other.

Friedrich, I also wish we could support these two requirements.  As an open 
source project, development happens mainly when somebody steps up and does it.  
Are you, or is anybody you know, able to help specify or design or implement 
these features?  Even starting to discuss how it could work can sometimes be 
enough to get other people involved.  I personally would be willing to help 
with supporting a more complete diff and patch, since I am currently working in 
the area of diff and merge and renames.

- Julian

Re: Create/Apply Patch - UTF-16 and binary support

[Branko Čibej]

On 22.05.2013 18:29, Friedrich.Brunzema wrote:
> When I do a commit, the changes are bunched up in some binary format
> and sent to the server.  Can we not capture this kind of data in some
> text-encoded format, and have the patch program understand the format?

A binary delta will apply correctly only to exactly the same, unmodified
source. A patch, on the other hand, contans context information that
help it find parts of the file that (appear to) match whatever source
the patch was created from.

See:
http://en.wikipedia.org/wiki/Delta_encoding
vs.
http://en.wikipedia.org/wiki/Diff

-- Brane


-- 
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com

Re: Create/Apply Patch - UTF-16 and binary support

[Stefan Sperling]

On Wed, May 22, 2013 at 12:29:16PM -0400, Friedrich.Brunzema wrote:
> I’m kind of surprised/disappointed that a reliable patch mechanism does not
> seem to exist for svn.

Well, before Subversion 1.7, there was no support for applying patches at all!

I think the basic problem you're asking about is that Subversion
does not support UTF-16 as a text format. This problem isn't specific
to the 'svn patch' command. Solving this problem probably isn't trivial.
See http://subversion.tigris.org/issues/show_bug.cgi?id=2194

move-tracking-1 branch (was: Re: svn commit: r1485307 - /subversion/branches/move-tracking-1/)

[Stefan Sperling]

On Wed, May 22, 2013 at 05:42:02PM -, julianf...@apache.org wrote:
> Author: julianfoad
> Date: Wed May 22 17:42:02 2013
> New Revision: 1485307
> 
> URL: http://svn.apache.org/r1485307
> Log:
> Create a branch on which to develop some parts of move-tracking.

Hey Julian,

What are your design ideas? :)

Create/Apply Patch - UTF-16 and binary support

[Friedrich . Brunzema]

Hi,

 

I’m requesting that someone fix the Create/Apply patch functionality of
Subversion.  Having spoken to Stefan Küng of TortoiseSVN fame, he mentioned
that there are a few problems with the create /apply patch that need fixing
on the Subversion side before we can do anything on the TortoiseSVN side.
Two specific things come to mind:

 

-UTF-16 support.

-Binary format file.

 

The use case is as follows:

I have some uncommitted changes in my working copy – an added .png binary
file, and a modified UTF-16 encoded windows resource-script file (.rc).  I
want to send changes to a friend for review before committing them to the
repo.  My friend should just receive the .patch file and be able to apply
the patch to get to the same Working copy as is on my system.  Right now, I
have to think about binary and UTF-16 encoded files, and put those in a zip
file. 

 

Stefan told me that someone had implemented such functionality in the past -
that it did not end up getting used because the diff clients needed
changing.

 

I’m kind of surprised/disappointed that a reliable patch mechanism does not
seem to exist for svn.  When I do a commit, the changes are bunched up in
some binary format and sent to the server.  Can we not capture this kind of
data in some text-encoded format, and have the patch program understand the
format?

 

Below are some archived posts about Create/Apply Patch

http://svn.haxx.se/tsvnusers/archive-2011-05/0012.shtml

http://svn.haxx.se/tsvn/archive-2009-04/0197.shtml

 

Best regards,

 

Friedrich Brunzema

 

 

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/ io.c

[Julian Foad]

Daniel Shahaf wrote:
> Bert Huijben wrote on Wed, May 22, 2013 at 10:50:17 +0200:
>>>    failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
>>>    failed_command = apr_pstrcat(pool, failed_command, " ");
>> 
>> Note that apr_pstrcat needs a final NULL argument :)
>> 
>> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
> 
> You need to explicitly cast the last NULL:
> 
>     apr_pstrcat(pool, failed_command, " ", cmd[i], (void *)NULL);
> 
> (because it's a variadic function so there is no implicit conversion to
> a pointer, when NULL is #define'd to be a 0 narrower than the pointer)

For a strcat, the arguments are pointers to 'char', so it looks better to use 
(char *)NULL.

- Julian

Re: [PATCH] SVN_UNALIGNED_ACCESS_IS_OK for ppc

[Ben Reser]

On Wed, May 22, 2013 at 12:58 AM, Markus Schaber  wrote:
> Hmm, when checking those places, I'm not sure whether modern compilers
> with their auto-vectorizing optimizers really profit from the hand-optimized
> code there, or whether that code might rather trouble the optimizer into
> generating suboptimal code.

Most of the code is just chunk processing of strings.  Shouldn't be
hard to write a simple example and benchmark it.

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/ io.c

[Daniel Shahaf]

Bert Huijben wrote on Wed, May 22, 2013 at 10:50:17 +0200:
> 
> 
> > -Original Message-
> > From: Philip Martin [mailto:philip.mar...@wandisco.com]
> > Sent: woensdag 22 mei 2013 10:46
> > To: dev@subversion.apache.org
> > Subject: Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> > 
> > g...@apache.org writes:
> > 
> > > Author: gbg
> > > Date: Tue May 21 22:57:23 2013
> > > New Revision: 1485007
> > >
> > > URL: http://svn.apache.org/r1485007
> > > Log:
> > > On the invoke-diff-cmd branch: Repair erroneous initialization.
> > >
> > > * subversion/svn/io.c
> > >
> > >   (svn_io_run_external_diff): Change pcalloc to palloc call.
> > >
> > > Modified:
> > > subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> > >
> > > Modified: subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> > > URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007
> > &view=diff
> > >
> > ==
> > 
> > > --- subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c (original)
> > > +++ subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
> > > @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> > > for (i = 0, size = 0; cmd[i]; i++)
> > >   size += strlen(cmd[i]) + 1;
> > >
> > > -   failed_command = apr_pcalloc(pool, size * sizeof(char *));
> > > +   failed_command = apr_palloc(pool, size * sizeof(char *));
> > >
> > > for (i = 0; cmd[i]; i++)
> > >  {
> > >
> > 
> > That's not right.  You have calculated 'size' by summing strlen to give
> > you the total text length.  Multiplying the text length by the sizeof a
> > pointer is wrong.  You could multiply by sizeof(char) but that is 1. You
> > need to add +1 for the terminating null byte.
> > 
> > However this is an error path so I'd go for simplicity rather than
> > efficiency:
> > 
> >const char *failed_command = "";
> >for (i = 0; cmd[i]; ++i)
> >  {
> >failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
> >failed_command = apr_pstrcat(pool, failed_command, " ");
> 
> Note that apr_pstrcat needs a final NULL argument :)
> 
> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);

You need to explicitly cast the last NULL:

apr_pstrcat(pool, failed_command, " ", cmd[i], (void *)NULL);

(because it's a variadic function so there is no implicit conversion to
a pointer, when NULL is #define'd to be a 0 narrower than the pointer)

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/ io.c

[Daniel Shahaf]

Philip Martin wrote on Wed, May 22, 2013 at 10:44:33 +0100:
> Philip Martin  writes:
> 
> > My first loop results in a final trailing " " and my second loop results
> > in a leading " ".  Perhaps:
> >
> >const char *failed_command = cmd[0];
> >for (i = 1; cmd[i]; ++i)
> >  failed_command = apr_psprintf(pool, "%s %s", failed_command, 
> > cmd[i]);
> 
> Another problem is that you go on to do:
> 
>   return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
>_("'%s' was expanded to '%s' and returned %d"),
>external_diff_cmd,
>svn_dirent_local_style(failed_command, pool),
>*pexitcode); 
> 
> which applies local style to the whole command string not just to paths.
> On Windows that will convert all '/' to '\' and command options on
> Windows can use '/' where Unix would use '-'.

I pointed out this issue already in my branch review.

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c

[Philip Martin]

Philip Martin  writes:

> My first loop results in a final trailing " " and my second loop results
> in a leading " ".  Perhaps:
>
>const char *failed_command = cmd[0];
>for (i = 1; cmd[i]; ++i)
>  failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);

Another problem is that you go on to do:

  return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
   _("'%s' was expanded to '%s' and returned %d"),
   external_diff_cmd,
   svn_dirent_local_style(failed_command, pool),
   *pexitcode); 

which applies local style to the whole command string not just to paths.
On Windows that will convert all '/' to '\' and command options on
Windows can use '/' where Unix would use '-'.

-- 
Philip

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c

[Philip Martin]

"Bert Huijben"  writes:

>> However this is an error path so I'd go for simplicity rather than
>> efficiency:
>> 
>>const char *failed_command = "";
>>for (i = 0; cmd[i]; ++i)
>>  {
>>failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
>>failed_command = apr_pstrcat(pool, failed_command, " ");
>
> Note that apr_pstrcat needs a final NULL argument :)
>
> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
>
>   Bert
>>  }
>> 
>> or:
>> 
>>const char *failed_command = "";
>>for (i = 0; cmd[i]; ++i)
>>  failed_command = apr_psprintf(pool, "%s %s", failed_command,
>> cmd[i]);

My first loop results in a final trailing " " and my second loop results
in a leading " ".  Perhaps:

   const char *failed_command = cmd[0];
   for (i = 1; cmd[i]; ++i)
 failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);

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

RE: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c

[Bert Huijben]

> -Original Message-
> From: Philip Martin [mailto:philip.mar...@wandisco.com]
> Sent: woensdag 22 mei 2013 10:46
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
> 
> g...@apache.org writes:
> 
> > Author: gbg
> > Date: Tue May 21 22:57:23 2013
> > New Revision: 1485007
> >
> > URL: http://svn.apache.org/r1485007
> > Log:
> > On the invoke-diff-cmd branch: Repair erroneous initialization.
> >
> > * subversion/svn/io.c
> >
> >   (svn_io_run_external_diff): Change pcalloc to palloc call.
> >
> > Modified:
> > subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
> >
> > Modified: subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
> > URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007
> &view=diff
> >
> ==
> 
> > --- subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c (original)
> > +++ subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
> > @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> > for (i = 0, size = 0; cmd[i]; i++)
> >   size += strlen(cmd[i]) + 1;
> >
> > -   failed_command = apr_pcalloc(pool, size * sizeof(char *));
> > +   failed_command = apr_palloc(pool, size * sizeof(char *));
> >
> > for (i = 0; cmd[i]; i++)
> >  {
> >
> 
> That's not right.  You have calculated 'size' by summing strlen to give
> you the total text length.  Multiplying the text length by the sizeof a
> pointer is wrong.  You could multiply by sizeof(char) but that is 1. You
> need to add +1 for the terminating null byte.
> 
> However this is an error path so I'd go for simplicity rather than
> efficiency:
> 
>const char *failed_command = "";
>for (i = 0; cmd[i]; ++i)
>  {
>failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
>failed_command = apr_pstrcat(pool, failed_command, " ");

Note that apr_pstrcat needs a final NULL argument :)

So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);

Bert
>  }
> 
> or:
> 
>const char *failed_command = "";
>for (i = 0; cmd[i]; ++i)
>  failed_command = apr_psprintf(pool, "%s %s", failed_command,
> cmd[i]);
> 
> 
> 
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download

Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c

[Philip Martin]

g...@apache.org writes:

> Author: gbg
> Date: Tue May 21 22:57:23 2013
> New Revision: 1485007
>
> URL: http://svn.apache.org/r1485007
> Log:
> On the invoke-diff-cmd branch: Repair erroneous initialization.
>
> * subversion/svn/io.c
>
>   (svn_io_run_external_diff): Change pcalloc to palloc call.
>
> Modified:
> subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
>
> Modified: 
> subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007&view=diff
> ==
> --- subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c 
> (original)
> +++ subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c 
> Tue May 21 22:57:23 2013
> @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> for (i = 0, size = 0; cmd[i]; i++) 
>   size += strlen(cmd[i]) + 1;
>  
> -   failed_command = apr_pcalloc(pool, size * sizeof(char *));
> +   failed_command = apr_palloc(pool, size * sizeof(char *));
>  
> for (i = 0; cmd[i]; i++) 
>  {
>

That's not right.  You have calculated 'size' by summing strlen to give
you the total text length.  Multiplying the text length by the sizeof a
pointer is wrong.  You could multiply by sizeof(char) but that is 1. You
need to add +1 for the terminating null byte.

However this is an error path so I'd go for simplicity rather than
efficiency:

   const char *failed_command = "";
   for (i = 0; cmd[i]; ++i)
 {
   failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
   failed_command = apr_pstrcat(pool, failed_command, " ");
 }

or:

   const char *failed_command = "";
   for (i = 0; cmd[i]; ++i)
 failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);
  


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

AW: [PATCH] SVN_UNALIGNED_ACCESS_IS_OK for ppc

[Markus Schaber]

Hi,

Von: Ben Reser [mailto:b...@reser.org]
> On Thu, May 16, 2013 at 2:27 PM, Branko Čibej  wrote:
> > On 16.05.2013 23:12, Mattias Engdegård wrote:
> >> [[[
> >> * subversion/include/svn_types.h:
> >>   Set SVN_UNALIGNED_ACCESS_IS_OK for PowerPC.
> >> ]]]
> >
> > At first I thought, "that can't be right!" but it turns out that it is
> > ... somewhat. Only if we're sure that we use that macro for integer
> > access only. Which should be the case.
> 
> Added documentation that this is for integer access and audited the existing
> uses of this macro, it's all integer accesses.

Hmm, when checking those places, I'm not sure whether modern compilers
with their auto-vectorizing optimizers really profit from the hand-optimized
code there, or whether that code might rather trouble the optimizer into
generating suboptimal code.

 
> For anyone that's interested on the PowerPC details see:
> http://www.ibm.com/developerworks/library/pa-dalign/




Best regards

Markus Schaber

CODESYS® a trademark of 3S-Smart Software Solutions GmbH

Inspiring Automation Solutions

3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50

E-Mail: m.scha...@codesys.com | Web: http://www.codesys.com | CODESYS store: 
http://store.codesys.com
CODESYS forum: http://forum.codesys.com

Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade 
register: Kempten HRB 6186 | Tax ID No.: DE 167014915