[PATCH] New XFail test case for issue 3013

[Noorul Islam K M]

Attached is the python test for issue 3013. This incorporates the steps
from the shell script attached in the tracker.

Log 
[[[

New XFail test for issue 3013.

* subversion/tests/cmdline/update_tests.py
  (update_after_switching_to_deleted_path, test_list): New XFail test

Patch by: Noorul Islam K M 
]]]

Thanks and Regards
Noorul

Index: subversion/tests/cmdline/update_tests.py
===
--- subversion/tests/cmdline/update_tests.py(revision 1063610)
+++ subversion/tests/cmdline/update_tests.py(working copy)
@@ -5347,6 +5347,34 @@
   svntest.main.run_svn(None, 'delete', os.path.join('A2', 'mu'))
   svntest.main.run_svn(None, 'update', os.path.join('A2', 'mu'))
 
+### regression test for issue #3013
+def update_after_switching_to_deleted_path(sbox):
+  "update after switching to deleted path"
+  
+  sbox.build()
+  wc_dir = sbox.wc_dir
+  repo_url = sbox.repo_url
+
+  # switch to A/B
+  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'switch',
+  repo_url + "/A/B", wc_dir)
+
+  # delete A/D
+  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'rm',
+  repo_url + "/A/D", '-m', 
+  'Remove A/D')
+
+  # switch to A/D and this is known to fail
+  svntest.actions.run_and_verify_svn2(None, None, svntest.verify.AnyOutput,
+  1, 'switch', repo_url + "/A/D", wc_dir)
+
+  # switch to A/D@1 and this is known to succeed
+  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'switch',
+  repo_url + "/A/D@1", wc_dir)
+
+  # update should succeed
+  svntest.actions.run_and_verify_svn2(None, None, [], 0, "up", wc_dir)
+
 ###
 # Run the tests
 
@@ -5412,6 +5440,7 @@
   update_with_excluded_subdir,
   XFail(update_with_file_lock_and_keywords_property_set),
   XFail(update_nonexistent_child_of_copy),
+  XFail(update_after_switching_to_deleted_path),
  ]
 
 if __name__ == '__main__':

EDEADLK in svn_repos_fs_begin_txn_for_commit2

[Blair Zajac]

We're seeing deadlocks in our Subversion multithreaded server when two distinct 
processes try to fcntl(F_SETLKW) on two fsfs repositories' db/txn-current-lock, 
when the processes begin transactions in reverse order.


Process 1   Process 2
-   -
thread 1: begin txn in repos A  thread 1: being txn in repos B
thread 2: begin txn in repos B  thread 2: begin txn in repos A

During normal working hours, we get over 1 commit per second, peaking at 6, 
which is why we're seeing this.


Questions:

Should a fix for this be put in libsvn_fs_fs() or should I do this in my 
application?  I'm thinking putting this in libsvn_fs_fs() is an appropriate fix, 
even though other people probably won't see it.


I'm also thinking the code should retry a maximum of 100 times with a 1ms sleep, 
doubling each sleep upon failure to a maximum 128 ms, such as WIN32_RETRY_LOOP.


Comments?

Blair

Re: [PATCH] Re: Incorporate svn 1.7 changes to vc-svn.el

[Daniel Shahaf]

This looks correct, perhaps someone with $EDITOR != vim can apply it?

