You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pt...@gmail.com> on 2010/08/03 16:40:58 UTC
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Ok, I am waving the white flag as far as implementing a fix for issue
#3657 in the RA layer. I simply don't see how it can be done outside
of this: Put the DAV update report handler into send-all=TRUE
<S:update-report send-all=TRUE> mode when creating a
svn_ra_reporter3_t * during a merge/diff. This would prevent
<S:fetch-props/> response that causes the client to fetch *all* the
properties of a path which subsequently pushes these to the
svn_delta_editor_t callbacks as if they were all prop *changes* (as
Mike discussed here
http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc9).
fyi: Currently this is how the two DAV RA layers use send-all mode:
send-all send-all
mode mode
operation ra_serf ra_neon
--------- -------- ---------
update FALSE TRUE
status FALSE TRUE
switch FALSE TRUE
diff FALSE FALSE
In that attached patch I reverted r966822 and tried the aforementioned
approach by tweaking ra_neon so the send-all mode on diff/merge is
TRUE. The whole test suite passes (including the test for issue
#3657). I haven't got an equivalent change to ra_serf to work yet,
but I'm not too worried about that yet because...
...After testing out this idea, I realized the problem with this
approach: It reopens issue #2048 'Primary connection (of parallel
network connections used) in ra-dav diff/merge timeout
unnecessarily.':
http://subversion.tigris.org/issues/show_bug.cgi?id=2048
http://svn.apache.org/viewvc?view=revision&revision=853457
So I'm at a dead-end right now. I'm happy to revert r966822 and
reopen issue #3657 if the consensus is that my initial fix is a sloppy
band-aid to cover incorrect ra_neon/ra_serf behavior and that the
latter two are where the fix belongs.
Paul
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Aug 12, 2010 at 6:01 PM, Paul Burba <pt...@gmail.com> wrote:
> On Tue, Aug 3, 2010 at 12:40 PM, Paul Burba <pt...@gmail.com> wrote:
>> Ok, I am waving the white flag as far as implementing a fix for issue
>> #3657 in the RA layer. I simply don't see how it can be done outside
>> of this: Put the DAV update report handler into send-all=TRUE
>> <S:update-report send-all=TRUE> mode when creating a
>> svn_ra_reporter3_t * during a merge/diff. This would prevent
>> <S:fetch-props/> response that causes the client to fetch *all* the
>> properties of a path which subsequently pushes these to the
>> svn_delta_editor_t callbacks as if they were all prop *changes* (as
>> Mike discussed here
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc9).
>>
>> fyi: Currently this is how the two DAV RA layers use send-all mode:
>>
>> send-all send-all
>> mode mode
>> operation ra_serf ra_neon
>> --------- -------- ---------
>> update FALSE TRUE
>> status FALSE TRUE
>> switch FALSE TRUE
>> diff FALSE FALSE
>>
>> In that attached patch I reverted r966822 and tried the aforementioned
>> approach by tweaking ra_neon so the send-all mode on diff/merge is
>> TRUE. The whole test suite passes (including the test for issue
>> #3657). I haven't got an equivalent change to ra_serf to work yet,
>> but I'm not too worried about that yet because...
>>
>> ...After testing out this idea, I realized the problem with this
>> approach: It reopens issue #2048 'Primary connection (of parallel
>> network connections used) in ra-dav diff/merge timeout
>> unnecessarily.':
>>
>> http://subversion.tigris.org/issues/show_bug.cgi?id=2048
>> http://svn.apache.org/viewvc?view=revision&revision=853457
>>
>> So I'm at a dead-end right now. I'm happy to revert r966822 and
>> reopen issue #3657 if the consensus is that my initial fix is a sloppy
>> band-aid to cover incorrect ra_neon/ra_serf behavior and that the
>> latter two are where the fix belongs.
>>
>> Paul
>
> Mike suggested to me that the fix for this problem can be made in
> mod_dav_svn, since the update reporter already knows which properties
> have changed, *regardless* of the send-all mode, but it discards this
> information if send-all=FALSE and instead instead sends the client a
> 'fetch-props' response. As described previously in this thread, this
> causes the client to fetch all the properties of the path, not just
> the changes, and *all* the props are pushed through the editor
> callbacks as if they were all actual changes (which again is the core
> problem of issue #3657).
>
> I'm working on that approach,
I had pushed off the fix-it-in-mod_dav_svn approach till 1.8 since I
was going in circles with it, but recently found another real-world
example where this issue causes spurious conflicts on directory
properties, see
http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc13.
This issue has existed before 1.5 so it's nothing new, but given that
I found another non-theoretical example and given that the presence of
mergeinfo is only going to make this issue more common[1] I thought
I'd take another look for 1.7.
I still haven't gotten anywhere with the mod_dav_svn fix, so not much
to talk about there, but I did commit the fix for the directory
property conflicts in
http://svn.apache.org/viewvc?view=revision&revision=1056565. The fix
here is pretty much the same thing I did in
http://svn.apache.org/viewvc?view=revision&revision=966822: Filter out
the phony prop changes in the libsvn_client/repos_diff.c
svn_delta_editor_t change_[dir|file]_prop() callback implementations.
Just to be clear about one thing, when I committed r966822 Bert asked:
>> (change_file_prop): Only stash true property differences in the file
>> baton. The DAV RA providers may be sending us all the properties on
>> a file, not just the differences.
>
> Shouldn't this be fixed in the ra layer implementations then?
>
> This would be just 'guessing' if it is a change or a complete set.
Why not the RA layer? Two reasons. The first I explained earlier in
this thread, it reopens issue #2048 'Primary connection (of parallel
network connections used) in ra-dav diff/merge timeout unnecessarily.'
I won't go over that again, but even if there is a way to avoid that,
the RA layer is simply doing what we ask it when using the DAV
providers. Specifically, the mod_dav_svn update report handler, when
in 'skelta' mode[2] and describing changes to a path on which a
property has changed, asks the client to later fetch all properties
and figure out what has changed itself, i.e. the client asks the RA
layer to give us *all* the properties, it obliges, so where is the
problem as far as the RA layer is concerned?
So this ultimately needs to be fixed on the server. Given that, I am
leaving these libsvn_client filters in place so a 1.7 client will DTRT
with older servers (or maybe 1.7 servers too, since as I said, I'm not
having great success there).
As to the 'guessing', that was true. We'd go through the motions of
filtering over ra_local and ra_svn too. A pretty minor performance
hit, but in r1056565 I tweaked the filtering so it only applies when
using ra_serf and ra_neon.
Paul
[1] simply by increasing the potential pool of property changes
incoming during a merge.
[2] Which merges always are, regardless of whether we use ra_serf or ra_neon.
Paul
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mon, Aug 16, 2010 at 3:40 AM, Julian Foad <ju...@wandisco.com> wrote:
> On Thu, 2010-08-12, Paul Burba wrote:
>> I'm working on that approach, but in the meantime I have a question
>> about the behavior of svn diff --summarize when describing the
>> addition of paths with properties.
>>
>> ### Let's take a vanilla greek tree, add a file and directory,
>> ### set a prop on each, and commit as r2:
>>
>> >echo nu file > nu
>>
>> >svn add nu
>> A nu
>>
>> >svn ps prop val nu
>> property 'prop' set on 'nu'
>>
>> >svn mkdir J
>> A J
>>
>> >svn ps prop val J
>> property 'prop' set on 'J'
>>
>> ### Status looks right, two scheduled additions:
>>
>> >svn st
>> A nu
>> A J
>>
>> ### Commit it
>>
>> >svn ci -m ""
>> Adding J
>> Adding nu
>> Transmitting file data .
>> Committed revision 2.
>>
>> ### Check diff --summarize for r2
>>
>> >svn diff -r1:2 --summarize
>> AM nu
>> AM J
>>
>> Should we really be describing the props for this added (not copied!)
>> file as modified? It seems incorrect to call the props modified in
>> this case. I ask because this is one of the problems I am running
>> into with fixing this issue in mod_dav_svn, and before I spend more
>> time trying to "fix" it I want to be sure the current behavior is
>> really what we expect.
>>
>> Paul "Dying slowly at the hands of mod_dav_svn" Burba
>
> I would suggest that the output of "svn diff --summarize" should be
> consistent with the likes of "svn add" (no 'M'), "svn status" (no 'M')
> and "svn update" (I haven't checked this one).
(I mentioned this in response to the commit, but also will do so here
for completeness.)
The other place we're inconsistent is the template shown in the editor
used for log messages when 'svn ci' is run without -F or -m. I
believe it still shows 'AM'. (In fact, this was one of the earliest
bugs I attempted to fix when I first started hacking Subversion
several years ago. Ah, nostalgia.)
-Hyrum
Re: svn commit: r966822 - in /subversion/trunk/subversion: libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Stephen Butler <sb...@elego.de>.
On Aug 16, 2010, at 12:11 , Julian Foad wrote:
> Stephen Butler wrote:
>> On Aug 16, 2010, at 10:40 , Julian Foad wrote:
>>> I would suggest that the output of "svn diff --summarize" should be
>>> consistent with the likes of "svn add" (no 'M'), "svn status" (no 'M')
>>> and "svn update" (I haven't checked this one).
>>
>> Fixed in r985735, while I was working on issue #2333 "svn diff URL1
>> URL2 != svn diff URL2 URL1".
>>
>> Strangely, the 'AM' output occurred only via ra_svn and ra_file, not
>> via ra_dav. But I guess that's not strange to anyone following this
>> thread!
>>
>> The bug occurs in 1.6, too. We never tested '--summarize' properly.
>>
>> My fix is a simple hack in libsvn_client/repos_diff_summarize.c, but
>> quite similar to some longstanding hacks in libsvn_client/repos_diff.c.
>
> Be positive - you wrote Correct Code, no need to call it a 'hack'! To
> confirm it, we should update the docs like this, yes?
>
> Index: subversion/include/svn_client.h
> ===================================================================
> typedef struct svn_client_diff_summarize
> {
> /** Path relative to the target. If the target is a file, path is
> * the empty string. */
> const char *path;
>
> /** Change kind */
> svn_client_diff_summarize_kind_t summarize_kind;
>
> - /** Properties changed? */
> + /** Properties changed? (False if summarize_kind is 'added' or 'deleted'.) */
> svn_boolean_t prop_changed;
>
> /** File or dir */
> svn_node_kind_t node_kind;
> } svn_client_diff_summarize_t;
>
>
> Hmm... or maybe instead we should have left the API and the "--xml"
> output unchanged and just changed the 'svn' output in
> diff-cmd.c:summarize_regular(). What do you think?
I think the normal and --xml output should be in sync.
In r985913, I added the comment you suggested.
Steve
--
Stephen Butler | Senior Consultant
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Julian Foad <ju...@wandisco.com>.
Stephen Butler wrote:
> On Aug 16, 2010, at 10:40 , Julian Foad wrote:
> > I would suggest that the output of "svn diff --summarize" should be
> > consistent with the likes of "svn add" (no 'M'), "svn status" (no 'M')
> > and "svn update" (I haven't checked this one).
>
> Fixed in r985735, while I was working on issue #2333 "svn diff URL1
> URL2 != svn diff URL2 URL1".
>
> Strangely, the 'AM' output occurred only via ra_svn and ra_file, not
> via ra_dav. But I guess that's not strange to anyone following this
> thread!
>
> The bug occurs in 1.6, too. We never tested '--summarize' properly.
>
> My fix is a simple hack in libsvn_client/repos_diff_summarize.c, but
> quite similar to some longstanding hacks in libsvn_client/repos_diff.c.
Be positive - you wrote Correct Code, no need to call it a 'hack'! To
confirm it, we should update the docs like this, yes?
Index: subversion/include/svn_client.h
===================================================================
typedef struct svn_client_diff_summarize
{
/** Path relative to the target. If the target is a file, path is
* the empty string. */
const char *path;
/** Change kind */
svn_client_diff_summarize_kind_t summarize_kind;
- /** Properties changed? */
+ /** Properties changed? (False if summarize_kind is 'added' or 'deleted'.) */
svn_boolean_t prop_changed;
/** File or dir */
svn_node_kind_t node_kind;
} svn_client_diff_summarize_t;
Hmm... or maybe instead we should have left the API and the "--xml"
output unchanged and just changed the 'svn' output in
diff-cmd.c:summarize_regular(). What do you think?
- Julian
Re: svn commit: r966822 - in /subversion/trunk/subversion: libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Stephen Butler <sb...@elego.de>.
On Aug 16, 2010, at 10:40 , Julian Foad wrote:
> On Thu, 2010-08-12, Paul Burba wrote:
[...]
>>
>> ### Check diff --summarize for r2
>>
>>> svn diff -r1:2 --summarize
>> AM nu
>> AM J
>>
>> Should we really be describing the props for this added (not copied!)
>> file as modified? It seems incorrect to call the props modified in
>> this case. I ask because this is one of the problems I am running
>> into with fixing this issue in mod_dav_svn, and before I spend more
>> time trying to "fix" it I want to be sure the current behavior is
>> really what we expect.
>>
>> Paul "Dying slowly at the hands of mod_dav_svn" Burba
>
> I would suggest that the output of "svn diff --summarize" should be
> consistent with the likes of "svn add" (no 'M'), "svn status" (no 'M')
> and "svn update" (I haven't checked this one).
Fixed in r985735, while I was working on issue #2333 "svn diff URL1
URL2 != svn diff URL2 URL1".
Strangely, the 'AM' output occurred only via ra_svn and ra_file, not
via ra_dav. But I guess that's not strange to anyone following this
thread!
The bug occurs in 1.6, too. We never tested '--summarize' properly.
My fix is a simple hack in libsvn_client/repos_diff_summarize.c, but
quite similar to some longstanding hacks in libsvn_client/repos_diff.c.
If that makes any sense....
Steve
--
Stephen Butler | Senior Consultant
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-08-12, Paul Burba wrote:
> I'm working on that approach, but in the meantime I have a question
> about the behavior of svn diff --summarize when describing the
> addition of paths with properties.
>
> ### Let's take a vanilla greek tree, add a file and directory,
> ### set a prop on each, and commit as r2:
>
> >echo nu file > nu
>
> >svn add nu
> A nu
>
> >svn ps prop val nu
> property 'prop' set on 'nu'
>
> >svn mkdir J
> A J
>
> >svn ps prop val J
> property 'prop' set on 'J'
>
> ### Status looks right, two scheduled additions:
>
> >svn st
> A nu
> A J
>
> ### Commit it
>
> >svn ci -m ""
> Adding J
> Adding nu
> Transmitting file data .
> Committed revision 2.
>
> ### Check diff --summarize for r2
>
> >svn diff -r1:2 --summarize
> AM nu
> AM J
>
> Should we really be describing the props for this added (not copied!)
> file as modified? It seems incorrect to call the props modified in
> this case. I ask because this is one of the problems I am running
> into with fixing this issue in mod_dav_svn, and before I spend more
> time trying to "fix" it I want to be sure the current behavior is
> really what we expect.
>
> Paul "Dying slowly at the hands of mod_dav_svn" Burba
I would suggest that the output of "svn diff --summarize" should be
consistent with the likes of "svn add" (no 'M'), "svn status" (no 'M')
and "svn update" (I haven't checked this one).
- Julian
Re: svn commit: r966822 - in /subversion/trunk/subversion:
libsvn_client/repos_diff.c tests/cmdline/merge_tests.py
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Aug 3, 2010 at 12:40 PM, Paul Burba <pt...@gmail.com> wrote:
> Ok, I am waving the white flag as far as implementing a fix for issue
> #3657 in the RA layer. I simply don't see how it can be done outside
> of this: Put the DAV update report handler into send-all=TRUE
> <S:update-report send-all=TRUE> mode when creating a
> svn_ra_reporter3_t * during a merge/diff. This would prevent
> <S:fetch-props/> response that causes the client to fetch *all* the
> properties of a path which subsequently pushes these to the
> svn_delta_editor_t callbacks as if they were all prop *changes* (as
> Mike discussed here
> http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc9).
>
> fyi: Currently this is how the two DAV RA layers use send-all mode:
>
> send-all send-all
> mode mode
> operation ra_serf ra_neon
> --------- -------- ---------
> update FALSE TRUE
> status FALSE TRUE
> switch FALSE TRUE
> diff FALSE FALSE
>
> In that attached patch I reverted r966822 and tried the aforementioned
> approach by tweaking ra_neon so the send-all mode on diff/merge is
> TRUE. The whole test suite passes (including the test for issue
> #3657). I haven't got an equivalent change to ra_serf to work yet,
> but I'm not too worried about that yet because...
>
> ...After testing out this idea, I realized the problem with this
> approach: It reopens issue #2048 'Primary connection (of parallel
> network connections used) in ra-dav diff/merge timeout
> unnecessarily.':
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=2048
> http://svn.apache.org/viewvc?view=revision&revision=853457
>
> So I'm at a dead-end right now. I'm happy to revert r966822 and
> reopen issue #3657 if the consensus is that my initial fix is a sloppy
> band-aid to cover incorrect ra_neon/ra_serf behavior and that the
> latter two are where the fix belongs.
>
> Paul
Mike suggested to me that the fix for this problem can be made in
mod_dav_svn, since the update reporter already knows which properties
have changed, *regardless* of the send-all mode, but it discards this
information if send-all=FALSE and instead instead sends the client a
'fetch-props' response. As described previously in this thread, this
causes the client to fetch all the properties of the path, not just
the changes, and *all* the props are pushed through the editor
callbacks as if they were all actual changes (which again is the core
problem of issue #3657).
I'm working on that approach, but in the meantime I have a question
about the behavior of svn diff --summarize when describing the
addition of paths with properties.
### Let's take a vanilla greek tree, add a file and directory,
### set a prop on each, and commit as r2:
>echo nu file > nu
>svn add nu
A nu
>svn ps prop val nu
property 'prop' set on 'nu'
>svn mkdir J
A J
>svn ps prop val J
property 'prop' set on 'J'
### Status looks right, two scheduled additions:
>svn st
A nu
A J
### Commit it
>svn ci -m ""
Adding J
Adding nu
Transmitting file data .
Committed revision 2.
### Check diff --summarize for r2
>svn diff -r1:2 --summarize
AM nu
AM J
Should we really be describing the props for this added (not copied!)
file as modified? It seems incorrect to call the props modified in
this case. I ask because this is one of the problems I am running
into with fixing this issue in mod_dav_svn, and before I spend more
time trying to "fix" it I want to be sure the current behavior is
really what we expect.
Paul "Dying slowly at the hands of mod_dav_svn" Burba