SVN patch issue with property changes

Post by Dipu H
Hi Team,
Index: xpathleak.py
===================================================================
--- xpathleak.py (revision 950359)
+++ xpathleak.py (working copy)
@@ -4,25 +4,15 @@
libxml2.debugMemory(True)
expect="""--> Invalid expression
---> xmlXPathEval: evaluation failed
--> Invalid expression
---> xmlXPathEval: evaluation failed
"""
err=""
Property changes on: xpathleak.py
___________________________________________________________________
Modified: svn:executable
## -0,0 +1 ##
+* <<<<<<<------------- Yes, there is a change, value * added to property svn:executable
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
================================
Now I generated the patch and applied it on another copy of the same branch.
UC xpathleak.py

rejected hunk ## -0,0 +1,1 ## (svn:executable)

Property conflicts: 1
The patch is rejected without changing the property. The destination file
already has svn:executable property present in it. But the value is empty.
Ideally, if the patch was applied properly, the value should have been
changed to "*".
After little bit of googling, I found the below link which looks like a similar issue.
https://jira.atlassian.com/browse/CRUC-6114?src=confmacro
SVN Version : 1.8.11
I understand that the latest svn clients add * as the value of
svn:executable property. However, if we generate a patch out of this and
apply on an existing file, `svn patch` is not able to identify that there is
a property change from ‘null’ to ‘*’. Could you please check if this is a
bug?

SVN 1.8.11 is very old (version 1.8.x is actually end-of-life now
[1]). Can you test with a 1.10 client to see if it shows the same
behaviour?

[1] http://subversion.apache.org/docs/release-notes/1.10.html#svn-1.8-deprecation

--
Johan

Dipu H

2018-06-28 14:24:02 UTC

Hi Johan,

Thanks for getting back. I tried to patch with svn 1.9 and 1.10. Still the same error.

***@contrail-ubm-dipuh:/tmp/tests# svn --version -q
1.10.0
***@contrail-ubm-dipuh:/tmp/tests# svn patch /tmp/patch
C xpathleak.py

rejected hunk ## -0,0 +1,1 ## (svn:executable)

Summary of conflicts:
Property conflicts: 1

***@contrail-ubm-dipuh:/tmp/tests# cat /tmp/patch
Index: xpathleak.py
===================================================================
--- xpathleak.py (revision 951471)
+++ xpathleak.py (working copy)

Property changes on: xpathleak.py
___________________________________________________________________
Modified: svn:executable
## -0,0 +1 ##
+*
\ No newline at end of property
***@contrail-ubm-dipuh:/tmp/tests#

Looks like svn is not able to properly handle the svn:executable property changes via patch.

Thanks,

Hi Team,
Index: xpathleak.py
===================================================================
--- xpathleak.py (revision 950359)
+++ xpathleak.py (working copy)
@@ -4,25 +4,15 @@
libxml2.debugMemory(True)
expect="""--> Invalid expression
---> xmlXPathEval: evaluation failed
--> Invalid expression
---> xmlXPathEval: evaluation failed
"""
err=""
Property changes on: xpathleak.py
___________________________________________________________________
Modified: svn:executable
## -0,0 +1 ##
+* <<<<<<<------------- Yes, there is a change, value * added to property svn:executable
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
================================
Now I generated the patch and applied it on another copy of the same branch.
UC xpathleak.py

rejected hunk ## -0,0 +1,1 ## (svn:executable)

Property conflicts: 1
The patch is rejected without changing the property. The destination file
already has svn:executable property present in it. But the value is empty.
Ideally, if the patch was applied properly, the value should have been
changed to "*".
After little bit of googling, I found the below link which looks like a similar issue.
https://urldefense.proofpoint.com/v2/url?u=https-3A__jira.atlassian.com_browse_CRUC-2D6114-3Fsrc-3Dconfmacro&d=DwIFaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=R5PFHc5rDhpXLKW9uD5GLI0W5asiYkw8WhxwHr2VyEY&m=2Csq4fBQTlRsi0n-Yy6XoE4sy-DbLVwSjkHoUJTGnGc&s=GOnZyVv3lNp9RPBWISwcxT6lg-OydhkL7p29RsFJcDo&e=
SVN Version : 1.8.11
I understand that the latest svn clients add * as the value of
svn:executable property. However, if we generate a patch out of this and
apply on an existing file, `svn patch` is not able to identify that there is
a property change from ‘null’ to ‘*’. Could you please check if this is a
bug?

