[PATCH v2 0/2] Don't make $GIT_DIR executable

[PATCH v2 0/2] Don't make $GIT_DIR executable

[Michael Haggerty]

Changes since v1 [1]:

* Check the chmod() return result in create_default_files(), as
  suggested by Torsten Bögershausen.

* Fix a comment typo found by Stefan Beller.

* Extend patch 2/2 to also clearing the executable bits when "git
  config --edit" is run.

* Add test cases to patch 2/2 that the executable bits really are
  cleaned up when they should be.

Thanks to Stefan Beller, Torsten Bögershausen, and Eric Wong for their
feedback about v1.

I have also pushed this series to my GitHub fork [2].

Please remember that this patch series applies to maint. This version
has a couple of conflicts with master; I have pushed my proposed
conflict resolution to GitHub [3], including a preparatory commit that
I recommend for master.

[1] http://thread.gmane.org/gmane.comp.version-control.git/259620/focus=259620
[2] https://github.com/mhagger/git branch "config-non-executable"
[3] https://github.com/mhagger/git branch "config-non-executable-merge"

Michael Haggerty (2):
  create_default_files(): don't set u+x bit on $GIT_DIR/config
  config: clear the executable bits (if any) on $GIT_DIR/config

 builtin/config.c       | 21 ++++++++++++++++++---
 builtin/init-db.c      |  1 +
 config.c               | 12 ++++++++++--
 t/t1300-repo-config.sh | 13 +++++++++++++
 4 files changed, 42 insertions(+), 5 deletions(-)

-- 
2.1.1

[PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty]

Since time immemorial, the test of whether to set "core.filemode" has
been done by trying to toggle the u+x bit on $GIT_DIR/config and then
testing whether the change "took". It is somewhat odd to use the
config file for this test, but whatever.

The test code didn't set the u+x bit back to its original state
itself, instead relying on the subsequent call to git_config_set() to
re-write the config file with correct permissions.

But ever since

    daa22c6f8d config: preserve config file permissions on edits (2014-05-06)

git_config_set() copies the permissions from the old config file to
the new one. This is a good change in and of itself, but it interacts
badly with create_default_files()'s sloppiness, causing "git init" to
leave the executable bit set on $GIT_DIR/config.

So change create_default_files() to reset the permissions on
$GIT_DIR/config after its test.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/init-db.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 56f85e2..4c8021d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
 		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
 				!lstat(path, &st2) &&
 				st1.st_mode != st2.st_mode);
+		filemode &= !chmod(path, st1.st_mode);
 	}
 	git_config_set("core.filemode", filemode ? "true" : "false");
 
-- 
2.1.1

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Since time immemorial, the test of whether to set "core.filemode" has
> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
> testing whether the change "took". It is somewhat odd to use the
> config file for this test, but whatever.

The last sentence should read "We could create a test file and use
it for this purpose and then remove it, but config is a file we know
exists at this point in the code (and it is the only file we know
that exists), so it was a very sensible trick".

Or remove it altogether.  In other words, do not sound as if you do
not know what you are doing in your log message.  That would rob
confidence in the change from the person who is reading "git log"
output later.

> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>  		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>  				!lstat(path, &st2) &&
>  				st1.st_mode != st2.st_mode);
> +		filemode &= !chmod(path, st1.st_mode);

Sounds good.

You could also &&-chain this "flip it back" to the above statement.
If filemode is not trustable on a filesytem, doing one extra chmod()
to correct would not help us anyway, no?


>  	}
>  	git_config_set("core.filemode", filemode ? "true" : "false");

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty]

On 11/16/2014 08:08 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Since time immemorial, the test of whether to set "core.filemode" has
>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
>> testing whether the change "took". It is somewhat odd to use the
>> config file for this test, but whatever.
> 
> The last sentence should read "We could create a test file and use
> it for this purpose and then remove it, but config is a file we know
> exists at this point in the code (and it is the only file we know
> that exists), so it was a very sensible trick".
> 
> Or remove it altogether.  In other words, do not sound as if you do
> not know what you are doing in your log message.  That would rob
> confidence in the change from the person who is reading "git log"
> output later.

