Re: Re-use connection for svn:externals

[Justin Erenkrantz]

On Mon, Feb 15, 2010 at 10:18 PM, Phillip Hellewell  wrote:
> Alright.  Here are my thoughts so far based on what others have said:
>
> I'm not quite sure about Justin's suggestion, because I don't think the
> delay is in establishing the TCP connection; I think it is in performing the
> authentication and stuff.  So just re-using a TCP connection won't buy us
> much, right?

If you are using Kerberos-based auth and you've already completed
authentication on that connection, you likely won't need to re-auth if
you reuse the TCP connection via Serf or Neon.  (All hail stupid
authentication systems like Kerberos which are connection-oriented.
Not.)  -- justin

Re: Re-use connection for svn:externals

[Ivan Zhakov]

On Tue, Feb 16, 2010 at 9:18 AM, Phillip Hellewell  wrote:
> Alright.  Here are my thoughts so far based on what others have said:
>
> I'm not quite sure about Justin's suggestion, because I don't think the
> delay is in establishing the TCP connection; I think it is in performing the
> authentication and stuff.  So just re-using a TCP connection won't buy us
> much, right?
>
Sharing TCP connection has problems in case of connection based
authentication/multithreading and etc.

> As for Bert's concern about always opening at the repository root could
> cause issues like #3242, does everyone agree that we should avoid doing that
> as part of the solution?
>
> So could we (as some suggested) take an existing RA session and reparent it
> to the root?  Then if using that doesn't work, we fall back to creating a
> new session (since as Stefan suggested, this is an optimization so it
> doesn't need to break anything).
>
Yes, you can avoid issues like #3242 if you open RA session to URL and
then reparent to the root.


-- 
Ivan Zhakov
VisualSVN Team

Re: Re-use connection for svn:externals

[Phillip Hellewell]

Alright.  Here are my thoughts so far based on what others have said:

I'm not quite sure about Justin's suggestion, because I don't think the
delay is in establishing the TCP connection; I think it is in performing the
authentication and stuff.  So just re-using a TCP connection won't buy us
much, right?

As for Bert's concern about always opening at the repository root could
cause issues like #3242, does everyone agree that we should avoid doing that
as part of the solution?

So could we (as some suggested) take an existing RA session and reparent it
to the root?  Then if using that doesn't work, we fall back to creating a
new session (since as Stefan suggested, this is an optimization so it
doesn't need to break anything).

Do I understand correctly so far?

Phillip

On Mon, Feb 15, 2010 at 3:18 AM, Julian Foad wrote:

> On Tue, 2010-02-09, Phillip Hellewell wrote:
> > On Tue, Feb 9, 2010 at 10:09 AM, Julian Foad  >wrote:
> >
> > >
> > > Would you, or anyone you know, be interested in working on it? I would
> > > be glad to give you some help and guidance.
> > >
> >
> > I don't have any experience with the subversion source code right now,
> but
> > I'd be willing to become familiar with it if no one else were interested
> in
> > implementing it.
> >
> > But based on the lively discussion I see so far, it sounds like a feature
> > other people really want to, so I'm guessing you or Ivan or C. Michael
> will
> > probably beat me to it.
>
> Not terribly likely. While I'm interested, there is more urgent stuff
> that I'm working on, and I expect the others are in the same position.
> So please do jump in and get the ball rolling by posting your first
> thought or question on how to do it.
>
> - Julian
>
>
>

Re: svn commit: r910298 - in /subversion/trunk/subversion/bindings/swig: include/svn_types.swg ruby/svn/wc.rb ruby/test/test_wc.rb

[Joe Swatosh]

Hi Hyrum,

On Mon, Feb 15, 2010 at 11:54 AM, Hyrum K. Wright
 wrote:
> Joe,
> Great to see this work happening.  Be aware that some of the APIs are still 
> moving targets, so some things might break.
>
> -Hyrum
>

With you mentioning elsewhere that a branch could happen as early as
May, I figured I better get started.  Understanding that there are no
guarantees, would you please point me at the most stable of the 1.7
moving targets? :-)  Or perhaps I should just table until you give me
a high sign?

Any hope of getting the Ubuntu buildbot going again?

Thanks,
--
Joe

Re: WC-NG: How can we help with 1.7-readiness? - move props into wc.db

[Greg Stein]

On Mon, Feb 15, 2010 at 15:02, Hyrum K. Wright
 wrote:
>
> On Feb 15, 2010, at 12:28 PM, Philip Martin wrote:
>
>> Julian Foad  writes:
>>
>>> I'll have a go at moving props into wc.db.  Has anyone already looked at
>>> that or got a clue where I could start?  I'll start searching for uses
>>> of the svn_wc__db_*_props functions and seeing where that leads, but I
>>> don't know if that's the right end of the stick.
>>
>> Hyrum said he had some code but that it didn't work properly.  I think
>> it might be the currently unused upgrade.c:migrate_props (if so then I
>> fixed a hash initialisation bug that would have prevented it working,
>> although I don't know if it now works).
>
> This is indeed true.  I've attached my patch to the issue, and noted the 
> reason it hasn't been committed yet, namely that the upgrade code was 
> completely non-functional, so migrating from an old working copy to a new 
> working copy wouldn't upgrade the properties.  Lately, Greg's reworked the 
> upgrade code, so this problem may be fixed, but I haven't tested it yet.

The upgrade/migration for properties is not (yet) fixed, but will be
as part of my upgrade rewrite.

Cheers,
-g

Re: WC-NG 3-trees description [was: '@BASE' vs. 'BASE tree']

[Greg Stein]

That page looks good.

On the definition of trees, the second para is correct: the path
changes for a renamed node. There will be an NG-WORKING node recording
"moved-away", and another NG-WORKING node recording "moved-here". An
NG-BASE node will be present for the original path, but will not exist
at the new path.

Cheers,
-g

On Mon, Feb 15, 2010 at 09:26, Julian Foad  wrote:
> Any WC-NG folks prepared to comment on this? We should put this
> description in note/wc-ng/... if it's OK.
>
> - Julian
>
> On Tue, 2010-02-09, I (Julian Foad) wrote:
>> Hi, all. I've written up this brief definition of the WC-NG DB trees
>> at
>> .
>>  Could you all have a look and add to it or just mention to me anything that 
>> is worth noting or correcting. After a few rounds I'll move it to an in-tree 
>> document.
>>
>> In particular, one question for Greg:
> [...]
>> > > So the wc_db trees are something like
>> > >
>> > >  * (the pristine one, known as BASE)
> [...]
>> > >  * (the tree-changes, known as WORKING)
> [...]
>> > >  * (the on-disk and in-property-storage stuff, known as ACTUAL)
>> > >
>> > >    A description of the text-content and properties of any nodes whose
>> > > text-content and/or properties are different from its WORKING version.
>> > > Any node that has no such differences does not appear in this tree.
>> >
>> > Its WORKING node; or if none, then the BASE node.
>>
>> Sorry Greg, I didn't understand your comment there. Could you clarify?
>>
>> > The ACTUAL tree can also record tree conflicts for nodes that exist in
>> > neither WORKING nor BASE.
>
>
>
>

Re: pristine store design

[Philip Martin]

Neels J Hofmeyr  writes:

> THE PRISTINE STORE
> ==
>
> The pristine store is a local cache of complete content of files that are
> known to be in the repository. It is hashed by a checksum of that content
> (SHA1).

I'm not sure whether you are planning one table per pristine store or
one table per working copy, but I think it's one per pristine store.
Obviously it doesn't makes no difference until pristine stores can be
shared (and it might be one per directory in the short term depending
on when stop being one database per directory).

> SOME IMPLEMENTATION INSIGHTS
> 
>
> There is a PRISTINE table in the SQLite database with columns
>  (checksum, md5_checksum, size, refcount)
>
> The pristine contents are stored in the local filesystem in a pristine file,
> which may or may not be compressed (opaquely hidden behind the pristines API).
> The goal is to be able to have a pristine store per working copy, per user as
> well as system-wide, and to configure each working copy as to which pristine
> store(s) it should use for reading/writing.
>   
> There is a canonical way of getting a given CHECKSUM's pristine file name for
> a given working copy without contacting the WC database (static function
> get_pristine_fname()).
>
> When interacting with the pristine store, we want to, as appropriate, check
> for (combos of):
>   db-presence- presence in the PRISTINE table with noted file size > 0
>   file-presence  - pristine file presence
>   stat-match - PRISTINE table's size and mtime match file system
>   checksum-match - validity of data in the file against the checksum
>
> file-presence is gotten for free from a successful stat-match (fstat),
> checksum-match (fopen) and unchecked read of the file (fopen).
>
> How fast we consider things:
>   db-presence- very fast to moderately fast (in case of "empty db cache")
>   file-presence  - slow (fstat or fopen)
>   stat-match - slow (fstat plus SQLite query)
>   checksum-match - super slow (reading, checksumming)

I'm prepared to believe a database query can be faster that stat when
the inode cache is cold, but what about when the inode cache is hot?
If the database query requires even one system call then it could well
be slower.  Multiple processes accessing a working copy, or writing to
the pristine store, might bias this further towards stat being faster,
If we decide to share the pristine store between several working
copies then a shared database could become a bottleneck.

[...]

> Use case "need": "I want to use this pristine's content, definitely."
> ---
> pseudocode:
>  pristine_check(&present, checksum, _usable) (3)
>  if !present:
>get_pristine_from_repos(checksum, ra) (9)
>  pristine_read(&stream, checksum)(6)
>
> (3) check for _usable:
>  - db-presence
>  - if the checksum is not present in the table, return that it is not
>present (don't check for file existence as well).
>  - stat-match (includes file-presence)
>  - if the checksum is present in the table but file is bad/not there,
>bail, asking user to 'svn cleanup --pristines' (or sth.)
>
> (9) See use case "fetch". After this, either the pristine file is ready for
> reading, or "fetch" has bailed already.
>
> (6) fopen()


I think this is the most important case from a performance point of
view.  This is what 'svn status' et al. use, and it's important for
GUIs as a lot of the "feel" depends on how fast a process can query
the metadata.

If we were to do away with the PRISTINE table, then we would not have
to worry about it becoming a bottleneck.  We don't need the existance
check if we are just about to open the file, since opening the file
proves that it exists.  We obviously have the checksum already, from
the BASE/WORKING table, so we only need the PRISTINE table for the
size/mtime.  Perhaps we could store those in the BASE/WORKING table
and eliminate the PRISTINE table, or is this too much of a layering
violation?  The pristine store is then just a sharded directory, into
which we move files and from which we read files.

-- 
Philip

Re: WC-NG: How can we help with 1.7-readiness? - move props into wc.db

[Hyrum K. Wright]

On Feb 15, 2010, at 12:28 PM, Philip Martin wrote:

> Julian Foad  writes:
> 
>> I'll have a go at moving props into wc.db.  Has anyone already looked at
>> that or got a clue where I could start?  I'll start searching for uses
>> of the svn_wc__db_*_props functions and seeing where that leads, but I
>> don't know if that's the right end of the stick.
> 
> Hyrum said he had some code but that it didn't work properly.  I think
> it might be the currently unused upgrade.c:migrate_props (if so then I
> fixed a hash initialisation bug that would have prevented it working,
> although I don't know if it now works).

This is indeed true.  I've attached my patch to the issue, and noted the reason 
it hasn't been committed yet, namely that the upgrade code was completely 
non-functional, so migrating from an old working copy to a new working copy 
wouldn't upgrade the properties.  Lately, Greg's reworked the upgrade code, so 
this problem may be fixed, but I haven't tested it yet.

Feel free to chuck, use, or rewrite the patch if it suits your purpose.

-Hyrum

Re: svn commit: r910298 - in /subversion/trunk/subversion/bindings/swig: include/svn_types.swg ruby/svn/wc.rb ruby/test/test_wc.rb

[Hyrum K. Wright]

Joe,
Great to see this work happening.  Be aware that some of the APIs are still 
moving targets, so some things might break.

-Hyrum

On Feb 15, 2010, at 12:51 PM, joeswat...@apache.org wrote:

> Author: joeswatosh
> Date: Mon Feb 15 18:51:12 2010
> New Revision: 910298
> 
> URL: http://svn.apache.org/viewvc?rev=910298&view=rev
> Log:
> First baby-step toward supporting WC-NG APIs in the Ruby Bindings: Provide a
> way to create a Context object.
> 
> * subversion/bindings/swig/ruby/test/test_wc.rb
>  (SvnWcTest#test_context_new_default_config,
>   SvnWcTest#test_context_new_specified_config,
>   SvnWcTest#test_context_create): New methods.
> 
> * subversion/bindings/swig/ruby/svn/wc.rb
>  (Svn::Wc::Context): New class.
>  (Svn::Wc::Context.create, Svn::Wc::Context.new, 
>   Svn::Wc::Context#destroy): New methods.
> 
> * subversion/bindings/swig/include/svn_types.swg
>  (OUTPARAM): add svn_wc_context_t for Ruby so it will be wrapped correctly.
>  (%apply apr_pool_t *pool): add *scratch_pool to the list of known names for
>   pools for Ruby bindings so it will be wrapped correctly.
> 
> Modified:
>subversion/trunk/subversion/bindings/swig/include/svn_types.swg
>subversion/trunk/subversion/bindings/swig/ruby/svn/wc.rb
>subversion/trunk/subversion/bindings/swig/ruby/test/test_wc.rb
> 
> Modified: subversion/trunk/subversion/bindings/swig/include/svn_types.swg
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/include/svn_types.swg?rev=910298&r1=910297&r2=910298&view=diff
> ==
> --- subversion/trunk/subversion/bindings/swig/include/svn_types.swg (original)
> +++ subversion/trunk/subversion/bindings/swig/include/svn_types.swg Mon Feb 
> 15 18:51:12 2010
> @@ -157,6 +157,10 @@
>   svn_wc_status_t **,
>   svn_wc_status2_t **,
>   svn_wc_committed_queue_t **,
> +#ifdef SWIGRUBY
> +  /* Only tested for Ruby */
> +  svn_wc_context_t **,
> +#endif
>   void **set_locks_baton
> };
> 
> @@ -592,7 +596,8 @@
> apr_pool_t *dir_pool,
> apr_pool_t *file_pool,
> apr_pool_t *node_pool,
> -apr_pool_t *result_pool
> +apr_pool_t *result_pool,
> +apr_pool_t *scratch_pool
> };
> #endif
> 
> 
> Modified: subversion/trunk/subversion/bindings/swig/ruby/svn/wc.rb
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/ruby/svn/wc.rb?rev=910298&r1=910297&r2=910298&view=diff
> ==
> --- subversion/trunk/subversion/bindings/swig/ruby/svn/wc.rb (original)
> +++ subversion/trunk/subversion/bindings/swig/ruby/svn/wc.rb Mon Feb 15 
> 18:51:12 2010
> @@ -704,5 +704,50 @@
> Wc.process_committed_queue(self, access, new_rev, rev_date, 
> rev_author)
>   end
> end
> +
> +Context = SWIG::TYPE_p_svn_wc_context_t
> +# A context is not associated with a particular working copy, but as
> +# operations are performed, will load the appropriate working copy
> +# information.
> +class Context
> +  class << self
> +
> +# Creates an new instance of Context.
> +#
> +#  arguments
> +#
> +# * :config (default=>nil) A \
> +# Svn::Core::Config with options that apply to this Context.
> +def new(arguments={})
> +  optional_arguments_defaults = { :config => nil }
> +  arguments = optional_arguments_defaults.merge(arguments)
> +  context = Wc.context_create(arguments[:config])
> +  return context
> +end
> +
> +# Creates an new instance of Context for use in the block, the 
> context 
> +# is destroyed when the block completes.
> +#
> +#  arguments
> +#
> +# see new.
> +def create(arguments={})
> +  context = new(arguments)
> +  begin
> +yield context
> +  ensure
> +context.destroy if context
> +  end
> +end
> +
> +  end
> +
> +  # Destroys the context, releasing any acquired resources.
> +  # The context is unavailable for any further operations.
> +  def destroy
> +Wc.context_destroy(self)
> +  end
> +end
> +
>   end
> end
> 
> Modified: subversion/trunk/subversion/bindings/swig/ruby/test/test_wc.rb
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/ruby/test/test_wc.rb?rev=910298&r1=910297&r2=910298&view=diff
> ==
> --- subversion/trunk/subversion/bindings/swig/ruby/test/test_wc.rb (original)
> +++ subversion/trunk/subversion/bindings/swig/ruby/test/test_wc.rb Mon Feb 15 
> 18:51:12 2010
> @@ -1073,4 +1073,30 @@
>   end
> end
>   end
> +
> +
> +  def test_context_new_default_config
> +assert_not_nil context = Svn::Wc::Context.new
> +  ensure
> +context.destroy
> +  end
> +
> +  def test_context_new_specified_config
>

Re: WC-NG: How can we help with 1.7-readiness? - move props into wc.db

[Philip Martin]

Julian Foad  writes:

> I'll have a go at moving props into wc.db.  Has anyone already looked at
> that or got a clue where I could start?  I'll start searching for uses
> of the svn_wc__db_*_props functions and seeing where that leads, but I
> don't know if that's the right end of the stick.

Hyrum said he had some code but that it didn't work properly.  I think
it might be the currently unused upgrade.c:migrate_props (if so then I
fixed a hash initialisation bug that would have prevented it working,
although I don't know if it now works).

-- 
Philip

Re: WC-NG: How can we help with 1.7-readiness? - move props into wc.db

[Julian Foad]

I (Julian Foad) wrote:
> On Thu, 2010-02-11, Greg Stein wrote:
> [...]
> > * move props into wc.db
> 
> I'll have a go at moving props into wc.db.  Has anyone already looked at
> that or got a clue where I could start?  I'll start searching for uses
> of the svn_wc__db_*_props functions and seeing where that leads, but I
> don't know if that's the right end of the stick.

I think the way in is to find all calls to svn_wc__prop_path(), and I've
just found Hyrum's big comment at the top of
subversion/libsvn_wc/props.c, about this.

- Julian


> I opened issue #3586 "Move properties into WC-NG DB"
>  to track
> this, and made the WC-NG meta-issue
>  depend on it.
> 
> - Julian

Re: WC-NG: How can we help with 1.7-readiness? - move props into wc.db

[Julian Foad]

On Thu, 2010-02-11, Greg Stein wrote:
[...]
> > What identifiable tasks remain to get WC-NG into a state of 1.7-readiness?
> > Are issues filed to track those things so that casual observers can a) be
> > aware of them and b) feel empowered to attack them without stepping on
> > somebody's toes?
> 
> I see several major items for wc-ng completion:
> 
> * remove entry_t usage within libsvn_wc/client
> * remove access_t usage withing libsvn_wc/client
> * move props into wc.db

I'll have a go at moving props into wc.db.  Has anyone already looked at
that or got a clue where I could start?  I'll start searching for uses
of the svn_wc__db_*_props functions and seeing where that leads, but I
don't know if that's the right end of the stick.

I opened issue #3586 "Move properties into WC-NG DB"
 to track
this, and made the WC-NG meta-issue
 depend on it.

- Julian


> * move to single/root wc.db
> * switch to new pristine file mgmt
[...]

Re: [PATCH] Fix race condition in svnsync locking

[Stefan Sperling]

On Fri, Nov 27, 2009 at 07:53:44PM -, Jon Foster wrote:
> Hi,
> 
> This fixes the race condition where two svnsync processes can lock
> the repository at the same time (as noticed by Stefan Sperling
> earlier today).  I wanted to do this without changing the RA layer,
> so that it can work with older servers.
> 
> Since the RA layer doesn't have a test-and-set primitive (other
> than a commit!), some raciness is inevitable.  This patch fixes the
> dangerous race, but introduces a much less dangerous race.  With
> this patch, two "svnsync" processes trying to lock the repository at
> the same time might both fail to get the lock.  To mitigate this,
> both processes will sleep for a random amount of time before
> retrying.  This should reduce the chance of them staying in lock-step
> and repeatedly failing.  (Hopefully the chance becomes negligible).

I've looked at this, and had a chat about it with Neels.

As you were saying, the fundamental problem is that we do not have a way
to lock the repository over a span of multiple commits. The per-commit
atomicity does not help svnsync in the way it is currently implemented.
It needs a repository lock which is valid across multiple commits and
revprop edits, and which can be set and tested atomically, without any
commits happening in-between.
As long as we don't have that, any "solution" we devise is going to be racy,
and there will be issues with stale locks.

We should consider adding an RA-layer primitive that gets a
file lock on a file somewhere within the server's filesystem.
This should already have been done when svnsync was invented,
and it's probably the only way to really fix this problem.

I'd rather not apply a band-aid. While your patch makes the race itself
less likely, it does not get rid of the stale lock problem.

So I don't think this is an acceptable solution, sorry. It's actually worse
than the existing workaround of using a file locking tool on the computer
running svnsync, and disabling the internal svnsync locking with the new
--disable-locking option which is now in trunk thanks to your other patch.
The workaround provides proper serialisation and does not suffer from stale
locks.

Stefan

Re: [Fwd: Yay! More problems serving up simple HTML!]

[Dan Poirier]

On Sat, Feb 13, 2010, at 03:25:04 PM, "C. Michael Pilato"  
wrote:

> Konstantin Kolinko wrote:
>> Is it still an issue? We saw the same for tomcat.apache.org several
>> hours ago, but I am not able to reproduce it now, nor do I observe it
>> with subversion.apache.org.
>
> It seems to be working okay for me at the moment.

FYI it's being discussed over at d...@httpd.apache.org.

Re: pristine store design

[Julian Foad]

Neels J Hofmeyr wrote:
> taking stock of the current state of the pristine store API and finding
> design docs missing, I have created a "design paper" to clarify.

Excellent. This is very useful.

BTW, I am joining the WC-NG effort so I'm glad to jump in here, as it
looks like one of the easier places to start.

Are you intending to use the descriptions in this paper to write doc
strings for this pristine store API functions? We should.

I'm interspersing comments on the API with comments on your doc, but I
try to say which are which.


> If you could be so kind to glance over it and straighten out my picture, if
> necessary. Upon approval, I'll check it into notes/ so we can edit.
> 
> Note, if my view is correct, this design text implies small changes to the
> current state of the API:
> - no need for _pristine_write()

Can I suggest that, instead, _install() and _get_temp_dir() should not
be exposed. _write() should accept NULL for the checksum parameter and,
in that case, calculate the checksum itself. The implementation could
(at least in that case) use _get_temp_dir() and _install() like your
pseudo-code for the use case "store". Even when the checksum is provided
it could do the same and then verify the provided checksum.

Then your use cases "store" and "new" and "fetch" would all use _write()
in simple ways.

However I think you are meaning to document the existing API first, so
we should perhaps keep these suggested changes separate.

> - need to add _pristine_forget()
> - change the _pristine_checkmode_t enum.
> 
> Still missing completely: how to handle a "high-water mark", i.e. how to
> determine which pristines get forgotten first.
> 
> Thanks!
> ~Neels
> plain text document attachment (pristine-store-design.txt)
> THE PRISTINE STORE
> ==
> 
> The pristine store is a local cache of complete content of files that are
> known to be in the repository. It is hashed by a checksum of that content
> (SHA1).
> 
> 
> SOME IMPLEMENTATION INSIGHTS
> 
> 
> There is a PRISTINE table in the SQLite database with columns
>  (checksum, md5_checksum, size, refcount)
> 
> The pristine contents are stored in the local filesystem in a pristine file,
> which may or may not be compressed (opaquely hidden behind the pristines API).
> The goal is to be able to have a pristine store per working copy, per user as
> well as system-wide, and to configure each working copy as to which pristine
> store(s) it should use for reading/writing.
>   
> There is a canonical way of getting a given CHECKSUM's pristine file name for
> a given working copy without contacting the WC database (static function
> get_pristine_fname()).

> When interacting with the pristine store, we want to, as appropriate, check
> for (combos of):
>   db-presence- presence in the PRISTINE table with noted file size > 0
>   file-presence  - pristine file presence
>   stat-match - PRISTINE table's size and mtime match file system
>   checksum-match - validity of data in the file against the checksum
> 
> file-presence is gotten for free from a successful stat-match (fstat),
> checksum-match (fopen) and unchecked read of the file (fopen).
> 
> How fast we consider things:
>   db-presence- very fast to moderately fast (in case of "empty db cache")
>   file-presence  - slow (fstat or fopen)
>   stat-match - slow (fstat plus SQLite query)
>   checksum-match - super slow (reading, checksumming)
> 
> 
> On the method of writing pristine files:
>   In short: *ALWAYS* copy to pristine temp and checksum along with that, then
>   'mv' into place.
> 
>   To avoid incomplete pristines, we never want to have a write stream to a
>   pristine file location. Instead, we write to a tempfile on the same file-
>   system device as the pristine store, after which a filesystem 'mv' puts it
>   into place and a row is stored in the database. Like that, a pristine file
>   is either intact or doesn't exist (unless corrupted by alien ray guns).
>   
>   When fetching a new pristine from the repository, get a temporary-file
>   location from the pristine API, receive&checksum to that file, install.
>   Typically, we have already checked if the pristine is stored yet.
> 
>   When there is some new content (added/modified file in the working copy), a
>   pristine with the same content may already exist "coincidentally" -- that
>   may be common in certain use scenarios, but is generally less common.
>   Discussing optimal ways in different situations:
>   - New file is *not* a temporary file ('svn add'/'commit', we are not allowed
> to 'mv' the file away), and a pristine with the same checksum and content
> as this file does *not* exist yet: The file needs to be checksummed and
> copied to the pristine store.
> --> write to pristine-tempfile, checksum along the way, install.
>   - New file is *not* a temporary file ('svn add'/'commit'), but a pristine
> with the same content already exists. To catch th

Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

[Philip Martin]

Philip Martin  writes:

> Matthew Bentham  writes:
>
>> wc-ng: work towards eliminating svn_wc_entry_t
>>
>> * subversion/libsvn_client/commit_util.c
>>(add_lock_token): Replace a use of svn_wc__maybe_get_entry with
>> use of svn_wc__node_get_*
>>
>> Patch by: Matthew Bentham 
>> ]]]
>> Index: subversion/libsvn_client/commit_util.c
>> ===
>> --- subversion/libsvn_client/commit_util.c   (revision 909397)
>> +++ subversion/libsvn_client/commit_util.c   (working copy)
>> @@ -195,19 +195,25 @@
>>  {
>>struct add_lock_token_baton *altb = walk_baton;
>>apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
>> -  const svn_wc_entry_t *entry;
>> +  const char* lock_token;
>> +  const char* url;
>> +  
>> +  SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx,
>> +local_abspath, scratch_pool, scratch_pool));
>
> Indentation is a bit off.  I'll fix and commit when the regression tests
> finish.

