Re: 1.7 timing tests: update great, checkout needs work, upgrade horrible

[Daniel Becroft]

 On Tue, Dec 7, 2010 at 2:01 PM, Blair Zajac  wrote:

> I'm working with a client and they have a large source source code checkout
> that takes over 10 minutes do to an svn update.  Their 1.6.x working copy
> size is 3.6 GB with 74,000 files and 19,230 directories.  The amount of IO
> to do all the locking and reading entries files is taking a significant
> amount of time.
>
> They are looking at techniques to speed up updates, as it's a significant
> productivity drain, but it's much slower than git-svn.
>
> So we thought we'd try the 1.7 client at r1042760.
>
> The good news is that "svn update" is significantly faster than 1.6,
> dropping from over 10 minutes to 3:07 with a cold cache and 0:48 with a warm
> cache.  This is great.
>
> Unfortunately, it looks like checkout times have gotten worse.  On a SATA
> disk with ext3:
>
>Clock   System  User
>timetimetime
>
> 1.6  5:4194.1   58.9
> 1.7 12:20   106.2   99.9
>
> I noticed that doing an strace of the checkout leads to a 1.3 GB file with
> 1.6.x and 4.1 GB with 1.7.x, so many more operations.
>
> I haven't been following 1.7 performance lately, but are we aware of this?
>  Are there any obvious performance fixes here?
>
> I realize that 1.7 is a ways away, but the scalability svn update brings to
> svn checkouts is a significant win.
>
> We also tried an 1.6.x checkout followed by a 1.7 upgrade.  Last I looked
> the process it had 110 minutes of CPU time.  Definitely not worth upgrading,
> faster to do a fresh checkout, even with the checkout performance slower
> than 1.6.x.
>

I've found the same problem during my testing[1]. I've only recently built
my own version of 1.7.x under ubuntu, and haven't had a chance to retest the
upgrade with this build. On Windows, I found that after a period of time,
the upgrade process would start reading the entire wc.db file continuously.
This was when the file started to get up around the 4Mb mark. I'm not sure
if it's a sqlite problem, or a table scan issue with a query, or some other
problem.

Cheers,
Daniel B.

[1] http://svn.haxx.se/dev/archive-2010-11/0503.shtml

Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h

[Blair Zajac]

On 12/6/10 7:47 AM, Julian Foad wrote:

On Sun, 2010-12-05, Stefan Fuhrmann wrote:

I've felt kind of uneasy about that issue as well.
However, it would have been difficult to implement
a deprecated svn_checksum_to_cstring() in terms
of svn_checksum_to_cstring2().


I don't think Blair was concerned about how the function is implemented,
but about the API promises.  (He can correct me if I'm wrong.)


Nope, that's correct.


But, as I said in my previous reply in this thread, I think it's fine to
just change the existing API as you have done.


Well, I don't agree, for problems that it could cause later, but don't feel that 
strongly to argue about it :)


Blair

1.7 timing tests: update great, checkout needs work, upgrade horrible

[Blair Zajac]

I'm working with a client and they have a large source source code checkout that 
takes over 10 minutes do to an svn update.  Their 1.6.x working copy size is 3.6 
GB with 74,000 files and 19,230 directories.  The amount of IO to do all the 
locking and reading entries files is taking a significant amount of time.


They are looking at techniques to speed up updates, as it's a significant 
productivity drain, but it's much slower than git-svn.


So we thought we'd try the 1.7 client at r1042760.

The good news is that "svn update" is significantly faster than 1.6, dropping 
from over 10 minutes to 3:07 with a cold cache and 0:48 with a warm cache.  This 
is great.


Unfortunately, it looks like checkout times have gotten worse.  On a SATA disk 
with ext3:


Clock   System  User
timetimetime

1.6  5:4194.1   58.9
1.7 12:20   106.2   99.9

I noticed that doing an strace of the checkout leads to a 1.3 GB file with 1.6.x 
and 4.1 GB with 1.7.x, so many more operations.


I haven't been following 1.7 performance lately, but are we aware of this?  Are 
there any obvious performance fixes here?


I realize that 1.7 is a ways away, but the scalability svn update brings to svn 
checkouts is a significant win.


We also tried an 1.6.x checkout followed by a 1.7 upgrade.  Last I looked the 
process it had 110 minutes of CPU time.  Definitely not worth upgrading, faster 
to do a fresh checkout, even with the checkout performance slower than 1.6.x.


Blair

Re: 1.7.x - merge now accesses all files in WC?

[Hyrum K. Wright]

On Mon, Dec 6, 2010 at 4:28 PM, Hyrum K. Wright
 wrote:
> On Mon, Dec 6, 2010 at 3:15 PM, Daniel Becroft  wrote:
>> On Mon, Dec 6, 2010 at 8:50 PM, Daniel Becroft  wrote:
>>
>>> On Mon, Dec 6, 2010 at 7:13 PM, Daniel Shahaf 
>>> wrote:
>>>
 Instead of guessing which function causes the lstat() calls, could we
 have a tool tell?

 I've looked at 'ltrace -S', but it seems to me that will only tell us
 which public svn_wc_* API is responsible for the calls.  Perhaps there
 is another tool that can help us follow the entire call chain down to
 the lstat() call?

 Daniel
 . o O (gdb with a breakpoint in lstat() and some fu to extract
 statistics about the stack trace?)

>>>
>>> Very true.
>>>
>>> Okay, after a crash course in using GDB, I've run some further testing.
>>> Running with breakpoints set on the lstat() system call, it looks to be the
>>> known issue that was originally pointed to. A partial stack trace is below:
>>>
>>> #0  0x7644ab45 in __lxstat (vers=,
>>>     name=, buf=0x7fffcbf0)
>>>     at ../sysdeps/unix/sysv/linux/wordsize-64/lxstat.c:38
>>> #1  0x769269b6 in lstat (finfo=0x7fffccf0,
>>>     fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     wanted=33137, pool=0x6bd588) at /usr/include/sys/stat.h:464
>>> #2  apr_stat (finfo=0x7fffccf0,
>>>     fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     wanted=33137, pool=0x6bd588) at file_io/unix/filestat.c:292
>>> #3  0x76fb1199 in io_check_path (
>>>     path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     resolve_symlinks=0, is_special_p=0x7fffced8, kind=0x7fffcedc,
>>>     pool=0x6bd588) at subversion/libsvn_subr/io.c:222
>>> #4  0x76fb1346 in svn_io_check_special_path (
>>>     path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     kind=0x7fffcedc, is_special=0x7fffced8, pool=0x6bd588)
>>>     at subversion/libsvn_subr/io.c:283
>>> #5  0x7794513d in svn_wc__db_pdh_parse_local_abspath (
>>>     pdh=0x7fffd038, local_relpath=0x7fffd030, db=0x64ad38,
>>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     smode=svn_sqlite__mode_readwrite, result_pool=0x6bd588,
>>>     scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db_pdh.c:382
>>> #6  0x77935d58 in svn_wc__db_read_info (status=0x7fffd184,
>>>     kind=0x7fffd188, revision=0x0, repos_relpath=0x0,
>>> repos_root_url=0x0,
>>>     repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0,
>>>     last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0,
>>>     target=0x0, changelist=0x0, original_repos_relpath=0x0,
>>>     original_root_url=0x0, original_uuid=0x0, original_revision=0x0,
>>>     props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0,
>>>     db=0x64ad38,
>>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     result_pool=0x6bd588, scratch_pool=0x6bd588)
>>>     at subversion/libsvn_wc/wc_db.c:5261
>>>
>>> However, I then put the breakpont in the svn_wc__db_read_info function and,
>>> just for A/beta.txt, it gets called 7 times, when in terms leads to 7
>>> additional lstat() calls. Looking further the call stack appears as follows:
>>>
>>> #0  svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188,
>>>     revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0,
>>>     changed_rev=0x0, changed_date=0x0, changed_author=0x0,
>>> last_mod_time=0x0,
>>>     depth=0x0, checksum=0x0, translated_size=0x0, target=0x0,
>>> changelist=0x0,
>>>     original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0,
>>>     original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0,
>>>     conflicted=0x0, lock=0x0, db=0x64ad38,
>>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>>     result_pool=0x6bd588, scratch_pool=0x6bd588)
>>>     at subversion/libsvn_wc/wc_db.c:5259
>>> #1  0x778fab96 in walker_helper (db=0x64ad38,
>>>     dir_abspath=0x6ba288 "/home/djcbecroft/dev/workingcopy/A",
>>> show_hidden=1,
>>>     walk_callback=0x77ba9026 ,
>>>     walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
>>>     cancel_func=0x4119c3 , cancel_baton=0x0,
>>>     scratch_pool=0x6ba208) at subversion/libsvn_wc/node.c:679
>>> #2  0x778facb5 in walker_helper (db=0x64ad38,
>>>     dir_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy", show_hidden=1,
>>>
>>>     walk_callback=0x77ba9026 ,
>>>     walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
>>>     cancel_func=0x4119c3 , cancel_baton=0x0,
>>>     scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:716
>>> #3  0x778faf9a in svn_wc__internal_walk_children (db=0x64ad38,
>>>     local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
>>> show_hidden=1,
>>>     walk_callback=0x77ba9026 ,
>>>     walk_baton=0x7fffd4e0, walk_depth=svn_depth_infinity,
>