The sentence is not meant to rob confidence in this change, but rather
to stimulate the reader's critical thinking about nearby code that I am
*not* changing.

By making this change without changing the function to use a temporary
file for its chmod experiments, I might otherwise give future readers
the impression that I like this shortcut, which I do not. For example,
if the original code had used a temporary file rather than "config",
then we would never have had the bug that I'm fixing. The "but whatever"
is meant to indicate that I don't disagree so strongly with the choice
of tradeoffs made by the original author that I think it is worth changing.

So maybe I am a coward (or lazy) for not proposing to change to using a
temporary file instead. But since this patch is suggested for maint, I
wanted to make the smallest change that would fix the bug.

Feel free to delete the controversial sentence if you prefer.

>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>>  		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>  				!lstat(path, &st2) &&
>>  				st1.st_mode != st2.st_mode);
>> +		filemode &= !chmod(path, st1.st_mode);
> 
> Sounds good.
> 
> You could also &&-chain this "flip it back" to the above statement.
> If filemode is not trustable on a filesytem, doing one extra chmod()
> to correct would not help us anyway, no?

Yes, that would be better. I will fix it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Eric Sunshine]

On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Since time immemorial, the test of whether to set "core.filemode" has
> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
> testing whether the change "took". It is somewhat odd to use the
> config file for this test, but whatever.
>
> The test code didn't set the u+x bit back to its original state
> itself, instead relying on the subsequent call to git_config_set() to
> re-write the config file with correct permissions.
>
> But ever since
>
>     daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
>
> git_config_set() copies the permissions from the old config file to
> the new one. This is a good change in and of itself, but it interacts
> badly with create_default_files()'s sloppiness, causing "git init" to
> leave the executable bit set on $GIT_DIR/config.
>
> So change create_default_files() to reset the permissions on
> $GIT_DIR/config after its test.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Should this patch include a test in t1300 to ensure that this bug does
not resurface (and to prove that this patch indeed fixes it)?

>  builtin/init-db.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 56f85e2..4c8021d 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>                 filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>                                 !lstat(path, &st2) &&
>                                 st1.st_mode != st2.st_mode);
> +               filemode &= !chmod(path, st1.st_mode);
>         }
>         git_config_set("core.filemode", filemode ? "true" : "false");
>
> --
> 2.1.1

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Torsten Bögershausen]

On 11/17/2014 02:40 AM, Eric Sunshine wrote:
> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Since time immemorial, the test of whether to set "core.filemode" has
>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
>> testing whether the change "took". It is somewhat odd to use the
>> config file for this test, but whatever.
>>
>> The test code didn't set the u+x bit back to its original state
>> itself, instead relying on the subsequent call to git_config_set() to
>> re-write the config file with correct permissions.
>>
>> But ever since
>>
>>      daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
>>
>> git_config_set() copies the permissions from the old config file to
>> the new one. This is a good change in and of itself, but it interacts
>> badly with create_default_files()'s sloppiness, causing "git init" to
>> leave the executable bit set on $GIT_DIR/config.
>>
>> So change create_default_files() to reset the permissions on
>> $GIT_DIR/config after its test.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> Should this patch include a test in t1300 to ensure that this bug does
> not resurface (and to prove that this patch indeed fixes it)?
>
>>   builtin/init-db.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 56f85e2..4c8021d 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>>                  filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>                                  !lstat(path, &st2) &&
>>                                  st1.st_mode != st2.st_mode);
>> +               filemode &= !chmod(path, st1.st_mode);
>>          }
>>          git_config_set("core.filemode", filemode ? "true" : "false");
>>
>> --
Sorry for the late reply, I actually had prepared a complete different patch
for a different problem, but it touches the very same lines of code.

(And we may want to add a
!chmod(path, st1.st_mode & ~S_IXUSR)
at the end of the operation)

There are systems when a file created with 666 have 766, and we
do not handle this situation yet.

Michael, if there is a chance that you integrate my small code changes 
in your patch ?

-------------

