Daniel Stenberg <daniel@haxx.se> writes:
> Thanks for your review and comments. I intend to address all comments and
> cleanup things next week.
Daniel, can you fix the compiler warning immediately please?
Style, code convention, etc problems can be lived with as long as
they're temporary. Compile warnings are more serious -- having them
in the checked-in build makes it more difficult for other developers
to find their own problems, because they can no longer assume that any
warning they see is the result of their own change.
Thanks,
-K
> > > +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
---------------------------------------------------------------------
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