lock_tests.py 13 is failing:

../../../../src/subversion/tests/cmdline/lock_tests.py 13
../src/subversion/svn/commit-cmd.c:142: (apr_err=160037)
../src/subversion/libsvn_client/commit.c:853: (apr_err=160037)
svn: Commit failed (details follow):
../src/subversion/libsvn_client/commit_util.c:1640: (apr_err=160037)
../src/subversion/libsvn_delta/path_driver.c:254: (apr_err=160037)
../src/subversion/libsvn_fs_fs/tree.c:1936: (apr_err=160037)
../src/subversion/libsvn_fs_fs/lock.c:633: (apr_err=160037)
../src/subversion/libsvn_fs_fs/lock.c:566: (apr_err=160037)
../src/subversion/libsvn_fs_fs/lock.c:545: (apr_err=160037)
../src/subversion/libsvn_fs_fs/lock.c:599: (apr_err=160037)
svn: Cannot verify lock on path '/A/D/G/tau'; no matching lock-token available
Traceback (most recent call last):
  File "/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/main.py", 
line 1197, in run
rc = self.pred.run(sandbox)
  File 
"/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/testcase.py", line 
160, in run
return self.func(sandbox)
  File "../../../../src/subversion/tests/cmdline/lock_tests.py", line 542, in 
