Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions

Scott, I had promised you a patch review, but Julian beat me to it,
and from this message I sense that the ball is in your court again --
i.e., that we're waiting for the next iteration of your patch. If
that's the case, I'll just keep an eye out for a future post from you.
But if there's a most recent patch still pending review, please point
me to it and I'll review. Thanks!

-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
Scott Palmer <scott.palmer@2connected.org> writes:
> 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
> 
-- 
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org