Noorul Islam K M wrote on Wed, Jan 26, 2011 at 10:35:02 +0530:
> Noorul Islam K M  writes:
> 
> > After migrating working copy to 1.7, vc-svn.el is not functioning
> > properly. This is because .svn/entries file does not exist. I think we
> > no longer need to check existence of this file. Since .svn exists at the
> > root of the working copy we no longer be able to assume that .svn will
> > be available in all folders of working copy.
> >
> > Log 
> > [[[
> >
> > Incorporate svn 1.7 changes.
> >
> > * contrib/client-side/emacs/vc-svn.el
> >   (vc-svn-registered): Since 1.7 '.svn/entries' file does not exist. No
> > need to check existence of this file. Running 'svn status' command
> > alone is good enough.
> >
> > Patch by: Noorul Islam K M 
> > ]]]
> >
> > Thanks and Regards
> > Noorul
> >
> > Index: contrib/client-side/emacs/vc-svn.el
> > ===
> > --- contrib/client-side/emacs/vc-svn.el (revision 1060693)
> > +++ contrib/client-side/emacs/vc-svn.el (working copy)
> > @@ -115,10 +115,7 @@
> >  
> >  (defun vc-svn-registered (file)
> >"Return true if FILE is registered under Subversion."
> > -  ;; First, a quick false positive test: is there a `.svn/entries' file?
> > -  (and (file-exists-p (expand-file-name ".svn/entries"
> > -(file-name-directory file)))
> > -   (not (null (vc-svn-run-status file)
> > +  (not (null (vc-svn-run-status file
> >  
> >  
> >  (put 'vc-svn-with-output-buffer 'lisp-indent-function 0)
> 
> I forgot to add [PATCH] tag in the subject line. Adding it now.
> 
> Thanks and Regards
> Noorul

Re: Status of the branch diff-optimizations-bytes branch

[Daniel Shahaf]

Johan Corveleyn wrote on Wed, Jan 26, 2011 at 03:31:11 +0100:
> Revving svn_diff_fns_t: what do you mean with parallelizing it? I must
> admit that I don't really know (yet) how to go about that. Very early
> during the branch work, danielsh pointed out that I modified this
> public struct (vtable for reading data from datasources), so it should
> be revved. Is it listed/documented somewhere what should be done for
> that (community guide perhaps)?

Briefly, revving a function includes re-declaring it, updating the old
declaration to be a diff against the new one, marking it as deprecated
(using doxygen and C preprocessor designators), and re-implementing the
old function as a deprecated.c wrapper around the new one.

For a struct, you need to re-declare the struct and revv functions that
use it.  Also, don't forget to add a constructor function
(svn_foo_t_create(), but s/_t_/_/) (and possibly a duplicator function),
and forbid people from defining variables of the struct type directly.

I'm not sure HACKING contains this.  On the other hand, the public
header files contain plenty of examples of everything I just said...

Re: Status of the branch diff-optimizations-bytes branch

[Daniel Shahaf]

Johan Corveleyn wrote on Wed, Jan 26, 2011 at 03:31:11 +0100:
> On Tue, Jan 25, 2011 at 4:58 PM, Hyrum K Wright  wrote:
> > Johan (and other interested parties),
> > I've been following some of the commits to the
> > diff-optimizations-branch with interest.  While I've not reviewed them
> > for technical merit, it appears that others have, and that there is
> > good work going on in the branch.
> 
> Thanks for your interest.
> 
> It might be good to get a little bit more review on the whole, I
> think. A lot of people have read parts of the code, but if I remember
> correctly most (if not all) of them have said things like "I haven't
> reviewed it in detail, but here are some suggestions, feedback, ...".
> 

Indeed.  I, for example, reviewed parts of the code; but for other parts
I've simply let Johan tell me what he had done, and haven't reviewed
them myself.

Re: [PATCH] svn command - cat - Multiple targets

[Noorul Islam K M]

Noorul Islam K M  writes:

> This patch is a followup of the following thread. All tests pass with
> this patch.
>
> http://svn.haxx.se/dev/archive-2011-01/0210.shtml
>
> Log
>
> [[[
>
> Make svn 'cat' command to return 1 when one or more targets fails. Also
> print error message at the end. This also fixes issue #3713.
>
> * subversion/svn/cat-cmd.c
>   (svn_cl__cat): Pass SVN_ERR_FS_NOT_FOUND to svn_cl__try in order to
> catch this error when processing non existent URL targets, print
> warning and proceed with other targets. Print error message at the
> end if operation fails on any one of the targets and return 1.
>
> * subversion/tests/cmdline/cat_tests.py
>   (cat_local_directory, cat_nonexistent_file, cat_skip_uncattable,
>cat_unversioned_file, cat_url_special_characters):
>  Update return code and use regular expressions to match output.
>   (cat_non_existing_remote_file): New test
>
> Patch by: Noorul Islam K M 
>
> ]]]
>
> Thanks and Regards
> Noorul
>
> Index: subversion/tests/cmdline/cat_tests.py
> ===
> --- subversion/tests/cmdline/cat_tests.py (revision 1060693)
> +++ subversion/tests/cmdline/cat_tests.py (working copy)
> @@ -25,7 +25,7 @@
>  ##
>  
>  # General modules
> -import os
> +import os, re
>  
>  # Our testing module
>  import svntest
> @@ -50,20 +50,23 @@
>sbox.build(read_only = True)
>  
>A_path = os.path.join(sbox.wc_dir, 'A')
> +  expected_err = "svn: warning: '" + os.path.abspath(A_path) + "'" + \
> + " refers to a directory\n.*"
>  
> -  svntest.actions.run_and_verify_svn2('No error where one is expected',
> -  None, svntest.verify.AnyOutput,
> -  0, 'cat', A_path)
> +  svntest.actions.run_and_verify_svn2(None, None, expected_err,
> +  1, 'cat', A_path)
>  
>  def cat_remote_directory(sbox):
>"cat a remote directory"
>sbox.build(create_wc = False, read_only = True)
>  
>A_url = sbox.repo_url + '/A'
> -  svntest.actions.run_and_verify_svn2('No error where one is expected',
> -  None, svntest.verify.AnyOutput,
> -  0, 'cat', A_url)
> +  expected_err = "svn: warning: URL '" + A_url + "'" + \
> + " refers to a directory\n.*"
>  
> +  svntest.actions.run_and_verify_svn2(None, None, expected_err,
> +  1, 'cat', A_url)
> +
>  def cat_base(sbox):
>"cat a file at revision BASE"
>sbox.build(read_only = True)
> @@ -88,10 +91,13 @@
>wc_dir = sbox.wc_dir
>  
>bogus_path = os.path.join(wc_dir, 'A', 'bogus')
> -  svntest.actions.run_and_verify_svn2('No error where one is expected',
> -  None, svntest.verify.AnyOutput,
> -  0, 'cat', bogus_path)
>  
> +  expected_err = "svn: warning: '" + os.path.abspath(bogus_path) + "'" + \
> + " is not under version control\n.*"
> +
> +  svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
> +  'cat', bogus_path)
> +
>  def cat_skip_uncattable(sbox):
>"cat should skip uncattable resources"
>sbox.build(read_only = True)
> @@ -113,14 +119,14 @@
>continue
>  item_to_cat = os.path.join(dir_path, file)
>  if item_to_cat == new_file_path:
> -  expected_err = ["svn: warning: '" + os.path.abspath(item_to_cat) + "'" 
> + \
> - " is not under version control\n"]
> -  svntest.actions.run_and_verify_svn2(None, None, expected_err, 0,
> +  expected_err = "svn: warning: '" + os.path.abspath(item_to_cat) + "'" 
> + \
> + " is not under version control\n.*"
> +  svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
>'cat', item_to_cat)
>  elif os.path.isdir(item_to_cat):
> -  expected_err = ["svn: warning: '" + os.path.abspath(item_to_cat) + "'" 
> + \
> - " refers to a directory\n"]
> -  svntest.actions.run_and_verify_svn2(None, None, expected_err, 0,
> +  expected_err = "svn: warning: '" + os.path.abspath(item_to_cat) + "'" 
> + \
> + " refers to a directory\n.*"
> +  svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
>'cat', item_to_cat)
>  else:
>svntest.actions.run_and_verify_svn(None,
> @@ -130,22 +136,33 @@
>G_path = os.path.join(dir_path, 'G')
>rho_path = os.path.join(G_path, 'rho')
>  
> -  expected_out = ["This is the file 'rho'.\n"]
> -  expected_err1 = ["svn: warning: '" + os.path.abspath(G_path) + "'"
> -   + " refers to a directory\n"]
> -  svntest.actions.run_and_verify_svn2(None, expected_out, expected_er

[PATCH] Re: Incorporate svn 1.7 changes to vc-svn.el

[Noorul Islam K M]

Noorul Islam K M  writes:

> After migrating working copy to 1.7, vc-svn.el is not functioning
> properly. This is because .svn/entries file does not exist. I think we
> no longer need to check existence of this file. Since .svn exists at the
> root of the working copy we no longer be able to assume that .svn will
> be available in all folders of working copy.
>
> Log 
> [[[
>
> Incorporate svn 1.7 changes.
>
> * contrib/client-side/emacs/vc-svn.el
>   (vc-svn-registered): Since 1.7 '.svn/entries' file does not exist. No
> need to check existence of this file. Running 'svn status' command
> alone is good enough.
>
> Patch by: Noorul Islam K M 
> ]]]
>
> Thanks and Regards
> Noorul
>
> Index: contrib/client-side/emacs/vc-svn.el
> ===
> --- contrib/client-side/emacs/vc-svn.el   (revision 1060693)
> +++ contrib/client-side/emacs/vc-svn.el   (working copy)
> @@ -115,10 +115,7 @@
>  
>  (defun vc-svn-registered (file)
>"Return true if FILE is registered under Subversion."
> -  ;; First, a quick false positive test: is there a `.svn/entries' file?
> -  (and (file-exists-p (expand-file-name ".svn/entries"
> -(file-name-directory file)))
> -   (not (null (vc-svn-run-status file)
> +  (not (null (vc-svn-run-status file
>  
>  
>  (put 'vc-svn-with-output-buffer 'lisp-indent-function 0)

I forgot to add [PATCH] tag in the subject line. Adding it now.

Thanks and Regards
Noorul

'svnrdump load' tests unmirrored

[Daniel Shahaf]

In svnrdump_tests.py, nearly all tests are paired --- dump test and
corresponding load test --- except a handful (basic_dump,
only_trunk_dump, only_trunk_A_with_changes_dump,
copy_bad_line_endings_dump).

Is there any particular reason that these don't have corresponding _load
tests?

Thanks,

Daniel

Re: Status of the branch diff-optimizations-bytes branch

[Johan Corveleyn]

On Tue, Jan 25, 2011 at 4:58 PM, Hyrum K Wright  wrote:
> Johan (and other interested parties),
> I've been following some of the commits to the
> diff-optimizations-branch with interest.  While I've not reviewed them
> for technical merit, it appears that others have, and that there is
> good work going on in the branch.

Thanks for your interest.

It might be good to get a little bit more review on the whole, I
think. A lot of people have read parts of the code, but if I remember
correctly most (if not all) of them have said things like "I haven't
reviewed it in detail, but here are some suggestions, feedback, ...".

> I'm wondering what the potential
> for merging back to trunk is.  This is the TODO section from the
> BRANCH-README:
>
> TODO:
>
>  * eliminate identical prefix                 [DONE]
>  * eliminate identical suffix                 [DONE]
>  * make diff3 and diff4 use datasources_open  [DONE]
>     (this may eliminate the need for datasource_open, and the flag
>      datasource_opened in token.c#svn_diff__get_tokens)
>  * implement (part of) increment_pointers and
>    decrement_pointers with a macro            [DONE]
>     (offers another 10% speedup)
>  * integrate (some of the) low-level optimizations for prefix/suffix
>    scanning, proposed by stefan2 [2]          []
>  * revv svn_diff.h#svn_diff_fns_t             []
>
> It looks like, for the most part, any destabilizing functionality is
> completed, and what remains are simply optimizations.  This
> optimization work can probably be performed on trunk, and if so, we
> should merge the branch back and do the cleanup work there.  The only
> bit on the current list of stuff is revving svn_diff_fns_t.  Can that
> work be parallelized?

Yes, you are correct. Most of the essential work is done. Further
optimizations can just as well be done on trunk.

Revving svn_diff_fns_t: what do you mean with parallelizing it? I must
admit that I don't really know (yet) how to go about that. Very early
during the branch work, danielsh pointed out that I modified this
public struct (vtable for reading data from datasources), so it should
be revved. Is it listed/documented somewhere what should be done for
that (community guide perhaps)?

(one slightly related note to this: I introduced the function
datasources_open, to open the multiple datasources at once (as opposed
to the original datasource_open, which only opens one datasource). But
maybe the new function should be renamed to datasource_open_all or
datasources_open_all or something, to make it stand out a little bit
more).

> I'm not trying to rush the work, nor do I think it is essential for
> 1.7, but it feels like a pretty good performance increase, and one
> that we shouldn't have any problem shipping.  (If there are currently
> API uncertainties, than it would be good to wait until 1.7.x branches,
> though.)

Yes, I think it's quite ready to be merged, and could provide a very
significant performance increase (for diff, merge and blame).

The API is stable now, I think, except maybe for the name of the
datasources_open function (see above). If we decide to go (for
optimizations reasons) for specialized prefix/suffix scanning
functions for 2, 3 or 4 datasources, I think it's best to keep the
generic datasources_open API (with an array of datasources), and only
split up between specialized versions in the implementation.

> Anyway, I just really like the concept / implementation of the branch,
> and I'm excited to get it into the hands of users.  Thoughts?
>
> -Hyrum
>

Cheers,
-- 
Johan

Re: My take on the diff-optimizations-bytes branch

[Johan Corveleyn]

On Tue, Jan 25, 2011 at 1:37 AM, Stefan Fuhrmann  wrote:
[ ... snip ...]
> And, as promised, here some ideas how to get more
> speed from the generic code. Your latest commit:
>
> +#if SVN_UNALIGNED_ACCESS_IS_OK
> +
> +      /* Skip quickly over the stuff between EOLs. */
> +      for (i = 0, can_read_word = TRUE; i<  file_len; i++)
> +        can_read_word = can_read_word
> +&&  (file[i].curp + sizeof(apr_size_t)<  file[i].endp);
> +      while (can_read_word)
> +        {
> +          for (i = 1, is_match = TRUE; i<  file_len; i++)
> +            is_match = is_match
> +&&  (   *(const apr_size_t *)file[0].curp
> +                           == *(const apr_size_t *)file[i].curp);
> +
> +          if (!is_match || contains_eol(*(const apr_size_t *)file[0].curp))
> +            break;
> +
> +          for (i = 0; i<  file_len; i++)
> +            file[i].curp += sizeof(apr_size_t);
> +          for (i = 0, can_read_word = TRUE; i<  file_len; i++)
> +            can_read_word = can_read_word
> +&&  (file[i].curp + sizeof(apr_size_t)<  file[i].endp);
> +        }
> +
> +#endif
>
> could be changed to something like the following.
> Please note that I haven't tested any of this:

Thanks. There was one error in your suggestion, which I found out
after testing. See below.

> /* Determine how far we may advance with chunky ops without reaching
>  * endp for any of the files.
>  * Signedness is important here if curp gets close to endp.
>  */
> apr_ssize_t max_delta = file[0].endp - file[0].curp - sizeof(apr_size_t);
> for (i = 1; i<  file_len; i++)
> {
>    apr_ssize_t delta = file[i].endp - file[i].curp - sizeof(apr_size_t);
>    if (delta<  max_delta)
>        max_delta = delta;
> }
>
> /* the former while() loop */
> is_match = TRUE;
> for (delta = 0; delta<  max_delta&&  is_match; delta += sizeof(apr_size_t))
> {
>    apr_size_t chunk = *(const apr_size_t *)(file[0].curp + delta);
>    if (contains_eol(chunk))
>        break;
>
>    for (i = 1; i<  file_len; i++)
>        if (chunk != *(const apr_size_t *)(file[i].curp + delta))
>        {
>            is_match = FALSE;

Here, I inserted:

delta -= sizeof(apr_size_t);

because otherwise, delta will be increased too far (it will still be
increased by the counting expression of the outer for-loop (after
which it will stop because of !is_match)). Maybe there is a
cleaner/clearer way to break out of the outer for-loop here, without
incrementing delta again, but for now, I've committed it with this
change (r1063565).

>            break;
>        }
> }
>
> /* We either found a mismatch or an EOL at or shortly behind curp+delta
>  * or we cannot proceed with chunky ops without exceeding endp.
>  * In any way, everything up to curp + delta is equal and not an EOL.
>  */
> for (i = 0; i<  file_len; i++)
>    file[i].curp += delta;

Thanks. This gives on my machine/example another 15-20% performance
increase (datasources_open time going down from ~21 s to ~17 s). We
should probably do the same for suffix scanning, but I'm too tired
right now :-) (and suffix scanning is more difficult to grok, so not a
good idea to do at 3 am).

Cheers,
-- 
Johan

Re: Document return codes of 'svn ls'

[anatoly techtonik]

On Tue, Jan 25, 2011 at 11:30 PM, C. Michael Pilato  wrote:
> On 01/25/2011 03:53 PM, anatoly techtonik wrote:
>> On Tue, Jan 25, 2011 at 9:35 PM, Daniel Shahaf  
>> wrote:
>>> I was suggesting a syntax we might want to implement in the future.
>>
>> So, you basically agree that there is no way to see if a path is
>> absent at specified revision in repository.
>> Should I create an issue?
>
> There comes a point where you have to realize that the command-line client
> isn't designed to provide an answer for every question you might have.  This
> is why we publish a public API in C, Python, Perl, Ruby, Java,   If you
> need to distinguish different error situations at a finer resolution than
> works/doesn't-work, perhaps it's time you wrote your own tooling.

This stuff is needed for correct uploading of changesets for rietveld
code reviews. Code review site (for example,
http://codereview.appspot.com) provides upload.py script, which is
downloaded by users and executed from command line. Proposing to
download SVN binding together with this script is not a solution. They
may not be compatible with SVN version installed on user system. More
than that - ctypes bindings were announced, but unavailable, SWIG are
lacking memory and have some other issues, so that hgsubversion uses
subvertpy. Speaking about Mercurial, the command line API is the only
guaranteed and stable API they guarantee to work.

The issues with return codes from command line utility previously
raised in Bitten project also. I can't understand why you still do not
think that this is a valid case for a ticket, i.e. that command line
client doesn't allow to see if a path is absent at specified revision
in repository?  To me the command call is no different from any other
call (esp. for command line tools), and you can only win from
consistent scheme for documented return codes.

-- 
anatoly t.

Re: log -g regression in r1028108

[Kevin Radke]

On Tue, Jan 25, 2011 at 1:23 PM, Hyrum K Wright  wrote:
> On Tue, Jan 25, 2011 at 1:14 PM, Stefan Sperling  wrote:
>> On Tue, Jan 25, 2011 at 10:34:43AM -0600, Kevin Radke wrote:
>>> (I'm moving this conversation from users to dev, since I have
>>> convinced myself a regression was introduced in r1028108)
>>> See: http://svn.haxx.se/users/archive-2010-12/0265.shtml
>>>
>>> log -v -g --xml http://server/repo/path commands will now fail for
>>> "complex" histories that contain file renames.
>>>
>>> The client sees:
>>> svn: REPORT of '/repo/!svn/bc/1234/path/in/repo': Could not read chunk
>>> size: connection was closed by server (http://server)
>>>
>>> The server logs:
>>> [Wed Dec 15 15:48:18 2010] [error] [client 192.168.1.1] File not
>>> found: revision 5678, path '/path/in/repo/file.txt'  [404, #160013]
>>>
>>> (The file was named oldfilename.txt in r5678, because it was renamed
>>> in r7890.  Something isn't using the correct name when more than
>>> MAX_OPEN_HISTORIES have been found.)
>>>
>>> The only source file modified in this commit was 
>>> subversion/libsvn_reops/log.c
>>>
>>> A few questions:
>>>
>>> 1) Does setting info->oldpool and info->newpool to NULL around lines
>>> 1113 potentially leak memory?
>>> 2) What is info->first_time used for?  It seems to always be set to
>>> true in the loop in get_path_histories() and then reset in
>>> get_history()
>>> 3) Increasing MAX_OPEN_HISTORIES to 128 "fixes" my test repository,
>>> but isn't the true fix.
>>
>> Thanks for bringing this to dev@.
>> If it's not too much of a bother, could you also file an issue for this
>> and set the target milestone to 1.7.0 so we don't forget about it?
>> This milestone doesn't mean that a fix for 1.6.x won't be made.
>> It just exploits the fact that currently most people are looking at
>> issues scheduled for 1.7.0 :)
>
> I milestone 1.7.0 issue also means that it's a blocker for the 1.7.x
> branch, iiuc.

Created issue 3789 and set target milestone 1.7.0.
http://subversion.tigris.org/issues/show_bug.cgi?id=3789

Kevin R.

Re: Document return codes of 'svn ls'

[C. Michael Pilato]

On 01/25/2011 03:53 PM, anatoly techtonik wrote:
> On Tue, Jan 25, 2011 at 9:35 PM, Daniel Shahaf  
> wrote:
>> I was suggesting a syntax we might want to implement in the future.
> 
> So, you basically agree that there is no way to see if a path is
> absent at specified revision in repository.
> Should I create an issue?

There comes a point where you have to realize that the command-line client
isn't designed to provide an answer for every question you might have.  This
is why we publish a public API in C, Python, Perl, Ruby, Java,   If you
need to distinguish different error situations at a finer resolution than
works/doesn't-work, perhaps it's time you wrote your own tooling.

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



signature.asc
Description: OpenPGP digital signature

Re: Document return codes of 'svn ls'

[Daniel Shahaf]

anatoly techtonik wrote on Tue, Jan 25, 2011 at 22:53:05 +0200:
> So, you basically agree that there is no way to see if a path is
> absent at specified revision in repository.

I did not express an opinion on the specific issue you raised in this thread.

Re: Document return codes of 'svn ls'

[anatoly techtonik]

On Tue, Jan 25, 2011 at 9:35 PM, Daniel Shahaf  wrote:
> I was suggesting a syntax we might want to implement in the future.

So, you basically agree that there is no way to see if a path is
absent at specified revision in repository.
Should I create an issue?

--
anatoly t.

Re: svn commit: r1058503, 1058515

[Daniel Shahaf]

Got it done before the release: r1063420.

Stefan Sperling wrote on Thu, Jan 13, 2011 at 15:39:05 +:
> On Thu, Jan 13, 2011 at 05:27:52PM +0200, Daniel Shahaf wrote:
> > s...@apache.org wrote on Thu, Jan 13, 2011 at 11:07:27 -:
> > > -With a 1.7 server, it is possible to fix the svnsync race condition
> > > -even for pre-1.7 svnsync clients by installing the pre-revprop-change 
> > > hook
> > > -script given in
> > > -http://subversion.tigris.org/issues/show_bug.cgi?id=3546#desc12";
> > > - >comment 12 of issue #3546.
> > >  
> > 
> > This should be reverted, per Philip's recent comment.
> 
> Yes, I've seen it.  Feel free to revert :)
> 
> > >  svnrdump
> > >  1.7
> > > -any
> > > +any (dump)1.7 (load)
> > >  any
> > >  svnrdump load should only be used with 1.7 servers
> > >  to avoid a race condition (see  > > href="#svnrdump">below).
> > 
> > Why not state just "1.7" in the table, since Philip's hook should
> > prevent the race condition here too?
> 
> Fine with me.

Re: Document return codes of 'svn ls'

[Daniel Shahaf]

I was suggesting a syntax we might want to implement in the future.

Re: Document return codes of 'svn ls'

[anatoly techtonik]

On Tue, Jan 25, 2011 at 8:47 PM, Daniel Shahaf  wrote:
> anatoly techtonik wrote on Fri, Jan 21, 2011 at 13:42:51 +0200:
>> No. Internet connection may be down during this specific request. How can I
>> distinguish these two errors?
>
> How about something like this:
>
> % svn $subcommand --options --output-apr_err-to-stderr 2>&1 >/dev/null | tail 
> -n1
> apr_err=210005

Doesn't work. Is it because I am on Windows?
> svn info path@X --options --output-apr_err-to-stderr
svn.EXE: invalid option: --options

> svn info path@X --config-option --output-apr_err-to-stderr
svn: Invalid syntax of argument of --config-option


I couldn't find any 'arp_err' references in svn binary code at all.
-- 
anatoly t.

Re: log -g regression in r1028108

[Hyrum K Wright]

On Tue, Jan 25, 2011 at 1:14 PM, Stefan Sperling  wrote:
> On Tue, Jan 25, 2011 at 10:34:43AM -0600, Kevin Radke wrote:
>> (I'm moving this conversation from users to dev, since I have
>> convinced myself a regression was introduced in r1028108)
>> See: http://svn.haxx.se/users/archive-2010-12/0265.shtml
>>
>> log -v -g --xml http://server/repo/path commands will now fail for
>> "complex" histories that contain file renames.
>>
>> The client sees:
>> svn: REPORT of '/repo/!svn/bc/1234/path/in/repo': Could not read chunk
>> size: connection was closed by server (http://server)
>>
>> The server logs:
>> [Wed Dec 15 15:48:18 2010] [error] [client 192.168.1.1] File not
>> found: revision 5678, path '/path/in/repo/file.txt'  [404, #160013]
>>
>> (The file was named oldfilename.txt in r5678, because it was renamed
>> in r7890.  Something isn't using the correct name when more than
>> MAX_OPEN_HISTORIES have been found.)
>>
>> The only source file modified in this commit was 
>> subversion/libsvn_reops/log.c
>>
>> A few questions:
>>
>> 1) Does setting info->oldpool and info->newpool to NULL around lines
>> 1113 potentially leak memory?
>> 2) What is info->first_time used for?  It seems to always be set to
>> true in the loop in get_path_histories() and then reset in
>> get_history()
>> 3) Increasing MAX_OPEN_HISTORIES to 128 "fixes" my test repository,
>> but isn't the true fix.
>
> Thanks for bringing this to dev@.
> If it's not too much of a bother, could you also file an issue for this
> and set the target milestone to 1.7.0 so we don't forget about it?
> This milestone doesn't mean that a fix for 1.6.x won't be made.
> It just exploits the fact that currently most people are looking at
> issues scheduled for 1.7.0 :)

I milestone 1.7.0 issue also means that it's a blocker for the 1.7.x
branch, iiuc.

-Hyrum

Re: log -g regression in r1028108

[Stefan Sperling]

On Tue, Jan 25, 2011 at 10:34:43AM -0600, Kevin Radke wrote:
> (I'm moving this conversation from users to dev, since I have
> convinced myself a regression was introduced in r1028108)
> See: http://svn.haxx.se/users/archive-2010-12/0265.shtml
> 
> log -v -g --xml http://server/repo/path commands will now fail for
> "complex" histories that contain file renames.
> 
> The client sees:
> svn: REPORT of '/repo/!svn/bc/1234/path/in/repo': Could not read chunk
> size: connection was closed by server (http://server)
> 
> The server logs:
> [Wed Dec 15 15:48:18 2010] [error] [client 192.168.1.1] File not
> found: revision 5678, path '/path/in/repo/file.txt'  [404, #160013]
> 
> (The file was named oldfilename.txt in r5678, because it was renamed
> in r7890.  Something isn't using the correct name when more than
> MAX_OPEN_HISTORIES have been found.)
> 
> The only source file modified in this commit was subversion/libsvn_reops/log.c
> 
> A few questions:
> 
> 1) Does setting info->oldpool and info->newpool to NULL around lines
> 1113 potentially leak memory?
> 2) What is info->first_time used for?  It seems to always be set to
> true in the loop in get_path_histories() and then reset in
> get_history()
> 3) Increasing MAX_OPEN_HISTORIES to 128 "fixes" my test repository,
> but isn't the true fix.

Thanks for bringing this to dev@.
If it's not too much of a bother, could you also file an issue for this
and set the target milestone to 1.7.0 so we don't forget about it?
This milestone doesn't mean that a fix for 1.6.x won't be made.
It just exploits the fact that currently most people are looking at
issues scheduled for 1.7.0 :)

Thanks,
Stefan

log -g regression in r1028108

[Kevin Radke]

(I'm moving this conversation from users to dev, since I have
convinced myself a regression was introduced in r1028108)
See: http://svn.haxx.se/users/archive-2010-12/0265.shtml

log -v -g --xml http://server/repo/path commands will now fail for
"complex" histories that contain file renames.

The client sees:
svn: REPORT of '/repo/!svn/bc/1234/path/in/repo': Could not read chunk
size: connection was closed by server (http://server)

The server logs:
[Wed Dec 15 15:48:18 2010] [error] [client 192.168.1.1] File not
found: revision 5678, path '/path/in/repo/file.txt'  [404, #160013]

(The file was named oldfilename.txt in r5678, because it was renamed
in r7890.  Something isn't using the correct name when more than
MAX_OPEN_HISTORIES have been found.)

The only source file modified in this commit was subversion/libsvn_reops/log.c

A few questions:

1) Does setting info->oldpool and info->newpool to NULL around lines
1113 potentially leak memory?
2) What is info->first_time used for?  It seems to always be set to
true in the loop in get_path_histories() and then reset in
get_history()
3) Increasing MAX_OPEN_HISTORIES to 128 "fixes" my test repository,
but isn't the true fix.

