Re: FSFS format 6

[Stefan Fuhrmann]

On 20.02.2011 21:02, Johan Corveleyn wrote:

On Sun, Feb 20, 2011 at 6:35 PM, Mark Mielke  wrote:


That said, I'm also (in principle) against implementing cache of open file
handles. I prefer architectures that cache intermediate data in a processed
form that the application has made a determined choice to make use of such
that the cache is the most useful to the application, rather than a
transparent caching layer that guesses at what is safe. The OS file system
layer is exactly this - any caching it does is transparent to the
application and a guess. Guesses are dangerous, which is exactly why the OS
file system layer cannot do as much caching unless it has 100% control of
the file system (= local file system).

Agreed. For that very reason, I added extensive
caching to the FSFS code and got even more of that
in the pipeline for 1.8.

That being said, there are still typical situations in
which the data cache may not be effective:

* access to relatively rarely read data
  (log, older tags;
   you still want to perform decently in that case)
* first access to the latest revision
  (due to the way transactions are implemented,
   it is difficult to fill all the caches upon write)
* amount of active data > available RAM
  (throws you back to the first issue more often)


I agree that it would be best if the architecture was so that svn
could organize its work for most use cases in a way that's efficient
for the lower levels of the system. For instance, for "svn log", svn
should in theory be able to do its work with exactly 1 open/close per
rev file (or in a packed repository, maybe even only 1 open/close per
packed file).

Yes, it may be very hard to anticipate what data may
be needed further down the road, even if we had a
marvelous "1 query gets it all" interface where feasible:
svn log, for instance, is often run with a limit on the number
of results. However, there is no way to tell how much of
a packed file needs to be read to process that query.
There is only a lower bound.

So, it can be very beneficial to keep a small number of
file handles around to "bridge" various stages / iterations
within a single request.

But right now, this isn't the case, and I think it would be a huge
amount of work, change in architecture, layering, ... Until that
happens, I think such a generic file-handle caching layer could prove
very helpful :-). Note though that, if I understood correctly, the
file-handle caching of the performance branch will not be reintegrated
into 1.7, but maybe 1.8 ...

But maybe stefan2 can comment more on that :-).

Because keeping file open for a potentially much
longer period of time may have an impact on other,
rarely run operations like pack, I don't think we should
risk merging this into 1.7.

-- Stefan^2.

[l10n] Translation status report for trunk r1073220

[Subversion Translation Status]

Translation status report for trunk@r1073220

  lang   trans untrans   fuzzy obs
--
de2045 132 249 216
es1984 193 282 354
fr2168   9  19  22
it1837 340 476 174
ja1975 202 351 602
ko2019 158 200 196
nb2036 141 208 325
pl2068 109 160  98
 pt_BR1805 372 496 166
sv1760 417 519 174
 zh_CN2174   3  18  13
 zh_TW1739 438 555 236

Re: svn commit: r1073102 - /subversion/trunk/subversion/include/private/svn_debug.h

[Noorul Islam K M]

Branko Čibej  writes:

> On 22.02.2011 02:50, Noorul Islam K M wrote:
>
>> danie...@apache.org writes:
>>
>>> Author: danielsh
>>> Date: Mon Feb 21 18:14:02 2011
>>> New Revision: 1073102
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1073102&view=rev
>>> Log:
>>> * subversion/include/private/svn_debug.h
>>>   (SVN_DBG): Merge docstring with the documentation comment elsefile.
>>>
>> Is this typo? 
>
> Nah, it's just creative language. "Elsewhere" sounds so plain and
> boring. Who cares that the result is total nonsense and likely to be
> misunderstood by 99% of all readers? :)
>

But why do we have to use that? Is it not possible to use proper English
word and still be creative?

Thanks and Regards
Noorul

Re: svn commit: r1073102 - /subversion/trunk/subversion/include/private/svn_debug.h

[Branko Čibej]

On 22.02.2011 02:50, Noorul Islam K M wrote:
> danie...@apache.org writes:
>
>> Author: danielsh
>> Date: Mon Feb 21 18:14:02 2011
>> New Revision: 1073102
>>
>> URL: http://svn.apache.org/viewvc?rev=1073102&view=rev
>> Log:
>> * subversion/include/private/svn_debug.h
>>   (SVN_DBG): Merge docstring with the documentation comment elsefile.
>>
> Is this typo? 

Nah, it's just creative language. "Elsewhere" sounds so plain and
boring. Who cares that the result is total nonsense and likely to be
misunderstood by 99% of all readers? :)

-- Brane

Re: svn commit: r1073102 - /subversion/trunk/subversion/include/private/svn_debug.h

[Noorul Islam K M]

danie...@apache.org writes:

> Author: danielsh
> Date: Mon Feb 21 18:14:02 2011
> New Revision: 1073102
>
> URL: http://svn.apache.org/viewvc?rev=1073102&view=rev
> Log:
> * subversion/include/private/svn_debug.h
>   (SVN_DBG): Merge docstring with the documentation comment elsefile.
>

Is this typo? 

Thanks and Regards
Noorul

> Suggested by: hwright, julianfoad
>
> Modified:
> subversion/trunk/subversion/include/private/svn_debug.h
>
> Modified: subversion/trunk/subversion/include/private/svn_debug.h
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_debug.h?rev=1073102&r1=1073101&r2=1073102&view=diff
> ==
> --- subversion/trunk/subversion/include/private/svn_debug.h (original)
> +++ subversion/trunk/subversion/include/private/svn_debug.h Mon Feb 21 
> 18:14:02 2011
> @@ -37,22 +37,6 @@
>  extern "C" {
>  #endif /* __cplusplus */
>  
> -/*
> - * The primary macro defined by this header is SVN_DBG(). It helps by
> - * printing stuff to stdout (or however SVN_DBG_OUTPUT is defined) for
> - * debugging purposes. Typical usage is like this:
> - *
> - *   SVN_DBG(("cleanup. type=%d  path='%s'\n", lock->type, lock->path));
> - *
> - * producing:
> - *
> - *   DBG: lock.c: 292: cleanup. type=2  path='include/private'
> - *
> - * Note that these output lines are filtered by our test suite automatically,
> - * so you don't have to worry about throwing off expected output.
> - */
> -
> -
>  /* A couple helper functions for the macros below.  */
>  void
>  svn_dbg__preamble(const char *file, long line, FILE *output);
> @@ -75,17 +59,21 @@ svn_dbg__printf(const char *fmt, ...)
>  #else
>  
>  /** Debug aid macro that prints the file:line of the call and printf-like
> - * arguments to the #SVN_DBG_OUTPUT stdio stream.  Typical usage:
> + * arguments to the #SVN_DBG_OUTPUT stdio stream (#stdout by default).  
> Typical
> + * usage:
>   *
>   * 
> - *   SVN_DBG(("path='%s' rev=%ld\n", dirent, revnum));
> + *   SVN_DBG(("rev=%ld kind=%s\n", revnum, svn_kind_to_word(kind)));
>   * 
>   *
>   * outputs:
>   *
>   * 
> - *   kitchensink.c:42:path='/tmp/svn/wc1/iota' rev=3141592
> + *   DBG: kitchensink.c: 42: rev=3141592 kind=file
>   * 
> + *
> + * Note that these output lines are filtered by our test suite automatically,
> + * so you don't have to worry about throwing off expected output.
>   */
>  #define SVN_DBG(ARGS) (svn_dbg__preamble(__FILE__, __LINE__, 
> SVN_DBG_OUTPUT), \
> svn_dbg__printf ARGS)

Re: apr_file_flush and short writes

[Branko Čibej]

On 21.02.2011 23:55, Blair Zajac wrote:
> On 2/9/11 10:30 AM, Blair Zajac wrote:
>> On 2/9/11 1:38 AM, John Szakmeister wrote:
>>> On Mon, Feb 7, 2011 at 4:26 PM, Blair Zajac wrote:
 [I sent this to d...@apr.apache.org but haven't received a response.
 Thread
 here:
 http://mail-archives.apache.org/mod_mbox/apr-dev/201102.mbox/%3cf7b1928d-d32f-48dd-b8d9-80b26906a...@orcaware.com%3E


 . Given the importance of writing complete files for svn, could
 somebody
 take a look and see if this is a valid issue].

 I was looking at apr_file_flush() and think I found a bug where if
 write()
 doesn't do a full write, then the apr_file_t will destroy and
 buffered data
 because it sets its internal buffer position back to 0.
>>>
>>> Yeah, that looks like a bug.
>>>
 Here's a patch for this:

 Index: file_io/unix/readwrite.c
 ===
 --- file_io/unix/readwrite.c (revision 1067340)
 +++ file_io/unix/readwrite.c (working copy)
 @@ -409,7 +409,11 @@
 rv = errno;
 } else {
 thefile->filePtr += written;
 - thefile->bufpos = 0;
 + if (written != thefile->bufpos)
 + memmove(thefile->buffer,
 + thefile->buffer + written,
 + thefile->bufpos - written);
 + thefile->bufpos -= written;
 }
 }

 Beyond this, there's no a mechanism to report that all the buffered
 data
 didn't get into the file. Perhaps apr_file_flush() should loop
 until the
 entire buffer is written or it gets a non-EINTR error?
>>>
>>> I think it you're right, it should loop around. Good catch!
>>
>> Thanks!
>>
>> Who here can commit to apr?
>
> Stefan Fritsch committed a looping solution in r1073142 (trunk),
> r1073145 (1.5.x), r1073146 (1.4.x).

The looping solution looks better than the memmove.
Are we ready to declare apr-0.9 out of bounds? Our code still nominally
supports that version, but I thin we should move forward, compatibility
guidelines notwithstanding.

If there's disagreement, I can probably port that fix to apr-0.9 ... but
there's not likely to be another 0.9.x release, ever.

-- Brane

Re: apr_file_flush and short writes

[Stefan Sperling]

