gmane.comp.version-control.subversion.devel

RE: svn commit: r1484341 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_client/diff.c

Bert Huijben — 2013-05-19T21:31:01



Can you please prefix your commits to this branch with something like

On the invoke diff branch:


Without that the log message reads as a commit to trunk while a recent message such as
[[
Author: stsp
Date: Sun May 19 10:17:07 2013
New Revision: 1484261

URL: http://svn.apache.org/r1484261
Log:
On the 1.6.x-issue4340 branch, filter control characters from an error message.

This corresponds to r1481627 on trunk, but the svn_path_illegal_path_escape()
API function used in that commit is not available in 1.6.x. So I've manually
marked the revision as merged using a --record-only merge.
]]

makes it immediately clear that we look at a patch applied to a branch.

Bert

Re: Releasing 1.7.10

Ben Reser — 2013-05-19T19:00:15

+1


I'm happy to do one of them for you if you want, but if you want to do
both I'm not going to object.

Re: Review of invoke-diff-cmd-feature branch

Daniel Shahaf — 2013-05-19T17:55:20

Also, layering violation in the error message: this code is in
libsvn_subr and as such is not allowed to talk about "--invoke-diff-cmd";
that is a cmdline-client concept which is not valid for other potential
callers of this function.

Review of invoke-diff-cmd-feature branch

Daniel Shahaf — 2013-05-19T17:23:13