Thanks!
Kevin R.

Re: Document return codes of 'svn ls'

[Daniel Shahaf]

anatoly techtonik wrote on Fri, Jan 21, 2011 at 13:42:51 +0200:
> No. Internet connection may be down during this specific request. How can I
> distinguish these two errors?

How about something like this:

% svn $subcommand --options --output-apr_err-to-stderr 2>&1 >/dev/null | tail 
-n1
apr_err=210005

Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out there? (was Re: svn commit: r966822)

[Paul Burba]

On Tue, Jan 25, 2011 at 11:27 AM, Paul Burba  wrote:
> On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba  wrote:
>> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato  
>> wrote:
>>> Hey, Paul.  Overall, I think the change makes sense.  I've given some inline
>>> review below.  (This is based mostly on a reading of the patch itself, not a
>>> reading of the patched source files.)
>>
>> Thanks for the review Mike, comments below.
>>
>>> On 01/18/2011 04:44 PM, Paul Burba wrote:
 Index: subversion/mod_dav_svn/reports/update.c
 ===
 --- subversion/mod_dav_svn/reports/update.c   (revision 1060528)
 +++ subversion/mod_dav_svn/reports/update.c   (working copy)
>>>
>>> [...]
>>>
 @@ -413,8 +407,16 @@
      return SVN_NO_ERROR;

    /* ### ack!  binary names won't float here! */
 -  /* If this is a copied file/dir, we can have removed props. */
 -  if (baton->removed_props && (! baton->added || baton->copyfrom))
 +  /* If this is a copied file/dir, we can have removed props.
 +
 +     Old features never die: 1.7+ clients don't require this block because
 +     they never ask for copyfrom information from the server when adding
 +     files created by a copy, but 1.5-1.6 clients will ask for it so we
 +     have to keep sending it.
 +
 +     See http://svn.haxx.se/dev/archive-2010-09/0265.shtml and
 +     http://subversion.tigris.org/issues/show_bug.cgi?id=3711. */
