Re: [RFC] diff-optimizations-bytes branch: avoiding function call overhead (?)

[Julian Foad]

On Mon, 2010-12-20, Johan Corveleyn wrote:
 New macro version (increment only, decrement is similar):
 [[[
 /* For all files in the FILE array, increment the curp pointer.  If a file
  * points before the beginning of file, let it point at the first byte again.
  * If the end of the current chunk is reached, read the next chunk in the
  * buffer and point curp to the start of the chunk.  If EOF is reached, set
  * curp equal to endp to indicate EOF. */
 #define increment_pointers(all_files, files_len, pool)   \
   do {   \
 int i;   \
  \
 for (i = 0; i  files_len; i++)  \
 {\
   if (all_files[i]-chunk == -1) /* indicates before beginning of file */\
 all_files[i]-chunk = 0; /* point to beginning of file again */  \
   else if (all_files[i]-curp != all_files[i]-endp - 1) \
 all_files[i]-curp++;\

Hi Johan.

Here you are having to test for two special cases every time: chunk==-1
and curp==endp-1.  I would suggest changing the specification of before
beginning of file to include the promise that curp==endp-1, so that you
don't have to use a separate test here and can instead test for this
special case within increment_chunk().

- Julian

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

[Julian Foad]

On Sun, 2010-12-19 at 16:22 -0800, Danny Trebbien wrote:
 Branko Čibej brane at xbc.nu writes:
   Hi Daniel.
  
   I haven't looked at your patch v5 yet, because this Git diff doesn't
   look quite right.  How will this memcmp expression work properly if the
   two strings being compared are different lengths?
   -  if (translated_eol != NULL  DIFFERENT_EOL_STRINGS(eol_str, 
   eol_str_len,
   -  newline_buf, 
   newline_len))
   +  if (translated_eol != NULL 
   +  memcmp(eol_str,
   + newline_buf,
   + eol_str_len  newline_len ? eol_str_len : newline_len) != 
   0)
 
  Not very well, because it's prefix matching that doesn't actually take
  different string lengths into account and you can get false matches.
  e.g., if eol_str == '\r' (ancient mac mode) and newline_bug == '\r\n'
  ... what you want is more like:
 
  if (translated_eol != NULL 
  (eol_str_len != newline_len ||
   memcmp(eol_str, newline_buff, eol_str_len) != 0))
 
 
 Ack! I didn't see your message until now. Yes, you are right.

Hi Danny.  Please could you enhance your tests so that they can detect
this error (so that they will fail if this error is present).

Thanks.
- Julian


 Attached is version 5.2 of the patch.

Re: [RFC] diff-optimizations-bytes branch: avoiding function call overhead (?)

[Philip Martin]

Johan Corveleyn jcor...@gmail.com writes:

 This makes the diff algorithm another 10% - 15%
 faster (granted, this was measured with my extreme testcase of a 1,5
 Mb file (6 lines), of which most lines are identical
 prefix/suffix).

Can you provide a test script?  Or decribe the test more fully, please.

-- 
Philip

Re: Input validation observations

[Julian Foad]

On Mon, 2010-12-13, Noorul Islam K M wrote:
 Julian Foad julian.f...@wandisco.com writes:
 
* svn mv ^/ ^/ - Cannot move path
  'https:/svn.apache.org/repos/asf' into itself; should not print the URL
  as if it's a local path.
 
 Attached is the patch which improves error message displayed.

 [[[
 Make 'svn move' display correct error message while moving any
 path/URL onto or into itself. Add missing tests.
 
 * subversion/libsvn_client/copy.c
   (try_copy): Display error message based on source type,
   (repos_to_repos_copy): Remove redundant code.
 
 * subversion/tests/cmdline/copy_tests.py
   (move_wc_and_repo_dir_to_itself, test_list): New test
 
 Patch by: Noorul Islam K M noorul{_AT_}collab.net
 ]]]

Committed revision 1051059.  Thanks.

- Julian

Re: Input validation observations - svn info - changing (Not a valid URL) to the specific error message

[Julian Foad]

Re:
  * svn info ^/b - Not a valid URL; should be path '/b' not found
in revision REV?

On Wed, 2010-12-08, Noorul Islam K M wrote:
[...]
 One of the solution that you suggested was to keep what the existing
 code is doing but saying something more helpful than Not a valid
 URL. I thought that the error returned by the API might have useful
 information and could be printed using svn_err_best_message(). For
 example, take the following two cases.
 
 1. a) With the patch
 
 noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn 
 info 
 ^/non-existing

 URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0
 
 svn: A problem occurred; see other errors for details
 
 1. b) Without the patch
 
 noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn 
 info 
 ^/non-existing
 file:///tmp/latestrepo/non-existing:  (Not a valid URL)
 
 svn: A problem occurred; see other errors for details
 
 2. a) With the patch 
 
 noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn 
 info 
 invalid://no-domain
 Unrecognized URL scheme for 'invalid://non-domain'
 
 svn: A problem occurred; see other errors for details
 
 2. b) Without patch
 
 noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn 
 info 
 invalid://no-domain   
 
 invalid://no-domain:  (Not a valid URL)
 
 svn: A problem occurred; see other errors for details
  
 In both the above cases the patch helps the user to have better
 information (what actually went wrong). Also If a user is using the
 client API, I think he will be having these messages than the one that
 we hard coded as of today.

Thanks for these examples and comments.  Yes, I agree the output is much
more helpful with the patch.

What about the (Not a versioned resource) warning?  Just above the
code you changed, there is code to print (Not a versioned resource).
When is that message used?  Should we change it in the same way?  I
don't think it makes sense to change one of these and not the other.


 I ran the test suite and found that one of the test cases was failing
 and I fixed the same. There were no other failures.

Thanks.

[...]
 Make svn info to display the actual error message returned by API when
 illegal URL is passed.
 
 * subversion/svn/info-cmd.c
   (svn_cl__info): Display the actual error message returned by
 svn_client_info3() instead of a custom one.
 
 * subversion/tests/cmdline/basic_tests.py
   (info_nonexisting_file): Modify test case
[...]

 Index: subversion/tests/cmdline/basic_tests.py
 ===
 --- subversion/tests/cmdline/basic_tests.py   (revision 1042948)
 +++ subversion/tests/cmdline/basic_tests.py   (working copy)
 @@ -2270,7 +2270,8 @@
  
# Check for the correct error message
for line in errput:
 -if re.match(.*\(Not a valid URL\).*, line):
 +if re.match(.* + idonotexist_url + .*non-existent in revision 1.*,
 +line):
return
  
# Else never matched the expected error output, so the test failed.
 Index: subversion/svn/info-cmd.c
 ===
 --- subversion/svn/info-cmd.c (revision 1042948)
 +++ subversion/svn/info-cmd.c (working copy)
 @@ -504,6 +504,7 @@
svn_opt_revision_t peg_revision;
svn_info_receiver_t receiver;
const char *path_prefix;
 +  char errbuf[256];

Please move this variable to the inner scope.

  
SVN_ERR(svn_cl__args_to_target_array_print_reserved(targets, os,
opt_state-targets,
 @@ -584,12 +585,9 @@
  }
else if (err-apr_err == SVN_ERR_RA_ILLEGAL_URL)
  {
 -  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), 

You can remove the _() wrapper because this string no longer contains
any local-language text to be translated.

 +svn_err_best_message(err, errbuf, sizeof(errbuf;
  }

I think if we are going to print the error message, we should print it
with an svn:  prefix like we normally do.

The old error message included a fixed string (the same for all possible
errors), which was helpful for people to recognize it as an error
message and for scripts to detect it.  The best message doesn't have
any fixed part.  I think adding an svn:  prefix would enable people
and scripts to recognize it.

- Julian

Re: [RFC] diff-optimizations-bytes branch: avoiding function call overhead (?)

[Johan Corveleyn]

On Mon, Dec 20, 2010 at 11:03 AM, Julian Foad julian.f...@wandisco.com wrote:
 On Mon, 2010-12-20, Johan Corveleyn wrote:
 New macro version (increment only, decrement is similar):
 [[[
 /* For all files in the FILE array, increment the curp pointer.  If a file
  * points before the beginning of file, let it point at the first byte again.
  * If the end of the current chunk is reached, read the next chunk in the
  * buffer and point curp to the start of the chunk.  If EOF is reached, set
  * curp equal to endp to indicate EOF. */
 #define increment_pointers(all_files, files_len, pool)                       
 \
   do {                                                                       
 \
     int i;                                                                   
 \
                                                                              
 \
     for (i = 0; i  files_len; i++)                                          
 \
     {                                                                        
 \
       if (all_files[i]-chunk == -1) /* indicates before beginning of file 
 */\
         all_files[i]-chunk = 0;     /* point to beginning of file again */  
 \
       else if (all_files[i]-curp != all_files[i]-endp - 1)                 
 \
         all_files[i]-curp++;                                                
 \

 Hi Johan.

 Here you are having to test for two special cases every time: chunk==-1
 and curp==endp-1.  I would suggest changing the specification of before
 beginning of file to include the promise that curp==endp-1, so that you
 don't have to use a separate test here and can instead test for this
 special case within increment_chunk().

Good idea. I'll try this tonight ...

Cheers,
-- 
Johan

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

[Julian Foad]

On Mon, 2010-12-20 at 10:11 +, Julian Foad wrote:
 On Sun, 2010-12-19 at 16:22 -0800, Danny Trebbien wrote:
  Branko Čibej brane at xbc.nu writes:
Hi Daniel.
   
I haven't looked at your patch v5 yet, because this Git diff doesn't
look quite right.  How will this memcmp expression work properly if the
two strings being compared are different lengths?
-  if (translated_eol != NULL  DIFFERENT_EOL_STRINGS(eol_str, 
eol_str_len,
-  newline_buf, 
newline_len))
+  if (translated_eol != NULL 
+  memcmp(eol_str,
+ newline_buf,
+ eol_str_len  newline_len ? eol_str_len : newline_len) 
!= 0)
  
   Not very well, because it's prefix matching that doesn't actually take
   different string lengths into account and you can get false matches.
   e.g., if eol_str == '\r' (ancient mac mode) and newline_bug == '\r\n'
   ... what you want is more like:
  
   if (translated_eol != NULL 
   (eol_str_len != newline_len ||
memcmp(eol_str, newline_buff, eol_str_len) != 0))
  
  
  Ack! I didn't see your message until now. Yes, you are right.
 
 Hi Danny.  Please could you enhance your tests so that they can detect
 this error (so that they will fail if this error is present).

Sometimes I should use more words.

What I meant to say is: I'm trying to devote as much of my time as
possible to other things (WC-NG specifically) so if you could help me
out as much as possible with reducing the review effort needed, I'd
really appreciate it.  I assume your test didn't pick up this error
already, and I can't see at a glance whether you have extended the test
so that it does.  If you do that, then that'll save me (and anyone now
and forever) from having to scrutinize the code to see whether it
handles this properly.  That precisely what a regression test is for.

So if you already extended it, apologies as I haven't tried it or
examined it, as I wanted to ask you first.

Also please include the log message each time you attach a patch.
Please could you re-post the patch and log message together even if no
further changes are needed.

Thanks,
- Julian

Re: svn commit: r1049646 - in /subversion/trunk/subversion: bindings/javahl/native/ include/ include/private/ libsvn_repos/ svnadmin/ svnrdump/ tests/cmdline/ tests/cmdline/svntest/

[C. Michael Pilato]

On 12/18/2010 04:03 PM, Daniel Shahaf wrote:
 cmpil...@apache.org wrote on Wed, Dec 15, 2010 at 17:24:17 -:
 @@ -947,14 +953,19 @@ subcommand_load(apr_getopt_t *os, void *
 +  if (err  err-apr_err == SVN_ERR_BAD_PROPERTY_VALUE)
 +return svn_error_quick_wrap(err, 
 +Invalid property value found in 
 dumpstream; 
 +consider repairing the source or using 
 +--bypass-prop-validation while loading.);
 
 Mark message for translation?

Good catch.  r1051144.  Thanks!

-- 
C. Michael Pilato cmpil...@collab.net
CollabNet  www.collab.net  Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1050216 - in /subversion/trunk/subversion: include/private/svn_ra_private.h libsvn_ra/util.c svnrdump/load_editor.c svnsync/main.c

[Daniel Shahaf]

C. Michael Pilato wrote on Mon, Dec 20, 2010 at 11:04:04 -0500:
 On 12/18/2010 04:29 PM, Daniel Shahaf wrote:
  cmpil...@apache.org wrote on Thu, Dec 16, 2010 at 23:10:10 -:
 
 [...]
 
  * subversion/libsvn_ra/util.c
(is_atomicity_error): Moved here from svnsync/main.c.
(svn_ra__release_operational_lock): New, abstracted from 
  svnsync/main.c:maybe_unlock().
(svn_ra__get_operational_lock): New, abstracted from
  svnsync/main.c:get_lock().
 
  
  Not exactly the same as svnsync's versions, since you added the
  'stolen_lock_p' parameter.  (and the log message doesn't mention that)
 
 I'm not claiming they are the same.  I'm claiming that essentially logic
 therein was culled from the svnsync functions.  I note that they are New,
 and it's not our practice to list the parameters of new functions. :-)
 
 If it was a simple function move, I would use the syntax as above with
 is_atomicity_error -- Move here from...  or Was 
 

When I read the log message, I assumed it was a function move+rename.

I didn't know we had just two hard-coded syntaxes whitelisted for use in
the event of moving a function :-)

Will we release 1.5.9? (was Re: 1.5.9 up for testing/signing)

[Hyrum K. Wright]

We got two sigs for each of Windows and Unix for 1.5.9, but the
signing process has stalled from there.  I'd still like to release
this, if possible, but we should probably make a decision at some
point.

-Hyrum

On Thu, Dec 2, 2010 at 3:55 PM, Hyrum K. Wright
hyrum_wri...@mail.utexas.edu wrote:
 1.5.9 tarballs are up for testing and signing.  The magic revision is 
 r1041577:
 http://people.apache.org/~hwright/svn/1.5.9/

 To sign the release, please input your signatures using the script here:
 http://work.hyrumwright.org/pub/svn/collect_sigs.py

 (The script worked pretty well for 1.6.15, but if you discover any
 bugs please let me know.)

 Testing by enthusiastic users is also welcomed (but remember that this
 is not yet a blessed release, with all that that implies).  If you are
 a package maintainer, please do not included this release in your
 distribution until after it has been formally released.

 I'd like to collect all the signatures in time to do a release by December 6.

 -Hyrum

Re: Will we release 1.5.9? (was Re: 1.5.9 up for testing/signing)

[Stefan Sperling]

On Mon, Dec 20, 2010 at 12:31:55PM -0600, Hyrum K. Wright wrote:
 We got two sigs for each of Windows and Unix for 1.5.9, but the
 signing process has stalled from there.  I'd still like to release
 this, if possible, but we should probably make a decision at some
 point.

I have run tests but haven't gotten around to send a sig yet.
Will try to do so later tonight.

Stefan

svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Blair Zajac]

The docs for svn_fs_commit_txn() for read:

 * @note Success or failure of the commit of @a txn is determined by
 * examining the value of @a *new_rev upon this function's return.  If
 * the value is a valid revision number, the commit was successful,
 * even though a n...@c NULL function return value may indicate that
 * something else went wrong.

However, svn_repos_fs_commit_txn() will only run the post-commit hook if 
SVN_NO_ERROR was returned:


  /* Commit. */
  SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));

  /* Run post-commit hooks.   Notice that we're wrapping the error
 with a -specific- errorcode, so that our caller knows not to try
 and abort the transaction. */
  if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
return svn_error_create
  (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
   _(Commit succeeded, but post-commit hook failed));

  return SVN_NO_ERROR;

Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if new_rev 
is a valid rev?


BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev is 
always modified, so the caller doesn't have to set *new_rev to SVN_INVALID_REVNUM.


Blair

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[C. Michael Pilato]

On 12/20/2010 02:14 PM, Blair Zajac wrote:
 Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
 new_rev is a valid rev?

That does seem reasonable, yes.

 BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev is
 always modified, so the caller doesn't have to set *new_rev to
 SVN_INVALID_REVNUM.

We normally don't make that sort of doc statement.  But, in a case such as
this -- where success/fail is determined by something other than an error
value -- I agree that it makes sense.

-- 
C. Michael Pilato cmpil...@collab.net
CollabNet  www.collab.net  Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Philip Martin]

Blair Zajac bl...@orcaware.com writes:

 The docs for svn_fs_commit_txn() for read:

  * @note Success or failure of the commit of @a txn is determined by
  * examining the value of @a *new_rev upon this function's return.  If
  * the value is a valid revision number, the commit was successful,
  * even though a n...@c NULL function return value may indicate that
  * something else went wrong.

 However, svn_repos_fs_commit_txn() will only run the post-commit hook
 if SVN_NO_ERROR was returned:

   /* Commit. */
   SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));

   /* Run post-commit hooks.   Notice that we're wrapping the error
  with a -specific- errorcode, so that our caller knows not to try
  and abort the transaction. */
   if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
 return svn_error_create
   (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
_(Commit succeeded, but post-commit hook failed));

   return SVN_NO_ERROR;

 Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
 new_rev is a valid rev?

I think you are correct, it should.

 BTW, we should have the docs for svn_fs_commit_txn mention that
 *new_rev is always modified, so the caller doesn't have to set
 *new_rev to SVN_INVALID_REVNUM.

It's implied by the note comment.  I suppose it could be clarified.

-- 
Philip

Re: svn commit: r1051164 - /subversion/trunk/subversion/libsvn_ra/util.c

[Philip Martin]

cmpil...@apache.org writes:

 Author: cmpilato
 Date: Mon Dec 20 16:07:08 2010
 New Revision: 1051164

 URL: http://svn.apache.org/viewvc?rev=1051164view=rev
 Log:
 Another follow-up to r1050216.

 * subversion/libsvn_ra/util.c
   (svn_ra__release_operational_lock): Switch the sense of the boolean
 return from svn_string_compare().  I forgot that it returns TRUE for
 matches, the opposite of the previous strcmp() code that was in place.

The original code did strcmp() == 0 so matches was TRUE there as
well.

-- 
Philip

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Daniel Shahaf]

Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
 The docs for svn_fs_commit_txn() for read:

  * @note Success or failure of the commit of @a txn is determined by
  * examining the value of @a *new_rev upon this function's return.  If
  * the value is a valid revision number, the commit was successful,
  * even though a n...@c NULL function return value may indicate that
  * something else went wrong.

 However, svn_repos_fs_commit_txn() will only run the post-commit hook if  
 SVN_NO_ERROR was returned:

   /* Commit. */
   SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));

   /* Run post-commit hooks.   Notice that we're wrapping the error
  with a -specific- errorcode, so that our caller knows not to try
  and abort the transaction. */
   if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
 return svn_error_create
   (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
_(Commit succeeded, but post-commit hook failed));

   return SVN_NO_ERROR;

 Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if 
 new_rev is a valid rev?