deleted_dir_lock
'-m', '', parent_dir)
  File 
"/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/actions.py", line 
219, in run_and_verify_svn
expected_exit, *varargs)
  File 
"/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/actions.py", line 
254, in run_and_verify_svn2
exit_code, out, err = main.run_svn(want_err, *varargs)
  File "/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/main.py", 
line 605, in run_svn
*(_with_auth(_with_config_dir(varargs
  File "/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/main.py", 
line 366, in run_command
None, *varargs)
  File "/home/pm/sw/subversion/src/subversion/tests/cmdline/svntest/main.py", 
line 538, in run_command_stdin
raise Failure
Failure

I think the problem is that svn_wc__node_get_url doesn't work for
deleted nodes, it certainly has no code to scan the working tree for
deletions.

-- 
Philip

Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

[Philip Martin]

Matthew Bentham  writes:

> wc-ng: work towards eliminating svn_wc_entry_t
>
> * subversion/libsvn_client/commit_util.c
>(add_lock_token): Replace a use of svn_wc__maybe_get_entry with
> use of svn_wc__node_get_*
>
> Patch by: Matthew Bentham 
> ]]]
> Index: subversion/libsvn_client/commit_util.c
> ===
> --- subversion/libsvn_client/commit_util.c(revision 909397)
> +++ subversion/libsvn_client/commit_util.c(working copy)
> @@ -195,19 +195,25 @@
>  {
>struct add_lock_token_baton *altb = walk_baton;
>apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
> -  const svn_wc_entry_t *entry;
> +  const char* lock_token;
> +  const char* url;
> +  
> +  SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx,
> +local_abspath, scratch_pool, scratch_pool));

