Re: Bug with svn_txdelta_window_t Python binding?

[Alexey Neyman]

On Tuesday, August 03, 2010 04:26:58 pm Роман Донченко wrote:
> Alexey Neyman  писал в своём письме Fri, 12 Mar 2010
> 
> 01:23:47 +0300:
> > On Thursday 11 March 2010 02:05:56 pm Роман Донченко wrote:
> >> Alexey Neyman  писал в своём письме Fri, 12 Mar 2010
> >> 
> >> 00:25:12 +0300:
> >> > Sorry to bug you, but are you working on a fix for this, or just
> >> > confirmed
> >> > it's a bug (in which case, I need to submit it to issues DB, I
> >> > guess)?
> >> > 
> >> > Best regards,
> >> > Alexey.
> >> 
> >> I've entered it into my TODO list, and I will (hopefully) fix it the
> >> next time I'm hacking on the bindings. I'm just kind of busy with other
> >> stuff right now.
> > 
> > Thanks! Just checking so that it does not get lost.
> > 
> > Regards,
> > Alexey.
> 
> Well, guess what. I actually got a round tuit, and fixed this. For the
> record, this happened in r981683, r981701 and r982064.
> 
> Roman.

Thanks! I updated issue 3688 to reflect that.

Regards,
Alexey.

Re: Bug with svn_txdelta_window_t Python binding?

[Роман Донченко]

Alexey Neyman  писал в своём письме Fri, 12 Mar 2010  
01:23:47 +0300:



On Thursday 11 March 2010 02:05:56 pm Роман Донченко wrote:

Alexey Neyman  писал в своём письме Fri, 12 Mar 2010

00:25:12 +0300:
> Sorry to bug you, but are you working on a fix for this, or just
> confirmed
> it's a bug (in which case, I need to submit it to issues DB, I
> guess)?
>
> Best regards,
> Alexey.

I've entered it into my TODO list, and I will (hopefully) fix it the
next time I'm hacking on the bindings. I'm just kind of busy with other
stuff right now.


Thanks! Just checking so that it does not get lost.

Regards,
Alexey.



Well, guess what. I actually got a round tuit, and fixed this. For the  
record, this happened in r981683, r981701 and r982064.


Roman.

Re: svn commit: r981828 - in /subversion/branches/performance/subversion: include/private/svn_file_handle_cache.h include/private/svn_temp_serializer.h libsvn_subr/svn_file_handle_cache.c libsvn_subr/

[Stefan Fuhrmann]

Hyrum K. Wright > 
wrote:



I this is what I get for not reading the entire identifier. I
apologize for the bad advice; I missed the second '__' farther down in
the symbol names.

-Hyrum


Apology accepted ;)

-- Stefan^2.

Re: svn commit: r981684 - in /subversion/branches/performance/subversion/libsvn_fs_fs: caching.c fs_fs.h

[Stefan Fuhrmann]

Blair Zajac wrote:

On 08/02/2010 01:51 PM, stef...@apache.org wrote:

Author: stefan2
Date: Mon Aug  2 20:51:35 2010
New Revision: 981684

URL: http://svn.apache.org/viewvc?rev=981684&view=rev
Log:
Bring the membuffer cache to its first use for the full text cache.
Also, provide functions to get / set the FSFS cache configuration
although not all of it is supported, yet.



Stefan,

I appreciate you working on this, I'm looking forward to using these 
improvements in production, which is why I'm closely looking at them.

Hi Blair,

I'm always open to constructive criticism.
So, tanks for your feedback!



+/* Access to the cache settings as a process-wide singleton. */
+static svn_fs_fs__cache_config_t *
+internal_get_cache_config(void)
+{
+  static svn_fs_fs__cache_config_t settings =
+{
+  /* default configuration:
+   */
+  0x800,   /* 128 MB for caches */
+  16,  /* up to 16 files kept open */
+  FALSE,   /* don't cache fulltexts */
+  FALSE,   /* don't cache text deltas */
+  FALSE/* assume multi-threaded operation */
+};
+
+  return&settings;
+}



Why not make "settings" static at file scope instead of inside this 
function?  What does this function provide for us?

