Julian,
Thanks a lot for the review. I figured it would take a couple tries
to get something that 'fit'. More inline...
On 8-Aug-05, at 7:33 PM, Julian Foad wrote:
> Scott Palmer wrote:
>
>> * 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?
I would like to avoid duplicating the string ("Performing..."), but I
didn't see a simple way to avoid the actual print call the in both
places with the specific implementation I chose for when "-v -q" was
specified.
I had a slightly different view of the problem that I was trying to
solve. I did issue #1935 with my own enhancement for when -v and -q
were specified together. I imagine with more time I could find an
alternate solution. I went with the simplest solution I could think
of. Simple is usually good :-)... but I could easily have missed
something simpler that I would have recognized if I was more familiar
with the code.
I explain what I do in the "-v -q" case below.
>> 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 knew it would be. Which was why I made sure to mention it, yet to
be honest I didn't even check that it did for sure break the tests.
(That was stupid of me*) I mentioned that based on the comments in
the bug report. My testing was restricted to checking that the
output matched what was expected of issue #1935, with the assumption
that such output would not be what the current test scripts expected.
I also originally developed the patch on the 1.2.x branch and
switched to the trunk to generate the diffs. When I did this there
were conflicts that I had to resolve related to XML status output,
and I simply don't know anything about that new feature. As you
mention, I also don't know Python, and don't think I would be up to
modifying the test scripts if that was needed.
That and I was eager to get some feedback.. knowing I was rushing it,
it seemed best to turn on the big red flashing light.
*I'm now running the tests to verify that the test scripts do indeed
break. If they don't, I'm not sure if that is good or bad :-) How
would the scripts know the output was correct if they accept the
output that has changed without being told about it?
The results are in... everything passed but:
FAIL: lt-fs-test 13: delete nodes tree
At least one test was SKIPPED, checking /Users/scottpalmer/dev/
svn_test/subversion/tests.log
SKIP: utf8_tests.py 1: conversion of paths and logs to/from utf8
I'm not sure if the fs-test failure is my fault. It doesn't sound
like it should be.
> 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.
The reason it is in two places is that I implemented the mode I
originally wanted for the combination of "-v -q". In that mode the
"Performing..." message is printed ONLY if it will be followed by
some other status output for that path. This was not a specific
requirement of issue #1935, but the expected output of "-v -q" was
not specified either. Perhaps I should just drop that mode.
>> 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));
To be honest, I was expecting to be scolded for the use of malloc. I
could tell that the pool was what I should be using. I blame my
total ignorance of the APR functions . Thanks for this, it is
exactly what I thought I should have done, but didn't know how to do.
> Sorry if it seems like a lot of problems!
Not at all! I will attempt to address the issues you have brought
up. Some are trivial to fix, like improving the log message and
using "apr_pstrndup". If you feel strongly about the "printing from
two places" issue I might be able to elminate the need for making the
batons "public" and drop my "-v -q" extension to the original issue.
Thanks,
Scott
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 9 15:51:51 2005