Code review comment for lp:~spiv/bzr/hardlink-2a-408193

John A Meinel wrote:
> Review: Approve
> 46 + self.assertEqual(source_stat, target_stat)
>
> ^- It is generally better to use "self.assertEqualStat()".
>
> I realize it isn't strictly required here, since you aren't doing an fstat
> versus lstat, etc.

Ok, although there's no assertNotEqualStat, so I'll leave the corresponding
assertNotEqual as it is.

> 118 + os.mkdir(self.test_home_dir + '/.bazaar')
> 119 + rules_filename = self.test_home_dir + '/.bazaar/rules'
> 120 + f = open(rules_filename, 'w')
> 121 + f.write('[name %s]\nrot13=yes\n' % (pattern,))
> 122 + f.close()
>
> ^- Could you use ".build_tree()" and ".build_tree_contents()" here? I'll also
> note that you should at least use 'wb', since I don't think you want to get
> \r\n content on Windows. (maybe this doesn't matter as much since it is a
> config file?)

Good point about 'wb', changed (although I don't know the precise rules for
newlines in config files).

I cannot easily use build_tree_contents, because doesn't provide a convenient
way to build a tree relative to a dir other than '.'. Perhaps it should be
updated to take an optional transport and use put_bytes_non_atomic like
TestCase.build_tree? And I still need to remove or rename the file afterwards
before I call reset_rules()... probably that's something that should be handled
by the base TestCase, but it isn't at the moment.

> Would it be easier to use something like "fn = config.get_rules_filename()" ?
> I don't know how that path is currently determined, but it doesn't seem like
> you should be going via self.test_home_dir. (If there isn't an api for it,
> just submit a bug, and leave the test alone.)
>
> Otherwise I think the change is reasonable.

Hmm, there's bzrlib.rules.rules_filename(). I kind of like the safety of using
the test_home_dir explicitly though, it's easy to imagine a naïve optimisation
of rules_filename() caching and returning the wrong value — after all I need to
call reset_rules() because of similar caching. I might be too pessimistic
though. Anyway, I'd still need to makedirs(os.path.dirname(fn)), so it wouldn't
end up being much easier.

You do have a point that ideally I'd have essentially a one-liner:

     write_file(rules_filename(), '''...''')

And instead I have 5 lines, even ignoring the reset_rules call and cleanups.

So this is something to revisit if we write more tests that want to temporarily
install some rules, but I hope it's good enough for now.

-Andrew.