On 25 Jan 2002 cmpilato@collab.net wrote:
Thanks for your review and comments. I intend to address all comments and
cleanup things next week.
> > +static void
> > +print_short_format (apr_file_t *file,
> > + const char *path,
> > + svn_wc_status_t *status)
> > +{
> This function, having obviously been copy-n-pasted from status.c, might
> deserve a new name that reveals the fact that it is printing out status
> lines. Since this copy of it doesn't live in *status.c*, it doesn't have
> the luxury of being assumed to be related to status-y things. Plus, it'll
> remove one more ambiguity when talking about symbols in Subversion, and
> make my TAGS table usage friendlier.
I honestly didn't work very much on the static function names. I'll keep it
in mind when I do my cleanups.
> Keep in mind that we use the function-space-paren coding style throughout
> this file. This should be:
Yes. I know I might have slipped that one a few times, I do find it hard to
write code with that space. I did walk through my code at least three times
just to pad function calls with spaces.
> > + command = (char *)malloc (editorlen + strlen(fullfile) + 2);
>
> Malloc? Not when we have pools lying all over the place!
Sorry, I'm not very comfortable with the overuse of pools but I figure the
correct thing to do here is getting back in line.
> > +#define DEFAULT_MSG \
> > +"\nSVN: ----------------------------------------------------------------------\n" \
> > +"SVN: Enter Log. Lines beginning with 'SVN:' are removed automatically\n" \
> > +"SVN: \n" \
> > +"SVN: Current status of the target files and directories:\n"\
> > +"SVN: \n"
>
> I think I'd personally rather this be a const char * (or would that be
> static const char * ?) string than a #DEFINE. But at least perhaps #undef
> the thing after using. I'm only -0 on this.
I'm not sure we want that hard-coded thing in there like this anyway. We
might introduce something that lets you create a template to get loaded into
the editor, and in that case we need to make the default text customizable.
I left this there since I don't think it'll remain so very long.
> > + /* we didn't manage to write the complete file, we can't fulfill
> > + what we're set out to do, get out */
> > + return SVN_ERR_BAD_URL; /* FIX! add a correct error code */
> > + }
>
> Aack. This causes a build warning. SVN_ERR_BAD_URL is but a numeric code.
> You need to use svn_error_create to build an svn_error_t * structure to
> return.
I know, but leaving it creating a compiler warning like this will (make
people) remind me about this and won't let me sleep well until I've cleaned
up the client error code situation. Then I'll be able to add good error codes
and messages to return.
> > + /* run the editor command line */
> > + system (command);
> > +
>
> Hm...is system() what we want here, or should (again, for consistency's
> sake) be using the APR process code? I think we have a nice wrapper --
> does svn_io_run_cmd allow us to halt the current process and wait for the
> subprocess to complete? Maybe I'm being too picky on this.
No, I think you're right. I just wasn't aware of that function. I'll convert
it.
> Here, out of the blue, we have another coding style inconsistency.
>
> if
> {
> }
> else {
> }
>
> ??
That's a plain mistake.
> I suppose I can live with the spirit of this patch. I've been pushing for
> the $EDITOR functionality, but I would prefer to simply pop up an empty
> editor, not one full of a bunch of files that I should know already.
Personally, I want the info in there even though I already knew it. I think
it makes writing the commit message easier. However, I wouldn't mind using
the verbose option or something to make that happen.
> - The code to open an editor populated with text and deal with the
> return from its usage should be modularized.
I'll make something up.
> - Every commit that falls back to this editor code is going to now
> have a `status' run on it on top of the regular commit stuff. That
> means we're running text_modified_p checks on every target twice
> now.
Yes, but to get the status output in the editor, I didn't know what else to
do.
--
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
Received on Sat Oct 21 14:36:59 2006