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