"KeyError: None" in Dirstate._entry_to_line when commiting

[Gary van der Merwe]

Steps to reproduce:

create new bzr branch
add file
bzr commit -m "test"

Actual result:

Committing revision 1 to "D:/My Documents/test/".
added NEWS
Committed revision 1.
bzr: ERROR: exceptions.KeyError: None

Traceback (most recent call last):
  File "D:\My Documents\bzr\bzr.dev\bzrlib\commands.py", line 802, in run_bzr_catch_errors
    return run_bzr(argv)
  File "D:\My Documents\bzr\bzr.dev\bzrlib\commands.py", line 758, in run_bzr
    ret = run(*run_argv)
  File "D:\My Documents\bzr\bzr.dev\bzrlib\commands.py", line 492, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "D:\My Documents\bzr\bzr.dev\bzrlib\builtins.py", line 2308, in run
    author=author)
  File "D:\My Documents\bzr\bzr.dev\bzrlib\decorators.py", line 167, in write_locked
    self.unlock()
  File "D:\My Documents\bzr\bzr.dev\bzrlib\workingtree_4.py", line 1126, in unlock
    self.flush()
  File "D:\My Documents\bzr\bzr.dev\bzrlib\workingtree_4.py", line 297, in flush
    self.current_dirstate().save()
  File "D:\My Documents\bzr\bzr.dev\bzrlib\dirstate.py", line 1641, in save
    self._state_file.writelines(self.get_lines())
  File "D:\My Documents\bzr\bzr.dev\bzrlib\dirstate.py", line 1228, in get_lines
    lines.extend(map(self._entry_to_line, self._iter_entries()))
  File "D:\My Documents\bzr\bzr.dev\bzrlib\dirstate.py", line 995, in _entry_to_line
    entire_entry[tree_offset + 3] = DirState._to_yesno[tree_data[3]]
KeyError: None

bzr 0.92.0.dev.0 on python 2.5.1.final.0 (win32)
arguments: ['D:\\My Documents\\bzr\\bzr.dev\\bzr', 'commit', '-m', 'test']
encoding: 'cp1252', fsenc: 'mbcs', lang: None
plugins:
  extmerge d:\My Documents\bzr-extmerge\trunk\extmerge [unknown]
  launchpad D:\My Documents\bzr\bzr.dev\bzrlib\plugins\launchpad [unknown]
  multiparent D:\My Documents\bzr\bzr.dev\bzrlib\plugins\multiparent.pyc [unknown]

** Please send this report to <email address hidden>
   with a description of what you were doing when the
   error occurred.
