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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2021, 04:53 UTC
Message-ID: <xmqq7dlz94by.fsf@gitster.g>
In-Reply-To: <20210322.081043.1437207928602570397.enometh@meer.net>

Madhu <enometh@meer.net> writes:

Show 15 quoted lines

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