Review in diff form:

Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h(revision 1484305)
+++ subversion/include/svn_client.h(working copy)
< at >< at > -2988,8 +2988,8 < at >< at > svn_client_blame(const char *path_or_url,
  *
  * < at >a invoke_diff_cmd is used to call an external diff program but may
  * not be < at >c NULL.  The command line invocation will override the
- * invoke-diff-cmd invocation entry(if any) in the Subversion
- * configuration file.
+ * invoke-diff-cmd invocation entry(if any) in < at >c ctx->config.
+ * ### "May not be NULL" !?  log-cmd.c and deprecated.c pass NULL for it.
  *
  * Generated headers are encoded using < at >a header_encoding.
  *
< at >< at > -3047,7 +3047,8 < at >< at > svn_client_diff7(const apr_array_header_t *options
                  svn_client_ctx_t *ctx,
                  apr_pool_t *pool);
 
-/** Similar to svn_client_diff7(), but with < at >a invoke_diff_cmd.
+/** Similar to svn_client_diff7(), but with < at >a invoke_diff_cmd
+ * s

Re: svn commit: r1484251 - /subversion/branches/invoke-diff-cmd-feature/subversion/svn/log-cmd.c

Daniel Shahaf — 2013-05-19T17:23:49

(except Makefile)

Re: svn commit: r1484251 - /subversion/branches/invoke-diff-cmd-feature/subversion/svn/log-cmd.c

Branko Čibej — 2013-05-19T17:05:20

The last hunk contains tabs. There should be no tabs in our source
files, ever.

Re: svn commit: r1484260 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_client/diff.c

Branko Čibej — 2013-05-19T17:04:14

Indeed, we tend to prefer the original form.

Re: svn commit: r1484260 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_client/diff.c

Daniel Shahaf — 2013-05-19T16:48:48

Why?  This doesn't seem to serve any useful purpose (in fact, I think it makes
the code harder to read).

Re: svnserve DoS attack (1.7.8)

Daniel Shahaf — 2013-05-19T15:00:40

Boris indicates he just did what the Big Yellow Box in
http://subversion.apache.org/reporting-issues instructed him to; I think we
have to update that page too, not only the /mailing-lists page.

Re: Releasing 1.7.10

Stefan Sperling — 2013-05-19T09:31:16

So 1.8.0rc2 is out now.

Can we roll 1.7.10 on Wednesday?

Personally, I'd like to see the svnserve issue fixed (nominations
for which have already passed review on both 1.6.x and 1.7.x branches), 
as well as the \n FSFS repository corruption issue (which still has
outstanding nominations on both branches).

I think we should also release a 1.6.22 with the same fixes.
I'd volunteer to roll that as well, and manage both releases at the
same time (unless Ben wants to do one of them, but if he wants a
break he deserves it very much).

Re: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Branko Čibej — 2013-05-18T13:04:36

This I can certainly agree with. The question remains, however: what can
we do /now/ in order to avoid the potential corruption? I propose we
have two options: use the fix Ivan came up with and optimize it later,
or delay 1.8.0 until what you propose can be implemented.

Personally I don't have a problem with the latter approach, but we know
it could take weeks and would definitely restart the soak. As far as I
know, we don't have any performance data that could help us decide.

RE: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Bert Huijben — 2013-05-18T09:52:07



But why do we perform the same flush for 3* on a tempfile for each property conflict that is postponed in 'svn'?

For files that aren't read back unless the user uses interactive conflict resolution?

This kind of behavior belongs in FSFS... well designed, not in a simple 'cheap' function to write a string to a file.

I 100% agree that FSFS should be robust...

But if we add a flush after every file write in our code nobody will use Subversion as it is so insanely slow... And we wouldn't resolve the problem we thought of resolving.


Getting proper revision commit handling requires a careful design and applying that. Not adding explicit flushes in low level functions that are used for more code paths than these.


The careful flush is needed at the time of committing a revision. Probably two or three times max during a revision creation. This flush will require writing to two MFT's (Master File Tables) and the file itself so probably at least 6 harddisk spin cycles each (and more if there are other operat

Re: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Branko Čibej — 2013-05-18T08:51:40

What the blazes are you going on about, Bert? Justin's +1 was for
putting repository integrity before performance. That has nothing at all
to do with any specific platform.


Ivan's analysis of the failure mode is correct, and it has nothing to do
with disks, emulated or otherwise, because it is a side effect of what
happens before the data hits the disk driver layer.


The SQLite docs clearly spell out what happens if you don't turn on the
whole sync'd WAL. The Subversion repository is not an application-level
persistent store, it's a centralized repository that we /know/ has to be
as robust as we can make it.


Excuse me? fsync() requires root? That's a new one.


Are you saying that it's OK to have a known repository corruption for
the sake of not having users breathe down our necks about performance on
Windows?


So why, in your opinion, is it OK to flush the file buffers on every
platform /except/ Windows? Are you saying that sysadmins on other
systems don't use journalled filesystems?

(Interestingly

Re: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Bert Huijben — 2013-05-18T06:55:32

I see all those easy +1’s on other operating systems...

I assume you all reproduced the problem on Windows???


Maybe also on actual hardware instead of a VM (with a VM harddisk emulation infrastructure with different powerfail handling)?


I still see no prove that the symptoms are not in this category!



But if you check the sqlite research: which other operating systems provide the same guarantees on power failure at the cost of a lot of performance?


We are talking about flushing the NTFS journal to ensure everything for a single file is flished. Something which in multi user systems such as *nix really requires root permissions as it allows trashing performance for the entire system.


Safety is a nice property, but you can’t get it via just these flushes. Usability is also important. And the rest of the system needs the same power safety security principles for these flushes to make sense. And then only in critical places, not after every small tempfile write.



E.g. part

Re: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Justin Erenkrantz — 2013-05-18T04:00:45


+1!  =P  -- justin

Re: svn commit: r1483999 - /subversion/trunk/subversion/libsvn_subr/io.c

Ivan Zhakov — 2013-05-17T22:01:51

Using delete_when for differentiate flush behavior is looks very
strange. Why you didn't add separate boolean argument for this?

Re: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Ivan Zhakov — 2013-05-17T21:58:17

Yes, this will be good improvement anyway, but I think repository
integrity should be first goal.

Also please note that FlushFileBuffers doesn't perform *full* sync: it
flushes data only for one file.

RE: svn commit: r1483795 - /subversion/branches/1.8.x/STATUS

Bert Huijben — 2013-05-17T20:55:15



If this and the future followups are going to have a huge performance impact we should probably make the full fsync option configurable for those who have a battery backed up storage.
(Looking at the current callers I don't think this is necessary. I think the original performance problem was mostly in the loggy handling)

Bert

Re: svn commit: r1483972 - /subversion/trunk/subversion/include/svn_config.h

Ben Reser — 2013-05-17T20:49:05

r1483984

Re: svn commit: r1483972 - /subversion/trunk/subversion/include/svn_config.h

Daniel Shahaf — 2013-05-17T20:26:08

Perhaps:

    * case sensitively, except for the < at >c "DEFAULT" section.

?

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

Daniel Shahaf — 2013-05-17T19:30:16

pburba< at >apache.org wrote on Fri, May 17, 2013 at 18:45:53 -0000:

Was this hunk intentional?