On Tue, Feb 05, 2002 at 10:25:53AM +0100, Daniel Stenberg wrote:
> On 4 Feb 2002, Philip Martin wrote:
>...
> > The above algorithm is not efficient (some lines get memmove'd more than
> > once),
>
> I agree, but I didn't consider this very important right now and it could
> easily be improved independently in the future without me having to go *all*
> the way in this single commit.
I completely agree with the sentiment of iterative improvement.
"Does this patch improve the status quo?" "Yes? check it in."
"Are there further improvements?" "Yes? Let's request an additional patch"
>...
> > I'd prefer to see an optional client side script provide the initial
> > message, with another to process it after editing. Then projects can define
> > their own format, and in addition, line ending is handled by the script
> > where it should be easy. Client side scripts could probably handle
> > multi-byte encoded log messages as well.
>
> I would like to see that too. That's also why I've worked on making the
> initial message configurable enough.
>
> However, I don't think that is part of what I intend to do here.
Agreed. In fact, I'll go further and state that I don't believe it has been
agreed upon that design. Personally, I'm not entirely sure that we want to
spawn client side scripts like this. That is an *entirely* separate design
decision from simply invoked EDITOR.
>...
> > > + /* setup the editor command line */
> > > + cmdargs[0] = editor;
> >
> > I don't think editor is one of the arguments.
>
> It isn't? The docstring for the funtion says:
>
> * ARGS is a list of (const char *)'s, terminated by NULL.
> * ARGS[0] is the name of the program, though it need not be the same
> * as CMD.
>
> In my mind that means that ARGS[0] is usually the same as the CMD.
Yes, the editor name needs to be part of the ARGS[] list.
>...
> > Use apr_palloc instead of malloc.
>
> This is just a silly work-around that is meant to be removed. I don't believe
> in fixing this, as the approach is totally wrong anyway. The correct fix here
> is to make sure the use of the svn_io_run_cmd() works as supposed.
>
> > > + sprintf(command, "%s %s", editor, fullfile);
> > > + system(command);
> >
> > No error checking? I'm not sure how portable system's error is...
>
> It doesn't really matter. Not only is it just a work-around, but even if the
> system() fails miserably, the effect is just gonna be that the temporary file
> will be unmodified and then the following code will notice and prompt the
> user accordingly.
malloc() is fine if it is extremely temporary. Of course, you could use
apr_palloc() for the same, temporary solution.
But I see that you have a patch/fix for svn_io_run_cmd(), which is Goodness
and will clear all this out anyways.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:04 2006