You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by VK Sameer <sa...@collab.net> on 2004/12/14 03:45:19 UTC
[PATCH] issue 1954 - v3
Hello,
Sorry for not replying to the review comments. I think all of them have
been accepted in this patch.
Thanks
Sameer
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2004-12-15 at 07:25, Julian Foad wrote:
> VK Sameer wrote:
> >>>+ return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> >>>+ "Invalid control char. '%x' in path '%s'",
> >>>+ *c,
> >>>+ svn_path_local_style (path, pool));
> [...]
> > Done. It was there initially - got removed during vain attempts at
> > staying within 72 cols.
>
> A width of 72 columns is good for e-mail prose to allow for quoting in replies,
> but up to 79 or maybe 80 is fine for source code.
OK.
> Here are two other ways used in Subversion source code to reduce the column count.
>
> Use the fact that adjacent strings are combined by the compiler, e.g.:
>
> return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> "Invalid control char. '%x' "
> "in path '%s'",
> *c,
> svn_path_local_style (path, pool));
>
> Start the argument list on a new line (indented by 2 spaces):
>
> return svn_error_createf
> (SVN_ERR_FS_PATH_SYNTAX, NULL,
> "Invalid control char. '%x' in path '%s'",
> *c,
> svn_path_local_style (path, pool));
This looks much nicer, to my admittedly biased eyes. I used to work with
somebody who coded like this:
f1 (
arg1,
f2 (
arg21,
arg22,
f3 (
arg31,
arg32
)
),
...,
argN
);
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Julian Foad <ju...@btopenworld.com>.
VK Sameer wrote:
>>>+ return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
>>>+ "Invalid control char. '%x' in path '%s'",
>>>+ *c,
>>>+ svn_path_local_style (path, pool));
[...]
> Done. It was there initially - got removed during vain attempts at
> staying within 72 cols.
A width of 72 columns is good for e-mail prose to allow for quoting in replies,
but up to 79 or maybe 80 is fine for source code.
Here are two other ways used in Subversion source code to reduce the column count.
Use the fact that adjacent strings are combined by the compiler, e.g.:
return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
"Invalid control char. '%x' "
"in path '%s'",
*c,
svn_path_local_style (path, pool));
Start the argument list on a new line (indented by 2 spaces):
return svn_error_createf
(SVN_ERR_FS_PATH_SYNTAX, NULL,
"Invalid control char. '%x' in path '%s'",
*c,
svn_path_local_style (path, pool));
(Note that there is no comma after the "Invalid... '%x' " string here.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-14 at 18:02, Julian Foad wrote:
> Also, write "@c" before each symbol (other function names, constants like
> SVN_NO_ERROR, etc.) to mark it for cross-reference.
Added in the latest version of the patch sent to dev@.
> > + return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> > + "Invalid control char. '%x' in path '%s'",
> > + *c,
> > + svn_path_local_style (path, pool));
>
> I recommend '0x%02X' for printing in hexadecimal, to give '0x0A' for a
> line-feed, rather than just 'A'. At least zero-pad it and give some indication
> that it is hex rather than, say, ASCII mnemonic.
Done. It was there initially - got removed during vain attempts at
staying within 72 cols.
Thanks
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Tue, 14 Dec 2004, VK Sameer wrote:
> + * returns SVN_NO_ERROR if valid
> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>
> Not sure how this will be formatted in the doxygen output. I assume it
> won't take the line breaks into account, but I haven't checked.
Also, write "@c" before each symbol (other function names, constants like
SVN_NO_ERROR, etc.) to mark it for cross-reference.
> + return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid control char. '%x' in path '%s'",
> + *c,
> + svn_path_local_style (path, pool));
I recommend '0x%02X' for printing in hexadecimal, to give '0x0A' for a
line-feed, rather than just 'A'. At least zero-pad it and give some indication
that it is hex rather than, say, ASCII mnemonic.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Thu, 2004-12-23 at 02:14, brane@xbc.nu wrote:
> Julian Foad wrote:
>
> > Branko Čibej wrote:
> >
> >> VK Sameer wrote:
> >>
> >>>> + * returns SVN_NO_ERROR if valid
> >>>> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
> >>>
> >>>
> >> And while you're at it, use @return, so that Doxygen knows what that
> >> paragraph is about.
> >
> >
> > Really? I don't see us doing that anywhere else.
>
> We don't usually have a "returns" paragraph all by itself, either (well,
> at least not one without proper case and punctuation). Unless this bit
> gets rewritten that way, then @return is correct, IMHO.
The latest version of the patch is v6 :), but that part was rewritten in
v4.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by br...@xbc.nu.
Julian Foad wrote:
> Branko Čibej wrote:
>
>> VK Sameer wrote:
>>
>>>> + * returns SVN_NO_ERROR if valid
>>>> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>>>
>>>
>> And while you're at it, use @return, so that Doxygen knows what that
>> paragraph is about.
>
>
> Really? I don't see us doing that anywhere else.
We don't usually have a "returns" paragraph all by itself, either (well,
at least not one without proper case and punctuation). Unless this bit
gets rewritten that way, then @return is correct, IMHO.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> VK Sameer wrote:
>>> + * returns SVN_NO_ERROR if valid
>>> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>>
> And while you're at it, use @return, so that Doxygen knows what that
> paragraph is about.
Really? I don't see us doing that anywhere else.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Branko Čibej <br...@xbc.nu>.
VK Sameer wrote:
>On Tue, 2004-12-14 at 14:35, Peter N. Lundblad wrote:
>
>
>>On Tue, 14 Dec 2004, VK Sameer wrote:
>>
>>In addition to Karl's review:
>>
>>+ * returns SVN_NO_ERROR if valid
>>+ * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>>
>>
>>Not sure how this will be formatted in the doxygen output. I assume it
>>won't take the line breaks into account, but I haven't checked.
>>
>>
>
>Yes, you're right, it won't. Changed to one sentence.
>
>
And while you're at it, use @return, so that Doxygen knows what that
paragraph is about.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 14 Dec 2004, VK Sameer wrote:
> On Tue, 2004-12-14 at 14:35, Peter N. Lundblad wrote:
> > On Tue, 14 Dec 2004, VK Sameer wrote:
> >
> > +
> > +svn_error_t*
> >
> >
> > Space after *.
>
> Hmm, didn't see it, unless you meant before? Did add that.
>
Good. Maybe better to read my mind than my mail. ;)
> > Index: subversion/libsvn_ra_local/ra_plugin.c
> > ===================================================================
> > --- subversion/libsvn_ra_local/ra_plugin.c (revision 12257)
> > +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> > @@ -688,6 +688,9 @@
> > svn_fs_root_t *root;
> > const char *abs_path = sbaton->fs_path;
> >
> > + if (path)
> > + SVN_ERR (svn_path_check_valid (path, pool));
> > +
> >
> >
> > svn_ra_plugin_t->check_path, which this implements, takes a path, checks
> > if it exists (in a particular revision) and what kind it is. So you don't
> > need the above.
>
> OK, thanks for double-checking the call sites.
>
As I might have said before, I'd like you to put these checks in the
servers as well, so some other client doesn't get invalid paths in by
mistake. That can be a different patch if you want.
> > I see that you are adding the checks to both libsvn_wc and libsvn_client.
> > Isn't it enough to add them to libsvn_wc (except in the import code)? For
> > example, wc_to_wc_copy it seems redundant. Or maybe you are plugging some
> > hole somewhere else?
>
> No, I haven't found a hole. Initially, the plan was to put the checks
> only in the libsvn_client layer to catch errors as close to the user as
> possible. Since I hadn't reviewed all the paths carefully, I added
> libsvn_wc based on Jon's patch. If libsvn_wc is a better layer in terms
> of greater coverage in fewer call sites, I can remove the checks from
> the libsvn_client layer.
>
I'm not sure what would yield the least number of call sites. YOu should
be consistent, though. PUtting most checks in libsvn_wc would make sure
other users of that library (if there are any) won't add such paths by
mistake.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-14 at 14:35, Peter N. Lundblad wrote:
> On Tue, 14 Dec 2004, VK Sameer wrote:
>
> In addition to Karl's review:
>
> + * returns SVN_NO_ERROR if valid
> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>
>
> Not sure how this will be formatted in the doxygen output. I assume it
> won't take the line breaks into account, but I haven't checked.
Yes, you're right, it won't. Changed to one sentence.
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 12257)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> #include "svn_utf.h"
> #include "svn_io.h" /* for svn_io_stat() */
> +#include "svn_ctype.h"
>
>
> /* The canonical empty path. Can this be changed? Well, change the empty
> @@ -1248,3 +1249,22 @@
> else
> return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> }
> +
> +svn_error_t*
>
>
> Space after *.
Hmm, didn't see it, unless you meant before? Did add that.
> +svn_path_check_valid (const char *path, apr_pool_t *pool)
> +{
> + const char *c;
> +
> + for (c = path; *c; c++)
> + {
> + if (svn_ctype_iscntrl(*c))
> + {
> + return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid control char. '%x' in path '%s'",
> + *c,
> + svn_path_local_style (path, pool));
>
>
> Since 1.1, svn is internationalized, so you need to put strings that
> should be translated into the users' native language inside _() macro
> calls. + }
OK.
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 12257)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -688,6 +688,9 @@
> svn_fs_root_t *root;
> const char *abs_path = sbaton->fs_path;
>
> + if (path)
> + SVN_ERR (svn_path_check_valid (path, pool));
> +
>
>
> svn_ra_plugin_t->check_path, which this implements, takes a path, checks
> if it exists (in a particular revision) and what kind it is. So you don't
> need the above.
OK, thanks for double-checking the call sites.
> I see that you are adding the checks to both libsvn_wc and libsvn_client.
> Isn't it enough to add them to libsvn_wc (except in the import code)? For
> example, wc_to_wc_copy it seems redundant. Or maybe you are plugging some
> hole somewhere else?
No, I haven't found a hole. Initially, the plan was to put the checks
only in the libsvn_client layer to catch errors as close to the user as
possible. Since I hadn't reviewed all the paths carefully, I added
libsvn_wc based on Jon's patch. If libsvn_wc is a better layer in terms
of greater coverage in fewer call sites, I can remove the checks from
the libsvn_client layer.
Thanks for the review.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 14 Dec 2004, VK Sameer wrote:
In addition to Karl's review:
Index: subversion/include/svn_path.h
===================================================================
--- subversion/include/svn_path.h (revision 12257)
+++ subversion/include/svn_path.h (working copy)
@@ -356,6 +356,26 @@
const char *path2,
apr_pool_t *pool);
+/**
+ * @since New in 1.2.
+ *
+ * Check whether @a path is a valid Subversion path.
+ * @a pool is used for temporary allocation.
+ *
+ * A valid Subversion pathname is a UTF-8 string without control
+ * characters. "Valid" means Subversion can store the pathname in
+ * a repository. There may be other OS-specific limitations on what
+ * paths can be represented in a working copy.
+ *
+ * returns SVN_NO_ERROR if valid
+ * returns SVN_ERR_FS_PATH_SYNTAX if invalid
Not sure how this will be formatted in the doxygen output. I assume it
won't take the line breaks into account, but I haven't checked.
Index: subversion/libsvn_subr/path.c
===================================================================
--- subversion/libsvn_subr/path.c (revision 12257)
+++ subversion/libsvn_subr/path.c (working copy)
@@ -29,6 +29,7 @@
#include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
#include "svn_utf.h"
#include "svn_io.h" /* for svn_io_stat() */
+#include "svn_ctype.h"
/* The canonical empty path. Can this be changed? Well, change the empty
@@ -1248,3 +1249,22 @@
else
return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
}
+
+svn_error_t*
Space after *.
+svn_path_check_valid (const char *path, apr_pool_t *pool)
+{
+ const char *c;
+
+ for (c = path; *c; c++)
+ {
+ if (svn_ctype_iscntrl(*c))
+ {
+ return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
+ "Invalid control char. '%x' in path '%s'",
+ *c,
+ svn_path_local_style (path, pool));
Since 1.1, svn is internationalized, so you need to put strings that
should be translated into the users' native language inside _() macro
calls. + }
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 12257)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -688,6 +688,9 @@
svn_fs_root_t *root;
const char *abs_path = sbaton->fs_path;
+ if (path)
+ SVN_ERR (svn_path_check_valid (path, pool));
+
svn_ra_plugin_t->check_path, which this implements, takes a path, checks
if it exists (in a particular revision) and what kind it is. So you don't
need the above.
I see that you are adding the checks to both libsvn_wc and libsvn_client.
Isn't it enough to add them to libsvn_wc (except in the import code)? For
example, wc_to_wc_copy it seems redundant. Or maybe you are plugging some
hole somewhere else?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2004-12-15 at 17:23, Julian Foad wrote:
> VK Sameer wrote:
> > On Wed, 2004-12-15 at 06:41, Garrett Rooney wrote:
> >
> >>Pools are hierarchical, if you don't destroy the subpool it will get
> >>cleaned up when its parent pool is eventually destroyed.
> >
> > OK, that's good to know. I guess I don't see the point of relying on
> > that when I can just re-use an existing pool.
>
> Sorry, I don't follow that sentence. Do you mean "... when I can use a
> sub-pool that is explicitly destroyed within this function"?
No, I meant use 'pool' instead of 'subpool' which gets destroyed at
'pool'-destruction time.
> Note that your new statement is not the only error return from this function.
> There is no point in making your new statement destroy the sub-pool and leaving
> all of the existing error returns as they are. Therefore there is no need for
> this change to be part of this patch: it is a separate change.
Yes, that was the major point I missed. Karl and Fitz mentioned that in
IRC as well. Given that, I'll put the initialization back to how it was.
> This is a reasonable way of handling the situation, though a little fragile. A
> more robust way is to wrap the function in a subpool-handler function that
> creates and destroys the subpool, but that usually feels too cumbersome. I
> think the "goto" method is the most appropriate here.
I've still got the "don't use goto" hangup, though it's slowly dying
away :). It makes perfect sense in this scenario, though.
Thanks for the detailed explanation.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Julian Foad <ju...@btopenworld.com>.
VK Sameer wrote:
> On Wed, 2004-12-15 at 06:41, Garrett Rooney wrote:
>>>Doesn't svn_destroy_pool (subpool) have to be called to free the
>>>subpool? If so, I don't see how subpool memory would get freed if
>>>SVN_ERR causes a jump out of add_or_open_file().
>>
>>Pools are hierarchical, if you don't destroy the subpool it will get
>>cleaned up when its parent pool is eventually destroyed.
>
> OK, that's good to know. I guess I don't see the point of relying on
> that when I can just re-use an existing pool.
Sorry, I don't follow that sentence. Do you mean "... when I can use a
sub-pool that is explicitly destroyed within this function"?
> Potentially, this function
> could get called multiple times _and_ SVN_ERR could fail all those
> times. In which case, there are multiple subpools to be destroyed.
Only if the given pool has not been cleared or destroyed before it is called
again. It is the caller's duty to clean up between multiple calls, and we can
rely on it to do so. I think we can assume that the caller will clean up
quickly after a failed call, in contrast to a successful call where the result
might be in use for a long time before it is finished with.
Having said that, I wouldn't object to add_or_open_file destroying its subpool
before every error return, for consistency with the non-error case.
Note that your new statement is not the only error return from this function.
There is no point in making your new statement destroy the sub-pool and leaving
all of the existing error returns as they are. Therefore there is no need for
this change to be part of this patch: it is a separate change.
> Another alternative is to change from SVN_ERR to this (untested):
>
> svn_error_t *err;
> if (err = svn_path_check_valid (path, subpool))
> {
> svn_pool_destroy (subpool);
> return err;
> }
>
> It doesn't seem very appealing.
This idiom is used in some places. When it occurs multiple times within a
function, as it would do here, it is replaced with:
svn_error_t *err;
if (err = svn_path_check_valid (path, subpool))
goto done;
if (err = ...)
goto done;
...
done:
svn_pool_destroy (subpool);
return err;
}
This is a reasonable way of handling the situation, though a little fragile. A
more robust way is to wrap the function in a subpool-handler function that
creates and destroys the subpool, but that usually feels too cumbersome. I
think the "goto" method is the most appropriate here.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2004-12-15 at 06:41, Garrett Rooney wrote:
> > Doesn't svn_destroy_pool (subpool) have to be called to free the
> > subpool? If so, I don't see how subpool memory would get freed if
> > SVN_ERR causes a jump out of add_or_open_file().
>
> Pools are hierarchical, if you don't destroy the subpool it will get
> cleaned up when its parent pool is eventually destroyed.
OK, that's good to know. I guess I don't see the point of relying on
that when I can just re-use an existing pool. Potentially, this function
could get called multiple times _and_ SVN_ERR could fail all those
times. In which case, there are multiple subpools to be destroyed.
Another alternative is to change from SVN_ERR to this (untested):
svn_error_t *err;
if (err = svn_path_check_valid (path, subpool))
{
svn_pool_destroy (subpool);
return err;
}
It doesn't seem very appealing.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
> Doesn't svn_destroy_pool (subpool) have to be called to free the
> subpool? If so, I don't see how subpool memory would get freed if
> SVN_ERR causes a jump out of add_or_open_file().
Pools are hierarchical, if you don't destroy the subpool it will get
cleaned up when its parent pool is eventually destroyed.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-14 at 19:25, Julian Foad wrote:
> VK Sameer wrote:
> >>>--- subversion/libsvn_wc/update_editor.c (revision 12257)
> >>>+++ subversion/libsvn_wc/update_editor.c (working copy)
> >>>@@ -1431,10 +1432,12 @@
> >>> const svn_wc_entry_t *entry;
> >>> svn_node_kind_t kind;
> >>> svn_wc_adm_access_t *adm_access;
> >>>+ apr_pool_t *subpool;
> >>>
> >>> /* the file_pool can stick around for a *long* time, so we want to use
> >>> a subpool for any temporary allocations. */
> >>>- apr_pool_t *subpool = svn_pool_create (pool);
> >>>+ subpool = svn_pool_create (pool);
> >>>+ SVN_ERR (svn_path_check_valid (path, subpool));
> >
> > Sorry, I meant to use pool instead. The thought was that this would
> > avoid increasing memory usage just before erroring out. Updated in
> > patch.
>
> It is not appropriate to optimise for reduced memory usage in the error
> condition, since
> + the error condition is the rare case;
> + it was only a small allocation, and one which would have been made in the
> common case anyway;
> + the pool memory will get freed soon enough, regardless of which way you do it.
Doesn't svn_destroy_pool (subpool) have to be called to free the
subpool? If so, I don't see how subpool memory would get freed if
SVN_ERR causes a jump out of add_or_open_file().
> The existing comment explains why we want to use a sub-pool: you saved memory
> in the error condition at the expense of potentially using extra memory in the
> file's main pool for every successful open, and that pool may persist for a
> long time.
True, I didn't consider that.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by Julian Foad <ju...@btopenworld.com>.
VK Sameer wrote:
>>>--- subversion/libsvn_wc/update_editor.c (revision 12257)
>>>+++ subversion/libsvn_wc/update_editor.c (working copy)
>>>@@ -1431,10 +1432,12 @@
>>> const svn_wc_entry_t *entry;
>>> svn_node_kind_t kind;
>>> svn_wc_adm_access_t *adm_access;
>>>+ apr_pool_t *subpool;
>>>
>>> /* the file_pool can stick around for a *long* time, so we want to use
>>> a subpool for any temporary allocations. */
>>>- apr_pool_t *subpool = svn_pool_create (pool);
>>>+ subpool = svn_pool_create (pool);
>>>+ SVN_ERR (svn_path_check_valid (path, subpool));
>
> Sorry, I meant to use pool instead. The thought was that this would
> avoid increasing memory usage just before erroring out. Updated in
> patch.
It is not appropriate to optimise for reduced memory usage in the error
condition, since
+ the error condition is the rare case;
+ it was only a small allocation, and one which would have been made in the
common case anyway;
+ the pool memory will get freed soon enough, regardless of which way you do it.
The existing comment explains why we want to use a sub-pool: you saved memory
in the error condition at the expense of potentially using extra memory in the
file's main pool for every successful open, and that pool may persist for a
long time. I say "potentially" because your current implementation of
svn_path_check_valid doesn't actually use pool unless it errors, but its
interface doesn't guarantee this and a subsequent version of it might well do so.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-14 at 12:10, kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
>
> I guess the warning doesn't need to be so strict anymore, since the
> patch is getting closer and closer :-).
OK, removed.
> > * subversion/tests/clients/cmdline/commit_tests.py
> > (tab_test): New test.
> > (test_list): Run it XFail.
> > (commit_uri_unsafe): Removed tab test
>
> For the last line, say "tab test portion moved to new test above" or
> something, to make it clear what's going on here.
OK.
> > +svn_boolean_t svn_path_is_valid (const char *path,
> > + apr_pool_t *pool,
> > + svn_boolean_t check_utf8);
> > +
>
> Oops, I think that just slipped in there :-).
Yup, gone.
> Although normally we assume UTF8 strings as input, here it might be
> good to explicitly state that the string must be UTF8. Because this
> is a validation function, and people might otherwise assume that it
> will validate that the string is valid UTF8, among other things.
OK.
> > --- subversion/libsvn_wc/update_editor.c (revision 12257)
> > +++ subversion/libsvn_wc/update_editor.c (working copy)
> > @@ -1014,6 +1014,7 @@
> > || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
> > abort();
> >
> > + SVN_ERR (svn_path_check_valid (db->path, db->pool));
> > /* There should be nothing with this name. */
> > SVN_ERR (svn_io_check_path (db->path, &kind, db->pool));
> > if (kind != svn_node_none)
> > @@ -1431,10 +1432,12 @@
> > const svn_wc_entry_t *entry;
> > svn_node_kind_t kind;
> > svn_wc_adm_access_t *adm_access;
> > + apr_pool_t *subpool;
> >
> > /* the file_pool can stick around for a *long* time, so we want to use
> > a subpool for any temporary allocations. */
> > - apr_pool_t *subpool = svn_pool_create (pool);
> > + subpool = svn_pool_create (pool);
> > + SVN_ERR (svn_path_check_valid (path, subpool));
Sorry, I meant to use pool instead. The thought was that this would
avoid increasing memory usage just before erroring out. Updated in
patch.
> Everything else looks good. If any of the call sites look like they
> should be doing UTF8 testing as well, then this would be a good time
> to add that.
OK, will do.
> > +def tab_test(sbox):
> > + "tab testing"
> > +
> > + # ripped out of commit_uri_unsafe - currently must be used with an XFail
> > + # add test for directory
>
> Why must it be used with XFail?
Only because I haven't figured out the testing code yet :) The easiest
way to test was to check whether a commit of a pathname with tab failed.
I'll fix that before my final version.
> You don't need to mention the source of the code, btw (unless the
> source has some special pertinence here?). Everything is
> cut-and-pasted from somewhere else.
It was more of a reminder for myself. But I'll take it out.
> So, is there any way we can test that a forbidden path gets the right
> error? Or would that make the test suite too unportable?
I should think so, just have to spend some time with the tests.
> If you moved the code, why leave commented-out bits? Just remove them
> entirely. It's all under version control anyway :-).
Yeah, just sloppy with test code :) Done.
> Why the commented-out "#tab_test" there?
Removed.
Thanks again for the review.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v3
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> Sorry for not replying to the review comments. I think all of them have
> been accepted in this patch.
The patch is the reply, no problem!
> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
>
> ####################################################
> ### ###
> ### Note: This is a draft patch for review only, ###
> ### please do not commit it. ###
> ### ###
> ####################################################
I guess the warning doesn't need to be so strict anymore, since the
patch is getting closer and closer :-).
> * subversion/include/svn_path.h
> (svn_path_check_valid): New function.
>
> * subversion/libsvn_subr/path.c
> (svn_path_check_valid): New function.
>
> * subversion/libsvn_wc/copy.c
> (copy_file_administratively): Call svn_path_check_valid().
> (svn_wc_copy): Same.
>
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_add): Same.
>
> * subversion/libsvn_wc/update_editor.c
> (add_directory, add_or_open_file): Same.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__do_check_path): Same.
>
> * subversion/libsvn_client/copy.c
> (wc_to_wc_copy): Same.
>
> * subversion/libsvn_client/add.c
> (add_dir_recursive, add_file): Same.
>
> * subversion/libsvn_client/commit.c
> (import_file, import_dir): Same.
>
> * subversion/tests/clients/cmdline/commit_tests.py
> (tab_test): New test.
> (test_list): Run it XFail.
> (commit_uri_unsafe): Removed tab test
For the last line, say "tab test portion moved to new test above" or
something, to make it clear what's going on here.
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 12257)
> +++ subversion/include/svn_path.h (working copy)
> @@ -356,6 +356,26 @@
> const char *path2,
> apr_pool_t *pool);
>
> +svn_boolean_t svn_path_is_valid (const char *path,
> + apr_pool_t *pool,
> + svn_boolean_t check_utf8);
> +
Oops, I think that just slipped in there :-).
> +/**
> + * @since New in 1.2.
> + *
> + * Check whether @a path is a valid Subversion path.
> + * @a pool is used for temporary allocation.
> + *
> + * A valid Subversion pathname is a UTF-8 string without control
> + * characters. "Valid" means Subversion can store the pathname in
> + * a repository. There may be other OS-specific limitations on what
> + * paths can be represented in a working copy.
> + *
> + * returns SVN_NO_ERROR if valid
> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
> + */
> +svn_error_t *svn_path_check_valid (const char *path, apr_pool_t *pool);
> +
Looks great!
Although normally we assume UTF8 strings as input, here it might be
good to explicitly state that the string must be UTF8. Because this
is a validation function, and people might otherwise assume that it
will validate that the string is valid UTF8, among other things.
>
> /** URI/URL stuff
> *
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 12257)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -141,6 +141,7 @@
> /* The 'dst_path' is simply dst_parent/dst_basename */
> const char *dst_path
> = svn_path_join (svn_wc_adm_access_path (dst_parent), dst_basename, pool);
> + SVN_ERR (svn_path_check_valid (dst_path, pool));
>
> /* Sanity check: if dst file exists already, don't allow overwrite. */
> SVN_ERR (svn_io_check_path (dst_path, &dst_kind, pool));
> @@ -470,6 +471,7 @@
> SVN_ERR (svn_wc_adm_probe_open2 (&adm_access, NULL, src_path, FALSE, -1,
> pool));
>
> + SVN_ERR (svn_path_check_valid (src_path, pool));
> SVN_ERR (svn_io_check_path (src_path, &src_kind, pool));
>
> if (src_kind == svn_node_file)
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 12257)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -879,6 +879,8 @@
> svn_node_kind_t kind;
> apr_uint32_t modify_flags = 0;
> svn_wc_adm_access_t *adm_access;
> +
> + SVN_ERR (svn_path_check_valid (path, pool));
>
> /* Make sure something's there. */
> SVN_ERR (svn_io_check_path (path, &kind, pool));
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 12257)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -1014,6 +1014,7 @@
> || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
> abort();
>
> + SVN_ERR (svn_path_check_valid (db->path, db->pool));
> /* There should be nothing with this name. */
> SVN_ERR (svn_io_check_path (db->path, &kind, db->pool));
> if (kind != svn_node_none)
> @@ -1431,10 +1432,12 @@
> const svn_wc_entry_t *entry;
> svn_node_kind_t kind;
> svn_wc_adm_access_t *adm_access;
> + apr_pool_t *subpool;
>
> /* the file_pool can stick around for a *long* time, so we want to use
> a subpool for any temporary allocations. */
> - apr_pool_t *subpool = svn_pool_create (pool);
> + subpool = svn_pool_create (pool);
> + SVN_ERR (svn_path_check_valid (path, subpool));
The subpool initialization changes are extraneous to this patch, and
anyway not mentioned in the log message. I'd say leave it as is, the
code was okay there anyway.
> [...]
Everything else looks good. If any of the call sites look like they
should be doing UTF8 testing as well, then this would be a good time
to add that.
> +def tab_test(sbox):
> + "tab testing"
> +
> + # ripped out of commit_uri_unsafe - currently must be used with an XFail
> + # add test for directory
Why must it be used with XFail?
You don't need to mention the source of the code, btw (unless the
source has some special pertinence here?). Everything is
cut-and-pasted from somewhere else.
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + if svntest.main.windows or sys.platform == 'cygwin':
> + tab_name = 'tab-path'
> + else:
> + tab_name = "tab\tpath"
> +
> + tab_path = os.path.join(wc_dir, 'A', tab_name)
> + svntest.main.file_append(tab_path, "This path has a tab in it.")
> + svntest.main.run_svn(None, 'add', '--non-recursive', tab_path)
> +
> + expected_output = svntest.wc.State(wc_dir, {
> + 'A/' + tab_name : Item(verb='Adding'),
> + })
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
> + # Items in the status list are all at rev 1
> + expected_status.tweak(wc_rev=1)
> +
> + # Items in our add list will be at rev 2
> + for item in expected_output.desc.keys():
> + expected_status.add({ item : Item(wc_rev=2, repos_rev=2, status=' ') })
> +
> + svntest.actions.run_and_verify_commit (wc_dir,
> + expected_output,
> + expected_status,
> + None, None, None, None, None,
> + wc_dir)
So, is there any way we can test that a forbidden path gets the right
error? Or would that make the test suite too unportable?
> +#----------------------------------------------------------------------
> +
> def hook_test(sbox):
> "hook testing"
>
> @@ -1069,17 +1107,17 @@
> if svntest.main.windows or sys.platform == 'cygwin':
> angle_name = '_angle_'
> nasty_name = '#![]{}()__%'
> - tab_name = 'tab-path'
> + #tab_name = 'tab-path'
> else:
> angle_name = '<angle>'
> nasty_name = '#![]{}()<>%'
> - tab_name = "tab\tpath"
> + #tab_name = "tab\tpath"
>
> # Make some convenient paths.
> hash_dir = os.path.join(wc_dir, '#hash#')
> nasty_dir = os.path.join(wc_dir, nasty_name)
> space_path = os.path.join(wc_dir, 'A', 'D', 'space path')
> - tab_path = os.path.join(wc_dir, 'A', 'D', 'G', tab_name)
> + #tab_path = os.path.join(wc_dir, 'A', 'D', 'G', tab_name)
> bang_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bang!')
> bracket_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra[ket')
> brace_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra{e')
> @@ -1091,7 +1129,7 @@
> os.mkdir(hash_dir)
> os.mkdir(nasty_dir)
> svntest.main.file_append(space_path, "This path has a space in it.")
> - svntest.main.file_append(tab_path, "This path has a tab in it.")
> + #svntest.main.file_append(tab_path, "This path has a tab in it.")
> svntest.main.file_append(bang_path, "This path has a bang in it.")
> svntest.main.file_append(bracket_path, "This path has a bracket in it.")
> svntest.main.file_append(brace_path, "This path has a brace in it.")
> @@ -1103,7 +1141,7 @@
> add_list = [hash_dir,
> nasty_dir, # not xml-safe
> space_path,
> - tab_path,
> + #tab_path,
> bang_path,
> bracket_path,
> brace_path,
> @@ -1119,7 +1157,7 @@
> '#hash#' : Item(verb='Adding'),
> nasty_name : Item(verb='Adding'),
> 'A/D/space path' : Item(verb='Adding'),
> - 'A/D/G/' + tab_name : Item(verb='Adding'),
> + #'A/D/G/' + tab_name : Item(verb='Adding'),
> 'A/D/H/bang!' : Item(verb='Adding'),
> 'A/D/H/bra[ket' : Item(verb='Adding'),
> 'A/D/H/bra{e' : Item(verb='Adding'),
If you moved the code, why leave commented-out bits? Just remove them
entirely. It's all under version control anyway :-).
> @@ -1891,6 +1929,8 @@
> hudson_part_1_variation_2,
> hudson_part_2,
> hudson_part_2_1,
> + XFail(tab_test),
> + #tab_test,
> XFail(hook_test),
> merge_mixed_revisions,
> commit_uri_unsafe,
Why the commented-out "#tab_test" there?
(And again not sure why the XFail.)
Rest of the patch looks good.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org