On Nov 14, 2007 5:04 AM, <stylesen@tigris.org> wrote:
> Author: stylesen
> Date: Wed Nov 14 05:04:57 2007
> New Revision: 27817
>
> Log:
> Fix issue #2982.
>
> Reflect dropped/renumbered revisions in modified svn:mergeinfo data
>
> * subversion/svndumpfilter/main.c
> (): Include svn_mergeinfo.h
> (parse_baton_t): Add skip_missing_merge_sources boolean for the new option
> (renumber_merge_source_rev_range): New function to renumber revs or skip
> missing merge sources in mergeinfo
This does double duty of renumbering and skipping; the name shouldn't
imply that it only does one.
> (set_node_property): If we have svn:mergeinfo then renumber revisions for
> valid merge sources in mergeinfo.
> (svn_opt_subcommand_t): Add --skip-missing-merge-sources option
> (options_table): Add description for --skip-missing-merge-sources option
> (cmd_table): Add the new option --skip-missing-merge-sources
> (svndumpfilter_opt_state): Add skip_missing_merge_sources boolean for the
> new option
> (parse_baton_initialize): Initialize skip_missing_merge_sources
> (main): Add svndumpfilter__skip_missing_merge_sources to switch case
>
> Patch by: me
> Approved by: kameshj
> Reviewed by: kameshj
> dlr
> glasser
Sorry for not getting a chance to review this before committing (this
is my first time actually looking at the patch.)
>
> Modified:
> trunk/subversion/svndumpfilter/main.c
>
> Modified: trunk/subversion/svndumpfilter/main.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svndumpfilter/main.c?pathrev=27817&r1=27816&r2=27817
> ==============================================================================
> --- trunk/subversion/svndumpfilter/main.c (original)
> +++ trunk/subversion/svndumpfilter/main.c Wed Nov 14 05:04:57 2007
> @@ -33,6 +33,7 @@
> #include "svn_pools.h"
> #include "svn_sorts.h"
> #include "svn_props.h"
> +#include "svn_mergeinfo.h"
>
>
> /*** Code. ***/
> @@ -151,6 +152,7 @@
> svn_boolean_t drop_empty_revs;
> svn_boolean_t do_renumber_revs;
> svn_boolean_t preserve_revprops;
> + svn_boolean_t skip_missing_merge_sources;
> apr_array_header_t *prefixes;
>
> /* Input and output streams. */
> @@ -627,6 +629,64 @@
> }
>
>
> +/* Renumber revisions for valid merge sources in mergeinfo. */
> +static svn_error_t *
> +renumber_merge_source_rev_range(const char **mergeinfo_val,
Why return const char * if the only caller actually wants
svn_string_t?
> + const char *mergeinfo_orig,
> + struct revision_baton_t *rb,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *modified_mergeinfo, *mergeinfo;
> + apr_hash_index_t *hi;
> + svn_stringbuf_t *merge_val;
> + const void *merge_source;
> + void *rangelist;
> +
> + SVN_ERR(svn_mergeinfo_parse(&mergeinfo, mergeinfo_orig, pool));
> + modified_mergeinfo = apr_hash_make(pool);
> + for (hi = apr_hash_first(NULL, mergeinfo); hi; hi = apr_hash_next(hi))
> + {
> + int i;
> + apr_hash_this(hi, &merge_source, NULL, &rangelist);
A few style notes. First, merge_source and rangelist should be
declared inside the loop since they are only used there. Second, when
using apr_hash_this we typically call it with variables like "void
*val" and "const void *key", and then immediately assign to pointer
variables of the actual type with more descriptive names, instead of
using casts later in the function.
> +
> + /* Look whether the merge_source is a part of the included prefix. */
> + if (!ary_prefix_match(rb->pb->prefixes, merge_source))
Eek! This isn't the right logic! Check all the other uses of
prefixes in the file: the conditional depends on whether or not we're
running "exclude" or "include".
I suggest creating a helper function which takes a path, the prefixes
hash, and the do_exclude flag, and returns whether or not to skip...
this logic is repeated way too many times.
> + {
> + if (rb->pb->skip_missing_merge_sources)
> + continue;
> + else
> + return svn_error_createf(SVN_ERR_INCOMPLETE_DATA, 0,
> + _("Missing merge source path '%s', try "
> + "with --skip-missing-merge-sources"),
> + (const char *)merge_source);
> + }
> +
> + /* Renumber mergeinfo rangelist. */
I am surprised to see no conditional on rb->pb->do_renumber_revs.
There is no other place in the code where renumber_history is accessed
(except its creation) outside of such a conditional
> + for (i = 0; i < ((apr_array_header_t *)rangelist)->nelts; i++)
> + {
> + struct revmap_t *revmap_start;
> + struct revmap_t *revmap_end;
> + svn_merge_range_t *range;
> +
> + range = APR_ARRAY_IDX((apr_array_header_t *)rangelist, i,
> + svn_merge_range_t *);
> + revmap_start = apr_hash_get(rb->pb->renumber_history, &range->start,
> + sizeof(svn_revnum_t));
> + revmap_end = apr_hash_get(rb->pb->renumber_history, &range->end,
> + sizeof(svn_revnum_t));
> + range->start = revmap_start->rev;
> + range->end = revmap_end->rev;
You need to check that revmap_start and revmap_end are valid here
before dereferencing them (like the check in the copyfrom_rev
renumbering logic). Otherwise malformed input data can cause a crash;
never good!
> + }
> + apr_hash_set(modified_mergeinfo, (const char*)merge_source,
> + APR_HASH_KEY_STRING, rangelist);
> + }
> + svn_mergeinfo_to_stringbuf(&merge_val, modified_mergeinfo, pool);
> + *mergeinfo_val = merge_val->data;
> +
> + return SVN_NO_ERROR;
> +}
> +
[snip]
Given that this patch had a couple of serious flaws, I'm curious: what
was your testing strategy for this patch? (It does look like we are
lacking in automated tests for svndumpfilter, which is a shame.)
--dave
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 14 18:38:36 2007