>>>
>>> 1.7's RA interface still allows clients to request copyfrom information, and
>>> we never know if we might later use this codepath again.  So I'm not sure
>>> it's accurate to claim that "1.7+ clients don't require this block".  Maybe
>>> TortoiseSVN is using it?  Maybe our repos diff code will use it (I seem to
>>> recall someone talking about doing this)?  I think this whole comment change
>>> can be reverted.
>>
>> Understood, I removed that comment.
>>
 -  SVN_ERR(dav_svn__brigade_puts(baton->uc->bb, baton->uc->output, 
 ""));
 -
 -  /* Both modern and non-modern clients need the checksum... */
 -  if (baton->text_checksum)
 -    {
 -      SVN_ERR(dav_svn__brigade_printf(baton->uc->bb, baton->uc->output,
 -                                      
 "%s",
 -                                      baton->text_checksum));
 -    }
 -
>>>
>>> This removal doesn't seem right.  There are two kinds of checksum sent over
>>> the wire in this REPORT response:
>>>
>>> 1.  a "base-checksum", which is the checksum of the file against which a
>>> content delta is being transmitted (so the client can verify that it's about
>>> to apply that delta against the right base), and
>>> 2.  a "text-checksum", which is the checksum of final text content (either
>>> as retrieved via fulltext or via delta application to a base.
>>>
>>> Maybe I'm overlooking it, but it seems you're no longer transmitting the
>>> text-checksum any longer.
>>
>> You are correct, I put that back.  I must have had some text_checksum
>> vs. base_checksum confusion, because despite what I said in the log,
>> we *don't* send the text_checksum anywhere else.  No tests failed
>> because the svn_delta_editor_t close_file callback ignores
>> text_checksum if it is NULL (it isn't even used during a merge, even
>> if present).
>>
>> Rerunning the tests for the new patch (attached).
>
> Everything passed.  Committed with a few more comment/log message
> tweaks in http://svn.apache.org/viewvc?view=revision&revision=1063337
>
> I suspect there is more I can do here, particularly in
> mod_dav_svn/reports/update.c: I don't see why upd_change_xxx_prop()
> ever needs to cache removed properties for additions in skelta mode
> (thus letting close_helper() send remove-prop).  Looking at that now.

Scratch that...

As Mike mentioned earlier, "1.7's RA interface still allows clients to
request copyfrom information".  And older (<1.7) clients do exactly
that during updates because they predate the issue #3711[1] fixes and
so get copyfrom-path and copyfrom-rev from the server.  When using
ra_serf, if we didn't send the prop deletions in close_helper, then an
update could add a path created by a copy with properties that don't
exist on the server (a bad thing).

Paul

[1] http://subversion.tigris.org/issues/show_bug.cgi?id=3711
http://svn.apache.org/viewvc?view=revision&revision=998183
http://svn.apache.org/viewvc?view=revision&revision=998193

Re: [PATCH] extend svn_subst_translate_string2() with a REPAIR parameter

[Daniel Shahaf]

Philip Martin wrote on Tue, Jan 25, 2011 at 17:02:18 +:
> Julian Foad  writes:
> 
> > On Tue, 2011-01-25 at 08:28 -0800, Danny Trebbien wrote:
> >> > I made one tweak: in the test for detecting an error, after detecting
> >> > the error, we need to clear the error, otherwise the test harness
> >> > reports success on the individual test but reports a problem at the end
> >> > of the test run:
> >> >
> >> >  PASS:  lt-subst_translate-test 1: test svn_subst_translate_string2()
> >> >  /home/julianfoad/bin/svn-c-test: line 35: 11406 Aborted
> >> >
> >> > So I added "svn_error_clear(err);" after "SVN_TEST_ASSERT(err->apr_err
> >> > == SVN_ERR_IO_INCONSISTENT_EOL);".
> >> 
> >> Is the svn_error_t object reused?
> >
> > It's not re-used.
> >
> > Normally, an error ends up being handled by svn_handle_error2() or some
> > such function, which (after printing out the details) destroys the pool
> > that was allocated for it.
> >
> > The problem was that the pool allocated for this particular error was
> > never being handled, and never being destroyed.  Somewhere there is some
> > code that detects this at program exit time and throws an assertion
> > failure to warn us programmers of this "leak", because normally an error
> > not being handled indicates a mistake in the program logic (and a
> > leaking of memory).
> 
> If you were not seeing the abort then you were probably building without
> -DSVN_DEBUG; that makes the error leak into just a memory leak without
> an abort.
> 

