On Thu, Jul 02, 2009 at 05:35:43PM +0800, HuiHuang wrote:
> Hey,
Hi,
sorry I had part of this mail typed up yesterday already but
something got in the way and I ended up being distracted and
forgot about finishing the mail :(
I have applied your patch to current trunk and tried to build it,
and the build succeeded :)
> I have modified mistakes you indicated, and the following is the new patch. I will debug
> and test it in the next few days.
OK.
How will you test?
Will you make a separate patch which modifies the svn client to
call the new svn_client_commit5() function?
Also, could you adjust the multiple_wc regression tests we have
to expect multiple commits in another patch?
Then we can test all patches together, and if they work, we can
commit them.
And don't forget to run *all* regression tests with your patches
applied. On Windows, you can run the win-tests.py script in the
root of the Suversion working copy to run all tests.
(But note that running all tests does not make sense before you
are actually calling your new code from the svn command line client.)
> line 344 in the patch:
> +lock_err = svn_wc_adm_open3(&base_dir_access, NULL,
> + new_base_dir,
> + TRUE, /* Write lock */
> + -1, /* lock levels */
> + ctx->cancel_func,
> + ctx->cancel_baton,
> + iterpool1);
> But here I have a question. If we lock new_base_dir successfully, base_dir_access and
> lock_err will be use later. So if we use iterpool1 here, we have to duplicate these two
> variables, but I do not find functions to duplicate them.
>
> Or do we just use pool instead of iterpool1 in the function?
Yes, pass 'pool'.
If you need the result in future iterations or even after the loop
terminates, don't pass an iterpool. Just pass whatever pool you have
which has sufficient lifetime.
Some functions now allow you to pass a result_pool and a scratch_pool,
in which case you could pass 'pool' for result_pool and iterpool for
scratch_pool.
Since collect_commit_packets() takes only one pool, you'll have to pass
'pool'. Or you could change collect_commit_packets() to dual-pool style
(result_pool, scratch_pool), and the pass 'pool, pool' when you invoke it.
This would allow us to later easily update the caller of
collect_commit_packets() to dual-pool style, too.
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 38280)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -67,7 +67,46 @@
>
> } import_ctx_t;
>
> +typedef struct commit_packet_t
> +{
> + /* Working copy root of a wc */
> + const char *base_dir;
>
> + /* Targets under base_dir */
> + apr_array_header_t *targets;
> +
> + /* Relative paths of targets base on base_dir */
You could say: "Target paths, relative to base_dir".
Rest looks fine to me and should be tested now.
Thank you!
Stefan
Received on 2009-07-03 13:05:44 CEST