Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 4:38 PM, Daniel Shahaf wrote:

Blair Zajac wrote on Wed, Dec 22, 2010 at 13:22:11 -0800:

On 12/22/10 10:56 AM, Daniel Shahaf wrote:

Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:

Since probably 100% of the time the last error in a
chain is not a tracing error, then err_abort() will be properly
deregistered by svn_error_clear().



Agreed.  As to the 'lost' links (those not pointed to any more),
I believe they're allocated from the same pool as every other link in
the chain, so they'll be freed when that chain is cleared.


Yes.


It seems the best thing is to have svn_error_purge_tracing() return a
brand new chain that shares no links with the original chain, and then
both need to be freed.



That makes sense; if we'll work with complete chains, there's a smaller
chance of more edge-case bugs.


Thinking on this a little more, since most people won't use debugging,
it'll be wasteful to have svn_error_purge_tracing() make a duplicate
chain that will have no tracing errors in it.



IOW, duplicating in maintainer mode forces duplicating in non-maintainer
mode too, making the common case costlier.  True.

However, we could have a macro that causes the dup() operation to only
be done in maintainer mode:

   void svn_error_purge_tracing_shell(svn_error_t **err_p)
   {
   #ifdef SVN_ERR__TRACING
 svn_error_t *err = svn_error_purge_tracing(*err_p);
 svn_error_clear(*err_p);
 *err_p = err;
   #endif /* SVN_ERR__TRACING */
   }


So maybe we should have svn_error_purge_tracing() be related to the input
chain and share what it can, but not be an error chain one should use
svn_error_clear() on.



IOW, change its promise such that it's the passed-in error, not the
returned one, which should be cleared?  And have it leave the passed-in
chain untouched?

It makes sense on a first scan, but at 3am it's too late for a full +1.


I've attached a patch to review.  It fixes both svn_error_purge_tracing() and 
the the behavior of svn_repos__post_commit_error_str().



For example, shouldn't the following code trigger err_abort() on the
outer (wrapper) link?

   svn_error_t *in = svn_error_create(...);
   svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
   svn_error_t *purged = svn_error_purge_tracing(original);
   svn_error_clear(purged);
   exit(0);

Am I making sense?


I don't think so, because if SVN_DEBUG is defined, then it finds the
error at the end of the chain, which in this example is a non-tracing
error, and it'll properly remove the err_abort.



You're right, I'll modify my example to this:

  SVN_ERR__TRACED  /* start of chain */
  SVN_ERR__TRACED
  "non-tracing link" /* end of chain */

In this case, the middle link would be lost.  Right?


Both SVN_ERR__TRACED would be lost by svn_error_purge_tracing(), right?



Won't the inner while() loop would skip over them?


Yes, I think it would.  We should probably write a unit test for 
svn_error_purge_tracing().


Blair
Index: subversion/libsvn_subr/error.c
===
--- subversion/libsvn_subr/error.c  (revision 1052167)
+++ subversion/libsvn_subr/error.c  (working copy)
@@ -349,39 +349,47 @@
 svn_error_purge_tracing(svn_error_t *err)
 {
 #ifdef SVN_ERR__TRACING
-  svn_error_t *tmp_err = err;
   svn_error_t *new_err = NULL, *new_err_leaf = NULL;
 
   if (! err)
 return SVN_NO_ERROR;
 
-  while (tmp_err)
+  do
 {
   /* Skip over any trace-only links. */
-  while (tmp_err && is_tracing_link(tmp_err))
-tmp_err = tmp_err->child;
+  while (err && is_tracing_link(err))
+err = err->child;
 
-  /* Add a new link to the new chain (creating the chain if necessary). */
-  if (! new_err)
+  if (err)
 {
-  new_err = tmp_err;
-  new_err_leaf = new_err;
+  /* Copy the current error except for its child error pointer
+ into the new error.  Share any message and source
+ filename strings from the error. */
+  svn_error_t *tmp_err = apr_palloc(err->pool, sizeof(*tmp_err));
+  *tmp_err = *err;
+  tmp_err->child = NULL;
+
+  /* Add a new link to the new chain (creating the chain if
+ necessary). */
+  if (! new_err)
+{
+  new_err = tmp_err;
+  new_err_leaf = tmp_err;
+}
+  else
+{
+  new_err_leaf->child = tmp_err;
+  new_err_leaf = tmp_err;
+}
+
+  /* Advance to the next link in the original chain. */
+  err = err->child;
 }
-  else
-{
-  new_err_leaf->child = tmp_err;
-  new_err_leaf = new_err_leaf->child;
-}
+} while (err);
 
-  /* Advance to the next link in the original chain. */
-  tmp_err = tmp_err->child;
-}
-
   /* If we get here, there had better be a real link 

Re: Setting $HOME in the testsuite

[Branko Čibej]

On 23.12.2010 06:09, Peter Samuelson wrote:
> And I still think that 90% of end users, maybe 99%, do _not_ think of
> Subversion as a stack of libraries that just happens to also ship some
> command-line wrapper programs as a convenience.  Consequently I think
> they'd prefer the smaller top-level-dotfile footprint of
> ~/.subversion/svnrc, even if we would historically regard it
> as a layering violation.

I agree with that part, not least because there's no concept of
~/.subversion or ~/.svnrc on Windows; we put the subversion config dir
in the user's (or system) profile dir, which is not equivalent to $HOME.
It's much easier for everyone to deal with one set of config file
locations, even on Unix-derived systems.

-- Brane

Re: Setting $HOME in the testsuite

[Peter Samuelson]

[Hyrum K Wright]
> Just curious if this suggestion is made in light of the concerns I
> expressed in the other thread.  Namely, that the location of the
> config directory isn't determined until parsing the commandline args,
> at which point it may be too late to introduce the additional args
> obtained through the newly-discovered config file.

Sorry, missed that half of that thread.  I've read it now.

Brane brings up the good point that, regardless of whether svnrc is
sensitive to --config-dir, it will need to be sensitive to --no-rc.
At which point, parsing order of --config-dir is a solved problem.

And I still think that 90% of end users, maybe 99%, do _not_ think of
Subversion as a stack of libraries that just happens to also ship some
command-line wrapper programs as a convenience.  Consequently I think
they'd prefer the smaller top-level-dotfile footprint of
~/.subversion/svnrc, even if we would historically regard it
as a layering violation.
-- 
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/

Re: Setting $HOME in the testsuite

[Branko Čibej]

On 23.12.2010 01:37, Hyrum K Wright wrote:
> More fallout from my issue #3765 work: I'm investing using ~/.svnrc to
> provide default arguments to the commandline client.  In doing so, it
> makes loads of sense not to use the a developer's ~/.svnrc when
> running the tests.  Since this is found via the svn_user_get_homedir()
> function, which uses the $HOME environment variable, we coud easily
> redirect that to avoid this conflict.  (It would also allow testing of
> this feature.)
>
> I know we provide a standard config directory in the testsuite, but
> wouldn't it also be reasonable to create a dummy home directory?
> Grepping through the python test suite yield no results for HOME, so I
> assume we are currently using the developer's home directory.

You need to add an option to ignore that "svnrc" (wherever it is)
anyway, otherwise there's no safe way to write scripts that work
reliably on different systems. By implication, the command-line parsing
will have to be revisited:

* parse the command-line
* if not ignored, parse "svnrc" and set option defaults based on the
  command
* re-apply options from the actual command line because these should
  (and currently do) override any coming from a config file.

Then just tell the test suite to ignore "svnrc". Problem solved, not
least because $HOME has no real meaning on Windows, AFAICR.

I don't believe you can skip one of the parsing steps (i.e., can't
introduce "svnrc" and /not/ dig deep into command and option resolution
order) and still make that config file useful.

-- Brane

Re: Setting $HOME in the testsuite

[Hyrum K Wright]

On Wed, Dec 22, 2010 at 9:31 PM, Peter Samuelson  wrote:
>
> [Hyrum K Wright]
>> More fallout from my issue #3765 work: I'm investing using ~/.svnrc
>> to provide default arguments to the commandline client.  In doing so,
>> it makes loads of sense not to use the a developer's ~/.svnrc when
>> running the tests.
>
> I'd suggest putting svnrc inside ~/.subversion (or --config-dir).  I
> know ~/.subversion is for the library and svnrc is for the command line
> binary, but I think for an end user that's a pretty fine distinction to
> make.  The last thing I need are even more top-level dotfiles in my
> $HOME.
>
> And using --config-dir would make your $HOME question moot.

Just curious if this suggestion is made in light of the concerns I
expressed in the other thread.  Namely, that the location of the
config directory isn't determined until parsing the commandline args,
at which point it may be too late to introduce the additional args
obtained through the newly-discovered config file.

I'm not claiming that ~/.svnrc is the optimal solution, just pointing
out something would would need to be handled if we used
~/.subversion/foo.

-Hyrum

Re: Setting $HOME in the testsuite

[Peter Samuelson]

[Hyrum K Wright]
> More fallout from my issue #3765 work: I'm investing using ~/.svnrc
> to provide default arguments to the commandline client.  In doing so,
> it makes loads of sense not to use the a developer's ~/.svnrc when
> running the tests.

I'd suggest putting svnrc inside ~/.subversion (or --config-dir).  I
know ~/.subversion is for the library and svnrc is for the command line
binary, but I think for an end user that's a pretty fine distinction to
make.  The last thing I need are even more top-level dotfiles in my
$HOME.

And using --config-dir would make your $HOME question moot.
-- 
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/

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

[Gavin Beau Baumanis]

Hi Johan,

I was intrigued by your requirement to create a large file for testing.
I remember from a really long time ago when I learnt C, that we used a specific 
algorithm for creating "natural" and "random" text.
With some help from Mr.Google found out about Markov Chains that look promising 
- I can't remember if that was what I learned about or not  - but it looks like 
it might be a prove helpful none the less.

A little further Googlng and I found this specific post on stackoverflow.
http://stackoverflow.com/questions/1037719/how-can-i-quickly-create-large-1gb-textbinary-files-with-natural-content


No Idea if it is going to help you specifically or not... but there are quite a 
few ideas in the comments;
 * Obtain a copy of the first 100MB from wikipedia - for example.

Finally, if you happen to a large enough file already, could you not use 
"split" (unix) function to give you a specific sized file?


Actually - I was so intrigued by the "challenge" of this - I have had a think 
of it over lunch;

Could you not do this?
(pseudo -code)

Start with a known "chunk" - say the license file.
get length of license file - for example only  = 125bytes
append the chunk to itself until such time as you have the required size.
write the file once.

startSize = length(knownFile);
int numberOfLoops = log2(desiredFileSize / knownFile) ;
newFile = knownFile

Loop for numberOfLoops
{
newFile = newFile +newFile
}

write newFile;



Gavin



On 23/12/2010, at 11:51 AM, Johan Corveleyn wrote:

> On Wed, Dec 22, 2010 at 11:50 AM, Philip Martin
>  wrote:
>> Johan Corveleyn  writes:
>> 
>>> On Mon, Dec 20, 2010 at 11:19 AM, Philip Martin
>>>  wrote:
 Johan Corveleyn  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.
>>> 
>>> Hmm, it's not easy to come up with a test script to test this "from
>>> scratch" (unless with testing diff directly, see below). I test it
>>> with a repository (a dump/load of an old version of our production
>>> repository) which contains this 6 line xml file (1,5 Mb) with 2272
>>> revisions.
>>> 
>>> I run blame on this file, over svnserve protocol on localhost (server
>>> running on same machine), with an svnserve built from Stefan^2's
>>> performance branch (with membuffer caching of full-texts, so server
>>> I/O is not the bottleneck). This gives me an easy way to call 2272
>>> times diff on this file, and measure it (with the help of some
>>> instrumentation code in blame.c, see attachment). And it's
>>> incidentally the actual use case I first started out wanting to
>>> optimize (blame for large files with many revisions).
>> 
>> Testing with real-world data is important, perhaps even more important
>> than artificial test data, but some test data would be useful.  If you
>> were to write a script to generate two test files of size 100MB, say,
>> then you could use the tools/diff/diff utility to run Subversion diff on
>> those two files.  Or tools/diff/diff3 if it's a 3-way diff that matters.
>> The first run might involve disk IO, but on most machines the OS should
>> be able to cache the files and subsequent hot-cache runs should be a
>> good way to profile the diff code, assumming it is CPU limited.
> 
> Yes, that's a good idea. I'll try to spend some time on that. But I'm
> wondering about a good way to write such a script.
> 
> I'd like the script to generate large files quickly, and with content
> that's not totally random, but also not 100 times the exact same
> line (either of those are not going to be representative for real
> world data, might hit some edge behavior of the diff algorithm).
> (maybe totally random is fine, but is there an easy/fast way to
> generate this?)
> 
> As a first attempt, I quickly hacked up a small shell script, writing
> out lines in a for loop, one by one, with a fixed string together with
> the line number (index of the iteration). But that's too slow (1
> lines of 70 bytes, i.e. 700Kb, is already taking 14 seconds).
> 
> Maybe I can start with 10 or 20 different lines (or generate 100 in a
> for loop), and then start doubling that until I have enough (cat
> file.txt >> file.txt). That will probably be faster. And it might be
> "real-worldish" enough (a single source file also contains many
> identical lines, e.g. all lines with a single brace etc.).
> 
> Other ideas? Maybe there is already something like this lying around?
> 
> Another question: a shell script might not be good, because not
> portable (and not fast)? Should I use python for this? Maybe the
> "write line by line with a line number in a for loop" would be a lot
> faster in Python? I don't know a lot of python, but it might be a good
> opportunity to learn some ...
> 
> Are there any examples of such "manua

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

[Johan Corveleyn]

On Wed, Dec 22, 2010 at 11:50 AM, Philip Martin
 wrote:
> Johan Corveleyn  writes:
>
>> On Mon, Dec 20, 2010 at 11:19 AM, Philip Martin
>>  wrote:
>>> Johan Corveleyn  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.
>>
>> Hmm, it's not easy to come up with a test script to test this "from
>> scratch" (unless with testing diff directly, see below). I test it
>> with a repository (a dump/load of an old version of our production
>> repository) which contains this 6 line xml file (1,5 Mb) with 2272
>> revisions.
>>
>> I run blame on this file, over svnserve protocol on localhost (server
>> running on same machine), with an svnserve built from Stefan^2's
>> performance branch (with membuffer caching of full-texts, so server
>> I/O is not the bottleneck). This gives me an easy way to call 2272
>> times diff on this file, and measure it (with the help of some
>> instrumentation code in blame.c, see attachment). And it's
>> incidentally the actual use case I first started out wanting to
>> optimize (blame for large files with many revisions).
>
> Testing with real-world data is important, perhaps even more important
> than artificial test data, but some test data would be useful.  If you
> were to write a script to generate two test files of size 100MB, say,
> then you could use the tools/diff/diff utility to run Subversion diff on
> those two files.  Or tools/diff/diff3 if it's a 3-way diff that matters.
> The first run might involve disk IO, but on most machines the OS should
> be able to cache the files and subsequent hot-cache runs should be a
> good way to profile the diff code, assumming it is CPU limited.

Yes, that's a good idea. I'll try to spend some time on that. But I'm
wondering about a good way to write such a script.

I'd like the script to generate large files quickly, and with content
that's not totally random, but also not 100 times the exact same
line (either of those are not going to be representative for real
world data, might hit some edge behavior of the diff algorithm).
(maybe totally random is fine, but is there an easy/fast way to
generate this?)

As a first attempt, I quickly hacked up a small shell script, writing
out lines in a for loop, one by one, with a fixed string together with
the line number (index of the iteration). But that's too slow (1
lines of 70 bytes, i.e. 700Kb, is already taking 14 seconds).

Maybe I can start with 10 or 20 different lines (or generate 100 in a
for loop), and then start doubling that until I have enough (cat
file.txt >> file.txt). That will probably be faster. And it might be
"real-worldish" enough (a single source file also contains many
identical lines, e.g. all lines with a single brace etc.).

Other ideas? Maybe there is already something like this lying around?

Another question: a shell script might not be good, because not
portable (and not fast)? Should I use python for this? Maybe the
"write line by line with a line number in a for loop" would be a lot
faster in Python? I don't know a lot of python, but it might be a good
opportunity to learn some ...

Are there any examples of such "manual test scripts" in svn? So I
could have a look at the style, coding habits, ... maybe borrow some
boilerplate code?

Cheers,
-- 
Johan

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Daniel Shahaf]

Blair Zajac wrote on Wed, Dec 22, 2010 at 13:22:11 -0800:
> On 12/22/10 10:56 AM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:
>>> On 12/22/10 10:27 AM, Daniel Shahaf wrote:
 Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
> On 12/22/10 5:47 AM, Daniel Shahaf wrote:
>> bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:
>>> Author: blair
>>> Date: Wed Dec 22 05:46:45 2010
>>> New Revision: 1051763
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
>>> Log:
>>> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
>>> URL: 
>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
>>> ==
>>> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
>>> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 
>>> 05:46:45 2010
>>> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
>>>name, value, pool);
>>> }
>>>
>>> +const char *
>>> +svn_repos__post_commit_error_str(svn_error_t *err,
>>> + apr_pool_t *pool)
>>> +{
>>> +  svn_error_t *hook_err1, *hook_err2;
>>> +
>>> +  if (! err)
>>> +return "(no error)";
>>> +
>>> +  err = svn_error_purge_tracing(err);
>>>
>>
>> This modifies the provided error.  Either the doc string should warn
>> that this is being done (i.e., the provided ERR is not safe for use
>> after calling this helper), or this call should be removed and another
>> means used to locate the first non-tracing error message.
>>
>> Where do you clear ERR?  You must do so in this function, since the
>> caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
>> promise).
>
> Is this promise a defensive promise, as the original err still looks
> usable by reviewing svn_error_purge_tracing()'s implementation?  It looks
> like only one of the errors should ever be cleared and it doesn't matter
> which one.  True, it's conceptually easier to remember that one should
> only clear the error returned from svn_error_purge_tracing().
>

 I hadn't actually examined the implementation at the time I wrote that
 email, so now I'm confused.

 svn_error_purge_tracing() returns a chain which is a subset of the
 original chain.  (The subset need not start at the same place as the
 original chain, eg in the common case that the first entry in the
 original chain is a tracing link.)

 The reason for clearing errors is to free() the memory and deinstall the
 err_abort() pool cleanup callback.  Since svn_error_purge_tracing()
 doesn't free() or deinstall the callback, it would seem that one has to
 clear the ORIGINAL (passed-in) error chain, not the returned one!
>>>
>>> Even this isn't guarenteed to work, if the error at the end of the chain
>>> is a tracing link, right, since it can be removed by
>>> svn_error_purge_tracing()?
>>>
>>
>> Ah, right.  Once you call svn_error_purge_tracing(), if the chain
>> contains a tracing link at any place other than the first position, then
>> all pointers to that link would be lost.
>>
>> (But if this is indeed a bug, then how come it hasn't been noticed yet?)
>
> I think it only matters if the last error in the chain is a tracing 
> error, because the removal of err_abort() from the pool has to use the 
> exact values that were used to register err_abort(), which is the last 
> error in the chain.

Hmm, right.  I read make_error_internal() too fast and missed that.

> Since probably 100% of the time the last error in a 
> chain is not a tracing error, then err_abort() will be properly 
> deregistered by svn_error_clear().
>

Agreed.  As to the 'lost' links (those not pointed to any more),
I believe they're allocated from the same pool as every other link in
the chain, so they'll be freed when that chain is cleared.

>>> It seems the best thing is to have svn_error_purge_tracing() return a
>>> brand new chain that shares no links with the original chain, and then
>>> both need to be freed.
>>>
>>
>> That makes sense; if we'll work with complete chains, there's a smaller
>> chance of more edge-case bugs.
>
> Thinking on this a little more, since most people won't use debugging, 
> it'll be wasteful to have svn_error_purge_tracing() make a duplicate 
> chain that will have no tracing errors in it.
>

IOW, duplicating in maintainer mode forces duplicating in non-maintainer
mode too, making the common case costlier.  True.

However, we could have a macro that causes the dup() operation to only
be done in maintainer mode:

  void svn_error_purge_tracing_shell(svn_error_t **err_p)
  {
  #ifdef SVN_ERR__TRACING
svn_

Setting $HOME in the testsuite

[Hyrum K Wright]

More fallout from my issue #3765 work: I'm investing using ~/.svnrc to
provide default arguments to the commandline client.  In doing so, it
makes loads of sense not to use the a developer's ~/.svnrc when
running the tests.  Since this is found via the svn_user_get_homedir()
function, which uses the $HOME environment variable, we coud easily
redirect that to avoid this conflict.  (It would also allow testing of
this feature.)

I know we provide a standard config directory in the testsuite, but
wouldn't it also be reasonable to create a dummy home directory?
Grepping through the python test suite yield no results for HOME, so I
assume we are currently using the developer's home directory.

-Hyrum

Re: [PATCH] support printing changed properties in svnlook

[Erik Johansson]

On Fri, Dec 10, 2010 at 15:08, Daniel Shahaf  wrote:
> [ I'm somewhat confused about this issue. ]
>
> Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
>> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf  wrote:
>> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
>> >> +  /** How this property entered the node tree: 'A'dd, 'D'elete, 
>> >> 'R'eplace */
>> >> +  char action;
>
> I should have caught this earlier, but "replaced" doesn't make sense
> for properties.  (A node can be replaced by another node at the same
> path, but for a fixed node properties can be added/modified/removed but
> not replaced.)