Re: [PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed (v. 4)

[Danny Trebbien]

> Just about to commit this ... FAIL: autoprop_tests.py 15, blame_tests.py
> 7, commit_tests.py 40, ...
>
> subst.c' line 668: assertion failed (STRING_IS_EOL(eol_str,
> eol_str_len))
>
> Whassup?  Attaching my version, as I'm out of time tonight.  Hope I
> didn't mess it up.  I was testing tr...@1042741 (patched as attached),
> svnserve and FSFS, in case it matters.

Oops. I never ran `make check`. I will have to look into this...

Re: 1.7.x - merge now accesses all files in WC?

[Hyrum K. Wright]

On Mon, Dec 6, 2010 at 3:15 PM, Daniel Becroft  wrote:
> On Mon, Dec 6, 2010 at 8:50 PM, Daniel Becroft  wrote:
>
>> On Mon, Dec 6, 2010 at 7:13 PM, Daniel Shahaf wrote:
>>
>>> Instead of guessing which function causes the lstat() calls, could we
>>> have a tool tell?
>>>
>>> I've looked at 'ltrace -S', but it seems to me that will only tell us
>>> which public svn_wc_* API is responsible for the calls.  Perhaps there
>>> is another tool that can help us follow the entire call chain down to
>>> the lstat() call?
>>>
>>> Daniel
>>> . o O (gdb with a breakpoint in lstat() and some fu to extract
>>> statistics about the stack trace?)
>>>
>>
>> Very true.
>>
>> Okay, after a crash course in using GDB, I've run some further testing.
>> Running with breakpoints set on the lstat() system call, it looks to be the
>> known issue that was originally pointed to. A partial stack trace is below:
>>
>> #0  0x7644ab45 in __lxstat (vers=,
>>     name=, buf=0x7fffcbf0)
>>     at ../sysdeps/unix/sysv/linux/wordsize-64/lxstat.c:38
>> #1  0x769269b6 in lstat (finfo=0x7fffccf0,
>>     fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     wanted=33137, pool=0x6bd588) at /usr/include/sys/stat.h:464
>> #2  apr_stat (finfo=0x7fffccf0,
>>     fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     wanted=33137, pool=0x6bd588) at file_io/unix/filestat.c:292
>> #3  0x76fb1199 in io_check_path (
>>     path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     resolve_symlinks=0, is_special_p=0x7fffced8, kind=0x7fffcedc,
>>     pool=0x6bd588) at subversion/libsvn_subr/io.c:222
>> #4  0x76fb1346 in svn_io_check_special_path (
>>     path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     kind=0x7fffcedc, is_special=0x7fffced8, pool=0x6bd588)
>>     at subversion/libsvn_subr/io.c:283
>> #5  0x7794513d in svn_wc__db_pdh_parse_local_abspath (
>>     pdh=0x7fffd038, local_relpath=0x7fffd030, db=0x64ad38,
>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     smode=svn_sqlite__mode_readwrite, result_pool=0x6bd588,
>>     scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db_pdh.c:382
>> #6  0x77935d58 in svn_wc__db_read_info (status=0x7fffd184,
>>     kind=0x7fffd188, revision=0x0, repos_relpath=0x0,
>> repos_root_url=0x0,
>>     repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0,
>>     last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0,
>>     target=0x0, changelist=0x0, original_repos_relpath=0x0,
>>     original_root_url=0x0, original_uuid=0x0, original_revision=0x0,
>>     props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0,
>>     db=0x64ad38,
>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     result_pool=0x6bd588, scratch_pool=0x6bd588)
>>     at subversion/libsvn_wc/wc_db.c:5261
>>
>> However, I then put the breakpont in the svn_wc__db_read_info function and,
>> just for A/beta.txt, it gets called 7 times, when in terms leads to 7
>> additional lstat() calls. Looking further the call stack appears as follows:
>>
>> #0  svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188,
>>     revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0,
>>     changed_rev=0x0, changed_date=0x0, changed_author=0x0,
>> last_mod_time=0x0,
>>     depth=0x0, checksum=0x0, translated_size=0x0, target=0x0,
>> changelist=0x0,
>>     original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0,
>>     original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0,
>>     conflicted=0x0, lock=0x0, db=0x64ad38,
>>     local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
>>     result_pool=0x6bd588, scratch_pool=0x6bd588)
>>     at subversion/libsvn_wc/wc_db.c:5259
>> #1  0x778fab96 in walker_helper (db=0x64ad38,
>>     dir_abspath=0x6ba288 "/home/djcbecroft/dev/workingcopy/A",
>> show_hidden=1,
>>     walk_callback=0x77ba9026 ,
>>     walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
>>     cancel_func=0x4119c3 , cancel_baton=0x0,
>>     scratch_pool=0x6ba208) at subversion/libsvn_wc/node.c:679
>> #2  0x778facb5 in walker_helper (db=0x64ad38,
>>     dir_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy", show_hidden=1,
>>
>>     walk_callback=0x77ba9026 ,
>>     walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
>>     cancel_func=0x4119c3 , cancel_baton=0x0,
>>     scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:716
>> #3  0x778faf9a in svn_wc__internal_walk_children (db=0x64ad38,
>>     local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
>> show_hidden=1,
>>     walk_callback=0x77ba9026 ,
>>     walk_baton=0x7fffd4e0, walk_depth=svn_depth_infinity,
>>     cancel_func=0x4119c3 , cancel_baton=0x0,
>>     scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:763
>> #4  0x778fb06d in svn_wc__node_wa

Re: 1.7.x - merge now accesses all files in WC?

[Daniel Becroft]

On Mon, Dec 6, 2010 at 8:50 PM, Daniel Becroft  wrote:

