On Mon, 24 Jul 2006, Madan S. wrote:
...
> Pl. find attached a follow-up to r20808. This code calculates the
> relative path to the target dir, from the repos root and uses that as
> input (the 'paths' parameter) to the svn_ra_get_merge_info() call.
>
> Pl. note that I removed the following comment.
> - /* ### TODO: Add target_wcpath to the list. Later, we may need to
> - ### list all child paths as well. */
>
> The first stated goal has been accomplished by this patch. The second
> sentence, IMHO is not correct, as there can be only one target path. Pl.
> correct me if am wrong.
We've discussed this quite a bit already over email.
When a child path of the target has different merge info in the WC
from the target itself, don't we have to look up its merge info as
known by the repository and reconcile the differences
(svn_mergeinfo_merge() may not be sufficient, as WC-local changes
may've performed a revert)?
When a child path of the target has different merge info as known by
the repository, we'll need to know that too. I'm not certain that our
API actually covers this case yet in a very friendly fashion (e.g. I
think you currently have to list all child paths under the target).
Comments on this patch inline below...
p.s. I've never seen anyone but you abbreviate the word "please".
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 20836)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -1948,6 +1948,7 @@
> const char *URL1, *URL2, *path1, *path2, *rel_path;
> svn_opt_revision_t *revision1, *revision2;
> int i;
> + const char *target_url, *repos_root, *relative_path;
We already have a variable in this scope named "rel_path" which
describes a very similar piece of information. In such a situation,
the variables would need to be renamed to differentiate between the
two.
> ENSURE_VALID_REVISION_KINDS(initial_revision1->kind,
> initial_revision2->kind);
> @@ -2010,10 +2011,24 @@
> /* Retrieve any inherited or direct merge info for the target from
> both the repos and the WC's merge info prop. Combine these two
> sets of merge info to determine what's already been merged into
> - the target. */
> - mergeinfo_paths = apr_array_make(pool, 1, sizeof(target_wcpath));
> - /* ### TODO: Add target_wcpath to the list. Later, we may need to
> - ### list all child paths as well. */
If anything, you should elaborate on this TODO, not remove it.
> + the target.
> +
> + But first, get the relative path of the working copy
> + wrt repos root */
I would make this snippet a entirely separate comment. Especially
since we have to do something similar in do_single_file_merge(), it'd
produce more comprehensible code to make this block a static function.
If you're going to abbreviate in inline comments (ill advised, as it
makes things harder to understand), uppercase your acronyms.
> + SVN_ERR(svn_client_url_from_path(&target_url, target_wcpath, pool));
> + if (! target_url)
> + return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> + _("'%s' has no URL"),
> + svn_path_local_style(target_wcpath, pool));
Don't we already have this information as the local variable "URL1"?
Also, your indentation level is off.
> + SVN_ERR(svn_ra_get_repos_root(ra_session, &repos_root, pool));
> + if(strlen(repos_root) < strlen(target_url))
^
Missing space.
> + relative_path = target_url + strlen(repos_root);
> +
> + relative_path = svn_path_canonicalize(relative_path, pool);
If relative_path wasn't already set, you'll be calling
svn_path_canonicalize() on a random piece of memory, and likely
trigger a SIGSEGV.
I'd end the function used to calculate the repository-relative path
for the target here. Let's call it something like
repos_relative_path_for_wc_target().
> +
> + mergeinfo_paths = apr_array_make(pool, 1, sizeof(char *));
> + APR_ARRAY_PUSH(mergeinfo_paths, const char *) = relative_path;
> +
> SVN_ERR(svn_ra_get_merge_info(ra_session, &repos_mergeinfo, mergeinfo_paths,
> SVN_INVALID_REVNUM, TRUE, pool));
> SVN_ERR(parse_merge_info(&target_mergeinfo, target_wcpath, adm_access, ctx,
...
Ditto for do_single_file_merge().
> Use appropriate input path for svn_ra_get_merge_info().
>
> On the merge-tracking branch:
In the summary of your change log message, speak as much to why you're
making the change as to what it is, and prefix it with the branch
information. Example:
On the merge-tracking branch: When determining the merge info for a
merge target, retrieve the merge info known to the repository by
including its path when calling svn_ra_get_merge_info().
> * subversion/libsvn_client/diff.c
> (do_single_file_merge, do_merge): Fill the target path, relative to the
> repos root into the mergeinfo_path variable before calling
> svn_ra_get_merge_info.
- application/pgp-signature attachment: stored
Received on Mon Jul 24 19:43:58 2006