On Mon, Feb 21, 2011 at 02:55:50PM -0800, Blair Zajac wrote:
> On 2/9/11 10:30 AM, Blair Zajac wrote:
> >On 2/9/11 1:38 AM, John Szakmeister wrote:
> >>On Mon, Feb 7, 2011 at 4:26 PM, Blair Zajac wrote:
> >>>[I sent this to d...@apr.apache.org but haven't received a response. Thread
> >>>here:
> >>>http://mail-archives.apache.org/mod_mbox/apr-dev/201102.mbox/%3cf7b1928d-d32f-48dd-b8d9-80b26906a...@orcaware.com%3E
> >>>
> >>>. Given the importance of writing complete files for svn, could somebody
> >>>take a look and see if this is a valid issue].
> >>>
> >>>I was looking at apr_file_flush() and think I found a bug where if write()
> >>>doesn't do a full write, then the apr_file_t will destroy and buffered data
> >>>because it sets its internal buffer position back to 0.
> >>
> >>Yeah, that looks like a bug.
> >>
> >>>Here's a patch for this:
> >>>
> >>>Index: file_io/unix/readwrite.c
> >>>===
> >>>--- file_io/unix/readwrite.c (revision 1067340)
> >>>+++ file_io/unix/readwrite.c (working copy)
> >>>@@ -409,7 +409,11 @@
> >>>rv = errno;
> >>>} else {
> >>>thefile->filePtr += written;
> >>>- thefile->bufpos = 0;
> >>>+ if (written != thefile->bufpos)
> >>>+ memmove(thefile->buffer,
> >>>+ thefile->buffer + written,
> >>>+ thefile->bufpos - written);
> >>>+ thefile->bufpos -= written;
> >>>}
> >>>}
> >>>
> >>>Beyond this, there's no a mechanism to report that all the buffered data
> >>>didn't get into the file. Perhaps apr_file_flush() should loop until the
> >>>entire buffer is written or it gets a non-EINTR error?
> >>
> >>I think it you're right, it should loop around. Good catch!
> >
> >Thanks!
> >
> >Who here can commit to apr?
> 
> Stefan Fritsch committed a looping solution in r1073142 (trunk),
> r1073145 (1.5.x), r1073146 (1.4.x).
> 
> Blair

Is it possible that this is related to issue 3705?
http://subversion.tigris.org/issues/show_bug.cgi?id=3705
"fix root cause of known fixable FSFS corruption"

Re: apr_file_flush and short writes

[Blair Zajac]

On 2/9/11 10:30 AM, Blair Zajac wrote:

On 2/9/11 1:38 AM, John Szakmeister wrote:

On Mon, Feb 7, 2011 at 4:26 PM, Blair Zajac wrote:

[I sent this to d...@apr.apache.org but haven't received a response. Thread
here:
http://mail-archives.apache.org/mod_mbox/apr-dev/201102.mbox/%3cf7b1928d-d32f-48dd-b8d9-80b26906a...@orcaware.com%3E

. Given the importance of writing complete files for svn, could somebody
take a look and see if this is a valid issue].

I was looking at apr_file_flush() and think I found a bug where if write()
doesn't do a full write, then the apr_file_t will destroy and buffered data
because it sets its internal buffer position back to 0.


Yeah, that looks like a bug.


Here's a patch for this:

Index: file_io/unix/readwrite.c
===
--- file_io/unix/readwrite.c (revision 1067340)
+++ file_io/unix/readwrite.c (working copy)
@@ -409,7 +409,11 @@
rv = errno;
} else {
thefile->filePtr += written;
- thefile->bufpos = 0;
+ if (written != thefile->bufpos)
+ memmove(thefile->buffer,
+ thefile->buffer + written,
+ thefile->bufpos - written);
+ thefile->bufpos -= written;
}
}

Beyond this, there's no a mechanism to report that all the buffered data
didn't get into the file. Perhaps apr_file_flush() should loop until the
entire buffer is written or it gets a non-EINTR error?


I think it you're right, it should loop around. Good catch!


Thanks!

Who here can commit to apr?


Stefan Fritsch committed a looping solution in r1073142 (trunk), r1073145 
(1.5.x), r1073146 (1.4.x).


Blair

Re: [PATCH] Fix issue #3686 - executable bit not set during merge

[Daniel Becroft]

On Sun, Feb 13, 2011 at 2:15 AM, Daniel Shahaf wrote:

> Daniel Becroft wrote on Sat, Feb 12, 2011 at 08:37:12 +1000:
> >  On Sat, Feb 12, 2011 at 7:31 AM, Daniel Shahaf  >wrote:
> >
> > > Daniel Becroft wrote on Sat, Feb 12, 2011 at 06:27:31 +1000:
> > > > On Fri, Feb 11, 2011 at 11:26 PM, Daniel Shahaf <
> d...@daniel.shahaf.name
> > > >wrote:
> > > > > Daniel Becroft wrote on Thu, Feb 10, 2011 at 07:21:30 +1000:
> > > > > > @@ -1118,6 +1120,33 @@ merge_binary_file(svn_skel_t **work_items,
> > > > > > +  /* Attempt to merge the binary file. At the moment, we can
> only
> > > > > > + handle the special case: if the LEFT side of the merge is
> equal
> > > > > > + to WORKING, then we can copy RIGHT directly. */
> > > > >
> > > > > The comment in libsvn_client mentioned two special case, what
> happened
> > > > > to the other one?  Does the existing wc code already handle it?
> (I'd be
> > > > > surprised)
> > > > >
> > > > >  - Alternately, if the 'left' side of the merge doesn't
> exist
> > > in
> > > > >  - the repository, and the 'right' side of the merge is
> > > > >  - identical to the WC, pretend we did the merge (a no-op).
> > > > >
> > > >
> > > > I've been trying to think of a valid scenario for this to occur, but
> I
> > > can't
> > > > seem to think of one. There's a comment further up:
> > > >
> > >
> > > Concocted:
> > > % svn add foo.bin
> > > % svn ci -m r5
> > > % svn rm ^/foo.bin -m r6
> > > % svnmucc -m r7 put foo.bin ^/foo.bin
> > > % svn merge -c7 ^/ ./
> > >
> >
> > Maybe it's early, and the coffee hasn't kicked in yet, but wouldn't (and
> > shouldn't) this give a tree-conflict? foo.bin@7 and foo.bin in the WC
> are
> > two different nodes (with the same name).
> >
>
> I hadn't considered that this might be a tree conflict (an add of an
> already-existing file with the same contents).  My point was not the
> tree conflict but that the scenario described in the "Alternately" can
> happen.
>

I've looked through this several times now to try and be sure that I'm not
missing anything.

Previously, the "Special case" block handled both when to merge the binary
file, as well as how. Now, the "when" logic is now the same logic as text
files, and the how has been moved into libsvn_wc (rather than
libsvn_client).

I've attached the file version of the patch.

> > >   /* Other easy outs:  if the merge target isn't under version
> > > >  control, or is just missing from disk, fogettaboutit.  There's
> no
> > > >  way svn_wc_merge4() can do the merge. */
> > > >
> > > > This should apply to all situations (binary and text files), so I
> think
> > > this
> > > > second "special case" is redundant.
> > > >
> > >
> > > Not sure.  If the merge-left does not exist, and the local file doesn't
> > > exist either, then the situation is 'compatible' and can be merged...
> > >
> >
> > Isn't this just an incoming add (which is handled by a different
> function)?
> >
>
> I don't know the code very well.  If you're saying that at the point of
> the comment we know that the merge-left exists, then I agree that it
> makes sense to flag a conflict if the merge target is non-existent.
>

Yep, this is already done as part of the standard merge logic (ie why
doesn't left exist? It is an incoming add, or already deleted in the WC,
etc).


>
> > > > I can submit a follow-up patch that fixes this as well, if necessary.
> > >
> > > That would be great, assuming that the FALSE *really* should be changed
> > > to TRUE.  (I haven't investigated that myself.)
> > >
> >
> > Not sure what is is in reference to. I was thinking of just putting a if
> > (content_state) before the local modifications check (again, coffee may
> not
> > have kicked in yet).
> >
>
> I was trying to say: "It would be great if you could look further into
> that matter, but I haven't done so myself yet so I don't know if a patch
> is required or the current code is correct."
>

Gotcha (I just couldn't figure out where the TRUE/FALSE reference came
from), but anywho ...


> In other words, thanks for looking into this, but I don't know myself
> whether a follow-up patch would be necessary.
>
> > Cheers,
> > Daniel^2.
>
> HTH


Yep. I've attached the (final) version of the patch (generated with the C
functions this time), and the log message is below.

