from:"Kamesh Jayachandran"


On 05/23/2011 02:28 PM, Prabhu Gnana Sundar wrote:

On Monday 23 May 2011 02:02 PM, Stefan Sperling wrote:

On Mon, May 23, 2011 at 01:08:28PM +0530, Prabhu Gnana Sundar wrote:

Hi all,

  I have attached my recent patch along with this mail. Please
share your views.



Can you provide a log message, too?


Sure Stefan :)

Attaching the log message with this mail. Also attached is the updated
patch with the change that you suggested below.



Thanks and regards
Prabhu

Index: subversion/svnadmin/main.c
===
--- subversion/svnadmin/main.c(revision 1126350)
+++ subversion/svnadmin/main.c(working copy)
@@ -585,7 +585,6 @@
  {
struct svnadmin_opt_state *opt_state = baton;
svn_repos_t *repos;
-  apr_hash_t *config;
apr_hash_t *fs_config = apr_hash_make(pool);

/* Expect no more arguments. */
@@ -624,12 +623,9 @@
   APR_HASH_KEY_STRING,
   1);

-  SVN_ERR(svn_config_get_config(config, opt_state-config_dir, 
pool));
-  SVN_ERR(svn_cmdline__apply_config_options(config, 
opt_state-config_options,
-svnadmin: , 
--config-option));



This PATCH ignores --config-option.

Bert via r1080198 you seemed to have introduced --config-option to svnadmin.

I am not aware of a case where --config-option would be needed for svnadmin.

Can you clarify?

With regards
Kamesh Jayachandran

SVN_ERR(svn_repos_create(repos, opt_state-repository_path,
 NULL, NULL,
-   config, fs_config, pool));
+   NULL, fs_config, pool));

We could wrap the above two lines into a single line.
So that we have just one line reading:

 NULL, NULL, NULL, fs_config_pool));

Looks good to me, thanks!


svn_fs_set_warning_func(svn_repos_fs(repos), warning_func, NULL);
return SVN_NO_ERROR;
  }

Re: [PATCH] remove --config-option argument from svnadmin


On 05/23/2011 04:46 PM, Prabhu Gnana Sundar wrote:

Hi all,

 The --config-option parameter is never used with svnadmin. It 
is newly introduced in the trunk code.
There is no use-case (as far as I know) to use the --config-option 
with svnadmin. So as discussed with

Stefan Sperling via the IRC, I removed the --config-option for svnadmin.

Attaching the patch and the log message for the same. Please share 
your views.



This patch is also associated with the patch at
http://svn.haxx.se/dev/archive-2011-05/0781.shtml which passes NULL 
instead of config.


Thanks and regards
Prabhu

Thanks Prabhu,

I committed your two patches together as they are related at r1126459.

With regards
Kamesh Jayachandran

Re: [PATCH] - Remove unneeded code from svnadmin


On 05/23/2011 02:28 PM, Prabhu Gnana Sundar wrote:

On Monday 23 May 2011 02:02 PM, Stefan Sperling wrote:

On Mon, May 23, 2011 at 01:08:28PM +0530, Prabhu Gnana Sundar wrote:

Hi all,

  I have attached my recent patch along with this mail. Please
share your views.



Can you provide a log message, too?


Sure Stefan :)

Attaching the log message with this mail. Also attached is the updated
patch with the change that you suggested below.



Thanks and regards
Prabhu

Index: subversion/svnadmin/main.c
===
--- subversion/svnadmin/main.c(revision 1126350)
+++ subversion/svnadmin/main.c(working copy)
@@ -585,7 +585,6 @@
  {
struct svnadmin_opt_state *opt_state = baton;
svn_repos_t *repos;
-  apr_hash_t *config;
apr_hash_t *fs_config = apr_hash_make(pool);

/* Expect no more arguments. */
@@ -624,12 +623,9 @@
   APR_HASH_KEY_STRING,
   1);

-  SVN_ERR(svn_config_get_config(config, opt_state-config_dir, 
pool));
-  SVN_ERR(svn_cmdline__apply_config_options(config, 
opt_state-config_options,
-svnadmin: , 
--config-option));

SVN_ERR(svn_repos_create(repos, opt_state-repository_path,
 NULL, NULL,
-   config, fs_config, pool));
+   NULL, fs_config, pool));

We could wrap the above two lines into a single line.
So that we have just one line reading:

 NULL, NULL, NULL, fs_config_pool));

Looks good to me, thanks!


svn_fs_set_warning_func(svn_repos_fs(repos), warning_func, NULL);
return SVN_NO_ERROR;
  }



Thanks Prabhu,

I committed your two patches together as they are related at r1126459.

With regards
Kamesh Jayachandran

Re: r916286


On 05/18/2011 02:04 AM, Paul Burba wrote:

Author: kameshj
Date: Thu Feb 25 13:40:22 2010
New Revision: 916286

URL: http://svn.apache.org/viewvc?rev=916286view=rev
Log:
With the below apache configuration(See the trailing slash at the end
of '/svn/').

Location /svn/
  DAV svn
  SVNParentPath /repositories
#See the trailing slash on the master URI also can cause the confusion.
  SVNMasterURI http://master/svn/
  SVNAdvertiseV2Protocol Off
/Location

We get the following error on the client side.