Typically one uses --enable-maintainer-mode, not -DSVN_DEBUG explicitly.

> -- 
> Philip

Re: [PATCH] extend svn_subst_translate_string2() with a REPAIR parameter

[Philip Martin]

Julian Foad  writes:

> On Tue, 2011-01-25 at 08:28 -0800, Danny Trebbien wrote:
>> > I made one tweak: in the test for detecting an error, after detecting
>> > the error, we need to clear the error, otherwise the test harness
>> > reports success on the individual test but reports a problem at the end
>> > of the test run:
>> >
>> >  PASS:  lt-subst_translate-test 1: test svn_subst_translate_string2()
>> >  /home/julianfoad/bin/svn-c-test: line 35: 11406 Aborted
>> >
>> > So I added "svn_error_clear(err);" after "SVN_TEST_ASSERT(err->apr_err
>> > == SVN_ERR_IO_INCONSISTENT_EOL);".
>> 
>> Is the svn_error_t object reused?
>
> It's not re-used.
>
> Normally, an error ends up being handled by svn_handle_error2() or some
> such function, which (after printing out the details) destroys the pool
> that was allocated for it.
>
> The problem was that the pool allocated for this particular error was
> never being handled, and never being destroyed.  Somewhere there is some
> code that detects this at program exit time and throws an assertion
> failure to warn us programmers of this "leak", because normally an error
> not being handled indicates a mistake in the program logic (and a
> leaking of memory).