Indentation is a bit off.  I'll fix and commit when the regression tests
finish.

-- 
Philip

Fwd: Re: Website: please add some content from the old links page

[Cristian Rigamonti]

Sorry, I read on the website that bugreports should be forwarded to users@ and
I mistakenly assumed this was valid also for reports concerning the svn website.

So I'm now forwarding to dev@

Cri

- Forwarded message from "C. Michael Pilato"  -

> Date: Sat, 13 Feb 2010 21:58:05 -0500
> From: "C. Michael Pilato" 
> To: Cristian Rigamonti 
> CC: us...@subversion.apache.org
> Subject: Re: Website: please add some content from the old links page
> User-Agent: Thunderbird 2.0.0.23 (X11/20090817)
> 
> Cristian Rigamonti wrote:
> > Hi, it seems the old http://subversion.tigris.org/links.html page was not
> > migrated to subversion.apache.org. While I can understand that it's quite a
> > burden to maintain a lot of information about third party tools etc, I think
> > that some of the content of that page should be added somewhere on the new
> > apache site: e.g. links to sample hook scripts
> > http://svn.apache.org/repos/asf/subversion/trunk/tools/hook-scripts/
> > http://svn.apache.org/repos/asf/subversion/trunk/contrib/hook-scripts/
> > 
> > Note that the page content is still reachable at
> > http://svn.apache.org/viewvc/subversion/trunk/www/links.html?revision=884848&view=co&pathrev=900404
> > (thanks to the IRC folks on #svn for the hint :)
> 
> This message would have a higher likelihood of being effective if aimed at
> the correct target audience.  The folks that maintain the Subversion website
> hang out on the dev@ list.
> 
> -- 
> C. Michael Pilato 
> CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

- End forwarded message -


signature.asc
Description: Digital signature

WC-NG 3-trees description [was: '@BASE' vs. 'BASE tree']

[Julian Foad]

Any WC-NG folks prepared to comment on this? We should put this
description in note/wc-ng/... if it's OK.

- Julian

On Tue, 2010-02-09, I (Julian Foad) wrote:
> Hi, all. I've written up this brief definition of the WC-NG DB trees
> at
> .
>  Could you all have a look and add to it or just mention to me anything that 
> is worth noting or correcting. After a few rounds I'll move it to an in-tree 
> document.
> 
> In particular, one question for Greg:
[...]
> > > So the wc_db trees are something like
> > >
> > >  * (the pristine one, known as BASE)
[...]
> > >  * (the tree-changes, known as WORKING)
[...]
> > >  * (the on-disk and in-property-storage stuff, known as ACTUAL)
> > >
> > >A description of the text-content and properties of any nodes whose
> > > text-content and/or properties are different from its WORKING version.
> > > Any node that has no such differences does not appear in this tree.
> > 
> > Its WORKING node; or if none, then the BASE node.
> 
> Sorry Greg, I didn't understand your comment there. Could you clarify?
> 
> > The ACTUAL tree can also record tree conflicts for nodes that exist in
> > neither WORKING nor BASE.

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

[Julian Foad]

Kamesh Jayachandran wrote:
> With the below apache configuration(See the trailing slash at the end of 
> '/svn/').
> 
> 
>DAV svn
>SVNParentPath /repositories
>SVNMasterURI http://master/svn/
>SVNAdvertiseV2Protocol Off
> 
[...]
> Attached patch fixes it.
> 
> This change came via fix for issue 3275. I believe this assertion is 
> just to clean the uri for double slash and not anything functional to 
> that issue, I may be wrong.
> 
> Thoughts?

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

[[[
Index: subversion/mod_dav_svn/dav_svn.h
===
--- subversion/mod_dav_svn/dav_svn.h(revision 910187)
+++ subversion/mod_dav_svn/dav_svn.h(working copy)
@@ -352,7 +352,7 @@
 /* Return the activities db */
 const char * dav_svn__get_activities_db(request_rec *r);
 
-/* Return the root directory */
+/* Return the root directory (not necessarily as a canonical path) */
 const char * dav_svn__get_root_dir(request_rec *r);


]]] 

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

- Julian

pristine store design

[Neels J Hofmeyr]

Hi all,

taking stock of the current state of the pristine store API and finding
design docs missing, I have created a "design paper" to clarify.

If you could be so kind to glance over it and straighten out my picture, if
necessary. Upon approval, I'll check it into notes/ so we can edit.

Note, if my view is correct, this design text implies small changes to the
current state of the API:
- no need for _pristine_write()
- need to add _pristine_forget()
- change the _pristine_checkmode_t enum.

Still missing completely: how to handle a "high-water mark", i.e. how to
determine which pristines get forgotten first.

Thanks!
~Neels
THE PRISTINE STORE
==

The pristine store is a local cache of complete content of files that are
known to be in the repository. It is hashed by a checksum of that content
(SHA1).


SOME IMPLEMENTATION INSIGHTS


There is a PRISTINE table in the SQLite database with columns
 (checksum, md5_checksum, size, refcount)

The pristine contents are stored in the local filesystem in a pristine file,
which may or may not be compressed (opaquely hidden behind the pristines API).
The goal is to be able to have a pristine store per working copy, per user as
well as system-wide, and to configure each working copy as to which pristine
store(s) it should use for reading/writing.
  
There is a canonical way of getting a given CHECKSUM's pristine file name for
a given working copy without contacting the WC database (static function
get_pristine_fname()).

When interacting with the pristine store, we want to, as appropriate, check
for (combos of):
  db-presence- presence in the PRISTINE table with noted file size > 0
  file-presence  - pristine file presence
  stat-match - PRISTINE table's size and mtime match file system
  checksum-match - validity of data in the file against the checksum

file-presence is gotten for free from a successful stat-match (fstat),
checksum-match (fopen) and unchecked read of the file (fopen).

How fast we consider things:
  db-presence- very fast to moderately fast (in case of "empty db cache")
  file-presence  - slow (fstat or fopen)
  stat-match - slow (fstat plus SQLite query)
  checksum-match - super slow (reading, checksumming)


On the method of writing pristine files:
  In short: *ALWAYS* copy to pristine temp and checksum along with that, then
  'mv' into place.

  To avoid incomplete pristines, we never want to have a write stream to a
  pristine file location. Instead, we write to a tempfile on the same file-
  system device as the pristine store, after which a filesystem 'mv' puts it
  into place and a row is stored in the database. Like that, a pristine file
  is either intact or doesn't exist (unless corrupted by alien ray guns).
  
  When fetching a new pristine from the repository, get a temporary-file
  location from the pristine API, receive&checksum to that file, install.
  Typically, we have already checked if the pristine is stored yet.

  When there is some new content (added/modified file in the working copy), a
  pristine with the same content may already exist "coincidentally" -- that
  may be common in certain use scenarios, but is generally less common.
  Discussing optimal ways in different situations:
  - New file is *not* a temporary file ('svn add'/'commit', we are not allowed
to 'mv' the file away), and a pristine with the same checksum and content
as this file does *not* exist yet: The file needs to be checksummed and
copied to the pristine store.
--> write to pristine-tempfile, checksum along the way, install.
  - New file is *not* a temporary file ('svn add'/'commit'), but a pristine
with the same content already exists. To catch this case, we would need to
checksum the file *before* copying to the tempfile and then don't bother to
copy if a match is found. BUT the file could be changed in-between us
checksumming in-place, not finding a match and then copying! We should
copy to a tempfile first anyway.
  - New file *is* a tempfile (we may 'mv' it away) and happens to be on the
same filesystem device as the pristine store. We could checksum in-place
and call pristine_install() on the tempfile directly. *But* code, in this
case, should already have chosen the pristine-store tempfile location to
begin with.
  So, it does not make sense to optimize here.
  (Depending on hardware, copying across devices can be faster or slower
  than copying within the same device. Can't optimise on that.)


On overwriting orphaned pristine files that have no database entry:
  "Maybe the user saved his favourite aunt's final poetic words in a file that
  has a name that is this checksum and which ended up in the pristine store
  accidentally ..."
  This is super highly unlikely, given that the file name would have to match
  the checksum. We don't really need to play safe.
  But, if we find a pristine file where the db didn't expect one, it may
  som

Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

[Matthew Bentham]

Greg Stein wrote:

For log messages, we also like to provide attribution, so at the end
of the above message you would have:

Patch by: Matthew Bentham 


btw I think this is not properly explained by 
http://subversion.apache.org/docs/community-guide/conventions.html#log-messages 
or 
http://subversion.apache.org/docs/community-guide/conventions.html#crediting
which say that "Patch by" is to be used when contributing a patch 
written by someone else.



No need to initialize these to NULL. The node functions will always
set the value, unless it returns an error (in which case, you don't
care about the values).

Hmm. I would also rejigger things a bit while you're at it: fetch the
lock token first, and if NULL, then early-exit from the function. The
URL can usually be derived, so will be present quite often. The lock
token is the more obvious discriminator. Also, when you fetch the URL,
you'll be able to use token_pool for the call's result_pool. Does that
all make sense?


Implemented these in revised patch attached.

[[[
wc-ng: work towards eliminating svn_wc_entry_t

* subversion/libsvn_client/commit_util.c
   (add_lock_token): Replace a use of svn_wc__maybe_get_entry with
use of svn_wc__node_get_*

Patch by: Matthew Bentham 
]]]
Index: subversion/libsvn_client/commit_util.c
===
--- subversion/libsvn_client/commit_util.c  (revision 909397)
+++ subversion/libsvn_client/commit_util.c  (working copy)
@@ -195,19 +195,25 @@
 {
   struct add_lock_token_baton *altb = walk_baton;
   apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
-  const svn_wc_entry_t *entry;
+  const char* lock_token;
+  const char* url;
+  
+  SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx,
+local_abspath, scratch_pool, scratch_pool));
 
-  SVN_ERR(svn_wc__maybe_get_entry(&entry, altb->wc_ctx, local_abspath,
-  svn_node_unknown, FALSE, FALSE,
-  scratch_pool, scratch_pool));
+  if (!lock_token)
+return SVN_NO_ERROR;
 
+  SVN_ERR(svn_wc__node_get_url(&url, altb->wc_ctx, local_abspath, 
+   token_pool, scratch_pool));
+
   /* I want every lock-token I can get my dirty hands on!
  If this entry is switched, so what.  We will send an irrelevant lock
  token. */
-  if (entry && entry->url && entry->lock_token)
-apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, entry->url),
+  if (url)
+apr_hash_set(altb->lock_tokens, url,
  APR_HASH_KEY_STRING,
- apr_pstrdup(token_pool, entry->lock_token));
+ apr_pstrdup(token_pool, lock_token));
 
   return SVN_NO_ERROR;
 }

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

