On Tue, 13 Nov 2007, firstname.lastname@example.org wrote:
> Fix issue #2982.
> Reflect dropped/renumbered revisions in modified svn:mergeinfo data
> * subversion/svndumpfilter/main.c
> (include): Include svn_mergeinfo.h
> (parse_baton_t): Add adjust_mergeinfo boolean for the new option
> (adjust_mergeinfo_value): New function to adjust the mergeinfo value
> (set_node_property): If we have svn:mergeinfo then adjust the
> mergeinfo value
> (svn_opt_subcommand_t): Add --adjust-mergeinfo subcommand
> (options_table): Add description for --adjust-mergeinfo subcommand
> (cmd_table): Add the new subcommand --adjust-mergeinfo
> (svndumpfilter_opt_state): Add adjust_mergeinfo boolean for the
> new option
> (parse_baton_initialize): Initialize adjust_mergeinfo
> (main): Add svndumpfilter__adjust_mergeinfo to switch case
* Dave Glasser mentioned that we might need something similar for
'svnadmin load'. I didn't see any further mention of this in the issue;
is there actually any way for 'svnadmin load' to renumber revisions? If
so, we'd need this. If not, we don't. On IRC, Dave just said (paraphrased):
"In svnadmin load (really in libsvn_repos) there is all this logic *already*
to keep track of renumberings and make sure copyfroms get fixed
so presumably you want that for mergeinfo too"
This seems to assume that 'svnadmin load' can renumber revisions. Can someone
* Your change log (which is quite well-written!) mentions "subcommand" for
--adjust-mergeinfo, which we typically refer to as "option".
* Did you add the --adjust-mergeinfo option for parity with --renumber-revs?
Any particular reason that you didn't just re-use --renumber-revs?
* set_node_property() is always creating a subpool, which it never explicitly
destroys. I'd create this subpool only when needed, and even if this subpool
is already getting implicitly destroyed within a reasonable scope, I'd
explicitly destroy this to make its lifetime more clear. (Perhaps something
more like what we did in libsvn_repos for the merge source path adjustments.)
* There seem to be an awful lot of parenthesis used in the
adjust_mergeinfo_value() function; certainly more than necessary. :)
+ if (!(ary_prefix_match(rb->pb->prefixes, merge_source)))
* Also in that function:
+ if (rb->pb->adjust_mergeinfo)
+ return svn_error_createf(SVN_ERR_INCOMPLETE_DATA, 0,
+ _("Invalid merge source path '%s'"),
I'd write this using a negated conditional, without the "continue" statement.
+ renum_start = (apr_hash_get(rb->pb->renumber_history, &range->start,
+ sizeof(svn_revnum_t *)));
+ renum_end = (apr_hash_get(rb->pb->renumber_history, &range->end,
+ sizeof(svn_revnum_t *)));
The start and end fields of svn_merge_range_t are of type svn_revnum_t,
rather than the address of one. (And the parens are unnecessary.)
Received on Wed Nov 14 03:12:56 2007
- application/pgp-signature attachment: stored