[[[
Fix issue #3686 - executable bit not set during merge.

The cause was the special case in libsvn_client, which bypassed the use of
the
workqueue. This logic has now been moved into libsvn_wc.

Additionally, this change allows the status of binary files (during a
dry-run
merge) to be reported correctly (previously, all binary files were reported
as
conflicted).

* subversion/libsvn_client/merge.c
 (merge_file_changed): Remove binary-merge special case (now in libsvn_wc).
  Remove merge_required variable (resulting in indentation changes).

* subversion/libsvn_wc/merge.c
 (merge_binary_file): Add dry_run parameter. Add the special case merging
  of binary files.
 (

Re: [PATCH] allow svnsync to translate non-UTF-8 log messages to UTF-8 (v. 5.1)

[Gavin Beau Baumanis]

Hi Everyone,

I have logged this submission in the issue tracker.

#3817 refers.
http://subversion.tigris.org/issues/show_bug.cgi?id=3817


On 21/02/2011, at 3:31 AM, Danny Trebbien wrote:

> On Mon, Feb 14, 2011 at 12:38 AM, Daniel Shahaf  
> wrote:
>> Danny Trebbien wrote on Sun, Feb 13, 2011 at 20:20:45 -0800:
>>> Attached are the corrected patch and log message. The new patch
>>> incorporates this fix:
>>> https://github.com/dtrebbien/subversion/commit/190f876b52626be6b30fe4e5a311c113fd87e589
>> 
>> Is there a github link that shows the equivalent of 'svn log --diff' of
>> this patch?  It's now about 700 lines, so having such a link would be
>> very helpful to reviewers.
>> 
>> Daniel
> 
> GitHub has a special "compare branches" URL, but you have to know the
> SHA1 of two commits:  my latest non-merge commit and the last commit
> of "upstream/trunk" that I merged into my master branch before that
> commit.  Currently those are 190f876b52626be6b30fe4e5a311c113fd87e589
> and c3e62d94c79a91176e2faab5bf6032bc070d5bc4, respectively (I
> determined this with the `gitk` tool, which draws the commit DAG
> [directed acyclic graph]).  The corresponding "compare" URL on GitHub
> is:  
> .
> If you clone my git repo, then this roughly corresponds to the output
> of `git diff c3e62d94c7...190f876b52`.
> 
> That lists all of my changes to my branch.  I'm not sure that it is
> going to be helpful because I haven't maintained separate branches for
> each of my proposed changes to Subversion trunk, so everything is kind
> of mixed in.

RE: svn commit: r1073043 - in /subversion/trunk/subversion: include/svn_delta.h mod_dav_svn/mod_dav_svn.c svnserve/main.c

[Bert Huijben]

> -Original Message-
> From: phi...@apache.org [mailto:phi...@apache.org]
> Sent: maandag 21 februari 2011 17:08
> To: comm...@subversion.apache.org
> Subject: svn commit: r1073043 - in /subversion/trunk/subversion:
> include/svn_delta.h mod_dav_svn/mod_dav_svn.c svnserve/main.c
> 
> Author: philip
> Date: Mon Feb 21 16:08:15 2011
> New Revision: 1073043
> 
> URL: http://svn.apache.org/viewvc?rev=1073043&view=rev
> Log:
> Avoid using "best" to refer to maximum compression.
> 
> * subversion/include/svn_delta.h
>   (SVN_BEST_COMPRESSION_LEVEL): Renamed to ...
>   (SVN_MAX_COMPRESSION_LEVEL): ... this.
> 
> * subversion/svnserve/main.c
>   (main): Adjust for rename.
> 
> * subversion/mod_dav_svn/mod_dav_svn.c
>   (SVNCompressionLevel_cmd): Adjust for rename.
>   (cmds): Use "maximum" rather than "best", don't refer to "ZIP".
> 
> Modified:
> subversion/trunk/subversion/include/svn_delta.h
> subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
> subversion/trunk/subversion/svnserve/main.c
> 
> Modified: subversion/trunk/subversion/include/svn_delta.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_de
> lta.h?rev=1073043&r1=1073042&r2=1073043&view=diff
> ==
> 
> --- subversion/trunk/subversion/include/svn_delta.h (original)
> +++ subversion/trunk/subversion/include/svn_delta.h Mon Feb 21 16:08:15
> 2011
> @@ -56,7 +56,7 @@ extern "C" {
> 
>  /** This is the maximum compression level we can pass to zlib.
>   */
> -#define SVN_BEST_COMPRESSION_LEVEL 9
> +#define SVN_MAX_COMPRESSION_LEVEL 9

Is this macro new in 1.7?

If it is we should document it as new and if not the old name should be 
properly deprecated.


Bert

Re: [PATCH] New XFail test case for issue 3013

[Gavin Beau Baumanis]

Ping. This thread has received no new comments.



On 28/01/2011, at 6:40 PM, Noorul Islam K M wrote:

> Daniel Shahaf  writes:
> 
>> Noorul Islam K M wrote on Fri, Jan 28, 2011 at 12:27:46 +0530:
>> 
>>> Daniel Shahaf  writes:
>>> 
 Looks good, but I have a question:
 
 Noorul Islam K M wrote on Wed, Jan 26, 2011 at 13:12:54 +0530:
> 
> Attached is the python test for issue 3013. This incorporates the steps
> from the shell script attached in the tracker.
> 
> Log 
> [[[
> 
> New XFail test for issue 3013.
> 
> * subversion/tests/cmdline/update_tests.py
>  (update_after_switching_to_deleted_path, test_list): New XFail test
> 
> Patch by: Noorul Islam K M 
> ]]]
> 
> Thanks and Regards
> Noorul
> 
 
> Index: subversion/tests/cmdline/update_tests.py
> ===
> --- subversion/tests/cmdline/update_tests.py  (revision 1063610)
> +++ subversion/tests/cmdline/update_tests.py  (working copy)
> @@ -5347,6 +5347,34 @@
>   svntest.main.run_svn(None, 'delete', os.path.join('A2', 'mu'))
>   svntest.main.run_svn(None, 'update', os.path.join('A2', 'mu'))
> 
> +### regression test for issue #3013
> +def update_after_switching_to_deleted_path(sbox):
> +  "update after switching to deleted path"
> +  
> +  sbox.build()
> +  wc_dir = sbox.wc_dir
> +  repo_url = sbox.repo_url
> +
> +  # switch to A/B
> +  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'switch',
> +  repo_url + "/A/B", wc_dir)
> +
> +  # delete A/D
> +  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'rm',
> +  repo_url + "/A/D", '-m', 
> +  'Remove A/D')
> +
> +  # switch to A/D and this is known to fail
> +  svntest.actions.run_and_verify_svn2(None, None, 
> svntest.verify.AnyOutput,
> +  1, 'switch', repo_url + "/A/D", 
> wc_dir)
> +
> +  # switch to A/D@1 and this is known to succeed
> +  svntest.actions.run_and_verify_svn2(None, None, [], 0, 'switch',
> +  repo_url + "/A/D@1", wc_dir)
> +
> +  # update should succeed
> +  svntest.actions.run_and_verify_svn2(None, None, [], 0, "up", wc_dir)
> +
 
 Should this 'update' succeed?  In my testing, updating the wc root to
 a revision it does not exist in fails.
>>> 
>>> "/A/D" @ revision 1 does exist. So update should succeed but it is
>>> failing. This is the issue.
>>> 
>> 
>> I think the issue is that the 'switch' is failing, nothing about the
>> update.
>> 
>> But, in fact: both switch and update work as long as the directory
>> they're targetting is a wc subdir (as opposed to a wc root).  Therefore,
>> I'm inclined to mark the issue as FIXED and adjust the test to test that
>> the switch works when the being-switched directory is not the wc root.
>> 
>> Thoughts?
>> 
> 
> Pasting our conversation on IRC here.
> 
>  danielsh_: The switch is known fail @HEAD and not @1  [12:48]
>  A/D is non-existent @HEAD and existent @1 hence first switch is known
>to fail and the second one to succeed
>  When the second one succeeds the subsequent update should update WC
>with repo_url@1   [12:49]
>  noorul: see mail, I think it's a different issue,
>  the switch actually works when you do 'svn sw $URL
>   /path/to/wc/rootdir/some/subdir'
>  I think I correctly converted
>
> http://subversion.tigris.org/nonav/issues/showattachment.cgi/1157/3013.sh
>into a test  [12:52]
>  yes
>  I think the 'update' shouldn't have been in the .sh script either
>   btw  [12:53]
>  but that's history
>  Before svn update if we use 'svn info' we could see that it is
>pointing to repo_url@1  [12:54]
>  Shouldn't svn up bring wc in sync with that revision?
>  A/D existing @1 the error message is misleading  [12:55]
>  currently the 'up' fails iff the
>   dir-being-updated-past-its-deletion is the wc root
>  I think that's sensible,
>  to error if the wcroot would have to be deleted,
>  but feel free to discuss that on dev@
>  re error messages: haven't checked whether they're misleading or
>   not
>  haven't read them actually, just checked if the up succeeded or
>   not and what rev it put me at
>  danielsh_: Do you mean to say that "svn up A/D" would succeed instead
>of "svn up ."   [13:03]
>  noorul: svn rm ^/A/D; svn up A/D/;   <-- succeeds  [13:04]
>  noorul: svn co ^/A foo; cd foo; svn rm ^/A; svn up;   <-- fails
>  that's current behaviour  [13:05]
>  danielsh_: With respect to the test case  [13:06]
>  I mean the test case that I submitted, do you think that svn up
>should succeed?
>  noorul: I think the 'svn up' doesn't belong in that

Re: Is the svn:// protocol secure when encrypted via SASL?

[Keith Palmer Jr.]

Thanks.

---
 - Keith Palmer
   ConsoliBYTE, LLC
   Ask for a quote! - QuickBooks Integration and Software Development
   ke...@consolibyte.com
   1-860-341-1464
   http://www.ConsoliBYTE.com/
   Follow us on Twitter at: https://twitter.com/consolibyte
   AIM: consolibyte
   MSN: supp...@consolibyte.com
   Yahoo: consolib...@yahoo.com
   Gtalk: consolibyte
   Skype: consolibyte




On Feb 21, 2011, at 3:03 PM, Greg Hudson wrote:

> On Mon, 2011-02-21 at 14:48 -0500, Keith Palmer Jr. wrote:
>> Nothing in what you just copy-pasted indicates whether it's *the
>> actual data stream* that's being encrypted, or just the
>> *authentication*. I need to know if the checked-out files that are
>> being transferred are encrypted or not. 
> 
> The SASL "security layer" refers to protection of the actual data
> stream.  "Encryption of the authentication" isn't really a meaningful
> concept in SASL parlance; mechanisms always perform authentication steps
> as securely as they are able.
> 
> 

Re: Is the svn:// protocol secure when encrypted via SASL?

[Keith Palmer Jr.]

When I asked last week (and on many, many forums and in the users list) 
everyone told me it only encrypts the authentication, not the data, despite 
what the docs seem to indicate. 

---
 - Keith Palmer
   ConsoliBYTE, LLC
   Ask for a quote! - QuickBooks Integration and Software Development
   ke...@consolibyte.com
   1-860-341-1464
   http://www.ConsoliBYTE.com/
   Follow us on Twitter at: https://twitter.com/consolibyte
   AIM: consolibyte
   MSN: supp...@consolibyte.com
   Yahoo: consolib...@yahoo.com
   Gtalk: consolibyte
   Skype: consolibyte




On Feb 21, 2011, at 3:02 PM, Philip Martin wrote:

> "Keith Palmer Jr."  writes:
> 
>> Nothing in what you just copy-pasted indicates whether it's *the
>> actual data stream* that's being encrypted, or just the
>> *authentication*. I need to know if the checked-out files that are
>> being transferred are encrypted or not.
> 
> SASL provides encryption if negogiated.  What makes you think it doesn't
> apply to the data stream if that what is configured?
> 
> -- 
> Philip

Re: Is the svn:// protocol secure when encrypted via SASL?

[Stefan Sperling]

On Mon, Feb 21, 2011 at 02:48:49PM -0500, Keith Palmer Jr. wrote:
> 
> Nothing in what you just copy-pasted indicates whether it's *the actual data 
> stream* that's being encrypted, or just the *authentication*. I need to know 
> if the checked-out files that are being transferred are encrypted or not. 
> 

See https://svn.eu.apache.org/repos/asf/subversion/trunk/notes/sasl.txt
the section called "6. Encryption".

Re: Is the svn:// protocol secure when encrypted via SASL?

[Greg Hudson]

On Mon, 2011-02-21 at 14:48 -0500, Keith Palmer Jr. wrote:
> Nothing in what you just copy-pasted indicates whether it's *the
> actual data stream* that's being encrypted, or just the
> *authentication*. I need to know if the checked-out files that are
> being transferred are encrypted or not. 

The SASL "security layer" refers to protection of the actual data
stream.  "Encryption of the authentication" isn't really a meaningful
concept in SASL parlance; mechanisms always perform authentication steps
as securely as they are able.

Re: Is the svn:// protocol secure when encrypted via SASL?

[Philip Martin]

"Keith Palmer Jr."  writes:

> Nothing in what you just copy-pasted indicates whether it's *the
> actual data stream* that's being encrypted, or just the
> *authentication*. I need to know if the checked-out files that are
> being transferred are encrypted or not.

SASL provides encryption if negogiated.  What makes you think it doesn't
apply to the data stream if that what is configured?

-- 
Philip

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Johan Corveleyn]

On Mon, Feb 21, 2011 at 1:50 PM, Noorul Islam K M  wrote:
> "Bert Huijben"  writes:
>
>>> -Original Message-
>>> From: Branko Čibej [mailto:br...@xbc.nu] On Behalf Of Branko Cibej
>>> Sent: maandag 21 februari 2011 13:19
>>> To: Philip Martin
>>> Cc: dev@subversion.apache.org
>>> Subject: Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple
>>> checkouts.
>>
>>> You could argue that tiny tests that do not modify a sandbox can be
>>> merged together. Of course, if there's a bug in the implementation and
>>> one of those subtests /does/ modify the sandbox, causing another down
>>> the line to fail for no obvious reason, that hardly makes life easier
>>> for the poor sod who has to analyse that mess.
>>
>> Wasn't there some option to ask for a read-only sandbox?
>>
>
> On trunk the test already use read-only option. This patch reduces
> number of checkouts. All these tests require only read-only working
> copy.

There's another way to make these tests go faster: we simply have to
make checkout faster :-). As in: "blazingly fast" ...

Anyone wants to take on this bite-sized task? ;-)