I'll change it to 'A'dd, 'D'elete or 'U'pdate then?

>> >
>> > This is copied from svn_repos_node_t->action.  There was recently
>> > a question about that field:
>> > http://mid.gmane.org/3abd28aa-a2fc-4d7d-a502-479d37995...@orcaware.com
>> >
>> > So, that asks whether 'C'hanged is a valid answer to the question that
>> > ->action is meant to answer.  I'll also ask how this interacts with node
>> > changes: for example; if r5 replaces-with-history a node that has
>> > 'fooprop' set with another node that also has 'fooprop' set, what would
>> > the value of 'action' be?
>>
>> What about this:
>> When a node is deleted all the properties it had are listed in
>> mod_prop with action D.
>>
>> When a node is added-with-history all the properties the source had
>> are listed in mod_prop with action A and a new flag copyfrom = TRUE.
>>
> There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
> I don't think we need another one.

I was thinking that the copyfrom flag would be useful in the case
where you copy a node with a propery and add another property before
committing. Here you would have two property with action A and one
would have copyfrom = TRUE.

>> A replace-with-history will result in two repos_nodes, each having a
>> mod_prop list. If the same property exists in both it means it has
>> been replaced.
>>
> Two nodes doesn't sound like a good idea; one node marked as 'replaced'
> should suffice.

In the current api a replaced node is represented by two nodes. Should
we change this? Should svnlook stay back-wards compatible and print a
replaced node twice, once as Deleted and once as Added?

> In case of a replacement, do we want the API to provide the replaced
> node's properties? (at least their names)

It would be pretty simple if there are two separate nodes for a
replaced node. The deleted node will have all the properties it had
marked as Deleted. The new node will have its properties marked as
appropiate.

// Erik

-- 
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 1:53 PM, Hyrum K Wright wrote:

On Wed, Dec 22, 2010 at 2:03 PM, Blair Zajac  wrote:

Lots of warnings in the build now.  I remember the days when we have a
warning free build.  It would be nice to have that back, but it would be a
bunch of work.


It's because we've made the compile more strict about flagging
warnings, not because we've introduced more warning-inducing code.
But yah, fixing the warnings would be nice...


We have gotten soft about removing deprecation warnings.

Blair

Re: [PATCH] support printing changed properties in svnlook

[Erik Johansson]

On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf  wrote:
>> >> +  /** How this property entered the node tree: 'A'dd, 'D'elete, 
>> >> 'R'eplace */
>> >> +  char action;
>> >
>> > This is copied from svn_repos_node_t->action.  There was recently
>> > a question about that field:
>> > http://mid.gmane.org/3abd28aa-a2fc-4d7d-a502-479d37995...@orcaware.com
>> >
>> > So, that asks whether 'C'hanged is a valid answer to the question that
>> > ->action is meant to answer.  I'll also ask how this interacts with node
>> > changes: for example; if r5 replaces-with-history a node that has
>> > 'fooprop' set with another node that also has 'fooprop' set, what would
>> > the value of 'action' be?
>>
>> What about this:
>> When a node is deleted all the properties it had are listed in
>> mod_prop with action D.
>>
>> When a node is added-with-history all the properties the source had
>> are listed in mod_prop with action A and a new flag copyfrom = TRUE.
>>
>> A replace-with-history will result in two repos_nodes, each having a
>> mod_prop list. If the same property exists in both it means it has
>> been replaced.
>>
>
> (will reply later)
>
>> >> +  /** The name of the property */
>> >> +  const char *name;
>> >
>> > Where is the value of the property?  How to get it?
>>
>> The idea was that the struct should indicate changes to properties,
>> not their values. In the same way that svn_repos_node_t shows changes,
>> not node content.
>>
>
> Flawed analogy: we never store node content in memory, but we do have
> all property values in memory, so the cost is just to add an
> svn_string_t * member to the struct.
>
> My question was how would callers get the value if they cared about it.
> i.e., I assume that whoever calls this function already has a property
> hash (or, at least, an fs object) available that they can get the
> property values from?

I agree that it is simple to add the value to the struct, but in
svnlook (which is the only current in-tree user of this api) I can't
see any need to get the value.

How a caller would get the property value if they indeed cared about
it is outside my knowledge of the api. Perhaps svn_repos_node_editor()
is not the editor to use if you want the property value?

>> >> +  /** Pointer to the next sibling property */
>> >> +  struct svn_repos_node_prop_t *sibling;
>> >> +
>> >
>> > You use a linked list.  How about using apr_array_header_t *?  Or a hash
>> > of (prop_name -> struct)?
>>
>> I guess anyone of those would work, but the reason I went for a linked
>> list was that svn_repos_node_t did that and I wanted them to be
>> similar.
>>
>
> Hmm.  I haven't seen that struct before: svn_repos_node_t indeed uses an
> explicit tree structure.
>
> What do you think will be more useful to consumers of the API?  A hash
> allows both random access and iteration --- do APR arrays or linked
> lists have advantages that outweigh that?

A hash seemed like overkill, but if you think it is better I have no
problem using that.

// Erik

-- 
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Hyrum K Wright]

On Wed, Dec 22, 2010 at 2:03 PM, Blair Zajac  wrote:
> Lots of warnings in the build now.  I remember the days when we have a
> warning free build.  It would be nice to have that back, but it would be a
> bunch of work.

It's because we've made the compile more strict about flagging
warnings, not because we've introduced more warning-inducing code.
But yah, fixing the warnings would be nice...

-Hyrum

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 10:56 AM, Daniel Shahaf wrote:

Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:

On 12/22/10 10:27 AM, Daniel Shahaf wrote:

Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:

On 12/22/10 5:47 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:

Author: blair
Date: Wed Dec 22 05:46:45 2010
New Revision: 1051763

URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
Log:
Modified: subversion/trunk/subversion/libsvn_repos/commit.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
==
--- subversion/trunk/subversion/libsvn_repos/commit.c (original)
+++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
@@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
   name, value, pool);
}

+const char *
+svn_repos__post_commit_error_str(svn_error_t *err,
+ apr_pool_t *pool)
+{
+  svn_error_t *hook_err1, *hook_err2;
+
+  if (! err)
+return "(no error)";
+
+  err = svn_error_purge_tracing(err);



This modifies the provided error.  Either the doc string should warn
that this is being done (i.e., the provided ERR is not safe for use
after calling this helper), or this call should be removed and another
means used to locate the first non-tracing error message.

Where do you clear ERR?  You must do so in this function, since the
caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
promise).


Is this promise a defensive promise, as the original err still looks
usable by reviewing svn_error_purge_tracing()'s implementation?  It looks
like only one of the errors should ever be cleared and it doesn't matter
which one.  True, it's conceptually easier to remember that one should
only clear the error returned from svn_error_purge_tracing().



I hadn't actually examined the implementation at the time I wrote that
email, so now I'm confused.

svn_error_purge_tracing() returns a chain which is a subset of the
original chain.  (The subset need not start at the same place as the
original chain, eg in the common case that the first entry in the
original chain is a tracing link.)

