A very quick review of Daniel's $EDITOR commit.
> +/*
> + * Prints a single status line to a given file about the given entry.
> + */
> +static void
> +print_short_format (apr_file_t *file,
> + const char *path,
> + svn_wc_status_t *status)
> +{
> + char str_status[5];
> + char array[128];
> + apr_size_t size;
> +
> + if (! status)
> + return;
> +
> + /* Create local-mod status code block. */
> + if (status->text_status != svn_wc_status_unversioned)
> + {
> + /* skip locally present files that aren't versioned, they won't
> + serve any purpose in this output */
> +
> + svn_cl__generate_status_codes (str_status,
> + status->text_status,
> + status->prop_status,
> + status->locked,
> + status->copied);
> +
> + apr_snprintf(array, 128, "SVN: %s %s\n", str_status, path);
> +
> + size = strlen(array);
> +
> + apr_file_write(file, array, &size);
> + }
> +}
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.
> +/*
> + * This function gather a status output to be used within a commit message,
> + * possibly edited in your favourite $EDITOR.
> + */
> +static svn_error_t *
> +write_status_to_file(apr_pool_t *pool,
> + apr_file_t *file,
> + svn_cl__opt_state_t *opt_state,
> + apr_array_header_t *targets)
Keep in mind that we use the function-space-paren coding style
throughout this file. This should be:
static svn_error_t *
write_status_to_file (apr_pool_t *pool,
apr_file_t *file,
svn_cl__opt_state_t *opt_state,
apr_array_header_t *targets)
Your commit was full of instances of this type of little oops.
> + /* we need to know the name of the temporary file */
> + apr_file_name_get (&fullfile, tempfile);
> +
> + editorlen = strlen (editor);
> +
> + command = (char *)malloc (editorlen + strlen(fullfile) + 2);
Malloc? Not when we have pools lying all over the place!
> +#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.
> +
> + if(command)
> + {
> + apr_finfo_t finfo_before;
> + apr_finfo_t finfo_after;
> + apr_size_t size;
> +
> + size = strlen (DEFAULT_MSG);
> +
> + rc = apr_file_write (tempfile, DEFAULT_MSG, &size);
> +
> + write_status_to_file(pool, tempfile, opt_state, targets);
> +
> + apr_file_close (tempfile);
> +
> + if((APR_SUCCESS != rc) || (strlen (DEFAULT_MSG) != size))
> + {
> + /* 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.
> + /* Get information about the temporary file before the user has
> + been allowed to edit any message */
> + apr_stat (&finfo_before, fullfile,
> + APR_FINFO_MTIME|APR_FINFO_SIZE, pool);
> +
> + /* create the command line */
> + apr_snprintf (command, editorlen + strlen(fullfile) + 2,
> + "%s %s", editor, fullfile);
> + /* 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.
> + /* Check if there seems to be any changes in the file */
> + if((finfo_before.mtime == finfo_after.mtime) &&
> + (finfo_before.size == finfo_after.size))
> + {
> + /* The file doesn't seem to have been modified, no
> + need to load it and strip it and such */
> + }
> + else {
> + apr_file_t *read_file;
> +
> + /* we have a commit message in a temporary file, get it */
> + rc = apr_file_open (&read_file, fullfile,
> + APR_READ, APR_UREAD, pool);
> +
Here, out of the blue, we have another coding style inconsistency.
if
{
}
else {
}
??
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.
Other complaints (yeah, I'm crabby this morning):
- The code to open an editor populated with text and deal with the
return from its usage should be modularized. The 'propedit'
command is going to want to do the same stuff here, by opening a
tmp file with the current contents of the property one is editing
in editor. The person edits the property value, saves the file,
closes the editor, and *poof* a property has been edited.
- 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. One of several more adequate ways to do this is to work this
into a callback function from the commit process -- once it has
built it's apr_array_header_t array of things it actually plans to
commit, this function could use a nonrecursive status on each of
those things, or better yet, the commit process could simply report
not only that it plans to commit such and such file, but why
(passing an effective status code back). At this point, we are
primed for doing what some folks were asking for: if we parse the
log message and see that some of the SVN:-prefixed paths were
removed, we can remove those paths from the commit process's target
list.
Just thoughts. Not trying to be too harsh here.
---------------------------------------------------------------------
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