threads / patch / 10126

patchgit-init: don't base core.filemode on the ability to chmod.

Subject: [PATCH] git-init: don't base core.filemode on the ability to chmod.

## tl;dr

14 messages between Oct 3, 2007 and Oct 10, 2007. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

Martin Waitz· Oct 3, 2007, 10:55 UTC · lore

At least on Linux the vfat file system honors chmod calls but does not store them permanently (as there is no on-disk format for it). So the filemode test which tries to chmod a file thinks that the file system does support file modes. This will result in problems when the file system gets mounted for the next time and all the executable bits are back.

A more reliable test for file systems without filemode support is to simply check if new files are created with the executable bit set.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---
 builtin-init-db.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
NOTE: this is only tested on Linux with ext3 and vfat file systems.
I do not know enough about the behaviour of other systems so there
may be regressions.
Show changes to builtin-init-db.c +1 −4
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..fbccacb 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -246,10 +246,7 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	/* 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 = !(st1.st_mode & S_IXUSR);
 	}
 	git_config_set("core.filemode", filemode ? "true" : "false");
 

-- 
1.5.3.3.8.g367dc7


-- 
Martin Waitz
Johannes Sixt· Oct 3, 2007, 12:19 UTC · re: Martin Waitz · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Martin Waitz schrieb:
Show 9 quoted lines
> At least on Linux the vfat file system honors chmod calls but does not
> store them permanently (as there is no on-disk format for it).
> So the filemode test which tries to chmod a file thinks that the file
> system does support file modes.  This will result in problems when the
> file system gets mounted for the next time and all the executable bits
> are back.
> 
> A more reliable test for file systems without filemode support is to
> simply check if new files are created with the executable bit set.

On Windows, we don't get an executable bit at all. Better use both heuristics, i.e. set core.filemode false if either one diagnoses an unreliable x-bit.

-- Hannes
Martin Waitz· Oct 3, 2007, 23:19 UTC · re: Johannes Sixt · lore

At least on Linux the vfat file system honors chmod calls but does not store them permanently (as there is no on-disk format for it). So the filemode test which tries to chmod a file thinks that the file system does support file modes which will result in problems later after the file system got remounted.

Now we check both that new files are created without the executable bit and that we can actually modify it with chmod.

Signed-off-by: Martin Waitz <tali@admingilde.org>
---  8<  ---
 builtin-init-db.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
On Wed, Oct 03, 2007 at 02:19:40PM +0200, Johannes Sixt wrote:
> On Windows, we don't get an executable bit at all. Better use both 
> heuristics, i.e. set core.filemode false if either one diagnoses an 
> unreliable x-bit.

this should work better for Windows. Previously I sent it only to Johannes and forgot to Cc the list.

Show changes to builtin-init-db.c +4 −1
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 763fa55..1d92916 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -247,7 +247,10 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	filemode = TEST_FILEMODE;
 	if (TEST_FILEMODE && !lstat(path, &st1)) {
 		struct stat st2;
-		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
+		/* test that new files are not created with X bit */
+		filemode = !(st1.st_mode & S_IXUSR);
+		/* test that we can modify the X bit */
+		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
 				!lstat(path, &st2) &&
 				st1.st_mode != st2.st_mode);
 	}

-- 
1.5.3.3.8.g367dc7

-- 
Martin Waitz
Johannes Schindelin· Oct 3, 2007, 23:54 UTC · re: Martin Waitz · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Hi,
On Thu, 4 Oct 2007, Martin Waitz wrote:
Show 5 quoted lines
> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> +		/* test that new files are not created with X bit */
> +		filemode = !(st1.st_mode & S_IXUSR);
> +		/* test that we can modify the X bit */
> +		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
Should that not be &&=?

Ciao, Dscho

Andreas Ericsson· Oct 4, 2007, 06:05 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Johannes Schindelin wrote:
Show 12 quoted lines
> Hi,
> 
> On Thu, 4 Oct 2007, Martin Waitz wrote:
> 
>> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>> +		/* test that new files are not created with X bit */
>> +		filemode = !(st1.st_mode & S_IXUSR);
>> +		/* test that we can modify the X bit */
>> +		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> 
> Should that not be &&=?
> 
I should think |=
-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Junio C Hamano· Oct 4, 2007, 06:23 UTC · re: Andreas Ericsson · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Andreas Ericsson <ae@op5.se> writes:
Show 15 quoted lines
> Johannes Schindelin wrote:
>> Hi,
>>
>> On Thu, 4 Oct 2007, Martin Waitz wrote:
>>
>>> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>> +		/* test that new files are not created with X bit */
>>> +		filemode = !(st1.st_mode & S_IXUSR);
>>> +		/* test that we can modify the X bit */
>>> +		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>
>> Should that not be &&=?
>>
>
> I should think |=
Is it?

