[MERGE] update for dirstate.initialize test

Martin Pool mbp at sourcefrog.net
Fri Apr 20 02:28:53 BST 2007


On 4/20/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> >> In check_state_with_reopen:
> >
> >>        del state # Callers should unlock
> >
> >> not sure what that does, probably nothing...
>
> Actually, this is a bad comment. It *used* to be that callers should
> unlock. But because the 'check_state_with_reopen' is reading state, it
> needs to be passed in locked, and then we want to open it again, so it
> needs to unlock it.

I would just delete that line.  I think there's some small value in
del-ing variables when the object they refer to is dead, just for
clarity or to make sure it's not reused when the code is changed.  But
in this case it's reassigned on the very next line and so the del just
makes me puzzled.

> Also, my understanding of the failure was incorrect. It seems that on
> windows, if you open the same file 2 times, and lock one of the handles,
> you cannot read from the other handle.

It should be possible to put that into the windows-simulating
transport, if we expect more bugs like that.

> So my final rewrite is:
>
>     def test_initialize(self):
>         expected_result = ([], [
>             (('', '', 'TREE_ROOT'), # common details
>              [('d', '', 0, False, dirstate.DirState.NULLSTAT), # current
> tree
>              ])
>             ])
>         state = dirstate.DirState.initialize('dirstate')
>         try:
>             self.assertIsInstance(state, dirstate.DirState)
>             lines = state.get_lines()
>         finally:
>             state.unlock()
>         self.assertFileEqual(''.join(lines), 'dirstate')
>         state.lock_read() # check_state_with_reopen will unlock
>         self.check_state_with_reopen(expected_result, state)

That looks good to me, except for two things:

 - check_state_with_reopen says it wants a write-locked dirstate,
though I think it doesn't really care.  maybe we should fix its
docstring.

 - this changes the intention of the test slightly - i thought it was
trying to assert about the in-memory state before it was originally
unlocked.  i don't think this is really a problem.

+1 anyhow, just though I'd mention them.

-- 
Martin



More information about the bazaar mailing list