Cheers,
-- 
Johan

Re: Is the svn:// protocol secure when encrypted via SASL?

[Keith Palmer Jr.]

Nothing in what you just copy-pasted indicates whether it's *the actual data 
stream* that's being encrypted, or just the *authentication*. I need to know if 
the checked-out files that are being transferred are encrypted or not. 

---
 - Keith Palmer
   ConsoliBYTE, LLC
   Ask for a quote! - QuickBooks Integration and Software Development
   ke...@consolibyte.com
   1-860-341-1464
   http://www.ConsoliBYTE.com/
   Follow us on Twitter at: https://twitter.com/consolibyte
   AIM: consolibyte
   MSN: supp...@consolibyte.com
   Yahoo: consolib...@yahoo.com
   Gtalk: consolibyte
   Skype: consolibyte




On Feb 21, 2011, at 2:13 PM, Philip Martin wrote:

> "Keith Palmer Jr."  writes:
> 
>> So there's no one out there that knows whether SASL encrypts the data
>> stream, or just the authentication (or which SASL modules encrypt the
>> data, and which don't)?
> 
> That depends on how you have configured your repository via
> svnserve.conf:
> 
> [sasl]
> ### This option specifies whether you want to use the Cyrus SASL
> ### library for authentication. Default is false.
> ### This section will be ignored if svnserve is not built with Cyrus
> ### SASL support; to check, run 'svnserve --version' and look for a line
> ### reading 'Cyrus SASL authentication is available.'
> #use-sasl = true
> ### These options specify the desired strength of the security layer
> ### that you want SASL to provide. 0 means no encryption, 1 means
> ### integrity-checking only, values larger than 1 are correlated
> ### to the effective key length for encryption (e.g. 128 means 128-bit
> ### encryption). The values below are the defaults.
> # min-encryption = 0
> # max-encryption = 256
> 
> -- 
> Philip

Re: Is the svn:// protocol secure when encrypted via SASL?

[Philip Martin]

"Keith Palmer Jr."  writes:

> So there's no one out there that knows whether SASL encrypts the data
> stream, or just the authentication (or which SASL modules encrypt the
> data, and which don't)?

That depends on how you have configured your repository via
svnserve.conf:

[sasl]
### This option specifies whether you want to use the Cyrus SASL
### library for authentication. Default is false.
### This section will be ignored if svnserve is not built with Cyrus
### SASL support; to check, run 'svnserve --version' and look for a line
### reading 'Cyrus SASL authentication is available.'
#use-sasl = true
### These options specify the desired strength of the security layer
### that you want SASL to provide. 0 means no encryption, 1 means
### integrity-checking only, values larger than 1 are correlated
### to the effective key length for encryption (e.g. 128 means 128-bit
### encryption). The values below are the defaults.
# min-encryption = 0
# max-encryption = 256

-- 
Philip

RE: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Bert Huijben]

> -Original Message-
> From: Hyrum K Wright [mailto:hy...@hyrumwright.org]
> Sent: maandag 21 februari 2011 18:40
> To: Noorul Islam K M; Subversion
> Cc: Stefan Sperling
> Subject: Re: [PATCH] cat_tests.py: Modify expected error string to use
only
> error codes.
> 
> On Mon, Feb 21, 2011 at 4:19 AM, Stefan Sperling  wrote:
> > On Mon, Feb 21, 2011 at 12:46:25PM +0530, Noorul Islam K M wrote:
> >>
> >> Modify expected error string to use only error codes and also restrict
> >> one of the lines to less than 80 characters.
> >>
> >> Log
> >> [[[
> >>
> >> * subversion/tests/cmdline/cat_tests.py
> >>   (cat_local_directory, cat_remote_directory, cat_nonexistent_file,
> >>    cat_skip_uncattable, cat_unversioned_file,
> >>    cat_url_special_characters, cat_non_existing_remote_file):
> >>     Modify expected error string to use only error codes.
> >>
> >>   (cat_skip_uncattable): Restrict line length to < 80
> >>
> >> Patch by: Noorul Islam K M 
> >> ]]]
> >>
> >
> > Sorry that I'm being rejecting again :)
> >
> > But I think there is value in seeing the error message in the test code.
> > It makes it much easier for those reading the test code to follow
> > what kind of error the test is expecting. If all people can go by
> > is the error code, then they'll have to keep looking up error codes
> > to understand what the test is trying to do.
> > I think that's worse than adjusting tests when the message changes
> > every once in a while.
> 
> And I'll say what I've been saying for a while: the test suite
> supports providing the error code (as an integer) for the expected
> error.  That means that you can say 'svntest.err.CLIENT_INVALID_ARGS'
> (or whatever the error code is) instead of providing a regex with the
> actual code as a string.  I've yet to hear why this isn't an
> acceptable solution to this problem.
> 
> If you want to test for a specific error (which I think is of value),
> but don't care what the actual message is (since it may change, or be
> in a different locale, or whatever), just provide the error code,
> conveniently mapped in svntest.err.

I can think of a few reasons against this new method for the short term:
* In a lot of cases you want to check if a specific path is part of the
error (remains valid)
* You want to compare the test result with 1.6 (or earlier)

Until a few weeks ago we didn't print the error code so third party code
invoking svn is more likely to check the error message then this new error
output for the near future.

Over time we should start checking error codes, but until we release the
first version with the new output no user of svn will have any problem when
an error code changes.
(But users of our bindings might, but the recent changes changed the error
codes svn specific)

Bert

Re: Is the svn:// protocol secure when encrypted via SASL?

[Keith Palmer Jr.]

So there's no one out there that knows whether SASL encrypts the data stream, 
or just the authentication (or which SASL modules encrypt the data, and which 
don't)? 

---
 - Keith Palmer
   ConsoliBYTE, LLC
   Ask for a quote! - QuickBooks Integration and Software Development
   ke...@consolibyte.com
   1-860-341-1464
   http://www.ConsoliBYTE.com/
   Follow us on Twitter at: https://twitter.com/consolibyte
   AIM: consolibyte
   MSN: supp...@consolibyte.com
   Yahoo: consolib...@yahoo.com
   Gtalk: consolibyte
   Skype: consolibyte




On Feb 16, 2011, at 8:35 AM, Keith Palmer Jr. wrote:

> 
> If there's an easy way to decompress it, I certainly don't know it. Wireshark 
> just gives me a pile of packets- I don't know what algorithm was used to 
> compress the data, or even where the compressed data starts and ends. 
> 
> Is there any documentation anywhere as to which SASL modules encrypt the 
> data, vs. which don't? 
> 
> ---
> - Keith Palmer
>   ConsoliBYTE, LLC
>   Ask for a quote! - QuickBooks Integration and Software Development
>   ke...@consolibyte.com
>   1-860-341-1464
>   http://www.ConsoliBYTE.com/
>   Follow us on Twitter at: https://twitter.com/consolibyte
>   AIM: consolibyte
>   MSN: supp...@consolibyte.com
>   Yahoo: consolib...@yahoo.com
>   Gtalk: consolibyte
>   Skype: consolibyte
> 
> 
> 
> 
> On Feb 16, 2011, at 7:58 AM, Daniel Shahaf wrote:
> 
>> No, I didn't mean it to sound harsh.  I assumed that, having already
>> captured the data in wireshark, it would be straightforward to get
>> wireshark to decompress it or dump it to a file.
>> 
>> Thanks for jumping in and correcting me.
>> 
>> Daniel
>> 
>> Julian Foad wrote on Wed, Feb 16, 2011 at 12:58:22 +:
>>> On Wed, 2011-02-16, Daniel Shahaf wrote:
 Keith Palmer Jr. wrote on Tue, Feb 15, 2011 at 16:26:19 -0500:
> 
> I tried to capture the traffic with Wireshark, but it appears that
> everything is compressed over the wire anyway... so I can't tell if
> I'm looking at just compressed data, or compressed+encrypted data. 
 
 Decompress it then?
>>> 
>>> Daniel, I'm sure you didn't mean that to sound harsh but it does to me!
>>> Decompressing it might be quite difficult to arrange, practically, and
>>> it was quite reasonable for Keith to enquire here before going to that
>>> effort.
>>> 
>>> Thanks for your "depends on the SASL module" answer a few minutes later.
>>> 
>>> - Julian
>>> 
>>> 
> 

Re: svn commit: r1072431 - /subversion/trunk/subversion/include/private/svn_debug.h

[Daniel Shahaf]

Done and done: r1073102.

Thanks.


Hyrum K Wright wrote on Mon, Feb 21, 2011 at 09:53:37 -0600:
> Perhaps combine with the comment on line 40 of svn_debug.h?  (One or
> the other feels quite redundant.)
> 
> It might also be worth noting that lines produced this way won't
> interfere with test suite expectations.
> 
> -Hyrum
> 
> On Sat, Feb 19, 2011 at 2:05 PM,   wrote:
> > Author: danielsh
> > Date: Sat Feb 19 20:05:27 2011
> > New Revision: 1072431

Julian Foad wrote on Mon, Feb 21, 2011 at 17:34:10 +:
> > > + * 
> > > + *   kitchensink.c:42:path='/tmp/svn/wc1/iota' rev=3141592
> 
> Doesn't the output start with "DBG:" before the file name?
> 
> - Julian

Re: svn commit: r1072084 - /subversion/trunk/subversion/svn/util.c

[Daniel Shahaf]

Hyrum K Wright wrote on Mon, Feb 21, 2011 at 12:18:19 +:
> On Fri, Feb 18, 2011 at 5:53 PM,   wrote:
> > Author: danielsh
> > Date: Fri Feb 18 17:53:36 2011
> > New Revision: 1072084
> >
> > URL: http://svn.apache.org/viewvc?rev=1072084&view=rev
> > Log:
> > * subversion/svn/util.c
> >  (svn_cl__edit_string_externally):
> >    Preserve the edit log message file if the editor exited non-zero.
> >
> > Review by: cmpilato
> > Hindered by: irony
> > (I committed this using a pre-this-patch svn, but Vim apparently
> > exited non-zero (this happens sometimes) and the log message was lost...)
> 
> Will this also help the case where interactive conflict resolution
> sometimes barfs on a non-zero vim exit?  (/me really hopes so...)
> 

No; that uses svn_cl__edit_file_externally().

> -Hyrum
> 
> >
> > Modified:
> >    subversion/trunk/subversion/svn/util.c
> >
> > Modified: subversion/trunk/subversion/svn/util.c
> > URL: 
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/util.c?rev=1072084&r1=1072083&r2=1072084&view=diff
> > ==
> > --- subversion/trunk/subversion/svn/util.c (original)
> > +++ subversion/trunk/subversion/svn/util.c Fri Feb 18 17:53:36 2011
> > @@ -438,11 +438,21 @@ svn_cl__edit_string_externally(svn_strin
> >       goto cleanup;
> >     }
> >
> > -  /* Now, run the editor command line.  */
> > +  /* Prepare the editor command line.  */
> >   err = svn_utf_cstring_from_utf8(&tmpfile_native, tmpfile_name, pool);
> >   if (err)
> >     goto cleanup;
> >   cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
> > +
> > +  /* If the caller wants us to leave the file around, return the path
> > +     of the file we'll use, and make a note not to destroy it.  */
> > +  if (tmpfile_left)
> > +    {
> > +      *tmpfile_left = svn_dirent_join(base_dir, tmpfile_name, pool);
> > +      remove_file = FALSE;
> > +    }
> > +
> > +  /* Now, run the editor command line.  */
> >   sys_err = system(cmd);
> >   if (sys_err != 0)
> >     {
> > @@ -462,14 +472,6 @@ svn_cl__edit_string_externally(svn_strin
> >       goto cleanup;
> >     }
> >
> > -  /* If the caller wants us to leave the file around, return the path
> > -     of the file we used, and make a note not to destroy it.  */
> > -  if (tmpfile_left)
> > -    {
> > -      *tmpfile_left = svn_dirent_join(base_dir, tmpfile_name, pool);
> > -      remove_file = FALSE;
> > -    }
> > -
> >   /* If the file looks changed... */
> >   if ((finfo_before.mtime != finfo_after.mtime) ||
> >       (finfo_before.size != finfo_after.size))
> >
> >
> >

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Hyrum K Wright]

On Mon, Feb 21, 2011 at 4:19 AM, Stefan Sperling  wrote:
> On Mon, Feb 21, 2011 at 12:46:25PM +0530, Noorul Islam K M wrote:
>>
>> Modify expected error string to use only error codes and also restrict
>> one of the lines to less than 80 characters.
>>
>> Log
>> [[[
>>
>> * subversion/tests/cmdline/cat_tests.py
>>   (cat_local_directory, cat_remote_directory, cat_nonexistent_file,
>>    cat_skip_uncattable, cat_unversioned_file,
>>    cat_url_special_characters, cat_non_existing_remote_file):
>>     Modify expected error string to use only error codes.
>>
>>   (cat_skip_uncattable): Restrict line length to < 80
>>
>> Patch by: Noorul Islam K M 
>> ]]]
>>
>
> Sorry that I'm being rejecting again :)
>
> But I think there is value in seeing the error message in the test code.
> It makes it much easier for those reading the test code to follow
> what kind of error the test is expecting. If all people can go by
> is the error code, then they'll have to keep looking up error codes
> to understand what the test is trying to do.
> I think that's worse than adjusting tests when the message changes
> every once in a while.

And I'll say what I've been saying for a while: the test suite
supports providing the error code (as an integer) for the expected
error.  That means that you can say 'svntest.err.CLIENT_INVALID_ARGS'
(or whatever the error code is) instead of providing a regex with the
actual code as a string.  I've yet to hear why this isn't an
acceptable solution to this problem.

If you want to test for a specific error (which I think is of value),
but don't care what the actual message is (since it may change, or be
in a different locale, or whatever), just provide the error code,
conveniently mapped in svntest.err.

