Daniel L. Rall wrote:
> 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?
I shall look into this.
> * Your change log (which is quite well-written!) mentions "subcommand" for
> --adjust-mergeinfo, which we typically refer to as "option".
Initially I mentioned it as option and later when I posted it I switched
to subcommand. Shall revert it in my next patch :)
> * Did you add the --adjust-mergeinfo option for parity with --renumber-revs?
> Any particular reason that you didn't just re-use --renumber-revs?
Now the options name is changed to --skip-missing-merge-sources as per
Kamesh's comments. I think this explains the option well than the
previous name. It is basically to manipulate the mergeinfo sources and
the rev numbers associated with it.
> * 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.)
I ve updated this in my latest patch.
> * 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.
I need this continue statement, since I dont want the rest of the code
in the for loop to get executed and directly go to the next rangelist.
If the continue is not there, for the current rangelist the revmap at
sometime returns "start=end" which results in a mergeinfo something like
the following:
Properties on '/tmp/wc3/trunk':
svn:mergeinfo : /branch1:3-2
> + 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.)
Corrected in the updated patch.
Dan, thank you for your review comments :)
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 14 08:43:24 2007