[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Revv svn_client_commit_item_t structure

From: Ivan Zhakov <chemodax_at_gmail.com>
Date: 2005-08-24 10:06:04 CEST

On 8/24/05, Erik Huelsmann <ehuels@gmail.com> wrote:
> On 8/22/05, Ivan Zhakov <chemodax@gmail.com> wrote:
> > Hello!
> > For work on branches/wc-replacements was required to revv
> > svn_client_commit_item_t structure. Because revv was complex and
> > usefull, I propose did this in trunk.
> > Idea in general aproved here by C. Michael Pilato:
> > http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=104319
>
> > [[
> > Isolate svn_client_commit_item_t.revision and add
> > svn_client_commit_item_t.copyfrom_rev. It's required for "replace from
> > copy operations".
>
> All in all the patch looks very nice. I'm running make check now, but
> do have some comments below. There were some extra ^M characters in
> your patch btw, nothing problematic, but the patch program will
> complain...
 
> Some lines you changed either already had, or have now a trailing
> space. Please don't add those, or better, if you change a line, take
> care to remove them.
Ok, I must be more carefull with trailing spaces.
 
> I prefixed my nits below with '=>'
> > * subversion/clients/cmdline/cl.h
> > (svn_cl__get_log_message): Fix documentation.
> > * subversion/clients/cmdline/copy-cmd.c
> > * subversion/clients/cmdline/commit-cmd.c
> > * subversion/clients/cmdline/delete-cmd.c
> > * subversion/clients/cmdline/import-cmd.c
> > * subversion/clients/cmdline/mkdir-cmd.c
> > * subversion/clients/cmdline/move-cmd.c
> > * subversion/clients/cmdline/main.c
> > (svn_cl__copy), (svn_cl__commit), (svn_cl__delete),
> > (svn_cl__import), (main), (svn_cl__mkdir), (svn_cl_move): use new
> > commit message API
> > (use ctx->log_msg_func2/ctx->log_msg_baton2 instead of
> > ctx->log_msg_func/ctx->log_msg_baton).
> > * subversion/clients/cmdline/util.c
> > (svn_cl__get_log_message): turn function into
> > svn_client_get_commit_log2_t type.
> > * subversion/include/svn_client.h:
> > (svn_client_commit_item2_t): New structure, copied from
> > svn_client_commit_item2_t with addition field copyfrom_rev.
> > (svn_client_commit_item_t): Deprecate.
> > (svn_client_get_commit_log2_t): Revv API, new callback accepts array
> > of svn_client_get_commit_log2_t.
> > (svn_client_get_commit_log_t): Deprecate.
> > (svn_client_ctx_t): Add new log_msg_func2/log_msg_batton2 members for
> > new API.
> > Deprecate log_msg_func/log_msg_batton.
> > * subversion/libsvn_client/add.c
> > (mkdir_urls): Use function svn_client__get_log_msg instead of direct
> > call to ctx->log_msg_func.
> > * subversion/libsvn_client/client.h
> > (svn_client__callback_baton_t): Fix documentation.
> > * subversion/libsvn_client/commit.c
> > (svn_client_import2): Use function svn_client__get_log_msg instead of direct
> > call to ctx->log_msg_func.
> > (have_processed_parent): Use svn_client_commit_item2_t in place of
> > svn_client_commit_item2_t.
> > (adjust_rel_targets): Use function svn_client__get_log_msg instead of direct
> > call to ctx->log_msg_func. Use svn_client_commit_item2_t in place of
> > svn_client_commit_item_t.
> > * subversion/libsvn_client/commit_util.c
> > (svn_client__get_log_msg): Utility function for backward compability
> > with 1.2 API.
> > * subversion/libsvn_client/copy.c
> > (wc_to_repos_copy), (repos_to_repos_copy): Use
> > svn_client_commit_item2_t in place of
> > svn_client_commit_item_t.
> > * subversion/libsvn_client/delete.c
> > (delete_urls): Use svn_client_commit_item2_t in place of
> > svn_client_commit_item_t.
> > * subversion/libsvn_client/ra.c
> > (get_wc_prop), (push_wc_prop): Use svn_client_commit_item2_t in place of
> > svn_client_commit_item_t.
> > ]]
>
> In the log message, please use full sentences and start each sentence
> with a capital. ["(svn_cl_move): Use new" instead of "(svn_cl_move):
> use new"]
Ok!

> +typedef svn_error_t *
> +(*svn_client_get_commit_log2_t) (const char **log_msg,
> + const char **tmp_file,
> + apr_array_header_t *commit_items,
> + void *baton,
> + apr_pool_t *pool);
> +
> +/** Callback type used by commit-y operations to get a commit log message
> + * from the caller.
> + *
> + * Set @a *log_msg to the log message for the commit, allocated in @a
> + * pool, or @c NULL if wish to abort the commit process. Set @a *tmp_file
> + * to the path of any temporary file which might be holding that log
> + * message, or @c NULL if no such file exists (though, if @a *log_msg is
> + * @c NULL, this value is undefined). The log message MUST be a UTF8
> + * string with LF line separators.
> + *
> * @a commit_items is a read-only array of @c svn_client_commit_item_t
> * structures, which may be fully or only partially filled-in,
> * depending on the type of commit operation.
>
> =>
> => nice!
> =>
It is pleasant to hear :)

>
> --- subversion/libsvn_client/ra.c (revision 15883)
>
> +++ subversion/libsvn_client/ra.c (working copy)
>
> @@ -92,8 +92,8 @@
>
> int i;
> for (i = 0; i < cb->commit_items->nelts; i++)
> {
> - svn_client_commit_item_t *item
> - = ((svn_client_commit_item_t **) cb->commit_items->elts)[i];
> + svn_client_commit_item2_t *item
> + = ((svn_client_commit_item2_t **) cb->commit_items->elts)[i];
>
> =>
> => 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.

> +
> +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?

-- 
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 24 10:07:06 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.