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

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']

Also this effect occurs after merge branch with such changes.

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
=:->

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.

[µ]

I have a fix for this in the associated branch.

sorry, wrong bug.

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.

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

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.

Already in bzr.dev