diff on modified file in directory moved to newly added directory fails

[Alexander Belchenko]

This sequence of commands illustrate bug in 0.15final and bzr.dev:

bzr init
bzr mkdir d
echo spam > d/a
bzr add
bzr ci -m 1

bzr mkdir e
bzr mv d e
echo foo > e/d/a
bzr di

When I try to do see diff I get error:

C:\Temp\111>bzr di
=== added directory 'e'
=== renamed directory 'd' => 'e/d'
=== modified file 'e/d/a'
bzr: ERROR: exceptions.TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

Traceback (most recent call last):
  File "bzrlib\commands.pyc", line 650, in run_bzr_catch_errors
  File "bzrlib\commands.pyc", line 612, in run_bzr
  File "bzrlib\commands.pyc", line 304, in run_argv_aliases
  File "bzrlib\commands.pyc", line 622, in ignore_pipe
  File "bzrlib\builtins.pyc", line 1438, in run
  File "bzrlib\diff.pyc", line 375, in diff_cmd_helper
  File "bzrlib\diff.pyc", line 403, in show_diff_trees
  File "bzrlib\diff.pyc", line 475, in _show_diff_trees
  File "bzrlib\diff.pyc", line 488, in _patch_header_date
  File "bzrlib\timestamp.pyc", line 131, in format_patch_date
TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

bzr 0.15.0 on python 2.5.0.final.0 (win32)
arguments: ['C:\\Program Files\\Bazaar\\bzr.EXE', 'di']

** please send this report to <email address hidden>

This errors occurs only with dirstate working tree. With knit format -- all OK.

My system is Windows (as usual).

[Alexander Belchenko (bialix)]

Another user report about this bug in ML.

aLTer wrote:

$ bzr di>d.diff
bzr: ERROR: exceptions.TypeError: unsupported operand type(s) for +:
'NoneType' and 'int'

Traceback (most recent call last):
 File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line
650, in run_bzr_catch_errors
   return run_bzr(argv)
 File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line
612, in run_bzr
   ret = run(*run_argv)
 File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line
304, in run_argv_aliases
   return self.run(**all_cmd_args)
 File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line
622, in ignore_pipe
   result = func(*args, **kwargs)
 File "/usr/lib/python2.4/site-packages/bzrlib/builtins.py", line 1438, in run
   old_label=old_label, new_label=new_label)
 File "/usr/lib/python2.4/site-packages/bzrlib/diff.py", line 375, in
diff_cmd_helper
   extra_trees=extra_trees)
 File "/usr/lib/python2.4/site-packages/bzrlib/diff.py", line 403, in
show_diff_trees
   extra_trees=extra_trees)
 File "/usr/lib/python2.4/site-packages/bzrlib/diff.py", line 474, in
_show_diff_trees
   old_name = '%s%s\t%s' % (old_label, path,
 File "/usr/lib/python2.4/site-packages/bzrlib/diff.py", line 488, in
_patch_header_date
   return timestamp.format_patch_date(tree.get_file_mtime(file_id, path))
 File "/usr/lib/python2.4/site-packages/bzrlib/timestamp.py", line
131, in format_patch_date
   tm = time.gmtime(secs+offset)
TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

bzr 0.15.0 on python 2.4.4.candidate.0 (linux2)
arguments: ['/usr/bin/bzr', 'di']

[Alexander Belchenko (bialix)]

Also this effect occurs after merge branch with such changes.

[John A Meinel (jameinel)]

Alexander Belchenko wrote:
> Another user report about this bug in ML.
>
> aLTer wrote:
>
> $ bzr di>d.diff
> bzr: ERROR: exceptions.TypeError: unsupported operand type(s) for +:
> 'NoneType' and 'int'

It seems we are returning a timestamp of None (and I think the reason
was reasonable, but I'm not sure why now).

Otherwise I wasn't sure why it was failing.

I think the timestamp was None because the file on disk was gone (so
what timestamp would you give it?)

I'm not sure why it would have worked in the past...

It is something that we should work out, though.

John
=:->

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> Alexander Belchenko wrote:
>> Another user report about this bug in ML.
>>
>> aLTer wrote:
>>
>> $ bzr di>d.diff
>> bzr: ERROR: exceptions.TypeError: unsupported operand type(s) for +:
>> 'NoneType' and 'int'
>
> It seems we are returning a timestamp of None (and I think the reason
> was reasonable, but I'm not sure why now).
>
> Otherwise I wasn't sure why it was failing.
>
> I think the timestamp was None because the file on disk was gone (so
> what timestamp would you give it?)

No! File is not gone, file is moved, because parent directory is moved.

> I'm not sure why it would have worked in the past...

This bug exist only in dirstate-based working tree.

[µ]

[John A Meinel (jameinel)]

I have a fix for this in the associated branch.

[John A Meinel (jameinel)]

sorry, wrong bug.

[John A Meinel (jameinel)]

This is actually a latent bug in the current 'diff' codebase.

The reason we didn't experience it until DirState is because the old 'get_file_mtime()' for older trees ignored the 'path' parameter, and only used the file-id.

What is happening is that the 'diff' code passes both a 'file_id' and a 'path' to find the modification time of a file.

And what is happening is that the current path for the file is not the same as the old path. But it doesn't show up as a 'rename' because the directory was renamed, not the file itself.

The fix is to not give the 'path' hint, when we can't be fully sure that it really is the path:

=== modified file 'bzrlib/diff.py'
--- bzrlib/diff.py 2007-03-13 02:16:17 +0000
+++ bzrlib/diff.py 2007-04-11 21:27:57 +0000
@@ -472,7 +472,7 @@
         prop_str = get_prop_change(meta_modified)
         print >>to_file, '=== modified %s %r%s' % (kind, path.encode('utf8'), prop_str)
         old_name = '%s%s\t%s' % (old_label, path,
- _patch_header_date(old_tree, file_id, path))
+ _patch_header_date(old_tree, file_id, None))
         new_name = '%s%s\t%s' % (new_label, path,
                                  _patch_header_date(new_tree, file_id, path))

I'll write some tests, and submit it to the mailing list.

In the mean time, the fix will be in the associated branch.

[John A Meinel (jameinel)]

Oh, and this can be triggered by simply renaming a directory. Here is a simpler example

bzr init foo
cd foo
mkdir a
touch a/foo
bzr add
bzr commit -m 1
bzr mv a b
echo bar >> b/foo
bzr diff

[John A Meinel (jameinel)]

This bug has now been fixed in the associated branch (I should have just waited :)

I changed the code so it properly looks up the old path when a file is modified, so if you rename a directory the diff shows:

--- olddir/file
+++ newdir/file

And I added a bunch more direct tests of 'show_diff_trees' which seemed like it wasn't quite tested enough anyway.

[Alexander Belchenko (bialix)]

Already in bzr.dev