The issue that started the thread was that chmod + stat check we originally had would say executable bit "seems to be" kept, while that is only true until the information is cached at VFS layer.

We create config file without asking for executable bit, so if we read it back as executable then that is a sure sign that the filesystem does not know what it is talking about, and we set filemode to zero in such a case. Similarly, if the chmod + stat check says we cannot set executable bit and read it back, then we also know the filesystem does not know about filemode.

So I think we can write it like this (indentation aside)...
filemode = !( (st1.st_mode & S_IXUSR)
        	/* we did not ask for x-bit -- bogus FS */
	    || chmod(path, st1.st_mode & S_IXUSR)
        	/* it does not let us flip x-bit -- bogus FS */
	    || lstat(path, &st2)
        	/* it does not let us read back -- bogus FS */
	    || (st1.st_mode == st2.st_mode)
	        /* it forgets we flipped -- bogus FS */
	    );
Martin Waitz· Oct 4, 2007, 07:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

hoi :)
On Wed, Oct 03, 2007 at 11:23:22PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> filemode = !( (st1.st_mode & S_IXUSR)
>         	/* we did not ask for x-bit -- bogus FS */
> 	    || chmod(path, st1.st_mode & S_IXUSR)
>         	/* it does not let us flip x-bit -- bogus FS */
> 	    || lstat(path, &st2)
>         	/* it does not let us read back -- bogus FS */
> 	    || (st1.st_mode == st2.st_mode)
> 	        /* it forgets we flipped -- bogus FS */
> 	    );
that looks good.
-- 
Martin Waitz
Junio C Hamano· Oct 4, 2007, 08:21 UTC · re: Martin Waitz · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Martin Waitz <tali@admingilde.org> writes:
Show 14 quoted lines
> hoi :)
>
> On Wed, Oct 03, 2007 at 11:23:22PM -0700, Junio C Hamano wrote:
>> filemode = !( (st1.st_mode & S_IXUSR)
>>         	/* we did not ask for x-bit -- bogus FS */
>> 	    || chmod(path, st1.st_mode & S_IXUSR)
>>         	/* it does not let us flip x-bit -- bogus FS */
>> 	    || lstat(path, &st2)
>>         	/* it does not let us read back -- bogus FS */
>> 	    || (st1.st_mode == st2.st_mode)
>> 	        /* it forgets we flipped -- bogus FS */
>> 	    );
>
> that looks good.

FWIW, I did not mean it to be an example for preferred indentation nor code layout, but as a better way to explain what the logic is computing.

I do not think git on Cygwin nor WinGit creates $GIT_DIR/config with executable bit set. Is this pretty much a workaround only for vfat-on-Linux ?

Johannes Sixt· Oct 4, 2007, 08:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Junio C Hamano schrieb:
Show 16 quoted lines
> Martin Waitz <tali@admingilde.org> writes:
>> On Wed, Oct 03, 2007 at 11:23:22PM -0700, Junio C Hamano wrote:
>>> filemode = !( (st1.st_mode & S_IXUSR)
>>>         	/* we did not ask for x-bit -- bogus FS */
>>> 	    || chmod(path, st1.st_mode & S_IXUSR)
>>>         	/* it does not let us flip x-bit -- bogus FS */
>>> 	    || lstat(path, &st2)
>>>         	/* it does not let us read back -- bogus FS */
>>> 	    || (st1.st_mode == st2.st_mode)
>>> 	        /* it forgets we flipped -- bogus FS */
>>> 	    );
>> that looks good.
> 
> I do not think git on Cygwin nor WinGit creates $GIT_DIR/config
> with executable bit set.  Is this pretty much a workaround only
> for vfat-on-Linux ?

I think so. Here on Windows, 'ls -l' after 'git init' tells that .git/config is not executable (both FAT and NTFS). But, anyway, as far as the MinGW port is concerned, at least the last condition in the sequence above triggers and causes filemode=false, which is good.

-- Hannes
Martin Waitz· Oct 4, 2007, 08:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

