Re: [PATCH] Re: $EDITOR commit code cleanups

On 4 Feb 2002, Philip Martin wrote:

Thanks for your excellent feedback!

> > - apr_snprintf(array, 128, "SVN: %s %s\n", str_status, path);
> > + apr_snprintf (array, sizeof (array),
> > + "SVN: %s %s\n", str_status, path);
>
> No need to sprintf path. Restrict the array to the prefix, the status and
> the spaces, then initialise to a fixed string, pass array+7 to the function
> that writes the status and then apr_file_write_full the array. After thst
> apr_file_write_full the path. No truncation, whatever the length of the
> path.

That's indeed a good idea if we really want to be able to get that long lines
into the editor buffer, which I intended to prevent. But I believe that is
not up to me to decide, if people want that long paths let them have them.
I'll hack up a version like you suggest.

> The magic prefix "SVN:" should not occur explicitly in the code in three
> different places. Do something like:

Correctomente. I'll use a proper #define for it.

> Given that the terminator is also set a few lines
> above, why does it have to be set here as well?
>
> > + buffer->data [ buffer->len] = 0;

The "few lines above" case has a 'break' just after it, which will make just
one of these to happen for each line. I needed them both (or none of them,
as the zero termination could've been moved to only get done once in the
end).

> 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'd do something like (warning: untested code)

Point taken. I'll use a different approach that is somwhat similar to yours
in my upcoming 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.

> I think APR has the required support now.

APR had it by the time I mailed the patch too, but svn_io_run_cmd() did not.
I believe that situation hasn't changed. I may be mistaking of course. Or I
may use the function wrongly. Please point out my mistakes if you can find
any.

> > + /* 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.

> > + {
> > + /* ### FIXME:
> > + this is just a work-around as the above doesn't seem to work!
> > + Remove as soon as possible.
> > + */
> > + char *command=(char *)malloc(strlen(editor)+strlen(fullfile)+2);
>
> 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.

Yet another patch post follows.

-- 
      Daniel Stenberg - http://daniel.haxx.se - +46-705-44 31 77
   ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org