-Hyrum

Re: svn commit: r1072431 - /subversion/trunk/subversion/include/private/svn_debug.h

[Julian Foad]

> > * subversion/include/private/svn_debug.h
> >  (SVN_DBG):  Add a docstring.
[...]
> > +/** Debug aid macro that prints the file:line of the call and printf-like
> > + * arguments to the #SVN_DBG_OUTPUT stdio stream.  Typical usage:
> > + *
> > + * 
> > + *   SVN_DBG(("path='%s' rev=%ld\n", dirent, revnum));
> > + * 
> > + *
> > + * outputs:
> > + *
> > + * 
> > + *   kitchensink.c:42:path='/tmp/svn/wc1/iota' rev=3141592

Doesn't the output start with "DBG:" before the file name?

- Julian


> > + * 
> > + */
> >  #define SVN_DBG(ARGS) (svn_dbg__preamble(__FILE__, __LINE__, 
> > SVN_DBG_OUTPUT), \
> >svn_dbg__printf ARGS)

Re: svn commit: r1072431 - /subversion/trunk/subversion/include/private/svn_debug.h

[Hyrum K Wright]

Perhaps combine with the comment on line 40 of svn_debug.h?  (One or
the other feels quite redundant.)

It might also be worth noting that lines produced this way won't
interfere with test suite expectations.

-Hyrum

On Sat, Feb 19, 2011 at 2:05 PM,   wrote:
> Author: danielsh
> Date: Sat Feb 19 20:05:27 2011
> New Revision: 1072431
>
> URL: http://svn.apache.org/viewvc?rev=1072431&view=rev
> Log:
> * subversion/include/private/svn_debug.h
>  (SVN_DBG):  Add a docstring.
>
> Modified:
>    subversion/trunk/subversion/include/private/svn_debug.h
>
> Modified: subversion/trunk/subversion/include/private/svn_debug.h
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_debug.h?rev=1072431&r1=1072430&r2=1072431&view=diff
> ==
> --- subversion/trunk/subversion/include/private/svn_debug.h (original)
> +++ subversion/trunk/subversion/include/private/svn_debug.h Sat Feb 19 
> 20:05:27 2011
> @@ -74,6 +74,19 @@ svn_dbg__printf(const char *fmt, ...)
>
>  #else
>
> +/** Debug aid macro that prints the file:line of the call and printf-like
> + * arguments to the #SVN_DBG_OUTPUT stdio stream.  Typical usage:
> + *
> + * 
> + *   SVN_DBG(("path='%s' rev=%ld\n", dirent, revnum));
> + * 
> + *
> + * outputs:
> + *
> + * 
> + *   kitchensink.c:42:path='/tmp/svn/wc1/iota' rev=3141592
> + * 
> + */
>  #define SVN_DBG(ARGS) (svn_dbg__preamble(__FILE__, __LINE__, 
> SVN_DBG_OUTPUT), \
>                        svn_dbg__printf ARGS)
>
>
>
>

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Noorul Islam K M]

"Bert Huijben"  writes:

>> -Original Message-
>> From: Branko Čibej [mailto:br...@xbc.nu] On Behalf Of Branko Cibej
>> Sent: maandag 21 februari 2011 13:19
>> To: Philip Martin
>> Cc: dev@subversion.apache.org
>> Subject: Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple
>> checkouts.
>
>> You could argue that tiny tests that do not modify a sandbox can be
>> merged together. Of course, if there's a bug in the implementation and
>> one of those subtests /does/ modify the sandbox, causing another down
>> the line to fail for no obvious reason, that hardly makes life easier
>> for the poor sod who has to analyse that mess.
>
> Wasn't there some option to ask for a read-only sandbox?
>

On trunk the test already use read-only option. This patch reduces
number of checkouts. All these tests require only read-only working
copy.

Thanks and Regards
Noorul

Re: Inconsistent indentation in python code across code base

[Hyrum K Wright]

On Mon, Feb 21, 2011 at 9:56 AM, Noorul Islam K M  wrote:
>
> I think we have in consistent python indentation. I can see that we use
> 2 space in test/ and 4 in bindings/ctypes-python. Is it not a good idea
> to stick to common indentation?  I can fix it.  Fixing ctypes-python
> will be easier because it has less number of lines.
>
> Any thoughts? Or is it already known to everyone and decided to live
> with it.

Just live with it.

We have a few similar quirks in our C codebase from code that is from
the days before we standardized our coding style.  Hasn't killed us
yet, though spaces-before-parens almost did. ;)

-Hyrum

RE: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Bert Huijben]

> -Original Message-
> From: Branko Čibej [mailto:br...@xbc.nu] On Behalf Of Branko Cibej
> Sent: maandag 21 februari 2011 13:19
> To: Philip Martin
> Cc: dev@subversion.apache.org
> Subject: Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple
> checkouts.

> You could argue that tiny tests that do not modify a sandbox can be
> merged together. Of course, if there's a bug in the implementation and
> one of those subtests /does/ modify the sandbox, causing another down
> the line to fail for no obvious reason, that hardly makes life easier
> for the poor sod who has to analyse that mess.

Wasn't there some option to ask for a read-only sandbox?

Bert

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 12:19:34PM +, Philip Martin wrote:
> Stefan Sperling  writes:
> 
> > Fair enough, but why not write a specific test for this instead of testing
> > it as a side effect of making the code of other tests harder to read?
> 
> The assumption is that translation only affects output to the user, not
> the internal behaviour of the commands.  We have some translation tests
> based on that.  To get full coverage of all the internal behaviour
> really requires that the tests can be run in a different locale.

I'd say it's an experiment with questionable gain for the price of
a lot of churn in existing tests. It might be difficult to come up
with an automated approach to substituting error messages with the
corresponding numbers. If this is done manually it will likely become
a time sink for the person who does it.

If, in spite of this, running the entire test suite in a different locale
is attempted, I'd prefer if the attempt was made on a branch instead of
trunk so that we can independently compare both approaches and come
to a decision based on these observations.

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Philip Martin]

Stefan Sperling  writes:

> Fair enough, but why not write a specific test for this instead of testing
> it as a side effect of making the code of other tests harder to read?

The assumption is that translation only affects output to the user, not
the internal behaviour of the commands.  We have some translation tests
based on that.  To get full coverage of all the internal behaviour
really requires that the tests can be run in a different locale.

-- 
Philip

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Branko Čibej]

On 21.02.2011 13:12, Philip Martin wrote:
> Branko Čibej  writes:
>
>> We should not be optimising tests for performance over clarity, ever. In
>> other words -- don't combine them. Anyone who has trouble with tests
>> taking too long can use a RAMdisk, --bdb-txn-nosync, and other tweaks to
>> make the tests run faster.
> I do that.  The time to run the testsuite keeps growing.
>
>> I think you've all gone way off track here. Reality check, please? What
>> is the purpose of tests? To validate the behaviour of the code, or to
>> make developers' life easier?
> To make it easy for developers to validate the code :)
>
> Like all code it requires a judgement about how it should be organised.
> I agree that complicated tests should be separate, but I think checking
> that:
>
> "svn revert foo"
>
>  and
>
> "svn resoved foo"
>
> both report "foo is not a local path" doesn't require two completely
> separate tests.
>
> How far do you think we should go?  A dozen separate tests for revert
> alone:
>
> "svn revert versioned_and_modified"
> "svn revert versioned_and_unmodified"
> "svn revert url"
> "svn revert unversioned"
> "svn revert versioned_and_modified versioned_and_unmodified"
> "svn revert versioned_and_unmodified unversioned"
> etc

