You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2007/07/09 16:48:29 UTC
Re: svn commit: r25692 - trunk/subversion/svn
sussman@tigris.org writes:
> --- trunk/subversion/svn/util.c (original)
> +++ trunk/subversion/svn/util.c Mon Jul 9 08:21:51 2007
> @@ -74,68 +74,53 @@
> }
>
> [...]
>
> +svn_error_t *
> +svn_cl__edit_file_externally(const char *path,
> + const char *editor_cmd,
> + apr_hash_t *config,
> + apr_pool_t *pool)
> +{
> + const char *editor, *cmd;
> + int sys_err;
> +
> + SVN_ERR(find_editor_binary(&editor, editor_cmd, config));
> +
> + cmd = apr_psprintf(pool, "%s %s", editor, path);
> + sys_err = system(cmd);
> + if (sys_err != 0)
> + /* Extracting any meaning from sys_err is platform specific, so just
> + use the raw value. */
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("system('%s') returned %d"), cmd, sys_err);
> +
> + return SVN_NO_ERROR;
> +}
Hmmm, don't we have an APR-y way of invoking external commands? I
realize the system() call was in the code before, I just wonder if
this isn't an opportunity to upgrade to something more portable. (I
guess the main advantage of an APR-y way would be that the error code
would be meaningful.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25692 - trunk/subversion/svn
Posted by David Glasser <gl...@mit.edu>.
On 7/9/07, C. Michael Pilato <cm...@collab.net> wrote:
> Garrett Rooney wrote:
> > IIRC there was a specific reason we used system(), but I can't recall
> > the details. Perhaps the archives would tell us...
>
> Yeah, I remember the same. Crawling the archives makes me think it had
> something to do with supporting editors with spaces in their command-lines
> (e.g., emacs -nw).
Hmm. This implies that we need to be careful about not passing files
with spaces or other shell metacharacters in their names as either the
"path" to svn_cl__edit_file_externally or as the "prefix" to
svn_cl__edit_string_internally.
A quick glance over the code implies that we're safe for now, though
(since the merge_file being edited with (e) has a boring temporary
filename).
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25692 - trunk/subversion/svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
Garrett Rooney wrote:
> IIRC there was a specific reason we used system(), but I can't recall
> the details. Perhaps the archives would tell us...
Yeah, I remember the same. Crawling the archives makes me think it had
something to do with supporting editors with spaces in their command-lines
(e.g., emacs -nw).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r25692 - trunk/subversion/svn
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Garrett Rooney" <ro...@electricjellyfish.net> writes:
> IIRC there was a specific reason we used system(), but I can't recall
> the details. Perhaps the archives would tell us...
Probably to make e.g. EDITOR='gvim -f' work. Do apr_proc_* send
the command through the shell? If not, they're no good for this.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25692 - trunk/subversion/svn
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 7/9/07, Ben Collins-Sussman <su...@red-bean.com> wrote:
> On 7/9/07, Karl Fogel <kf...@red-bean.com> wrote:
>
> > Hmmm, don't we have an APR-y way of invoking external commands? I
> > realize the system() call was in the code before, I just wonder if
> > this isn't an opportunity to upgrade to something more portable. (I
> > guess the main advantage of an APR-y way would be that the error code
> > would be meaningful.)
>
> Yeah, that thought crossed my mind as well... hmmm... I think we do
> something like when we run an external diff3 tool. Ah, yes, it looks
> like we have a whole svn_io_run_cmd() infrastructure, which ultimately
> calls apr_proc*() functions, manages process i/o, etc.
>
> Maybe 'svn propedit' and the conflict-callback should be using this
> code to launch $EDITOR? (Incidentally, I can't figure out where 'svn
> commit' is launching the editor!)
IIRC there was a specific reason we used system(), but I can't recall
the details. Perhaps the archives would tell us...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25692 - trunk/subversion/svn
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On 7/9/07, Karl Fogel <kf...@red-bean.com> wrote:
> Hmmm, don't we have an APR-y way of invoking external commands? I
> realize the system() call was in the code before, I just wonder if
> this isn't an opportunity to upgrade to something more portable. (I
> guess the main advantage of an APR-y way would be that the error code
> would be meaningful.)
Yeah, that thought crossed my mind as well... hmmm... I think we do
something like when we run an external diff3 tool. Ah, yes, it looks
like we have a whole svn_io_run_cmd() infrastructure, which ultimately
calls apr_proc*() functions, manages process i/o, etc.
Maybe 'svn propedit' and the conflict-callback should be using this
code to launch $EDITOR? (Incidentally, I can't figure out where 'svn
commit' is launching the editor!)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org