svn: Commit failed (details follow):
svn: MKACTIVITY of
'/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could
not
read status line: connection was closed by server (http://localhost)

Hi Kamesh,



Sorry for the late response Paul.

Are there any threads or IRC logs in which this is discussed
(particularly a more detailed replication)?


No. Paul this error caught my eyes while testing something locally.


While reviewing r916286
and r917512 I tried to replicate this problem by adding a trailing '/'
to the location and SVNMasterURI.  I *did* get an error, just a
different one:

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
st
M   file.txt

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
ci -m commit to slave
svn: Commit failed (details follow):
svn: OPTIONS of 'http://localhost/svn-test-work/slave': 200 OK
(http://localhost)





What's worse is that I get this error with a server at the latest
trunk (r1104523), trunk right after you fixed this bug (r917512), and
my own local attempt to backport this branch to 1.6.x (addressing the
conflicts and the use of 1.7 APIs).  They all fail the same way.



I could not see this error(I mean everything works as expected with 
Location /svn/ and SVNMasterURI witj trailing '/') with against 
trunk@1126459


I built trunk@916285(prior to my fix) and saw this error and with 
trunk@916286 this error has gone away.



May be something to do with win32 build.

Can you attach your patch against r916285, I will build it/test it and 
let you know what is wrong?


With regards
Kamesh Jayachandran

Any insight is appreciated.

Paul


On the server(proxy) it is an assertion error on the following code
block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup

   assert((uri_segment[0] == '\0')
   || (uri_segment[0] == '/'));

For the above configuration we get the uri_segment with the value
'reponame/some/path/inside/the/repo'.

We fix this by canonicalizing the 'root_dir'(The one in Location) and
'uri.path'(Path portion of Master URI).
* subversion/mod_dav_svn/dav_svn.h
(dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
* subversion/mod_dav_svn/mod_dav_svn.c
(create_dir_config): Canonicalize the root_dir.
* subversion/mod_dav_svn/mirror.c
(dav_svn__location_in_filter, dav_svn__location_body_filter):
As root_dir is in canonical form canonicalize the uri.path too to avoid
spurious errors.
(dav_svn__location_header_filter): As root_dir is canonical we need to
  explicitly introduce the path seperator.

Suggested by: julianfoad

Modified:
subversion/trunk/subversion/mod_dav_svn/dav_svn.h
subversion/trunk/subversion/mod_dav_svn/mirror.c
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c

Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286r1=916285r2=916286view=diff
==
--- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
+++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
@@ -352,7 +352,7 @@
  /* Return the activities db */
  const char * dav_svn__get_activities_db(request_rec *r);

-/* Return the root directory */
+/* Return the root directory in canonicalized form */
  const char * dav_svn__get_root_dir(request_rec *r);



Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286r1=916285r2=916286view=diff
==
--- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
@@ -128,7 +128,7 @@
 locate_ctx_t *ctx = f-ctx;
 apr_status_t rv;
 apr_bucket *bkt;
-const char *master_uri, *root_dir;
+const char *master_uri, *root_dir, *canonicalized_uri;
 apr_uri_t uri;

 /* Don't filter if we're in a subrequest or we aren't setup to
@@ -143,7 +143,11 @@
(that is, if our root path matches that of the master server). */
 apr_uri_parse(r-pool, master_uri,uri);
 root_dir = dav_svn__get_root_dir(r);
-if (strcmp(uri.path, root_dir) == 0) {
+if (uri.path)
+canonicalized_uri = svn_dirent_canonicalize(uri.path, r-pool

Re: r916286


On 05/23/2011 09:09 PM, Paul Burba wrote:

On Mon, May 23, 2011 at 10:13 AM, Kamesh Jayachandrankam...@collab.net  wrote:

On 05/18/2011 02:04 AM, Paul Burba wrote:

Author: kameshj
Date: Thu Feb 25 13:40:22 2010
New Revision: 916286

URL: http://svn.apache.org/viewvc?rev=916286view=rev
Log:
With the below apache configuration(See the trailing slash at the end
of '/svn/').

Location /svn/
  DAV svn
  SVNParentPath /repositories
#See the trailing slash on the master URI also can cause the confusion.
  SVNMasterURI http://master/svn/
  SVNAdvertiseV2Protocol Off
/Location

We get the following error on the client side.

svn: Commit failed (details follow):
svn: MKACTIVITY of
'/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could
not
read status line: connection was closed by server (http://localhost)

Hi Kamesh,


Sorry for the late response Paul.

Are there any threads or IRC logs in which this is discussed
(particularly a more detailed replication)?

No. Paul this error caught my eyes while testing something locally.


While reviewing r916286
and r917512 I tried to replicate this problem by adding a trailing '/'
to the location and SVNMasterURI.  I *did* get an error, just a
different one:


C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
st
M   file.txt


C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
ci -m commit to slave
svn: Commit failed (details follow):
svn: OPTIONS of 'http://localhost/svn-test-work/slave': 200 OK
(http://localhost)




What's worse is that I get this error with a server at the latest
trunk (r1104523), trunk right after you fixed this bug (r917512), and
my own local attempt to backport this branch to 1.6.x (addressing the
conflicts and the use of 1.7 APIs).  They all fail the same way.

I could not see this error(I mean everything works as expected with
Location /svn/  and SVNMasterURI witj trailing '/') with against
trunk@1126459

Hi Kamesh,

Hmmm, I get yet another error if the Location and SVNMasterURI have a
trailing '/':

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slave-1.7-r1126459svn
st
M   file.txt

slave-1.7-r1126459svn ci -m Commit to proxy with trailing / in
Location and SVNMasterURI
..\..\..\subversion\svn\commit-cmd.c:190: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:846: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:846: (apr_err=175002)
svn: E175002: Commit failed (details follow):
..\..\..\subversion\libsvn_client\commit.c:644: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:644: (apr_err=175002)
..\..\..\subversion\libsvn_client\ra.c:356: (apr_err=175002)
..\..\..\subversion\libsvn_ra\ra_loader.c:496: (apr_err=175002)
..\..\..\subversion\libsvn_ra\ra_loader.c:496: (apr_err=175002)
svn: E175002: Unable to connect to a repository at URL
'http://localhost/svn-test-work/slave'
..\..\..\subversion\libsvn_ra_neon\util.c:1556: (apr_err=175002)
..\..\..\subversion\libsvn_ra_neon\util.c:1152: (apr_err=175002)
svn: E175002: The OPTIONS request returned invalid XML in the
response: XML parse error at line 1: no element found
(http://localhost/svn-test-work/slave)



May be you suffer from issue 3445

Without the trailing '/' this commit succeeds as expected.



But issue 3445 should be present irrespective of '/' slash.

I built trunk@916285(prior to my fix) and saw this error and with
trunk@916286 this error has gone away.

May be something to do with win32 build.

I have not used the write-through proxy functionality before so maybe
I botched something with my configuration?  I attached my httpd.conf
in the event something problematic jumps out at you. Though, as I
said, everything works fine without the trailing '/'.



Ok.. I will copy you snippets in my environment to see if you caught 
some lurking bugs.

Can you attach your patch against r916285, I will build it/test it and let
you know what is wrong?

I don't have a patch against trunk@916285, I don't have any patch
against trunk at all.  The only patch I mentioned is for the backport
of r916286 and r917512 against 1.6.x.  Is this what you meant?



Sorry, I was wrong, I meant your patch against 1.6.x branch as one of my 
colleague has win32 svn build setup for 1.6.x I can build the binaries 
to see if it is peculiar to win32.


With regards
Kamesh Jayachandran



Paul

Re: r916286


On 05/23/2011 09:09 PM, Paul Burba wrote:

On Mon, May 23, 2011 at 10:13 AM, Kamesh Jayachandrankam...@collab.net  wrote:

On 05/18/2011 02:04 AM, Paul Burba wrote:

Author: kameshj
Date: Thu Feb 25 13:40:22 2010
New Revision: 916286

URL: http://svn.apache.org/viewvc?rev=916286view=rev
Log:
With the below apache configuration(See the trailing slash at the end
of '/svn/').

Location /svn/
  DAV svn
  SVNParentPath /repositories
#See the trailing slash on the master URI also can cause the confusion.
  SVNMasterURI http://master/svn/
  SVNAdvertiseV2Protocol Off
/Location

We get the following error on the client side.

svn: Commit failed (details follow):
svn: MKACTIVITY of
'/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could
not
read status line: connection was closed by server (http://localhost)

Hi Kamesh,


Sorry for the late response Paul.

Are there any threads or IRC logs in which this is discussed
(particularly a more detailed replication)?

No. Paul this error caught my eyes while testing something locally.


While reviewing r916286
and r917512 I tried to replicate this problem by adding a trailing '/'
to the location and SVNMasterURI.  I *did* get an error, just a
different one:


C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
st
M   file.txt


C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slavesvn
ci -m commit to slave
svn: Commit failed (details follow):
svn: OPTIONS of 'http://localhost/svn-test-work/slave': 200 OK
(http://localhost)




What's worse is that I get this error with a server at the latest
trunk (r1104523), trunk right after you fixed this bug (r917512), and
my own local attempt to backport this branch to 1.6.x (addressing the
conflicts and the use of 1.7 APIs).  They all fail the same way.

I could not see this error(I mean everything works as expected with
Location /svn/  and SVNMasterURI witj trailing '/') with against
trunk@1126459

Hi Kamesh,

Hmmm, I get yet another error if the Location and SVNMasterURI have a
trailing '/':

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slave-1.7-r1126459svn
st
M   file.txt

slave-1.7-r1126459svn ci -m Commit to proxy with trailing / in
Location and SVNMasterURI
..\..\..\subversion\svn\commit-cmd.c:190: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:846: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:846: (apr_err=175002)
svn: E175002: Commit failed (details follow):
..\..\..\subversion\libsvn_client\commit.c:644: (apr_err=175002)
..\..\..\subversion\libsvn_client\commit.c:644: (apr_err=175002)
..\..\..\subversion\libsvn_client\ra.c:356: (apr_err=175002)
..\..\..\subversion\libsvn_ra\ra_loader.c:496: (apr_err=175002)
..\..\..\subversion\libsvn_ra\ra_loader.c:496: (apr_err=175002)
svn: E175002: Unable to connect to a repository at URL
'http://localhost/svn-test-work/slave'
..\..\..\subversion\libsvn_ra_neon\util.c:1556: (apr_err=175002)
..\..\..\subversion\libsvn_ra_neon\util.c:1152: (apr_err=175002)
svn: E175002: The OPTIONS request returned invalid XML in the
response: XML parse error at line 1: no element found
(http://localhost/svn-test-work/slave)

Without the trailing '/' this commit succeeds as expected.


I built trunk@916285(prior to my fix) and saw this error and with
trunk@916286 this error has gone away.

May be something to do with win32 build.

I have not used the write-through proxy functionality before so maybe
I botched something with my configuration?  I attached my httpd.conf
in the event something problematic jumps out at you. Though, as I
said, everything works fine without the trailing '/'.


Can you attach your patch against r916285, I will build it/test it and let
you know what is wrong?

I don't have a patch against trunk@916285, I don't have any patch
against trunk at all.  The only patch I mentioned is for the backport
of r916286 and r917512 against 1.6.x.  Is this what you meant?

Paul
Yes I could reproduce your original error even while doing a checkout 
using my distro 1.6 client binaries against the following configuration.



This issue has nothing to do with write through proxy as it happens even 
while checking out.


Location /svn-test-work/slave/
   DAV svn
   SVNPath /repositories/thanu
   SVNMasterURI http://127.0.0.1:18080/svn-test-work/master/
   AuthType Basic
   AuthName Subversion
   AuthBasicProvider csvn-file-users
   AuthzSVNAccessFile conf.d/svn_access_file
   SVNPathAuthz short_circuit
   Require valid-user
/Location

This error do not occur in the following configurations.

* when SVNParentPath is used instead of SVNPath

* When Location do not have trailing slash.

snip of error
[kamesh@kamesh tmp]$ svn co http://127.0.0.1/svn-test-work/slave
svn: OPTIONS of 'http://127.0.0.1/svn-test-work/slave': 200 OK 
(http://127.0.0.1)

/snip

With regards
Kamesh Jayachandran

Re: [PATCH] - Remove unneeded code from svnadmin

2011-05-21 Thread Kamesh Jayachandran


On 05/20/2011 10:28 PM, Mark Phippard wrote:

Log message and patch attached.

svnadmin create accepts the --config-dir option but no longer uses it
for anything.  I do not believe we can remove the option from the UI,
but we can remove the code that get the configuration.  The reason is
that I have observed a couple of odd scenarios where this code causes
problems.

1) The process running svnadmin does not have read authority to
$HOME/.subversion  For example, here is an example from a mailing
list:

http://lists.okfn.org/pipermail/kforge-user/2007-February/88.html

2) Have observed similar problem on Windows when the process is
running and it could not determine the home folder.  Never figured out
the root cause of this problem, as normally this does work OK on
Windows.

The workaround people have used is to specify --config-dir and point
it at ' ' or some random folder they can read.  But if we just removed
this code they would not get a problem in the first place.

Ran make check and manually tested the binary.





-  SVN_ERR(svn_config_get_config(config, opt_state-config_dir, pool));
-  SVN_ERR(svn_cmdline__apply_config_options(config, opt_state-config_options,
-svnadmin: , --config-option));
   SVN_ERR(svn_repos_create(repos, opt_state-repository_path,
NULL, NULL,
config, fs_config, pool));



You can as well remove the config parameter and pass NULL instead.

Prabhu posted similar patch 4 months back

http://svn.haxx.se/dev/archive-2011-01/0157.shtml

My +1 for this change.

With regards
Kamesh Jayachandran

RE: [PATCH] Fix for issue3870 - File descriptor leaks during svnsync

2011-05-13 Thread Kamesh Jayachandran

Committed it in 1102690.

-Original Message-
From: Kamesh Jayachandran [mailto:kam...@collab.net]
Sent: Thu 5/12/2011 10:38 PM
To: dev@subversion.apache.org
Subject: [PATCH] Fix for issue3870 - File descriptor leaks during svnsync

Hi All,

Attached patch fixes the File descriptor leaks in ra_serf's way of 
driving the destination delta editor with *LONG* living pools.

This patch fixes it.

I think similar stuff needs to be done for a case of directory having 
lots of subdirectories.

All tests pass over ra_serf... I am just posting here just in case 
somebody has some opinions.

If there are no objections I will commit tomorrow my time.

With regards
Kamesh Jayachandran

Re: svn commit: r1102690 - in /subversion/trunk/subversion: libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py

2011-05-13 Thread Kamesh Jayachandran

On 05/13/2011 07:51 PM, Bert Huijben wrote:

-Original Message-
From: kame...@apache.org [mailto:kame...@apache.org]
Sent: vrijdag 13 mei 2011 14:22
To: comm...@subversion.apache.org
Subject: svn commit: r1102690 - in /subversion/trunk/subversion:
libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py

Author: kameshj
Date: Fri May 13 12:22:15 2011
New Revision: 1102690

URL: http://svn.apache.org/viewvc?rev=1102690view=rev
Log:
Fix for issue3870 File descriptor leaks during svnsync.

Before this fix, all of destination delta editor's interfaces
are called with *LONG* living pool(dst_rev_pool living for one full revision).

This makes it a memory bloat and bloat of other OS resources like
file descriptors to live as long the dst_rev_pool.

I'm pretty sure this fixes this issue, but shouldn't the file pool live inside 
a revision pool? (Or maybe in a directory pool, that lives in, ... etc. etc.)

That is how it is in neon.

Current State of serf's *state* handling do not have room for that 
hierarchy or revision-directory-file.

I thought to implement similar hierarchy as a part of this patch.
But having closely looked at the neon's code with respect to the file 
handling portions of editor drive, I felt simple file_pool that gets 
cleared upon before starting a new file seemed a simpler approach.

Anyways as I stated in the original mail... Similar problems could 
remain for directories having huge number of sub directories anyway 
I will write a testcase and fix on a permanent problem.

Without looking at the code I would have expected that serf might operate on 
multiple files at once. (But I assume our test suite would have caught that by 
now)

Bert

As per delta editor contract... one can not open two files at once on 
the same editor drive, so no issue.

With regards
Kamesh Jayachandran

RE: svn commit: r1102690 - in /subversion/trunk/subversion: libsvn_ra_serf/replay.c tests/cmdline/svnsync_tests.py

2011-05-13 Thread Kamesh Jayachandran


[For the record, I'm not saying your code is wrong -- I didn't even look at
it.  I'm only correcting your statement about simultaneous open files.]

Because transmitting file contents is generally considered the most costly
part of describing a tree delta, the editor interface allows you to describe
all the tree changes and property changes before transmitting any file
content changes.  (This allows editor implementations to detect
out-of-dateness and conflicts before the file content changes are
transmitted).  So, for example, the following is valid:

   # transmit tree structure changes
   d1 = open_root()
 d2 = open_dir('A')
   f1 = open_file('A/mu')
   d3 = open_dir('A/B')
 f2 = open_file('A/B/alpha')
 f3 = open_file('A/B/beta')
   close_dir(d3)
 close_dir(d2)
   close_dir(d1)

   # transmit post-fix text deltas
   apply_textdeltas(f1)
   close_file(f1)
   apply_textdeltas(f2)
   close_file(f2)
   apply_textdeltas(f3)
   close_file(f3)

   # complete the editor drive
   close_edit()

Naturally, in such situations, you don't want to open your file batons in
pools that will be destroyed when their parent directory baton's pool is
also destroyed.  You need instead for file baton's to have lifetimes that's
about as long as the whole editor drive.

Thanks Mike for explaining this. Then such a long living pool would cause 
similar leak!!!

With regards
Kamesh Jayachandran

Re: [PATCH] Add a test in dav-mirror-autocheck.sh to showcase a out-of-date slave related bug

2011-04-21 Thread Kamesh Jayachandran


Thanks, Committed this patch in r1095662.

With regards
Kamesh Jayachandran
On 04/18/2011 04:05 PM, Arwin Arni wrote:

Hi All,

This patch adds a test case to 
subversion/tests/cmdline/dav-mirror-autocheck.sh to showcase the 
following bug.


In a master/slave repositories setup where writes are proxied to the 
master and reads are handles by the slave repository, if the slave 
repository is behind the master by one or more revisions, certain 
operations that one would expect to succeed are failing.



Example:

$svn cp -m Create branch ^/trunk ^/branches/newbranch (from a 
working copy of the slave)


will FAIL if the master has an unsynced commit (not yet synced with 
slave) anywhere below ^/branches.


i.e Coder 1 does a commit to branches/some-other-branch (master has 
received the commit, slave hasn't yet synced)

Coder 2 tries to create a new branch via above command. It fails.

I'll shortly file an issue in the tigris tracker for this.

Regards,
Arwin Arni

Re: [PATCH] Add a test in dav-mirror-autocheck.sh to showcase a out-of-date slave related bug

2011-04-19 Thread Kamesh Jayachandran


On 04/19/2011 07:44 AM, Mark Phippard wrote:

On Mon, Apr 18, 2011 at 7:41 AM, Daniel Shahafd...@daniel.shahaf.name  wrote:

On Mon, 18 Apr 2011 07:08 -0400, Mark Phippardmarkp...@gmail.com  wrote:

I know why it fails, but I would not expect it to fail as a user, even
with a proxy.  I did not look at Arwin's test but it does not require
a WC to show the failure.  This also fails:

$ svn mkdir url://branches/branch1
$ svn mkdir url://branches/branch2

Because in the second mkdir, the slave's idea of HEAD is behind master's and 
user's idea of HEAD.

As I said, I know why it fails.  That does not mean we should not
identify these problems and look for ways to solve them.  If proxy was
a first class feature it would know you were running a command where
the all of the HTTP requests should be proxied and this would not
fail.


I agree it would be nice if this worked, but given that we have to remain sane 
to people who open an RA session to the slave before it has
synced up I'm afraid it might be tricky to address.  (That corresponds to the 
case of concurrent commits --- two people attempting Mark's two
mkdirs concurrently.)

Separately: we might want to teach the wc that one repos_url (out of several 
with the same repos_uuid) is preferred in given circumstances...
  then we could have '^mirror/' and '^master/' syntaxes./random-thoughts

To reiterate, there is no WC involved here.  So a change like that is
not needed.  This problem exists with commands that are run on URL's
alone.  As you know most of our SVN commands involved many HTTP
requests and the problem here is that some of them are handled by the
proxy and others are proxied to the master.  A solution would likely
require the proxy to have more awareness of the SVN command being
executed so that it knew to proxy all of the requests back to the
master rather than handle any of them off the local replica.


FWIW at [1] I originally sent a patch which would provide a RA API to 
set a request header 'SVN-ACTION' indicating high level svn operation.


[1] http://svn.haxx.se/dev/archive-2010-01/0101.shtml

With regards
Kamesh Jayachandran

Re: [PATCH] Remove redundant url-encoding added in r917523

2011-04-04 Thread Kamesh Jayachandran

Thanks Vijay for the detailed summary and the fix.

I committed it at r1088602.

With regards
Kamesh Jayachandran
On 04/04/2011 12:00 PM, vijay wrote:

On Thursday 24 March 2011 07:41 PM, Kamesh Jayachandran wrote:

On 03/24/2011 05:54 PM, vijay wrote:

Just now I came to know that it fails in neon only.

I configured neon as a default ra_layer in my runtime configuration
area.

When I use serf as ra_layer, the commit succeeds.

Yes that answers the failure I observed.

Please provide a testcase that exhibits the same failure for both
neon and serf.

The testcase exhibits the following behaviour when tested with neon
and serf.

RA_LAYER
HTTP V2
pre-HTTP V2
Neon
Fail
Fail
Serf
Pass
Fail

There are two options to make the testcase fail for both neon and serf.

1.Disabling SVNAdvertiseV2Protocol by making it explicitly off in
the slave server's Location directive in Apache configuration.

2. Passing the command line option --config-option
servers:global:http-library=neon to svnmucc for the particular commit
operation as it fails in neon consistently.

I prefer to the second option(using neon for the particular commit)
instead of disabling HTTP V2 in server side which may affect other
testcases.

Meanwhile analysing why it succeeds in serf would teach something
interesting.

First, let me tell why it fails in neon.

As in the test case, the slave repo is hosted in 127.0.0.1 and master
repo is hosted in 127.0.0.2

1. For the particular commit which has space in its path, the CHECKOUT
request from client reaches the slave repo.

CHECKOUT /repo/!svn/ver/5/branch%20new HTTP/1.1
User-Agent: SVN/1.7.0-dev (under development) neon/0.29.3

2. The request from the client is proxied by the slave to the master
and the Master sends the response with Location header as follows.

Header Name: [location], Value:
[http://127.0.0.2:30679/repo//!svn/wrk/69179a87-38cb-43c4-945c-de1e0b297aad/branch%20new]

3. The slave encodes the location header again(r917523) and forward it
to the client.

Header Name: [location], Value:
[http://127.0.0.1:26248/repo//!svn/wrk/5b604aca-a4b1-41fc-87ae-51299bf565b2/branch%2520new]

The above location header is used in subsequent dav request which
fails as it has double-encoding of the space.

Here, PUT request fails as follows.

PUT
/repo/!svn/wrk/5b604aca-a4b1-41fc-87ae-51299bf565b2/branch%2520new/file HTTP/1.1

svnmucc: E160013: File not found: transaction '5-5', path
'/branch%20new/file'

But why didn't it fail in serf?

Serf has the implementations of HTTP V2 stuffs which does not use
CHECKOUT request during commit.

It processes as follows.

OPTIONS - POST - PROPFIND - PROPPATCH - HEAD - PUT - MERGE - DELETE

It gets the transaction id from POST request and directly put the
contents there.

But I could see the same failure while committing with serf when I
disable SVNAdvertiseV2Protocol , because it uses the CHECKOUT
request there.

I think we can use neon for the particular commit operation instead of
disabling HTTP V2 wholly in server side.

Please correct me if I am wrong.

Anyway we can enhance these tests further once we started to implement
make davmirrorautocheck which I am going to take as my next activity.

Attaching the patch and log message.

Thanks Regards,
Vijayaguru

With regards
Kamesh Jayachandran

Thanks Regards,
vijayaguru

On Thursday 24 March 2011 05:12 PM, Kamesh Jayachandran wrote:

Thanks for the Patch Vijay.

In my FC14 your testcase passes both with and without patch.

I am investigating

Will get back.

With regards
Kamesh Jayachandran
On 03/24/2011 04:11 PM, vijay wrote:

Hi,

This patch adds a testcase for the regression triggered by r917523
and fixes it.

The revision r917523 do some url-encodings to the paths and uris
in subversion/mod_dav_svn/mirror.c.

snip
$svn log -r917523

r917523 | kameshj | 2010-03-01 19:18:01 +0530 (Mon, 01 Mar 2010) |
26 lines

With the below apache configuration(See the space character
/svn 1/).

Location /svn 1/
DAV svn
SVNParentPath /repositories
/Location
Location /svn 2/
DAV svn
SVNParentPath /repositories-slave
SVNMasterURI http://localhost/svn 1
/Location

Write through proxy is *not* happening and commit happens
*directly* inside the slave.

* subversion/mod_dav_svn/mirror.c
(proxy_request_fixup): URI encode the to be proxied file name.
(dav_svn__proxy_request_fixup): r-unparsed_uri is in url
encoded form while
root_dir is not in encoded form. So use r-uri to compare with
root_dir.
(dav_svn__location_in_filter): URL Encode the 'find replace'
urls as

the request body has it in url encoded format.
(dav_svn__location_header_filter): Encode the master_uri as the
response from
master has the Location header url encoded already. Set the
outgoing Location

header url encoded.
(dav_svn__location_body_filter

Re: [PATCH] Remove redundant url-encoding added in r917523

2011-03-24 Thread Kamesh Jayachandran


On 03/24/2011 05:54 PM, vijay wrote:

Just now I came to know that it fails in neon only.

I configured neon as a default ra_layer in my runtime configuration area.

When I use serf as ra_layer, the commit succeeds.


Yes that answers the failure I observed.

Please provide a testcase that exhibits the same failure for both neon 
and serf.


Meanwhile analysing why it succeeds in serf would teach something 
interesting.


With regards
Kamesh Jayachandran


Thanks  Regards,
vijayaguru

On Thursday 24 March 2011 05:12 PM, Kamesh Jayachandran wrote:

Thanks for the Patch Vijay.

In my FC14 your testcase passes both with and without patch.

I am investigating

Will get back.

With regards
Kamesh Jayachandran
On 03/24/2011 04:11 PM, vijay wrote:

Hi,

This patch adds a testcase for the regression triggered by r917523 
and fixes it.


The revision r917523 do some url-encodings to the paths and uris in 
subversion/mod_dav_svn/mirror.c.


snip
$svn log -r917523
 

r917523 | kameshj | 2010-03-01 19:18:01 +0530 (Mon, 01 Mar 2010) | 
26 lines


With the below apache configuration(See the space character /svn 
1/).


Location /svn 1/
  DAV svn
  SVNParentPath /repositories
/Location
Location /svn 2/
  DAV svn
  SVNParentPath /repositories-slave
  SVNMasterURI http://localhost/svn 1
/Location

Write through proxy is *not* happening and commit happens *directly* 
inside the slave.


* subversion/mod_dav_svn/mirror.c
  (proxy_request_fixup): URI encode the to be proxied file name.
  (dav_svn__proxy_request_fixup): r-unparsed_uri is in url encoded 
form while
root_dir is not in encoded form. So use r-uri to compare with 
root_dir.
  (dav_svn__location_in_filter): URL Encode the 'find  replace' 
urls as

the request body has it in url encoded format.
  (dav_svn__location_header_filter): Encode the master_uri as the 
response from
master has the Location header url encoded already. Set the 
outgoing Location

header url encoded.
  (dav_svn__location_body_filter): URL Encode the 'find  replace' 
urls as

the response body has it in url encoded format.

 


/snip

There is one extra url encoding in 
mirror.c:dav_svn__location_header_filter() which encodes the 
Location header response from master second time, which in turn 
causes failure while committing a path to slave repo which has space 
in it. The space in the path is encoded as %20 first time, again 
it is encoded as %2520, which fails with error File not found 
while committing.


I have added a testcase for this issue in 
subversion/tests/cmdline/dav-mirror-autocheck.sh. I have just used 
the existing master and slave repo setup for my new test case.


I have a plan of introducing davmirrorautocheck which we can 
execute like make davmirrorautocheck, it will run all our python 
test suites via write-through proxy. All commits to the slave will 
be proxied to the master repo. The master repo's post commit hook 
will sync every commit to the slave repo. We can check whether slave 
and master repo are in sync in 'run_and_verify_commit'.
We can extensively test our write-through proxy by making use of all 
our existing tests.


Attaching the patch and log message.

Thanks  Regards,
Vijayaguru

Re: svn commit: r1081390 - /subversion/trunk/subversion/tests/cmdline/merge_tests.py

2011-03-15 Thread Kamesh Jayachandran


On 03/14/2011 08:18 PM, Stefan Sperling wrote:

On Mon, Mar 14, 2011 at 02:24:58PM -, kame...@apache.org wrote:

Author: kameshj
Date: Mon Mar 14 14:24:58 2011
New Revision: 1081390

URL: http://svn.apache.org/viewvc?rev=1081390view=rev
Log:
Adds an XFail test to catch regression created by r1075802

* subversion/tests/cmdline/merge_tests.py
   (dry_run_merge_conflicting_binary): New XFail testcase.
   (test_list): Add dry_run_merge_conflicting_binary.

Patch by: Arwin Arniarwin{_AT_}collab.net

Modified:
 subversion/trunk/subversion/tests/cmdline/merge_tests.py
+@XFail()
+def dry_run_merge_conflicting_binary(sbox):
+  dry run shouldn't make conflict resoln files

^
typo?


Thanks Stefan. Fixed in r1081703.

With regards
Kamesh Jayachandran

Re: [PATCH] Add a test to cover the regression introduced in r1075802

2011-03-14 Thread Kamesh Jayachandran


On 03/14/2011 06:11 PM, Arwin Arni wrote:

On Monday 14 March 2011 11:40 AM, Arwin Arni wrote:

On Monday 14 March 2011 02:47 AM, Daniel Becroft wrote:
On Mon, Mar 14, 2011 at 7:06 AM, Daniel Becroft 
djcbecr...@gmail.com mailto:djcbecr...@gmail.com wrote:


On Sat, Mar 12, 2011 at 1:50 AM, C. Michael Pilato
cmpil...@collab.net mailto:cmpil...@collab.net wrote:

On 03/11/2011 10:03 AM, Arwin Arni wrote:
 Index: ../subversion/tests/cmdline/merge_tests.py


===
 --- ../subversion/tests/cmdline/merge_tests.py   
(revision 1080126)
 +++ ../subversion/tests/cmdline/merge_tests.py   
(working copy)

 @@ -16586,6 +16586,102 @@
if not os.access(beta_path, os.X_OK):
  raise svntest.Failure(beta is not marked as executable
after commit)

 +@XFail()
 +def dry_run_merge_conflicting_binary(sbox):
 +  dry run merge should not create conflict resolution files

This long description line triggers the AssertionError about
the test
docstring needing to be 50 characters or less.

 +  svntest.actions.run_and_verify_merge(other_wc, '2', '3',
 +   sbox.repo_url, None,
 +   expected_output,
 +  
expected_mergeinfo_output,

 +   expected_elision_output,
 +   expected_disk,
 +   expected_status,
 +   expected_skip,
 +   None, None, None,
None, None,
 +   True, True,
'--allow-mixed-revisions',
 +   other_wc)

As this is a test of a dry-run merge, I find the use of
run_and_verify_merge() a bit obfuscating.  I think it'd be
better to
explicitly run a --dry-run merge so that it's obvious that
what you're
testing is exactly that.

And, as I said elsethread, the patch didn't even apply to
HEAD.  So that
needs to be reworked.


Hi Mike,

One of the advantages in using run_and_verify_merge() is that if
dry_run = TRUE, it does it's own check to ensure that the working
copy is not modified. IMO, this is better than explicitly building
the tree prior to the merge, and then re-checking the merge.

However, I'm finding that running an explicit merge works, but
running run_and_verify_merge() does not (conflict files still get
created).


Never mind, I just found the problem. Using run_and_verify_merge() 
with dry_run = True runs both a dry-run and a wet-run update. Since 
the wet-run update always gets run, the conflict files always get 
created.


So the test-case is fine right.. I mean.. The dry_run checks are 
independent of the wet run.. They happen before the wet run happens..


Is there anything else that we need to add to this test?

I've attached an updated patch that should now apply without hiccups..

Regards,
Arwin Arni
I'm sorry I overlooked the docstring length again.. Here's the 
corrected patch.


Regards,
Arwin Arni


Thanks Arwin. Committed your patch in r1081390.

With regards
Kamesh Jayachandran

RE: [PATCH] Add --dry-run flag to svn update client command

2011-03-14 Thread Kamesh Jayachandran

 
Kamesh Jayachandran wrote on Fri, Mar 11, 2011 at 18:12:21 +0530:
 Unless there are no objection I will commit this patch post I
 receive a r1075802 regression fix(and testcase) from either Daniel
 Becroft or Arwin.

Hold on, are you suggesting to commit the two thousand, six hundred and
sixty-three line diff that started this thread?


Yes the same patch, I would hold on. 

I would be more than happy to hear technical feedback about this patch or 
*best* alternative way to implement the same.

With regards
Kamesh Jayachandran

Re: [PATCH] Add --dry-run flag to svn update client command

2011-03-11 Thread Kamesh Jayachandran


Hi All,

I reviewed the patch and I am fine with it.

We did not hear any technical objections for this patch so far.

Bert said we can implement it via separate dry_run editor but it would 
end up copying the code from subversion/libsvn_wc/update_editor.c as we 
need access to wc.


Anyways we have a similar implementation(if not exact) for merge 
--dry-run, I mean *no* dry_run_merge_editor.


One good thing about this patch it helped Arwin to find a regression 
caused by a commit(r1075802) which our testsuite could not find.



My +1 for this patch for the following reasons.

* It is a straightforward way to do dry-run(I remember thrice I have 
been asked by CVS converts for equivalent of cvs update -n).


* I understand the patch fully.

* It exercises our code for more test-coverage(As exhibited by r1075802)


Unless there are no objection I will commit this patch post I receive a 
r1075802 regression fix(and testcase) from either Daniel Becroft or Arwin.


With regards
Kamesh Jayachandran




On 03/02/2011 09:00 PM, Arwin Arni wrote:

Hi All,

Thanks for all the feedback.

As Stefan said, yes, this patch was an immense learning experience for 
me and I wouldn't trade it for anything else.


All said and done, I hope this is not the end of this patch. It 
implements exactly what the issue tracker describes and I think it 
would be an excellent feature enhancement. So, I would really 
appreciate it if this patch gets a closer look.


If anybody can throw light on how this can be done any differently 
(not some combination of existing commands, but a working update 
--dry-run), I would gladly lend my ears.


Thanks and regards,

Arwin Arni

Re: svn commit: r1080198 - in /subversion/trunk: subversion/svnadmin/main.c tools/client-side/svnmucc/svnmucc.c

2011-03-10 Thread Kamesh Jayachandran


On 03/10/2011 05:53 PM, rhuij...@apache.org wrote:

Author: rhuijben
Date: Thu Mar 10 12:23:09 2011
New Revision: 1080198

URL: http://svn.apache.org/viewvc?rev=1080198view=rev
Log:
Implement --config-option in the last two commandline tools that did support
--config-dir, but didn't support --config-option.


Not sure why we need --config-option for svnadmin?

With regards
Kamesh Jayachandran

Re: [PATCH] Add --dry-run flag to svn update client command

2011-03-02 Thread Kamesh Jayachandran


On 03/02/2011 07:47 PM, Daniel Shahaf wrote:

Bert Huijben wrote on Wed, Mar 02, 2011 at 11:14:24 +0100:

-Original Message-
From: Arwin Arni [mailto:ar...@collab.net]
Please review this and share your thoughts.

I don't think this is the way we should implement this.

Which is precisely why one should contact the mailing list BEFORE
writing a patch.  Especially a large/long patch such as this one.


Daniel,

Let me share *my* concern here in proposing a idea to any *new* community.

I would be happy to propose an idea where I lack direction and have a 
ambiguity in implementation.


I would be happy to post a patch if idea is simpler enough which is the 
case here.


Yes one can declare in advance what they wish to work on, But this 
declaration has a negative side effect of stigma if the idea is not 
complete in implementation especially this stigma is too much for a 
newcomer.


As a newcomer I would post a working patch than start a discussion(of 
course only if the idea is straightforward) which is often open-ended 
and confuses/discourages the new-comer if he is *not* that serious about 
the feature he proposes.



With regards
Kamesh Jayachandran

Re: [PATCH] Compiling subversion trunk with httpd trunk code fails

2011-03-02 Thread Kamesh Jayachandran


Thanks Vijay.

Committed your in r1076234.

With regards
Kamesh Jayachandran

On 03/02/2011 03:18 PM, vijay wrote:
Attached the patch that uses macro based solution. APLOG_USE_MODULE is 
used only in case of HTTPD 2.3.


Thanks  Regards,
Vijayaguru

On Wednesday 02 March 2011 12:20 AM, Stefan Sperling wrote:

On Tue, Mar 01, 2011 at 11:08:22PM +0530, Kamesh Jayachandran wrote:

On the whole I preferred the macro solution.

Me to prefer the orignal macro based solution.

OK. Let's go for that, then.

RE: [PATCH] Compiling subversion trunk with httpd trunk code fails

2011-03-01 Thread Kamesh Jayachandran


No! If you are going to use separate functions then write:

static void log_access_verdict(...)
{

It still needs a signature change to log_access_verdict.

#if AP_MODULE_MAGIC_AT_LEAST(...)
   log_access_verdict_httpd_v23(...)
#else
   log_access_verdict_httpd_v22(...)
#endif
}

so that the callers don't have to change.

On the whole I preferred the macro solution.

Me to prefer the orignal macro based solution.

With regards
Kamesh Jayachandran

Re: [PATCH] Fix failing expected error in blame_tests.py

2011-02-17 Thread Kamesh Jayachandran


On 02/17/2011 08:16 PM, Noorul Islam K M wrote:

Noorul Islam K Mnoo...@collab.net  writes:


Log
[[[

Fix failing expected error regex.

* subversion/tests/cmdline/blame_tests.py
   (blame_non_existent_url_target): Relax regex to allow errors from
http: and svn: protocols.

Patch by: Noorul Islam K Mnoorul{_AT_}collab.net
]]]

Index: subversion/tests/cmdline/blame_tests.py
===
--- subversion/tests/cmdline/blame_tests.py (revision 1071613)
+++ subversion/tests/cmdline/blame_tests.py (working copy)
@@ -759,8 +759,7 @@
   2jrandom New contents for iota\n,
  ]

-  expected_err = svn: warning: W160017: '/non-existent'  +  \
-  is not a file in revision 2\n + \
+  expected_err = svn: warning: W160017: .*\n +  \
.*\nsvn: E29: Could not perform blame on all targets  + \
because some targets don't exist\n


Do we need full error message in the expected error?

With regards
Kamesh Jayachandran

expected_err_re = re.compile(expected_err)

Stefen,

Please find attached modified patch.

  Log
  [[[

  Fix failing expected error regex. Also capture ra_neon error.

  * subversion/svn/blame-cmd.c
(svn_cl__blame): Catch SVN_ERR_FS_NOT_FOUND and display warning.

  * subversion/tests/cmdline/blame_tests.py
(blame_non_existent_url_target): Relax regex to allow errors from
 http: and svn: protocols.

  Patch by: Noorul Islam K Mnoorul{_AT_}collab.net
  ]]]

Re: [PATCH] Fix for Issue #3781 (Case Sensitive Authz)

2011-02-11 Thread Kamesh Jayachandran


On 02/10/2011 01:15 PM, Arwin Arni wrote:

On Thursday 10 February 2011 12:48 PM, Arwin Arni wrote:

On Wednesday 09 February 2011 09:21 PM, Arwin Arni wrote:

Hi All,

Here's the patch for the implementation of the logic.

Regards,
Arwin Arni

Sorry, Please ignore the previous patch. There was a stray caller of 
svn_config_create (whose signature I changed and didn't rev as it was 
new in the 1.7 API.). Here's the correct patch.


Regards,
Arwin Arni
I missed adding the documentation for the deviation in behavior for 
SVN_CONFIG__DEFAULT_SECTION. So here is a patch with the documentation.

This is the last correction. I promise.



Thanks Arwin. Committed at r1069791.

With regards
Kamesh Jayachandran


Regards,
Arwin Arni

Re: [PATCH] Fix for Issue #3781 (Case Sensitive Authz)

2011-02-11 Thread Kamesh Jayachandran


On 02/11/2011 07:49 PM, Arwin Arni wrote:

On Friday 11 February 2011 07:06 PM, Kamesh Jayachandran wrote:


Thanks Arwin. Committed at r1069791.

With regards
Kamesh Jayachandran



Here is the follow-up patch that fixes the deprecated calls of 
svn_config_read.


Regards,
Arwin Arni

Thanks Arwin committed in r1069821.

With regards
Kamesh Jayachandran

RE: svn commit: r1068411 - /subversion/trunk/subversion/tests/cmdline/authz_tests.py

2011-02-11 Thread Kamesh Jayachandran

 Bad idea to hard code so much of the specific message we're generating
 today; it's bound to change later.
 
 (eg, the XML parsing thing would ideally go away at some point)
 
 Just 'Access.*${mu_url}.*forbidden' is enough IMO.
 
 
  Thanks We fixed this in r1068802 Still we have E170001: for svnserve 
  would
  remove it as soon as I reach before my working copy.

Thanks.  But why remove the E170001?  Its very purpose is to provide
a machine-readable identification of the error... (exactly what tests
need)


In r1068802 We did the following,

-  expected_error_for_cat = svn: E175013: Unable to connect to a repository+ \
- at URL ' + mu_url + '\n + \
-svn: E175013: Access to '/ + mu_repo_path + \
-' forbidden\n + \
-svn: E175009: XML parsing failed: (403 Forbidden)
 
+  if sbox.repo_url.startswith(http):
+expected_error_for_cat = .*[Ff]orbidden.*
+  else:
+expected_error_for_cat = .*svn: E170001: Authorization failed.*
+

Do you mean to reinstate errorcodes(E175013, E175013, E175009) check ?

With regards
Kamesh Jayachandran

RE: [PATCH] New XFail test for issue 3781

2011-02-09 Thread Kamesh Jayachandran

Thanks Prabhu committed your patch in r1068802.


With regards
Kamesh Jayachandran
-Original Message-
From: Prabhu Gnana Sundar Ponnarasu
Sent: Wed 2/9/2011 2:30 PM
To: Bert Huijben
Cc: Kamesh Jayachandran; dev@subversion.apache.org
Subject: Re: [PATCH] New XFail test for issue 3781
 
Hi Bert,

The problem was that my patch handled the expected error more 
specifically and also failed to handle the svnserve case. That was wrong 
on my part. I have tweaked the test case and attached a patch and log 
message with this mail.
Please share your views on the same.


On Tuesday 08 February 2011 09:08 PM, Bert Huijben wrote:
 This test XFails for the wrong reason on svn://.

 EXPECTED STDERR (regexp):
 svn: E175013: Unable to connect to a repository at URL
 'svn://localhost/svn-test-work/repositories/authz_tests-20/A/mu'
 svn: E175013: Access to '/svn-test-work\repositories\authz_tests-20/A/mu'
 forbidden
 svn: E175009: XML parsing failed: (403 Forbidden)
 ACTUAL STDERR:
 ..\..\..\subversion\svn\cat-cmd.c:81: (apr_err=170001)
 ..\..\..\subversion\svn\util.c:967: (apr_err=170001)
 ..\..\..\subversion\libsvn_client\cat.c:233: (apr_err=170001)
 ..\..\..\subversion\libsvn_ra_svn\client.c:1557: (apr_err=170001)
 ..\..\..\subversion\libsvn_ra_svn\client.c:242: (apr_err=170001)
 ..\..\..\subversion\svnserve\serve.c:174: (apr_err=170001)
 svn: E170001: Authorization failed
 EXCEPTION: SVNUnmatchedError

 See http://subversion.apache.org/buildbot/ or more specifically
 http://ci.apache.org/builders/svn-slik-w2k3-x64-ra/builds/1675/steps/Test%20
 fsfs%2Bsvn/logs/testlog

   Bert



Thanks and regards
Prabhu

RE: [PATCH] New XFail test for issue 3781

2011-02-08 Thread Kamesh Jayachandran

Prabhu,


Please send the patch against the current HEAD.

With regards
Kamesh Jayachandran

-Original Message-
From: Prabhu Gnana Sundar Ponnarasu
Sent: Tue 2/8/2011 1:11 PM
To: Kamesh Jayachandran
Cc: dev@subversion.apache.org
Subject: [PATCH] New XFail test for issue 3781
 
Hi Kamesh,

I have created a new thread in order to prevent the confusion about the 
test case. I have tweaked the test case as you mentioned in the previous 
thread.

Here is the link for the same...
http://mail-archives.apache.org/mod_mbox/subversion-dev/201102.mbox/%3c0213965108dead48960ce83455e07dff0192b...@maa-exchmb.maa.corp.collab.net%3E
Thank you for your valuable suggestions.

I hope this test case would make things a bit more clear.
I have attached the patch and the log message with this mail. Please 
share your views.



Thanks and regards
Prabhu

RE: [PATCH] New XFail test for issue 3781

2011-02-08 Thread Kamesh Jayachandran


Thanks Prabhu.

I committed with the following tweaks in r1068411.

1. Added @XFail(), @Issue(3781) decorators
2.

+  # test the case-sensitivity of the repo name
+  write_authz_file(sbox, {},
+   sections = {mixed_case_repo_dir + :/: jrandom = r,
+   mixed_case_repo_dir + :/A: jrandom = r,
+   sbox.repo_dir + :/A/mu: jrandom =,
+   mixed_case_repo_dir + :/A/mu: jrandom = rw})

Replaced this snippet by the following snippet. 
+  # test the case-sensitivity of the repo name
+  sec_mixed_case = {mixed_case_repo_dir + :/: jrandom = r,
+mixed_case_repo_dir + :/A: jrandom = r,
+os.path.basename(sbox.repo_dir) + :/A/mu: jrandom =,
+mixed_case_repo_dir + :/A/mu: jrandom = rw}
+  write_authz_file(sbox, {}, sec_mixed_case)


You can reduce this to simpler one as most of the time same 'cat' and 'commit' 
is tested with one 'for' loop
which iterates over [(rule1, section1), (rule2, section2)...]



With regards
Kamesh Jayachandran
-Original Message-
From: Prabhu Gnana Sundar Ponnarasu
Sent: Tue 2/8/2011 6:23 PM
To: Kamesh Jayachandran
Cc: dev@subversion.apache.org
Subject: Re: [PATCH] New XFail test for issue 3781
 

Hi,

Sorry for posting an older patch. Now attached the correct patch.
Please share your views.


Thanks and regards
Prabhu

On Tuesday 08 February 2011 04:41 PM, Kamesh Jayachandran wrote:

 Prabhu,


 Please send the patch against the current HEAD.

 With regards
 Kamesh Jayachandran

Re: [PATCH] Fix for Issue #3781 repo prefix rules in authz section is checked case sensitively for commit


On 01/20/2011 11:38 AM, Arwin Arni wrote:

On Wednesday 19 January 2011 09:46 PM, C. Michael Pilato wrote:

On 01/19/2011 10:52 AM, Arwin Arni wrote:

Hi All,

Authz section names(and paths too) are parsed/loaded in a case 
insensitive way.


So they need to be compared in a case-insensitive way.

Following functions do authz,

1. libsvn_repos/authz.c:authz_get_path_access()
2. libsvn_repos/authz.c:authz_get_any_access() is called when the 
path is

NULL (for MKACTIVITY, DELETE).

'1' is leaving it to svn_config_enumerate2() to handle case 
(in)sensitiveness.

'2' is explicitly comparing each config item in a case sensitive way.

My patch is just fixing the '2' to check it in a case insensitive way.


So, just to be clear, now read and write access checks will both 
compare the

repository name in a case-insensitive way?


That's Correct.

Index: subversion/libsvn_repos/authz.c
===
--- subversion/libsvn_repos/authz.c(revision 1060836)
+++ subversion/libsvn_repos/authz.c(working copy)
@@ -398,7 +398,7 @@

/* Does the section apply to the query? */
if (section_name[0] == '/'
-  || strncmp(section_name, b-repos_path,
+  || strncasecmp(section_name, b-repos_path,
   strlen(b-repos_path)) == 0)


You've goofed up the indentation here.

Again.. extremely sorry.. I guess I'm still not completely accustomed 
to the indentation conventions.. Here's a patch with the indentation 
fixed..




Thanks Arwin. Committed the patch in r1064093.

With regards
Kamesh Jayachandran

Regards,
Arwin Arni

RE: svn commit: r1064093 -/subversion/trunk/subversion/libsvn_repos/authz.c


 It might be a rare situation,
 but *if* folks have come to depend on the case-sensitivity of these checks,
 they need to prepare for the fallout of this loosening of the policy.
 

Sorry for not thinking about this earlier, but:
Why was the fix to make all checks case-INsensitive, as opposed to
making all checks case-sensitive?  

We use svn_config_read to parse the authz which always stores the section names 
in *lower* case in its internal hash table.

See subversion/libsvn_subr/config.c:make_hash_key().

We need to rev svn_config_read to accept bool case_sensitive_section_parser. 
Then we can change the behaviour to always case-sensitive approach as against 
today's case-insensitive approach.

With regards
Kamesh Jayachandran

RE: svn commit: r1064093-/subversion/trunk/subversion/libsvn_repos/authz.c

-Original Message-
From: Daniel Shahaf [mailto:d...@daniel.shahaf.name]
Sent: Fri 1/28/2011 12:16 AM
To: Kamesh Jayachandran; Arwin Arni Nandagopal
Cc: dev@subversion.apache.org
Subject: Re: svn commit: 
r1064093-/subversion/trunk/subversion/libsvn_repos/authz.c

Kamesh Jayachandran wrote on Fri, Jan 28, 2011 at 00:14:35 +0530:

  It might be a rare situation,
  but *if* folks have come to depend on the case-sensitivity of these 
  checks,
  they need to prepare for the fallout of this loosening of the policy.

 Sorry for not thinking about this earlier, but:
 Why was the fix to make all checks case-INsensitive, as opposed to
 making all checks case-sensitive?  

 We use svn_config_read to parse the authz which always stores the section 
 names in *lower* case in its internal hash table.

 See subversion/libsvn_subr/config.c:make_hash_key().

 We need to rev svn_config_read to accept bool case_sensitive_section_parser. 
 Then we can change the behaviour to always case-sensitive approach as 
 against today's case-insensitive approach.

Thanks for the reminder.

Do you think it's a good idea?

Personally I'm +0.5, since IMO we should be treating pathnames
case-insensitively.

Same case with me.

With regards
Kamesh Jayachandran

Re: svn commit: r1064168 - in /subversion/trunk/subversion/include/private: svn_eol_private.h svn_fs_util.h svn_mergeinfo_private.h svn_opt_private.h svn_sqlite.h svn_wc_private.h

On Thu, Jan 27, 2011 at 9:28 PM, style...@apache.org wrote:

 Author: stylesen
 Date: Thu Jan 27 15:58:21 2011
 New Revision: 1064168

 URL: http://svn.apache.org/viewvc?rev=1064168view=rev
 Log:
 * subversion/include/private/svn_wc_private.h
 * subversion/include/private/svn_fs_util.h
 * subversion/include/private/svn_mergeinfo_private.h
 * subversion/include/private/svn_eol_private.h
 * subversion/include/private/svn_opt_private.h
 * subversion/include/private/svn_sqlite.h
  Fix typos in comments.


Hi Senthil,


   /* Return the first eol marker found in [@a buf, @a endp) as a
 - * NUL-terminated string, or NULL if no eol marker is found.


NUL is not a typo I guess, NUL means '\0', While NULL is (void *) 0x.


With regards
Kamesh Jayachandran

Re: Any idea why public function like svn_fspathdirname have double in its name?

2011-01-05 Thread Kamesh Jayachandran




With regards
Kamesh Jayachandran

[PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos
too instead of svn_path_* wherever applicable and hence a chance to
become public.

I don't quite understand what you mean here.



I meant subversion/libsvn_repos/commit.c:delete_entry() uses deprecated 
svn_path_join which can be made to use svn_fspath__join.


If my guess is true, we can as well make svn_fspath__join as 
svn_fspath_join and make it public as it is used by 2 modules(libsvn_fs 
and libsvn_repos).


With regards
Kamesh Jayachandran

Re: [PATCH] tweaked svnrdump to handle junk values with --version

2011-01-03 Thread Kamesh Jayachandran


Hi Prabhu,

I committed this patch in r1054583.

Thanks

With regards
Kamesh Jayachandran
On 01/01/2011 03:07 AM, Gavin Beau Baumanis wrote:

Ping.
This patch submission has received no comments.


On 17/12/2010, at 5:46 PM, Prabhu Gnana Sundar wrote:


Hi,

In my previous patch, I just tweaked svnrdump to display the help
documentation properly.

This patch would solve the problems that I mentioned in my earlier
patch, such as

1. displaying a better header for svnrdump --vresion

2. svnrdump --version should *not* accept junk values, like svnrdump
--version --junk.

I have the patch and the log message attached with this mail.
Please review and comment.



Thanks and regards
Prabhu
svnrdump.logsvnrdump.patch

RE: svn commit: r1054229 - /subversion/trunk/subversion/libsvn_subr/opt.c

2011-01-01 Thread Kamesh Jayachandran

-  SVN_ERR(svn_cmdline_fputs(_(Copyright (C) 2010 The Apache Software 
Foundation.\n
+  SVN_ERR(svn_cmdline_fputs(_(Copyright (C) 2011 The Apache Software 
Foundation.\n

Won't this be 2010-2011?

With regards
Kamesh Jayachandran

Re: svn upgrade segfaults


Arwin,

Sorry, I guess my description can be reduced to even simple form.

I mean we do not need *failed commit* condition, just scheduled change 
alone is enough.


Can you provide a new patch which covers the following case?

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
svn-1.7 upgrade


With regards
Kamesh Jayachandran
On 12/31/2010 02:58 PM, Arwin Arni wrote:

On Thursday 30 December 2010 09:35 PM, Kamesh Jayachandran wrote:

Hi All,

I did the following.

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
#Make sure below commit fails either by a pre-commit hook or File 
system perm error, we just need this to fail.


svn-1.6 ci -m msg

svn-1.7 upgrade
Segfaults

My svn-1.7 trunk build corresponds to 1053813:1053833

With regards
Kamesh Jayachandran

Hi Kamesh,

I've written an XFail test case which reproduces this Seg Fault in 
upgrade_tests.py


I've attached the patch + log message and also a .tar.bz2 file to be 
added to

subversion/tests/cmdline/upgrade_tests_data/

Regards,
Arwin Arni

Re: svn upgrade segfaults


Thanks Arwin. Committed your patch at r1054090.

With regards
Kamesh Jayachandran
On 12/31/2010 04:31 PM, Arwin Arni wrote:

Hi Kamesh,

Here is the new patch and log message and the .tar.bz2 file.

Regards,
Arwin Arni

Arwin,

Sorry, I guess my description can be reduced to even simple form.

I mean we do not need *failed commit* condition, just scheduled 
change alone is enough.


Can you provide a new patch which covers the following case?

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
svn-1.7 upgrade


With regards
Kamesh Jayachandran
On 12/31/2010 02:58 PM, Arwin Arni wrote:

On Thursday 30 December 2010 09:35 PM, Kamesh Jayachandran wrote:

Hi All,

I did the following.

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
#Make sure below commit fails either by a pre-commit hook or File 
system perm error, we just need this to fail.


svn-1.6 ci -m msg

svn-1.7 upgrade
Segfaults

My svn-1.7 trunk build corresponds to 1053813:1053833

With regards
Kamesh Jayachandran

Hi Kamesh,

I've written an XFail test case which reproduces this Seg Fault in 
upgrade_tests.py


I've attached the patch + log message and also a .tar.bz2 file to be 
added to

subversion/tests/cmdline/upgrade_tests_data/

Regards,
Arwin Arni

Re: [PATCH] Make error messages visible to translator


Vijay,

I removed the server side(related portions from libsvn_subr) of this 
change from this patch and committed the rest in r1054115.


With regards
Kamesh Jayachandran


On 12/29/2010 12:28 PM, vijay wrote:

Hi,

Attaching the patch that prefixes an underscore with the error 
messages to make it visible to translators.


I could see the error messages in mod_dav_svn are not prefixed by an 
underscore, may be the subsequent layers will handle the errors and 
translate it?


Thanks  Regards,
Vijayaguru

Re: svn upgrade segfaults


Committed the fix in r1054133.

With regards
Kamesh Jayachandran
On 12/30/2010 09:35 PM, Kamesh Jayachandran wrote:

Hi All,

I did the following.

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
#Make sure below commit fails either by a pre-commit hook or File 
system perm error, we just need this to fail.


svn-1.6 ci -m msg

svn-1.7 upgrade
Segfaults

My svn-1.7 trunk build corresponds to 1053813:1053833

With regards
Kamesh Jayachandran

RE: svn commit: r1054168 - /subversion/trunk/subversion/libsvn_ra_serf/win32_auth_sspi.c




URL: http://svn.apache.org/viewvc?rev=1054168view=rev
Log:
* subversion/libsvn_ra_serf/win32_auth_sspi.c
  (sspi_maxtokensize): Add a paren missing from r1054115.

Thanks for fixing. Sorry for *not* taking note of it.

With regards
Kamesh Jayachandran

svn upgrade segfaults

2010-12-30 Thread Kamesh Jayachandran


Hi All,

I did the following.

cd /tmp
cd svn-1.6 co file:///repo/abc
touch abc/test
svn-1.6 add abc/test
#Make sure below commit fails either by a pre-commit hook or File system 
perm error, we just need this to fail.


svn-1.6 ci -m msg

svn-1.7 upgrade
Segfaults

My svn-1.7 trunk build corresponds to 1053813:1053833

With regards
Kamesh Jayachandran

RE: Any idea why public function like svn_fspathdirname have double in its name?

2010-12-29 Thread Kamesh Jayachandran

 Now I'm really mystified. When I added those location-tracing functions
 and macros, they were only ever enabled with SVN_DEBUG turned on (i.e.,
 in maintainer-mode). Now I see that those #ifdef wrappers are gone, and
 can't recall an explanation as to why that's a good thing.

 Can anyone remember what that change was good for?

Found it, r843793 and I agree with the change. So, effectively,
svn_error__locate has been public since then, and r1053469 should be
reverted anyway because it breaks the ABI.

Not sure why r1053469 need to be reverted. IIUC it still keeps the 
svn_error__locate by changing it to NOOP. Which I think is enough to keep ABI.


With regards
Kamesh Jayachandran

RE: Any idea why public function like svn_fspathdirname have double in its name?

2010-12-29 Thread Kamesh Jayachandran

 While we've mandated that __ must be used for semi-public
 functions, we've never said that it can't be used for private ones.

Kamesh is talking about public, not private, functions.  I.e., ones
where we actually do have an ABI promise to keep.

I looked at svn_error__locate last week.  It's really only useful in
--enable-maintainer-mode, but the way it's implemented, it ends up
being used by macros in the public (non-maintainer) ABI, so even if we
eliminate those callers, we have to supply the function itself forever.
I've addressed this as best we can, in r1053469.

Thanks

Supplying a given ABI forever is the sort of thing I thought we didn't
have to do for __ functions.  I think that's Kamesh's question too.

Yes. Following is my understanding, correct me if I am wrong.

my understanding
Any function/macro inside the header *directly* under subversion/include should 
only have a public functions(ones that starts with svn_ and do *not* have __). 

If somebody differs from this convention then there should be *subversion dev* 
community independent convention here may be like these.

* doc string of such functions(semi-public a.k.a intra library 
functions/private(should we even have such one in headers?)) should promptly 
say so in the headers.

* start with __svn like we see in some of the libc headers. The vague __ 
starting would signal about the scope/ABI nature of the API.

/my understanding

With regards
Kamesh Jayachandran

[PS] Unless I am mistaken svn_fspath__* can be used in libsvn_repos too instead 
of svn_path_* wherever applicable and hence a chance to become public.

Any idea why public function like svn_fspathdirname have double in its name?

2010-12-28 Thread Kamesh Jayachandran


Hi All,

I could see the following functions in our public header with '__' in 
their names,


svn_fspath__is_canonical
svn_fspath__dirname
svn_fspath__basename
svn_fspath__split
svn_fspath__join
svn_fspath__is_child
svn_fspath__skip_ancestor
svn_fspath__is_ancestor
svn_fspath__get_longest_ancestor
svn_error__locate

svn_error__malfunction

IIUC __ convention is used for semi-public functions where we may not 
keep BC promise.


Do I miss something?

With regards
Kamesh Jayachandran

Re: [PATCH] fixes some path related deprecation warnings

2010-12-28 Thread Kamesh Jayachandran


Prabhu

--- subversion/libsvn_repos/commit.c(revision 1053267)
+++ subversion/libsvn_repos/commit.c(working copy)
@@ -231,7 +231,7 @@
   svn_node_kind_t kind;
   svn_revnum_t cr_rev;
   svn_repos_authz_access_t required = svn_authz_write;
-  const char *full_path = svn_path_join(eb-base_path, path, pool);

+  const char *full_path = svn_dirent_join(eb-base_path, path, pool);


I think you need to use svn_fspath__join

Same in other parts of this patch.

Because eb-basepath is a path inside fs as per the below snipped comment.

snip
  /* Location in fs where the edit will begin. */
  const char *base_path;
/snip

With regards
Kamesh Jayachandran

RE: [PATCH] Fix some deprecation warnings

2010-12-24 Thread Kamesh Jayachandran

Thanks Vijay. 

Committed in r1052547.

With regards
Kamesh Jayachandran


-Original Message-
From: Vijayaguru Guruchave
Sent: Fri 12/24/2010 9:42 PM
To: dev@subversion.apache.org
Cc: Daniel Shahaf; Kamesh Jayachandran
Subject: Re: [PATCH] Fix some deprecation warnings
 
On Friday 24 December 2010 03:46 PM, vijay wrote:
 On Wednesday 22 December 2010 06:42 PM, Daniel Shahaf wrote:
 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)

 I used iterpool in place of scratch pool and moved the iterpool 
 destruction down. There are two failures in log_tests.py:7  9 with 
 the following error_trace.

 snip
 CMD: /home/vijayaguru/svn-sandbox/svn-trunk/vpath/subversion/svn/svn 
 log svn-test-work/working_copies/log_tests-7/A/B/E/b...@8 --config-dir 
 /home/vijayaguru/svn-sandbox/svn-trunk/vpath/subversion/tests/cmdline/svn-test-work/local_tmp/config
  
 --password rayjandom --no-auth-cache --username jrandom exited with 1
 TIME = 0.034555
 ../subversion/svn/log-cmd.c:743: (apr_err=22)
 ../subversion/libsvn_client/log.c:486: (apr_err=22)
 ../subversion/libsvn_client/ra.c:482: (apr_err=22)
 ../subversion/libsvn_client/url.c:53: (apr_err=22)
 ../subversion/libsvn_subr/dirent_uri.c:1667: (apr_err=22)
 ../subversion/libsvn_subr/utf.c:837: (apr_err=22)
 ../subversion/libsvn_subr/utf.c:604: (apr_err=22)
 svn: Valid UTF-8 data
 (hex:)
 followed by invalid UTF-8 sequence
 (hex: e0 65 30 00)
 Traceback (most recent call last):
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/main.py,
  
 line 1212, in run
 rc = self.pred.run(sandbox)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/testcase.py,
  
 line 170, in run
 return self.func(sandbox)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/log_tests.py,
  
 line 762, in log_wc_with_peg_revision
 'log', my_path)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/actions.py,
  
 line 264, in run_and_verify_svn
 expected_exit, *varargs)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/actions.py,
  
 line 302, in run_and_verify_svn2
 exit_code, out, err = main.run_svn(want_err, *varargs)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/main.py,
  
 line 580, in run_svn
 *(_with_auth(_with_config_dir(varargs
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/main.py,
  
 line 338, in run_command
 None, *varargs)
   File 
 /home/vijayaguru/svn-sandbox/svn-trunk/subversion/tests/cmdline/svntest/main.py,
  
 line 513, in run_command_stdin
 raise Failure
 Failure
 FAIL:  log_tests.py 7: 'svn log wc_tar...@n'
 snip

 The following function 'svn_uri_condense_targets()' stores its return 
 value of 'url_or_path' in scratch pool(here iterpool) which should not 
 be the case.

 SVN_ERR(svn_uri_condense_targets(url_or_path, condensed_targets,
  target_urls, TRUE, pool, iterpool));

 We have to copy the return value from scratch pool to result pool 
 before exiting from the function, right?
 I will send a patch to fix it in dirent_uri.c: 
 svn_uri_condense_targets().

 Thanks  Regards,
 vijayaguru

The above mentioned bug is fixed in r1052505. Attaching the updated 
patch and log message.

Thanks  Regards,
Vijayaguru
 +  SVN_ERR(svn_uri_condense_targets(url_or_path,condensed_targets,
 +   target_urls, TRUE, pool, 
 pool));




 With regards
 Kamesh Jayachandran

Re: [PATCH] Fix some deprecation warnings

2010-12-22 Thread 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

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

2010-12-21 Thread Kamesh Jayachandran


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=1051745view=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=1051745r1=1051744r2=1051745view=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.

Thanks
With regards
Kamesh Jayachandran

Re: [Question] Svn log show code diff

2010-12-19 Thread Kamesh Jayachandran


Hi Zhang,

This feature is already available via  --diff   : 
produce diff output option log command in upcoming svn 1.7.



With regards
Kamesh Jayachandran

On 12/20/2010 08:47 AM, Zhang, Wei-Jovi (NSN - CN/Hangzhou) wrote:

Hi,
I have a question about svn log command, every time when I use
svn log command, I'm very upset that I need to input svn diff to check
changesets using revision number.
Why not combine svn log and diff command together? Or is there
have any script for this purpose? Like this:



r229 | w3zhang | 2010-12-15 10:48:20 +0800 (Wed, 15 Dec 2010) | 1 line
Changed paths:
M /trunk/SS_MGWSemiPermCtrl/src/semipermanent_heathcheck.sh

Index: semipermanent_heathcheck.sh
===
--- semipermanent_heathcheck.sh (revision 228)
+++ semipermanent_heathcheck.sh (revision 229)
@@ -75,6 +75,7 @@
  if [ $RET_STATUS !=  ]; then
  echo -e add tdm-tdm crct1:\t$CRCT1-$TSL \tcrct2:\t$CRCT2-$TSL
\t[FAILED]
  CRCT_CREATE_FAILED=$CRCT1-$TSL $CRCT2-$TSL
+echo -e command: fsclish -c \add tdm semipermanent-connection
tdm-tdm crct1 $CRCT1-$TSL crct2 $CRCT2-$TSL\
  echo -e $RET_STATUS
  break
  else
semipermanent_heathcheck: echo failed command, used for re-executed
later


r227 | w3zhang | 2010-12-14 16:59:23 +0800 (Tue, 14 Dec 2010) | 1 line
Changed paths:
M /trunk/SS_MGWSemiPermCtrl/src/semper_h01.sdl
M /trunk/SS_MGWSemiPermCtrl/src/semper_h02.sdl
M /trunk/SS_MGWSemiPermCtrl/src/semper_leg.sdl
M /trunk/SS_MGWSemiPermCtrl/src/semper_m01.sdl
M /trunk/SS_MGWSemiPermCtrl/src/semper_m02.sdl
M /trunk/SS_MGWSemiPermCtrl/src/semper_mas.sdl
M /trunk/SS_MGWSemiPermCtrl/src/z00lib.sdl

using script to cleanup TNSDL coding style

Index: semipermanent_heathcheck.sh
===
--- semipermanent_heathcheck.sh (revision 228)
+++ semipermanent_heathcheck.sh (revision 229)
@@ -93,6 +94,7 @@
  if [ $RET_STATUS !=  ]; then
  echo -e delete tdm-tdm crct1:\t$CRCT1-$TSL
\tcrct2:\t$CRCT2-$TSL \t[FAILED]
  CRCT_DELETE_FAILED=$CRCT_DELETE_FAILED $CRCT1-$TSL
$CRCT2-$TSL
+echo -e command: fsclish -c \delete tdm
semipermanent-connection tdm-tdm crct $CRCT1-$TSL\
  echo -e $RET_STATUS
  else
  echo -e delete tdm-tdm crct1:\t$CRCT1-$TSL \tcrct2:
\t$CRCT2-$TSL \t[OK]



Thanks for your time.

Re: [PATCH] enhance diff to make it behave uniformly

2010-12-08 Thread Kamesh Jayachandran


On 12/08/2010 09:21 PM, Julian Foad wrote:

On Sat, 2010-12-04, Kamesh Jayachandran wrote:

I understand your patch fixes the following two cases.

1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
Hi Kamesh and Prabhu.
What you're describing here sounds good - it sounds like a simpler and
more definite change than what I understood before - but I'm not sure
precisely what explicitly_reinstated_file_with_mod_at_rN means.

I mean the following,

[...]

Thanks, Kamesh - that clarifies it.  So that's the case where a file is
deleted and then a pre-deletion revision of it is copied back to the
same path where it existed before.

In my experiments I find the same problem also exists when a file is
copied to a new path from a revision older than the value of HEAD at the
time of the copy.  For example:

$ cd wc

$ echo line1  test.c

$ svn add test.c
A test.c

$ svn ci -m  test.c
Adding test.c
Transmitting file data .
Committed revision 1.

$ svn mkdir ^/foo -m An unrelated change

Committed revision 2.

$ svn cp test.c new.c
A new.c

$ echo line2  new.c

$ svn ci -m  new.c
Adding new.c
Transmitting file data .
Committed revision 3.

$ svn diff -c3 new.c
svn: Unable to find repository location for 'new.c' in revision 2

$ svn diff -c3
Index: new.c
===
--- new.c (revision 0)
+++ new.c (revision 3)
@@ -0,0 +1,2 @@
+line1
+line2



Please could you include a test for these cases in your patch?  Then it
will be absolutely clear.

Prabhu already has one. Right now he is working on removing the
profileration of UI param diff-copy-from from all the layer in favour
of generic send_copyfrom_args.

That's great.  It would be good to include the above test scenario as
well.

Thanks.
- Julian


Sure he would. Right now he is teaching the 'svn_wc_get_diff_editor6' 
what he has taught for svn_client__get_diff_editor.


He will have tests for that too.

With regards
Kamesh Jayachandran

Re: 1.5.9 up for testing/signing

2010-12-03 Thread Kamesh Jayachandran


On 12/03/2010 09:04 PM, Mark Phippard wrote:

On Fri, Dec 3, 2010 at 10:28 AM, Kamesh Jayachandrankam...@collab.net  wrote:

Forgive my ignorance Paul. IIUC in 1.5.x we can only have neon or serf not
both. Did you do 2 builds to test this?

I think you are confusing it with 1.4.x.  We added support in 1.5.x
for supporting both:

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

Thanks Mark, Yes I got confused with 1.4.x(To add to my confusion, my 
1.5.9 build install was showing only serf it did not take the neon from 
subversion-deps-1.5.9-bundle).


With regards
Kamesh Jayachandran

RE: Will send_copyfrom_args be reverted for 'ra' layers too just like in issue #3711

2010-11-24 Thread Kamesh Jayachandran


AFAIK svn diff always prints a diff against the copy source if the
items being diffed are source and target of a copy. How is what you
intend to do different from the default behaviour?

What Prabhu's '--diff-copy-from' do is 'get the modification to file *alone* 
done in a same commit as copy'.

Without this switch it would show all the lines as 'added' with this switch it 
would show only 'the modified lines'.

While debugging some issue I could see 'svnlook diff --diff-copy-from' doing 
exactly the same and asked Prabhu to do the same.

With regards
Kamesh Jayachandran

There is a new option in 1.7 called --show-copies-as-adds which
changes this behaviour such that copied files are always shown as
fully added.

Stefan

Re: [PATCH] Fix for issue 3609 - cat command

2010-11-09 Thread Kamesh Jayachandran


On 11/08/2010 11:40 AM, Noorul Islam K M wrote:

Followup to r1030010

Log

[[[

Canonicalize paths before passing them to svn_client_cat2.

* subversion/svn/cat-cmd.c
   (svn_cl__cat): Canonicalize url or dirent before passing over to API.

* subversion/tests/cmdline/cat_tests.py
   (cat_url_special_characters, test_list): New test

Patch by: Noorul Islam K Mnoorul{_AT_}collab.net

]]]

Thanks and Regards
Noorul



Thanks for the patch, Committed in r1033028.

With regards
Kamesh Jayachandran

Re: Fix for issue 3609 - ls command

2010-11-09 Thread Kamesh Jayachandran


On 11/08/2010 11:59 AM, Noorul Islam K M wrote:

Followup to r1030010

Log

[[[

Canonicalize paths before passing them to svn_client_list2.

* subversion/svn/list-cmd.c
   (svn_cl__list): Canonicalize url or dirent before passing over to API.

* subversion/tests/cmdline/basic_tests.py
   (ls_url_special_characters, test_list): New test

Patch by: Noorul Islam K Mnoorul{_AT_}collab.net

]]]

Thanks Noorul. I tweaked for a cosmetic consistency and committed in 
r1033045.


With regards
Kamesh Jayachandran

Re: [PATCH] mailer.py : Use python difflib.unified_diff instead of *nix diff command

2010-11-08 Thread Kamesh Jayachandran


On 11/05/2010 09:45 PM, Noorul Islam K M wrote:

[[[

If diff command does not exist on the system then use
difflib.unified_diff() to generate diff contents.

* tools/hook-scripts/mailer/mailer.py: Fall back to
   difflib.unified_diff() if the script fails to execute native 'diff'
   command.

Patch by: Noorul Islam K Mnoorul{_AT_}collab.net
Suggested by: Kamesh Jayachandrankamesh{_AT_}collab.net

]]]

Thanks for the patch Noorul.

I could not *see* the difference in the output. I tried with few file 
mod commit mails both via external diff and difflib.


I committed this patch with small refactoring at r1032568.

With regards
Kamesh Jayachandran

Re: [PATCH] To improve the documentation of svn_path.h

2010-11-08 Thread Kamesh Jayachandran




[[[
Improved the documentation of deprecated functions by providing links
to the deprecating functions.

* subversion/include/svn_path.h
  (svn_path_internal_style, svn_path_local_style,
   svn_path_join, svn_path_join_many,
   svn_path_basename, svn_path_dirname,
   svn_path_split, svn_path_canonicalize,
   svn_path_is_canonical, svn_path_get_longest_ancestor,
   svn_path_get_absolute, svn_path_split,
   svn_path_condense_targets, svn_path_is_child,
   svn_path_is_ancestor, svn_path_split_if_file,


Thanks Arwin for the patch 'svn_path_split_if_file' is deprectated but 
svn_{uri|dirent|relpath}_split do not seem like its deprecating 
function. So removed that portion from your patch.


Committed the patch at r1032610.

With regards
Kamesh Jayachandran



   svn_path_url_add_component)

Re: [PATCH] Avoid allocating memory to extract basename using svn_relpath_basename

2010-10-04 Thread Kamesh Jayachandran


On 10/04/2010 05:52 PM, vijayaguru wrote:

Hi,

   From svn_relpath_basename documentation, if pool is NULL, it simply
returns a pointer to the string without allocating additional memory.
Pass NULL pointer to svn_relpath_basename wherever the returned basename
need not be persistent. It could save few bytes of memory.


[[[
   Log:
   Pass NULL pointer to svn_relpath_basename wherever the returned
   basename need not be allocated in pool.

   * subversion/mod_dav_svn/reports/update.c
 (absent_helper, upd_delete_entry)
   * subversion/libsvn_wc/update_editor.c
 (delete_entry)
   * subversion/svnlook/main.c
 (print_tree)
   * subversion/libsvn_client/repos_diff.c
 (absent_directory)
   * subversion/libsvn_ra_neon/commit.c
 (commit_delete_entry)
   * subversion/libsvn_ra_serf/commit.c
 (checkout_dir)
   * subversion/libsvn_repos/node_tree.c
 (add_open_helper)

 Pass NULL pointer to svn_relpath_basename wherever the returned
 basename need not be persistent.

Patch by: Vijayaguru Gvijay_at_collab.net
Suggested by: Kameshj

]]]
   


Thanks committed in r1004306.

With regards
Kamesh Jayachandran

Thanks  Regards,
Vijayaguru

Re: svn commit: r981665 - in /subversion/branches/performance/subversion: include/private/svn_file_handle_cache.h include/private/svn_temp_serializer.h libsvn_subr/svn_file_handle_cache.c libsvn_subr/

2010-08-03 Thread Kamesh Jayachandran

(svn_temp_serializer__context_t *context);
+svn__temp_serializer__get(svn__temp_serializer__context_t *context);

   


having '__' in two places in an identifier looks bit odd.



  /**
   * Deserialization is straightforward: just copy the serialized buffer to
@@ -117,4 +118,4 @@ svn_temp_serializer__get(svn_temp_serial
   * the pointer to resolve in @a ptr.
   */
  void
-svn_temp_deserializer__resolve(void *buffer, void **ptr);
+svn__temp_deserializer__resolve(void *buffer, void **ptr);
   


having '__' in two places in an identifier looks bit odd.


Modified: 
subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c
URL: 
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c?rev=981665r1=981664r2=981665view=diff
==
--- 
subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c 
(original)
+++ 
subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c 
Mon Aug  2 19:36:59 2010
@@ -1,5 +1,5 @@
  /*
- * svn_file_handle_cache.c: open file handle caching for Subversion
+ * svn__file_handle_cache.c: open file handle caching for Subversion
   *
   


I guess it is a find and replace error. I mean file name still remains same.

Thanks
With regards
Kamesh Jayachandran

Re: [PATCH] new feature, lazy sync via export --skipfilesmatchingsize


Hi Ann,

Patch has not reached the list.

Did you forget attach it?

With regards
Kamesh Jayachandran

On 07/26/2010 08:34 AM, svnusert...@href.com wrote:

[[[
Contributed Feature: enable lazy sync via new export flag, 
--skipfilesmatchingsize


   * subversion/libsvn_client/export.c
   * subversion/svn/cl.h
   * subversion/svn/export-cmd.c
   * subversion/svn/main.c

Summary: This feature is meant to provide an extremely easy, 80% 
effective way for
Windows users to avoid using rsync when they want to export files 
repeatedly from a
subversion repository.  Although many experienced svn users are happy 
using rsync,
Ann believes that enough Windows users would benefit from this 
feature, and that it

makes sense to add --skipfilesmatchingsize as a flag.

Discussed in part here: 
http://svn.haxx.se/users/archive-2010-07/0282.shtml


Patch by: Paul Breen greenbr...@yahoo.com
Suggested by: Ann Lynnworth svnusert...@href.com

]]]

The attached files are patches against subversion 1.6.12.

We modified one further file which you probably do NOT need
( * subversion/libsvn_subr/opt.c ).  Our change modifies the output of 
the svn --version command
to say that this version is modified from the Collabnet version.  In 
case you want to see that
for some reason, it is here: 
http://greenbreen.com/svn_mod_source/libsvn_subr/opt.c.patch


This is Paul's first patch to svn and Ann's first attempt at 
contributing.  We tried to follow
all the rules and hope that, if for some reason, this patch is 
unusable, you let us know how
to improve it.  Please copy me off-list because I am not subscribed to 
the dev list.


Thank you for your consideration.

Ann

Re: svn commit: r979094 - /subversion/trunk/subversion/svnrdump/svnrdump.c


This breaks the 2 tests in svnrdump_tests.


Though the following inline patch may fix it you may have different plans

Index: subversion/tests/cmdline/svnrdump_tests.py
===
--- subversion/tests/cmdline/svnrdump_tests.py  (revision 979178)
+++ subversion/tests/cmdline/svnrdump_tests.py  (working copy)
@@ -73,7 +73,8 @@
   svntest.actions.run_and_verify_load(sbox.repo_dir, svnadmin_dumpfile)

   # Create a dump file using svnrdump
-  r, svnrdump_dumpfile, err = svntest.main.run_svnrdump('-q', 
sbox.repo_url)

+  r, svnrdump_dumpfile, err = svntest.main.run_svnrdump(dump, '-q',
+sbox.repo_url)

   # Check error code
   if (r != 0):
@@ -94,7 +95,7 @@
   dump the standard sbox repos
   sbox.build(read_only = True, create_wc = False)

-  r, out, err = svntest.main.run_svnrdump(sbox.repo_url)
+  r, out, err = svntest.main.run_svnrdump(dump, sbox.repo_url)

   if (r != 0):
 raise svntest.Failure('Result code not 0')

With regards
Kamesh Jayachandran
On 07/26/2010 12:51 AM, artag...@apache.org wrote:

Author: artagnon
Date: Sun Jul 25 19:21:02 2010
New Revision: 979094

URL: http://svn.apache.org/viewvc?rev=979094view=rev
Log:
svnrdump: Add support for subcommands

* subversion/svnrdump/svnrdump.c

   (svnrdump__cmd_table): A table of subcommands along with related
   information.

   (opt_baton_t): New struct for storing options to pass around and
   dispatching subcommands.

   (help, help_cmd): Replace the help function with a new help_cmd
   function, effectively making help a subcommand.
   (replay_range, replay_revisions): Rename function.

   (dump_cmd, load_cmd): New functions to extract arguments from the
   apr_getopt_t and opt_baton and dispatch calls to the corresponding
   functions that do the real work. dump_cmd eventually calls
   replay_revisions, while load_cmd is currently a noop.

   (main): Replace local variables with fields in opt_baton where
   appropriate. Add logic for reading and dispatching a
   subcommand. Track changes made to other functions.

Modified:
 subversion/trunk/subversion/svnrdump/svnrdump.c

Modified: subversion/trunk/subversion/svnrdump/svnrdump.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/svnrdump.c?rev=979094r1=979093r2=979094view=diff
==
--- subversion/trunk/subversion/svnrdump/svnrdump.c (original)
+++ subversion/trunk/subversion/svnrdump/svnrdump.c Sun Jul 25 19:21:02 2010
@@ -36,6 +36,8 @@

  #include dump_editor.h

+static svn_opt_subcommand_t dump_cmd, load_cmd;
+
  enum svn_svnrdump__longopt_t
{
  opt_config_dir = SVN_OPT_FIRST_LONGOPT_ID,
@@ -46,6 +48,28 @@ enum svn_svnrdump__longopt_t
  opt_version,
};

+static const svn_opt_subcommand_desc2_t svnrdump__cmd_table[] =
+  {
+{ dump, dump_cmd, { d },
+  N_(usage: svnrdump dump URL [-r LOWER[:UPPER]]\n\n
+ Dump revisions LOWER to UPPER of repository at remote URL 
+ to stdout in a 'dumpfile' portable format.\n
+ If omitted, LOWER defaults to zero and UPPER to the latest 
+ latest revision.\n),
+  { 0 } },
+{ load, load_cmd, { l },
+  N_(usage: svnrdump load URL\n\n
+ Load a 'dumpfile' given on stdin to a repository 
+ at remote URL.\n
+ ## This feature is not yet available.\n),
+  { 0 } },
+{ help, 0, { ?, h },
+  N_(usage: svnrdump help [SUBCOMMAND...]\n\n
+ Describe the usage of this program or its subcommands.\n),
+  { 0 } },
+{ NULL, NULL, { 0 }, NULL, { 0 } }
+  };
+
  static const apr_getopt_option_t svnrdump__options[] =
{
  {revision, 'r', 1, N_(REV1[:REV2] range of revisions to dump)},
@@ -77,6 +101,14 @@ struct replay_baton {
svn_boolean_t quiet;
  };

+typedef struct {
+  svn_ra_session_t *session;
+  const char *url;
+  svn_revnum_t start_revision;
+  svn_revnum_t end_revision;
+  svn_boolean_t quiet;
+} opt_baton_t;
+
  static svn_error_t *
  replay_revstart(svn_revnum_t revision,
  void *replay_baton,
@@ -179,9 +211,9 @@ open_connection(svn_ra_session_t **sessi
  }

  static svn_error_t *
-replay_range(svn_ra_session_t *session, const char *url,
- svn_revnum_t start_revision, svn_revnum_t end_revision,
- svn_boolean_t quiet, apr_pool_t *pool)
+replay_revisions(svn_ra_session_t *session, const char *url,
+ svn_revnum_t start_revision, svn_revnum_t end_revision,
+ svn_boolean_t quiet, apr_pool_t *pool)
  {
const svn_delta_editor_t *dump_editor;
struct replay_baton *replay_baton;
@@ -268,39 +300,12 @@ usage(const char *progname, apr_pool_t *
progname = ensure_appname(progname, pool);

SVN_ERR(svn_cmdline_fprintf(stderr, pool,
-  _(Type '%s --help' for usage.\n),
+  _(Type '%s help' for usage.\n

Re: svn commit: r979094 - /subversion/trunk/subversion/svnrdump/svnrdump.c


On 07/26/2010 05:25 PM, Ramkumar Ramachandra wrote:

Hi Kamesh,

Kamesh Jayachandran writes:
   

This breaks the 2 tests in svnrdump_tests.


Though the following inline patch may fix it you may have different plans
 

Thanks! Yes, I'm aware that it breaks tests. Unfortunately, I have too
many uncommitted changes, and I'm still waiting for them to be
approved (see the three recent threads that I just bumped up) before I
fix this.

About my plan: I'm going to add load support too, so this patch isn't
generic enough.
   


Yes I know it, I tweaked 'run_svnrdump' to always pass 'dump', but it 
caused segfault.


svnrdump some_unknown_subcommand segfaults.

With regards
Kamesh Jayachandran

Thanks again.

-- Ram

Re: svn commit: r979282 - in /subversion/trunk/subversion/svnrdump: dump_editor.c dump_editor.h load_editor.c load_editor.h svnrdump.c


On 07/26/2010 07:25 PM, artag...@apache.org wrote:

Author: artagnon
Date: Mon Jul 26 13:55:25 2010
New Revision: 979282

URL: http://svn.apache.org/viewvc?rev=979282view=rev
Log:
svnrdump: Add an a no-op load editor

* subversion/svnrdump/svnrdump.c
   (load_revisions): New function to fetch and drive the load
   editor. Currently just fetches the load editor.
   (load_cmd): Call load_revisions with the appropriate arguments.

* subversion/svnrdump/load_editor.c
   (get_load_editor): Add a no-op load editor.
   


Please name this function 'get_load_editor' as 'svnrdump__get_load_editor'.

With regards
Kamesh Jayachandran

Re: Info needed in 1.6.x STATUS file

2010-06-16 Thread Kamesh Jayachandran


On 06/15/2010 10:51 PM, Julian Foad wrote:

Hi devs.  Going through the 1.6.x. STATUS file, I am finding some items
that could do with a better description or justification to make it
easier to evaluate them.  Can anyone provide information on the
following:


   

  * r934599, r934603
Fix a concurrency issue in the FSFS rep-cache code.  This indirectly
concerns issue #3506.
Justification:
  Opening the DB twice (and overwriting the handle) is undesired.
 

Why is that undesired?  I mean that as a serious practical question -
obviously it's theoretically redundant but that in itself doesn't
necessarily matter.  What is the user-visible effect of the problem, and
of the proposed change?

   

Notes:
  r934599 updates the svn_atomic__init_once() interface.
  r934603 is the actual fix.
Notes:
  Depends on the r879966 group, which does not merge cleanly.
Branch:
  ^/subversion/branches/1.6.x-issue3506-init_once
  (based on ^/subversion/branches/1.6.x-issue3506)
Votes:
  +1: danielsh
 


   

  * r878590, r878607, r878625, r878626, r878627
In trunk we optimized the common case of 'find-and-replace with same uri'
of proxied content thanks to issue 3445 WebDAV proxy code munging user
data.
 

What is the purpose and effect of the change proposed for back-port?
   


Today with 1.6.11 write through proxy would corrupt the commit if the 
url of master and slave(actually path portions of the request) are 
different.


We avoid this problem by configuring the the path portions to be the same.

Even if we avoid this we end up parsing the commit body stream to find 
and replace the same string with same string(i.e s/\/svn/\/svn/g).



This particular block of fixes is about *avoiding* finding and replace 
of same thing with same thing and hence provide better response time.



   

r878590 is just a change that adds FIXME marker to code comment. We take it
to avoid spurious conflicts with other revisions.
r878607 Special cases no-op find and replace.
r878625, r878626, r878627 are follow-up to r878607 and the other long
pending Master info leak to the clients.
Justification:
  1. This group has the most common 'optimization' fix of *not* groking the
 proxied response to find and replace with same string.
  2. Fixes the master information leak via the Location header.
  3. Need this to be ported for the other defect to be ported
 without conflict.
Votes:
  +1: kameshj, cmpilato
  -0: julianfoad (seem to be multiple changes here for different reasons -
at least issue '3445 and an optimization and an information leak;
r878607 log msg says it fixes bugs but it's not clear what bugs;
don't know how to tell whether justification 1 is significant;
justifications don't seem to refer to issue #3445. Please can we
separate these changes and clearly describe each one? And update
the r878607 log msg.)
 

Ah, I already put my concerns here.


   

  * r916286, r917512
???
 

What is the purpose and effect of the change proposed for back-port?

(It was me who added the question marks, some time ago.)
   


Hope I made it clear now in the STATUS file in my recent commit.

   

Notes:
  This backport depends on the r878590 group only for the conflict free 
port.
Justification:
  Vague commit error when server has been configured withLocation /svn/
  and write through proxy.
 

How bad is the old error message?  How much better is the new one?  An
example might help.  Does the change have any other effect?

   


Hope I made it clear now in the STATUS file in my recent commit.

Votes:
  +1: kameshj
 


   

  * r917523
???
 

Ditto.

   


Hope I made it clear now in the STATUS file in my recent commit.

Notes:
  This backport depends on the r878590 and r916286 groups only for the
  conflict free port.
Justification:
  If the configured slave url has the *encodable* characters *write through*
  is not happening rather commit happens in slave itself.
 

Commit happens in the slave?  That's certainly and obviously wrong.  But
I don't understand what you mean by the configured slave url has the
*encodable* characters.
   



Hope I made it clear now in the STATUS file in my recent commit.


With regards
Kamesh Jayachandran

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

2010-04-12 Thread Kamesh Jayachandran


On 04/12/2010 07:49 PM, C. Michael Pilato wrote:

kame...@apache.org wrote:
   

Author: kameshj
Date: Mon Apr 12 11:26:28 2010
New Revision: 933194

URL: http://svn.apache.org/viewvc?rev=933194view=rev
Log:
[issue2753] Fix issue 2753.

Relax requests aimed at the repo Parent path from authz control.

* subversion/mod_authz_svn/mod_authz_svn.c
   (create_authz_svn_dir_config): Canonicalize conf-base_path.
   (req_check_access): When canonicalized 'uri' and 'conf-base_path' are same
allow the request.
   (access_checker, check_user_id, auth_checker):
Initialize repos_path to 'NULL' otherwise it can point
to stray values when req_check_access relaxes certain requests without
initialising the out parameters.
 

In a perfect world, I would expect that requests to the parent directory
would not be authz-denied, but that each repository in the listing of
repositories would be authz-checked against the authz configuration.  In
other words, say I have a parent-path with three repositories: calc, watch,
lamp.  And say I have an authz file like so:

[lamp:/]
* =
   




I would expect that a request to the parent directory would yield a listing
that included the 'calc' and 'watch' repositories, but not the 'lamp' one.

Is that the case?

   

No.

These authz rule should *not* list anything inside the repo 'lamp' but 
not lamp itself when requested for the parent path root.



The feature that you ask for is possible only if 'mod_dav_svn'(which 
implements SVNListParentPath) consults mod_authz_svn(or some authorizer) 
for every item listed which is not the case today.



With regards
Kamesh Jayachandran

RE: Expansion of authz policy name leak (was: svn commit: r933194 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c)

2010-04-12 Thread Kamesh Jayachandran

IIUC, prior to your change, nobody who had enabled authz at all could make
use of the SVNListParentPath feature (because the authorization for that
display would systematically fail).  But this also means that Subversion
never leaked the name of a repository that was intended to be private/hidden
from particular users.  Now, we no longer suffer the blanket authz failure,
but we are showing the name of every repository in the parent directory
without regard to any authz rules whatsoever.


Whoever wanted SVNListParentPath to work with authz prior to this commit was 
using a workaround of Location /svn/ which was leaking the same info.

With regards
Kamesh Jayachandran

Re: [PATCH] Fix for issue 2753 SVNListParentPath feature doesn't work when svn authz is used.

2010-04-09 Thread Kamesh Jayachandran


On 04/08/2010 09:23 PM, Philip Martin wrote:

Kamesh Jayachandrankam...@collab.net  writes:

   

[issue2753] Fix issue 2753.
 

Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff.  This is a bug because the
authz file doesn't control parent path.

Your patch recognises this request and avoids doing the authz check.

   


Yes, exactly.



Relax requests aimed at the repo Parent path from authz control.

* subversion/mod_authz_svn/mod_authz_svn.c
   (req_check_access): When canonicalized 'uri' and 'root_path' are same
allow the request.
]]]

If there are no objections will commit this in next couple of days.

Thanks
With regards
Kamesh Jayachandran

Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 931820)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -210,6 +210,8 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+  const char *canonicalized_uri;
+  const char *canonicalized_root_path;
const char *username_to_authorize = get_username_to_authorize(r, conf);

switch (r-method_number)
@@ -249,6 +251,15 @@
  break;
  }

+  canonicalized_uri = svn_uri_canonicalize(r-uri, r-pool);
+  canonicalized_root_path = svn_uri_canonicalize(conf-base_path, r-pool);
 

Can conf-base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?
   


Yes done. Attached patch has this.

   

+  if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
+{
+  /*Do no access control when root_path(as configured inLocation) and
+   given uri are same.*/
+  return OK;
+}
 

What happens if SVNParentPath is not being used?  Is base_path is the
root of the repository?  Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?
   


As said in my other email this works without any issue, the comment 
added in the patch details the same.




I think this check would make more sense in access_checker rather than
req_check_access.
   


May be, I prefer req_check_access as there are 3 callers for this function.

The code needs a comment to say why no access control is neccessary in
this case.

   


Check my new comment in the attached patch.


The attached patch fixes one segfault(introduced in my earlier patch) also.


With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 932331)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -66,6 +66,9 @@
   authz_svn_config_rec *conf = apr_pcalloc(p, sizeof(*conf));
   conf-base_path = d;
 
+  if (d)
+conf-base_path = svn_uri_canonicalize(d, p);
+
   /* By default keep the fortress secure */
   conf-authoritative = 1;
   conf-anonymous = 1;
@@ -210,6 +213,7 @@
   svn_authz_t *access_conf = NULL;
   svn_error_t *svn_err;
   char errbuf[256];
+  const char *canonicalized_uri;
   const char *username_to_authorize = get_username_to_authorize(r, conf);
 
   switch (r-method_number)
@@ -249,6 +253,22 @@
 break;
 }
 
+  canonicalized_uri = svn_uri_canonicalize(r-uri, r-pool);
+  if (strcmp(canonicalized_uri, conf-base_path) == 0)
+{
+  /* Do no access control when conf-base_path(as configured in Location)
+   * and given uri are same. The reason for such relaxation of access
+   * control is This module is meant to control access inside the
+   * repository path, in this case inside PATH is empty and hence
+   * dav_svn_split_uri fails saying no repository name present.
+   * One may ask it will allow access to '/' inside the repository if
+   * repository is served via SVNPath instead of SVNParentPath.
+   * It does not, The other methods(PROPFIND, MKACTIVITY) for
+   * accomplishing the operation takes care of making a request to
+   * proper URL */
+  return OK;
+}
+
   dav_err = dav_svn_split_uri(r,
   r-uri,
   conf-base_path,
@@ -554,7 +574,7 @@
 {
   authz_svn_config_rec *conf = ap_get_module_config(r-per_dir_config,
 authz_svn_module);
-  const char *repos_path;
+  const char *repos_path = NULL;
   const char *dest_repos_path = NULL;
   int status;
 
@@ -611,7 +631,7 @@
 {
   authz_svn_config_rec *conf = ap_get_module_config(r-per_dir_config,
 authz_svn_module);
-  const char *repos_path;
+  const char *repos_path = NULL;
   const char *dest_repos_path = NULL;
   int status;
 
@@ -639,7 +659,7 @@
 {
   authz_svn_config_rec *conf = ap_get_module_config(r

[PATCH] Fix for issue 2753 SVNListParentPath feature doesn't work when svn authz is used.

2010-04-08 Thread Kamesh Jayachandran


Hi All,

Attached patch fixes issue 2753.

Quick description of 2753.

Location /svn
  DAV svn
  SVNParentPath /repositories
  AuthType Basic
  AuthName My SVN
  AuthUserFile /etc/httpd/conf.d/users
  allow from all
  AuthzSVNAccessFile /etc/httpd/conf.d/svn_access_file
/Location

With the above configuration 'wget http://localhost/svn' gets 403 Access 
forbidden.


Thrown from the following stack trace.

mod_dav_svn/repos.c:dav_svn_split_uri() -- This function throws this 
403 logging the following in the error_log

   The URI does not contain the name 
   of a repository.);
mod_authz_svn:req_check_access()
mod_authz_svn:access_checker()

The suggested work around for this issue is to define a Location with 
a trailing slash i.e Location /svn/


Why this work around works?

Whatever that is defined in the Location /svn or Location /svn/ is 
passed as is in the variable name 'root_path'.

dav_svn_split_uri() always removes the trailing slash of the uri.

So uri becomes '/svn' and root_path becomes '/svn' or '/svn/' based on 
how it is configured in the Location block.


In the work around case

relative = ap_stripprefix(/svn, /svn/);  //relative becomes '/svn' 
and hence passes rest of the code path without error.


While 'relative' becomes empty string for ap_stripprefix(/svn, /svn) 
and hence this 403.



About the fix:
Fix is to 'relax' mod_authz_svn for 'requests' that are for the repo parent.

I tested the following cases with this patch:
With the restrictive(read-only) authz, tried to set prop on the '/' of 
the repo(configured to serve via SVNPath), it failed as expected.


Ran through the testsuite, It did not break any new tests.

[[[
[issue2753] Fix issue 2753.

Relax requests aimed at the repo Parent path from authz control.

* subversion/mod_authz_svn/mod_authz_svn.c
  (req_check_access): When canonicalized 'uri' and 'root_path' are same
   allow the request.
]]]

If there are no objections will commit this in next couple of days.

Thanks
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===
--- subversion/mod_authz_svn/mod_authz_svn.c(revision 931820)
+++ subversion/mod_authz_svn/mod_authz_svn.c(working copy)
@@ -210,6 +210,8 @@
   svn_authz_t *access_conf = NULL;
   svn_error_t *svn_err;
   char errbuf[256];
+  const char *canonicalized_uri;
+  const char *canonicalized_root_path;
   const char *username_to_authorize = get_username_to_authorize(r, conf);
 
   switch (r-method_number)
@@ -249,6 +251,15 @@
 break;
 }
 
+  canonicalized_uri = svn_uri_canonicalize(r-uri, r-pool);
+  canonicalized_root_path = svn_uri_canonicalize(conf-base_path, r-pool);
+  if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
+{
+  /*Do no access control when root_path(as configured in Location) and 
+   given uri are same.*/
+  return OK;
+}
+
   dav_err = dav_svn_split_uri(r,
   r-uri,
   conf-base_path,

RE: [PATCH] Fix for issue 2753 SVNListParentPath feature doesn't work when svn authz is used.

2010-04-08 Thread Kamesh Jayachandran

Thanks for the review Philip.


Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff.  This is a bug because the
authz file doesn't control parent path.

Your patch recognises this request and avoids doing the authz check.

Yes, exactly.

 +  canonicalized_uri = svn_uri_canonicalize(r-uri, r-pool);
 +  canonicalized_root_path = svn_uri_canonicalize(conf-base_path, r-pool);


Can conf-base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?

Yes should be, Will update my patch to handle this.

 +  if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
 +{
 +  /*Do no access control when root_path(as configured in Location) 
 and 
 +   given uri are same.*/
 +  return OK;
 +}

What happens if SVNParentPath is not being used?  Is base_path is the
root of the repository?  Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?


I tested this 

$svn co http://localhost/svn -- Repo itself instead of parent of repositories.
$cd svn
$svn ps 'a' 'b' .
$svn ci -m commit -This worked as per the authz rules. Anyway will do the 
directory/file creations to check in case!.

I think this check would make more sense in access_checker rather than
req_check_access.

Let me see and do if needed.

The code needs a comment to say why no access control is neccessary in
this case.

Will update the comment.

With regards
Kamesh Jayachandran

RE: svn commit: r917512 - in /subversion/trunk/subversion/mod_dav_svn: mirror.c mod_dav_svn.c

2010-03-01 Thread Kamesh Jayachandran


Modified as suggested.

Thanks
With regards
Kamesh Jayachandran

-Original Message-
From: Blair Zajac [mailto:bl...@orcaware.com]
Sent: Mon 3/1/2010 10:47 PM
To: kame...@apache.org
Cc: dev@subversion.apache.org
Subject: Re: svn commit: r917512 - in /subversion/trunk/subversion/mod_dav_svn: 
mirror.c mod_dav_svn.c
 
kame...@apache.org wrote:
 Author: kameshj
 Date: Mon Mar  1 13:15:58 2010
 New Revision: 917512
 
 URL: http://svn.apache.org/viewvc?rev=917512view=rev
 Log:
 Follow-up to r916286.
 
 * subversion/mod_dav_svn/mod_dav_svn.c
 (create_dir_config):
 * subversion/mod_dav_svn/mirror.c
 (dav_svn__location_in_filter, dav_svn__location_body_filter):
 Use 'svn_uri_canonicalize' on url paths.

A minor comment on the log messages.  The lines beginning with () are indented 
two spaces and each source file has one vertical whitespace separating it from 
other files.  This makes it much easier to visually parse the log message to 
quickly see what's being changed:

* subversion/mod_dav_svn/mod_dav_svn.c
   (create_dir_config):
   Use 'svn_uri_canonicalize' on url paths.

* subversion/mod_dav_svn/mirror.c
   (dav_svn__location_in_filter, dav_svn__location_body_filter):
   Use 'svn_uri_canonicalize' on url paths.

Regards,
Blair

Re: [PATCH] Fix the commit failure over the write-through-proxy if apache is configured as 'Location /svn/'

2010-02-25 Thread Kamesh Jayachandran


On 02/15/2010 07:44 PM, Julian Foad wrote:

Kamesh Jayachandran wrote:
   

With the below apache configuration(See the trailing slash at the end of
'/svn/').

Location /svn/
DAV svn
SVNParentPath /repositories
SVNMasterURI http://master/svn/
SVNAdvertiseV2Protocol Off
/Location
 

[...]
   

Attached patch fixes it.

This change came via fix for issue 3275. I believe this assertion is
just to clean the uri for double slash and not anything functional to
that issue, I may be wrong.

Thoughts?
 

The logic in your patch looks correct IF we accept that
dav_svn__get_root_dir() can return a non-canonical path. But if that's
the case, then we should document it as such:

[[[
Index: subversion/mod_dav_svn/dav_svn.h
===
--- subversion/mod_dav_svn/dav_svn.h(revision 910187)
+++ subversion/mod_dav_svn/dav_svn.h(working copy)
@@ -352,7 +352,7 @@
  /* Return the activities db */
  const char * dav_svn__get_activities_db(request_rec *r);

-/* Return the root directory */
+/* Return the root directory (not necessarily as a canonical path) */
  const char * dav_svn__get_root_dir(request_rec *r);


]]]

or if it's meant to be canonical, then we should fix that function
instead.

- Julian
   


Changed 'dav_svn__get_root_dir' to return canonical path.


Committed this in r916286.

With regards
Kamesh Jayachandran

Bug in svn merge --show-revs eligible ^/subversion/trunk ^/subversion/branches/1.6.x

2010-02-25 Thread Kamesh Jayachandran

svn mergeinfo --show-revs eligible 
https://svn.apache.org/repos/asf/subversion/trunk 
https://svn.apache.org/repos/asf/subversion/branches/1.6.x


Above command lists 'r876233' while that has already been merged!

With regards
Kamesh Jayachandran

[PATCH] Fix the commit failure over the write-through-proxy if apache is configured as 'Location /svn/'

2010-02-15 Thread Kamesh Jayachandran


Hi All,

With the below apache configuration(See the trailing slash at the end of 
'/svn/').


Location /svn/
  DAV svn
  SVNParentPath /repositories
  SVNMasterURI http://master/svn/
  SVNAdvertiseV2Protocol Off
/Location


We get the following error on the client while.

../subversion/svn/commit-cmd.c:142: (apr_err=175002)
../subversion/libsvn_client/commit.c:853: (apr_err=175002)
svn: Commit failed (details follow):
../subversion/libsvn_ra_neon/commit.c:1463: (apr_err=175002)
../subversion/libsvn_ra_neon/commit.c:334: (apr_err=175002)
../subversion/libsvn_ra_neon/util.c:613: (apr_err=175002)
svn: MKACTIVITY of 
'/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could not 
read status line: connection was closed by server (http://localhost)



On the server it is a assertion error on the following code block from 
subversion/mod_dav_svn/mirror.c:proxy_request_fixup


   assert((uri_segment[0] == '\0')
   || (uri_segment[0] == '/'));

For the above configuration we get the uri_segment with the value 
'reponame/some/path/inside/the/repo'.




Attached patch fixes it.


This change came via fix for issue 3275. I believe this assertion is 
just to clean the uri for double slash and not anything functional to 
that issue, I may be wrong.




Thoughts?

Thanks
With regards
Kamesh Jayachandran
Index: subversion/mod_dav_svn/mirror.c
===
--- subversion/mod_dav_svn/mirror.c (revision 910164)
+++ subversion/mod_dav_svn/mirror.c (working copy)
@@ -64,6 +64,7 @@
 
 if (root_dir  master_uri) {
 const char *seg;
+size_t root_dir_len = strlen(root_dir);
 
 /* We know we can always safely handle these. */
 if (r-method_number == M_REPORT ||
@@ -85,7 +86,10 @@
 /txn/, NULL))
 || ap_strstr_c(seg, apr_pstrcat(r-pool, special_uri,
 /txr/, NULL))) {
-seg += strlen(root_dir);
+if (root_dir[root_dir_len - 1] == '/')
+seg += (root_dir_len - 1);
+else
+seg += root_dir_len;
 proxy_request_fixup(r, master_uri, seg);
 }
 }
@@ -100,7 +104,10 @@
 r-method_number == M_LOCK ||
 r-method_number == M_UNLOCK ||
 ap_strstr_c(seg, special_uri))) {
-seg += strlen(root_dir);
+if (root_dir[root_dir_len - 1] == '/')
+seg += (root_dir_len - 1);
+else
+seg += root_dir_len;
 proxy_request_fixup(r, master_uri, seg);
 return OK;
 }

Re: Commit failure via out of date proxy - issue #3561 - with Serf

2010-01-29 Thread Kamesh Jayachandran


On 01/28/2010 09:08 PM, C. Michael Pilato wrote:

Julian Foad wrote:
   

On Thu, 2010-01-28 at 10:23 -0500, C. Michael Pilato wrote:
 

Julian Foad wrote:
   

On Wed, 2010-01-20, Julian Foad wrote:
 

I have added issue #3561
http://subversion.tigris.org/issues/show_bug.cgi?id=3561  to track this
   

Just now, using a client built from today's trunk code, I got the same
problem when committing with Serf:
 

[...]

   

In earlier emails [1] and [2], Kamesh indicated that Serf does not
suffer the same problem because it does not issue the PROPFIND, but that
was in the context of Mike asking about HTTPv2, so I am not clear
whether that applies to an ordinary build or if I have to do something
special to use HTTPv2.
 

You have to have a server that supports HTTPv2.  The ASF server is not such
a server (because we haven't released Subversion 1.7 yet).
   

Ah, thanks. So when it's upgraded, that part of the problem will be
solved too. That's good.
 

Right.  Now, that said, it might be prudent to make the same fix in Serf's
non-HTTPv2 codepath that Kamesh made for Neon.  I mean, if there's an easy
client-side fix to be had, seems silly to require a server upgrade instead.

Kamesh, can you look into this?

   
Attached patch fixes it, though I need to get better context to the 
relevant code base to give a 100% confident fix, which I will do it sooner.


With regards
Kamesh Jayachandran
Index: subversion/libsvn_ra_serf/commit.c
===
--- subversion/libsvn_ra_serf/commit.c  (revision 904436)
+++ subversion/libsvn_ra_serf/commit.c  (working copy)
@@ -91,6 +91,7 @@
   checkout_context_t *baseline;  /* checkout for the baseline */
   const char *checked_in_url;/* checked-in root to base CHECKOUTs from */
   const char *baseline_url;  /* root baseline collection */
+  const char *vcc_url;   /* vcc url */
 
 } commit_context_t;
 
@@ -254,6 +255,7 @@
 
   svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, D:href);
   svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, D:activity-set);
+  svn_ra_serf__add_tag_buckets(body_bkt, D:apply-to-version, NULL, alloc);
   svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, D:checkout);
 
   return body_bkt;
@@ -393,7 +395,7 @@
*/
   if (!dir-parent_dir  !dir-commit-baseline)
 {
-  checkout_ctx-checkout_url = dir-commit-baseline_url;
+  checkout_ctx-checkout_url = dir-commit-vcc_url;
   dir-commit-baseline = checkout_ctx;
 }
   else
@@ -410,6 +412,7 @@
   handler-response_baton = checkout_ctx;
 
   handler-method = CHECKOUT;
+  printf(checkout_ctx-checkout_url=%s\n, checkout_ctx-checkout_url);
   handler-path = checkout_ctx-checkout_url;
 
   svn_ra_serf__request_create(handler);
@@ -1080,7 +1083,6 @@
   svn_ra_serf__handler_t *handler;
   proppatch_context_t *proppatch_ctx;
   dir_context_t *dir;
-  apr_hash_t *props;
   apr_hash_index_t *hi;
   const char *proppatch_target;
 
@@ -1150,9 +1152,7 @@
 {
   svn_ra_serf__options_context_t *opt_ctx;
   svn_ra_serf__simple_request_context_t *mkact_ctx;
-  svn_ra_serf__propfind_context_t *propfind_ctx;
   const char *activity_str;
-  const char *vcc_url;
 
   SVN_ERR(svn_ra_serf__create_options_req(opt_ctx, ctx-session,
   ctx-session-conns[0],
@@ -1202,31 +1202,10 @@
 }
 
   /* Now go fetch our VCC and baseline so we can do a CHECKOUT. */
-  SVN_ERR(svn_ra_serf__discover_vcc(vcc_url, ctx-session,
+  SVN_ERR(svn_ra_serf__discover_vcc((ctx-vcc_url), ctx-session,
 ctx-conn, ctx-pool));
 
-  props = apr_hash_make(ctx-pool);
-  propfind_ctx = NULL;
-  SVN_ERR(svn_ra_serf__deliver_props(propfind_ctx, props, ctx-session,
- ctx-conn, vcc_url,
- SVN_INVALID_REVNUM, 0,
- checked_in_props, FALSE,
- NULL, ctx-pool));
 
-  SVN_ERR(svn_ra_serf__wait_for_props(propfind_ctx, ctx-session,
-  ctx-pool));
-
-  ctx-baseline_url = svn_ra_serf__get_ver_prop(props, vcc_url,
-SVN_INVALID_REVNUM,
-DAV:, checked-in);
-
-  if (!ctx-baseline_url)
-{
-  return svn_error_create(SVN_ERR_RA_DAV_OPTIONS_REQ_FAILED, NULL,
-  _(The OPTIONS response did not include the 
-requested checked-in value));
-}
-
   /* Build our directory baton. */
   dir = apr_pcalloc(dir_pool, sizeof(*dir));
   dir-pool = dir_pool;

[PATCH] Much simpler fix to commit failure via out of date proxy


Hi All

Mike has suggestion at [1] to make use 'DAV:apply-to-version' to avoid 
the problematic PROPFIND altogether.


This suggestion works very well.

It is just the ra_neon fix i.e no mod_dav_svn fix

Attached patch implements this fix.

All tests pass with this test(except the known mergeinfo test 8 failure.)


In next few hours I will commit this as it looks fairly obvious unless 
there are some objections.


Thanks Mike,

With regards
Kamesh Jayachandran

PS
[1] 
http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b4cc1aa.6000...@collab.net%3e
Make CHECKOUT request on VCC url rather than the baseline url as that avoids
the detection of *out of date* baseline url from the mirror and leave that
to CHECKOUT itself via 'D:apply-to-version/'.

As CHECKOUT is proxied by the mirror we are not getting hit by the original
issue.

* subversion/libsvn_ra_neon/commit.c
(APPLY_TO_VERSION): New macro.
(do_checkout): Accept 'is_vcc' and make CHECKOUT with 'D:apply-to-version/'
 if is_vcc is TRUE.
(checkout_resource): Accept 'is_vcc' and cascade.
(commit_delete_entry, commit_add_dir,
 commit_change_dir_prop, commit_add_file,
 commit_open_file, commit_change_file_prop): 
 Call 'checkout_resource' with is_vcc as FALSE.
(apply_revprops): Avoid baseline detecion,
 Call 'checkout_resource' with is_vcc as TRUE and rsrc being that of VCC.

Suggested by: cmpilato
Index: subversion/libsvn_ra_neon/commit.c
===
--- subversion/libsvn_ra_neon/commit.c  (revision 900682)
+++ subversion/libsvn_ra_neon/commit.c  (working copy)
@@ -48,6 +48,7 @@
 #include ra_neon.h
 
 
+#define APPLY_TO_VERSION D:apply-to-version/
 /*
 ** version_rsrc_t: identify the relevant pieces of a resource on the server
 **
@@ -399,6 +400,7 @@
  const char *vsn_url,
  svn_boolean_t allow_404,
  const char *token,
+ svn_boolean_t is_vcc,
  int *code,
  const char **locn,
  apr_pool_t *pool)
@@ -423,7 +425,9 @@
   D:checkout xmlns:D=\DAV:\
   D:activity-set
   D:href%s/D:href
-  /D:activity-set/D:checkout, cc-activity_url);
+  /D:activity-set%s/D:checkout,
+  cc-activity_url,
+  is_vcc ? APPLY_TO_VERSION: );
 
   if (token)
 {
@@ -459,6 +463,7 @@
version_rsrc_t *rsrc,
svn_boolean_t allow_404,
const char *token,
+   svn_boolean_t is_vcc,
apr_pool_t *pool)
 {
   int code;
@@ -473,7 +478,8 @@
 }
 
   /* check out the Version Resource */
-  err = do_checkout(cc, rsrc-vsn_url, allow_404, token, code, locn, pool);
+  err = do_checkout(cc, rsrc-vsn_url, allow_404, token,
+is_vcc, code, locn, pool);
 
   /* possibly run the request again, with a re-fetched Version Resource */
   if (err == NULL  allow_404  code == 404)
@@ -484,7 +490,8 @@
   SVN_ERR(get_version_url(cc, NULL, rsrc, TRUE, pool));
 
   /* do it again, but don't allow a 404 this time */
-  err = do_checkout(cc, rsrc-vsn_url, FALSE, token, code, locn, pool);
+  err = do_checkout(cc, rsrc-vsn_url, FALSE, token,
+is_vcc, code, locn, pool);
 }
 
   /* special-case when conflicts occur */
@@ -711,7 +718,8 @@
 }
 
   /* get the URL to the working collection */
-  SVN_ERR(checkout_resource(parent-cc, parent-rsrc, TRUE, NULL, pool));
+  SVN_ERR(checkout_resource(parent-cc, parent-rsrc, TRUE,
+NULL, FALSE, pool));
 
   /* create the URL for the child resource */
   child = svn_path_url_add_component(parent-rsrc-wr_url, name, pool);
@@ -847,7 +855,8 @@
 
   /* check out the parent resource so that we can create the new collection
  as one of its children. */
-  SVN_ERR(checkout_resource(parent-cc, parent-rsrc, TRUE, NULL, dir_pool));
+  SVN_ERR(checkout_resource(parent-cc, parent-rsrc, TRUE,
+NULL, FALSE, dir_pool));
 
   /* create a child object that contains all the resource urls */
   child = apr_pcalloc(dir_pool, sizeof(*child));
@@ -959,7 +968,7 @@
   record_prop_change(dir-pool, dir, name, value);
 
   /* do the CHECKOUT sooner rather than later */
-  SVN_ERR(checkout_resource(dir-cc, dir-rsrc, TRUE, NULL, pool));
+  SVN_ERR(checkout_resource(dir-cc, dir-rsrc, TRUE, NULL, FALSE, pool));
 
   /* Add this path to the valid targets hash. */
   add_valid_target(dir-cc, dir-rsrc-local_path, svn_nonrecursive);
@@ -1000,7 +1009,8 @@
   */
 
   /* Do the parent CHECKOUT first */
-  SVN_ERR(checkout_resource(parent-cc, parent-rsrc, TRUE, NULL, workpool

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2


On 01/18/2010 11:47 PM, Kamesh Jayachandran wrote:
   

whether it addresses any of the points Mike raised and in what ways,
that sort of thing.
 

Hey forgot to answer this bit. Mike was suggesting to make use of 
'DAV:apply-to-version' in CHECKOUT rather than the problematic PROPFIND. I did 
not explore this bit yet.

   


This 'DAV:apply-to-version' on CHECKOUT works, fairly a simpler fix, 
Thanks Mike for the suggestion.


I have attached the patch in other thread.


With regards
Kamesh Jayachandran

Re: [PATCH] Much simpler fix to commit failure via out of date proxy


On 01/19/2010 06:55 PM, Mark Phippard wrote:

On Tue, Jan 19, 2010 at 7:58 AM, Kamesh Jayachandrankam...@collab.net  wrote:

   

Mike has suggestion at [1] to make use 'DAV:apply-to-version' to avoid the
problematic PROPFIND altogether.

This suggestion works very well.

It is just the ra_neon fix i.e no mod_dav_svn fix
 

How does this work without also changing mod_dav_svn?  I thought it
did not already have code to handle this?
   


snip from http://www.ietf.org/rfc/rfc3253.txt

 (DAV:create-working-resource-from-checked-in-version): If the
  request-URL identified a version-controlled resource, and
  DAV:apply-to-version is specified in the request body, the
  CHECKOUT is applied to the DAV:checked-in version of the version-
  controlled resource, and not the version-controlled resource
  itself.
/snip

I think mod_dav does this for us.



   

Attached patch implements this fix.

All tests pass with this test(except the known mergeinfo test 8 failure.)
 

Does this change eliminate the PROPFIND?  If it doesn't, given that we
do not have any tests that show this problem it does not seem
unexpected that tests would pass.
   


It eliminates the PROPFIND. I manually tested this by examining the 
concerned neon-debug output.






   

In next few hours I will commit this as it looks fairly obvious unless there
are some objections.
 

It sounds great if this fixes the problem.  I am still a little
perplexed as to how this works and how it changes the commit process
and the interactions with a proxy.
   


It just does not do the problematic PROPFIND leaving way to CHECKOUT to 
figure it out.


With regards
Kamesh Jayachandran

Re: [PATCH] Much simpler fix to commit failure via out of date proxy


On 01/19/2010 07:23 PM, Mark Phippard wrote:

On Tue, Jan 19, 2010 at 8:49 AM, C. Michael Pilatocmpil...@collab.net  wrote:
   

Mark Phippard wrote:
 

Any idea how it works?  CHECKOUT is serviced by the proxy server
right?  I would think it would lead to the same problem?  Maybe the
code winds up creating some kind of floating HEAD link as opposed to
tying it to what it determines to be the HEAD revision.
   

CHECKOUT is serviced by the master, as it is part of the commit process.
CHECKOUT in WebDAV isn't a read operation like svn checkout is.  It's more
like prepare to be modified or somesuch.
 

Thanks.  I knew this was a DAV checkout, but I thought it was also
used in some read operations.  Anyway, that probably explains why it
works.

Nice suggestion.

   

Committed in r900797.

With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2

2010-01-18 Thread Kamesh Jayachandran


On 01/13/2010 05:07 PM, Kamesh Jayachandran wrote:

On 01/13/2010 12:08 AM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:

Hi All,

Last week I posted the patch to implement 'svn client' to identify the
svn operation they are about to do with a given ra_session.

Following thread has the detailed discussion.

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e 





Below is the summary:

Concern/Suggestion 1:
Michael Pilato and Philip Martin were suggesting to tweak
libsvn_ra_(serf|neon) to detect the operation rather than making a
change across all layers from the simplicity point of view.

My answer to 1:
I feel it would be too hackish to tweak one general API inside these
modules to flag 'commit or write' operation to the server when the
solution I propose handles this in a transparent way.
I'm sorry, but did you say transparent?  What's transparent about 
bubbling
an RA-layer hack all the way up into the client?!  A transparent 
solution


This patch is *not* an RA layer hack rather a transparent generic 
feature so I see nothing wrong in propagating it to higher layers.


is one that preserves the existing transparency of the mirroring 
subsystem.


I do *not* like to expose the back-end internals to higher layers 
especially when it is not so common setup and not normally supposed to 
know(I mean why my client should know the repository it commits to is 
a mirror directly or indirectly).


  A transparent solution is one that doesn't allow ignorant 
third-party
consumers of the Subversion APIs to accidentally break their proxy 
setups
because they decide they wanted to pass checkin instead of commit 
as the

innocuous-appearing 'high_level_svn_operation' parameter.


Yes I agree. I was concerned about the *magical flag deep inside the 
code* for only one operation, now it looks like the only way to go.





There *must* be a better way to do this.

subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:

   /* ### we should use DAV:apply-to-version on the CHECKOUT so we 
can skip

  ### retrieval of the baseline */

I looked a little bit into this.  IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request.  Currently,
though, mod_dav_svn doesn't handle the flag.  So there's still a 
server-side

change needed, but at least its one that would take us closer to better
WebDAV handling.  Maybe you could explore this option instead?



I will take a fresh look at this problem  and a independent patch in 
this way.


Thanks
With regards
Kamesh Jayachandran



Attached patch just fixes this at ra_neon layer alone(ra_serf do not 
need the fix at least on trunk, as it do not use the PROPFIND for this 
rather uses POST which do not suffer from this issue as it simply 
proxies to MASTER).


Have not tested the full test suite, I hope this is final unless I learn 
something new.


With regards
Kamesh Jayachandran



Index: subversion/libsvn_ra_neon/ra_neon.h
===
--- subversion/libsvn_ra_neon/ra_neon.h (revision 900319)
+++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
@@ -447,6 +447,7 @@
  const char *url,
  int depth,
  const char *label,
+ svn_boolean_t for_commit,
  const ne_propname *which_props,
  apr_pool_t *pool);
 
@@ -455,6 +456,7 @@
   svn_ra_neon__session_t *sess,
   const char *url,
   const char *label,
+  svn_boolean_t for_commit,
   const ne_propname *which_props,
   apr_pool_t *pool);
 
@@ -491,6 +493,7 @@
 svn_ra_neon__session_t *sess,
 const char *url,
 const char *label,
+svn_boolean_t for_commit,
 const ne_propname *propname,
 apr_pool_t *pool);
 
Index: subversion/libsvn_ra_neon/props.c
===
--- subversion/libsvn_ra_neon/props.c   (revision 900319)
+++ subversion/libsvn_ra_neon/props.c   (working copy)
@@ -495,6 +495,7 @@
  const char *url,
  int depth,
  const char *label,
+ svn_boolean_t for_commit

RE: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2

2010-01-18 Thread Kamesh Jayachandran


Hi Kamesh.

What does this patch do?

It adds the magical header 'SVN-COMMIT' for PROPFIND calls associated with a 
commit operation over neon.

Server catches this magical header and decides whether to proxy or not.

As I mentioned already we need to do this only for neon as serf do not make 
PROPFIND and the POST it does already gets proxied.

With regards
Kamesh Jayachandran

RE: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2

2010-01-18 Thread Kamesh Jayachandran


Sorry, Kamesh, that wasn't a very helpful question.

I know the aim is to to fix the problem that occurs during a commit
through a proxy server - that's the one-line overview. I know the patch
adds new parameters to a bunch of functions and sets them to particular
values and such like - I can see that by reading the patch. What I meant
was please can you show us the log message, tell us what protocol
changes it makes, what specific cases it fixes and what it doesn't, say
whether it addresses any of the points Mike raised and in what ways,
that sort of thing.

The patch is in concept complete(Yet to run through the testsuite and to 
justify few cases by exercising those code base, which I will do tomorrow) to 
make the indication of 'commit' operation via the new header 'SVN-COMMIT'. With 
this patch original problem gets fixed.

All the function signature changes just accepts a new boolean parameter 
'for_commit' which is set to TRUE only by commit.c:apply_revprops() while 
others set it to FALSE and hence no SVN-COMMIT header.

As it was a in concept patch I just did not write a log message, sorry for that.

With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV) Version 2

2010-01-13 Thread Kamesh Jayachandran

On 01/13/2010 12:08 AM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:

Hi All,

Last week I posted the patch to implement 'svn client' to identify the
svn operation they are about to do with a given ra_session.

Following thread has the detailed discussion.

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e

Below is the summary:

Concern/Suggestion 1:
Michael Pilato and Philip Martin were suggesting to tweak
libsvn_ra_(serf|neon) to detect the operation rather than making a
change across all layers from the simplicity point of view.

My answer to 1:
I feel it would be too hackish to tweak one general API inside these
modules to flag 'commit or write' operation to the server when the
solution I propose handles this in a transparent way.

I'm sorry, but did you say transparent? What's transparent about bubbling
an RA-layer hack all the way up into the client?! A transparent solution

This patch is *not* an RA layer hack rather a transparent generic
feature so I see nothing wrong in propagating it to higher layers.

is one that preserves the existing transparency of the mirroring subsystem.

I do *not* like to expose the back-end internals to higher layers
especially when it is not so common setup and not normally supposed to
know(I mean why my client should know the repository it commits to is a
mirror directly or indirectly).

A transparent solution is one that doesn't allow ignorant third-party
consumers of the Subversion APIs to accidentally break their proxy setups
because they decide they wanted to pass checkin instead of commit as the
innocuous-appearing 'high_level_svn_operation' parameter.

Yes I agree. I was concerned about the *magical flag deep inside the
code* for only one operation, now it looks like the only way to go.

There *must* be a better way to do this.

subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:

/* ### we should use DAV:apply-to-version on the CHECKOUT so we can skip
### retrieval of the baseline */

I looked a little bit into this. IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request. Currently,
though, mod_dav_svn doesn't handle the flag. So there's still a server-side
change needed, but at least its one that would take us closer to better
WebDAV handling. Maybe you could explore this option instead?

I will take a fresh look at this problem and a independent patch in
this way.

Thanks
With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)

2010-01-08 Thread Kamesh Jayachandran


On 01/06/2010 09:09 PM, C. Michael Pilato wrote:

Philip Martin wrote:
   

Kamesh Jayachandrankam...@collab.net  writes:

 

This patch is with respect to the original thread

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/browser
   

This one I suppose:

http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/4b41f1bd.8090...@collab.net

It includes:

We can proxy this request to the Master but we *should not* do
 that if it is for read operation.
 

With all due respect, the proposed solution looks enormous compared to the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
dont-proxy-this without even touching the RA (and higher) APIs?

   


May be I can tweak 'svn_ra_neon__get_one_prop' which I believe the one 
that makes the problematic PROPFIND call.


Though the quantum of change would be relatively smaller than my 
original patch, I do *not* like it as it looks too secretive to set one 
custom flag deep inside the code for one special case.


My patch has the additional(tangential to its original intension) 
benefit of 'administrative control of activity based on operation names'.


With regards
Kamesh Jayachandran

svn ps versioned_prop val url_to_resource does not work

2010-01-07 Thread Kamesh Jayachandran


Hi All,

svn ps versioned_prop val url_to_resource fails!

Yes 'svn help ps' does not say that it supports it.

But it has some code as if it supports it but fails.

I have the following in-progress patch(It is not fully complete but 
works with ra_local and unauthenticated http).


Currently this patch relies on '-r' to tell about base revision for this 
commit.


Just thought of recording it here.

With regards
Kamesh Jayachandran
Index: subversion/svn/propset-cmd.c
===
--- subversion/svn/propset-cmd.c(revision 896770)
+++ subversion/svn/propset-cmd.c(working copy)
@@ -126,13 +126,6 @@
   rev, opt_state-force, ctx,
   scratch_pool));
 }
-  else if (opt_state-start_revision.kind != svn_opt_revision_unspecified)
-{
-  return svn_error_createf
-(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _(Cannot specify revision for setting versioned property '%s'),
- pname);
-}
   else  /* operate on a normal, versioned property (not a revprop) */
 {
   apr_pool_t *iterpool;
@@ -181,13 +174,22 @@
 {
   const char *target = APR_ARRAY_IDX(targets, i, const char *);
   svn_commit_info_t *commit_info;
+  svn_revnum_t base_revision_for_url = SVN_INVALID_REVNUM;
 
   svn_pool_clear(iterpool);
   SVN_ERR(svn_cl__check_cancel(ctx-cancel_baton));
+  if (opt_state-start_revision.kind != svn_opt_revision_unspecified)
+{
+  base_revision_for_url = opt_state-start_revision.value.number;
+  SVN_ERR(svn_cl__make_log_msg_baton((ctx-log_msg_baton3),
+ opt_state, ,
+ ctx-config, iterpool));
+}
+
   SVN_ERR(svn_cl__try(svn_client_propset3
   (commit_info, pname_utf8, propval, target,
opt_state-depth, opt_state-force,
-   SVN_INVALID_REVNUM, opt_state-changelists,
+   base_revision_for_url, opt_state-changelists,
NULL, ctx, iterpool),
   NULL, opt_state-quiet,
   SVN_ERR_UNVERSIONED_RESOURCE,
@@ -195,7 +197,13 @@
   SVN_NO_ERROR));
 
   if (! opt_state-quiet)
-svn_cl__check_boolean_prop_val(pname_utf8, propval-data, 
iterpool);
+{
+  if (opt_state-start_revision.kind != 
svn_opt_revision_unspecified)
+{
+  SVN_ERR(svn_cl__print_commit_info(commit_info, iterpool));
+}
+  svn_cl__check_boolean_prop_val(pname_utf8, propval-data, 
iterpool);
+}
 }
   svn_pool_destroy(iterpool);
 }
Index: subversion/svn/main.c
===
--- subversion/svn/main.c   (revision 896770)
+++ subversion/svn/main.c   (working copy)
@@ -875,7 +875,7 @@
svn:needs-lock properties cannot be set on a directory.  A 
non-recursive\n
attempt will fail, and a recursive attempt will set the property\n
only on the file children of the directory.\n),
-{'F', opt_encoding, 'q', 'r', opt_targets, 'R', opt_depth, opt_revprop,
+{'F', opt_encoding, 'q', 'r', SVN_CL__LOG_MSG_OPTIONS, opt_targets, 'R', 
opt_depth, opt_revprop,
  opt_force, opt_changelist },
 {{'F', N_(read property value from file ARG)}} },

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)

2010-01-07 Thread Kamesh Jayachandran




With all due respect, the proposed solution looks enormous compared to the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
dont-proxy-this without even touching the RA (and higher) APIs?

   


I *think* neon still uses the old protocol i.e do not make POST requests 
so suffers from original PROPFIND problem and hence need our fix.


*serf* uses 'POST' request to get the baseline revision, 'POST' request 
is proxied to MASTER so it does not suffer from this problem.


With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)

2010-01-07 Thread Kamesh Jayachandran


On 01/07/2010 07:53 PM, C. Michael Pilato wrote:

Kamesh Jayachandran wrote:
   
 

With all due respect, the proposed solution looks enormous compared to
the
size of the problem.   Does the original problem exist in HTTPv2?  At a
minimum, could the ra_dav providers not annotate the PROPFIND as
dont-proxy-this without even touching the RA (and higher) APIs?


   

I *think* neon still uses the old protocol i.e do not make POST requests
so suffers from original PROPFIND problem and hence need our fix.

*serf* uses 'POST' request to get the baseline revision, 'POST' request
is proxied to MASTER so it does not suffer from this problem.
 

Yes, that's correct.  Neon only grew partial HTTPv2 support, mostly for the
read operations, but definitely *not* for commits.  If switching to POST
will fix this, then maybe that's the least invasive approach -- you (or
someone) simply needs to get Neon doing HTTPv2 for commit stuffs.

Another option (and I've not researched this, so it might be bogus) is to
look into the target of that problematic PROPFIND.  I mean, is the default
VCC the only resource that can answer the query?  If not, could perhaps some
obviously-commit-related resource answer it (something in the activity,
perhaps)?  Do you happen to know exactly what is being asked for in that
PROPFIND today?

   


Request look like this,
PROPFIND /svn2/demo/!svn/vcc/default HTTP/1.1^M


?xml version=1.0 encoding=utf-8?
propfind xmlns=DAV:
prop
checked-in xmlns=DAV:/
/prop/propfind


With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)


On 01/06/2010 06:41 PM, Senthil Kumaran S wrote:

C. Michael Pilato wrote:
   

Kamesh Jayachandran wrote:
 

With  Without this patch mergeinfo_test-8 fails both over ra_neon and
ra_serf.
   

Ooh!  I was *just* about to start debugging my mergeinfo_test-8 failures on
the issue-3242-dev branch when I happened to see this part of your mail.
I'll still debug, but it's good to know that perhaps the failures aren't
local to my branch.
 

Just to note, the test passes in ra_local only.
   


It fails *only* with dav layers.

With regards
Kamesh Jayachandran

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)


Julian,


Let me explain the strategy.

mod_dav_svn on the proxy should identify whether a given PROPFIND is for 
commit or *not*, we are not bothered about any of the other read 
operation intricasies.


First I had a plan to persist the occurence of preceding 'MKACTIVITY' on 
the connection pool to classify subsequent PROPFIND on same connection 
as 'commit PROPFIND'.


Justin was not happy about this as he was saying some bad old clients 
can make each requests in its own connection so this detection can be 
faulty for those poor clients.


So this new SVN-ACTION request header came.

The patch attached to the Philips response earlier applies 'SVN-ACTION' 
based detection and if it fails defaults to connection based detection.


Once detected a PROPFIND as for commit we just proxy it.

Hope this explains.

With regards
Kamesh Jayachandran


On 01/06/2010 07:35 PM, Julian Foad wrote:

Kamesh,

Can you explain your strategy for fixing the original commit via
outdated proxy issue, and why this change is part of that strategy?
Something about this approach doesn't feel right to me, as I don't see
how a single word can accurately convey the proxy semantics of a
   


Client never need to know anything about the proxy.


high-level command. I am wondering whether it is even possible to make
the proxy do what you want without having total control over the
   


We can, we have two detection strategies explained above which should 
catch most of the clients(even the ones which do *not* set SVN-ACTION 
header).



clients.

(And if you do want to make some APIs take a high-level operation text
field, you need to specify what values are allowed and required - e.g.
does it have to be one of a fixed set of values, or any restrictions on
the length and what characters can be used, and whether the value has to
be known by the server, etc.)
   


Why not allow arbitrary value, let us say some fancy svn app can create 
custom workflow and give its own identifier say 
'merge_aware_revision_graph etc.'


As long as they are not calling 'commit' operation as a something else 
we are fine(Even then our backup logic would catch).


With regards
Kamesh Jayachandran

- Julian


Bert Huijben wrote:
   
 

-Original Message-
From: Kamesh Jayachandran [mailto:kam...@collab.net]
Sent: woensdag 6 januari 2010 14:00
To: dev@subversion.apache.org
Subject: [PATCH] Make svn clients indicate their operation name to
backend(right now only to DAV)

Hi All,

This patch is with respect to the original thread

http://mail-archives.apache.org/mod_mbox/subversion-
dev/201001.mbox/browser

Once this patch gets committed I can commit the mod_dav_svn change to
handle the original commit via outdated proxy issue.

This Patch revs the following public APIs,

'svn_client_uuid_from_url', 'svn_client_open_ra_session' and
'svn_ra_open3'.
   

I have a few high level questions about this patch:

Why do you rev svn_client_uuid_from_url?

I would think that that function is a high level API, so it would be an
operation by itself.

(looking at svn_client.h) What should I put in there as client that just
needs the uuid or verify that the repository exists?

checking-uuid-for-visualization-to-my-great-users?

I don't think we should rev the svn_client_ API for this specific change
here; especially since older clients will not pass anything anyway.
libsvn_client should fill that high level operation for library users or the
value is of no use on the server.

And it should never be forwarded to master servers as the uuid is supposed
to be constant per repository.

(BTW. the api is new in 1.7, so it needs no revving at all)


Then on to the rest of the patch:
 

For ra_neon and ra_serf layers it sets the http client header SVN-ACTION
with the concerned svn command name.

   

If the operation is a plain string that can be set by any future client, how
is the server to understand what the user wants? How can the server
understand a new 'shelve' command we might add in Subversion 1.9?

mod_dav_svn only knows RA operations and doesn't understand high level
commands; we would have to add this knowledge.


Shouldn't the individual RA operations tell whether the user needs access to
the master or the slave?

Thinking a bit further about that last issue... What if the session is
reused for e.g. requests like 'svn info', 'svn update' and then a 'svn
commit'.
Our standard client libsvn_client can't do this, but other clients can
certainly do that.

There is nothing in the ra api that forbids using it that way, but just
specifying a high level operation at open time doesn't tell enough about
what the clients application intent is.

Maybe we should just add a boolean to requests indicating whether to forward
to a master? That seems like a much simpler solution, that we could possibly
port back to older subversion releases.

Bert

 

With  Without this patch mergeinfo_test-8 fails both over ra_neon and
ra_serf

Re: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)

On 01/06/2010 06:58 PM, Bert Huijben wrote:

-Original Message-
From: Kamesh Jayachandran [mailto:kam...@collab.net]
Sent: woensdag 6 januari 2010 14:00
To: dev@subversion.apache.org
Subject: [PATCH] Make svn clients indicate their operation name to
backend(right now only to DAV)

Hi All,

This patch is with respect to the original thread

http://mail-archives.apache.org/mod_mbox/subversion-
dev/201001.mbox/browser

Once this patch gets committed I can commit the mod_dav_svn change to
handle the original commit via outdated proxy issue.

This Patch revs the following public APIs,

'svn_client_uuid_from_url', 'svn_client_open_ra_session' and
'svn_ra_open3'.

First We need this change as an easy means to tell the server what the 
client's intensions are.

We are not bothered about the value of SVN-ACTION except when it is 
'commit'.

If the user uses old client(1.7) we have a fall back mechanism in place 
to detect.

This fall back mechanism *relies* on connection persistence which is not 
a reliable assumption as per Justin.

So we have this per request logging.

I have a few high level questions about this patch:

Why do you rev svn_client_uuid_from_url?

I would think that that function is a high level API, so it would be an
operation by itself.

(looking at svn_client.h) What should I put in there as client that just
needs the uuid or verify that the repository exists?

You can put anything you like that identifies the new operation you come 
up with.

Suppose some gui svn app has a feature by name 'merge aware revision 
graph' a custom implementation can open the ra_session with the string 
'merge-aware-revision-graph' So that server admin can identify it if needed.

checking-uuid-for-visualization-to-my-great-users?

Yes, it can be any string.

I don't think we should rev the svn_client_ API for this specific change
here; especially since older clients will not pass anything anyway.
libsvn_client should fill that high level operation for library users or the
value is of no use on the server.

We need to rev this as we do not want to know 'micro operations like 
getting UUID' rather 'SOME custom command which makes use of this 
self-contained utility function'.

And it should never be forwarded to master servers as the uuid is supposed
to be constant per repository.

Client never *knows* about the master.

This implementation is just one Broad level identification tied to sub 
requests helping with big operation.

(BTW. the api is new in 1.7, so it needs no revving at all)

Yes it checked it seemed to be there since 2003.

Then on to the rest of the patch:

For ra_neon and ra_serf layers it sets the http client header SVN-ACTION
with the concerned svn command name.

If the operation is a plain string that can be set by any future client, how
is the server to understand what the user wants? How can the server
understand a new 'shelve' command we might add in Subversion 1.9?

Server do not bother about at it, It just gets the clue only when the 
operation is 'commit'.

It *can* be useful for some server admin to keep track of how his server 
resources are utilised.

May be if he sees some operation by name 
'SERVER-RESOURCE-INTENSIVE-OPERATION' as its SVN-ACTION header he can 
deny the connection atleast temporarily till he equips his server to 
handle this intensive operation.

mod_dav_svn only knows RA operations and doesn't understand high level
commands; we would have to add this knowledge.

Shouldn't the individual RA operations tell whether the user needs access to
the master or the slave?

No ra sessions do not even know the existence of slave. Only proxy knows 
that it is a proxy *not* anyone else.

Thinking a bit further about that last issue... What if the session is
reused for e.g. requests like 'svn info', 'svn update' and then a 'svn
commit'.
Our standard client libsvn_client can't do this, but other clients can
certainly do that.

There is nothing in the ra api that forbids using it that way, but just
specifying a high level operation at open time doesn't tell enough about
what the clients application intent is.

May be we can implement the svn_ra_redescribe or something to tell its 
new intensions.

Maybe we should just add a boolean to requests indicating whether to forward
to a master? That seems like a much simpler solution, that we could possibly
port back to older subversion releases.

No need, clients can/should not know anything about Master.

With regards
Kamesh Jayachandran

RE: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)


