Use plain sh instead of bash in the git template hooks.

The scripts are simple enough and should work fine under any POSIX shell,
which is particularly helpful when one is using `gclient sync' on a platform
that does not have /bin/bash (ie. it's installed in another location or not
present at all), since the hooks are automatically added to the git
checkouts.

While here, just `exec' the scripts directly instead of invoking another
shell by hand and remove the redundant `exit 0' lines.

R=cmp@chromium.org,dpranke@chromium.org,szager@chromium.org

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

[Raphael Kubo da Costa (rakuco), 2013-07-28 22:15:41]

Ping?

[iannucci, 2013-07-28 22:18:21]

On 2013/07/28 22:15:41, Raphael Kubo da Costa (rakuco) wrote:
> Ping?

szager, you should look at this since you do the most with the git
hooks/templates.

[iannucci, 2013-07-28 22:18:56]

On 2013/07/28 22:18:21, iannucci wrote:
> On 2013/07/28 22:15:41, Raphael Kubo da Costa (rakuco) wrote:
> > Ping?
> 
> szager, you should look at this since you do the most with the git
> hooks/templates.

(that said, this looks fine to me)

[szager1, 2013-07-29 06:41:44]

lgtm

I'm curious, though, what platform you have in mind here.  As far as I know,
chromium doesn't compile on any platforms that don't have bash (assuming you
have cygwin or mingw).

[Isaac (away), 2013-07-29 08:15:10]

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
File git-templates/hooks/applypatch-msg (right):

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
git-templates/hooks/applypatch-msg:1: #!/bin/sh
These lines look fine.

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
git-templates/hooks/applypatch-msg:3: [ -e "build/git-hooks/applypatch-msg" ] &&
exec sh "build/git-hooks/applypatch-msg" "$@"
Can we exec the script directly -- i.e. remove the bash/sh from these lines?

[Isaac (away), 2013-07-29 08:18:01]

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
File git-templates/hooks/applypatch-msg (right):

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
git-templates/hooks/applypatch-msg:3: [ -e "build/git-hooks/applypatch-msg" ] &&
exec sh "build/git-hooks/applypatch-msg" "$@"
On 2013/07/29 08:15:10, Isaac wrote:
> Can we exec the script directly -- i.e. remove the bash/sh from these lines?

Actually -- I'm pretty confident it is OK to remove the bash/sh from these
lines.  Could you upload a fresh patch w/ that?

[Raphael Kubo da Costa (rakuco), 2013-07-29 08:18:51]

On 2013/07/29 06:41:44, szager1 wrote:
> lgtm
> 
> I'm curious, though, what platform you have in mind here.  As far as I know,
> chromium doesn't compile on any platforms that don't have bash (assuming you
> have cygwin or mingw).

I mostly have the BSDs in mind, though other platforms may have the same issue:
it's not really that bash is not available, but it is not in /bin. Since it's a
third-party tool, it's installed by default into /usr/local/bin/bash.

[Raphael Kubo da Costa (rakuco), 2013-07-29 08:23:09]

On 2013/07/29 08:18:51, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/07/29 06:41:44, szager1 wrote:
> > lgtm
> > 
> > I'm curious, though, what platform you have in mind here.  As far as I know,
> > chromium doesn't compile on any platforms that don't have bash (assuming you
> > have cygwin or mingw).
> 
> I mostly have the BSDs in mind, though other platforms may have the same
issue:
> it's not really that bash is not available, but it is not in /bin. Since it's
a
> third-party tool, it's installed by default into /usr/local/bin/bash.

It may also be a bit far-fetched, but not using bashisms and being
POSIX-compliant means one can also use plain sh on, say, Debian-based systems
and avoid running a full-blown shell for running just a few commands.

[Isaac (away), 2013-07-29 08:24:53]

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
File git-templates/hooks/applypatch-msg (right):

https://codereview.chromium.org/19278004/diff/1/git-templates/hooks/applypatc...
git-templates/hooks/applypatch-msg:4: exit 0
also exit 0 is redundant, can you remove these lines too?

[Isaac (away), 2013-07-29 08:25:24]

Sorry; didn't see you alreayd uploaded patch.  LGTM

[Isaac (away), 2013-07-29 08:27:25]

Thanks!  (Still lgtm)

[commit-bot: I haz the power, 2013-07-29 08:34:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/192780...

[commit-bot: I haz the power, 2013-07-29 08:35:57]

Presubmit check for 19278004-16001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
tests/gclient_smoketest.py failed
......FFFFF.....FEFFFFF..................
======================================================================
ERROR: testRevertAndStatus (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 996, in testRevertAndStatus
    write(join(self.root_dir, 'src', 'repo2', 'hi'), 'Hey!')
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
28, in write
    f = open(path, 'wb')
IOError: [Errno 2] No such file or directory:
'/tmp/trialAsaoFD/__main__.GClientSmokeGIT.testRevertAndStatus/src/repo2/hi'

======================================================================
FAIL: testMultiSolutions (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1108, in testMultiSolutions
    'running', 'running', 'running'])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet master returned
non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeBoth.testMultiSolutions/src-git\n'

======================================================================
FAIL: testMultiSolutionsJobs (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1136, in testMultiSolutionsJobs
    self.checkString('', stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet master returned
non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeBoth.testMultiSolutionsJobs/src-git\n'

======================================================================
FAIL: testMultiSolutionsMultiRev (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1173, in testMultiSolutionsMultiRev
    'You must specify the full solution name like --revision src@1\n'
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet
fc829265ce543fe0ea5f76ce964a7d116a1cff3e returned non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeBoth.testMultiSolutionsMultiRev/src-git\n'

======================================================================
FAIL: testRecurse (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1255, in testRecurse
    self.assertEquals(sorted(entries), sorted(expected))
AssertionError: Lists differ: [("Can't find .gclient_entries... != [('git',
'git://127.0.0.1:2000...

First differing element 0:
("Can't find .gclient_entries",)
('git', 'git://127.0.0.1:20001/git/repo_1',
'/tmp/trialAsaoFD/__main__.GClientSmokeBoth.testRecurse/src-git')

Second list contains 5 additional elements.
First extra element 1:
('git', 'git://127.0.0.1:20001/git/repo_2@5a81e9d',
'/tmp/trialAsaoFD/__main__.GClientSmokeBoth.testRecurse/src/repo2')

Diff is 830 characters long. Set self.maxDiff to None to see it.

======================================================================
FAIL: testRevInfo (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1209, in testRevInfo
    self.check((out, '', 0), results)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
743, in check
    self.checkString(expected[0], results[0])
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: 'src: svn://127.0.0.1:10002/svn/trunk/src/\nsrc-git:
git://127.0.0.1:20001/git/repo_1\nsrc/file/other:
File("svn://127.0.0.1:10002/svn/trunk/other/DEPS")\nsrc/other:
svn://127.0.0.1:10002/svn/trunk/other\nsrc/repo2:
git://127.0.0.1:20001/git/repo_2@5a81e9d\nsrc/repo2/repo_renamed:
git://127.0.0.1:20001/git/repo_3\nsrc/third_party/foo:
svn://127.0.0.1:10002/svn/trunk/third_party/foo@1\n' != ''

======================================================================
FAIL: testRevInfo (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1070, in testRevInfo
    self.check((out, '', 0), results)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
743, in check
    self.checkString(expected[0], results[0])
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: 'src: git://127.0.0.1:20001/git/repo_1\nsrc/repo2:
git://127.0.0.1:20001/git/repo_2@5a81e9d\nsrc/repo2/repo_renamed:
git://127.0.0.1:20001/git/repo_3\n' != ''

======================================================================
FAIL: testRunHooks (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1041, in testRunHooks
    self.assertTree(tree)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
757, in assertTree
    self.assertEquals(diff, [])
AssertionError: {'src/repo2/DEPS': "\ndeps = {\n  'foo/bar': '/repo_3',\n}\n",
'src/git_hooked2': 'git_hooked2', 'src/repo2/origin': 'git/repo_2@1\n',
'src/git_hooked1': 'git_hooked1', 'src/repo2/repo_renamed/origin':
'git/repo_3@2\n'} != []

======================================================================
FAIL: testSync (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 847, in testSync
    'running', 'running'])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet master returned
non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeGIT.testSync/src\n'

======================================================================
FAIL: testSyncIgnoredSolutionName (__main__.GClientSmokeGIT)
TODO(maruel): This will become an error soon.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 904, in testSyncIgnoredSolutionName
    'Please fix your script, having invalid --revision flags '
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet master returned
non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeGIT.testSyncIgnoredSolutionName/src\n'

======================================================================
FAIL: testSyncJobs (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 943, in testSyncJobs
    untangle=True)
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet master returned
non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmokeGIT.testSyncJobs/src\n'

======================================================================
FAIL: testSyncNoSolutionName (__main__.GClientSmokeGIT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 924, in testSyncNoSolutionName
    'running', ('running', self.root_dir + '/src/repo4')])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
739, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: Command git checkout --quiet
fc829265ce543fe0ea5f76ce964a7d116a1cff3e returned non-zero exit status 1 in
/tmp/trialAsaoFD/__main__.GClientSmoke…
(message too large)

[Raphael Kubo da Costa (rakuco), 2013-07-29 11:59:37]

Committed manually as
https://src.chromium.org/viewvc/chrome?view=revision&revision=214177
unfortunately it got in without the review url in the message, I apologize :(

I tried using git-cl dcommit in all possible ways but in the end I always got
something like this, so I git-svn dcommitted and the used gclient set_close:

Traceback (most recent call last):
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 2278, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 2260, in main
    return command(parser, argv[1:])
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 1785, in CMDdcommit
    return SendUpstream(parser, args, 'dcommit')
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 1623, in SendUpstream
    change=cl.GetChange(base_branch, author))
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 737, in GetChange
    description = self.GetDescription()
  File "/home/rakuco/dev/depot_tools/git_cl.py", line 635, in GetDescription
    self.description = self.RpcServer().get_description(issue).strip()
  File "/home/rakuco/dev/depot_tools/rietveld.py", line 428, in get_description
    super(CachingRietveld, self).get_description)
  File "/home/rakuco/dev/depot_tools/rietveld.py", line 421, in _lookup
    function_cache[args] = update(*args)
  File "/home/rakuco/dev/depot_tools/rietveld.py", line 87, in get_description
    return '\n'.join(self.get('/%d/description' % issue).strip().splitlines())
  File "/home/rakuco/dev/depot_tools/rietveld.py", line 355, in get
    return self._send(request_path, **kwargs)
  File "/home/rakuco/dev/depot_tools/rietveld.py", line 381, in _send
    result = self.rpc_server.Send(request_path, **kwargs)
  File "/home/rakuco/dev/depot_tools/third_party/upload.py", line 446, in Send
    f = self.opener.open(req)
  File "/usr/lib/python2.7/urllib2.py", line 404, in open
    response = self._open(req, data)
  File "/usr/lib/python2.7/urllib2.py", line 422, in _open
    '_open', req)
  File "/usr/lib/python2.7/urllib2.py", line 382, in _call_chain
    result = func(*args)
  File "/usr/lib/python2.7/urllib2.py", line 1222, in https_open
    return self.do_open(httplib.HTTPSConnection, req)
  File "/usr/lib/python2.7/urllib2.py", line 1184, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno -2] Name or service not known>

[commit-bot: I haz the power, 2013-07-29 12:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/192780...

[Isaac (away), 2013-07-29 12:12:04]

Hi Raphael -- I had to revert your commit, I realized the exit 0 was not
redudent because the line before can return non-zero code. 

I checked the CQ box for the patchset the keeps the exit 0 calls.  Should land
automatically.

[commit-bot: I haz the power, 2013-07-29 12:13:39]

Change committed as 214181

[Raphael Kubo da Costa (rakuco), 2013-07-29 12:49:41]

On 2013/07/29 12:12:04, Isaac wrote:
> Hi Raphael -- I had to revert your commit, I realized the exit 0 was not
> redudent because the line before can return non-zero code. 
> 
> I checked the CQ box for the patchset the keeps the exit 0 calls.  Should land
> automatically.

Thank you for taking care of this!

As for the `exit 0' thing, shouldn't the hooks return a non-zero status if the
scripts they called actually failed?

[szager1, 2013-07-29 16:04:31]

On 2013/07/29 12:49:41, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/07/29 12:12:04, Isaac wrote:
> > Hi Raphael -- I had to revert your commit, I realized the exit 0 was not
> > redudent because the line before can return non-zero code. 
> > 
> > I checked the CQ box for the patchset the keeps the exit 0 calls.  Should
land
> > automatically.
> 
> Thank you for taking care of this!
> 
> As for the `exit 0' thing, shouldn't the hooks return a non-zero status if the
> scripts they called actually failed?

If the target script (i.e., build/git-hooks/*) exists, then the exec-ed process
will determine the exit status.

If it doesn't exist, this script should exit 0 explicitly, as Isaac says.

Isaac, this is why aggressive drive-by's are generally discouraged.

[Isaac (away), 2013-07-29 16:47:44]

Except on windows exec doesn't work that way.