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
Received on 2016-11-23 22:03:59 CET