[Kamesh Jayachandran]

Hi All,

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



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



We get the following error on the client while.

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



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


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

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




Attached patch fixes it.


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




Thoughts?

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

RE: [PATCH] Add svnsync --use-external-locking option

[Jon Foster]

Hi,

Stefan Sperling wrote:
> On Fri, Nov 27, 2009 at 04:14:08PM -, Jon Foster wrote:
> > Add --using-external-locking option to svnsync.
> Hi Jon,
> 
> I've finally found time to take a look at this. Sorry for taking so
> long. I have committed your patch in r910212, with some tweaks.

Thankyou for this!

Apache now has an ICLA from me and a CCLA from my employer, which
means that the legal side is (finally) sorted.

Kind regards,

Jon


**
This email and its attachments may be confidential and are intended solely for 
the use of the individual to whom it is addressed. Any views or opinions 
expressed are solely those of the author and do not necessarily represent those 
of Cabot Communications Ltd.

If you are not the intended recipient of this email and its attachments, you 
must take no action based upon them, nor must you copy or show them to anyone.

Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232

Co. Registered in England number 02817269

Please contact the sender if you believe you have received this email in error.

**


__
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email 
__

Re: [PATCH] Add svnsync --use-external-locking option

[Stefan Sperling]

On Fri, Nov 27, 2009 at 04:14:08PM -, Jon Foster wrote:
> Hi,
> 
> As discussed on this list, we don't always need svnsync's networked
> lock.  If svnsync only runs on a single server, the administrator
> can use the "flock" tool to prevent running multiple copies of
> svnsync at the same time.
> 
> And if svnsync's lock is not needed, then it is actually an
> inconvenience.  E.g. if the network connection fails, then a stale
> lock can be left behind.  This requires manual administrator
> intervention to fix.
> 
> One workaround is for your scripts to run this command immediately
> before they start svnsync:
> svn propdel svn:sync-lock --revprop -r0 $target_url
> 
> However, it would be better to provide a command-line option to
> disable svnsync's locking.  So, here's a patch to do that.
> 
> The option name has been chosen to try to make it obvious that no
> locking is a bad idea, and administrators should either use
> svnsync's internal locking or have their own external locking.
> 
> [[[
> Add --using-external-locking option to svnsync.
> 
> * subversion/svnsync/main.c
>   (svnsync__opt): Add svnsync_opt_using_external_locking.
>   (SVNSYNC_OPTS_DEFAULT): Add svnsync_opt_using_external_locking.
>   (svnsync_options): Add entry for --using-external-locking.
>   (opt_baton_t): Add using_external_locking member.
>   (initialize_cmd, synchronize_cmd, copy_revprops_cmd): Don't take the
>   lock if using external locking.
>   (main): Handle svnsync_opt_using_external_locking option.
> 
> Patch by: Jon Foster 
> ]]]

