You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie <ep...@pretzelnet.org> on 2007/09/20 21:31:20 UTC
[PATCH] Move issue 1550 check from svn to svn_client
In working on issue 2850, I introduced a bug in svn log's
handling of multiple wc targets. This escaped my detection (but
not David Glasser's :) as svn_cl__log blocks this case, making it
impossible to test with our test suite. Let's go ahead and block
this in svn_client_log4; it never worked right anyway (1550), and
we shouldn't make guarantees we can't test.
[[[
svn_client_log4's handling of multiple wc targets is unreachable via the
svn command-line client, and therefore untestable. So, reject multiple wc
targets in svn_client_log4 instead of svn_cl__log.
In support of this, add a new test function for verifying svn log; we
should extend log_tests.py to make more use of this in the future (and the
in-progress change for issue 2850 uses it).
* subversion/libsvn_client/log.c
(svn_client_log4): Drop unused base_name variable. Reject multiple wc
targets with the error message from svn_cl__log, and document the
sordid history of this case. Now we can drop the for loop for wc
targets and other special handling, leaving only one call to
svn_ra_get_log2.
* subversion/include/svn_client.h
(svn_client_log4): Update doc string.
* subversion/svn/log-cmd.c
(log_message_receiver_xml): Drop extra logentry close tag, found by
run_and_verify_log_xml.
(svn_cl__log): Don't check for multiple wc targets.
* subversion/tests/cmdline/svntest/actions.py
(LogEntry, LogParser): Add utility classes for testing and parsing svn
log --xml output.
(run_and_verify_log_xml): Add function to test svn log --xml output.
* subversion/tests/cmdline/log_tests.py
(dynamic_revision): This test did nothing more than make sure the log
commands exited 0 with no error output. Use run_and_verify_log_xml to
ensure the log commands actually work. Consequently, break PREV out of
the loop, as it is a different revision from the others.
(only_one_wc_path): Add test to ensure multiple wc targets are rejected.
]]]
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 26666)
+++ subversion/include/svn_client.h (working copy)
@@ -1742,15 +1742,12 @@
* given log message more than once).
*
* @a targets contains either a URL followed by zero or more relative
- * paths, or a list of working copy paths, as <tt>const char *</tt>,
- * for which log messages are desired. The repository info is
- * determined by taking the common prefix of the target entries' URLs.
- * @a receiver is invoked only on messages whose revisions involved a
- * change to some path in @a targets. @a peg_revision indicates in
- * which revision @a targets are valid. If @a peg_revision is @c
- * svn_opt_revision_unspecified, it defaults to @c
- * svn_opt_revision_head for URLs or @c svn_opt_revision_working for
- * WC paths.
+ * paths, or 1 working copy path, as <tt>const char *</tt>, for which log
+ * messages are desired. @a receiver is invoked only on messages whose
+ * revisions involved a change to some path in @a targets. @a peg_revision
+ * indicates in which revision @a targets are valid. If @a peg_revision is
+ * @c svn_opt_revision_unspecified, it defaults to @c svn_opt_revision_head
+ * for URLs or @c svn_opt_revision_working for WC paths.
*
* If @a limit is non-zero only invoke @a receiver on the first @a limit
* logs.
Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c (revision 26666)
+++ subversion/libsvn_client/log.c (working copy)
@@ -294,7 +294,6 @@
svn_ra_session_t *ra_session;
const char *url_or_path;
const char *ignored_url;
- const char *base_name = NULL;
apr_array_header_t *condensed_targets;
svn_revnum_t ignored_revnum;
svn_opt_revision_t session_opt_rev;
@@ -350,6 +349,12 @@
apr_array_header_t *real_targets;
int i;
+ /* See FIXME about multiple wc targets, below. */
+ if (targets->nelts > 1)
+ return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ _("When specifying working copy paths, only "
+ "one target may be given"));
+
/* Get URLs for each target */
target_urls = apr_array_make(pool, 1, sizeof(const char *));
real_targets = apr_array_make(pool, 1, sizeof(const char *));
@@ -444,94 +449,53 @@
*
* in which case we want to avoid recomputing the static revision on
* every iteration.
+ *
+ * ### FIXME: However, we can't yet handle multiple wc targets anyway.
+ *
+ * We used to iterate over each target in turn, getting the logs for
+ * the named range. This led to revisions being printed in strange
+ * order or being printed more than once. This is issue 1550.
+ *
+ * In r11599, jpieper blocked multiple wc targets in svn/log-cmd.c,
+ * meaning this block not only doesn't work right in that case, but isn't
+ * even testable that way (svn has no unit test suite; we can only test
+ * via the svn command). So, that check is now moved into this function
+ * (see above).
+ *
+ * kfogel ponders future enhancements in r4186:
+ * I think that's okay behavior, since the sense of the command is
+ * that one wants a particular range of logs for *this* file, then
+ * another range for *that* file, and so on. But we should
+ * probably put some sort of separator header between the log
+ * groups. Of course, libsvn_client can't just print stuff out --
+ * it has to take a callback from the client to do that. So we
+ * need to define that callback interface, then have the command
+ * line client pass one down here.
+ *
+ * epg wonders if the repository could send a unified stream of log
+ * entries if the paths and revisions were passed down.
*/
{
- svn_error_t *err = SVN_NO_ERROR; /* Because we might have no targets. */
svn_revnum_t start_revnum, end_revnum;
+ const char *path = APR_ARRAY_IDX(targets, 0, const char *);
- svn_boolean_t start_is_local = svn_client__revision_is_local(start);
- svn_boolean_t end_is_local = svn_client__revision_is_local(end);
+ SVN_ERR(svn_client__get_revision_number
+ (&start_revnum, ra_session, start, path, pool));
+ SVN_ERR(svn_client__get_revision_number
+ (&end_revnum, ra_session, end, path, pool));
- if (! start_is_local)
- SVN_ERR(svn_client__get_revision_number
- (&start_revnum, ra_session, start, base_name, pool));
-
- if (! end_is_local)
- SVN_ERR(svn_client__get_revision_number
- (&end_revnum, ra_session, end, base_name, pool));
-
- if (start_is_local || end_is_local)
- {
- /* ### FIXME: At least one revision is locally dynamic, that
- * is, we're in a case similar to one of these:
- *
- * $ svn log -rCOMMITTED foo.txt bar.c
- * $ svn log -rCOMMITTED:42 foo.txt bar.c
- *
- * We'll iterate over each target in turn, getting the logs
- * for the named range. This means that certain revisions may
- * be printed out more than once. I think that's okay
- * behavior, since the sense of the command is that one wants
- * a particular range of logs for *this* file, then another
- * range for *that* file, and so on. But we should
- * probably put some sort of separator header between the log
- * groups. Of course, libsvn_client can't just print stuff
- * out -- it has to take a callback from the client to do
- * that. So we need to define that callback interface, then
- * have the command line client pass one down here.
- *
- * In any case, at least it will behave uncontroversially when
- * passed only one argument, which I would think is the common
- * case when passing a local dynamic revision word.
- */
-
- int i;
-
- for (i = 0; i < targets->nelts; i++)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
-
- if (start_is_local)
- SVN_ERR(svn_client__get_revision_number
- (&start_revnum, ra_session, start, target, pool));
-
- if (end_is_local)
- SVN_ERR(svn_client__get_revision_number
- (&end_revnum, ra_session, end, target, pool));
-
- err = svn_ra_get_log2(ra_session,
- condensed_targets,
- start_revnum,
- end_revnum,
- limit,
- discover_changed_paths,
- strict_node_history,
- include_merged_revisions,
- omit_log_text,
- receiver,
- receiver_baton,
- pool);
- if (err)
- break;
- }
- }
- else /* both revisions are static, so no loop needed */
- {
- err = svn_ra_get_log2(ra_session,
- condensed_targets,
- start_revnum,
- end_revnum,
- limit,
- discover_changed_paths,
- strict_node_history,
- include_merged_revisions,
- omit_log_text,
- receiver,
- receiver_baton,
- pool);
- }
-
- return err;
+ return svn_ra_get_log2(ra_session,
+ condensed_targets,
+ start_revnum,
+ end_revnum,
+ limit,
+ discover_changed_paths,
+ strict_node_history,
+ include_merged_revisions,
+ omit_log_text,
+ receiver,
+ receiver_baton,
+ pool);
}
}
Index: subversion/tests/cmdline/log_tests.py
===================================================================
--- subversion/tests/cmdline/log_tests.py (revision 26666)
+++ subversion/tests/cmdline/log_tests.py (working copy)
@@ -504,8 +504,16 @@
guarantee_repos_and_wc(sbox)
os.chdir(sbox.wc_dir)
- for rev in ('HEAD', 'BASE', 'COMMITTED', 'PREV'):
- svntest.actions.run_and_verify_svn(None, None, [], 'log', '-r', rev)
+ revprops = [{'svn:author':'jrandom',
+ 'svn:date':'', 'svn:log': 'Log message for revision 9'}]
+ for rev in ('HEAD', 'BASE', 'COMMITTED'):
+ svntest.actions.run_and_verify_log_xml(expected_revprops=revprops,
+ args=['-r', rev])
+ revprops[0]['svn:log'] = ('Log message for revision 8\n'
+ ' but with multiple lines\n'
+ ' to test the code')
+ svntest.actions.run_and_verify_log_xml(expected_revprops=revprops,
+ args=['-r', 'PREV'])
#----------------------------------------------------------------------
def log_wc_with_peg_revision(sbox):
@@ -1027,7 +1035,19 @@
log_chain = parse_log_output(output)
check_log_chain(log_chain, [2, 5, 7])
+#----------------------------------------------------------------------
+def only_one_wc_path(sbox):
+ "svn log of two wc paths is disallowed"
+ sbox.build()
+ os.chdir(sbox.wc_dir)
+
+ svntest.actions.run_and_verify_log_xml(
+ expected_stderr=('.*When specifying working copy paths,'
+ ' only one target may be given'),
+ args=['A/mu', 'iota'])
+
+
########################################################################
# Run the tests
@@ -1056,6 +1076,7 @@
XFail(log_single_change),
XFail(log_changes_range),
XFail(log_changes_list),
+ only_one_wc_path,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 26666)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -16,6 +16,8 @@
######################################################################
import os, shutil, re, sys, errno
+import difflib, pprint
+import xml.parsers.expat
import main, tree, wc # general svntest routines in this module.
from svntest import Failure, SVNAnyOutput
@@ -428,6 +430,153 @@
run_and_verify_status(wc_dir_name, status_tree)
+# run_and_verify_log_xml
+
+class LogEntry:
+ def __init__(self, revision, changed_paths=None, revprops=None):
+ self.revision = revision
+ if changed_paths == None:
+ self.changed_paths = {}
+ else:
+ self.changed_paths = changed_paths
+ if revprops == None:
+ self.revprops = {}
+ else:
+ self.revprops = revprops
+
+ def assert_changed_paths(self, changed_paths):
+ """Not implemented, so just raises svntest.Failure.
+ """
+ raise Failure('NOT IMPLEMENTED')
+
+ def assert_revprops(self, revprops):
+ """Assert that the dict revprops is the same as this entry's revprops.
+
+ Raises svntest.Failure if not.
+ """
+ if self.revprops != revprops:
+ raise Failure('\n' + '\n'.join(difflib.ndiff(
+ pprint.pformat(revprops).splitlines(),
+ pprint.pformat(self.revprops).splitlines())))
+
+class LogParser:
+ def parse(self, data):
+ """Return a list of LogEntrys parsed from the sequence of strings data.
+
+ This is the only method of interest to callers.
+ """
+ try:
+ for i in data:
+ self.parser.Parse(i)
+ self.parser.Parse('', True)
+ except xml.parsers.expat.ExpatError, e:
+ raise SVNUnexpectedStdout('%s\n%s\n' % (e, ''.join(data),))
+ return self.entries
+
+ def __init__(self):
+ # for expat
+ self.parser = xml.parsers.expat.ParserCreate()
+ self.parser.StartElementHandler = self.handle_start_element
+ self.parser.EndElementHandler = self.handle_end_element
+ self.parser.CharacterDataHandler = self.handle_character_data
+ # Ignore some things.
+ self.ignore_elements('log', 'paths', 'path', 'revprops')
+ self.ignore_tags('logentry_end', 'author_start', 'date_start', 'msg_start')
+ # internal state
+ self.cdata = []
+ self.property = None
+ # the result
+ self.entries = []
+
+ def ignore(self, *args, **kwargs):
+ del self.cdata[:]
+ def ignore_tags(self, *args):
+ for tag in args:
+ setattr(self, tag, self.ignore)
+ def ignore_elements(self, *args):
+ for element in args:
+ self.ignore_tags(element + '_start', element + '_end')
+
+ # expat handlers
+ def handle_start_element(self, name, attrs):
+ getattr(self, name + '_start')(attrs)
+ def handle_end_element(self, name):
+ getattr(self, name + '_end')()
+ def handle_character_data(self, data):
+ self.cdata.append(data)
+
+ # element handler utilities
+ def use_cdata(self):
+ result = ''.join(self.cdata).strip()
+ del self.cdata[:]
+ return result
+ def svn_prop(self, name):
+ self.entries[-1].revprops['svn:' + name] = self.use_cdata()
+
+ # element handlers
+ def logentry_start(self, attrs):
+ self.entries.append(LogEntry(int(attrs['revision'])))
+ def author_end(self):
+ self.svn_prop('author')
+ def msg_end(self):
+ self.svn_prop('log')
+ def date_end(self):
+ # svn:date could be anything, so just note its presence.
+ self.cdata[:] = ['']
+ self.svn_prop('date')
+ def property_start(self, attrs):
+ self.property = attrs['name']
+ def property_end(self):
+ self.entries[-1].revprops[self.property] = self.use_cdata()
+
+def run_and_verify_log_xml(message=None, expected_paths=None,
+ expected_revprops=None, expected_stdout=None,
+ expected_stderr=None, args=[]):
+ """Call run_and_verify_svn with log --xml and args (optional) as command
+ arguments, and pass along message, expected_stdout, and expected_stderr.
+
+ If message is None, pass the svn log command as message.
+
+ expected_paths checking is not yet implemented.
+
+ expected_revprops is an optional list of dicts, compared to each
+ revision's revprops. The list must be in the same order the log entries
+ come in. Any svn:date revprops in the dicts must be '' in order to
+ match, as the actual dates could be anything.
+
+ expected_paths and expected_revprops are ignored if expected_stdout or
+ expected_stderr is specified.
+ """
+ if message == None:
+ message = ' '.join(args)
+
+ # We'll parse the output unless the caller specifies expected_stderr or
+ # expected_stdout for run_and_verify_svn.
+ parse = True
+ if expected_stderr == None:
+ expected_stderr = []
+ else:
+ parse = False
+ if expected_stdout != None:
+ parse = False
+
+ log_args = list(args)
+ if expected_paths != None:
+ log_args.append('-v')
+
+ (stdout, stderr) = run_and_verify_svn(
+ message, expected_stdout, expected_stderr,
+ 'log', '--xml', *log_args)
+ if not parse:
+ return
+
+ for (index, entry) in enumerate(LogParser().parse(stdout)):
+ if expected_revprops != None:
+ entry.assert_revprops(expected_revprops[index])
+ if expected_paths != None:
+ entry.assert_changed_paths(expected_paths[index])
+
+
def run_and_verify_update(wc_dir_name,
output_tree, disk_tree, status_tree,
error_re_string = None,
Index: subversion/svn/log-cmd.c
===================================================================
--- subversion/svn/log-cmd.c (revision 26666)
+++ subversion/svn/log-cmd.c (working copy)
@@ -322,10 +322,7 @@
if (! SVN_IS_VALID_REVNUM(log_entry->revision))
{
- svn_xml_make_close_tag(&sb, pool, "logentry");
- SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
apr_array_pop(lb->merge_stack);
-
return SVN_NO_ERROR;
}
@@ -507,17 +504,8 @@
}
}
- /* Verify that we pass at most one working copy path. */
- if (! svn_path_is_url(target) )
+ if (svn_path_is_url(target))
{
- if (targets->nelts > 1)
- return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
- _("When specifying working copy paths, only "
- "one target may be given"));
- }
- else
- {
- /* Check to make sure there are no other URLs. */
for (i = 1; i < targets->nelts; i++)
{
target = APR_ARRAY_IDX(targets, i, const char *);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Move issue 1550 check from svn to svn_client
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> Index: subversion/svn/log-cmd.c
> ===================================================================
> --- subversion/svn/log-cmd.c (revision 26666)
> +++ subversion/svn/log-cmd.c (working copy)
> @@ -322,10 +322,7 @@
>
> if (! SVN_IS_VALID_REVNUM(log_entry->revision))
> {
> - svn_xml_make_close_tag(&sb, pool, "logentry");
> - SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> apr_array_pop(lb->merge_stack);
> -
> return SVN_NO_ERROR;
> }
>
Hyrum was right, this is the wrong change. I added XML test to
merge_sensitive_log_added_path and fixed it properly. Or did I?
Index: log-cmd.c
===================================================================
--- log-cmd.c (revision 26666)
+++ log-cmd.c (working copy)
@@ -322,10 +322,12 @@
if (! SVN_IS_VALID_REVNUM(log_entry->revision))
{
- svn_xml_make_close_tag(&sb, pool, "logentry");
- SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
- apr_array_pop(lb->merge_stack);
-
+ if (lb->merge_stack->nelts > 0)
+ {
+ svn_xml_make_close_tag(&sb, pool, "logentry");
+ SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
+ apr_array_pop(lb->merge_stack);
+ }
return SVN_NO_ERROR;
}
I think this change is wrong, too, now that I realize we only get
the broken XML for ra-{neon,serf,svn}; ra-local is fine. Here's
what the broken XML looks like (frommerge_sensitive_log_added_path):
<?xml version="1.0"?>
<log>
<logentry
revision="14">
<author>jrandom</author>
<date>2007-06-07T17:29:48.480221Z</date>
<msg>Merge branches/b to trunk</msg>
</logentry>
<logentry
revision="12">
<author>jrandom</author>
<date>2007-06-07T17:27:29.282205Z</date>
<msg>Merge branches/a to branches/b</msg>
</logentry>
<logentry
revision="11">
<author>jrandom</author>
<date>2007-06-07T17:24:53.981098Z</date>
<msg>Added 'xi' to branches/a, made a few other changes.</msg>
</logentry>
</logentry>
</log>
Note the extra </logentry> at the end, which does not appear for
ra-local. Looking at libsvn_repos/log.c:send_logs and
libsvn_ra_svn/client.c:ra_svn_log, I see that the latter sends
SVN_INVALID_REVNUM again at the end:
/* Send the "end of messages" sentinal. */
log_entry = svn_log_entry_create(subpool);
log_entry->revision = SVN_INVALID_REVNUM;
SVN_ERR(receiver(receiver_baton, log_entry, subpool));
(It's "sentinel", BTW :)
So, my fix above is right if svn_repos_get_logs4 should be
calling the receiver with that final sentinel. But we could also
fix it by dropping the extra sentinel from the other ra modules.
I'm not really sure.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org