You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <B....@competence.biz> on 2008/04/09 10:04:48 UTC
Commit notifies invalid paths on windows (Issue #3168)
Hi,
As I noted on irc yesterday I created the following issue
(I did some more research on the bug to find out whether other commands
had the same problem.. but they have not)
--
The definition of svn_wc_notify_func2_t function uses svn_wc_notify_t
which
declares
typedef struct svn_wc_notify_t {
/** Path, either absolute or relative to the current working directory
* (i.e., not relative to an anchor). */
const char *path;
But calling (at the client level in pseudo code)
svn.Commit("E:\tmp\wc",....)
when your current directory is "E:\dev\sharpsvn\src\SharpSvn.Tests"
Gives you notifies where the path is "tmp\wc\testfile.txt" which is
neither a
valid relative path nor a valid absolute path.
lgo mentioned on irc this bug is related to issue #1711.
Further testing in the SharpSvn testsuite showed the issue is triggered
on all
notifies from Commit() but is /not/ triggered from most import calls
(did not
test all variants), or any of the other commands run from the SharpSvn
testsuite (extended version of AnkhSVN's NSvn tests).
I really would like to have this issue fixed for (at least) absolute
paths in
1.5, as this breaks using the notify for updating the in memory cache in
the
new AnkhSVN. (More on this in issue #3147)
--
Looking through the code seems to tell this issue has been in subversion
since far before 1.0, but I really would like to have some solution in
1.5 (or at least the 1.5.x branch, so I can merge it in the subversion
build from the SharpSvn environment)
Bert Huijben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (1/3)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> I split the previous patch in three parts
Thank you! It's easier to handle now...
> 1. The documentation update
> 2. Move the path prefix cutting to the notify handler
> 3. Changes to commit.c that make clear how the path is cut by the
> notification handler
>
> Patch 1 and 3 contain no functional changes
Patch 1 applied in r31174. I'm now on to look at patches 2 and 3.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (1/3)
Posted by Bert Huijben <be...@qqmail.nl>.
Hi All,
I split the previous patch in three parts
1. The documentation update
2. Move the path prefix cutting to the notify handler
3. Changes to commit.c that make clear how the path is cut by the
notification handler
Patch 1 and 3 contain no functional changes
Log message for patch 1:
[[
Note when svn_wc_notify_t members where added
* subversion/include/svn_wc.h
(svn_wc_notify_t): Note versions in which new fields were added.
Patch by: Bert Huijben <b....@competence.biz>
Tweaked by: kfogel
]]
RE: [PATCH] RE: Commit strips common prefix instead of current
directory (Issue #3168)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Bert Huijben wrote on Tue, 6 May 2008 at 11:28 +0200:
> Karl Fogel wrote:
> > ### If possible, it's best to submit those other cleanups as a
> > ### separate patch. I know it's slightly more work, but it really
> > ### helps keep things simple for reviewers.
>
> If I had used two patches they would not apply cleanly if used
> separately (and once applied I merge this patch in the SharpSvn builds
> to fix some AnkhSvn issues).
> (Didn't change this for the updated patch)
>
It is okay to split a submission into two related patches that depend on
each other. Just make sure to state the dependencies clearly.
As Karl said, splitting changes into small, separate patches also help
reviewers, since small changes are easier and faster to understand.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (2/3)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Okay, after talking on irc I found an essential part of patch 2 was missing
> which I had in my working copy when testing. The missing part is the patch
> to notify.c which restores the non-absolute behavior for the CLI.
>
> I recreated the patch with the tweaks you applied and attached it to this
> message
Applied in r31211.
> To answer your other mail:
> Patch 3 should be a 100% standalone patch. Patch 3 might only need some
> further tweaking if the CLI behavior should change (which I don't expect).
I'm on to patch 3 no, yup.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (2/3)
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Karl Fogel [mailto:kfogel@red-bean.com]
> Sent: woensdag 14 mei 2008 22:19
> To: Bert Huijben
> Cc: Bert Huijben; 'Daniel Shahaf'; dev@subversion.tigris.org; 'Mark
> Phippard'
> Subject: Re: [PATCH] RE: Commit strips common prefix instead of current
> directory (Issue #3168) (2/3)
>
> "Bert Huijben" <be...@qqmail.nl> writes:
> > [[[
> > Move the creation of final display paths to the CLI notification
> > handler, to make sure notifications always receive a valid path in
> the
> > path field.
> >
> > [...]
>
> This patch causes virtually every command-line regression test to fail.
Okay, after talking on irc I found an essential part of patch 2 was missing
which I had in my working copy when testing. The missing part is the patch
to notify.c which restores the non-absolute behavior for the CLI.
I recreated the patch with the tweaks you applied and attached it to this
message
> Did you run 'make check' before posting? :-)
As I told on IRC, I ran an alternative test script which for most actions
immediately calls into libsvn_client and uses absolute paths in most cases.
Since then I updated my build environment to run the fsfs checks of
win-tests.py which pass on the new version. (I have a few local test
failures on onrelated parts; but those are the same with and without this
patch.. Probably generated by the static compilation patches used for
building SharpSvn).
(I will send another patch to fix a Windows Vista issue on running the tests
in a few days)
> I assume that it was not your goal to always produce absolute paths in
> svn output? If it was, we'd better discuss some more; I think in most
> circumstances that is not what users want. I'm trying to locate the
> mail where you described a proposed new behavior, actually...
No it was not. But the code making the paths non-absolute was missing ;)
> Below is the exact patch I tested with. It's the same as the patch you
> posted, just with a couple of documentation tweaks.
The new patch is your patch (from your mail) with the patch of
subversion/svn/notify.c (from my original patch) regenerated on trunk of a
few hours ago.
> [[[
> Move the creation of final display paths to the CLI notification
> handler, to make sure notifications always receive a valid path in the
> path field.
>
> Patch by: Bert Huijben <b....@competence.biz>
>
> * subversion/include/svn_wc.h
> (svn_wc_notify_t): New field path_prefix.
>
> * subversion/libsvn_wc/util.c
> (svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix
> field.
>
> * subversion/libsvn_client/client.h
> (svn_client__do_commit): Document the new handling of
> notify_path_prefix.
>
> * subversion/libsvn_client/commit_util.c
> (svn_client__do_commit, do_item_commit) Remove local handling of
> notify_path_prefix. Instead pass path_prefix to the notification
> handler.
>
> * subversion/svn/notify.c
> (notify): Use the new path_prefix to split the first part of the path
> if it matches the specified prefix. Don't bother testing for urls
> as the path is documented to be a local path.
> ]]]
(I think this should still apply to the new version)
To answer your other mail:
Patch 3 should be a 100% standalone patch. Patch 3 might only need some
further tweaking if the CLI behavior should change (which I don't expect).
Bert
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (2/3)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> [[[
> Move the creation of final display paths to the CLI notification
> handler, to make sure notifications always receive a valid path in the
> path field.
>
> [...]
This patch causes virtually every command-line regression test to fail.
Did you run 'make check' before posting? :-)
You probably know why already: output paths are apparently now absolute
instead of relative. For example, here's the output of a simple script
to commit one file (see http://paste.lisp.org/display/60776 for the
script, if you want). This is with your patch applied:
$ ./doit.sh
### Making a Greek Tree for import...
### Done.
### Importing it...
### Done.
### Making a change to A/D/G/pi...
### Done. Now committing it...
Sending /home/kfogel/src/subversion/sandbox/wc/A/D/G/pi
Transmitting file data .
Committed revision 2.
### Done.
$
(I've slightly reformatted this output to save some vertical space, by
the way, but not in a way that affects the results.) Now here's a run
of the same script without your patch:
$ ./doit.sh
### Making a Greek Tree for import...
### Done.
### Importing it...
### Done.
### Making a change to A/D/G/pi...
### Done. Now committing it...
Sending A/D/G/pi
Transmitting file data .
Committed revision 2.
### Done.
$
If you regexp search for "^FAIL:" in tests.log after a 'make check' run,
you'll see the same problem:
EXPECTED OUTPUT TREE:
ROOT
+-- svn-test-work
+-- working_copies
+-- basic_tests-3
+-- A
|-- mu
+-- D
+-- G
+-- rho
ACTUAL OUTPUT TREE:
ROOT
+--
+-- home
+-- kfogel
+-- src
+-- subversion
+-- subversion
+-- tests
+-- cmdline
+-- svn-test-work
+-- [...]
(There's more, but I think you get the idea.)
I assume that it was not your goal to always produce absolute paths in
svn output? If it was, we'd better discuss some more; I think in most
circumstances that is not what users want. I'm trying to locate the
mail where you described a proposed new behavior, actually...
Below is the exact patch I tested with. It's the same as the patch you
posted, just with a couple of documentation tweaks.
Best,
-Karl
[[[
Move the creation of final display paths to the CLI notification
handler, to make sure notifications always receive a valid path in the
path field.
Patch by: Bert Huijben <b....@competence.biz>
* subversion/include/svn_wc.h
(svn_wc_notify_t): New field path_prefix.
* subversion/libsvn_wc/util.c
(svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix field.
* subversion/libsvn_client/client.h
(svn_client__do_commit): Document the new handling of notify_path_prefix.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit, do_item_commit) Remove local handling of
notify_path_prefix. Instead pass path_prefix to the notification
handler.
* subversion/svn/notify.c
(notify): Use the new path_prefix to split the first part of the path
if it matches the specified prefix. Don't bother testing for urls
as the path is documented to be a local path.
]]]
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 31174)
+++ subversion/include/svn_wc.h (working copy)
@@ -909,6 +909,11 @@
* left and right sides of the merge are from the same URL. In all
* other cases, it is @c NULL. @since New in 1.5 */
svn_merge_range_t *merge_range;
+ /** If non-NULL, specifies an absolute path prefix that can be subtracted
+ * from the start of the absolute path in @c path. Its purpose is to
+ * allow notification to remove a common prefix from all the paths
+ * displayed for an operation. @since New in 1.6 */
+ const char *path_prefix;
/* NOTE: Add new fields at the end to preserve binary compatibility.
Also, if you add fields here, you have to update svn_wc_create_notify
and svn_wc_dup_notify. */
Index: subversion/libsvn_wc/util.c
===================================================================
--- subversion/libsvn_wc/util.c (revision 31174)
+++ subversion/libsvn_wc/util.c (working copy)
@@ -128,6 +128,7 @@
ret->revision = SVN_INVALID_REVNUM;
ret->changelist_name = NULL;
ret->merge_range = NULL;
+ ret->path_prefix = NULL;
return ret;
}
@@ -164,6 +165,8 @@
ret->changelist_name = apr_pstrdup(pool, ret->changelist_name);
if (ret->merge_range)
ret->merge_range = svn_merge_range_dup(ret->merge_range, pool);
+ if (ret->path_prefix)
+ ret->path_prefix = apr_pstrdup(pool, ret->path_prefix);
return ret;
}
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 31174)
+++ subversion/libsvn_client/client.h (working copy)
@@ -915,9 +915,8 @@
CTX->NOTIFY_FUNC/CTX->BATON will be called as the commit progresses, as
a way of describing actions to the application layer (if non NULL).
- NOTIFY_PATH_PREFIX is used to send shorter, relative paths to the
- notify_func (it's a prefix that will be subtracted from the front
- of the paths.)
+ NOTIFY_PATH_PREFIX will be passed to CTX->notify_func2() as the
+ common absolute path prefix of the committed paths. It can be NULL.
If the caller wants to keep track of any outstanding temporary
files left after the transmission of text and property mods,
Index: subversion/libsvn_client/commit_util.c
===================================================================
--- subversion/libsvn_client/commit_util.c (revision 31174)
+++ subversion/libsvn_client/commit_util.c (working copy)
@@ -1056,7 +1056,8 @@
void *edit_baton; /* commit editor's baton */
apr_hash_t *file_mods; /* hash: path->file_mod_t */
apr_hash_t *tempfiles; /* hash of tempfiles created */
- const char *notify_path_prefix; /* notification path prefix */
+ const char *notify_path_prefix; /* notification path prefix
+ (NULL is okay, else abs path) */
svn_client_ctx_t *ctx; /* client context baton */
apr_hash_t *commit_items; /* the committables */
};
@@ -1080,7 +1081,6 @@
svn_wc_adm_access_t *adm_access = cb_baton->adm_access;
const svn_delta_editor_t *editor = cb_baton->editor;
apr_hash_t *file_mods = cb_baton->file_mods;
- const char *notify_path_prefix = cb_baton->notify_path_prefix;
svn_client_ctx_t *ctx = cb_baton->ctx;
/* Do some initializations. */
@@ -1121,20 +1121,9 @@
describe what we're about to do to this item. */
if (ctx->notify_func2)
{
- /* Convert an absolute path into a relative one (if possible.) */
- const char *npath = NULL;
+ const char *npath = item->path;
svn_wc_notify_t *notify;
- if (notify_path_prefix)
- {
- if (strcmp(notify_path_prefix, item->path))
- npath = svn_path_is_child(notify_path_prefix, item->path, pool);
- else
- npath = ".";
- }
- if (! npath)
- npath = item->path; /* Otherwise just use full path */
-
if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
&& (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD))
{
@@ -1183,6 +1172,7 @@
if (notify)
{
notify->kind = item->kind;
+ notify->path_prefix = cb_baton->notify_path_prefix;
(*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
}
}
@@ -1434,22 +1424,11 @@
if (ctx->notify_func2)
{
svn_wc_notify_t *notify;
- const char *npath = NULL;
-
- if (notify_path_prefix)
- {
- if (strcmp(notify_path_prefix, item->path) != 0)
- npath = svn_path_is_child(notify_path_prefix, item->path,
- subpool);
- else
- npath = ".";
- }
- if (! npath)
- npath = item->path;
- notify = svn_wc_create_notify(npath,
+ notify = svn_wc_create_notify(item->path,
svn_wc_notify_commit_postfix_txdelta,
subpool);
notify->kind = svn_node_file;
+ notify->path_prefix = notify_path_prefix;
(*ctx->notify_func2)(ctx->notify_baton2, notify, subpool);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168) (2/3)
Posted by Bert Huijben <be...@qqmail.nl>.
Hi All,
This is part 2 of 3 patches.
Log message for patch 2:
[[[
Move the creation of final display paths to the CLI notification
handler, to make sure notifications always receive a valid path in the
path field.
* subversion/include/svn_wc.h
(svn_wc_notify_t): New field path_prefix.
* subversion/libsvn_wc/util.c
(svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix field.
* subversion/libsvn_client/client.h
(svn_client__do_commit): Document the new handling of notify_path_prefix.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit, do_item_commit) Remove local handling of
notify_path_prefix. Instead pass path_prefix to the notification
handler.
* subversion/svn/notify.c
(notify): Use the new path_prefix to split the first part of the path
if it matches the specified prefix. Don't bother testing for urls
as the path is documented to be a local path.
Patch by: Bert Huijben <b....@competence.biz>
Tweaked by: kfogel
]]]
Bert
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Log message for patch 3:
>
> [[[
> Calculate a notify prefix for commit notifications.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): Calculate a new-style notify prefix to pass to
> svn_client__do_commit (no functional change).
>
> Patch by: Bert Huijben <b....@competence.biz>
> Tweaked by: kfogel
> ]]]
I just posted a mail about patch 2, saying that it breaks all the
regression tests.
Then when I saw patch 3, I thought "Oh, maybe he just forgot to note in
his log message for patch 2 that it breaks notification paths for commit
and that upcoming patch 3 will fix that". So I applied patch 3 on top
of patch 2, and ran my repro script (http://paste.lisp.org/display/60776),
and the path printed there seemed to be okay.
So then I ran the regression tests again... But they still all fail,
even with both patch 2 and 3 applied.
Obviously, if there is any interdependency between patches (like, one
causes tests to break, but the next one unbreaks them), that should
always be noted in the log message.
But I'm not sure that's going on here, since things still seem to be
broken with patch 3 applied.
Hey, you appear to be in IRC now, I'll go chat there.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> [[[
> Calculate a notify prefix for commit notifications.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): Calculate a new-style notify prefix to pass to
> svn_client__do_commit (no functional change).
>
> Patch by: Bert Huijben <b....@competence.biz>
> Tweaked by: kfogel
> ]]]
Committed in r31212. Thanks, Bert.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Bert Huijben <be...@qqmail.nl>.
Hi All,
This is part 3 of 3 patches which fix issue #3168.
Log message for patch 3:
[[[
Calculate a notify prefix for commit notifications.
* subversion/libsvn_client/commit.c
(svn_client_commit4): Calculate a new-style notify prefix to pass to
svn_client__do_commit (no functional change).
Patch by: Bert Huijben <b....@competence.biz>
Tweaked by: kfogel
]]]
Bert
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Bert Huijben <B....@competence.biz>.
> -----Original Message-----
> From: Karl Fogel [mailto:kfogel@red-bean.com]
> Sent: dinsdag 6 mei 2008 3:04
> To: Daniel Shahaf
> Cc: Bert Huijben; dev@subversion.tigris.org; Mark Phippard
> Subject: Re: [PATCH] RE: Commit strips common prefix instead of
current
> directory (Issue #3168)
>
> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> > Patch manager hat on.
> >
> > [...]
> >
> > Numerous stylistic violations here in the log message (four
different
> > issues, at least). Please pay more attention to adhering to our
> > log message format; badly formatted log messages are as much of an
> > obstacle to our processing a patch as any issue in the code itself.
> > Thank you.
>
> True... but these sorts of things are easy for us to fix as we read.
I would have expected only a functional review at this time (Not one of
the actual implementation ;-).
> Bert, here's a new version of your log message, followed by some
> comments on the code itself. My edits to the log message were:
>
> - standardize line lengths
> - put colons where they ought to be
> - avoid repeating what new doc strings in the code say anyway
> - fixed a few misspellings fixed
> - describe implementation details less, describe behaviors more
> - put periods put on the ends of sentences
> - correct a few actual errors (see my "###" comments)
> - put many sentences into the active voice (my personal crusade)
Thanks..
> [[[
> Move the creation of final display paths to the CLI notification
> handler, to make sure notifications always receive a valid path in the
> path field.
>
> * subversion/include/svn_wc.h
> (svn_wc_notify_t): New field path_prefix. Unrelatedly, note
versions
> in which other new fields were added.
> ### If possible, it's best to submit those other cleanups as a
> ### separate patch. I know it's slightly more work, but it really
> ### helps keep things simple for reviewers.
If I had used two patches they would not apply cleanly if used
separately (and once applied I merge this patch in the SharpSvn builds
to fix some AnkhSvn issues).
(Didn't change this for the updated patch)
> * subversion/libsvn_wc/util.c
> (svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix
> field.
>
> * subversion/libsvn_client/client.h
> (svn_client__do_commit): Document the new handling of
> notify_path_prefix.
>
> * subversion/libsvn_client/commit_util.c
> (svn_client__do_commit, do_item_commit) Remove local handling of
> notify_path_prefix. Instead pass path_prefix to the notification
> handler.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): Calculate a new-style notify prefix to pass to
> svn_client__do_commit.
> ### Your original log message here said "Document the calculation
> of
> ### the notify prefix by renaming and adding a local variable."
> But
> ### there is no documentation change here, only a code change!
I added an extra variable and renamed another; no functional changes
here..
(And copied your text into the new log message)
> * subversion/svn/notify.c
> (notify): Use the new path_prefix to split the first part of the
path
> if it matches the specified prefix. Don't bother testing for urls
> as the path is documented to be a local path.
>
> Patch by: Bert Huijben <b....@competence.biz>
> ]]]
New version attached
>
> Okay, now comments on the code:
>
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 31025)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -903,12 +903,16 @@
> > * In all other cases, it is @c SVN_INVALID_REVNUM. */
> > svn_revnum_t revision;
> > /** When @c action is @c svn_wc_notify_changelist_add or name.
In
> all other
> > - * cases, it is @c NULL. */
> > + * cases, it is @c NULL. @since New in 1.5 */
> > const char *changelist_name;
> > /** When @c action is @c svn_wc_notify_merge_begin, and both the
> > - left and right sides of the merge are from the same URL. In
> all
> > - other cases, it is @c NULL. */
> > + * left and right sides of the merge are from the same URL. In
> all
> > + * other cases, it is @c NULL. @since New in 1.5 */
> > svn_merge_range_t *merge_range;
> > + /** If non-NULL, specifies an absolute path prefix that can be
> subtracted
> > + * from the start of the absolute path in @c path to print paths
> without a
> > + * common prefix. @since New in 1.6 */
> > + const char *path_prefix;
> > /* NOTE: Add new fields at the end to preserve binary
> compatibility.
> > Also, if you add fields here, you have to update
> svn_wc_create_notify
> > and svn_wc_dup_notify. */
>
> The wording of the doc string is ambiguous: it suggest that we are
> printing paths that fall into the class of "paths without a common
> prefix" -- that is, the phrase "without a common prefix" sounds like
> it's a adjectival phrase modifying "path", but you meant it to be an
> adverbial phrase modifying "print". How about:
>
> /** If non-NULL, specifies an absolute path prefix that can be
> subtracted
> * from the start of the absolute path in @c path. Its purpose is
to
> * allow notification to remove a common prefix from all the paths
> * displayed for an operation. @since New in 1.6 */
>
> ...instead, or something like that?
Copied literally in the new patch
>
> > --- subversion/libsvn_wc/util.c (revision 31025)
> > +++ subversion/libsvn_wc/util.c (working copy)
> > @@ -128,6 +128,7 @@
> > ret->revision = SVN_INVALID_REVNUM;
> > ret->changelist_name = NULL;
> > ret->merge_range = NULL;
> > + ret->path_prefix = NULL;
> >
> > return ret;
> > }
> > @@ -164,6 +165,8 @@
> > ret->changelist_name = apr_pstrdup(pool, ret->changelist_name);
> > if (ret->merge_range)
> > ret->merge_range = svn_merge_range_dup(ret->merge_range, pool);
> > + if (ret->path_prefix)
> > + ret->path_prefix = apr_pstrdup(pool, ret->path_prefix);
> >
> > return ret;
> > }
>
> Glad you read the comment at the end of svn_wc_notify_t :-).
I have to know this code for the SharpSvn bindings and the AnkhSVN
status cache updating ;)
> > --- subversion/libsvn_client/client.h (revision 31025)
> > +++ subversion/libsvn_client/client.h (working copy)
> > @@ -915,9 +915,9 @@
> > CTX->NOTIFY_FUNC/CTX->BATON will be called as the commit
> progresses, as
> > a way of describing actions to the application layer (if non
> NULL).
> >
> > - NOTIFY_PATH_PREFIX is used to send shorter, relative paths to
the
> > - notify_func (it's a prefix that will be subtracted from the
front
> > - of the paths.)
> > + NOTIFY_PATH_PREFIX will be passed to the notification handler to
> > + print shorter paths (it's a prefix that will be subtracted from
> the
> > + front of absolute paths.)
> >
> > If the caller wants to keep track of any outstanding temporary
> > files left after the transmission of text and property mods,
>
> Hmmm. This isn't really your fault, but we've sort of mis-documented
> NOTIFY_PATH_PREFIX here. We should just say that it is passed to
> CTX->notify_func2(). Maybe we can say what we *think* the notify func
> is likely to do with it, but we should have svn_wc_notify_func2_t
> formally document the behavior, since svn_client__do_commit() actually
> has no control over that.
The new patch contains:
+ NOTIFY_PATH_PREFIX will be passed to CTX->notify_func2() as common
absolute
+ path prefix of the committed absolute paths.
Your suggested text for svn_wc_notify_t should fix the other part.
> > --- subversion/svn/notify.c (revision 31025)
> > +++ subversion/svn/notify.c (working copy)
> > @@ -55,13 +55,21 @@
> > {
> > struct notify_baton *nb = baton;
> > char statchar_buf[5] = " ";
> > - const char *path_local;
> > + const char *path_local = n->path;
> > svn_error_t *err;
> > +
> > + if (n->path_prefix)
> > + {
> > + if (strcmp(n->path->prefix, path_local))
> > + path_local = svn_path_is_child(n->path_prefix, path_local,
> pool);
> > + else
> > + path_local = ".";
> > +
> > + if (!path_local)
> > + path_local = n->path; /* if path is not below path_prefix
*/
> > + }
>
> Why check strcmp() before calling svn_path_is_child()? You could just
> call svn_path_is_child() directly -- you'll either get NULL or the
> remainder you're looking for, right?
I wrote this code at least three times in different forms. Finally I
concluded to use the original form from the commit code to make sure no
unnecessary allocations were made.
After making sure that svn_path_is_child not do any unneeded allocations
when returning NULL I updated the code to check the exact match after
the svn_path_is_child call. (That code path should be only accessed once
during a commit; but using "." in all cases where path_local is NULL
might obsure future bugs in other places that use this new field)
> -Karl
Thanks for your review.
(New log message and new patch attached to stop Outlook updating the
layout)
Bert
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Patch manager hat on.
>
> [...]
>
> Numerous stylistic violations here in the log message (four different
> issues, at least). Please pay more attention to adhering to our
> log message format; badly formatted log messages are as much of an
> obstacle to our processing a patch as any issue in the code itself.
> Thank you.
True... but these sorts of things are easy for us to fix as we read.
Bert, here's a new version of your log message, followed by some
comments on the code itself. My edits to the log message were:
- standardize line lengths
- put colons where they ought to be
- avoid repeating what new doc strings in the code say anyway
- fixed a few misspellings fixed
- describe implementation details less, describe behaviors more
- put periods put on the ends of sentences
- correct a few actual errors (see my "###" comments)
- put many sentences into the active voice (my personal crusade)
[[[
Move the creation of final display paths to the CLI notification
handler, to make sure notifications always receive a valid path in the
path field.
* subversion/include/svn_wc.h
(svn_wc_notify_t): New field path_prefix. Unrelatedly, note versions
in which other new fields were added.
### If possible, it's best to submit those other cleanups as a
### separate patch. I know it's slightly more work, but it really
### helps keep things simple for reviewers.
* subversion/libsvn_wc/util.c
(svn_wc_create_notify, svn_wc_dup_notify): Handle new path_prefix field.
* subversion/libsvn_client/client.h
(svn_client__do_commit): Document the new handling of notify_path_prefix.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit, do_item_commit) Remove local handling of
notify_path_prefix. Instead pass path_prefix to the notification
handler.
* subversion/libsvn_client/commit.c
(svn_client_commit4): Calculate a new-style notify prefix to pass to
svn_client__do_commit.
### Your original log message here said "Document the calculation of
### the notify prefix by renaming and adding a local variable." But
### there is no documentation change here, only a code change!
* subversion/svn/notify.c
(notify): Use the new path_prefix to split the first part of the path
if it matches the specified prefix. Don't bother testing for urls
as the path is documented to be a local path.
Patch by: Bert Huijben <b....@competence.biz>
]]]
Okay, now comments on the code:
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 31025)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -903,12 +903,16 @@
> * In all other cases, it is @c SVN_INVALID_REVNUM. */
> svn_revnum_t revision;
> /** When @c action is @c svn_wc_notify_changelist_add or name. In all other
> - * cases, it is @c NULL. */
> + * cases, it is @c NULL. @since New in 1.5 */
> const char *changelist_name;
> /** When @c action is @c svn_wc_notify_merge_begin, and both the
> - left and right sides of the merge are from the same URL. In all
> - other cases, it is @c NULL. */
> + * left and right sides of the merge are from the same URL. In all
> + * other cases, it is @c NULL. @since New in 1.5 */
> svn_merge_range_t *merge_range;
> + /** If non-NULL, specifies an absolute path prefix that can be subtracted
> + * from the start of the absolute path in @c path to print paths without a
> + * common prefix. @since New in 1.6 */
> + const char *path_prefix;
> /* NOTE: Add new fields at the end to preserve binary compatibility.
> Also, if you add fields here, you have to update svn_wc_create_notify
> and svn_wc_dup_notify. */
The wording of the doc string is ambiguous: it suggest that we are
printing paths that fall into the class of "paths without a common
prefix" -- that is, the phrase "without a common prefix" sounds like
it's a adjectival phrase modifying "path", but you meant it to be an
adverbial phrase modifying "print". How about:
/** If non-NULL, specifies an absolute path prefix that can be subtracted
* from the start of the absolute path in @c path. Its purpose is to
* allow notification to remove a common prefix from all the paths
* displayed for an operation. @since New in 1.6 */
...instead, or something like that?
> --- subversion/libsvn_wc/util.c (revision 31025)
> +++ subversion/libsvn_wc/util.c (working copy)
> @@ -128,6 +128,7 @@
> ret->revision = SVN_INVALID_REVNUM;
> ret->changelist_name = NULL;
> ret->merge_range = NULL;
> + ret->path_prefix = NULL;
>
> return ret;
> }
> @@ -164,6 +165,8 @@
> ret->changelist_name = apr_pstrdup(pool, ret->changelist_name);
> if (ret->merge_range)
> ret->merge_range = svn_merge_range_dup(ret->merge_range, pool);
> + if (ret->path_prefix)
> + ret->path_prefix = apr_pstrdup(pool, ret->path_prefix);
>
> return ret;
> }
Glad you read the comment at the end of svn_wc_notify_t :-).
> --- subversion/libsvn_client/client.h (revision 31025)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -915,9 +915,9 @@
> CTX->NOTIFY_FUNC/CTX->BATON will be called as the commit progresses, as
> a way of describing actions to the application layer (if non NULL).
>
> - NOTIFY_PATH_PREFIX is used to send shorter, relative paths to the
> - notify_func (it's a prefix that will be subtracted from the front
> - of the paths.)
> + NOTIFY_PATH_PREFIX will be passed to the notification handler to
> + print shorter paths (it's a prefix that will be subtracted from the
> + front of absolute paths.)
>
> If the caller wants to keep track of any outstanding temporary
> files left after the transmission of text and property mods,
Hmmm. This isn't really your fault, but we've sort of mis-documented
NOTIFY_PATH_PREFIX here. We should just say that it is passed to
CTX->notify_func2(). Maybe we can say what we *think* the notify func
is likely to do with it, but we should have svn_wc_notify_func2_t
formally document the behavior, since svn_client__do_commit() actually
has no control over that.
> --- subversion/svn/notify.c (revision 31025)
> +++ subversion/svn/notify.c (working copy)
> @@ -55,13 +55,21 @@
> {
> struct notify_baton *nb = baton;
> char statchar_buf[5] = " ";
> - const char *path_local;
> + const char *path_local = n->path;
> svn_error_t *err;
> +
> + if (n->path_prefix)
> + {
> + if (strcmp(n->path->prefix, path_local))
> + path_local = svn_path_is_child(n->path_prefix, path_local, pool);
> + else
> + path_local = ".";
> +
> + if (!path_local)
> + path_local = n->path; /* if path is not below path_prefix */
> + }
Why check strcmp() before calling svn_path_is_child()? You could just
call svn_path_is_child() directly -- you'll either get NULL or the
remainder you're looking for, right?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current
directory (Issue #3168)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Patch manager hat on.
Bert Huijben wrote on Mon, 5 May 2008 at 15:49 +0200:
> [[
> Moved the creation of the final display paths to the cli notification
> handler, to
> make sure notifications always receive a valid path in the path field.
>
> * subversion/include/svn_wc.h
> (svn_wc_notify_t) Added path_prefix containing an absolute prefix
> which
> can be subtracted from the absolute path passed. Noted versions in
> which
> new fields where added.
>
> * subversion/libsvn_wc/util.c
> (svn_wc_create_notify, svn_wc_dup_notify) Added path_prefix handling
>
> * subversion/libsvn_client/client.h
> (svn_client__do_commit) Updated documentation to document the new
> handling
> of notify_path_prefix
>
> * subversion/libsvn_client/commit_util.c
> (svn_client__do_commit, do_item_commit) Removed local handling of
> notify_path_prefix. Instead pass the prefix path via path_prefix to
> the
> notification handler.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4) Document the calculation of the notify prefix by
> renaming
> and adding a local variable.
>
> * subversion/svn/notify.c
> (notify) Use the new path_prefix to split the first part of the path
> if it
> matches the specified prefix. Don't bother testing for urls as the
> path is documented
> to be a local path.
>
> Patch by: Bert Huijben <b.huijben@competence.biz
> ]]
>
Numerous stylistic violations here in the log message (four different
issues, at least). Please pay more attention to adhering to our
log message format; badly formatted log messages are as much of an
obstacle to our processing a patch as any issue in the code itself.
Thank you.
Thanks for sending it text/plain,
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Bert Huijben <B....@competence.biz>.
Sorry for the delay in responding. I was a few days away from home..
> -----Original Message-----
> From: Karl Fogel [mailto:kfogel@red-bean.com]
> Sent: donderdag 1 mei 2008 20:03
> To: Bert Huijben
> Cc: 'Mark Phippard'; 'Bert Huijben'; dev@subversion.tigris.org
> Subject: Re: [PATCH] RE: Commit strips common prefix instead of
current
> directory (Issue #3168)
>
> "Bert Huijben (TCG)" <b....@competence.biz> writes:
> > So both functions work correctly, following their own definition..
> But the
> > result of those two combined is commit notifications use a not-
> relative and
> > a not-absolute path. (Which breaks using the notifications to keep
an
> in
> > memory-file-state-cache up to date).
> >
> > The real problem is: How can we fix this?
> >
> > A user does not expect to see absolute paths when committing, unless
> he
> > passed absolute paths to 'svn ci' and he does not expect to see
> relative
> > paths if he passed only absolute paths.
>
> Hmmm, yeah, choosing a good behavior here is hard. The primary goal
is
> predictability, I would think. One possibility, off the top of my
> head:
>
> 1. If any path was absolute, print all paths as absolute.
This should be easy to implement and would fix my usage pattern. (I
always provide absolute paths as I can't change the working directory
without side effects)
>
> 2. Otherwise, print all paths as relative to the top ("root") of
the
> working copy, OR:
>
> Print them relative to cwd iff cwd is a (grand, etc)parent of
all
> the committed paths, else default to relative-to-root-of-wc.
The notify method is documented to be called with absolute paths, or
relative from the current working directory (no other values allowed).
To 'print' paths relative from another directory than cwd in the
svn-cli, the extra information should be provided to the cli notify
handler which could then print them in any other format.
(Not sure if we should calculate the working copy root; but providing
the action root should be very easy and it would give the same cli
output as the current version)
The easiest solution to integrate 1 and 2 is to move the path splitting
code to the cli notification handler and pass absolute paths to the
notification handler. The attached patch does exactly this.
So this patch does not fix the issue for the cli, but it will for
library users. And it moves the code to a place where it can be changed
per client. (I don't think the CLI output is documented to be a valid
path or the issue would have been noticed a long time ago)
Bert
[[
Moved the creation of the final display paths to the cli notification
handler, to
make sure notifications always receive a valid path in the path field.
* subversion/include/svn_wc.h
(svn_wc_notify_t) Added path_prefix containing an absolute prefix
which
can be subtracted from the absolute path passed. Noted versions in
which
new fields where added.
* subversion/libsvn_wc/util.c
(svn_wc_create_notify, svn_wc_dup_notify) Added path_prefix handling
* subversion/libsvn_client/client.h
(svn_client__do_commit) Updated documentation to document the new
handling
of notify_path_prefix
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit, do_item_commit) Removed local handling of
notify_path_prefix. Instead pass the prefix path via path_prefix to
the
notification handler.
* subversion/libsvn_client/commit.c
(svn_client_commit4) Document the calculation of the notify prefix by
renaming
and adding a local variable.
* subversion/svn/notify.c
(notify) Use the new path_prefix to split the first part of the path
if it
matches the specified prefix. Don't bother testing for urls as the
path is documented
to be a local path.
Patch by: Bert Huijben <b.huijben@competence.biz
]]
Re: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by Karl Fogel <kf...@red-bean.com>.
"Bert Huijben (TCG)" <b....@competence.biz> writes:
> So both functions work correctly, following their own definition.. But the
> result of those two combined is commit notifications use a not-relative and
> a not-absolute path. (Which breaks using the notifications to keep an in
> memory-file-state-cache up to date).
>
> The real problem is: How can we fix this?
>
> A user does not expect to see absolute paths when committing, unless he
> passed absolute paths to 'svn ci' and he does not expect to see relative
> paths if he passed only absolute paths.
Hmmm, yeah, choosing a good behavior here is hard. The primary goal is
predictability, I would think. One possibility, off the top of my head:
1. If any path was absolute, print all paths as absolute.
2. Otherwise, print all paths as relative to the top ("root") of the
working copy, OR:
Print them relative to cwd iff cwd is a (grand, etc)parent of all
the committed paths, else default to relative-to-root-of-wc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] RE: Commit strips common prefix instead of current directory (Issue #3168)
Posted by "Bert Huijben (TCG)" <b....@competence.biz>.
> -----Original Message-----
> From: Mark Phippard [mailto:markphip@gmail.com]
> Sent: vrijdag 11 april 2008 15:35
> To: Bert Huijben
> Cc: dev@subversion.tigris.org
> Subject: Re: [PATCH] RE: Commit strips common prefix instead of current
> directory
>
> 2008/4/11 Bert Huijben <be...@vmoo.com>:
> > > -----Original Message-----
> > > From: Bert Huijben [mailto:bert@vmoo.com]
> > > Sent: donderdag 10 april 2008 12:03
> > > To: dev@subversion.tigris.org
> > > Subject: Commit strips common prefix instead of current directory
> (Was:
> > > RE: Commit notifies invalid paths (Issue #3168))
> > >
> >
> > > I would like to send a patch to resolve this issue for 1.5, but I
> need
> > > some
> > > more input on which solution to take.
> >
> > Here is an initial patch which unbreaks the notifications and keeps
> the
> > short
> > Notifications in the common cases.
>
> Did you do any investigation to determine what change broke this and
> introduced the problem? Also, a test case would be useful to have.
Hi,
This mail got into my spam box :(
(Which I checked after reading #svn-dev on irc)
In my investigation I found out it probably never worked. At least not after
1.0 was released (all code around it is from the first few milestones and
not really touched after that).
The problem with the commit code is that all paths are normalized on a
calculated common parent directory containing all files to commit. All other
client handlers I checked just passes the given paths (absolute or relative)
around and gives those to the notify handler.
After normalizing the paths the commit gets the largest common path of the
current directory and the calculated common parent and passes that as the
path to make the notifications relative to. (This is suggested by the
variable names used)
The handler then uses the given path as the point to hide the first part of
the path (not to make the path relative to as the commit code assumed).
So both functions work correctly, following their own definition.. But the
result of those two combined is commit notifications use a not-relative and
a not-absolute path. (Which breaks using the notifications to keep an in
memory-file-state-cache up to date).
The real problem is: How can we fix this?
A user does not expect to see absolute paths when committing, unless he
passed absolute paths to 'svn ci' and he does not expect to see relative
paths if he passed only absolute paths.
(My patch only fixes the fact that the paths are invalid in specific cases..
But it will give you absolute paths in the notifications if you use 'svn ci
../file -m ""')
I will try if I can get some testcase running in the python framework \
The simple example
(From a working copy in /home/bert/tst/wc)
# mkdir b
# cd b
# svn mkdir ../a
A ../a
# svn ci ../a -m ""
Adding a
Transmitting file data .
Committed revision 1.
#
Shows the issue (but I think nobody noticed before).. There should have been
a 'Adding ../a' or 'Adding /home/bert/tst/wc/a', unless this case was
special cased in the notification handler (which it isn't)
Note that 'svn mkdir' does the right thing.
Bert
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current directory
Posted by Mark Phippard <ma...@gmail.com>.
2008/4/11 Bert Huijben <be...@vmoo.com>:
> > -----Original Message-----
> > From: Bert Huijben [mailto:bert@vmoo.com]
> > Sent: donderdag 10 april 2008 12:03
> > To: dev@subversion.tigris.org
> > Subject: Commit strips common prefix instead of current directory (Was:
> > RE: Commit notifies invalid paths (Issue #3168))
> >
>
> > I would like to send a patch to resolve this issue for 1.5, but I need
> > some
> > more input on which solution to take.
>
> Here is an initial patch which unbreaks the notifications and keeps the
> short
> Notifications in the common cases.
Did you do any investigation to determine what change broke this and
introduced the problem? Also, a test case would be useful to have.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Re: [PATCH] RE: Commit strips common prefix instead of current
directory
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Fri, 18 Apr 2008 at 18:38 -0400:
> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> > I'm not tracking this patch.
>
> Just curious: why not?
>
Because I know Bert will ping it himself if he thinks it is being
overlooked.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current directory
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> I'm not tracking this patch.
Just curious: why not?
> Bert: Please make sure attachments use a textual MIME type (your
> attachment was application/octet-stream).
Agreed, using "text/plain" helps a lot :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] RE: Commit strips common prefix instead of current
directory
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
I'm not tracking this patch.
Bert: Please make sure attachments use a textual MIME type (your
attachment was application/octet-stream).
Daniel
Bert Huijben wrote on Fri, 11 Apr 2008 at 11:01 +0200:
> > -----Original Message-----
> > From: Bert Huijben [mailto:bert@vmoo.com]
> > Sent: donderdag 10 april 2008 12:03
> > To: dev@subversion.tigris.org
> > Subject: Commit strips common prefix instead of current directory (Was:
> > RE: Commit notifies invalid paths (Issue #3168))
> >
>
> > I would like to send a patch to resolve this issue for 1.5, but I need
> > some
> > more input on which solution to take.
>
> Here is an initial patch which unbreaks the notifications and keeps the
> short
> Notifications in the common cases.
>
> [[
> Stop providing a notify_path_prefix if the specified path would make the
> Notification paths invalid
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4) Compare the notify_path_prefix with the current
> directory
> and only provide it to svn_client__do_commit() if it matches.
>
> Patch by: Bert Huijben <b....@competence.biz>
> ]]
>
>
> >
> >
> > Bert Huijben
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] RE: Commit strips common prefix instead of current directory
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@vmoo.com]
> Sent: donderdag 10 april 2008 12:03
> To: dev@subversion.tigris.org
> Subject: Commit strips common prefix instead of current directory (Was:
> RE: Commit notifies invalid paths (Issue #3168))
>
> I would like to send a patch to resolve this issue for 1.5, but I need
> some
> more input on which solution to take.
Here is an initial patch which unbreaks the notifications and keeps the
short
Notifications in the common cases.
[[
Stop providing a notify_path_prefix if the specified path would make the
Notification paths invalid
* subversion/libsvn_client/commit.c
(svn_client_commit4) Compare the notify_path_prefix with the current
directory
and only provide it to svn_client__do_commit() if it matches.
Patch by: Bert Huijben <b....@competence.biz>
]]
>
>
> Bert Huijben
Commit strips common prefix instead of current directory (Was: RE: Commit notifies invalid paths (Issue #3168))
Posted by Bert Huijben <be...@vmoo.com>.
Hi,
After further investigation, this seems to be a problem on all supported
operating systems. (not just Windows)
(Thanks lgo!)
svn_client_commit4() calls svn_client__do_commit() with as
notify_path_prefix the shortest common part
of the calculated basepath of the commit and the current directory.
e.g.
Current path: /folder1/current/
Commit path: /folder1/my/commit/location
The path placed in display_dir will be '/folder1'
(This part is pretty obscured by several translation functions using the
same variable as input and output and doing several path validations)
Variable display_dir is first filled with the current directory (which would
be a valid value) and then the last part is cut off which makes it invalid
This is then passed to svn_client__do_commit() which gets the display_path
as notify_path prefix.
This argument is documented as:
---- client.h:
NOTIFY_PATH_PREFIX is used to send shorter, relative paths to the
notify_func (it's a prefix that will be subtracted from the front
of the paths.)
-------------
When the call finally reaches do_item_commit() in commit_util.c it does what
it said in client.h
It verifies if the commit item starts with the prefix, and if it does strips
of the prefix.
For my example it would check if '/folder1/my/commit/location/my.txt' would
start with '/folder1' and would then call notify with jusr
'my/commit/location/my.txt' which is neither a valid absolute path, nor a
valid relative path from the current path.
This example is not commonly touched from the cli, as you normally just move
your current directory to a more logical location; but GUI clients will see
this issue quite frequently as they would not change the current directory
very often.
Possible solutions:
* Only pass notify_path_prefix if the prefix matches the current directory..
(Or just pass current directory all the time. Though this might give strange
results if your current directory lies beneath your commit base)
* Add an extra parameter to svn_client__do_commit() which contains a prefix
to be applied if the path is stripped of. In the example case '..' should be
set in this value.
* Some combination of the above which calculates whether a relative prefix
would be shorter than the absolute one and chooses based on that
information.
More thoughts:
* A global search revealed there is only one caller of
svn_client__do_commit() which actually sets the notify_path_prefix argument.
All other callers (just 1) just pass NULL. (See issue #3168).
* It was quite easy to apply a new global test on invalid paths in the wc
notify handler in the SharpSvn test framework. It was only triggered by this
exact issue. All other notifies (of the current tests) received at least a
valid path.
I would like to send a patch to resolve this issue for 1.5, but I need some
more input on which solution to take.
Bert Huijben
> -----Original Message-----
> From: Bert Huijben [mailto:B.Huijben@competence.biz]
> Sent: woensdag 9 april 2008 12:05
> To: dev@subversion.tigris.org
> Subject: Commit notifies invalid paths on windows (Issue #3168)
>
> Hi,
>
> As I noted on irc yesterday I created the following issue
> (I did some more research on the bug to find out whether other commands
> had the same problem.. but they have not)
>
> --
> The definition of svn_wc_notify_func2_t function uses svn_wc_notify_t
> which
> declares
>
> typedef struct svn_wc_notify_t {
> /** Path, either absolute or relative to the current working
> directory
> * (i.e., not relative to an anchor). */
> const char *path;
>
> But calling (at the client level in pseudo code)
> svn.Commit("E:\tmp\wc",....)
>
> when your current directory is "E:\dev\sharpsvn\src\SharpSvn.Tests"
>
> Gives you notifies where the path is "tmp\wc\testfile.txt" which is
> neither a
> valid relative path nor a valid absolute path.
>
> lgo mentioned on irc this bug is related to issue #1711.
>
> Further testing in the SharpSvn testsuite showed the issue is triggered
> on all
> notifies from Commit() but is /not/ triggered from most import calls
> (did not
> test all variants), or any of the other commands run from the SharpSvn
> testsuite (extended version of AnkhSVN's NSvn tests).
>
> I really would like to have this issue fixed for (at least) absolute
> paths in
> 1.5, as this breaks using the notify for updating the in memory cache
> in
> the
> new AnkhSVN. (More on this in issue #3147)
>
> --
>
> Looking through the code seems to tell this issue has been in
> subversion
> since far before 1.0, but I really would like to have some solution in
> 1.5 (or at least the 1.5.x branch, so I can merge it in the subversion
> build from the SharpSvn environment)
>
> Bert Huijben
>
> ---------------------------------------------------------------------
> 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