That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
return value.  When can that happen?

(just on Saturday I drafted a patch to make failing to update
rep-cache.db after the commit itself succeeded not be considered
a fatal error; that would be one case when that can happen.)

 BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev 
 is always modified, so the caller doesn't have to set *new_rev to 
 SVN_INVALID_REVNUM.

 Blair

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Philip Martin]

Daniel Shahaf d...@daniel.shahaf.name writes:

 Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:

 Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if 
 new_rev is a valid rev?


 That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
 return value.  When can that happen?

Packing or rep-caching processing after the new rev is created.

-- 
Philip

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Blair Zajac]

On Dec 20, 2010, at 11:48 AM, Daniel Shahaf wrote:

 Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
 The docs for svn_fs_commit_txn() for read:
 
 * @note Success or failure of the commit of @a txn is determined by
 * examining the value of @a *new_rev upon this function's return.  If
 * the value is a valid revision number, the commit was successful,
 * even though a n...@c NULL function return value may indicate that
 * something else went wrong.
 
 However, svn_repos_fs_commit_txn() will only run the post-commit hook if  
 SVN_NO_ERROR was returned:
 
  /* Commit. */
  SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
 
  /* Run post-commit hooks.   Notice that we're wrapping the error
 with a -specific- errorcode, so that our caller knows not to try
 and abort the transaction. */
  if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
