"Jeremy Whitlock" <jcscoobyrs@gmail.com> writes:
> I have attached an updated version of my original patch for Issue
> 2967 to said issue. This patch does not change the
> functionality/implementation of the previous code. This updated patch
> was purely to make some minor changes to the syntax of the code. As
> part of doing this, the patch has been derived from revision 27562
> instead of revision 27293 which should make applying it easier. Also
> per Karl's request, I have attached the patch and it's commit log
> message to the issue instead of to this email. Please let me know if
> there is anything I can do to help apply this patch. Just in case,
> here is a direct url to the issue:
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=2967
Thank you. Review below:
> [[[
> Add "--xml" support to "svn diff --summarize". (Issue 2967)
Excellent summary line.
> * subversion/tests/cmdline/diff_tests.py
> (diff_summarize_xml): Created unit test to test "--xml" flag functionality.
> (test_list): Added diff_summarize_xml to the list of tests to run for diff.
>
> * subversion/tests/cmdline/svntest/actions.py
> (run_and_verify_diff_summarize_xml): Added a convenience function
> for testing the new "--xml" flag funtionality.
>
> * subversion/svn/diff-cmd.c
> (kind_to_char): Refactored text_mod_char to kind_to_char for readability.
> Translates a svn_client_diff_summarize_kind_t into a char representation.
There's no refactoring here, just a rename.
Also, no point documenting what the function does in the log message
-- documentation like that belongs in the code. IOW, if you're going
to write documentation for an existing function anyway, put it where
it will do the most good! :-)
> (kind_to_word): Translates a svn_client_diff_summarize_kind_t to a word
> representation.
Just say "New function." here, and document that function where it's
declared (which in this case, is also where it's defined).
> (summarize_func_xml): New function for outputting the "diff --summarize"
> in XML format.
Good -- the important thing is to make it clear that this function is
new in this commit. Saying basically what it does is fine, though if
the description were extensive, it would belong in the code, of
course. (See that function below for more comments.)
> (summarize_func): Updated to use kind_to_char instead of text_mod_char.
Phrasing it that way implies some sort of functional change, when in
fact you're just adjusting for a rename. So say "Adjust for above
rename" or whatever, that way it will be clear that this is a trivial
change that the reader shouldn't give much consideration to. (It's
all about guiding the reader -- giving them hints as where their
mental effort will be best spent.)
> (svn_cl__diff): Added logic to handle the "--xml" flag.
>
> * subversion/svn/schema/diff.rnc
> (added to version control): This is the Relax-NG schema for the XML output.
The usual way to write that is:
* subversion/svn/schema/diff.rnc: New file, Relax-NG schema for XML output.
> * subversion/svn/main.c
> (svn_cl__cmd_table): Updated to have the diff command support the
> passing of the "--xml" flag.
Yup. Pretty wordy, though. The more usual & concise way is just:
* subversion/svn/main.c
(svn_cl__cmd_table.diff): Support svn_cl__xml_opt.
Okay, on to the code...
> Index: subversion/tests/cmdline/diff_tests.py
> ===================================================================
> --- subversion/tests/cmdline/diff_tests.py (revision 27562)
> +++ subversion/tests/cmdline/diff_tests.py (working copy)
> @@ -2814,6 +2814,8 @@
> 'diff', '-x', '--ignore-eol-style',
> file_path)
>
> +
> +
> def diff_backward_repos_wc_copy(sbox):
> "backward repos->wc diff with copied file"
>
Spurious whitespace change, a bit distracting but no big deal.
> @@ -2844,7 +2846,98 @@
> svntest.actions.run_and_verify_svn(None, diff_repos_wc, [],
> 'diff', '-r' , '2')
>
> +#----------------------------------------------------------------------
>
> +def diff_summarize_xml(sbox):
> + "xml diff summarize"
> +
> + [...]
> +
> + # 1) Test --xml without --summarize
> + svntest.actions.run_and_verify_svn(None, None,
> + ".*--xml' option only valid with '--summarize' option",
> + 'diff', wc_dir, '--xml')
> +
> + # 2) Test --xml on invalid revision
> + svntest.actions.run_and_verify_diff_summarize_xml(".*No such revision 5555555",
> + wc_dir, None, None, None,
> + None, '-r0:5555555')
> +
> + # 3) Test working copy summarize
> + svntest.actions.run_and_verify_diff_summarize_xml(".*Summarizing diff can only compare repository to repository",
> + wc_dir, None, None, None,
> + None)
I unfortunately don't have time to review the tests in depth, but
watch out for lines >= 80 columns. Use more line breaks to avoid
this.
> --- subversion/tests/cmdline/svntest/actions.py (revision 27562)
> +++ subversion/tests/cmdline/svntest/actions.py (working copy)
> @@ -18,6 +18,7 @@
> import os, shutil, re, sys, errno
> import difflib, pprint
> import xml.parsers.expat
> +from xml.dom.minidom import parseString
>
> import main, verify, tree, wc # general svntest routines in this module.
> from svntest import Failure
> @@ -883,7 +884,68 @@
> else:
> tree.compare_trees (actual, output_tree)
>
> +def run_and_verify_diff_summarize_xml(error_re_string = [],
> + object = '',
> + expected_paths = [],
> + expected_items = [],
> + expected_props = [],
> + expected_kinds = [],
> + *args):
> + """Run 'diff --summarize --xml' with the arguments *ARGS.
> + If ERROR_RE_STRING, the ocmmand must exit with error, and the error
> + message must match regular expression ERROR_RE_STRING.
>
> + Else if ERROR_RE_STRING is None, the subcommand output will be parsed
> + into an XML document and will then be verified by comparing the parsed
> + output to the contents in the EXPECTED_PATHS, EXPECTED_ITEMS,
> + EXPECTED_PROPS and EXPECTED_KINDS. Returns on success, raises
> + on failure."""
The doc string does not mention the OBJECT argument.
(Congrats for writing a new run_and_verify_foo() function, though --
that's the Right Thing to do, but many of us wouldn't have bitten the
bullet :-) ).
Apologies that I have to concentrate on the C code and not the
substance of the Python tests, just due to time constraints. On to
the C code...
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 27562)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -57,6 +58,52 @@
> }
> }
>
> +/* Convert KIND into a word describing the kind to the user. */
> +static const char *
> +kind_to_word(svn_client_diff_summarize_kind_t kind)
> +{
> + switch (kind)
> + {
> + case svn_client_diff_summarize_kind_modified: return "modified";
> + case svn_client_diff_summarize_kind_added: return "added";
> + case svn_client_diff_summarize_kind_deleted: return "deleted";
> + default: return "none";
> + }
> +}
Your patch has TAB characters in this function.
> +/* Print summary information about a given change as XML, implements the
> + * svn_client_diff_summarize_func_t interface. */
> +static svn_error_t *
> +summarize_func_xml(const svn_client_diff_summarize_t *summary,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + const char *path = baton;
Document the after-cast type and meaning of BATON. Yes, it seems
clear from the first line of the function that it's a 'char *', but
users of the function shouldn't need to read the code to know how to
use it -- the doc string should be enough.
(I realize that summarize_func should do that too, it's not your fault
for following the lead of insufficiently-documented code!)
Also...
> + /* Tack on the target path, so we can differentiate between different parts
> + * of the output when we're given multiple targets. */
> + path = svn_path_join(path, summary->path, pool);
...having a parameter named 'path' and joining it to 'summary->path'
is confusing if one doesn't already know what 'path' means. Is it
some kind of prefix path? Giving it a clearer name and explaining it
in the doc string will answer these questions.
> @@ -120,6 +167,21 @@
> if ((status = apr_file_open_stderr(&errfile, pool)))
> return svn_error_wrap_apr(status, _("Can't open stderr"));
>
> + if (opt_state->xml)
> + {
> + /* Check that the --summarize is passed as well. */
> + if (!opt_state->summarize)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'--xml' option only valid with "
> + "'--summarize' option"));
> +
> + SVN_ERR(svn_cl__xml_print_header("diff", pool));
> +
> + svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
> + svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "paths", NULL);
> + SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> + }
Good approach, IMHO. Require both flags now, rather than have --xml
always imply --summarize. That way, if in the future we make --xml
mean something independently of --summarize, we won't be changing the
meaning of existing scripts.
> /* Before allowing svn_opt_args_to_target_array2() to canonicalize
> all the targets, we need to build a list of targets made of both
> ones the user typed, as well as any specified by --changelist. */
> @@ -261,6 +323,8 @@
> iterpool = svn_pool_create(pool);
> for (i = 0; i < targets->nelts; ++i)
> {
> + const svn_client_diff_summarize_func_t summarize_callback =
> + (opt_state->xml ? summarize_func_xml : summarize_func);
> const char *path = APR_ARRAY_IDX(targets, i, const char *);
> const char *target1, *target2;
Any reason to keep doing that conditional assignment of
summarize_callback in the path loop? It's never going to change,
since it's based on opt_state->xml, so it would make more sense to do
it outside the loop.
It's hard to distinguish between the names 'summarize_func' and
'summarize_callback'. Might want to use
summarize_regular
summarize_xml
and then use the name 'summarize_func' for 'summarize_callback'.
> Index: subversion/svn/schema/diff.rnc
> ===================================================================
> --- subversion/svn/schema/diff.rnc (revision 0)
> +++ subversion/svn/schema/diff.rnc (revision 0)
> @@ -0,0 +1,25 @@
> +# XML RELAX NG schema for Subversion command-line client output
> +# For "svn diff --summarize --xml"
> +
> +include "common.rnc"
> +
> +start = diff
> +
> +paths = element paths { path* }
> +
> +## A path entry
> +path = element path { attlist.path, text }
> +attlist.path &=
> + ## The props of the entry.
> + attribute props { "none" | "modified" },
> + ## The kind of the entry.
> + attribute kind { "dir" | "file" },
> + ## The item of the entry.
> + attribute item { "none" | "added" | "modified" | "deleted" }
Recommend explaining the odd terminology 'item' here, since it won't
be in intuitive to most readers (I know you chose it for consistency).
This is almost ready for commit!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Nov 2 23:12:24 2007