> On Mon, Dec 6, 2010 at 7:13 PM, Daniel Shahaf wrote:
>
>> Instead of guessing which function causes the lstat() calls, could we
>> have a tool tell?
>>
>> I've looked at 'ltrace -S', but it seems to me that will only tell us
>> which public svn_wc_* API is responsible for the calls.  Perhaps there
>> is another tool that can help us follow the entire call chain down to
>> the lstat() call?
>>
>> Daniel
>> . o O (gdb with a breakpoint in lstat() and some fu to extract
>> statistics about the stack trace?)
>>
>
> Very true.
>
> Okay, after a crash course in using GDB, I've run some further testing.
> Running with breakpoints set on the lstat() system call, it looks to be the
> known issue that was originally pointed to. A partial stack trace is below:
>
> #0  0x7644ab45 in __lxstat (vers=,
> name=, buf=0x7fffcbf0)
> at ../sysdeps/unix/sysv/linux/wordsize-64/lxstat.c:38
> #1  0x769269b6 in lstat (finfo=0x7fffccf0,
> fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> wanted=33137, pool=0x6bd588) at /usr/include/sys/stat.h:464
> #2  apr_stat (finfo=0x7fffccf0,
> fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> wanted=33137, pool=0x6bd588) at file_io/unix/filestat.c:292
> #3  0x76fb1199 in io_check_path (
> path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> resolve_symlinks=0, is_special_p=0x7fffced8, kind=0x7fffcedc,
> pool=0x6bd588) at subversion/libsvn_subr/io.c:222
> #4  0x76fb1346 in svn_io_check_special_path (
> path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> kind=0x7fffcedc, is_special=0x7fffced8, pool=0x6bd588)
> at subversion/libsvn_subr/io.c:283
> #5  0x7794513d in svn_wc__db_pdh_parse_local_abspath (
> pdh=0x7fffd038, local_relpath=0x7fffd030, db=0x64ad38,
> local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> smode=svn_sqlite__mode_readwrite, result_pool=0x6bd588,
> scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db_pdh.c:382
> #6  0x77935d58 in svn_wc__db_read_info (status=0x7fffd184,
> kind=0x7fffd188, revision=0x0, repos_relpath=0x0,
> repos_root_url=0x0,
> repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0,
> last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0,
> target=0x0, changelist=0x0, original_repos_relpath=0x0,
> original_root_url=0x0, original_uuid=0x0, original_revision=0x0,
> props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0,
> db=0x64ad38,
> local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> result_pool=0x6bd588, scratch_pool=0x6bd588)
> at subversion/libsvn_wc/wc_db.c:5261
>
> However, I then put the breakpont in the svn_wc__db_read_info function and,
> just for A/beta.txt, it gets called 7 times, when in terms leads to 7
> additional lstat() calls. Looking further the call stack appears as follows:
>
> #0  svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188,
> revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0,
> changed_rev=0x0, changed_date=0x0, changed_author=0x0,
> last_mod_time=0x0,
> depth=0x0, checksum=0x0, translated_size=0x0, target=0x0,
> changelist=0x0,
> original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0,
> original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0,
> conflicted=0x0, lock=0x0, db=0x64ad38,
> local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
> result_pool=0x6bd588, scratch_pool=0x6bd588)
> at subversion/libsvn_wc/wc_db.c:5259
> #1  0x778fab96 in walker_helper (db=0x64ad38,
> dir_abspath=0x6ba288 "/home/djcbecroft/dev/workingcopy/A",
> show_hidden=1,
> walk_callback=0x77ba9026 ,
> walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
> cancel_func=0x4119c3 , cancel_baton=0x0,
> scratch_pool=0x6ba208) at subversion/libsvn_wc/node.c:679
> #2  0x778facb5 in walker_helper (db=0x64ad38,
> dir_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy", show_hidden=1,
>
> walk_callback=0x77ba9026 ,
> walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
> cancel_func=0x4119c3 , cancel_baton=0x0,
> scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:716
> #3  0x778faf9a in svn_wc__internal_walk_children (db=0x64ad38,
> local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
> show_hidden=1,
> walk_callback=0x77ba9026 ,
> walk_baton=0x7fffd4e0, walk_depth=svn_depth_infinity,
> cancel_func=0x4119c3 , cancel_baton=0x0,
> scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:763
> #4  0x778fb06d in svn_wc__node_walk_children (wc_ctx=0x64ad20,
> local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
> show_hidden=1,
> walk_callback=0x77ba9026 ,
>  

Re: gpg-agent branch treats PGP passphrase as repository password?

[Daniel Shahaf]

Greg Hudson wrote on Mon, Dec 06, 2010 at 10:31:05 -0500:
> On Mon, 2010-12-06 at 07:30 -0500, Daniel Shahaf wrote:
> > Ideally, Subversion won't know the PGP passphrase.  (If it does, then
> > a malicious libsvn_subr can compromise a private key.)
> 
> I think you're trying to solve a different problem here.  The goal is to
> minimize typing of passwords without storing passwords in a fixed
> medium, not to protect keys against malicious or broken Subversion code.
> 

Agreed, there are two different problems here:

P1.  Minimize typing of passwords.
S1a. Cache the password in gpg-agent.

P2.  Do not store passwords on disk in plaintext.
S2a. Encrypt the passwords with GPGME before storing them to disk.

I was commenting on Stefan's suggestion that Subversion should
explicitly know the passphrase of some PGP key.  (I'm assuming some
people will use for this purpose the same PGP key they use for email
communications.)

> > For comparison, the ssh-agent protocol[1] only allows a client of the
> > agent to authenticate himself (using the agent) to a third party, but
> > does not have a "Retrieve secret key" option [2].  If we are to use PGP,
> > could we find a solution with similar properties?
> 
> ssh-agent has special knowledge of the operations which will be
> performed using the keying material.  PGP probably doesn't have any
> interest in the operations Subversion needs to do with passwords.
> 

I'm not sure I understand this comment.  Originally, I assumed that PGP
might implement a generic   lambda ctxt: decrypt(pgp_sk, ctxt)   interface.

> PKCS#11 is the most commonly used general API for operations where an
> application can use a key but isn't allowed to know what it is.  The
> most useful free software implementation of PKCS#11 is probably NSS.  I
> don't think we want to go there, though.
> 

We only have the "Subversion isn't allowed to know the key" requirement
if that key is used by something other than Subversion.  Therefore, an
alternative is to require keys that are used only by Subversion (so
leaking the passphrases compromises nothing but ~/.subversion/auth/):

* we could store a secret gpg keyring under ~/.subversion/.gnupg/ :-)

* we could use symmetric passwords (as I suggested elsethread)

> 

Thanks for your comments,

Daniel

Re: svn commit: r1040832 - Port a fix for a FSFS packing race

[Philip Martin]

Julian Foad  writes:

> I'm going to drop this "Remove the re-try logic from
> svn_fs_fs__path_rev_absolute()" follow-up patch, as I don't want to get
> into checking or messing with the txn-correctness of FSFS now.  If
> Daniel or anyone else wants to pursue it, I'd be glad to help.

I thought the patch was an improvement.  The existing retry in
svn_fs_fs__path_rev_absolute doesn't fix the race, it simply makes the
window smaller.  As the patch makes the window larger we are more likely
to see and fix any places where the caller doesn't handle the race.

-- 
Philip

Re: [PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed (v. 4)

[Julian Foad]

On Tue, 2010-11-30, Danny Trebbien wrote:
> Attached is version 4 of the patch and corresponding log message that
> address Daniel Shahaf's feedback from November 9.
> 
> Per a message from Julian
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/124073)
> and Daniel Shahaf
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/124082),
> I have removed the changes to optimize translate_newline() for now.

Thanks, Danny.

This looks fine.

I added doc strings to the new static functions stream_translated() and
translate_cstring().  I clarified in all doc strings whether
TRANSLATED_EOL is set to FALSE or untouched if there is no translation.
(In some cases you documented these more in the log message than in the
code :-)