hoi :)
On Thu, Oct 04, 2007 at 01:21:02AM -0700, Junio C Hamano wrote:
> FWIW, I did not mean it to be an example for preferred
> indentation nor code layout, but as a better way to explain what
> the logic is computing.
sure, and I think it makes sense the way you wrote it.
> I do not think git on Cygwin nor WinGit creates $GIT_DIR/config
> with executable bit set.  Is this pretty much a workaround only
> for vfat-on-Linux ?

I just checked Cygwin, it creates files without executable bit and disregards a chmod +x. So yes, this seems to be a Linux-only problem.

-- 
Martin Waitz
Jan Hudec· Oct 10, 2007, 09:47 UTC · re: Martin Waitz · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

On Thu, Oct 04, 2007 at 10:42:37 +0200, Martin Waitz wrote:
Show 15 quoted lines
> hoi :)
> 
> On Thu, Oct 04, 2007 at 01:21:02AM -0700, Junio C Hamano wrote:
> > FWIW, I did not mean it to be an example for preferred
> > indentation nor code layout, but as a better way to explain what
> > the logic is computing.
> 
> sure, and I think it makes sense the way you wrote it.
> 
> > I do not think git on Cygwin nor WinGit creates $GIT_DIR/config
> > with executable bit set.  Is this pretty much a workaround only
> > for vfat-on-Linux ?
> 
> I just checked Cygwin, it creates files without executable bit and
> disregards a chmod +x.  So yes, this seems to be a Linux-only problem.

AFAIR in cygwin it depends on both configuration of cygwin and whether you are on NTFS or FAT. On NTFS with proper setting (CYGWIN=ntsec IIRC), cygwin actually implements the x bit (uses the NT ACL to somewhat represent it). On other filesystem there is an option somewhere to tell whether it should show the x bit always set or always cleared (for me it seems to be always set).

On the other hand MSYS shows the x bit as set whenever the file has executable extension. I don't think cygwin has this mode.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>
Johannes Schindelin· Oct 4, 2007, 12:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Hi,
On Thu, 4 Oct 2007, Junio C Hamano wrote:
> I do not think git on Cygwin nor WinGit creates $GIT_DIR/config with 
> executable bit set.  Is this pretty much a workaround only for 
> vfat-on-Linux ?

I do not know precisely about Cygwin (a quick test on my USB stick shows that .git/config is created without the executable bit set), but in MSys plain files which start with a she-bang are automatically +x, while all other plain files are automatically -x (therefore this applies to .git/config).

Given these findings, I fail to see what the patch should achieve, as the first test (for -x) always succeeds...

Ciao, Dscho

Andreas Ericsson· Oct 4, 2007, 10:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

Junio C Hamano wrote:
Show 18 quoted lines
> Andreas Ericsson <ae@op5.se> writes:
> 
>> Johannes Schindelin wrote:
>>> Hi,
>>>
>>> On Thu, 4 Oct 2007, Martin Waitz wrote:
>>>
>>>> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>>> +		/* test that new files are not created with X bit */
>>>> +		filemode = !(st1.st_mode & S_IXUSR);
>>>> +		/* test that we can modify the X bit */
>>>> +		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>> Should that not be &&=?
>>>
>> I should think |=
> 
> Is it?
>

Nopes. I misread the first expression and simply assumed that "filemode" should be != 0 for FS not supporting the x bit. I'd rename the variable to bogus_fs and flip the logic, but I have no strong opinion either way.

Show 12 quoted lines
> 
> So I think we can write it like this (indentation aside)...
> 
> filemode = !( (st1.st_mode & S_IXUSR)
>         	/* we did not ask for x-bit -- bogus FS */
> 	    || chmod(path, st1.st_mode & S_IXUSR)
>         	/* it does not let us flip x-bit -- bogus FS */
> 	    || lstat(path, &st2)
>         	/* it does not let us read back -- bogus FS */
> 	    || (st1.st_mode == st2.st_mode)
> 	        /* it forgets we flipped -- bogus FS */
> 	    );

For "filemode=0 means FS doesn't support x-bit" it looks about right, but kinda cumbersome to read.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Martin Waitz· Oct 4, 2007, 07:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-init: don't base core.filemode on the ability to chmod.

hoi :)
On Thu, Oct 04, 2007 at 12:54:06AM +0100, Johannes Schindelin wrote:
Show 7 quoted lines
> > -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> > +		/* test that new files are not created with X bit */
> > +		filemode = !(st1.st_mode & S_IXUSR);
> > +		/* test that we can modify the X bit */
> > +		filemode &= (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> 
> Should that not be &&=?

originally I wrote it that way but the compiler complaint. I guess we are too used to perl ;-)

-- 
Martin Waitz

← back to recent threads