+1 with some comments...
On Wed, Jan 09, 2002 at 10:07:27PM -0500, Garrett Rooney wrote:
>...
> (incedentally, libsvn_wc/get_editor.c is currently hard coding diff
> instead of using SVN_CLIENT_DIFF, which is bad, and should be fixed
> with or without this patch.)
And that this patch *does* fix it. (wasn't obvious from your comment :-)
>...
> +++ ./subversion/libsvn_wc/get_editor.c Wed Jan 9 21:46:14 2002
>...
> - /* Set io style. */
> - apr_err =
> - apr_procattr_io_set (diffproc_attr, 0,
> - APR_CHILD_BLOCK, APR_CHILD_BLOCK);
This patch ends up using svn_io_run_cmd() which passes APR_FULL_BLOCK rather
than the magic 0 constant. I'm going to presume that is correct.
>...
> +++ ./subversion/libsvn_subr/io.c Wed Jan 9 21:50:00 2002
>...
> +/* Invoke SVN_CLIENT_DIFF, with USER_ARGS (which is an array of NUM_USER_ARGS
> + arguments), if they are specified, or "-u" if they are not.
> +
> + Diff runs in DIR, and it's exit status is stored in EXITCODE, if it is not
> + NULL.
> +
> + If LABEL is given, it will be passed in as the argument to the "-L" option.
> +
> + FROM is the first file passed to diff, and TO is the second. The stdout of
> + diff will be sent to OUTFILE, and the stderr to ERRFILE. All memory will be
> + allocated using POOL. */
> +svn_error_t *
> +svn_io_run_diff (const char *dir,
We don't replicate the comment in the source file. It is hard enough to
maintain the comments in the header, let alone in two places.
Please omit this when you check in the change.
> + const char *const *user_args,
> + const int num_user_args,
> + const char *label,
> + const char *from,
> + const char *to,
> + int *ecode,
> + apr_file_t *outfile,
> + apr_file_t *errfile,
> + apr_pool_t *pool)
> +{
> + const char **args;
> + int i;
> + int *exitcode;
> + int nargs = 4; /* the diff command itself, two paths, plus a trailing NULL */
> +
> + apr_pool_t *subpool = svn_pool_create (pool);
> +
> + if (ecode == NULL)
> + exitcode = NULL;
> + else
> + exitcode = ecode;
What is this for? Why don't you just pass "ecode" to svn_io_run_cmd ?
> +
> + if (user_args != NULL)
> + nargs += num_user_args;
> + else
> + nargs += 1; /* -u */
> +
> + if (label != NULL)
> + nargs += 2; /* the -L and the label itself */
> +
> + args = apr_palloc(subpool, nargs * sizeof(char *));
> +
> + i = 0;
> + args[i++] = SVN_CLIENT_DIFF;
> +
> + if (user_args != NULL)
> + {
> + int j;
> + for (j = 0; j < num_user_args; ++j)
> + args[i++] = user_args[j];
> + }
> + else
> + args[i++] = "-u"; /* assume -u if the user didn't give us any args */
> +
> + if (label != NULL)
> + {
> + args[i++] = "-L";
> + args[i++] = label;
> + }
> +
> + args[i++] = from;
> + args[i++] = to;
> + args[i++] = NULL;
> +
> + assert (i == nargs);
> +
> + SVN_ERR(svn_io_run_cmd (dir, SVN_CLIENT_DIFF, args, exitcode, NULL, NULL,
> + outfile, errfile, subpool));
> +
> + if (*exitcode < 0 || *exitcode > 2)
exitcode may be NULL, so you need to be careful here.
I think what you want to do is rename ecode to "pexitcode". Declare "int
exitcode" locally. If pexitcode == NULL, then pexitcode = &exitcode. IOW,
you'll always have a valid pexitcode. The caller may not want it, but you
definitely want it. Make sure that you check *pexitcode rather than exitcode
(since 'exitcode' might not be used).
I'd say go ahead and check this in, with the changes noted above. If others
have comments, then they can do a post-commit review :-)
[ in case you're wondering: yes, COMMITTERS says you have restricted access,
but that is only social convention; there are no software-enforced
restrictions. however, outside of the freebsd stuff, it is
review-then-commit; within the freebsd, it is commit-then-review ]
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:36:55 2006