Hi Jon,

I've finally found time to take a look at this. Sorry for taking so long.
I have committed your patch in r910212, with some tweaks.

Just so you know what the tweaks were:

> Index: subversion/svnsync/main.c
> ===
> --- subversion/svnsync/main.c (revision 884900)
> +++ subversion/svnsync/main.c (working copy)
> @@ -61,6 +61,7 @@
>svnsync_opt_sync_password,
>svnsync_opt_config_dir,
>svnsync_opt_config_options,
> +  svnsync_opt_using_external_locking,

I've renamed this option to --disable-locking. Matter of taste, really.

>svnsync_opt_version,
>svnsync_opt_trust_server_cert,
>svnsync_opt_allow_non_empty,
> @@ -76,7 +77,8 @@
>   svnsync_opt_sync_username, \
>   svnsync_opt_sync_password, \
>   svnsync_opt_config_dir, \
> - svnsync_opt_config_options
> + svnsync_opt_config_options, \
> + svnsync_opt_using_external_locking

You've added the option as a global option for all subcommands.
I've made the option specific to the 'init', 'sync', and 'copy-revprops'
subcommands instead. It's not useful with other subcommands.

>  
>  static const svn_opt_subcommand_desc2_t svnsync_cmd_table[] =
>{
> @@ -180,6 +182,10 @@
>"For example:\n"
>" "
>"servers:global:http-library=serf")},
> +{"using-external-locking",  svnsync_opt_using_external_locking, 0,
> +   N_("Disable built in locking.  You must ensure that\n"
> +  " "
> +  "no other instance of svnsync is running.")},

I've tweaked the help text a bit, to make clear that the mirror
can get corrupted if this option is used and nothing is done
to avoid running multiple instances of svnsync in parallel.