Just a habit of mine from the C++ world (Meyer's singleton).
But since this is a POD structure, there should be no
initialization issues. I changed it to a static variable.


+/* Access the process-global (singleton) membuffer cache. The first 
call
+ * will automatically allocate the cache using the current cache 
config.

+ * NULL will be returned if the desired cache size is 0.
+ */
+static svn_membuffer_t *
+get_global_membuffer_cache(void)
+{
+  static svn_membuffer_t *cache = NULL;
+
+  apr_uint64_t cache_size = svn_fs_fs__get_cache_config()->cache_size;
+  if (!cache&&  cache_size)


s/cache&&/cache &&/

This seems to be a mailer artifact. At least in my editor it shows up fine.



+{
+  /* auto-allocate cache*/
+  apr_allocator_t *allocator = NULL;
+  apr_pool_t *pool = NULL;
+
+  if (apr_allocator_create(&allocator))
+return NULL;
+
+  /* ensure that a we free partially allocated data if we run OOM



s/a we/we/

Fixed.

+   */
+  apr_allocator_max_free_set(allocator, 1);
+  pool = svn_pool_create_ex(NULL, allocator);
+
+  svn_cache__membuffer_cache_create
+  (&cache,
+   cache_size,
+   cache_size / 16,
+   ! svn_fs_fs__get_cache_config()->single_threaded,
+   pool);
+}

+   * before the cache is complete.

So the allocator is associated with this pool forever then?  So it 
cannot be destroyed after

It limits the amount of *unused* memory held by the pool to 1 byte.
I extended on the commentary how that helps with certain OOM
situations.

+{
+  ffd->fulltext_cache = NULL;
+}
+
+  if (ffd->fulltext_cache&&  ! no_handler)

-ffd->fulltext_cache = NULL;

s/fulltext_cache&&  ! no_handler/fulltext_cache && ! no_handler

Same artifact as above.



+SVN_ERR(svn_cache__set_error_handler(ffd->fulltext_cache,
+warn_on_cache_errors, fs, pool));

return SVN_NO_ERROR;
  }

Modified: 
subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h
URL: 
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h?rev=981684&r1=981683&r2=981684&view=diff 

== 

--- subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h 
(original)
+++ subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h 
Mon Aug  2 20:51:35 2010

@@ -523,6 +523,39 @@ svn_fs_fs__get_node_origin(const svn_fs_
  svn_error_t *
  svn_fs_fs__initialize_caches(svn_fs_t *fs, apr_pool_t *pool);

+/* FSFS cache settings. It controls what caches, in what size and
+   how they will be created. The settomgs apply for the whole process.


s/settomgs/settings/

Fixed.



+ */
+typedef struct svn_fs_fs__cache_config_t
+{
+  /* total cache size in bytes. May be 0, resulting in no cache 
being used */

+  apr_uint64_t cache_size;
+
+  /* maximum number of files kept open */
+  apr_size_t file_handle_count;
+
+  /* shall fulltexts be cached? */
+  svn_boolean_t cache_fulltexts;
+
+  /* shall text deltas be cached? */
+  svn_boolean_t cache_txdeltas;
+
+  /* is this a guaranteed single-threaded application? */
+  svn_boolean_t single_threaded;
+} svn_fs_fs__cache_config_t;
+
+/* Get the current FSFS cache configuration. If it has not been set,
+   yet, this function will return the default settings.



s/yet,//

dito.



+ */
+const svn_fs_fs__cache_config_t *
+svn_fs_fs__get_cache_config(void);
+
+/* Set the FSFS cache configuration. Please note that it may not change
+   the actual configuration *in use*. Therefore, call it before reading
+   data from any FSFS repo and call it only once.


Should also document that this is not multi-threaded safe and does not 
protect agai

Re: Need fast ways to get Info once WC-NG is introduced

[Stefan Küng]

On 03.08.2010 12:42, Philip Martin wrote:

Stefan Küng  writes:


On 02.08.2010 12:32, Bert Huijben wrote:

So is there an (almost as) fast way to check whether a folder is
versioned or not?


I think the fastest way in the current code would be to call
svn_wc_read_kind() on the directory, maybe after first checking that there
is some .svn in at least one of the parent directories.


I thought about implementing a small cache for that, so that I don't
have to walk up the tree every time to find an .svn dir.
But I thought I read something about such a small cache getting
implemented in the svn library itself so I wanted to ask first - maybe
there's already an API to use that cache. Or maybe I just remember it
wrong.


Does TSVN cache/reuse svn_client_ctx_t handles?  In 1.7 the client
context contains an opaque wc context which in turn includes a
database context, svn_wc__db_t.  The database context caches sqlite
connections and has a cache mapping directory->database.


No, the TSVN cache doesn't reuse those at the moment. I might change 
that though...



Quite when TSVN should create/destroy svn_client_ctx_t is an
interesting question.  Reusing a long-lived context (or perhaps a
small number, one per-thread say) is likely to make individual svn
calls faster.  However the open database handles means that Windows
won't be able to delete root directories.  It's not clear to me how
or when TSVN would close those handles.


That's why I currently don't reuse SVN pools and contexts. There is a 
mechanism in place which tells the cache to release all handles, but it 
isn't very reliable. I will have to test whether I can reuse the 
contexts or whether I have to recreate them.


Stefan

--
   ___
  oo  // \\  "De Chelonian Mobile"
 (_,\/ \_/ \ TortoiseSVN
   \ \_/_\_/>The coolest Interface to (Sub)Version Control
   /_/   \_\ http://tortoisesvn.net

Re: Need fast ways to get Info once WC-NG is introduced

[Stefan Küng]

On 03.08.2010 00:31, Talden wrote:

Did you try compiling Subversion with the SVN_WC__SINGLE_DB and SINGLE_DB
defined in wc.h yet? (This enables the experimental single-db mode)

It should give some impression on what you can expect with single-db. (I
think the current status is about 40 testfailures (9 in the upgrade
tests),
but it almost reduces the testsuite time by 50% compared to multi-db)


I don't like to build the TSVN nightlies with such experimental features
yet. Once the features get into trunk without compile switches, I will of
course start using them. But as long as they're not activated, I think I'll
stay away from those. Not just because they might be too unstable, but
mostly because that means the APIs still change a lot and that's just too
much work for me to adjust TSVN every time. There's enough work to be done
in TSVN itself :)

Stefan


That's a shame, those build of yours are handy - I've started some
testing of 1.7 already using the nightly builds of
svn/svnadmin/svnserve but I would love to test something closer to the
end-game (particularly single db) even with known bugs.  I can
understand though why you're not building these combinations for TSVN
yet.

I'm not aware of anyone else doing nightly win32 Subversion binaries
so yours have been most helpful.

I'm using builds from here:

  http://nightlybuilds.tortoisesvn.net/latest/win32/full/


These are only built twice a week. If you want the builds that run every 
day, use the ones here too:

http://nightlybuilds.tortoisesvn.net/latest/win32/small/
Those are only missing the language packs.

Stefan


--
   ___
  oo  // \\  "De Chelonian Mobile"
 (_,\/ \_/ \ TortoiseSVN
   \ \_/_\_/>The coolest Interface to (Sub)Version Control
   /_/   \_\ http://tortoisesvn.net

RE: Mergeinfo not inherited due to file externals

[Brandt, Servatius (External)]

Hi Paul,

This sounds reasonable.  Thanks for looking into the issue.

Servatius

> -Original Message-
> From: Paul Burba [mailto:ptbu...@gmail.com]
> Sent: Tuesday, August 03, 2010 10:50 PM
> To: Brandt, Servatius (External)
> Cc: Subversion Development List
> Subject: Re: Mergeinfo not inherited due to file externals
> 
> On Mon, Jul 26, 2010 at 4:50 AM, Brandt, Servatius (External)
>  wrote:
> > A merge into a directory with a file external shows the following
> > problems:
> >
> > 1) mergeinfo at the directory is not inherited (and all files and
> >   subdirectories get their own inheritable mergeinfo).
> >
> > 2) mergeinfo is added to the external file.
> >
> > The mergeinfo at the external file is just wrong and useless, and 1)
> > will cause problems on a reintegrate merge.  A use case and an example
> > are following.
> 
> Hi Servatius,
> 
> What's happening here is that the merge tracking logic erroneously
> treats externals the same as switched subtrees and sets mergeinfo to
> segregate the "switched" subtree, so if it is later unswitched, it
> doesn't inherit mergeinfo from the root of the branch which was never
> actually merged.  This is fine for switched subtrees, but I agree with
> you, it doesn't make much sense for externals (which might not even be
> from the same repository!).
> 
> Your second issue goes away in 1.7 because we only set mergeinfo on
> paths affected by the merge, which the external never will be.
> 
> The first problem still exists however.  I think we can safely ignore
> externals in the merge target as far as recording mergeinfo describing
> the merge goes.  I'll look into this right now.
> 
> Paul
> 
> > Use case:
> >
> > I would like to place different types of branches into different
> > directories, e.g. ^/branches/feature/my-feature, ^/branches/release/1.0.
> > The developers should follow different codeline policies on each type of
> > branch, and the correct codeline policy should be automatically added to
> > each working copy of a new branch.  The policies are stored as:
> >
> > ^/CodelinePolicy.txt (trunk codeline policy)
> > ^/branches/feature/CodelinePolicy.txt
> > ^/branches/release/CodelinePolicy.txt
> >
> > svn ps svn:externals '../CodelinePolicy.txt CodelinePolicy.txt' trunk
> >
> > would do the trick.  Whenever a new branch is being created:
> >
> > svn cp ^/trunk ^/branches/feature/new-feature-branch
> > svn cp ^/trunk ^/branches/release/new-release-branch
> >
> > the correct CodelinePolicy would be shown in each WC of the new
> > branches.
> >
> > Currently, this would cause non-inheritable mergeinfo on the root
> > directory of each branch so that a reintegrate merge of a feature branch
> > to the trunk will fail with "Missing ranges".
> >
> > Example:
> >
> > $ svn -q --version
> > 1.6.12
> >
> > $ svn mkdir trunk trunk/dir
> > A         trunk
> > A         trunk\dir
> >
> > $ echo x >trunk/x.txt
> >
> > $ echo d >trunk/dir/d.txt
> >
> > $ svn add trunk/x.txt trunk/dir/d.txt
> > A         trunk\x.txt
> > A         trunk\dir\d.txt
> >
> > $ svn ci -m 'initial trunk'
> > Adding         trunk
> > Adding         trunk\dir
> > Adding         trunk\dir\d.txt
> > Adding         trunk\x.txt
> > Transmitting file data ..
> > Committed revision 1.
> >
> > $ echo e >e.txt
> >
> > $ svn add e.txt
> > A         e.txt
> >
> > $ svn ps svn:externals '../e.txt r.txt' trunk
> > property 'svn:externals' set on 'trunk'
> >
> > $ svn ci -m 'file external r.txt on trunk referring to e.txt in parent dir'
> > Adding         e.txt
> > Sending        trunk
> > Transmitting file data .
> > Committed revision 2.
> >
> > $ svn cp trunk branch
> > A         branch
> >
> > $ svn ci -m 'branch created from trunk'
> > Adding         branch
> > Adding         branch\dir
> > Adding         branch\x.txt
> >
> > Committed revision 3.
> >
> > $ svn up
> >
> > Fetching external item into 'trunk\r.txt'
> > E    trunk\r.txt
> > Updated external to revision 3.
> >
> >
> > Fetching external item into 'branch\r.txt'
> > E    branch\r.txt
> > Updated external to revision 3.
> >
> > Updated to revision 3.
> >
> > $ cd trunk
> >
> > $ echo xx >>x.txt
> >
> > $ svn ci -m 'trunk/x.txt changed'
> > Sending        trunk\x.txt
> > Transmitting file data .
> > Committed revision 4.
> >
> > $ cd ../branch
> >
> > $ svn merge ^/trunk
> > --- Merging r3 through r4 into 'x.txt':
> > U    x.txt
> >
> > $ svn st
> >  M      .
> >  M  X   r.txt
> > MM      x.txt
> >  M      dir
> >
> > $ svn diff
> >
> > Property changes on: .
> >
> __
> _
> > Added: svn:mergeinfo
> >   Merged /trunk:r3-4*
> >
> >
> > Property changes on: r.txt
> >
> __
> _
> > Added: svn:mergeinfo
> >   Merged /trunk/r.txt:r3-4
> >
> > Index: x.txt
> >
> ==
> =
> > --- x.txt       (revision 3)
> > +++ x.txt       (working copy)
> > @@ -1 +1,2 

Re: Mergeinfo not inherited due to file externals

[Paul Burba]

On Mon, Jul 26, 2010 at 4:50 AM, Brandt, Servatius (External)
 wrote:
> A merge into a directory with a file external shows the following
> problems:
>
> 1) mergeinfo at the directory is not inherited (and all files and
>   subdirectories get their own inheritable mergeinfo).
>
> 2) mergeinfo is added to the external file.
>
> The mergeinfo at the external file is just wrong and useless, and 1)
> will cause problems on a reintegrate merge.  A use case and an example
> are following.

Hi Servatius,

What's happening here is that the merge tracking logic erroneously
treats externals the same as switched subtrees and sets mergeinfo to
segregate the "switched" subtree, so if it is later unswitched, it
doesn't inherit mergeinfo from the root of the branch which was never
actually merged.  This is fine for switched subtrees, but I agree with
you, it doesn't make much sense for externals (which might not even be
from the same repository!).

Your second issue goes away in 1.7 because we only set mergeinfo on
paths affected by the merge, which the external never will be.

The first problem still exists however.  I think we can safely ignore
externals in the merge target as far as recording mergeinfo describing
the merge goes.  I'll look into this right now.

Paul

> Use case:
>
> I would like to place different types of branches into different
> directories, e.g. ^/branches/feature/my-feature, ^/branches/release/1.0.
> The developers should follow different codeline policies on each type of
> branch, and the correct codeline policy should be automatically added to
> each working copy of a new branch.  The policies are stored as:
>
> ^/CodelinePolicy.txt (trunk codeline policy)
> ^/branches/feature/CodelinePolicy.txt
> ^/branches/release/CodelinePolicy.txt
>
> svn ps svn:externals '../CodelinePolicy.txt CodelinePolicy.txt' trunk
>
> would do the trick.  Whenever a new branch is being created:
>
> svn cp ^/trunk ^/branches/feature/new-feature-branch
> svn cp ^/trunk ^/branches/release/new-release-branch
>
> the correct CodelinePolicy would be shown in each WC of the new
> branches.
>
> Currently, this would cause non-inheritable mergeinfo on the root
> directory of each branch so that a reintegrate merge of a feature branch
> to the trunk will fail with "Missing ranges".
>
> Example:
>
> $ svn -q --version
> 1.6.12
>
> $ svn mkdir trunk trunk/dir
> A         trunk
> A         trunk\dir
>
> $ echo x >trunk/x.txt
>
> $ echo d >trunk/dir/d.txt
>
> $ svn add trunk/x.txt trunk/dir/d.txt
> A         trunk\x.txt
> A         trunk\dir\d.txt
>
> $ svn ci -m 'initial trunk'
> Adding         trunk
> Adding         trunk\dir
> Adding         trunk\dir\d.txt
> Adding         trunk\x.txt
> Transmitting file data ..
> Committed revision 1.
>
> $ echo e >e.txt
>
> $ svn add e.txt
> A         e.txt
>
> $ svn ps svn:externals '../e.txt r.txt' trunk
> property 'svn:externals' set on 'trunk'
>
> $ svn ci -m 'file external r.txt on trunk referring to e.txt in parent dir'
> Adding         e.txt
> Sending        trunk
> Transmitting file data .
> Committed revision 2.
>
> $ svn cp trunk branch
> A         branch
>
> $ svn ci -m 'branch created from trunk'
> Adding         branch
> Adding         branch\dir
> Adding         branch\x.txt
>
> Committed revision 3.
>
> $ svn up
>
> Fetching external item into 'trunk\r.txt'
> E    trunk\r.txt
> Updated external to revision 3.
>
>
> Fetching external item into 'branch\r.txt'
> E    branch\r.txt
> Updated external to revision 3.
>
> Updated to revision 3.
>
> $ cd trunk
>
> $ echo xx >>x.txt
>
> $ svn ci -m 'trunk/x.txt changed'
> Sending        trunk\x.txt
> Transmitting file data .
> Committed revision 4.
>
> $ cd ../branch
>
> $ svn merge ^/trunk
> --- Merging r3 through r4 into 'x.txt':
> U    x.txt
>
> $ svn st
>  M      .
>  M  X   r.txt
> MM      x.txt
>  M      dir
>
> $ svn diff
>
> Property changes on: .
> ___
> Added: svn:mergeinfo
>   Merged /trunk:r3-4*
>
>
> Property changes on: r.txt
> ___
> Added: svn:mergeinfo
>   Merged /trunk/r.txt:r3-4
>
> Index: x.txt
> ===
> --- x.txt       (revision 3)
> +++ x.txt       (working copy)
> @@ -1 +1,2 @@
>  x
> +xx
>
> Property changes on: x.txt
> ___
> Added: svn:mergeinfo
>   Merged /trunk/x.txt:r3-4
>
>
> Property changes on: dir
> ___
> Added: svn:mergeinfo
>   Merged /trunk/dir:r3-4
>
>
> $ svn ci -m 'merge of trunk to branch, strange mergeinfo'
> Sending        branch
> Sending        branch\dir
> Sending        branch\x.txt
> Sending        branch\r.txt
> Transmitting file data .
> Committed revision 5.
>
> $ svn diff -c 5
> Index: x.txt
> ===
> --- x.txt      

Re: [RFC] Incremental dumps and mergeinfo (Was: Vetoing latest issue #3020 fix in 1.6.10)

[Paul Burba]

Sweeping through my TODO list turned up this thread.

I'll commit this last patch tomorrow if there are no objections.

I pause a day only because I recall Mike saying he was planning to
take a look at this.

Paul

On Tue, May 11, 2010 at 4:00 PM, Paul Burba  wrote:
> On Tue, Apr 20, 2010 at 11:08 PM, C. Michael Pilato  
> wrote:
>> Paul Burba wrote:
>>> Hi Mike and Julian,
>>>
>>> Sorry for the delayed response; been working on/thinking about other
>>> mergey stuff lately...
>>>
>>> A few inline comments and then a proposed course of action at the end.
>>>
>>> On Thu, Apr 15, 2010 at 9:28 AM, C. Michael Pilato  
>>> wrote:
 The question then becomes, "How do users deal with legitimate partial dumps
 that will be loaded atop something other than loads from previous
 incremental windows?"  I think they do this the same way they handle the
 copy case:  either hand-hacking the dumpstream or using 'svndumpfilter'.  
 So
 maybe 'svndumpfilter' grows the logic and options required to pare away
 mergeinfo that doesn't meet some criteria (such as "ranges outside the dump
 stream range")?
>>>
>>> I vote for svndumpfilter growing the logic to filter ranges predating
>>> the start of the dump stream in the same way it strips mergeinfo with
>>> merge sources filtered from the dump stream.  Basically this means
>>> moving the r927243 functionality to svndumpfilter.  No really sure we
>>> need a new option, seems we could overload
>>> --skip-missing-merge-sources to include this behavior.  Thoughts?
>>
>> +1
>>
 Julian Foad wrote:
> On Wed, 2010-04-14, C. Michael Pilato wrote:
>> Assuming similar behavior for mergeinfo handling, we have, at a minimum:
>>
>> 1.  'svnadmin dump' warns when it is in incremental mode and must 
>> generate
>> mergeinfo from a merge source that predates the beginning of the dump
>> window, but it's only a warning and the dump continues.
>>>
>>> Why only --incremental mode?  We should warn for both --incremental
>>> dumps *and* non-incremental dumps because in the latter case we might
>>> be specifying a dump range -rX:Y where rX>0* and there might be
>>> mergeinfo that refers to revs < X.  And while we might want to be
>>> clever about skipping the warning when no range is specified (i.e.
>>> defaulting to -r0:HEAD), even then we should warn because issue #3020
>>> might already have resulted in a r1 with mergeinfo in the starting
>>> repository.
>>
>> You're right, of course.  I was thinking "partial dump" and saying
>> "incremental".  My bad.
>>
>> 2.  'svnadmin load' does nothing smart, trusting that the dump it's being
>> fed is a sensible one.
> Better: 'svnadmin load' tries to validate the mergeinfo, and issues a
> warning if it refers to a non-existent source.  The admin then gets to
> figure out whether it was bad in the first place or because of his/her
> partial-dump/partial-load scenario.
>
> However, it depends how efficient the checking is.  If that would make
> the 'load' really slow, I can see that not being wanted.
>>
>> [...]
>>
>>> No unsurprisingly, this took significantly (62%) longer, 00:05:39.609.
>>
>> (My vote was for 'svnadmin load' does nothing smart, and I stand by it.)
>>
>>> Taking what you've had to say into consideration, I propose the following:
>>>
>>> 1) Revert http://svn.apache.org/viewvc?view=revision&revision=927243.
>>> This stops svnadmin load from automatically filtering mergeinfo
>>> referring to revisions outside of the load stream.
>>>
>>> 2) Reapply or reimplement the part of r327243 that accounts for the
>>> case where the first loaded revision in a load stream has mergeinfo,
>>> see http://subversion.tigris.org/issues/show_bug.cgi?id=3020#desc10
>>> item #1.
>>
>> (r927243, you mean.)
>>
>>> 3) Keep the inline copy-source warnings as they exist today in
>>> svnadmin dump, but add a new end-of-process summary warning e.g.:
>>>
>>>   * Dumped revision 504.
>>>   * Dumped revision 505.
>>>   * Dumped revision 506.
>>>   * Dumped revision 507.
>>>   WARNING: Referencing data in revision 250, which is older than the oldest
>>>   WARNING: dumped revision (504).  Loading this dump into an empty 
>>> repository
>>>   WARNING: will fail.
>>>   * Dumped revision 58.
>>>   * Dumped revision 59.
>>>   * Dumped revision 510.
>>>   * Dumped revision 511.
>>>   .
>>>   .
>>>   .
>>>   WARNING: The range of revisions dumped contained references to copy
>>> sources outside that range.
>>>
>>> 4) Implement inline and summary functionality analogous to 3) but for
>>> mergeinfo ranges outside of the dump stream, e.g.:
>>>
>>>   * Dumped revision 504.
>>>   * Dumped revision 505.
>>>   * Dumped revision 506.
>>>   * Dumped revision 507.
>>>   WARNING: Mergeinfo referencing revision(s) prior to revision 250,
>>> which is older
>>>   WARNING: than the oldest dumped revision (504).  Loading this dump may 
>>> result
>>>   WARNING: in invalid mergeinfo.
>>>   * Dumped revis

Re: svn commit: r981989 - /subversion/trunk/subversion/libsvn_wc/status.c

[Greg Stein]

On Tue, Aug 3, 2010 at 14:02,   wrote:
> Author: rhuijben
> Date: Tue Aug  3 18:02:00 2010
> New Revision: 981989
>
> URL: http://svn.apache.org/viewvc?rev=981989&view=rev
> Log:
> * subversion/libsvn_wc/status.c
>  (get_dir_status): Following up on r981893, remove !Single-DB block and an
>     inaccurate comment. Hidden is not a parent-stub status.

I'm not sure this message is correct. Hidden can certainly be a
parent-stub status. For a not-present directory (entry->deleted), it
is "hidden".

I suspect in this case, wc_db reads the stub and returns not-present.
But it *is* reading the stub, so maybe  your log message should say
something different? (and verify the code *is* doing what I
hypothesize)

Cheers,
-g

Why doesn't svn_ra_do_diff3() provide a reporter that supplies copyfrom info?

[Daniel Näslund]

Hi!

Why doesn't svn_ra_do_diff3() provide a reporter that supplies copyfrom
info? svn_ra_do_update2() does, so why doesn't the diff func?

Is it based on a notion of diffs as beeing text compares and thus not
interrested in tree changes such as copies/moves?

Is there something in the FS layer that doesn't allow us to fetch
copyfrom info?

Thanks,
Daniel

Re: svn commit: r981885 - /subversion/trunk/subversion/tests/cmdline/resolved_tests.py

[Hyrum K. Wright]

I understand and agree with Greg that we may want to be a bit more
explanatory in commit comments.

That being said, out test suite has a nasty habit of itself relying
too deeply upon implementation details.  Historically, when you
recursively remove a directory, the directories have to stick around
(so you have a .svn in which to record the fact that they've been
deleted), which is strictly an implementation detail.  The fact is
that the test suite cares too much about such things, hence the need
to update the expectations in this (and other similar cases).

Oh, and never mind the fact that we conflate a number of different
issues in the test suite.  Was the test a performance test, regression
test, acceptance test, or some other kind of test?  We don't record
that information, nor do we separate the various types in the test
suite, which makes knowing whether a change like this is valid even
harder.

So, I agree that a bit more explanation is in order, but I partly
blame the test suite that such explanation is even required.

-Hyrum

On Tue, Aug 3, 2010 at 12:44 PM, Greg Stein  wrote:
> These are *regression* tests. You can't just make them pass by changing them.
>
> Let's see some rationale!! Why should the output change? And if it
> does, then there better be some commentary about WHY that is. I see no
> comments about why this is allowed to change. No explanation. No
> nothing.
>
> The tests should produce the exact same result. That is why they are
> there. To ensure we haven't buggered something up.
>
> Any time the output is supposed to be different now, then we need a
> full explanation on why that has happened. We may need to write an
> errata. We may need to update documentation. Or more likely, we need
> to fix some bugs.
>
> Why are all these tests changing? This doesn't seem right.
>
> -g
>
> On Tue, Aug 3, 2010 at 09:56,   wrote:
>> Author: philip
>> Date: Tue Aug  3 13:56:58 2010
>> New Revision: 981885
>>
>> URL: http://svn.apache.org/viewvc?rev=981885&view=rev
>> Log:
>> Make resolved_tests.py 1 pass in single-db.
>>
>> * subversion/tests/cmdline/switc_tests.py
>>  (resolved_on_wc_root): Tweak expectations for single-db.
>>
>> Modified:
>>    subversion/trunk/subversion/tests/cmdline/resolved_tests.py
>>
>> Modified: subversion/trunk/subversion/tests/cmdline/resolved_tests.py
>> URL: 
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/resolved_tests.py?rev=981885&r1=981884&r2=981885&view=diff
>> ==
>> --- subversion/trunk/subversion/tests/cmdline/resolved_tests.py (original)
>> +++ subversion/trunk/subversion/tests/cmdline/resolved_tests.py Tue Aug  3 
>> 13:56:58 2010
>> @@ -112,6 +112,8 @@ def resolved_on_wc_root(sbox):
>>                        'A/B/lambda',
>>                        'A/B/E/alpha', 'A/B/E/beta',
>>                        'A/D/gamma')
>> +  if svntest.main.wc_is_singledb(sbox.wc_dir):
>> +    expected_disk.remove('A/B/E', 'A/B/F', 'A/B')
>>
>>   expected_status = svntest.actions.get_virginal_state(wc, 2)
>>   expected_status.tweak('iota', 'A/B', 'A/D/gamma',
>>
>>
>>
>

Re: svn commit: r981905 - /subversion/trunk/subversion/tests/cmdline/depth_tests.py

[Greg Stein]

On Tue, Aug 3, 2010 at 13:41, Julian Foad  wrote:
> Greg Stein wrote:
>> Why would this be true only in single-db? Shouldn't the test perform
>> exactly the same in both cases? I see no rationale for WHY these
>> should differ.
>>
>> On Tue, Aug 3, 2010 at 10:56,   wrote:
>> > Make depth_tests.py 37-40 pass in single-db.
>> >
>> > * subversion/tests/cmdline/switc_tests.py
>> >  (make_depth_tree_conflicts): Tweak expectations for single-db.
> [...]
>> >   expected_disk.remove('A/mu',
>> >                        'A/B', 'A/B/lambda', 'A/B/E/alpha', 'A/B/E/beta',
>> >                        'A/D/gamma');
>> > +  if svntest.main.wc_is_singledb(sbox.wc_dir):
>> > +    expected_disk.remove('A/B/E', 'A/B/F')
>
> It's just because in single-db mode, deleted dirs really go away from
> disk, isn't it?

Beats me. Where is the comment to that effect? Where is the
explanation in the log message?

Why are these tests allowing *variant* outputs without any explanation?

-g

Re: svn commit: r981839 - in /subversion/trunk/subversion/tests/cmdline: stat_tests.py svntest/wc.py

[Greg Stein]

On Tue, Aug 3, 2010 at 08:18,   wrote:
> Author: philip
> Date: Tue Aug  3 12:18:56 2010
> New Revision: 981839
>
> URL: http://svn.apache.org/viewvc?rev=981839&view=rev
> Log:
> In single-db the added/normal state of the node is available for missing
> directories, while in per-directory it is simply missing.
>
> * subversion/tests/cmdline/svntest/wc.py
>  (missing_dir_in_anchor): Adjust expected status in single-db.
>
> * subversion/tests/cmdline/stat_tests.py
>  (tweak_for_entries_compare): Use entry_status.
>  (StateItem): Add .entry_status.

This kind of change is better. We can demonstrate precisely where our
new code varies from the old code, and produces a different output.

Cheers,
-g

Re: svn commit: r981789 - /subversion/trunk/subversion/libsvn_wc/workqueue.c

[Greg Stein]

On Tue, Aug 3, 2010 at 05:30,   wrote:
> Author: rhuijben
> Date: Tue Aug  3 09:30:08 2010
> New Revision: 981789
>
> URL: http://svn.apache.org/viewvc?rev=981789&view=rev
> Log:
> * subversion/libsvn_wc/workqueue.c
>  (run_deletion_postcommit): Insert the absent node with the right kind.
>    (pre single-db this code is only used for files)

And why does it change in single-db mode? This goes back to my other
question: it shouldn't matter outside of wc_db. What concept leaked
such that this is called for directories, too?

Cheers,
-g

Re: svn commit: r981885 - /subversion/trunk/subversion/tests/cmdline/resolved_tests.py

[Greg Stein]

These are *regression* tests. You can't just make them pass by changing them.

Let's see some rationale!! Why should the output change? And if it
does, then there better be some commentary about WHY that is. I see no
comments about why this is allowed to change. No explanation. No
nothing.

The tests should produce the exact same result. That is why they are
there. To ensure we haven't buggered something up.

Any time the output is supposed to be different now, then we need a
full explanation on why that has happened. We may need to write an
errata. We may need to update documentation. Or more likely, we need
to fix some bugs.

Why are all these tests changing? This doesn't seem right.

-g

On Tue, Aug 3, 2010 at 09:56,   wrote:
> Author: philip
> Date: Tue Aug  3 13:56:58 2010
> New Revision: 981885
>
> URL: http://svn.apache.org/viewvc?rev=981885&view=rev
> Log:
> Make resolved_tests.py 1 pass in single-db.
>
> * subversion/tests/cmdline/switc_tests.py
>  (resolved_on_wc_root): Tweak expectations for single-db.
>
> Modified:
>    subversion/trunk/subversion/tests/cmdline/resolved_tests.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/resolved_tests.py
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/resolved_tests.py?rev=981885&r1=981884&r2=981885&view=diff
> ==
> --- subversion/trunk/subversion/tests/cmdline/resolved_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/resolved_tests.py Tue Aug  3 
> 13:56:58 2010
> @@ -112,6 +112,8 @@ def resolved_on_wc_root(sbox):
>                        'A/B/lambda',
>                        'A/B/E/alpha', 'A/B/E/beta',
>                        'A/D/gamma')
> +  if svntest.main.wc_is_singledb(sbox.wc_dir):
> +    expected_disk.remove('A/B/E', 'A/B/F', 'A/B')
>
>   expected_status = svntest.actions.get_virginal_state(wc, 2)
>   expected_status.tweak('iota', 'A/B', 'A/D/gamma',
>
>
>

Re: svn commit: r981905 - /subversion/trunk/subversion/tests/cmdline/depth_tests.py

[Julian Foad]

Greg Stein wrote:
> Why would this be true only in single-db? Shouldn't the test perform
> exactly the same in both cases? I see no rationale for WHY these
> should differ.
> 
> On Tue, Aug 3, 2010 at 10:56,   wrote:
> > Make depth_tests.py 37-40 pass in single-db.
> >
> > * subversion/tests/cmdline/switc_tests.py
> >  (make_depth_tree_conflicts): Tweak expectations for single-db.
[...]
> >   expected_disk.remove('A/mu',
> >'A/B', 'A/B/lambda', 'A/B/E/alpha', 'A/B/E/beta',
> >'A/D/gamma');
> > +  if svntest.main.wc_is_singledb(sbox.wc_dir):
> > +expected_disk.remove('A/B/E', 'A/B/F')

It's just because in single-db mode, deleted dirs really go away from
disk, isn't it?

- Julian

Re: svn commit: r981905 - /subversion/trunk/subversion/tests/cmdline/depth_tests.py

[Greg Stein]

Why would this be true only in single-db? Shouldn't the test perform
exactly the same in both cases? I see no rationale for WHY these
should differ.

On Tue, Aug 3, 2010 at 10:56,   wrote:
> Author: philip
> Date: Tue Aug  3 14:56:08 2010
> New Revision: 981905
>
> URL: http://svn.apache.org/viewvc?rev=981905&view=rev
> Log:
> Make depth_tests.py 37-40 pass in single-db.
>
> * subversion/tests/cmdline/switc_tests.py
>  (make_depth_tree_conflicts): Tweak expectations for single-db.
>
> Modified:
>    subversion/trunk/subversion/tests/cmdline/depth_tests.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/depth_tests.py
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/depth_tests.py?rev=981905&r1=981904&r2=981905&view=diff
> ==
> --- subversion/trunk/subversion/tests/cmdline/depth_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/depth_tests.py Tue Aug  3 
> 14:56:08 2010
> @@ -2442,6 +2442,8 @@ def make_depth_tree_conflicts(sbox):
>   expected_disk.remove('A/mu',
>                        'A/B', 'A/B/lambda', 'A/B/E/alpha', 'A/B/E/beta',
>                        'A/D/gamma');
> +  if svntest.main.wc_is_singledb(sbox.wc_dir):
> +    expected_disk.remove('A/B/E', 'A/B/F')
>
>   # This test is set XFail because this (correct) status cannot be
>   # verified due to an "svn update" bug. The tree-conflict on A/B
>
>
>

Re: svn commit: r981893 - /subversion/trunk/subversion/libsvn_wc/status.c

[Greg Stein]

On Tue, Aug 3, 2010 at 10:13,   wrote:
> Author: rhuijben
> Date: Tue Aug  3 14:13:10 2010
> New Revision: 981893
>
> URL: http://svn.apache.org/viewvc?rev=981893&view=rev
> Log:
> * subversion/libsvn_wc/status.c
>  (get_dir_status): In single-db don't use svn_wc__db_node_hidden, if we
>    already have status.

This seems like a dangerous disparity. It is exposing too much of
SINGLE_DB outside of wc_db.h.

I would very much encourage finding a way to collapse this under the
wc_db covers before continuing this pattern. Ideally, the only
SINGLE_DB conditions should be *inside* wc_db. If not, then something
is probably wrong in that API.

Cheers,
-g

Re: opening fsfs rev files rw

[Daniel Shahaf]

Julian Foad wrote on Tue, Aug 03, 2010 at 17:36:00 +0100:
> On Tue, 2010-08-03 at 18:19 +0300, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Tue, Aug 03, 2010 at 11:58:32 +0300:
> > > There was yesterday on IRC [1] some discussion around whether revprop
> > > editing can corrupt rev files.
> > > 
> > > [[[
> > > 0:% rm -rf r
> > > 0:% ./subversion/svnadmin/svnadmin create r
> > > 0:% ls -l r/db/revs/0/0
> > > -rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0
> > > ]]]
> > > 
> > > Shouldn't we create rev files with the write bit disabled?  (i.e., 0222 
> > > umask)
> > 
> > r981921 + nominated for backport.
> 
> The idea looks sane.
> 
> There's perhaps a problem with revPROPS files: fs_fs.c copies the
> permissions of a revprops file from the corresponding rev file, which
> now means all revprops files will be read-only, which might cause a
> problem when trying to edit a revprop.
> 

Good catch.

As far as I can tell, 'svnadmin setrevprop' and 'svn propset --revprop'
still work and fs-test and fs-pack-test (which test svn_fs_change_rev_prop())
still pass (even though the revprop files are mode 0444), so I'm tempted to
leave the code as-is.

Does anyone see a problem here, or see an error when trying to edit revprops
with this patch applied?

> - Julian
> 
> 

Thanks,

Daniel

Re: [WIP] Fix for issue 2333 (repos-repos diff skips deleted dirs)

[Stephen Butler]

Sending again with a non-null attachment MIME-type...

Steve

Quoting Stephen Butler :


Hi folks,

here's a work in progress for fixing a long-standing diff bug.

  "'svn diff URL1 URL2' not reverse of 'svn diff URL2 URL1'"
  http://subversion.tigris.org/issues/show_bug.cgi?id=2333

The basic approach was suggested by cmpilato in a comment
to the issue.  In repos_diff.c:delete_entry(), I call svn_client_list2(),
handing it a callback that prints diffs for all files in the "old"
tree.  It's a bit awkward to call a client API function in repos_diff,
whose edit_baton was never yet sullied by an svn_client_ctx_t.

The ugliest hack (so far) is in diff_deleted_dir_cb(), the callback.
I need to hand get_file_from_ra() a path relative to the RA
session's URL.

TODOs include:

Find out why diff_tests.py 28 still fails.

Investigate other failing diff tests.

Include dir diffs in the list2-callback.

Handle authz failures gracefully.

Anything else?

Comments welcome.

Steve






--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194


Index: subversion/tests/cmdline/diff_tests.py
===
--- subversion/tests/cmdline/diff_tests.py  (revision 981661)
+++ subversion/tests/cmdline/diff_tests.py  (working copy)
@@ -3533,7 +3533,7 @@ test_list = [ None,
   diff_keywords,
   diff_force,
   diff_schedule_delete,
-  XFail(diff_renamed_dir),
+  diff_renamed_dir,
   diff_property_changes_to_base,
   diff_mime_type_changes,
   diff_prop_change_local_propmod,
Index: subversion/libsvn_client/repos_diff.c
===
--- subversion/libsvn_client/repos_diff.c   (revision 981661)
+++ subversion/libsvn_client/repos_diff.c   (working copy)
@@ -54,6 +54,10 @@ struct edit_baton {
  repository operation. */
   svn_wc_context_t *wc_ctx;
 
+  /* A client context passed to client APIs when fetching missing data
+ during an editor drive.  May be NULL. */
+  svn_client_ctx_t *ctx;
+
   /* The callback and calback argument that implement the file comparison
  function */
   const svn_wc_diff_callbacks4_t *diff_callbacks;
@@ -89,6 +93,14 @@ struct edit_baton {
   svn_wc_notify_func2_t notify_func;
   void *notify_baton;
 
+  /* TRUE if the operation needs to walk deleted dirs on the "old" side.
+ FALSE otherwise. */
+  svn_boolean_t walk_deleted_dirs;
+
+  /* If WALK_DELETED_DIRS is TRUE, the walk callback will need the repos
+ root URL.  Otherwise NULL. */
+  const char *root_url;
+
   apr_pool_t *pool;
 };
 
@@ -443,6 +455,59 @@ open_root(void *edit_baton,
   return SVN_NO_ERROR;
 }
 
+/* This implements the svn_client_list_func_t API.  To be called during
+   URL-URL diffs only.  Prints diff output for each file in a deleted dir
+   in the "old" URL. */
+static svn_error_t *
+diff_deleted_tree_cb(void *baton,
+ const char *path,
+ const svn_dirent_t *dirent,
+ const svn_lock_t *lock,
+ const char *abs_path,
+ apr_pool_t *pool)
+{
+  struct edit_baton *eb = baton;
+
+  if (dirent->kind == svn_node_file)
+{
+  const char *file_relpath, *file_path, *url;
+  struct file_baton *b;
+  const char *mimetype1, *mimetype2;
+  svn_wc_notify_state_t state = svn_wc_notify_state_inapplicable;
+  svn_boolean_t tree_conflicted = FALSE;
+
+  /* Compare a file being deleted against an empty file */
+ 
+  /* ### Hack! get_file_from_ra() expects the file's repository path
+ relative to the RA session's URL. */
+  file_path = svn_dirent_join(abs_path, path, pool);
+  while (*file_path && *file_path == '/')
+file_path++;
+  url = svn_uri_join(eb->root_url, file_path, pool);
+  svn_ra_get_path_relative_to_session(eb->ra_session,
+  &file_relpath,
+  url,
+  pool);
+  b = make_file_baton(file_relpath, FALSE, eb, pool);
+  SVN_ERR(get_file_from_ra(b, eb->revision));
+
+  SVN_ERR(get_empty_file(b->edit_baton, &(b->path_end_revision)));
+  
+  get_file_mime_types(&mimetype1, &mimetype2, b);
+
+  SVN_ERR(eb->diff_callbacks->file_deleted
+  (NULL, &state, &tree_conflicted, b->wcpath,
+   b->path_start_revision,
+   b->path_end_revision,
+   mimetype1, mimetype2,
+   b->pristine_props,
+   b->edit_baton->diff_cmd_baton,
+   pool));
+}
+
+  return SVN_NO_ERROR

Re: svn commit: r966822 - in /subversion/trunk/subversion: libsvn_client/repos_diff.c tests/cmdline/merge_tests.py

[Paul Burba]

Ok, I am waving the white flag as far as implementing a fix for issue
#3657 in the RA layer.  I simply don't see how it can be done outside
of this:  Put the DAV update report handler into send-all=TRUE
 mode when creating a
svn_ra_reporter3_t * during a merge/diff.  This would prevent
 response that causes the client to fetch *all* the
properties of a path which subsequently pushes these to the
svn_delta_editor_t callbacks as if they were all prop *changes* (as
Mike discussed here
http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc9).

fyi: Currently this is how the two DAV RA layers use send-all mode:

   send-all   send-all
   mode   mode
  operationra_serfra_neon
  -   -
  update   FALSE  TRUE
  status   FALSE  TRUE
  switch   FALSE  TRUE
  diff FALSE  FALSE

In that attached patch I reverted r966822 and tried the aforementioned
approach by tweaking ra_neon so the send-all mode on diff/merge is
TRUE.  The whole test suite passes (including the test for issue
#3657).  I haven't got an equivalent change to ra_serf to work yet,
but I'm not too worried about that yet because...

...After testing out this idea, I realized the problem with this
approach: It reopens issue #2048 'Primary connection (of parallel
network connections used) in ra-dav diff/merge timeout
unnecessarily.':

  http://subversion.tigris.org/issues/show_bug.cgi?id=2048
  http://svn.apache.org/viewvc?view=revision&revision=853457

So I'm at a dead-end right now.  I'm happy to revert r966822 and
reopen issue #3657 if the consensus is that my initial fix is a sloppy
band-aid to cover incorrect ra_neon/ra_serf behavior and that the
latter two are where the fix belongs.

Paul

[WIP] Fix for issue 2333 (repos-repos diff skips deleted dirs)

[Stephen Butler]

Hi folks,

here's a work in progress for fixing a long-standing diff bug.

  "'svn diff URL1 URL2' not reverse of 'svn diff URL2 URL1'"
  http://subversion.tigris.org/issues/show_bug.cgi?id=2333

The basic approach was suggested by cmpilato in a comment 
to the issue.  In repos_diff.c:delete_entry(), I call svn_client_list2(),
handing it a callback that prints diffs for all files in the "old"
tree.  It's a bit awkward to call a client API function in repos_diff,
whose edit_baton was never yet sullied by an svn_client_ctx_t.

The ugliest hack (so far) is in diff_deleted_dir_cb(), the callback.
I need to hand get_file_from_ra() a path relative to the RA 
session's URL.

TODOs include:

Find out why diff_tests.py 28 still fails.

Investigate other failing diff tests.

Include dir diffs in the list2-callback.

Handle authz failures gracefully.

Anything else?

Comments welcome.

Steve



--
Stephen Butler | Senior Consultant
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194

Re: opening fsfs rev files rw

[Julian Foad]

On Tue, 2010-08-03 at 18:19 +0300, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Tue, Aug 03, 2010 at 11:58:32 +0300:
> > There was yesterday on IRC [1] some discussion around whether revprop
> > editing can corrupt rev files.
> > 
> > [[[
> > 0:% rm -rf r
> > 0:% ./subversion/svnadmin/svnadmin create r
> > 0:% ls -l r/db/revs/0/0
> > -rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0
> > ]]]
> > 
> > Shouldn't we create rev files with the write bit disabled?  (i.e., 0222 
> > umask)
> 
> r981921 + nominated for backport.

The idea looks sane.

There's perhaps a problem with revPROPS files: fs_fs.c copies the
permissions of a revprops file from the corresponding rev file, which
now means all revprops files will be read-only, which might cause a
problem when trying to edit a revprop.

- Julian

Re: svn commit: r981684 - in /subversion/branches/performance/subversion/libsvn_fs_fs: caching.c fs_fs.h

[Blair Zajac]

On 08/02/2010 01:51 PM, stef...@apache.org wrote:

Author: stefan2
Date: Mon Aug  2 20:51:35 2010
New Revision: 981684

URL: http://svn.apache.org/viewvc?rev=981684&view=rev
Log:
Bring the membuffer cache to its first use for the full text cache.
Also, provide functions to get / set the FSFS cache configuration
although not all of it is supported, yet.



Stefan,

I appreciate you working on this, I'm looking forward to using these 
improvements in production, which is why I'm closely looking at them.



+/* Access to the cache settings as a process-wide singleton. */
+static svn_fs_fs__cache_config_t *
+internal_get_cache_config(void)
+{
+  static svn_fs_fs__cache_config_t settings =
+{
+  /* default configuration:
+   */
+  0x800,   /* 128 MB for caches */
+  16,  /* up to 16 files kept open */
+  FALSE,   /* don't cache fulltexts */
+  FALSE,   /* don't cache text deltas */
+  FALSE/* assume multi-threaded operation */
+};
+
+  return&settings;
+}



Why not make "settings" static at file scope instead of inside this 
function?  What does this function provide for us?





+/* Access the process-global (singleton) membuffer cache. The first call
+ * will automatically allocate the cache using the current cache config.
+ * NULL will be returned if the desired cache size is 0.
+ */
+static svn_membuffer_t *
+get_global_membuffer_cache(void)
+{
+  static svn_membuffer_t *cache = NULL;
+
+  apr_uint64_t cache_size = svn_fs_fs__get_cache_config()->cache_size;
+  if (!cache&&  cache_size)


s/cache&&/cache &&/


+{
+  /* auto-allocate cache*/
+  apr_allocator_t *allocator = NULL;
+  apr_pool_t *pool = NULL;
+
+  if (apr_allocator_create(&allocator))
+return NULL;
+
+  /* ensure that a we free partially allocated data if we run OOM



s/a we/we/



+   * before the cache is complete.
+   */
+  apr_allocator_max_free_set(allocator, 1);
+  pool = svn_pool_create_ex(NULL, allocator);
+
+  svn_cache__membuffer_cache_create
+  (&cache,
+   cache_size,
+   cache_size / 16,
+   ! svn_fs_fs__get_cache_config()->single_threaded,
+   pool);
+}


So the allocator is associated with this pool forever then?  So it 
cannot be destroyed after



-ffd->fulltext_cache = NULL;
+{
+  ffd->fulltext_cache = NULL;
+}
+
+  if (ffd->fulltext_cache&&  ! no_handler)


s/fulltext_cache&&  ! no_handler/fulltext_cache && ! no_handler


+SVN_ERR(svn_cache__set_error_handler(ffd->fulltext_cache,
+warn_on_cache_errors, fs, pool));

return SVN_NO_ERROR;
  }

Modified: subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h
URL: 
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h?rev=981684&r1=981683&r2=981684&view=diff
==
--- subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h Mon Aug  2 
20:51:35 2010
@@ -523,6 +523,39 @@ svn_fs_fs__get_node_origin(const svn_fs_
  svn_error_t *
  svn_fs_fs__initialize_caches(svn_fs_t *fs, apr_pool_t *pool);

+/* FSFS cache settings. It controls what caches, in what size and
+   how they will be created. The settomgs apply for the whole process.


s/settomgs/settings/


+ */
+typedef struct svn_fs_fs__cache_config_t
+{
+  /* total cache size in bytes. May be 0, resulting in no cache being used */
+  apr_uint64_t cache_size;
+
+  /* maximum number of files kept open */
+  apr_size_t file_handle_count;
+
+  /* shall fulltexts be cached? */
+  svn_boolean_t cache_fulltexts;
+
+  /* shall text deltas be cached? */
+  svn_boolean_t cache_txdeltas;
+
+  /* is this a guaranteed single-threaded application? */
+  svn_boolean_t single_threaded;
+} svn_fs_fs__cache_config_t;
+
+/* Get the current FSFS cache configuration. If it has not been set,
+   yet, this function will return the default settings.



s/yet,//



+ */
+const svn_fs_fs__cache_config_t *
+svn_fs_fs__get_cache_config(void);
+
+/* Set the FSFS cache configuration. Please note that it may not change
+   the actual configuration *in use*. Therefore, call it before reading
+   data from any FSFS repo and call it only once.


Should also document that this is not multi-threaded safe and does not 
protect against races, just as you do at the functions implementation.



+ */
+void
+svn_fs_fs__set_cache_config(svn_fs_fs__cache_config_t *settings);


Instead of

  svn_fs_fs__cache_config_t *settings

make it const:

  const svn_fs_fs__cache_config_t *settings

Regards,
Blair

Re: [patch] fsfs revprop packing paranoia

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, Aug 02, 2010 at 23:44:10 +0300:
> Is this necessary to avoid some race conditions around a revprop becoming
> packed just before commit_body()'s write-lock had been acquired?
> 
> [[[
> Index: fs_fs.c
> ===
> --- fs_fs.c (revision 981659)
> +++ fs_fs.c (working copy)
> @@ -6094,6 +6094,7 @@ commit_body(void *baton, apr_pool_t *pool)
>SVN_ERR(svn_fs_fs__change_txn_prop(cb->txn, SVN_PROP_REVISION_DATE,
>   &date, pool));
>  
> +  SVN_ERR(update_min_unpacked_revprop(fs, pool));
>if (ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
>new_rev >= ffd->min_unpacked_revprop)
>  {
> ]]]
> 

Okay, sorry.  I should have added that call to set_revision_proplist() (which
is run under the write lock *during a revprop edit*), not where I did.

That said, my question still stands whether the following is necessary:

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===
--- subversion/libsvn_fs_fs/fs_fs.c (revision 981921)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -2781,6 +2789,7 @@ set_revision_proplist(svn_fs_t *fs,
 
   SVN_ERR(ensure_revision_exists(fs, rev, pool));
 
+  SVN_ERR(update_min_unpacked_revprop(fs, pool));
   if (ffd->format < SVN_FS_FS__MIN_PACKED_REVPROP_FORMAT ||
   rev >= ffd->min_unpacked_revprop)
 {
]]]


And, to be honest, I think I'd rather fix this once and for all by updating
the ffd->min_unpacked_* caches whenever a write-lock is obtained anywhere: 

[[[
Index: subversion/libsvn_fs_fs/fs_fs.c
===
--- subversion/libsvn_fs_fs/fs_fs.c (revision 981921)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -150,6 +150,9 @@ read_min_unpacked_rev(svn_revnum_t *min_unpacked_r
 static svn_error_t *
 update_min_unpacked_rev(svn_fs_t *fs, apr_pool_t *pool);
 
+static svn_error_t *
+update_min_unpacked_revprop(svn_fs_t *fs, apr_pool_t *pool);
+
 /* Pathname helper functions */
 
 /* Return TRUE is REV is packed in FS, FALSE otherwise. */
@@ -567,7 +570,8 @@ get_lock_on_filesystem(const char *lock_filename,
BATON and that subpool, destroy the subpool (releasing the write
lock) and return what BODY returned. */
 static svn_error_t *
-with_some_lock(svn_error_t *(*body)(void *baton,
+with_some_lock(svn_fs_t *fs,
+   svn_error_t *(*body)(void *baton,
 apr_pool_t *pool),
void *baton,
const char *lock_filename,
@@ -594,7 +598,11 @@ static svn_error_t *
   err = get_lock_on_filesystem(lock_filename, subpool);
 
   if (!err)
-err = body(baton, subpool);
+{
+  SVN_ERR(update_min_unpacked_rev(fs, pool));
+  SVN_ERR(update_min_unpacked_revprop(fs, pool));
+  err = body(baton, subpool);
+}
 
   svn_pool_destroy(subpool);
 
@@ -622,7 +630,7 @@ svn_fs_fs__with_write_lock(svn_fs_t *fs,
   apr_thread_mutex_t *mutex = ffsd->fs_write_lock;
 #endif
 
-  return with_some_lock(body, baton,
+  return with_some_lock(fs, body, baton,
 path_lock(fs, pool),
 #if SVN_FS_FS__USE_LOCK_MUTEX
 mutex,
@@ -645,7 +653,7 @@ with_txn_current_lock(svn_fs_t *fs,
   apr_thread_mutex_t *mutex = ffsd->txn_current_lock;
 #endif
 
-  return with_some_lock(body, baton,
+  return with_some_lock(fs, body, baton,
 path_txn_current_lock(fs, pool),
 #if SVN_FS_FS__USE_LOCK_MUTEX
 mutex,
]]]

Re: opening fsfs rev files rw

[Daniel Shahaf]

Daniel Shahaf wrote on Tue, Aug 03, 2010 at 11:58:32 +0300:
> There was yesterday on IRC [1] some discussion around whether revprop
> editing can corrupt rev files.
> 
> [[[
> 0:% rm -rf r
> 0:% ./subversion/svnadmin/svnadmin create r
> 0:% ls -l r/db/revs/0/0
> -rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0
> ]]]
> 
> Shouldn't we create rev files with the write bit disabled?  (i.e., 0222 umask)
> 

r981921 + nominated for backport.

Re: [PATCH] Add a '--fsfs-no-repsharing' flag to svnadmin

[Daniel Shahaf]

Simon Atanasyan wrote on Tue, Aug 03, 2010 at 12:28:20 +0400:
> On Mon, Aug 2, 2010 at 23:04, Daniel Shahaf  wrote:
> > Simon Atanasyan wrote on Mon, Aug 02, 2010 at 19:59:31 +0400:
> >> From the programmer's point of view if you create repository calling
> >> svn_fs_create with the new configuration option you easily get well 
> >> commented
> >> db/fsfs.conf with necessary settings.
> >
> > svn_fs_create() already takes an FS_CONFIG parameter.  We could populate the
> > initially-created fsfs.conf with values from that parameter...
> 
> If I understand you correctly my patch does exactly the same things.
> It checks file system configuration options in the write_config
> function (called from svn_fs_fs__create) and generates corresponded
> db/fsfs.conf.
> 

The API part of the patch might be useful then.

Perhaps the patch would face more friendly winds if you retained the API
bit, but replaced --fsfs-no-rep-sharing by some more generic --config-option
flag?  (compare 'svn --config-option')

Daniel
(you may want to wait for others' opinions before implementing what I just said)

> -- 
> Simon Atanasyan
> VisualSVN Limited

Re: svn commit: r981828 - in /subversion/branches/performance/subversion: include/private/svn_file_handle_cache.h include/private/svn_temp_serializer.h libsvn_subr/svn_file_handle_cache.c libsvn_sub

[Hyrum K. Wright]

I this is what I get for not reading the entire identifier.  I
apologize for the bad advice; I missed the second '__' farther down in
the symbol names.

-Hyrum

On Tue, Aug 3, 2010 at 6:41 AM,   wrote:
> Author: stefan2
> Date: Tue Aug  3 11:41:16 2010
> New Revision: 981828
>
> URL: http://svn.apache.org/viewvc?rev=981828&view=rev
> Log:
> Revert r981665 because there is no such naming convention.
> I guess it is my fault when I blindly follow advise .. *sigh*
>
> Modified:
>    
> subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
>    (contents, props changed)
>    
> subversion/branches/performance/subversion/include/private/svn_temp_serializer.h
>    (contents, props changed)
>    
> subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c
>    (contents, props changed)
>    
> subversion/branches/performance/subversion/libsvn_subr/svn_temp_serializer.c  
>  (contents, props changed)
>
> Modified: 
> subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h?rev=981828&r1=981827&r2=981828&view=diff
> ==
> --- 
> subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
>  (original)
> +++ 
> subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
>  Tue Aug  3 11:41:16 2010
> @@ -20,7 +20,7 @@
>  * 
>  * @endcopyright
>  *
> - * @file svn__file_handle_cache.h
> + * @file svn_file_handle_cache.h
>  * @brief File handle cache API
>  */
>
> @@ -30,14 +30,14 @@
>  /**
>  * An opaque structure representing a cache for open file handles.
>  */
> -typedef struct svn__file_handle_cache_t svn__file_handle_cache_t;
> +typedef struct svn_file_handle_cache_t svn_file_handle_cache_t;
>
>  /**
>  * An opaque structure representing a cached file handle being used
>  * by the calling application.
>  */
>  typedef
> -struct svn__file_handle_cache__handle_t svn__file_handle_cache__handle_t;
> +struct svn_file_handle_cache__handle_t svn_file_handle_cache__handle_t;
>
>  /**
>  * Get an open file handle in @a f, for the file named @a fname with the
> @@ -51,14 +51,14 @@ struct svn__file_handle_cache__handle_t
>  * returned is undefined.
>  */
>  svn_error_t *
> -svn__file_handle_cache__open(svn__file_handle_cache__handle_t **f,
> -                             svn__file_handle_cache_t *cache,
> -                             const char *fname,
> -                             apr_int32_t flag,
> -                             apr_fileperms_t perm,
> -                             apr_off_t offset,
> -                             int cookie,
> -                             apr_pool_t *pool);
> +svn_file_handle_cache__open(svn_file_handle_cache__handle_t **f,
> +                            svn_file_handle_cache_t *cache,
> +                            const char *fname,
> +                            apr_int32_t flag,
> +                            apr_fileperms_t perm,
> +                            apr_off_t offset,
> +                            int cookie,
> +                            apr_pool_t *pool);
>
>  /**
>  * Efficiently check whether the file handle cache @a cache holds an open
> @@ -67,8 +67,8 @@ svn__file_handle_cache__open(svn__file_h
>  * that the respective file does not exist.
>  */
>  svn_boolean_t
> -svn__file_handle_cache__has_file(svn__file_handle_cache_t *cache,
> -                                 const char *fname);
> +svn_file_handle_cache__has_file(svn_file_handle_cache_t *cache,
> +                                const char *fname);
>
>  /**
>  * Return the APR level file handle underlying the cache file handle @a f.
> @@ -76,7 +76,7 @@ svn__file_handle_cache__has_file(svn__fi
>  * invalidated.
>  */
>  apr_file_t *
> -svn__file_handle_cache__get_apr_handle(svn__file_handle_cache__handle_t *f);
> +svn_file_handle_cache__get_apr_handle(svn_file_handle_cache__handle_t *f);
>
>  /**
>  * Return the name of the file that the cached handle @a f refers to.
> @@ -84,7 +84,7 @@ svn__file_handle_cache__get_apr_handle(s
>  * invalidated.
>  */
>  const char *
> -svn__file_handle_cache__get_name(svn__file_handle_cache__handle_t *f);
> +svn_file_handle_cache__get_name(svn_file_handle_cache__handle_t *f);
>
>  /**
>  * Return the cached file handle @a f to the cache. Depending on the number
> @@ -92,13 +92,13 @@ svn__file_handle_cache__get_name(svn__fi
>  * is NULL, already closed or an invalidated handle, this is a no-op.
>  */
>  svn_error_t *
> -svn__file_handle_cache__close(svn__file_handle_cache__handle_t *f);
> +svn_file_handle_cache__close(svn_file_handle_cache__handle_t *f);
>
>  /**
>  * Close all file handles currently not held by the application.
>  */
>  svn_error_t *
> -svn__file_handle_cache__flush(svn__file_

Re: [PATCH] Fix a historical detail in the commit editor

[Ramkumar Ramachandra]

Hi Bert,

Bert Huijben writes:
> But I think you forget that before creating a commit editor that user had to
> setup an ra session. And every ra session allows retrieving the repository
> root (in most cases it is even cached before the first ra operation). So you
> can just store this in the initial commit editor baton.
> 
> I think this is a compromise between keeping an option on allowing to record
> copies from different repositories (maybe compatibility with DeltaV?) and
> otherwise the option to hide the real repository root. (We now need it in
> several places so it is available everywhere, but around 1.0 the repository
> root was mostly optional information).
> 
> 
> But if we would rev this function today, I would recommend switching to a
> repos_relpath. 
> (And we should certainly check how we are going to handle this case with the
> new function of automatic relocation).

Thanks for the explanation. Fixed along with other minor things in
r981876.

-- Ram

Re: NODE_DATA (2nd iteration)

[Julian Foad]

On Mon, 2010-07-12, Erik Huelsmann wrote:
> After lots of discussion regarding the way NODE_DATA/4th tree should
> be working, I'm now ready to post a summary of the progress. In my
> last e-mail (http://svn.haxx.se/dev/archive-2010-07/0262.shtml) I
> stated why we need this; this post is about the conclusion of what
> needs to happen. Also included are the first steps there.
> 
> 
> With the advent of NODE_DATA, we distinguish node values specifically
> related to BASE nodes, those specifically related to "current" WORKING
> nodes and those which are to be maintained for multiple levels of
> WORKING nodes (not only the "current" view) (the latter category is
> most often also shared with BASE).
> 
> The respective tables will hold the columns shown below.
> 
> 
> -
> TABLE WORKING_NODE (
>   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
>   local_relpath  TEXT NOT NULL,
>   parent_relpath  TEXT,
>   moved_here  INTEGER,
>   moved_to  TEXT,
>   original_repos_id  INTEGER REFERENCES REPOSITORY (id),
>   original_repos_path  TEXT,
>   original_revnum  INTEGER,
>   translated_size  INTEGER,
>   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
>   keep_local  INTEGER,
> 
>   PRIMARY KEY (wc_id, local_relpath)
>   );
> 
> CREATE INDEX I_WORKING_PARENT ON WORKING_NODE (wc_id, parent_relpath);
> 
> 
> The moved_* and original_* columns are typical examples of "WORKING
> fields only maintained for the visible WORKING nodes": the original_*
> and moved_* fields are inherited from the operation root by all
> children part of the operation. The operation root will be the visible
> change on its own level, meaning it'll have rows both in the
> WORKING_NODE and NODE_DATA tables. The fact that these columns are not
> in the WORKING_NODE table means that tree changes are not preserved
> accros overlapping changes. This is fully compatible with what we do
> today: changes to higher levels destroy changes to lower levels.
> 
> The translated_size and last_mod_time columns exist in WORKING_NODE
> and BASE_NODE; they explicitly don't exist in NODE_DATA. The fact that
> they exist in BASE_NODE is a bit of a hack: it's to prevent creation
> of WORKING_NODE data for every file which has keyword expansion or eol
> translation properties set: these columns serve only to optimize
> working copy scanning for changes and as such only relate to the
> visible WORKING_NODEs.
> 

Can we come up with an English description of what each table will now
represent?

"The BASE_NODE table lists the existing node-revs in the repository that
comprise the mixed-revision tree that was most recently updated/switched
to or checked out.  (The kind and content of these nodes is not here;
see the NODE_DATA table.)"

>  TABLE BASE_NODE (
>   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
>   local_relpath  TEXT NOT NULL,
>   repos_id  INTEGER REFERENCES REPOSITORY (id),
>   repos_relpath  TEXT,

We need a revision number column here to go along with repos_id and
relpath to make a valid node-rev reference, don't we?

>   parent_relpath  TEXT,

(While we're reorganising, can we move that "parent_relpath" column to
adjacent to "local_relpath"?)

>   translated_size  INTEGER,
>   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
>   dav_cache  BLOB,
>   incomplete_children  INTEGER,
>   file_external  TEXT,
> 
>   PRIMARY KEY (wc_id, local_relpath)
>   );
> 

"The NODE_DATA table records the kind and shallow content (props, text,
link target) of each node in the WC.  It includes both the nodes that
comprise the currently 'visible' (or 'actual' or 'on-disk') state of the
WC and also all nodes that are part of a copied or moved tree but
currently shadowed by a replacement performed inside that tree.

At least one row exists for each WC path, including paths with no change
and all paths affected by a tree change (add, delete, etc.).  If the
same path is affected by multiple levels of tree change - a replacement
inside a copied directory, for example - then multiple rows exist with
different 'op_depth' values."

> TABLE NODE_DATA (
>   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
>   local_relpath  TEXT NOT NULL,
>   op_depth  INTEGER NOT NULL,
>   presence  TEXT NOT NULL,
>   kind  TEXT NOT NULL,
>   checksum  TEXT,
>   changed_rev  INTEGER,
>   changed_date  INTEGER,  /* an APR date/time (usec since 1970) */
>   changed_author  TEXT,

The changed_* columns can only belong to a node-rev that exists in the
repository.  What node-rev do they belong to and why aren't they
alongside the node-rev details?

(The changed_* columns convey essentially a rev number and two of the
rev-props associated with that revnum that can be used in keyword
expansions.  We should consider representing that information in a more
general form, both to avoid tying the DB format to the choice of those
two particular revprops, and to avoid the redundancy of storing these
same data and author va

Re: NODE_DATA (2nd iteration)

[Julian Foad]

On Tue, 2010-08-03 at 10:12 +0100, Julian Foad wrote:
> On Mon, 2010-08-02, I (Julian Foad) wrote:
> > Hi Erik.
> > 
> > Would you or anybody volunteer to draw a diagram of how these table rows
> > look in various simple-ish WC states?
> 
> Maybe I can help by drawing my best interpretation of it and getting
> your feedback.  I'll have a go.

Take a look at my attempt.  I've started as textual tables rather than a
visual diagram.

.

You're welcome to edit it directly if you email/IRC me and ask.

- Julian



> > I feel stupid saying this, but I haven't yet got much of an idea at all
> > about how a set of database rows will represent a particular collection
> > of repository nodes and local changes in the new scheme.  I know roughly
> > what the aim is (to be able to represent nested tree changes more
> > flexibly), and I can read what elements of data will be stored in each
> > table, but I am missing the part that says how those are connected.
> > 
> > At this point we might as well assume it's a single DB - I think that
> > will be clearest.
> > 
> > Thanks.
> > 
> > - Julian
> > 
> > 
> > On Mon, 2010-07-12 at 23:23 +0200, Erik Huelsmann wrote:
> > > After lots of discussion regarding the way NODE_DATA/4th tree should
> > > be working, I'm now ready to post a summary of the progress. In my
> > > last e-mail (http://svn.haxx.se/dev/archive-2010-07/0262.shtml) I
> > > stated why we need this; this post is about the conclusion of what
> > > needs to happen. Also included are the first steps there.
> > > 
> > > 
> > > With the advent of NODE_DATA, we distinguish node values specifically
> > > related to BASE nodes, those specifically related to "current" WORKING
> > > nodes and those which are to be maintained for multiple levels of
> > > WORKING nodes (not only the "current" view) (the latter category is
> > > most often also shared with BASE).
> > > 
> > > The respective tables will hold the columns shown below.
> > > 
> > > 
> > > -
> > > TABLE WORKING_NODE (
> > >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> > >   local_relpath  TEXT NOT NULL,
> > >   parent_relpath  TEXT,
> > >   moved_here  INTEGER,
> > >   moved_to  TEXT,
> > >   original_repos_id  INTEGER REFERENCES REPOSITORY (id),
> > >   original_repos_path  TEXT,
> > >   original_revnum  INTEGER,
> > >   translated_size  INTEGER,
> > >   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
> > >   keep_local  INTEGER,
> > > 
> > >   PRIMARY KEY (wc_id, local_relpath)
> > >   );
> > > 
> > > CREATE INDEX I_WORKING_PARENT ON WORKING_NODE (wc_id, parent_relpath);
> > > 
> > > 
> > > The moved_* and original_* columns are typical examples of "WORKING
> > > fields only maintained for the visible WORKING nodes": the original_*
> > > and moved_* fields are inherited from the operation root by all
> > > children part of the operation. The operation root will be the visible
> > > change on its own level, meaning it'll have rows both in the
> > > WORKING_NODE and NODE_DATA tables. The fact that these columns are not
> > > in the WORKING_NODE table means that tree changes are not preserved
> > > accros overlapping changes. This is fully compatible with what we do
> > > today: changes to higher levels destroy changes to lower levels.
> > > 
> > > The translated_size and last_mod_time columns exist in WORKING_NODE
> > > and BASE_NODE; they explicitly don't exist in NODE_DATA. The fact that
> > > they exist in BASE_NODE is a bit of a hack: it's to prevent creation
> > > of WORKING_NODE data for every file which has keyword expansion or eol
> > > translation properties set: these columns serve only to optimize
> > > working copy scanning for changes and as such only relate to the
> > > visible WORKING_NODEs.
> > > 
> > > 
> > >  TABLE BASE_NODE (
> > >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> > >   local_relpath  TEXT NOT NULL,
> > >   repos_id  INTEGER REFERENCES REPOSITORY (id),
> > >   repos_relpath  TEXT,
> > >   parent_relpath  TEXT,
> > >   translated_size  INTEGER,
> > >   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
> > >   dav_cache  BLOB,
> > >   incomplete_children  INTEGER,
> > >   file_external  TEXT,
> > > 
> > >   PRIMARY KEY (wc_id, local_relpath)
> > >   );
> > > 
> > > 
> > > TABLE NODE_DATA (
> > >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> > >   local_relpath  TEXT NOT NULL,
> > >   op_depth  INTEGER NOT NULL,
> > >   presence  TEXT NOT NULL,
> > >   kind  TEXT NOT NULL,
> > >   checksum  TEXT,
> > >   changed_rev  INTEGER,
> > >   changed_date  INTEGER,  /* an APR date/time (usec since 1970) */
> > >   changed_author  TEXT,
> > >   depth  TEXT,
> > >   symlink_target  TEXT,
> > >   properties  BLOB,
> > > 
> > >   PRIMARY KEY (wc_id, local_relpath, oproot)
> > >   );
> > > 
> > > CREATE INDEX I_NODE_WC_RELPATH ON NODE_DATA (wc_id, local

Re: Need fast ways to get Info once WC-NG is introduced

[Philip Martin]

Stefan Küng  writes:

> On 02.08.2010 12:32, Bert Huijben wrote:
>>> So is there an (almost as) fast way to check whether a folder is
>>> versioned or not?
>>
>> I think the fastest way in the current code would be to call
>> svn_wc_read_kind() on the directory, maybe after first checking that there
>> is some .svn in at least one of the parent directories.
>
> I thought about implementing a small cache for that, so that I don't
> have to walk up the tree every time to find an .svn dir.
> But I thought I read something about such a small cache getting
> implemented in the svn library itself so I wanted to ask first - maybe
> there's already an API to use that cache. Or maybe I just remember it
> wrong.

Does TSVN cache/reuse svn_client_ctx_t handles?  In 1.7 the client
context contains an opaque wc context which in turn includes a
database context, svn_wc__db_t.  The database context caches sqlite
connections and has a cache mapping directory->database.

Quite when TSVN should create/destroy svn_client_ctx_t is an
interesting question.  Reusing a long-lived context (or perhaps a
small number, one per-thread say) is likely to make individual svn
calls faster.  However the open database handles means that Windows
won't be able to delete root directories.  It's not clear to me how
or when TSVN would close those handles.

-- 
Philip

Re: NODE_DATA (2nd iteration)

[Julian Foad]

On Mon, 2010-08-02, I (Julian Foad) wrote:
> Hi Erik.
> 
> Would you or anybody volunteer to draw a diagram of how these table rows
> look in various simple-ish WC states?

Maybe I can help by drawing my best interpretation of it and getting
your feedback.  I'll have a go.

- Julian


> I feel stupid saying this, but I haven't yet got much of an idea at all
> about how a set of database rows will represent a particular collection
> of repository nodes and local changes in the new scheme.  I know roughly
> what the aim is (to be able to represent nested tree changes more
> flexibly), and I can read what elements of data will be stored in each
> table, but I am missing the part that says how those are connected.
> 
> At this point we might as well assume it's a single DB - I think that
> will be clearest.
> 
> Thanks.
> 
> - Julian
> 
> 
> On Mon, 2010-07-12 at 23:23 +0200, Erik Huelsmann wrote:
> > After lots of discussion regarding the way NODE_DATA/4th tree should
> > be working, I'm now ready to post a summary of the progress. In my
> > last e-mail (http://svn.haxx.se/dev/archive-2010-07/0262.shtml) I
> > stated why we need this; this post is about the conclusion of what
> > needs to happen. Also included are the first steps there.
> > 
> > 
> > With the advent of NODE_DATA, we distinguish node values specifically
> > related to BASE nodes, those specifically related to "current" WORKING
> > nodes and those which are to be maintained for multiple levels of
> > WORKING nodes (not only the "current" view) (the latter category is
> > most often also shared with BASE).
> > 
> > The respective tables will hold the columns shown below.
> > 
> > 
> > -
> > TABLE WORKING_NODE (
> >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> >   local_relpath  TEXT NOT NULL,
> >   parent_relpath  TEXT,
> >   moved_here  INTEGER,
> >   moved_to  TEXT,
> >   original_repos_id  INTEGER REFERENCES REPOSITORY (id),
> >   original_repos_path  TEXT,
> >   original_revnum  INTEGER,
> >   translated_size  INTEGER,
> >   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
> >   keep_local  INTEGER,
> > 
> >   PRIMARY KEY (wc_id, local_relpath)
> >   );
> > 
> > CREATE INDEX I_WORKING_PARENT ON WORKING_NODE (wc_id, parent_relpath);
> > 
> > 
> > The moved_* and original_* columns are typical examples of "WORKING
> > fields only maintained for the visible WORKING nodes": the original_*
> > and moved_* fields are inherited from the operation root by all
> > children part of the operation. The operation root will be the visible
> > change on its own level, meaning it'll have rows both in the
> > WORKING_NODE and NODE_DATA tables. The fact that these columns are not
> > in the WORKING_NODE table means that tree changes are not preserved
> > accros overlapping changes. This is fully compatible with what we do
> > today: changes to higher levels destroy changes to lower levels.
> > 
> > The translated_size and last_mod_time columns exist in WORKING_NODE
> > and BASE_NODE; they explicitly don't exist in NODE_DATA. The fact that
> > they exist in BASE_NODE is a bit of a hack: it's to prevent creation
> > of WORKING_NODE data for every file which has keyword expansion or eol
> > translation properties set: these columns serve only to optimize
> > working copy scanning for changes and as such only relate to the
> > visible WORKING_NODEs.
> > 
> > 
> >  TABLE BASE_NODE (
> >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> >   local_relpath  TEXT NOT NULL,
> >   repos_id  INTEGER REFERENCES REPOSITORY (id),
> >   repos_relpath  TEXT,
> >   parent_relpath  TEXT,
> >   translated_size  INTEGER,
> >   last_mod_time  INTEGER,  /* an APR date/time (usec since 1970) */
> >   dav_cache  BLOB,
> >   incomplete_children  INTEGER,
> >   file_external  TEXT,
> > 
> >   PRIMARY KEY (wc_id, local_relpath)
> >   );
> > 
> > 
> > TABLE NODE_DATA (
> >   wc_id  INTEGER NOT NULL REFERENCES WCROOT (id),
> >   local_relpath  TEXT NOT NULL,
> >   op_depth  INTEGER NOT NULL,
> >   presence  TEXT NOT NULL,
> >   kind  TEXT NOT NULL,
> >   checksum  TEXT,
> >   changed_rev  INTEGER,
> >   changed_date  INTEGER,  /* an APR date/time (usec since 1970) */
> >   changed_author  TEXT,
> >   depth  TEXT,
> >   symlink_target  TEXT,
> >   properties  BLOB,
> > 
> >   PRIMARY KEY (wc_id, local_relpath, oproot)
> >   );
> > 
> > CREATE INDEX I_NODE_WC_RELPATH ON NODE_DATA (wc_id, local_relpath);
> > 
> > 
> > Which leaves the NODE_DATA structure above. The op_depth column
> > contains the depth of the node - relative to the wc root - on which
> > the operation was run which caused the creation of the given NODE_DATA
> > node.  In the final scheme (based on single-db), the value will be 0
> > for base and a positive integer for WORKING related data.
> > 
> > In order to be able to implement NODE_DATA even without having a fully
> > functional SINGLE_DB yet, a transitional node numbering scheme needs
> >

Re: svn commit: r981665 - in /subversion/branches/performance/subversion: include/private/svn_file_handle_cache.h include/private/svn_temp_serializer.h libsvn_subr/svn_file_handle_cache.c libsvn_subr/

[Kamesh Jayachandran]

On 08/03/2010 01:07 AM, stef...@apache.org wrote:

Author: stefan2
Date: Mon Aug  2 19:36:59 2010
New Revision: 981665

URL: http://svn.apache.org/viewvc?rev=981665&view=rev
Log:
Rename all svn_* in include/private to svn__* as requested in
http://svn.haxx.se/dev/archive-2010-08/0043.shtml

* subversion/include/private/svn_file_handle_cache.h
   (svn_*): rename to svn__*
* subversion/include/private/svn_temp_serializer.h
   (svn_*): rename to svn__*

* subversion/libsvn_subr/svn_file_handle_cache.c
   (svn_*): rename to svn__*
   (lock_cache, unlock_cache, find_first, internal_file_open, 
internal_close_file,
open_entry, close_oldest_idle, auto_close_oldest): adapt signatures
   (close_handle_before_cleanup): adapt implementation
* subversion/libsvn_subr/svn_temp_serializer.c
   (svn_*): rename to svn__*

Modified:
 
subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
 
subversion/branches/performance/subversion/include/private/svn_temp_serializer.h
 
subversion/branches/performance/subversion/libsvn_subr/svn_file_handle_cache.c
 
subversion/branches/performance/subversion/libsvn_subr/svn_temp_serializer.c

Modified: 
subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
URL: 
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h?rev=981665&r1=981664&r2=981665&view=diff
==
--- 
subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
 (original)
+++ 
subversion/branches/performance/subversion/include/private/svn_file_handle_cache.h
 Mon Aug  2 19:36:59 2010
@@ -20,7 +20,7 @@
   * 
   * @endcopyright
   *
- * @file svn_file_handle_cache.h
+ * @file svn__file_handle_cache.h
   


I guess it is a find and replace error. I mean file name still remains same.

   * @brief File handle cache API
   */

@@ -30,14 +30,14 @@
  /**
   * An opaque structure representing a cache for open file handles.
   */
-typedef struct svn_file_handle_cache_t svn_file_handle_cache_t;
+typedef struct svn__file_handle_cache_t svn__file_handle_cache_t;

  /**
   * An opaque structure representing a cached file handle being used
   * by the calling application.
   */
  typedef
-struct svn_file_handle_cache__handle_t svn_file_handle_cache__handle_t;
+struct svn__file_handle_cache__handle_t svn__file_handle_cache__handle_t;
   


having '__' in two places in an identifier looks bit odd.



  /**
   * Get an open file handle in @a f, for the file named @a fname with the
@@ -51,14 +51,14 @@ struct svn_file_handle_cache__handle_t s
   * returned is undefined.
   */
  svn_error_t *
-svn_file_handle_cache__open(svn_file_handle_cache__handle_t **f,
-svn_file_handle_cache_t *cache,
-const char *fname,
-apr_int32_t flag,
-apr_fileperms_t perm,
-apr_off_t offset,
-int cookie,
-apr_pool_t *pool);
+svn__file_handle_cache__open(svn__file_handle_cache__handle_t **f,
+ svn__file_handle_cache_t *cache,
+ const char *fname,
+ apr_int32_t flag,
+ apr_fileperms_t perm,
+ apr_off_t offset,
+ int cookie,
+ apr_pool_t *pool);
   


having '__' in two places in an identifier looks bit odd.



  /**
   * Efficiently check whether the file handle cache @a cache holds an open
@@ -67,8 +67,8 @@ svn_file_handle_cache__open(svn_file_han
   * that the respective file does not exist.
   */
  svn_boolean_t
-svn_file_handle_cache__has_file(svn_file_handle_cache_t *cache,
-const char *fname);
+svn__file_handle_cache__has_file(svn__file_handle_cache_t *cache,
+ const char *fname);
   


having '__' in two places in an identifier looks bit odd.



  /**
   * Return the APR level file handle underlying the cache file handle @a f.
@@ -76,7 +76,7 @@ svn_file_handle_cache__has_file(svn_file
   * invalidated.
   */
  apr_file_t *
-svn_file_handle_cache__get_apr_handle(svn_file_handle_cache__handle_t *f);
+svn__file_handle_cache__get_apr_handle(svn__file_handle_cache__handle_t *f);
   


having '__' in two places in an identifier looks bit odd.



  /**
   * Return the name of the file that the cached handle @a f refers to.
@@ -84,7 +84,7 @@ svn_file_handle_cache__get_apr_handle(sv
   * invalidated.
   */
  const char *
-svn_file_handle_cache__get_name(svn_file_handle_cache__handle_t *f);
+svn__file_handle_cache__get_name(svn__file_handle_cache__handle_t *f);
   


having '__' in two places in an identifier looks bit odd.



  /

opening fsfs rev files rw

[Daniel Shahaf]

There was yesterday on IRC [1] some discussion around whether revprop
editing can corrupt rev files.

[[[
0:% rm -rf r
0:% ./subversion/svnadmin/svnadmin create r
0:% ls -l r/db/revs/0/0
-rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0
]]]

Shouldn't we create rev files with the write bit disabled?  (i.e., 0222 umask)

Daniel
(I'll add this to my list)



[1] http://colabti.org/irclogger/irclogger_log/svn-dev?date=2010-08-02

Re: [PATCH] Add a '--fsfs-no-repsharing' flag to svnadmin

[Simon Atanasyan]

On Mon, Aug 2, 2010 at 23:04, Daniel Shahaf  wrote:
> Simon Atanasyan wrote on Mon, Aug 02, 2010 at 19:59:31 +0400:
>> From the programmer's point of view if you create repository calling
>> svn_fs_create with the new configuration option you easily get well commented
>> db/fsfs.conf with necessary settings.
>
> svn_fs_create() already takes an FS_CONFIG parameter.  We could populate the
> initially-created fsfs.conf with values from that parameter...

If I understand you correctly my patch does exactly the same things.
It checks file system configuration options in the write_config
function (called from svn_fs_fs__create) and generates corresponded
db/fsfs.conf.

-- 
Simon Atanasyan
VisualSVN Limited