Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
+1 for trunk. Thanks. On May 31, 2012 11:28 PM, "Vladimir Berezniker" wrote: > Attached please find a followup patch that fixes issues you have raised > for > CPPADDR_NULL_PTR for other macros in JNIUtil.h, so that they become > consistent. > > [[[ > JavaHL: Make handling of expr and whitespace after ret_val parameters > consistent accross macros > > [ in subversion/bindings/javahl/native ] > > * JNIUtil.h > (SVN_JNI_NULL_PTR_EX): parenthesize expr for safety > (SVN_JNI_NULL_PTR_EX, SVN_JNI_ERR, POP_AND_RETURN): eliminate unnecessary > whitespace after ret_val > ]]] > > Regards, > > Vladimir > > On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: > >> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker >> wrote: >> > Patch 01 - Introduce macro >> > >> > [[[ >> > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code >> > checking C++ pointer extracted from the java object >> > >> > [ in subversion/bindings/javahl/native ] >> > >> > * JNIUtil.h >> > (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java >> > exception if necessary >> > ]]] >> >> Replying to just this patch. The second patch seems pretty mechanical. >> And we're only looking at minor nits. >> >> (sorry, but the patch doesn't inline into this response, so let's just >> be flexible here...) >> >> >> The macro argument substitutions need to be parenthesized for safety. >> So it would be: (expr) == NULL, and it would be: return (ret_val); >> >> Next bit: the indentation in the diff seems to be off. Are there TAB >> characters in there? the JNIUTIL:: and the return lines have different >> indents in the patch that I'm looking at. That is either sloppy SPC >> character indents, or a TAB is present. >> >> Lastly, there is an extra space character before the ";" in the return >> statement. That should be eliminated. >> >> >> Fix the above three problems, and I'm +1 for you to commit just patch #1. >> >> I have not reviewed #2, but the first patch seems reasonable to >> simplify (as done in #2). I also await others to comment on the >> applicability of patch #2. >> >> I do seem to recall that C++ tried to do away with the preprocessor. >> It would be nice to follow that ideal, but looking at this macro... I >> have no idea how to map it into "proper, non-CPP concepts". If you >> know, that would be better. Otherwise... meh. CPP is just fine with me >> (and screw the C++ academic purity). >> >> Cheers, >> -g >> > >
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On May 31, 2012 11:07 PM, "Vladimir Berezniker" wrote: > On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: >>... >> (sorry, but the patch doesn't inline into this response, so let's just >> be flexible here...) >> > Sorry, I cannot figure out how to get gmail to inline attachments, I'll setup > IMAP client when I have a spare moment to to figure out which one will > work for me. Oh, attachments are best because they don't get munged in various ways. It was just that I was on my tablet (now, too), and it is near impossible to get the attachments into the body. *shrug* >> The macro argument substitutions need to be parenthesized for safety. >> So it would be: (expr) == NULL, and it would be: return (ret_val); > > > will take care of (expr). Same cannot be done for ret_val because it is > valid for the ret_val to be empty when used inside void methods. Saw that, yeah. >... > I am a java guy, with a some C/C++ understanding, afraid I won't be of > much help in this department. Most of the macro is repeat of the existing > SVN_JNI_NULL_PTR_EX with different exception being thrown. No worries. Just curious. Cheers, -g
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
+1 On May 31, 2012 11:25 PM, "Vladimir Berezniker" wrote: > I applied the patch to the javahl-ra branch instead of the trunk by > mistake, is > it ok if a cherry pick it onto trunk? > > Sorry about that, > > Vladimir > > On Thu, May 31, 2012 at 3:37 AM, Greg Stein wrote: > >> On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: >> >... >> > Fix the above three problems, and I'm +1 for you to commit just patch >> #1. >> >> To clarify: I trust you to make the three changes, and you're free to >> commit with my +1. I don't need another round of review. >> >> (if something is missing, then we can fix in a second commit) >> >> Thanks, >> -g >> > >
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
Attached please find a followup patch that fixes issues you have raised for
CPPADDR_NULL_PTR for other macros in JNIUtil.h, so that they become
consistent.
[[[
JavaHL: Make handling of expr and whitespace after ret_val parameters
consistent accross macros
[ in subversion/bindings/javahl/native ]
* JNIUtil.h
(SVN_JNI_NULL_PTR_EX): parenthesize expr for safety
(SVN_JNI_NULL_PTR_EX, SVN_JNI_ERR, POP_AND_RETURN): eliminate unnecessary
whitespace after ret_val
]]]
Regards,
Vladimir
On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote:
> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker
> wrote:
> > Patch 01 - Introduce macro
> >
> > [[[
> > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code
> > checking C++ pointer extracted from the java object
> >
> > [ in subversion/bindings/javahl/native ]
> >
> > * JNIUtil.h
> > (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java
> > exception if necessary
> > ]]]
>
> Replying to just this patch. The second patch seems pretty mechanical.
> And we're only looking at minor nits.
>
> (sorry, but the patch doesn't inline into this response, so let's just
> be flexible here...)
>
>
> The macro argument substitutions need to be parenthesized for safety.
> So it would be: (expr) == NULL, and it would be: return (ret_val);
>
> Next bit: the indentation in the diff seems to be off. Are there TAB
> characters in there? the JNIUTIL:: and the return lines have different
> indents in the patch that I'm looking at. That is either sloppy SPC
> character indents, or a TAB is present.
>
> Lastly, there is an extra space character before the ";" in the return
> statement. That should be eliminated.
>
>
> Fix the above three problems, and I'm +1 for you to commit just patch #1.
>
> I have not reviewed #2, but the first patch seems reasonable to
> simplify (as done in #2). I also await others to comment on the
> applicability of patch #2.
>
> I do seem to recall that C++ tried to do away with the preprocessor.
> It would be nice to follow that ideal, but looking at this macro... I
> have no idea how to map it into "proper, non-CPP concepts". If you
> know, that would be better. Otherwise... meh. CPP is just fine with me
> (and screw the C++ academic purity).
>
> Cheers,
> -g
>
Index: subversion/bindings/javahl/native/JNIUtil.h
===
--- subversion/bindings/javahl/native/JNIUtil.h (revision 1344562)
+++ subversion/bindings/javahl/native/JNIUtil.h (working copy)
@@ -214,9 +214,9 @@
*/
#define SVN_JNI_NULL_PTR_EX(expr, str, ret_val) \
- if (expr == NULL) { \
+ if ((expr) == NULL) { \
JNIUtil::throwNullPointerException(str);\
-return ret_val ;\
+return ret_val; \
}
/**
@@ -235,7 +235,7 @@
svn_error_t *svn_jni_err__temp = (expr);\
if (svn_jni_err__temp != SVN_NO_ERROR) {\
JNIUtil::handleSVNError(svn_jni_err__temp); \
- return ret_val ; \
+ return ret_val; \
} \
} while (0)
@@ -251,7 +251,7 @@
do\
{ \
env->PopLocalFrame(NULL); \
- return ret_val ; \
+ return ret_val; \
} \
while (0)
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
I applied the patch to the javahl-ra branch instead of the trunk by mistake, is it ok if a cherry pick it onto trunk? Sorry about that, Vladimir On Thu, May 31, 2012 at 3:37 AM, Greg Stein wrote: > On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: > >... > > Fix the above three problems, and I'm +1 for you to commit just patch #1. > > To clarify: I trust you to make the three changes, and you're free to > commit with my +1. I don't need another round of review. > > (if something is missing, then we can fix in a second commit) > > Thanks, > -g >
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: > On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker > wrote: > > Patch 01 - Introduce macro > > > > [[[ > > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code > > checking C++ pointer extracted from the java object > > > > [ in subversion/bindings/javahl/native ] > > > > * JNIUtil.h > > (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java > > exception if necessary > > ]]] > > Replying to just this patch. The second patch seems pretty mechanical. > And we're only looking at minor nits. > > (sorry, but the patch doesn't inline into this response, so let's just > be flexible here...) > > Sorry, I cannot figure out how to get gmail to inline attachments, I'll setup IMAP client when I have a spare moment to to figure out which one will work for me. > > The macro argument substitutions need to be parenthesized for safety. > So it would be: (expr) == NULL, and it would be: return (ret_val); > will take care of (expr). Same cannot be done for ret_val because it is valid for the ret_val to be empty when used inside void methods. > > Next bit: the indentation in the diff seems to be off. Are there TAB > characters in there? the JNIUTIL:: and the return lines have different > indents in the patch that I'm looking at. That is either sloppy SPC > character indents, or a TAB is present. > I managed to sneak a single TAB in there, fixed now. > > Lastly, there is an extra space character before the ";" in the return > statement. That should be eliminated. > > > Fix the above three problems, and I'm +1 for you to commit just patch #1. > Will do. > > I have not reviewed #2, but the first patch seems reasonable to > simplify (as done in #2). I also await others to comment on the > applicability of patch #2. > > I do seem to recall that C++ tried to do away with the preprocessor. > It would be nice to follow that ideal, but looking at this macro... I > have no idea how to map it into "proper, non-CPP concepts". If you > know, that would be better. I am a java guy, with a some C/C++ understanding, afraid I won't be of much help in this department. Most of the macro is repeat of the existing SVN_JNI_NULL_PTR_EX with different exception being thrown. > Otherwise... meh. CPP is just fine with me > (and screw the C++ academic purity). > > Cheers, > -g > Thank you for the feedback, Vladimir
Re: svn commit: r1344825 - in /subversion/trunk/tools/dev/mergegraph: mergegraph.py save_as_sh.py
On May 31, 2012 4:45 PM, "Julian Foad" wrote: > > I (Julian Foad) wrote: > > > Greg Stein wrote: > >>> +def command(out, cmd, *args): > >>> + """Write the shell command CMD with the arguments ARGS to the file-like > >>> + object OUT.""" > >> Please review my earlier comments about standard docstring formatting in > >> Python. More specifically, PEP 8. (Google it) > > > > OK, r1344888 for 'most > > importantly, the """ that ends a multiline docstring should be > > on a line by itself [...]'. Anything further will have to wait till another > > day. > > Let me add a few more words here. I support efforts to both standardize and raise the usefulness of doc strings. I just haven't yet spent the time to read PEP 257 (being the relevant one) in full, digest it into my Python-commenting brain-space, decide what subset of its suggestions are appropriate for us to use in scripts like these, and go back and change them. Not a problem. As long as we move towards the PEP styles in time. I consider using the standardized docstrings as being similar to the doxygen rules (and what we've established for internal docstrings). Thx, -g
Re: svn commit: r1344825 - in /subversion/trunk/tools/dev/mergegraph: mergegraph.py save_as_sh.py
I (Julian Foad) wrote: > Greg Stein wrote: >>> +def command(out, cmd, *args): >>> + """Write the shell command CMD with the arguments ARGS to the file-like >>> + object OUT.""" >> Please review my earlier comments about standard docstring formatting in >> Python. More specifically, PEP 8. (Google it) > > OK, r1344888 for 'most > importantly, the """ that ends a multiline docstring should be > on a line by itself [...]'. Anything further will have to wait till another > day. Let me add a few more words here. I support efforts to both standardize and raise the usefulness of doc strings. I just haven't yet spent the time to read PEP 257 (being the relevant one) in full, digest it into my Python-commenting brain-space, decide what subset of its suggestions are appropriate for us to use in scripts like these, and go back and change them. - Julian
Re: svn commit: r1344825 - in /subversion/trunk/tools/dev/mergegraph: mergegraph.py save_as_sh.py
Greg Stein wrote:
>On May 31, 2012 1:30 PM, wrote:
>>...
>> +++ subversion/trunk/tools/dev/mergegraph/save_as_sh.py Thu May 31 17:30:17
>> 2012
>>...
>> +def command(out, cmd, *args):
>> + """Write the shell command CMD with the arguments ARGS to the file-like
>> + object OUT."""
>Please review my earlier comments about standard docstring formatting in
>Python. More specifically, PEP 8. (Google it)
OK, r1344888 for 'most
importantly, the """ that ends a multiline docstring should be
on a line by itself [...]'. Anything further will have to wait till another
day.
>> +def write_recipe(graph, out):
>> + """Write out a sequence of svn commands that will execute the branching
>> + and merging shown in GRAPH. Write to the file-like object OUT."""
>> + revs = {} # keyed by revnum
>> +
>> + def node_branch(node_name):
>> + """Extract branch name from a node name.
>> + ### TODO: multi-char names."""
>> + return node_name[:1]
>> +
>> + def node_url(node_name):
>> + """Extract the URL (in command-line repo-relative URL syntax) from a
>> + node name."""
>> + return '^/' + node_branch(node_name)
>> +
>> + def node_rev(node_name):
>> + """Extract revnum (as an integer) from a node name.
>> + ### TODO: multi-char names."""
>> + return int(node_name[1:]) + 1
>> +
>> + def add(node_name, action, *args):
>> + """Add the tuple (ACTION, (ARGS)) to the list REVS[REVNUM]."""
>> + revnum = node_rev(node_name)
>> + if not revnum in revs:
>> + revs[revnum] = []
>> + revs[revnum].append((action, args))
> There is no need to embed these functions. It makes write_recipe() less clear
> to have this clutter in here.
OK. 1344885.
>> + raise 'unknown action: %s' % action
> This form of exception is deprecated. Please raise an instance of Exception
> (or of a subclass).
Agreed. 1344885.
- Julian
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On Thu, May 31, 2012 at 4:36 AM, Julian Foad wrote: > > > > > - Original Message - >> From: Greg Stein >> To: Vladimir Berezniker >> Cc: dev@subversion.apache.org >> Sent: Thursday, 31 May 2012, 8:35 >> Subject: Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check >> C++ pointer extracted from the java object >> >> On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker >> wrote: >>> Patch 01 - Introduce macro >>> >>> [[[ >>> JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code >>> checking C++ pointer extracted from the java object >>> >>> [ in subversion/bindings/javahl/native ] >>> >>> * JNIUtil.h >>> (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java >>> exception if necessary >>> ]]] >> >> Replying to just this patch. The second patch seems pretty mechanical. >> And we're only looking at minor nits. >> >> (sorry, but the patch doesn't inline into this response, so let's just >> be flexible here...) >> >> >> The macro argument substitutions need to be parenthesized for safety. >> So it would be: (expr) == NULL, and it would be: return (ret_val); > > I notice the second patch relies on being able to pass an empty > (whitespace-only) second argument in order to generate "return;" in the > macro, so putting parentheses there wouldn't work. Actually I didn't know it > was possible to pass an empty (or, rather, whitespace-only) argument to a > macro, but apparently it is. Is it standardized? If so, this seems fine to > me, to use the argument without adding parentheses around it. We use this same trick for other macros, such as SVN_JNI_NULL_PTR_EX(). I don't know if it is standardized, but we've been using it for years. We do have to omit the parenthesis around the return value in the macro definition, as "return ();" is not valid syntax. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On Thu, May 31, 2012 at 2:35 AM, Greg Stein wrote: > On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker > wrote: >> Patch 01 - Introduce macro >> >> [[[ >> JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code >> checking C++ pointer extracted from the java object >> >> [ in subversion/bindings/javahl/native ] >> >> * JNIUtil.h >> (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java >> exception if necessary >> ]]] > > Replying to just this patch. The second patch seems pretty mechanical. > And we're only looking at minor nits. > > (sorry, but the patch doesn't inline into this response, so let's just > be flexible here...) > > > The macro argument substitutions need to be parenthesized for safety. > So it would be: (expr) == NULL, and it would be: return (ret_val); > > Next bit: the indentation in the diff seems to be off. Are there TAB > characters in there? the JNIUTIL:: and the return lines have different > indents in the patch that I'm looking at. That is either sloppy SPC > character indents, or a TAB is present. > > Lastly, there is an extra space character before the ";" in the return > statement. That should be eliminated. > > > Fix the above three problems, and I'm +1 for you to commit just patch #1. > > I have not reviewed #2, but the first patch seems reasonable to > simplify (as done in #2). I also await others to comment on the > applicability of patch #2. > > I do seem to recall that C++ tried to do away with the preprocessor. > It would be nice to follow that ideal, but looking at this macro... I > have no idea how to map it into "proper, non-CPP concepts". If you > know, that would be better. Otherwise... meh. CPP is just fine with me > (and screw the C++ academic purity). Like most things, C++ tried to reinvent the preprocessor and ended up with a half-baked solution which only partially replaces the thing it was intended to fix. We have several similar helper macros in the JavaHL C++ layer which do are similar in checking for error conditions (SVN_JNI_NULL_PTR_EX, for example) and possibly raising an exception. I don't know if the proposed macro is for a common situation, but there is precedent. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: [PATCH] Add 'Error validating server' section in FAQ
On Fri, Jun 01, 2012 at 12:20:23AM +0530, Jeyanthan wrote: > Hi Team, > > I hereby attached a patch to add a question in FAQ section. I am > hoping it helps some end users who encounters this client side > 'Error validating server' issue. > > I believe the attached patch and log message is self explanatory. > Please let me know if I have missed something important. > > [[[ > > Added an entry to the FAQ section : For every Subversion operation, > I get "Error validating server certificate" exception though I > configure the SSL certificates correctly in the server. > > * subversion/site/publish/faq.html : Added an FAQ entry. > > Patch by: Jeyanthan Inbasekaran > > ]]] > > -- > Regards, > Jeyanthan > Thanks for the patch! Review below. > Index: faq.html > === > --- faq.html (revision 1344429) > +++ faq.html (working copy) > @@ -266,6 +266,8 @@ a file: URL? > When performing Subversion operations > over SSL, I get the error SSL handshake failed: SSL error code > -1/1/336032856 > +For every Subversion > operation, I get Error Please don't use upper case letters in the 'href' link. > +validating server certificate exception though I configure the SSL > certificates correctly in the server. > After importing files to my repository, > I don't see them in the repository directory. Where are they? > I'd suggest the following wording for this FAQ entry's title: I get an "Error validating server certificate" error even though SSL certificates are correctly configured on the server-side. I wouldn't use the term "exception". We call them "errors" in other FAQ entries. It would be nice to keep this consistent. > @@ -4188,6 +4190,39 @@ See http://svn.haxx.se/dev/archive-2011-0 > > > > + > +For every Subversion operation, I get "Error validating server > +certificate" exception though I configure the SSL certificates > +correctly in the server. href="faq.html#Error-validating-server-certificate" title="Link to this > section">¶ > + > +This error appears in the > +clients if the certificate issuer is not recognized as 'Trusted' Maybe: "This error occurs if the certificate issuer ..." > +by the SVN client Full stop here? And make the following the start of a new sentence? > and will ask you whether you trust the certificate Maybe say: "Subversion will ask you " > and if you want to add this certificate. Maybe say "store" or "save", instead of "add"? Otherwise it's not clear what the cert is being added to. > + > + > + > + Empty paragraph? > +$ svn info https://mysite.com/svn/repo />Error validating server certificate for 'https://mysite.com:443':- > The certificate is not issued by a trusted authority. Use the />fingerprint to validate the certificate manually!Certificate > information:- Hostname: mysite.com- Valid: from Wed, 18 Jan 2012 > 00:00:00 GMT until Fri, 18 Jan 201323:59:59 GMT- Issuer: Google > Inc, US- Fingerprint: />34:4b:90:e7:e3:36:81:0d:53:1f:10:c0:4c:98:66:90:4a:9e:05:c9(R)eject, > accept (t)emporarily or accept (p)ermanently? > + > + Empty paragraph? > + Why add a tag here? > + This seems to have no opening . > +In some cases, even if you accept this by enterinp 'p' option, the > +next time you access SVN, the same exception appears again. There can Again, I'd prefer "error" instead of "exception". > +be multiple reasons. I'd suggest saying "... reasons for this". > The problem may be, that the subversion The above comma is not necessary. > +configuration directory, which is normally under your ~/.subversion has Again, superfluous comma. Maybe say "your ~/.subversion directory"? > +wrong permissions, so that each time you want to permanently add the > +credentials, svn actually cannot do so, and also doesn???t inform you > +that it can???t. Hmmm... I suppose we should consider making 'svn' issue a warning if this happens. That is an unrelated problem to your path submission though :) > + > +This can be solved by cleaning the directory > +???~/.subversion/auth/svn.ssl.server??? and this gets created automatically > +the next time you access. > +rm ~/.subversion/auth/svn.ssl.server/* I'm not sure if this is good advice. So in your case, the files inside the directory had wrong permissions? Do you know why this happened? If just one file is affected, I think it would be nicer to fix permissions on the offending file using chmod. If you had a directory with wrong permissions, it might likewise be a better idea to fix the permissions instead of purging the auth cache. > + > + You added just one tag but close two. That doesn't seem right. > + > + > > > After importing files to my repository, >
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
On Thu, May 31, 2012 at 03:09:01PM -0400, Greg Stein wrote: > Why ever would we want to rely in any way on a hash table iterator's order? > What!? You misunderstood what I wrote. I'm not suggesting we do so. I was just assuming that the APR-1.4.5 resulted in lexically sorted output (and apparently I was wrong about this, see Dustin's reply). > We want a stable ordering. That generally means "sort" rather "hash table > implementation details". Yes, we agreed on that weeks ago.
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
On May 31, 2012 2:52 PM, "Stefan Sperling" wrote: > > On Thu, May 31, 2012 at 02:31:01PM -0400, Dustin Lang wrote: > > > > The point of this diff is to ensure that "svnadmin dump" lists the > > deleted nodes in a consistent order between runs. At this point in > > the code, we're just dealing with a list of strings, so it really > > doesn't matter, we just need them in a consistent ordering. > > lexically looks simpler and faster (not that this is > > performance-critical). > > I think we'd also like to get output that is similar to what happened > with ARP-1.4.5 (i.e. before the hash table randomness change in APR-1.4.6). > I believe that was equivalent to lexical sort. The entries are hash keys, > and the hash function in APR didn't result in the kind of ordering that > svn_sort_compare_paths would choose. Why ever would we want to rely in any way on a hash table iterator's order? What!? We want a stable ordering. That generally means "sort" rather "hash table implementation details".
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
I think we'd also like to get output that is similar to what happened with ARP-1.4.5 (i.e. before the hash table randomness change in APR-1.4.6). I believe that was equivalent to lexical sort. The entries are hash keys, and the hash function in APR didn't result in the kind of ordering that svn_sort_compare_paths would choose. Interesting, I hadn't thought of backward-compatibility as a real option; I assumed that the hash table iterator would emit the entries sorted by their hash values. For what it's worth, here's what the old version of svnadmin I have kicking around does: /usr/bin/svnadmin --version svnadmin, version 1.6.6 (r40053) ... ls -l /usr/lib/libapr-1.so.0 lrwxrwxrwx 1 root root 17 May 25 2011 /usr/lib/libapr-1.so.0 -> libapr-1.so.0.3.8 dstn@apps3:~/svn-backup-astrometry ls -l /usr/lib/libaprutil-1.so.0 lrwxrwxrwx 1 root root 21 Nov 26 2010 /usr/lib/libaprutil-1.so.0 -> libaprutil-1.so.0.3.9 Node-path: trunk/documents/papers/archetypes/paper_plots/1qq.ps Node-path: trunk/documents/papers/archetypes/paper_plots/2qq.ps Node-path: trunk/documents/papers/archetypes/paper_plots/3qq.ps Node-path: trunk/documents/papers/archetypes/paper_plots/4qq.ps Node-path: trunk/documents/papers/archetypes/paper_plots/36gg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/12ggg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/66gg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/18lg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/55lg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/1gg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/2gg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/3gg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/103lg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/59lgg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/68lgg.ps Node-path: trunk/documents/papers/archetypes/paper_plots/36ggSLACS.ps Node-path: trunk/documents/papers/archetypes/paper_plots/66ggSLACS.ps Which sure isn't lexical :) --dstn
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
On Thu, May 31, 2012 at 02:31:01PM -0400, Dustin Lang wrote: > > The point of this diff is to ensure that "svnadmin dump" lists the > deleted nodes in a consistent order between runs. At this point in > the code, we're just dealing with a list of strings, so it really > doesn't matter, we just need them in a consistent ordering. > lexically looks simpler and faster (not that this is > performance-critical). I think we'd also like to get output that is similar to what happened with ARP-1.4.5 (i.e. before the hash table randomness change in APR-1.4.6). I believe that was equivalent to lexical sort. The entries are hash keys, and the hash function in APR didn't result in the kind of ordering that svn_sort_compare_paths would choose.
[PATCH] Add 'Error validating server' section in FAQ
Hi Team, I hereby attached a patch to add a question in FAQ section. I am hoping it helps some end users who encounters this client side 'Error validating server' issue. I believe the attached patch and log message is self explanatory. Please let me know if I have missed something important. [[[ Added an entry to the FAQ section : For every Subversion operation, I get "Error validating server certificate" exception though I configure the SSL certificates correctly in the server. * subversion/site/publish/faq.html : Added an FAQ entry. Patch by: Jeyanthan Inbasekaran ]]] -- Regards, Jeyanthan Index: faq.html === --- faq.html (revision 1344429) +++ faq.html (working copy) @@ -266,6 +266,8 @@ a file: URL? When performing Subversion operations over SSL, I get the error SSL handshake failed: SSL error code -1/1/336032856 +For every Subversion operation, I get Error +validating server certificate exception though I configure the SSL certificates correctly in the server. After importing files to my repository, I don't see them in the repository directory. Where are they? @@ -4188,6 +4190,39 @@ See http://svn.haxx.se/dev/archive-2011-0 + +For every Subversion operation, I get "Error validating server +certificate" exception though I configure the SSL certificates +correctly in the server. ¶ + +This error appears in the +clients if the certificate issuer is not recognized as 'Trusted' +by the SVN client and will ask you whether you trust the certificate and if you want to add this certificate. + + + + +$ svn info https://mysite.com/svn/repoError validating server certificate for 'https://mysite.com:443':- The certificate is not issued by a trusted authority. Use thefingerprint to validate the certificate manually!Certificate information:- Hostname: mysite.com- Valid: from Wed, 18 Jan 2012 00:00:00 GMT until Fri, 18 Jan 201323:59:59 GMT- Issuer: Google Inc, US- Fingerprint:34:4b:90:e7:e3:36:81:0d:53:1f:10:c0:4c:98:66:90:4a:9e:05:c9(R)eject, accept (t)emporarily or accept (p)ermanently? + + + + +In some cases, even if you accept this by enterinp 'p' option, the +next time you access SVN, the same exception appears again. There can +be multiple reasons. The problem may be, that the subversion +configuration directory, which is normally under your ~/.subversion has +wrong permissions, so that each time you want to permanently add the +credentials, svn actually cannot do so, and also doesnât inform you +that it canât. + +This can be solved by cleaning the directory +â~/.subversion/auth/svn.ssl.serverâ and this gets created automatically +the next time you access. +rm ~/.subversion/auth/svn.ssl.server/* + + + + After importing files to my repository,
Re: svn commit: r1344825 - in /subversion/trunk/tools/dev/mergegraph: mergegraph.py save_as_sh.py
On May 31, 2012 1:30 PM, wrote:
>...
> +++ subversion/trunk/tools/dev/mergegraph/save_as_sh.py Thu May 31
17:30:17 2012
>...
> +def command(out, cmd, *args):
> + """Write the shell command CMD with the arguments ARGS to the file-like
> + object OUT."""
Please review my earlier comments about standard docstring formatting in
Python. More specifically, PEP 8. (Google it)
>...
> +def write_recipe(graph, out):
> + """Write out a sequence of svn commands that will execute the branching
> + and merging shown in GRAPH. Write to the file-like object OUT."""
> + revs = {} # keyed by revnum
> +
> + def node_branch(node_name):
> +"""Extract branch name from a node name.
> + ### TODO: multi-char names."""
> +return node_name[:1]
> +
> + def node_url(node_name):
> +"""Extract the URL (in command-line repo-relative URL syntax) from a
> + node name."""
> +return '^/' + node_branch(node_name)
> +
> + def node_rev(node_name):
> +"""Extract revnum (as an integer) from a node name.
> + ### TODO: multi-char names."""
> +return int(node_name[1:]) + 1
> +
> + def add(node_name, action, *args):
> +"""Add the tuple (ACTION, (ARGS)) to the list REVS[REVNUM]."""
> +revnum = node_rev(node_name)
> +if not revnum in revs:
> + revs[revnum] = []
> +revs[revnum].append((action, args))
There is no need to embed these functions. It makes write_recipe() less
clear to have this clutter in here.
>...
> +raise 'unknown action: %s' % action
This form of exception is deprecated. Please raise an instance of Exception
(or of a subclass).
>...
Cheers,
-g
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
The point of this diff is to ensure that "svnadmin dump" lists the deleted nodes in a consistent order between runs. At this point in the code, we're just dealing with a list of strings, so it really doesn't matter, we just need them in a consistent ordering. lexically looks simpler and faster (not that this is performance-critical). How about this comment: // We just need the paths to be emitted in a consistent order, so lexical sort // is fine (ie, we don't need svn_sort_compare_paths, but it work work too). cheers, --dstn On Thu, 31 May 2012, Julian Foad wrote: Stefan Sperling wrote: On Wed, May 30, 2012 at 07:55:34AM +0100, Julian Foad wrote: If you're using _lexically on these paths, please explain this in a comment, otherwise this will be confusing to readers. Bert expanded the docstring of svn_sort_compare_items_lexically in r1344158. No, he expanded docs of svn_path_compare_paths() and svn_sort_compare_items_as_paths(). I guess that addresses your concern? Not really; there's no direct links from this usage of _lexically() to Bert's comment, and I'm taking the fact that a to-and-fro email discussion occurred as evidence that it's not totally obvious. If so, I'll apply Dustin's patch unmodified. I'm only offering my opinion; I won't be offended if you think it's clear enough and go ahead without adding a comment. - Julian
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
On Thu, May 31, 2012 at 07:12:08PM +0100, Julian Foad wrote: > Stefan Sperling wrote: > > > On Wed, May 30, 2012 at 07:55:34AM +0100, Julian Foad wrote: > >> If you're using _lexically on these paths, please explain this in a > >> comment, otherwise this will be confusing to readers. > > > > Bert expanded the docstring of svn_sort_compare_items_lexically > > in r1344158. > > No, he expanded docs of svn_path_compare_paths() and > svn_sort_compare_items_as_paths(). > > > I guess that addresses your concern? > > Not really; there's no direct links from this usage of _lexically() to > Bert's comment, and I'm taking the fact that a to-and-fro email > discussion occurred as evidence that it's not totally obvious. > Ah. Right, you're correct. > > If so, I'll apply Dustin's patch unmodified. > > I'm only offering my opinion; I won't be offended if you think it's clear > enough and go ahead without adding a comment. I'll commit the patch and add the comment in a follow-up commit.
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
Stefan Sperling wrote: > On Wed, May 30, 2012 at 07:55:34AM +0100, Julian Foad wrote: >> If you're using _lexically on these paths, please explain this in a >> comment, otherwise this will be confusing to readers. > > Bert expanded the docstring of svn_sort_compare_items_lexically > in r1344158. No, he expanded docs of svn_path_compare_paths() and svn_sort_compare_items_as_paths(). > I guess that addresses your concern? Not really; there's no direct links from this usage of _lexically() to Bert's comment, and I'm taking the fact that a to-and-fro email discussion occurred as evidence that it's not totally obvious. > If so, I'll apply Dustin's patch unmodified. I'm only offering my opinion; I won't be offended if you think it's clear enough and go ahead without adding a comment. - Julian
Re: Merge bug -- svn:keywords and conflict resolution
Stefan Sperling wrote: > [moving this thread from users@ to dev@] > > On Thu, May 31, 2012 at 05:22:41PM +0200, Stefan Sperling wrote: >> On Thu, May 31, 2012 at 02:36:56PM +0100, neil.tu...@rwe.com wrote: >> > I would like to confirm this issue in v 1.6.17 - using binary files >> > via TortoiseSVN. Test scenario was to create a binary file in trunk >> > with the "svn:keywords = Revision" property set*; branch the trunk to >> > $BAU; change the file in $BAU (and commit); merge the change revision >> > from $BAU back into trunk, this reported the file as a conflict >> > (performing same on a second file without the property set worked >> > without conflict). >> >> Hi Neil, >> >> We don't fix these kinds of bugs in the 1.6 series anymore. >> The 1.6 series receives only security or data corruption fixes. >> >> By code inspection I would guess that 1.7 has the same problem, however. >> Can you confirm that? If so, please file an issue. I believe there might >> be a bug where the merge compares a version of the file with keywords >> expanded to a version of a file with keywords contracted. I don't have >> time right now to dig deeper so it would be great if you could help by >> confirming that the same problem exists in 1.7. Cheers! > > I've spent some time on this now and I can reproduce the issue. > > You've added a comment to issue #4155 but I am not sure if that is > really the same problem. > > What is happening here is that your assumption to use svn:keywords > on a binary file conflicts with how the merge logic is currently > implemented. The merge logic purposefully skips keyword handling > on binary files entirely (see the detranslate_wc_file() function in > libsvn_client/merge.c), yet keywords are still expanded in the binary file. > > This is inconsistent behaviour. But I'm not sure which way we should fix it. > > I'm inclined to fix the merge logic to assume that binary files might have > keywords expanded. Fixing the inconsistency the other way, by preventing > keyword expansion in binary files in the first place, would be a more > invasive change and possibly be perceived as a regression by users (Neil > included :) > > Any thoughts from others about this? I have no particular preference for what to do here; your analysis sounds fine. - Julian
Re: [PATCH] issue #4134 (partial): sort deleted nodes before dumping them
On Wed, May 30, 2012 at 07:55:34AM +0100, Julian Foad wrote: > If you're using _lexically on these paths, please explain this in a comment, > otherwise this will be confusing to readers. Bert expanded the docstring of svn_sort_compare_items_lexically in r1344158. I guess that addresses your concern? If so, I'll apply Dustin's patch unmodified.
Re: Merge bug -- svn:keywords and conflict resolution
[moving this thread from users@ to dev@] On Thu, May 31, 2012 at 05:22:41PM +0200, Stefan Sperling wrote: > On Thu, May 31, 2012 at 02:36:56PM +0100, neil.tu...@rwe.com wrote: > > I would like to confirm this issue in v 1.6.17 - using binary files > > via TortoiseSVN. Test scenario was to create a binary file in trunk > > with the "svn:keywords = Revision" property set*; branch the trunk to > > $BAU; change the file in $BAU (and commit); merge the change revision > > from $BAU back into trunk, this reported the file as a conflict > > (performing same on a second file without the property set worked > > without conflict). > > Hi Neil, > > We don't fix these kinds of bugs in the 1.6 series anymore. > The 1.6 series receives only security or data corruption fixes. > > By code inspection I would guess that 1.7 has the same problem, however. > Can you confirm that? If so, please file an issue. I believe there might > be a bug where the merge compares a version of the file with keywords > expanded to a version of a file with keywords contracted. I don't have > time right now to dig deeper so it would be great if you could help by > confirming that the same problem exists in 1.7. Cheers! I've spent some time on this now and I can reproduce the issue. You've added a comment to issue #4155 but I am not sure if that is really the same problem. What is happening here is that your assumption to use svn:keywords on a binary file conflicts with how the merge logic is currently implemented. The merge logic purposefully skips keyword handling on binary files entirely (see the detranslate_wc_file() function in libsvn_client/merge.c), yet keywords are still expanded in the binary file. This is inconsistent behaviour. But I'm not sure which way we should fix it. I'm inclined to fix the merge logic to assume that binary files might have keywords expanded. Fixing the inconsistency the other way, by preventing keyword expansion in binary files in the first place, would be a more invasive change and possibly be perceived as a regression by users (Neil included :) Any thoughts from others about this?
Re: svn commit: r1344741 - /subversion/trunk/subversion/libsvn_client/diff.c
On Thu, May 31, 2012 at 02:27:28PM -, hwri...@apache.org wrote: > Author: hwright > Date: Thu May 31 14:27:28 2012 > New Revision: 1344741 > > URL: http://svn.apache.org/viewvc?rev=1344741&view=rev > Log: > When running diff, determine the depth capability *before* using it. > > * subversion/libsvn_client/diff.c > (diff_repos_wc): Initialize server_supports_depth before using it. Oops... thanks for tidying up after me :) > > Modified: > subversion/trunk/subversion/libsvn_client/diff.c > > Modified: subversion/trunk/subversion/libsvn_client/diff.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/diff.c?rev=1344741&r1=1344740&r2=1344741&view=diff > == > --- subversion/trunk/subversion/libsvn_client/diff.c (original) > +++ subversion/trunk/subversion/libsvn_client/diff.c Thu May 31 14:27:28 2012 > @@ -3034,6 +3034,8 @@ diff_repos_wc(const char *path_or_url1, > } > >/* Use the diff editor to generate the diff. */ > + SVN_ERR(svn_ra_has_capability(ra_session, &server_supports_depth, > +SVN_RA_CAPABILITY_DEPTH, pool)); >SVN_ERR(svn_wc__get_diff_editor(&diff_editor, &diff_edit_baton, >ctx->wc_ctx, >anchor_abspath, > @@ -3050,8 +3052,6 @@ diff_repos_wc(const char *path_or_url1, >ctx->cancel_func, ctx->cancel_baton, >pool, pool)); >SVN_ERR(svn_ra_reparent(ra_session, anchor_url, pool)); > - SVN_ERR(svn_ra_has_capability(ra_session, &server_supports_depth, > -SVN_RA_CAPABILITY_DEPTH, pool)); > >if (depth != svn_depth_infinity) > diff_depth = depth; >
Re: svn commit: r1336833 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_client/merge.c libsvn_wc/deprecated.c libsvn_wc/merge.c
On Thu, May 10, 2012 at 2:13 PM, wrote:
> Author: rhuijben
> Date: Thu May 10 19:13:11 2012
> New Revision: 1336833
>
> URL: http://svn.apache.org/viewvc?rev=1336833&view=rev
> Log:
> Make the text and property merge handling of 'svn merge' of a single file an
> atomic operation by moving the handling into a single libsvn_wc call that
> installs or doesn't install the working queue items.
>
> * subversion/include/svn_wc.h
> (svn_wc_merge5): New function.
> (svn_wc_merge4): Deprecate function.
>
> * subversion/libsvn_client/merge.c
> (merge_file_changed): Update caller.
>
> * subversion/libsvn_wc/deprecated.c
> (svn_wc_merge4): New function. Wraps svn_wc_merge5().
>
> * subversion/libsvn_wc/merge.c
> (svn_wc_merge4): Rename to ...
> (svn_wc_merge5): ... this and add support for merging properties in the same
> operation. At the same time avoid a few more unneeded db operations.
>
...
> Modified: subversion/trunk/subversion/libsvn_wc/merge.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/merge.c?rev=1336833&r1=1336832&r2=1336833&view=diff
> ==
> --- subversion/trunk/subversion/libsvn_wc/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/merge.c Thu May 10 19:13:11 2012
...
> @@ -1476,7 +1476,8 @@ svn_wc__internal_merge(svn_skel_t **work
>
>
> svn_error_t *
> -svn_wc_merge4(enum svn_wc_merge_outcome_t *merge_outcome,
> +svn_wc_merge5(enum svn_wc_merge_outcome_t *merge_content_outcome,
> + enum svn_wc_notify_state_t *merge_props_outcome,
> svn_wc_context_t *wc_ctx,
> const char *left_abspath,
> const char *right_abspath,
> @@ -1489,6 +1490,7 @@ svn_wc_merge4(enum svn_wc_merge_outcome_
> svn_boolean_t dry_run,
> const char *diff3_cmd,
> const apr_array_header_t *merge_options,
> + apr_hash_t *original_props,
> const apr_array_header_t *prop_diff,
> svn_wc_conflict_resolver_func2_t conflict_func,
> void *conflict_baton,
> @@ -1497,8 +1499,11 @@ svn_wc_merge4(enum svn_wc_merge_outcome_
> apr_pool_t *scratch_pool)
> {
> const char *dir_abspath = svn_dirent_dirname(target_abspath, scratch_pool);
> + svn_skel_t *prop_items = NULL;
> svn_skel_t *work_items;
> - apr_hash_t *actual_props;
> + apr_hash_t *pristine_props = NULL;
> + apr_hash_t *actual_props = NULL;
> + apr_hash_t *new_actual_props = NULL;
>
> SVN_ERR_ASSERT(svn_dirent_is_absolute(left_abspath));
> SVN_ERR_ASSERT(svn_dirent_is_absolute(right_abspath));
> @@ -1508,37 +1513,86 @@ svn_wc_merge4(enum svn_wc_merge_outcome_
> if (!dry_run)
> SVN_ERR(svn_wc__write_check(wc_ctx->db, dir_abspath, scratch_pool));
>
> - /* Sanity check: the merge target must be under revision control,
> - * unless the merge target is a copyfrom text, which lives in a
> - * temporary file and does not exist in ACTUAL yet. */
> + /* Sanity check: the merge target must be a file under revision control */
> {
> + svn_wc__db_status_t status;
> svn_kind_t kind;
> - svn_boolean_t hidden;
> - SVN_ERR(svn_wc__db_read_kind(&kind, wc_ctx->db, target_abspath, TRUE,
> - scratch_pool));
> + svn_boolean_t had_props;
> + svn_boolean_t props_mod;
>
> - if (kind == svn_kind_unknown)
> + SVN_ERR(svn_wc__db_read_info(&status, &kind, NULL, NULL, NULL, NULL,
> NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, &had_props, &props_mod, NULL, NULL,
> + NULL,
> + wc_ctx->db, target_abspath,
> + scratch_pool, scratch_pool));
> +
> + if (kind != svn_kind_file || (status != svn_wc__db_status_normal
> + && status != svn_wc__db_status_added))
> {
> - *merge_outcome = svn_wc_merge_no_merge;
> + *merge_content_outcome = svn_wc_merge_no_merge;
> + if (merge_props_outcome)
> + *merge_props_outcome = svn_wc_merge_no_merge;
There is a type mismatch here: *merge_props_outcome is an enum of type
svn_wc_notify_state_t, but svn_wc_merge_no_merge is one of
svn_wc_merge_outcome_t.
-Hyrum
> return SVN_NO_ERROR;
> }
>
> - SVN_ERR(svn_wc__db_node_hidden(&hidden, wc_ctx->db, target_abspath,
> - scratch_pool));
> + if (merge_props_outcome && had_props)
> + {
> + SVN_ERR(svn_wc__db_read_pristine_props(&pristine_props,
> + wc_ctx->db, target_abspath,
> + scratch_pool, scratch_pool));
> + }
> + else if (merge_props_outcome)
> + pristine_props = apr_hash_make(scratch_pool
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Thu, May 31, 2012 at 1:27 PM, Greg Stein wrote: > On Thu, May 31, 2012 at 4:39 AM, Philip Martin > wrote: >> Ivan Zhakov writes: >> >>> Config files are also used for authz settings and they can be even >>> more than 100 MB in real world scenarios. >> >> Yes, particularly for setups that use SVNParentPath but not the new >> AuthzSVNReposRelativeAccessFile. > > Woah. Wait a second here. > > My understanding is that these are read on *every* request (per > Daniel). Are you suggesting that we are reading/parsing 100 Mb on > every request? > > There is something wrong here. 100 Mb config files are flat out wrong. > We should be doing better. And if we are *actually* parsing those > per-request, then we've gone off the deep-end. > > ??? Currently Subversion reads authz file per-connection, not per-request. -- Ivan Zhakov
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Greg Stein writes: > On Thu, May 31, 2012 at 4:39 AM, Philip Martin > wrote: >> Ivan Zhakov writes: >> >>> Config files are also used for authz settings and they can be even >>> more than 100 MB in real world scenarios. >> >> Yes, particularly for setups that use SVNParentPath but not the new >> AuthzSVNReposRelativeAccessFile. > > Woah. Wait a second here. > > My understanding is that these are read on *every* request (per > Daniel). Are you suggesting that we are reading/parsing 100 Mb on > every request? > > There is something wrong here. 100 Mb config files are flat out wrong. > We should be doing better. And if we are *actually* parsing those > per-request, then we've gone off the deep-end. We cache the parsed data in the connection pool, see mod_authz_svn.c:get_access_conf, so it's per-connection rather than per-request. That's still not brilliant. I'm not sure whether the SVNParentPath case discards parsed data for repositories that are not the one in the current request. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
- Original Message - > From: Greg Stein > To: Vladimir Berezniker > Cc: dev@subversion.apache.org > Sent: Thursday, 31 May 2012, 8:35 > Subject: Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check > C++ pointer extracted from the java object > > On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker > wrote: >> Patch 01 - Introduce macro >> >> [[[ >> JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code >> checking C++ pointer extracted from the java object >> >> [ in subversion/bindings/javahl/native ] >> >> * JNIUtil.h >> (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java >> exception if necessary >> ]]] > > Replying to just this patch. The second patch seems pretty mechanical. > And we're only looking at minor nits. > > (sorry, but the patch doesn't inline into this response, so let's just > be flexible here...) > > > The macro argument substitutions need to be parenthesized for safety. > So it would be: (expr) == NULL, and it would be: return (ret_val); I notice the second patch relies on being able to pass an empty (whitespace-only) second argument in order to generate "return;" in the macro, so putting parentheses there wouldn't work. Actually I didn't know it was possible to pass an empty (or, rather, whitespace-only) argument to a macro, but apparently it is. Is it standardized? If so, this seems fine to me, to use the argument without adding parentheses around it. - Julian > Next bit: the indentation in the diff seems to be off. Are there TAB > characters in there? the JNIUTIL:: and the return lines have different > indents in the patch that I'm looking at. That is either sloppy SPC > character indents, or a TAB is present. > > Lastly, there is an extra space character before the ";" in the return > statement. That should be eliminated. > > > Fix the above three problems, and I'm +1 for you to commit just patch #1. > > I have not reviewed #2, but the first patch seems reasonable to > simplify (as done in #2). I also await others to comment on the > applicability of patch #2. > > I do seem to recall that C++ tried to do away with the preprocessor. > It would be nice to follow that ideal, but looking at this macro... I > have no idea how to map it into "proper, non-CPP concepts". If you > know, that would be better. Otherwise... meh. CPP is just fine with me > (and screw the C++ academic purity). > > Cheers, > -g >
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Thu, May 31, 2012 at 4:39 AM, Philip Martin wrote: > Ivan Zhakov writes: > >> Config files are also used for authz settings and they can be even >> more than 100 MB in real world scenarios. > > Yes, particularly for setups that use SVNParentPath but not the new > AuthzSVNReposRelativeAccessFile. Woah. Wait a second here. My understanding is that these are read on *every* request (per Daniel). Are you suggesting that we are reading/parsing 100 Mb on every request? There is something wrong here. 100 Mb config files are flat out wrong. We should be doing better. And if we are *actually* parsing those per-request, then we've gone off the deep-end. ???
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
Ivan Zhakov writes: > Config files are also used for authz settings and they can be even > more than 100 MB in real world scenarios. Yes, particularly for setups that use SVNParentPath but not the new AuthzSVNReposRelativeAccessFile. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
RE: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
> -Original Message- > From: Greg Stein [mailto:gst...@gmail.com] > Sent: donderdag 31 mei 2012 9:49 > To: Ivan Zhakov > Cc: dev@subversion.apache.org; Bert Huijben > Subject: Re: svn commit: r1344347 - > /subversion/trunk/subversion/libsvn_subr/config_file.c > > On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov > wrote: > > On Wed, May 30, 2012 at 8:52 PM, wrote: > >> Author: rhuijben > >> Date: Wed May 30 16:52:36 2012 > >> New Revision: 1344347 > >> > >> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev > >> Log: > >> Remove about 15% of the cpu overhead of svnserve and httpd while > running the > >> test suite by introducing a very simple parser buffer in the config file > parser > >> code. > >> > >> The nice clean stream translation code had an 7 times overhead on this > very > >> performance critical code path, while clients of svnserve and httpd were > >> waiting. (Before this patch the this code path used 22% of the cpu time, > >> now just 3%) > > > > Great improvement, but it may be worth to factor out this code to > > special stream like svn_stream_buffered(). > > When I saw this change, my first thought was, "wtf? just read the > whole damned file into memory". (I just haven't had time to look into > this to provide some actual advice beyond amazement) > > Config files are NOT 100 megabytes. Read the damned things into memory > in one giant piece. I/O is reduced, memory is available. Everybody > wins. I'm sure it could use further improvements. I don't know what really large authz files some users have, but reading in 10 Kbyte chunks as it does now gives a huge boost by itself. The current code assumes the line endings have at least a '\n' (and ignores '\r'), which would work for any typical user, but we used to support '\r' by itself, because that is what our translated streams do. I'm not sure if we should call this a regression as we probably never really intended to support that. (And the Mac switched to '\n' years ago) Most of the server side time was spend in reading the FSFS config files. I didn't know we had that many user editable files in a typical repository and all those commented lines explaining the flags were (and are) the performance killer. Bert
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Thu, May 31, 2012 at 11:49 AM, Greg Stein wrote: > On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov wrote: >> On Wed, May 30, 2012 at 8:52 PM, wrote: >>> Author: rhuijben >>> Date: Wed May 30 16:52:36 2012 >>> New Revision: 1344347 >>> >>> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev >>> Log: >>> Remove about 15% of the cpu overhead of svnserve and httpd while running the >>> test suite by introducing a very simple parser buffer in the config file >>> parser >>> code. >>> >>> The nice clean stream translation code had an 7 times overhead on this very >>> performance critical code path, while clients of svnserve and httpd were >>> waiting. (Before this patch the this code path used 22% of the cpu time, >>> now just 3%) >> >> Great improvement, but it may be worth to factor out this code to >> special stream like svn_stream_buffered(). > > When I saw this change, my first thought was, "wtf? just read the > whole damned file into memory". (I just haven't had time to look into > this to provide some actual advice beyond amazement) > > Config files are NOT 100 megabytes. Read the damned things into memory > in one giant piece. I/O is reduced, memory is available. Everybody > wins. > Config files are also used for authz settings and they can be even more than 100 MB in real world scenarios. -- Ivan Zhakov
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Thu, May 31, 2012 at 3:41 AM, Ivan Zhakov wrote: > On Wed, May 30, 2012 at 8:52 PM, wrote: >> Author: rhuijben >> Date: Wed May 30 16:52:36 2012 >> New Revision: 1344347 >> >> URL: http://svn.apache.org/viewvc?rev=1344347&view=rev >> Log: >> Remove about 15% of the cpu overhead of svnserve and httpd while running the >> test suite by introducing a very simple parser buffer in the config file >> parser >> code. >> >> The nice clean stream translation code had an 7 times overhead on this very >> performance critical code path, while clients of svnserve and httpd were >> waiting. (Before this patch the this code path used 22% of the cpu time, >> now just 3%) > > Great improvement, but it may be worth to factor out this code to > special stream like svn_stream_buffered(). When I saw this change, my first thought was, "wtf? just read the whole damned file into memory". (I just haven't had time to look into this to provide some actual advice beyond amazement) Config files are NOT 100 megabytes. Read the damned things into memory in one giant piece. I/O is reduced, memory is available. Everybody wins. -g
Re: svn commit: r1344347 - /subversion/trunk/subversion/libsvn_subr/config_file.c
On Wed, May 30, 2012 at 8:52 PM, wrote: > Author: rhuijben > Date: Wed May 30 16:52:36 2012 > New Revision: 1344347 > > URL: http://svn.apache.org/viewvc?rev=1344347&view=rev > Log: > Remove about 15% of the cpu overhead of svnserve and httpd while running the > test suite by introducing a very simple parser buffer in the config file > parser > code. > > The nice clean stream translation code had an 7 times overhead on this very > performance critical code path, while clients of svnserve and httpd were > waiting. (Before this patch the this code path used 22% of the cpu time, > now just 3%) > Great improvement, but it may be worth to factor out this code to special stream like svn_stream_buffered(). -- Ivan Zhakov
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On Thu, May 31, 2012 at 3:35 AM, Greg Stein wrote: >... > Fix the above three problems, and I'm +1 for you to commit just patch #1. To clarify: I trust you to make the three changes, and you're free to commit with my +1. I don't need another round of review. (if something is missing, then we can fix in a second commit) Thanks, -g
Re: [PATCH] JavaHL: Reduce amount of duplicate code used to check C++ pointer extracted from the java object
On Thu, May 31, 2012 at 12:43 AM, Vladimir Berezniker wrote: > Patch 01 - Introduce macro > > [[[ > JavaHL: Added CPPADDR_NULL_PTR macro to reduce amount of duplicate code > checking C++ pointer extracted from the java object > > [ in subversion/bindings/javahl/native ] > > * JNIUtil.h > (CPPADDR_NULL_PTR): New macro to test for NULL pointer and raise java > exception if necessary > ]]] Replying to just this patch. The second patch seems pretty mechanical. And we're only looking at minor nits. (sorry, but the patch doesn't inline into this response, so let's just be flexible here...) The macro argument substitutions need to be parenthesized for safety. So it would be: (expr) == NULL, and it would be: return (ret_val); Next bit: the indentation in the diff seems to be off. Are there TAB characters in there? the JNIUTIL:: and the return lines have different indents in the patch that I'm looking at. That is either sloppy SPC character indents, or a TAB is present. Lastly, there is an extra space character before the ";" in the return statement. That should be eliminated. Fix the above three problems, and I'm +1 for you to commit just patch #1. I have not reviewed #2, but the first patch seems reasonable to simplify (as done in #2). I also await others to comment on the applicability of patch #2. I do seem to recall that C++ tried to do away with the preprocessor. It would be nice to follow that ideal, but looking at this macro... I have no idea how to map it into "proper, non-CPP concepts". If you know, that would be better. Otherwise... meh. CPP is just fine with me (and screw the C++ academic purity). Cheers, -g