The reason for clearing errors is to free() the memory and deinstall the
err_abort() pool cleanup callback.  Since svn_error_purge_tracing()
doesn't free() or deinstall the callback, it would seem that one has to
clear the ORIGINAL (passed-in) error chain, not the returned one!


Even this isn't guarenteed to work, if the error at the end of the chain
is a tracing link, right, since it can be removed by
svn_error_purge_tracing()?



Ah, right.  Once you call svn_error_purge_tracing(), if the chain
contains a tracing link at any place other than the first position, then
all pointers to that link would be lost.

(But if this is indeed a bug, then how come it hasn't been noticed yet?)


I think it only matters if the last error in the chain is a tracing error, 
because the removal of err_abort() from the pool has to use the exact values 
that were used to register err_abort(), which is the last error in the chain. 
Since probably 100% of the time the last error in a chain is not a tracing 
error, then err_abort() will be properly deregistered by svn_error_clear().



It seems the best thing is to have svn_error_purge_tracing() return a
brand new chain that shares no links with the original chain, and then
both need to be freed.



That makes sense; if we'll work with complete chains, there's a smaller
chance of more edge-case bugs.


Thinking on this a little more, since most people won't use debugging, it'll be 
wasteful to have svn_error_purge_tracing() make a duplicate chain that will have 
no tracing errors in it.


So maybe we should have svn_error_purge_tracing() be related to the input chain 
and share what it can, but not be an error chain one should use 
svn_error_clear() on.



Separately, it seems that svn_error_dup() installs a pool cleanup
callback only on one link in the duplicate chain, rather than on every
link.  (whereas make_error_internal() causes every link of a 'normal'
chain to have a cleanup attached)


make_error_internal() only installs a callback on the last error in the chain, 
that is if "child" is NULL.



For example, shouldn't the following code trigger err_abort() on the
outer (wrapper) link?

  svn_error_t *in = svn_error_create(...);
  svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
  svn_error_t *purged = svn_error_purge_tracing(original);
  svn_error_clear(purged);
  exit(0);

Am I making sense?


I don't think so, because if SVN_DEBUG is defined, then it finds the
error at the end of the chain, which in this example is a non-tracing
error, and it'll properly remove the err_abort.



You're right, I'll modify my example to thi

Re: [PATCH] Correct documentation for svn_repos_node_editor and friends

[Erik Johansson]

On Sat, Dec 18, 2010 at 20:43, Daniel Shahaf  wrote:
> Erik Johansson wrote on Tue, Dec 14, 2010 at 19:19:17 +0100:
>> [[[
>> r846201 added svn_repos_replay as a "replacement not so much for
>> svn_repos_dir_delta, but for at least a whole class of functionality that
>> svn_repos_dir_delta was never intended to handle."
>>
>> One place where svn_repos_replay replaced svn_repos_dir_delta was in
>> combination with svn_repos_node_editor (only used in svnlook). This commit
>> updates the documentation for svn_repos_node_editor and friends to reflect
>> this.
>>
>> * subversion/include/svn_repos.h
>>   (svn_repos_node_editor, svn_repos_node_from_baton): Doc update
>> ]]]
>
> The patch basically does s/svn_repos_dir_delta2/svn_repos_replay2/g.
> However, despite the log message, I still don't understand why that is
> a correct change.

According to the log message for r846201 it seems like
svn_repos_replay is faster than svn_repos_dir_delta for some
operations. I can only assume that this is one such case.

> Why are we mentioning a specific driver at all?  Can svn_repos_node_editor()
> be driven only by svn_repos_replay2()?
>
> (I don't see why that would be the case.)  If not, then its doc string
> should avoid mention that the returned editor can be driven only by %s,
> when in fact it can be driven by anything.

I don't know the anything about the other drivers, but my assumption
is that any driver works, but svn_repos_replay is the prefered one in
this case.

> We have three mechanisms: svn_repos_dir_delta2(), svn_repos_begin_report2(),
> and svn_repos_replay2().  I think it would be useful to just mention all of
> them --- after all, none of them is deprecated and all of them can represent
> a tree delta using an editor.

Mention all on equal footing or say that svn_repos_replay2 is the preferred one?

I can fix the docs, but I don't know enough about svn api to actually
determine the correct fix. E.g. do all the drivers call methods in the
editor in the same order?

// Erik

-- 
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc

Re: svn commit: r1051778 - in /subversion/trunk/subversion: libsvn_repos/commit.c libsvn_repos/load-fs-vtable.c mod_dav_svn/lock.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 5:57 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 07:10:06 -:

Author: blair
Date: Wed Dec 22 07:10:05 2010
New Revision: 1051778

URL: http://svn.apache.org/viewvc?rev=1051778&view=rev
Log:
Have all remaining calls of svn_fs_commit_txn() and
svn_repos_fs_commit_txn() use the contract that a commit was
successful if the returned revision is a valid revision number.  The
returned error, if any, is no longer used as an indication of commit
success.



+  /* If the commit failed, it's *probably* due to a conflict --
+ that is, the txn being out-of-date.  The filesystem gives us
+ the ability to continue diddling the transaction and try
+ again; but let's face it: that's not how the cvs or svn works
+ from a user interface standpoint.  Thus we don't make use of
+ this fs feature (for now, at least.)
+
+ So, in a nutshell: svn commits are an all-or-nothing deal.
+ Each commit creates a new fs txn which either succeeds or is
+ aborted completely.  No second chances;  the user simply
+ needs to update and commit again  :)
+
+ We ignore the possible error result from svn_fs_abort_txn();
+ it's more important to return the original error. */
+  svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
+  return svn_error_return(err);


Shouldn't we return a non-NULL error even if ERR is SVN_NO_ERROR here?


If the commit fails, then there should always be an error.  The documentation 
for svn_fs_txn_commit() and svn_repos_fs_txn_commit() isn't too clear on this, 
so I'll add a note.  But all the unit tests I added assert on this.


BTW, if the commit failed, then the checks that I added for SVN_NO_ERROR were 
defensive to prevent core dumps because some code was dereferencing the error.



Modified: subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c?rev=1051778&r1=1051777&r2=1051778&view=diff
==
--- subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c (original)
+++ subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c Wed Dec 22 
07:10:05 2010
@@ -840,7 +840,18 @@ close_revision(void *baton)
  }

/* Commit. */
-  if ((err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool)))
+  err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool);
+  if (SVN_IS_VALID_REVNUM(*new_rev))
+{
+  if (err)
+{
+  /* ### Log any error, but better yet is to rev
+ ### close_revision()'s API to allow both new_rev and err
+ ### to be returned, see #3768. */
+  svn_error_clear(err);
+}
+}
+  else
  {
svn_error_clear(svn_fs_abort_txn(rb->txn, rb->pool));
if (conflict_msg)


Same question, though here we probably need to revv the API or use
some callback?


Yes, this goes to the other thread about the API changes we're discussing.


Modified: subversion/trunk/subversion/mod_dav_svn/lock.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/lock.c?rev=1051778&r1=1051777&r2=1051778&view=diff
==
--- subversion/trunk/subversion/mod_dav_svn/lock.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/lock.c Wed Dec 22 07:10:05 2010


The remaining hunks do check for the case that *NEW_REV == -1
and ERR == SVN_NO_ERROR, look good.


Again, this case shouldn't ever happen, but I coded against it anyway.

Blair

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 8:43 AM, Julian Foad wrote:

On Wed, 2010-12-22, Blair Zajac wrote:

On 12/22/10 4:36 AM, Julian Foad wrote:

subversion/libsvn_repos/commit.c:648: no previous prototype for
'svn_repos__post_commit_error_str'


Thanks, fixed in r1051968.

Which compiler are you using that gives this warning?


GCC.


From the output of "make":


[...]/libtool --tag=CC --silent --mode=link gcc  -Wno-system-headers
-Wold-style-definition -Wdeclaration-after-statement -Wpointer-arith
-Wwrite-strings -Wshadow -ansi -Wall -Wformat=2 -Wunused
-Waggregate-return -Wstrict-prototypes -Wmissing-prototypes
-Wmissing-declarations -Wno-multichar -Wredundant-decls -Wnested-externs
-Wunreachable-code -Winline -Wno-long-long -g -DAPR_POOL_DEBUG -Wundef
-Wendif-labels -Wcast-qual -Wno-deprecated-declarations
-Wno-unreachable-code -Wno-format-nonliteral -pthread
-D_LARGEFILE64_SOURCE -DNE_LFS -Werror=implicit-function-declaration
-DSVN_DEBUG -DAP_DEBUG [...]

(The first group of options are inserted by Subversion's configury, and
my config/build/test script sets CFLAGS to "-g -O -DAPR_POOL_DEBUG
-Wundef -Wendif-labels -Wcast-qual -Wno-deprecated-declarations
-Wno-unreachable-code -Wno-format-nonliteral".)


Thanks, I've added these to my script.

Lots of warnings in the build now.  I remember the days when we have a warning 
free build.  It would be nice to have that back, but it would be a bunch of work.


Blair

Re: Version 1.6.15 missing from archive.apache.org

[Hyrum K. Wright]

1.6.15 should be on archive.apache.org (or will when the mirrors
propagate), along with the recently-released 1.5.9.

Cheers,
-Hyrum

On Fri, Dec 17, 2010 at 4:04 AM, Rainer Jung  wrote:
> Hi Hyrum,
>
> On 17.12.2010 03:39, Hyrum K. Wright wrote:
>>
>> Rainer,
>> Thanks for the poke.  I need to upload the last two releases (1.6.13
>> and 1.6.15) to archive.apache.org.  I plan on doing that on Monday.
>
> Great. 1.6.13 is already there (owner "hwright" ;) ), only 1.6.15 is
> missing.
>
> Regards,
>
> Rainer
>
>> On Thu, Dec 16, 2010 at 6:25 PM, Rainer Jung
>>  wrote:
>>>
>>> Hi Subversion Devs,
>>>
>>> the recent version 1.6.15 is missing from the archives at
>>> archive.apache.org. Although 1.6 releases are still published via tigris,
>>> all other 1.6 releases can also be found at
>>> http:/archive.apache.org/dist/subversion, so I think 1.6.15 should also
>>> be
>>> copied there.
>

Re: svn commit: r1051978 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 10:39 AM, Philip Martin wrote:

bl...@apache.org writes:


Author: blair
Date: Wed Dec 22 16:46:40 2010
New Revision: 1051978



URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/version.c?rev=1051978&r1=1051977&r2=1051978&view=diff
==
--- subversion/trunk/subversion/mod_dav_svn/version.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/version.c Wed Dec 22 16:46:40 2010
@@ -922,15 +922,15 @@ dav_svn__checkin(dav_resource *resource,
  {
if (serr)
  {
+  apr_status_t apr_err = serr->apr_err;


../src/subversion/mod_dav_svn/version.c: In function ‘dav_svn__checkin’:
../src/subversion/mod_dav_svn/version.c:925: warning: declaration of ‘apr_err’ 
shadows a previous local
../src/subversion/mod_dav_svn/version.c:852: warning: shadowed declaration is 
here


Thanks, fixed in r1052041.

I've switched over to using --enable-maintainer-mode.

Blair

Subversion 1.5.9 Released

[Hyrum Wright]

I'm happy to announce the release of Subversion 1.5.9.  This release
is intended for users who are still running the 1.5 release line, and
contains a number of stability and performance fixes to previous 1.5.x
releases.  This release does *not* contain all the bug fixes and
features which are found in the most current release of Subversion,
1.6.15.  Subversion 1.5.9 is available from:

http://subversion.tigris.org/downloads/subversion-1.5.9.tar.bz2
http://subversion.tigris.org/downloads/subversion-1.5.9.tar.gz
http://subversion.tigris.org/downloads/subversion-1.5.9.zip
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.tar.bz2
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.tar.gz
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.zip

The MD5 checksums are:

d8de4f33decb9e608c8cfd43288ebe89  subversion-1.5.9.tar.bz2
973e87cd8aa64f44ed6b4e569c635abc  subversion-1.5.9.tar.gz
77c879a1b78e26a521618659ec4798c4  subversion-1.5.9.zip
50b4559ad6cef3caa810b80f1c18679c  subversion-deps-1.5.9.tar.bz2
1154485516cfb59db0f009b2d1a5c0cc  subversion-deps-1.5.9.tar.gz
5c75a3209507ec9e2f4edce73003fb85  subversion-deps-1.5.9.zip

The SHA1 checksums are:

f2299e41be933cf0376a4c32e8c2807b9f3e709e  subversion-1.5.9.tar.bz2
d755f97f82035add644495309c15d7ab9711c019  subversion-1.5.9.tar.gz
874235a6b79bfdb9a83fcb6f2066da7b1d9c0e4a  subversion-1.5.9.zip
9a89bba0da49d73949d5182375901abb80408429  subversion-deps-1.5.9.tar.bz2
2948f4611c43a2f7c070da4ad868e02d36d111e1  subversion-deps-1.5.9.tar.gz
fe8b20f2d94fc6d7f0892e334e9007b22a10cf22  subversion-deps-1.5.9.zip

PGP Signatures are available at:

http://subversion.tigris.org/downloads/subversion-1.5.9.tar.bz2.asc
http://subversion.tigris.org/downloads/subversion-1.5.9.tar.gz.asc
http://subversion.tigris.org/downloads/subversion-1.5.9.zip.asc
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.tar.bz2.asc
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.tar.gz.asc
http://subversion.tigris.org/downloads/subversion-deps-1.5.9.zip.asc

For this release, the following people have provided PGP signatures:

   C. Michael Pilato [1024D/1706FD6E] with fingerprint:
20BF 14DC F02F 2730 7EA4  C7BB A241 06A9 1706 FD6E
   Paul T. Burba [1024D/53FCDC55] with fingerprint:
E630 CF54 792C F913 B13C  32C5 D916 8930 53FC DC55
   Julian Foad [1024D/353E25BC] with fingerprint:
6604 5A4B 43BC F994   5728 351F 33E4 353E 25BC
   Bert Huijben [1024D/9821F7B2] with fingerprint:
2017 F51A 2572 0E78 8827  5329 FCFD 6305 9821 F7B2
   Hyrum K. Wright [1024D/4E24517C] with fingerprint:
3324 80DA 0F8C A37D AEE6  D084 0B03 AE6E 4E24 517C
   Philip Martin [2048R/ED1A599C] with fingerprint:
A844 790F B574 3606 EE95  9207 76D7 88E1 ED1A 599C
   Mark Phippard [1024D/035A96A9] with fingerprint:
D315 89DB E1C1 E9BA D218  39FD 265D F8A0 035A 96A9

Release notes for the 1.5.x release series may be found at:

http://subversion.apache.org/docs/release-notes/1.5.html

You can find the list of changes between 1.5.9 and earlier versions at:

http://svn.apache.org/repos/asf/subversion/tags/1.5.9/CHANGES

Questions, comments, and bug reports to us...@subversion.apache.org.

Thanks,
- The Subversion Team

Re: svn commit: r1051988 - /subversion/trunk/subversion/libsvn_repos/commit.c

[Blair Zajac]

On 12/22/10 9:09 AM, Blair Zajac wrote:

On 12/22/10 9:01 AM, bl...@apache.org wrote:

Author: blair
Date: Wed Dec 22 17:01:20 2010
New Revision: 1051988

URL: http://svn.apache.org/viewvc?rev=1051988&view=rev
Log:
Improve error messages from svn_repos__post_commit_error_str(). Also,
improve docs.


I see the unit test failures from this commit, working on them now.


Fixed in r1052029.

Blair

Re: svn commit: r1051988 - /subversion/trunk/subversion/libsvn_repos/commit.c

[Blair Zajac]

On 12/22/10 10:30 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 17:01:21 -:

Author: blair
Date: Wed Dec 22 17:01:20 2010
New Revision: 1051988

URL: http://svn.apache.org/viewvc?rev=1051988&view=rev
Log:
Improve error messages from svn_repos__post_commit_error_str().  Also,
improve docs.

Found by: danielsh

* subversion/libsvn_repos/commit.c
   (svn_repos__post_commit_error_str):
 Since this skips over SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED,
 include in all messages where the message is coming from,
 post-commit processing or post-commit hook.

Modified:
 subversion/trunk/subversion/libsvn_repos/commit.c

Modified: subversion/trunk/subversion/libsvn_repos/commit.c
hook_err1 = svn_error_find_cause(err, 
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED);
if (hook_err1&&  hook_err1->child)
  hook_err2 = hook_err1->child;
@@ -672,15 +676,15 @@ svn_repos__post_commit_error_str(svn_err
  {
if (err == hook_err1)
  {
-  if (hook_err2->message)
-msg = apr_pstrdup(pool, hook_err2->message);
-  else
-msg = "(no error message)";
+  msg = apr_psprintf(pool,
+ "Post-commit hook had error '%s'.",
+ hook_err2->message ? hook_err2->message
+: "(no error message)");
  }
else
  {
msg = apr_psprintf(pool,
- "Post commit processing had error and '%s' "
+ "Post commit processing had error '%s' and "
   "post-commit hook had error '%s'.",


These messages should be marked for translation?


Did those in r1052029.

Blair

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Daniel Shahaf]

Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:
> On 12/22/10 10:27 AM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
>>> On 12/22/10 5:47 AM, Daniel Shahaf wrote:
 bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:
> Author: blair
> Date: Wed Dec 22 05:46:45 2010
> New Revision: 1051763
>
> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
> Log:
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 
> 2010
> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
>   name, value, pool);
>}
>
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool)
> +{
> +  svn_error_t *hook_err1, *hook_err2;
> +
> +  if (! err)
> +return "(no error)";
> +
> +  err = svn_error_purge_tracing(err);
>

 This modifies the provided error.  Either the doc string should warn
 that this is being done (i.e., the provided ERR is not safe for use
 after calling this helper), or this call should be removed and another
 means used to locate the first non-tracing error message.

 Where do you clear ERR?  You must do so in this function, since the
 caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
 promise).
>>>
>>> Is this promise a defensive promise, as the original err still looks
>>> usable by reviewing svn_error_purge_tracing()'s implementation?  It looks
>>> like only one of the errors should ever be cleared and it doesn't matter
>>> which one.  True, it's conceptually easier to remember that one should
>>> only clear the error returned from svn_error_purge_tracing().
>>>
>>
>> I hadn't actually examined the implementation at the time I wrote that
>> email, so now I'm confused.
>>
>> svn_error_purge_tracing() returns a chain which is a subset of the
>> original chain.  (The subset need not start at the same place as the
>> original chain, eg in the common case that the first entry in the
>> original chain is a tracing link.)
>>
>> The reason for clearing errors is to free() the memory and deinstall the
>> err_abort() pool cleanup callback.  Since svn_error_purge_tracing()
>> doesn't free() or deinstall the callback, it would seem that one has to
>> clear the ORIGINAL (passed-in) error chain, not the returned one!
>
> Even this isn't guarenteed to work, if the error at the end of the chain 
> is a tracing link, right, since it can be removed by 
> svn_error_purge_tracing()?
>

Ah, right.  Once you call svn_error_purge_tracing(), if the chain
contains a tracing link at any place other than the first position, then
all pointers to that link would be lost.

(But if this is indeed a bug, then how come it hasn't been noticed yet?)

> It seems the best thing is to have svn_error_purge_tracing() return a 
> brand new chain that shares no links with the original chain, and then 
> both need to be freed.
>

That makes sense; if we'll work with complete chains, there's a smaller
chance of more edge-case bugs.

Separately, it seems that svn_error_dup() installs a pool cleanup
callback only on one link in the duplicate chain, rather than on every
link.  (whereas make_error_internal() causes every link of a 'normal'
chain to have a cleanup attached)

>> For example, shouldn't the following code trigger err_abort() on the
>> outer (wrapper) link?
>>
>>  svn_error_t *in = svn_error_create(...);
>>  svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
>>  svn_error_t *purged = svn_error_purge_tracing(original);
>>  svn_error_clear(purged);
>>  exit(0);
>>
>> Am I making sense?
>
> I don't think so, because if SVN_DEBUG is defined, then it finds the 
> error at the end of the chain, which in this example is a non-tracing 
> error, and it'll properly remove the err_abort.
>

You're right, I'll modify my example to this:

SVN_ERR__TRACED  /* start of chain */
SVN_ERR__TRACED
"non-tracing link" /* end of chain */

In this case, the middle link would be lost.  Right?

> void
> svn_error_clear(svn_error_t *err)
> {
>   if (err)
> {
> #if defined(SVN_DEBUG)
>   while (err->child)
> err = err->child;
>   apr_pool_cleanup_kill(err->pool, err, err_abort);
> #endif
>   svn_pool_destroy(err->pool);
> }
> }
>
> Blair

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 5:47 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:

Author: blair
Date: Wed Dec 22 05:46:45 2010
New Revision: 1051763

URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
Log:
Add a private function that takes the error returned from
svn_repos_fs_commit_txn() and builds a error message string containing
either or both of the svn_fs_commit_txn() error and the
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped post-commit error.  The
function skips over tracing errors.



+  "post-commit hook had error '%s'.",
+  err->message ? err->message
+   : "(no error message)",
+  hook_err2->message ? hook_err2->message
+ : "(no error message)");
+}
+}
+  else
+{
+  if (err->message)
+return apr_pstrdup(pool, err->message);


In this case, and in the apr_pstrdup() above, might it be useful to say
explicitly whether the given error is from the post-commit hook or from
some FS post-commit fiddling?  You've skipped over the
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED link, so the "post-commit hook
failed" message that used to be there would be gone.


Turns out we don't need to say that because the wrapped error message is already 
pretty self-describing.


Further fixed in r1052029.

Blair

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 10:27 AM, Daniel Shahaf wrote:

Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:

On 12/22/10 5:47 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:

Author: blair
Date: Wed Dec 22 05:46:45 2010
New Revision: 1051763

URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
Log:
Modified: subversion/trunk/subversion/libsvn_repos/commit.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
==
--- subversion/trunk/subversion/libsvn_repos/commit.c (original)
+++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
@@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
  name, value, pool);
   }

+const char *
+svn_repos__post_commit_error_str(svn_error_t *err,
+ apr_pool_t *pool)
+{
+  svn_error_t *hook_err1, *hook_err2;
+
+  if (! err)
+return "(no error)";
+
+  err = svn_error_purge_tracing(err);



This modifies the provided error.  Either the doc string should warn
that this is being done (i.e., the provided ERR is not safe for use
after calling this helper), or this call should be removed and another
means used to locate the first non-tracing error message.

Where do you clear ERR?  You must do so in this function, since the
caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
promise).


Is this promise a defensive promise, as the original err still looks
usable by reviewing svn_error_purge_tracing()'s implementation?  It looks
like only one of the errors should ever be cleared and it doesn't matter
which one.  True, it's conceptually easier to remember that one should
only clear the error returned from svn_error_purge_tracing().



I hadn't actually examined the implementation at the time I wrote that
email, so now I'm confused.

svn_error_purge_tracing() returns a chain which is a subset of the
original chain.  (The subset need not start at the same place as the
original chain, eg in the common case that the first entry in the
original chain is a tracing link.)

The reason for clearing errors is to free() the memory and deinstall the
err_abort() pool cleanup callback.  Since svn_error_purge_tracing()
doesn't free() or deinstall the callback, it would seem that one has to
clear the ORIGINAL (passed-in) error chain, not the returned one!


Even this isn't guarenteed to work, if the error at the end of the chain is a 
tracing link, right, since it can be removed by svn_error_purge_tracing()?


It seems the best thing is to have svn_error_purge_tracing() return a brand new 
chain that shares no links with the original chain, and then both need to be freed.



For example, shouldn't the following code trigger err_abort() on the
outer (wrapper) link?

 svn_error_t *in = svn_error_create(...);
 svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
 svn_error_t *purged = svn_error_purge_tracing(original);
 svn_error_clear(purged);
 exit(0);

Am I making sense?


I don't think so, because if SVN_DEBUG is defined, then it finds the error at 
the end of the chain, which in this example is a non-tracing error, and it'll 
properly remove the err_abort.


void
svn_error_clear(svn_error_t *err)
{
  if (err)
{
#if defined(SVN_DEBUG)
  while (err->child)
err = err->child;
  apr_pool_cleanup_kill(err->pool, err, err_abort);
#endif
  svn_pool_destroy(err->pool);
}
}

Blair

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

[Daniel Shahaf]

Your quoting is broken, please send MIME=text/plain replies so we don't
have to think where my text ends and yours starts, thanks.

Tijn Porcelijn wrote on Wed, Dec 22, 2010 at 07:37:50 -0800:
> On 2010-12-22 14:02, Daniel Shahaf wrote:
> > 
> > Forwarding back to the list.
> > 
> > Tijn Porcelijn wrote on Wed, Dec 22, 2010 at 01:54:07 -0800:
> > > Hi Daniel,
> > >
> > > I guess you're right. It makes more sense to echo the external URL
> > > than the local directory in the "Updated external
> > > 'url://remote/path/to/somewhere' to revision %ld". Anyway, my
> > > suggestion was to make sure that "some" reference to the specific url
> > > was made *at all* after removing the other notice (as opposed to just
> > > printing a revision number and having to guess the repository it
> > > refers to).
> > 
> > I'm still not sure I understand the issue, or why you think adding that
> > would be useful.  (Not saying that it isn't useful; just that I don't
> > understand why it would be.)
> > 
> > Before you invest more time in coding, could you please give more
> > concrete examples of how the current output is not satisfactory?
> > 
> > e.g., are you trying to parse it with a script?  Or is it just that the
> > information you want has scrolled offscreen and you want to repeat it
> > nearer to the end of the output?
> 
> My personal itch is primarily that "svn up" is too verbose when using
> externals. This shows especially when there are no changes whatsoever.
> 
> For example, when I do update a normal directory and there are no
> changes (ie.: my BASE equals HEAD) I get almost no feedback: one line
> saying: "At revision 123". Now if I do the same for a directory that
> includes one svn:externals reference, I get:
> 
>  1.  <>
>  2.  Fetching external item into 'path/to/local/dir'
>  3.  External at revision 12345.
>  4.  <>
> 
> For the project I'm currently working on (using 10+ externals) this
> easily fills up my screen.  In an (admittedly half-hearted) attempt
> I removed line 2 when doing svn up (but not for export and checkout).
> This was submitted in a different mail to this mailing list, using the
> same issue number.  Then, in below patch, I added the
> 'path/to/local/dir' to line 3, in an attempt to explain to the
> end-user what files the revision number is referring to. So, for every
> svn:externals entry I now get one line with the same output:
> 
>  1.  External 'path/to/local/dir' at revision 12345.
> 

I see.  However, I'm not sure we should make this change, since I find
the existing output format reasonable (though possibly not ideal).

In other words, I'd like to see more opinions on whether we should be
making this change.  (Making the change is probably just a SMOP in
notify.c...)

> I hope this helps explain the fix.
> 
> Tijn
> 
> 
> > Thanks,
> > 
> > Daniel
> > 
> > > I will look into printing the remote external's path.
> > >

Thanks!

> > > Thanks, tijn
> > >
> > > On 2010-12-22 02:54, Daniel Shahaf wrote:
> > >
> > > This has a bug, when updating a file external it displays the
> > > external's directory rather than the external itself.  (But maybe this
> > > is a bug in the way the library generates the notifications?)
> > >
> > > May I ask what is the motivation for this change?  The normal
> > > notifications (U   path/to/somewhere) will always immediately precede
> > > the "Updated external 'path/to/somewhere' to revision %ld", so
> > > repeating the external's path there seems a bit redundant.
> > >
> > > I haven't run 'make check'.
> > >
> > > Daniel
> > >
> > >
> > > Tijn Porcelijn wrote on Fri, Nov 26, 2010 at 01:56:22 -0800:
> > > > [[[
> > > > Improves interaction, issue #3653: svn update should not output 
> > > > svn:external
> > > > * subversion/svn/notify.c (notify)
> > > >   Add  to Externals messages
> > > >   Note: po files should also be updated
> > > > ]]]
> > > >
> > > >
> > > >
> > > >
> > > > Hi,
> > > >
> > > > Here's a small patch for making svn:externals messages a bit more 
> > > > informative. With the "Fetching external item into ''" 
> > > > -message removed, interpretation of svn_wc_notify_update_completed 
> > > > messages becomes a bit less obvious. You'll see stuff like:
> > > > External at revision 20
> > > > External at revision 2321
> > > > External at revision 1082367
> > > > At revision 19
> > > > The patch improves this to read:
> > > > External 'third-party' at revision 20
> > > > External 'snapshots' at revision 2321
> > > > External 'legacy' at revision 1082367
> > > > At revision 19
> > > > See attached notify.c.patch, Thanks,
> > > >
> > > > tijn
> > > >
> > >
> > > Content-Description: notify.c.patch
> > > > Index: subversion/svn/notify.c
> > > > ===
> > > > --- subversion/svn/notify.c   (revision 1038983)
> > > > +++ subversion/svn/notify.c   (working copy)
> > > > @@ -567,44 +567,66 @@
> > > >{
> > > >  if (nb->is_export)
> > > >

Re: svn commit: r1051978 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Philip Martin]