return svn_error_create
  (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
   _(Commit succeeded, but post-commit hook failed));
 
  return SVN_NO_ERROR;
 
 Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if 
 new_rev is a valid rev?
 
 
 That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
 return value.  When can that happen?
 
 (just on Saturday I drafted a patch to make failing to update
 rep-cache.db after the commit itself succeeded not be considered
 a fatal error; that would be one case when that can happen.)

Was the patch going to swallow the error?  If there is a deployment issue 
causing rep-cache.db not to be updated, I would like to know about it.

Blair

Re: svn commit: r1051164 - /subversion/trunk/subversion/libsvn_ra/util.c

[C. Michael Pilato]

On 12/20/2010 02:45 PM, Philip Martin wrote:
 cmpil...@apache.org writes:
 
 Author: cmpilato
 Date: Mon Dec 20 16:07:08 2010
 New Revision: 1051164

 URL: http://svn.apache.org/viewvc?rev=1051164view=rev
 Log:
 Another follow-up to r1050216.

 * subversion/libsvn_ra/util.c
   (svn_ra__release_operational_lock): Switch the sense of the boolean
 return from svn_string_compare().  I forgot that it returns TRUE for
 matches, the opposite of the previous strcmp() code that was in place.
 
 The original code did strcmp() == 0 so matches was TRUE there as
 well.