>  {"version",svnsync_opt_version, 0,
> N_("show program version information")},
>  {"help",   'h', 0,
> @@ -201,6 +207,7 @@
>const char *sync_password;
>const char *config_dir;
>apr_hash_t *config;
> +  svn_boolean_t using_external_locking;
>svn_boolean_t quiet;
>svn_boolean_t allow_non_empty;
>svn_boolean_t version;
> @@ -798,7 +805,14 @@
>SVN_ERR(svn_ra_open3(&to_session, baton->to_url, NULL,
> &(baton->sync_callbacks), baton, baton->config, 
> pool));
>SVN_ERR(check_if_session_is_at_repos_root(to_session, baton->to_url, 
> pool));
> -  SVN_ERR(with_locked(to_session, do_initialize, baton, pool));
> +  if (opt_baton->using_external_locking)
> +{

We don't require braces around single-line if-blocks, so I've removed them.

> +  SVN_ERR(do_initialize(to_session, baton, pool));
> +}
> +  else
> +{
> +  SVN_ERR(with_locked(to_session, do_initialize, baton, pool));
> +}
>  
>return SVN_NO_ERROR;
>  }
> @@ -1276,7 +1290,14 @@
>SVN_ERR(svn_ra_open3(&to_session, baton->to_url, NULL,
> &(baton->sync_callbacks), baton, baton->config, 
> pool));
>SVN_ERR(check_if_session_is_at_repos_root(to_session, baton->to_url, 
> pool));
> -  SVN_ERR(with_locked(to_session, do_synchronize, baton, pool));
> +  if (opt_baton->using_e

Re: [RFC] v2 Tree conflict resolver spec.

[Julian Foad]

On Wed, 2010-02-10, Neels J Hofmeyr wrote:
> Hi TC folks,
> 
> I tried to put the tree-conflicts design information so far into tables.

Thanks, Neels. This is a really useful way to present the info.


> Find the result attached: a tc-cheatsheet up for discussion. If it gets
> initial approval I'll check it into notes/tree-conflicts/ so we can edit.

[...]

> plain text document attachment (tc-cheatsheet.txt)
> TREE CONFLICT CHEAT SHEET (DESIGN PHASE)
> 
> 
>  After tree  | 'theirs'  | 'mine' is  | 'svn status' shows
>  conflict during | is found in   | found in   | TC and is otherwise
> -+---++---
>  |   ||
>  update/switch   | checked-out state | check-in state | adapted to be a
>  |   | (unchanged)| mod of the new
>  |   || checked-out state
>  |   ||
>  merge   | conflict info | check-in state | unchanged
>  |   | (unchanged)|
>  |   ||
>  
> 
> 
> 
>  Upon recording | ... changed during  |
>  a tree conflict, is| update/switch? | merge? |
> +++
>  checked-out state  | Yes| No |
>  check-in state | No | No |
>  'svn status' other than TC | Yes| No |
> 
> 
> 
> 
>  When resolving a | ...which was caused by a  |
>  tree-conflict with   | switch/update  |  merge   |
>   | , the check-in state is   |
> --+---+
>   ||  |
>  --accept=theirs  | cleared| reset to theirs  |
>   ||  |
>  --accept=mine| unchanged  | unchanged|
>   ||  |
>  'svn revert' | cleared| cleared  |
>   ||  |

All the above: +1.


> 
> 
> Assuming that we have full info on copy/move (i.e. an add here is really
> just an add and *not* possibly part of a copy/move), then:
> 
>  When resolving a  | 
>  tree-conflict of  | 
>  local vs. | incoming  | offer these --accept= resolution options
> ===+===+=
>|   |  
>  (all combinations of) | 
>  add   | add   | theirs, mine, move-theirs, move-mine 
>  copy-here | copy-here | 
>  move-here | move-here | 

We also want to offer some kind of "auto-merge" option which keeps one
version (just like "theirs" and like "mine") if both are identical, and
offers some kind of merging if they differ. Or which re-prompts if they
differ. Or the initial prompt should differ depending on whether the two
added things differ. It gets complex with directories, and with
differences in copy-from but identical content, etc.

>|   | 
> ---+---+-
>|   | 
>  delete| delete| (is not a tree-conflict)

I agree that most users will want this silently allowed. That's a
sensible behaviour for "svn".

Theoretically, however, that's only true in the same sense that
add-on-add is not a tree conflict if the same node is added twice: it's
an assumption that the user doesn't ever want to know about this kind of
conflict. Some users in some merging tasks will want a strict mode,
where they are not expecting any double-deletes and would like it
flagged if it happens.

Somehow I want to introduce a "conflict rules" framework, not
necessarily accessible through the "svn" UI, but within the
implementation and APIs. The rules will be a read-only context passed in
from the client that is like a table saying "delete-delete is to be a
silent delete, add-add is to be flagged as a conflict, edit-rename is to
be a silent edit of the renamed file, ...".

Then it will be easy for people to write a client that behaves more or
less strictly without having to first rev all our APIs.

And a second issue with "delete + delete": in a merge, we have to
consider whether the two things being deleted were identical. I haven't
yet tried to expand the rules to take account of that.

I *think* the same considerations also apply to all the other cases that
involve a delete during a merge.

>|   | 
> ---+---+-
>|   | 
>   (line-wise)  | 
>  delete| move-away | (i

Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

[Philip Martin]

Matthew Bentham  writes:

> How do you guys generally run an
> individual test or test program?  It's implied in the documentation
> that the test programs support it, but does 'make check' have any
> options?

On Linux, if you build in the source directory

$ cd subversion/tests/cmdline
$ ./lock_tests.py -h
$ ./lock_tests.py -v 18

If you build in a separate object directory that is a sibling of the
source directory:

$ cd subversion/tests/cmdline
$ ../../../../src/subversion/tests/cmdline/lock_tests.py -h

-- 
Philip

Re: Changing the "native" newline mode

[Philip Martin]

Stefan Sperling  writes:

> I agree that, in general, it sucks if something suddenly stops working.
> And I also think people missing this feature will survive.

It should be simple to provide a detach utility or subcommand so that
'svn detach wc/foo bar' creates a new working copy bar as a copy of
wc/foo complete with any local modifications and with the appropriate
metadata stored in the root of bar.

-- 
Philip

Re: [PATCH] wc-ng: remove a use of svn_wc_entry_t from libsvn_client

[Matthew Bentham]

Hyrum K. Wright wrote:

On Feb 12, 2010, at 8:10 AM, Greg Stein wrote:


On Fri, Feb 12, 2010 at 10:39, Matthew Bentham  wrote:

Index: subversion/libsvn_client/commit_util.c
===
--- subversion/libsvn_client/commit_util.c  (revision 909397)
+++ subversion/libsvn_client/commit_util.c  (working copy)
@@ -195,19 +195,21 @@
 {
  struct add_lock_token_baton *altb = walk_baton;
  apr_pool_t *token_pool = apr_hash_pool_get(altb->lock_tokens);
-  const svn_wc_entry_t *entry;
+  const char* lock_token = NULL;
+  const char* url = NULL;

No need to initialize these to NULL. The node functions will always
set the value, unless it returns an error (in which case, you don't
care about the values).


+  SVN_ERR(svn_wc__node_get_url(&url, altb->wc_ctx, local_abspath,
+   scratch_pool, scratch_pool));
+  SVN_ERR(svn_wc__node_get_lock_token(&lock_token, altb->wc_ctx,
+local_abspath, scratch_pool, scratch_pool));

-  SVN_ERR(svn_wc__maybe_get_entry(&entry, altb->wc_ctx, local_abspath,
-  svn_node_unknown, FALSE, FALSE,
-  scratch_pool, scratch_pool));
-
  /* I want every lock-token I can get my dirty hands on!
 If this entry is switched, so what.  We will send an irrelevant lock
 token. */
-  if (entry && entry->url && entry->lock_token)
-apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, entry->url),
+  if (url && lock_token)
+apr_hash_set(altb->lock_tokens, apr_pstrdup(token_pool, url),
 APR_HASH_KEY_STRING,
- apr_pstrdup(token_pool, entry->lock_token));
+ apr_pstrdup(token_pool, lock_token));

Rather than dup'ing into the token_pool, you could pass that pool as
the result_pool to the node functions.

However, I'd go ahead and suggest leaving it as above so that
token_pool won't grow if only one of the values is present.

Hmm. I would also rejigger things a bit while you're at it: fetch the
lock token first, and if NULL, then early-exit from the function. The
URL can usually be derived, so will be present quite often. The lock
token is the more obvious discriminator. Also, when you fetch the URL,
you'll be able to use token_pool for the call's result_pool. Does that
all make sense?


FWIW, the original patch passes 'make check'.



That's worth quite a bit actually, I haven't been able to get it to pass 
'make check' after implementing Greg's suggestions.  It fails one of the 
tests in lock_tests.py.  How do you guys generally run an individual 
test or test program?  It's implied in the documentation that the test 
programs support it, but does 'make check' have any options?


Matthew

Re: Re-use connection for svn:externals

[Julian Foad]

On Tue, 2010-02-09, Phillip Hellewell wrote:
> On Tue, Feb 9, 2010 at 10:09 AM, Julian Foad wrote:
> 
> >
> > Would you, or anyone you know, be interested in working on it? I would
> > be glad to give you some help and guidance.
> >
> 
> I don't have any experience with the subversion source code right now, but
> I'd be willing to become familiar with it if no one else were interested in
> implementing it.
> 
> But based on the lively discussion I see so far, it sounds like a feature
> other people really want to, so I'm guessing you or Ivan or C. Michael will
> probably beat me to it.

Not terribly likely. While I'm interested, there is more urgent stuff
that I'm working on, and I expect the others are in the same position.
So please do jump in and get the ball rolling by posting your first
thought or question on how to do it.

- Julian

WC-NG - on detachable WCs and pristine store options [was: Changing the "native" newline mode]

[Julian Foad]

Just changing the Subject line to match the topic of conversation.

- Julian


On Mon, 2010-02-15 at 01:43 +0100, Stefan Sperling wrote:
> On Sun, Feb 14, 2010 at 07:31:07PM -0500, Glenn Maynard wrote:
> > On Sun, Feb 14, 2010 at 6:47 PM, Mark Mielke  wrote:
> > > Heck, if one can ask the server for missing pristine copies - why not 
> > > treat
> > > it like a "least recently used" cache, where users can cap the shared
> > > pristine copy to a certain size, and it will download the missing pristine
> > > copies as required when it needs them, rather than always keeping 
> > > everything
> > > local?
> > 
> > Putting aside shared data, being able to tell Subversion to go back to
> > CVS's "download the prestine copy as needed" behavior (eg. a
> > "svn:no-cache" property) would be extremely useful.  For large
> > compressed binaries (eg. .ogg files, the bulk of my large data),
> > binary diffs are utterly useless, so the benefits of the prestine
> > files around are mostly lost (except for, off-hand, svn revert).  For
> > me, this would solve the prestine-overhead problem completely.  If you
> > have many gigs of data that *can* be diffed, however, it obviously
> > wouldn't be as effective for you as shared prestine data would be.
> 
> Making the pristine store optional should be easy and I've seen
> this mentioned before:
> http://subversion.tigris.org/issues/show_bug.cgi?id=525#desc19
> 
> > On Sun, Feb 14, 2010 at 7:14 PM, Stefan Sperling  wrote:
> > > As I said, I don't expect pristines to clean up themselves.
> > 
> > Sorry; if you said that, I missed it and can't find it.
> 
> I was not explicit about it in my first reply. It was between the lines,
> and in my head :)
> 
> > > But if it's not a design goal, then there's no point in complaining
> > > when the feature goes away. Either the feature is part of the design
> > > or it isn't.
> > 
> > It's a use case that's always been handled by SVN and CVS.  When you
> > have a feature that's been supported for that long, which real users
> > expect to be able to do, then it's an oversight in the design if it
> > doesn't mention it.  That doesn't make it any less of a real use case
> > whose support is disappearing.  (Fortunately, it's a relatively minor
> > use case; I'll survive.)
> 
> I agree that, in general, it sucks if something suddenly stops working.
> And I also think people missing this feature will survive.
> 
> Stefan

Re: community guide redux

[Julian Foad]

On Fri, 2010-02-12 at 10:54 -0800, Justin Erenkrantz wrote:
> On Thu, Feb 4, 2010 at 1:59 AM, Julian Foad  wrote:
> > - Nav-menu needs to look significantly different from the main site nav
> > menu, and/or be integrated with it. It needs at least to say "*Community
> > Guide*" at the top of it, otherwise it looks like it's the nav menu of
> > the whole site.
> 
> It now looks a bit different - see now.  I just noticed that the
> background images are expecting to be against a white background, so
> may be remove the image and just go to the color matching.

Thanks, Justin. It looks really good now.


> > - The pages need a page title saying "Community Guide" (or "Subversion
> > Community Guide") at the top of the main pane (above the section title
> > "General"/"Community Roles"/etc).
> 
> Okay, I tried to do this...I'm ambivalent about how it looks, but it's there.

That's gone away again now. Not a big deal, though.

> > - Links to other sections will need to be updated to point to the new
> > section file. Currently they try to link to the same file.
> 
> This is an issue with how the page generation works - if we want to
> use the same source for either one-page or multi-page, this is a bit
> harder to do.  I haven't looked to see how prevalent these
> inter-section links are...

I haven't checked if inter-section links are working.

> > - Would be nice to have "Next page" and "Previous page" links at the
> > very end of the page, for those reading through. (But that's an
> > enhancement that can come later.)
> 
> Deferred.  =)
> 
> > Comments on the all-in-one page:
> >
> > - Apply the same style to it. (Take care that logically corresponding
> > heading levels match up.)
> 
> If we keep the all-in-one page, I think it's best to keep as-is
> (minimal/no formatting) so that it's easily searchable and printable.