Just about to commit this ... FAIL: autoprop_tests.py 15, blame_tests.py
7, commit_tests.py 40, ...

subst.c' line 668: assertion failed (STRING_IS_EOL(eol_str,
eol_str_len))

Whassup?  Attaching my version, as I'm out of time tonight.  Hope I
didn't mess it up.  I was testing tr...@1042741 (patched as attached),
svnserve and FSFS, in case it matters.

Cheers,
- Julian


Add a public API function, svn_subst_translate_string2(), an extension of
svn_subst_translate_string(), that has two additional output parameters for
determining whether re-encoding and/or line ending translation were performed.

As discussed at:
  
  

The essential changes are to the translate_newline() function, which now takes
an svn_boolean_t pointer, the value at which is set to TRUE if the pointer is
non-NULL and a different newline is written out. Most other changes are to pass
the svn_boolean_t pointer through to translate_newline().

* subversion/include/svn_subst.h
  (svn_subst_translate_string2): New function.
  (svn_subst_translate_string): Deprecate in favor of
svn_subst_translate_string2().

* subversion/libsvn_subr/subst.c
  (STRING_IS_EOL): New macro that tests whether a string is an end-of-line
string ("\n", "\r", "\r\n").
  (DIFFERENT_EOL_STRINGS): New macro that tests whether two end-of-line strings
are different.
  (translate_newline): Add the TRANSLATED_EOL parameter. If the function
writes out a different newline, then it sets TRANSLATED_EOL to TRUE.
  (translation_baton): Add the TRANSLATED_EOL field.
  (create_translation_baton): Add a new parameter TRANSLATED_EOL that is
passed to the resulting translation_baton.
  (translate_chunk): When calling translate_newline(), pass TRANSLATED_EOL from
the translation_baton.
  (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_cstring): 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_cstring().
  (svn_subst_translate_string): Move to deprecated.c.
  (svn_subst_translate_string2): New function. It takes three additional
parameters: TRANSLATED_TO_UTF8, TRANSLATED_LINE_ENDINGS, and another pool
parameter. The task of recording whether it translates a line ending is
delegated to translate_cstring().

* subversion/libsvn_subr/deprecated.c
  (svn_subst_translate_string): Now a wrapper for svn_subst_translate_string2().

Patch by: Danny Trebbien 
--This line, and those below, will be ignored--

