Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.

Madhu [off-list ref] writes:

quoted

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 [off-list ref] <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 :)