Images are binaries too.

Images are binaries too.

[iannucci, 2013-05-07 01:22:11]

+maruel

This change lgtm. Without it we're turning on universal newlines for binary
files when we get the file content on git.

[iannucci, 2013-05-07 01:29:00]

On 2013/05/07 01:22:11, iannucci wrote:
> +maruel
> 
> This change lgtm. Without it we're turning on universal newlines for binary
> files when we get the file content on git.

Actually on second thought, this code may just be broken. is_binary depends on
base_content, but the correct value of base_content depends on is_binary.

It may be better to change git's GetFileContent to do the binary check itself.

[M-A, 2013-05-07 01:45:27]

https://codereview.appspot.com/9183044/diff/1/upload.py
File upload.py (right):

https://codereview.appspot.com/9183044/diff/1/upload.py#newcode1599
upload.py:1599: is_binary = self.IsBinaryData(base_content) or is_image
This helps so lgtm but it's not sufficient. It should ask the SCM for metadata
when available.
https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html#_marking_...
but I'm not sure how to ask git about that auto-detection. Sadly,
http://stackoverflow.com/questions/6119956/how-to-determine-if-git-handles-a-...
seems to imply the heuristic is quite simple. IMHO, anything that is not valid
UTF-8 should be treated as binary. In chromium we enforce UTF-8 files throughout
the TS and CQ for sanity purposes.

[jparent1, 2013-05-07 21:53:56]

If that was a LG, can someone with commit access check this in for me?

On 2013/05/07 01:45:27, M-A wrote:
> https://codereview.appspot.com/9183044/diff/1/upload.py
> File upload.py (right):
> 
> https://codereview.appspot.com/9183044/diff/1/upload.py#newcode1599
> upload.py:1599: is_binary = self.IsBinaryData(base_content) or is_image
> This helps so lgtm but it's not sufficient. It should ask the SCM for metadata
> when available.
>
https://www.kernel.org/pub/software/scm/git/docs/gitattributes.html#_marking_...
> but I'm not sure how to ask git about that auto-detection. Sadly,
>
http://stackoverflow.com/questions/6119956/how-to-determine-if-git-handles-a-...
> seems to imply the heuristic is quite simple. IMHO, anything that is not valid
> UTF-8 should be treated as binary. In chromium we enforce UTF-8 files
throughout
> the TS and CQ for sanity purposes.

[iannucci, 2013-05-07 23:06:24]

On 2013/05/07 21:53:56, jparent1 wrote:
> If that was a LG, can someone with commit access check this in for me?

Yup I'll be happy to land this

[iannucci, 2013-05-07 23:06:24]

On 2013/05/07 21:53:56, jparent1 wrote:
> If that was a LG, can someone with commit access check this in for me?

Yup I'll be happy to land this

[iannucci, 2013-05-07 23:53:25]

On 2013/05/07 23:06:24, iannucci wrote:
> On 2013/05/07 21:53:56, jparent1 wrote:
> > If that was a LG, can someone with commit access check this in for me?
> 
> Yup I'll be happy to land this

Landed on default as:
https://code.google.com/p/rietveld/source/detail?r=5e407a9de5b91bcd53759997a8...
Landed on chromium as:
https://code.google.com/p/rietveld/source/detail?r=38317077662667fca6cf856949...

jparent@, you can close this CL now