bl...@apache.org writes:

> Author: blair
> Date: Wed Dec 22 16:46:40 2010
> New Revision: 1051978

> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/version.c?rev=1051978&r1=1051977&r2=1051978&view=diff
> ==
> --- subversion/trunk/subversion/mod_dav_svn/version.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/version.c Wed Dec 22 16:46:40 2010
> @@ -922,15 +922,15 @@ dav_svn__checkin(dav_resource *resource,
>  {
>if (serr)
>  {
> +  apr_status_t apr_err = serr->apr_err;

../src/subversion/mod_dav_svn/version.c: In function ‘dav_svn__checkin’:
../src/subversion/mod_dav_svn/version.c:925: warning: declaration of ‘apr_err’ 
shadows a previous local
../src/subversion/mod_dav_svn/version.c:852: warning: shadowed declaration is 
here

>const char *post_commit_err = svn_repos__post_commit_error_str
>(serr, resource->pool);
> -  ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> -resource->pool,
> +  serr = SVN_NO_ERROR;
> +  ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
>  "commit of r%ld succeeded, but an error occurred 
> "
>  "after the commit: '%s'",
>  new_rev,
>  post_commit_err);
> -  svn_error_clear(serr);
>  }
>  }

-- 
Philip

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Daniel Shahaf]

Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
> Should we have an official term for FS fiddling,

