You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Scott Palmer <sc...@2connected.org> on 2005/08/05 22:19:43 UTC
[PATCH] Issue #1935: svn status too verbose with svn:externals definitions
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.
This patch is against the trunk. Be gentle - it's my first time ;-)
[[[
Fix issue #1935: svn status too verbose with svn:externals definitions
* subversion/clients/cmdline/cl.h
Moved status_baton and notify baton to here
* 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
NOTE: This will break test scripts.
]]]
Index: subversion/clients/cmdline/cl.h
===================================================================
--- subversion/clients/cmdline/cl.h (revision 15608)
+++ subversion/clients/cmdline/cl.h (working copy)
@@ -150,7 +150,38 @@
svn_client_ctx_t *ctx;
} svn_cl__cmd_baton_t;
+struct svn_cl__notify_baton;
+struct svn_cl__status_baton
+{
+ /* These fields all correspond to the ones in the
+ svn_cl__print_status() interface. */
+ svn_boolean_t detailed;
+ svn_boolean_t show_last_committed;
+ svn_boolean_t skip_unrecognized;
+ svn_boolean_t repos_locks;
+ apr_pool_t *pool;
+ svn_boolean_t had_print_error; /* To avoid printing lots of
errors if we get
+ errors while printing to stdout */
+ svn_boolean_t xml_mode;
+ struct svn_cl__notify_baton *nb;
+};
+
+/* Baton for notify and friends. */
+struct svn_cl__notify_baton
+{
+ svn_boolean_t received_some_change;
+ svn_boolean_t is_checkout;
+ svn_boolean_t is_export;
+ svn_boolean_t suppress_final_line;
+ svn_boolean_t sent_first_txdelta;
+ svn_boolean_t in_external;
+ svn_boolean_t had_print_error; /* Used to not keep printing error
messages
+ when we've already had one print error. */
+ int extern_path_prefix_length; /* Used to recreate external folder
name */
+ struct svn_cl__status_baton *sb;
+};
+
/* Declare all the command procedures */
svn_opt_subcommand_t
svn_cl__add,
Index: subversion/clients/cmdline/notify.c
===================================================================
--- subversion/clients/cmdline/notify.c (revision 15608)
+++ subversion/clients/cmdline/notify.c (working copy)
@@ -33,27 +33,12 @@
#include "svn_private_config.h"
-
-/* Baton for notify and friends. */
-struct notify_baton
-{
- svn_boolean_t received_some_change;
- svn_boolean_t is_checkout;
- svn_boolean_t is_export;
- svn_boolean_t suppress_final_line;
- svn_boolean_t sent_first_txdelta;
- svn_boolean_t in_external;
- svn_boolean_t had_print_error; /* Used to not keep printing error
messages
- when we've already had one print
error. */
-};
-
-
/* This implements `svn_wc_notify_func2_t'.
* NOTE: This function can't fail, so we just ignore any print
errors. */
static void
notify (void *baton, const svn_wc_notify_t *n, apr_pool_t *pool)
{
- struct notify_baton *nb = baton;
+ struct svn_cl__notify_baton *nb = baton;
char statchar_buf[5] = " ";
const char *path_local;
svn_error_t *err;
@@ -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:
@@ -391,7 +391,7 @@
svn_boolean_t suppress_final_line,
apr_pool_t *pool)
{
- struct notify_baton *nb = apr_palloc (pool, sizeof (*nb));
+ struct svn_cl__notify_baton *nb = apr_palloc (pool, sizeof (*nb));
nb->received_some_change = FALSE;
nb->sent_first_txdelta = FALSE;
@@ -400,6 +400,8 @@
nb->suppress_final_line = suppress_final_line;
nb->in_external = FALSE;
nb->had_print_error = FALSE;
+ nb->extern_path_prefix_length = 0;
+ nb->sb = NULL;
*notify_func_p = notify;
*notify_baton_p = nb;
Index: subversion/clients/cmdline/status-cmd.c
===================================================================
--- subversion/clients/cmdline/status-cmd.c (revision 15608)
+++ subversion/clients/cmdline/status-cmd.c (working copy)
@@ -22,6 +22,7 @@
/*** Includes. ***/
+#include "svn_cmdline.h"
#include "svn_wc.h"
#include "svn_client.h"
#include "svn_error.h"
@@ -32,26 +33,8 @@
#include "svn_private_config.h"
-
-
/*** Code. ***/
-struct status_baton
-{
- /* These fields all correspond to the ones in the
- svn_cl__print_status() interface. */
- svn_boolean_t detailed;
- svn_boolean_t show_last_committed;
- svn_boolean_t skip_unrecognized;
- svn_boolean_t repos_locks;
- apr_pool_t *pool;
-
- svn_boolean_t had_print_error; /* To avoid printing lots of
errors if we get
- errors while printing to stdout */
- svn_boolean_t xml_mode;
-};
-
-
/* Prints XML target element with path attribute TARGET, using POOL for
temporary allocations. */
static svn_error_t *
@@ -119,9 +102,23 @@
const char *path,
svn_wc_status2_t *status)
{
- struct status_baton *sb = baton;
+ struct svn_cl__status_baton *sb = baton;
svn_error_t *err;
-
+
+ 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);
+
+ free(tmp);
+ sb->nb->extern_path_prefix_length = 0;
+ }
+
if (sb->xml_mode)
err = svn_cl__print_status_xml (path, status, sb->pool);
else
@@ -156,7 +153,7 @@
apr_pool_t * subpool;
int i;
svn_opt_revision_t rev;
- struct status_baton sb;
+ struct svn_cl__status_baton sb;
svn_revnum_t repos_rev = SVN_INVALID_REVNUM;
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
@@ -169,7 +166,10 @@
if (! opt_state->xml)
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2,
FALSE,
FALSE, FALSE, pool);
-
+ struct svn_cl__notify_baton *nb = ctx->notify_baton2;
+ sb.nb = nb;
+ nb->sb = &sb;
+
/* Add "." if user passed 0 arguments */
svn_opt_push_implicit_dot_target(targets, pool);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by "C. Michael Pilato" <cm...@collab.net>.
Scott Palmer <sc...@2connected.org> writes:
> 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.
fs-test failure is not your fault. It's a latent bug in the
libsvn_fs_fs code recently brought to light by actually testing that
code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by Scott Palmer <sc...@2connected.org>.
Yes, the ball is in my court. But then a truckload of work fell on
my desk (upcoming trade show)...
I plan to make the minor changes that Julian mentioned, e.g. using
the APR routine for duplicating a string using a pool, improving the
log message, etc. Then I planned to bounce it back to you guys. I
have not found an elegant way to only print the message in one spot.
I will try yo get something posted this week, as I'm off on vacation
the following week.. far away from the tentacles of the internet.
Scott
On 23-Aug-05, at 2:33 PM, kfogel@collab.net wrote:
> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by kf...@collab.net.
Scott Palmer <sc...@2connected.org> writes:
> What is the apr function to convert to UTF8? I don't suppose there
> an easy way to fold the conversion into the strndup without
> converting the entire path, not just the prefix that I need to print?
> I'm just thinking of avoiding more of those allocations, but I guess
> if we clear the pool when the message is printed it doesn't matter.
> I just didn't have time to understand enough of how the pool was used
> to feel comfortable with that. How do we know when it is "safe" to
> clear the pool? It seems any user of the pool has to be very careful
> that it isn't cleared before they are done with it.
See the interfaces in svn_utf.h, those are what you should use.
The difference between converting an entire path and a prefix of that
path is not worth worrying about, IMHO.
The way to know if it's safe to clear a pool is to look at what
objects are allocated in that pool, and know what their lifetimes
should be. Fortunately, in this case, that was very easy to do by
inspecting the code (at least, it was easy from looking at the
original, pre-change code -- you might want to double check that a
pool clear will still be okay after the change too).
> Now I'm concerned about the skipped utf8 test... Perhaps it would
> have caught my UTF8 strlen mistake?
> I'm running on OS X, I think there were some known issues with that
> test on OS X at one point.
I don't know what's up with that test. It's been skipping forever on
all the systems I run 'make check' on, but I haven't looked into it.
I doubt it would have caught that mistake anyway, as it probably
doesn't do anything with externals. (Feel free to write your a new
regression test, though! In your copious spare time, of course... :-) ).
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by Scott Palmer <sc...@2connected.org>.
Thank you for your comments, I don't have any disagreements with your
assessment. To summarize there are three changes to make
- clean up the log message, make it more concise (in conjunction with
added comments in the code) and adhere to the guidelines.
- improve/add comments in the code
- fix the UTF8 <-> native encoding issue when remembering the
external folder length
... but since I'm on vacation next week and busy getting work done
this week so I can afford to take the vacation, you won't see
anything for a while ;-)
>
> print_status() just calls svn_cl__print_status_xml() and
> svn_cl__print_status() to do the real work. Both of those functions
> take their 'path' arguments in UTF8, as does print_status() itself,
> since it implements a public interface and Subversion public
> interfaces always take paths in UTF8 unless otherwise stated.
>
> So, you can't just print out 'path'; you have to convert it to native
> encoding and print that instead. And this solves the problem that
> back in notify.c, you calculate 'extern_path_prefix_length' based on
> 'path_local', because now the calculation will match what's being
> printed later.
Oops! I missed the distinction between path_local in notify.c and
path in status-cmd.c. That was silly, I should have known the names
were different for a reason.
What is the apr function to convert to UTF8? I don't suppose there
an easy way to fold the conversion into the strndup without
converting the entire path, not just the prefix that I need to
print? I'm just thinking of avoiding more of those allocations, but
I guess if we clear the pool when the message is printed it doesn't
matter. I just didn't have time to understand enough of how the pool
was used to feel comfortable with that. How do we know when it is
"safe" to clear the pool? It seems any user of the pool has to be
very careful that it isn't cleared before they are done with it.
> Have you verified that the change passes 'make check', by the way?
I did verify that the changes passed "make check". There was concern
expressed in the comments for issue #1935 that the tests might fail
after this change, but they did not.
I wrote earlier in this thread:
> 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.
Now I'm concerned about the skipped utf8 test... Perhaps it would
have caught my UTF8 strlen mistake?
I'm running on OS X, I think there were some known issues with that
test on OS X at one point.
Thanks,
Scott
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by kf...@collab.net.
This is looking pretty good, IMHO. Below are some comments, mostly
about minor matters. However, there is one C89 compliance issue, one
interesting pool allocation point, and one correctness issue having to
do with UTF8 versus native encoding.
Scott Palmer <sc...@2connected.org> writes:
> [[[
> Fix issue #1935: svn status too verbose with svn:externals definitions
>
> Unless the verbose option (-v) is specified, the message "Performing
> status on external item at ..." is suppressed. When verbose and
> quiet (-q) options are specified together the message will be printed
> only when there will be relevant status output for that external
> folder.
>
> * 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.
> The above structures were needed to communicate information about
> the command line options to svn_wc_notify_status_external in notify.c
> and to provide a place to remember information about the message that
> will be accessible should the code in status-cmd.c determine that the
> message is needed when verbose and quiet options are specified
> together.
>
> While I generally didn't like to tie these two structures together or
> duplicate the printing of the same message in two places, it appears
> to be the simplest, most straightforward method to handle issue
> #1935. Other possible implementations may require API changes that
> should probably be considered outside the scope of this single issue.
Those explanations should really be comments at the appropriate places
in the code, I think (for example, at each duplicate message, and/or
at the new structures). Readers of the code won't necessarily look at
this log message, but they'll see code comments, and anyone changing
(say) one of the messages needs to know to change the other as well.
> * subversion/clients/cmdline/notify.c
> Modified processing of svn_wc_notify_status_external to suppress
> output unless -v is specified (without -q). If the -q option was
> specified then save enough information to regenerate the message from
> status_cmd.c
This is a good, concise description, and in this case it's probably
enough to enable a clueful reader to understand the code change.
However, when we commit this, we'll need to make it follow the log
message guidelines. This is not merely procedure for procedure's
sake; if this log message followed those guidelines, it would be even
*more* helpful to those trying to understand the change, e.g., me :-).
The guidelines are at:
http://subversion.tigris.org/hacking.html#log-messages
They're pretty easy to grok, I think; let us know if anything there is
unclear.
> * subversion/clients/cmdline/status-cmd.c
> If there is status to print and -q was specified with -v, print
> the svn_wc_notify_status_external message from here, since this is
> the point where we know that relevant status output for the external
> folder will happen. There does not appear to be a straightforward
> way to eliminate printing this message from two possible locations
> (here and from notify.c)
> ]]]
(Same comment about the log message.)
Yeah, I tried to find another way myself and was unable to avoid a
large, complex change, so I sympathize.
> Index: subversion/clients/cmdline/cl.h
> ===================================================================
> --- subversion/clients/cmdline/cl.h (revision 15888)
> +++ subversion/clients/cmdline/cl.h (working copy)
> @@ -150,7 +150,38 @@
> svn_client_ctx_t *ctx;
> } svn_cl__cmd_baton_t;
>
> +struct svn_cl__notify_baton;
> +struct svn_cl__status_baton
> +{
> + /* These fields all correspond to the ones in the
> + svn_cl__print_status() interface. */
> + svn_boolean_t detailed;
> + svn_boolean_t show_last_committed;
> + svn_boolean_t skip_unrecognized;
> + svn_boolean_t repos_locks;
> + apr_pool_t *pool;
>
> + svn_boolean_t had_print_error; /* To avoid printing lots of errors if we get
> + errors while printing to stdout */
> + svn_boolean_t xml_mode;
> + struct svn_cl__notify_baton *nb;
> +};
svn_cl__status_baton should get a general comment explaining what it's
for. Ideally, the old 'struct status_baton' of which this is the heir
would have had such a doc string already, but anyway it becomes even
more important when the struct is pulled out of a status-specific
file.
In general, a lot of the log message's explanatory text about the
situation really belongs near these two structures.
When documenting an individual field (e.g., 'had_print_error'), don't
just state its purpose, state its precise semantics. For example,
"/* TRUE if blah blah blah, FALSE otherwise. */" It's fine to also
describe the purpose, if that's not clear from the semantics, just
don't leave the semantics out.
> +/* Baton for notify and friends. */
> +struct svn_cl__notify_baton
> +{
> + svn_boolean_t received_some_change;
> + svn_boolean_t is_checkout;
> + svn_boolean_t is_export;
> + svn_boolean_t suppress_final_line;
> + svn_boolean_t sent_first_txdelta;
> + svn_boolean_t in_external;
> + svn_boolean_t had_print_error; /* Used to not keep printing error messages
> + when we've already had one print error. */
> + int extern_path_prefix_length; /* Used to recreate external folder name */
> + struct svn_cl__status_baton *sb;
> +};
> +
Same comment about individual fields.
Where it says "Baton for notify and friends", it's not clear what kind
of entity "notify" is. Is it a function? A subcommand? A general
concept? :-) I think a more verbose doc string for the overall
structure would be helpful.
I realize you inherited that comment from the old code. It was too
terse even in its original context; it should have said "notify()"
with parens, probably. But in its new context, it's positively
cryptic, because the function notify() is not local to svn_cl.h.
> /* Declare all the command procedures */
> svn_opt_subcommand_t
> svn_cl__add,
> Index: subversion/clients/cmdline/notify.c
> ===================================================================
> --- subversion/clients/cmdline/notify.c (revision 15888)
> +++ subversion/clients/cmdline/notify.c (working copy)
> @@ -33,27 +33,12 @@
>
> #include "svn_private_config.h"
>
> -
> -/* Baton for notify and friends. */
> -struct notify_baton
> -{
> - svn_boolean_t received_some_change;
> - svn_boolean_t is_checkout;
> - svn_boolean_t is_export;
> - svn_boolean_t suppress_final_line;
> - svn_boolean_t sent_first_txdelta;
> - svn_boolean_t in_external;
> - svn_boolean_t had_print_error; /* Used to not keep printing error messages
> - when we've already had one print error. */
> -};
By the way, the reason I'm not complaining about the fact that most of
the individual fields don't (didn't) have descriptions is that their
names make their purposes and semantics obvious. But my feeling is,
if the name wasn't enough -- and someone obviously felt it wasn't in
the case of 'had_print_error' -- then the very first thing to describe
is the semantics, because that's what someone debugging a problem woud
need to know most urgently.
> /* This implements `svn_wc_notify_func2_t'.
> * NOTE: This function can't fail, so we just ignore any print errors. */
> static void
> notify (void *baton, const svn_wc_notify_t *n, apr_pool_t *pool)
> {
> - struct notify_baton *nb = baton;
> + struct svn_cl__notify_baton *nb = baton;
> char statchar_buf[5] = " ";
> const char *path_local;
> svn_error_t *err;
> @@ -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;
<nod> Okay, I see the logic there. It makes me think that
'extern_path_prefix_length' is perhaps too implementation-oriented a
name, and that 'extern_path_directory_length' (along with detailed
documentation in the structure, of course) might be better.
Formatting nit: space before paren in funcall: "strlen (...)".
And, obviously, this is one of the places where you should point out
that the same message is printed in status-cmd.c:print_status().
It's a problem that you're calculating the length based on a path in
native encoding, but then later in print_status() you're printing the
path as UTF8. See my comments later on that.
> case svn_wc_notify_status_completed:
> @@ -391,7 +391,7 @@
> svn_boolean_t suppress_final_line,
> apr_pool_t *pool)
> {
> - struct notify_baton *nb = apr_palloc (pool, sizeof (*nb));
> + struct svn_cl__notify_baton *nb = apr_palloc (pool, sizeof (*nb));
>
> nb->received_some_change = FALSE;
> nb->sent_first_txdelta = FALSE;
> @@ -400,6 +400,8 @@
> nb->suppress_final_line = suppress_final_line;
> nb->in_external = FALSE;
> nb->had_print_error = FALSE;
> + nb->extern_path_prefix_length = 0;
> + nb->sb = NULL;
>
> *notify_func_p = notify;
> *notify_baton_p = nb;
> Index: subversion/clients/cmdline/status-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/status-cmd.c (revision 15888)
> +++ subversion/clients/cmdline/status-cmd.c (working copy)
> @@ -22,6 +22,7 @@
>
> /*** Includes. ***/
>
> +#include "svn_cmdline.h"
> #include "svn_wc.h"
> #include "svn_client.h"
> #include "svn_error.h"
> @@ -32,26 +33,8 @@
>
> #include "svn_private_config.h"
>
> -
> -
> /*** Code. ***/
>
> -struct status_baton
> -{
> - /* These fields all correspond to the ones in the
> - svn_cl__print_status() interface. */
> - svn_boolean_t detailed;
> - svn_boolean_t show_last_committed;
> - svn_boolean_t skip_unrecognized;
> - svn_boolean_t repos_locks;
> - apr_pool_t *pool;
> -
> - svn_boolean_t had_print_error; /* To avoid printing lots of errors if we get
> - errors while printing to stdout */
> - svn_boolean_t xml_mode;
> -};
> -
> -
> /* Prints XML target element with path attribute TARGET, using POOL for
> temporary allocations. */
> static svn_error_t *
> @@ -119,9 +102,17 @@
> const char *path,
> svn_wc_status2_t *status)
> {
> - struct status_baton *sb = baton;
> + struct svn_cl__status_baton *sb = baton;
> svn_error_t *err;
> -
> +
> + if (sb->nb->extern_path_prefix_length > 0)
> + {
> + 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));
> + sb->nb->extern_path_prefix_length = 0;
> + }
> +
Same point about mentioning that this message is also printed in
notify.c:notify(), of course.
Somehow it bugs me that we have to dup part of the string every time
to print this out. But I agree it would be over-optimization to keep
an svn_stringbuf_t in the structure, solely for the purpose of
avoiding these little pool allocations.
Someday, someone's going to show up on the list asking why they're
running out of memory, and it's going to turn out that they have
65,000,000 svn:externals properties set, and we're going to feel dumb.
Oh well.
One thing that would be helpful, as long as you're moving status_baton
out to svn_cl.h, is to document its 'pool' field. Specifically,
whether that gets cleared at certain points during a status run, or
not. It's a great example of why it's important for the semantics of
every field to be documented: if the pool parameter had been
adequately documented in the first place, I wouldn't be sitting here
wondering whether someone might run out of memory someday... :-)
Inspection of the code reveals that, unfortunately, sb->pool is never
cleared right now. On the other hand, it also looks like
print_status() could clear it without harm. That's the only place the
pool is used, and it's used only for ephemeral function calls anyway.
Since your change adds more allocation burden to that pool, I think it
would be appropriate to have a pool clear be part of this change.
One problem in the current code is that while print_status()
implements the 'svn_wc_status_func2_t' interface, it does not say so
explicitly. We should always be saying what interfaces we implement;
in most of the code, we live up to this ideal, but apparently we
forgot in this case.
So the comment above print_status() should really read:
/* A callback function for printing STATUS for PATH.
This implements the 'svn_wc_status_func2_t' interface. */
Why am I bothering to say this, when it's not part of your change?
Well...
print_status() just calls svn_cl__print_status_xml() and
svn_cl__print_status() to do the real work. Both of those functions
take their 'path' arguments in UTF8, as does print_status() itself,
since it implements a public interface and Subversion public
interfaces always take paths in UTF8 unless otherwise stated.
So, you can't just print out 'path'; you have to convert it to native
encoding and print that instead. And this solves the problem that
back in notify.c, you calculate 'extern_path_prefix_length' based on
'path_local', because now the calculation will match what's being
printed later.
Do remember to document in the structure that
'extern_path_prefix_length' is the length in native encoding, not in
UTF8 as might be normally expected.
> if (sb->xml_mode)
> err = svn_cl__print_status_xml (path, status, sb->pool);
> else
> @@ -156,7 +147,7 @@
> apr_pool_t * subpool;
> int i;
> svn_opt_revision_t rev;
> - struct status_baton sb;
> + struct svn_cl__status_baton sb;
> svn_revnum_t repos_rev = SVN_INVALID_REVNUM;
>
> SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
> @@ -169,7 +160,10 @@
> if (! opt_state->xml)
> svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
> FALSE, FALSE, pool);
> -
> + struct svn_cl__notify_baton *nb = ctx->notify_baton2;
> + sb.nb = nb;
> + nb->sb = &sb;
> +
> /* Add "." if user passed 0 arguments */
> svn_opt_push_implicit_dot_target(targets, pool);
We write C the old-fashioned way around here -- no declarations after
code :-). The 'svn_cl__notify_baton *nb' should be up in the
declaration block at the beginning of the function.
Have you verified that the change passes 'make check', by the way?
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by Scott Palmer <sc...@2connected.org>.
I'm working late anyway, so I snuck in this quick update to my patch...
Scott
[[[
Fix issue #1935: svn status too verbose with svn:externals definitions
Unless the verbose option (-v) is specified, the message "Performing
status on external item at ..." is suppressed. When verbose and
quiet (-q) options are specified together the message will be printed
only when there will be relevant status output for that external folder.
* 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.
The above structures were needed to communicate information about
the command line options to svn_wc_notify_status_external in notify.c
and to provide a place to remember information about the message that
will be accessible should the code in status-cmd.c determine that the
message is needed when verbose and quiet options are specified together.
While I generally didn't like to tie these two structures together or
duplicate the printing of the same message in two places, it appears
to be the simplest, most straightforward method to handle issue
#1935. Other possible implementations may require API changes that
should probably be considered outside the scope of this single issue.
* subversion/clients/cmdline/notify.c
Modified processing of svn_wc_notify_status_external to
suppress output unless -v is specified (without -q). If the -q
option was specified then save enough information to regenerate the
message from status_cmd.c
* subversion/clients/cmdline/status-cmd.c
If there is status to print and -q was specified with -v, print
the svn_wc_notify_status_external message from here, since this is
the point where we know that relevant status output for the external
folder will happen. There does not appear to be a straightforward
way to eliminate printing this message from two possible locations
(here and from notify.c)
]]]
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by kf...@collab.net.
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 <sc...@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
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals
definitions
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 12 Aug 2005, Julian Foad wrote:
> > 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.
>
> I still don't like having the code to print it split between two different
> places, but leave it in for now and I'll see if I can find a way to combine
> them. (Sharing just the text of the message would be a half-way house.)
>
As a maintainer of sv.po, I can say that we have lots of places where we
duplicate text strings. Just look at the offsets in a .po file... I'm not
saying that I love duplicate, but introducing some kind of abstraction if
the only duplicated thing is a string might be over-kill.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals
definitions
Posted by Julian Foad <ju...@btopenworld.com>.
Scott Palmer wrote:
>
> 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.
[...]
> 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.
To begin with I was thinking that that behaviour would be a funny special case,
inconsistent with the individual meanings of "-q" and "-v", but now I see that
it is OK: the new meaning of "-q" with respect to externals will be something
like "Don't print 'X', and don't print 'Performing...' for a directory if there
are no statuses to report in it."
I have now come to like this extra behaviour.
[...]
> 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.
I still don't like having the code to print it split between two different
places, but leave it in for now and I'll see if I can find a way to combine
them. (Sharing just the text of the message would be a half-way house.)
I look forward to your patch.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals definitions
Posted by Scott Palmer <sc...@2connected.org>.
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
Re: [PATCH] Issue #1935: svn status too verbose with svn:externals
definitions
Posted by Julian Foad <ju...@btopenworld.com>.
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