On Tue, Jun 11, 2002 at 06:12:17PM -0500, Karl Fogel wrote:
> This is a preview of an upcoming commit.
Does it need to be do darned large? Your last big commit (2024) blew away
practically a whole day of Pilato's time to review it. The notification
stuff is *perfectly* decomposable. Change an API at a time.
Or maybe change the notification functions to support their new
functionality in one commit, then start using that in further commits.
I'm concerned. Your commit rate has been near zero the past few days, which
is kind of implying a power plant RSN. That scares me :-)
>...
> +++ ./subversion/libsvn_client/commit.c Tue Jun 11 16:35:09 2002
> @@ -90,9 +90,16 @@
> /* Import file PATH as EDIT_PATH in the repository directory indicated
> * by DIR_BATON in EDITOR.
> *
> - * Use POOL for any temporary allocation. */
> + * If NOTIFY_FUNC is non-null, invoke it with NOTIFY_BATON for each
> + * file. ### add mime-type (or at least binary) indicator to
> + * notify_func ###
Use the mime type. A client might want to be more descriptive.
>...
> /* Skip entries for this dir and its parent.
> - ### kff todo: APR actually promises that they'll come first,
> - so this guard could be moved outside the loop. */
> + (APR promises that they'll come first, so technically
> + this guard could be moved outside the loop. But somehow
> + that feels iffy. */
> if (! (strcmp (finfo.name, ".") && strcmp (finfo.name, "..")))
> continue;
Personally, I like:
if (finfo.name[0] == '.'
&& (finfo.name[1] == '\0'
|| (finfo.name[1] == '.' && finfo.name[2] == '\0')))
continue;
But that is a "micro optimization nit" :-)
>...
> +++ ./subversion/clients/cmdline/cl.h Tue Jun 11 18:22:25 2002
>...
> +/* Set *NOTIFY_FUNC_P and *NOTIFY_BATON_P to a notifier/baton for all
> + * operations, allocated in POOL.
> + *
> + * If this is a checkout, set IS_CHECKOUT.
Why? What are the semantics? How does the *behavior* change. A little more
description will help somebody decide whether they qualify as a "checkout".
(yah... this is all internal, but the flag seems a bit arbitrary without
more info on what is happening)
>...
> +++ ./subversion/clients/cmdline/feedback.c Tue Jun 11 17:12:18 2002
>...
> + case svn_wc_notify_commit_modified:
> + /* ### trace-commit.c has:
> + printf ("Sending %s\n", db->path);
> + */
> + printf ("Sending %s\n", path);
Isn't the extra space in there so that "(bin)" can be inserted sometimes?
>...
> +++ ./subversion/clients/cmdline/trace-commit.c Tue Jun 11 18:34:04 2002
> @@ -24,6 +24,7 @@
> /*** Includes. ***/
>
>
> +#if 0 /* now unused, left only for reference */
Just nuke the file. This *is* version control, after all. We have a copy.
[ if you leave it, I'll lay 10-to-1 odds that it will still get maintained.
people will jump to the change, and miss the #if 0 around the file. (well,
for people that *search* for changes rather than (ahem) rely on the
compiler to catch problems (and then miss, say, mod_dav_svn because they
aren't compiling it)) ]
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 Thu Jun 13 23:43:06 2002