Re: [PATCH] Recreate test data for Git mirrors

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