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

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

[Martin Waitz]

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.


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

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

[Johannes Sixt]

Martin Waitz schrieb:
> 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

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

[Martin Waitz]

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.


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

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

[Johannes Schindelin]

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 &&=?

Ciao,
Dscho

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

[Andreas Ericsson]

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 |=

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

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

[Junio C Hamano]

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?

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 */
	    );

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

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.

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

Martin Waitz <tali@admingilde.org> writes:

> 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 ?

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Jan Hudec]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Thu, Oct 04, 2007 at 10:42:37 +0200, Martin Waitz wrote:
> 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>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

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

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

[Andreas Ericsson]

Junio C Hamano wrote:
> 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.

> 
> 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

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

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

hoi :)

On Thu, Oct 04, 2007 at 12:54:06AM +0100, Johannes Schindelin 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 &&=?

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

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]