Index: subversion/include/svn_subst.h
===
--- subversion/include/svn_subst.h	(revision 1042741)
+++ subversion/include/svn_subst.h	(working copy)
@@ -592,19 +592,46 @@ svn_subst_stream_detranslated(svn_stream_t **strea
 
 /* EOL conversion and character encodings */
 
+/** Similar to svn_subst_translate_string2(), except that the information about
+ * whether re-encoding or line ending translation were performed is discarded.
+ *
+ * @deprecated Provided for backward compatibility with the 1.6 API.
+ */
+SVN_DEPRECATED
+svn_error_t *svn_subst_translate_string(svn_string_t **new_value,
+const svn_string_t *value,
+const char *encoding,
+apr_pool_t *pool);
+
 /** Translate the string @a value from character encoding @a encoding to
  * UTF8, and also from its current line-ending style to LF line-endings.  If
  * @a encoding is @c NULL, translate from the system-default encoding.
  *
+ * If @a translated_to_utf8 is not @c NULL, then set @a *translated_to_utf8
+ * to @c TRUE if at lea

Re: svn commit: r1040832 - Port a fix for a FSFS packing race

[Julian Foad]

I'm going to drop this "Remove the re-try logic from
svn_fs_fs__path_rev_absolute()" follow-up patch, as I don't want to get
into checking or messing with the txn-correctness of FSFS now.  If
Daniel or anyone else wants to pursue it, I'd be glad to help.

- Julian


On Thu, 2010-12-02, Daniel Shahaf wrote:
> Julian Foad wrote on Thu, Dec 02, 2010 at 14:33:19 +:
> > On Thu, 2010-12-02 at 14:56 +0200, Daniel Shahaf wrote:
> > > Julian Foad wrote on Thu, Dec 02, 2010 at 12:15:19 +:
> > > > Remove the re-try logic from svn_fs_fs__path_rev_absolute().  Since
> > > > r1040832, all its callers correctly account for the possibility of an
> > > > out-of-date result due to a concurrent packing operation.
> > > > 
> > > > The re-try logic was introduced in r875097 and reduced but did not 
> > > > eliminate
> > > > the window of opportunity for the caller to use an out-of-date result.
> > > > 
> > > > See the email thread 
> > > > ,
> > > > subject "Re: svn commit: r1040832 - Port a fix for a FSFS packing race".
> > > > 
> > > > * subversion/libsvn_fs_fs/fs_fs.c
> > > >   (svn_fs_fs__path_rev_absolute): Remove the re-try logic.
> > > > 
> > > > * subversion/libsvn_fs_fs/fs_fs.h
> > > >   (svn_fs_fs__path_rev_absolute): Update the doc string accordingly.
> > > >  
> > > 
> > > In fact, doesn't the correctness of this change depend on the fact that
> > > svn_fs_fs__with_write_lock() calls update_min_unpacked_rev() before
> > > executing the body() callback?
> > 
> > Yes, it does.  Do you think it shouldn't?
> > 
> 
> I'm not sure whether it "should" or "shouldn't" depend.  The change IS
> correct, but it is only correct because the ffd->min_unpacked_rev is
> known to be up-to-date whenever the write lock is held.
> 
> That's a bit of a spaghetti effect there.  Now, is there another such
> effect that means the patch is wrong?  (I haven't come up with one yet,
> but that doesn't mean there isn't any.)
> 
> > >   (Otherwise we don't know whether there
> > > is a "concurrent" packer, or just ffd->min_unpacked_rev is stale.)
> > 
> > You know, I'm not too clear on the design intention here.  I would have
> > assumed it would be the job of all the fs_fs.c code to ensure that
> > ffd->min_unpacked_rev is never stale (due to its own actions) at any
> > point where it matters.  But the way it does that seems to be to re-read
> > the file in several places, instead of simply updating
> > ffd->min_unpacked_rev when it writes the file.  Maybe the idea was that
> > that method would pick up both internal and concurrent (external)
> > changes in the same way - I don't know.  Seems odd.
> > 
> 
> Yes, that was the idea.  If two processes access the filesystem at the
> same time, and one of them calls svn_fs_pack(), then the other's
> ffd->min_unpacked_rev would be stale.
> 
> It's the same story with ffd->youngest_rev, by the way; compare
> ensure_revision_exists(), which I believe predates the packing logic.
> 
> > Do you have an idea whether something should be done differently?  I
> > don't, not without studying it further.
> > 
> 
> I'm not sure.  Ultimately, ffd->min_unpacked_rev is just a cache, so we
> have to update it when we're about to do something that depends on its
> value.
> 
> In the meantime, let me at least attempt to define the "something" which
> perhaps should be done differently:
> 
> Given the way packing works today, it's possible for a revision file to
> stop existing where you expected it to exist.  Today the code informs
> itself of that situation by re-reading min-packed-rev and retrying.
> (Further up the stack, the code calculating the offset to seek to also
> needs to know whether it's seeking a pack file or a revision file.)
> 
> > I note that the following comment in pack_shard() is not quite right:
> > 
> >   /* Update the min-unpacked-rev file to reflect our newly packed shard.
> >* (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().)
> > 
> > That may or may not be true, but it doesn't seem like this function has
> > any right to make that assertion.
> > 
> 
> We could remove the comment and have that function call 
> update_min_unpacked_rev().
> (This would also save us a failed disk access later.)
> 
> But is it strictly *necessary* to do so?  I think not: by not calling
> update_min_unpacked_rev(), we cause ffd->min_unpacked_rev to become
> stale --- a situation which the code has to account for anyway.
> 
> > And in pack_revprop_shard(), it's the same, verbatim:
> > 
> >   /* Update the min-unpacked-rev file to reflect our newly packed shard.
> > 
> > should say 'the min-unpacked-revprop file';
> > 
> >* (ffd->min_unpacked_rev will be updated by open_pack_or_rev_file().)
> > 
> > and that second line looks completely bogus in this context.
> > 
> 
> Yes, whoever did the copy-paste forgot to update some places.
> 
> > 
> > > > Index: subversion/libsvn_fs_fs/fs_fs.h
> > > > ==

Re: Input validation observations

[Julian Foad]

On Sat, 2010-12-04, Noorul Islam K M wrote:
> Julian Foad  writes:
> > Noorul Islam K M wrote:
> >> Julian Foad  writes:
> >> >   * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> >> > in revision REV"?
[...]
> >> -  SVN_ERR(svn_cmdline_fprintf
> >> -  (stderr, subpool,
> >> -   _("%s:  (Not a valid URL)\n\n"),
> >> -   svn_path_is_url(truepath)
> >> - ? truepath
> >> - : svn_dirent_local_style(truepath, pool)));
> >> +  SVN_ERR(svn_cmdline_fprintf (stderr, subpool, 
> >> + _("%s\n\n"), 
> >> err->message));
> >
> > Unfortunately we cannot assume that err->message is a good user-friendly
> > description of the problem.  It appears that it *is* in the specific
> > case we're looking at:
> >
> > $ svn info ^/b
> > URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
> > 1041775
> >
> > That's lovely.  But in other cases it's not:
> >
> > $ svn info hhh://aa.cc.dd/foo
> > traced call
> >
> > See how err->message is just "traced call" in this case.  We can't even
> > assume it is not NULL, in fact.
> >
> > I can think of two options.  We could try to use one of the
> > svn_error_*() functions to print out a "nice" description of the actual
> > returned error.  Alternatively, we could ignore 'err' and print a simple
> > message here, like the existing code is doing but saying something more
> > helpful than "Not a valid URL".  What do you think?
> >
> > Does the documentation for svn_client_info3() say under what conditions
> > it returns the error code SVN_ERR_RA_ILLEGAL_URL?  Does that help?
> 
> Attached is the updated patch using svn_err_best_message() 

Hi Noorul.

Thank you for the updated patch.  Even though this is a very
simple-looking patch, I need a bit more information to help me review
it.

Do you think both of the options I suggested are possible solutions?
What are the advantages of the option you chose?  What disadvantages do
you know about?  What are the risks with it - in what ways could it
possibly go wrong or make a user unhappy?  For example, what other error
conditions might this code possibly handle?  Which of them did you try?
Show us the results.  Do you think they are user-friendly?

Checking those sorts of things normally takes much more effort than
actually writing the few lines of source code.  That's all part of
making a patch.  Whenever you send a patch, it's helpful to say these
things.  When the reviewer knows what's already been checked, he or she
can then concentrate on verifying the correctness of the reasoning and
of the code.

Please take a little extra time to provide this help.

Thank you in advance.

- Julian


> -  SVN_ERR(svn_cmdline_fprintf
> -  (stderr, subpool,
> -   _("%s:  (Not a valid URL)\n\n"),
> -   svn_path_is_url(truepath)
> - ? truepath
> - : svn_dirent_local_style(truepath, pool)));
> +  SVN_ERR(svn_cmdline_fprintf(
> +stderr, subpool, _("%s\n\n"), 
> +svn_err_best_message(err, errbuf, sizeof(errbuf;

Re: Input validation observations

[Julian Foad]

On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
> Julian Foad  writes:
> 
> > Noorul Islam K M wrote:
> >
> >> Julian Foad  writes:
> >> >   * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> >> > mix URL and local targets"?
> > [...]
> >> Make 'svn mkdir' verify that both working copy paths and URLs are
> >> not passed.
> >> 
> >> * subversion/svn/mkdir-cmd.c,
> >>   subversion/libsvn_client/add.c
> >>   (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
> >>   copy paths and URLs are passed.
> >> 
> >> * subversion/tests/cmdline/input_validation_tests.py
> >>   (invalid_mkdir_targets, test_list): New test
> > [...]
> >> Index: subversion/svn/mkdir-cmd.c
> >> ===
> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
> >> +++ subversion/svn/mkdir-cmd.c (working copy)
> >> @@ -48,6 +48,8 @@
> >> +  svn_boolean_t wc_present = FALSE, url_present = FALSE;
> >> +  int i;
> >> @@ -56,6 +58,22 @@
> >> +  /* Check to see if at least one of our paths is a working copy
> >> + path or a repository url. */
> >> +  for (i = 0; i < targets->nelts; ++i)
> >> +{
> >> +  const char *target = APR_ARRAY_IDX(targets, i, const char *);
> >> +  if (! svn_path_is_url(target))
> >> +   wc_present = TRUE;
> >> +  else
> >> +   url_present = TRUE;
> >> +}
> >> +
> >> +  if (url_present && wc_present)
> >> +return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> >> +_("Cannot mix repository and working copy "
> >> +  "targets"));
> >
> > This is fine.
> >
> > The same code already exists in three other files and equivalent but
> > different code also exists in at least delete-cmd.c and probably other
> > files.  I think it is time to factor it out.  We can do that in a
> > subsequent patch.
> 
> Do you mean we need to come up with new function that will do this check
> and return the error message?

Yes please.

- Julian

Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h

[Julian Foad]

On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> On 02.12.2010 07:57, Blair Zajac wrote:
> > On 12/1/10 4:38 PM, stef...@apache.org wrote:
> >> Author: stefan2
> >> Date: Thu Dec  2 00:38:17 2010
> >> New Revision: 1041230
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1041230&view=rev
> >> Log:
> >> Fix the svn_checksum_to_cstring() docstring to actually say what
> >> was intended. Also, make clear that the behavior is new for 1.7 and
> >> trying to use it in 1.6 will cause segfaults.
> >>
> >> * subversion/include/svn_checksum.h
> >>(svn_checksum_to_cstring): fix docstring
> >
> > What happens if somebody makes a svn tool that is compiled and built 
> > against the new 1.7 behavior and then it is backported to 1.6, it may 
> > core dump.
> >
> > Should we add a svn_checksum_to_cstring2() instead with the new 
> > behavior or backport this change to 1.6?  But even then we'll have 1.6 
> > versions with different behavior.  It seems making a new 
> > svn_checksum_to_cstring2() is better.

> I've felt kind of uneasy about that issue as well.
> However, it would have been difficult to implement
> a deprecated svn_checksum_to_cstring() in terms
> of svn_checksum_to_cstring2().

I don't think Blair was concerned about how the function is implemented,
but about the API promises.  (He can correct me if I'm wrong.)

> For instance: [...]
> {
>if (checksum == NULL)
>  abort();
> 
>return svn_checksum_to_cstring2(checksum, pool);
> }
> 
> does not have the exact same behavior as the old
> implementation. [...]

That doesn't matter.  Passing checksum=NULL was not a supported usage in
1.6.x so there is no need to preserve compatibility with that failure
mode.  It would be perfectly acceptable to implement it as:

const char *
svn_checksum_to_cstring(const svn_checksum_t *checksum,
 apr_pool_t *pool)
{
   return svn_checksum_to_cstring2(checksum, pool);
}

But, as I said in my previous reply in this thread, I think it's fine to
just change the existing API as you have done.

- Julian

Re: [PATCH] Combine redundant 'if' block.

[C. Michael Pilato]

On 12/04/2010 04:08 AM, Noorul Islam K M wrote:
> * subversion/libsvn_client/copy.c
>   (try_copy): Combine redundant 'if' block.
> 
> Patch by: Noorul Islam K M 

   Sendingtrunk/subversion/libsvn_client/copy.c
   Transmitting file data .
   Committed revision 1042679.

Thanks!

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature

Re: gpg-agent branch treats PGP passphrase as repository password?

[Greg Hudson]

On Mon, 2010-12-06 at 07:30 -0500, Daniel Shahaf wrote:
> Ideally, Subversion won't know the PGP passphrase.  (If it does, then
> a malicious libsvn_subr can compromise a private key.)