Doh!  That's what I get for reading the code too quickly.  (Thanks for the
review, Philip.)

-- 
C. Michael Pilato cmpil...@collab.net
CollabNet  www.collab.net  Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Daniel Shahaf]

Blair Zajac wrote on Mon, Dec 20, 2010 at 12:03:09 -0800:
 
 On Dec 20, 2010, at 11:48 AM, Daniel Shahaf wrote:
 
  Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
  The docs for svn_fs_commit_txn() for read:
  
  * @note Success or failure of the commit of @a txn is determined by
  * examining the value of @a *new_rev upon this function's return.  If
  * the value is a valid revision number, the commit was successful,
  * even though a n...@c NULL function return value may indicate that
  * something else went wrong.
  
  However, svn_repos_fs_commit_txn() will only run the post-commit hook if  
  SVN_NO_ERROR was returned:
  
   /* Commit. */
   SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
  
   /* Run post-commit hooks.   Notice that we're wrapping the error
  with a -specific- errorcode, so that our caller knows not to try
  and abort the transaction. */
   if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
 return svn_error_create
   (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
_(Commit succeeded, but post-commit hook failed));
  
   return SVN_NO_ERROR;
  
  Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if 
  new_rev is a valid rev?
  
  
  That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
  return value.  When can that happen?
  
  (just on Saturday I drafted a patch to make failing to update
  rep-cache.db after the commit itself succeeded not be considered
  a fatal error; that would be one case when that can happen.)
 
 Was the patch going to swallow the error?  If there is a deployment issue 
 causing rep-cache.db not to be updated, I would like to know about it.
 