It's certainly easier to read the test output that way. :)

You could argue that tiny tests that do not modify a sandbox can be
merged together. Of course, if there's a bug in the implementation and
one of those subtests /does/ modify the sandbox, causing another down
the line to fail for no obvious reason, that hardly makes life easier
for the poor sod who has to analyse that mess.

There's a balancing act here. Talk about a "global sandbox" reused by
several test cases definitely makes my hair stand on end and I do
recommend against that. Combining several small tests into a single
testcase is a different and slightly more palatable approach, as long as
everyone agrees to be the poor sod that has to analyse the fallout. :)

-- Brane

Re: svn commit: r1072084 - /subversion/trunk/subversion/svn/util.c

[Hyrum K Wright]

On Fri, Feb 18, 2011 at 5:53 PM,   wrote:
> Author: danielsh
> Date: Fri Feb 18 17:53:36 2011
> New Revision: 1072084
>
> URL: http://svn.apache.org/viewvc?rev=1072084&view=rev
> Log:
> * subversion/svn/util.c
>  (svn_cl__edit_string_externally):
>    Preserve the edit log message file if the editor exited non-zero.
>
> Review by: cmpilato
> Hindered by: irony
> (I committed this using a pre-this-patch svn, but Vim apparently
> exited non-zero (this happens sometimes) and the log message was lost...)

Will this also help the case where interactive conflict resolution
sometimes barfs on a non-zero vim exit?  (/me really hopes so...)

-Hyrum

>
> Modified:
>    subversion/trunk/subversion/svn/util.c
>
> Modified: subversion/trunk/subversion/svn/util.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/util.c?rev=1072084&r1=1072083&r2=1072084&view=diff
> ==
> --- subversion/trunk/subversion/svn/util.c (original)
> +++ subversion/trunk/subversion/svn/util.c Fri Feb 18 17:53:36 2011
> @@ -438,11 +438,21 @@ svn_cl__edit_string_externally(svn_strin
>       goto cleanup;
>     }
>
> -  /* Now, run the editor command line.  */
> +  /* Prepare the editor command line.  */
>   err = svn_utf_cstring_from_utf8(&tmpfile_native, tmpfile_name, pool);
>   if (err)
>     goto cleanup;
>   cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
> +
> +  /* If the caller wants us to leave the file around, return the path
> +     of the file we'll use, and make a note not to destroy it.  */
> +  if (tmpfile_left)
> +    {
> +      *tmpfile_left = svn_dirent_join(base_dir, tmpfile_name, pool);
> +      remove_file = FALSE;
> +    }
> +
> +  /* Now, run the editor command line.  */
>   sys_err = system(cmd);
>   if (sys_err != 0)
>     {
> @@ -462,14 +472,6 @@ svn_cl__edit_string_externally(svn_strin
>       goto cleanup;
>     }
>
> -  /* If the caller wants us to leave the file around, return the path
> -     of the file we used, and make a note not to destroy it.  */
> -  if (tmpfile_left)
> -    {
> -      *tmpfile_left = svn_dirent_join(base_dir, tmpfile_name, pool);
> -      remove_file = FALSE;
> -    }
> -
>   /* If the file looks changed... */
>   if ((finfo_before.mtime != finfo_after.mtime) ||
>       (finfo_before.size != finfo_after.size))
>
>
>

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Philip Martin]

Branko Čibej  writes:

> We should not be optimising tests for performance over clarity, ever. In
> other words -- don't combine them. Anyone who has trouble with tests
> taking too long can use a RAMdisk, --bdb-txn-nosync, and other tweaks to
> make the tests run faster.

I do that.  The time to run the testsuite keeps growing.

> I think you've all gone way off track here. Reality check, please? What
> is the purpose of tests? To validate the behaviour of the code, or to
> make developers' life easier?

To make it easy for developers to validate the code :)

Like all code it requires a judgement about how it should be organised.
I agree that complicated tests should be separate, but I think checking
that:

"svn revert foo"

 and

"svn resoved foo"

both report "foo is not a local path" doesn't require two completely
separate tests.

How far do you think we should go?  A dozen separate tests for revert
alone:

"svn revert versioned_and_modified"
"svn revert versioned_and_unmodified"
"svn revert url"
"svn revert unversioned"
"svn revert versioned_and_modified versioned_and_unmodified"
"svn revert versioned_and_unmodified unversioned"
etc

-- 
Philip

Re: Unable to compile since r1072302

[Hyrum K Wright]

