Fix coloring madness in depot_tools.

Fix coloring madness in depot_tools.

'setup_color' now contains logic to correctly detect:
  * cmd
  * cmd pipe
  * msys bash
  * msys pipe
  * cmd running inside msys bash (git-command)
  * cmd pipe running inside msys bash (git-command > outfile)

R=brucedawson@chromium.org, dnj@chromium.org
BUG=600049
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299682

[iannucci, 2016-04-02 03:00:29]

On 2016/04/02 at 02:59:41, iannucci wrote:
> 

(not quite ready, need to test on linux/mac, though I think it'll just be
guarding away the msys ptty detection)

[iannucci, 2016-04-02 05:42:12]

OK, PTAL.

[iannucci, 2016-04-02 06:08:21]

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py
File setup_color.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:18: OUT_TYPE = 'console'
yay, we detected it via the normal mechanism! This can happen when python is
attached directly to a tty (shell, cmd).

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:20: OUT_TYPE = 'pipe'
Assume it's a pipe

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:24: h = msvcrt.get_osfhandle(sys.stdout.fileno())
h is the win32 HANDLE for stdout.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:26: if ftype == 2: # FILE_TYPE_CHAR
This happens when were running in cmd but via some `git command` (which is
git->bash->command). I'm not exactly sure why 'isatty' doesn't detect this case,
but it definitely doesn't.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:29: elif ftype == 3: # FILE_TYPE_PIPE
This happens if we're running under bash in interactive-mode (e.g. under the
`git bash` shell).

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:45: ('NameBuffer', ctypes.c_wchar_p)]
These are all defined in the MSDN documentation under the same names.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:49: if NT_SUCCESS(ctypes.windll.ntdll.NtQueryObject(h, 1, buf,
len(buf)-2,
Ask NT what the name of thing attached to the stdout HANDLE is. ctypes is
awesome.

GetFileInformationByHandleEx is another way to get this, but it's only available
on Vista+... which should probably be OK, but the method above is the way that
git-on-windows does it so *shrug*.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:54: IS_TTY = name.startswith('msys-') and '-pty' in name
The full name is something like "\Device\NamedPipe\msys-*-pty*", so we just care
about the last bit.

If it's in the msys shell and like `some_process | cat`, some_process will see
just a randomized pipe name (not a pty).

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:55: should_wrap = False
either way, we don't need to do the weird win32 syscalls to change the console
state.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:58: else:
Not on windows, so isatty is reliable.

[iannucci, 2016-04-02 06:11:23]

(interestingly, this is also why running `python` from inside the git bash shell
doesn't automatically go into interactive mode; python doesn't think it's
talking to a tty :/).

[iannucci, 2016-04-02 07:12:00]

On 2016/04/02 at 06:11:23, iannucci wrote:
> (interestingly, this is also why running `python` from inside the git bash
shell doesn't automatically go into interactive mode; python doesn't think it's
talking to a tty :/).

Fix for python here: https://chromiumcodereview.appspot.com/1851113003

[brucedawson, 2016-04-02 20:42:50]

Just some quick weekend comments.

https://chromiumcodereview.appspot.com/1851283002/diff/20001/git_common.py
File git_common.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/20001/git_common.py#ne...
git_common.py:285: def blame(filename, revision=None, porcelain=False, *_args):
What's this change for?

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py
File setup_color.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:18: OUT_TYPE = 'console'
On 2016/04/02 06:08:20, iannucci wrote:
> yay, we detected it via the normal mechanism! This can happen when python is
> attached directly to a tty (shell, cmd).

Can you make this explanation and others comments in the code? Otherwise this
will seem like black magic to the next person who looks at this code.

[iannucci, 2016-04-02 23:45:35]

PTAL

https://chromiumcodereview.appspot.com/1851283002/diff/20001/git_common.py
File git_common.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/20001/git_common.py#ne...
git_common.py:285: def blame(filename, revision=None, porcelain=False, *_args):
On 2016/04/02 at 20:42:50, brucedawson wrote:
> What's this change for?

pylint was annoying me (this argument is unused.)

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py
File setup_color.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/20001/setup_color.py#n...
setup_color.py:18: OUT_TYPE = 'console'
On 2016/04/02 at 20:42:50, brucedawson wrote:
> On 2016/04/02 06:08:20, iannucci wrote:
> > yay, we detected it via the normal mechanism! This can happen when python is
> > attached directly to a tty (shell, cmd).
> 
> Can you make this explanation and others comments in the code? Otherwise this
will seem like black magic to the next person who looks at this code.

Yeah should have done that in the first place :). Done.

[iannucci, 2016-04-02 23:45:35]

PTAL

[dnj, 2016-04-03 00:57:45]

Thanks for the comments. lgtm w/ a few comments of my own.

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py
File setup_color.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py#n...
setup_color.py:14: should_wrap = True
I feel like this should start False and turn True for more readable intent in
the logic.

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py#n...
setup_color.py:54: if status >= 0 and status <= 0x3FFFFFFF:
Windows success and information types are contiguous, so you might as well just
make this:

return status >= 0 and status <= 0x7FFFFFFF

?

[brucedawson, 2016-04-04 17:16:19]

I did some basic tests (git cl status to console, file, and pipe) and everything
seems to be behaving correctly. lgtm.

[iannucci, 2016-04-04 20:24:59]

OK, PTAL. I addressed comments and am going to re-test now. Bruce if you
wouldn't mind re-testing too that would give me some peace of mind :)

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py
File setup_color.py (right):

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py#n...
setup_color.py:14: should_wrap = True
On 2016/04/03 at 00:57:45, dnj wrote:
> I feel like this should start False and turn True for more readable intent in
the logic.

done

https://chromiumcodereview.appspot.com/1851283002/diff/40001/setup_color.py#n...
setup_color.py:54: if status >= 0 and status <= 0x3FFFFFFF:
On 2016/04/03 at 00:57:45, dnj wrote:
> Windows success and information types are contiguous, so you might as well
just make this:
> 
> return status >= 0 and status <= 0x7FFFFFFF
> 
> ?

yeah changed and added comment.

[iannucci, 2016-04-04 20:29:37]

OK, retested on win10 + os x and it works.

[brucedawson, 2016-04-04 21:30:47]

On 2016/04/04 20:29:37, iannucci wrote:
> OK, retested on win10 + os x and it works.

Retested. Still lgtm.

[iannucci, 2016-04-04 21:32:01]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-04-04 21:32:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1851283002/#ps60001
(title: "Improve logic a bit")

[commit-bot: I haz the power, 2016-04-04 21:32:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1851283002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1851283002/60001

[commit-bot: I haz the power, 2016-04-04 21:34:40]

Description was changed from

==========
Fix coloring madness in depot_tools.

'setup_color' now contains logic to correctly detect:
  * cmd
  * cmd pipe
  * msys bash
  * msys pipe
  * cmd running inside msys bash (git-command)
  * cmd pipe running inside msys bash (git-command > outfile)

R=brucedawson@chromium.org, dnj@chromium.org
BUG=600049
==========

to

==========
Fix coloring madness in depot_tools.

'setup_color' now contains logic to correctly detect:
  * cmd
  * cmd pipe
  * msys bash
  * msys pipe
  * cmd running inside msys bash (git-command)
  * cmd pipe running inside msys bash (git-command > outfile)

R=brucedawson@chromium.org, dnj@chromium.org
BUG=600049

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299682
==========

[commit-bot: I haz the power, 2016-04-04 21:34:41]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299682