I think you're trying to solve a different problem here.  The goal is to
minimize typing of passwords without storing passwords in a fixed
medium, not to protect keys against malicious or broken Subversion code.

> For comparison, the ssh-agent protocol[1] only allows a client of the
> agent to authenticate himself (using the agent) to a third party, but
> does not have a "Retrieve secret key" option [2].  If we are to use PGP,
> could we find a solution with similar properties?

ssh-agent has special knowledge of the operations which will be
performed using the keying material.  PGP probably doesn't have any
interest in the operations Subversion needs to do with passwords.

PKCS#11 is the most commonly used general API for operations where an
application can use a key but isn't allowed to know what it is.  The
most useful free software implementation of PKCS#11 is probably NSS.  I
don't think we want to go there, though.

Re: svn commit: r1040831 - in /subversion/trunk/subversion: include/svn_checksum.h libsvn_subr/checksum.c

[Julian Foad]

On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> On 02.12.2010 12:18, Julian Foad wrote:
> > A good test for whether it's worth making an API accept NULL as an input
> > is: what proportion of the callers would find that useful?  I see there
> > are about 40 callers in the code base.  Would you mind scanning through
> > them and letting us know?
> >
> There were two places where the callers had to check
> for NULL before calling svn_checksum_to_cstring().
> I removed those checks in r1042460.

Great.  And three more in load-fs-vtable.c, which I removed in r1042673.

> Two more places might try to call the function with a NULL
> checksum under adverse conditions:
> 
> * 6x in dump_node() (subversion/libsvn_repos/dump.c)
>because the svn_fs_file_checksum() is not instructed
>to calculate missing checksums
> * While constructing an error message in window_handler()
>(subversion\libsvn_wc\update_editor.c)
> 
> There are about ten more places where it is not entirely
> crystal clear that the desired checksum is actually available.
> However, I'm reasonably sure that they will in the respective
> contexts (e.g. wc operations).

Thanks for checking, Stefan.  That gives me reason to like this change.

- Julian

Re: gpg-agent branch treats PGP passphrase as repository password?

[Daniel Shahaf]

Stefan Sperling wrote on Sun, Oct 17, 2010 at 16:03:13 +0200:
> The GPGME library will probably help with these steps:
> http://www.gnupg.org/gpgme.html

By the way, the GPGME library supports symmetric (password-keyed, as
with 'gpg -c') encryptions [1], in addition to PGP assymetric
encryptions.

What is it good for?  To remember just one password (the symmetric
password) instead of N password cached by Subversion.  Otherwise it
just adds a layer of redirection...

> I haven't checked, but it's possible that this library will talk to
> the GPG agent on behalf of Subversion.

[1] 
,
documentation of recp=NULL.

Re: gpg-agent branch treats PGP passphrase as repository password?

[Daniel Shahaf]

[ crypto student hat on ]

Stefan Sperling wrote on Sun, Oct 17, 2010 at 16:03:13 +0200:
> Retreiving a password:
> Step 1: svn contacts the gpg-agent to find out the passphrase for the
> private pgp key the agent is managing. If the agent cannot be
>   contacted svn asks the user for the passphrase.
> Step 2: svn uses this passphrase to decrypt the user's PGP private key

Ideally, Subversion won't know the PGP passphrase.  (If it does, then
a malicious libsvn_subr can compromise a private key.)

For comparison, the ssh-agent protocol[1] only allows a client of the
agent to authenticate himself (using the agent) to a third party, but
does not have a "Retrieve secret key" option [2].  If we are to use PGP,
could we find a solution with similar properties?

> Step 3: svn uses this private key to decrypt the password stored
> inside the ~/.subversion/auth area
> Step 4: svn sends the decrypted password to the server
> 
> The GPGME library will probably help with these steps:
> http://www.gnupg.org/gpgme.html

It seems straightforward enough to use this for decryption.  However,
does this library provides access control?  Namely, does it allow the
end user to say "libsvn may perform decryptions, but may not export
secret keys"?

> Thanks,
> Stefan

[1] 
,
Section 2.6 and Section 3.

[2] This is documented in ssh(1) under the -A option.

Re: 1.7.x - merge now accesses all files in WC?

[Daniel Becroft]

 On Mon, Dec 6, 2010 at 7:13 PM, Daniel Shahaf wrote:

> Instead of guessing which function causes the lstat() calls, could we
> have a tool tell?
>
> I've looked at 'ltrace -S', but it seems to me that will only tell us
> which public svn_wc_* API is responsible for the calls.  Perhaps there
> is another tool that can help us follow the entire call chain down to
> the lstat() call?
>
> Daniel
> . o O (gdb with a breakpoint in lstat() and some fu to extract
> statistics about the stack trace?)
>

Very true.

Okay, after a crash course in using GDB, I've run some further testing.
Running with breakpoints set on the lstat() system call, it looks to be the
known issue that was originally pointed to. A partial stack trace is below:

#0  0x7644ab45 in __lxstat (vers=,
name=, buf=0x7fffcbf0)
at ../sysdeps/unix/sysv/linux/wordsize-64/lxstat.c:38
#1  0x769269b6 in lstat (finfo=0x7fffccf0,
fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
wanted=33137, pool=0x6bd588) at /usr/include/sys/stat.h:464
#2  apr_stat (finfo=0x7fffccf0,
fname=0x6bd638 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
wanted=33137, pool=0x6bd588) at file_io/unix/filestat.c:292
#3  0x76fb1199 in io_check_path (
path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
resolve_symlinks=0, is_special_p=0x7fffced8, kind=0x7fffcedc,
pool=0x6bd588) at subversion/libsvn_subr/io.c:222
#4  0x76fb1346 in svn_io_check_special_path (
path=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
kind=0x7fffcedc, is_special=0x7fffced8, pool=0x6bd588)
at subversion/libsvn_subr/io.c:283
#5  0x7794513d in svn_wc__db_pdh_parse_local_abspath (
pdh=0x7fffd038, local_relpath=0x7fffd030, db=0x64ad38,
local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
smode=svn_sqlite__mode_readwrite, result_pool=0x6bd588,
scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db_pdh.c:382
#6  0x77935d58 in svn_wc__db_read_info (status=0x7fffd184,
kind=0x7fffd188, revision=0x0, repos_relpath=0x0,
repos_root_url=0x0,
repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0,
last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0,
target=0x0, changelist=0x0, original_repos_relpath=0x0,
original_root_url=0x0, original_uuid=0x0, original_revision=0x0,
props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0,
db=0x64ad38,
local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
result_pool=0x6bd588, scratch_pool=0x6bd588)
at subversion/libsvn_wc/wc_db.c:5261

However, I then put the breakpont in the svn_wc__db_read_info function and,
just for A/beta.txt, it gets called 7 times, when in terms leads to 7
additional lstat() calls. Looking further the call stack appears as follows:

