Daniel Trebbien wrote on Tue, Sep 21, 2010 at 08:12:38 -0700:
> On Sun, Sep 19, 2010 at 9:39 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > Need the word "to" after the semicolon for the English to be correct.
>
> Fixed. (When I say "fixed", I mean that I committed a fix in my own
> fork of Subversion: http://github.com/dtrebbien/subversion )
>
Fine, but please post patches and log messages to this list as usual.
It's easier for me/us to process them that way. Thanks.
> >> +svn_error_t *
> >> +svn_subst_translate_cstring2(const char *src,
> >> + const char **dst,
> >> + const char *eol_str,
> >> + svn_boolean_t repair,
> >> + apr_hash_t *keywords,
> >> + svn_boolean_t expand,
> >> + apr_pool_t *pool)
> >> +{
> >> + return translate_cstring2(dst, NULL, src, eol_str, repair, keywords, expand,
> >> + pool);
> >> +}
> >
> > So, this is revved because svn_subst_translate_string2() needs it (and
> > svn_subst_translate_string() doesn't).
>
> I am unsure of what you mean.
>
> I took the original implementation of `svn_subst_translate_cstring2`
> and placed it in the new static utility function `translate_cstring2`.
> `translate_cstring2` accepts another parameter, and its definition
> (the old definition on `svn_subst_translate_cstring2`) was
> appropriately modified to handle this new parameter. This way, I don't
> have to modify the public API.
>
Yes, yes. You revved three svn_subst_* functions; one publicly and two
privately. So I was pointing out (to myself), on each of the latter
two, where is the call site that needs the additional functionality.
> > I suggest you go ahead (respond to the review, submit an updated patch,
> > etc) without waiting; the svnsync part will eventually get its share of
> > reviews. Also, you may want to consider splitting this patch into two
> > parts (subst work separate from svnsync work).
>
> I like the idea of splitting this into two patches.
Okay. Please post an updated patch to this list when you have it ready :-)
Best,
Daniel
Received on 2010-09-21 18:57:09 CEST