If you were not seeing the abort then you were probably building without
-DSVN_DEBUG; that makes the error leak into just a memory leak without
an abort.

-- 
Philip

Re: [PATCH] extend svn_subst_translate_string2() with a REPAIR parameter

[Julian Foad]

On Tue, 2011-01-25 at 08:28 -0800, Danny Trebbien wrote:
> > I made one tweak: in the test for detecting an error, after detecting
> > the error, we need to clear the error, otherwise the test harness
> > reports success on the individual test but reports a problem at the end
> > of the test run:
> >
> >  PASS:  lt-subst_translate-test 1: test svn_subst_translate_string2()
> >  /home/julianfoad/bin/svn-c-test: line 35: 11406 Aborted
> >
> > So I added "svn_error_clear(err);" after "SVN_TEST_ASSERT(err->apr_err
> > == SVN_ERR_IO_INCONSISTENT_EOL);".
> 
> Is the svn_error_t object reused?

It's not re-used.

Normally, an error ends up being handled by svn_handle_error2() or some
such function, which (after printing out the details) destroys the pool
that was allocated for it.

The problem was that the pool allocated for this particular error was
never being handled, and never being destroyed.  Somewhere there is some
code that detects this at program exit time and throws an assertion
failure to warn us programmers of this "leak", because normally an error
not being handled indicates a mistake in the program logic (and a
leaking of memory).

>   I thought that whenever an error
> occurs, a new svn_error_t object is allocated.

That's correct.

- Julian

Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out there? (was Re: svn commit: r966822)

[Paul Burba]

