[PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Dmitry Pavlenko <pa...@tmatesoft.com>]

Hello again,
I'm attaching a reproducing test.

I'm not 100% sure that 

[
    'Index: %s\n' % sbox.path('iota'),
    '===================================================================\n',
    '--- %s\t(revision 1)\n' % sbox.path('iota'),
    '+++ %s\t(working copy)\n' % sbox.path('iota'),
    '@@ -1 +1 @@\n',
    '-This is the file \'iota\'.\n',
    '+link iota\n',
    '\ No newline at end of file\n',
    '\n',
    'Property changes on: iota\n',
    '___________________________________________________________________\n',
    'Added: svn:special\n',
    '## -0,0 +1 ##\n',
    '+*\n',
    '\ No newline at end of property\n',
  ]

is the expected output but definitely the diff command shouldn't fail with 
exit code 1 as it does now.
-- 
Dmitry Pavlenko,
TMate Software,
http://subgit.com/ - git-svn bridge

On среда, 11 июля 2018 г. 16:13:06 CEST Dmitry Pavlenko wrote:
> Hello.
> The following scenario fails for me: delete a file, create a symlink with
> the same name, run "svn diff". Instead of displaying the diff, SVN tries to
> read the link content. If "NOWHERE" variable is set to the file name, the
> symlink refers to itself, showing another error.
> 
> Instead I would expect something with "-" and "+" markers. If you uncomment
> #echo "link $NOWHERE" > $WC_PATH/trunk/file
> 
> and comment out "ln -s" call, the script will display approximately what I
> would expect (but without svn:special).
> 
> I'm not sure if this is a known bug, sorry for reporting something known if
> it is.
> 
> Locally I also have similar scenario failing in another way: before reading
> the symlink it displays deletion of "trunk/trunk/symlink" (yes, unexpectedly
> double "trunk", in my local test the file is called "symlink"). But I think
> it's the same issue. If it will still fail after this one is fixed, I'll
> analyze why.
> 
> 
> 
> #!/bin/sh
> 
> SVN=svn
> 
> #1. Create an empty SVN repository.
> 
> REPOSITORY_PATH="$PWD/svn.repo"
> 
> svnadmin create "$REPOSITORY_PATH"
> 
> # 2. Add a file to the repository.
> 
> WC_PATH="/tmp/wc"
> REPOSITORY_URL="file://$REPOSITORY_PATH"
> 
> $SVN co $REPOSITORY_URL $WC_PATH
> $SVN mkdir $WC_PATH/trunk
> 
> echo "content" > $WC_PATH/trunk/file
> 
> $SVN add $WC_PATH/trunk/file
> $SVN commit -m "A file added." $WC_PATH
> 
> # 3. Replace the file with a symlink pointing to nowhere (or to itself ---
> for another kind of error)
> 
> NOWHERE="changed/path"
> #NOWHERE="file"
> 
> rm $WC_PATH/trunk/file
> 
> ln -s $NOWHERE $WC_PATH/trunk/file
> 
> # By the way: uncomment the following line and comment out the previous one
> for comparison
> #echo "link $NOWHERE" > $WC_PATH/trunk/file
> 
> # 4. Create diff between repository HEAD and working copy:
> 
> $SVN diff --git $REPOSITORY_URL $WC_PATH
> 
> echo ""
> echo ""
> echo "-----------------------------------------"
> echo "If you see something like"
> echo "-content"
> echo "+link $NOWHERE"
> echo "And svn:special property set to '*'"
> echo "Then consider test passed"
> echo "-----------------------------------------"

Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Branko Čibej <br...@apache.org>]

