> > =>
> > => it would be nicer to use APR_ARRAY_IDX in the above line: it's
> > better readabel
> > => and you're changing the code anyway. The same goes for a number of similar
> > => instances below
> > =>
> Yes, I know about this APR_ARRAY_IDX and APR_ARRAY_PUSH macros. But I
> hold opinion: the patch should change only what it changes. So I
> against fixing style on fly in existing code. If subversion developers
> holds another opinion, I can fix my patch or send another patch. But I
> should use APR_ARRAY_PUSH/APR_ARRAY_IDX in new code (function
> svn_client__get_log_msg). It is my fault. I'll fix it.
True, but some of the files already use the APR_ARRAY_* macros, just
not on the lines you changed. Since this is only a semantical change
(leading to exactly the same code after macro-expansion), I think it's
nice to adhere to 'the new way of doing array manipulation'.
Especially if the file already uses APR_ARRAY_* macros.
I think of it on the lines of changing trailing spaces on the fly.
> > +
> > +svn_error_t * svn_client__get_log_msg(const char **log_msg,
> > + const char **tmp_file,
> > + apr_array_header_t *commit_items,
> > + svn_client_ctx_t *ctx, apr_pool_t *pool)
> > +{
> > + /* client provided new callback function. simply forward call to him */
> > + if (ctx->log_msg_func2)
> > + return (*ctx->log_msg_func2) (log_msg, tmp_file, commit_items,
> > + ctx->log_msg_baton2, pool);
> > +
> > + /* client want use old (pre 1.3) API, therefore build
> > + * svn_client_commit_item_t array */
> > +
> > + if (ctx->log_msg_func)
> > + {
> > + int i;
> > + svn_client_commit_item_t *old_item;
> > + apr_array_header_t *old_commit_items
> > + = apr_array_make (pool, commit_items->nelts, sizeof (old_item));
> > +
> > + for (i = 0; i < commit_items->nelts; i++)
> > + {
> > + svn_client_commit_item2_t *item =
> > + APR_ARRAY_IDX (commit_items, i, svn_client_commit_item2_t *);
> > +
> > + old_item = apr_pcalloc (pool, sizeof (*old_item));
> > + old_item->path = item->path;
> > + old_item->kind = item->kind;
> > + old_item->url = item->url;
> > + /* pre 1.3 API use revision field for copyfrom_rev and revision
> > + * depeding of copyfrom_url */
> > + old_item->revision = item->copyfrom_url ?
> > + item->copyfrom_rev : item->revision;
> > + old_item->copyfrom_url = item->copyfrom_url;
> > + old_item->state_flags = item->state_flags;
> > + old_item->wcprop_changes = item->wcprop_changes;
> > +
> > + APR_ARRAY_PUSH (old_commit_items,
> > svn_client_commit_item_t *) = old_item;
> > + }
> > +
> > + return (*ctx->log_msg_func) (log_msg, tmp_file, old_commit_items,
> > + ctx->log_msg_baton, pool);
> > + }
> > +
> > + *log_msg = "";
> > + *tmp_file = NULL;
> > +
> > + return SVN_NO_ERROR;
> > +}
> >
> > =>
> > => I had some discussion about this on IRC with other devs
> > => but I think this is indeed the cleanest way of supporting the old api
> > =>
>
> My IRC client was disconnected tonight and I have not found #svn-dev
> archives in the web. To what decision you come?
Only one thing which I forgot to mention in my last mail: we think it
would be clean to allocate old_commit_items and all the old_item-s in
a subpool which you destroy after the call to (*ctx->log_msg_func).
That way the impact of the extra space-allocation is minimal.
One last remark I forgot to tell you about: at least one of the lines
in your patch was beyond 80 characters. Please make your lines 78
characters long (or less).
Thanks for working on this!
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 24 11:01:02 2005