Scott Palmer wrote:
> Sorry to include the patch inline... Apple's Mail program wanted to
> use octet-stream instead of text when I tried it as an attachment.
In-line is the preferred style if it is lossless. Unfortunately the mailer has
messed up the white space, losing blanks at the beginning of the lines, and
whole blank lines. It is unusable by "patch", though I can still read through
it and comment of course.
Could you send the next version as an attachment, please, as
"application/octet-stream" if you have to. ("octet-stream" isn't terribly
friendly - it doesn't show up as text in my mail reader - but I can at least
save it and then open it. Just a guess: try naming the file with a ".txt"
extension and see if the mail program then gives it a text MIME type.)
> This patch is against the trunk. Be gentle - it's my first time ;-)
I'm glad it's against the trunk, since that's where we'll apply it.
I'm not sure I know how to be very gentle :-) I'll just say pretty much
everything that I think you need to know, which includes some little style nits
mixed in with major implementation concerns[1]. You don't have to re-submit
the patch if there are only simple things to fix: I can do that when I commit it.
I'll start with some comments about the log message, since it's the first thing
I or anybody will read about the patch.
> [[[
> Fix issue #1935: svn status too verbose with svn:externals definitions
Here you'll also need to say what the patch does, in a couple of sentences or
so, because issue #1935 doesn't have a single, obvious solution. I can
remember that you told me in a previous email what you were going to do, but
tell me again so everybody else can see :-)
> * subversion/clients/cmdline/cl.h
> Moved status_baton and notify baton to here
You didn't just move them, you also renamed them and added fields. We'd write
this (starting with the name of each top-level item in parentheses) something like:
* subversion/clients/cmdline/cl.h
(svn_cl__status_baton): New. Replaces status_baton in status-cmd.c.
(svn_cl__notify_baton): New. Replaces notify_baton in notify.c.
> * subversion/clients/cmdline/notify.c
> Modified processing of svn_wc_notify_status_external to suppress
> output unless -v specified
> * subversion/clients/cmdline/status-cmd.c
> Print the svn_wc_notify_status_external message from here, if there
> is status to print and -q was specified with -v
You are printing exactly the same message in two completely separate places.
Is there a significant conceptual difference between the two, or is that just
an implementation "convenience"? If the latter, can you find a way to have it
done in exactly one place?
>
> NOTE: This will break test scripts.
Yikes! That's a big red flashing light. We almost never intentionally commit
a change that breaks the regression tests.
I remember this was mentioned in the issue. Tell us why and how it breaks
them, and we'll see whether a fix can be made separately before this patch.
That would be best if possible. If not, we'll probably need to include the fix
for them in this patch.
I'll understand if you don't want to do this, since you might not know Python
and even if you do I got the impression that this impacts the test framework
utility functions rather than the tests themselves, and that's something you
might not want to get into yet. In that case, if you could summarise what
needs to be done, maybe someone else will do it or I will.
> ]]]
My first concern with the implementation is whether it is necessary to make
both the status baton and the notify baton public, and to make them refer to
each other. I realise that this is a big part of the problem you have been
studying; I just wanted to say I'm not too sure about this. I think it may go
hand-in-hand with the message being printed in two places: if you reduce that
to one, I expect at least half of this sharing will be unnecessary.
> Index: subversion/clients/cmdline/notify.c
> ===================================================================
> --- subversion/clients/cmdline/notify.c (revision 15608)
> +++ subversion/clients/cmdline/notify.c (working copy)
> @@ -281,10 +266,25 @@
> break;
> case svn_wc_notify_status_external:
> - if ((err = svn_cmdline_printf
> - (pool, _("\nPerforming status on external item at '%s'\n"),
> - path_local)))
> - goto print_error;
> + if (nb->sb->detailed) /* -v, --verbose */
> + {
> + if (nb->sb->skip_unrecognized) /* -q, --quiet */
> + {
> + nb->extern_path_prefix_length = strlen(path_local);
> + }
> + else
> + {
> + nb->extern_path_prefix_length = 0;
> + if ((err = svn_cmdline_printf
> + (pool, _("\nPerforming status on external item at '%s'\n"),
> + path_local)))
> + goto print_error;
> + }
> + }
> + else
> + {
> + nb->extern_path_prefix_length = 0;
> + }
> break;
> case svn_wc_notify_status_completed:
> Index: subversion/clients/cmdline/status-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/status-cmd.c (revision 15608)
> +++ subversion/clients/cmdline/status-cmd.c (working copy)
> + if (sb->nb->extern_path_prefix_length > 0)
> + {
> + char *tmp = malloc((sb->nb->extern_path_prefix_length+1) * sizeof(char));
> + strncpy(tmp, path, sb->nb->extern_path_prefix_length);
> + tmp[sb->nb->extern_path_prefix_length] = 0;
> +
> + err = svn_cmdline_printf
> + (sb->pool, _("\nPerforming status on external item at '%s'\n"),
> + tmp);
Style: please give even a local variable a meaningful name. Note that all
local variables are temporary! Also, we put a space between a function name
and its open parenthesis.
We don't use "malloc", we use the APR pool functions such as apr_palloc. The
APR pools give us semi-automated memory management (and other resource
management) so we don't need to explicitly "free" everything. In this case of
duplicating the beginning of a string, just use apr_pstrndup:
err = svn_cmdline_printf
(sb->pool, _("\nPerforming status on external item at '%s'\n"),
apr_pstrndup (sb->pool, path, sb->nb->extern_path_prefix_length));
> +
> + free(tmp);
> + sb->nb->extern_path_prefix_length = 0;
> + }
Sorry if it seems like a lot of problems!
- Julian
[1] We recently had a major discussion about this recently; other committers
will be keeping an eye on how I respond. Hi folks!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 9 01:34:04 2005