Fix 236148: Avoid triggering a pager in internal git commands.

Fix 236148: Avoid triggering a pager in internal git commands.

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

[iannucci, 2013-04-28 03:30:41]

It may be worth just forcing the PAGER environment variable to be set but empty.
This seems to have the same effect as --no-pager (and would require a smaller
code change).

[iannucci, 2013-04-28 03:33:21]

On 2013/04/28 03:30:41, iannucci wrote:
> It may be worth just forcing the PAGER environment variable to be set but
empty.
> This seems to have the same effect as --no-pager (and would require a smaller
> code change).

** This seems to have the same effect on mac/linux. I didn't try on windows,
which may behave differently.

[Daniel Bratell, 2013-04-29 06:48:33]

On 2013/04/28 03:33:21, iannucci wrote:
> On 2013/04/28 03:30:41, iannucci wrote:
> > It may be worth just forcing the PAGER environment variable to be set but
> empty.
> > This seems to have the same effect as --no-pager (and would require a
smaller
> > code change).
> 
> ** This seems to have the same effect on mac/linux. I didn't try on windows,
> which may behave differently.

It would have been a small change if there hadn't been a million tests and a 80
char line limit. :-)

I tried to test with an empty PAGER in Windows cmd.exe now, but it's tricky
since an empty variable seems to be the same as no variable. Setting it to an
illegal value triggered an error message along with the output so that was not a
good choice.

[M-A Ruel, 2013-05-01 14:15:32]

On 2013/04/29 06:48:33, bratell wrote:
> On 2013/04/28 03:33:21, iannucci wrote:
> > On 2013/04/28 03:30:41, iannucci wrote:
> > > It may be worth just forcing the PAGER environment variable to be set but
> > empty.
> > > This seems to have the same effect as --no-pager (and would require a
> smaller
> > > code change).
> > 
> > ** This seems to have the same effect on mac/linux. I didn't try on windows,
> > which may behave differently.
> 
> It would have been a small change if there hadn't been a million tests and a
80
> char line limit. :-)
> 
> I tried to test with an empty PAGER in Windows cmd.exe now, but it's tricky
> since an empty variable seems to be the same as no variable. Setting it to an
> illegal value triggered an error message along with the output so that was not
a
> good choice.

Also it could be a problem with git config --global core.pager. Note that git
also honors GIT_PAGER.

Anyhow, lgtm.