On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba  wrote:
> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato  
> wrote:
>> Hey, Paul.  Overall, I think the change makes sense.  I've given some inline
>> review below.  (This is based mostly on a reading of the patch itself, not a
>> reading of the patched source files.)
>
> Thanks for the review Mike, comments below.
>
>> On 01/18/2011 04:44 PM, Paul Burba wrote:
>>> Index: subversion/mod_dav_svn/reports/update.c
>>> ===
>>> --- subversion/mod_dav_svn/reports/update.c   (revision 1060528)
>>> +++ subversion/mod_dav_svn/reports/update.c   (working copy)
>>
>> [...]
>>
>>> @@ -413,8 +407,16 @@
>>>      return SVN_NO_ERROR;
>>>
>>>    /* ### ack!  binary names won't float here! */
>>> -  /* If this is a copied file/dir, we can have removed props. */
>>> -  if (baton->removed_props && (! baton->added || baton->copyfrom))
>>> +  /* If this is a copied file/dir, we can have removed props.
>>> +
>>> +     Old features never die: 1.7+ clients don't require this block because
>>> +     they never ask for copyfrom information from the server when adding
>>> +     files created by a copy, but 1.5-1.6 clients will ask for it so we
>>> +     have to keep sending it.
>>> +
>>> +     See http://svn.haxx.se/dev/archive-2010-09/0265.shtml and
>>> +     http://subversion.tigris.org/issues/show_bug.cgi?id=3711. */
>>
>> 1.7's RA interface still allows clients to request copyfrom information, and
>> we never know if we might later use this codepath again.  So I'm not sure
>> it's accurate to claim that "1.7+ clients don't require this block".  Maybe
>> TortoiseSVN is using it?  Maybe our repos diff code will use it (I seem to
>> recall someone talking about doing this)?  I think this whole comment change
>> can be reverted.
>
> Understood, I removed that comment.
>
>>> -  SVN_ERR(dav_svn__brigade_puts(baton->uc->bb, baton->uc->output, 
>>> ""));
>>> -
>>> -  /* Both modern and non-modern clients need the checksum... */
>>> -  if (baton->text_checksum)
>>> -    {
>>> -      SVN_ERR(dav_svn__brigade_printf(baton->uc->bb, baton->uc->output,
>>> -                                      
>>> "%s",
>>> -                                      baton->text_checksum));
>>> -    }
>>> -
>>
>> This removal doesn't seem right.  There are two kinds of checksum sent over
>> the wire in this REPORT response:
>>
>> 1.  a "base-checksum", which is the checksum of the file against which a
>> content delta is being transmitted (so the client can verify that it's about
>> to apply that delta against the right base), and
>> 2.  a "text-checksum", which is the checksum of final text content (either
>> as retrieved via fulltext or via delta application to a base.
>>
>> Maybe I'm overlooking it, but it seems you're no longer transmitting the
>> text-checksum any longer.
>
> You are correct, I put that back.  I must have had some text_checksum
> vs. base_checksum confusion, because despite what I said in the log,
> we *don't* send the text_checksum anywhere else.  No tests failed
> because the svn_delta_editor_t close_file callback ignores
> text_checksum if it is NULL (it isn't even used during a merge, even
> if present).
>
> Rerunning the tests for the new patch (attached).

Everything passed.  Committed with a few more comment/log message
tweaks in http://svn.apache.org/viewvc?view=revision&revision=1063337

I suspect there is more I can do here, particularly in
mod_dav_svn/reports/update.c: I don't see why upd_change_xxx_prop()
ever needs to cache removed properties for additions in skelta mode
(thus letting close_helper() send remove-prop).  Looking at that now.

Paul

> New Log:
> [[[
> Server-side fix for issue #3657 'dav update report handler in skelta mode can
> cause spurious conflicts'.
>
> This change has two parts that are loosely related:
>
> 1) It stops mod_dav_svn from ever sending the 'fetch-props' response to a
>  client, even when in skelta (i.e. send-all=false) mode.  The server
>  already knows what properties have changed so now it simply sends them,
>  rather than telling the client to ask for them later.  This prevents
>  the svn_delta_editor_t change_[file|dir]_prop callback from receiving
>  faux property changes when the client asks for *all* the properties.
>  See http://subversion.tigris.org/issues/show_bug.cgi?id=3657 for all the
>  gory details.
>
> 2) The comments in our code frequently make reference to 'modern' servers
>  and clients in the context of mod_dav_svn.  Unfortunately there is no such
>  thing as a pre-modern server, they are all modern!  The use of the term
>  'modern' in our code comments predates Subversion 1.0.0, so the comments
>  are misleading and the special case code for pre-modern servers (or clients)
>  is unnecessary.  The special-case code is interdependent with the changes in
>  #1 though, so these changes are being made in one commit, despite the fact
>  one could argue they are logica

Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 4)

[Danny Trebbien]

> Hi Danny.
>
> I committed the patch you refer to, on which this one depends, in
> r1063320.  However, I'm not likely to get around to reviewing this patch
> any time soon.  It would be great if someone else could take it.
>
> - Julian

Okay. That's fine. Thank you for the update.

Re: [PATCH] extend svn_subst_translate_string2() with a REPAIR parameter

[Danny Trebbien]

> I made one tweak: in the test for detecting an error, after detecting
> the error, we need to clear the error, otherwise the test harness
> reports success on the individual test but reports a problem at the end
> of the test run:
>
>  PASS:  lt-subst_translate-test 1: test svn_subst_translate_string2()
>  /home/julianfoad/bin/svn-c-test: line 35: 11406 Aborted
>
> So I added "svn_error_clear(err);" after "SVN_TEST_ASSERT(err->apr_err
> == SVN_ERR_IO_INCONSISTENT_EOL);".

Is the svn_error_t object reused?  I thought that whenever an error
occurs, a new svn_error_t object is allocated.

Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out there? (was Re: svn commit: r966822)

[Paul Burba]