It wasn't going to silently swallow the error:
http://mid.gmane.org/20101218184143.ga8...@daniel3.local

 Blair
 

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Daniel Shahaf]

Philip Martin wrote on Mon, Dec 20, 2010 at 19:59:37 +:
 Daniel Shahaf d...@daniel.shahaf.name writes:
 
  Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
 
  Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if 
  new_rev is a valid rev?
 
 
  That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
  return value.  When can that happen?
 
 Packing or rep-caching processing after the new rev is created.
 

Agreed that (in both of these cases) running the post-commit hook anyway
would make sense.

 -- 
 Philip

Re: [RFC] diff-optimizations-bytes branch: avoiding function call overhead (?)

[Johan Corveleyn]

On Mon, Dec 20, 2010 at 2:16 PM, Johan Corveleyn jcor...@gmail.com wrote:
 On Mon, Dec 20, 2010 at 11:03 AM, Julian Foad julian.f...@wandisco.com 
 wrote:
 On Mon, 2010-12-20, Johan Corveleyn wrote:
 New macro version (increment only, decrement is similar):
 [[[
 /* For all files in the FILE array, increment the curp pointer.  If a file
  * points before the beginning of file, let it point at the first byte 
 again.
  * If the end of the current chunk is reached, read the next chunk in the
  * buffer and point curp to the start of the chunk.  If EOF is reached, set
  * curp equal to endp to indicate EOF. */
 #define increment_pointers(all_files, files_len, pool)                      
  \
   do {                                                                      
  \
     int i;                                                                  
  \
                                                                             
  \
     for (i = 0; i  files_len; i++)                                         
  \
     {                                                                       
  \
       if (all_files[i]-chunk == -1) /* indicates before beginning of file 
 */\
         all_files[i]-chunk = 0;     /* point to beginning of file again */ 
  \
       else if (all_files[i]-curp != all_files[i]-endp - 1)                
  \
         all_files[i]-curp++;                                               
  \

 Hi Johan.

 Here you are having to test for two special cases every time: chunk==-1
 and curp==endp-1.  I would suggest changing the specification of before
 beginning of file to include the promise that curp==endp-1, so that you
 don't have to use a separate test here and can instead test for this
 special case within increment_chunk().

 Good idea. I'll try this tonight ...

Ok, this worked pretty well. It's a little bit faster (about 1 second,
which seems right intuitively).

One style question though: should these macros be all upper case
(INCREMENT_POINTERS and DECREMENT_POINTERS)? I guess so.

The code now looks as follows (in attachment a patch relative to
diff-optimizations-bytes branch):

[[[
/* For all files in the FILE array, increment the curp pointer.  If a file
 * points before the beginning of file, let it point at the first byte again.
 * If the end of the current chunk is reached, read the next chunk in the
 * buffer and point curp to the start of the chunk.  If EOF is reached, set
 * curp equal to endp to indicate EOF. */
#define increment_pointers(all_files, files_len, pool)   \
  do {   \
int i;   \
 \
for (i = 0; i  files_len; i++)  \
{\
  if (all_files[i]-curp  all_files[i]-endp - 1)   \
all_files[i]-curp++;\
  else   \
SVN_ERR(increment_chunk(all_files[i], pool));\
}\
  } while (0)


/* For all files in the FILE array, decrement the curp pointer.  If the
 * start of a chunk is reached, read the previous chunk in the buffer and
 * point curp to the last byte of the chunk.  If the beginning of a FILE is
 * reached, set chunk to -1 to indicate BOF. */
#define decrement_pointers(all_files, files_len, pool)   \
  do {   \
int i;   \
 \
for (i = 0; i  files_len; i++)  \
{\
  if (all_files[i]-curp  all_files[i]-buffer) \
all_files[i]-curp--;\
  else   \
SVN_ERR(decrement_chunk(all_files[i], pool));\
}\
  } while (0)


static svn_error_t *
increment_chunk(struct file_info *file, apr_pool_t *pool)
{
  apr_off_t length;
  apr_off_t last_chunk = offset_to_chunk(file-size);

  if (file-chunk == -1)
{
  /* We are at BOF (Beginning Of File). Point to first chunk/byte again. */
  file-chunk = 0;
  file-curp = file-buffer;
}
  else if (file-chunk == last_chunk)
{
  /* We are at the last chunk. Indicate EOF by setting curp == endp. */
  file-curp = file-endp;
}
  

Default commandline args

[Hyrum K. Wright]

In issue #3765, I suggest the possibility of a CVS-style config file
wherein a user could specify a set of default arguments for a given
subcommand.  For example, 'svn diff' would default to 'svn diff -x -p'
if the config file had an entry for such.  Thinking this would be an
SMOP, I jumped in...only to discover that it isn't.

The actual implementation isn't hard, I'm just wondering about where
the configuration should live.  We have a ~/.subversion/config file,
but these options aren't for the client library, they are specific to
the commandline client.  This makes me think that we should have a
separate file for the commandline client, and that it may want to be
something like ~/.svnrc or something equally commandline-ish.

Thoughts?

-Hyrum

http://subversion.tigris.org/issues/show_bug.cgi?id=3765

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

[Julian Foad]

On Mon, 2010-12-20, Danny Trebbien wrote:
 [[[
 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.
[...]

Thanks, Danny.

Committed revision 1051322.

The main patch looks great.  In the test file, I made the minimum
changes necessary to comply with the C'89 language (no // comments, no
declarations after code), and committed it.  However I suggest many
further changes to the test code, mainly making it data-driven (use a
table of inputs and expected outputs) and use helpers such as
SVN_TEST_STRING_ASSERT() to maximize readability.  To this end, I
suggest the version attached here.  Do you want to check this for me, to
see if it still does what you wanted it to, and let me know if I should
commit it.

Thanks again.
- Julian

/*
 * subst_translate-test.c -- test the svn_subst_translate* functions
 *
 * 
 *Licensed to the Apache Software Foundation (ASF) under one
 *or more contributor license agreements.  See the NOTICE file
 *distributed with this work for additional information
 *regarding copyright ownership.  The ASF licenses this file
 *to you under the Apache License, Version 2.0 (the
 *License); you may not use this file except in compliance
 *with the License.  You may obtain a copy of the License at
 *
 *  http://www.apache.org/licenses/LICENSE-2.0
 *
 *Unless required by applicable law or agreed to in writing,
 *software distributed under the License is distributed on an
 *AS IS BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 *KIND, either express or implied.  See the License for the
 *specific language governing permissions and limitations
 *under the License.
 * 
 */

#include ../svn_test.h

#include svn_types.h
#include svn_string.h
#include svn_subst.h

/* Test inputs and expected output for svn_subst_translate_string2(). */
struct translate_string2_data_t
{
  const char *source;
  const char *expected_str;
  svn_boolean_t translated_to_utf8;
  svn_boolean_t translated_line_endings;
};

static svn_error_t *
test_svn_subst_translate_string2(apr_pool_t *pool)
{
  static const struct translate_string2_data_t tests[] =
{
  /* No reencoding, no translation of line endings */
  { abcdefz,
abcdefz, FALSE, FALSE },
  /* No reencoding, translation of line endings */
  {  \r\n\r\n  \r\n\r\n,
 \n\n  \n\n, FALSE, TRUE },
  /* Reencoding, no translation of line endings */
  { \xc7\xa9\xf4\xdf,
\xc3\x87\xc2\xa9\xc3\xb4\xc3\x9f, TRUE, FALSE },
  /* Reencoding, translation of line endings */
  { \xc7\xa9\xf4\xdf\r\n,
\xc3\x87\xc2\xa9\xc3\xb4\xc3\x9f\n, TRUE, TRUE },
  { NULL, NULL, FALSE, FALSE }
};
  const struct translate_string2_data_t *t;

  for (t = tests; t-source != NULL; t++)
  {
svn_string_t *source_string = svn_string_create(t-source, pool);
svn_string_t *new_value = NULL;
svn_boolean_t translated_line_endings = ! t-translated_line_endings;
svn_boolean_t translated_to_utf8;

SVN_ERR(svn_subst_translate_string2(new_value,
NULL, translated_line_endings,
source_string, ISO-8859-1,
pool, pool));
SVN_TEST_STRING_ASSERT(new_value-data, t-expected_str);
SVN_TEST_ASSERT(translated_line_endings == t-translated_line_endings);

new_value = NULL;
translated_to_utf8 = ! t-translated_to_utf8;
translated_line_endings = ! t-translated_line_endings;
SVN_ERR(svn_subst_translate_string2(new_value, translated_to_utf8,
translated_line_endings,
source_string, ISO-8859-1,
pool, pool));
SVN_TEST_STRING_ASSERT(new_value-data, t-expected_str);
SVN_TEST_ASSERT(translated_to_utf8 == t-translated_to_utf8);
SVN_TEST_ASSERT(translated_line_endings == t-translated_line_endings);
  }

  return SVN_NO_ERROR;
}

/*static svn_error_t *
test_svn_subst_copy_and_translate4(apr_pool_t *pool)
{
  return SVN_NO_ERROR;
}

static svn_error_t *
test_svn_subst_stream_translated(apr_pool_t *pool)
{
  return SVN_NO_ERROR;
}*/

/* Test inputs and expected output for svn_subst_translate_cstring2(). */
struct translate_cstring2_data_t
{
  const char *source;
  const char *eol_str;
  svn_boolean_t repair;
  const char *expected_str;
};

static svn_error_t *
test_svn_subst_translate_cstring2(apr_pool_t *pool)
{
  static const struct translate_cstring2_data_t tests[] =
{
  /* Test the unusual case where EOL_STR is an empty string. */
  {\r   \n\r\n \n\n\n, , TRUE,
   

Re: Default commandline args

[C. Michael Pilato]

On 12/20/2010 05:28 PM, Hyrum K. Wright wrote:
 In issue #3765, I suggest the possibility of a CVS-style config file
 wherein a user could specify a set of default arguments for a given
 subcommand.  For example, 'svn diff' would default to 'svn diff -x -p'
 if the config file had an entry for such.  Thinking this would be an
 SMOP, I jumped in...only to discover that it isn't.
 
 The actual implementation isn't hard, I'm just wondering about where
 the configuration should live.  We have a ~/.subversion/config file,
 but these options aren't for the client library, they are specific to
 the commandline client.  This makes me think that we should have a
 separate file for the commandline client, and that it may want to be
 something like ~/.svnrc or something equally commandline-ish.

~/.svnrc makes perfect sense to me (with /etc/svnrc as a system-wide
default, if you wanna roll that way).

-- 
C. Michael Pilato cmpil...@collab.net
CollabNet  www.collab.net  Distributed Development On Demand

Re: Default commandline args

[Blair Zajac]

On 12/20/10 8:30 PM, C. Michael Pilato wrote:

On 12/20/2010 05:28 PM, Hyrum K. Wright wrote:

In issue #3765, I suggest the possibility of a CVS-style config file
wherein a user could specify a set of default arguments for a given
subcommand.  For example, 'svn diff' would default to 'svn diff -x -p'
if the config file had an entry for such.  Thinking this would be an
SMOP, I jumped in...only to discover that it isn't.

The actual implementation isn't hard, I'm just wondering about where
the configuration should live.  We have a ~/.subversion/config file,
but these options aren't for the client library, they are specific to
the commandline client.  This makes me think that we should have a
separate file for the commandline client, and that it may want to be
something like ~/.svnrc or something equally commandline-ish.


~/.svnrc makes perfect sense to me (with /etc/svnrc as a system-wide
default, if you wanna roll that way).


What about ~/.subversion/cli or ~/.subversion/command-line-client.  If somebody 
wants to copy an existing configuration from another user, they just can't copy 
~/.subversion now.


Blair

Re: Default commandline args

[Stefan Sperling]

On Mon, Dec 20, 2010 at 10:34:46PM -0800, Blair Zajac wrote:
 What about ~/.subversion/cli or ~/.subversion/command-line-client.
 If somebody wants to copy an existing configuration from another
 user, they just can't copy ~/.subversion now.

There are other clients such as TortoiseSVN which have some config
settings outside of ~/.subversion. Users if those clients cannot
just copy ~/.subversion either.

But I suppose the CLI client is the default client we support.
So I don't see an issue with having its configuration live within
~/.subversion.  FWIW, I'd prefer the name ~/.subversion/svnrc.

The svnrc file should not be created by libsvn_client, though.
The cli client could create it after initializing the client library,
at which point the config directory is guaranteed to exist (if I remember
correctly).

Stefan