[commit-bot: I haz the power, 2013-05-02 09:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/14104005/2001

[commit-bot: I haz the power, 2013-05-02 09:11:44]

Change committed as 197872

[iannucci, 2013-06-25 23:25:45]

On 2013/05/02 09:11:44, I haz the power (commit-bot) wrote:
> Change committed as 197872

Hm... I think this change broke all of tests/*.sh :/

I'm not sure why CQ didn't catch it though. Here's an example:
https://chromiumcodereview.appspot.com/17644004/

[Daniel Bratell, 2013-06-26 07:47:18]

On 2013/06/25 23:25:45, iannucci wrote:
> On 2013/05/02 09:11:44, I haz the power (commit-bot) wrote:
> > Change committed as 197872
> 
> Hm... I think this change broke all of tests/*.sh :/
> 
> I'm not sure why CQ didn't catch it though. Here's an example:
> https://chromiumcodereview.appspot.com/17644004/

I am not that good at decoding the output there. I only see "FAILED". Do you
know what goes wrong more in detail and I can try fixing it?

[iannucci, 2013-06-26 22:15:23]

On 2013/06/26 07:47:18, bratell wrote:
> On 2013/06/25 23:25:45, iannucci wrote:
> > On 2013/05/02 09:11:44, I haz the power (commit-bot) wrote:
> > > Change committed as 197872
> > 
> > Hm... I think this change broke all of tests/*.sh :/
> > 
> > I'm not sure why CQ didn't catch it though. Here's an example:
> > https://chromiumcodereview.appspot.com/17644004/
> 
> I am not that good at decoding the output there. I only see "FAILED". Do you
> know what goes wrong more in detail and I can try fixing it?

I'm not sure, but when I run it locally, I'll get stuff like:
patch.sh failed
Command /usr/local/google/home/iannucci/s/depot_tools/tests/patch.sh returned
non-zero exit status 1 in /usr/local/google/home/iannucci/s/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: upload succeeds (needs a server running on localhost)
Command "git --no-pager config user.email" failed.

FAILURE: upload succeeds (needs a server running on localhost)

Could we revert this, and just do e.g. `os.environ['PAGER'] = 'cat';
os.environ['GIT_PAGER'] = 'cat'` ?

[Daniel Bratell, 2013-06-27 07:23:26]

On 2013/06/26 22:15:23, iannucci wrote:
> On 2013/06/26 07:47:18, bratell wrote:
> > On 2013/06/25 23:25:45, iannucci wrote:
> > > On 2013/05/02 09:11:44, I haz the power (commit-bot) wrote:
> > > > Change committed as 197872
> > > 
> > > Hm... I think this change broke all of tests/*.sh :/
> > > 
> > > I'm not sure why CQ didn't catch it though. Here's an example:
> > > https://chromiumcodereview.appspot.com/17644004/
> > 
> > I am not that good at decoding the output there. I only see "FAILED". Do you
> > know what goes wrong more in detail and I can try fixing it?
> 
> I'm not sure, but when I run it locally, I'll get stuff like:
> patch.sh failed
> Command /usr/local/google/home/iannucci/s/depot_tools/tests/patch.sh returned
> non-zero exit status 1 in /usr/local/google/home/iannucci/s/depot_tools/tests
> Setting up test SVN repo...
> Setting up test git-svn repo...
> TESTING: upload succeeds (needs a server running on localhost)
> Command "git --no-pager config user.email" failed.
> 
> FAILURE: upload succeeds (needs a server running on localhost)
> 
> Could we revert this, and just do e.g. `os.environ['PAGER'] = 'cat';
> os.environ['GIT_PAGER'] = 'cat'` ?

I would not mind a patch that uses GIT_PAGER=cat (as told by
https://www.kernel.org/pub/software/scm/git/docs/ ). When I wrote this patch I
wasn't convinced that was portable but it should be according to documentation
and that would be much cleaner and less visible (and thus less scary).

On the other hand, I can't see why you think this patch is the cause of that
failure. Seems much more likely there is something unseen that goes wrong in
that test case.

[iannucci, 2013-06-27 17:22:39]

On 2013/06/27 07:23:26, bratell wrote:
> On 2013/06/26 22:15:23, iannucci wrote:
> > On 2013/06/26 07:47:18, bratell wrote:
> > > On 2013/06/25 23:25:45, iannucci wrote:
> > > > On 2013/05/02 09:11:44, I haz the power (commit-bot) wrote:
> > > > > Change committed as 197872
> > > > 
> > > > Hm... I think this change broke all of tests/*.sh :/
> > > > 
> > > > I'm not sure why CQ didn't catch it though. Here's an example:
> > > > https://chromiumcodereview.appspot.com/17644004/
> > > 
> > > I am not that good at decoding the output there. I only see "FAILED". Do
you
> > > know what goes wrong more in detail and I can try fixing it?
> > 
> > I'm not sure, but when I run it locally, I'll get stuff like:
> > patch.sh failed
> > Command /usr/local/google/home/iannucci/s/depot_tools/tests/patch.sh
returned
> > non-zero exit status 1 in
/usr/local/google/home/iannucci/s/depot_tools/tests
> > Setting up test SVN repo...
> > Setting up test git-svn repo...
> > TESTING: upload succeeds (needs a server running on localhost)
> > Command "git --no-pager config user.email" failed.
> > 
> > FAILURE: upload succeeds (needs a server running on localhost)
> > 
> > Could we revert this, and just do e.g. `os.environ['PAGER'] = 'cat';
> > os.environ['GIT_PAGER'] = 'cat'` ?
> 
> I would not mind a patch that uses GIT_PAGER=cat (as told by
> https://www.kernel.org/pub/software/scm/git/docs/ ). When I wrote this patch I
> wasn't convinced that was portable but it should be according to documentation
> and that would be much cleaner and less visible (and thus less scary).
> 
> On the other hand, I can't see why you think this patch is the cause of that
> failure. Seems much more likely there is something unseen that goes wrong in
> that test case.

Huh weird... So previously, I tried running `git --no-pager config user.name`,
and it exit'd 1 with no output. I just tried it again and it exits 0 with the
correct output. 

*looks back through bash history*

Or, I just mistyped the command :(

OH! I know what it is... it's totally my fault. I don't have a global git
user.email set up (I set it per-repo, because some are @google, some are
@chromium, and some are my own private repos). The tests create a new repo, and
they assume that the global email is set correctly (which, in my case, it's
not).

This has nothing to do with your CL at all. Sorry about that! I'll put up
another one to fix the tests though.