Re: svn commit: r1296604 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
Daniel Shahaf danie...@elego.de writes: I believe you cannot assume svn_fs_t's will be short lived, and _certainly_ cannot assume that the access pattern to an svn_fs_t will be in any way related to random clients having random RA sessions; for all the FS API knows, svnserve is a daemon that calls svn_fs_open() once per reboot. And yes, people do this. Perhaps the read routines could be changed to always read the revprop-gen file and refresh the cache if required. Obviously this would involve more IO than the current caching strategy, but it would still be more efficient than the the previous non-cached approach. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: svn commit: r1297239 - /subversion/trunk/build/run_tests.py
Hyrum K Wright wrote: Greg Stein gst...@gmail.com wrote: On Mon, Mar 5, 2012 at 16:49, danie...@apache.org wrote: Author: danielsh Date: Mon Mar 5 21:49:15 2012 New Revision: 1297239 URL: http://svn.apache.org/viewvc?rev=1297239view=rev Log: Revert r1297223 and r1297231. Modified: subversion/trunk/build/run_tests.py Why? .. I see it is reverted, but what's the motivation? http://colabti.org/irclogger/irclogger_log/svn-dev?date=2012-03-05#l334 was the discussion that led to this revert. I've added that link to the log message. - Julian
Re: svn commit: r1296640 - use sizeof() instead of magic numbers
stef...@apache.org wrote: URL: http://svn.apache.org/viewvc?rev=1296640view=rev Log: * subversion/libsvn_repos/rev_hunt.c (svn_repos_get_committed_info): use sizeof() instead of magic numbers - committed_date_s = apr_hash_get(revprops, - SVN_PROP_REVISION_DATE, - 8); [...] + committed_date_s = apr_hash_get(revprops, + SVN_PROP_REVISION_DATE, + sizeof(SVN_PROP_REVISION_DATE)-1); [...] Consider using the special value APR_HASH_KEY_STRING. Last time I looked at the APR hash source code, it looked like the implementation was (as far as I could see by inspection) approximately just as fast with APR_HASH_KEY_STRING as with passing the specific length. Using it has the benefit of simplicity for the developers, reducing the opportunity for mistakes such as the one fixed by r1242353 (which is merely an example of an unnoticed typo in repetitive code, nothing to do with hash keys). - Julian
Re: svn commit: r1296628 - Make svn ls faster and more streamy on svn://
stef...@apache.org wrote:
Log:
Make svn ls faster and more streamy on svn://
* subversion/svnserve/serve.c
(get_dir): don't collect entries but send them immediately
+ /* Fetch the directory entries if requested and send them immediately. */
if (want_contents)
{
+ const char *zero_date = svn_time_to_cstring(0, pool);
This looks wrong. Here you construct a date string something like 1 Jan
1970, to send if no date is available...
+ cdate = NULL;
if ((dirent_fields SVN_DIRENT_LAST_AUTHOR)
|| (dirent_fields SVN_DIRENT_TIME)
|| (dirent_fields SVN_DIRENT_CREATED_REV))
{
/* created_rev, last_author, time */
[...]
SVN_CMD_ERR(svn_repos_get_committed_info(entry.created_rev,
cdate,
}
+ if (cdate == NULL)
+ cdate = zero_date;
[...]
- if (cdate)
- SVN_CMD_ERR(svn_time_from_cstring(entry-time, cdate,
- subpool));
- else
- entry-time = (time_t) -1;
}
[...]
- /* Write out response. */
- if (want_contents)
- {
- for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
- {
- const char *name = svn__apr_hash_index_key(hi);
- svn_dirent_t *entry = svn__apr_hash_index_val(hi);
- cdate = (entry-time == (time_t) -1) ? NULL
- : svn_time_to_cstring(entry-time, pool);
... but the original code used NULL as the value to send if there was no date.
- Julian
Re: svn commit: r1296596 - /subversion/trunk/subversion/libsvn_delta/xdelta.c
Hi Stefan. Please edit the log message for this rev. (I assume you'll revisit this soon, as my original comment still stands. Sorry if it was confusing. What I meant, basically, is that the function doesn't return what the doc string says it will return, AFAICT. Quite likely it's the doc string that's wrong.) - Julian - Original Message - From: Stefan Fuhrmann stefanfuhrm...@alice-dsl.de To: Cc: dev@subversion.apache.org Sent: Sunday, 4 March 2012, 23:48 Subject: Re: svn commit: r1296596 - /subversion/trunk/subversion/libsvn_delta/xdelta.c On 04.03.2012 11:42, Daniel Shahaf wrote: stef...@apache.org wrote on Sat, Mar 03, 2012 at 10:53:16 -: Author: stefan2 Date: Sat Mar 3 10:53:16 2012 New Revision: 1296596 URL: http://svn.apache.org/viewvc?rev=1296596view=rev Log: * subversion/libsvn_delta/xdelta.c (reverse_match_length): actually return MAX_LEN if MAX_LEN chars match. Found by: julianfoad Modified: subversion/trunk/subversion/libsvn_delta/xdelta.c Modified: subversion/trunk/subversion/libsvn_delta/xdelta.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/xdelta.c?rev=1296596r1=1296595r2=1296596view=diff == --- subversion/trunk/subversion/libsvn_delta/xdelta.c (original) +++ subversion/trunk/subversion/libsvn_delta/xdelta.c Sat Mar 3 10:53:16 2012 @@ -260,11 +260,15 @@ reverse_match_length(const char *a, cons #endif + /* If we find a mismatch at -pos, pos-1 characters matched. + */ while (++pos= max_len) if (a[0-pos] != b[0-pos]) - break; + return pos - 1; - return pos-1; + /* No mismatch found - at least MAX_LEN machting chars. + */ + return max_len; I may be blind, but isn't the code before this diff and after it equivalent? Both the old and new code return POS-1 when the if() statement is entered, and if the code falls off the end of the while() loop then necessarily POS=1+MAX_LEN, again meaning that the old and new code are equivalent. You are right. It's been too early in the morning for me and Julian's comment got me confused. But at least, the code slightly clearer now. -- Stefan^2.
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Daniel Shahaf wrote: Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200: Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100: On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote: This still strips whitespace around ='s in the value: SVNHooksEnv name = x = y will result in setenv(name, x=y, 1) whereas I believe it should result in setenv(name, x = y, 1) (and, to be honest, I'd be happy with setenv(name , x = y, 1) as well). WDYT? How should it behave? I agree. would telling svn_cstring_split() to no strip whitespace suffice? I assume that should result in the third setenv() case above, so +1. Ping? trunk@HEAD still strips whitespace around equal signs in the value. My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before name and/or after x = y. - Julian
Re: svn commit: r1296604 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
On 03/05/2012 01:57 PM, Stefan Fuhrmann wrote: On 05.03.2012 15:20, Daniel Shahaf wrote: Philip Martin wrote on Mon, Mar 05, 2012 at 13:58:18 +: Daniel Shahafdanie...@elego.de writes: Philip Martin wrote on Mon, Mar 05, 2012 at 12:23:40 +: Daniel Shahafdanie...@elego.de writes: You're right, I misread the code: I mistakenly thought line 2867 will always re-read the revprop-gen file from disk. How about: Index: subversion/libsvn_fs_fs/fs_fs.c === --- subversion/libsvn_fs_fs/fs_fs.c (revision 1297002) +++ subversion/libsvn_fs_fs/fs_fs.c (working copy) @@ -583,8 +583,9 @@ with_some_lock_file(svn_fs_t *fs, fs_fs_data_t *ffd = fs-fsap_data; if (ffd-format= SVN_FS_FS__MIN_PACKED_FORMAT) SVN_ERR(update_min_unpacked_rev(fs, pool)); SVN_ERR(get_youngest(ffd-youngest_rev_cache, fs-path, pool)); + SVN_ERR(read_revprop_generation(fs, pool)); err = body(baton, subpool); } That looks like it works. But it only works if all writers update the generation file so this whole caching scheme requires an FSFS format bump to exclude 1.7 and earlier. +1 (It was pointed on IRC, too.) If we were to clear the cache after taking the write lock, either unconditionally or if the revprop generation has changed, then we would become compatible with pre-1.8 style writers. Of course this does highlight a change in svn_fs_t behaviour. A long lived svn_fs_t may never see revprop updates as the cache never expires. The cached values might get pushed out if the process (thead?) reads other revisions, to get reasonably up-to-date values the caller must not hold svn_fs_t objects too long. Which is a regression --- even a process that does proplist() in a loop should eventually see changes. Hm. I would expect the live of a fs_t to be limited by the life of the RA session. At least for our CL client, this would not be a problem. For TSVN dialogs like the repository browser that keep client contexts open for a long time, it all hinges on how long the RA session is kept open by the SVN libs and whether the client has any control over it. Can anyone comment on that? We (Sony Imageworks) have a custom built Subversion server that does 2,000 RPCs/sec, so we LRU cache svn_fs_t, svn_repos_t, and svn_fs_root_t to avoid filesystem IO. Blair
Re: svn commit: r1296868 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql
danie...@apache.org writes: Author: danielsh Date: Sun Mar 4 20:14:01 2012 New Revision: 1296868 URL: http://svn.apache.org/viewvc?rev=1296868view=rev Log: * subversion/libsvn_fs_fs/rep-cache-db.sql (I_HASH): New index. Suggested by: Trent Nelson tr...@snakebite.org Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql Modified: subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql?rev=1296868r1=1296867r2=1296868view=diff == --- subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql (original) +++ subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql Sun Mar 4 20:14:01 2012 @@ -33,6 +33,11 @@ CREATE TABLE rep_cache ( expanded_size INTEGER NOT NULL ); +/* There isn't an implicit index on a TEXT column, so create an explicit one. */ +CREATE INDEX I_HASH on REP_CACHE (hash); It may be TEXT but it is also PRIMARY KEY and according to the SQLite docs: http://sqlite.org/lang_createtable.html INTEGER PRIMARY KEY columns aside, both UNIQUE and PRIMARY KEY constraints are implemented by creating an index in the database (in the same way as a CREATE UNIQUE INDEX statement would). Such an index is used like any other index in the database to optimize queries. As a result, there often no advantage (but significant overhead) in creating an index on a set of columns that are already collectively subject to a UNIQUE or PRIMARY KEY constraint. It appears creating the index is unnecessary and may be actively bad. This reminds me that shortly before 1.7 I was asking about the exact opposite of this change: whether removing existing indices from PRIMARY KEYs, and making indices UNIQUE where possible, would be a good idea. -- Philip
Re: svn commit: r1296868 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql
Philip Martin philip.mar...@wandisco.com writes: It may be TEXT but it is also PRIMARY KEY and according to the SQLite docs: http://sqlite.org/lang_createtable.html INTEGER PRIMARY KEY columns aside, both UNIQUE and PRIMARY KEY constraints are implemented by creating an index in the database (in the same way as a CREATE UNIQUE INDEX statement would). Such an index is used like any other index in the database to optimize queries. As a result, there often no advantage (but significant overhead) in creating an index on a set of columns that are already collectively subject to a UNIQUE or PRIMARY KEY constraint. If I create a repository using 1.7 and look at the rep-cache.db I see: $ sqlite3 rep-cache.db select * from sqlite_master | grep index index|sqlite_autoindex_rep_cache_1|rep_cache|4| An index has been created automatically and so adding another index can only slow things down. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: svn commit: r1296868 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql
Philip Martin philip.mar...@wandisco.com writes: If I create a repository using 1.7 and look at the rep-cache.db I see: $ sqlite3 rep-cache.db select * from sqlite_master | grep index index|sqlite_autoindex_rep_cache_1|rep_cache|4| I'm using SQLite 3.7.8 on Linux. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: svn commit: r1241050 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
On Tue, Mar 06, 2012 at 12:39:27PM +, Julian Foad wrote: Daniel Shahaf wrote: Daniel Shahaf wrote on Mon, Feb 06, 2012 at 18:20:28 +0200: Stefan Sperling wrote on Mon, Feb 06, 2012 at 17:12:32 +0100: On Mon, Feb 06, 2012 at 05:59:04PM +0200, Daniel Shahaf wrote: This still strips whitespace around ='s in the value: SVNHooksEnv name = x = y will result in setenv(name, x=y, 1) whereas I believe it should result in setenv(name, x = y, 1) (and, to be honest, I'd be happy with setenv(name , x = y, 1) as well). WDYT? How should it behave? I agree. would telling svn_cstring_split() to no strip whitespace suffice? I assume that should result in the third setenv() case above, so +1. Ping? trunk@HEAD still strips whitespace around equal signs in the value. My tuppence-worth? I agree that the current behaviour as stated above is wrong. Unless there is precedent to the contrary, I think it should do no stripping at all. If you can find precedent for some stripping in such a setting, then follow the precedent. Note that it's not only possible to strip spaces before and/or after the first '=' character, but also before name and/or after x = y. This option string is parsed by HTTPD. The API provided by HTTPD is quite bad for this use case. This is work in progress, and the above behaviour will be changed. See http://subversion.tigris.org/issues/show_bug.cgi?id=4113 In the long term, the argument to the SVNHooksEnv option will be changed anyway. It will become a path to a file which contains a list of environment variables and their values. That file will then be parsed by our own config parsing code.
Re: svn commit: r1297604 [1/12] - in /subversion/branches/reintegrate-keep-alive: ./ build/ build/ac-macros/ build/generator/ build/generator/templates/ build/win32/ notes/ notes/api-errata/1.7/ subve
On Tue, Mar 6, 2012 at 12:50, julianf...@apache.org wrote: Author: julianfoad Date: Tue Mar 6 17:50:23 2012 New Revision: 1297604 URL: http://svn.apache.org/viewvc?rev=1297604view=rev Log: On the 'reintegrate-keep-alive' branch: Catch up to trunk@1297591. Added: subversion/branches/reintegrate-keep-alive/tools/dev/mergegraph/ - copied from r1295003, subversion/trunk/tools/dev/mergegraph/ subversion/branches/reintegrate-keep-alive/tools/examples/info.rb - copied unchanged from r1295003, subversion/trunk/tools/examples/info.rb subversion/branches/reintegrate-keep-alive/tools/server-side/svnpubsub/ - copied from r1295003, subversion/trunk/tools/server-side/svnpubsub/ subversion/branches/reintegrate-keep-alive/tools/server-side/svnpubsub/svnwcsub.tac - copied unchanged from r1297591, subversion/trunk/tools/server-side/svnpubsub/svnwcsub.tac Modified: ... Propchange: subversion/branches/reintegrate-keep-alive/ -- --- svn:mergeinfo (original) +++ svn:mergeinfo Tue Mar 6 17:50:23 2012 @@ -57,4 +57,4 @@ /subversion/branches/tree-conflicts:868291-873154 /subversion/branches/tree-conflicts-notify:873926-874008 /subversion/branches/uris-as-urls:1060426-1064427 -/subversion/trunk:1234907-1239494 +/subversion/trunk:1234907-1295003,1295006-1297591 More ugliness from our discontinuity on trunk/ :-(
Re: svn commit: r1296868 - /subversion/trunk/subversion/libsvn_fs_fs/rep-cache-db.sql
On 3/6/12 5:35 PM, Philip Martin philip.mar...@wandisco.com wrote: Philip Martin philip.mar...@wandisco.com writes: It may be TEXT but it is also PRIMARY KEY and according to the SQLite docs: http://sqlite.org/lang_createtable.html INTEGER PRIMARY KEY columns aside, both UNIQUE and PRIMARY KEY constraints are implemented by creating an index in the database (in the same way as a CREATE UNIQUE INDEX statement would). Such an index is used like any other index in the database to optimize queries. As a result, there often no advantage (but significant overhead) in creating an index on a set of columns that are already collectively subject to a UNIQUE or PRIMARY KEY constraint. If I create a repository using 1.7 and look at the rep-cache.db I see: $ sqlite3 rep-cache.db select * from sqlite_master | grep index index|sqlite_autoindex_rep_cache_1|rep_cache|4| An index has been created automatically and so adding another index can only slow things down. Yeah this is interesting; you've provided a wealth of information contrary to everything I said to Daniel yesterday. I did some SQLite index tuning a month ago and I could have sworn having a TEXT field as primary key inhibited index creation -- but, as you've demonstrated with your explain plan and sqlite_master query, that's clearly not the case. To rewind things a little: I was manually repairing my asf mirror yesterday that happened to sync earlier last week at the wrong time and picked up a dodgy revision. I manually deleted the affected rows from rep-cache.db and noticed that all my select queries seemed to be taking an inordinate amount of time to complete (5s+ at least). I explain plan the problematic query, saw no indexes, created one manually, and wallah, problem solved, all queries returned instantly and explain plan showed index usage. I've got a bunch of zfs snapshots of the repo I can have a play around with tomorrow to see if I can replicate. Plausible theories off the top of my head: a) There's something different with my env and indexes were not being created automatically. (I do remember having a lot of trouble getting the asf repo to load from dumps and then complete from subsequent syncs.) b) There's something else going on. (Plausible theories? Yes. Good theories? Debatable ;-) Trent.
Re: svn commit: r1297676 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py
On Tue, Mar 6, 2012 at 15:18, hwri...@apache.org wrote:
...
+++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue Mar 6
20:18:16 2012
...
@@ -78,6 +79,10 @@ SVN_VER_MINOR = 8
default_num_threads = 5
+# Set up logging
+logger = logging.getLogger(__name__)
You may as well use the default logger. That way, every module can
just: import logging ; logging.info('whatever: %s %s', foo, bar)
...
@@ -1473,6 +1469,9 @@ def create_default_options():
def _create_parser():
Return a parser for our test suite.
+ def set_log_debug(option, opt, value, parser):
+ logger.setLevel(logging.DEBUG)
axe this, see below:
+
# set up the parser
_default_http_library = 'serf'
usage = 'usage: %prog [options] [test ...]'
@@ -1481,7 +1480,8 @@ def _create_parser():
help='Print test doc strings instead of running them')
parser.add_option('--milestone-filter', action='store',
dest='milestone_filter',
help='Limit --list to those with target milestone
specified')
- parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
+ parser.add_option('-v', '--verbose', action='callback',
+ callback=set_log_debug,
help='Print binary command-lines (not with --quiet)')
callback=logger.setLevel, callback_args=(logging.DEBUG,)
...
You may want to change most of the calls to logging.FOO rather than
logger.FOO for consistency with other modules that will use the
logging framework.
I would also recommend creating a Formatter and attaching it to that
StreamHandler:
formatter = logging.Formatter('%(asctime)s [%(levelname)s] %(message)s',
'%Y-%m-%d %H:%M:%S')
handler.setFormatter(formatter)
that will give us a timestamp for each line.
Cheers,
-g
[PATCH] Revision 1297676 broke --list option of command line tests
Hi all, Revision 1297676 by hwright broke the command line tests' --list option (and probably, some other interfaces): the options dict no longer contains verbose option as --verbose handler was changed from 'store_true' to 'callback'. The attached patch fixes it by reintroducing options.verbose. [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Set options.verbose to False by default, override with True from set_log_debug() callback. ]]] Regards, Alexey. Index: subversion/tests/cmdline/svntest/main.py === --- subversion/tests/cmdline/svntest/main.py (revision 1297679) +++ subversion/tests/cmdline/svntest/main.py (working copy) @@ -1471,6 +1471,7 @@ Return a parser for our test suite. def set_log_debug(option, opt, value, parser): logger.setLevel(logging.DEBUG) +options.verbose = True # set up the parser _default_http_library = 'serf' @@ -1533,6 +1534,7 @@ # most of the defaults are None, but some are other values, set them here parser.set_defaults( +verbose=False, server_minor_version=SVN_VER_MINOR, url=file_scheme_prefix + \ urllib.pathname2url(os.path.abspath(os.getcwd())),
Re: [PATCH] Revision 1297676 broke --list option of command line tests
Please use the attached patch instead (the first one fixed --list, but not --list --verbose). Regards, Alexey. On Tuesday, March 06, 2012 12:45:21 pm Alexey Neyman wrote: Hi all, Revision 1297676 by hwright broke the command line tests' --list option (and probably, some other interfaces): the options dict no longer contains verbose option as --verbose handler was changed from 'store_true' to 'callback'. The attached patch fixes it by reintroducing options.verbose. [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Set options.verbose to False by default, override with True from set_log_debug() callback. ]]] Regards, Alexey. Index: subversion/tests/cmdline/svntest/main.py === --- subversion/tests/cmdline/svntest/main.py (revision 1297679) +++ subversion/tests/cmdline/svntest/main.py (working copy) @@ -1469,8 +1469,10 @@ def _create_parser(): Return a parser for our test suite. + verbosity = False def set_log_debug(option, opt, value, parser): logger.setLevel(logging.DEBUG) +verbosity = True # set up the parser _default_http_library = 'serf' @@ -1533,6 +1535,7 @@ # most of the defaults are None, but some are other values, set them here parser.set_defaults( +verbose=verbosity, server_minor_version=SVN_VER_MINOR, url=file_scheme_prefix + \ urllib.pathname2url(os.path.abspath(os.getcwd())),
Re: svn commit: r1297676 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py
On Tue, Mar 6, 2012 at 2:42 PM, Greg Stein gst...@gmail.com wrote:
On Tue, Mar 6, 2012 at 15:18, hwri...@apache.org wrote:
...
+++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue Mar 6
20:18:16 2012
...
@@ -78,6 +79,10 @@ SVN_VER_MINOR = 8
default_num_threads = 5
+# Set up logging
+logger = logging.getLogger(__name__)
You may as well use the default logger. That way, every module can
just: import logging ; logging.info('whatever: %s %s', foo, bar)
I thought about that, but figured using the custom logger would give
us more flexibility. Separate test suites could use separate loggers
when run in parallel, for instance, and they wouldn't interleave each.
By establishing the habit of using a named logger, rather than the
default one, it means we can just change the logger reference, instead
of go back and rewrite everything to use a custom logger later.
This first pass is to just get as much as possible using the logging
framework, and then we can go back and worry about such things. I
just don't want us to prematurely optimize. :)
...
@@ -1473,6 +1469,9 @@ def create_default_options():
def _create_parser():
Return a parser for our test suite.
+ def set_log_debug(option, opt, value, parser):
+ logger.setLevel(logging.DEBUG)
axe this, see below:
+
# set up the parser
_default_http_library = 'serf'
usage = 'usage: %prog [options] [test ...]'
@@ -1481,7 +1480,8 @@ def _create_parser():
help='Print test doc strings instead of running them')
parser.add_option('--milestone-filter', action='store',
dest='milestone_filter',
help='Limit --list to those with target milestone
specified')
- parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
+ parser.add_option('-v', '--verbose', action='callback',
+ callback=set_log_debug,
help='Print binary command-lines (not with --quiet)')
callback=logger.setLevel, callback_args=(logging.DEBUG,)
Mentioned this on IRC, but:
TypeError: setLevel() takes exactly 2 arguments (6 given)
Essentially, the optparser provides a number of extra arguments that
setLevel() doesn't expect (or use).
...
You may want to change most of the calls to logging.FOO rather than
logger.FOO for consistency with other modules that will use the
logging framework.
I would also recommend creating a Formatter and attaching it to that
StreamHandler:
formatter = logging.Formatter('%(asctime)s [%(levelname)s] %(message)s',
'%Y-%m-%d %H:%M:%S')
handler.setFormatter(formatter)
that will give us a timestamp for each line.
Future Work. A valuable suggestion, but right now I'm focused on
getting the logging infrastructure in place, so we can make this kind
of changes on a system-wide basis.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1297676 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py
On Tue, Mar 6, 2012 at 15:58, Hyrum K Wright hyrum.wri...@wandisco.com wrote:
On Tue, Mar 6, 2012 at 2:42 PM, Greg Stein gst...@gmail.com wrote:
...
You may as well use the default logger. That way, every module can
just: import logging ; logging.info('whatever: %s %s', foo, bar)
I thought about that, but figured using the custom logger would give
us more flexibility. Separate test suites could use separate loggers
when run in parallel, for instance, and they wouldn't interleave each.
By establishing the habit of using a named logger, rather than the
default one, it means we can just change the logger reference, instead
of go back and rewrite everything to use a custom logger later.
This first pass is to just get as much as possible using the logging
framework, and then we can go back and worry about such things. I
just don't want us to prematurely optimize. :)
Euh... that's not premature optimization. That is keeping things
*simple* unless and until circumstances dictate that we need more
complexity. You haven't demonstrated that at all yet.
And as long as you keep doing a per-module logger, then each and every
one of those loggers will need to be configured. Add a stream handler.
Add a formatter. Adjust their loglevel based on the verbose flag.
etc-f'in-etc.
By sticking to the root logger, the main.py can get it configured
properly and all modules can use it without concern. As it stands,
per-module loggers absolutely increases complexity with zero benefit.
...
You may want to change most of the calls to logging.FOO rather than
logger.FOO for consistency with other modules that will use the
logging framework.
I would also recommend creating a Formatter and attaching it to that
StreamHandler:
formatter = logging.Formatter('%(asctime)s [%(levelname)s] %(message)s',
'%Y-%m-%d %H:%M:%S')
handler.setFormatter(formatter)
that will give us a timestamp for each line.
Future Work. A valuable suggestion, but right now I'm focused on
getting the logging infrastructure in place, so we can make this kind
of changes on a system-wide basis.
It can only be done (easily) on a system-wide basis if you use the
root logger :-)
Cheers,
-g
Re: svn commit: r1297676 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py
On Tue, Mar 6, 2012 at 3:02 PM, Greg Stein gst...@gmail.com wrote:
On Tue, Mar 6, 2012 at 15:58, Hyrum K Wright hyrum.wri...@wandisco.com
wrote:
On Tue, Mar 6, 2012 at 2:42 PM, Greg Stein gst...@gmail.com wrote:
...
You may as well use the default logger. That way, every module can
just: import logging ; logging.info('whatever: %s %s', foo, bar)
I thought about that, but figured using the custom logger would give
us more flexibility. Separate test suites could use separate loggers
when run in parallel, for instance, and they wouldn't interleave each.
By establishing the habit of using a named logger, rather than the
default one, it means we can just change the logger reference, instead
of go back and rewrite everything to use a custom logger later.
This first pass is to just get as much as possible using the logging
framework, and then we can go back and worry about such things. I
just don't want us to prematurely optimize. :)
Euh... that's not premature optimization. That is keeping things
*simple* unless and until circumstances dictate that we need more
complexity. You haven't demonstrated that at all yet.
And as long as you keep doing a per-module logger, then each and every
one of those loggers will need to be configured. Add a stream handler.
Add a formatter. Adjust their loglevel based on the verbose flag.
etc-f'in-etc.
By sticking to the root logger, the main.py can get it configured
properly and all modules can use it without concern. As it stands,
per-module loggers absolutely increases complexity with zero benefit.
I understand your eloquently-put point. I've no objection to using the root
logger, but I'd like to refer to it through a named variable, rather
than using the global variable implicit in the logging.debug()
construction.
...
You may want to change most of the calls to logging.FOO rather than
logger.FOO for consistency with other modules that will use the
logging framework.
I would also recommend creating a Formatter and attaching it to that
StreamHandler:
formatter = logging.Formatter('%(asctime)s [%(levelname)s] %(message)s',
'%Y-%m-%d %H:%M:%S')
handler.setFormatter(formatter)
that will give us a timestamp for each line.
Future Work. A valuable suggestion, but right now I'm focused on
getting the logging infrastructure in place, so we can make this kind
of changes on a system-wide basis.
It can only be done (easily) on a system-wide basis if you use the
root logger :-)
Or have a logger constructor/fetcher method. :)
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: [PATCH] Revision 1297676 broke --list option of command line tests
Argh, damn. Last one:
[[[
* subversion/tests/cmdline/svntest/main.py
(_create_parser): Partially revert r1297676, other code needs
options.verbose.
(_parse_options): Call logger.setLevel(logging.DEBUG) if verbose
output is requested.
]]]
On Tuesday, March 06, 2012 12:50:30 pm Alexey Neyman wrote:
Please use the attached patch instead (the first one fixed --list, but
not --list --verbose).
Regards,
Alexey.
On Tuesday, March 06, 2012 12:45:21 pm Alexey Neyman wrote:
Hi all,
Revision 1297676 by hwright broke the command line tests' --list option
(and probably, some other interfaces): the options dict no longer
contains verbose option as --verbose handler was changed from
'store_true' to 'callback'. The attached patch fixes it by reintroducing
options.verbose.
[[[
* subversion/tests/cmdline/svntest/main.py
(_create_parser): Set options.verbose to False by default, override
with
True from set_log_debug() callback.
]]]
Regards,
Alexey.
Index: subversion/tests/cmdline/svntest/main.py
===
--- subversion/tests/cmdline/svntest/main.py (revision 1297679)
+++ subversion/tests/cmdline/svntest/main.py (working copy)
@@ -1469,9 +1469,6 @@
def _create_parser():
Return a parser for our test suite.
- def set_log_debug(option, opt, value, parser):
-logger.setLevel(logging.DEBUG)
-
# set up the parser
_default_http_library = 'serf'
usage = 'usage: %prog [options] [test ...]'
@@ -1480,8 +1477,7 @@
help='Print test doc strings instead of running them')
parser.add_option('--milestone-filter', action='store', dest='milestone_filter',
help='Limit --list to those with target milestone specified')
- parser.add_option('-v', '--verbose', action='callback',
-callback=set_log_debug,
+ parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
help='Print binary command-lines (not with --quiet)')
parser.add_option('-q', '--quiet', action='store_true',
help='Print only unexpected results (not with --verbose)')
@@ -1550,6 +1546,9 @@
parser = _create_parser()
(options, args) = parser.parse_args(arglist)
+ if options.verbose:
+logger.setLevel(logging.DEBUG)
+
# some sanity checking
if options.fsfs_packing and not options.fsfs_sharding:
parser.error(--fsfs-packing requires --fsfs-sharding)
Re: [PATCH] Revision 1297676 broke --list option of command line tests
I think the solution here is to rip out places that require options.verbose, rather than attempt to paper over the problem by resurrecting that field. I've attempted to do so in subsequent commits (see r1297724 and r1297725). -Hyrum On Tue, Mar 6, 2012 at 3:11 PM, Alexey Neyman sti...@att.net wrote: Argh, damn. Last one: [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Partially revert r1297676, other code needs options.verbose. (_parse_options): Call logger.setLevel(logging.DEBUG) if verbose output is requested. ]]] On Tuesday, March 06, 2012 12:50:30 pm Alexey Neyman wrote: Please use the attached patch instead (the first one fixed --list, but not --list --verbose). Regards, Alexey. On Tuesday, March 06, 2012 12:45:21 pm Alexey Neyman wrote: Hi all, Revision 1297676 by hwright broke the command line tests' --list option (and probably, some other interfaces): the options dict no longer contains verbose option as --verbose handler was changed from 'store_true' to 'callback'. The attached patch fixes it by reintroducing options.verbose. [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Set options.verbose to False by default, override with True from set_log_debug() callback. ]]] Regards, Alexey. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: [PATCH] Revision 1297676 broke --list option of command line tests
The code which accesses options.verbose is still there in some places, including TestRunner.list(): 1071809 pburba # If there is no filter or this test made if through 1071809 pburba # the filter then print it! 1071809 pburba if options.milestone_filter is None or len(issues): 1071809 pburba if options.verbose and self.pred.inprogress: ... Shouldn't all the options.verbose instances be removed _before_ changing the option handler to 'callback'? Regards, Alexey. On Tuesday, March 06, 2012 01:20:10 pm Hyrum K Wright wrote: I think the solution here is to rip out places that require options.verbose, rather than attempt to paper over the problem by resurrecting that field. I've attempted to do so in subsequent commits (see r1297724 and r1297725). -Hyrum On Tue, Mar 6, 2012 at 3:11 PM, Alexey Neyman sti...@att.net wrote: Argh, damn. Last one: [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Partially revert r1297676, other code needs options.verbose. (_parse_options): Call logger.setLevel(logging.DEBUG) if verbose output is requested. ]]] On Tuesday, March 06, 2012 12:50:30 pm Alexey Neyman wrote: Please use the attached patch instead (the first one fixed --list, but not --list --verbose). Regards, Alexey. On Tuesday, March 06, 2012 12:45:21 pm Alexey Neyman wrote: Hi all, Revision 1297676 by hwright broke the command line tests' --list option (and probably, some other interfaces): the options dict no longer contains verbose option as --verbose handler was changed from 'store_true' to 'callback'. The attached patch fixes it by reintroducing options.verbose. [[[ * subversion/tests/cmdline/svntest/main.py (_create_parser): Set options.verbose to False by default, override with True from set_log_debug() callback. ]]] Regards, Alexey.
Re: [PATCH] Revision 1297676 broke --list option of command line tests
On Tue, Mar 6, 2012 at 3:27 PM, Alexey Neyman sti...@att.net wrote: The code which accesses options.verbose is still there in some places, including TestRunner.list(): 1071809 pburba # If there is no filter or this test made if through 1071809 pburba # the filter then print it! 1071809 pburba if options.milestone_filter is None or len(issues): 1071809 pburba if options.verbose and self.pred.inprogress: ... Shouldn't all the options.verbose instances be removed _before_ changing the option handler to 'callback'? In retrospect, yes. I guess I'm still in my run the tests, oh it passes, commit it way of working as I withdraw from the long slog of Ev2 compat shims. And I'm lazy, as Greg likes to point out. ;) -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Logging in the python tests
After a bit of a bump today, I've added the use of the python logging module to our test suite as a way of logging output from the test framework and the tests themselves. The end goal is to remove all the many print() calls we have in the tests, and run almost everything through the logger, allowing us to better control where the log gets sent to, its formatting, etc. I'm sure I'll break more things along the way, and I've yet to fully iron out the standards for using INFO vs. WARN vs. ERROR within the tests, but I'd appreciate any insight or help or questions folks might have as this transition goes forward. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: [PATCH] svn: Attempted to get textual contents of a *non*-file node
Thanks for the patch Alexey. Forwarding it to dev@.
Alexey Neyman wrote on Tue, Mar 06, 2012 at 16:27:11 -0800:
Hi Daniel,
On Monday, March 05, 2012 11:33:33 pm Daniel Shahaf wrote:
Alexey Neyman wrote on Mon, Mar 05, 2012 at 16:14:24 -0800:
Hi all,
I ran into the following error message with Subversion:
svn: Attempted to get textual contents of a *non*-file node
The issue, as pointed out by email thread [1], is that the directory
being merged contains a file with the same name as the directory. I.e.,
there is /trunk/foo directory containing /trunk/foo/foo file.
However, even if I tried the suggestion from Ben Collins-Sussman, it
didn't help: 'svn merge' still complained with the same error message.
Reproduction script attached. Is there a way such projects can use 'svn
merge' command?
I tried with Subversion trunk and, although the error message is
different (svn: E160017: '/trunk/foo' is not a file), the result is
still the same. While it is a better message than the one in 1.6, it
still does not explain why Subversion expects /trunk/foo to be a file
for the following commands:
svn merge -c 4 ^/trunk/foo
svn merge -c 4 ^/trunk/foo .
svn merge ^/trunk/foo@3 ^/trunk/foo@4 .
svn merge -r 3:4 ^/trunk/foo .
Yeah, I just tried with trunk, couldn't get the merge to work, with
those commands (some of which are made equivalent by the argument
parser) or with svn merge ^/trunk/foo@3 ^/trunk/foo@4 foo.
Looks like a bug to me, assuming it works when the dir and the file are
not both named the same thing.
I confirm it works when dir and file do not have the same name. E.g., if you
rename new file 'B' in the attached testcase to, say, 'X' - it passes. There
is one more condition for this bug to manifest: the offending directory must
be the current directory: even though
$ svn merge -c 4 ^/trunk/foo .
fails, the following
$ wc=`pwd`
$ cd ..
$ svn merge -c 4 ^/trunk/foo $wc
works. I guess, it's sort of a workaround.
As another side note, Subversion leaves behind a zero-sized temporary
file created for the merge.
And this one too. (the file is in the wc root)
Regards,
Alexey.
[1] http://markmail.org/message/qqh3r6d4tcdyjnz2#query:
+page:1+mid:vcjektlfn37mxyld+state:results
Could you file an issue? Perhaps send a patch adding a regression test
for this (in Python)? (See subversion/tests/cmdline/README)
Issue 4139 created. Attached is a patch that adds an XFail to the test suite
for this issue.
Regards,
Alexey.
Index: subversion/tests/cmdline/merge_tests.py
===
--- subversion/tests/cmdline/merge_tests.py (revision 1297725)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -17467,6 +17467,64 @@
'merge', sbox.repo_url + '/A',
A_COPY_path)
+@XFail()
+@Issue(4139)
+def merge_dir_file_same_name(sbox):
+ merged directory has file with same name
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ # Paths of interest in first WC
+ A_B_B_path = os.path.join(wc_dir, 'A', 'B', 'B')
+
+ # Create file 'B' in path '/A/B' - revision 2
+ svntest.main.file_write(A_B_B_path, Project B now has file B)
+ svntest.main.run_svn(None, 'add', A_B_B_path)
+ expected_output = svntest.wc.State(wc_dir, {
+'A/B/B' : Item(verb='Adding'),
+})
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.add({
+'A/B/B' : Item(status=' ', wc_rev=2),
+})
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+expected_status, None,
+wc_dir)
+
+ # Copy '/A' to '/A_COPY' - revision 3
+ sbox.simple_repo_copy('A', 'A_COPY')
+
+ # Now modify '/A/B/B' - revision 4
+ svntest.main.file_write(A_B_B_path, File B is now modified)
+ expected_output = svntest.wc.State(wc_dir, {
+'A/B/B' : Item(verb='Sending'),
+})
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.add({
+'A/B/B' : Item(status=' ', wc_rev=4),
+})
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+expected_status, None,
+wc_dir)
+
+ # Now check out A_COPY/B in a separate WC
+ wc_copy = sbox.add_wc_path('copy')
+ svntest.actions.run_and_verify_svn(None, None, [], 'checkout',
+ sbox.repo_url + '/A_COPY/B', wc_copy)
+
+ # And try to merge the change from revision 4 of /A/B/B to
+ # /A_COPY/B/B from the WC dir.
+ saved_cwd = os.getcwd()
+ os.chdir(wc_copy)
+ expected_output = expected_merge_output([[4]],
+ ['UB\n',
+' U \.',])
+