#0  svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188,
revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0,
changed_rev=0x0, changed_date=0x0, changed_author=0x0,
last_mod_time=0x0,
depth=0x0, checksum=0x0, translated_size=0x0, target=0x0,
changelist=0x0,
original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0,
original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0,
conflicted=0x0, lock=0x0, db=0x64ad38,
local_abspath=0x6bd608 "/home/djcbecroft/dev/workingcopy/A/beta.txt",
result_pool=0x6bd588, scratch_pool=0x6bd588)
at subversion/libsvn_wc/wc_db.c:5259
#1  0x778fab96 in walker_helper (db=0x64ad38,
dir_abspath=0x6ba288 "/home/djcbecroft/dev/workingcopy/A",
show_hidden=1,
walk_callback=0x77ba9026 ,
walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
cancel_func=0x4119c3 , cancel_baton=0x0,
scratch_pool=0x6ba208) at subversion/libsvn_wc/node.c:679
#2  0x778facb5 in walker_helper (db=0x64ad38,
dir_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy", show_hidden=1,
walk_callback=0x77ba9026 ,
walk_baton=0x7fffd4e0, depth=svn_depth_infinity,
cancel_func=0x4119c3 , cancel_baton=0x0,
scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:716
#3  0x778faf9a in svn_wc__internal_walk_children (db=0x64ad38,
local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
show_hidden=1,
walk_callback=0x77ba9026 ,
walk_baton=0x7fffd4e0, walk_depth=svn_depth_infinity,
cancel_func=0x4119c3 , cancel_baton=0x0,
scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:763
#4  0x778fb06d in svn_wc__node_walk_children (wc_ctx=0x64ad20,
local_abspath=0x67d258 "/home/djcbecroft/dev/workingcopy",
show_hidden=1,
walk_callback=0x77ba9026 ,
walk_baton=0x7fffd4e0, walk_depth=svn_depth_infinity,
cancel_func=0x4119c3 , cancel_baton=0x0,
scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:785

Using additional breakpoints, I have verified that
svn_wc__node_

Re: subversion cross compile (arm)

[Philip Martin]

Branko Čibej  writes:

> On 06.12.2010 08:48, Takács András wrote:
>>> Here you are printing 64-bits, so some part of your system thinks that
>>> apr_off_t is 64-bits.  How are apr_off_t and APR_HAS_LARGE_FILES defined
>>> in apr.h?
>> #define APR_HAS_LARGE_FILES   0
>> typedef  off_t   apr_off_t;
>>
>> I think this is OK, isn't it?
>
> That depends on how off_t is defined ... but I expect it should be OK. I
> suggest you find the definition of APR_OFF_T_FMT, could be that that one
> is wrong, expectint 64- instead of 32-bit values.

Ah, yes. If off_t really is 32-bits but APR_OFF_T_FMT is 64-bits then
every write would write an extra 32-bits, thus storing the wrong value,
and every read would modify an extra 32-bits of memory, leading to
corruption.

-- 
Philip

Re: subversion cross compile (arm)

[Philip Martin]

Takács András  writes:

>> Here you are printing 64-bits, so some part of your system thinks that
>> apr_off_t is 64-bits.  How are apr_off_t and APR_HAS_LARGE_FILES defined
>> in apr.h?
>
> #define APR_HAS_LARGE_FILES   0
> typedef  off_t   apr_off_t;
>
> I think this is OK, isn't it?

It shows that APR is just following the rest of the system.  When you
printed values you showed 64 bits so it looks like off_t is 64-bits,
which conflicts with your earlier statement that you were using 32-bit
file offsets.  However the 64-bit values you printed look as if the
lower 32-bits are valid and the higher 32-bits are junk.  Your
environment appears to be confused about the size of file offsets.

-- 
Philip

Re: [PATCH] Issue #3653: svn update should not output svn:external fetches if they have not been updated

[Gavin Beau Baumanis]

Hi Tim,

I am the patch manager - but am not an SVN developer.
So excuse me if this is a "silly" question.

This is the second email from you with the same subject line - and the patch 
file has the same title too.
So is this a second version of the original patch - or an additional one at 
this URI;
http://svn.haxx.se/dev/archive-2010-11/0505.shtml

Subsequently to all others;
These two submissions have received no comments.


Gavin "Beau" Baumanis



On 26/11/2010, at 8:56 PM, Tijn Porcelijn wrote:

> [[[
> Improves interaction, issue #3653: svn update should not output svn:external
> * subversion/svn/notify.c (notify)
>   Add  to Externals messages
>   Note: po files should also be updated
> ]]]
> 
> 
> 
> 
> Hi,
> 
> Here's a small patch for making svn:externals messages a bit more 
> informative. With the "Fetching external item into ''" -message 
> removed, interpretation of svn_wc_notify_update_completed messages becomes a 
> bit less obvious. You'll see stuff like:
> External at revision 20
> External at revision 2321
> External at revision 1082367
> At revision 19
> The patch improves this to read:
> External 'third-party' at revision 20
> External 'snapshots' at revision 2321
> External 'legacy' at revision 1082367
> At revision 19
> See attached notify.c.patch, Thanks,
> 
> tijn
> 
> 

Re: subversion cross compile (arm)

[Branko Čibej]

On 06.12.2010 08:48, Takács András wrote:
>> Here you are printing 64-bits, so some part of your system thinks that
>> apr_off_t is 64-bits.  How are apr_off_t and APR_HAS_LARGE_FILES defined
>> in apr.h?
> #define APR_HAS_LARGE_FILES   0
> typedef  off_t   apr_off_t;
>
> I think this is OK, isn't it?

That depends on how off_t is defined ... but I expect it should be OK. I
suggest you find the definition of APR_OFF_T_FMT, could be that that one
is wrong, expectint 64- instead of 32-bit values.

-- Brane

Re: svn commit: r1040832 - Port a fix for a FSFS packing race

[Daniel Shahaf]

Stefan Fuhrmann wrote on Sun, Dec 05, 2010 at 23:45:50 +0100:
> On 01.12.2010 14:16, Daniel Shahaf wrote:
>> Julian Foad wrote on Wed, Dec 01, 2010 at 12:32:45 +:
>>> On Wed, 2010-12-01, stef...@apache.org wrote:
 Port (not merge) a fix for a FSFS packing race condition from the
 performance branch to /trunk: There is a slight time window
 between finding the name of a rev file and actually open it. If
 the revision in question gets packed just within this window,
 we will find the new (and final) file name with just one retry.

 * subversion/libsvn_fs_fs/fs_fs.c
