Re: [PATCH] Redundant creation of svn_client_commit_item3_t's in WC -> repos copy/move operations?

Daniel Rall wrote:
> Hyrum, I think we can remove the apparent redundant creation of
> svn_client_commit_item3_t's in WC -> repos copy/move operations that
> we discussed on IRC.
>
> If for some reason the list of commit items gathered by way of
> harvest_commitables() was different than those inferred from the copy
> pairs (though I don't think it would be), we'd want to use the
> harvested list when prompting the user for a change log message
> anyways.
>
> I think the trade-off here is that we'll end up doing more WC I/O
> before prompting the user for a change log message, meaning that in
> the failure case -- no log message provided -- we've performed some
> extra work. This seems like a very reasonable trade-off for
> simplifying the code.

Dan,
I've taken a look at your patch. I actually tested a very similar one
yesterday afternoon, before our second brief conversation on IRC. With
both my patch and yours, I see the following change in behavior.

To obtain these results, I tested doing wc->repo copies using a free
greek tree, and running:
svn cp A/B file:///path/to/repo/F

Using a stock trunk build, the log message editor shows this:

--This line, and those below, will be ignored--

A file:///path/to/repo/F

Using our patched version, I get this:

--This line, and those below, will be ignored--

A + A/B

In the second version, the path shown is not the path being added, but
rather the source of the path being added. That's a bit confusing, and
not congruent with the rest of the UI.

It appears that the first commit_items array is just being populated
with the destination urls for the sole purpose of getting the log
message. We aren't populating any of the other fields in the
commit_item, except for url and state_flags.

The second commit_items array (the one fetched via
harvest_committables()) uses the source url, not the destination url in
the commit_item3->url field, and can actually be used to do the commit,
but not to fetch the log message. Hence the purpose for two
commit_items arrays.

We could probably fix this by looking at how svn_client__get_log_msg()
uses the commit_item3->url field and doing something intelligent based
upon whether of not there is a commit_item3->copyfrom_url. I haven't
looked at it, though.

I hope this all makes sense. If not, or you aren't seeing the behavior
that I am, let me know.

-Hyrum

> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 23421)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -869,34 +869,6 @@
> }
> }
>
> - /* Create a new commit item and add it to the array. */
> - if (SVN_CLIENT__HAS_LOG_MSG_FUNC(ctx))
> - {
> - svn_client_commit_item3_t *item;
> - const char *tmp_file;
> - commit_items = apr_array_make(pool, copy_pairs->nelts, sizeof(item));
> -
> - for (i = 0; i < copy_pairs->nelts; i++ )
> - {
> - svn_client__copy_pair_t *pair = APR_ARRAY_IDX(copy_pairs, i,
> - svn_client__copy_pair_t *);
> -
> - SVN_ERR(svn_client_commit_item_create
> - ((const svn_client_commit_item3_t **) &item, pool));
> -
> - item->url = pair->dst;
> - item->state_flags = SVN_CLIENT_COMMIT_ITEM_ADD;
> - APR_ARRAY_PUSH(commit_items, svn_client_commit_item3_t *) = item;
> - }
> -
> - SVN_ERR(svn_client__get_log_msg(&message, &tmp_file, commit_items,
> - ctx, pool));
> - if (! message)
> - return SVN_NO_ERROR;
> - }
> - else
> - message = "";
> -
> /* Crawl the working copy for commit items. */
> SVN_ERR(svn_io_check_path(top_src_path, &base_kind, pool));
> if (base_kind == svn_node_dir)
> @@ -916,11 +888,23 @@
> canonical repository URLs. Then, the hacked name can go away and
> be replaced with a entry->repos (or whereever the entry's
> canonical repos URL is stored). */
> - if (! ((commit_items = apr_hash_get(committables,
> - SVN_CLIENT__SINGLE_REPOS_NAME,
> - APR_HASH_KEY_STRING))))
> + if (! (commit_items = apr_hash_get(committables,
> + SVN_CLIENT__SINGLE_REPOS_NAME,
> + APR_HASH_KEY_STRING)))
> goto cleanup;
>
> + /* Create a new commit item and add it to the array. */
> + if (SVN_CLIENT__HAS_LOG_MSG_FUNC(ctx))
> + {
> + const char *tmp_file;
> + SVN_ERR(svn_client__get_log_msg(&message, &tmp_file, commit_items,
> + ctx, pool));
> + if (! message)
> + return SVN_NO_ERROR;
> + }
> + else
> + message = "";
> +
> /* Sort and condense our COMMIT_ITEMS. */
> if ((cmt_err = svn_client__condense_commit_items(&top_dst_url,
> commit_items,