Re: [Issue 1230] - dedicated 'svn export' editor

On Wed, Apr 16, 2003 at 01:55:08PM -0500, Ben Collins-Sussman wrote:
>...
> The things I folded in:
>
> * dir_baton doesn't need the "props" member
> * it doesn't look like you use hb->source or hb->dest
> * in apply_textdelta(), you should use svn_stream_empty()
> rather than passing hb->source to stream_from_aprfile
> * why handler_pool instead of just 'pool'
> * dir_baton doesn't need the parent_dir_baton. thus, it only holds
> the edit_baton. thus, just return the edit_baton as a dir_baton

Missed the second part here. dir_baton isn't needed -- you can simply return
the edit_baton as a dir_baton.

This also implies the file_baton simply refer directly to the edit_baton.

>...
> There are two things I didn't agree with or understand, though:
>
> * the error checkingin window_handler can be simplified. no need for
> the first "if"
>
> --> huh? we need that 'if' to discover whether it was a NULL
> --> window or whether we got a real error.

The logic around "err" isn't fully needed. The following works exactly the
same:

  err = hb->apply_handler (...);
  if (err != SVN_NO_ERROR)
    {
      apr_file_remove (...);
    }

  return err;

In the previous code, the "if" would only fire when err==NULL. But in that
case, the above "if" code won't fire. So it can reduce to the logic above.

As a style nit, I'd also make that "if (err)". We rarely (and shouldn't)
test against SVN_NO_ERROR.

> * why keep a full hash of the props when you're only interested in
> three of them.
>
> --> actually, we're about to use a whole bunch of the
> --> svn:wc:entry: props as keyword substitution values. I'd
> --> rather not bother to filter the proplist just yet. maybe
> --> later.

Ah. Gotcha. Maybe leave a note in there about this? Something like "we don't
really need to save all the properties, but blindly inserting them into the
hash (and retrieving them later) is simpler than individual checks."

Altho... now that I think about it: the code to pull out the change is about
the same complexity as a test in change_file_prop(). In other words, you'll
have code that looks like:

  if (name == "foo")
    fb.keywords->last_author = apr_memdup(...)
  if (name == "bar")
    fb.keywords->last_rev = apr_memdup(...)

vs the current organization:

  keywords.last_author = apr_hash_get(...)
  keywords.last_rev = apr_hash_get(...)

Both would work just fine...

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