+1.

> or call it post-commit  processing?  So we'll have to distinguish
> between "hook" and "processing".
>

"Post-commit FS processing" seems fine to me...

> Blair

Re: svn commit: r1051988 - /subversion/trunk/subversion/libsvn_repos/commit.c

[Daniel Shahaf]

bl...@apache.org wrote on Wed, Dec 22, 2010 at 17:01:21 -:
> Author: blair
> Date: Wed Dec 22 17:01:20 2010
> New Revision: 1051988
> 
> URL: http://svn.apache.org/viewvc?rev=1051988&view=rev
> Log:
> Improve error messages from svn_repos__post_commit_error_str().  Also,
> improve docs.
> 
> Found by: danielsh
> 
> * subversion/libsvn_repos/commit.c
>   (svn_repos__post_commit_error_str):
> Since this skips over SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED,
> include in all messages where the message is coming from,
> post-commit processing or post-commit hook.
> 
> Modified:
> subversion/trunk/subversion/libsvn_repos/commit.c
> 
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051988&r1=1051987&r2=1051988&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 17:01:20 2010
> @@ -657,6 +657,10 @@ svn_repos__post_commit_error_str(svn_err
>  
>err = svn_error_purge_tracing(err);
>  
> +  /* hook_err1 is the SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped
> + error from the post-commit script, if any, and hook_err2 should
> + be the original error, but be defensive and handle a case where
> + SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED doesn't wrap an error. */

Thanks.

>hook_err1 = svn_error_find_cause(err, 
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED);
>if (hook_err1 && hook_err1->child)
>  hook_err2 = hook_err1->child;
> @@ -672,15 +676,15 @@ svn_repos__post_commit_error_str(svn_err
>  {
>if (err == hook_err1)
>  {
> -  if (hook_err2->message)
> -msg = apr_pstrdup(pool, hook_err2->message);
> -  else
> -msg = "(no error message)";
> +  msg = apr_psprintf(pool,
> + "Post-commit hook had error '%s'.",
> + hook_err2->message ? hook_err2->message
> +: "(no error message)");
>  }
>else
>  {
>msg = apr_psprintf(pool,
> - "Post commit processing had error and '%s' "
> + "Post commit processing had error '%s' and "
>   "post-commit hook had error '%s'.",

These messages should be marked for translation?

>   err->message ? err->message
>: "(no error message)",
> @@ -690,10 +694,10 @@ svn_repos__post_commit_error_str(svn_err
>  }
>else
>  {
> -  if (err->message)
> -msg = apr_pstrdup(pool, err->message);
> -  else
> -msg = "(no error message)";
> +  msg = apr_psprintf(pool,
> + "Post-commit processing had error '%s'.",
> + hook_err2->message ? hook_err2->message
> +: "(no error message)");
>  }
>  
>/* Because svn_error_purge_tracing() was used on the input error,
> 
> 

svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Daniel Shahaf]

Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
> On 12/22/10 5:47 AM, Daniel Shahaf wrote:
>> bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:
>>> Author: blair
>>> Date: Wed Dec 22 05:46:45 2010
>>> New Revision: 1051763
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
>>> Log:
>>> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
>>> URL: 
>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
>>> ==
>>> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
>>> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 
>>> 2010
>>> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
>>>  name, value, pool);
>>>   }
>>>
>>> +const char *
>>> +svn_repos__post_commit_error_str(svn_error_t *err,
>>> + apr_pool_t *pool)
>>> +{
>>> +  svn_error_t *hook_err1, *hook_err2;
>>> +
>>> +  if (! err)
>>> +return "(no error)";
>>> +
>>> +  err = svn_error_purge_tracing(err);
>>>
>>
>> This modifies the provided error.  Either the doc string should warn
>> that this is being done (i.e., the provided ERR is not safe for use
>> after calling this helper), or this call should be removed and another
>> means used to locate the first non-tracing error message.
>>
>> Where do you clear ERR?  You must do so in this function, since the
>> caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
>> promise).
>
> Is this promise a defensive promise, as the original err still looks 
> usable by reviewing svn_error_purge_tracing()'s implementation?  It looks 
> like only one of the errors should ever be cleared and it doesn't matter 
> which one.  True, it's conceptually easier to remember that one should 
> only clear the error returned from svn_error_purge_tracing().
>

I hadn't actually examined the implementation at the time I wrote that
email, so now I'm confused.

svn_error_purge_tracing() returns a chain which is a subset of the
original chain.  (The subset need not start at the same place as the
original chain, eg in the common case that the first entry in the
original chain is a tracing link.)

The reason for clearing errors is to free() the memory and deinstall the
err_abort() pool cleanup callback.  Since svn_error_purge_tracing()
doesn't free() or deinstall the callback, it would seem that one has to
clear the ORIGINAL (passed-in) error chain, not the returned one!

For example, shouldn't the following code trigger err_abort() on the
outer (wrapper) link?

svn_error_t *in = svn_error_create(...);
svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
svn_error_t *purged = svn_error_purge_tracing(original);
svn_error_clear(purged);
exit(0);

Am I making sense?

> This function is meant to consume the error, so I've updated the docs to 
> say that it shouldn't be used after it and clear it.
>
> r1051978.

Re: svn commit: r1051988 - /subversion/trunk/subversion/libsvn_repos/commit.c

[Blair Zajac]

On 12/22/10 9:01 AM, bl...@apache.org wrote:

Author: blair
Date: Wed Dec 22 17:01:20 2010
New Revision: 1051988

URL: http://svn.apache.org/viewvc?rev=1051988&view=rev
Log:
Improve error messages from svn_repos__post_commit_error_str().  Also,
improve docs.


I see the unit test failures from this commit, working on them now.

Blair

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 5:47 AM, Daniel Shahaf wrote:

bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:

Author: blair
Date: Wed Dec 22 05:46:45 2010
New Revision: 1051763

URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
Log:
Modified: subversion/trunk/subversion/libsvn_repos/commit.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
==
--- subversion/trunk/subversion/libsvn_repos/commit.c (original)
+++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
@@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
 name, value, pool);
  }

+const char *
+svn_repos__post_commit_error_str(svn_error_t *err,
+ apr_pool_t *pool)
+{
+  svn_error_t *hook_err1, *hook_err2;
+
+  if (! err)
+return "(no error)";
+
+  err = svn_error_purge_tracing(err);



This modifies the provided error.  Either the doc string should warn
that this is being done (i.e., the provided ERR is not safe for use
after calling this helper), or this call should be removed and another
means used to locate the first non-tracing error message.

Where do you clear ERR?  You must do so in this function, since the
caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
promise).


Is this promise a defensive promise, as the original err still looks usable by 
reviewing svn_error_purge_tracing()'s implementation?  It looks like only one of 
the errors should ever be cleared and it doesn't matter which one.  True, it's 
conceptually easier to remember that one should only clear the error returned 
from svn_error_purge_tracing().


This function is meant to consume the error, so I've updated the docs to say 
that it shouldn't be used after it and clear it.


r1051978.


+  hook_err1 = svn_error_find_cause(err, SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED);
+  if (hook_err1&&  hook_err1->child)
+hook_err2 = hook_err1->child;
+  else
+hook_err2 = hook_err1;
+


So, ERR is the svn_fs_commit_txn() error (which has the hook error
somewhere down the chain), and HOOK_ERR1 is the post-commit hook error?


Yes, and HOOK_ERR2 is the un-wrapped error from the hook script.


+  /* This implementation counts on svn_repos_fs_commit_txn() returning
+ svn_fs_commit_txn() as the parent error with a child
+ SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error.  If the parent error
+ is SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED then there was no error
+ in svn_fs_commit_txn(). */
+  if (hook_err1)
+{
+  if (err == hook_err1)
+{
+  if (hook_err2->message)
+return apr_pstrdup(pool, hook_err2->message);
+  else
+return "(no error message)";
+}
+  else
+{
+  return apr_psprintf(pool,
+  "Post commit processing had error and '%s' "


s/and '%s'/'%s' and/


+  "post-commit hook had error '%s'.",
+  err->message ? err->message
+   : "(no error message)",
+  hook_err2->message ? hook_err2->message
+ : "(no error message)");
+}
+}
+  else
+{
+  if (err->message)
+return apr_pstrdup(pool, err->message);


In this case, and in the apr_pstrdup() above, might it be useful to say
explicitly whether the given error is from the post-commit hook or from
some FS post-commit fiddling?  You've skipped over the
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED link, so the "post-commit hook
failed" message that used to be there would be gone.


Fixes in r1051988.

Should we have an official term for FS fiddling, or call it post-commit 
processing?  So we'll have to distinguish between "hook" and "processing".


Blair

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Julian Foad]

On Wed, 2010-12-22, Blair Zajac wrote:
> On 12/22/10 4:36 AM, Julian Foad wrote:
> > subversion/libsvn_repos/commit.c:648: no previous prototype for
> > 'svn_repos__post_commit_error_str'
> 
> Thanks, fixed in r1051968.
> 
> Which compiler are you using that gives this warning?

GCC.

>From the output of "make":

[...]/libtool --tag=CC --silent --mode=link gcc  -Wno-system-headers
-Wold-style-definition -Wdeclaration-after-statement -Wpointer-arith
-Wwrite-strings -Wshadow -ansi -Wall -Wformat=2 -Wunused
-Waggregate-return -Wstrict-prototypes -Wmissing-prototypes
-Wmissing-declarations -Wno-multichar -Wredundant-decls -Wnested-externs
-Wunreachable-code -Winline -Wno-long-long -g -DAPR_POOL_DEBUG -Wundef
-Wendif-labels -Wcast-qual -Wno-deprecated-declarations
-Wno-unreachable-code -Wno-format-nonliteral -pthread
-D_LARGEFILE64_SOURCE -DNE_LFS -Werror=implicit-function-declaration
-DSVN_DEBUG -DAP_DEBUG [...]

(The first group of options are inserted by Subversion's configury, and
my config/build/test script sets CFLAGS to "-g -O -DAPR_POOL_DEBUG
-Wundef -Wendif-labels -Wcast-qual -Wno-deprecated-declarations
-Wno-unreachable-code -Wno-format-nonliteral".)

- Julian

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Blair Zajac]

On 12/22/10 4:36 AM, Julian Foad wrote:

subversion/libsvn_repos/commit.c:648: no previous prototype for
'svn_repos__post_commit_error_str'


Thanks, fixed in r1051968.

Which compiler are you using that gives this warning?

Blair

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

[Tijn Porcelijn]

On 2010-12-22 14:02, Daniel Shahaf wrote:

Forwarding back to the list.

Tijn Porcelijn wrote on Wed, Dec 22, 2010 at 01:54:07 -0800:
> Hi Daniel,
>
> I guess you're right. It makes more sense to echo the external URL
> than the local directory in the "Updated external
> 'url://remote/path/to/somewhere' to revision %ld". Anyway, my
> suggestion was to make sure that "some" reference to the specific url
> was made *at all* after removing the other notice (as opposed to just
> printing a revision number and having to guess the repository it
> refers to).

I'm still not sure I understand the issue, or why you think adding that
would be useful.  (Not saying that it isn't useful; just that I don't
understand why it would be.)

Before you invest more time in coding, could you please give more
concrete examples of how the current output is not satisfactory?

e.g., are you trying to parse it with a script?  Or is it just that the
information you want has scrolled offscreen and you want to repeat it
nearer to the end of the output?

My personal itch is primarily that "svn up" is too verbose when using 
externals. This shows especially when there are no changes whatsoever.

For example, when I do update a normal directory and there are no changes (ie.: 
my BASE equals HEAD) I get almost no feedback: one line saying: "At revision 
123". Now if I do the same for a directory that includes one svn:externals 
reference, I get:

 1.  <>
 2.  Fetching external item into 'path/to/local/dir'
 3.  External at revision 12345.
 4.  <>

For the project I'm currently working on (using 10+ externals) this easily 
fills up my screen.
In an (admittedly half-hearted) attempt I removed line 2 when doing svn up (but 
not for export and checkout). This was submitted in a different mail to this 
mailing list, using the same issue number.
Then, in below patch, I added the 'path/to/local/dir' to line 3, in an attempt 
to explain to the end-user what files the revision number is referring to. So, 
for every svn:externals entry I now get one line with the same output:

 1.  External 'path/to/local/dir' at revision 12345.

I hope this helps explain the fix.

Tijn


Thanks,

Daniel

> I will look into printing the remote external's path.
>
> Thanks, tijn
>
> On 2010-12-22 02:54, Daniel Shahaf wrote:
>
> This has a bug, when updating a file external it displays the
> external's directory rather than the external itself.  (But maybe this
> is a bug in the way the library generates the notifications?)
>
> May I ask what is the motivation for this change?  The normal
> notifications (U   path/to/somewhere) will always immediately precede
> the "Updated external 'path/to/somewhere' to revision %ld", so
> repeating the external's path there seems a bit redundant.
>
> I haven't run 'make check'.
>
> Daniel
>
>
> Tijn Porcelijn wrote on Fri, Nov 26, 2010 at 01:56:22 -0800:
> > [[[
> > Improves interaction, issue #3653: svn update should not output svn:external
> > * subversion/svn/notify.c (notify)
> >   Add  to Externals messages
> >   Note: po files should also be updated
> > ]]]
> >
> >
> >
> >
> > Hi,
> >
> > Here's a small patch for making svn:externals messages a bit more 
> > informative. With the "Fetching external item into ''" -message 
> > removed, interpretation of svn_wc_notify_update_completed messages becomes 
> > a bit less obvious. You'll see stuff like:
> > External at revision 20
> > External at revision 2321
> > External at revision 1082367
> > At revision 19
> > The patch improves this to read:
> > External 'third-party' at revision 20
> > External 'snapshots' at revision 2321
> > External 'legacy' at revision 1082367
> > At revision 19
> > See attached notify.c.patch, Thanks,
> >
> > tijn
> >
>
> Content-Description: notify.c.patch
> > Index: subversion/svn/notify.c
> > ===
> > --- subversion/svn/notify.c   (revision 1038983)
> > +++ subversion/svn/notify.c   (working copy)
> > @@ -567,44 +567,66 @@
> >{
> >  if (nb->is_export)
> >{
> > -if ((err = svn_cmdline_printf
> > - (pool, nb->in_external
> > -  ? _("Exported external at revision %ld.\n")
> > -  : _("Exported revision %ld.\n"),
> > -  n->revision)))
> > -  goto print_error;
> > +if (nb->in_external)
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Exported external '%s' at revision %ld.\n"),
> > +  path_local,
> > +  n->revision);
> > +else
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Exported revision %ld.\n"),
> > +  n->revision);
> >}
> >  

Re: Windows svnsync test suite failures and a clue

[C. Michael Pilato]

> Problem solved, finally!

Hooray!  Huzzah!  ${LC_HEARTY_EXPRESSION_OF_APPROVAL}!

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



signature.asc
Description: OpenPGP digital signature

Re: [RFC] Include the svn version number in the "Working copy is too old" error message

[Julian Foad]

Hearing no dissent, I committed a version of this patch in r1051945.

A typical message is now:

  svn: Please see the 'svn upgrade' command
  svn: Working copy '/home/julianfoad/tmp/svn/walk/wc' is too old
(format 10, created by Subversion 1.6)

- Julian


On Fri, 2010-12-17 at 17:20 +, Julian Foad wrote:
> Currently (at r1050442), running a 1.7-dev svn on a 1.6 WC gives:
> 
>   $ svn st
>   svn: Please see the 'svn upgrade' command
>   svn: Working copy format of '/home/julianfoad/src/subversion-n-gamma'
> is too old (10)
> 
> I would like to see a more user-friendly message, giving the version
> number instead of (or as well as) the WC format number, e.g.:
> 
>   $ svn st
>   svn: Please see the 'svn upgrade' command
>   svn: Working copy '/home/julianfoad/src/subversion-n-gamma' is too old
> (format 10, created by svn 1.6)
> 
> Agreed so far?
> 
> The simple way is to embed a (format => version) conversion table in
> libsvn_wc where this message is generated.  Patch attached.
> 
> The more "proper" layered way is for the the client to have that
> knowledge and do that conversion.  That requires somehow passing the
> format number of that WC path to the client.  (Extracting that info from
> the error message is the wrong way, of course - and not portable to
> other locales.)  Lacking parameterized error objects, I can't think of a
> practical way to achieve this.
> 
> I think the simple patch (or something very like it) should be
> acceptable.  What do you think?  If not, any bright ideas how to do it
> better?
> 
> - Julian
> 
> differences between files attachment
> (version-in-wc-format-error-1.patch)
> * subversion/libsvn_wc/upgrade.c
>   (upgrade_to_wcng): 
>   (svn_wc__upgrade_sdb): 
> 
> --This line, and those below, will be ignored--
> 
> Index: subversion/libsvn_wc/upgrade.c
> ===
> --- subversion/libsvn_wc/upgrade.c(revision 1050449)
> +++ subversion/libsvn_wc/upgrade.c(working copy)
> @@ -1309,6 +1309,21 @@ upgrade_to_wcng(void **dir_baton,
>  }
>  
> 
> +/* Return a string indicating the released version (or versions) of
> + * Subversion that used WC format number WC_FORMAT, or else "unknown"
> + * if no released version used WC_FORMAT. */
> +static const char *
> +version_string_from_format(int wc_format)
> +{
> +  switch (wc_format)
> +{
> +case 4: return "<=1.3";
> +case 8: return "1.4";
> +case 9: return "1.5";
> +case 10: return "1.6";
> +}
> +  return _("(development version)");
> +}
>  
>  svn_error_t *
>  svn_wc__upgrade_sdb(int *result_format,
> @@ -1321,15 +1336,18 @@ svn_wc__upgrade_sdb(int *result_format,
>  
>if (start_format < SVN_WC__WC_NG_VERSION /* 12 */)
>  return svn_error_createf(SVN_ERR_WC_UPGRADE_REQUIRED, NULL,
> - _("Working copy format of '%s' is too old 
> (%d)"),
> + _("Working copy '%s' is too old (format %d, "
> +   "created by Subversion %s)"),
>   svn_dirent_local_style(wcroot_abspath,
>  scratch_pool),
> - start_format);
> + start_format,
> + version_string_from_format(start_format));
>  
>/* Early WCNG formats no longer supported. */
>if (start_format < 19)
>  return svn_error_createf(SVN_ERR_WC_UPGRADE_REQUIRED, NULL,
> - _("Working copy format of '%s' is too old (%d); 
> "
> + _("Working copy '%s' is an old development "
> +   "version (format %d); to upgrade it, "
> "use a format 18 client, then "
> "use 'tools/dev/wc-ng/bump-to-19.py', then "
> "use the current client"),

Re: svn commit: r1051819 - /subversion/trunk/subversion/tests/cmdline/entries-dump.c

[Philip Martin]

"Hyrum K. Wright"  writes:

>> After this commit I get the following warning when I run ./autogen.sh
>>
>> WARNING: "../libsvn_wc/entries.h" header not found, file
>> subversion/tests/cmdline/entries-dump.c
>
> This may also be causing the build failure on the windows builtslave:
>
> "D:\svn-w2k3-ra\slik-w2k3-x64-ra\build\subversion_vcnet.sln"
> (__ALL_TESTS__ target) (1) ->
> (Programs\entries-dump target) ->
>   entries-dump.obj : error LNK2019: unresolved external symbol
> _svn_wc__read_entries_old referenced in function _entries_dump
>   ..\..\..\Debug\subversion\tests\cmdline\entries-dump.exe : fatal
> error LNK1120: 1 unresolved externals

I believe these are fixed by r1051906.

-- 
Philip

Re: Disable transform_libtool_scripts.py by default?

[Philip Martin]

Philip Martin  writes:

> I'd like to disable libtool transformation by default, i.e. make
> --disable-local-library-preloading the default.  The script messes with
> the internals of libtool and so runs the risk of breaking the build,
> particularly on less common platforms.

I've made this change in r1051931.  Another effect of the libtool
transformation script, on my Linux machine at least, is that it causes
the install process to install the binaries as libtool scripts rather
than directly as ELF executables.

-- 
Philip

Re: svn commit: r1051778 - in /subversion/trunk/subversion: libsvn_repos/commit.c libsvn_repos/load-fs-vtable.c mod_dav_svn/lock.c mod_dav_svn/version.c

[Daniel Shahaf]

bl...@apache.org wrote on Wed, Dec 22, 2010 at 07:10:06 -:
> Author: blair
> Date: Wed Dec 22 07:10:05 2010
> New Revision: 1051778
> 
> URL: http://svn.apache.org/viewvc?rev=1051778&view=rev
> Log:
> Have all remaining calls of svn_fs_commit_txn() and
> svn_repos_fs_commit_txn() use the contract that a commit was
> successful if the returned revision is a valid revision number.  The
> returned error, if any, is no longer used as an indication of commit
> success.
> 
> * subversion/mod_dav_svn/version.c
>   (dav_svn__checkin),
>   (merge):
> Use revision number returned from svn_repos_fs_commit_txn() to
> test for a successful commit.
> 
> * subversion/mod_dav_svn/lock.c
>   (append_locks):
> Ditto.
> 
> * subversion/libsvn_repos/load-fs-vtable.c
>   (close_revision):
> Ditto.
> 
> * subversion/libsvn_repos/commit.c
>   (close_edit):
> Ditto.
> 
> Modified:
> subversion/trunk/subversion/libsvn_repos/commit.c
> subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
> subversion/trunk/subversion/mod_dav_svn/lock.c
> subversion/trunk/subversion/mod_dav_svn/version.c
> 
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051778&r1=1051777&r2=1051778&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 07:10:05 2010
> @@ -716,9 +716,9 @@ close_edit(void *edit_baton,
>err = svn_repos_fs_commit_txn(&conflict, eb->repos,
>  &new_revision, eb->txn, pool);
>  
> -  if (err)
> +  if (SVN_IS_VALID_REVNUM(new_revision))
>  {
> -  if (err->apr_err == SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED)
> +  if (err)
>  {
>/* If the error was in post-commit, then the commit itself
>   succeeded.  In which case, save the post-commit warning
> @@ -729,28 +729,28 @@ close_edit(void *edit_baton,
>svn_error_clear(err);
>err = SVN_NO_ERROR;
>  }
> -  else  /* Got a real error -- one that stopped the commit */
> -{
> -  /* ### todo: we should check whether it really was a conflict,
> - and return the conflict info if so? */
> +}
> +  else
> +{
> +  /* ### todo: we should check whether it really was a conflict,
> + and return the conflict info if so? */
>  
> -  /* If the commit failed, it's *probably* due to a conflict --
> - that is, the txn being out-of-date.  The filesystem gives us
> - the ability to continue diddling the transaction and try
> - again; but let's face it: that's not how the cvs or svn works
> - from a user interface standpoint.  Thus we don't make use of
> - this fs feature (for now, at least.)
> -
> - So, in a nutshell: svn commits are an all-or-nothing deal.
> - Each commit creates a new fs txn which either succeeds or is
> - aborted completely.  No second chances;  the user simply
> - needs to update and commit again  :)
> -
> - We ignore the possible error result from svn_fs_abort_txn();
> - it's more important to return the original error. */
> -  svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
> -  return svn_error_return(err);
> -}
> +  /* If the commit failed, it's *probably* due to a conflict --
> + that is, the txn being out-of-date.  The filesystem gives us
> + the ability to continue diddling the transaction and try
> + again; but let's face it: that's not how the cvs or svn works
> + from a user interface standpoint.  Thus we don't make use of
> + this fs feature (for now, at least.)
> +
> + So, in a nutshell: svn commits are an all-or-nothing deal.
> + Each commit creates a new fs txn which either succeeds or is
> + aborted completely.  No second chances;  the user simply
> + needs to update and commit again  :)
> +
> + We ignore the possible error result from svn_fs_abort_txn();
> + it's more important to return the original error. */
> +  svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
> +  return svn_error_return(err);

Shouldn't we return a non-NULL error even if ERR is SVN_NO_ERROR here?

>  }
>  
>/* Pass new revision information to the caller's callback. */
> 
> Modified: subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c?rev=1051778&r1=1051777&r2=1051778&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c (original)
> +++ subversion/trunk/subversion/lib

Re: [PATCH] Fix syntax inconsistency and update comments.

[Noorul Islam K M]

"Hyrum K. Wright"  writes:

> On Wed, Dec 22, 2010 at 12:27 AM, Noorul Islam K M  wrote:
>
>>
>> Find attached minor patch which makes 'if' syntax consistent and also
>> some comment improvements.
>>
>> Log
>>
>> [[[
>>
>> Fix syntax inconsistency and update comments.
>>
>> * libsvn_client/locking_commands.c
>>  (organize_lock_targets): Fix syntax inconsistency. Update comments.
>
> *What* about the syntax inconsistency are you fixing?  White space?
> Function names? Use of braces?  (I can tell by looking at the patch,
> but a more informative log message would be nice.)
>
> Same with "update comments".  Update them in what way?
>

Please find updated log message.

[[[

Fix syntax inconsistency and update comments.

* libsvn_client/locking_commands.c
  (organize_lock_targets): Fix syntax inconsistency by removing
unnecessary braces from 'if' block. Update comments to reflect new
function names used.

Patch by: Noorul Islam K M 
]]]

Thanks and Regards
Noorul

Re: svn commit: r1051745 - /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c

[Hyrum K. Wright]

On Wed, Dec 22, 2010 at 1:15 AM, Kamesh Jayachandran  wrote:
> On 12/22/2010 09:38 AM, bl...@apache.org wrote:
>>
>> Author: blair
>> Date: Wed Dec 22 04:08:14 2010
>> New Revision: 1051745
>>
>> URL: http://svn.apache.org/viewvc?rev=1051745&view=rev
>> Log:
>> Update test_commit_txn() to handle svn_fs_commit_txn()'s return
>> semantics.
>>
>> * subversion/tests/libsvn_fs/fs-test.c
>>   (test_commit_txn):
>>     If svn_fs_commit_txn() returns an error, then always return an
>>       error to the caller, just use a different wrapping error message
>>       if the commit succeeded or failed.
>>     If svn_fs_commit_txn() returns no error, than assert that a valid
>>       revision number was returned.
>>
>> Modified:
>>     subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>>
>> Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1051745&r1=1051744&r2=1051745&view=diff
>>
>> ==
>> --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
>> +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Wed Dec 22
>> 04:08:14 2010
>> @@ -61,6 +61,7 @@
>>   * EXPECTED_CONFLICT.  If they don't match, return error.
>>   *
>>   * If a conflict is expected but the commit succeeds anyway, return
>> + * error.  If the commit fails but does not provide an error, return
>>   * error.
>>   */
>>  static svn_error_t *
>> @@ -110,13 +111,24 @@ test_commit_txn(svn_revnum_t *new_rev,
>>               "conflicting commit returned valid new revision");
>>          }
>>      }
>> -  else if (err)   /* commit failed, but not due to conflict */
>> +  else if (err)   /* commit may have succeeded, but always report an
>> error */
>>      {
>> -      return svn_error_quick_wrap
>> -        (err, "commit failed due to something other than a conflict");
>> +      if (SVN_IS_VALID_REVNUM(*new_rev))
>> +        return svn_error_quick_wrap
>> +          (err, "commit succeeded but something else failed");
>
> Should this error not be wrapped inside "_()"?
>
>> +      else
>> +        return svn_error_quick_wrap
>> +          (err, "commit failed due to something other than a conflict");
>
> Same as above.
>
>>      }
>> -  else            /* err == NULL, so commit succeeded */
>> +  else            /* err == NULL, commit should have succeeded */
>>      {
>> +      if (! SVN_IS_VALID_REVNUM(*new_rev))
>> +        {
>> +          return svn_error_create
>> +            (SVN_ERR_FS_GENERAL, NULL,
>> +             "commit failed but no error was returned");
>
> Same as above.

Do we ask our translators to translate error messages specific to the
testsuite?  (Honest question; I really don't know.)

If we do, I'd propose we stop. :)  Their time is better spent
localizing the core product, not the tests.  (And none of the Python
test suite is localized, iirc.)

-Hyrum

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Daniel Shahaf]

bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -:
> Author: blair
> Date: Wed Dec 22 05:46:45 2010
> New Revision: 1051763
> 
> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
> Log:
> Add a private function that takes the error returned from
> svn_repos_fs_commit_txn() and builds a error message string containing
> either or both of the svn_fs_commit_txn() error and the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped post-commit error.  The
> function skips over tracing errors.
> 
> * subversion/include/private/svn_repos_private.h
>   (svn_repos__post_commit_error_str):
> New private function.
> 
> * subversion/libsvn_repos/commit.c
>   (svn_repos__post_commit_error_str):
> Implement.
>   (close_edit):
> Use svn_repos__post_commit_error_str() instead of processing the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error.
> 
> * subversion/mod_dav_svn/version.c
>   (merge):
> Use svn_repos__post_commit_error_str() instead of processing the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error.
> 
> Modified:
> subversion/trunk/subversion/include/private/svn_repos_private.h
> subversion/trunk/subversion/libsvn_repos/commit.c
> subversion/trunk/subversion/mod_dav_svn/version.c
> 
> Modified: subversion/trunk/subversion/include/private/svn_repos_private.h
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_repos_private.h?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==
> --- subversion/trunk/subversion/include/private/svn_repos_private.h (original)
> +++ subversion/trunk/subversion/include/private/svn_repos_private.h Wed Dec 
> 22 05:46:45 2010
> @@ -84,6 +84,25 @@ svn_repos__validate_prop(const char *nam
>   const svn_string_t *value,
>   apr_pool_t *pool);
>  
> +/**
> + * Given the error @a err from svn_repos_fs_commit_txn(), return an
> + * string containing either or both of the svn_fs_commit_txn() error
> + * and the SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error from
> + * the post-commit hook.  Any error tracing placeholders in the error
> + * chain are skipped over.
> + *
> + * ### This method should not be necessary, but there are a few
> + * ### places, e.g. mod_dav_svn, where only a single error message
> + * ### string is returned to the caller and it is useful to have both
> + * ### error messages included in the message.
> + *
> + * Use @a pool to allocate the string in.
> + *
> + * @since New in 1.7.
> + */
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool);
>  
>  #ifdef __cplusplus
>  }
> 
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
> name, value, pool);
>  }
>  
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool)
> +{
> +  svn_error_t *hook_err1, *hook_err2;
> +
> +  if (! err)
> +return "(no error)";
> +
> +  err = svn_error_purge_tracing(err);
>  

This modifies the provided error.  Either the doc string should warn
that this is being done (i.e., the provided ERR is not safe for use
after calling this helper), or this call should be removed and another
means used to locate the first non-tracing error message.

Where do you clear ERR?  You must do so in this function, since the
caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
promise).

> +  hook_err1 = svn_error_find_cause(err, 
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED);
> +  if (hook_err1 && hook_err1->child)
> +hook_err2 = hook_err1->child;
> +  else
> +hook_err2 = hook_err1;
> +

So, ERR is the svn_fs_commit_txn() error (which has the hook error
somewhere down the chain), and HOOK_ERR1 is the post-commit hook error?

> +  /* This implementation counts on svn_repos_fs_commit_txn() returning
> + svn_fs_commit_txn() as the parent error with a child
> + SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error.  If the parent error
> + is SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED then there was no error
> + in svn_fs_commit_txn(). */
> +  if (hook_err1)
> +{
> +  if (err == hook_err1)
> +{
> +  if (hook_err2->message)
> +return apr_pstrdup(pool, hook_err2->message);
> +  else
> +return "(no error message)";
> +}
> +  else
> +{
> +  return apr_psprintf(pool,
> +  "Post c

Re: svn commit: r1051819 - /subversion/trunk/subversion/tests/cmdline/entries-dump.c

[Hyrum K. Wright]

On Wed, Dec 22, 2010 at 6:21 AM, Noorul Islam K M  wrote:
> phi...@apache.org writes:
>
>> Author: philip
>> Date: Wed Dec 22 10:41:38 2010
>> New Revision: 1051819
>>
>> URL: http://svn.apache.org/viewvc?rev=1051819&view=rev
>> Log:
>> Make entries-dump fallback to operating on pre-wcng working copies.
>> This allows the 1.7 command line regression tests to be run against
>> the 1.6 binaries.
>>
>> * subversion/tests/cmdline/entries-dump.c
>>   (entries_dump): Read old entries and check for lock file if opening
>>    access baton fails, only close access baton if it was opened.
>>   (directory_dump_old): New.
>>   (directory_dump): Walk old entries if walking children fails.
>>
>> Modified:
>>     subversion/trunk/subversion/tests/cmdline/entries-dump.c
>>
>> Modified: subversion/trunk/subversion/tests/cmdline/entries-dump.c
>> URL: 
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/entries-dump.c?rev=1051819&r1=1051818&r2=1051819&view=diff
>> ==
>> --- subversion/trunk/subversion/tests/cmdline/entries-dump.c (original)
>> +++ subversion/trunk/subversion/tests/cmdline/entries-dump.c Wed Dec 22 
>> 10:41:38 2010
>> @@ -33,6 +33,7 @@
>>  #include "svn_dirent_uri.h"
>>
>>  #include "private/svn_wc_private.h"
>> +#include "../libsvn_wc/entries.h"
>>
>
> After this commit I get the following warning when I run ./autogen.sh
>
> WARNING: "../libsvn_wc/entries.h" header not found, file
> subversion/tests/cmdline/entries-dump.c

This may also be causing the build failure on the windows builtslave:

"D:\svn-w2k3-ra\slik-w2k3-x64-ra\build\subversion_vcnet.sln"
(__ALL_TESTS__ target) (1) ->
(Programs\entries-dump target) ->
  entries-dump.obj : error LNK2019: unresolved external symbol
_svn_wc__read_entries_old referenced in function _entries_dump
  ..\..\..\Debug\subversion\tests\cmdline\entries-dump.exe : fatal
error LNK1120: 1 unresolved externals

-Hyrum

Re: [PATCH] Fix syntax inconsistency and update comments.

[Hyrum K. Wright]

On Wed, Dec 22, 2010 at 12:27 AM, Noorul Islam K M  wrote:
>
> Find attached minor patch which makes 'if' syntax consistent and also
> some comment improvements.
>
> Log
>
> [[[
>
> Fix syntax inconsistency and update comments.
>
> * libsvn_client/locking_commands.c
>  (organize_lock_targets): Fix syntax inconsistency. Update comments.

*What* about the syntax inconsistency are you fixing?  White space?
Function names? Use of braces?  (I can tell by looking at the patch,
but a more informative log message would be nice.)

Same with "update comments".  Update them in what way?

>
> Patch by: Noorul Islam K M 
> ]]]
>
> Thanks and Regards
> Noorul
>
>
> Index: libsvn_client/locking_commands.c
> ===
> --- libsvn_client/locking_commands.c    (revision 1051763)
> +++ libsvn_client/locking_commands.c    (working copy)
> @@ -191,18 +191,15 @@
>
>   /* Get the common parent and all paths */
>   if (url_mode)
> -    {
>       SVN_ERR(svn_uri_condense_targets(common_parent_url, &rel_targets,
>                                        targets, TRUE, pool, pool));
> -    }
>   else
> -    {
>       SVN_ERR(svn_dirent_condense_targets(common_parent_url, &rel_targets,
>                                           targets, TRUE, pool, pool));
> -    }
>
> -  /* svn_path_condense_targets leaves paths empty if TARGETS only had
> -     1 member, so we special case that. */
> +  /* svn_uri_condense_targets and svn_dirent_condense_targets leaves
> +     URLs/paths empty if TARGETS only had 1 member, so we special case
> +     that. */
>   if (apr_is_empty_array(rel_targets))
>     {
>       const char *parent, *base;
> @@ -273,8 +270,8 @@
>       SVN_ERR(svn_uri_condense_targets(&common_url, &rel_urls, urls,
>                                        FALSE, pool, pool));
>
> -      /* svn_path_condense_targets leaves paths empty if TARGETS only had
> -         1 member, so we special case that (again). */
> +      /* svn_uri_condense_targets leaves URLs empty if TARGETS only
> +         had 1 member, so we special case that (again). */
>       if (apr_is_empty_array(rel_urls))
>         {
>           const char *base_name = svn_uri_basename(common_url, pool);
>
>

Re: Default commandline args

[Hyrum K. Wright]

On Tue, Dec 21, 2010 at 12:34 AM, Blair Zajac  wrote:
> 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.

There a chicken-and-egg problem with using ~/.subversion (and it's
other-platform equivalents).  We have a commandline arg which allows a
user to specify, at runtime, the configuration directory to use.  In
order to see this arg, we have to parse the comandline arguments.  It
becomes rather more convoluted if we parse some of the arguments to
determine the config directory, and then add more arguments *based* on
that config directory.

Also, I agree with the other point that everything in ~/.subversion is
libsvn_client (or lower), and since this is strictly a commandline
client feature, it should go in a separate location.

-Hyrum

Re: svn commit: r1051875 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c

[Daniel Shahaf]

julianf...@apache.org wrote on Wed, Dec 22, 2010 at 12:48:01 -:
> Author: julianfoad
> Date: Wed Dec 22 12:48:01 2010
> New Revision: 1051875
> 
> URL: http://svn.apache.org/viewvc?rev=1051875&view=rev
> Log:
> Fix a printf format-string insecurity. A follow-up to r1030536. Found by
> my compiler.
> 
> * subversion/mod_authz_svn/mod_authz_svn.c
>   (get_access_conf): Insert the error message using 
> "subversion/mod_authz_svn/mod_authz_svn.cs" rather than directly
> as the format string argument.
> 

So /that/ is why people use Emacs...

Re: [PATCH] Fix some deprecation warnings

[Daniel Shahaf]

Kamesh Jayachandran wrote on Wed, Dec 22, 2010 at 15:10:46 +0530:
> On 12/22/2010 02:02 AM, vijay wrote:
>> Hi,
>>
>> I have attached a patch that fixes few deprecation warnings while  
>> compiling libsvn_client/log.c.
>> Attached log message also.
>>
>> Thanks & Regards,
>> Vijayaguru
>
> Can you pass scratch_pool for the below call as 'iterpool' and move the  
> iterpool destruction down?
>

... presumably in order to save a bit of memory.

Daniel
(not disagreeing)

> +  SVN_ERR(svn_uri_condense_targets(&url_or_path,&condensed_targets,
> +   target_urls, TRUE, pool, pool));
>
>
>
>
> With regards
> Kamesh Jayachandran

Re: svn commit: r1051566 - /subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c

[Daniel Shahaf]

Julian Foad wrote on Wed, Dec 22, 2010 at 12:34:05 +:
> danie...@apache.org wrote:
> > When ignoring a warning, do so noisily if in maintainer mode.
> > 
> > * subversion/libsvn_ra_local/ra_plugin.c
> >   (ignore_warnings):
> > In maintainer mode, output notice that an error is being ignored.
> > Update the comment, which was outdated by r1051559.
> 
> subversion/libsvn_ra_local/ra_plugin.c: In function ‘ignore_warnings’:
> subversion/libsvn_ra_local/ra_plugin.c:437: warning: format ‘%ld’
> expects type ‘long int’, but argument 2 has type ‘int’
> 

Thanks, r1051886.

(There is no APR_STATUS_T_FMT, but our maintainer-mode printing uses %d,
so I went with that.)

> - Julian
> 
> 
> > Modified: subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
> > URL: 
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c?rev=1051566&r1=1051565&r2=1051566&view=diff
> > ==
> > --- subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c Tue Dec 21 
> > 17:18:53 2010
> > @@ -416,7 +416,7 @@ svn_ra_local__get_schemes(apr_pool_t *po
> >  
> >  /* Do nothing.
> >   *
> > - * Why is this acceptable?  As of now, FS warnings are used for only
> > + * Why is this acceptable?  FS warnings used to be used for only
> >   * two things: failures to close BDB repositories and failures to
> >   * interact with memcached in FSFS (new in 1.6).  In 1.5 and earlier,
> >   * we did not call svn_fs_set_warning_func in ra_local, which means
> > @@ -433,6 +433,9 @@ static void
> >  ignore_warnings(void *baton,
> >  svn_error_t *err)
> >  {
> > +#ifdef SVN_DEBUG
> > +  SVN_DBG(("Ignoring FS warning %ld\n", err ? err->apr_err : 0));
> > +#endif
> >return;
> >  }
> 
> 

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

[Daniel Shahaf]

Blair Zajac wrote on Tue, Dec 21, 2010 at 19:48:49 -0800:
> On 12/21/10 5:57 PM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 13:47:40 -0800:
>>> On 12/21/10 11:44 AM, Daniel Shahaf wrote:
 Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>> svn_fs_commit_txn()'s error as the parent followed by the
>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child?  This seems to be
>> the standard ordering of chained errors.  On the other hand, it makes it
>> harder to find a post-commit script error.
>
> Actually, it will make it impossible to detect post-commit errors over
> ra_dav, since that RA layer marshals only the outermost error code in an
> error chain.

 This is now.

 (Details are partly from memory, partly from quick testing I re-did today.)
>>>
>>> While we're opening tickets, I found an issue with
>>> svn_repos_parse_fns2_t.close_revision(), it doesn't support the
>>> documented svn_fs_commit_txn() contract, I opened this to track it and
>>> put it in 1.7-consider, as it shouldn't block 1.7, but it would be nice
>>> to have it in there.
>>>
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=3768
>>>
>>
>> svn_repos_parse_fns2_t is about parsing a dumpstream, not about
>> committing the expressed revisions to some repository, so I don't
>> understand why the "NEW_REV" parameter belongs there and not in
>> svn_repos_get_fs_build_parser3().
>
> I haven't used nor looked too closely at this API, but reading the API, 
> it can commit multiple revisions, so shouldn't the errors be returned 
> from each revision committed?
>

Definitely.

> In svn_repos_parse_dumpstream2(), I see that it commits each revision.
>

As I understand it:

* svn_repos_parse_dumpstream2()
input: dump stream
output: drives a provided svn_repos_parse_fns2_t vtable

* svn_repos_get_fs_build_parser3()
input: svn_repos_t handle
output: svn_repos_parse_fns2_t vtable that, when driven, commits to
the provided repository

Therefore, any notion of "Did the commit succeed" and "What revnum it
resulted in" belongs in the latter, not in the former.

Just consider alternative implementors of the vtable: does NEW_REV make
sense for 'svnrdump load'?  Does it make sense for a vtable
implementation that just prints what callbacks were called (akin to the
debug_editor)?

>   /* If we already have a rev_baton open, we need to close it
>  and clear the per-revision subpool. */
>   if (rev_baton != NULL)
> {
>   SVN_ERR(parse_fns->close_revision(rev_baton));
>
> This code couldn't handle a successful commit followed by an internal FS  
> failure, so close_revision() would need to have a *new_rev argument.
>
> Let me know if I'm reading this correctly.
>
> Blair

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

[Daniel Shahaf]

Forwarding back to the list.

Tijn Porcelijn wrote on Wed, Dec 22, 2010 at 01:54:07 -0800:
> Hi Daniel,
> 
> I guess you're right. It makes more sense to echo the external URL
> than the local directory in the "Updated external
> 'url://remote/path/to/somewhere' to revision %ld". Anyway, my
> suggestion was to make sure that "some" reference to the specific url
> was made *at all* after removing the other notice (as opposed to just
> printing a revision number and having to guess the repository it
> refers to).

I'm still not sure I understand the issue, or why you think adding that
would be useful.  (Not saying that it isn't useful; just that I don't
understand why it would be.)

Before you invest more time in coding, could you please give more
concrete examples of how the current output is not satisfactory?

e.g., are you trying to parse it with a script?  Or is it just that the
information you want has scrolled offscreen and you want to repeat it
nearer to the end of the output?

Thanks,

Daniel

> I will look into printing the remote external's path.
> 
> Thanks, tijn
> 
> On 2010-12-22 02:54, Daniel Shahaf wrote:
> 
> This has a bug, when updating a file external it displays the
> external's directory rather than the external itself.  (But maybe this
> is a bug in the way the library generates the notifications?)
> 
> May I ask what is the motivation for this change?  The normal
> notifications (U   path/to/somewhere) will always immediately precede
> the "Updated external 'path/to/somewhere' to revision %ld", so
> repeating the external's path there seems a bit redundant.
> 
> I haven't run 'make check'.
> 
> Daniel
> 
> 
> Tijn Porcelijn wrote on Fri, Nov 26, 2010 at 01:56:22 -0800:
> > [[[
> > Improves interaction, issue #3653: svn update should not output svn:external
> > * subversion/svn/notify.c (notify)
> >   Add  to Externals messages
> >   Note: po files should also be updated
> > ]]]
> >
> >
> >
> >
> > Hi,
> >
> > Here's a small patch for making svn:externals messages a bit more 
> > informative. With the "Fetching external item into ''" -message 
> > removed, interpretation of svn_wc_notify_update_completed messages becomes 
> > a bit less obvious. You'll see stuff like:
> > External at revision 20
> > External at revision 2321
> > External at revision 1082367
> > At revision 19
> > The patch improves this to read:
> > External 'third-party' at revision 20
> > External 'snapshots' at revision 2321
> > External 'legacy' at revision 1082367
> > At revision 19
> > See attached notify.c.patch, Thanks,
> >
> > tijn
> >
> 
> Content-Description: notify.c.patch
> > Index: subversion/svn/notify.c
> > ===
> > --- subversion/svn/notify.c   (revision 1038983)
> > +++ subversion/svn/notify.c   (working copy)
> > @@ -567,44 +567,66 @@
> >{
> >  if (nb->is_export)
> >{
> > -if ((err = svn_cmdline_printf
> > - (pool, nb->in_external
> > -  ? _("Exported external at revision %ld.\n")
> > -  : _("Exported revision %ld.\n"),
> > -  n->revision)))
> > -  goto print_error;
> > +if (nb->in_external)
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Exported external '%s' at revision %ld.\n"),
> > +  path_local,
> > +  n->revision);
> > +else
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Exported revision %ld.\n"),
> > +  n->revision);
> >}
> >  else if (nb->is_checkout)
> >{
> > -if ((err = svn_cmdline_printf
> > - (pool, nb->in_external
> > -  ? _("Checked out external at revision %ld.\n")
> > -  : _("Checked out revision %ld.\n"),
> > -  n->revision)))
> > -  goto print_error;
> > +if (nb->in_external)
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Checked out external '%s' at revision 
> > %ld.\n"),
> > +  path_local,
> > +  n->revision);
> > +else
> > +  err = svn_cmdline_printf
> > + (pool,
> > +  _("Checked out revision %ld.\n"),
> > +  n->revision);
> >}
> >  else
> >{
> >  if (nb->received_some_change)
> >{
> >  nb->received_some_change 

Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

[Julian Foad]

subversion/libsvn_repos/commit.c:648: no previous prototype for
'svn_repos__post_commit_error_str'

- Julian


bl...@apache.org wrote:
> Add a private function that takes the error returned from
> svn_repos_fs_commit_txn() and builds a error message string containing
> either or both of the svn_fs_commit_txn() error and the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped post-commit error.  The
> function skips over tracing errors.
> 
> * subversion/include/private/svn_repos_private.h
>   (svn_repos__post_commit_error_str):
> New private function.
> 
> * subversion/libsvn_repos/commit.c
>   (svn_repos__post_commit_error_str):
> Implement.
>   (close_edit):
> Use svn_repos__post_commit_error_str() instead of processing the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error.
[...]

> Modified: subversion/trunk/subversion/include/private/svn_repos_private.h
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_repos_private.h?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==
> --- subversion/trunk/subversion/include/private/svn_repos_private.h (original)
> +++ subversion/trunk/subversion/include/private/svn_repos_private.h Wed Dec 
> 22 05:46:45 2010
> @@ -84,6 +84,25 @@ svn_repos__validate_prop(const char *nam
>   const svn_string_t *value,
>   apr_pool_t *pool);
>  
> +/**
> + * Given the error @a err from svn_repos_fs_commit_txn(), return an
> + * string containing either or both of the svn_fs_commit_txn() error
> + * and the SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error from
> + * the post-commit hook.  Any error tracing placeholders in the error
> + * chain are skipped over.
> + *
> + * ### This method should not be necessary, but there are a few
> + * ### places, e.g. mod_dav_svn, where only a single error message
> + * ### string is returned to the caller and it is useful to have both
> + * ### error messages included in the message.
> + *
> + * Use @a pool to allocate the string in.
> + *
> + * @since New in 1.7.
> + */
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool);
>  
>  #ifdef __cplusplus
>  }
> 
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
> name, value, pool);
>  }
>  
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool)
> +{
> +  svn_error_t *hook_err1, *hook_err2;
[...]

Re: svn commit: r1051566 - /subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c

[Julian Foad]

danie...@apache.org wrote:
> When ignoring a warning, do so noisily if in maintainer mode.
> 
> * subversion/libsvn_ra_local/ra_plugin.c
>   (ignore_warnings):
> In maintainer mode, output notice that an error is being ignored.
> Update the comment, which was outdated by r1051559.

subversion/libsvn_ra_local/ra_plugin.c: In function ‘ignore_warnings’:
subversion/libsvn_ra_local/ra_plugin.c:437: warning: format ‘%ld’
expects type ‘long int’, but argument 2 has type ‘int’

- Julian


> Modified: subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c?rev=1051566&r1=1051565&r2=1051566&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c Tue Dec 21 
> 17:18:53 2010
> @@ -416,7 +416,7 @@ svn_ra_local__get_schemes(apr_pool_t *po
>  
>  /* Do nothing.
>   *
> - * Why is this acceptable?  As of now, FS warnings are used for only
> + * Why is this acceptable?  FS warnings used to be used for only
>   * two things: failures to close BDB repositories and failures to
>   * interact with memcached in FSFS (new in 1.6).  In 1.5 and earlier,
>   * we did not call svn_fs_set_warning_func in ra_local, which means
> @@ -433,6 +433,9 @@ static void
>  ignore_warnings(void *baton,
>  svn_error_t *err)
>  {
> +#ifdef SVN_DEBUG
> +  SVN_DBG(("Ignoring FS warning %ld\n", err ? err->apr_err : 0));
> +#endif
>return;
>  }

Re: svn commit: r1051819 - /subversion/trunk/subversion/tests/cmdline/entries-dump.c

[Noorul Islam K M]

phi...@apache.org writes:

> Author: philip
> Date: Wed Dec 22 10:41:38 2010
> New Revision: 1051819
>
> URL: http://svn.apache.org/viewvc?rev=1051819&view=rev
> Log:
> Make entries-dump fallback to operating on pre-wcng working copies.
> This allows the 1.7 command line regression tests to be run against
> the 1.6 binaries.
>
> * subversion/tests/cmdline/entries-dump.c
>   (entries_dump): Read old entries and check for lock file if opening
>access baton fails, only close access baton if it was opened.
>   (directory_dump_old): New.
>   (directory_dump): Walk old entries if walking children fails.
>
> Modified:
> subversion/trunk/subversion/tests/cmdline/entries-dump.c
>
> Modified: subversion/trunk/subversion/tests/cmdline/entries-dump.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/entries-dump.c?rev=1051819&r1=1051818&r2=1051819&view=diff
> ==
> --- subversion/trunk/subversion/tests/cmdline/entries-dump.c (original)
> +++ subversion/trunk/subversion/tests/cmdline/entries-dump.c Wed Dec 22 
> 10:41:38 2010
> @@ -33,6 +33,7 @@
>  #include "svn_dirent_uri.h"
>  
>  #include "private/svn_wc_private.h"
> +#include "../libsvn_wc/entries.h"
>

After this commit I get the following warning when I run ./autogen.sh

WARNING: "../libsvn_wc/entries.h" header not found, file
subversion/tests/cmdline/entries-dump.c

Thanks and Regards
Noorul

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

[Julian Foad]

Danny Trebbien wrote:
> That's much better than mine, and I have verified that the tests are
> the same. Please commit.

Thanks very much.  Committed revision 1051864.

- Julian

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

[Philip Martin]

Johan Corveleyn  writes:

> On Mon, Dec 20, 2010 at 11:19 AM, Philip Martin
>  wrote:
>> Johan Corveleyn  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.
>
> Hmm, it's not easy to come up with a test script to test this "from
> scratch" (unless with testing diff directly, see below). I test it
> with a repository (a dump/load of an old version of our production
> repository) which contains this 6 line xml file (1,5 Mb) with 2272
> revisions.
>
> I run blame on this file, over svnserve protocol on localhost (server
> running on same machine), with an svnserve built from Stefan^2's
> performance branch (with membuffer caching of full-texts, so server
> I/O is not the bottleneck). This gives me an easy way to call 2272
> times diff on this file, and measure it (with the help of some
> instrumentation code in blame.c, see attachment). And it's
> incidentally the actual use case I first started out wanting to
> optimize (blame for large files with many revisions).

Testing with real-world data is important, perhaps even more important
than artificial test data, but some test data would be useful.  If you
were to write a script to generate two test files of size 100MB, say,
then you could use the tools/diff/diff utility to run Subversion diff on
those two files.  Or tools/diff/diff3 if it's a 3-way diff that matters.
The first run might involve disk IO, but on most machines the OS should
be able to cache the files and subsequent hot-cache runs should be a
good way to profile the diff code, assumming it is CPU limited.

-- 
Philip

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

[Noorul Islam K M]

rhuij...@apache.org writes:

> Author: rhuijben
> Date: Wed Dec 22 09:40:57 2010
> New Revision: 1051804
>
> URL: http://svn.apache.org/viewvc?rev=1051804&view=rev
> Log:
> * subversion/libsvn_ra/util.c
>   (is_atomicity_error): Following up on r1051702, check pointer against NULL
> instead of just using it as a svn_boolean_t to fix compilation error on
> Windows (and a possible invalid cast if svn_boolean_t is smaller then a
> pointer).

Typo?

Thanks and Regards
Noorul

Re: [PATCH] Fix some deprecation warnings

[Kamesh Jayachandran]

On 12/22/2010 02:02 AM, vijay wrote:

Hi,

I have attached a patch that fixes few deprecation warnings while 
compiling libsvn_client/log.c.

Attached log message also.

Thanks & Regards,
Vijayaguru


Can you pass scratch_pool for the below call as 'iterpool' and move the 
iterpool destruction down?


+  SVN_ERR(svn_uri_condense_targets(&url_or_path,&condensed_targets,
+   target_urls, TRUE, pool, pool));




With regards
Kamesh Jayachandran