| Message ID | 20210321.175821.1385189088303987287.enometh@meer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | init: don't reset core.filemode on git-new-workdirs. | expand |
Madhu <enometh@meer.net> writes: > From: Madhu <enometh@net.meer> > > If the .git/config file is a symlink (as is the case of a .git created > by the contrib/workdir/git-new-workdir script) then the filemode tests > fail, and the filemode is reset to be false. To avoid this only munge > core.filemode if .git/config is a regular file. Hmph, what's the sequence of events? You let "git new-workdir" to create a cheap copy of a working tree and then? When new-workdir returns, you already have a functional working tree with .git/ directory (in which there are many symbolic links). So who wants or needs to run "git init" there in the directory in the first place? Is the problem being solved that running an unnecessary "git init" in an already initialized repository does an unnecessary filemode check? If that is the case, I am not sure if asking "is it a symlink?" to avoid the filemode trustability check is a good approach. At that point in the code you are patching, we have already determined if we are running the "git init" in an already initialized repository (i.e. "reinit"), so shouldn't we be basing the decision on it instead? I see that in a later part of the same function, we test if the filesystem supports symbolic links but do so only when we are running "git init" afresh. Perhaps the filemode trustability check and the config-set to record core.filemode should all be moved there inside the "if (!reinit)" block. All of the above assumes that the problem being solved is about what happens when "git init" is run in an already functioning working tree. If I misread what problem you are trying to solve, then none of what I suggested in the above may apply. > --- > builtin/init-db.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Missing sign-off; please review Documentation/SubmittingPatches > diff --git a/builtin/init-db.c b/builtin/init-db.c > index dcc45bef51..b053107336 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -285,7 +285,8 @@ static int create_default_files(const char *template_path, > /* Check filemode trustability */ > path = git_path_buf(&buf, "config"); > filemode = TEST_FILEMODE; > - if (TEST_FILEMODE && !lstat(path, &st1)) { > + if (TEST_FILEMODE && !lstat(path, &st1) > + && (st1.st_mode & S_IFMT) == S_IFREG) { > struct stat st2; > filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) && > !lstat(path, &st2) &&
* Junio C Hamano <gitster@pobox.com> <xmqq1rc89nk7.fsf@gitster.g> Wrote on Sun, 21 Mar 2021 14:58:16 -0700 > Madhu <enometh@meer.net> writes: >> If the .git/config file is a symlink (as is the case of a .git created >> by the contrib/workdir/git-new-workdir script) then the filemode tests >> fail, and the filemode is reset to be false. To avoid this only munge >> core.filemode if .git/config is a regular file. > > Hmph, what's the sequence of events? You let "git new-workdir" to > create a cheap copy of a working tree and then? When new-workdir > returns, you already have a functional working tree with .git/ > directory (in which there are many symbolic links). So who wants or > needs to run "git init" there in the directory in the first place? In this case it was part of a script which created commits from tarballs. If there was no .git git-init would create it. If there was, and it should ov harmlessly "re-initialized" it (whatever that means) > Is the problem being solved that running an unnecessary "git init" > in an already initialized repository does an unnecessary filemode > check? Yes the problem could have be avoided by not calling git-init in an existing repository. But the docs say that if there is a .git directory, it would "reinitialize it". Re-initialization was not expected to change the filemode incorrectly. git-new-workdir is extremly useful in this case. With no overhead I can create a .git without doing a checkout. Then i can move the .git into a directory where the tarball has been unpacked - do a "git add -A -f", "git commit" I expect the commit to have the exact contents of the tarball. and by paying attention to commit info and dates i have a commit-sha1 reproducible history which can be created anywhere. If the filemode is being changed in .git/config without my knowledge all further commits in the repo are affected future tarball imports are corrupted. I was using this extensively to track changes to point releases and the impact is serious (personally) and I have a lot of useless repositories which I have been tracking for over a year whose histories are not commit-sha reproducible. > If that is the case, I am not sure if asking "is it a symlink?" to > avoid the filemode trustability check is a good approach. At that > point in the code you are patching, we have already determined if we > are running the "git init" in an already initialized repository > (i.e. "reinit"), so shouldn't we be basing the decision on it > instead? > I see that in a later part of the same function, we test if the > filesystem supports symbolic links but do so only when we are > running "git init" afresh. Perhaps the filemode trustability check > and the config-set to record core.filemode should all be moved there > inside the "if (!reinit)" block. > > All of the above assumes that the problem being solved is about what > happens when "git init" is run in an already functioning working > tree. If I misread what problem you are trying to solve, then none > of what I suggested in the above may apply. I think you have understood the problem. At present But doing the filemode trustability check on .git/config assumes it is a regular file anyway if it is to work at all. My suggestion in the patch only enforces that assumption explicitly.
Madhu <enometh@meer.net> writes: >> I see that in a later part of the same function, we test if the >> filesystem supports symbolic links but do so only when we are >> running "git init" afresh. Perhaps the filemode trustability check >> and the config-set to record core.filemode should all be moved there >> inside the "if (!reinit)" block. >> >> All of the above assumes that the problem being solved is about what >> happens when "git init" is run in an already functioning working >> tree. If I misread what problem you are trying to solve, then none >> of what I suggested in the above may apply. > > I think you have understood the problem. At present But doing the > filemode trustability check on .git/config assumes it is a regular > file anyway if it is to work at all. My suggestion in the patch only > enforces that assumption explicitly. There are two points we should consider. * Historically, we've used .git/config as the sample file to check, but that was not because we wanted to make sure we can chmod the config file, but because we knew the file has to be there. If .git/config is sometimes a symbolic link, and if chmod test requires a regular file, we do not have to use .git/config as the sample file. We could instead switch to use a different, temporary, file. After all, the symlink check I pointed out in the message you are responding to uses a brand new temporary filename for that, and there is no reason why we shouldn't do the same with a regular file for the filemode test. * If running "git init" in an already functioning repository can be a useful way to "re-initialize" and/or "correct" various aspect of the repository (e.g. perhaps core.filemode is incorrectly set originally and running "git init" again corrects it), we would want to allow that in a normal repository as well as in a repository that is created by new-workdir the same way. Or if it is not useful and we want "re-initialize" not to touch the filemode, we would want to skip the check in a normal repository as well as in a new-workdir repository the same way. That is why "if symlink, then skip" is wrong---it targets the new-workdir case specifically. I personally do not have a strong opinion either way, but to me, it seems that "does the filesystem support filemode?" and "does the filesystem support symbolic link?" are at about the same level and should be treated similarly unless there is a good reason not to. And the symlink check is never done in "reinit" case, so perhaps when "git init" is run again in an already functioning repository, we should not muck with the filemode, either. A natural conclusion of the line of thought is that we can move the "check filemode trustability" block (from the comment to concluding git_config_set()) inside the "if (!reinit)" that happens a bit later and we'd be fine---as an existing normal repository, as well as what new-workdir creates, won't have to do the "let's chmod +x/-x the config file and see what happens" code at all (perhaps the attached patch, which hasn't even been compile tested). On the other hand, if it is worth "fixing" the filemode setting while re-initializing, we probably should switch to use a temporary file instead of 'config'. And we may want to reconsider the placement of the "is symlink supported?" check---which may also have to be redone to "fix" its existing value. builtin/init-db.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git c/builtin/init-db.c w/builtin/init-db.c index dcc45bef51..61817a02a8 100644 --- c/builtin/init-db.c +++ w/builtin/init-db.c @@ -282,20 +282,6 @@ static int create_default_files(const char *template_path, initialize_repository_version(fmt->hash_algo, 0); - /* Check filemode trustability */ - path = git_path_buf(&buf, "config"); - 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 && - !chmod(path, st1.st_mode)); - if (filemode && !reinit && (st1.st_mode & S_IXUSR)) - filemode = 0; - } - git_config_set("core.filemode", filemode ? "true" : "false"); - if (is_bare_repository()) git_config_set("core.bare", "true"); else { @@ -309,6 +295,20 @@ static int create_default_files(const char *template_path, } if (!reinit) { + /* Check filemode trustability */ + path = git_path_buf(&buf, "config"); + 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 && + !chmod(path, st1.st_mode)); + if (filemode && !reinit && (st1.st_mode & S_IXUSR)) + filemode = 0; + } + git_config_set("core.filemode", filemode ? "true" : "false"); + /* Check if symlink is supported in the work tree */ path = git_path_buf(&buf, "tXXXXXX"); if (!close(xmkstemp(path)) &&
* Junio C Hamano <gitster@pobox.com> <xmqq7dlz94by.fsf@gitster.g> Wrote on Sun, 21 Mar 2021 21:53:37 -0700 > There are two points we should consider. > > * Historically, we've used .git/config as the sample file to check, > but that was not because we wanted to make sure we can chmod the > config file, but because we knew the file has to be there. If > .git/config is sometimes a symbolic link, and if chmod test > requires a regular file, we do not have to use .git/config as the > sample file. We could instead switch to use a different, > temporary, file. After all, the symlink check I pointed out in > the message you are responding to uses a brand new temporary > filename for that, and there is no reason why we shouldn't do the > same with a regular file for the filemode test. > > * If running "git init" in an already functioning repository can be > a useful way to "re-initialize" and/or "correct" various aspect > of the repository (e.g. perhaps core.filemode is incorrectly set > originally and running "git init" again corrects it), we would > want to allow that in a normal repository as well as in a > repository that is created by new-workdir the same way. Or if it > is not useful and we want "re-initialize" not to touch the > filemode, we would want to skip the check in a normal repository > as well as in a new-workdir repository the same way. That is why > "if symlink, then skip" is wrong---it targets the new-workdir > case specifically. I'd say it doesn't (target the new-workdir case specifically). The lstat test on `config' is incorrect if `config' (or a new file) is not a regular file, so I think it should be fixed anyway. The new-workdir case happens to be handled correctly once this is fixed. > I personally do not have a strong opinion either way, but to me, it > seems that "does the filesystem support filemode?" and "does the > filesystem support symbolic link?" are at about the same level and > should be treated similarly unless there is a good reason not to. > And the symlink check is never done in "reinit" case, so perhaps > when "git init" is run again in an already functioning repository, > we should not muck with the filemode, either. I'd think so (on the last point). While it is understandable to expect consistent re-initing behaviour (which is why the spurious git-init was there) the expectations may not hold if the git directory and work tree are on different filesystems - there is more scope for making wrong inferences. Typically checkout worktrees with new-workdir in /dev/shm for advantages in the low overhead. I do have repositories where .git/config is a symlink to a config.A and I have other config.B files - all for the same repo but with different upstreams. (I know better ways to do it but why should I be prevented from doing this) > A natural conclusion of the line of thought is that we can move the > "check filemode trustability" block (from the comment to concluding > git_config_set()) inside the "if (!reinit)" that happens a bit later > and we'd be fine---as an existing normal repository, as well as what > new-workdir creates, won't have to do the "let's chmod +x/-x the > config file and see what happens" code at all (perhaps the attached > patch, which hasn't even been compile tested). > > On the other hand, if it is worth "fixing" the filemode setting > while re-initializing, we probably should switch to use a temporary > file instead of 'config'. And we may want to reconsider the placement > of the "is symlink supported?" check---which may also have to be > redone to "fix" its existing value. I don't think the posted patch (snipped) would work as reinit is always 1 and we are always a candidate for reiniting - I may be missing something. Using a new file for the filemode test would be a natural improvement. But I dont see the added complexity as a win. I can imagine other scenarios that need to be fixed: calling git-init on a worktree prepared by git-new-worktree (not git-new-workdir) on a FAT fs with .git on ext2. Calling git-init on a worktree prepared by calling git-new-worktree with a parent which is checked out by git-new-workdir. (The parent has a symlinked config). The possibilities for fun endless :)
Madhu <enometh@meer.net> writes: > * Junio C Hamano <gitster@pobox.com> <xmqq7dlz94by.fsf@gitster.g> >> ... >> And the symlink check is never done in "reinit" case, so perhaps >> when "git init" is run again in an already functioning repository, >> we should not muck with the filemode, either. > > I'd think so (on the last point).... So, assuming that you are with me to think that reinit should not touch the filemode thing, ... >> A natural conclusion of the line of thought is that we can move the >> "check filemode trustability" block (from the comment to concluding >> git_config_set()) inside the "if (!reinit)" that happens a bit later >> and we'd be fine---as an existing normal repository, as well as what >> new-workdir creates, won't have to do the "let's chmod +x/-x the >> config file and see what happens" code at all (perhaps the attached >> patch, which hasn't even been compile tested). >> ... ... wouldn't the illustration patch I gave, which removed the "check filemode" bit from the main codepath and moved it to inside an if block that is executed only when "if (!reinit)" is true, "skip" the problematic "check if config is a regular file whose executable bit can be flipped and flopped" code in your use case, i.e. in an existing repository? > I don't think the posted patch (snipped) would work as reinit is > always 1 and we are always a candidate for reiniting - I may be > missing something. In other words, yes, the illustration patch you are responding to assumes that the "reinit" variable is set correctly (i.e. the HEAD exists and sensibly readable if you run "git init" in an already functioning working tree) and we can use it to avoid the filemode check. > Using a new file for the filemode test would be a natural > improvement. That becomes necessary only if we want to futz with core.filemode while doing "reinit", as .git/config can be a symlink. When we are creating a repository from scratch, we always create a regular file to prepare .git/config, and there is no need to do that, if we are happy to set core.filemode the same way as core.symlinks, i.e. only check once when the repository is created. No? Thanks.
* Junio C Hamano <gitster@pobox.com> <xmqqr1k76p8d.fsf@gitster.g> Wrote on Mon, 22 Mar 2021 11:02:42 -0700 >> I don't think the posted patch (snipped) would work as reinit is >> always 1 and we are always a candidate for reiniting - I may be >> missing something. > > In other words, yes, the illustration patch you are responding to > assumes that the "reinit" variable is set correctly (i.e. the HEAD > exists and sensibly readable if you run "git init" in an already > functioning working tree) and we can use it to avoid the filemode > check. Ah yes indeed. >> Using a new file for the filemode test would be a natural >> improvement. > > That becomes necessary only if we want to futz with core.filemode > while doing "reinit", as .git/config can be a symlink. When we are > creating a repository from scratch, we always create a regular file > to prepare .git/config, and there is no need to do that, if we are > happy to set core.filemode the same way as core.symlinks, i.e. only > check once when the repository is created. No? Avoiding the filemode check completely during reinit is ok with me because it gave me wrong results. I can't speak for the original author of the code - if his intention was to do it explicitly as part of "reinitialization". [In the latter case personally I'd want to edit the config file by hand in response to a message from git - and I certainly do not want git to change the config file behind my back without informing me.] Thanks again.
Madhu <enometh@meer.net> writes: > Avoiding the filemode check completely during reinit is ok with me > because it gave me wrong results. I can't speak for the original > author of the code - if his intention was to do it explicitly as part > of "reinitialization". As the original author of the code, I know I meant filemode check to be done and redone upon reinitialization in 4f629539 (init-db: check template and repository format., 2005-11-25). But then when 75d24499 (git-init: autodetect core.symlinks, 2007-08-31) started to autodetect symbolic link support, I somehow ended up doing it only upon the repository creation. Later, 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated to check case sensitivity in the same block, doing it only once. Either of these two commits would have been a good chance for us to realize that filemode check should be done the same way, but somehow nobody noticed X-<.
On Mon, Mar 22, 2021 at 11:39:31PM -0700, Junio C Hamano wrote: > Madhu <enometh@meer.net> writes: > > > Avoiding the filemode check completely during reinit is ok with me > > because it gave me wrong results. I can't speak for the original > > author of the code - if his intention was to do it explicitly as part > > of "reinitialization". > > As the original author of the code, I know I meant filemode check to > be done and redone upon reinitialization in 4f629539 (init-db: check > template and repository format., 2005-11-25). > > But then when 75d24499 (git-init: autodetect core.symlinks, > 2007-08-31) started to autodetect symbolic link support, I somehow > ended up doing it only upon the repository creation. Later, > 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated > to check case sensitivity in the same block, doing it only once. > > Either of these two commits would have been a good chance for us to > realize that filemode check should be done the same way, but somehow > nobody noticed X-<. > In the very long run, there may be room for improvements: While core.filemode works for a loal repo on a local disk, there are lots of cases where I whish a better handling. Exporting a git repo from e.g. Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off Visiting the same repo under Git-for-Windows and cygwin: cygwin supports the executable bit, Git-for-Windows does not. And now we have the worktree (which may cross filesytem borders) Today there are many use cases, where a single config variable is not ideal. If there is a chance to have a "core.filemode=auto", which does probe the filemode for this very OS/filesytem/worktree combination: I would be happy to test/review/mentor such a code change.
Torsten Bögershausen <tboegi@web.de> writes: > In the very long run, there may be room for improvements: > While core.filemode works for a loal repo on a local disk, > there are lots of cases where I whish a better handling. But that is why we have multiple worktrees and per worktree configuration, no? Fundamentally, you cannot share a worktree from two systems and let one work with filemode on while the other with filemode off, as you summarized here: > Exporting a git repo from e.g. > Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on > Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off > > Visiting the same repo under Git-for-Windows and cygwin: > cygwin supports the executable bit, Git-for-Windows does not. The "new-workdirs" was a cute hack that was quite useful before the multiple worktree support materialized, and it and (proper) worktree share the quirk that by default the per-repo configuration file is shared. > And now we have the worktree (which may cross filesytem borders) > > Today there are many use cases, where a single config variable is not ideal. > > If there is a chance to have a "core.filemode=auto", which does probe the > filemode for this very OS/filesytem/worktree combination: > I would be happy to test/review/mentor such a code change. I do not think we want to go there. filemode is not the only thing that would be shared. What do you want to do with core.eol=native, for example? Paths touched while switching branches get the 'native' line endings on the system that the user happened to be on when the "switch" command was run, and working tree files end up with mixture?
On Tue, Mar 23, 2021 at 10:45:38AM -0700, Junio C Hamano wrote: > Torsten Bögershausen <tboegi@web.de> writes: > > > In the very long run, there may be room for improvements: > > While core.filemode works for a loal repo on a local disk, > > there are lots of cases where I whish a better handling. > > But that is why we have multiple worktrees and per worktree > configuration, no? > > Fundamentally, you cannot share a worktree from two systems and let > one work with filemode on while the other with filemode off, as you > summarized here: > > > Exporting a git repo from e.g. > > Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on > > Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off > > > > Visiting the same repo under Git-for-Windows and cygwin: > > cygwin supports the executable bit, Git-for-Windows does not. > > The "new-workdirs" was a cute hack that was quite useful before the > multiple worktree support materialized, and it and (proper) worktree > share the quirk that by default the per-repo configuration file is > shared. > > > And now we have the worktree (which may cross filesytem borders) > > > > Today there are many use cases, where a single config variable is not ideal. > > > > If there is a chance to have a "core.filemode=auto", which does probe the > > filemode for this very OS/filesytem/worktree combination: > > I would be happy to test/review/mentor such a code change. > > I do not think we want to go there. filemode is not the only thing > that would be shared. What do you want to do with core.eol=native, > for example? Paths touched while switching branches get the 'native' > line endings on the system that the user happened to be on when the > "switch" command was run, and working tree files end up with mixture? I don't intend to solve all possible confusions caused by sharing all config variables - just this very one. After some thinking, the following may illustrate a brute-force method, not nice, neither optimized, nor perfect or bug free: $ alias alias git='~/bin2/git.sh' cat ~/bin2/git.sh #!/bin/sh probe_executable() { touch $$.xxx if test -x $$.xxx; then rm $$.xxx echo "-c core.filemode=false" return fi chmod +x $$.xxx if ! test -x $$.xxx; then rm $$.xxx echo "-c core.filemode=false" return fi rm $$.xxx return } git $(probe_executable) "$@"
Torsten Bögershausen <tboegi@web.de> writes: >> I do not think we want to go there. filemode is not the only thing >> that would be shared. What do you want to do with core.eol=native, >> for example? Paths touched while switching branches get the 'native' >> line endings on the system that the user happened to be on when the >> "switch" command was run, and working tree files end up with mixture? > > I don't intend to solve all possible confusions caused by sharing all > config variables - just this very one. I intend to help users by drawing a clear red line and telling them that crossing that line leads them to danger. Sharing a working tree across systems that require different core.* settings (filemode included) is on the other side of that line, and that is the reason why I said I do not think we want to go there. By saying "this is only about core.filemode but you can share a working tree between incompatible systems", I am afraid that we end up training users to go where they should not get nearby.
I don't think this got any further attention? I notice git-2.32 still has the problem - in which case I re-commend my original patch. Regards ---Madhu * Junio C Hamano <gitster@pobox.com> <xmqqr1k64bmk.fsf@gitster.g> Wrote on Mon, 22 Mar 2021 23:39:31 -0700 > Madhu <enometh@meer.net> writes: >> Avoiding the filemode check completely during reinit is ok with me >> because it gave me wrong results. I can't speak for the original >> author of the code - if his intention was to do it explicitly as part >> of "reinitialization". > > As the original author of the code, I know I meant filemode check to > be done and redone upon reinitialization in 4f629539 (init-db: check > template and repository format., 2005-11-25). > > But then when 75d24499 (git-init: autodetect core.symlinks, > 2007-08-31) started to autodetect symbolic link support, I somehow > ended up doing it only upon the repository creation. Later, > 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated > to check case sensitivity in the same block, doing it only once. > > Either of these two commits would have been a good chance for us to > realize that filemode check should be done the same way, but somehow > nobody noticed X-<. >
diff --git a/builtin/init-db.c b/builtin/init-db.c index dcc45bef51..b053107336 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -285,7 +285,8 @@ static int create_default_files(const char *template_path, /* Check filemode trustability */ path = git_path_buf(&buf, "config"); filemode = TEST_FILEMODE; - if (TEST_FILEMODE && !lstat(path, &st1)) { + if (TEST_FILEMODE && !lstat(path, &st1) + && (st1.st_mode & S_IFMT) == S_IFREG) { struct stat st2; filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) && !lstat(path, &st2) &&
From: Madhu <enometh@net.meer> If the .git/config file is a symlink (as is the case of a .git created by the contrib/workdir/git-new-workdir script) then the filemode tests fail, and the filemode is reset to be false. To avoid this only munge core.filemode if .git/config is a regular file. --- builtin/init-db.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)