SVN 1.8.11 is very old (version 1.8.x is actually end-of-life now
[1]). Can you test with a 1.10 client to see if it shows the same
behaviour?

[1] https://urldefense.proofpoint.com/v2/url?u=http-3A__subversion.apache.org_docs_release-2Dnotes_1.10.html-23svn-2D1.8-2Ddeprecation&d=DwIFaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=R5PFHc5rDhpXLKW9uD5GLI0W5asiYkw8WhxwHr2VyEY&m=2Csq4fBQTlRsi0n-Yy6XoE4sy-DbLVwSjkHoUJTGnGc&s=1HQYS3C3zQt48pqpi3F0MrOObkTvG

Daniel Shahaf

2018-06-28 14:43:05 UTC

Post by Dipu H
Looks like svn is not able to properly handle the svn:executable
property changes via patch.

Works for me:

[[[
% svnadmin create r
% svn co -q file://`pwd`/r wc
% cd wc
% touch iota
% svn add -q iota
% svn ci -q -m add
% svn up -q
% svn ps -q svn:executable yes iota
% svn diff > foo
% svn revert -q iota
% svn patch -q foo
% svn diff
Index: iota
===================================================================
--- iota (revision 1)
+++ iota (working copy)

Property changes on: iota
___________________________________________________________________
Added: svn:executable
## -0,0 +1 ##
+*
\ No newline at end of property
]]]

You mentioned earlier that svn:executable was present with an empty
value. Is that really the case? If it is --- which would be surprising,
as it's supposed to be an impossible state --- it would explain why you
got a conflict.

Dipu H

2018-06-28 15:15:24 UTC

Hi Daniel,

Thanks for the update. Yes, I have many files in the repository with "svn:executable" value set to "null". This is a very old repository, may be created using a very older version of svn. Older clients might have added the property without a value.

***@contrail-ubm-dipuh:/tmp/tests# svn pl xpathleak.py
Properties on 'xpathleak.py':
svn:executable
***@contrail-ubm-dipuh:/tmp/tests# svn pg svn:executable xpathleak.py

***@contrail-ubm-dipuh:/tmp/tests#

Thanks,
Dipu H

Post by Dipu H
Looks like svn is not able to properly handle the svn:executable
property changes via patch.

Philip Martin

2018-06-28 15:19:30 UTC

Post by Daniel Shahaf
You mentioned earlier that svn:executable was present with an empty
value. Is that really the case? If it is --- which would be surprising,
as it's supposed to be an impossible state --- it would explain why you
got a conflict.

The normalization of svn:executable values is implemented in the client.
If some client chooses not to implement that normalization the backend
will store any value given.

Should the svn client silently merge '' and '*' for svn:executable
values or should it be a conflict? I favour the conflict but I can see
arguments for a silent merge.

--
Philip

Dipu H

2018-06-28 15:23:35 UTC

Hi Philip,

It is a conflict. Values didn’t change after applying the patch.

***@contrail-ubm-dipuh:/tmp/tests# svn patch /tmp/patch
C xpathleak.py

rejected hunk ## -0,0 +1,1 ## (svn:executable)

Summary of conflicts:
Property conflicts: 1
***@contrail-ubm-dipuh:/tmp/tests# svn diff xpathleak.py
***@contrail-ubm-dipuh:/tmp/tests#

Thanks,
Dipu H

You mentioned earlier that svn:executable was present with an empty
value. Is that really the case? If it is --- which would be surprising,
as it's supposed to be an impossible state --- it would explain why you
got a conflict.

Daniel Shahaf

2018-06-29 17:59:42 UTC

Post by Dipu H
It is a conflict. Values didn’t change after applying the patch.
C xpathleak.py

