Don't make hook script read-only

When a hook script is not executable, Git will ignore its presence
in the hooks dir. To work around that, git-review checks the
downloaded commit-message hook to see whether it's executable and
then adjusts its permissions accordingly. This behavior has been
included since the initial release, but its naive implementation
wiped all existing permissions and then added only read and execute
for the file's owner (0o500/r-x------), leaving it set read-only.
This is overly-restrictive and can lead to minor annoyances when
deleting directories or for atypical multi-user and group ownership
scenarios due to ignoring the umask set for the process.

It is expected that, at this time, the described behavior is not
widely observed outside workflows which rely on fetching the hook
script over HTTP, as the SCP protocol preserves filesystem
permission flags from the source system, but it will have a much
broader impact in the future if git-review's default workflow shifts
away from SCP.

Replace the naive chmod implementation with one which adds execute
for anyone who already has read permission, but does not remove any
existing permissions, for example:

    0o644/rw-r--r-- .. 0o755/rwx-r-xr-x
    0o640/rw-r----- .. 0o750/rwx-r-x---

This new behavior should be more intuitive and less surprising for
users.

Change-Id: I48ac230df09bc802610cfef65bd9818c5b01673d

git_review/cmd.py

@@ -24,6 +24,7 @@ import json
 import os
 import re
 import shlex
+import stat
 import subprocess
 import sys
 import textwrap
@@ -429,8 +430,17 @@ def set_hooks_commit_msg(remote, target_file):
             "git", "submodule", "foreach",
             'cp -p %s "$(git rev-parse --git-dir)/hooks/"' % target_file)
 
-    if not os.access(target_file, os.X_OK):
-        os.chmod(target_file, os.path.stat.S_IREAD | os.path.stat.S_IEXEC)
+    old_mask = stat.S_IMODE(os.stat(target_file).st_mode)
+    # make sure the owner always has read+write+exec perms
+    mask = old_mask | stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
+    # add executable permission for everyone else who has read access
+    if mask | stat.S_IRGRP == mask:
+        mask |= stat.S_IXGRP
+    if mask | stat.S_IROTH == mask:
+        mask |= stat.S_IXOTH
+    # only call chmod if we need to change the mask
+    if mask != old_mask:
+        os.chmod(target_file, mask)
 
 
 def test_remote_url(remote_url):

git_review/tests/test_git_review.py

@@ -105,6 +105,24 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
         self._run_git_review(
             '-s', chdir=os.path.join(self.test_dir, 'subdirectory'))
 
+    def test_install_remote_hook(self):
+        """Test whether git-review -s correctly creates the commit-msg hook
+        from the Gerrit server with appropriate permissions and content.
+        """
+        hooks_subdir = ".git/hooks"
+        hook_file = os.path.join(self.test_dir, hooks_subdir, 'commit-msg')
+        self.reset_remote()
+
+        self._run_git_review('-s')
+        self.assertTrue(os.path.exists(hook_file))
+        self.assertTrue(os.access(hook_file, os.W_OK))
+        self.assertTrue(os.access(hook_file, os.X_OK))
+        with open(hook_file) as f:
+            content = f.read()
+            self.assertTrue(content.startswith('#!/'))
+            # This line is injected by the Gerrit server in its version
+            self.assertIn('# From Gerrit Code Review', content)
+
     def test_git_review_s_without_core_hooks_path_option(self):
         """Test whether git-review -s correctly creates the commit-msg hook,
         with the Git core.hooksPath option unset.