Re: A two-part vision for Subversion and large binary objects.
On Mon, 14 Feb 2022 at 01:39, Karl Fogel wrote: > On 12 Feb 2022, Mark Phippard wrote: > >Just to offer a counterpoint Karl, I always assumed the goal of > >the > >branch was to have no pristines in the WC and the "on-demand" > >aspect > >was referring to an internal SVN detail that it would have to > >fetch > >pristines when they were needed to complete a command that I have > >executed like diff or revert. > > > >I know we discussed whether the entire WC, or individual files > >would > >not have pristines but I never considered the "on-demand" aspect > >to be > >about my ability to decide this. It was about SVN just doing what > >it > >needed to when it needed to. > > Ah, I see. That might be where the branch name came from, yeah. > But the key (necessary) part of the feature is the absence of > pristines, whereas the restoration of some pristines on demand is > an optional enhancement (and one we're not even doing in the first > MVP version). > > In fact, selected rehydration is not necessarily even the first > enhancement we might make after MVP. There's an argument for > prioritizing flexible client-side configuration specs first, so > that all the diffable files get pristines on checkout while all > the big binary blob files get no pristines. IOW, if we get the > checkout right the first time, then selected rehydration becomes > less important to have; also, there is an easy workaround for it; > just make a copy of the working file :-). > > (I still think selected rehydration would be good to have, of > course; I'm just pointing out that we haven't really discussed > where it sits relative to other possible things.) > > In any case, the branch name doesn't matter too much here, > especially since it's going to get merged soon. However, for the > user-facing name of the feature, we should pick a name based on > the essence of the feature, not on a not-yet-fully-implemented > optional enhancement to the feature, discussed further below. > > On 13 Feb 2022, Julian Foad wrote: > >That name came, as far as I am aware, from Evgeny's branch which > >implements the latter. > > > >This may be a case where the public facing name for the feature > >ought to differ from the internal development name. > > > >Any ideas for a good public name? > > > >Pristines on Subversion's demand? > >Dehydrated WC? > > I kind of like the dehydration/rehydration theme -- it's certainly > memorable! Other possibilities: > > - blob-optimized checkouts > > - "blobtimized" checkouts (okay, kidding there... :-) ) > > I would suggest: - optional pristines Just my two cents. -- Ivan Zhakov
[PATCH] Add XFail test for relocating working copy with removed externals
Log message:
[[[
Add XFail test for relocating working copy with removed externals.
Found by: TortoiseSVN dump
Patch by: Ivan Zhakov {ivan {at} visualsvn.com}
* subversion/tests/cmdline/relocate_tests.py
(relocate_with_removed_externals): New test.
(test_list): Add new test to the test list.
]]]
--
Ivan Zhakov
Index: subversion/tests/cmdline/relocate_tests.py
===
--- subversion/tests/cmdline/relocate_tests.py (revision 1806425)
+++ subversion/tests/cmdline/relocate_tests.py (working copy)
@@ -418,7 +418,37 @@
svntest.actions.run_and_verify_info([{ 'URL' : '.*.yyyother$' }],
wc_dir)
+
+@XFail()
+def relocate_with_removed_externals(sbox):
+ "relocate a directory with removed externals"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ # Add a relative external.
+ change_external(os.path.join(wc_dir, 'A', 'B'),
+ "^/A/D/G G-ext", commit=True)
+ svntest.actions.run_and_verify_svn(None, [], 'update', wc_dir)
+
+ svntest.main.safe_rmtree(os.path.join(wc_dir, 'A', 'B', 'G-ext'), 1)
+
+ # Move our repository to another location.
+ repo_dir = sbox.repo_dir
+ repo_url = sbox.repo_url
+ other_repo_dir, other_repo_url = sbox.add_repo_path('other')
+ svntest.main.copy_repos(repo_dir, other_repo_dir, 2, 0)
+ svntest.main.safe_rmtree(repo_dir, 1)
+
+ # Now relocate our working copy.
+ svntest.actions.run_and_verify_svn(None, [], 'relocate',
+ repo_url, other_repo_url, wc_dir)
+
+ # Check the URLs of the externals -- were they updated to point to the
+ # .other repository URL?
+ svntest.actions.run_and_verify_info([{ 'URL' : '.*.other/A/D/G$' }],
+ os.path.join(wc_dir, 'A', 'B', 'G-ext'))
# Run the tests
@@ -431,6 +461,7 @@
relocate_with_switched_children,
relocate_with_relative_externals,
prefix_partial_component,
+ relocate_with_removed_externals,
]
if __name__ == '__main__':
Re: [PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files
On 23 August 2017 at 19:47, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > Evgeny Kotkov <evgeny.kot...@visualsvn.com> writes: > >> In the meanwhile, apparently, there is an oversight in the core V1 patch >> (3-reduce-syscalls-for-buffered-writes-v1.patch.txt): >> >> If the buffer is not empty, and the caller issues a write with a chunk >> that slightly exceeds the buffer size, for example, 4100 bytes, it would >> result in flushing the buffer _and_ writing the remaining couple of bytes >> with WriteFile(). An appropriate thing to do here would be to flush the >> buffer, but put the few remaining bytes into the buffer, instead of writing >> them to disk and thus making an unnecessary syscall. >> >> With all this in mind, I will prepare the V2 version of the core patch. > > I have attached the V2 version of the core patch. > > To solve the aforementioned oversight in the V1 patch, I implemented the > optimization in a slightly different manner, by keeping the existing loop > but also handling a condition where we can write the remaining data with > a single WriteFile() call. Apart from this, the V2 patch also includes an > additional test, test_small_and_large_writes_buffered(). > > Speaking of the discussed idea of adjusting the strategy to avoid a memcpy() > with the write pattern like > > WriteFile(13) > WriteFile(86267) > ... > > instead of > > WriteFile(4096) > WriteFile(82185) > ... > > I preferred to keep the latter approach which keeps the minimum size of > the WriteFile() chunk equal to 4096, for the following reasons: > > - My benchmarks do not show that the alternative pattern that also avoids > a memcpy() results in a quantifiable performance improvement. > > - The existing code had a property that all WriteFile() calls, except > for maybe the last one, were performed in chunks with sizes that are > never less than 4096. Although I can't reproduce this in my environment, > it could be that introducing a possibility of writing in smaller chunks > would result in the decreased performance with specific hardware, non- > file handles or in situations when the OS write caching is disabled. > Therefore, currently, I think that it would be better to keep this > existing property. > > - Probably, implementing the first approach would result in a bit more > complex code, as I think that it would require introducing an additional > if-else code path. > > The log message is included in the beginning of the patch file. > Committed 3-reduce-syscalls-for-buffered-writes-v2.patch.txt patch in r1806308. Thanks! -- Ivan Zhakov
Re: [PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files
On 21 August 2017 at 18:45, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > Hi everyone, > [..] > The implementation is split into three dependent patches. The first two > patches lay the necessary groundwork by factoring out a couple of helper > functions. Refactoring patches committed in r1806299 and r1806301. > The third patch is the core change that implements the > described optimization. > Review of core changes (3-reduce-syscalls-for-buffered-writes-v2.patch.txt ) is in progress. -- Ivan Zhakov
[PATCH] Use svn_hash_gets() instead of apr_hash_get(APR_HASH_KEY_STRING)
Log message:
[[[
* subversion/libsvn_fs_fs/stats.c
* subversion/tests/libsvn_client/client-test.c
* subversion/tests/libsvn_repos/repos-test.c
* subversion/tests/libsvn_subr/stream-test.c
* subversion/tests/libsvn_subr/translate-test.c
* subversion/tests/libsvn_wc/conflict-data-test.c
(): Include svn_hash.h.
(add_change, test_elide_mergeinfo_catalog, patch_collection_func, test_patch,
rmlocks_change_prop, prop_validation_commit_with_revprop,
test_stream_seek_translated, substitute_and_verify,
test_serialize_prop_conflict): Use svn_hash_gets() instead of
apr_hash_get(APR_HASH_KEY_STRING).
]]]
--
Ivan Zhakov
Index: subversion/libsvn_fs_fs/stats.c
===
--- subversion/libsvn_fs_fs/stats.c (revision 1805933)
+++ subversion/libsvn_fs_fs/stats.c (working copy)
@@ -22,6 +22,7 @@
#include "svn_dirent_uri.h"
#include "svn_fs.h"
+#include "svn_hash.h"
#include "svn_pools.h"
#include "svn_sorts.h"
@@ -338,7 +339,7 @@
extension = "(none)";
/* get / auto-insert entry for this extension */
- info = apr_hash_get(stats->by_extension, extension, APR_HASH_KEY_STRING);
+ info = svn_hash_gets(stats->by_extension, extension);
if (info == NULL)
{
apr_pool_t *pool = apr_hash_pool_get(stats->by_extension);
@@ -345,8 +346,7 @@
info = apr_pcalloc(pool, sizeof(*info));
info->extension = apr_pstrdup(pool, extension);
- apr_hash_set(stats->by_extension, info->extension,
- APR_HASH_KEY_STRING, info);
+ svn_hash_sets(stats->by_extension, info->extension, info);
}
/* update per-extension histogram */
Index: subversion/tests/libsvn_client/client-test.c
===
--- subversion/tests/libsvn_client/client-test.c(revision 1805933)
+++ subversion/tests/libsvn_client/client-test.c(working copy)
@@ -122,8 +122,7 @@
SVN_ERR(svn_mergeinfo_parse(, item->unparsed_mergeinfo,
iterpool));
- apr_hash_set(mergeinfo_catalog, item->path, APR_HASH_KEY_STRING,
- mergeinfo);
+ svn_hash_sets(mergeinfo_catalog, item->path, mergeinfo);
}
SVN_ERR(svn_client__elide_mergeinfo_catalog(mergeinfo_catalog,
@@ -131,8 +130,8 @@
for (item = elide_testcases[i]; item->path; item++)
{
- apr_hash_t *mergeinfo = apr_hash_get(mergeinfo_catalog, item->path,
- APR_HASH_KEY_STRING);
+ apr_hash_t *mergeinfo = svn_hash_gets(mergeinfo_catalog, item->path);
+
if (item->remains && !mergeinfo)
return svn_error_createf(SVN_ERR_TEST_FAILED, NULL,
"Elision for test case #%d incorrectly "
@@ -314,16 +313,14 @@
struct patch_collection_baton *pcb = baton;
if (patch_abspath)
-apr_hash_set(pcb->patched_tempfiles,
- apr_pstrdup(pcb->state_pool, canon_path_from_patchfile),
- APR_HASH_KEY_STRING,
- apr_pstrdup(pcb->state_pool, patch_abspath));
+svn_hash_sets(pcb->patched_tempfiles,
+ apr_pstrdup(pcb->state_pool, canon_path_from_patchfile),
+ apr_pstrdup(pcb->state_pool, patch_abspath));
if (reject_abspath)
-apr_hash_set(pcb->reject_tempfiles,
- apr_pstrdup(pcb->state_pool, canon_path_from_patchfile),
- APR_HASH_KEY_STRING,
- apr_pstrdup(pcb->state_pool, reject_abspath));
+svn_hash_sets(pcb->reject_tempfiles,
+ apr_pstrdup(pcb->state_pool, canon_path_from_patchfile),
+ apr_pstrdup(pcb->state_pool, reject_abspath));
if (filtered)
*filtered = FALSE;
@@ -418,14 +415,12 @@
SVN_TEST_ASSERT(apr_hash_count(pcb.patched_tempfiles) == 1);
key = "A/D/gamma";
- patched_tempfile_path = apr_hash_get(pcb.patched_tempfiles, key,
- APR_HASH_KEY_STRING);
+ patched_tempfile_path = svn_hash_gets(pcb.patched_tempfiles, key);
SVN_ERR(check_patch_result(patched_tempfile_path, expected_gamma, "\n",
EXPECTED_GAMMA_LINES, pool));
SVN_TEST_ASSERT(apr_hash_count(pcb.reject_tempfiles) == 1);
key = "A/D/gamma";
- reject_tempfile_path = apr_hash_get(pcb.reject_tempfiles, key,
- APR_HASH_KEY_STRING);
+ reject_tempfile_path = svn_hash_gets(pcb.reject_tempfiles, key);
SVN_ERR(check_patch_result(reject_tempfile_path, expected_gamma_reject,
APR_EOL_STR, EXPECTED_GAMMA_REJECT_LINES, pool));
Index: subversion/tests/libsvn_repos/repos
Re: [PATCH] Add LZ4 version info to svn --version -v
On 11 August 2017 at 23:28, Ivan Zhakov <i...@visualsvn.com> wrote: > Log message: > [[[ > Add LZ4 library verison info to svn --version -v. > > * subversion/libsvn_subr/sysinfo.c > (): Include lz4/lz4internal.h or lz4.h depending on SVN_INTERNAL_LZ4. > (svn_sysinfo__linked_libs): Add LZ4 library version info. > ]]] > V2 version of the patch: - display 'static' for statically linked LZ4 - do not use LZ4_VERSION_STRING and LZ4_versionString() API, because they are not available in older version of LZ4 library. -- Ivan Zhakov Index: subversion/libsvn_subr/sysinfo.c === --- subversion/libsvn_subr/sysinfo.c(revision 1804819) +++ subversion/libsvn_subr/sysinfo.c(working copy) @@ -51,6 +51,12 @@ #include "sysinfo.h" #include "svn_private_config.h" +#if SVN_INTERNAL_LZ4 +#include "lz4/lz4internal.h" +#else +#include +#endif + #if HAVE_SYS_UTSNAME_H #include #endif @@ -167,6 +173,20 @@ lib->compiled_version = apr_pstrdup(pool, svn_zlib__compiled_version()); lib->runtime_version = apr_pstrdup(pool, svn_zlib__runtime_version()); + lib = _ARRAY_PUSH(array, svn_version_ext_linked_lib_t); + lib->name = "LZ4"; + lib->compiled_version = apr_psprintf(pool, "%d.%d.%d", LZ4_VERSION_MAJOR, + LZ4_VERSION_MINOR, + LZ4_VERSION_RELEASE); +#ifdef SVN_INTERNAL_LZ4 + lib->runtime_version = NULL; +#else + lib->runtime_version = apr_psprintf(pool, "%d.%d.%d", + LZ4_versionNumber() / (100*100), + (LZ4_versionNumber() / 100) % 100, + LZ4_versionNumber() % 100); +#endif + return array; }
[PATCH] Add LZ4 version info to svn --version -v
Log message: [[[ Add LZ4 library verison info to svn --version -v. * subversion/libsvn_subr/sysinfo.c (): Include lz4/lz4internal.h or lz4.h depending on SVN_INTERNAL_LZ4. (svn_sysinfo__linked_libs): Add LZ4 library version info. ]]] -- Ivan Zhakov Index: subversion/libsvn_subr/sysinfo.c === --- subversion/libsvn_subr/sysinfo.c(revision 1804853) +++ subversion/libsvn_subr/sysinfo.c(working copy) @@ -51,6 +51,12 @@ #include "sysinfo.h" #include "svn_private_config.h" +#if SVN_INTERNAL_LZ4 +#include "lz4/lz4internal.h" +#else +#include +#endif + #if HAVE_SYS_UTSNAME_H #include #endif @@ -167,6 +173,11 @@ lib->compiled_version = apr_pstrdup(pool, svn_zlib__compiled_version()); lib->runtime_version = apr_pstrdup(pool, svn_zlib__runtime_version()); + lib = _ARRAY_PUSH(array, svn_version_ext_linked_lib_t); + lib->name = "LZ4"; + lib->compiled_version = apr_pstrdup(pool, LZ4_VERSION_STRING); + lib->runtime_version = apr_pstrdup(pool, LZ4_versionString()); + return array; }
[PATCH] Add version information to libsvn_swig_py-1.dll
[[[ Add version information to libsvn_swig_py-1.dll on Windows. * build.conf (libsvn_swig_py): Add description. ]]] -- Ivan Zhakov Index: build.conf === --- build.conf (revision 1783776) +++ build.conf (working copy) @@ -573,6 +573,7 @@ compile-cmd = $(COMPILE_SWIG_PY) msvc-static = no msvc-export = ../bindings/swig/python/libsvn_swig_py/swigutil_py.h +description = Subversion utility library for Python bindings # SWIG utility library for Perl modules [libsvn_swig_perl]
'svn --version -v' displays wrong OS version information on Windows 10
Hi, I've noticed that 'svn --version -v' displays wrong information about OS version on Windows 10 and later. For example: $ svn --version -v System information: * running on x86/x86_64-microsoft-windows6.2.9200 - Windows 10 Enterprise 2015 LTSB, build 10240 [6.3 Client Multiprocessor Free] * linked dependencies: - APR 1.5.1 (compiled with 1.5.2) - APR-Util 1.5.3 (compiled with 1.5.4) - Expat 2.2.0 (compiled with 2.2.0) - SQLite 3.11.1 (static) - Utf8proc 1.1.5 (compiled with 1.1.5) - ZLib 1.2.8 (compiled with 1.2.8) * loaded shared libraries: - C:\Ivan\SVN\trunk\Debug\subversion\svn\svn.exe (1.10) - C:\WINDOWS\SYSTEM32\ntdll.dll (6.2.10240.17184) - C:\WINDOWS\SYSTEM32\KERNEL32.DLL (6.2.10240.17113) - C:\WINDOWS\SYSTEM32\KERNELBASE.dll (6.2.10240.17184) [..] Notice that version is '6.2' while it should be 10.0. Version for OS modules are also wrong. This happens because svn.exe is not manifested properly and Windows enables 'version lie shims' in this case [1]. Attached patch fixes this problem. With patched version I get correct result: $ svn --version -v System information: * running on x86/x86_64-microsoft-windows10.0.10240 - Windows 10 Enterprise 2015 LTSB, build 10240 [6.3 Client Multiprocessor Free] * linked dependencies: - APR 1.5.1 (compiled with 1.5.2) - APR-Util 1.5.3 (compiled with 1.5.4) - Expat 2.2.0 (compiled with 2.2.0) - SQLite 3.11.1 (static) - Utf8proc 1.1.5 (compiled with 1.1.5) - ZLib 1.2.8 (compiled with 1.2.8) * loaded shared libraries: - C:\Ivan\SVN\trunk\Debug\subversion\svn\svn.exe (1.10) - C:\WINDOWS\SYSTEM32\ntdll.dll (10.0.10240.17184) - C:\WINDOWS\SYSTEM32\KERNEL32.DLL (10.0.10240.17113) - C:\WINDOWS\SYSTEM32\KERNELBASE.dll (10.0.10240.17184) [...] [1] https://msdn.microsoft.com/en-us/library/windows/desktop/dn481241(v=vs.85).aspx -- Ivan Zhakov Index: build/generator/templates/vcnet_vcxproj.ezt === --- build/generator/templates/vcnet_vcxproj.ezt (revision 1766680) +++ build/generator/templates/vcnet_vcxproj.ezt (working copy) @@ -94,7 +94,10 @@ [is platforms "X64"]MachineX64[else]MachineX86[end] [is configs.name "Debug"] msvcrt.lib [end] -[end][end][end] +[end][end][end][is config_type "Application"] + ..\compatibility.manifest + +[end] [end][end][if-any target.desc] Index: build/win32/compatibility.manifest === --- build/win32/compatibility.manifest (nonexistent) +++ build/win32/compatibility.manifest (working copy) @@ -0,0 +1,17 @@ + + + + + + + + + + + + + + + + +
Re: svn commit: r1765286 - in /subversion/trunk/subversion: include/svn_xml.h libsvn_subr/xml.c tests/libsvn_subr/xml-test.c
On 17 October 2016 at 17:29, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message- >> From: i...@apache.org [mailto:i...@apache.org] >> Sent: maandag 17 oktober 2016 15:49 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1765286 - in /subversion/trunk/subversion: >> include/svn_xml.h libsvn_subr/xml.c tests/libsvn_subr/xml-test.c >> >> Author: ivan >> Date: Mon Oct 17 13:49:05 2016 >> New Revision: 1765286 >> >> URL: http://svn.apache.org/viewvc?rev=1765286=rev >> Log: >> Implement standard lifetime semantics for svn_xml_parser_t: the object will >> be >> automatically freed on pool cleanup. But it still can be freed explicitly >> using svn_xml_free_parser(). It's the same behavior we already have for >> svn_sqlite__db_t and similar. > > Are you planning a new use of this api? > Yes. Currently we link Expat XML three times: in libsvn_subr, libsvn_ra_serf and mod_dontdothat. I'd like to fix it. > It is currently only used by subversion/libsvn_wc/old-and-busted.c, and I > don't > think we should really spend time optimizing that specific usecase (reading > pre 1.4 working copies) > Of course I'm aware of this. -- Ivan Zhakov
Re: [PATCH] Improve conformance to ISO C90
On 14 October 2016 at 15:24, Patrick Steinhardt <patrick.steinha...@elegosoft.com> wrote: > Hi, > > attached patch fixes compatibility with ISO C90. It fixes > trailing commas in enum lists as well as one case where variadic > macros are used, which are a feature of C99. What it does not fix > is strings with overlength (C90 only allows for fixed strings > with a maximum length of 509 characters). > > This allows us to build the code with `-Werror -pedantic > -Wno-overlength-strings`. > > [[ > Improve C90 compatibility by removing trailing commas in enum > lists as well as converting a variadic macro stub with an empty > function. > > * subversion/include/svn_client.h: > (svn_client_config_option_id_t): remove trailing comma > * subversion/include/svn_wc.h: > (svn_wc_notify_action_t): remove trailing comma > * subversion/libsvn_subr/cache-membuffer.c: > (prefix_pool_create): remove trailing comma > * subversion/svn/svn.c: > (svn_cl__longopt_t): remove trailing comma > * subversion/svnmucc/svnmucc.c: > (sub_main): remove trailing comma > * subversion/svnmover/linenoise/linenouse.c: > (lndebug): replace variadic macro stub with an empty variadic > function > ]] Committed in 1764919 with tweaked log message. -- Ivan Zhakov
Re: [PATCH v3] Conflict option labels
On 14 October 2016 at 12:06, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 14 October 2016 at 11:43, Ivan Zhakov <i...@visualsvn.com> wrote:
>> On 13 October 2016 at 17:26, Patrick Steinhardt <p...@pks.im> wrote:
>>> Hi,
>>>
>>> the third version re-adds the result pool to
>>> `svn_client_conflict_option_get_lazel`.
>>>
>> [...]
>>> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
>>> }
>>>else
>>> {
>>> - opt = options++;
>>> - if (! opt->code)
>>> + if (i >= options->nelts)
>>> break;
>>> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
>>> + i++;
>>> }
>>>
>>>if (! first)
>>> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
>>>s = apr_psprintf(pool, " (%s) %s", opt->code,
>>> - opt->short_desc ? _(opt->short_desc) :
>>> opt->long_desc);
>>> + opt->label ? _(opt->label) : opt->long_desc);
>> The opt->label is already localized, so _() is not needed.
>>
>> Beside of that patch looks fine and I'm ready to commit it in current
>> state. Stefan, do you have any comments on the patch?
>>
> Here is updated patch and log message that adapted to r1764848 changes.
Committed in r1764883. Thanks!
--
Ivan Zhakov
Re: [PATCH] Reject checkouts to existing directory
On 12 October 2016 at 16:28, Patrick Steinhardt <patrick.steinha...@elegosoft.com> wrote: > Hi, > > attached is a patch to reject checkouts to already existing > directories when `--force` is not given. This is according to > `svn co --help`. > > [[ > Reject checkout to existing paths without force > > * subversion/svn/checkout-cmd.c: > - (svn_cl__checkout): Reject checkout to existing directory > without --force > ]] > As far I see we have four different cases of checking out to existing directory: a. Checkout of non-empty directory to non-empty local directory b. Checkout of non-empty directory to empty local directory c. Checkout of empty directory to non-empty local directory d. Checkout of empty directory to empty local directory I think that cases (b),(c),(d) should not require '--force'. It seems to be pretty safe operations. Also (c) is common way to 'takeover' (version) existing repository. I meant: 1. Create empty directory in repository 2. Check it to existing directory with source code 3. Add all requested files/directories 4. Review all changes and commit. But it makes sense for to require '--force' flag for case (a): it's very easy mess up local changes in this situation. -- Ivan Zhakov
Re: [PATCH] Resolve issue #4647 on trunk (v3)
On 14 October 2016 at 00:29, Stefan <luke1...@posteo.de> wrote: > On 10/13/2016 11:38 AM, Stefan wrote: >> On 10/13/2016 11:08 AM, Stefan wrote: >>> On 10/10/2016 11:39 PM, Stefan wrote: >>>> On 10/10/2016 6:12 PM, Ivan Zhakov wrote: >>>>> On 10 October 2016 at 17:53, Stefan <luke1...@posteo.de> wrote: >>>>>> On 8/28/2016 11:32 PM, Bert Huijben wrote: >>>>>>>> -Original Message- >>>>>>>> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] >>>>>>>> Sent: zondag 28 augustus 2016 20:23 >>>>>>>> To: Stefan <luke1...@posteo.de> >>>>>>>> Cc: dev@subversion.apache.org >>>>>>>> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary >>>>>>>> files (patch >>>>>>>> v4) >>>>>>>> >>>>>>>> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200: >>>>>>>>> The regression test was tested against 1.9.4, 1.9.x and trunk >>>>>>>>> r1743999. >>>>>>>>> >>>>>>>>> I also tried to run the test against 1.8.16 but there it fails (didn't >>>>>>>>> investigate in detail). >>>>>>>>> Trunk r1758069 caused some build issues on my machine. Therefore I >>>>>>>>> couldn't validate/check the patch against the latest trunk (maybe it's >>>>>>>>> just some local issue with my build machine rather than some actual >>>>>>>>> problem on trunk - didn't look into that yet). >>>>>>>> For future reference, you might have tried building trunk@HEAD after >>>>>>>> locally reverting r1758069; i.e.: >>>>>>>> >>>>>>>> svn up >>>>>>>> svn merge -c -r1758069 >>>>>>>> >>>>>>>> make check >>>>>>>> >>>>>>>> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200: >>>>>>>>> Got approved by Bert. >>>>>>>>> >>>>>>>> (Thanks for stating so on the thread.) >>>>>>>> >>>>>>>>> Separated the repro test from the actual fix in order to have the >>>>>>>>> possibility to selectively only backport the regression test to the >>>>>>>>> 1.8 >>>>>>>>> branch. >>>>>>>> Good call, but the fix and the "remove XFail markers" (r1758129 and >>>>>>>> r1758130) should have been done in a single revision: they _are_ >>>>>>>> a single logical change. That would also avoid breaking 'make check' >>>>>>>> (at r1758129 'make check' exits non-zero because of the XPASS). >>>>>>> I do this the same way sometimes, when I want to use the separate >>>>>>> revision for backporting... But usually I commit things close enough >>>>>>> that nobody notices the bot results ;-) >>>>>>> (While the initial XFail addition is still running, you can commit the >>>>>>> two follow ups, and the buildbots collapses all the changes to a single >>>>>>> build) >>>>>>> >>>>>>> I just committed the followup patch posted in another thread to unbreak >>>>>>> the bots for the night... >>>>>>> >>>>>>> Bert >>>>>> Attached is a patch which should resolve the test case you added, Bert. >>>>>> Anybody feels like approving it? Or is there something I should >>>>>> improve/change? >>>>>> >>>>>> [[[ >>>>>> >>>>>> Add support for the svn_client_conflict_option_working_text resolution >>>>>> for >>>>>> binary file conflicts. >>>>>> >>>>>> * subversion/libsvn_client/conflicts.c >>>>>> (): Add svn_client_conflict_option_working_text to >>>>>> binary_conflict_options >>>>>> >>>>>> * subversion/tests/cmdline/resolve_tests.py >>>>>> (automatic_binary_conflict_resolution): Remove XFail marker >>>>>> >>>>>> ]]] >>>>>> >>>>> It
Re: [PATCH v3] Conflict option labels
On 14 October 2016 at 11:43, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 13 October 2016 at 17:26, Patrick Steinhardt <p...@pks.im> wrote:
>> Hi,
>>
>> the third version re-adds the result pool to
>> `svn_client_conflict_option_get_lazel`.
>>
> [...]
>> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
>> }
>>else
>> {
>> - opt = options++;
>> - if (! opt->code)
>> + if (i >= options->nelts)
>> break;
>> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
>> + i++;
>> }
>>
>>if (! first)
>> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
>>s = apr_psprintf(pool, " (%s) %s", opt->code,
>> - opt->short_desc ? _(opt->short_desc) :
>> opt->long_desc);
>> + opt->label ? _(opt->label) : opt->long_desc);
> The opt->label is already localized, so _() is not needed.
>
> Beside of that patch looks fine and I'm ready to commit it in current
> state. Stefan, do you have any comments on the patch?
>
Here is updated patch and log message that adapted to r1764848 changes.
[[
Move conflict resolution options' labels out of the client
* subversion/include/svn_client.h:
(svn_client_conflict_option_get_label): New function.
* subversion/libsvn_client/conflicts.c:
(svn_client_conflict_option_t): Add label.
(add_resolution_option): Add label argument.
(svn_client_conflict_option_get_label): New function.
(svn_client_conflict_text_get_reslution_options,
svn_client_conflict_prop_get_resolution_options,
configure_option_accept_current_wc_state,
configure_option_move_destination,
configure_option_update_raise_moved_away_children,
configure_option_incoming_add_ignore,
configure_option_incoming_added_file_text_merge,
configure_option_incoming_added_file_replace_and_merge,
configure_option_incoming_added_dir_merge,
configure_option_incoming_added_dir_replace,
configure_option_incoming_added_dir_replace_and_merge,
configure_option_incoming_delete_ignore,
configure_option_incoming_delete_accept,
configure_option_incoming_move_file_merge,
configure_option_incoming_dir_merge,
configure_option_local_move_file_merge,
svn_client_conflict_tree_get_resolution_options): Set
resolution option labels.
* subversion/svn/conflict-callbacks.c:
(resolver_option_t): Remove short_desc and long_desc.
(client_option_t): New struct for client options.
(builtin_resolver_options): Remove short_desc and long_desc.
(extra_resolver_options,
extra_resolver_options_text,
extra_resolver_options_prop,
extra_resolver_options_tree): Convert to client_option_t.
(find_option): Accept options as apr_array_header_t.
(find_option_by_builtin): New function to create provided
options from builtin library options.
(find_option_by_id): Replaced by find_option_by_builtin.
(prompt_string,
help_string,
prompt_user,
build_text_conflict_options,
build_prop_conflict_options,
build_prop_text_conflict_options,
handle_one_prop_conflict.
build_tree_conflict_options,
handle_tree_conflict): Accept options as apr_array_header_t.
]]
--
Ivan Zhakov
Index: subversion/include/svn_client.h
===
--- subversion/include/svn_client.h (revision 1764848)
+++ subversion/include/svn_client.h (working copy)
@@ -4720,6 +4720,20 @@
svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
/**
+ * Return a textual human-readable label of @a option, allocated in
+ * @a result_pool. The label is encoded in UTF-8 and usually
+ * contains up to three words.
+ *
+ * Additionally, the label may be localized to the language used
+ * by the current locale.
+ *
+ * @since New in 1.10.
+ */
+const char *
+svn_client_conflict_option_get_label(svn_client_conflict_option_t *option,
+ apr_pool_t *result_pool);
+
+/**
* Return a textual human-readable description of @a option, allocated in
* @a result_pool. The description is encoded in UTF-8 and may contain
* multiple lines separated by @c APR_EOL_STR.
Index: subversion/libsvn_client/conflicts.c
===
--- subversion/libsvn_client/conflicts.c(revision 1764848)
+++ subversion/libsvn_client/conflicts.c(working copy)
@@ -122,6 +122,7 @@
struct svn_client_conflict_option_t
{
svn_client_conflict_option_id_t id;
+ const char *label;
const char *description;
svn_client_conflict_t *conflict;
@@ -7312,6 +7313,7 @@
add_resolution_option(apr_array_header_t *options,
svn_client_conflict_t *conflict,
Re: [PATCH v3] Conflict option labels
On 13 October 2016 at 17:26, Patrick Steinhardt <p...@pks.im> wrote:
> Hi,
>
> the third version re-adds the result pool to
> `svn_client_conflict_option_get_lazel`.
>
[...]
> @@ -582,15 +604,16 @@ prompt_string(const resolver_option_t *options,
> }
>else
> {
> - opt = options++;
> - if (! opt->code)
> + if (i >= options->nelts)
> break;
> + opt = APR_ARRAY_IDX(options, i, client_option_t *);
> + i++;
> }
>
>if (! first)
> result = apr_pstrcat(pool, result, ",", SVN_VA_NULL);
>s = apr_psprintf(pool, " (%s) %s", opt->code,
> - opt->short_desc ? _(opt->short_desc) :
> opt->long_desc);
> + opt->label ? _(opt->label) : opt->long_desc);
The opt->label is already localized, so _() is not needed.
Beside of that patch looks fine and I'm ready to commit it in current
state. Stefan, do you have any comments on the patch?
--
Ivan Zhakov
Re: [PATCH v3] Conflict option labels
On 14 October 2016 at 10:27, Stefan Sperling <s...@elego.de> wrote:
> On Thu, Oct 13, 2016 at 05:59:01PM +0200, Stefan wrote:
>> On 10/13/2016 5:26 PM, Patrick Steinhardt wrote:
>> > sion re-adds the result pool to
>> > `svn_client_conflict_option_get_lazel`.
>>
>> > diff --git a/subversion/include/svn_client.h
>> > b/subversion/include/svn_client.h
>> > index 9bbe62b..f456c92 100644
>> > --- a/subversion/include/svn_client.h
>> > +++ b/subversion/include/svn_client.h
>> > @@ -4718,6 +4718,20 @@ svn_client_conflict_option_id_t
>> > svn_client_conflict_option_get_id(svn_client_conflict_option_t *option);
>> >
>> > [...]
>> >
>> >add_resolution_option(*options, conflict,
>> > svn_client_conflict_option_merged_text,
>> > +_("Mark as resolved"),
>> > _("accept binary file as it appears in the working copy"),
>> > resolve_text_conflict);
>> Not sure whether "Mark as resolved" means much to the user. Maybe
>> "Accept/Use current version" would be easier to get? Or maybe
>> "Accept/Use current" to keep the label consistent with the style of the
>> others.
>
> To avoid yet another round-trip for Patrick, I'd prefer us to focus on
> the mechanical nature of this change, which is about moving these labels
> as-is into the library and exposing them via API.
>
> We can still tweak these strings after this change is committed (and I
> agree they should be changed).
+1.
--
Ivan Zhakov
Re: [PATCH v2] Conflict option labels
On 13 October 2016 at 15:46, Patrick Steinhardt <patrick.steinha...@elegosoft.com> wrote: > Hi, > > here's the second version of the conflict option label patch. > Changes: > > - reworded some labels > - now using apr_array to pass around options > - renamed and simplified svn_client_resolver_option_label > > The functionality has been lightly tested by creating conflict > scenarios. > Quick review: > + * Return a textual human-readable label of @a option, allocated in > + * @a result_pool. The label is encoded in UTF-8 and usually > + * contains up to three words. > + * > + * Additionally, the label may be localized to the language used > + * by the current locale. > + * > + * @since New in 1.10. > + */ > +const char * > +svn_client_conflict_option_get_label(svn_client_conflict_option_t *option); The docstring mentions RESULT_POOL, but there is no such argument. I think it would be better to RESULT_POOL for this function. This would help to avoid slightly incorrect pool usage like in find_option_by_builtin(): [[[ client_opt = apr_pcalloc(result_pool, sizeof(*client_opt)); client_opt->choice = id; client_opt->code = opt->code; client_opt->label = svn_client_conflict_option_get_label( builtin_option); ^ the label is not copied to RESULT_POOL. SVN_ERR(svn_client_conflict_option_describe(_opt->long_desc, builtin_option, result_pool, scratch_pool)); ]]] -- Ivan Zhakov
Re: svn commit: r1764676 - /subversion/trunk/subversion/libsvn_fs_fs/pack.c
On 13 October 2016 at 15:49, <stef...@apache.org> wrote:
> Author: stefan2
> Date: Thu Oct 13 13:49:47 2016
> New Revision: 1764676
>
> URL: http://svn.apache.org/viewvc?rev=1764676=rev
> Log:
> Make the FSFS pack no longer depend on a working file trunc() operation.
>
> * subversion/libsvn_fs_fs/pack.c
> (reset_pack_context): Instead of emptying the temporary files, close,
> auto-delete and re-create them.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/pack.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/pack.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/pack.c?rev=1764676=1764675=1764676=diff
> ==
> --- subversion/trunk/subversion/libsvn_fs_fs/pack.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/pack.c Thu Oct 13 13:49:47 2016
> @@ -341,20 +341,38 @@ static svn_error_t *
> reset_pack_context(pack_context_t *context,
> apr_pool_t *pool)
> {
> + const char *temp_dir;
> +
>apr_array_clear(context->changes);
> - SVN_ERR(svn_io_file_trunc(context->changes_file, 0, pool));
> + SVN_ERR(svn_io_file_close(context->changes_file, pool));
>apr_array_clear(context->file_props);
> - SVN_ERR(svn_io_file_trunc(context->file_props_file, 0, pool));
> + SVN_ERR(svn_io_file_close(context->file_props_file, pool));
>apr_array_clear(context->dir_props);
> - SVN_ERR(svn_io_file_trunc(context->dir_props_file, 0, pool));
> + SVN_ERR(svn_io_file_close(context->dir_props_file, pool));
>
>apr_array_clear(context->rev_offsets);
>apr_array_clear(context->path_order);
>apr_array_clear(context->references);
>apr_array_clear(context->reps);
> - SVN_ERR(svn_io_file_trunc(context->reps_file, 0, pool));
> + SVN_ERR(svn_io_file_close(context->reps_file, pool));
>
>svn_pool_clear(context->info_pool);
> +
> + /* The new temporary files must live at least as long as any other info
> + * object in CONTEXT. */
> + SVN_ERR(svn_io_temp_dir(_dir, pool));
> + SVN_ERR(svn_io_open_unique_file3(>changes_file, NULL, temp_dir,
> + svn_io_file_del_on_close,
> + context->info_pool, pool));
> + SVN_ERR(svn_io_open_unique_file3(>file_props_file, NULL, temp_dir,
> + svn_io_file_del_on_close,
> + context->info_pool, pool));
> + SVN_ERR(svn_io_open_unique_file3(>dir_props_file, NULL, temp_dir,
> + svn_io_file_del_on_close,
> + context->info_pool, pool));
> + SVN_ERR(svn_io_open_unique_file3(>reps_file, NULL, temp_dir,
> + svn_io_file_del_on_close,
> + context->info_pool, pool));
Probably we should update code in initialize_pack_context() to use
CONTEX->INFO_POOL when opening these files initially?
--
Ivan Zhakov
Re: [PATCH v2] Conflict option labels
On 13 October 2016 at 15:52, Patrick Steinhardt <p...@pks.im> wrote: > On Thu, Oct 13, 2016 at 03:46:32PM +0200, Patrick Steinhardt wrote: >> Hi, >> [..] > [snip] > > By the way, one problem that still exists is consistency. Right > now, we have a mixture of labels where the first character is > uppercased and labels where the first character is lowercased. > With GUI clients in mind I personally lend towards using > uppercased labels, but I'd need to adjust remaining labels to > provide them (maybe in a separate patch, this one is big enough > already as-is). > > Any opinions? > I'm for uppercased labels. -- Ivan Zhakov
Re: [RFC] Subversion command line UI for interactive conflict resolution
On 13 October 2016 at 15:23, Stefan Sperling <s...@elego.de> wrote: > On Thu, Oct 13, 2016 at 03:01:39PM +0200, Ivan Zhakov wrote: >> I suggest to change behavior to something like the following: >> [[[ >> $ svn resolve >> Searching tree conflict details for >> 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository: >> Checking r5... done >> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': >> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved >> to '^/branches/b1/bar' by ivan in r5. >> A file which differs from the corresponding file on the merge source >> branch was found in the working copy. >> >> Resolution options: >> (p) - postpone >> (r) - mark as resolved >> (m) - move and merge >> (h) - help >> (q) - postpone all remaining conflicts >> >> Select: >> ]]] >> >> When user types 'h' the some prompt will be shown, but with more >> detailed description: >> [[[ >> Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': >> File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved >> to '^/branches/b1/bar' by ivan in r5. >> A file which differs from the corresponding file on the merge source >> branch was found in the working copy. >> >> Resolution options: >> (p) - postpone >> skip this conflict and leave it unresolved [postpone] >> (r) - mark as resolved >> accept current working copy state [working] >> (m) - move and merge >> move 'foo' to 'bar' and merge >> (h) - help >> (q) - postpone all remaining conflicts >> >> Select: >> ]]] > > +1 > > It might also be nice to show a detailed conflict description only > if the user asks for help: > > File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved > to '^/branches/b1/bar' by ivan in r5. > A file which differs from the corresponding file on the merge source > branch was found in the working copy. > > By default we could show an abbreviated version of this description. Good idea. But we need short_description API to implement this :) -- Ivan Zhakov
[RFC] Subversion command line UI for interactive conflict resolution
Hi, I'm thinking new conflict resolution should look like in Subversion command line client. Currently 'svn resolve' works like the following: [[[ $ svn resolve Searching tree conflict details for 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository: Checking r5... done Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved to '^/branches/b1/bar' by ivan in r5. A file which differs from the corresponding file on the merge source branch was found in the working copy. Select: (p) postpone, (r) accept current working copy state, (m) move 'foo' to 'bar' and merge, (h) help, (q) quit resolution: ]]] Then when user types 'h' it will see expanded conflict resolutions options with one option on each line: [[[ File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved to '^/branches/b1/bar' by ivan in r5. A file which differs from the corresponding file on the merge source branch was found in the working copy. (p) - skip this conflict and leave it unresolved [postpone] (r) - accept current working copy state [working] (m) - move 'foo' to 'bar' and merge (h) - show this help (also '?') (q) - postpone all remaining conflicts Words in square brackets are the corresponding --accept option arguments. Select: (p) postpone, (r) accept current working copy state, (m) move 'foo' to 'bar' and merge, (h) help, (q) quit resolution: ]]] I suggest to change behavior to something like the following: [[[ $ svn resolve Searching tree conflict details for 'D:\ivan\svn\test-wc\add-versus-add\foo' in repository: Checking r5... done Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved to '^/branches/b1/bar' by ivan in r5. A file which differs from the corresponding file on the merge source branch was found in the working copy. Resolution options: (p) - postpone (r) - mark as resolved (m) - move and merge (h) - help (q) - postpone all remaining conflicts Select: ]]] When user types 'h' the some prompt will be shown, but with more detailed description: [[[ Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved to '^/branches/b1/bar' by ivan in r5. A file which differs from the corresponding file on the merge source branch was found in the working copy. Resolution options: (p) - postpone skip this conflict and leave it unresolved [postpone] (r) - mark as resolved accept current working copy state [working] (m) - move and merge move 'foo' to 'bar' and merge (h) - help (q) - postpone all remaining conflicts Select: ]]] Alternative layout: [[[ Tree conflict on 'D:\ivan\svn\test-wc\add-versus-add\foo': File merged from '^/trunk/foo@2' to '^/branches/b1/foo@16' was moved to '^/branches/b1/bar' by ivan in r5. A file which differs from the corresponding file on the merge source branch was found in the working copy. Resolution options: (p) - Postpone: skip this conflict and leave it unresolved. [postpone] (r) - Mark as resolved: accept current working copy state. [working] (m) - Move and merge: move 'foo' to 'bar' and merge. (h) - Help (q) - Postpone all remaining conflicts Select: ]]] -- Ivan Zhakov
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
On 13 October 2016 at 14:00, Branko Čibej <br...@apache.org> wrote: > On 13.10.2016 13:28, Johan Corveleyn wrote: >> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <i...@apache.org> wrote: >>> On 12 October 2016 at 22:30, Johan Corveleyn <jcor...@gmail.com> wrote: >>>> On Wed, Oct 12, 2016 at 10:13 PM, <i...@apache.org> wrote: >>>>> Author: ivan >>>>> Date: Wed Oct 12 20:13:24 2016 >>>>> New Revision: 1764536 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1764536=rev >>>>> Log: >>>>> * STATUS: Veto r1759116 group. >>>>> >>>>> Modified: >>>>> subversion/branches/1.9.x/STATUS >>>> ... >>>>> @@ -135,6 +119,24 @@ Candidate changes: >>>>> Veto-blocked changes: >>>>> = >>>>> >>>>> + * r1759116, r1759117, r1759122, r1759123, r1759124 >>>>> + Fix FSFS format 7 packing for revisions packs with lots of changes. >>>>> + Justification: >>>>> + Problem occurred in at least two user repositories. Without the >>>>> fix, >>>>> + format 7 repositories with an exceptionally large number of changes >>>>> in >>>>> + a pack cannot be packed - which renders using f7 pointless for those >>>>> + users. >>>>> + Branch: >>>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes >>>>> + Notes: >>>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t >>>>> internal state. >>>>> + r1759117-23 provide the actual fixes. >>>>> + r1759124 adds a regression test with the necessary internal API >>>>> changes. >>>>> + Votes: >>>>> + +1: stefan2 >>>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR, >>>>> + not by unreliable workaround with unknown consequences) >>>>> + >>> [...] >>>> So that leaves you claiming that it's an "unreliable workaround with >>>> unknown consequences". Why? Please clarify. We are talking about a >>>> bugfix which fixes an important problem reported by users. Do you have >>>> any suggestions for improvement? Perhaps the fix should be technically >>>> discussed on the mailing list? Or should we just leave this important >>>> problem unfixed? >>> I think it's almost always better to solve the problem in its origin. >>> Any workaround is unreliable, so we should always try to fix the >>> problem at the root before trying the workarounds. Note that we are >>> talking about a relatively obvious bug in other Apache's project, not >>> about something irrecoverable. >>> >>> As far as I know, this problem has never been reported to the APR's >>> dev list. And this is the first action that we should have done. >>> >>>> Do you have any suggestions for improvement? >>> I have plans to send a patch to APR in order to solve this problem. So >>> we don't need to release this workaround. >> Okay, I understand this should be fixed in APR. >> >> But should we block a workaround in our code for a bug that causes >> problems in the wild for existing svn 1.9 installations? Those users >> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this >> by fixing it in APR, creating a patch release of APR, and then >> creating a 1.9.x svn release with that fix (in a reasonable >> timeframe), then fine. But otherwise I think a svn-internal-workaround >> is warranted. > > > There's no chance of doing this in a reasonable time-frame because we > require APR-1.3 and no fix we can possibly come up with will be released > in a 1.3 patch. > > We have any number of workarounds for APR quirks in the code. I can't > think of a single valid reason to veto another one, especially since > we'd /still/ have to have the workaround in our code for people who use > older APR versions (likely versions prior to 1.5 or even 1.6, depending > on what the fix ends up looking like). > > "Any workaround is unreliable" is just nonsense, IMHO. > It seems that you are pulling my phrase out of context. Do you suggest to rely on workarounds instead of fixing problems at they roots? Note that this particular discussion is about our workaround, not about something officially suggested by APR devs. -- Ivan Zhakov
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
On 12 October 2016 at 22:30, Johan Corveleyn <jcor...@gmail.com> wrote: > On Wed, Oct 12, 2016 at 10:13 PM, <i...@apache.org> wrote: >> >> Author: ivan >> Date: Wed Oct 12 20:13:24 2016 >> New Revision: 1764536 >> >> URL: http://svn.apache.org/viewvc?rev=1764536=rev >> Log: >> * STATUS: Veto r1759116 group. >> >> Modified: >> subversion/branches/1.9.x/STATUS > ... >> @@ -135,6 +119,24 @@ Candidate changes: >> Veto-blocked changes: >> = >> >> + * r1759116, r1759117, r1759122, r1759123, r1759124 >> + Fix FSFS format 7 packing for revisions packs with lots of changes. >> + Justification: >> + Problem occurred in at least two user repositories. Without the fix, >> + format 7 repositories with an exceptionally large number of changes in >> + a pack cannot be packed - which renders using f7 pointless for those >> + users. >> + Branch: >> + ^/subversion/branches/1.9.x-fsfs-pack-fixes >> + Notes: >> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal >> state. >> + r1759117-23 provide the actual fixes. >> + r1759124 adds a regression test with the necessary internal API >> changes. >> + Votes: >> + +1: stefan2 >> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR, >> + not by unreliable workaround with unknown consequences) >> + > [...] > So that leaves you claiming that it's an "unreliable workaround with > unknown consequences". Why? Please clarify. We are talking about a > bugfix which fixes an important problem reported by users. Do you have > any suggestions for improvement? Perhaps the fix should be technically > discussed on the mailing list? Or should we just leave this important > problem unfixed? I think it's almost always better to solve the problem in its origin. Any workaround is unreliable, so we should always try to fix the problem at the root before trying the workarounds. Note that we are talking about a relatively obvious bug in other Apache's project, not about something irrecoverable. As far as I know, this problem has never been reported to the APR's dev list. And this is the first action that we should have done. > Do you have any suggestions for improvement? I have plans to send a patch to APR in order to solve this problem. So we don't need to release this workaround. -- Ivan Zhakov
Stop backport.pl running as cronjob? (was Re: svn commit: r1764631 - /subversion/branches/1.9.x-r1721488/)
On 13 October 2016 at 10:59, <jcor...@apache.org> wrote: > Author: jcorvel > Date: Thu Oct 13 08:59:07 2016 > New Revision: 1764631 > > URL: http://svn.apache.org/viewvc?rev=1764631=rev > Log: > Resurrect the '1.9.x-r1721488' branch, to give backport.pl another > chance to execute the correct backport commands, after backport mess. > I'm wondering if we really need backport.pl running as cronjob to merge backports automatically to the stable branches? It's not a first time when this automatic job performs invalid merges. And as far I understand we still spend some time to babysit this tool, fix bugs etc. What is wrong with old proven process by merging revisions manually? -- Ivan Zhakov
Re: [PATCH] Conflict option labels
On 12 October 2016 at 17:06, James McCoy <james...@jamessan.com> wrote: > On Wed, Oct 12, 2016 at 04:44:10PM +0200, Ivan Zhakov wrote: >> On 12 October 2016 at 15:38, Patrick Steinhardt >> <patrick.steinha...@elegosoft.com> wrote: >> > Hi, >> > >> > please find attached a patch pulling out the short descriptions >> > of conflict resolution options from the client and puts them into >> > libsvn_client. >> > >> > [[ >> > Move conflict resolution options' labels out of the client >> > >> > * include/svn_client.h: >> > - Provide function `svn_client_conflict_option_label` >> > * libsvn_client/conflicts.c: >> > - Implement function `svn_client_conflict_option_label` >> > - Introduce and set label field for svn_conflict_option_t >> > * svn/conflict-callbacks.c: >> > - Split client-specific and built-in resolver options >> > - Implement conversion from built-in resolvers to >> > client-specific options >> > ]] >> >> Hi Patrick. Thank you for the patch. >> >> See my review inline: >> > Index: subversion/include/svn_client.h >> > === >> > --- subversion/include/svn_client.h(revision 1764453) >> > +++ subversion/include/svn_client.h(working copy) >> > @@ -4718,6 +4718,22 @@ >> > svn_client_conflict_option_get_id(svn_client_conflict_option_t *option); >> > >> > /** >> > + * Return a textual human-readable label of @a option, allocated in >> > + * @a result_pool. The label is encoded in UTF-8 and may contain >> > + * up to three words. >> May replace 'may contain up to three words' -> 'usually contain up to >> three words'? > > 'usually contain' -> 'usually contains'. > Sure! -- Ivan Zhakov
Re: [PATCH] Conflict option labels
n NULL if not found. */
> -static const resolver_option_t *
> -find_option_by_id(const resolver_option_t *options,
> - svn_client_conflict_option_id_t choice)
> +static svn_error_t *
> +find_option_by_builtin(struct client_option_t **out,
> + const resolver_option_t *options,
> + svn_client_conflict_option_t *builtin_option,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
>const resolver_option_t *opt;
> + svn_client_conflict_option_id_t id;
>
> + *out = NULL;
I think it would be more clear to set '*out = NULL' at the end of the
function. But the all code will be simplified if we drop svn_error_t
from svn_client_option_get_label().
> +
> + id = svn_client_conflict_option_get_id(builtin_option);
> +
>for (opt = options; opt->code; opt++)
> {
> - if (opt->choice == choice)
> -return opt;
> + if (opt->choice == id)
> +{
> + client_option_t *client_opt;
> +
> + client_opt = apr_pcalloc(result_pool, sizeof(*client_opt));
> + client_opt->choice = id;
> + client_opt->code = opt->code;
> + SVN_ERR(svn_client_conflict_option_label(_opt->label,
> + builtin_option,
> + result_pool,
> + scratch_pool));
Indentation problem: function arguments should be aligned:
[[
SVN_ERR(svn_client_conflict_option_label(_opt->label,
builtin_option,
result_pool,
scratch_pool));
]]
> + SVN_ERR(svn_client_conflict_option_describe(_opt->long_desc,
> + builtin_option,
> + result_pool,
> + scratch_pool));
> + client_opt->accept_arg = opt->accept_arg;
> +
> + *out = client_opt;
> +
> + return SVN_NO_ERROR;
> +}
> }
> - return NULL;
> + return SVN_NO_ERROR;
> }
>
[...]
> @@ -693,7 +711,7 @@
>
> /* Set *OPTIONS to an array of resolution options for CONFLICT. */
> static svn_error_t *
> -build_text_conflict_options(resolver_option_t **options,
> +build_text_conflict_options(client_option_t **options,
> svn_client_conflict_t *conflict,
> svn_client_ctx_t *ctx,
> svn_boolean_t is_binary,
> @@ -700,8 +718,8 @@
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> - resolver_option_t *opt;
> - const resolver_option_t *o;
> + client_option_t *opt;
> + const client_option_t *o;
>apr_array_header_t *builtin_options;
>apr_size_t nopt;
>int i;
> @@ -714,7 +732,7 @@
>nopt = builtin_options->nelts + ARRAY_LEN(extra_resolver_options);
>if (!is_binary)
> nopt += ARRAY_LEN(extra_resolver_options_text);
> - *options = apr_pcalloc(result_pool, sizeof(*opt) * (nopt + 1));
> + *options = apr_pcalloc(result_pool, sizeof(opt) * (nopt + 1));
This change looks suspicions: Are you sure that you passing write size
to apr_pcalloc()?
[...]
svn_client_conflict_option_label
[1]
https://subversion.apache.org/docs/community-guide/conventions.html#log-messages
--
Ivan Zhakov
Re: [PATCH] Remove unused variables in conflict resolution code
On 12 October 2016 at 15:05, Patrick Steinhardt <patrick.steinha...@elegosoft.com> wrote: > Hi, > > see the attached patch for fixes to some unused-variable > warnings. > > [[ > Remove variables which are written but not used in any way. > > * subversion/libsvn_client/conflicts.c: > Remove unused variables > ]] Hi, Patrick. The log message should mention all affected functions/symbols. See 'Writing log messages' section in Community Guide [1] [1] https://subversion.apache.org/docs/community-guide/conventions.html#log-messages -- Ivan Zhakov
Re: New SHA1 property for nodes returned 'svn ls --xml' invocations.
On 12 October 2016 at 14:03, Paul Hammant <p...@hammant.org> wrote: > It's very exciting to hear that Subversion already calculates shas somewhere > in the backend :) I noted this multiple times on this thread: SHAs are optional at the repository backend layer. -- Ivan Zhakov
Re: [PATCH] Fix number of array elements
On 12 October 2016 at 12:58, Ivan Zhakov <i...@visualsvn.com> wrote: > On 12 October 2016 at 12:48, Stefan <luke1...@gmx.de> wrote: >> Hi, >> >> hope this patch is correct... As far as I see the number of elements is >> off by one here: >> >> [[[ >> Allocate the correct number of element entries. >> >> * subversion/libsvn_client/conflicts.c >> (svn_client_conflict_text_get_resolution_options): Correct number of array >> entires. >> ]]] > I think that log message could be improvement: because this not the > number of how many entries is allocated. This is initial size of > array, and apr_array will expand it if needed. > I've tweaked log message and committed change in r1764439. Thanks. -- Ivan Zhakov
Re: [PATCH] Fix number of array elements
On 12 October 2016 at 12:48, Stefan <luke1...@gmx.de> wrote: > Hi, > > hope this patch is correct... As far as I see the number of elements is > off by one here: > > [[[ > Allocate the correct number of element entries. > > * subversion/libsvn_client/conflicts.c > (svn_client_conflict_text_get_resolution_options): Correct number of array > entires. > ]]] I think that log message could be improvement: because this not the number of how many entries is allocated. This is initial size of array, and apr_array will expand it if needed. PS: Please submit patches with text/plain mime type as recommended in Community Guide [1] [1] https://subversion.apache.org/docs/community-guide/general.html#patches-submission -- Ivan Zhakov
Re: [PATCH] Resolve issue #4647 on trunk
On 10 October 2016 at 17:53, Stefan <luke1...@posteo.de> wrote: > On 8/28/2016 11:32 PM, Bert Huijben wrote: >> >>> -Original Message- >>> From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] >>> Sent: zondag 28 augustus 2016 20:23 >>> To: Stefan <luke1...@posteo.de> >>> Cc: dev@subversion.apache.org >>> Subject: Re: [PATCH] Fix a conflict resolution issue related to binary >>> files (patch >>> v4) >>> >>> Stefan wrote on Sun, Aug 28, 2016 at 13:31:39 +0200: >>>> The regression test was tested against 1.9.4, 1.9.x and trunk r1743999. >>>> >>>> I also tried to run the test against 1.8.16 but there it fails (didn't >>>> investigate in detail). >>>> Trunk r1758069 caused some build issues on my machine. Therefore I >>>> couldn't validate/check the patch against the latest trunk (maybe it's >>>> just some local issue with my build machine rather than some actual >>>> problem on trunk - didn't look into that yet). >>> For future reference, you might have tried building trunk@HEAD after >>> locally reverting r1758069; i.e.: >>> >>> svn up >>> svn merge -c -r1758069 >>> >>> make check >>> >>> Stefan wrote on Sun, Aug 28, 2016 at 18:33:55 +0200: >>>> Got approved by Bert. >>>> >>> (Thanks for stating so on the thread.) >>> >>>> Separated the repro test from the actual fix in order to have the >>>> possibility to selectively only backport the regression test to the 1.8 >>>> branch. >>> Good call, but the fix and the "remove XFail markers" (r1758129 and >>> r1758130) should have been done in a single revision: they _are_ >>> a single logical change. That would also avoid breaking 'make check' >>> (at r1758129 'make check' exits non-zero because of the XPASS). >> I do this the same way sometimes, when I want to use the separate revision >> for backporting... But usually I commit things close enough that nobody >> notices the bot results ;-) >> (While the initial XFail addition is still running, you can commit the two >> follow ups, and the buildbots collapses all the changes to a single build) >> >> I just committed the followup patch posted in another thread to unbreak the >> bots for the night... >> >> Bert > > Attached is a patch which should resolve the test case you added, Bert. > Anybody feels like approving it? Or is there something I should > improve/change? > > [[[ > > Add support for the svn_client_conflict_option_working_text resolution for > binary file conflicts. > > * subversion/libsvn_client/conflicts.c > (): Add svn_client_conflict_option_working_text to binary_conflict_options > > * subversion/tests/cmdline/resolve_tests.py > (automatic_binary_conflict_resolution): Remove XFail marker > > ]]] > It seems this patch breaks interactive conflict resolve: With trunk I get the following to 'svn resolve' on binary file: [[[ Merge conflict discovered in binary file 'A_COPY\theta'. Select: (p) postpone, (r) accept binary file as it appears in the working copy, (tf) accept incoming version of binary file: h (p) - skip this conflict and leave it unresolved [postpone] (tf) - accept incoming version of binary file [theirs-full] (r) - accept binary file as it appears in the working copy [working] (q) - postpone all remaining conflicts ]]] But with patch I get the following: [[[ Merge conflict discovered in binary file 'A_COPY\theta'. Select: (p) postpone, (r) accept binary file as it appears in the working copy, (tf) accept incoming version of binary file: h (p) - skip this conflict and leave it unresolved [postpone] (tf) - accept incoming version of binary file [theirs-full] (mf) - accept binary file as it appears in the working copy [mine-full] (r) - accept binary file as it appears in the working copy [working] (q) - postpone all remaining conflicts ]]] I think it's confusing and we should not offer the same option twice. -- Ivan Zhakov
Re: New SHA1 property for nodes returned 'svn ls --xml' invocations.
On 24 September 2016 at 15:36, Paul Hammant <p...@hammant.org> wrote: > In order to be able to do some Merkle-tree style functions on sets of files > canonically held in Subversion, it would be great to ask Svn for a SHA1 for > the files, or collections thereof from that node downwards. > > I would raise a new feature request direct into Svn, but the JIRA notes says > to not do that, and instead to come here to discuss. > > More info: the technology I'm playing with doesn't do a svn checkout, but > instead monitors the the repo via 'svn ls' (via polling). It is easiest to > hit up the root note and ask for a sha1, then walk the tree (remotely) to > get the actually changes nodes deeper in the tree. Sure, the revision > integer is there too - but I need to compare to a *local* representation of > the same tree that's not under subversion control, and I'll have to > calculate SHA1 of the resource immediately after bringing it down from the > server (rather that just trusting the server's version). > > Of course, I'm focussed on 'svn ls' and I am sure there are other functions > that could report the SHA1 too. > > Someone else might say SHA-2 or 3, and I'm happy to bow to their expertise. > The problem that SHA-1 checksum for files is optional: older repositories/servers may not have this information stored. -- Ivan Zhakov
Re: Check SHA vs Content (was: RE: svn commit: r1759233 - /subversion/trunk/subversion/libsvn_wc/questions.c)
On 5 September 2016 at 19:23, Ivan Zhakov <i...@visualsvn.com> wrote: > On 5 September 2016 at 14:46, Bert Huijben <b...@qqmail.nl> wrote: >>> -Original Message- >>> From: i...@apache.org [mailto:i...@apache.org] >>> Sent: maandag 5 september 2016 13:33 >>> To: comm...@subversion.apache.org >>> Subject: svn commit: r1759233 - >>> /subversion/trunk/subversion/libsvn_wc/questions.c >>> >>> Author: ivan >>> Date: Mon Sep 5 11:32:54 2016 >>> New Revision: 1759233 >>> >>> URL: http://svn.apache.org/viewvc?rev=1759233=rev >>> Log: >>> Use SHA-1 checksum to find whether files are actually modified in working >>> copy if timestamps don't match. >>> >>> Before this change we were doing this: >>> 1. Compare file timestamps: if they match, assume that files didn't change. >>> 2. Open pristine file. >>> 3. Read properties from wc.db and find whether translation is required. >>> 4. Compare filesize with pristine filesize for files that do not >>>require translation. Assume that file is modified if the sizes differ. >>> 5. Compare detranslated contents of working file with pristine. >>> >>> Now behavior is the following: >>> 1. Compare file timestamps: if they match, assume that files didn't change. >>> 3. Read properties from wc.db and find whether translation is required. >>> 3. Compare filesize with pristine filesize for files that do not >>>require translation. Assume that file is modified if the sizes differ. >>> 4. Calculate SHA-1 checksum of detranslated contents of working file >>>and compare it with pristine's checksum stored in wc.db. >> > Hi Bert, > >> We looked at this before, and this change has pro-s and con-s, depending on >> specific use cases. >> > Thanks for bringing this to dev@ list, I was not aware that this topic > was discussed before. > [...] >> If the file happens to be a database file or something similar >> there is quite commonly a change in the first 'block', when >> there are changes somewhere later on. (Checksum, change >> counter, etc.). File formats like sqlite were explicitly designed >> for this (and other cheap checks), with a change counter at the start. > >> I don't think we should 'just change behavior' here, if we don't >> have actual usage numbers for our users. Perhaps we should make >> this feature configurable... or depending on filesize. >> > > Let me summarize all possible cases that I considered before my > change. First of all some definitions: > * Text file (T) -- text file that require translation, due to eol > style or keywords expansion > * Text file (N) -- text file that doesn't require translation > * Binary file -- some kind of binary file (database, pdf, zip, docx). > Let's assume that user doesn't configure svn:eol-style and > svn:keywords for them. > * WS -- size of working file > * PS -- size of pristine file > > * Old=xxx -- average required read size for old behavior in terms of > working and pristine file sizes > * New=xxx -- average required read size for new behavior in terms of > working and pristine file sizes > > 1. Text file (T), not modified: Old = WS + PS, New = WS > 2. Text file (N), not modified: Old = WS + PS, New = WS > 3. Binary file, not modified: Old = WS + PS, New = WS > 4. Text file (T), modified, same size: Old = 0.5 * WS + 0.5 * PS, New = WS > 5. Text file (N), modified, same size: Old = 0.5 * WS + 0.5 * PS, New = WS > 6. Binary file, modified, same size: Old = 0.5 * WS + 0.5 * PS, New = WS > 7. Text file (T), modified, different size: Old = 0.5 * WS + 0.5 * PS, New = > WS > 8. Text file (N), modified, different size: Old = 0, New = 0 > 9. Binary file, modified, different size: Old = 0, New = 0 > Hi Bert, I tested several different binary file formats for no-op/minimal change: 1. Microsoft Word (docx): change single character at the end of document: - filesize changes (case 9) - first change at offset 2,295 of 233,323 2. Microsoft Word (doc): change single character at the end of document: - filesize didn't change (case 6) - first change at offset 540 of 479,232 3. sqlite database: insert one row to wc.db (2.5mb) - filesize didn't change (case 6) - first change at offset 27 4. zip archive: change single character in one of many text files (43 mb uncompressed) - filesize changes (case 9) - first change at offset 7,182,933 of 10,352,080 5. pdf file: no-op change of 800kb file - filesize changes (case 9) - first change at offset 47 of 854,971 6. Photoshop image (psd): change one pixel in the middle - filesi
Re: Check SHA vs Content (was: RE: svn commit: r1759233 - /subversion/trunk/subversion/libsvn_wc/questions.c)
e: Old = 0.5 * WS + 0.5 * PS, New = WS 7. Text file (T), modified, different size: Old = 0.5 * WS + 0.5 * PS, New = WS 8. Text file (N), modified, different size: Old = 0, New = 0 9. Binary file, modified, different size: Old = 0, New = 0 (There is some overhead for SHA1 calculation: SHA1 performance is about 200-500 MB/s, but currently it's out of scope) With all above the new behavior should be working better or the same in all cases. I agree that 50% approximation may be incorrect for some specific binary formats (case 6) like sqlite db. > We certainly want the new behavior for non-pristine working copies > (on the IDEA list for years), but I'm not sure if we always want this > behavior as only option. > > This mail is partially, to just discuss this topic on the list, to make sure > everybody knows what happened here and why. -- Ivan Zhakov
Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c
On 29 August 2016 at 21:05, Stefan Sperling <s...@elego.de> wrote: > On Mon, Aug 29, 2016 at 02:44:28PM +0200, Stefan Sperling wrote: >> If you have time to fix it, please do! > > I got an opportunity to fix this myself: http://svn.apache.org/r1758269 Great, thanks! I started looking to this issue, but then was distracted by another task. -- Ivan Zhakov
Re: svn commit: r1758069 - /subversion/trunk/subversion/libsvn_subr/dirent_uri.c
On 28 August 2016 at 00:02, <s...@apache.org> wrote: > Author: stsp > Date: Sat Aug 27 21:02:27 2016 > New Revision: 1758069 > > URL: http://svn.apache.org/viewvc?rev=1758069=rev > Log: > Fix issue #4652 which shows how to trigger an assertion failure in > svn_dirent_get_absolute() by passing invalid input on the command line. > > Report a proper error message instead. > > * subversion/libsvn_subr/dirent_uri.c > (svn_dirent_get_absolute): If the caller passed a URL, return an error. > Hi Stefan, I'm not sure that it's proper way to fix the reported reported: svn_dirent_get_absolute() strictly requires @a absolute to be a *canonicalized dirent*: [[[ * Convert @a relative canonicalized dirent to an absolute dirent and * return the results in @a *pabsolute. ]]] Passing URL to svn_dirent_get_absolute() is an API violation and SVN_ERR_ASSERT() is a proper way to react. As far I remember all svn_dirent_*() functions are only expected to work with filesystem paths, svn_url_*() functions are designed to work with URLs, while svn_path_*() functions accepts filesystem path and URLs. User input should be validated where it's received, i.e. in Subversion command line client. We also may have problem above the stack, when this URL is passed to another function that doesn't have an assertion or something like that. In this particular case the problem is in svn_cl__merge() which passes URL as the TARGET_WCPATH argument to svn_client_merge5(). -- Ivan Zhakov
Re: Remove svn_client_ctx_t from svn_client_conflict_t
On 26 August 2016 at 13:36, Stefan Sperling <s...@elego.de> wrote: > On Fri, Aug 26, 2016 at 01:15:16PM +0300, Ivan Zhakov wrote: >> Hi Stefan, >> >> Currently svn_client_conflict_t stores a pointer to svn_client_ctx_t >> inside and uses it for every operation like >> svn_client_conflict_tree_get_details() or >> svn_client_conflict_*_resolve(). It may be useful for simple cases, >> but for other cases it makes using new API more difficult. That's >> because an API user needs to guarantee that lifetime of >> svn_client_ctx_t is longer than of the svn_client_conflict_t instance. >> >> I suggest changing svn_client_conflict_* API so that it wouldn't keep >> reference to svn_client_ctx_t and add it as argument to all API >> functions where it's required. >> >> I can do that, if there are no objections. What do you think? > > Sure. I have no objection. Done in r1758183. -- Ivan Zhakov
Re: svn commit: r1757532 - /subversion/trunk/subversion/mod_dav_svn/repos.c
On 26 August 2016 at 14:27, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote:
> Ivan Zhakov <i...@visualsvn.com> writes:
>
>>> @@ -3266,15 +3264,13 @@ static svn_error_t * __attribute__((war
>>> close_filter(void *baton)
>>> {
>>>diff_ctx_t *dc = baton;
>>> - apr_bucket_brigade *bb;
>>>apr_bucket *bkt;
>>>apr_status_t status;
>>>
>>>/* done with the file. write an EOS bucket now. */
>>> - bb = apr_brigade_create(dc->pool, dc->output->c->bucket_alloc);
>>>bkt = apr_bucket_eos_create(dc->output->c->bucket_alloc);
>>> - APR_BRIGADE_INSERT_TAIL(bb, bkt);
>>> - if ((status = ap_pass_brigade(dc->output, bb)) != APR_SUCCESS)
>>> + APR_BRIGADE_INSERT_TAIL(dc->bb, bkt);
>>> + if ((status = ap_pass_brigade(dc->output, dc->bb)) != APR_SUCCESS)
>>> return svn_error_create(status, NULL, "Could not write EOS to filter");
>>>
>> As far I understand apr_brigade_cleanup() should be called after
>> ap_pass_brigade(). So code should be something like:
>> [[[
>> APR_BRIGADE_INSERT_TAIL(dc->bb, bkt);
>> status = ap_pass_brigade(dc->output, dc->bb);
>> apr_brigade_cleanup(dc->bb);
>> if (status != APR_SUCCESS)
>> return svn_error_create(status, NULL, "Could not write EOS to filter");
>> ]]]
>
> Thanks, I will add the missing call in trunk.
>
> While it's not strictly required here (this is the stream's close_fn(), and
> we always call apr_brigade_destroy() upon returning from the deliver()
> hook), this may not hold in the future. Or someone might improperly use
> this code as a reference.
>
Ok, thanks! I added my +1 for backport anyway, since there is no
actual problem/memory leak in this particular case.
--
Ivan Zhakov
Remove svn_client_ctx_t from svn_client_conflict_t
Hi Stefan, Currently svn_client_conflict_t stores a pointer to svn_client_ctx_t inside and uses it for every operation like svn_client_conflict_tree_get_details() or svn_client_conflict_*_resolve(). It may be useful for simple cases, but for other cases it makes using new API more difficult. That's because an API user needs to guarantee that lifetime of svn_client_ctx_t is longer than of the svn_client_conflict_t instance. I suggest changing svn_client_conflict_* API so that it wouldn't keep reference to svn_client_ctx_t and add it as argument to all API functions where it's required. I can do that, if there are no objections. What do you think? -- Ivan Zhakov
Re: svn commit: r1757761 - /subversion/trunk/subversion/libsvn_client/conflicts.c
On 26 August 2016 at 10:53, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message- >> From: i...@apache.org [mailto:i...@apache.org] >> Sent: vrijdag 26 augustus 2016 00:08 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1757761 - >> /subversion/trunk/subversion/libsvn_client/conflicts.c >> >> Author: ivan >> Date: Thu Aug 25 22:08:19 2016 >> New Revision: 1757761 >> >> URL: http://svn.apache.org/viewvc?rev=1757761=rev >> Log: >> Simplify tree conflict resolution code a bit. >> >> * subversion/libsvn_client/conflicts.c >> (resolve_update_incoming_added_file_replace): Pass NULL as FILE to >>svn_io_open_uniquely_named() -- in this case it will close temporary file >>automatically. >> >> Modified: >> subversion/trunk/subversion/libsvn_client/conflicts.c >> >> Modified: subversion/trunk/subversion/libsvn_client/conflicts.c >> URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflic >> ts.c?rev=1757761=1757760=1757761=diff >> >> == >> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original) >> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Thu Aug 25 22:08:19 >> 2016 >> @@ -5402,7 +5402,6 @@ resolve_update_incoming_added_file_repla >>const char *lock_abspath; >>svn_client_ctx_t *ctx = conflict->ctx; >>svn_error_t *err; >> - apr_file_t *backup_file; >>const char *backup_path; >> >>option_id = svn_client_conflict_option_get_id(option); >> @@ -5422,7 +5421,7 @@ resolve_update_incoming_added_file_repla >> * which means it does not exist in the repository. So it's a good idea >> * to keep a backup, just in case someone picks this option by accident. >> * First, reserve a name in the filesystem. */ >> - err = svn_io_open_uniquely_named(_file, _path, >> + err = svn_io_open_uniquely_named(NULL, _path, >> svn_dirent_dirname(local_abspath, >>scratch_pool), >> svn_dirent_basename(local_abspath, >> @@ -5433,13 +5432,9 @@ resolve_update_incoming_added_file_repla >>if (err) >> goto unlock_wc; >> >> - /* Close and remove the file. We're going to move the conflict victim >> - * on top and, at least on Windows, open files can't be replaced. >> + /* Remove the file. We're going to move the conflict victim on top and, at >> + * least on Windows, open files can't be replaced. >> * The WC is locked so anything racing us here is external to SVN. */ >> - err = svn_io_file_close(backup_file, scratch_pool); >> - if (err) >> -goto unlock_wc; >> - >>err = svn_error_compose_create(err, svn_io_remove_file2(backup_path, >> TRUE, >>scratch_pool)); > > This remove file right after the change will not always work if the file is > not > explicitly closed before. On Windows it depends on apr opening the file > with delete share flags (10% perf hit in some cases) and nobody else opening > the file. > > In general strictly closing before deleting is a safer procedure. > Hi Bert, thanks for review! I'm aware about Windows behavior of deleting opened file, but it's not that case, because svn_io_open_uniquely_named() *explicitly* closes file @a file is NULL: [[[ * Open a new file (for reading and writing) with a unique name based on * utf-8 encoded @a filename, in the directory @a dirpath. The file handle is * returned in @a *file, and the name, which ends with @a suffix, is returned * in @a *unique_name, also utf8-encoded. Either @a file or @a unique_name * may be @c NULL. If @a file is @c NULL, the file will be created but not * open. ]]] -- Ivan Zhakov
Re: svn commit: r1756266 - /subversion/trunk/subversion/libsvn_fs_fs/transaction.c
On 17 August 2016 at 01:47, Daniel Shahaf <d...@daniel.shahaf.name> wrote: > i...@apache.org wrote on Sat, Aug 13, 2016 at 13:18:57 -: >> Author: ivan >> Date: Sat Aug 13 13:18:57 2016 >> New Revision: 1756266 >> >> URL: http://svn.apache.org/viewvc?rev=1756266=rev >> Log: >> FSFS: Write the sha1->rep mapping in transaction *after* we successfully >> written node revision to disk. Otherwise may get orphaned sha1->rep mapping >> entry if an error occurs when writing p2l index entry. > > Should this be backported? I.e., what are the consequences of an > orphaned sha1->rep entry? > > If it's possible for an orphaned entry to be added to rep-cache.db > [e.g., by reopening and committing the transaction], then I think this > should be backported. [since that constitutes latent corruption] > Nominated to 1.9.x branch. Note that standard Subversion tools should not be affected because it aborts transaction on any error and this clears on sha1->rep entries for transaction. -- Ivan Zhakov
Re: svn commit: r1755486 - in /subversion/trunk/subversion: include/svn_io.h libsvn_fs/fs-loader.c libsvn_repos/config_pool.c libsvn_subr/io.c libsvn_subr/stream.c tests/libsvn_fs/fs-test.c tests/libs
On 17 August 2016 at 19:23, Daniel Shahaf <d...@daniel.shahaf.name> wrote: > Ivan Zhakov wrote on Wed, Aug 17, 2016 at 19:09:53 +0300: >> I've tried to improve docstring in r1756647. > > Looks great. > >> >> + * Use @a scratch_pool for temporary allocations and @a result_pool >> >> + * to allocate @a *checksum. >> >> + * >> >> + * @since New in 1.10. >> >> + */ >> >> +svn_error_t * >> >> +svn_stream_checksum(svn_checksum_t **checksum, >> > >> > This interface seems very similar to svn_stream_checksummed(), both in >> > name and in functionality. >> > >> I don't see problem here, but I could suggest name it as >> svn_stream_contents_checksum(). What do you think? > > +1, that's harder to confuse with svn_stream_checksummed2() and > analogous to svn_stream_contents_same2(). > Done in r1756651. Thanks for review! -- Ivan Zhakov
Re: Drop Windows 2000 compatibility
On 11 August 2016 at 15:17, C. Michael Pilato <cmpil...@collab.net> wrote: > On 08/10/2016 08:44 AM, Ivan Zhakov wrote: >> Serf doesn't support Windows 2000 (due direct usage of SSPI). >> >> Given all above I propose to officially drop Windows 2000 support in >> Subversion and cleanup some Windows 2000 compatibility code. > > Does this mean I need to upgrade my only Windows machine? Do you really use Windows 2000? How did you obtain it? :) -- Ivan Zhakov
Re: Drop Windows 2000 compatibility
On 10 August 2016 at 15:44, Ivan Zhakov <i...@visualsvn.com> wrote: > Windows 2000 extended support by Microsoft ended on July 13, 2010 > (almost 6 years ago) [1] > > One problem that MSDN doesn't mention Windows 2000 in API > compatibility. Even for functions that was available in Windows NT > 4.0, for example IsDebuggerPresent: it's defined with #if _WIN32_WINNT >> 0x0400, while MSDN states that it's only available since Windows XP > [2]. It's very confusing and we should use LoadLibrary() for all > Windows API functions even CreateFile() to be Windows 2000 compatible > if we trust MSDN. > > I also hardly believe that someone even test Subversion on Windows > 2000 these days. > Another argument: Windows 2000 is not available for download even for MSDN subscribers due to Java-related Settlement [1] [1] https://msdn.microsoft.com/en-us/subscriptions/ff723773.aspx -- Ivan Zhakov
Re: [PATCH] Resolve testsuite interruption in SVN 1.8 (v2)
On 8 August 2016 at 23:29, Stefan <luke1...@posteo.de> wrote:
> On 8/8/2016 14:53, Stefan Hett wrote:
>> On 8/8/2016 12:22 PM, Ivan Zhakov wrote:
>>
>>> On 7 August 2016 at 23:14, Stefan <luke1...@posteo.de> wrote:
>>>
>>> Btw what are the problems with approach to disable watson reports on
>>> abort(), except backporting? I mean we already override Windows Error
>>> Reporting by installing our own SEH exception handler, so it looks
>>> natural to disable abort() reporting also.
>>>
>> I agree with you on this one. I wasn't aware that SVN does already
>> alter the default exception handling on Windows (and therefore
>> effectively disable the Watson crash dump reports in case of an
>> unhanled exception). So disabling the Watson crash dumps on abort
>> calls does indeed sound like increasing the system/design consistency
>> in SVN.
>> You got my non-binding +1 on that proposal (which obviously would be a
>> replacement for my proposed patch).
>>
> I've been thinking more about your arguments and since I agree with you,
> I'd like to replace my previously proposed patch with this updated
> version based on your idea.
> Due to the behavior change it imposes, I'm not going to propose this for
> backporting to 1.8 or 1.9, though.
>
> Note that I kept the design to only disable Watson crash reports, unless
> SVN_CMDLINE_USE_DIALOG_FOR_ABORT is set. The reasoning is as given in my
> previous reply as in as far as I understand the design of that
> environment-variable-setting, it should control whether an abort
> produces a popup or not. Disabling the Watson crash reports would
> prevent that popup (in certain cases) and therefore negate the setting.
>
Hi Stefan,
I've tested your patch, but it seems that calling
_set_abort_behavior(0, _CALL_REPORTFAULT) is not enough. The problem
with this approach that abort() uses RaiseFailFastException() that
bypasses all exception handlers [1].
So Subversion doesn't write crash dump and even doesn't print error
message on abort() failure. One solution would be to register signal
handler() and use RaiseException(STATUS_FATAL_APP_EXIT). This
exception would be handled by our own exception handler. Subversion
already uses such approach for handling out-of-memory errors (see
r1724784 [2]). I've tested this approach (see attached patch) and it
seems to be working fine, but I want to test it more before
committing.
[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/dd941688(v=vs.85).aspx
[2] https://svn.apache.org/r1724784
--
Ivan Zhakov
Index: subversion/libsvn_subr/cmdline.c
===
--- subversion/libsvn_subr/cmdline.c(revision 1751923)
+++ subversion/libsvn_subr/cmdline.c(working copy)
@@ -91,6 +91,12 @@
static svn_boolean_t shortcut_stderr_to_console = FALSE;
#endif
+#if defined(WIN32) && defined(SVN_USE_WIN32_CRASHHANDLER)
+static void __cdecl win32_sigabrt_handler(int sig)
+{
+ RaiseException(STATUS_FATAL_APP_EXIT, EXCEPTION_NONCONTINUABLE, 0, NULL);
+}
+#endif
int
svn_cmdline_init(const char *progname, FILE *error_stream)
@@ -163,6 +169,13 @@
/* In release mode: Redirect abort() errors to stderr */
_set_error_mode(_OUT_TO_STDERR);
+ /* In release mode: Disable Watson crash reports on abort(). */
+ _set_abort_behavior(0, _CALL_REPORTFAULT);
+
+ /* Register abort() signal handler to create crash dump on
+ abort(). */
+ apr_signal(SIGABRT, win32_sigabrt_handler);
+
/* In _DEBUG mode: Redirect all debug output (E.g. assert() to
stderr.
(Ignored in release builds) */
_CrtSetReportFile( _CRT_WARN, _CRTDBG_FILE_STDERR);
Index: subversion/tests/svn_test_main.c
===
--- subversion/tests/svn_test_main.c(revision 1751923)
+++ subversion/tests/svn_test_main.c(working copy)
@@ -879,6 +879,9 @@
/* In release mode: Redirect abort() errors to stderr */
_set_error_mode(_OUT_TO_STDERR);
+ /* In release mode: Disable Watson crash reports on abort(). */
+ _set_abort_behavior(0, _CALL_REPORTFAULT);
+
/* In _DEBUG mode: Redirect all debug output (E.g. assert() to stderr.
(Ignored in releas builds) */
_CrtSetReportFile( _CRT_ASSERT, _CRTDBG_FILE_STDERR);
Re: [PATCH] Resolve testsuite interruption in SVN 1.8
On 7 August 2016 at 23:14, Stefan <luke1...@posteo.de> wrote:
> On 8/7/2016 21:16, Ivan Zhakov wrote:
>> On 7 August 2016 at 21:27, Stefan Sperling <s...@elego.de> wrote:
>>> On Sun, Aug 07, 2016 at 08:23:58PM +0200, Stefan Sperling wrote:
>>>> On Sun, Aug 07, 2016 at 02:08:35PM +0200, Stefan wrote:
>>>>> Hi,
>>>>>
>>>>> the attached patch is the outcome of talking with jcorval on IRC about a
>>>>> test suite issue on Windows (release builds) in SVN 1.8. It resolves the
>>>>> fact that the tests will be interrupted on Windows by a Windows popup
>>>>> upon an abort()-call.
>>>>>
>>>>> Atm this is triggered for the 1.8 test suite for the move-test (no 8)
>>>>> which is marked as XFail and triggers an SVN_ERR_ASSERT() and therefore
>>>>> breaks fully automated tests.
>>>> I'm not a windows person but this looks reasonable to me. +1
>>> Forgot to mention: If this is a 1.8-only fix, I'd suggest you create
>>> a branch of 1.8.x and commit your patch there, and then nominate your
>>> branch in the 1.8.x/STATUS file (you can add my +1 to the nomination).
>> I'm not sure that we should add handling of new environment variable
>> in patch release.
>>
>>
> I can understand the concern. Please bare in mind however that the
> default behavior (aka: if the environment variable is not set) would
> stay unchanged and the effect of the environment variable is quite
> limited (it will only disable the Watson crash dumps in release builds -
> obviously only applies on Windows builds). It's by default only
> activated for the test suites and therefore the risks involved is IMO
> close to non existent.
>
Could you please first clarify: does this problem exist in trunk? I'm
asking because subject is somewhat confusing because it only mention
"SVN 1.8". If yes, we should discuss solution for trunk first. And
then discuss backporting committed solution to active stable branches
separately.
Regarding the patch itself (assuming it for trunk): why you have
decided to check for SVN_CMDLINE_DISABLE_WATSON_ON_ABORT environment
variable only if SVN_CMDLINE_USE_DIALOG_FOR_ABORT is not set? It
creates very hard to understand dependency on other environment
variables that controls crashhandler/abort() behavior. Because
currently suggested behavior would be:
if (!getenv("SVN_CMDLINE_DISABLE_CRASH_HANDLER"))
{
if (!getenv("SVN_CMDLINE_USE_DIALOG_FOR_ABORT"))
{
if (getenv("SVN_CMDLINE_DISABLE_WATSON_ON_ABORT"))
{
_set_abort_behavior(0, _CALL_REPORTFAULT);
}
}
}
Btw what are the problems with approach to disable watson reports on
abort(), except backporting? I mean we already override Windows Error
Reporting by installing our own SEH exception handler, so it looks
natural to disable abort() reporting also.
Also your patch is missing documentation changes in notes/knobs.
--
Ivan Zhakov
Re: [PATCH] Resolve testsuite interruption in SVN 1.8
On 7 August 2016 at 21:27, Stefan Sperling <s...@elego.de> wrote: > On Sun, Aug 07, 2016 at 08:23:58PM +0200, Stefan Sperling wrote: >> On Sun, Aug 07, 2016 at 02:08:35PM +0200, Stefan wrote: >> > Hi, >> > >> > the attached patch is the outcome of talking with jcorval on IRC about a >> > test suite issue on Windows (release builds) in SVN 1.8. It resolves the >> > fact that the tests will be interrupted on Windows by a Windows popup >> > upon an abort()-call. >> > >> > Atm this is triggered for the 1.8 test suite for the move-test (no 8) >> > which is marked as XFail and triggers an SVN_ERR_ASSERT() and therefore >> > breaks fully automated tests. >> >> I'm not a windows person but this looks reasonable to me. +1 > > Forgot to mention: If this is a 1.8-only fix, I'd suggest you create > a branch of 1.8.x and commit your patch there, and then nominate your > branch in the 1.8.x/STATUS file (you can add my +1 to the nomination). I'm not sure that we should add handling of new environment variable in patch release. -- Ivan Zhakov
Re: svn commit: r1745192 - /subversion/trunk/subversion/tests/libsvn_subr/io-test.c
On 25 May 2016 at 20:23, Branko Čibej <br...@apache.org> wrote: > On 23.05.2016 15:12, i...@apache.org wrote: >> Author: ivan >> Date: Mon May 23 13:12:50 2016 >> New Revision: 1745192 >> >> URL: http://svn.apache.org/viewvc?rev=1745192=rev >> Log: >> Follow-up to r1745173: Fix test expectation. >> >> * subversion/tests/libsvn_subr/io-test.c >> (test_open_uniquely_named): Expect APR_STATUS_IS_ENOENT() instead of >>APR_STATUS_IS_ENOTDIR() on attempt to open uniquely named file in >>directory that doesn't exist. > > ISTR this may actually be platform-dependent; you'd get ENOENT on > Windows but ENOTDIR on Linux or the other way around. I think we have > some code in libsvn_subr/io.c that takes care of this. > Are you sure that other platforms may raise ENOTDIR in this specific case (opening a file when the parent directory doesn't exist)? As far I remember the reverse case (attempting to open a file where some path component is not directory) is platform dependent. E.g. attempting to open /iota/foo may return ENOENT or ENOTDIR on different platforms. I wanted to make test case as explicit as possible, since it's not production code. We may relax this check later if needed. -- Ivan Zhakov
Re: Deadlock-like behaviour of svnserve in multi-threaded mode (-T)
On 31 May 2016 at 17:19, Radek Krotil <radek.kro...@polarion.com> wrote: > Polarion is our application and it uses SvnKit (http://svnkit.com/) as a > connector to SVN. We use SVNKit version 1.8.12. > > Dump from our application also shows that the threads are waiting for data > from network. Could it be an issue in Windows itself? Anyway, we have seen > this behavior both in Windows and Linux environment and never seen it with > Subversion 1.8 and earlier, so we suspect it's a regression in Subversion. > I suggest to check network dump prior deadlock. -- Ivan Zhakov
Re: Deadlock-like behaviour of svnserve in multi-threaded mode (-T)
On 31 May 2016 at 14:43, Radek Krotil <radek.kro...@polarion.com> wrote: > Hi Ivan. > > I managed to get the debug symbols for the Subversion binaries, we've been > using. It can be downloaded at > http://www.apachehaus.de/subversion-1.9.3-ap24-x64_pdb.zip. > As far I see all 22 workers threads are waiting for data from network: [[ ntdll.dll!NtWaitForSingleObject ()Unknown mswsock.dll!SockWaitForSingleObject ()Unknown mswsock.dll!WSPRecv ()Unknown ws2_32.dll!WSARecv ()Unknown libapr-1.dll!57bff0aa()Unknown svnserve.exe!sock_read_cb(void * baton, char * buffer, unsigned __int64 * len) Line 120C svnserve.exe!readbuf_fill(svn_ra_svn_conn_st * conn, apr_pool_t * pool) Line 391C svnserve.exe!svn_ra_svn__read_tuple(svn_ra_svn_conn_st * conn, apr_pool_t * pool, const char * fmt, ...) Line 1379C svnserve.exe!serve_interruptable(int * terminate_p, connection_t * connection, int(*)(connection_t *) is_busy, apr_pool_t * pool) Line 4057C svnserve.exe!serve_thread(apr_thread_t * tid, void * data) Line 598C ]]] It also seems that you're using some third-party (Polarion) svn:// client for Subversion. Is it true? -- Ivan Zhakov
Re: [PATCH] troubleshooting http-pipelining for 1.9 release notes
On 16 May 2016 at 13:43, Stefan <luke1...@posteo.de> wrote: > On 5/16/2016 11:42, Ivan Zhakov wrote: >> On 15 May 2016 at 03:02, Stefan <luke1...@gmx.de> wrote: >>> On 5/15/2016 01:13, Stefan wrote: >>>> [[[ >>>> Add a troubleshooting section to 1.9 to help users tracing down problems >>>> related to proxies when locking/unlocking multiple files. >>>> >>>> * docs/release-notes/1.9.html >>>> (troubleshooting): Add new section including http-pipelining issue >>>> description. >>>> ]]] >>> >>> Small correction to patchnotes: >>> >>> [[[ >>> Add a troubleshooting section to 1.9 to help users tracing down problems >>> related to proxies when locking/unlocking multiple files. >>> >>> * docs/release-notes/1.9.html >>> (troubleshooting): Add new section including http-pipelining issue >>> description. >>> (news): Add link to new troubleshooting section. >>> ]]] >>> >> I think it's better to use term "HTTP pipelining" instead of >> "http-pipelining" on the website. Another wording suggestion: replace >> ".. protocols/applications involved in processing http-pipelining." >> with something like ".. protocols/applications involved in processing >> pipelined HTTP requests." > > Thanks for the review Ivan, attached patch incorporates your changes and > also changes the section name (http-pipeline-issue -> > http-pipelining-issue). > Thanks for fixing that, but title still uses term 'http-pipelining": +Lock/Unlock errors related to http-pipelining + + -- Ivan Zhakov
Re: [PATCH] clean up 1.9 release notes - part 2
On 15 May 2016 at 02:21, Stefan <luke1...@gmx.de> wrote: > Hi, > > following patch fixes two more minor issues in the 1.9 release notes > > [[[ > Clean up 1.9 release notes html to improve XHTML1.1 compliance a bit. > > * docs/release-notes/1.9.html > (format7-comparison): add missing closing tr-tag > (svnfsfs-issues): move pre-section into li-section > ]]] > +1, looks good. Please commit. Couple suggestions for patch posting: - Create patches from working copy root instead of patches for single file. - Do not post multiple patches with same name -- Ivan Zhakov
Re: [PATCH] troubleshooting http-pipelining for 1.9 release notes
On 15 May 2016 at 03:02, Stefan <luke1...@gmx.de> wrote: > On 5/15/2016 01:13, Stefan wrote: >> >> [[[ >> Add a troubleshooting section to 1.9 to help users tracing down problems >> related to proxies when locking/unlocking multiple files. >> >> * docs/release-notes/1.9.html >> (troubleshooting): Add new section including http-pipelining issue >> description. >> ]]] > > > Small correction to patchnotes: > > [[[ > Add a troubleshooting section to 1.9 to help users tracing down problems > related to proxies when locking/unlocking multiple files. > > * docs/release-notes/1.9.html > (troubleshooting): Add new section including http-pipelining issue > description. > (news): Add link to new troubleshooting section. > ]]] > I think it's better to use term "HTTP pipelining" instead of "http-pipelining" on the website. Another wording suggestion: replace ".. protocols/applications involved in processing http-pipelining." with something like ".. protocols/applications involved in processing pipelined HTTP requests." -- Ivan Zhakov
Re: Issues to close
On 15 May 2016 at 01:24, Stefan <luke1...@posteo.de> wrote: > On 5/9/2016 10:05, Ivan Zhakov wrote: > > On 9 May 2016 at 10:15, Stefan Hett <ste...@egosoft.com> wrote: > >> On 5/6/2016 5:33 PM, Ivan Zhakov wrote: >> >> On 6 May 2016 at 14:27, Stefan Hett <ste...@egosoft.com> wrote: >> >> I'd suggest to also close the following issue: >> * SVN-4557: ra_serf fails to delete directory containing many files >> https://issues.apache.org/jira/browse/SVN-4557 >> The fix for this particular issue got in 1.9.4 and even though it's >> not >> yet backported to the 1.8 branch, the described issue is fixed in 1.9 now. >> >> Probably, but in this case we need to file different issue for >> *replacing* directory containing many files with locks -- this part of >> issue not fixed. >> >> I take it that this is not fixed on trunk yet, right? >> > > Yes, replacing directory containing many files with locks is not fixed on > trunk. > > I've now created a separate issue for the replace case: > https://issues.apache.org/jira/browse/SVN-4634 > So I take it SVN-4557 could be closed as fixed now. > Marked SVN-4557 as resolved. Thank you very much! -- Ivan Zhakov
Re: [jira] [Updated] (SVN-4630) Unrestricted internal XML entities expansion
On 10 May 2016 at 01:15, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> Ivan Zhakov (JIRA) wrote on Mon, May 09, 2016 at 10:53:12 +:
>>
>> [
>> https://issues.apache.org/jira/browse/SVN-4630?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
>> ]
>>
>> Ivan Zhakov updated SVN-4630:
>> -
>> Component/s: (was: mod_dav_svn)
>> tools
>>
>> Changing {{Component}} to {{tools}}, since this is mod_dontdothat
>> specific problem and mod_dav_svn is not affected.
>
> Just making sure: did you see the following remark at the end of the
> report:
>
>> > The Expat parser creation in {{subversion/libsvn_ra_serf/util.c}} and
> {{subversion/libsvn_subr/xml.c}} should be fixed as well, but these are
> in the client-side code (I think), and therefore less of a security concern.
>
Yes, I noticed that, but forgot that JIRA allows to specify multiple
components for issues. I've added libsvn_ra_serf as component for this
issue. Thanks for heads-up!
--
Ivan Zhakov
Re: Issues to close
On 9 May 2016 at 12:23, Bert Huijben <b...@qqmail.nl> wrote: > The lock add issue is completely independent…This implementation was > changed to make multiple locks atomic, instead of one at a time. It depends of RA layer: ra_serf uses pipelining to issue multiple single path LOCK requests without waiting for other requests to complete, while ra_svn uses new 'lock-many' command to perform operation in atomic way. -- Ivan Zhakov
Re: random, unpredictable sort of svn diff
On 8 May 2016 at 16:55, Vincent Lefevre <vincent-...@vinc17.net> wrote: > Hi, > > The output of "svn diff" is still in random order (not even > pseudo-random, completely unpredictable). There had been discussions > in the past to fix this, e.g. > > > https://mail-archives.apache.org/mod_mbox/subversion-users/201203.mbox/%3C1332298973.3028.17.camel@segulix%3E > > What's the status? > > I haven't found a bug on this subject on Jira. > I'm getting stable sorted output when using Subversion 1.9.3: $ touch a b c $ svn add a b c $ svn diff Index: a === Index: b === Index: c ======= The same for svn st -- Ivan Zhakov
Re: Issues to close
On 6 May 2016 at 14:27, Stefan Hett <ste...@egosoft.com> wrote: > I'd suggest to also close the following issue: > * SVN-4557: ra_serf fails to delete directory containing many files > https://issues.apache.org/jira/browse/SVN-4557 > The fix for this particular issue got in 1.9.4 and even though it's not > yet backported to the 1.8 branch, the described issue is fixed in 1.9 now. > Probably, but in this case we need to file different issue for *replacing* directory containing many files with locks -- this part of issue not fixed. -- Ivan Zhakov
Issues to close
I suggest to close the following in Subversion issue tracker: * SVN-2935: mod_dav_svn + Apache/mpm_worker don't play nice https://issues.apache.org/jira/browse/SVN-2935 Looks like problem in BDB which is deprecated. Similar issue SVN-4157 already resolved as Won't fix due BDB deprecation. * SVN-2128: svnserve slows down / stops file sharing on Win2003 https://issues.apache.org/jira/browse/SVN-2128 Very old issue. No reproduction script, no prior discussion on mailing list. I suggest close as "Cannot reproduce". * SVN-4285: svnpubsub API changes https://issues.apache.org/jira/browse/SVN-4285 It's unclear what is goal or scope of this issue. Subversion 1.8.0 was release long time ago, so timeframe for svnpubsub API changes is already passed. * SVN-3448: svn cleanup fails, instructing to run svn cleanup https://issues.apache.org/jira/browse/SVN-3448 Pre wc-ng bug report. Most likely fixed in Subversion 1.7.0 * SVN-3829: 64-bit Subversion crashes on Mac OS X https://issues.apache.org/jira/browse/SVN-3829 The real problem is APR issue 48476 [1] fixed in APR 1.4.4. I propose to resolve as 'Not A Problem' since it's not Subversion problem. * SVN-3224: Compile fails on x86 Solaris https://issues.apache.org/jira/browse/SVN-3224 Very old issue. No prior discussion on mailing list. I'm pretty sure that build on Solaris works now, since we have Solaris buildbot. I suggest close as "Cannot reproduce". * SVN-3229: Problem with syncing from 1.4 server https://issues.apache.org/jira/browse/SVN-3229 No reproduction script, no prior discussion on mailing list. I suggest close as "Invalid". [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=48476 -- Ivan Zhakov
Re: svn commit: r1741096 - in /subversion/trunk/subversion: include/svn_error_codes.h libsvn_fs_fs/rep-cache.c libsvn_fs_fs/rep-cache.h libsvn_fs_fs/transaction.c libsvn_subr/sqlite.c tests/libsvn_fs/
On 26 April 2016 at 23:11, <kot...@apache.org> wrote:
> Author: kotkov
> Date: Tue Apr 26 20:11:30 2016
> New Revision: 1741096
>
> URL: http://svn.apache.org/viewvc?rev=1741096=rev
> Log:
> Rollback an sqlite transaction in case we fail to COMMIT it.
>
> Otherwise, the db connection might be left in an unusable state and can
> be causing different issues, especially in case the connection is a
> long-living one.
>
> See r1741071 and the commit_with_locked_rep_cache() test.
>
[...]
[...]
> Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1741096=1741095=1741096=diff
> ==
> --- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Tue Apr 26
> 20:11:30 2016
> @@ -3824,6 +3824,8 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
>
>if (ffd->rep_sharing_allowed)
> {
> + svn_error_t *err;
> +
>SVN_ERR(svn_fs_fs__open_rep_cache(fs, pool));
>
>/* Write new entries to the rep-sharing database.
> @@ -3834,9 +3836,21 @@ svn_fs_fs__commit(svn_revnum_t *new_rev_
>/* ### A commit that touches thousands of files will starve other
> (reader/writer) commits for the duration of the below call.
> Maybe write in batches? */
> - SVN_SQLITE__WITH_TXN(
> -write_reps_to_cache(fs, cb.reps_to_cache, pool),
> -ffd->rep_cache_db);
> + SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
> + err = write_reps_to_cache(fs, cb.reps_to_cache, pool);
> + err = svn_sqlite__finish_transaction(ffd->rep_cache_db, err);
> +
> + if (err && svn_error_find_cause(err, SVN_ERR_SQLITE_ROLLBACK_FAILED))
There is no need check for 'err' since svn_error_find_cause() already
checks for SVN_NO_ERROR.
> +{
> + /* Failed rollback means that our db connection is unusable, and
> + the only thing we can do is close it. The connection will be
> + reopened during the next operation with rep-cache.db. */
> + return svn_error_trace(
> + svn_error_compose_create(err,
> + svn_fs_fs__close_rep_cache(fs)));
> +}
> + else if (err)
> +return svn_error_trace(err);
> }
>
>return SVN_NO_ERROR;
>
--
Ivan Zhakov
Re: patch manager
On 26 April 2016 at 10:58, Stefan Sperling <s...@apache.org> wrote: > Hi Karl, > > On IRC, you asked: > >stsp: Is there anyone still in Patch Manager role in the project? >I just saw this, http://jk.ozlabs.org/projects/patchwork/, and thought it >might be helpful. > * kfogel asks the above question of Bert, Arfrever, or anyone else who > might know the answer > > I don't think we've got an active patch manager at the moment. > > We don't get many patch submissions these days, so there's not a lot to > keep track of. However, we don't have anyone working on Subversion full-time > either, so perhaps it's more likely now that patch submissions may end up > being ignored. > > I like the idea, but at the moment I don't have time to make it happen. > We could try delegating the installation process to infra, but we'd need > someone at our end taking care of our patchwork instance beyond that. Why not just use JIRA to store patches? We used this in the past and I don't remember serious problems with this approach. -- Ivan Zhakov
Re: 1.8.16 up for signing/testing
On 21 April 2016 at 19:43, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > The 1.8.16 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. I plan to try and release on April 28th > so please try and get your votes/signatures in place by April 27th. > Summary: +1 to release. TESTED: --- [ fsfs ] x [ file | svn | http v1 | http v2] x [win32 | x64] RESULTS: All Tests Pass PLATFORM: - MS Windows 7 Ultimate (x64) Microsoft Visual Studio 2013 Update 3 DEPENDENCIES: - APR: 1.5.2 APR-Util: 1.5.4 Apache HTTPD: 2.2.31 zlib: 1.2.8 OpenSSL: 1.0.1r sqlite: 3.7.12.1 Python: 2.7.8 serf: 1.3.8 Signatures: --- (See the appropriate distribution files.) -- Ivan Zhakov
Re: 1.9.4 up for signing/testing
On 21 April 2016 at 19:43, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > The 1.9.4 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. I plan to try and release on April 28th > so please try and get your votes/signatures in place by April 27th. > Summary: +1 to release. TESTED: --- [ fsfs ] x [ file | svn | http v1 | http v2] x [win32 | x64] RESULTS: All Tests Pass. PLATFORM: - MS Windows 7 Ultimate (x64) Microsoft Visual Studio 2013 Update 3 DEPENDENCIES: - APR: 1.5.2 APR-Util: 1.5.4 Apache HTTPD: 2.2.31 zlib: 1.2.8 OpenSSL: 1.0.2g sqlite: 3.7.12.1 Python: 2.7.8 serf: 1.3.8 Signatures: --- (See the appropriate distribution files.) -- Ivan Zhakov
Re: Really funny test failure on Windows
On 23 April 2016 at 18:43, Branko Čibej <br...@apache.org> wrote: > This just happened when I was running trunk tests in a Windows 10 VM > (with Windows Defender enabled). I see now that the test is actually > expected to tickle the interest of antivirus software. :) I've raised this problem month ago on dev@s.a.o: http://svn.haxx.se/dev/archive-2016-03/0043.shtml -- Ivan Zhakov
Re: svn commit: r1740320 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/conflicts.c svn/conflict-callbacks.c tests/libsvn_client/conflicts-test.c
On 21 April 2016 at 19:44, Stefan Sperling <s...@elego.de> wrote: > On Thu, Apr 21, 2016 at 07:37:50PM +0300, Ivan Zhakov wrote: >> On 21 April 2016 at 17:34, Stefan Sperling <s...@apache.org> wrote: >> > Implementation aside, I do think the option to merge the two directories >> > makes sense, even if they are ancestrally unrelated. >> May there are some implementation problems, but I think merging two >> directories makes sense: it's real world case during some refactoring. > > Yes, it makes sense, as I already stated (did you read "don't" where > I wrote "do"?) > Oops, sorry. I misread your sentence. > But the implementation should work. > Any idea how can we can avoid the problems I have described? I don't understand all details, but I don't think that using merging code in resolver would be sufficient in long term. But we can use for initial implementation though. Also as far I remember sub-tree mergeinfo makes further operations significantly slower. Can we just leave svn:mergeinfo unchanged for subtree? -- Ivan Zhakov
Re: svn commit: r1740320 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/conflicts.c svn/conflict-callbacks.c tests/libsvn_client/conflicts-test.c
On 21 April 2016 at 17:34, Stefan Sperling <s...@apache.org> wrote: > On Thu, Apr 21, 2016 at 02:04:08PM -, s...@apache.org wrote: >> Author: stsp >> Date: Thu Apr 21 14:04:08 2016 >> New Revision: 1740320 >> >> URL: http://svn.apache.org/viewvc?rev=1740320=rev >> Log: >> Add a conflict resolution option for dir/dir "incoming add vs local >> obstruction upon merge". This option merges the two directories. >> >> Again, the implementation is not atomic, yet. >> And it doesn't always work as expected. >> Add two XFAIL regression tests which illustrate known problems. > > I'll need some help here to decide how to proceed. > > Briefly, the problems I'm running into are: > > - It is not possible to merge an "only-adds" delta from rN-1 to rN for >a path created in rN. Essentially, this is the same issue as we had >for 'svn diff -cN' for a node created in rN, which was fixed at some point. >So this is a case where svn diff -cN works, but svn merge -cN doesn't. >I now need svn merge -cN to work in this case (see the 1st of 3 tests >added in this commit). > > - I'm not sure to what extent the resolver should be responsible for >mergeinfo. Should it assume that existing mergeinfo in a working copy >remains valid when further merges are performed to resolve a tree >conflict? I guess not. In the case I'm looking at, the merge only >produces a delta if run with --no-ancestry (why?) which disables >mergeinfo recording. It is possible to construct cases where the lack >of additional mergeinfo recording seems wrong (see the 3rd of 3 tests >added in this commit). > > Does anyone have suggestions about these problems? > > Should the resolver be using the standard merge code at all in this case? > Perhaps that's the wrong approach? > > Implementation aside, I do think the option to merge the two directories > makes sense, even if they are ancestrally unrelated. May there are some implementation problems, but I think merging two directories makes sense: it's real world case during some refactoring. -- Ivan Zhakov
Re: svn commit: r1663500 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c libsvn_ra_serf/merge.c libsvn_ra_serf/ra_serf.h tests/cmdline/lock_tests.py
On 12 April 2016 at 10:30, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 3 March 2015 at 04:06, <rhuij...@apache.org> wrote:
>> Author: rhuijben
>> Date: Tue Mar 3 01:06:16 2015
>> New Revision: 1663500
>>
>> URL: http://svn.apache.org/r1663500
>> Log:
>> In ra-serf (for issue #4557) reinstate a bit of code that retries a delete
>> with an altered request if the original request fails, because the server
>> determined that it is an invalid request, because it has too many bytes
>> in the headers.
>>
>> Before r1553501 we always retried DELETE requests that required lock
>> tokens, as the initial request didn't have an If header at all.
>>
> Hi Bert,
>
> See my review inline.
>
> [...]
>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500=1663499=1663500=diff
>> ==
>> --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16
>> 2015
> [...]
>
>> static svn_error_t *
>> delete_entry(const char *path,
>> svn_revnum_t revision,
>> @@ -1412,6 +1443,7 @@ delete_entry(const char *path,
>>delete_context_t *delete_ctx;
>>svn_ra_serf__handler_t *handler;
>>const char *delete_target;
>> + svn_error_t *err;
>>
>>if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
>> {
>> @@ -1446,7 +1478,23 @@ delete_entry(const char *path,
>>handler->method = "DELETE";
>>handler->path = delete_target;
>>
>> - SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
>> + err = svn_ra_serf__context_run_one(handler, pool);
>> + if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
>> + && handler->sline.code == 400)
> May be it's better set handler->no_fail_on_http_failure_status to TRUE
> and check for handler->sline.code? This will remove dependency on
> specific error code and simplify code a bit.
>
>> +{
>> + svn_error_clear(err);
>> +
>> + /* Try again with non-standard body to overcome Apache Httpd
>> + header limit */
>> + delete_ctx->non_recursive_if = TRUE;
>> + handler->body_type = "text/xml";
>> + handler->body_delegate = create_delete_body;
>> + handler->body_delegate_baton = delete_ctx;
>> +
>> + SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
> I'm not sure that we can reuse svn_ra_serf__handler_t instance for
> multiple requests.
>
> I propose the following patch. Do you have any objections?
>
Committed in r1739278 and r1739280. And added them to r1663500
nomination in 1.9.x.
--
Ivan Zhakov
Re: Deadlock-like behaviour of svnserve in multi-threaded mode (-T)
On 14 April 2016 at 19:14, Radek Krotil <radek.kro...@polarion.com> wrote: > Hi all. > > Our application generates lot of concurrent read requests to subversion > using svn: protocol. When we tested the multithreaded mode of svnserve after > upgrade to 1.9.3, we noticed strange 'deadlock-like' behavior: at some point > all the requests are blocked in svnserve and wait there for a few minutes (3 > to 15 minutes, no CPU activity), after which they continue to work. This is > making our application significantly slower. > > Operating system: Windows 10, CentOS 6.6, CentOS 7.2 > > The release and/or revision of Subversion: 1.9.3 > > The compiler and configuration options you built Subversion with: WANDisco > binaries for CentOS, Apache Haus binaries for Windows > > The workaround on Linux is to run svnserve without -T switch, i.e. not using > multithreaded mode. For Windows, there is no workaround as svnserve only > supports the multi-threaded mode. > > Here is a sample of thread dump of svnserve.exe during the 'deadlock' > obtained on Windows 10 using Process Explorer: > > ntoskrnl.exe!KeSynchronizeExecution+0x3de6 > ntoskrnl.exe!KeWaitForMutexObject+0xc7a > ntoskrnl.exe!KeWaitForMutexObject+0x709 > ntoskrnl.exe!KeWaitForMutexObject+0x375 > ntoskrnl.exe!IoThreadToProcess+0xff0 > ntoskrnl.exe!KeRemoveQueueEx+0x16ba > ntoskrnl.exe!KeWaitForMutexObject+0xe8e > ntoskrnl.exe!KeWaitForMutexObject+0x709 > ntoskrnl.exe!KeWaitForMutexObject+0x375 > ntoskrnl.exe!NtWaitForSingleObject+0xf2 > ntoskrnl.exe!setjmpex+0x3963 > ntdll.dll!NtWaitForSingleObject+0x14 > MSWSOCK.dll!Tcpip6_WSHSetSocketInformation+0x155 > MSWSOCK.dll+0x1bf1 > WS2_32.dll!WSAAccept+0xce > > WS2_32.dll!accept+0x12 > libapr-1.dll!apr_socket_accept+0x46 > svnserve.exe+0xc11c > svnserve.exe+0xbae5 > svnserve.exe+0xaf6c > svnserve.exe+0x13ab > KERNEL32.DLL!BaseThreadInitThunk+0x22 > ntdll.dll!RtlUserThreadStart+0x34 > > The similar stack can be seen with other threads too. > 1. Do you have debug symbols for Subversion binaries you're using? 2. Other threads should have different stack trace, because AFAIK only one thread calls accept(). 3. Could you please create full memory dump of locked process and send it to me including debug symbols? -- Ivan Zhakov
Re: svn commit: r1663500 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c libsvn_ra_serf/merge.c libsvn_ra_serf/ra_serf.h tests/cmdline/lock_tests.py
On 3 March 2015 at 04:06, <rhuij...@apache.org> wrote:
> Author: rhuijben
> Date: Tue Mar 3 01:06:16 2015
> New Revision: 1663500
>
> URL: http://svn.apache.org/r1663500
> Log:
> In ra-serf (for issue #4557) reinstate a bit of code that retries a delete
> with an altered request if the original request fails, because the server
> determined that it is an invalid request, because it has too many bytes
> in the headers.
>
> Before r1553501 we always retried DELETE requests that required lock
> tokens, as the initial request didn't have an If header at all.
>
Hi Bert,
See my review inline.
[...]
> Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500=1663499=1663500=diff
> ==
> --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16
> 2015
[...]
> static svn_error_t *
> delete_entry(const char *path,
> svn_revnum_t revision,
> @@ -1412,6 +1443,7 @@ delete_entry(const char *path,
>delete_context_t *delete_ctx;
>svn_ra_serf__handler_t *handler;
>const char *delete_target;
> + svn_error_t *err;
>
>if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
> {
> @@ -1446,7 +1478,23 @@ delete_entry(const char *path,
>handler->method = "DELETE";
>handler->path = delete_target;
>
> - SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
> + err = svn_ra_serf__context_run_one(handler, pool);
> + if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
> + && handler->sline.code == 400)
May be it's better set handler->no_fail_on_http_failure_status to TRUE
and check for handler->sline.code? This will remove dependency on
specific error code and simplify code a bit.
> +{
> + svn_error_clear(err);
> +
> + /* Try again with non-standard body to overcome Apache Httpd
> + header limit */
> + delete_ctx->non_recursive_if = TRUE;
> + handler->body_type = "text/xml";
> + handler->body_delegate = create_delete_body;
> + handler->body_delegate_baton = delete_ctx;
> +
> + SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
I'm not sure that we can reuse svn_ra_serf__handler_t instance for
multiple requests.
I propose the following patch. Do you have any objections?
--
Ivan Zhakov
Index: subversion/libsvn_ra_serf/commit.c
===
--- subversion/libsvn_ra_serf/commit.c (revision 1736882)
+++ subversion/libsvn_ra_serf/commit.c (working copy)
@@ -1415,7 +1415,6 @@
delete_context_t *delete_ctx;
svn_ra_serf__handler_t *handler;
const char *delete_target;
- svn_error_t *err;
if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
{
@@ -1449,16 +1448,26 @@
handler->method = "DELETE";
handler->path = delete_target;
+ handler->no_fail_on_http_failure_status = TRUE;
- err = svn_ra_serf__context_run_one(handler, pool);
- if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
- && handler->sline.code == 400)
+ SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
+ if (handler->sline.code == 400)
{
- svn_error_clear(err);
-
/* Try again with non-standard body to overcome Apache Httpd
header limit */
delete_ctx->non_recursive_if = TRUE;
+
+ handler = svn_ra_serf__create_handler(dir->commit_ctx->session, pool);
+
+ handler->response_handler = svn_ra_serf__expect_empty_body;
+ handler->response_baton = handler;
+
+ handler->header_delegate = setup_delete_headers;
+ handler->header_delegate_baton = delete_ctx;
+
+ handler->method = "DELETE";
+ handler->path = delete_target;
+
handler->body_type = "text/xml";
handler->body_delegate = create_delete_body;
handler->body_delegate_baton = delete_ctx;
@@ -1465,9 +1474,10 @@
SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
}
- else
-SVN_ERR(err);
+ if (handler->server_error)
+return svn_ra_serf__server_error_create(handler, pool);
+
/* 204 No Content: item successfully deleted */
if (handler->sline.code != 204)
return svn_error_trace(svn_ra_serf__unexpected_status(handler));
Re: svn commit: r1706049 - in /subversion/trunk/subversion: libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
On 30 September 2015 at 17:49, <rhuij...@apache.org> wrote: > Author: rhuijben > Date: Wed Sep 30 14:49:25 2015 > New Revision: 1706049 > > URL: http://svn.apache.org/viewvc?rev=1706049=rev > Log: > Make it possible to possible to apply git style mode changes and binary file > patches at the same time. > [...] > > Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/patch_tests.py?rev=1706049=1706048=1706049=diff > == > --- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original) > +++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Wed Sep 30 > 14:49:25 2015 > @@ -6572,6 +6572,107 @@ def patch_empty_vs_delete(sbox): > [], True, True, > '--reverse-diff') > > +def patch_add_remove_executable(sbox): > + "add and remove executable file" > + > + sbox.build() > + wc_dir = sbox.wc_dir > + > + eicar_data = 'X5O!P%@AP[4\PZX54(P^)7CC)7}$' \ > + 'EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*\0' > + other_data = 'X5O!P%@AP[4\PZX54(P^)7CC)7}$' \ > + 'SOME-LESS-INTERESTING-OTHER-TEXT!!!$H+H*\0' > + Hi Bert, This test fails for me on Windows 10 with Windows defender enabled with error like this: [[[ W: svn: E720225: Can't open file 'C:\Users\ivan.OSTYSERVER\AppData\Local\Temp\svn-test-trunk-http\subversion\tests\cmdline\svn-test-work\working_copies\patch_tests-69\eicar.com': Operation did not complete successfully because the file contains a virus or potentially unwanted software. ]]] The file is recognized as virus since it's special test file for antivirus testing and blocks access to it. Do we really need using eicar.com in this test case? -- Ivan Zhakov
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
On 28 March 2016 at 16:37, Stefan Sperling <s...@apache.org> wrote:
> On Mon, Mar 28, 2016 at 04:13:11PM +0300, Ivan Zhakov wrote:
>> On 20 March 2016 at 01:18, <danie...@apache.org> wrote:
>> > {
>> >if (err->apr_err == SVN_ERR_CANCELLED)
>> > {
>> > - svn_error_clear(err);
>> >*may_save_plaintext = FALSE;
>> > - return SVN_NO_ERROR;
>> > + return err;
>> Daniel, do you know what was the original idea behind ignoring the
>> SVN_ERR_CANCELLED error? I see stsp committed the original code in
>> r870804, so there's probably some rationale behind it.
>>
>> Stefan, do you remember any details?
>
> I don't think there was a special reason to ignore the error.
>
> The question is whether we want Ctrl-C to mean "no" at the plaintext
> prompt, or whether it should abort the process.
>
> I agree with Daniel's patch. Ctrl-C should abort the process.
OK. Thanks for clarification! I just wanted to double check that we're
not missing something important.
--
Ivan Zhakov
Re: svn commit: r1735826 - in /subversion/trunk: ./ subversion/libsvn_subr/prompt.c
On 20 March 2016 at 01:18, <danie...@apache.org> wrote:
> Author: danielsh
> Date: Sat Mar 19 22:18:35 2016
> New Revision: 1735826
>
> URL: http://svn.apache.org/viewvc?rev=1735826=rev
> Log:
> Merge r1735680 to trunk, which was accidentally committed to a different
> branch.
>
> That revision's log message is:
>
>
> r1735680 | danielsh | 2016-03-18 21:10:34 + (Fri, 18 Mar 2016) | 9
> lines
>
> Make SIGINT abort a commit, even at the interactive plaintext prompt.
> (Issue #4624.)
>
> Follow-up to r30730 (r870804).
>
> Found by: Richlv
>
> * subversion/libsvn_subr/prompt.c
> (plaintext_prompt_helper): Propagate canncellations.
>
>
[...]
> Modified: subversion/trunk/subversion/libsvn_subr/prompt.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/prompt.c?rev=1735826=1735825=1735826=diff
> ==
> --- subversion/trunk/subversion/libsvn_subr/prompt.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/prompt.c Sat Mar 19 22:18:35 2016
> @@ -831,9 +831,8 @@ plaintext_prompt_helper(svn_boolean_t *m
> {
>if (err->apr_err == SVN_ERR_CANCELLED)
> {
> - svn_error_clear(err);
>*may_save_plaintext = FALSE;
> - return SVN_NO_ERROR;
> + return err;
Daniel, do you know what was the original idea behind ignoring the
SVN_ERR_CANCELLED error? I see stsp committed the original code in
r870804, so there's probably some rationale behind it.
Stefan, do you remember any details?
--
Ivan Zhakov
Re: svn commit: r1736576 - /subversion/trunk/subversion/bindings/ctypes-python/README
On 25 March 2016 at 17:27, <james...@apache.org> wrote: > Author: jamessan > Date: Fri Mar 25 14:27:42 2016 > New Revision: 1736576 > > URL: http://svn.apache.org/viewvc?rev=1736576=rev > Log: > * subversion/bindings/ctypes-python/README > (BUILDING CSVN): Update ctypesgen instructions for move from Google Code to > GitHub. > > Modified: > subversion/trunk/subversion/bindings/ctypes-python/README > > Modified: subversion/trunk/subversion/bindings/ctypes-python/README > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/ctypes-python/README?rev=1736576=1736575=1736576=diff > == > --- subversion/trunk/subversion/bindings/ctypes-python/README (original) > +++ subversion/trunk/subversion/bindings/ctypes-python/README Fri Mar 25 > 14:27:42 2016 > @@ -12,11 +12,11 @@ installed: >apr-1-config / apr-config script) > > Next, checkout the latest version of ctypesgen from > -http://code.google.com/p/ctypesgen/ to a "ctypesgen" subdirectory under > +https://github.com/davidjamesca/ctypesgen to a "ctypesgen" subdirectory under > the ctypes python bindings. > > For example: > - $ svn co http://ctypesgen.googlecode.com/svn/trunk ctypesgen > + $ git clone https://github.com/davidjamesca/ctypesgen I think it's better to refer 'svn co' to checkout from GitHub in Subversion build instructions. The following command works for me: [[[ svn co https://github.com/davidjamesca/ctypesgen/trunk ]]] Subversion developers and users may not have git installed -- Ivan Zhakov
Re: svn commit: r1730617 - /subversion/trunk/subversion/libsvn_repos/log.c
On 16 February 2016 at 00:47, <stef...@apache.org> wrote: > Author: stefan2 > Date: Mon Feb 15 21:47:00 2016 > New Revision: 1730617 > > URL: http://svn.apache.org/viewvc?rev=1730617=rev > Log: > Continue work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration: > Switch the last svn_fs_paths_changed2 call to svn_fs_paths_changed3. > > * subversion/libsvn_repos/log.c > (fs_mergeinfo_changed): No longer fetch the whole changes list. However, > we need to iterate twice for best total performance > and we need to minimize FS iterator lifetimes. > It seems that I would be -1 against this particular change. In the current implementation the svn_fs_paths_changed3() is called twice that in the worst case will lead to *double read from disk*. As far as I understand you're relying to the fact that the second call will hit the FSFS/FSX cache. But there will be a significant performance degradation comparing to the 1.9 implementation in the case of cache miss. As we are adding more and more of such code, more and more users become faced with the significant performance regression (see [1] and other cases). Do you intend to resolve this problem in the future commits? I have some obvious solutions in mind, but maybe you also know something about this. [1] http://svn.haxx.se/users/archive-2015-12/0060.shtml -- Ivan Zhakov
Re: svn commit: r1730389 - /subversion/trunk/subversion/libsvn_repos/log.c
On 16 February 2016 at 08:47, Stefan Fuhrmann <stef...@apache.org> wrote: > On 15.02.2016 09:39, Ivan Zhakov wrote: >> >> On 14 February 2016 at 22:25, <stef...@apache.org> wrote: >>> >>> Author: stefan2 >>> Date: Sun Feb 14 19:25:12 2016 >>> New Revision: 1730389 >>> >>> URL: http://svn.apache.org/viewvc?rev=1730389=rev >>> Log: >>> Begin work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration. >>> >>> As a first step, we introduce what will become svn_repos_get_logs5 as >>> a static function and implement svn_repos_get_logs4 as a wrapper. >> >> >> The svn_repos__get_logs5() doesn't have docstring, while as far I >> understand it has very tricky semantic of delivering data using two >> callbacks. Without documented svn_repos__get_logs5() API it's almost >> impossible to get idea of all following commits in log.c > > > The API definition / migration is complete now and the next > step is publish it. It'll then get a proper docstring. > You do this in the wrong way because nobody can meaningfully review commits you've already made without this doctring. Existence of docstring will also help not to develop the program in the bottom-up mode. If you don't have all the details at the moment, even the preliminary version of docstring will help. -- Ivan Zhakov
Re: svn commit: r1730389 - /subversion/trunk/subversion/libsvn_repos/log.c
On 14 February 2016 at 22:25, <stef...@apache.org> wrote: > Author: stefan2 > Date: Sun Feb 14 19:25:12 2016 > New Revision: 1730389 > > URL: http://svn.apache.org/viewvc?rev=1730389=rev > Log: > Begin work on the svn_repos_get_logs4 to svn_repos_get_logs5 migration. > > As a first step, we introduce what will become svn_repos_get_logs5 as > a static function and implement svn_repos_get_logs4 as a wrapper. The svn_repos__get_logs5() doesn't have docstring, while as far I understand it has very tricky semantic of delivering data using two callbacks. Without documented svn_repos__get_logs5() API it's almost impossible to get idea of all following commits in log.c -- Ivan Zhakov
Re: Making FS and repos layer log API streamy
On 31 January 2016 at 14:03, Stefan Fuhrmann <stef...@apache.org> wrote: > Hi there, > > When the server needs to transmit the list of changed paths > in a revision, its memory usage is O(#changes), i.e. practically > unbound. The problems are: > > * FS and repos API require a full collection of all changes > Most consumers simply scan that data once. So, they can > just as well work on a one change at a time basis as a > callback. > > * The data types are different, so we end up with two copies. > A revised svn_fs_path_change_t with identical > svn_repo_path_change3_t can fix this. Minor adjustments > to the element data types further improve efficiency. > > I've played with a revised version of svn_fs_paths_changed > and svn_repos_get_logs to use callbacks for the individual > path changes. Effectively, the FS layer can now pump the > info through the repos / authz filter into the RA layer. Here is some feedback. Making FS API streamy is a good goal. But as far I remember repos layer still has to store all changed path in hashtable for authorization purposes. Also I think we should not use callbacks to deliver data from the FS layer. Currently FS API is passive and I think it should remain the same: FS API users may invoke FS function from callback and this will require FS implementation to be fully reentrant (to avoid problems like fixed in r1718167 [1]) Instead of callbacks we should create svn_fs_changed_paths_t and some kind of iterator of it. As a nice benefit FS API user may request only interesting information by providing NULL for some arguments (like WC-NG API). I also noticed that svn_fs_nodeid_t member is removed from svn_fs_path_change3_t in r1727822. Are you sure about this change? Some FSFS bugs are not a reason to drop this kind of information. We should fix nodeid in changed path and maybe eventually replace it with svn_fs_node_t. [1] http://svn.haxx.se/dev/archive-2015-12/0006.shtml -- Ivan Zhakov
Re: Last-Modified HTTP header in GET responses
On 7 January 2016 at 10:34, Ivan Zhakov <i...@visualsvn.com> wrote: > On 6 January 2016 at 08:14, Greg Stein <gst...@gmail.com> wrote: >> Personally, I'd be more interested in the effects on the network and its >> caching ability. Do we really need to save CPU/IO on the server? Today's >> servers seem more than capable, and are there really svn servers out in the >> wild getting so crushed, that this is important? It seems that as long as >> proxies/etc can properly cache the results, and (thus) avoid future touches >> on the backend server, then we're good to go. >> >> If the patch doesn't affect the caching (which it sounds like "no"), then >> just go with it. Sure, it is neat to look at ayscalls, but... why? I don't >> understand the need to examine/profile. Educate me? >> > > The patch should not affect HTTP caching for two reasons: > 1. Browsers and proxies supports ETag and use it instead of > Last-Modified header. > 2. ETag and Last-Modified headers are used only for cache > re-validation when max-age is expired. But Subversion sets max-age=1 > week for resources with specific revision in URL > (http://server/!svn/rvr/1/path). max-age=0 is only used for public > URLs without revision, i.e. http://server/path) > > As far I know proxy usage are limited to public servers with anonymous > access, since caching of HTTP responses with Authorization is > prohibited by RFC. > > Anyway I agree that trading bandwidth usage to save some CPU/IO on the > server doesn't make sense, but Last-Modified case is the different: > Subversion server wasting 10%+ of server resources to produce unused > header. > > I don't have access to svn.apache.org server performance stats, but I > suppose it's pretty busy server and Infra team would welcome any > Subversion server performance improvements. > Committed in r1724790. -- Ivan Zhakov
Re: [RFC/PATCH] Handling PROPFIND in mod_dav_svn (was: Re: Issue with browsing a SVN 1.9.2, schema 7, packed, repository)
On 15 January 2016 at 09:26, Daniel Shahaf <danie...@apache.org> wrote: > Evgeny Kotkov wrote on Thu, Jan 14, 2016 at 18:40:11 +0300: >> Not quite sure on how do we continue from here. > > Well, it isn't consensus, but there's an old fallback we can use: > pick an agreed-upon third party and let him decide. > > So let's ask a mod_dav/mod_dav_svn dev for his opinion and do what he says. > I am not a mod_dav developer, but I've been hacking on mod_dav_svn for quite a while now. In my opinion we should start reimplemeting mod_dav in mod_dav_svn like we already did with implementing our own HTTPv2 protocol instead of DAV and move forward. As far as I remember we discussed this approach with Ben Reser two years ago on SVN Hackathon in Berlin and he said that this might be good idea. -- Ivan Zhakov
Re: FS Node API
> consistent (e.g. svn_fs_node_make instead of > svn_fs_node_make_file and svn_fs_node_make_dir). > I think it's of scope at least for experimental branch. Personally I'm fine with current naming and would like to avoid just flavoring changes. > > Observations in my Experiments > -- > > * Despite calling it "node API" it actually concerned with > efficiently navigating directories. So, always represent > the node using 2 structs: the actual node plus a (usually > shared) struct containing all the context info - representing > the directory. > > * The parameters for svn_fs_node_make, svn_fs_node_copy etc. > should take pure paths to describe the target instead of > parent-node + local-name. > > * Some function API function names clash with existing ones, > i.e. need to be revved (svn_fs_node_history3, > svn_fs_node_relation2 and everything prop). > > * Txn nodes and revision nodes should use 2 different > instances of the vtable. Error handling for operations > that either apply to txn nodes or revision nodes only > can then be moved to lib_fs by checking for NULL pointers > in the vtable. > What is purpose of different vtables for txn and revision nodes? > > Proposal for an Implementation Strategy > --- > > * node_compat.* > - Provide a default / fallback implementation in lib_fs > for backends that don't have native node API support. > - Blueprint for backend code, i.e. ensure we can have > a clean structure > - Implement API (roughly) one function at a time. > > * Ensure semantic equivalence to old API and high test > coverage from the start > - Implement node_compat in terms of the old backend API > - Implement the old FS API in terms of the new API, > i.e. route all calls through node_compat.* > - Have #define that controls whether the old API is run > natively or gets diverted through node_compat.* > - If the backend vtable entry is NULL for an old API > function, divert it through node_compat.* I've already added '#if 0' code for that. See svn_fs_open_node(). > > * Switch API users over to the new API > - This is what makes the branch worthwhile. > - Deprecate the old API only after migrating most callers > > * Implement the node API in FSFS > - Take node_compat and only replace the vtable calls > with direct function calls. > - Add direct addressing info (noderev IDs) to the structs. I don't understand this point. Why we cannot use pointer to dag_node_t for svn_fs_node_t implemention in FSFS? > - Switch functions over to "native" code. > - NULL the old API vtable entries to enforce the new > backend logic. > > * Final review and merger into /trunk > - Report processing as in "diff --summary" is a good > performance indicator. My first goal is to switch 'svn ls -v' to new API and see how it works. -- Ivan Zhakov
Re: Last-Modified HTTP header in GET responses
On 6 January 2016 at 08:14, Greg Stein <gst...@gmail.com> wrote: > Personally, I'd be more interested in the effects on the network and its > caching ability. Do we really need to save CPU/IO on the server? Today's > servers seem more than capable, and are there really svn servers out in the > wild getting so crushed, that this is important? It seems that as long as > proxies/etc can properly cache the results, and (thus) avoid future touches > on the backend server, then we're good to go. > > If the patch doesn't affect the caching (which it sounds like "no"), then > just go with it. Sure, it is neat to look at ayscalls, but... why? I don't > understand the need to examine/profile. Educate me? > The patch should not affect HTTP caching for two reasons: 1. Browsers and proxies supports ETag and use it instead of Last-Modified header. 2. ETag and Last-Modified headers are used only for cache re-validation when max-age is expired. But Subversion sets max-age=1 week for resources with specific revision in URL (http://server/!svn/rvr/1/path). max-age=0 is only used for public URLs without revision, i.e. http://server/path) As far I know proxy usage are limited to public servers with anonymous access, since caching of HTTP responses with Authorization is prohibited by RFC. Anyway I agree that trading bandwidth usage to save some CPU/IO on the server doesn't make sense, but Last-Modified case is the different: Subversion server wasting 10%+ of server resources to produce unused header. I don't have access to svn.apache.org server performance stats, but I suppose it's pretty busy server and Infra team would welcome any Subversion server performance improvements. -- Ivan Zhakov
Re: Last-Modified HTTP header in GET responses
On 4 January 2016 at 16:25, Philip Martin <philip.mar...@wandisco.com> wrote: > Branko Čibej <br...@apache.org> writes: > >> Your analysis looks sound, but I wonder if doing this would have any >> serious effect on checkout/update times; after all, the bulk of the work >> there is in report generation, only HTTPv2 is affected by GET response >> construction. > > When I checkout Subversion trunk from my local mirror I cannot measure a > client gain, but I can measure better server efficiency: > > - The CPU used by Apache goes down from 1.2s to 1.1s. > > - The number of system calls made by Apache goes down > > 68822 to 50664 for hot FSFS cache > 178885 to 161722 for cold FSFS cache > I'm getting similar results on Windows. svnbench null-export of 2000 15kb random files in 20 directories: Cold FSFS caches: CPU: 2028ms, IO Reads: 16350, Elapsed: 1374ms Hot FSFS caches: CPU: 1482ms, IO Reads: 9403, Elapsed: 1253ms Patched: Cold FSFS caches: CPU: 1794ms, IO Reads: 14440, Elapsed: 1230ms Hot FSFS caches: CPU: 1263ms, IO Reads: 7787, Elapsed: 1184ms Elapsed is total execution time of svnbench null-export. CPU is processor used by server to serve client operation, IO reads is number of IO read operations of server process. So it's about 10% improvement in case of hot disk caches and local disk. Improvement could be more significant in case of network storage, spinning disks or high-load. I've also tested with 128 MB FSFS caches and full-text caching enabled: Baseline: Cold: CPU: 2028ms, IO Reads: 16028, Elapsed: 1643ms Hot: CPU: 1138ms, IO Reads: 2083, Elapsed: 1164ms Patched: Cold: CPU: 1934ms, IO Reads: 14028, Elapsed: 1473ms Hot: CPU: 1060ms, IO Reads:83, Elapsed: 1059ms -- Ivan Zhakov
Last-Modified HTTP header in GET responses
Currently mod_dav_svn sets ETag and Last-Modified HTTP headers for GET
responses. These headers are optional and and are not used by
Subversion client. But they used by browsers and intermediate proxies
to cache responses.
ETag header is cheap to construct: it's just last modification
revision number of the node.
Last-Modified header is relatively expensive to calculate: it's
svn:date revision property of revision where path was modified.
Revision properties are mutable and cannot be cached effectively.The
other minor problem with Last-Modified header: svn:date revision
property can be changed to any value, while RFC 7223 requires
Last-Modified date to be earlier than the server's time of message
origination (Date) [1]
All browsers support ETag header (it's HTTP/1.1 header) and RFC 7232
recommends to prefer using ETag instead of Last-Modified [2].
Given all above I propose to stop adding Last-Modified header for HTTP
GET responses.
[1] https://tools.ietf.org/html/rfc7232#section-2.2.1
[2] https://tools.ietf.org/html/rfc7232#section-3.3
--
Ivan Zhakov
Index: subversion/mod_dav_svn/repos.c
===
--- subversion/mod_dav_svn/repos.c (revision 1721898)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -3036,50 +3036,6 @@
&& resource->baselined))
-/* Return the last modification time of RESOURCE, or -1 if the DAV
- resource type is not handled, or if an error occurs. Temporary
- allocations are made from RESOURCE->POOL. */
-static apr_time_t
-get_last_modified(const dav_resource *resource)
-{
- apr_time_t last_modified;
- svn_error_t *serr;
- svn_revnum_t created_rev;
- svn_string_t *date_time;
-
- if (RESOURCE_LACKS_ETAG_POTENTIAL(resource))
-return -1;
-
- if ((serr = svn_fs_node_created_rev(_rev, resource->info->root.root,
- resource->info->repos_path,
- resource->pool)))
-{
- svn_error_clear(serr);
- return -1;
-}
-
- if ((serr = svn_fs_revision_prop2(_time, resource->info->repos->fs,
-created_rev, SVN_PROP_REVISION_DATE,
-TRUE, resource->pool, resource->pool)))
-{
- svn_error_clear(serr);
- return -1;
-}
-
- if (date_time == NULL || date_time->data == NULL)
-return -1;
-
- if ((serr = svn_time_from_cstring(_modified, date_time->data,
-resource->pool)))
-{
- svn_error_clear(serr);
- return -1;
-}
-
- return last_modified;
-}
-
-
const char *
dav_svn__getetag(const dav_resource *resource, apr_pool_t *pool)
{
@@ -3149,7 +3105,6 @@
svn_error_t *serr;
svn_filesize_t length;
const char *mimetype = NULL;
- apr_time_t last_modified;
/* As version resources don't change, encourage caching. */
if (is_cacheable(r, resource))
@@ -3161,15 +3116,6 @@
if (!resource->exists)
return NULL;
- last_modified = get_last_modified(resource);
- if (last_modified != -1)
-{
- /* Note the modification time for the requested resource, and
- include the Last-Modified header in the response. */
- ap_update_mtime(r, last_modified);
- ap_set_last_modified(r);
-}
-
/* generate our etag and place it into the output */
apr_table_setn(r->headers_out, "ETag",
dav_svn__getetag(resource, resource->pool));
Re: 1.9.3 up for signing/testing
On 8 December 2015 at 11:47, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > The 1.9.3 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. I plan to try and release on December 15th > so please try and get your votes/signatures in place by December 14th. > Summary: +1 to release. TESTED: --- [ fsfs ] x [ file | svn | http v1 | http v2] x [win32 | x64] RESULTS: All Tests Pass, except update_tests.py 76: "test filename with backslashes inside" over http:// protocol with Apache HTTP Server 2.2.31. But it seems to be false negative, because it returns the same error that expected for other protocols. PLATFORM: - MS Windows 7 Ultimate (x64) Microsoft Visual Studio 2013 Update 3 DEPENDENCIES: - APR: 1.5.2 APR-Util: 1.5.4 Apache HTTPD: 2.2.31 zlib: 1.2.8 OpenSSL: 1.0.1q sqlite: 3.7.12.1 Python: 2.7.8 serf: 1.3.8 Signatures: --- (See the appropriate distribution files.) -- Ivan Zhakov
Re: 1.8.15 up for signing/testing
On 8 December 2015 at 11:47, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > The 1.8.15 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. I plan to try and release on December 15th > so please try and get your votes/signatures in place by December 14th. > > Thanks! Summary: +1 to release. TESTED: --- [ fsfs ] x [ file | svn | http v1 | http v2] x [win32 | x64] RESULTS: All Tests Pass PLATFORM: - MS Windows 7 Ultimate (x64) Microsoft Visual Studio 2013 Update 3 DEPENDENCIES: - APR: 1.5.2 APR-Util: 1.5.4 Apache HTTPD: 2.2.31 zlib: 1.2.8 OpenSSL: 1.0.1q sqlite: 3.7.12.1 Python: 2.7.8 serf: 1.3.8 Signatures: --- (See the appropriate distribution files.) -- Ivan Zhakov
Re: apr_hash_overlay returns hash with duplicate keys
On 10 December 2015 at 20:20, Julian Foad <julianf...@apache.org> wrote:
> Ivan Zhakov wrote:
>> On 10 December 2015 at 19:14, Julian Foad <julianf...@apache.org> wrote:
>>> APR devs, Subversion devs:
>>>
>>> On Subversion's Mac OS buildbots it appears that apr_hash_overlay()
>>> sometimes returns a hash containing duplicate keys, which (as I
>>> understand it) should be impossible.
>>>
>>> We had an issue where some 'svnmover' tests were failing only on Mac
>>> OS buildbots. I added some debugging in Subversion commits r1719056,
>>> r1719067, r1719072, r1719074.
>>>
>>> Buildbot result:
>>>
>>> https://ci.apache.org/builders/svn-x64-macosx-bdb/builds/485/steps/Test%20ra_svn%2Bbdb
>>> --> debug output in 'faillog' shows duplicate keys in hash:
>>>"union_children={A, iota, foo, boozle, boozle, iota}"
>>>
>>> I replaced apr_hash_overlay() with my own simple re-implementation:
>>>
>>> http://svn.apache.org/r1719089 -- re-implement hash overlay
>>>
>> Hi Julian,
>>
>> That could be possible if two hashes uses different hash functions.
>> This could happen if you're using svn_hash__make() directly or
>> indirectly: for example RA get_dirent for svn:// protocol returns hash
>> with non-standard hash-function.
>
> Ugh. Yes, that is probably the cause. Thanks.
>
> I can see now that the doc string of apr_hash_overlay() does say "Both
> hash tables must use the same hash function" but that was not obvious,
> and I had totally forgotten that our Subversion code was using more
> than one hash function.
>
> I will use my own hash overlay function from now on.
>
I don't think this is proper fix for this problem. I think returning
hash with non-standard hash function from public API is a bug (and API
regression). Other API users may get to the same situation. So proper
fix would be revert these optimizations from public API imo.
--
Ivan Zhakov
Re: apr_hash_overlay returns hash with duplicate keys
On 11 December 2015 at 04:12, Branko Čibej <br...@apache.org> wrote: > On 10.12.2015 18:23, Ivan Zhakov wrote: >> I think returning >> hash with non-standard hash function from public API is a bug (and API >> regression). Other API users may get to the same situation. So proper >> fix would be revert these optimizations from public API imo. > > I really can't agree with this. A user of the APR public API cannot > assume which hash function is used by any instance of apr_hash_t that > she did not create herself. In other words, no-one should be calling > apr_hash_overlay() (with APR <= 1.4.5) unless they know that the hashes > use the same hash function. > > The only "bug" here is a design bug in older versions of > apr_hash_overlay() (and apr_hash_merge()), and even that is debatable. > This sounds good in theory, but in reality even Subversion developers encounters this really weird behavior. Btw is it possible to backport apr_hash_overlay()/apr_hash_merge() fix to APR 1.3.x? > As far as the Subversion code is concerned, we should, IMO, be using > svn_hash__make() everywhere, since we have it. > We cannot use svn_hash__make() everywhere at least it doesn't immune to hash-collision attacks like CVE-2012-0840 [1] [1] https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2012-0840 -- Ivan Zhakov
Re: apr_hash_overlay returns hash with duplicate keys
On 11 December 2015 at 06:18, William A Rowe Jr <wr...@rowe-clan.net> wrote: > Which public API, APR's or svn's? > Subversion's. Sorry I forgot that dev@a.a.o list is in cc. -- Ivan Zhakov
Re: apr_hash_overlay returns hash with duplicate keys
On 10 December 2015 at 19:14, Julian Foad <julianf...@apache.org> wrote:
> APR devs, Subversion devs:
>
> On Subversion's Mac OS buildbots it appears that apr_hash_overlay()
> sometimes returns a hash containing duplicate keys, which (as I
> understand it) should be impossible.
>
> We had an issue where some 'svnmover' tests were failing only on Mac
> OS buildbots. I added some debugging in Subversion commits r1719056,
> r1719067, r1719072, r1719074.
>
> Buildbot result:
>
> https://ci.apache.org/builders/svn-x64-macosx-bdb/builds/485/steps/Test%20ra_svn%2Bbdb
> --> debug output in 'faillog' shows duplicate keys in hash:
>"union_children={A, iota, foo, boozle, boozle, iota}"
>
> I replaced apr_hash_overlay() with my own simple re-implementation:
>
> http://svn.apache.org/r1719089 -- re-implement hash overlay
>
Hi Julian,
That could be possible if two hashes uses different hash functions.
This could happen if you're using svn_hash__make() directly or
indirectly: for example RA get_dirent for svn:// protocol returns hash
with non-standard hash-function.
--
Ivan Zhakov
Re: svn commit: r1716575 - in /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c
On 26 November 2015 at 11:05, <rhuij...@apache.org> wrote: > > Author: rhuijben > Date: Thu Nov 26 08:05:36 2015 > New Revision: 1716575 > > URL: http://svn.apache.org/viewvc?rev=1716575=rev > Log: > In ra_serf: when a to-be committed file has text and property changes to be > applied, pipeline both requests. > This could fail over HTTP/2 if both pipelined requests will be handled by different worker threads: FSFS doesn't allow concurrent access to transactions. -- Ivan Zhakov
Re: svn commit: r1716575 - in /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c
On 26 November 2015 at 11:41, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message----- >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> Sent: donderdag 26 november 2015 09:19 >> To: dev@subversion.apache.org; Bert Huijben <b...@qqmail.nl> >> Subject: Re: svn commit: r1716575 - in >> /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c >> >> On 26 November 2015 at 11:05, <rhuij...@apache.org> wrote: >> > >> > Author: rhuijben >> > Date: Thu Nov 26 08:05:36 2015 >> > New Revision: 1716575 >> > >> > URL: http://svn.apache.org/viewvc?rev=1716575=rev >> > Log: >> > In ra_serf: when a to-be committed file has text and property changes to >> be >> > applied, pipeline both requests. >> > >> This could fail over HTTP/2 if both pipelined requests will be handled >> by different worker threads: FSFS doesn't allow concurrent access to >> transactions. > > I'm surprised to learn that. I would have guessed concurrent access was > always part of the design of the fs layer, by the way we use it in mod_dav. > > I hope we can somehow lift that restriction, as improving commit performance > over mod_dav is high on a few wish lists. > First of all I'm not sure that concurrent *writing* could speed up commit: there is no fsync() calls when working with transactions. As far I remember svn:// also waits for every TXN operation to complete before sending next one, so svn:// and http:// performance should be the same when working over high-latency network. Another problem could be than TXN operations are have dependencies on each other and order is very important. -- Ivan Zhakov
Re: svn commit: r1716575 - in /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c
On 26 November 2015 at 12:11, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message----- >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> Sent: donderdag 26 november 2015 09:54 >> To: Bert Huijben <b...@qqmail.nl> >> Cc: dev@subversion.apache.org >> Subject: Re: svn commit: r1716575 - in >> /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c >> >> On 26 November 2015 at 11:41, Bert Huijben <b...@qqmail.nl> wrote: >> >> -Original Message- >> >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> >> Sent: donderdag 26 november 2015 09:19 >> >> To: dev@subversion.apache.org; Bert Huijben <b...@qqmail.nl> >> >> Subject: Re: svn commit: r1716575 - in >> >> /subversion/trunk/subversion/libsvn_ra_serf: commit.c ra_serf.h util.c >> >> >> >> On 26 November 2015 at 11:05, <rhuij...@apache.org> wrote: >> >> > >> >> > Author: rhuijben >> >> > Date: Thu Nov 26 08:05:36 2015 >> >> > New Revision: 1716575 >> >> > >> >> > URL: http://svn.apache.org/viewvc?rev=1716575=rev >> >> > Log: >> >> > In ra_serf: when a to-be committed file has text and property changes >> to >> >> be >> >> > applied, pipeline both requests. >> >> > >> >> This could fail over HTTP/2 if both pipelined requests will be handled >> >> by different worker threads: FSFS doesn't allow concurrent access to >> >> transactions. >> > >> > I'm surprised to learn that. I would have guessed concurrent access was >> > always part of the design of the fs layer, by the way we use it in mod_dav. >> > >> > I hope we can somehow lift that restriction, as improving commit >> performance >> > over mod_dav is high on a few wish lists. >> > >> First of all I'm not sure that concurrent *writing* could speed up >> commit: there is no fsync() calls when working with transactions. As >> far I remember svn:// also waits for every TXN operation to complete >> before sending next one, so svn:// and http:// performance should be >> the same when working over high-latency network. > > No, in svn:// the client sends out all the requests and only peeks the > connection to see if there is waiting data (an error) during commit. The > client > only waits for the server after the entire commit is completed. Not after > every txn operation. Yes, you are right. But such error handling will make connection unusable after error, is not it? And as far I remember ra_svn doesn't have code to reconnect if needed. > > This is why 'in general' you get not identical error reporting on commits > when you use > svn:// (or svn+ssh://, etc.). In many cases the only failure you see is the > text from the > server as the client doesn't even know which file/directory failed during the > commit. > > With a bit of luck you receive errors a bit earlier than the final commit > step. But with > <= 1.9.0 this didn't work for svn+ssh:// on Windows at all. (We handled the > error for > not implemented as if no data was waiting) > Ouch. I think it's just an example that async/delayed error handling is very tricky. -- Ivan Zhakov
Re: svn commit: r1715832 - /subversion/trunk/win-tests.py
On 23 November 2015 at 17:29, <rhuij...@apache.org> wrote:
> Author: rhuijben
> Date: Mon Nov 23 14:29:59 2015
> New Revision: 1715832
>
> URL: http://svn.apache.org/viewvc?rev=1715832=rev
> Log:
> * win-tests.py
> (Svnserve.start): Pass the SVN_DBG_STACKTRACES_TO_STDERR environment
> variable to svnserve.exe to get more information on crashes on testruns.
>
> Modified:
> subversion/trunk/win-tests.py
>
> Modified: subversion/trunk/win-tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/win-tests.py?rev=1715832=1715831=1715832=diff
> ==
> --- subversion/trunk/win-tests.py (original)
> +++ subversion/trunk/win-tests.py Mon Nov 23 14:29:59 2015
> @@ -447,7 +447,9 @@ class Svnserve:
[...]
> if use_ssl and use_http2:
> - fp.write('Protocols h2 http/1.1\n')
> + fp.write('Protocols h2\n')
> elif use_http2:
> - fp.write('Protocols h2c http/1.1\n')
> + fp.write('Protocols h2c\n')
>fp.write('H2Direct on\n')
^^^ Looks like unintended change.
>
> # Don't handle .htaccess, symlinks, etc.
--
Ivan Zhakov
Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 12:18, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message----- >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> Sent: vrijdag 20 november 2015 10:12 >> To: Bert Huijben <b...@qqmail.nl> >> Cc: dev@subversion.apache.org; d...@serf.apache.org >> Subject: Re: svn commit: r1715228 - >> /subversion/trunk/subversion/libsvn_ra_serf/util.c >> >> On 20 November 2015 at 11:53, Bert Huijben <b...@qqmail.nl> wrote: >> > Ack. >> > >> > Not pipelining, or not sending simultaneous/parallel requests if you don’t >> > want to depend on the order in which they are received is the safest thing. >> > >> > And the current code can break even in http/1. Svnrdump even has special >> > precautions for this even though I have no idea why it would see the >> > problems it describes it has. (The editor drive is 100% from one http >> > response) >> > >> > What we could do is replace the current assertion with a request on a >> > completely different connection to retrieve the properties if we don’t have >> > them in time… as a fallback mechanism to at least continue going. This also >> > works for legacy servers if the first improvement is applied. >> > >> > I’m not sure if the current implementation has more problems… E.g. if >> > revisions can also be received out of order. >> > >> > Going to a single report –that includes the revprops- for multiple >> > revisions >> > is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike. >> > >> I understand that current is also unsafe, that's why I suggest go >> single REPORT and disable pipeling for older servers. I'll add this to >> my TODO list you agree that this approach makes sense. > > +1. > Ok, I'll try implement it. But I want release Subversion 1.9.3 first. > This would solve +- 50% of the current testfailures running over http/2. > > But we should implement a fix that works for old servers too. I will work on > that part. > Yes, but I think disable pipelining is also viable option if we implement replay-range REPORT. > > At least some other failures I see are caused by getting httpd temporarily > in a 100% CPU loop, which causes other tests that run at the same time to > fail. My current best guess is that this is a server side issue, but I'll > have to > investigate. But not in the daytime hours of the next few days. > > > Note that the easiest way not to pipeline is: not scheduling requests that > you are not able to handle yet :) > > Bert > -- Ivan Zhakov
Re: svn+ssh long-lived daemon
On 20 November 2015 at 17:20, Mark Phippard <markp...@gmail.com> wrote: > I've always felt the same, but now that I've used SSH more (with Git) I kind > of question it. > > Are HTTP client certs much better than passwords? The cert itself still has > to be physically secured and if you protect the cert with a passphrase then > you have all of the same cache problems that passwords do. > HTTP client certs a slightly better than passwords because evildoer cannot intercept password over the wire. But on the other hand connection is already encrypted so even plain-text password is not big problem. -- Ivan Zhakov
Re: svn+ssh long-lived daemon
On 20 November 2015 at 22:02, Branko Čibej <br...@apache.org> wrote: > On 20.11.2015 15:20, Mark Phippard wrote: >> I've always felt the same, but now that I've used SSH more (with Git) I >> kind of question it. >> >> Are HTTP client certs much better than passwords? > > Please ... SSL/TLS client certs. Just nitpicking to make sure we use > correct terminology. > > >> The cert itself still >> has to be physically secured and if you protect the cert with a passphrase >> then you have all of the same cache problems that passwords do. > > Yup. > >> With SSH there is infrastructure like ssh-agent that just does not exist >> for HTTP. > > s/HTTP/TLS/ but otherwise, yes. Also with X509 certificates you force > users to either rely on a 3rd-party authority or create self-signed > certs, which are equivalent to SSH keypairs, just a lot more complicated > to manage. > > It's, IMO, it would be a better idea to integrate, e.g., libssh2 > directly into our code as an alternative to using an external SSH tool. > I'm sure we could make long-term tunnel management work on the RA level. > As far I understand Philip's goal to reuse svnserve process on the server, that means we would need ssh protocol server-side implementation in svnserve. -- Ivan Zhakov
Re: svn commit: r1715228 -/subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 11:53, Bert Huijben <b...@qqmail.nl> wrote: > Ack. > > Not pipelining, or not sending simultaneous/parallel requests if you don’t > want to depend on the order in which they are received is the safest thing. > > And the current code can break even in http/1. Svnrdump even has special > precautions for this even though I have no idea why it would see the > problems it describes it has. (The editor drive is 100% from one http > response) > > What we could do is replace the current assertion with a request on a > completely different connection to retrieve the properties if we don’t have > them in time… as a fallback mechanism to at least continue going. This also > works for legacy servers if the first improvement is applied. > > I’m not sure if the current implementation has more problems… E.g. if > revisions can also be received out of order. > > Going to a single report –that includes the revprops- for multiple revisions > is a safe extension, that will improve in all cases: HTTP and HTTP/2 alike. > I understand that current is also unsafe, that's why I suggest go single REPORT and disable pipeling for older servers. I'll add this to my TODO list you agree that this approach makes sense. > I will start looking in supporting priority scheduling anyway…. But that > requires more work in other parts of serf first. (I can’t use the request as > an argument while serf still thinks it can destroy and recreate requests at > a different address at will as part of the auth handling) > > Ack. -- Ivan Zhakov
Re: svn commit: r1715228 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
On 20 November 2015 at 00:03, Bert Huijben <b...@qqmail.nl> wrote: >> -Original Message- >> From: rhuij...@apache.org [mailto:rhuij...@apache.org] >> Sent: donderdag 19 november 2015 19:03 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1715228 - >> /subversion/trunk/subversion/libsvn_ra_serf/util.c >> >> Author: rhuijben >> Date: Thu Nov 19 18:03:11 2015 >> New Revision: 1715228 >> >> URL: http://svn.apache.org/viewvc?rev=1715228=rev >> Log: >> Add a tiny bit of code to allow testing with Apache Serf's http/2 support. >> >> I committed this patch to celebrate that I got through basic_tests.py >> using the current http/2 support. >> >> * subversion/libsvn_ra_serf/util.c >> (conn_negotiate_protocol): New function. >> (conn_setup): Register usage of conn_negotiate_protocol when >> a very recent (read: trunk) serf + SVN__SERF_TEST_HTTP2 are used. > > Currently most tests pass for me over http2 when running with some minor > tweaks. I still get some aborted connections that aren't retried as cleanly > as with http 1.1. Not sure what causes these yet; but this could also be an > httpd issues. > > The only ra function that really causes problems is the implementation of the > replay > range api. This 100% expects that results will be received in the same order > in which > they are sent. This is typically the correct way in http 1.1 using serf, but > even there it > is sometimes possible to get results out of order. (Authentication can > re-queue > requests... and sometimes authorization is necessary even when earlier > requests > succeeded, depending on the auth scheme). > Can we use WINDOW_UPDATE frame to control flow of replay-revision response? In this case we can use spillbuf for buffering responses to avoid latency performance regressions. What do you think? -- Ivan Zhakov
Re: svn+ssh long-lived daemon
On 20 November 2015 at 06:08, Daniel Shahaf <d...@daniel.shahaf.name> wrote: > Philip Martin wrote on Thu, Nov 19, 2015 at 18:22:29 +: >> Are there alternative ways to get a long-lived daemon to do >> authentication with public/private key pairs? > > 1. Plain old ssh port forwarding: > > server# svnserve -d > client% ssh -MNf -L 3690:localhost:3690 $remotebox > client% svn info svn://localhost/myrepos > > 2. Same, but without allocating a TCP port on the client: > > server# svnserve -d > client% cat .subversion/config > [tunnels] > office = $SVN_OFFICE ssh -W localhost:3690 svn.office.com.example ;: > client% svn info svn+office:///myrepos > > The ";:" at the end is to ignore the "svnserve -t" string that gets > appended to the command line after stripping the variable and before > passing it to system(). The URI has an an empty "host:port" part > because the tunnel hardcodes the hostname. The client might still run > 'ssh -MNf' beforehand, but unlike in #1 where running ssh manually was > required, here it is merely a performance optimization. > > "ControlPath" may need to be set in ssh_config(5). > > 3. VPN with key-based authentication, then just use svn:// over the VPN > subnet. For example, OpenVPN can do this. > > 4. An ra_svn proxy that adds authentication info. The server runs > 'svnserve -i --listen-host=localhost' and sshd. In > .ssh/authorized_keys, instead of running socat as in your description, > run a proxy that understands the ra_svn protocol, intercepts > server-to-client authentication requests and answers them with > credentials determined as a function of the authenticated ssh identity, > and passes everything else back-and-forth unmodified. > 5. HTTPS authentication using client certificates -- Ivan Zhakov
