You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2020/02/17 09:27:11 UTC
Re: svn commit: r1874093 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
I think this needs a specific implementation for Windows. The rules on
Windows are different and not really tied to some shell.
Applications receive the full commandline as string and then need to parse
them themselves. Usually they leave this to the C library so there are some
rules that are pretty common, but there are always exceptions.
For most programs it is best to start by escaping arguments that have some
special characters (such as whitespace)using quotes. Within quotes only
other quotes and backslashes next to quotes need further escaping
On Windows the current test fails with
[[
W: DIFF STDOUT (match_all=True):
W: | --- EXPECTED STDOUT (match_all=True)
W: | +++ ACTUAL STDOUT
W: | @@ -1,6 +1,9 @@
W: | Updating 'svn-test-work\working_copies\update_tests-38.backup\A\D\G\p; i':
W: | C svn-test-work\working_copies\update_tests-38.backup\A\D\G\p; i
W: | Updated to revision 3.
W: | +usage: svneditor.py file
W: | + svneditor.py base theirs mine merged wc_path
W: | +arguments passed were:
['D:\\ra\\svn-ra\\build\\subversion\\tests\\cmdline\\\\svneditor.py',
'p\\;\\', 'i']
W: | Merge conflicts in
'svn-test-work\working_copies\update_tests-38.backup\A\D\G\p; i'
marked as resolved.
W: | Summary of conflicts:
W: | Text conflicts: 0 remaining (and 1 already resolved)
W: CWD: E:\svn-ra\tests\subversion\tests\cmdline
]]
(See https://ci.apache.org/builders/svn-windows-ra/builds/2871/steps/Test%20fsfs%2Bsvn/logs/faillog
)
Using ';' on a path works on Windows, but is certainly not recommended. It
is the path separator used in environment variables like %PATH%.
Bert
On Mon, Feb 17, 2020 at 3:13 AM <ja...@apache.org> wrote:
> Author: jamessan
> Date: Mon Feb 17 02:13:34 2020
> New Revision: 1874093
>
> URL: http://svn.apache.org/viewvc?rev=1874093&view=rev
> Log:
> Followup to r1874057, escape whitespace instead of quoting filename
>
> As danielsh pointed out, only specific characters can be escaped by a
> backslash
> in quoted shell strings. Rather than surrounding the escaped path with
> double
> quotes, post-process the quoted path and manually escape any whitespace
> that is
> present.
>
> * subversion/libsvn_subr/cmdline.c
> (escape_path): New function, wrapper around apr_pescape_shell(), which
> handles escaping of whitespace.
> (svn_cmdline__edit_file_externally, svn_cmdline__edit_string_externally):
> Call the new function instead of apr_pescape_shell()
> * subversion/tests/cmdline/update_tests.py
> (update_accept_conflicts): Include ';' in renamed path ("p; i"), to
> ensure
> non-whitespace escaping is handled correctly.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/cmdline.c
> subversion/trunk/subversion/tests/cmdline/update_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1874093&r1=1874092&r2=1874093&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Mon Feb 17 02:13:34
> 2020
> @@ -1234,7 +1234,7 @@ svn_cmdline__be_interactive(svn_boolean_
> }
>
>
> -/* Helper for the next two functions. Set *EDITOR to some path to an
> +/* Helper for the edit_externally functions. Set *EDITOR to some path to
> an
> editor binary. Sources to search include: the EDITOR_CMD argument
> (if not NULL), $SVN_EDITOR, the runtime CONFIG variable (if CONFIG
> is not NULL), $VISUAL, $EDITOR. Return
> @@ -1300,6 +1300,42 @@ find_editor_binary(const char **editor,
> return SVN_NO_ERROR;
> }
>
> +/* Wrapper around apr_pescape_shell() which also escapes whitespace. */
> +static const char *
> +escape_path(apr_pool_t *pool, const char *orig_path)
> +{
> + apr_size_t len, esc_len;
> + const char *path;
> + char *p, *esc_path;
> +
> + path = apr_pescape_shell(pool, orig_path);
> +
> + len = esc_len = 0;
> +
> + /* Now that apr has done its escaping, we can check whether there's any
> + whitespace that also needs to be escaped. This must be done after
> the
> + fact, otherwise apr_pescape_shell() would escape the backslashes
> we're
> + inserting. */
> + for (p = (char *)path; *p; p++)
> + {
> + len++;
> + if (*p == ' ' || *p == '\t')
> + esc_len++;
> + }
> +
> + if (esc_len == 0)
> + return path;
> +
> + p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
> + while (*path)
> + {
> + if (*path == ' ' || *path == '\t')
> + *p++ = '\\';
> + *p++ = *path++;
> + }
> +
> + return esc_path;
> +}
>
> svn_error_t *
> svn_cmdline__edit_file_externally(const char *path,
> @@ -1333,8 +1369,7 @@ svn_cmdline__edit_file_externally(const
>
> /* editor is explicitly documented as being interpreted by the user's
> shell,
> and as such should already be quoted/escaped as needed. */
> - cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> - apr_pescape_shell(pool, file_name));
> + cmd = apr_psprintf(pool, "%s %s", editor, escape_path(pool, file_name));
> sys_err = system(cmd);
>
> apr_err = apr_filepath_set(old_cwd, pool);
> @@ -1496,8 +1531,7 @@ svn_cmdline__edit_string_externally(svn_
>
> /* editor is explicitly documented as being interpreted by the user's
> shell,
> and as such should already be quoted/escaped as needed. */
> - cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> - apr_pescape_shell(pool, tmpfile_native));
> + cmd = apr_psprintf(pool, "%s %s", editor, escape_path(pool,
> tmpfile_native));
>
> /* If the caller wants us to leave the file around, return the path
> of the file we'll use, and make a note not to destroy it. */
>
> Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/update_tests.py?rev=1874093&r1=1874092&r2=1874093&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Mon Feb 17
> 02:13:34 2020
> @@ -3649,12 +3649,12 @@ def update_accept_conflicts(sbox):
> alpha_path = sbox.ospath('A/B/E/alpha')
> beta_path = sbox.ospath('A/B/E/beta')
> pi_path = sbox.ospath('A/D/G/pi')
> - p_i_path = sbox.ospath('A/D/G/p i')
> + p_i_path = sbox.ospath('A/D/G/p; i')
> rho_path = sbox.ospath('A/D/G/rho')
>
> - # Rename pi to "p i" so we can exercise SVN_EDITOR's handling of paths
> with
> - # whitespace
> - sbox.simple_move('A/D/G/pi', 'A/D/G/p i')
> + # Rename pi to "p; i" so we can exercise SVN_EDITOR's handling of paths
> with
> + # special characters
> + sbox.simple_move('A/D/G/pi', 'A/D/G/p; i')
> sbox.simple_commit()
> sbox.simple_update()
>
> @@ -3676,7 +3676,7 @@ def update_accept_conflicts(sbox):
> mu_path_backup = os.path.join(wc_backup, 'A', 'mu')
> alpha_path_backup = os.path.join(wc_backup, 'A', 'B', 'E', 'alpha')
> beta_path_backup = os.path.join(wc_backup, 'A', 'B', 'E', 'beta')
> - p_i_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'p i')
> + p_i_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'p; i')
> rho_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'rho')
> svntest.main.file_append(iota_path_backup,
> 'My appended text for iota\n')
> @@ -3700,7 +3700,7 @@ def update_accept_conflicts(sbox):
> 'A/mu' : Item(verb='Sending'),
> 'A/B/E/alpha': Item(verb='Sending'),
> 'A/B/E/beta': Item(verb='Sending'),
> - 'A/D/G/p i' : Item(verb='Sending'),
> + 'A/D/G/p; i' : Item(verb='Sending'),
> 'A/D/G/rho' : Item(verb='Sending'),
> })
>
> @@ -3710,8 +3710,8 @@ def update_accept_conflicts(sbox):
> expected_status.tweak('A/mu', wc_rev=3)
> expected_status.tweak('A/B/E/alpha', wc_rev=3)
> expected_status.tweak('A/B/E/beta', wc_rev=3)
> - expected_status.rename({'A/D/G/pi': 'A/D/G/p i'})
> - expected_status.tweak('A/D/G/p i', wc_rev=3)
> + expected_status.rename({'A/D/G/pi': 'A/D/G/p; i'})
> + expected_status.tweak('A/D/G/p; i', wc_rev=3)
> expected_status.tweak('A/D/G/rho', wc_rev=3)
>
> # Commit.
> @@ -3806,15 +3806,15 @@ def update_accept_conflicts(sbox):
> 'My appended text for
> alpha\n'))
> expected_disk.tweak('A/B/E/beta', contents=("This is the file 'beta'.\n"
> 'Their appended text for
> beta\n'))
> - expected_disk.rename({'A/D/G/pi': 'A/D/G/p i'})
> - expected_disk.tweak('A/D/G/p i', contents=("This is the file 'pi'.\n"
> - '<<<<<<< .mine\n'
> - 'My appended text for pi\n'
> - '||||||| .r2\n'
> - '=======\n'
> - 'Their appended text for
> pi\n'
> - '>>>>>>> .r3\n'
> - 'foo\n'))
> + expected_disk.rename({'A/D/G/pi': 'A/D/G/p; i'})
> + expected_disk.tweak('A/D/G/p; i', contents=("This is the file 'pi'.\n"
> + '<<<<<<< .mine\n'
> + 'My appended text for pi\n'
> + '||||||| .r2\n'
> + '=======\n'
> + 'Their appended text for
> pi\n'
> + '>>>>>>> .r3\n'
> + 'foo\n'))
> expected_disk.tweak('A/D/G/rho', contents=("This is the file 'rho'.\n"
> '<<<<<<< .mine\n'
> 'My appended text for rho\n'
> @@ -3831,16 +3831,16 @@ def update_accept_conflicts(sbox):
>
> # Set the expected status for the test
> expected_status = svntest.actions.get_virginal_state(wc_backup, 3)
> - expected_status.rename({'A/D/G/pi': 'A/D/G/p i'})
> + expected_status.rename({'A/D/G/pi': 'A/D/G/p; i'})
> expected_status.tweak('iota', 'A/B/lambda', 'A/mu',
> 'A/B/E/alpha', 'A/B/E/beta',
> - 'A/D/G/p i', 'A/D/G/rho', wc_rev=3)
> + 'A/D/G/p; i', 'A/D/G/rho', wc_rev=3)
> expected_status.tweak('iota', status='C ')
> expected_status.tweak('A/B/lambda', status='C ')
> expected_status.tweak('A/mu', status='M ')
> expected_status.tweak('A/B/E/alpha', status='M ')
> expected_status.tweak('A/B/E/beta', status=' ')
> - expected_status.tweak('A/D/G/p i', status='M ')
> + expected_status.tweak('A/D/G/p; i', status='M ')
> expected_status.tweak('A/D/G/rho', status='C ')
>
> # Set the expected output for the test
>
>
>
Re: svn commit: r1874093 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by James McCoy <ja...@jamessan.com>.
On Mon, Feb 17, 2020 at 10:27:11AM +0100, Bert Huijben wrote:
> I think this needs a specific implementation for Windows. The rules on Windows
> are different and not really tied to some shell.
> Applications receive the full commandline as string and then need to parse them
> themselves. Usually they leave this to the C library so there are some rules
> that are pretty common, but there are always exceptions.
Indeed. I had hoped for more from the APR APIs.
I've previously referenced
https://docs.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
when I've had to recall the intricacies of escaping for Windows.
> Using ';' on a path works on Windows, but is certainly not recommended. It is
> the path separator used in environment variables like %PATH%.
It's not recommended on *nix either. :)
I've been pertty overwhelmed with work lately, which is why I haven't
followed up on this until now.
I'll try to get some time to work on it, but any other help would be
appreciated.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB