Hi, and thanks for the patch!
On Sat, 24 Sep 2005, Paul Dugas wrote:
> On Fri, September 23, 2005 2:31 pm, Peter N. Lundblad wrote:
> >> Don't add a new subcommand, just add a new flag --show-copy-info to
>
> Okay. Done.
>
> > Except that this isn't safe against spaces in pathnames. I like the
> > solution with two lines better.
>
> Agreed but what should the lines look like? Is the idea to keep the "A"
> line identical to the normal format and simply add another after it?
> Seems to me I'd want some indication in the first line that the next is
> relevant.
>
> How about I add a second character to the line leader? Something like so?
>
> A+ tags/1.0/
> (from trunk/:r1)
>
See the review below, "A + path" would be better.
> New patch attached.
>
This is simple and I reviewed it as-si, but it is easier if you also
provide a log message explaining what the patch does. See hacking.html for
instructions.
> Index: subversion/svnlook/main.c
> ===================================================================
> --- subversion/svnlook/main.c (revision 16222)
> +++ subversion/svnlook/main.c (working copy)
> @@ -124,7 +125,10 @@
> {"full-paths", svnlook__full_paths, 0,
> N_("show full paths instead of indenting them")},
>
> + {"show-copy-info", svnlook__show_copy_info, 0,
> + N_("show details for copies")},
What about calling this option --copies instead?
>
...
> @@ -479,6 +485,7 @@
> static svn_error_t *
> print_changed_tree (svn_repos_node_t *node,
> const char *path /* UTF-8! */,
> + svn_boolean_t show_copy_info,
> apr_pool_t *pool)
> {
> const char *full_path;
> @@ -493,7 +500,11 @@
>
> /* Print the node. */
> if (node->action == 'A')
> - status[0] = 'A';
> + {
> + status[0] = 'A';
> + if ( show_copy_info && node->copyfrom_path )
^ ^
We don't put spaces inside parens.
> + status[1] = '+';
The second column is used for property information, but luckily, there is a third column available to use for this + sign.
> + }
> else if (node->action == 'D')
> status[0] = 'D';
> else if (node->action == 'R')
> @@ -515,6 +526,15 @@
> status,
> path,
> node->kind == svn_node_dir ? "/" : ""));
> + if ( status[1] == '+' )
(Same about spaces) And, I think it is clearer to use a flag for this
instead of checking status, but it's a matter of taste.
> + {
> + SVN_ERR (svn_cmdline_printf (pool, " (from %s%s:r%d)\n",
Use %ld when printf'ing a revision number.
> + (node->copyfrom_path[0]=='/'
> + ? node->copyfrom_path+1
> + : node->copyfrom_path),
Why do you remove the leading slash? (And, be more generous with spaces
around operators, please.
> + (node->kind == svn_node_dir ? "/" : ""),
> + node->copyfrom_rev));
> + }
> }
>
...
Else, it looks good. Would you mind creating a basic test case for this in
svnlook_tests.py?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 27 11:16:13 2005