Re: [PATCH] Trace update, and cancel update editors patch

On Thu, Nov 29, 2001 at 04:33:33PM -0800, Bill Tutt wrote:
> Log:
> UI apps need trace editors. Seemed like a shame to force folks to
> constantly rewrite this useful bit of code. So, we now take postpone
> the actual printing to stdout in the cmdline client to a callback
> function. (for the trace-update editor only atm, trace-commit to
> follow shortly) The diff below shows trace-update still in the
> cmdline client directory. The intent of the patch, and the MSVC++
> .dsp patch shows the intent is to move the trace-update and
> trace-commit editors into libsvn_client. Additoinally, it's a shame
> that all editors constantly need to recalculate the current editor
> state. Why can't every editor just have access to the appropriate
> data? Of course, it'd be even handier if you could cache the data up
> front, that way you wouldn't be constantly recalculating the
> identical data with different editors using common code. But that
> isn't happening now. :)

The log should be about the change. Commentary can go elsewhere :-)

> Additionally, UI apps need asynchronous access to SVN, and therefore
> need a cancelation editor.

This patch is awfully big. Break it down into the distinct parts. One patch
for the trace editor. One to intro the cancellation. etc.

>...
> When these patches are approved, I can commit the current state of
> the COM layer. That diff isn't included since I need to remove and
> readd all of the VB source files as bianry files until line-ending
> conversions are implemented.

Again, not relevant for a log msg.

>...
> * libsvn_client/libsvn_client.dsp: Add cancel-update.c. Add trace-update.c.
>...
> * clients/cmdline/subversion_client.dsp: Add mkdir-cmd.c Remove trace-update.c.

trace-update wasn't moved, so the file move in the .dsp shouldn't there.

>...
> * libsvn_client/trace-update.c: (edit_baton): Rename baton to trace_edit_baton.
> MSVC's debugger didn't like it when there was more than one identically
> named struct (bad user expericence issues..) Add trace_ui vtable memeber
> and a baton for the callback.

Please make this a separate patch. It is hard to review with a rename *and*
functional changes in the same patch. If you do a rename *only*, then you
can check in the rename with a message describing the rename and saying "no
functional change." Further patches can do the functional change, and
they'll be much easier to review.

>...

I'll review the bulk of the patch after it has been busted up into pieces.

One note: the cancel editor is horrifically repetitive. Write a single
function to take the cancel_baton and make the call to check for cancelling
and returning the appropriate error. Each of the editor functions would look
something like:

some_function(void *baton, ...)
{
  return check_cancel(baton);
}

And:

static svn_error_t *check_cancel(cancel_baton *cb)
{
  if (cb->should_i_cancel(cb->inner_baton))
    return ...
  return SVN_NO_ERROR;
}

The editor functions are really simple. Not even a need for a local since
the void* simply auto-casts to a cancel_baton as it is passed to
check_cancel.

>...
> +/* Used by the trace_update editor in libsvn_client
> + */
> +void
> +svn_cl__print_message(void *ui_baton,
> + svn_stringbuf_t *msg)
> +{
> + fputs(msg->data, stdout);
> +}

1) that should be const svn_string_t *, or just const char *. No need for a
   darned stringbuf.

2) use fwrite if you have a string(buf), since you already have the length.

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