rejected hunk ## -0,0 +1,1 ## (svn:executable)

Property conflicts: 1

You should probably find all files that have svn:executable set to empty and
change the property's value to "*", and commit that. That will sidestep the
issue with 'svn patch' modifying an empty property to non-empty that Julian has
described elsethread.

Cheers,

Daniel

Daniel Shahaf

2018-06-29 17:46:21 UTC

Post by Philip Martin
The normalization of svn:executable values is implemented in the client.
If some client chooses not to implement that normalization the backend
will store any value given.
Should the svn client silently merge '' and '*' for svn:executable
values or should it be a conflict? I favour the conflict but I can see
arguments for a silent merge.

Thinking out loud:

Since the normalization happens in the client, and since the client (evidently)
round-trips unnormalised values unmodified, in 1.11 we could implement
additional values for svn:executable (say, svn:executable=0755 and
svn:executable=0775 to set only the respective unix permission bits); both 1.10
and 1.11 clients would round-trip the new values; 1.11 clients would map the
new values to new semantics and 1.10 clients would map the new values to the
old semantics (0777 &~ umask).

That seems to be an argument in favour of raising a conflict.

Cheers,

Daniel
(It's Friday, though, so I may have made a logical leap of faith along the way… ☺)

Julian Foad

2018-06-28 17:22:46 UTC

Post by Dipu H
Property changes on: xpathleak.py
___________________________________________________________________
Modified: svn:executable
## -0,0 +1 ##
+* <<<<<<<------------- Yes, there is a change, value * added...
\ No newline at end of property

[...]

Post by Dipu H
The patch is rejected without changing the property. The destination file already has svn:executable property present in it. But the value is empty. Ideally, if the patch was applied properly, the value should have been changed to "*".

I tested this on trunk, and get the same conflict with a property named 'p' and value changing from empty to 'v'. It is not caused by special handling of the value '*'.

The bug seems to be that 'svn patch' fails to apply any patch of this form, that tries to change a property value from empty to non-empty.

- Julian

Julian Foad

2018-06-28 17:26:39 UTC

Post by Julian Foad
The bug seems to be that 'svn patch' fails to apply any patch of this form, that tries to change a property value from empty to non-empty.

I committed a test for this in http://svn.apache.org/r1834628

- Julian

Philip Martin

2018-06-28 18:03:55 UTC

Post by Julian Foad
The bug seems to be that 'svn patch' fails to apply any patch of
this form, that tries to change a property value from empty to
non-empty.

I committed a test for this in http://svn.apache.org/r1834628

I'm confused, you are treating as '' special? Suppose an existing
property P has value 'foo'. Should a patch that adds P with value 'bar'
simply merge and change the existing 'foo' to 'bar'? I think a conflict
is the correct outcome. You seem to be saying that if 'foo' is '' then
the conflict should not occur.

Given an existing property P with value 'foo' a patch that adds P with
value 'foo' would apply cleanly, it's an "already applied" patch. A
patch that modifes P from 'foo' to 'bar' would also apply cleanly, but
this latter patch would conflict if P does not exist.

I think the original question here is whether svn:executable should be
special and whether the add of svn:executable with value V1 should merge
with an existing svn:executable with a different value V2 and
automatically resolve to '*'. If it should merge, should there be any
restrictions? Should V1='foo' and V2='bar' resolve to '*'? Or only if
one of V1, V2 is '' and the other is '*'? Or something else?

I think a conflict is the right answer and there is no bug. I think
your new test is wrong.

--
Philip

Julian Foad

2018-06-28 20:08:09 UTC

Post by Julian Foad
The bug seems to be that 'svn patch' fails to apply any patch of
this form, that tries to change a property value from empty to
non-empty.

I committed a test for this in http://svn.apache.org/r1834628

I'm confused, you are treating as '' special? Suppose an existing
property P has value 'foo'. Should a patch that adds P with value 'bar'

The issue is about a patch that *changes* the current value to another value, not a patch that *adds* a property.

I am pointing out that a patch that changes the empty value '' to 'bar' should be (and isn't being) applied successfully to a property that already has the empty value ''.

The patch format for modifying an empty value is mostly the same as the patch format for adding a new property:

[[[
Property changes on: f
___________________________________________________________________
Added: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]
vs.
[[[
Property changes on: f
___________________________________________________________________
Modified: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]

It's similar to the ambiguous patch representation of create a file or add text to an empty file. In that case, 'svn patch' first creates the file if it doesn't exist, and in both scenarios adds the patch text.

For a property patch, a property-patch header line specifies whether it's a modify or an add, so there is no ambiguity. If the patch asks to 'modify' from empty to non-empty, when the target property already exists and has an empty value, this should certainly succeed.

- Julian

Philip Martin

2018-06-28 20:58:26 UTC

Post by Julian Foad
The issue is about a patch that *changes* the current value to another
value, not a patch that *adds* a property.

Yes, sorry. I misinterprted the patches.

--
Philip

Branko Čibej

2018-06-29 08:34:17 UTC

Post by Julian Foad
The bug seems to be that 'svn patch' fails to apply any patch of
this form, that tries to change a property value from empty to
non-empty.

I committed a test for this in http://svn.apache.org/r1834628

I'm confused, you are treating as '' special? Suppose an existing
property P has value 'foo'. Should a patch that adds P with value 'bar'

The issue is about a patch that *changes* the current value to another value, not a patch that *adds* a property.
I am pointing out that a patch that changes the empty value '' to 'bar' should be (and isn't being) applied successfully to a property that already has the empty value ''.
[[[
Property changes on: f
___________________________________________________________________
Added: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]
vs.
[[[
Property changes on: f
___________________________________________________________________
Modified: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]
It's similar to the ambiguous patch representation of create a file or add text to an empty file. In that case, 'svn patch' first creates the file if it doesn't exist, and in both scenarios adds the patch text.
For a property patch, a property-patch header line specifies whether it's a modify or an add, so there is no ambiguity. If the patch asks to 'modify' from empty to non-empty, when the target property already exists and has an empty value, this should certainly succeed.

However, when libsvn_client creates the svn:executable property, it
*always* sets its value to *. Applying a patch from empty to something
else will then very likely result in a conflict; because the
svn:executable property value cannot be empty (unless someone used a
broken client). This has been true since at least version 0.14., some 15
years ago.

-- Brane

Johan Corveleyn

2018-06-29 08:45:40 UTC

Post by Branko Äibej

Post by Julian Foad
The bug seems to be that 'svn patch' fails to apply any patch of
this form, that tries to change a property value from empty to
non-empty.

I committed a test for this in http://svn.apache.org/r1834628

I'm confused, you are treating as '' special? Suppose an existing
property P has value 'foo'. Should a patch that adds P with value 'bar'

The issue is about a patch that *changes* the current value to another value, not a patch that *adds* a property.
I am pointing out that a patch that changes the empty value '' to 'bar' should be (and isn't being) applied successfully to a property that already has the empty value ''.
[[[
Property changes on: f
___________________________________________________________________
Added: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]
vs.
[[[
Property changes on: f
___________________________________________________________________
Modified: empty
## -0,0 +1 ##
+foo
\ No newline at end of property
]]]
It's similar to the ambiguous patch representation of create a file or add text to an empty file. In that case, 'svn patch' first creates the file if it doesn't exist, and in both scenarios adds the patch text.
For a property patch, a property-patch header line specifies whether it's a modify or an add, so there is no ambiguity. If the patch asks to 'modify' from empty to non-empty, when the target property already exists and has an empty value, this should certainly succeed.

I think the cvs2svn conversion tool makes it possible to have an empty
svn:executable property. I don't remember if it does this by default,
or you need to do something special. I just recall we had this with an
early attempt at converting years ago (we ended up throwing that
version away, and replaced it with *).

I remember thinking at the time: "Why do we need to set it to *? The
docs only say the property needs to be set, but it doesn't require it
to be *, it can be _any value_ ... it's not because the default svn
client does so that it's required ..."

--
Johan

Branko Čibej

2018-06-29 08:58:49 UTC

Post by Johan Corveleyn

Post by Branko Äibej