Re: [Issue 2982] Reflect dropped/renumbered revisions in modified svn:mergeinfo data

On Tue, 13 Nov 2007, stylesen@tigris.org wrote:

> http://subversion.tigris.org/issues/show_bug.cgi?id=2982
...
> 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

Some questions/comments:

* 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
verify this?

* 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. :)
Example:
+ if (!(ary_prefix_match(rb->pb->prefixes, merge_source)))

* Also in that function:
+ if (rb->pb->adjust_mergeinfo)
+ continue;
+ else
+ return svn_error_createf(SVN_ERR_INCOMPLETE_DATA, 0,
+ _("Invalid merge source path '%s'"),
+ merge_source);
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.)

Thanks Senthil!
- Dan