D:\My Documents\bzr\bzr.dev\bzrlib\lockable_files.py:110: UserWarning: file group LockableFiles(<bzrlib.transport.local.
LocalTransport url=file:///D:/My%20Documents/test/.bzr/checkout/>) was not explicitly unlocked
  warn("file group %r was not explicitly unlocked" % self)
D:\My Documents\bzr\bzr.dev\bzrlib\lock.py:79: UserWarning: lock on <open file u'D:/My Documents/testbzr/.bzr/checkout/d
irstate', mode 'rb+' at 0x00FC0BF0> not released
  warn("lock on %r not released" % self.f)

[Martin Pool (mbp)]

thankyou for the report. is this always reproducible? your instructions say 'add file', were you always adding the same file?

[Gary van der Merwe (garyvdm)]

Yes - it's all ways reproducible. No - different files.

I have isolated the regression as been introduced at revision 2879. (http://codebrowse.launchpad.net/~bzr/bzr/trunk/revision/pqm%40pqm.ubuntu.com-20071003050442-e0x9ofdfo0hwxnal) Hence assigning to lifeless.

[Alexander Belchenko (bialix)]

This bug 100% reproducible in selftest on windows: right now I have more than 1000 failing tests. IMO, it's breaks windows support.

[Alexander Belchenko (bialix)]

Analysis of problem from John:

Alexander Belchenko wrote:
> > When I'm testing the recent patch from Lukas I found that almost
> > ALL blackbox non-ascii tests are failed with the same error KeyError:

...

> > File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\dirstate.py", line 1231, in
> > get_lines
> > lines.extend(map(self._entry_to_line, self._iter_entries()))
> > File "C:\work\Bazaar\mydev\bzr.dev\bzrlib\dirstate.py", line 1000, in
> > _entry_to_line
> > entire_entry[tree_offset + 3] = DirState._to_yesno[tree_data[3]]
> > KeyError: None
> >
> > ----------------------------------------------------------------------
> >
> >
> > I'll try to run full selftest ASAP, but it seems that there is a BIG
> > regression on win32, either in bzrlib or tests.
> >
> > Alexander

It looks like something is setting "executable = None" rather than "executable
= True" or "executable = False".

Just to check, do you get the same result if you run "bzr selftest --no-plugins
..." ?

Otherwise I don't see how it is happening. When we "Dirstate.add()" we set the
executable field to False. And on win32 when we update_entry we use:

def _is_executable_win32(self, mode, saved_executable):
  return saved_executable

I agree that this seems like a serious regression, I just don't quite
understand what is causing tree_data[3] to be set to None.

Ah.... maybe this one:

def _inv_entry_to_details(self, inv_entry):
...
        elif kind == 'file':
            fingerprint = inv_entry.text_sha1 or ''
            size = inv_entry.text_size or 0
            executable = inv_entry.executable

We could try:

  executable = inv_entry.executable or False

Does that fix the problem? I'm not sure why inv_entry.executable isn't being
set. Maybe we removed one of the "_read_state_from_tree()" calls, or something
else that was supposed to set it.

Can you investigate further?

John
=:->

[Ian Clatworthy (ian-clatworthy)]

This might have been broken by the path_content_summary changes. Rev 2879 changed commit to call this new routine. The code itself for path_content_summary was introduced in 2862. It needs more digging but the place that stands out as suspect is the implementation of path_content_summary in workingtree.py, particularly this bit:

            if not supports_executable():
                executable = None # caller can decide policy.
            else:
                mode = stat_result.st_mode
                executable = bool(stat.S_ISREG(mode) and stat.S_IEXEC & mode)

For win32 systems, this will always set executable to None. So either this bit of code is wrong, or the code calling it needs to do something intelligent when it gets back the combination of "kind is 'file' and executable is None".

[John A Meinel (jameinel)]

I have to try this on a real win32 machine, but I think this patch may fix it
=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py 2007-10-16 02:42:33 +0000
+++ bzrlib/repository.py 2007-10-16 19:16:50 +0000
@@ -313,6 +313,7 @@
         # ancestors, we write a new node.
         if len(heads) != 1:
             store = True
+ parent_entry = None
         if not store:
             # There is a single head, look it up for comparison
             parent_entry = parent_candiate_entries[heads[0]]
@@ -327,6 +328,11 @@
             if kind != parent_entry.kind:
                 store = True
         if kind == 'file':
+ if content_summary[2] is None:
+ if parent_entry is not None:
+ content_summary[2] == parent_entry.executable
+ else:
+ content_summary[2] = False
             if not store:
                 if (# if the file length changed we have to store:
                     parent_entry.text_size != content_summary[1] or

I would really rather prefer it if Ian or Robert worked out the correct location for this sort of fix.

[John A Meinel (jameinel)]

For the record, that should be "
if parent_entry is not None:
  content_summary[2] = parent_entry.executable
else:
  content_summary[2] = False
"

Just a bit of a typo on my part.

[John A Meinel (jameinel)]

The fix is available in the associated branch.
It isn't quite in the right location, but at least it gets things working again. I'll try to update the branch for a more correct fix.

[John A Meinel (jameinel)]

Submitted as bzr.dev 2918