On Wednesday 27 February 2013 11:10 AM, Julian Foad wrote:
@@ -439,14 +442,23 @@
/* write out the revision */
/* Revision is written out in the following cases:
- 1. No --drop-empty-revs has been supplied.
- 2. --drop-empty-revs has been supplied,
- but revision has not all nodes dropped
- 3. Revision had no nodes to begin with.
+ 1. If the revision has nodes or it is revision 0.
+ 2. --drop-empty-revs has been supplied,
+ but revision has not all nodes dropped.
+ 3. No --drop-all-empty-revs has been supplied.
+ 4. If no --drop-empty-revs or --drop-all-empty-revs have been supplied,
+ write out the revision which has no nodes to begin with.
This comment does not sound completely right: the rev is not always written out
in case 3.
I have updated the code and comment slightly.
*/
- if (rb->has_nodes
- || (! rb->pb->drop_empty_revs)
- || (! rb->had_dropped_nodes))
+ if (rb->has_nodes || (rb->rev_orig == 0))
+ write_out_rev = TRUE;
+ else if (rb->pb->drop_empty_revs)
+ write_out_rev = rb->had_dropped_nodes ? FALSE : TRUE;
As a matter of style, please don't write "x ? FALSE : TRUE", but rather "! x".
Done. I will keep it in mind.
+ else if (rb->pb->drop_all_empty_revs)
+ write_out_rev = FALSE;
+ else
+ write_out_rev = TRUE;
+
+ if (write_out_rev)
{
In your new test for this option, you test with --renumber-revs; could you also
test without that? That seems worth testing too, in my opinion, if it's as
easy to do as it looks.
Done. I have updated the test.
Everything else looks fine, from just a read-through review.
Thanks Julian. As cmpilato said, I have updated the usage message and
code docs to have a note about special-casing revision 0.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
Index: subversion/svndumpfilter/svndumpfilter.c
===================================================================
--- subversion/svndumpfilter/svndumpfilter.c (revision 1450596)
+++ subversion/svndumpfilter/svndumpfilter.c (working copy)
@@ -210,6 +210,7 @@
svn_boolean_t quiet;
svn_boolean_t glob;
svn_boolean_t drop_empty_revs;
+ svn_boolean_t drop_all_empty_revs;
svn_boolean_t do_renumber_revs;
svn_boolean_t preserve_revprops;
svn_boolean_t skip_missing_merge_sources;
@@ -376,6 +377,7 @@
int bytes_used;
char buf[SVN_KEYLINE_MAXLEN];
apr_hash_index_t *hi;
+ svn_boolean_t write_out_rev = FALSE;
apr_pool_t *hash_pool = apr_hash_pool_get(rb->props);
svn_stringbuf_t *props = svn_stringbuf_create_empty(hash_pool);
apr_pool_t *subpool = svn_pool_create(hash_pool);
@@ -391,7 +393,8 @@
if ((! rb->pb->preserve_revprops)
&& (! rb->has_nodes)
&& rb->had_dropped_nodes
- && (! rb->pb->drop_empty_revs))
+ && (! rb->pb->drop_empty_revs)
+ && (! rb->pb->drop_all_empty_revs))
{
apr_hash_t *old_props = rb->props;
rb->has_props = TRUE;
@@ -439,14 +442,21 @@
/* write out the revision */
/* Revision is written out in the following cases:
- 1. No --drop-empty-revs has been supplied.
- 2. --drop-empty-revs has been supplied,
- but revision has not all nodes dropped
- 3. Revision had no nodes to begin with.
+ 1. If the revision has nodes or
+ it is revision 0 (Special case: To preserve the props on r0).
+ 2. --drop-empty-revs has been supplied,
+ but revision has not all nodes dropped.
+ 3. If no --drop-empty-revs or --drop-all-empty-revs have been supplied,
+ write out the revision which has no nodes to begin with.
*/
- if (rb->has_nodes
- || (! rb->pb->drop_empty_revs)
- || (! rb->had_dropped_nodes))
+ if (rb->has_nodes || (rb->rev_orig == 0))
+ write_out_rev = TRUE;
+ else if (rb->pb->drop_empty_revs)
+ write_out_rev = (! rb->had_dropped_nodes) ? TRUE : FALSE;
+ else if (! rb->pb->drop_all_empty_revs)
+ write_out_rev = TRUE;
+
+ if (write_out_rev)
{
/* This revision is a keeper. */
SVN_ERR(svn_stream_write(rb->pb->out_stream,
@@ -994,6 +1004,7 @@
enum
{
svndumpfilter__drop_empty_revs = SVN_OPT_FIRST_LONGOPT_ID,
+ svndumpfilter__drop_all_empty_revs,
svndumpfilter__renumber_revs,
svndumpfilter__preserve_revprops,
svndumpfilter__skip_missing_merge_sources,
@@ -1023,6 +1034,9 @@
N_("Treat the path prefixes as file glob patterns.") },
{"drop-empty-revs", svndumpfilter__drop_empty_revs, 0,
N_("Remove revisions emptied by filtering.")},
+ {"drop-all-empty-revs", svndumpfilter__drop_all_empty_revs, 0,
+ N_("Remove all empty revisions found in dumpstream\n"
+ " except revision 0.")},
{"renumber-revs", svndumpfilter__renumber_revs, 0,
N_("Renumber revisions left after filtering.") },
{"skip-missing-merge-sources",
@@ -1045,7 +1059,8 @@
{"exclude", subcommand_exclude, {0},
N_("Filter out nodes with given prefixes from dumpstream.\n"
"usage: svndumpfilter exclude PATH_PREFIX...\n"),
- {svndumpfilter__drop_empty_revs, svndumpfilter__renumber_revs,
+ {svndumpfilter__drop_empty_revs, svndumpfilter__drop_all_empty_revs,
+ svndumpfilter__renumber_revs,
svndumpfilter__skip_missing_merge_sources, svndumpfilter__targets,
svndumpfilter__preserve_revprops, svndumpfilter__quiet,
svndumpfilter__glob} },
@@ -1053,7 +1068,8 @@
{"include", subcommand_include, {0},
N_("Filter out nodes without given prefixes from dumpstream.\n"
"usage: svndumpfilter include PATH_PREFIX...\n"),
- {svndumpfilter__drop_empty_revs, svndumpfilter__renumber_revs,
+ {svndumpfilter__drop_empty_revs, svndumpfilter__drop_all_empty_revs,
+ svndumpfilter__renumber_revs,
svndumpfilter__skip_missing_merge_sources, svndumpfilter__targets,
svndumpfilter__preserve_revprops, svndumpfilter__quiet,
svndumpfilter__glob} },
@@ -1076,6 +1092,7 @@
svn_boolean_t glob; /* --pattern */
svn_boolean_t version; /* --version */
svn_boolean_t drop_empty_revs; /* --drop-empty-revs */
+ svn_boolean_t drop_all_empty_revs; /* --drop-all-empty-revs */
svn_boolean_t help; /* --help or -? */
svn_boolean_t renumber_revs; /* --renumber-revs */
svn_boolean_t preserve_revprops; /* --preserve-revprops */
@@ -1107,9 +1124,11 @@
/* Ignore --renumber-revs if there can't possibly be
anything to renumber. */
baton->do_renumber_revs =
- (opt_state->renumber_revs && opt_state->drop_empty_revs);
+ (opt_state->renumber_revs && (opt_state->drop_empty_revs
+ || opt_state->drop_all_empty_revs));
baton->drop_empty_revs = opt_state->drop_empty_revs;
+ baton->drop_all_empty_revs = opt_state->drop_all_empty_revs;
baton->preserve_revprops = opt_state->preserve_revprops;
baton->quiet = opt_state->quiet;
baton->glob = opt_state->glob;
@@ -1188,11 +1207,13 @@
{
SVN_ERR(svn_cmdline_fprintf(stderr, subpool,
do_exclude
- ? opt_state->drop_empty_revs
+ ? (opt_state->drop_empty_revs
+ || opt_state->drop_all_empty_revs)
? _("Excluding (and dropping empty "
"revisions for) prefix
patterns:\n")
: _("Excluding prefix patterns:\n")
- : opt_state->drop_empty_revs
+ : (opt_state->drop_empty_revs
+ || opt_state->drop_all_empty_revs)
? _("Including (and dropping empty "
"revisions for) prefix
patterns:\n")
: _("Including prefix patterns:\n")));
@@ -1201,11 +1222,13 @@
{
SVN_ERR(svn_cmdline_fprintf(stderr, subpool,
do_exclude
- ? opt_state->drop_empty_revs
+ ? (opt_state->drop_empty_revs
+ || opt_state->drop_all_empty_revs)
? _("Excluding (and dropping empty "
"revisions for) prefixes:\n")
: _("Excluding prefixes:\n")
- : opt_state->drop_empty_revs
+ : (opt_state->drop_empty_revs
+ || opt_state->drop_all_empty_revs)
? _("Including (and dropping empty "
"revisions for) prefixes:\n")
: _("Including prefixes:\n")));
@@ -1427,6 +1450,9 @@
case svndumpfilter__drop_empty_revs:
opt_state.drop_empty_revs = TRUE;
break;
+ case svndumpfilter__drop_all_empty_revs:
+ opt_state.drop_all_empty_revs = TRUE;
+ break;
case svndumpfilter__renumber_revs:
opt_state.renumber_revs = TRUE;
break;
@@ -1448,6 +1474,16 @@
} /* close `switch' */
} /* close `while' */
+ /* Disallow simultaneous use of both --drop-empty-revs and
+ --drop-all-empty-revs. */
+ if (opt_state.drop_empty_revs && opt_state.drop_all_empty_revs)
+ {
+ err = svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
+ _("--drop-empty-revs cannot be used with "
+ "--drop-all-empty-revs"));
+ return svn_cmdline_handle_exit_error(err, pool, "svndumpfilter: ");
+ }
+
/* If the user asked for help, then the rest of the arguments are
the names of subcommands to get help on (if any), or else they're
just typos/mistakes. Whatever the case, the subcommand to
Index: subversion/tests/cmdline/svndumpfilter_tests.py
===================================================================
--- subversion/tests/cmdline/svndumpfilter_tests.py (revision 1450596)
+++ subversion/tests/cmdline/svndumpfilter_tests.py (working copy)
@@ -705,7 +705,68 @@
os.close(fd)
os.remove(targets_file)
+@Issue(3681)
+def drop_all_empty_revisions(sbox):
+ "drop all empty revisions except revision 0"
+
+ dumpfile_location = os.path.join(os.path.dirname(sys.argv[0]),
+ 'svndumpfilter_tests_data',
+ 'empty_revisions.dump')
+ dump_contents = open(dumpfile_location).read()
+
+ filtered_dumpfile, filtered_err = filter_and_return_output(
+ dump_contents,
+ 8192, # Set a sufficiently large bufsize to avoid a deadlock
+ "include", "branch1",
+ "--drop-all-empty-revs")
+
+ expected_err = [
+ "Including (and dropping empty revisions for) prefixes:\n",
+ " '/branch1'\n",
+ "\n",
+ "Revision 0 committed as 0.\n",
+ "Revision 1 skipped.\n",
+ "Revision 2 committed as 2.\n",
+ "Revision 3 skipped.\n",
+ "\n",
+ "Dropped 2 revisions.\n",
+ "\n"]
+
+ svntest.verify.verify_outputs(
+ "Actual svndumpfilter stderr does not agree with expected stderr",
+ None, filtered_err, None, expected_err)
+ # Test with --renumber-revs option.
+ filtered_dumpfile, filtered_err = filter_and_return_output(
+ dump_contents,
+ 8192, # Set a sufficiently large bufsize to avoid a deadlock
+ "include", "branch1",
+ "--drop-all-empty-revs",
+ "--renumber-revs")
+
+ expected_err = [
+ "Including (and dropping empty revisions for) prefixes:\n",
+ " '/branch1'\n",
+ "\n",
+ "Revision 0 committed as 0.\n",
+ "Revision 1 skipped.\n",
+ "Revision 2 committed as 1.\n",
+ "Revision 3 skipped.\n",
+ "\n",
+ "Dropped 2 revisions.\n",
+ "\n",
+ "Revisions renumbered as follows:\n",
+ " 3 => (dropped)\n",
+ " 2 => 1\n",
+ " 1 => (dropped)\n",
+ " 0 => 0\n",
+ "\n"]
+
+ svntest.verify.verify_outputs(
+ "Actual svndumpfilter stderr does not agree with expected stderr",
+ None, filtered_err, None, expected_err)
+
+
########################################################################
# Run the tests
@@ -721,6 +782,7 @@
match_empty_prefix,
accepts_deltas,
dumpfilter_targets_expect_leading_slash_prefixes,
+ drop_all_empty_revisions,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svndumpfilter_tests_data/empty_revisions.dump
===================================================================
--- subversion/tests/cmdline/svndumpfilter_tests_data/empty_revisions.dump
(revision 0)
+++ subversion/tests/cmdline/svndumpfilter_tests_data/empty_revisions.dump
(working copy)
@@ -0,0 +1,94 @@
+SVN-fs-dump-format-version: 2
+
+UUID: 40278d28-80c2-4ce3-9606-68ce4b659d51
+
+Revision-number: 0
+Prop-content-length: 56
+Content-length: 56
+
+K 8
+svn:date
+V 27
+2012-06-24T14:02:12.037632Z
+PROPS-END
+
+Revision-number: 1
+Prop-content-length: 105
+Content-length: 105
+
+K 10
+svn:author
+V 7
+jrandom
+K 8
+svn:date
+V 27
+2012-06-24T14:02:13.264066Z
+K 7
+svn:log
+V 4
+init
+PROPS-END
+
+Revision-number: 2
+Prop-content-length: 115
+Content-length: 115
+
+K 10
+svn:author
+V 7
+jrandom
+K 8
+svn:date
+V 27
+2012-06-24T14:02:14.070370Z
+K 7
+svn:log
+V 13
+make a branch
+PROPS-END
+
+Node-path: branch1
+Node-kind: dir
+Node-action: add
+Prop-content-length: 41
+Content-length: 41
+
+K 4
+soup
+V 16
+No soup for you!
+PROPS-END
+
+
+Node-path: branch1/foo
+Node-kind: file
+Node-action: add
+Prop-content-length: 10
+Text-content-length: 20
+Text-content-md5: 6f2d0469e1b4e16adf755b7e18f09d8a
+Text-content-sha1: 3df9ea3dfa67b8dea7968ecfd30e726285a2b383
+Content-length: 30
+
+PROPS-END
+This is file 'foo'.
+
+
+Revision-number: 3
+Prop-content-length: 112
+Content-length: 112
+
+K 10
+svn:author
+V 7
+jrandom
+K 8
+svn:date
+V 27
+2012-06-24T14:02:15.135672Z
+K 7
+svn:log
+V 10
+prop delta
+PROPS-END
+
Fix issue #3681: Add '--drop-all-empty-revs' to 'svndumpfilter include/exclude'
to remove all the empty revisions found in the dumpstream except revision 0.
This would allow admins to purge empty revisions from repositories previously
populated by 'svndumpfilter' when --drop-empty-revs was not provided,
or by partial-repository mirroring via 'svnsync', etc.
* subversion/svndumpfilter/svndumpfilter.c
(struct parse_baton_t): Add 'drop_all_empty_revs' member.
(output_revision): If 'drop_all_empty_revs' is set, remove all the empty
revisions (those that make no path modifications) found in the dumpstream
except r0. Meanwhile, preserve the meaning of 'drop-empty-revs'
which removes only the revisions emptied by filtering process.
(svndumpfilter__drop_all_empty_revs): New.
(options_table): Add new 'drop-all-empty-revs' option.
(cmd_table): Make 'exclude' and 'include' accept the new option.
(struct svndumpfilter_opt_state): Add 'drop-all-empty-revs'.
(parse_baton_initialize): Add new option value to parse baton.
(do_filter): Check if 'drop-all-empty-revs' is set while printing the
filtering notification message.
(main): Parse the new option. Handle the case of 'drop-empty-revs' and
'drop-all-empty-revs' being used together.
* subversion/tests/cmdline/svndumpfilter_tests.py
(drop_all_empty_revisions): New test.
(test_list): Add reference to the new test.
* subversion/tests/cmdline/svndumpfilter_tests_data/empty_revisions.dump:
New dump file for the test.
Patch by: Vijayaguru G <vijay{_AT_}collab.net>