Re: Input validation observations
Julian Foad julian.f...@wandisco.com writes:
On Sat, 2010-12-04, Noorul Islam K M wrote:
Julian Foad julian.f...@wandisco.com writes:
Noorul Islam K M wrote:
Julian Foad julian.f...@wandisco.com writes:
* svn info ^/b - Not a valid URL; should be path '/b' not found
in revision REV?
[...]
- SVN_ERR(svn_cmdline_fprintf
- (stderr, subpool,
- _(%s: (Not a valid URL)\n\n),
- svn_path_is_url(truepath)
- ? truepath
- : svn_dirent_local_style(truepath, pool)));
+ SVN_ERR(svn_cmdline_fprintf (stderr, subpool,
+ _(%s\n\n),
err-message));
Unfortunately we cannot assume that err-message is a good user-friendly
description of the problem. It appears that it *is* in the specific
case we're looking at:
$ svn info ^/b
URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
1041775
That's lovely. But in other cases it's not:
$ svn info hhh://aa.cc.dd/foo
traced call
See how err-message is just traced call in this case. We can't even
assume it is not NULL, in fact.
I can think of two options. We could try to use one of the
svn_error_*() functions to print out a nice description of the actual
returned error. Alternatively, we could ignore 'err' and print a simple
message here, like the existing code is doing but saying something more
helpful than Not a valid URL. What do you think?
Does the documentation for svn_client_info3() say under what conditions
it returns the error code SVN_ERR_RA_ILLEGAL_URL? Does that help?
Attached is the updated patch using svn_err_best_message()
Hi Noorul.
Thank you for the updated patch. Even though this is a very
simple-looking patch, I need a bit more information to help me review
it.
Do you think both of the options I suggested are possible solutions?
What are the advantages of the option you chose? What disadvantages do
you know about? What are the risks with it - in what ways could it
possibly go wrong or make a user unhappy? For example, what other error
conditions might this code possibly handle? Which of them did you try?
Show us the results. Do you think they are user-friendly?
One of the solution that you suggested was to keep what the existing
code is doing but saying something more helpful than Not a valid
URL. I thought that the error returned by the API might have useful
information and could be printed using svn_err_best_message(). For
example, take the following two cases.
1. a) With the patch
noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn
info
^/non-existing
URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0
svn: A problem occurred; see other errors for details
1. b) Without the patch
noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn
info
^/non-existing
file:///tmp/latestrepo/non-existing: (Not a valid URL)
svn: A problem occurred; see other errors for details
2. a) With the patch
noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn
info
invalid://no-domain
Unrecognized URL scheme for 'invalid://non-domain'
svn: A problem occurred; see other errors for details
2. b) Without patch
noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn
info
invalid://no-domain
invalid://no-domain: (Not a valid URL)
svn: A problem occurred; see other errors for details
In both the above cases the patch helps the user to have better
information (what actually went wrong). Also If a user is using the
client API, I think he will be having these messages than the one that
we hard coded as of today.
I ran the test suite and found that one of the test cases was failing
and I fixed the same. There were no other failures.
Checking those sorts of things normally takes much more effort than
actually writing the few lines of source code. That's all part of
making a patch. Whenever you send a patch, it's helpful to say these
things. When the reviewer knows what's already been checked, he or she
can then concentrate on verifying the correctness of the reasoning and
of the code.
Please take a little extra time to provide this help.
I will keep this in mind.
Please find attached the updated patch.
Log
[[[
Make svn info to display the actual error message returned by API when
illegal URL is passed.
* subversion/svn/info-cmd.c
(svn_cl__info): Display the actual error message returned by
svn_client_info3() instead of a custom one.
* subversion/tests/cmdline/basic_tests.py
(info_nonexisting_file): Modify test case
Patch by: Noorul Islam K M noorul{_AT_}collab.net
]]]
Thanks and Regards
1.6 compatibility copied=TRUE and deletes
In 1.6 svn_wc_entry_t had a 'copied' field that was used to distinguish copies from adds. In 1.7 we have compatibility code that constructs 1.6-like entries from the 1.7 metadata, so it has to set the copied flag. In 1.6 this flag gets set on all the entries in a copied tree, so 1.7 does the same. However if nodes in the copied tree are deleted 1.7 clears the copied flag for those nodes. There are comments scattered about: entries_tests.py:237 # case (1) of the DELETED nodes COPIED handling (see comment in # read_entries). we are a deletion of a copied subtree. thus, extra # work at commit time. thus, not COPIED. # oh, and this sucker has a URL, too entries.c:340 Not so fast. Back to the general concept of an ancestor operation took care of me. Further consider two possibilities: 1) this node is scheduled for deletion from the copied subtree, so at commit time, we copy then delete 2) this node is scheduled for deletion because a subtree was deleted and then a copied subtree was added (causing a replacement). at commit time, we delete a subtree, and then copy a subtree. we do not need to specifically touch this node -- all operations occur on ancestors. Given the ancestor operation concept, then in case (1) we must *clear* the COPIED flag since we'll have more work to do. In case (2), we *set* the COPIED flag to indicate that no real work is going to happen on this node. The problem I have is that 1.6 does not behave this way. In 1.6 the copied flag is set on deletes within a copy. I don't understand why 1.7 is doing it differently. My guess is that at some point in the past the copied flag was used internally by the WCNG code. However these days the compatibility code is just a read-only view of the metadata for clients that want to use the 1.6 interface, setting/clearing it has no effect on 1.7 operations. Can anyone recall why 1.7 is different? -- Philip
[PATCH] Very minor fix in comment, follow-up to r873134
Log
[[[
Follow-up to r873134. Update comment with correct function name.
* subversion/svn/copy-cmd.c
(svn_cl__copy): Fix comment
Patch by: Noorul Islam K M noorul{_AT_}collab.net
]]]
Index: subversion/svn/copy-cmd.c
===
--- subversion/svn/copy-cmd.c (revision 1042948)
+++ subversion/svn/copy-cmd.c (working copy)
@@ -79,7 +79,7 @@
SVN_ERR(svn_cl__eat_peg_revisions(targets, targets, pool));
/* Figure out which type of trace editor to use.
- If the src_paths are not homogeneous, setup_copy will return an error. */
+ If the src_paths are not homogeneous, try_copy will return an error. */
src_path = APR_ARRAY_IDX(targets, 0, const char *);
srcs_are_urls = svn_path_is_url(src_path);
dst_path = APR_ARRAY_IDX(targets, targets-nelts - 1, const char *);
Re: svn commit: r1040832 - Port a fix for a FSFS packing race
I (Julian Foad) wrote: [...] Studying the FSFS source code for the issues raised in this thread has given me confidence that it seems to be doing the right thing, in practice, at the moment. In 1043360 I added some comments about how to remove one source of fragility in using the cached value. Again I'll say this looks perfectly correct in its current usage. I'll go ahead and apply the remove-retry-logic-from-path-rev-absolute patch now. Committed revision 1043408. - Julian
Re: Input validation observations
Noorul Islam K M wrote:
Ran test suite and found some failures and fixed them. All of them were
passing URL to 'svn up' which is not at all required. Now all tests are
passing with the below attached patch.
Thanks, Noorul. That's lovely. Committed revision 1043409.
- Julian
Log
[[[
Make 'svn update' verify that URLs are not passed as targets.
* subversion/svn/update-cmd.c,
subversion/libsvn_client/update.c:
(svn_cl__update, svn_client_update4): Raise an error if a URL was
passed. Remove code that notifies 'Skipped' message for URL targets.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_update_targets, test_list): New test
* subversion/tests/cmdline/externals_tests.py,
subversion/tests/cmdline/svntest/actions.py,
(cannot_move_or_remove_file_externals,
can_place_file_external_into_dir_external,
external_into_path_with_spaces,
inject_conflict_into_wc): Remove URL target from 'svn up' command.
* subversion/tests/cmdline/basic_tests.py
(basic_update): URLs are no longer accepted as targets. Remove test
case for 'Skipped' message.
Patch by: Noorul Islam K M noorul{_AT_}collab.net
]]]
[PATCH] fix a compilation warning
Hi,
I have attached a patch with a minor change which fixes a compiler
warning.
Thanks and regards
Prabhu
Index: subversion/libsvn_client/update.c
===
--- subversion/libsvn_client/update.c (revision 1041694)
+++ subversion/libsvn_client/update.c (working copy)
@@ -180,7 +180,7 @@
relocate our working copy first. */
if (corrected_url)
{
- SVN_ERR(svn_client_relocate(anchor_abspath, anchor_url, corrected_url,
+ SVN_ERR(svn_client_relocate2(anchor_abspath, anchor_url, corrected_url,
TRUE, ctx, pool));
anchor_url = corrected_url;
}
[[[
fix a compiler warning.
* subversion/libsvn_client/update.c
(update_internal): changed the 'relocate' to 'relocate2' since 'relocate' is deprecated.
Patch by: Prabhu Gnana Sundar prabh...@collab.net
Suggested by: Kamesh Jayachandran kam...@collab.net
]]]
Re: [PATCH] fix a compilation warning
Prabhu Gnana Sundar wrote:
I have attached a patch with a minor change which fixes a compiler
warning.
Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
replacement for svn_client_relocate() in this case? What is difference
between svn_client_relocate() and svn_client_relocate2()? Does it
matter? Can you think of any way of testing or verifying it?
- Julian
[[[
fix a compiler warning.
* subversion/libsvn_client/update.c
(update_internal): changed the 'relocate' to 'relocate2' since
'relocate' is deprecated.
Patch by: Prabhu Gnana Sundar prabh...@collab.net
Suggested by: Kamesh Jayachandran kam...@collab.net
]]]
Index: subversion/libsvn_client/update.c
===
--- subversion/libsvn_client/update.c (revision 1041694)
+++ subversion/libsvn_client/update.c (working copy)
@@ -180,7 +180,7 @@
relocate our working copy first. */
if (corrected_url)
{
- SVN_ERR(svn_client_relocate(anchor_abspath, anchor_url, corrected_url,
+ SVN_ERR(svn_client_relocate2(anchor_abspath, anchor_url, corrected_url,
TRUE, ctx, pool));
anchor_url = corrected_url;
}
Re: [PATCH] fix a compilation warning
Julian Foad julian.f...@wandisco.com writes: On Wed, 2010-12-08 at 14:26 +, Julian Foad wrote: Prabhu Gnana Sundar wrote: I have attached a patch with a minor change which fixes a compiler warning. Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in replacement for svn_client_relocate() in this case? What is difference between svn_client_relocate() and svn_client_relocate2()? Does it matter? Can you think of any way of testing or verifying it? Did you run the test suite? Does the test suite exercise this code path? That's the http redirect code, tested by redirect_tests.py. The patch is correct. -- Philip
Re: [PATCH] fix a compilation warning
On Wed, 2010-12-08, Philip Martin wrote: Julian Foad julian.f...@wandisco.com writes: On Wed, 2010-12-08 at 14:26 +, Julian Foad wrote: Prabhu Gnana Sundar wrote: I have attached a patch with a minor change which fixes a compiler warning. Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in replacement for svn_client_relocate() in this case? What is difference between svn_client_relocate() and svn_client_relocate2()? Does it matter? Can you think of any way of testing or verifying it? Did you run the test suite? Does the test suite exercise this code path? That's the http redirect code, tested by redirect_tests.py. The patch is correct. Great. Thanks, Philip. I over-reacted and made a mistake. I immediately looked at the doc string of svn_client_relocate() and saw the words dir is required to be a working copy root, and mis-read it as specifying a restriction of relocate2() compared to relocate(). As I did not know whether Prabhu had verified or tested anything, that misunderstanding made me suspect that the patch was broken. Prabhu: I'm sorry I responded with a barrage of questions. It appears your patch is fine. In the future, if you could say just a few words about what investigation and/or testing you have done, each time you submit a patch, that would help me to know what level of confidence I should have when I start looking at it. Thanks in advance. And in the future I'll try not to be so hasty in my responses. - Julian
Re: [PATCH] fix a compilation warning
FWIW, my concerns here are:
* svn_client_relocate{,2} have the same signature. This might be
confusing sometimes. (but probably should be left alone)
* svn_client_relocate2() takes an IGNORE_EXTERNALS parameter. Should
we pass TRUE always to that parameter, or should we pass the
identically-named parameter of the calling function? (the calling
function *happens* to have an appropriately-named parameter, but
I haven't checked its semantics)
Daniel
Julian Foad wrote on Wed, Dec 08, 2010 at 15:09:32 +:
On Wed, 2010-12-08, Philip Martin wrote:
Julian Foad julian.f...@wandisco.com writes:
On Wed, 2010-12-08 at 14:26 +, Julian Foad wrote:
Prabhu Gnana Sundar wrote:
I have attached a patch with a minor change which fixes a compiler
warning.
Hi Prabhu. How do you know that svn_client_relocate2() is a drop-in
replacement for svn_client_relocate() in this case? What is difference
between svn_client_relocate() and svn_client_relocate2()? Does it
matter? Can you think of any way of testing or verifying it?
Did you run the test suite? Does the test suite exercise this code
path?
That's the http redirect code, tested by redirect_tests.py. The patch
is correct.
Great. Thanks, Philip. I over-reacted and made a mistake. I
immediately looked at the doc string of svn_client_relocate() and saw
the words dir is required to be a working copy root, and mis-read it
as specifying a restriction of relocate2() compared to relocate(). As I
did not know whether Prabhu had verified or tested anything, that
misunderstanding made me suspect that the patch was broken.
Prabhu: I'm sorry I responded with a barrage of questions. It appears
your patch is fine.
In the future, if you could say just a few words about what
investigation and/or testing you have done, each time you submit a
patch, that would help me to know what level of confidence I should have
when I start looking at it. Thanks in advance.
And in the future I'll try not to be so hasty in my responses.
- Julian
Re: Input validation observations
Noorul Islam K M wrote: Julian Foad julian.f...@wandisco.com writes: Thank you for the updated patch. Even though this is a very simple-looking patch, I need a bit more information to help me review it. Do you think both of the options I suggested are possible solutions? What are the advantages of the option you chose? What disadvantages do you know about? What are the risks with it - in what ways could it possibly go wrong or make a user unhappy? For example, what other error conditions might this code possibly handle? Which of them did you try? Show us the results. Do you think they are user-friendly? One of the solution that you suggested was to keep what the existing code is doing but saying something more helpful than Not a valid URL. I thought that the error returned by the API might have useful information and could be printed using svn_err_best_message(). For example, take the following two cases. 1. a) With the patch noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info ^/non-existing URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0 svn: A problem occurred; see other errors for details 1. b) Without the patch noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info ^/non-existing file:///tmp/latestrepo/non-existing: (Not a valid URL) svn: A problem occurred; see other errors for details 2. a) With the patch noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info invalid://no-domain Unrecognized URL scheme for 'invalid://non-domain' svn: A problem occurred; see other errors for details 2. b) Without patch noo...@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info invalid://no-domain invalid://no-domain: (Not a valid URL) svn: A problem occurred; see other errors for details In both the above cases the patch helps the user to have better information (what actually went wrong). Also If a user is using the client API, I think he will be having these messages than the one that we hard coded as of today. I ran the test suite and found that one of the test cases was failing and I fixed the same. There were no other failures. Hi Noorul. Thank you very much for this extra information. It makes my job as a reviewer very much easier. I haven't reviewed it yet but I will get back to it soon. Checking those sorts of things normally takes much more effort than actually writing the few lines of source code. That's all part of making a patch. Whenever you send a patch, it's helpful to say these things. When the reviewer knows what's already been checked, he or she can then concentrate on verifying the correctness of the reasoning and of the code. Please take a little extra time to provide this help. I will keep this in mind. Thank you very much. I really appreciate it. - Julian
Re: [PATCH] Follow-up to r879452.
On Tue, 2010-12-07 at 21:57 +0530, Noorul Islam K M wrote: Julian Foad julian.f...@wandisco.com writes: On Tue, 2010-12-07 at 16:06 +0530, Noorul Islam K M wrote: Julian Foad julian.f...@wandisco.com writes: Noorul Islam K M wrote: Log [[[ Follow-up to r879452. * subversion/libsvn_ra_local/ra_plugin.c (svn_ra_local__obliterate_path_rev): Replace call to svn_path_join() with svn_dirent_join() function. Hi Noorul. Why? Please explain. Because svn_path_join() is deprecated. I could see similar change done in r879452. I thought it will be obvious from the log message because I mentioned the revision. The problem is that svn_dirent_join() is not a simple drop-in replacement for svn_path_join(). The doc string of svn_path_join() says: * New code should use either svn_dirent_join() (for local paths) or * svn_uri_join() (for urls) or svn_relpath_join() (for relative paths). * * @deprecated Provided for backward compatibility with the 1.6 API. So you have to work out which kind of path is being joined. Have a look at where the arguments come from and how the result is used, and read up about the three kinds of path mentioned in the doc string, and work out which one. (There is also a fourth kind of path, fspath which means a relative path starting with /, and the function svn_fspath__join(), which should also be mentioned. I'll update the doc string in a moment, to mention that option.) Also please say how you tested the change: did you run make check (which combination?)? Did you step through the code in a debugger and observe the values? Did you test it in another way? I did not test it because I thought it is an obvious change. I was confident that the change was correct. I was sure that it should be svn_dirent_join() since I was making the change to function svn_ra_local__obliterate_path_rev under subversion/libsvn_ra_local. This code is indeed in RA-local, but that doesn't mean the paths that it is processing are local disk paths - they are not. But after reading your mail, I felt my assumptions might have been wrong. So I went and checked further. It looks like this function is not implemented for ra_neon, ra_serf and ra_svn. There are two test cases for obliterate command in test/cmdline and they are all marked as SKIP. Later I realised that obliterate command is not yet part of svn command line or am I missing something? You are correct. The obliterate stuff is not implemented for the other RA layers. In addition, I might remove it all from the code base before we branch for 1.7, because it is not developed far enough to be useful. If you want to stop working on this patch, that's fine with me, or if you want to continue, that's fine too. - Julian
Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
On Mon, 2010-12-06, Blair Zajac wrote: On 12/6/10 7:47 AM, Julian Foad wrote: On Sun, 2010-12-05, Stefan Fuhrmann wrote: I've felt kind of uneasy about that issue as well. However, it would have been difficult to implement a deprecated svn_checksum_to_cstring() in terms of svn_checksum_to_cstring2(). I don't think Blair was concerned about how the function is implemented, but about the API promises. (He can correct me if I'm wrong.) Nope, that's correct. But, as I said in my previous reply in this thread, I think it's fine to just change the existing API as you have done. Well, I don't agree, for problems that it could cause later, but don't feel that strongly to argue about it :) Ok. FWIW, I'd also be very happy with a revved API, I just don't feel strongly enough to do it myself. - Julian
RE: [PATCH] enhance diff to make it behave uniformly
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
Re: [PATCH] enhance diff to make it behave uniformly
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: [PATCH] copy_tests.py - expand move_file_back_and_forth, to verify issue #3429
On Wed, Dec 8, 2010 at 3:17 PM, Julian Foad julian.f...@wandisco.com wrote: On Wed, 2010-11-17, Johan Corveleyn wrote: On Wed, Nov 17, 2010 at 2:14 AM, Johan Corveleyn jcor...@gmail.com wrote: The attached patch expands the test move_file_back_and_forth (copy_tests.py 45) as a regression test for issue#3429. [...] I just realized that I can add a similar check to move_dir_back_and_forth. Here is a second patch that expands both tests. Thank you, Johan. Committed revision 1043427. (I tweaked the comments a bit.) I have closed issue #3429. Thanks! (note: can the if svntest.main.wc_is_singledb(wc_dir) be dropped from move_dir_back_and_forth?) Because single-db is now always enabled? Yes. But it's sometimes nice to be able to go back and test older code with the current tests, so let's not hurry to remove it. Ok, got it. Cheers, -- Johan
Re: Input validation observations
Julian Foad julian.f...@wandisco.com writes:
On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
Julian Foad julian.f...@wandisco.com writes:
Noorul Islam K M wrote:
Julian Foad julian.f...@wandisco.com writes:
* svn mkdir ^/ a - Illegal repository URL 'a'; should say can't
mix URL and local targets?
[...]
Make 'svn mkdir' verify that both working copy paths and URLs are
not passed.
* subversion/svn/mkdir-cmd.c,
subversion/libsvn_client/add.c
(svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
copy paths and URLs are passed.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_mkdir_targets, test_list): New test
[...]
Index: subversion/svn/mkdir-cmd.c
===
--- subversion/svn/mkdir-cmd.c(revision 1041293)
+++ subversion/svn/mkdir-cmd.c(working copy)
@@ -48,6 +48,8 @@
+ svn_boolean_t wc_present = FALSE, url_present = FALSE;
+ int i;
@@ -56,6 +58,22 @@
+ /* Check to see if at least one of our paths is a working copy
+ path or a repository url. */
+ for (i = 0; i targets-nelts; ++i)
+{
+ const char *target = APR_ARRAY_IDX(targets, i, const char *);
+ if (! svn_path_is_url(target))
+ wc_present = TRUE;
+ else
+ url_present = TRUE;
+}
+
+ if (url_present wc_present)
+return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+_(Cannot mix repository and working copy
+ targets));
This is fine.
The same code already exists in three other files and equivalent but
different code also exists in at least delete-cmd.c and probably other
files. I think it is time to factor it out. We can do that in a
subsequent patch.
Do you mean we need to come up with new function that will do this check
and return the error message?
Yes please.
Attached is the patch which has the new functions to replace the
existing blocks. All tests pass when tested with 'make check'. No need
for test cases as they are already available. Also I have not modified
libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
forward. I will go through them again and will come up with follow-up
patch.
Log
[[[
Two new functions one each for command line and client API which checks
whether the target types are homogeneous.
* subversion/svn/cl.h,
subversion/svn/util.c
(svn_cl__assert_homogeneous_target_type): New function
* subversion/libsvn_client/client.h
subversion/libsvn_client/util.c
(svn_client__assert_homogeneous_target_type): New function
* subversion/svn/mkdir-cmd.c,
subversion/svn/delete-cmd.c,
subversion/svn/lock-cmd.c,
subversion/svn/unlock-cmd.c
(svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
Replace existing logic with call to new function
svn_cl__assert_homogeneous_target_type
* subversion/libsvn_client/delete.c,
subversion/libsvn_client/locking_commands.c,
subversion/libsvn_client/add.c
(svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
Replace existing logic with call to new function
svn_client__assert_homogeneous_target_type
Patch by: Noorul Islam K M noorul{_AT_}collab.net
]]]
Thanks and Regards
Noorul
Index: subversion/svn/cl.h
===
--- subversion/svn/cl.h (revision 1043482)
+++ subversion/svn/cl.h (working copy)
@@ -817,6 +817,11 @@
const char *path,
apr_pool_t *pool);
+/* This function checks to see if targets contain mixture of URLs
+ * and paths, returning an error if it finds a mixture of both. */
+svn_error_t *
+svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/svn/mkdir-cmd.c
===
--- subversion/svn/mkdir-cmd.c (revision 1043482)
+++ subversion/svn/mkdir-cmd.c (working copy)
@@ -48,8 +48,6 @@
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)-ctx;
apr_array_header_t *targets;
svn_error_t *err;
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(targets, os,
opt_state-targets,
@@ -58,22 +56,8 @@
if (! targets-nelts)
return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
- /* Check to see if at least one of our paths is a working copy
- path or a repository url. */
- for (i = 0; i targets-nelts; ++i)
-{
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
- if (! svn_path_is_url(target))
-wc_present = TRUE;
- else
-url_present = TRUE;
-}
+ SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
- if (url_present wc_present)
-return
Updating revision references in CHANGES
Quick question to find out what people think. In writing the CHANGES entry for 1.7, it would be useful to be able to compare with previous releases what has already gone into the various patch releases, and what hasn't. That can best happen by comparing revision numbers, but given the revision cut over when we moved to the ASF repo, such a comparison is tedious as best. Does anybody have any opinion on rewriting the revision numbers in CHANGES (assuming it could be appropriately scripted)? -Hyrum
Re: Updating revision references in CHANGES
On 08.12.2010 20:25, C. Michael Pilato wrote: On 12/08/2010 02:00 PM, Hyrum K. Wright wrote: Quick question to find out what people think. In writing the CHANGES entry for 1.7, it would be useful to be able to compare with previous releases what has already gone into the various patch releases, and what hasn't. That can best happen by comparing revision numbers, but given the revision cut over when we moved to the ASF repo, such a comparison is tedious as best. Does anybody have any opinion on rewriting the revision numbers in CHANGES (assuming it could be appropriately scripted)? I'm okay with it. There's no much gained in preserving those old revision numbers. Besides, if folks want the old numbers, they can be found in older versions of the CHANGES file, right? :-) If it's going to be HTML, might as well put in ViewVC links instead of just revision numbers. -- Brane
Re: Updating revision references in CHANGES
On Wed, Dec 8, 2010 at 9:27 PM, Branko Čibej br...@xbc.nu wrote: On 08.12.2010 20:25, C. Michael Pilato wrote: On 12/08/2010 02:00 PM, Hyrum K. Wright wrote: Quick question to find out what people think. In writing the CHANGES entry for 1.7, it would be useful to be able to compare with previous releases what has already gone into the various patch releases, and what hasn't. That can best happen by comparing revision numbers, but given the revision cut over when we moved to the ASF repo, such a comparison is tedious as best. Does anybody have any opinion on rewriting the revision numbers in CHANGES (assuming it could be appropriately scripted)? I'm okay with it. There's no much gained in preserving those old revision numbers. Besides, if folks want the old numbers, they can be found in older versions of the CHANGES file, right? :-) If it's going to be HTML, might as well put in ViewVC links instead of just revision numbers. CHANGES isn't HTML, it's always been flat text. The release notes are HTML. -Hyrum
Re: svn commit: r1040832 - Port a fix for a FSFS packing race
Julian Foad wrote on Tue, Dec 07, 2010 at 18:13:09 +:
On Tue, 2010-12-07, Daniel Shahaf wrote:
Julian Foad wrote on Tue, Dec 07, 2010 at 15:27:05 +:
(Quoting and replying to two emails at once.)
First: I'm assuming that no process will ever do packing without getting
the exclusive write lock. Can you confirm that for me? If that's
false, all my reasoning would be bogus.
Why should I confirm that? You have the code too. :-)
Heh. I meant: I am assuming that's how FSFS is *intended* to work; is
that your understanding too?
I'm not sure how it was intended to work. I know that at some point
after packing was added I pointed out that two concurrent 'svnadmin pack'
operations will cause data loss if not serialized, and in response
glasser said they should run under the write lock:
http://thread.gmane.org/gmane.comp.version-control.subversion.devel/108040/focus=108048
Daniel
(who, at the time, didn't fully appreciate the importance of using the
same write lock as other FSFS operations)
- Julian
(I have not read the rest of this email yet.)
Seriously: as of the last time I looked, svn_fs_pack() calls ... which
takes the write lock and calls pack_body(), which calls pack_shard(),
which does
cat db/revs/$shard/* db/revs/$shard.pack/pack
echo '($shard + 1) * 1000' db/min-unpacked-revprop
rm -rf db/revs/$shard/
while still holding the write lock.
On Tue, 2010-12-07 at 12:13 +0200, Daniel Shahaf wrote:
Philip Martin wrote on Tue, Dec 07, 2010 at 09:49:40 +:
Julian Foad julian.f...@wandisco.com writes:
On Mon, 2010-12-06 at 18:44 +, Philip Martin wrote:
Julian Foad julian.f...@wandisco.com writes:
I'm going to drop this Remove the re-try logic from
svn_fs_fs__path_rev_absolute() follow-up patch, as I don't want
to get
into checking or messing with the txn-correctness of FSFS now.
If
Daniel or anyone else wants to pursue it, I'd be glad to help.
I thought the patch was an improvement. The existing retry in
svn_fs_fs__path_rev_absolute doesn't fix the race, it simply makes
the
window smaller. As the patch makes the window larger we are more
likely
to see and fix any places where the caller doesn't handle the race.
I *think* the problem is that a caller may use this function within
a
transaction and depend on this internal re-try logic simply to
update a
stale cached min-unpacked-foo value. Stale in the sense that
*this*
transaction has changed the real value and hasn't updated the cached
value.
Yes, it's conceivable that a caller might do:
for (int i = 0; i 2; i++)
{
...
SVN_ERR(svn_fs_fs__path_rev_absolute());
...
if (i == 0)
/* other process runs svn_fs_pack(); */;
[...]
No, that's not what I mean. I mean a caller might do:
with a write lock:
{
svn_fs_pack();
SVN_ERR(svn_fs_fs__path_rev_absolute(path, ...));
foo(path);
}
where neither this code nor foo() re-tries. With the current version of
svn_fs_fs__path_rev_absolute(), foo() will get the correct path,
Unless a commit occurs after svn_fs_fs__path_rev_absolute() returns but
before foo() is called. (This is the condition Stefan2 fixed.)
whereas if we remove the internal re-try then foo() will get the wrong
path.
If there are any
such cases then your patch should expose them and we will fix them.
Any
such errors are easy to handle, compared to the difficulty of fixing
races.
The real problem with the existing code is that it partially hides
races, by making them harder to trigger, but doesn't fix the race. If
we want to fix the race properly making it easier to trigger is the
right thing to do.
Agreed.
What I meant to indicate is that, before making this change, I would
like to see clearly that a caller with the write lock cannot use an
out-of-date value. One way to ensure that would be:
update the cached value whenever we get the write lock (which we do);
update the cached value whenever we update the value on disk (which we
don't);
anything not using a write lock must accept that the cached value may
be stale, and re-try if necessary.
Another way would be:
don't update the cached value when we get the write lock;
don't update the cached value when we update the value on disk;
every time we use the cached value within a write lock, either update
before using it or try and re-try if necessary;
every time we use the cached value without a write lock, try and
re-try if necessary.
I don't have a strong preference between those, but the current
inconsistency
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Greetings, So I have forward-ported Stephane's patch to add recursion to SVNParentPath for 1.6.15 (see attached patch). I have also started, but had to stop, work on making it function with trunk (see also, attached patch). The 1.6.15 patch works, the trunk patch does not (some functions take different arguments now, it seems -- and that may be part of it since I excised those bits entirely). Anyways, I am hoping at minimum that this will allow the patch to be used by someone else who needs this functionality. Ideally someone with some better knowledge of the SVN coding standards and internals would finish this for us and include it into the base distribution when they have time. If I have or am given such time in the future, I will do that -- but the time isn't now unfortunately. As for Glasser's prior concern that this would change the functionality and potentially cause some security issues -- my response would be to add it to a new devel line and note it as a functional change. I don't think there's much use or safety in storing the repository in a deeper dir of something that is apache accessible anyways -- so this would effectively make things function more like I think should be expected anyways. I am going to also attach these patches to the bug below. Cheers, David Burley Systems Programmer/Analyst, Geeknet, Inc. da...@geek.net References to others with same request: http://subversion.tigris.org/issues/show_bug.cgi?id=3588 http://serverfault.com/questions/68818/is-svnparentpath-recursive Original Email: http://svn.haxx.se/dev/archive-2007-12/0280.shtml
Re: 1.7.x - merge now accesses all files in WC?
On Wed, Dec 8, 2010 at 9:27 PM, Daniel Becroft djcbecr...@gmail.com wrote: On Tue, Dec 7, 2010 at 12:39 PM, Hyrum K. Wright hyrum_wri...@mail.utexas.edu wrote: On Mon, Dec 6, 2010 at 4:28 PM, Hyrum K. Wright hyrum_wri...@mail.utexas.edu wrote: On Mon, Dec 6, 2010 at 3:15 PM, Daniel Becroft djcbecr...@gmail.com wrote: On Mon, Dec 6, 2010 at 8:50 PM, Daniel Becroft djcbecr...@gmail.com wrote: On Mon, Dec 6, 2010 at 7:13 PM, Daniel Shahaf d...@daniel.shahaf.namewrote: Instead of guessing which function causes the lstat() calls, could we have a tool tell? I've looked at 'ltrace -S', but it seems to me that will only tell us which public svn_wc_* API is responsible for the calls. Perhaps there is another tool that can help us follow the entire call chain down to the lstat() call? Daniel . o O (gdb with a breakpoint in lstat() and some fu to extract statistics about the stack trace?) Very true. Okay, after a crash course in using GDB, I've run some further testing. Running with breakpoints set on the lstat() system call, it looks to be the known issue that was originally pointed to. A partial stack trace is below: #0 0x7644ab45 in __lxstat (vers=value optimised out, name=value optimised out, buf=0x7fffcbf0) at ../sysdeps/unix/sysv/linux/wordsize-64/lxstat.c:38 #1 0x769269b6 in lstat (finfo=0x7fffccf0, fname=0x6bd638 /home/djcbecroft/dev/workingcopy/A/beta.txt, wanted=33137, pool=0x6bd588) at /usr/include/sys/stat.h:464 #2 apr_stat (finfo=0x7fffccf0, fname=0x6bd638 /home/djcbecroft/dev/workingcopy/A/beta.txt, wanted=33137, pool=0x6bd588) at file_io/unix/filestat.c:292 #3 0x76fb1199 in io_check_path ( path=0x6bd608 /home/djcbecroft/dev/workingcopy/A/beta.txt, resolve_symlinks=0, is_special_p=0x7fffced8, kind=0x7fffcedc, pool=0x6bd588) at subversion/libsvn_subr/io.c:222 #4 0x76fb1346 in svn_io_check_special_path ( path=0x6bd608 /home/djcbecroft/dev/workingcopy/A/beta.txt, kind=0x7fffcedc, is_special=0x7fffced8, pool=0x6bd588) at subversion/libsvn_subr/io.c:283 #5 0x7794513d in svn_wc__db_pdh_parse_local_abspath ( pdh=0x7fffd038, local_relpath=0x7fffd030, db=0x64ad38, local_abspath=0x6bd608 /home/djcbecroft/dev/workingcopy/A/beta.txt, smode=svn_sqlite__mode_readwrite, result_pool=0x6bd588, scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db_pdh.c:382 #6 0x77935d58 in svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188, revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0, last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0, target=0x0, changelist=0x0, original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0, original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0, db=0x64ad38, local_abspath=0x6bd608 /home/djcbecroft/dev/workingcopy/A/beta.txt, result_pool=0x6bd588, scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db.c:5261 However, I then put the breakpont in the svn_wc__db_read_info function and, just for A/beta.txt, it gets called 7 times, when in terms leads to 7 additional lstat() calls. Looking further the call stack appears as follows: #0 svn_wc__db_read_info (status=0x7fffd184, kind=0x7fffd188, revision=0x0, repos_relpath=0x0, repos_root_url=0x0, repos_uuid=0x0, changed_rev=0x0, changed_date=0x0, changed_author=0x0, last_mod_time=0x0, depth=0x0, checksum=0x0, translated_size=0x0, target=0x0, changelist=0x0, original_repos_relpath=0x0, original_root_url=0x0, original_uuid=0x0, original_revision=0x0, props_mod=0x0, have_base=0x0, have_work=0x0, conflicted=0x0, lock=0x0, db=0x64ad38, local_abspath=0x6bd608 /home/djcbecroft/dev/workingcopy/A/beta.txt, result_pool=0x6bd588, scratch_pool=0x6bd588) at subversion/libsvn_wc/wc_db.c:5259 #1 0x778fab96 in walker_helper (db=0x64ad38, dir_abspath=0x6ba288 /home/djcbecroft/dev/workingcopy/A, show_hidden=1, walk_callback=0x77ba9026 get_mergeinfo_walk_cb, walk_baton=0x7fffd4e0, depth=svn_depth_infinity, cancel_func=0x4119c3 svn_cl__check_cancel, cancel_baton=0x0, scratch_pool=0x6ba208) at subversion/libsvn_wc/node.c:679 #2 0x778facb5 in walker_helper (db=0x64ad38, dir_abspath=0x67d258 /home/djcbecroft/dev/workingcopy, show_hidden=1, walk_callback=0x77ba9026 get_mergeinfo_walk_cb, walk_baton=0x7fffd4e0, depth=svn_depth_infinity, cancel_func=0x4119c3 svn_cl__check_cancel, cancel_baton=0x0, scratch_pool=0x6a0dc8) at subversion/libsvn_wc/node.c:716 #3 0x778faf9a in svn_wc__internal_walk_children
Re: [PATCH] support printing changed properties in svnlook
[ a few observations, but I haven't reviewed all of this yet ]
Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
Hi,
Please see attached patch. Comments most welcome, specially if there
is a better/less intrusive way of implementing subject.
Please CC me as I'm not subscribed to the list.
// Erik
[[[
Support printing changed properties in svnlook by passing the -v parameter
to
the svnlook changed command. The changed properties are printed one property
per line with the format: status path;prop name
To support this, the editor created by svn_repos_node_editor has been modified
to record changes to properties (requires the replay to be done with deltas).
Do you mean: text_deltas=FALSE should be passed to svn_repos_dir_delta2()?
(Usually 'replay' refers to svn_repos_replay(), the API behind svnsync;
an editor is driven, not replayed.)
* subversion/include/svn_repos.h
(svn_repos_node_prop_t): New struct to represent a change to a property.
(svn_repos_node_t): Add pointer to list of changed properties.
* subversion/libsvn_repos/node_tree.c
(change_node_prop): Add changed properties to the node structure.
* subversion/svnlook/main.c
(cmd_table subcommand_changed do_changed): New verbose option to indicate
if changed properties should be printed or not.
(generate_delta_tree): New parameter to make the replay with or without
deltas.
(do_dirs_changed do_diff): Add new parameter to generate_delta_tree call.
(print_changed_tree): Support printing changed properties.
]]]
Log message looks good.
Index: subversion/include/svn_repos.h
===
--- subversion/include/svn_repos.h(revision 1043363)
+++ subversion/include/svn_repos.h(working copy)
@@ -2239,6 +2239,20 @@
* result of having svn_repos_dir_delta2() drive that editor.
*/
+/** A property on a node */
+typedef struct svn_repos_node_prop_t
+{
+ /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
+ char action;
+
This is copied from svn_repos_node_t-action. There was recently
a question about that field:
http://mid.gmane.org/3abd28aa-a2fc-4d7d-a502-479d37995...@orcaware.com
So, that asks whether 'C'hanged is a valid answer to the question that
-action is meant to answer. I'll also ask how this interacts with node
changes: for example; if r5 replaces-with-history a node that has
'fooprop' set with another node that also has 'fooprop' set, what would
the value of 'action' be?
+ /** The name of the property */
+ const char *name;
+
Where is the value of the property? How to get it?
+ /** Pointer to the next sibling property */
+ struct svn_repos_node_prop_t *sibling;
+
You use a linked list. How about using apr_array_header_t *? Or a hash
of (prop_name - struct)?
+} svn_repos_node_prop_t;
+
/** A node in the repository. */
typedef struct svn_repos_node_t
{
@@ -2272,6 +2286,9 @@
/** Pointer to the parent of this node */
struct svn_repos_node_t *parent;
+ /** Pointer to the first modified property */
+ svn_repos_node_prop_t *mod_prop;
+
} svn_repos_node_t;
I'm afraid you can't extend this struct due to binary compatibility
considerations (an application built against 1.6 but running against 1.7
will create too short a struct).
Usually we provide constructor (and duplicator) API's for structs
defined in the headers... but we haven't done so in this case.
Index: subversion/svnlook/main.c
Index: subversion/libsvn_repos/node_tree.c
I haven't read these parts yet.
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500: So I have forward-ported Stephane's patch to add recursion to SVNParentPath for 1.6.15 (see attached patch). The patch didn't get to the list. Please re-send it with *.txt extension. Thanks. Daniel
1.7 performance requirements for release
Do we have any performance requirements for the 1.7 release? If a particular operation, say checkout, is X times slower than the 1.6 operation, we consider it a performance regression? I feel like checkout, with a 2x performance drop which I saw in recent performance tests, is a large regression: http://svn.haxx.se/dev/archive-2010-12/0161.shtml Do we need to be faster than 1.6 to do a release? Do we want to say, all operations are at least X% faster? It's a much more powerful statement than saying, some operations are faster and some are the same speed. I don't feel that strongly about upgrade taking forever, since many people can do a fresh checkout, but I could see this being an issue for people with uncommitted changes in their working copy or changelists. Blair
Error in configure when searching for Berkeley DB
I was trying to configure with a specific Berkeley DB, and ran across this bug in configure. This is on Mac OS X with an unmodified working copy at r1043705. [[[ svn-hackers:svn-trunk Hyrum$ ./configure --enable-maintainer-mode --with-serf=/opt/local --with-apr=/opt/local --with-apr-util=/opt/local --disable-shared --with-berkeley-db=/opt/local/include/db46/db.h:/opt/local/include/db46/:/opt/local/lib/db46/:db configure: creating ./config.status config.status: creating Makefile config.status: creating tools/backup/hot-backup.py config.status: creating tools/hook-scripts/commit-access-control.pl config.status: creating subversion/bindings/swig/perl/native/Makefile.PL config.status: creating packages/solaris/pkginfo config.status: creating subversion/svn_private_config.h config.status: executing svn_private_config.h commands /opt/local/bin/gsed: -e expression #1, char 30: unknown option to `s' svn-hackers:svn-trunk Hyrum$ ]]] I'm not sure where or why sed is getting an error in its expression. This is GNU sed, version 4.2.1. -Hyrum
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Patches attached again but can also be found here:
http://subversion.tigris.org/issues/show_bug.cgi?id=3588
On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf d...@daniel.shahaf.namewrote:
David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
So I have forward-ported Stephane's patch to add recursion to
SVNParentPath
for 1.6.15 (see attached patch).
The patch didn't get to the list. Please re-send it with *.txt extension.
Thanks.
Daniel
diff -ur subversion-1.6.15.orig/subversion/libsvn_repos/repos.c
subversion-1.6.15/subversion/libsvn_repos/repos.c
--- subversion-1.6.15.orig/subversion/libsvn_repos/repos.c 2010-03-16
15:22:28.0 +
+++ subversion-1.6.15/subversion/libsvn_repos/repos.c 2010-12-07
18:13:30.173736691 +
@@ -1251,7 +1251,7 @@
* on errors (which would be permission errors, probably) so that
* we the user will see them after we try to open the repository
* for real. */
-static svn_boolean_t
+svn_boolean_t
check_repos_path(const char *path,
apr_pool_t *pool)
{
diff -ur subversion-1.6.15.orig/subversion/mod_dav_svn/repos.c
subversion-1.6.15/subversion/mod_dav_svn/repos.c
--- subversion-1.6.15.orig/subversion/mod_dav_svn/repos.c 2010-11-10
14:56:31.0 +
+++ subversion-1.6.15/subversion/mod_dav_svn/repos.c2010-12-07
18:52:09.487936650 +
@@ -1014,34 +1014,63 @@
{
/* SVNParentPath was used instead: assume the first component of
'relative' is the name of a repository. */
- const char *magic_component, *magic_end;
+ extern svn_boolean_t check_repos_path(const char *path, apr_pool_t
*pool);
+ const char *magic_component = NULL, *magic_end;
+ char *repos_path;
- /* A repository name is required here.
- Remember that 'relative' always starts with a /. */
- if (relative[1] == '\0')
-{
- /* ### are SVN_ERR_APMOD codes within the right numeric space? */
- return dav_new_error(r-pool, HTTP_FORBIDDEN,
- SVN_ERR_APMOD_MALFORMED_URI,
- The URI does not contain the name
- of a repository.);
-}
-
- magic_end = ap_strchr_c(relative + 1, '/');
- if (!magic_end)
-{
- /* ### Request was for parent directory with no trailing
- slash; we probably ought to just redirect to same with
- trailing slash appended. */
- magic_component = relative + 1;
- relative = /;
-}
- else
-{
- magic_component = apr_pstrndup(r-pool, relative + 1,
- magic_end - relative - 1);
- relative = magic_end;
-}
+ do
+ {
+ /* A repository name is required here.
+Remember that 'relative' always starts with a /. */
+ if (relative[1] == '\0')
+ {
+ /* ### are SVN_ERR_APMOD codes within the right numeric space? */
+ return dav_new_error(r-pool, HTTP_FORBIDDEN,
+ SVN_ERR_APMOD_MALFORMED_URI,
+ The URI does not contain the name
+ of a repository.);
+ }
+
+ magic_end = ap_strchr_c(relative + 1, '/');
+ if (!magic_end)
+ {
+ /* ### Request was for parent directory with no trailing
+slash; we probably ought to just redirect to same with
+trailing slash appended. */
+ if (!magic_component)
+ {
+ magic_component = relative + 1;
+ }
+ else
+ {
+ magic_component = apr_pstrcat(r-pool, magic_component,
+ relative, NULL);
+ }
+ relative = /;
+ }
+ else
+ {
+ if (!magic_component)
+ {
+ magic_component = apr_pstrndup(r-pool, relative + 1,
+magic_end - relative - 1);
+ }
+ else
+ {
+ char *tmp_magic_component;
+
+ tmp_magic_component = apr_pstrndup(r-pool, relative,
+magic_end - relative);
+ magic_component = apr_pstrcat(r-pool, magic_component,
+ tmp_magic_component, NULL);
+ }
+ relative = magic_end;
+ }
+
+ repos_path = svn_path_join(fs_parent_path, magic_component,
+r-pool);
+ }
+ while (check_repos_path(repos_path, r-pool) != TRUE);
/* return answer */
*repos_name = magic_component;
@@ -1224,6 +1252,7 @@
droot-rev = SVN_INVALID_REVNUM;
comb-priv.repos = repos;
+ repos-root_path = root_path;
repos-pool = r-pool;
Re: Error in configure when searching for Berkeley DB
Hyrum K. Wright wrote on Wed, Dec 08, 2010 at 16:52:29 -0800: I was trying to configure with a specific Berkeley DB, and ran across this bug in configure. This is on Mac OS X with an unmodified working copy at r1043705. [[[ svn-hackers:svn-trunk Hyrum$ ./configure --enable-maintainer-mode --with-serf=/opt/local --with-apr=/opt/local --with-apr-util=/opt/local --disable-shared --with-berkeley-db=/opt/local/include/db46/db.h:/opt/local/include/db46/:/opt/local/lib/db46/:db configure: creating ./config.status config.status: creating Makefile config.status: creating tools/backup/hot-backup.py config.status: creating tools/hook-scripts/commit-access-control.pl config.status: creating subversion/bindings/swig/perl/native/Makefile.PL config.status: creating packages/solaris/pkginfo config.status: creating subversion/svn_private_config.h config.status: executing svn_private_config.h commands /opt/local/bin/gsed: -e expression #1, char 30: unknown option to `s' svn-hackers:svn-trunk Hyrum$ ]]] I'm not sure where or why sed is getting an error in its expression. This is GNU sed, version 4.2.1. -Hyrum Because the header file is given via absolute path perhaps? Does this help? Index: configure.ac === --- configure.ac(revision 1042961) +++ configure.ac(working copy) @@ -1247,7 +1247,7 @@ AC_CONFIG_HEADERS(subversion/svn_private_config.h) AC_CONFIG_COMMANDS([svn_private_config.h], - [$SED -e s/@SVN_DB_HEADER@/$SVN_DB_HEADER/ subversion/svn_private_config.h subversion/svn_private_config.h.new + [$SED -e s...@svn_db_header@#$SVN_DB_HEADER# subversion/svn_private_config.h subversion/svn_private_config.h.new mv -f subversion/svn_private_config.h.new subversion/svn_private_config.h], [SED=$SED SVN_DB_HEADER=$SVN_DB_HEADER])
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
Patches attached again but can also be found here:
http://subversion.tigris.org/issues/show_bug.cgi?id=3588
On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf d...@daniel.shahaf.namewrote:
David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
So I have forward-ported Stephane's patch to add recursion to
SVNParentPath
for 1.6.15 (see attached patch).
The patch didn't get to the list. Please re-send it with *.txt extension.
Thanks.
Daniel
The 1.7.x patch doesn't apply to HEAD of trunk.
Index: subversion/mod_dav_svn/repos.c
===
--- subversion/mod_dav_svn/repos.c (revision 1043620)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1235,34 +1235,63 @@
{
/* SVNParentPath was used instead: assume the first component of
'relative' is the name of a repository. */
- const char *magic_component, *magic_end;
+ extern svn_boolean_t check_repos_path(const char *path, apr_pool_t
*pool);
+ const char *magic_component = NULL, *magic_end;
+ char *repos_path;
- /* A repository name is required here.
- Remember that 'relative' always starts with a /. */
- if (relative[1] == '\0')
-{
- /* ### are SVN_ERR_APMOD codes within the right numeric space? */
- return dav_svn__new_error(r-pool, HTTP_FORBIDDEN,
-SVN_ERR_APMOD_MALFORMED_URI,
-The URI does not contain the name
-of a repository.);
-}
+ do
+ {
+ /* A repository name is required here.
+Remember that 'relative' always starts with a /. */
+ if (relative[1] == '\0')
+ {
+ /* ### are SVN_ERR_APMOD codes within the right numeric space?
*/
+ return dav_new_error(r-pool, HTTP_FORBIDDEN,
+ SVN_ERR_APMOD_MALFORMED_URI,
+ The URI does not contain the name
+ of a repository.);
+ }
+
+ magic_end = ap_strchr_c(relative + 1, '/');
+ if (!magic_end)
+ {
+ /* ### Request was for parent directory with no trailing
+slash; we probably ought to just redirect to same with
+trailing slash appended. */
+ if (!magic_component)
+ {
+ magic_component = relative + 1;
+ }
+ else
+ {
+ magic_component = apr_pstrcat(r-pool, magic_component,
+ relative, NULL);
+ }
+ relative = /;
+ }
+ else
+ {
+ if (!magic_component)
+ {
+ magic_component = apr_pstrndup(r-pool, relative + 1,
+magic_end - relative - 1);
+ }
+ else
+ {
+ char *tmp_magic_component;
- magic_end = ap_strchr_c(relative + 1, '/');
- if (!magic_end)
-{
- /* ### Request was for parent directory with no trailing
- slash; we probably ought to just redirect to same with
- trailing slash appended. */
- magic_component = relative + 1;
- relative = /;
-}
- else
-{
- magic_component = apr_pstrndup(r-pool, relative + 1,
- magic_end - relative - 1);
- relative = magic_end;
-}
+ tmp_magic_component = apr_pstrndup(r-pool, relative,
+magic_end - relative);
+ magic_component = apr_pstrcat(r-pool, magic_component,
+ tmp_magic_component, NULL);
+ }
+ relative = magic_end;
+ }
+
+ repos_path = svn_path_join(fs_parent_path, magic_component,
+r-pool);
+ }
+ while (check_repos_path(repos_path, r-pool) != TRUE);
/* return answer */
*repos_basename = magic_component;
@@ -1881,7 +1910,7 @@
dav_resource_combined *comb;
dav_svn_repos *repos;
const char *cleaned_uri;
- const char *repo_basename;
+ const char *repo_basename = NULL;
const char *relative;
const char *repos_path;
const char *repos_key;
@@ -1897,9 +1926,15 @@
xslt_uri = dav_svn__get_xslt_uri(r);
fs_parent_path = dav_svn__get_fs_parent_path(r);
+ /* This does all the work of interpreting/splitting the request uri. */
+ err = dav_svn_split_uri(r, r-uri, root_path,
+ cleaned_uri, had_slash,
+