On 20.07.2018 12:46, Julian Foad wrote:
> Julian Foad wrote:
>> Daniel Shahaf wrote:
>>> Dmitry Pavlenko wrote on Thu, Jul 19, 2018 at 19:03:30 +0200:
>>>> I'm not 100% sure that [...] is the expected output [...]
> [...]
>>> I suggest that we add a regression test that simply expects any output [...]
> But on Windows the buildbots have discovered that this 'diff' command does not error out, and so this test gives an XPASS result (which we choose to treat as an overall failure of the test suite).
>
> What to do? Add in some reasonable expectation?
>
> We need to design the expectation.
>
> Intuitively I expect a replacement of file with symlink to be presented as file deleted and symlink added, not as a content-modification (from normal file content to the 'special' representation of the symlink).
>
> Does that make sense, anyone? Does that fit with our architecture whereby knowledge of 'special files' is client-side?
>
> Presumably we would want a diff requested by the client to behave the same way, regardless whether the two items being diffed are located on the server and/or on the client. In contrast, if we request a diff through a server-layer tool such as 'svnadmin' then presumably it must display a content-modification and property-modification, because it is looking at the server-side representation of a symlink (using a 'special file').
>
> How does this fit with our ideas of object lifelines? Was that a replacement, starting a new lifeline, or not?

That's a tricky question. You can always add an svn:special property to
a file and modify its contents to the correct format. It's still the
same object; the server won't care.

It becomes a new object lifeline if the equivalent of the following
happened on the client:

   svn rm foo
   ln -s /some/thing foo
   svn add foo

Then it's just a replacement of file by file as far a the server is
concerned.

(Could be we have checks in the client that forbids manually adding
svn:special, but I can't recall).

-- Brane

Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Julian Foad <ju...@apache.org>]

Julian Foad wrote:
> Daniel Shahaf wrote:
> > Dmitry Pavlenko wrote on Thu, Jul 19, 2018 at 19:03:30 +0200:
> > > I'm not 100% sure that [...] is the expected output [...]
[...]
> > I suggest that we add a regression test that simply expects any output [...]

But on Windows the buildbots have discovered that this 'diff' command does not error out, and so this test gives an XPASS result (which we choose to treat as an overall failure of the test suite).

What to do? Add in some reasonable expectation?

We need to design the expectation.

Intuitively I expect a replacement of file with symlink to be presented as file deleted and symlink added, not as a content-modification (from normal file content to the 'special' representation of the symlink).

Does that make sense, anyone? Does that fit with our architecture whereby knowledge of 'special files' is client-side?

Presumably we would want a diff requested by the client to behave the same way, regardless whether the two items being diffed are located on the server and/or on the client. In contrast, if we request a diff through a server-layer tool such as 'svnadmin' then presumably it must display a content-modification and property-modification, because it is looking at the server-side representation of a symlink (using a 'special file').

How does this fit with our ideas of object lifelines? Was that a replacement, starting a new lifeline, or not?

-- 
- Julian

Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Julian Foad <ju...@apache.org>]

Daniel Shahaf wrote:
> Dmitry Pavlenko wrote on Thu, Jul 19, 2018 at 19:03:30 +0200:
> > I'm not 100% sure that [...] is the expected output [...]
> 
> There isn't a patch flying around that produces this output, right?

No, there isn't yet.

> I suggest that we add a regression test that simply expects any output [...]
> There should be an "@XFail()" decorator [...]
> Since this test doesn't commit, it can pass read_only=True [...]
> Please don't add new instances of backslash-space [...]

I made those tweaks and committed it:
  r1836336

Thanks for the patch!

-- 
- Julian

Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Bert Huijben <be...@qqmail.nl>]

For git style patches there is some magic that also allows creating
symlinks via the file mode. I think we have some test cases on that
behavior. you might be able to use this for building a regression test for
this case.

Bert

On Thu, Jul 19, 2018 at 8:51 PM, Daniel Shahaf <d....@daniel.shahaf.name>
wrote:

> Dmitry Pavlenko wrote on Thu, Jul 19, 2018 at 19:03:30 +0200:
> > I'm attaching a reproducing test.
> >
>
> Thanks for the test!
>
> > I'm not 100% sure that
> >
> > [
> >     'Index: %s\n' % sbox.path('iota'),
> >     '===========================================================
> ========\n',
> >     '--- %s\t(revision 1)\n' % sbox.path('iota'),
> >     '+++ %s\t(working copy)\n' % sbox.path('iota'),
> >     '@@ -1 +1 @@\n',
> >     '-This is the file \'iota\'.\n',
> >     '+link iota\n',
> >     '\ No newline at end of file\n',
> >     '\n',
> >     'Property changes on: iota\n',
> >     '___________________________________________________________
> ________\n',
> >     'Added: svn:special\n',
> >     '## -0,0 +1 ##\n',
> >     '+*\n',
> >     '\ No newline at end of property\n',
> >   ]
> >
> > is the expected output but definitely the diff command shouldn't fail
> with
> > exit code 1 as it does now.
>
> There isn't a patch flying around that produces this output, right?
>
> In this case, I suggest that we add a regression test that simply expects
> any
> output and exit code zero — that's «run_and_verify_svn(svntest.
> verify.AnyOutput,
> [], 'diff', wc_dir)» — and add a comment reminding us to write a more
> explicit
> expectation once the issue is fixed.  This would make sure the patch start
> passing as soon as we change the behaviour, even if the expected output
> predicted is a little off.
>
> > +++ subversion/tests/cmdline/diff_tests.py    (working copy)
> > @@ -5201,7 +5201,36 @@ def diff_summary_repo_wc_local_
> copy_unmodified(sbo
> >                      '--old=' + sbox.ospath('iota') + '@HEAD',
> >                      '--new=' + sbox.ospath('iota2'))
> >
> > +def diff_file_replaced_by_symlink(sbox):
>
> There should be an "@XFail()" decorator here, so `make test` (and
> `./diff_tests.py`) still exit 0 despite this test (X)FAILing.
>
> > +  "diff base vs working: symlink replaces a file"
> > +  sbox.build()
>
> Since this test doesn't commit, it can pass read_only=True, then build()
> would
> be cheaper.
>
> > +  svntest.actions.run_and_verify_svn([
> > +    '\ No newline at end of file\n',
>
> Please don't add new instances of backslash-space; it's an
> undefined/deprecated
> syntax that generates warnings in newer Pythons.  See r1834787.
>
> Cheers,
>
> Daniel
>

Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally

[Daniel Shahaf <d....@daniel.shahaf.name>]

Dmitry Pavlenko wrote on Thu, Jul 19, 2018 at 19:03:30 +0200:
> I'm attaching a reproducing test.
> 

Thanks for the test!

> I'm not 100% sure that 
> 
> [
>     'Index: %s\n' % sbox.path('iota'),
>     '===================================================================\n',
>     '--- %s\t(revision 1)\n' % sbox.path('iota'),
>     '+++ %s\t(working copy)\n' % sbox.path('iota'),
>     '@@ -1 +1 @@\n',
>     '-This is the file \'iota\'.\n',
>     '+link iota\n',
>     '\ No newline at end of file\n',
>     '\n',
>     'Property changes on: iota\n',
>     '___________________________________________________________________\n',
>     'Added: svn:special\n',
>     '## -0,0 +1 ##\n',
>     '+*\n',
>     '\ No newline at end of property\n',
>   ]
> 
> is the expected output but definitely the diff command shouldn't fail with 
> exit code 1 as it does now.

There isn't a patch flying around that produces this output, right?

In this case, I suggest that we add a regression test that simply expects any
output and exit code zero — that's «run_and_verify_svn(svntest.verify.AnyOutput,
[], 'diff', wc_dir)» — and add a comment reminding us to write a more explicit
expectation once the issue is fixed.  This would make sure the patch start
passing as soon as we change the behaviour, even if the expected output
predicted is a little off.

> +++ subversion/tests/cmdline/diff_tests.py	(working copy)
> @@ -5201,7 +5201,36 @@ def diff_summary_repo_wc_local_copy_unmodified(sbo
>                      '--old=' + sbox.ospath('iota') + '@HEAD',
>                      '--new=' + sbox.ospath('iota2'))
>  
> +def diff_file_replaced_by_symlink(sbox):

There should be an "@XFail()" decorator here, so `make test` (and
`./diff_tests.py`) still exit 0 despite this test (X)FAILing.

> +  "diff base vs working: symlink replaces a file"
> +  sbox.build()

Since this test doesn't commit, it can pass read_only=True, then build() would
be cheaper.

> +  svntest.actions.run_and_verify_svn([
> +    '\ No newline at end of file\n',

Please don't add new instances of backslash-space; it's an undefined/deprecated
syntax that generates warnings in newer Pythons.  See r1834787.

Cheers,

Daniel