(open_pack_or_rev_file): retry once upon "missing file" error
>>> Hi Stefan.
>>>
>>> (1) I think the doc string of svn_fs_fs__path_rev_absolute() should note
>>> that the returned path may no longer be valid by the time you come to
>>> use it, and the reason for that.  Does my patch below sound right?
>>>
>> Well, yes, and Stefan already added such a comment at the definition at
>> my suggestion.  But +1 to move that to the declaration.
>>
>
>>> Patch for (1) above:
>>> [[[
>>> Index: subversion/libsvn_fs_fs/fs_fs.h
>>> ===
>>> --- subversion/libsvn_fs_fs/fs_fs.h (revision 1040662)
>>> +++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
>>> @@ -404,13 +404,16 @@ svn_error_t *svn_fs_fs__txn_changes_fetc
>>>  PERMS_REFERENCE.  Temporary allocations are from POOL. */
>>>   svn_error_t *svn_fs_fs__move_into_place(const char *old_filename,
>>>   const char *new_filename,
>>>   const char *perms_reference,
>>>   apr_pool_t *pool);
>>>
>>> -/* Sets *PATH to the path of REV in FS, whether in a pack file or not.
>>> +/* Set *PATH to the path of REV in FS, whether in a pack file or not.
>>> +   The path is not guaranteed to remain correct after the function returns,
>>> +   because it is possible that the revision will become packed just after
>>> +   this call, so the caller should re-try once if the path is not found.
>>>  Allocate in POOL. */
>> Sounds right.
>>
>> Bordering on bikeshed, I would suggest some changes:
>>
>> * Separate describing what the function does ("Sets *PATH to FOO and
>>allocates in POOL") from describing the conditions the caller should
>>beware of / check for ("sometimes the return value will be out-of-date
>>by the time you receive it").
>>
>> * Mention that the non-guarantee is not in effect if the caller has the
>>write lock.
>>
> I took the comment from the performance branch and added the
> info about guarantees in presence of a write lock in r1042479.
>

Thanks.

However, Julian already fixed this in fs_fs.h.  Normally we keep the doc
strings only at the declaration of a function, and have no comments (or
only comments aimed at people patching the function itself --- but not
at people who call the function) at the definition.

> -- Stefan^2.

Re: 1.7.x - merge now accesses all files in WC?

[Daniel Shahaf]

Instead of guessing which function causes the lstat() calls, could we
have a tool tell?

I've looked at 'ltrace -S', but it seems to me that will only tell us
which public svn_wc_* API is responsible for the calls.  Perhaps there
is another tool that can help us follow the entire call chain down to
the lstat() call?

Daniel
. o O (gdb with a breakpoint in lstat() and some fu to extract
statistics about the stack trace?)


Daniel Becroft wrote on Mon, Dec 06, 2010 at 18:32:09 +1000:
> On Mon, Dec 6, 2010 at 8:09 AM, Stefan Sperling  wrote:
> 
> > On Mon, Dec 06, 2010 at 07:18:56AM +1000, Daniel Becroft wrote:
> > > On Fri, Dec 3, 2010 at 9:22 AM, Peter Samuelson  wrote:
> > >
> > > >
> > > > [Daniel Becroft]
> > > > > I've just managed to build/install trunk on my ubuntu box at home
> > (first
> > > > > application I've ever compiled on it - yey!).
> > > > >
> > > > > What debugging tools would you recommend to investigate this further?
> > > > I've
> > > > > seen output posted that lists function names, and time spent on each.
> > > >
> > > > The obvious start is 'strace', as in 'strace svn merge ...'.  It spits
> > > > out every system call.  There's a lot of noise up front as it's loading
> > > > shared libraries and such, but it should still be obvious what we're
> > > > doing when crawling the tree (stat / lstat, open, etc.).<
> > http://p12n.org/>
> > >
> > >
> > > Thanks. I've run that, and all the calls are lstat(). There are 16 for
> > each
> > > file (or at least, those not affected by the merge). I've copied on of
> > the
> > > calls below (all 16 of these are identical).
> > >
> > > lstat("../dev/workingcopy/A/beta.txt", {st_dev=makedev(8, 1),
> > > st_ino=23855167, st_mode=S_IFREG|0644, st_nlink=1, st_uid=1000,
> > st_gid=1000,
> > > st_blksize=4096, st_blocks=8, st_size=5, st_atime=2010/12/06-07:10:42,
> > > st_mtime=2010/12/06-07:10:42, st_ctime=2010/12/06-07:10:42}) = 0
> > >
> > > It does appear that it's a known issue though. I'll keep investigating to
> > > try and understand why it's getting called at all. Thanks for your help.
> >
> > That's probably the svn_wc_revision_status2() call in
> > ensure_wc_is_suitable_merge_target() (subversion/libsvn_client/merge.c).
> >
> > 1.6.x should be doing the same during reintegrate merges.
> > 1.7 does it for every merge.
> >
> > To verify my theory you can try the patch below.
> > We could try to optimize this if it is a problem.
> >
> > Stefan
> >
> > Index: subversion/libsvn_client/merge.c
> > ===
> > --- subversion/libsvn_client/merge.c(revision 1042308)
> > +++ subversion/libsvn_client/merge.c(working copy)
> > @@ -9022,6 +9022,7 @@ ensure_wc_is_suitable_merge_target(const char *tar
> >svn_boolean_t allow_switched_subtrees,
> >apr_pool_t *scratch_pool)
> >  {
> > +#if 0
> >   svn_wc_revision_status_t *wc_stat;
> >
> >   /* Avoid the following status crawl if we don't need it. */
> > @@ -9068,6 +9069,7 @@ ensure_wc_is_suitable_merge_target(const char *tar
> >wc_stat->min_rev, wc_stat->max_rev);
> > }
> >
> > +#endif
> >   return SVN_NO_ERROR;
> >  }
> >
> 
> Hey Stefan,
> 
> I've applied the patch, and re-run the test. It's cut the calls down from 16
> to 10.
> 
> Cheers,
> Daniel B.

Re: 1.7.x - merge now accesses all files in WC?

[Daniel Becroft]

On Mon, Dec 6, 2010 at 8:09 AM, Stefan Sperling  wrote:

> On Mon, Dec 06, 2010 at 07:18:56AM +1000, Daniel Becroft wrote:
> > On Fri, Dec 3, 2010 at 9:22 AM, Peter Samuelson  wrote:
> >
> > >
> > > [Daniel Becroft]
> > > > I've just managed to build/install trunk on my ubuntu box at home
> (first
> > > > application I've ever compiled on it - yey!).
> > > >
> > > > What debugging tools would you recommend to investigate this further?
> > > I've
> > > > seen output posted that lists function names, and time spent on each.
> > >
> > > The obvious start is 'strace', as in 'strace svn merge ...'.  It spits
> > > out every system call.  There's a lot of noise up front as it's loading
> > > shared libraries and such, but it should still be obvious what we're
> > > doing when crawling the tree (stat / lstat, open, etc.).<
> http://p12n.org/>
> >
> >
> > Thanks. I've run that, and all the calls are lstat(). There are 16 for
> each
> > file (or at least, those not affected by the merge). I've copied on of
> the
> > calls below (all 16 of these are identical).
> >
> > lstat("../dev/workingcopy/A/beta.txt", {st_dev=makedev(8, 1),
> > st_ino=23855167, st_mode=S_IFREG|0644, st_nlink=1, st_uid=1000,
> st_gid=1000,
> > st_blksize=4096, st_blocks=8, st_size=5, st_atime=2010/12/06-07:10:42,
> > st_mtime=2010/12/06-07:10:42, st_ctime=2010/12/06-07:10:42}) = 0
> >
> > It does appear that it's a known issue though. I'll keep investigating to
> > try and understand why it's getting called at all. Thanks for your help.
>
> That's probably the svn_wc_revision_status2() call in
> ensure_wc_is_suitable_merge_target() (subversion/libsvn_client/merge.c).
>
> 1.6.x should be doing the same during reintegrate merges.
> 1.7 does it for every merge.
>
> To verify my theory you can try the patch below.
> We could try to optimize this if it is a problem.
>
> Stefan
>
> Index: subversion/libsvn_client/merge.c
> ===
> --- subversion/libsvn_client/merge.c(revision 1042308)
> +++ subversion/libsvn_client/merge.c(working copy)
> @@ -9022,6 +9022,7 @@ ensure_wc_is_suitable_merge_target(const char *tar
>svn_boolean_t allow_switched_subtrees,
>apr_pool_t *scratch_pool)
>  {
> +#if 0
>   svn_wc_revision_status_t *wc_stat;
>
>   /* Avoid the following status crawl if we don't need it. */
> @@ -9068,6 +9069,7 @@ ensure_wc_is_suitable_merge_target(const char *tar
>wc_stat->min_rev, wc_stat->max_rev);
> }
>
> +#endif
>   return SVN_NO_ERROR;
>  }
>

Hey Stefan,

I've applied the patch, and re-run the test. It's cut the calls down from 16
to 10.

Cheers,
Daniel B.