My favorites | Sign in
Project Home Wiki Issues
New issue   Search
for
  Advanced search   Search tips
Issue 1173: Binary files aren't listed in the diff?
1 person starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  chip...@gmail.com
Closed:  Oct 2009


Sign in to add a comment
 
Reported by jamesd...@gmail.com, Jun 17, 2009
*NOTE: Do not post confidential information in this bug report.*

What version are you running?
chipx86 says 1.0RC3.

What's the URL of the page containing the problem?


What steps will reproduce the problem?
1. Create a review that includes adds or edits to binary files.
2. View the diff.

What is the expected output? What do you see instead?
The binary files are listed among the "Files changed" momentarily with
throbber icons, but after they're fetched, they're removed.

Although we can't show diffs of binary files, listing them (and showing
whether they're added, deleted, or edited) is important. (How else can you
know if someone forgot to include a required binary file in their change?)


What operating system are you using? What browser?
Windows XP x64 SP2, Firefox 3.0.11

Please provide any additional information below.

Comment 1 by m...@roboss.com, Jun 30, 2009
Unicode encoded files don't diff and are treated as binary files.  Not seeing them in
the diff can be dangerous.  It's good to know what's touched and what's not.
Comment 2 by erich...@gmail.com, Oct 6, 2009
I looked at this briefly.  The diff fragment URL is telling it to remove the entry. 
From a URL like this: /r/10594/diff/1/fragment/113257/?index=1&1254766381
I got the following HTML code:

</table>
<script type="text/javascript">
  $(document).ready(function() {

    $("li.change_file_1").remove();

  });
</script>

from the bottom of diff_file_fragment.html.  Also, as a side issue, the </table>
probably shouldn't be there.  It looks like it's trying to explicitly remove changes
that don't have change_chunks, presumably for the interfilediff case where you don't
want to show files that have no interdiff changes.  If that is correct, maybe the
check for "if file.changed_chunks" should also be checking "if not file.interfilediff"?
Comment 3 by project member chip...@gmail.com, Oct 6, 2009
That seems to be the problem. We should get this in for 1.0.5.
Status: Confirmed
Labels: Milestone-Release1.0.x Component-DiffViewer
Comment 4 by project member chip...@gmail.com, Oct 6, 2009
(No comment was entered for this change.)
Labels: -Priority-Medium Priority-High
Comment 5 by project member chip...@gmail.com, Oct 13, 2009
Fixed in master (reb80a58) and release-1.0.x (reb80a58)
Status: Fixed
Owner: chipx86
Comment 6 by jamesd...@gmail.com, Dec 4, 2009
Hm, so it looks like binary files now show up, but AFAICT there's no indication
whether they're added/edited/deleted.
Sign in to add a comment

Powered by Google Project Hosting