commit 3228bedef6d45bfaf8986b6367f9388738476345
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Sun Oct 19 00:15:00 2014 +0200

     Improve the filemode trustability check

     Some file systems do not fully support the executable bit:
     a) The user executable bit is always 0
     b) The user executable bit is always 1
     c) Is similar to b):
        When a file is created with mode 0666 the file mode on disc is 766
        and the user executable bit is 1 even if it should be 0 like b)

        There are smbfs implementations where the file mode can be 
maintained
        locally and chmod -x changes the file mode from 0766 to 0666.
        When the file system is unmounted and remounted,
        the file mode is 0766 and executable bit is 1 again.

     A typical example for a) is a VFAT drive mounted with -onoexec,
     or cifs with -ofile_mode=0644.
     b) is VFAT mounted with -oexec or cifs is mounted with -ofile_mode=0755

     The behaviour described in c) has been observed when a Windows 
machine with
     NTFS exports a share via smb (or afp) to Mac OS X.
     (CIFS is an enhanced version of SMB
      The command to mount on the command line may differ,
      e.g mount.cifs under Linux or mount_smbfs under Mac OS X)

     Case a) and b) are detected by the current code.
     Case c) qualifies as "non trustable executable bit",
     and core.filemode should be false by default.

     Solution:
     Detect when ".git/config" has the user executable bit set after
     creat(".git/config", 0666) and set core.filemode to false.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 587a505..d3e4fb3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -249,13 +249,11 @@ static int create_default_files(const char 
*template_path)
      strcpy(path + len, "config");

      /* Check filemode trustability */
-    filemode = TEST_FILEMODE;
-    if (TEST_FILEMODE && !lstat(path, &st1)) {
-        struct stat st2;
-        filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
-                !lstat(path, &st2) &&
-                st1.st_mode != st2.st_mode);
-    }
+    filemode = TEST_FILEMODE &&
+        !lstat(path, &st1) &&    !(st1.st_mode & S_IXUSR) &&
+        !chmod(path, st1.st_mode | S_IXUSR) &&
+        !lstat(path, &st1) && (st1.st_mode & S_IXUSR);
+
      git_config_set("core.filemode", filemode ? "true" : "false");

      if (is_bare_repository())

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty]

On 11/17/2014 10:08 AM, Torsten Bögershausen wrote:
> On 11/17/2014 02:40 AM, Eric Sunshine wrote:
>> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty
>> <mhagger@alum.mit.edu> wrote:
>>> [...]
> Sorry for the late reply, I actually had prepared a complete different
> patch
> for a different problem, but it touches the very same lines of code.
> 
> (And we may want to add a
> !chmod(path, st1.st_mode & ~S_IXUSR)
> at the end of the operation)
> 
> There are systems when a file created with 666 have 766, and we
> do not handle this situation yet.
> 
> Michael, if there is a chance that you integrate my small code changes
> in your patch ?

Your change seems like a good idea, but is is logically separate from
mine and I think it would be awkward to yoke them together. Both changes
are short enough that conflict resolution shouldn't be a big problem.

It will be easier for you to follow up on your patch if you submit it
separately. The first question you get might be: "Should it be applied
to maint or is it more suitable for master?", which also hints at one of
the reasons that yoking the patches together might be awkward.

Regarding your patch itself:

> [...]
>     b) The user executable bit is always 1
>     c) Is similar to b):
>        When a file is created with mode 0666 the file mode on disc is 766
>        and the user executable bit is 1 even if it should be 0 like b)
> 
>        There are smbfs implementations where the file mode can be maintained
>        locally and chmod -x changes the file mode from 0766 to 0666.
>        When the file system is unmounted and remounted,
>        the file mode is 0766 and executable bit is 1 again.

It seems surprising that there isn't also a case (d) like (c) except
that a file created with mode 0666 initially has the correct mode, and
the executable bits can be maintained locally while the filesystem is
mounted, but where executable bits are lost whenever the filesystem is
unmounted and remounted. Does that case not arise, or is it simply that
it is impossible to test for it in create_default_files()?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty]