Let me see if I've got this right.  There is a MKACTIVITY that goes to
the master, then a PROPFIND that goes to the slave, and finally a
CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
master because they are obviously write operations, the PROPFIND goes
to the slave because it cannot be distinguished from a read-only
operation that doesn't need to access the master.  The PROPFIND
returns out-of-date info that causes the CHECKOUT to fail.

Yes you got it correct.

You mentioned an objection from Justin related to old clients, is this
it:

http://mail-archives.apache.org/mod_mbox/subversion-dev/200912.mbox/5c902b9e0912220853j110d893ewe412259e48b7b...@mail.gmail.com

Yes.

Is that about old clients or is it more about things like serf that
use multiple connections?

It can even happen to neon connections if the client exceeded the maximum 
requests allowed per connection.

Or some forced closure of the connection due to intermittent network issues on 
client or server end.

With regards
Kamesh Jayachandran

RE: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)


Let me see if I've got this right.  There is a MKACTIVITY that goes to
the master, then a PROPFIND that goes to the slave, and finally a
CHECKOUT that goes to master.  The MKACTIVITY and CHECKOUT go to the
master because they are obviously write operations, the PROPFIND goes
to the slave because it cannot be distinguished from a read-only
operation that doesn't need to access the master.  The PROPFIND
returns out-of-date info that causes the CHECKOUT to fail.

 Yes you got it correct.

So, on the client side libsvn_ra_neon and libsvn_ra_serf could track
whether the PROPFIND needs to go to the master or the slave and then
add a suitable header to the request.  libsvn_client doesn't need to
change (which is what I think Justin and Mike are suggesting).

NO, Client is unaware of existence of master or proxying scheme at all.

Only the proxy knows the existence of Master.

With regards
Kamesh Jayachandran

RE: [PATCH] Make svn clients indicate their operation name to backend(right now only to DAV)