Re: [PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally
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 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
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
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
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
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
[PATCH] Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally
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 "-"
Index: subversion/tests/cmdline/diff_tests.py
===
--- subversion/tests/cmdline/diff_tests.py (revision 1836275)
+++ 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):
+ "diff base vs working: symlink replaces a file"
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ iota_path = sbox.ospath('iota')
+ os.remove(iota_path)
+
+ # create a symlink pointing to itself
+ # alternatively it could point to a non-existing path
+ sbox.simple_symlink('iota', 'iota')
+
+ svntest.actions.run_and_verify_svn([
+'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',
+ ], [], 'diff', wc_dir)
+
#Run the tests
@@ -5300,6 +5329,7 @@ test_list = [ None,
diff_unversioned_files_git,
diff_summary_repo_wc_local_copy,
diff_summary_repo_wc_local_copy_unmodified,
+ diff_file_replaced_by_symlink,
]
if __name__ == '__main__':
Re: "svn diff" doesn't work correctly if a file is replaced with a symlink locally
Thanks for the report. It would be great if you could file an issue and write a test for this problem. - Julian Dmitry Pavlenko wrote on 2018-07-11: > 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: "svn diff" doesn't work correctly if a file is replaced with a symlink locally
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 "-" -- Dmitry Pavlenko, TMate Software, http://subgit.com/ - git-svn bridge
