Re: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config

2014-11-17 Thread Michael Haggerty
On 11/16/2014 09:06 AM, Johannes Sixt wrote:
 Am 16.11.2014 um 08:21 schrieb Michael Haggerty:
 @@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char 
 *prefix)
  if (given_config_source.blob)
  die(editing blobs is not supported);
  git_config(git_default_config, NULL);
 -launch_editor(given_config_source.file ?
 -  given_config_source.file : git_path(config),
 -  NULL, NULL);
 +config_file = xstrdup(given_config_source.file ?
 +  given_config_source.file : 
 git_path(config));
 +launch_editor(config_file, NULL, NULL);
 +
 +/*
 + * In git 2.1, there was a bug in git init that left
 + * the u+x bit set on the config file. To clean up any
 + * repositories affected by that bug, and just because
 + * it doesn't make sense for a config file to be
 + * executable anyway, clear any executable bits from
 + * the file (on a best effort basis):
 + */
 +if (!lstat(config_file, st)  (st.st_mode  0111))
 
 At this point we cannot be sure that config_file is a regular file, can
 we? It could also be a symbolic link. Wouldn't plain stat() be more
 correct then?

You make a good point. But I'm a little nervous about following symlinks
and changing permissions on some distant file. Also, the bug that we are
trying clean up after would not have created a symlink in this place, so
I think the cleanup is not so important if config is a symlink.

So I suggest that we stick with lstat(), but add S_ISREG(st.st_mode) to
the  chain above. Does that sound reasonable?

 +chmod(config_file, st.st_mode  07666);
 +free(config_file);
  }
  else if (actions == ACTION_SET) {
  int ret;

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config

2014-11-16 Thread Johannes Sixt
Am 16.11.2014 um 08:21 schrieb Michael Haggerty:
 @@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char 
 *prefix)
   if (given_config_source.blob)
   die(editing blobs is not supported);
   git_config(git_default_config, NULL);
 - launch_editor(given_config_source.file ?
 -   given_config_source.file : git_path(config),
 -   NULL, NULL);
 + config_file = xstrdup(given_config_source.file ?
 +   given_config_source.file : 
 git_path(config));
 + launch_editor(config_file, NULL, NULL);
 +
 + /*
 +  * In git 2.1, there was a bug in git init that left
 +  * the u+x bit set on the config file. To clean up any
 +  * repositories affected by that bug, and just because
 +  * it doesn't make sense for a config file to be
 +  * executable anyway, clear any executable bits from
 +  * the file (on a best effort basis):
 +  */
 + if (!lstat(config_file, st)  (st.st_mode  0111))

At this point we cannot be sure that config_file is a regular file, can
we? It could also be a symbolic link. Wouldn't plain stat() be more
correct then?

 + chmod(config_file, st.st_mode  07666);
 + free(config_file);
   }
   else if (actions == ACTION_SET) {
   int ret;

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config

2014-11-15 Thread Michael Haggerty
There is no reason for $GIT_DIR/config to be executable, plus there
used to be a bug (fixed by the previous commit) that caused git init
to set the u+x bit on that file. So whenever rewriting the config
file, take the opportunity to remove any executable bits that the file
might have.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/config.c   | 21 ++---
 config.c   | 12 ++--
 t/t1300-repo-config.sh | 13 +
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 7bba516..1a7c17e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -551,6 +551,9 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
}
}
else if (actions == ACTION_EDIT) {
+   char *config_file;
+   struct stat st;
+
check_argc(argc, 0, 0);
if (!given_config_source.file  nongit)
die(not in a git directory);
@@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
-   launch_editor(given_config_source.file ?
- given_config_source.file : git_path(config),
- NULL, NULL);
+   config_file = xstrdup(given_config_source.file ?
+ given_config_source.file : 
git_path(config));
+   launch_editor(config_file, NULL, NULL);
+
+   /*
+* In git 2.1, there was a bug in git init that left
+* the u+x bit set on the config file. To clean up any
+* repositories affected by that bug, and just because
+* it doesn't make sense for a config file to be
+* executable anyway, clear any executable bits from
+* the file (on a best effort basis):
+*/
+   if (!lstat(config_file, st)  (st.st_mode  0111))
+   chmod(config_file, st.st_mode  07666);
+   free(config_file);
}
else if (actions == ACTION_SET) {
int ret;
diff --git a/config.c b/config.c
index 9e42d38..47eaef4 100644
--- a/config.c
+++ b/config.c
@@ -1653,7 +1653,15 @@ int git_config_set_multivar_in_file(const char 
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
 
-   if (chmod(lock-filename, st.st_mode  0)  0) {
+   /*
+* We mask off the executable bits because (a) it
+* doesn't make sense to have executable bits set on
+* the config file, and (b) there was a bug in git 2.1
+* which caused the config file to be created with u+x
+* set, so this will help repair repositories created
+* with that version.
+*/
+   if (chmod(lock-filename, st.st_mode  07666)  0) {
error(chmod on %s failed: %s,
lock-filename, strerror(errno));
ret = CONFIG_NO_WRITE;
@@ -1832,7 +1840,7 @@ int git_config_rename_section_in_file(const char 
*config_filename,
 
fstat(fileno(config_file), st);
 
-   if (chmod(lock-filename, st.st_mode  0)  0) {
+   if (chmod(lock-filename, st.st_mode  07666)  0) {
ret = error(chmod on %s failed: %s,
lock-filename, strerror(errno));
goto out;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 46f6ae2..7637701 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -7,6 +7,12 @@ test_description='Test git config in different settings'
 
 . ./test-lib.sh
 
+test_expect_success POSIXPERM 'any executable bits cleared' '
+   chmod u+x .git/config 
+   git config test.me foo 
+   test ! -x .git/config
+'
+
 test_expect_success 'clear default config' '
rm -f .git/config
 '
@@ -1078,6 +1084,13 @@ test_expect_success 'git config --edit respects 
core.editor' '
test_cmp expect actual
 '
 
+test_expect_success POSIXPERM 'git config --edit clears executable bits' '
+   git config -f tmp test.value no 
+   chmod u+x tmp 
+   GIT_EDITOR=echo [test]value=yes  git config -f tmp --edit 
+   test ! -x tmp
+'
+
 # malformed configuration files
 test_expect_success 'barf on syntax error' '
cat .git/config -\EOF 
-- 
2.1.1

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html