Now it's using the same style as the multi-page. I don't think that
hampers searching and I think it enhances the printed output. (For
printing it renders without the side nav-bar.)

- Julian

My double-posts [was: very large revision file - very small amount of data]

[Julian Foad]

Sorry about my re-posts. It's happened a few times now. Sometimes my
mail client (Evolution) doesn't remove a post from the "out" box after
posting it, and next time I start it up it sends it again if I don't
delete it first.

- Julian


On Fri, 2010-02-12 at 16:42 +, Julian Foad wrote:
> Kevin mailed me today to say:
> > Appreciate the response. This is the same repository that Trent is
> > working on, and he has explained the issue to me. This group created a
> > directory which now has 50k plus directories in it causing the
> > problem. We have asked them to restructure the repository so there are
> > not so many directories in one place. 
> 
> The existence of a directory containing 5 entries, which take around
> 40 or so bytes each to store, explained the 2 or 3 MB revision file size
> they were seeing.
> 
> - Julian

Re: [PATCH] svn_fs.h doc updates on 'nodes' and 'node revisions'

[Julian Foad]

On Fri, 2010-02-12, C. Michael Pilato wrote:
> Julian Foad wrote:
> > * subversion/include/svn_fs.h
> >   Clarify the definition of 'nodes' and 'node revisions'.

> +1.  Commit away.  And thanks for taking the time to clarify some of this
> stuff ... as you know the terms can get a little fuzzy.

Thanks. Committed revision 910183.

- Julian