On Thu, Jan 10, 2002 at 02:43:21AM -0800, Greg Stein wrote:
> +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 :-)
oops, yeah, i should have mentioned that ;-)
> > +++ ./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.
i think so, but if someone more familiar with APR would veryify that i
would appreciate it.
> >...
> > +++ ./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.
i see, will do.
> > + 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 ?
your forthcoming comments are correct.
> > +
[ ... skip bunch of code ... ]
> > +
> > + 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).
yeah, that's what i was trying to do, but i screwed it up.
> 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 ]
sounds good. i will make those changes and check them in.
thanks for the review,
-garrett
--
garrett rooney Unix was not designed to stop you from
rooneg@electricjellyfish.net doing stupid things, because that would
http://electricjellyfish.net/ stop you from doing clever things.
---------------------------------------------------------------------
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