Re: crash opening Repository Browser for svn:// archives
I have a reproducible crash when trying to open the repository browser, sparse - checkout selection, revision graph and probably other features. Reproduce: - tortoiseproc /command:repobrowser /path:svn://somewhere - enter your correct credentials It seems to only happen for svn:// repositories. (I don't know whether it also happens for non-authenticated). I'm getting the same crash with: Windows 7 SP1 x64 TortoiseSVN 1.6.99, Build 21919 - 64 Bit -dev, 2011/08/31 21:32:47 Subversion 1.7.0, -dev apr 1.4.5 apr-utils 1.3.12 neon 0.29.6 OpenSSL 1.0.0d 8 Feb 2011 zlib 1.2.5 Child-SP RetAddr Call Site `0445f448 07fe`edbbaa27 0x6a `0445f450 07fe`f89ab49a libsasl!_sasl_log+0x627 [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\common.c @ 1924] `0445f520 07fe`edbb53c2 saslDIGESTMD5!digestmd5_client_mech_dispose+0x4a [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\plugins\digestmd5.c @ 4174] `0445f560 07fe`edbb84aa libsasl!client_dispose+0x72 [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\client.c @ 289] `0445f5a0 07fe`f0a24e8d libsasl!sasl_dispose+0x5a [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\common.c @ 848] `0445f5e0 `6c6c3eee libsvn_tsvn!svn_ra_svn__sasl_init+0x4d (note: this is actually sasl_dispose_cb) `0445f610 `6c6c3ecf libapr_tsvn!apr_pool_destroy+0x6e `0445f640 `6c6c3ecf libapr_tsvn!apr_pool_destroy+0x4f `0445f670 0001`3fb625ce libapr_tsvn!apr_pool_destroy+0x4f `0445f6a0 0001`3fc3ef04 TortoiseProc+0x725ce `0445f7c0 0001`3fc96ca5 TortoiseProc+0x14ef04 `0445f830 0001`3fc97c20 TortoiseProc+0x1a6ca5 `0445f860 0001`3fc98f4d TortoiseProc+0x1a7c20 `0445f8a0 `6a871bc7 TortoiseProc+0x1a8f4d `0445f8d0 `6a871c55 MSVCR100!endthread+0x53 `0445f900 `773e652d MSVCR100!endthread+0xe1 `0445f930 `77adc521 kernel32!BaseThreadInitThunk+0xd `0445f960 ` ntdll!RtlUserThreadStart+0x1d The crash is coming from libsasl's common.c line 1924, where it's trying to call through the log_cb function pointer. However, log_cb is 0xfa (or some other value that's definitely not the address of a function). log_cb is set by the call to _sasl_getcallback() earlier (line 1788), and it turns out the problem is that conn-callbacks points to corrupted memory, and _sasl_getcallback() happens to find the right bytes to make it think that it's found a log callback function. Subversion's libsvn_ra_svn\cyrus_auth.c svn_ra_svn__do_cyrus_auth() (line 732) looks fishy to me, although I certainly haven't looked into it in detail... The callbacks[] array, which is passed ot new_sasl_ctx(), then to libsasl's sasl_client_new(), is a local/automatic variable, and it goes out of scope and the memory is available for reuse after the function exits. However, new_sasl_ctx() adds the sasl_ctx to a pool and registers a cleanup function for it. When the pool is destroyed, libsasl wants to use that callbacks[] array during the cleanup, but that memory's already filled with other stuff. Shouldn't callbacks[] be allocated on the heap somehow, rather than on the stack?
Re: crash opening Repository Browser for svn:// archives
On Thu, Sep 08, 2011 at 01:14:27AM -0500, Dave Huang wrote: I have a reproducible crash when trying to open the repository browser, sparse - checkout selection, revision graph and probably other features. Reproduce: - tortoiseproc /command:repobrowser /path:svn://somewhere - enter your correct credentials It seems to only happen for svn:// repositories. (I don't know whether it also happens for non-authenticated). I'm getting the same crash with: Windows 7 SP1 x64 TortoiseSVN 1.6.99, Build 21919 - 64 Bit -dev, 2011/08/31 21:32:47 Subversion 1.7.0, -dev apr 1.4.5 apr-utils 1.3.12 neon 0.29.6 OpenSSL 1.0.0d 8 Feb 2011 zlib 1.2.5 Child-SP RetAddr Call Site `0445f448 07fe`edbbaa27 0x6a `0445f450 07fe`f89ab49a libsasl!_sasl_log+0x627 [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\common.c @ 1924] `0445f520 07fe`edbb53c2 saslDIGESTMD5!digestmd5_client_mech_dispose+0x4a [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\plugins\digestmd5.c @ 4174] `0445f560 07fe`edbb84aa libsasl!client_dispose+0x72 [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\client.c @ 289] `0445f5a0 07fe`f0a24e8d libsasl!sasl_dispose+0x5a [c:\projects\svn\tortoisesvn\ext\cyrus-sasl\lib\common.c @ 848] `0445f5e0 `6c6c3eee libsvn_tsvn!svn_ra_svn__sasl_init+0x4d (note: this is actually sasl_dispose_cb) `0445f610 `6c6c3ecf libapr_tsvn!apr_pool_destroy+0x6e `0445f640 `6c6c3ecf libapr_tsvn!apr_pool_destroy+0x4f `0445f670 0001`3fb625ce libapr_tsvn!apr_pool_destroy+0x4f `0445f6a0 0001`3fc3ef04 TortoiseProc+0x725ce `0445f7c0 0001`3fc96ca5 TortoiseProc+0x14ef04 `0445f830 0001`3fc97c20 TortoiseProc+0x1a6ca5 `0445f860 0001`3fc98f4d TortoiseProc+0x1a7c20 `0445f8a0 `6a871bc7 TortoiseProc+0x1a8f4d `0445f8d0 `6a871c55 MSVCR100!endthread+0x53 `0445f900 `773e652d MSVCR100!endthread+0xe1 `0445f930 `77adc521 kernel32!BaseThreadInitThunk+0xd `0445f960 ` ntdll!RtlUserThreadStart+0x1d The crash is coming from libsasl's common.c line 1924, where it's trying to call through the log_cb function pointer. However, log_cb is 0xfa (or some other value that's definitely not the address of a function). log_cb is set by the call to _sasl_getcallback() earlier (line 1788), and it turns out the problem is that conn-callbacks points to corrupted memory, and _sasl_getcallback() happens to find the right bytes to make it think that it's found a log callback function. Subversion's libsvn_ra_svn\cyrus_auth.c svn_ra_svn__do_cyrus_auth() (line 732) looks fishy to me, although I certainly haven't looked into it in detail... The callbacks[] array, which is passed ot new_sasl_ctx(), then to libsasl's sasl_client_new(), is a local/automatic variable, and it goes out of scope and the memory is available for reuse after the function exits. However, new_sasl_ctx() adds the sasl_ctx to a pool and registers a cleanup function for it. When the pool is destroyed, libsasl wants to use that callbacks[] array during the cleanup, but that memory's already filled with other stuff. Shouldn't callbacks[] be allocated on the heap somehow, rather than on the stack? Can you try this patch? It ties the callbacks to the pool which the sasl context lives in. Index: subversion/libsvn_ra_svn/cyrus_auth.c === --- subversion/libsvn_ra_svn/cyrus_auth.c (revision 1166543) +++ subversion/libsvn_ra_svn/cyrus_auth.c (working copy) @@ -738,9 +738,7 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__session_bato const char *mechstring = , *last_err = , *realmstring; const char *local_addrport = NULL, *remote_addrport = NULL; svn_boolean_t success; - /* Reserve space for 3 callbacks (for the username, password and the - array terminator). */ - sasl_callback_t callbacks[3]; + sasl_callback_t *callbacks; cred_baton_t cred_baton; int i; @@ -776,6 +774,10 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__session_bato cred_baton.realmstring = realmstring; cred_baton.pool = pool; + /* Reserve space for 3 callbacks (for the username, password and the + array terminator). */ + callbacks = apr_palloc(sess-conn-pool, sizeof(*callbacks) * 3); + /* Initialize the callbacks array. */ /* The username callback. */
Re: crash opening Repository Browser for svn:// archives
On 9/8/2011 2:22 AM, Stefan Sperling wrote: Can you try this patch? It ties the callbacks to the pool which the sasl context lives in. That seems to have fixed it. Thanks! :)
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, Sep 08, 2011 at 01:08:33AM -, danie...@apache.org wrote: Author: danielsh Date: Thu Sep 8 01:08:33 2011 New Revision: 1166489 URL: http://svn.apache.org/viewvc?rev=1166489view=rev Log: On the fs-successor-ids branch, actually implement sharding. Found by: stsp * subversion/libsvn_fs_fs/fs_fs.c (FSFS_SUCCESSORS_REVISIONS_PER_SHARD): New helper. (path_successor_ids_shard, path_successor_ids, path_successor_node_revs_shard, path_successor_node_revs): Fix path calculations. (update_successor_ids_file): Fix checks for 'New shard' and 'New file in a shard'. Just to make sure we both have the same idea: Each file in the successor store is responsible for a fixed number of revisions (currently 1000). max-files-per-dir tells us how many files can be in a single directory. If more than max-files-per-dir files exist in a given directory we open a new directory and store files there instead. So I would expect sharding within the successors tree to behave like this: filename: file stores successor data created in: db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r100..r199 db/successors/ids/1/0 r200..r2000999 ...... Data for the first million revs goes into the first shard, data for the second million revs goes into the second shard, etc. Is this what you've implemented? I probably would have used FSFS_SUCCESSORS_FILES_PER_SHARD instead of FSFS_SUCCESSORS_REVISIONS_PER_SHARD, and then computed the filename based on that number. I don't like thinking of it in terms of revisions per shard because the numbers get so big :)
RE: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
-Original Message- From: s...@apache.org [mailto:s...@apache.org] Sent: donderdag 8 september 2011 10:05 To: comm...@subversion.apache.org Subject: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Author: stsp Date: Thu Sep 8 08:05:00 2011 New Revision: 1166555 URL: http://svn.apache.org/viewvc?rev=1166555view=rev Log: Fix a possible crash in ra_svn if SASL authentication is active. * subversion/libsvn_ra_svn/cyrus_auth.c (svn_ra_svn__do_cyrus_auth): Give the auth callbacks sufficient lifetime to survive until connection pool cleanup. CyrusSASL needs the callbacks in the cleanup handler of this pool. Found by: Dave Huang k...@azeotrope.org Modified: subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Modified: subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/c yrus_auth.c?rev=1166555r1=1166554r2=1166555view=diff == --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original) +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Thu Sep 8 08:05:00 2011 @@ -738,9 +738,7 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se const char *mechstring = , *last_err = , *realmstring; const char *local_addrport = NULL, *remote_addrport = NULL; svn_boolean_t success; - /* Reserve space for 3 callbacks (for the username, password and the - array terminator). */ - sasl_callback_t callbacks[3]; + sasl_callback_t *callbacks; cred_baton_t cred_baton; int i; @@ -776,6 +774,10 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se cred_baton.realmstring = realmstring; cred_baton.pool = pool; + /* Reserve space for 3 callbacks (for the username, password and the + array terminator). */ + callbacks = apr_palloc(sess-conn-pool, sizeof(*callbacks) * 3); + /* Initialize the callbacks array. */ This isn't going to help when the baton that is passed (by pointer) to the callbacks is also allocated on the stack. (The baton should probably move to heap as well if this is the right fix) The function seems to assume that this callback infrastructure isn't used after returning from svn_ra_svn__do_cyrus_auth(), which would make allocating on the stack safe. Any idea why this worked for years in 1.6 but now starts failing? Bert
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, Sep 08, 2011 at 10:36:13AM +0200, Stefan Sperling wrote: So I would expect sharding within the successors tree to behave like this: filename: file stores successor data created in: db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r100..r199 db/successors/ids/1/0 r200..r2000999 ...... (Of course, this example assumes max-files-per-dir == 1000, which is the default. With a different value the one-thousand-revisions-files would distribute across directories in a different way.)
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, 2011-09-08 at 10:40 +0200, Stefan Sperling wrote: On Thu, Sep 08, 2011 at 10:36:13AM +0200, Stefan Sperling wrote: So I would expect sharding within the successors tree to behave like this: filename: file stores successor data created in: db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r100..r199 db/successors/ids/1/0 r200..r2000999 ...... Oops - something's wrong with that pattern. Did you mean .../ids/(rev % 100)/(rev % 1000) which would give db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r999000..r99 db/successors/ids/1/0 r100..r1000999 ...... ? - Julian (Of course, this example assumes max-files-per-dir == 1000, which is the default. With a different value the one-thousand-revisions-files would distribute across directories in a different way.)
Re: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
Bert Huijben b...@qqmail.nl writes: --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original) +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Thu Sep 8 08:05:00 2011 @@ -738,9 +738,7 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se const char *mechstring = , *last_err = , *realmstring; const char *local_addrport = NULL, *remote_addrport = NULL; svn_boolean_t success; - /* Reserve space for 3 callbacks (for the username, password and the - array terminator). */ - sasl_callback_t callbacks[3]; + sasl_callback_t *callbacks; cred_baton_t cred_baton; int i; @@ -776,6 +774,10 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se cred_baton.realmstring = realmstring; cred_baton.pool = pool; + /* Reserve space for 3 callbacks (for the username, password and the + array terminator). */ + callbacks = apr_palloc(sess-conn-pool, sizeof(*callbacks) * 3); + /* Initialize the callbacks array. */ This isn't going to help when the baton that is passed (by pointer) to the callbacks is also allocated on the stack. (The baton should probably move to heap as well if this is the right fix) In practice it doesn't matter, see below. We should probably change it or add a comment. The function seems to assume that this callback infrastructure isn't used after returning from svn_ra_svn__do_cyrus_auth(), which would make allocating on the stack safe. Any idea why this worked for years in 1.6 but now starts failing? The original bug report explained it. During pool cleanup we call the SASL function sasl_dispose and that looks at the callback structs. The stack struct has random values and if the id member makes it look like the logging callback it gets invoked. Most callbacks won't get invoked at that stage so only particular values will cause a problem. Also since the auth callback won't get invoked it explains why the baton on the stack works. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
Julian Foad wrote on Thu, Sep 08, 2011 at 10:56:43 +0100: On Thu, 2011-09-08 at 10:40 +0200, Stefan Sperling wrote: On Thu, Sep 08, 2011 at 10:36:13AM +0200, Stefan Sperling wrote: So I would expect sharding within the successors tree to behave like this: filename: file stores successor data created in: db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r100..r199 db/successors/ids/1/0 r200..r2000999 ...... Oops - something's wrong with that pattern. Did you mean .../ids/(rev % 100)/(rev % 1000) which would give db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r999000..r99 db/successors/ids/1/0 r100..r1000999 ...... That's what the code is doing. I'm testing it with FSFS_SUCCESSORS_MAX_REVS_PER_FILE=3 and SVN_FS_FS_MAX_FILES_PER_DIR=4. I'd like to encourage people to use different values for these constants than I use --- my experience shows that some bugs only surface with some values of the constants. ? - Julian (Of course, this example assumes max-files-per-dir == 1000, which is the default. With a different value the one-thousand-revisions-files would distribute across directories in a different way.)
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
Stefan Sperling wrote on Thu, Sep 08, 2011 at 10:36:13 +0200: I probably would have used FSFS_SUCCESSORS_FILES_PER_SHARD instead of FSFS_SUCCESSORS_REVISIONS_PER_SHARD, and then computed the filename based on that number. I don't like thinking of it in terms of revisions per shard because the numbers get so big :) First of all, the big numbers are 12(decimal) or 35(decimal), not more. Second of all, I don't even think about the big numbers _at all_. When I see (revision % FSFS_SUCCESSORS_REVISIONS_PER_SHARD) / ffd-max_files_per_dir, I parse the parenthesized expression as the index of 'revision' within its shard; but I don't work out its value.
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, Sep 08, 2011 at 10:56:43AM +0100, Julian Foad wrote: Oops - something's wrong with that pattern. Did you mean .../ids/(rev % 100)/(rev % 1000) which would give db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r999000..r99 db/successors/ids/1/0 r100..r1000999 ...... ? Yes, of course. Thanks :)
Re: svn commit: r1166489 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, Sep 08, 2011 at 01:31:51PM +0300, Daniel Shahaf wrote: Julian Foad wrote on Thu, Sep 08, 2011 at 10:56:43 +0100: db/successors/ids/0/0 r0..r999 db/successors/ids/0/1 r1000..r1999 ...... db/successors/ids/0/999r999000..r99 db/successors/ids/1/0 r100..r1000999 ...... That's what the code is doing. Great! I'm happy. Thanks for fixing this up.
Re: svn commit: r1166496 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
On Thu, Sep 08, 2011 at 01:39:24AM -, danie...@apache.org wrote:
Author: danielsh
Date: Thu Sep 8 01:39:23 2011
New Revision: 1166496
URL: http://svn.apache.org/viewvc?rev=1166496view=rev
Log:
On the fs-successor-ids branch, factor out repeated logic.
This misbehaves a little when I test it, but eyeballing the diff convinces
me that those the behaviour of the code must be identical before/after this
patch... so I'm committing it anyway :).
How does it misbehave?
==
--- subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
(original)
+++ subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c Thu
Sep 8 01:39:23 2011
@@ -494,6 +494,32 @@ path_node_origin(svn_fs_t *fs, const cha
node_id_minus_last_char, NULL);
}
+static svn_error_t *
+make_shard_dir(const char *(*path_some_shard)(svn_fs_t *,
+ svn_revnum_t,
+ apr_pool_t *),
This is awkward.
Why not pass the shard path as an argument to this function?
+ svn_fs_t *fs,
+ svn_revnum_t revision,
+ apr_pool_t *pool)
+{
+ /* We don't care if this fails because the shard already existed
+ * for some reason. */
+ svn_error_t *err;
+ const char *new_dir;
+
+ new_dir = path_some_shard(fs, revision, pool);
+ err = svn_io_dir_make(new_dir, APR_OS_DEFAULT, pool);
+ if (err !APR_STATUS_IS_EEXIST(err-apr_err))
+SVN_ERR(err);
+ svn_error_clear(err);
+ SVN_ERR(svn_fs_fs__dup_perms(new_dir,
+ svn_dirent_dirname(new_dir, pool),
+ pool));
+
+ return SVN_NO_ERROR;
+}
Re: svn commit: r1166496 - /subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
Stefan Sperling wrote on Thu, Sep 08, 2011 at 14:21:34 +0200:
On Thu, Sep 08, 2011 at 01:39:24AM -, danie...@apache.org wrote:
Author: danielsh
Date: Thu Sep 8 01:39:23 2011
New Revision: 1166496
URL: http://svn.apache.org/viewvc?rev=1166496view=rev
Log:
On the fs-successor-ids branch, factor out repeated logic.
This misbehaves a little when I test it, but eyeballing the diff convinces
me that those the behaviour of the code must be identical before/after this
patch... so I'm committing it anyway :).
How does it misbehave?
I chmod'd revs/ shards and files and successors/ shards and files, and
committed revisions ('svn mkdir file://$PWD/r1/$RANDOM -mm' in a loop)
until new shards were opened, and then I examined the permissions on the
new shards. The permissions weren't always preserved, and I think in at
least one case the 0100 bit was removed from a directory.
==
--- subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
(original)
+++ subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs/fs_fs.c
Thu Sep 8 01:39:23 2011
@@ -494,6 +494,32 @@ path_node_origin(svn_fs_t *fs, const cha
node_id_minus_last_char, NULL);
}
+static svn_error_t *
+make_shard_dir(const char *(*path_some_shard)(svn_fs_t *,
+ svn_revnum_t,
+ apr_pool_t *),
This is awkward.
Why not pass the shard path as an argument to this function?
What would making this change gain?
+ svn_fs_t *fs,
+ svn_revnum_t revision,
+ apr_pool_t *pool)
+{
+ /* We don't care if this fails because the shard already existed
+ * for some reason. */
+ svn_error_t *err;
+ const char *new_dir;
+
+ new_dir = path_some_shard(fs, revision, pool);
+ err = svn_io_dir_make(new_dir, APR_OS_DEFAULT, pool);
+ if (err !APR_STATUS_IS_EEXIST(err-apr_err))
+SVN_ERR(err);
+ svn_error_clear(err);
+ SVN_ERR(svn_fs_fs__dup_perms(new_dir,
+ svn_dirent_dirname(new_dir, pool),
+ pool));
+
+ return SVN_NO_ERROR;
+}
Re: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
Semi-related question: how does this fix interact with this part of svnserve's main(): /* Use a subpool for the connection to ensure that if SASL is used * the pool cleanup handlers that call sasl_dispose() (connection_pool) * and sasl_done() (pool) are run in the right order. See issue #3664. */ connection_pool = svn_pool_create(pool); conn = svn_ra_svn_create_conn2(NULL, in_file, out_file, params.compression_level, connection_pool); svn_error_clear(serve(conn, params, connection_pool)); exit(0); ? Both are SASL pool lifetime issues. Is the above hunk still needed after the change below? Philip Martin wrote on Thu, Sep 08, 2011 at 11:20:00 +0100: Bert Huijben b...@qqmail.nl writes: --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original) +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Thu Sep 8 08:05:00 2011 @@ -738,9 +738,7 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se const char *mechstring = , *last_err = , *realmstring; const char *local_addrport = NULL, *remote_addrport = NULL; svn_boolean_t success; - /* Reserve space for 3 callbacks (for the username, password and the - array terminator). */ - sasl_callback_t callbacks[3]; + sasl_callback_t *callbacks; cred_baton_t cred_baton; int i; @@ -776,6 +774,10 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se cred_baton.realmstring = realmstring; cred_baton.pool = pool; + /* Reserve space for 3 callbacks (for the username, password and the + array terminator). */ + callbacks = apr_palloc(sess-conn-pool, sizeof(*callbacks) * 3); + /* Initialize the callbacks array. */ This isn't going to help when the baton that is passed (by pointer) to the callbacks is also allocated on the stack. (The baton should probably move to heap as well if this is the right fix) In practice it doesn't matter, see below. We should probably change it or add a comment. The function seems to assume that this callback infrastructure isn't used after returning from svn_ra_svn__do_cyrus_auth(), which would make allocating on the stack safe. Any idea why this worked for years in 1.6 but now starts failing? The original bug report explained it. During pool cleanup we call the SASL function sasl_dispose and that looks at the callback structs. The stack struct has random values and if the id member makes it look like the logging callback it gets invoked. Most callbacks won't get invoked at that stage so only particular values will cause a problem. Also since the auth callback won't get invoked it explains why the baton on the stack works. -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com
Re: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
On Thu, Sep 08, 2011 at 05:20:20PM +0300, Daniel Shahaf wrote: Semi-related question: how does this fix interact with this part of svnserve's main(): /* Use a subpool for the connection to ensure that if SASL is used * the pool cleanup handlers that call sasl_dispose() (connection_pool) * and sasl_done() (pool) are run in the right order. See issue #3664. */ connection_pool = svn_pool_create(pool); conn = svn_ra_svn_create_conn2(NULL, in_file, out_file, params.compression_level, connection_pool); svn_error_clear(serve(conn, params, connection_pool)); exit(0); ? Both are SASL pool lifetime issues. Is the above hunk still needed after the change below? Quite likely still needed. The above was a crash that happened at exit(0) time, and only when svnserve was run in inetd mode. I don't know how the reporters of the new bug are running svnserve, but I would guess that it's a different bug. In any case, even if this was now redundant, there is no harm in keeping this as it is.
Re: svn commit: r1166555 - /subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c
On Thu, Sep 08, 2011 at 04:37:00PM +0200, Stefan Sperling wrote: On Thu, Sep 08, 2011 at 05:20:20PM +0300, Daniel Shahaf wrote: Semi-related question: how does this fix interact with this part of svnserve's main(): /* Use a subpool for the connection to ensure that if SASL is used * the pool cleanup handlers that call sasl_dispose() (connection_pool) * and sasl_done() (pool) are run in the right order. See issue #3664. */ connection_pool = svn_pool_create(pool); conn = svn_ra_svn_create_conn2(NULL, in_file, out_file, params.compression_level, connection_pool); svn_error_clear(serve(conn, params, connection_pool)); exit(0); ? Both are SASL pool lifetime issues. Is the above hunk still needed after the change below? Quite likely still needed. Also, note that the above fix was server-side, while the new fix is on the client side :)
Diff shows added svn:mergeinfo prop as lots of new merges
If Subversion creates subtree mergeinfo on a path that previously didn't have any, then svn diff incorrectly shows all the source revs in that mergeinfo as newly merged. In a Subversion trunk WC, using 1.7.0-rc2: $ svn merge -c 100 ^/subversion/branches/1.6.x/INSTALL INSTALL --- Merging r100 into 'INSTALL': UINSTALL --- Recording mergeinfo for merge of r100 into 'INSTALL': G INSTALL [Note that r100 is a no-op on trunk, so in fact no content change was made despite the 'U' marker.] $ svn diff INSTALL Index: INSTALL === --- INSTALL (revision 1166027) +++ INSTALL (working copy) Property changes on: INSTALL ___ Added: svn:mergeinfo Merged /subversion/branches/atomic-revprop/INSTALL:r965046-1000689 Merged /subversion/branches/diff-callbacks3/INSTALL:r870059-870761 Merged /subversion/branches/bdb-reverse-deltas/INSTALL:r872050-872529 Merged /subversion/branches/double-delete/INSTALL:r870511-872970 [...] This is wrong. The property certainly was added, but that does not mean all those revisions were merged. The expected output is something like: [...] Added: svn:mergeinfo Merged /subversion/branches/1.6.x/INSTALL:r100 The bug is that svn diff shows a mergeinfo diff assuming that the previous merginfo was an explicit empty set of mergeinfo, but it wasn't, it was inherited mergeinfo. - Julian
Re: Diff shows added svn:mergeinfo prop as lots of new merges
Can I file an issue for this? Philip said the server makes (or used to make?) the same mistake internally when processing mergeinfo - presumably in the guts of the ra_get_mergeinfo2 API? I assume the deletion (elision) of a mergeinfo prop would generate an all-revs-unmerged diff output, but haven't tested that. - Julian On Thu, 2011-09-08 at 16:59 +0100, Julian Foad wrote: If Subversion creates subtree mergeinfo on a path that previously didn't have any, then svn diff incorrectly shows all the source revs in that mergeinfo as newly merged. In a Subversion trunk WC, using 1.7.0-rc2: $ svn merge -c 100 ^/subversion/branches/1.6.x/INSTALL INSTALL --- Merging r100 into 'INSTALL': UINSTALL --- Recording mergeinfo for merge of r100 into 'INSTALL': G INSTALL [Note that r100 is a no-op on trunk, so in fact no content change was made despite the 'U' marker.] $ svn diff INSTALL Index: INSTALL === --- INSTALL (revision 1166027) +++ INSTALL (working copy) Property changes on: INSTALL ___ Added: svn:mergeinfo Merged /subversion/branches/atomic-revprop/INSTALL:r965046-1000689 Merged /subversion/branches/diff-callbacks3/INSTALL:r870059-870761 Merged /subversion/branches/bdb-reverse-deltas/INSTALL:r872050-872529 Merged /subversion/branches/double-delete/INSTALL:r870511-872970 [...] This is wrong. The property certainly was added, but that does not mean all those revisions were merged. The expected output is something like: [...] Added: svn:mergeinfo Merged /subversion/branches/1.6.x/INSTALL:r100 The bug is that svn diff shows a mergeinfo diff assuming that the previous merginfo was an explicit empty set of mergeinfo, but it wasn't, it was inherited mergeinfo. - Julian
Re: Diff shows added svn:mergeinfo prop as lots of new merges
On Thu, Sep 8, 2011 at 12:08 PM, Julian Foad julian.f...@wandisco.com wrote: Can I file an issue for this? Hi Julian, What problem(s) is the current behavior causing? I understand your point, but I hesitate to add merge tracking awareness to diff unless there is some benefit. Philip said the server makes (or used to make?) the same mistake internally when processing mergeinfo - presumably in the guts of the ra_get_mergeinfo2 API? I assume the deletion (elision) Minor semantic nitpick, elision isn't synonymous with mergeinfo deletion. A svn:mergeinfo property might be deleted outside of merge tracking (e.g. svn pd). Which makes me wonder: The current behavior is arguably wrong *if* the mergeinfo change in question was made by a merge tracking aware merge. But what if we simply delete a svn:mergeinfo property with 'svn pd'? Shouldn't the diff show that all the mergeinfo was removed in that case? Of course there is currently no fool-proof way to differentiate between a real mergetracking merge and manual edits of mergeinfo. Or do we just assume that all mergeinfo changes originate in merge tracking aware merges? of a mergeinfo prop would generate an all-revs-unmerged diff output, but haven't tested that. It does. Paul - Julian On Thu, 2011-09-08 at 16:59 +0100, Julian Foad wrote: If Subversion creates subtree mergeinfo on a path that previously didn't have any, then svn diff incorrectly shows all the source revs in that mergeinfo as newly merged. In a Subversion trunk WC, using 1.7.0-rc2: $ svn merge -c 100 ^/subversion/branches/1.6.x/INSTALL INSTALL --- Merging r100 into 'INSTALL': U INSTALL --- Recording mergeinfo for merge of r100 into 'INSTALL': G INSTALL [Note that r100 is a no-op on trunk, so in fact no content change was made despite the 'U' marker.] $ svn diff INSTALL Index: INSTALL === --- INSTALL (revision 1166027) +++ INSTALL (working copy) Property changes on: INSTALL ___ Added: svn:mergeinfo Merged /subversion/branches/atomic-revprop/INSTALL:r965046-1000689 Merged /subversion/branches/diff-callbacks3/INSTALL:r870059-870761 Merged /subversion/branches/bdb-reverse-deltas/INSTALL:r872050-872529 Merged /subversion/branches/double-delete/INSTALL:r870511-872970 [...] This is wrong. The property certainly was added, but that does not mean all those revisions were merged. The expected output is something like: [...] Added: svn:mergeinfo Merged /subversion/branches/1.6.x/INSTALL:r100 The bug is that svn diff shows a mergeinfo diff assuming that the previous merginfo was an explicit empty set of mergeinfo, but it wasn't, it was inherited mergeinfo. - Julian
[RFC] - Proper encoding for patch file?
This is a JavaHL issue. See the attached patch which resolves the problem I face. If I use the JavaHL diff API to produce a patch it fails if there are paths in the patch with UTF8 characters in the name. Here is an example of the Exception: Invalid argument svn: Can't convert string from 'UTF-8' to native encoding: svn: Index: ?\230?\181?\139?\232?\175?\149?\230?\150?\135?\228?\187?\182.txt === RA layer request failed svn: Error reading spooled REPORT request response The problem seems to be that JavaHL creates the output file for the patch with the encoding of SVN_APR_LOCALE_CHARSET. If I change this to utf-8 as shown in the patch then the method works. The command line client from the same system works fine. How do people feel about this? Does it make sense that JavaHL should create the patch file with UTF-8 encoding? I tend to think it does, but thought I would raise the question here. -- Thanks Mark Phippard http://markphip.blogspot.com/ Index: subversion/bindings/javahl/native/SVNClient.cpp === --- subversion/bindings/javahl/native/SVNClient.cpp (revision 1166827) +++ subversion/bindings/javahl/native/SVNClient.cpp (working copy) @@ -987,7 +987,7 @@ showCopiesAsAdds, force, FALSE, - SVN_APR_LOCALE_CHARSET, + utf-8, outfile, NULL /* error file */, changelists.array(subPool), @@ -1019,7 +1019,7 @@ showCopiesAsAdds, force, FALSE, - SVN_APR_LOCALE_CHARSET, + utf-8, outfile, NULL /* error file */, changelists.array(subPool),
Re: [RFC] - Proper encoding for patch file?
I should point out this is on OSX. The results on Windows are more interesting: 1. Unlike OSX, on Windows the API completes without error. 2. However, the paths in the index are show ??? in place of UTF-8 3. But the content within the patch, shows up fine. So this seems like another data point in favor of just telling SVN to output as UTF-8 since it seems to only apply to the pathnames. Comments? On Thu, Sep 8, 2011 at 2:07 PM, Mark Phippard markp...@gmail.com wrote: This is a JavaHL issue. See the attached patch which resolves the problem I face. If I use the JavaHL diff API to produce a patch it fails if there are paths in the patch with UTF8 characters in the name. Here is an example of the Exception: Invalid argument svn: Can't convert string from 'UTF-8' to native encoding: svn: Index: ?\230?\181?\139?\232?\175?\149?\230?\150?\135?\228?\187?\182.txt === RA layer request failed svn: Error reading spooled REPORT request response The problem seems to be that JavaHL creates the output file for the patch with the encoding of SVN_APR_LOCALE_CHARSET. If I change this to utf-8 as shown in the patch then the method works. The command line client from the same system works fine. How do people feel about this? Does it make sense that JavaHL should create the patch file with UTF-8 encoding? I tend to think it does, but thought I would raise the question here. -- Thanks Mark Phippard http://markphip.blogspot.com/ -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: [RFC] - Proper encoding for patch file?
On 09/08/2011 02:07 PM, Mark Phippard wrote: This is a JavaHL issue. See the attached patch which resolves the problem I face. If I use the JavaHL diff API to produce a patch it fails if there are paths in the patch with UTF8 characters in the name. Here is an example of the Exception: Invalid argument svn: Can't convert string from 'UTF-8' to native encoding: svn: Index: ?\230?\181?\139?\232?\175?\149?\230?\150?\135?\228?\187?\182.txt === RA layer request failed svn: Error reading spooled REPORT request response The problem seems to be that JavaHL creates the output file for the patch with the encoding of SVN_APR_LOCALE_CHARSET. If I change this to utf-8 as shown in the patch then the method works. The command line client from the same system works fine. How do people feel about this? Does it make sense that JavaHL should create the patch file with UTF-8 encoding? I tend to think it does, but thought I would raise the question here. Why does the command-line client work? Does it not also use the locale encoding for its diff headers? At any rate, consistency between the behaviors of the relevant Java and C APIs seems like a reasonable goal. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand signature.asc Description: OpenPGP digital signature
Re: [RFC] - Proper encoding for patch file?
On Thu, Sep 8, 2011 at 2:27 PM, C. Michael Pilato cmpil...@collab.net wrote: Why does the command-line client work? Does it not also use the locale encoding for its diff headers? At any rate, consistency between the behaviors of the relevant Java and C APIs seems like a reasonable goal. I have not tested exhaustively, but my OSX Terminal says UTF-8 is the default encoding. Maybe that is why I do not see it from command line? -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: [RFC] - Proper encoding for patch file?
On Thu, Sep 8, 2011 at 2:30 PM, Mark Phippard markp...@gmail.com wrote: On Thu, Sep 8, 2011 at 2:27 PM, C. Michael Pilato cmpil...@collab.net wrote: Why does the command-line client work? Does it not also use the locale encoding for its diff headers? At any rate, consistency between the behaviors of the relevant Java and C APIs seems like a reasonable goal. I have not tested exhaustively, but my OSX Terminal says UTF-8 is the default encoding. Maybe that is why I do not see it from command line? Changed Terminal to use MacOS Roman as default encoding. Now I get this: $ svn diff subversion/svn/diff-cmd.c:373: (apr_err=22) subversion/libsvn_client/diff.c:1989: (apr_err=22) subversion/libsvn_client/diff.c:1667: (apr_err=22) subversion/libsvn_wc/diff_local.c:560: (apr_err=22) subversion/libsvn_wc/status.c:2364: (apr_err=22) subversion/libsvn_wc/status.c:1171: (apr_err=22) subversion/libsvn_wc/status.c:1157: (apr_err=22) subversion/libsvn_wc/diff_local.c:474: (apr_err=22) subversion/libsvn_wc/diff_local.c:474: (apr_err=22) subversion/libsvn_wc/diff_local.c:419: (apr_err=22) subversion/libsvn_client/diff.c:1098: (apr_err=22) subversion/libsvn_client/diff.c:1012: (apr_err=22) subversion/libsvn_subr/stream.c:248: (apr_err=22) subversion/libsvn_subr/utf.c:775: (apr_err=22) subversion/libsvn_subr/utf.c:580: (apr_err=22) svn: E22: Can't convert string from 'UTF-8' to native encoding: subversion/libsvn_subr/utf.c:578: (apr_err=22) svn: E22: Index: Design Documents/?\230?\181?\139?\232?\175?\149?\230?\150?\135?\228?\187?\182.txt -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: [RFC] - Proper encoding for patch file?
On Thu, Sep 8, 2011 at 2:30 PM, Mark Phippard markp...@gmail.com wrote: On Thu, Sep 8, 2011 at 2:27 PM, C. Michael Pilato cmpil...@collab.net wrote: Why does the command-line client work? Does it not also use the locale encoding for its diff headers? At any rate, consistency between the behaviors of the relevant Java and C APIs seems like a reasonable goal. I have not tested exhaustively, but my OSX Terminal says UTF-8 is the default encoding. Maybe that is why I do not see it from command line? FWIW, even if I explicitly set LANG=en_US.UTF-8 before launching Java, and even if I change all of the JVM properties to make UTF-8 the default encoding for files for the JVM, I still get this error. So JavaHL does not seem to pickup the environment in the same ways as the command line. -- Thanks Mark Phippard http://markphip.blogspot.com/
Re: [RFC] - Proper encoding for patch file?
On Thu, Sep 8, 2011 at 1:49 PM, Mark Phippard markp...@gmail.com wrote: On Thu, Sep 8, 2011 at 2:30 PM, Mark Phippard markp...@gmail.com wrote: On Thu, Sep 8, 2011 at 2:27 PM, C. Michael Pilato cmpil...@collab.net wrote: Why does the command-line client work? Does it not also use the locale encoding for its diff headers? At any rate, consistency between the behaviors of the relevant Java and C APIs seems like a reasonable goal. I have not tested exhaustively, but my OSX Terminal says UTF-8 is the default encoding. Maybe that is why I do not see it from command line? FWIW, even if I explicitly set LANG=en_US.UTF-8 before launching Java, and even if I change all of the JVM properties to make UTF-8 the default encoding for files for the JVM, I still get this error. So JavaHL does not seem to pickup the environment in the same ways as the command line. FWIW, JavaHL is just using SVN_APR_LOCALE_CHARSET, which is a magic number inside of APR. I've no idea what it actually does. -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: Diff shows added svn:mergeinfo prop as lots of new merges
On Thu, 2011-09-08 at 12:37 -0400, Paul Burba wrote: On Thu, Sep 8, 2011 at 12:08 PM, Julian Foad julian.f...@wandisco.com wrote: Can I file an issue for this? Hi Julian, What problem(s) is the current behavior causing? I understand your point, but I hesitate to add merge tracking awareness to diff unless there is some benefit. I'm currently looking at merging from a high-level POV, looking at what clues and information we give the users about what they're doing, that hopefully guide them in doing the Right Thing and don't mislead and distract them. That's where this comes in: I do a simple little merge and run svn diff to check what's happened in the WC and suddenly it says loads of stuff has been merged, not the simple little merge that I expected. Philip said the server makes (or used to make?) the same mistake internally when processing mergeinfo - presumably in the guts of the ra_get_mergeinfo2 API? I assume the deletion (elision) Minor semantic nitpick, elision isn't synonymous with mergeinfo deletion. A svn:mergeinfo property might be deleted outside of merge tracking (e.g. svn pd). Ack. Which makes me wonder: The current behavior is arguably wrong *if* the mergeinfo change in question was made by a merge tracking aware merge. But what if we simply delete a svn:mergeinfo property with 'svn pd'? Shouldn't the diff show that all the mergeinfo was removed in that case? Of course there is currently no fool-proof way to differentiate between a real mergetracking merge and manual edits of mergeinfo. Or do we just assume that all mergeinfo changes originate in merge tracking aware merges? We have to treat mergeinfo as describing merges. Even if it was a manual edit. There's no mileage in attempting to distinguish real from fake merges; rather, the definition of a merge in terms of tracking has to be what a mergeinfo diff says. If the user's intended text edits accompany that mergeinfo change, that's well and good, the correct text change will be associated with the logical merge that's recorded; if no text edits or the wrong ones accompany the logical merge as described by the mergeinfo, then the user has partially lost track of exactly when the merge happened and may have difficulties selecting the right revisions to cherry-pick etc. That's just tough on the user, we have no better way (short of dump+load) to do manual mergeinfo edits. So in diff we have a choice. Do we tell the user the physical difference of a particular mergeinfo property, or do we interpret and display what it means? It appears from the wording Merged that we are attempting to display the meaning. If so, we're doing it wrong in the cases where the property is added or removed. If, instead, we simply want to show a diff of the mergeinfo property itself rather than trying to interpret what it means, the current behaviour would not be surprising. (Nor would it be particularly useful; the raw change of mergeinfo in a particular prop is the sort of thing the user generally doesn't want to know about, beyond the fact that there is some change that they need to commit.) But then we should not say Merged but rather mergeinfo diff or something. I think the interpreted output is useful. I share your hesitation about add merge tracking awareness to diff but there definitely *is* a benefit in showing the user what's logically changed. - Julian of a mergeinfo prop would generate an all-revs-unmerged diff output, but haven't tested that. It does. Paul - Julian On Thu, 2011-09-08 at 16:59 +0100, Julian Foad wrote: If Subversion creates subtree mergeinfo on a path that previously didn't have any, then svn diff incorrectly shows all the source revs in that mergeinfo as newly merged. In a Subversion trunk WC, using 1.7.0-rc2: $ svn merge -c 100 ^/subversion/branches/1.6.x/INSTALL INSTALL --- Merging r100 into 'INSTALL': UINSTALL --- Recording mergeinfo for merge of r100 into 'INSTALL': G INSTALL [Note that r100 is a no-op on trunk, so in fact no content change was made despite the 'U' marker.] $ svn diff INSTALL Index: INSTALL === --- INSTALL (revision 1166027) +++ INSTALL (working copy) Property changes on: INSTALL ___ Added: svn:mergeinfo Merged /subversion/branches/atomic-revprop/INSTALL:r965046-1000689 Merged /subversion/branches/diff-callbacks3/INSTALL:r870059-870761 Merged /subversion/branches/bdb-reverse-deltas/INSTALL:r872050-872529 Merged /subversion/branches/double-delete/INSTALL:r870511-872970 [...] This is wrong. The property certainly was added, but that does not mean all those revisions were merged. The expected output is something like: [...] Added: svn:mergeinfo Merged /subversion/branches/1.6.x/INSTALL:r100
State of Editor v2
As an astute follower of commits@ will notice, I've started poking at Editor v2 (Ev2). This is a brief mail to hopefully keep interested parties aware of what I'm doing, and to solicit potential help. Background: For those who don't know, Ev2 is a replacement API for the ubiquitous delta editor throughout Subversion. Much like wc-ng, it's designed with the benefit of 10 years of hindsight, and the hope of both simplifying things and improving them moving forward. There are some docs in notes/ and some on the mailing list, but the canonical source of information is the header file: svn_editor.h. With the hopes of eventually enabling some of the features that depend upon it, I've started looking at implementing Ev2. *Unlike* wc-ng, I think that Ev2 can be implemented in a modular fashion: we don't have to rewrite all the editors in Subversion simultaneously. Part of that modularity is a pair of shims, one from Ev2 to the delta editor, and a reverse version. Protoversion of these can be found in libsvn_delta/compat.c. In r1166900 I instrumented a large portion of the Subversion code to optionally insert these shims when creating delta editors to aid in testing (this is disabled by default). Once the shims are written, we should be able to start looking at rewriting individual editors. I don't anticipate writing the shims to be difficult, just time consuming, due to the differing paradigms between Ev2 and the delta editor. I do expect these shims will introduce a bit of memory and runtime overhead, which may limit our desire to use them in their naïve form in a public release. The existing editors will serve as the primary tests, with additional test cases in libsvn_delta/editor-test.c That's about all there is right now, but I figured folks would be interested. I welcome any help, review, comments or questions as this work goes forward. Thanks, -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Re: [RFC] - Proper encoding for patch file?
On Thu, Sep 08, 2011 at 02:07:03PM -0400, Mark Phippard wrote:
This is a JavaHL issue. See the attached patch which resolves the
problem I face.
If I use the JavaHL diff API to produce a patch it fails if there are
paths in the patch with UTF8 characters in the name. Here is an
example of the Exception:
Invalid argument
svn: Can't convert string from 'UTF-8' to native encoding:
svn: Index: ?\230?\181?\139?\232?\175?\149?\230?\150?\135?\228?\187?\182.txt
===
This might be related to the following TODO comment in libsvn_client/patch.c.
In other words, this is a known limitation of the current implementation.
[[[
static svn_error_t *
grab_filename(const char **file_name, const char *line, apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
const char *utf8_path;
const char *canon_path;
/* Grab the filename and encode it in UTF-8. */
/* TODO: Allow specifying the patch file's encoding.
* For now, we assume its encoding is native. */
/* ### This can fail if the filename cannot be represented in the current
* ### locale's encoding. */
SVN_ERR(svn_utf_cstring_to_utf8(utf8_path,
line,
scratch_pool));
]]]
Re: Thoughts about issue #3625 (shelving)
Stefan Sperling wrote on Thu, Sep 08, 2011 at 00:36:05 +0200: On Wed, Sep 07, 2011 at 05:48:47PM -0400, Greg Stein wrote: On Wed, Sep 7, 2011 at 03:51, Stefan Sperling s...@elego.de wrote: A first-class 'shelving' feature wouldn't have to worry about conflicts. It would simply restore the working copy to the shelved state (either destroying unrelated local modifications, or raising an error in case of their presence). I think unshelving can create a full set of conflicts. As above, even a simple add/add conflict. If local mods exist, then we can simple disallow the unshelving. For now. With some additional work on conflict handling, we could do a full merge of the local mods and the shelved mods. Sure. But in the initial implementation we could just restore the former working copy state, including mixed-revisioness etc. Just rewind everything back to where it was and let a subsequent update sort out the conflicts. That would already be a big improvement over the diff/patch approach. If we do this, it would be nice if 'restore the former state' could be done offline --- e.g., by retaining the relevant pristines as long as the shelf exists. (Now, if the working copy is at state S and someone asks to return to a previously-shelved state, what do we do with S? Do we discard it, or do we first save it somewhere? And if we save it... do we save it as a new shelf?)
Re: Thoughts about issue #3625 (shelving)
On Thu, Sep 8, 2011 at 19:33, Daniel Shahaf d...@daniel.shahaf.name wrote: Stefan Sperling wrote on Thu, Sep 08, 2011 at 00:36:05 +0200: ... Sure. But in the initial implementation we could just restore the former working copy state, including mixed-revisioness etc. Just rewind everything back to where it was and let a subsequent update sort out the conflicts. That would already be a big improvement over the diff/patch approach. To do this, we'd also want to move op_depth==0 over to the SHELF_NODES. Probably a good idea to just copy all of it, rather than piecemeal like I suggested, as I suspect that we'd eventually find a reason to have BASE present. If we do this, it would be nice if 'restore the former state' could be done offline --- e.g., by retaining the relevant pristines as long as the shelf exists. Absolutely. There shouldn't be *any* reason to contact the server. All the pristines would be held via SHELF_NODES and SHELF_ACTUAL. (Now, if the working copy is at state S and someone asks to return to a previously-shelved state, what do we do with S? Do we discard it, or do we first save it somewhere? And if we save it... do we save it as a new shelf?) I *do* envision multiple shelves, and that makes it (imo) even more important to look at unshelve as merging changes onto the current state. Regarding current state S... I think unshelve should NOT be allowed if local mods exist. Thus, we never need to worry about S. The user can always explicitly shelve that, return to a no-mods state, and then unshelve state-R. And yes, we can have an --ignore-local-mods switch to unshelve even when local mods exist. Also consider: the shelves can then act as multiplexors for the working copy. You could have one shelf for trunk, one for branches/1.7.x, one for 1.6.x, one for branches/fs-successor-ids, and for some trunk changes that you set aside. I've had to use git lately, and our shelves could almost look like git's branches. Swap around among them based on what you're doing at the time. Cheers, -g
Re: Thoughts about issue #3625 (shelving)
On Thu, 2011-09-08 at 23:43 -0400, Greg Stein wrote: I've had to use git lately, and our shelves could almost look like git's branches. Swap around among them based on what you're doing at the time. I think this is closer to git's stash feature than git branches. In fact, I was thinking of jumping in and asking why this was being called something gratuitously different.