2011/2/20 Branko Čibej :
> On 20.02.2011 22:05, Daniel Becroft wrote:
>> ...
>> /subversion/libsvn_fs_fs/.libs/libsvn_fs_fs-1.so.0: undefined
>> reference to `svn_temp_serializer__add_string'
>> collect2: ld returned 1 exit status
>>
>> I've narrowed it down to r1072302, and if I update back to -1, everything
>> builds correctly. I've re-run 'make clean', and ./configure, but still no
>> luck. It seems that the libsvn_subr/svn_temp_serializer.c is not getting
>> compiled at all during 'make'.
>>
>> I assume no-one else is having this problem (or no-one's noticed it yet),
>> but is there anything else I need to do (do I need to go right back and
>> re-run ./autogen.sh?)
>
> if the commit added files, you have to re-run autogen.

Actually, the full autogen isn't required, just the gen-make.py step
(which can be done independently).

-Hyrum

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 11:57:50AM +, Philip Martin wrote:
> Stefan Sperling  writes:
> 
> > On Mon, Feb 21, 2011 at 11:21:10AM +, Philip Martin wrote:
> >> Stefan Sperling  writes:
> >> 
> >> > But I think there is value in seeing the error message in the test code.
> >> > It makes it much easier for those reading the test code to follow
> >> > what kind of error the test is expecting. If all people can go by
> >> > is the error code, then they'll have to keep looking up error codes
> >> > to understand what the test is trying to do.
> >> > I think that's worse than adjusting tests when the message changes
> >> > every once in a while.
> >> 
> >> For those reading the test code a comment could be used, although I
> >> admit that a comment is less reliable.  On the other hand, if we just
> >> check error numbers that might make it possible to run the regression
> >> tests in some locale other than C, and some language other than English.
> >
> > Sure but is that something we really need? After all this would mean
> > testing gettext, which we don't maintain.
> 
> We have got this far without it, but it would be interesting to try it.
> The regression tests depend on lots of things we don't maintain BDB,
> libxml, etc.

Fair enough, but why not write a specific test for this instead of testing
it as a side effect of making the code of other tests harder to read?

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Philip Martin]

Stefan Sperling  writes:

> On Mon, Feb 21, 2011 at 11:21:10AM +, Philip Martin wrote:
>> Stefan Sperling  writes:
>> 
>> > But I think there is value in seeing the error message in the test code.
>> > It makes it much easier for those reading the test code to follow
>> > what kind of error the test is expecting. If all people can go by
>> > is the error code, then they'll have to keep looking up error codes
>> > to understand what the test is trying to do.
>> > I think that's worse than adjusting tests when the message changes
>> > every once in a while.
>> 
>> For those reading the test code a comment could be used, although I
>> admit that a comment is less reliable.  On the other hand, if we just
>> check error numbers that might make it possible to run the regression
>> tests in some locale other than C, and some language other than English.
>
> Sure but is that something we really need? After all this would mean
> testing gettext, which we don't maintain.

We have got this far without it, but it would be interesting to try it.
The regression tests depend on lots of things we don't maintain BDB,
libxml, etc.

-- 
Philip

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Branko Čibej]

On 21.02.2011 12:04, Philip Martin wrote:
> Stefan Sperling  writes:
>
>> On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
>>> This patch reduces checkout by around 23 times.
>> On my system the difference is 43 seconds vs. 30 seconds.
> On my low-end Linux desktop it's 7.5 seconds and 3.5 seconds, run
> sequentially on a SATA disk.
>
>> We lose the ability to easily spot which of the subtest is failing
>> if we do this. I.e. instead of:
>>
>> ...
>> PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
>> FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
>> PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
>> ...
>>
>> all we'd get is:
>>
>> FAIL:  input_validation_tests.py 1: inavlid wc and url targets
> When a test fails the first thing I do is look in tests.log, that will
> still work just as well with the combined test.  I suppose we do lose
> out if there are multiple independent bugs, as the test will stop at the
> first one and not report the others.
>
> I feel we should be optimising for the common case where the tests PASS,
> and that optimising for FAILs is the wrong approach.
>
> I agree that combining big, complex tests, like merge, is a bad idea.
> But for relatively trivial tests I think reduced runtime is more
> important.

We should not be optimising tests for performance over clarity, ever. In
other words -- don't combine them. Anyone who has trouble with tests
taking too long can use a RAMdisk, --bdb-txn-nosync, and other tweaks to
make the tests run faster.

I think you've all gone way off track here. Reality check, please? What
is the purpose of tests? To validate the behaviour of the code, or to
make developers' life easier?

-- Brane

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Branko Čibej]

On 21.02.2011 11:32, Noorul Islam K M wrote:
> Noorul Islam K M  writes:
>
>> Stefan Sperling  writes:
>>
>>> On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
>>>
 This patch reduces checkout by around 23 times.
>>> On my system the difference is 43 seconds vs. 30 seconds.
>>>
>>> We lose the ability to easily spot which of the subtest is failing
>>> if we do this. I.e. instead of:
>>>
>>> ...
>>> PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
>>> FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
>>> PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
>>> ...
>>>
>>> all we'd get is:
>>>
>>> FAIL:  input_validation_tests.py 1: inavlid wc and url targets
>>>
>>> Is there a way of keeping these as individual tests but also
>>> avoiding the overhead of creating a repository and a working copy?
>>> If there isn't I would prefer to just leave this as it is now because
>>> I prefer the current output.
>> I think it will be possible by keeping sandbox global. I will modify and
>> send an updated patch.
>>
> I looked into it. I don't think it is straight forward. I will leave it
> as such. As you said 13 seconds gain is no big deal.

Given that the one commandment for writing test cases is to make them
independent of each other, I can hardly see how you could keep a global
sandbox. The most important "optimization" for tests is to make them
more reliable, testcase performance comes third on the list at best
(after comprehensive).

-- Brane

Re: Inconsistent indentation in python code across code base

[Branko Čibej]

On 21.02.2011 11:20, Philip Martin wrote:
> Noorul Islam K M  writes:
>
>> I think we have in consistent python indentation. I can see that we use
>> 2 space in test/ and 4 in bindings/ctypes-python. Is it not a good idea
>> to stick to common indentation?  I can fix it.  Fixing ctypes-python
>> will be easier because it has less number of lines.
>>
>> Any thoughts? Or is it already known to everyone and decided to live
>> with it.
> I've never looked at the ctypes-python code, it is essentially a
> separate code base from the test code.  Do we ever cut-and-paste code
> from one to the other?  Do developers spend a lot of time switching
> between them?  If it really is separate then it's reasonable for it to
> have its own style, and AFAIK 4 spaces is the more common style in the
> Python world.

It is -- but our hacking guide (used to?) require 2-space indent for
Python, which is why the makefile generators and the test suite use that.

-- Brane

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 11:21:10AM +, Philip Martin wrote:
> Stefan Sperling  writes:
> 
> > But I think there is value in seeing the error message in the test code.
> > It makes it much easier for those reading the test code to follow
> > what kind of error the test is expecting. If all people can go by
> > is the error code, then they'll have to keep looking up error codes
> > to understand what the test is trying to do.
> > I think that's worse than adjusting tests when the message changes
> > every once in a while.
> 
> For those reading the test code a comment could be used, although I
> admit that a comment is less reliable.  On the other hand, if we just
> check error numbers that might make it possible to run the regression
> tests in some locale other than C, and some language other than English.

Sure but is that something we really need? After all this would mean
testing gettext, which we don't maintain.

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Philip Martin]

Stefan Sperling  writes:

> But I think there is value in seeing the error message in the test code.
> It makes it much easier for those reading the test code to follow
> what kind of error the test is expecting. If all people can go by
> is the error code, then they'll have to keep looking up error codes
> to understand what the test is trying to do.
> I think that's worse than adjusting tests when the message changes
> every once in a while.

For those reading the test code a comment could be used, although I
admit that a comment is less reliable.  On the other hand, if we just
check error numbers that might make it possible to run the regression
tests in some locale other than C, and some language other than English.


-- 
Philip

Re: Pristine store - spec

[Julian Foad]

On Fri, 2011-02-18, Daniel Shahaf wrote:
[...]
> In other words, the lower-layer API is oblivious of WC locks.  I see.
> 
> That said, I just noticed that your bullet points have both a "wc lock"
> and an "exclusive wc lock" --- I had overlooked that distinction
> earlier.  I assume that an 'exclusive' lock is a recursive lock from
> the wc root and below?

Yes, that's right.

> Thanks,
> 
> Daniel
> (I'm behind on following the commits@ list --- if there are relevant
> commits/milestones of the doc, feel free to direct my attention to them
> explicitly)

I committed updates in r1071729 and r1071795 so far.

- Julian

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Philip Martin]

Stefan Sperling  writes:

> On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
>> 
>> This patch reduces checkout by around 23 times.
>
> On my system the difference is 43 seconds vs. 30 seconds.

On my low-end Linux desktop it's 7.5 seconds and 3.5 seconds, run
sequentially on a SATA disk.

> We lose the ability to easily spot which of the subtest is failing
> if we do this. I.e. instead of:
>
> ...
> PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
> FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
> PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
> ...
>
> all we'd get is:
>
> FAIL:  input_validation_tests.py 1: inavlid wc and url targets

When a test fails the first thing I do is look in tests.log, that will
still work just as well with the combined test.  I suppose we do lose
out if there are multiple independent bugs, as the test will stop at the
first one and not report the others.

I feel we should be optimising for the common case where the tests PASS,
and that optimising for FAILs is the wrong approach.

I agree that combining big, complex tests, like merge, is a bad idea.
But for relatively trivial tests I think reduced runtime is more
important.



-- 
Philip

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Noorul Islam K M]

Stefan Sperling  writes:

> On Mon, Feb 21, 2011 at 03:58:00PM +0530, Noorul Islam K M wrote:
>
>> Stefan Sperling  writes:
>> > But I think there is value in seeing the error message in the test code.
>> > It makes it much easier for those reading the test code to follow
>> > what kind of error the test is expecting. If all people can go by
>> > is the error code, then they'll have to keep looking up error codes
>> > to understand what the test is trying to do.
>> > I think that's worse than adjusting tests when the message changes
>> > every once in a while.
>> 
>> Actually earlier there was request like this. So I thought it is ok to
>> have it here also. Please see r1071961.
>
> That has expected errors that only list error codes, yes.
> I would prefer a regex that contains at least part of the error message
> to make it more obvious what the error is about.

Then I think that revision can be reverted. Because that also contains
combining multiple tests which also you rejected.

Thanks and Regards
Noorul

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 03:58:00PM +0530, Noorul Islam K M wrote:
> Stefan Sperling  writes:
> > But I think there is value in seeing the error message in the test code.
> > It makes it much easier for those reading the test code to follow
> > what kind of error the test is expecting. If all people can go by
> > is the error code, then they'll have to keep looking up error codes
> > to understand what the test is trying to do.
> > I think that's worse than adjusting tests when the message changes
> > every once in a while.
> 
> Actually earlier there was request like this. So I thought it is ok to
> have it here also. Please see r1071961.

That has expected errors that only list error codes, yes.
I would prefer a regex that contains at least part of the error message
to make it more obvious what the error is about.

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Noorul Islam K M]

Noorul Islam K M  writes:

> Stefan Sperling  writes:
>
>> On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
>>
>>> 
>>> This patch reduces checkout by around 23 times.
>>
>> On my system the difference is 43 seconds vs. 30 seconds.
>>
>> We lose the ability to easily spot which of the subtest is failing
>> if we do this. I.e. instead of:
>>
>> ...
>> PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
>> FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
>> PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
>> ...
>>
>> all we'd get is:
>>
>> FAIL:  input_validation_tests.py 1: inavlid wc and url targets
>>
>> Is there a way of keeping these as individual tests but also
>> avoiding the overhead of creating a repository and a working copy?
>> If there isn't I would prefer to just leave this as it is now because
>> I prefer the current output.
>
> I think it will be possible by keeping sandbox global. I will modify and
> send an updated patch.
>

I looked into it. I don't think it is straight forward. I will leave it
as such. As you said 13 seconds gain is no big deal.

Thanks and Regards
Noorul

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Noorul Islam K M]

Stefan Sperling  writes:

> On Mon, Feb 21, 2011 at 12:46:25PM +0530, Noorul Islam K M wrote:
>
>> 
>> Modify expected error string to use only error codes and also restrict
>> one of the lines to less than 80 characters.
>> 
>> Log
>> [[[
>> 
>> * subversion/tests/cmdline/cat_tests.py
>>   (cat_local_directory, cat_remote_directory, cat_nonexistent_file,
>>cat_skip_uncattable, cat_unversioned_file,
>>cat_url_special_characters, cat_non_existing_remote_file):
>> Modify expected error string to use only error codes.
>> 
>>   (cat_skip_uncattable): Restrict line length to < 80
>> 
>> Patch by: Noorul Islam K M 
>> ]]]
>> 
>
> Sorry that I'm being rejecting again :)
>

No issues. 

> But I think there is value in seeing the error message in the test code.
> It makes it much easier for those reading the test code to follow
> what kind of error the test is expecting. If all people can go by
> is the error code, then they'll have to keep looking up error codes
> to understand what the test is trying to do.
> I think that's worse than adjusting tests when the message changes
> every once in a while.

Actually earlier there was request like this. So I thought it is ok to
have it here also. Please see r1071961.

Thanks and Regards
Noorul

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Noorul Islam K M]

Stefan Sperling  writes:

> On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
>
>> 
>> This patch reduces checkout by around 23 times.
>
> On my system the difference is 43 seconds vs. 30 seconds.
>
> We lose the ability to easily spot which of the subtest is failing
> if we do this. I.e. instead of:
>
> ...
> PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
> FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
> PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
> ...
>
> all we'd get is:
>
> FAIL:  input_validation_tests.py 1: inavlid wc and url targets
>
> Is there a way of keeping these as individual tests but also
> avoiding the overhead of creating a repository and a working copy?
> If there isn't I would prefer to just leave this as it is now because
> I prefer the current output.

I think it will be possible by keeping sandbox global. I will modify and
send an updated patch.

Thanks and Regards
Noorul

Re: Inconsistent indentation in python code across code base

[Philip Martin]

Noorul Islam K M  writes:

> I think we have in consistent python indentation. I can see that we use
> 2 space in test/ and 4 in bindings/ctypes-python. Is it not a good idea
> to stick to common indentation?  I can fix it.  Fixing ctypes-python
> will be easier because it has less number of lines.
>
> Any thoughts? Or is it already known to everyone and decided to live
> with it.

I've never looked at the ctypes-python code, it is essentially a
separate code base from the test code.  Do we ever cut-and-paste code
from one to the other?  Do developers spend a lot of time switching
between them?  If it really is separate then it's reasonable for it to
have its own style, and AFAIK 4 spaces is the more common style in the
Python world.

-- 
Philip

Re: [PATCH] cat_tests.py: Modify expected error string to use only error codes.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 12:46:25PM +0530, Noorul Islam K M wrote:
> 
> Modify expected error string to use only error codes and also restrict
> one of the lines to less than 80 characters.
> 
> Log
> [[[
> 
> * subversion/tests/cmdline/cat_tests.py
>   (cat_local_directory, cat_remote_directory, cat_nonexistent_file,
>cat_skip_uncattable, cat_unversioned_file,
>cat_url_special_characters, cat_non_existing_remote_file):
> Modify expected error string to use only error codes.
> 
>   (cat_skip_uncattable): Restrict line length to < 80
> 
> Patch by: Noorul Islam K M 
> ]]]
> 

Sorry that I'm being rejecting again :)

But I think there is value in seeing the error message in the test code.
It makes it much easier for those reading the test code to follow
what kind of error the test is expecting. If all people can go by
is the error code, then they'll have to keep looking up error codes
to understand what the test is trying to do.
I think that's worse than adjusting tests when the message changes
every once in a while.

Re: [PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Stefan Sperling]

On Mon, Feb 21, 2011 at 01:44:35PM +0530, Noorul Islam K M wrote:
> 
> This patch reduces checkout by around 23 times.

On my system the difference is 43 seconds vs. 30 seconds.

We lose the ability to easily spot which of the subtest is failing
if we do this. I.e. instead of:

...
PASS:  input_validation_tests.py 19: non-working copy paths for 'status'
FAIL:  input_validation_tests.py 20: non-working copy paths for 'patch'
PASS:  input_validation_tests.py 21: non-working copy paths for 'switch'
...

all we'd get is:

FAIL:  input_validation_tests.py 1: inavlid wc and url targets

Is there a way of keeping these as individual tests but also
avoiding the overhead of creating a repository and a working copy?
If there isn't I would prefer to just leave this as it is now because
I prefer the current output.

Re: Inconsistent indentation in python code across code base

[Stefan Sperling]

On Mon, Feb 21, 2011 at 03:26:26PM +0530, Noorul Islam K M wrote:
> 
> I think we have in consistent python indentation. I can see that we use
> 2 space in test/ and 4 in bindings/ctypes-python. Is it not a good idea
> to stick to common indentation?  I can fix it.  Fixing ctypes-python
> will be easier because it has less number of lines.
> 
> Any thoughts? Or is it already known to everyone and decided to live
> with it.

I would say we just live with it.
As long as the indentation style within one subsystem is consistent
there's no problem.

Inconsistent indentation in python code across code base

[Noorul Islam K M]

I think we have in consistent python indentation. I can see that we use
2 space in test/ and 4 in bindings/ctypes-python. Is it not a good idea
to stick to common indentation?  I can fix it.  Fixing ctypes-python
will be easier because it has less number of lines.

Any thoughts? Or is it already known to everyone and decided to live
with it.

Thanks and Regards
Noorul

[PATCH] Fix ctypes-python tests by clearing pool

[Noorul Islam K M]

This patch fixes some minor issues with ctypes-python. Now we are
explicitly clearing the pool.

Log
[[[

Fix ctypes-python tests by explicitly clearing pool.

* subversion/bindings/ctypes-python/csvn/wc.py,
  subversion/bindings/ctypes-python/csvn/repos.py
  (WC.clear_pool, LocalRepository.clear_pool, 
   RemoteRepository.clear_pool): New method to clear pool.

* subversion/bindings/ctypes-python/test/cw.py,
  subversion/bindings/ctypes-python/test/localrepos.py,
  subversion/bindings/ctypes-python/test/remoterepos.py
  (WCTestCase.setUp, LocalRepositoryTestCase.setUp, 
   RemoteRepositoryTestCase.setUp): Initialize instance variable.

  (WCTestCase.tearDown, LocalRepositoryTestCase.tearDown, 
   RemoteRepositoryTestCase.tearDown): Clear pool.

Patch by: Noorul Islam K M 
]]]

Index: subversion/bindings/ctypes-python/test/wc.py
===
--- subversion/bindings/ctypes-python/test/wc.py(revision 1072854)
+++ subversion/bindings/ctypes-python/test/wc.py(working copy)
@@ -56,6 +56,8 @@
 """Test case for Subversion WC layer."""
 
 def setUp(self):
+self.repos = None
+self.wc = None
 dumpfile = open(os.path.join(os.path.split(__file__)[0],
 'test.dumpfile'))
 
@@ -63,7 +65,7 @@
 self.tearDown()
 self.repos = LocalRepository(repos_location, create=True)
 self.repos.load(dumpfile)
-
+
 self.wc = WC(wc_location)
 self.wc.checkout(repo_url)
 
@@ -73,6 +75,10 @@
 svn_io_remove_dir(wc_location, pool)
 if os.path.exists(repos_location):
 svn_repos_delete(repos_location, pool)
+if self.repos:
+self.repos.clear_pool()
+if self.wc:
+self.wc.clear_pool()
 self.wc = None
 
 def _info_receiver(self, path, info):
Index: subversion/bindings/ctypes-python/test/localrepos.py
===
--- subversion/bindings/ctypes-python/test/localrepos.py(revision 
1072854)
+++ subversion/bindings/ctypes-python/test/localrepos.py(working copy)
@@ -30,6 +30,7 @@
 class LocalRepositoryTestCase(unittest.TestCase):
 
 def setUp(self):
+self.repos = None
 dumpfile = open(os.path.join(os.path.split(__file__)[0],
 'test.dumpfile'))
 
@@ -41,6 +42,8 @@
 def tearDown(self):
 if os.path.exists(repos_location):
 svn_repos_delete(repos_location, Pool())
+if self.repos:
+self.repos.clear_pool()
 self.repos = None
 
 def test_local_latest_revnum(self):
Index: subversion/bindings/ctypes-python/test/remoterepos.py
===
--- subversion/bindings/ctypes-python/test/remoterepos.py   (revision 
1072854)
+++ subversion/bindings/ctypes-python/test/remoterepos.py   (working copy)
@@ -43,43 +43,51 @@
 class RemoteRepositoryTestCase(unittest.TestCase):
 
 def setUp(self):
+self.local_repo = None
+self.remote_repo = None
+  
 dumpfile = open(os.path.join(os.path.split(__file__)[0],
-'test.dumpfile'))
+ 'test.dumpfile'))
 
 # Just in case a preivous test instance was not properly cleaned up
 self.tearDown()
-self.repos = LocalRepository(repos_location, create=True)
-self.repos.load(dumpfile)
+self.local_repo = LocalRepository(repos_location, create=True)
+self.local_repo.load(dumpfile)
 
-self.repos = RemoteRepository(repos_url)
+self.remote_repo = RemoteRepository(repos_url)
 
 def tearDown(self):
 if os.path.exists(repos_location):
 svn_repos_delete(repos_location, Pool())
-self.repos = None
+if self.remote_repo:
+self.remote_repo.clear_pool()
+if self.local_repo:
+self.local_repo.clear_pool()
+self.remote_repo = None
+self.local_repo = None
 
 def test_remote_latest_revnum(self):
-self.assertEqual(9, self.repos.latest_revnum())
+self.assertEqual(9, self.remote_repo.latest_revnum())
 
 def test_remote_check_path(self):
 self.assertEqual(svn_node_file,
-self.repos.check_path("trunk/README.txt"))
+self.remote_repo.check_path("trunk/README.txt"))
 self.assertEqual(svn_node_dir,
-self.repos.check_path("trunk/dir", 6))
+self.remote_repo.check_path("trunk/dir", 6))
 self.assertEqual(svn_node_none,
-self.repos.check_path("trunk/dir", 7))
+self.remote_repo.check_path("trunk/dir", 7))
 self.assertEqual(svn_node_none,
-self.repos.check_path("does_not_compute"))
+self.remote_repo.check_path("does_not_compute"))
 
 def test_revprop_list(self):
 # Test argument-free case
-pr

[PATCH] input_validation_tests.py: Reduce overhead of multiple checkouts.

[Noorul Islam K M]

This patch reduces checkout by around 23 times.

Log
[[[
Combine input validation tests to reduce the overhead of multiple checkouts.

* subversion/tests/cmdline/input_validation_tests.py:
  (invalid_targets): Build sandbox once and make following methods local.
(invalid_wcpath_add, invalid_wcpath_changelist,
invalid_wcpath_cleanup, invalid_wcpath_commit, invalid_copy_sources,
invalid_copy_target, invalid_delete_targets, invalid_diff_targets,
invalid_export_targets, invalid_import_rags, invalid_log_targets,
invalid_merge_rags, invalid_wcpath_upgrade, invalid_resolve_targets,
invalid_resolved_targets, invalid_revert_targets,
invalid_lock_targets, invalid_unlock_targets,
invalid_status_targets, invalid_patch_targets,
invalid_switch_targets, invalid_relocate_targets,
invalid_mkdir_targets, invalid_update_targets)

  (test_list): Add new combined test and remove individual tests.

Patch by: Noorul Islam K M 
]]]

Index: subversion/tests/cmdline/input_validation_tests.py
===
--- subversion/tests/cmdline/input_validation_tests.py  (revision 1072854)
+++ subversion/tests/cmdline/input_validation_tests.py  (working copy)
@@ -64,222 +64,203 @@
 #   Each test must return on success or raise on failure.
 #--
 
-def invalid_wcpath_add(sbox):
-  "non-working copy paths for 'add'"
+def invalid_targets(sbox):
+  "inavlid wc and url targets"
   sbox.build(read_only=True)
-  for target in _invalid_wc_path_targets:
-run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'add', target)
+  
+  def invalid_wcpath_add():
+"non-working copy paths for 'add'"
+for target in _invalid_wc_path_targets:
+  run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'add', 
target)
 
-def invalid_wcpath_changelist(sbox):
-  "non-working copy paths for 'changelist'"
-  sbox.build(read_only=True)
-  for target in _invalid_wc_path_targets:
-run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'changelist',
- 'foo', target)
-run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'changelist',
- '--remove', target)
+  def invalid_wcpath_changelist():
+"non-working copy paths for 'changelist'"
+for target in _invalid_wc_path_targets:
+  run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'changelist',
+   'foo', target)
+  run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'changelist',
+   '--remove', target)
 
-def invalid_wcpath_cleanup(sbox):
-  "non-working copy paths for 'cleanup'"
-  sbox.build(read_only=True)
-  for target in _invalid_wc_path_targets:
-run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'cleanup',
- target)
+  def invalid_wcpath_cleanup():
+"non-working copy paths for 'cleanup'"
+for target in _invalid_wc_path_targets:
+  run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'cleanup',
+   target)
 
-def invalid_wcpath_commit(sbox):
-  "non-working copy paths for 'commit'"
-  sbox.build(read_only=True)
-  for target in _invalid_wc_path_targets:
-run_and_verify_svn_in_wc(sbox, "svn: E205000: '.*' is a URL, but URLs 
cannot be " +
- "commit targets", 'commit', target)
+  def invalid_wcpath_commit():
+"non-working copy paths for 'commit'"
+for target in _invalid_wc_path_targets:
+  run_and_verify_svn_in_wc(sbox, "svn: E205000: '.*' is a URL, but URLs 
cannot be " +
+   "commit targets", 'commit', target)
 
-def invalid_copy_sources(sbox):
-  "invalid sources for 'copy'"
-  sbox.build(read_only=True)
-  for (src1, src2) in [("iota", "^/"), ("^/", "iota"), ("file://", "iota")]:
-run_and_verify_svn_in_wc(sbox, "svn: E27: Cannot mix repository and 
working " +
- "copy sources", 'copy', src1, src2, "A")
+  def invalid_copy_sources():
+"invalid sources for 'copy'"
+for (src1, src2) in [("iota", "^/"), ("^/", "iota"), ("file://", "iota")]:
+  run_and_verify_svn_in_wc(sbox, "svn: E27: Cannot mix repository and 
working " +
+   "copy sources", 'copy', src1, src2, "A")
 
-def invalid_copy_target(sbox):
-  "invalid target for 'copy'"
-  sbox.build(read_only=True)
-  mu_path = os.path.join('A', 'mu')
-  C_path = os.path.join('A', 'C')
-  run_and_verify_svn_in_wc(sbox, "svn: E155007: Path '.*' is not a directory",
-   'copy', mu_path, C_path, "iota")
+  def invalid_copy_target():
+"invalid target for 'copy'"
+mu_path = os.path.join('A', 'mu')
+C_path = os.path.join('A', 'C')
+run_and_verify_svn_in_wc(sbox, "svn: E155007: Path '.*' is not a 
directory",
+ 'copy', mu_path, C_path, "