On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba  wrote:
> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato  
> wrote:
>> Hey, Paul.  Overall, I think the change makes sense.  I've given some inline
>> review below.  (This is based mostly on a reading of the patch itself, not a
>> reading of the patched source files.)
>
> Thanks for the review Mike, comments below.
>
>> On 01/18/2011 04:44 PM, Paul Burba wrote:
>>> Index: subversion/mod_dav_svn/reports/update.c
>>> ===
>>> --- subversion/mod_dav_svn/reports/update.c   (revision 1060528)
>>> +++ subversion/mod_dav_svn/reports/update.c   (working copy)
>>
>> [...]
>>
>>> @@ -413,8 +407,16 @@
>>>      return SVN_NO_ERROR;
>>>
>>>    /* ### ack!  binary names won't float here! */
>>> -  /* If this is a copied file/dir, we can have removed props. */
>>> -  if (baton->removed_props && (! baton->added || baton->copyfrom))
>>> +  /* If this is a copied file/dir, we can have removed props.
>>> +
>>> +     Old features never die: 1.7+ clients don't require this block because
>>> +     they never ask for copyfrom information from the server when adding
>>> +     files created by a copy, but 1.5-1.6 clients will ask for it so we
>>> +     have to keep sending it.
>>> +
>>> +     See http://svn.haxx.se/dev/archive-2010-09/0265.shtml and
>>> +     http://subversion.tigris.org/issues/show_bug.cgi?id=3711. */
>>
>> 1.7's RA interface still allows clients to request copyfrom information, and
>> we never know if we might later use this codepath again.  So I'm not sure
>> it's accurate to claim that "1.7+ clients don't require this block".  Maybe
>> TortoiseSVN is using it?  Maybe our repos diff code will use it (I seem to
>> recall someone talking about doing this)?  I think this whole comment change
>> can be reverted.
>
> Understood, I removed that comment.
>
>>> -  SVN_ERR(dav_svn__brigade_puts(baton->uc->bb, baton->uc->output, 
>>> ""));
>>> -
>>> -  /* Both modern and non-modern clients need the checksum... */
>>> -  if (baton->text_checksum)
>>> -    {
>>> -      SVN_ERR(dav_svn__brigade_printf(baton->uc->bb, baton->uc->output,
>>> -                                      
>>> "%s",
>>> -                                      baton->text_checksum));
>>> -    }
>>> -
>>
>> This removal doesn't seem right.  There are two kinds of checksum sent over
>> the wire in this REPORT response:
>>
>> 1.  a "base-checksum", which is the checksum of the file against which a
>> content delta is being transmitted (so the client can verify that it's about
>> to apply that delta against the right base), and
>> 2.  a "text-checksum", which is the checksum of final text content (either
>> as retrieved via fulltext or via delta application to a base.
>>
>> Maybe I'm overlooking it, but it seems you're no longer transmitting the
>> text-checksum any longer.
>
> You are correct, I put that back.  I must have had some text_checksum
> vs. base_checksum confusion, because despite what I said in the log,
> we *don't* send the text_checksum anywhere else.  No tests failed
> because the svn_delta_editor_t close_file callback ignores
> text_checksum if it is NULL (it isn't even used during a merge, even
> if present).
>
> Rerunning the tests for the new patch (attached).

Everything passed.  Committed with a few more comment/log message
tweaks in http://svn.apache.org/viewvc?view=revision&revision=1063337

I suspect there is more I can do here, particularly in
mod_dav_svn/reports/update.c: I don't see why upd_change_xxx_prop()
ever needs to cache removed properties for additions in skelta mode
(thus letting close_helper() send remove-prop).  Looking at that now.

Paul

> New Log:
> [[[
> Server-side fix for issue #3657 'dav update report handler in skelta mode can
> cause spurious conflicts'.
>
> This change has two parts that are loosely related:
>
> 1) It stops mod_dav_svn from ever sending the 'fetch-props' response to a
>  client, even when in skelta (i.e. send-all=false) mode.  The server
>  already knows what properties have changed so now it simply sends them,
>  rather than telling the client to ask for them later.  This prevents
>  the svn_delta_editor_t change_[file|dir]_prop callback from receiving
>  faux property changes when the client asks for *all* the properties.
>  See http://subversion.tigris.org/issues/show_bug.cgi?id=3657 for all the
>  gory details.
>
> 2) The comments in our code frequently make reference to 'modern' servers
>  and clients in the context of mod_dav_svn.  Unfortunately there is no such
>  thing as a pre-modern server, they are all modern!  The use of the term
>  'modern' in our code comments predates Subversion 1.0.0, so the comments
>  are misleading and the special case code for pre-modern servers (or clients)
>  is unnecessary.  The special-case code is interdependent with the changes in
>  #1 though, so these changes are being made in one commit, despite the fact
>  one could argue they are logica

Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 4)

[Julian Foad]

On Fri, 2011-01-14, Danny Trebbien wrote:
> Attached are version 4 of the patch and log message. Also attached are
> two TGZ archives containing the six DUMP files that the new svnsync
> and svnrdump tests depend upon. These TGZ archives can be extracted
> directly into your working copy of trunk.
> 
> Note that this patch depends on
> ,
> which must be committed first.

Hi Danny.

I committed the patch you refer to, on which this one depends, in
r1063320.  However, I'm not likely to get around to reviewing this patch
any time soon.  It would be great if someone else could take it.

- Julian

Status of the branch diff-optimizations-bytes branch

[Hyrum K Wright]

Johan (and other interested parties),
I've been following some of the commits to the
diff-optimizations-branch with interest.  While I've not reviewed them
for technical merit, it appears that others have, and that there is
good work going on in the branch.  I'm wondering what the potential
for merging back to trunk is.  This is the TODO section from the
BRANCH-README:

TODO:

  * eliminate identical prefix [DONE]
  * eliminate identical suffix [DONE]
  * make diff3 and diff4 use datasources_open  [DONE]
 (this may eliminate the need for datasource_open, and the flag
  datasource_opened in token.c#svn_diff__get_tokens)
  * implement (part of) increment_pointers and
decrement_pointers with a macro[DONE]
 (offers another 10% speedup)
  * integrate (some of the) low-level optimizations for prefix/suffix
scanning, proposed by stefan2 [2]  []
  * revv svn_diff.h#svn_diff_fns_t []

It looks like, for the most part, any destabilizing functionality is
completed, and what remains are simply optimizations.  This
optimization work can probably be performed on trunk, and if so, we
should merge the branch back and do the cleanup work there.  The only
bit on the current list of stuff is revving svn_diff_fns_t.  Can that
work be parallelized?

I'm not trying to rush the work, nor do I think it is essential for
1.7, but it feels like a pretty good performance increase, and one
that we shouldn't have any problem shipping.  (If there are currently
API uncertainties, than it would be good to wait until 1.7.x branches,
though.)

Anyway, I just really like the concept / implementation of the branch,
and I'm excited to get it into the hands of users.  Thoughts?

-Hyrum

Re: [PATCH] extend svn_subst_translate_string2() with a REPAIR parameter

[Julian Foad]

On Fri, 2011-01-14, Danny Trebbien wrote:
> On Fri, Jan 14, 2011 at 12:32 PM, Danny Trebbien  wrote:
> > Attached are a patch and log message to extend
> > svn_subst_translate_string2() with a new parameter, REPAIR. This
> > allows the caller to customize whether the translation repairs line
> > endings.
[...]
> Attached are the patch and log message for version 1.1 of this patch.
[...]

Hi Danny.

It looks fine.

I made one tweak: in the test for detecting an error, after detecting
the error, we need to clear the error, otherwise the test harness
reports success on the individual test but reports a problem at the end
of the test run:

  PASS:  lt-subst_translate-test 1: test svn_subst_translate_string2()
  /home/julianfoad/bin/svn-c-test: line 35: 11406 Aborted

So I added "svn_error_clear(err);" after "SVN_TEST_ASSERT(err->apr_err
== SVN_ERR_IO_INCONSISTENT_EOL);".

Committed in r1063320.

Thanks.
- Julian

Re: crash in serf on windows

[Justin Erenkrantz]

On Tue, Jan 25, 2011 at 12:02 AM, Marc Haesen
 wrote:
> I have seen a crash (null pointer access) during svn merge using serf. The
> same command using neon was successful.
>
>
>
> I am using a trunk compiled version of svn (revision 1061787) on windows
> together with a trunk version of serf (revision 1421) compiled with visual
> studio 2010
>
>
>
> The reason is that the serf_httpconn_socket received a null pointer as
> bucket.

The serf_httpconn stuff is probably not quite ready yet - I know
Lieven is still working through some issues there.  You may have far
better luck with the 0.7.x branch which we're prepping for a new
release soon:

 https://serf.googlecode.com/svn/branches/0.7.x

If you can give that a try, it'd be appreciated!

> Attached are the 2 crash report files and a screen dump of the .dmp file
> loaded in the debugger.

Sadly, there's nothing attached...so I have no clue what you're
seeing.  =(  -- justin

crash in serf on windows

[Marc Haesen]

I have seen a crash (null pointer access) during svn merge using serf.
The same command using neon was successful.

 

I am using a trunk compiled version of svn (revision 1061787) on windows
together with a trunk version of serf (revision 1421) compiled with
visual studio 2010

 

The reason is that the serf_httpconn_socket received a null pointer as
bucket.

 

Attached are the 2 crash report files and a screen dump of the .dmp file
loaded in the debugger.

 

Marc