git-init: don't base core.filemode on the ability to chmod.
PATCH 1/1 git-init: don't base core.filemode on the ability to chmod.
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.
--- 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.
builtin-init-db.c +1 -4
@@ -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" );
Signature · 5 lines
1.5.3.3.8.g367dc7 -- Martin Waitz
Quoted 9 lines · 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.
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 1/1 git-init: don't base core.filemode on the ability to chmod.
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.
--- 8< ---
builtin-init-db.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
Quoted 3 lines · Johannes Sixt
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.
builtin-init-db.c +4 -1
@@ -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); }
Signature · 4 lines
1.5.3.3.8.g367dc7 -- Martin Waitz
Hi,
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 253
Quoted 5 lines · Martin Waitz
builtin-init-db.c +4
-1- 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
hoi :)
Quoted 7 lines · Johannes Schindelin
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 253
Quoted 5 lines
builtin-init-db.c +4
-1- 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 ;-)
Signature · 1 line
Martin Waitz
Quoted 4 lines · Johannes Schindelin
Hi,
On Thu, 4 Oct 2007, Martin Waitz wrote:
Quoted 8 lines
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 253
Quoted 5 lines
builtin-init-db.c +4
-1- 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 |=
Signature · 3 lines
Andreas Ericsson [email protected] OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231
Quoted 14 lines · Andreas Ericsson
Quoted 4 lines · Johannes Schindelin
Hi,
On Thu, 4 Oct 2007, Martin Waitz wrote:
Quoted 8 lines
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 253
Quoted 5 lines
builtin-init-db.c +4
-1- 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 &&=?
Quoted 1 line
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 */
);hoi :)
Quoted 9 lines · Junio C Hamano
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.
Signature · 1 line
Martin Waitz
Quoted 13 lines · Martin Waitz
hoi :)
Quoted 9 lines · Junio C Hamano
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 */ );
Quoted 1 line
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 ?
hoi :)
Quoted 3 lines · Junio C Hamano
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.
Quoted 3 lines
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.
Signature · 1 line
Martin Waitz
Quoted 7 lines · Martin Waitz
hoi :)
Quoted 3 lines · Junio C Hamano
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.
Quoted 8 lines
sure, and I think it makes sense the way you wrote it.
Quoted 3 lines
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.
Signature · 1 line
Jan 'Bulb' Hudec <[email protected]>
Quoted 11 lines · Junio C Hamano
Quoted 10 lines · Martin Waitz
Quoted 9 lines · Junio C Hamano
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 */ );
Quoted 5 lines
Quoted 1 line
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
Hi,
Quoted 3 lines · Junio C Hamano
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
Quoted 14 lines · Junio C Hamano
Quoted 12 lines · Andreas Ericsson
Quoted 11 lines · Johannes Schindelin
Hi,
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 253
Quoted 5 lines · Martin Waitz
builtin-init-db.c +4
-1- 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 &&=?
Quoted 4 lines
Quoted 1 line
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.
Quoted 4 lines
So I think we can write it like this (indentation aside)...
filemode = !( (st1.st_mode &
S_IXUSR)
Quoted 8 lines
/* 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.
Signature · 3 lines
Andreas Ericsson [email protected] OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231