On 11/17/2014 02:40 AM, Eric Sunshine wrote:
> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Since time immemorial, the test of whether to set "core.filemode" has
>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
>> testing whether the change "took". It is somewhat odd to use the
>> config file for this test, but whatever.
>>
>> The test code didn't set the u+x bit back to its original state
>> itself, instead relying on the subsequent call to git_config_set() to
>> re-write the config file with correct permissions.
>>
>> But ever since
>>
>>     daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
>>
>> git_config_set() copies the permissions from the old config file to
>> the new one. This is a good change in and of itself, but it interacts
>> badly with create_default_files()'s sloppiness, causing "git init" to
>> leave the executable bit set on $GIT_DIR/config.
>>
>> So change create_default_files() to reset the permissions on
>> $GIT_DIR/config after its test.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> Should this patch include a test in t1300 to ensure that this bug does
> not resurface (and to prove that this patch indeed fixes it)?

This seems like a one-off bug caused by a specific instance of odd code.
It could only recur if somebody were to remove the line that I added,
which would be a *very* odd mistake to make given that its purpose is
pretty obvious.

I tested manually that this patch fixes the problem. Admittedly, my
manual testing was only on one particular version of Linux. But it seems
to me that the function being used is sufficiently portable to be
trusted, and the fact that the same function is being used a few lines
earlier suggests that any portability problems would have wider
ramifications anyway.

So in summary, I think the chance that such a test would ever catch a
new problem is too small to justify the effort of writing and
maintaining it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This seems like a one-off bug caused by a specific instance of odd code.
> It could only recur if somebody were to remove the line that I added,
> which would be a *very* odd mistake to make given that its purpose is
> pretty obvious.

Or some other code that comes _after_ your new code touches the
file.

You cannot anticipate what mistake others make.

Shouldn't something like this be sufficient?

 t/t0001-init.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index e62c0ff..acdc1df 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -12,6 +12,13 @@ check_config () {
 		echo "expected a directory $1, a file $1/config and $1/refs"
 		return 1
 	fi
+
+	if ! test_have_prereq POSIXPERM || test -x "$1/config"
+	then
+		echo "$1/config is executable?"
+		return 1
+	fi
+
 	bare=$(cd "$1" && git config --bool core.bare)
 	worktree=$(cd "$1" && git config core.worktree) ||
 	worktree=unset

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Michael Haggerty]

On 11/17/2014 04:42 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> This seems like a one-off bug caused by a specific instance of odd code.
>> It could only recur if somebody were to remove the line that I added,
>> which would be a *very* odd mistake to make given that its purpose is
>> pretty obvious.
> 
> Or some other code that comes _after_ your new code touches the
> file.
> 
> You cannot anticipate what mistake others make.

And yet we do so all the time, adding tests for the things we consider
most likely to break in the future and omitting them for breakages that
seem unlikely. Otherwise, what frees us from the obligation to add a '!
test -x "$GIT_DIR/config"' following every single git operation?

But it's OK with me, we can add a test.

> Shouldn't something like this be sufficient?
> 
>  t/t0001-init.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index e62c0ff..acdc1df 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -12,6 +12,13 @@ check_config () {
>  		echo "expected a directory $1, a file $1/config and $1/refs"
>  		return 1
>  	fi
> +
> +	if ! test_have_prereq POSIXPERM || test -x "$1/config"
> +	then
> +		echo "$1/config is executable?"
> +		return 1
> +	fi
> +
>  	bare=$(cd "$1" && git config --bool core.bare)
>  	worktree=$(cd "$1" && git config core.worktree) ||
>  	worktree=unset
> 

I think the logic should be

	if test_have_prereq POSIXPERM && test -x "$1/config"

, right? If the system doesn't have POSIXPERM, then aren't all bets off
regarding file permissions?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I think the logic should be
>
> 	if test_have_prereq POSIXPERM && test -x "$1/config"
>
> , right?

Yeah ;-)

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

[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 <mhagger@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 & 07777) < 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 & 07777) < 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

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

[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

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

[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
mhagger@alum.mit.edu