You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Patrick Steinhardt <pa...@elegosoft.com> on 2016/11/07 13:28:08 UTC
[PATCH] Recreate test data for Git mirrors
Hi,
attached is a patch that fixes a test for Git mirrors. The error
results from the fact that Git does not track empty directories,
which one test relies upon.
As we're already doing some path processing for the missing
directories and as we're also creating directories in the other
import tests, I think this is the most sensible place to fix the
issue. For Subversion working copies it becomes a no-op anyway as
the directories already exist.
Regards
Patrick
[[[
tests: fix import tests for git mirrors
Subversion's import tests test behavior when importing empty
directories. These empty directories are checked in as part of
our test data. This however breaks our import tests when using
Subversion's Git mirror at git.apache.org, as Git is unable to
version empty directories.
As our tests rely on the fact that these directories are indeed
empty it is not possible to work around the issue by creating a
`.gitkeep` file inside these directories, as is commonly done
with git. Instead, the issue is fixed inside the tests itself by
creating the directories if they do not already exist. This is in
fact a no-op for Subversion working copies and fixes tests for
Git repositories.
* subversion/tests/cmdline/import_tests.py:
(import_inherited_ignores): Create empty directories if they do
not exist yet.
]]]
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH] Recreate test data for Git mirrors
Posted by Patrick Steinhardt <pa...@elegosoft.com>.
On Thu, Nov 24, 2016 at 11:48:23AM +0000, Julian Foad wrote:
> Julian Foad wrote:
> > Daniel Shahaf wrote:
> >> My one comment about your patch is that the import tree should be
> >> created under svn-test-work/local_tmp/, rather than under
> >> import_tests_data/; I believe the latter should be considered read-only.
> >
> > Oh yes! I forgot to call that out. I'll re-work it.
>
> OK, committed in http://svn.apache.org/r1771118
> using most of your log message Patrick. Thank you for flagging this issue.
>
> - Julian
Thanks to you both!
Patrick
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH] Recreate test data for Git mirrors
Posted by Julian Foad <ju...@foad.me.uk>.
Julian Foad wrote:
> Daniel Shahaf wrote:
>> My one comment about your patch is that the import tree should be
>> created under svn-test-work/local_tmp/, rather than under
>> import_tests_data/; I believe the latter should be considered read-only.
>
> Oh yes! I forgot to call that out. I'll re-work it.
OK, committed in http://svn.apache.org/r1771118
using most of your log message Patrick. Thank you for flagging this issue.
- Julian
Re: [PATCH] Recreate test data for Git mirrors
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Julian Foad wrote on Wed, Nov 23, 2016 at 21:03:52 +0000:
>> Would anyone else care to comment, and review my patch if you think it looks
>> good in principle, please?
>
> Thanks for the ping.
>
> Both your approach and Patrick's approach would solve the original
> problem, namely, would allow running 'make check' from a git-svn
> "working copy". (They would also allow future archeologists to run
> 'make check' on historical revisions, if we ever reopen INFRA-7524.)
>
> However, I think the "mkdir everything in Python" approach would be
> easier to maintain in the long run: that approach results in one
> codepath, whereas Patrick's approach results in two codepaths.
Thanks, Daniel. That's how I see it, too, and "one code path" is a good
way of describing it.
> My one comment about your patch is that the import tree should be
> created under svn-test-work/local_tmp/, rather than under
> import_tests_data/; I believe the latter should be considered read-only.
Oh yes! I forgot to call that out. I'll re-work it.
- Julian
Re: [PATCH] Recreate test data for Git mirrors
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, Nov 23, 2016 at 21:03:52 +0000:
> Would anyone else care to comment, and review my patch if you think it looks
> good in principle, please?
Thanks for the ping.
Both your approach and Patrick's approach would solve the original
problem, namely, would allow running 'make check' from a git-svn
"working copy". (They would also allow future archeologists to run
'make check' on historical revisions, if we ever reopen INFRA-7524.)
However, I think the "mkdir everything in Python" approach would be
easier to maintain in the long run: that approach results in one
codepath, whereas Patrick's approach results in two codepaths.
My one comment about your patch is that the import tree should be
created under svn-test-work/local_tmp/, rather than under
import_tests_data/; I believe the latter should be considered read-only.
Cheers,
Daniel
Re: [PATCH] Recreate test data for Git mirrors
Posted by Julian Foad <ju...@apache.org>.
Patrick Steinhardt wrote:
> On Wed, Nov 16, 2016 at 04:30:55PM +0000, Julian Foad wrote:
>> On 07/11/16, Patrick Steinhardt wrote:
>>> attached is a patch that fixes a test for Git mirrors. The error
>>> results from the fact that Git does not track empty directories,
>>> which one test relies upon.
>>>
>>> As we're already doing some path processing for the missing
>>> directories and as we're also creating directories in the other
>>> import tests, I think this is the most sensible place to fix the
>>> issue. For Subversion working copies it becomes a no-op anyway as
>>> the directories already exist.
>>
>> I just took a quick look at this. Your patch is of a kind which would
>> have to be repeated for each new test that uses the same approach. It
>> also seems a bit redundant: that small extra bit of code is almost
>> enough code to create the whole tree from scratch.
>>
>> It seems to me that storing what is supposed to be a plain unversioned
>> disk tree within our Subversion versioned tree isn't the right approach.
>> Instead I think it would be better to store the import data tree as a
>> Python data structure within the test and create it at run time. If
>> there isn't already a handy function for doing this, there should be.
>>
>> Something like this:
>>
>> [[[
>> Index: subversion/tests/cmdline/import_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/import_tests.py (revision 1767744)
>> +++ subversion/tests/cmdline/import_tests.py (working copy)
>> @@ -446,8 +446,8 @@ def import_inherited_ignores(sbox):
>> # DIR7
>> # file7.foo
>> # DIR8.noo
>> - import_tree_dir = os.path.join(os.path.dirname(sys.argv[0]),
>> - 'import_tests_data', 'import_tree')
>> + import_tree_path = 'import_tree'
>> + import_tree_dir = os.path.join(sbox.wc_dir, import_tree_path)
>>
>> # Relative WC paths of the imported tree.
>> dir1_path = os.path.join('DIR1.noo')
>> @@ -466,6 +466,31 @@ def import_inherited_ignores(sbox):
>> file7_path = os.path.join('DIR6', 'DIR7', 'file7.foo')
>> dir8_path = os.path.join('DIR6', 'DIR7', 'DIR8.noo')
>>
>> + import_dirs = [os.path.join(import_tree_path, p) for p in [
>> + dir1_path,
>> + dir2_path,
>> + dir3_path,
>> + dir4_path,
>> + dir5_path,
>> + dir6_path,
>> + dir7_path,
>> + dir8_path,
>> + ]]
>> + import_files = [os.path.join(import_tree_path, p) for p in [
>> + file1_path,
>> + file2_path,
>> + file3_path,
>> + file4_path,
>> + file5_path,
>> + file6_path,
>> + file7_path,
>> + ]]
>> +
>> + sbox.simple_mkdir(import_tree_path)
>> + sbox.simple_mkdir(*[svntest.wc.to_relpath(p) for p in import_dirs])
>> + sbox.simple_add_text('A file',
>> + *[svntest.wc.to_relpath(p) for p in import_files])
>> +
>> # Import the tree to ^/A/B/E.
>> # We should not see any *.noo paths because those are blocked at the
>> # root of the repository by the svn:global-ignores property. Likewise
>> ]]]
>>
>> and:
>>
>> svn rm subversion/tests/cmdline/import_tests_data
>>
>> Does that work for you?
>
> It certainly would, yes. I opted for the other path as Git
> mirrors are not really something Subversion developers care
> about, so I wanted to keep the patch as small as possible. If
> your approach is preferred, though, I don't mind to take that one
> instead.
Would anyone else care to comment, and review my patch if you think it
looks good in principle, please?
- Julian
Re: [PATCH] Recreate test data for Git mirrors
Posted by Patrick Steinhardt <ps...@pks.im>.
On Wed, Nov 16, 2016 at 04:30:55PM +0000, Julian Foad wrote:
> On 07/11/16, Patrick Steinhardt wrote:
> > attached is a patch that fixes a test for Git mirrors. The error
> > results from the fact that Git does not track empty directories,
> > which one test relies upon.
> >
> > As we're already doing some path processing for the missing
> > directories and as we're also creating directories in the other
> > import tests, I think this is the most sensible place to fix the
> > issue. For Subversion working copies it becomes a no-op anyway as
> > the directories already exist.
>
> I just took a quick look at this. Your patch is of a kind which would
> have to be repeated for each new test that uses the same approach. It
> also seems a bit redundant: that small extra bit of code is almost
> enough code to create the whole tree from scratch.
>
> It seems to me that storing what is supposed to be a plain unversioned
> disk tree within our Subversion versioned tree isn't the right approach.
> Instead I think it would be better to store the import data tree as a
> Python data structure within the test and create it at run time. If
> there isn't already a handy function for doing this, there should be.
>
> Something like this:
>
> [[[
> Index: subversion/tests/cmdline/import_tests.py
> ===================================================================
> --- subversion/tests/cmdline/import_tests.py (revision 1767744)
> +++ subversion/tests/cmdline/import_tests.py (working copy)
> @@ -446,8 +446,8 @@ def import_inherited_ignores(sbox):
> # DIR7
> # file7.foo
> # DIR8.noo
> - import_tree_dir = os.path.join(os.path.dirname(sys.argv[0]),
> - 'import_tests_data', 'import_tree')
> + import_tree_path = 'import_tree'
> + import_tree_dir = os.path.join(sbox.wc_dir, import_tree_path)
>
> # Relative WC paths of the imported tree.
> dir1_path = os.path.join('DIR1.noo')
> @@ -466,6 +466,31 @@ def import_inherited_ignores(sbox):
> file7_path = os.path.join('DIR6', 'DIR7', 'file7.foo')
> dir8_path = os.path.join('DIR6', 'DIR7', 'DIR8.noo')
>
> + import_dirs = [os.path.join(import_tree_path, p) for p in [
> + dir1_path,
> + dir2_path,
> + dir3_path,
> + dir4_path,
> + dir5_path,
> + dir6_path,
> + dir7_path,
> + dir8_path,
> + ]]
> + import_files = [os.path.join(import_tree_path, p) for p in [
> + file1_path,
> + file2_path,
> + file3_path,
> + file4_path,
> + file5_path,
> + file6_path,
> + file7_path,
> + ]]
> +
> + sbox.simple_mkdir(import_tree_path)
> + sbox.simple_mkdir(*[svntest.wc.to_relpath(p) for p in import_dirs])
> + sbox.simple_add_text('A file',
> + *[svntest.wc.to_relpath(p) for p in import_files])
> +
> # Import the tree to ^/A/B/E.
> # We should not see any *.noo paths because those are blocked at the
> # root of the repository by the svn:global-ignores property. Likewise
> ]]]
>
> and:
>
> svn rm subversion/tests/cmdline/import_tests_data
>
> Does that work for you?
It certainly would, yes. I opted for the other path as Git
mirrors are not really something Subversion developers care
about, so I wanted to keep the patch as small as possible. If
your approach is preferred, though, I don't mind to take that one
instead.
Regards
Patrick
Re: [PATCH] Recreate test data for Git mirrors
Posted by Julian Foad <ju...@apache.org>.
On 07/11/16, Patrick Steinhardt wrote:
> attached is a patch that fixes a test for Git mirrors. The error
> results from the fact that Git does not track empty directories,
> which one test relies upon.
>
> As we're already doing some path processing for the missing
> directories and as we're also creating directories in the other
> import tests, I think this is the most sensible place to fix the
> issue. For Subversion working copies it becomes a no-op anyway as
> the directories already exist.
I just took a quick look at this. Your patch is of a kind which would
have to be repeated for each new test that uses the same approach. It
also seems a bit redundant: that small extra bit of code is almost
enough code to create the whole tree from scratch.
It seems to me that storing what is supposed to be a plain unversioned
disk tree within our Subversion versioned tree isn't the right approach.
Instead I think it would be better to store the import data tree as a
Python data structure within the test and create it at run time. If
there isn't already a handy function for doing this, there should be.
Something like this:
[[[
Index: subversion/tests/cmdline/import_tests.py
===================================================================
--- subversion/tests/cmdline/import_tests.py (revision 1767744)
+++ subversion/tests/cmdline/import_tests.py (working copy)
@@ -446,8 +446,8 @@ def import_inherited_ignores(sbox):
# DIR7
# file7.foo
# DIR8.noo
- import_tree_dir = os.path.join(os.path.dirname(sys.argv[0]),
- 'import_tests_data', 'import_tree')
+ import_tree_path = 'import_tree'
+ import_tree_dir = os.path.join(sbox.wc_dir, import_tree_path)
# Relative WC paths of the imported tree.
dir1_path = os.path.join('DIR1.noo')
@@ -466,6 +466,31 @@ def import_inherited_ignores(sbox):
file7_path = os.path.join('DIR6', 'DIR7', 'file7.foo')
dir8_path = os.path.join('DIR6', 'DIR7', 'DIR8.noo')
+ import_dirs = [os.path.join(import_tree_path, p) for p in [
+ dir1_path,
+ dir2_path,
+ dir3_path,
+ dir4_path,
+ dir5_path,
+ dir6_path,
+ dir7_path,
+ dir8_path,
+ ]]
+ import_files = [os.path.join(import_tree_path, p) for p in [
+ file1_path,
+ file2_path,
+ file3_path,
+ file4_path,
+ file5_path,
+ file6_path,
+ file7_path,
+ ]]
+
+ sbox.simple_mkdir(import_tree_path)
+ sbox.simple_mkdir(*[svntest.wc.to_relpath(p) for p in import_dirs])
+ sbox.simple_add_text('A file',
+ *[svntest.wc.to_relpath(p) for p in import_files])
+
# Import the tree to ^/A/B/E.
# We should not see any *.noo paths because those are blocked at the
# root of the repository by the svn:global-ignores property. Likewise
]]]
and:
svn rm subversion/tests/cmdline/import_tests_data
Does that work for you?
- Julian