{"thread":{"id":"55361","subject":"[PATCH] init: don't reset core.filemode on git-new-workdirs.","startedAt":"2021-03-21T12:36:30Z","lastAt":"2021-06-18T04:25:37Z","messageCount":13,"participants":["Madhu","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"419848","messageId":"20210321.175821.1385189088303987287.enometh@meer.net","threadId":"55361","inReplyTo":null,"subject":"[PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Madhu","fromEmail":"enometh@meer.net","sentAt":"2021-03-21T12:28:21Z","receivedAt":"2021-03-21T12:36:30Z","isPatch":true,"sender":{"key":"enometh@meer.net","avatar":null},"body":"From: Madhu \n\nIf the .git/config file is a symlink (as is the case of a .git created\nby the contrib/workdir/git-new-workdir script) then the filemode tests\nfail, and the filemode is reset to be false. To avoid this only munge\ncore.filemode if .git/config is a regular file.\n---\n builtin/init-db.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex dcc45bef51..b053107336 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -285,7 +285,8 @@ static int create_default_files(const char *template_path,\n \t/* Check filemode trustability */\n \tpath = git_path_buf(&buf, \"config\");\n \tfilemode = TEST_FILEMODE;\n-\tif (TEST_FILEMODE && !lstat(path, &st1)) {\n+\tif (TEST_FILEMODE && !lstat(path, &st1)\n+\t && (st1.st_mode & S_IFMT) == S_IFREG) {\n \t\tstruct stat st2;\n \t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n \t\t\t\t!lstat(path, &st2) &&\n-- \n2.31.0.dirty\n"},{"id":"419890","messageId":"xmqq1rc89nk7.fsf@gitster.g","threadId":"55361","inReplyTo":"20210321.175821.1385189088303987287.enometh@meer.net","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-21T21:58:16Z","receivedAt":"2021-03-21T21:59:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Madhu writes:\n\n> From: Madhu \n>\n> If the .git/config file is a symlink (as is the case of a .git created\n> by the contrib/workdir/git-new-workdir script) then the filemode tests\n> fail, and the filemode is reset to be false. To avoid this only munge\n> core.filemode if .git/config is a regular file.\n\nHmph, what's the sequence of events? You let \"git new-workdir\" to\ncreate a cheap copy of a working tree and then? When new-workdir\nreturns, you already have a functional working tree with .git/\ndirectory (in which there are many symbolic links). So who wants or\nneeds to run \"git init\" there in the directory in the first place?\n\nIs the problem being solved that running an unnecessary \"git init\"\nin an already initialized repository does an unnecessary filemode\ncheck?\n\nIf that is the case, I am not sure if asking \"is it a symlink?\" to\navoid the filemode trustability check is a good approach. At that\npoint in the code you are patching, we have already determined if we\nare running the \"git init\" in an already initialized repository\n(i.e. \"reinit\"), so shouldn't we be basing the decision on it\ninstead?\n\nI see that in a later part of the same function, we test if the\nfilesystem supports symbolic links but do so only when we are\nrunning \"git init\" afresh. Perhaps the filemode trustability check\nand the config-set to record core.filemode should all be moved there\ninside the \"if (!reinit)\" block.\n\nAll of the above assumes that the problem being solved is about what\nhappens when \"git init\" is run in an already functioning working\ntree. If I misread what problem you are trying to solve, then none\nof what I suggested in the above may apply.\n\n> ---\n> builtin/init-db.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n\nMissing sign-off; please review Documentation/SubmittingPatches\n\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index dcc45bef51..b053107336 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -285,7 +285,8 @@ static int create_default_files(const char *template_path,\n> \t/* Check filemode trustability */\n> \tpath = git_path_buf(&buf, \"config\");\n> \tfilemode = TEST_FILEMODE;\n> -\tif (TEST_FILEMODE && !lstat(path, &st1)) {\n> +\tif (TEST_FILEMODE && !lstat(path, &st1)\n> +\t && (st1.st_mode & S_IFMT) == S_IFREG) {\n> \t\tstruct stat st2;\n> \t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n> \t\t\t\t!lstat(path, &st2) &&\n"},{"id":"419900","messageId":"20210322.081043.1437207928602570397.enometh@meer.net","threadId":"55361","inReplyTo":"xmqq1rc89nk7.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Madhu","fromEmail":"enometh@meer.net","sentAt":"2021-03-22T02:40:43Z","receivedAt":"2021-03-22T02:41:46Z","isPatch":true,"sender":{"key":"enometh@meer.net","avatar":null},"body":"* Junio C Hamano \nWrote on Sun, 21 Mar 2021 14:58:16 -0700\n> Madhu writes:\n>> If the .git/config file is a symlink (as is the case of a .git created\n>> by the contrib/workdir/git-new-workdir script) then the filemode tests\n>> fail, and the filemode is reset to be false. To avoid this only munge\n>> core.filemode if .git/config is a regular file.\n>\n> Hmph, what's the sequence of events? You let \"git new-workdir\" to\n> create a cheap copy of a working tree and then? When new-workdir\n> returns, you already have a functional working tree with .git/\n> directory (in which there are many symbolic links). So who wants or\n> needs to run \"git init\" there in the directory in the first place?\n\nIn this case it was part of a script which created commits from\ntarballs. If there was no .git git-init would create it. If there\nwas, and it should ov harmlessly \"re-initialized\" it (whatever that\nmeans)\n\n> Is the problem being solved that running an unnecessary \"git init\"\n> in an already initialized repository does an unnecessary filemode\n> check?\n\nYes the problem could have be avoided by not calling git-init in an\nexisting repository. But the docs say that if there is a .git\ndirectory, it would \"reinitialize it\". Re-initialization was not\nexpected to change the filemode incorrectly.\n\ngit-new-workdir is extremly useful in this case. With no overhead I\ncan create a .git without doing a checkout. Then i can move the .git\ninto a directory where the tarball has been unpacked - do a \"git add\n-A -f\", \"git commit\" I expect the commit to have the exact contents of\nthe tarball. and by paying attention to commit info and dates i have a\ncommit-sha1 reproducible history which can be created anywhere.\n\nIf the filemode is being changed in .git/config without my knowledge\nall further commits in the repo are affected future tarball imports\nare corrupted.\n\nI was using this extensively to track changes to point releases and\nthe impact is serious (personally) and I have a lot of useless\nrepositories which I have been tracking for over a year whose\nhistories are not commit-sha reproducible.\n\n> If that is the case, I am not sure if asking \"is it a symlink?\" to\n> avoid the filemode trustability check is a good approach. At that\n> point in the code you are patching, we have already determined if we\n> are running the \"git init\" in an already initialized repository\n> (i.e. \"reinit\"), so shouldn't we be basing the decision on it\n> instead?\n\n> I see that in a later part of the same function, we test if the\n> filesystem supports symbolic links but do so only when we are\n> running \"git init\" afresh. Perhaps the filemode trustability check\n> and the config-set to record core.filemode should all be moved there\n> inside the \"if (!reinit)\" block.\n>\n> All of the above assumes that the problem being solved is about what\n> happens when \"git init\" is run in an already functioning working\n> tree. If I misread what problem you are trying to solve, then none\n> of what I suggested in the above may apply.\n\nI think you have understood the problem. At present But doing the\nfilemode trustability check on .git/config assumes it is a regular\nfile anyway if it is to work at all. My suggestion in the patch only\nenforces that assumption explicitly.\n"},{"id":"419905","messageId":"xmqq7dlz94by.fsf@gitster.g","threadId":"55361","inReplyTo":"20210322.081043.1437207928602570397.enometh@meer.net","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-22T04:53:37Z","receivedAt":"2021-03-22T04:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Madhu writes:\n\n>> I see that in a later part of the same function, we test if the\n>> filesystem supports symbolic links but do so only when we are\n>> running \"git init\" afresh. Perhaps the filemode trustability check\n>> and the config-set to record core.filemode should all be moved there\n>> inside the \"if (!reinit)\" block.\n>>\n>> All of the above assumes that the problem being solved is about what\n>> happens when \"git init\" is run in an already functioning working\n>> tree. If I misread what problem you are trying to solve, then none\n>> of what I suggested in the above may apply.\n>\n> I think you have understood the problem. At present But doing the\n> filemode trustability check on .git/config assumes it is a regular\n> file anyway if it is to work at all. My suggestion in the patch only\n> enforces that assumption explicitly.\n\nThere are two points we should consider.\n\n * Historically, we've used .git/config as the sample file to check,\n but that was not because we wanted to make sure we can chmod the\n config file, but because we knew the file has to be there. If\n .git/config is sometimes a symbolic link, and if chmod test\n requires a regular file, we do not have to use .git/config as the\n sample file. We could instead switch to use a different,\n temporary, file. After all, the symlink check I pointed out in\n the message you are responding to uses a brand new temporary\n filename for that, and there is no reason why we shouldn't do the\n same with a regular file for the filemode test.\n\n * If running \"git init\" in an already functioning repository can be\n a useful way to \"re-initialize\" and/or \"correct\" various aspect\n of the repository (e.g. perhaps core.filemode is incorrectly set\n originally and running \"git init\" again corrects it), we would\n want to allow that in a normal repository as well as in a\n repository that is created by new-workdir the same way. Or if it\n is not useful and we want \"re-initialize\" not to touch the\n filemode, we would want to skip the check in a normal repository\n as well as in a new-workdir repository the same way. That is why\n \"if symlink, then skip\" is wrong---it targets the new-workdir\n case specifically.\n\nI personally do not have a strong opinion either way, but to me, it\nseems that \"does the filesystem support filemode?\" and \"does the\nfilesystem support symbolic link?\" are at about the same level and\nshould be treated similarly unless there is a good reason not to.\nAnd the symlink check is never done in \"reinit\" case, so perhaps\nwhen \"git init\" is run again in an already functioning repository,\nwe should not muck with the filemode, either.\n\nA natural conclusion of the line of thought is that we can move the\n\"check filemode trustability\" block (from the comment to concluding\ngit_config_set()) inside the \"if (!reinit)\" that happens a bit later\nand we'd be fine---as an existing normal repository, as well as what\nnew-workdir creates, won't have to do the \"let's chmod +x/-x the\nconfig file and see what happens\" code at all (perhaps the attached\npatch, which hasn't even been compile tested).\n\nOn the other hand, if it is worth \"fixing\" the filemode setting\nwhile re-initializing, we probably should switch to use a temporary\nfile instead of 'config'. And we may want to reconsider the placement\nof the \"is symlink supported?\" check---which may also have to be\nredone to \"fix\" its existing value.\n\n\n builtin/init-db.c | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git c/builtin/init-db.c w/builtin/init-db.c\nindex dcc45bef51..61817a02a8 100644\n--- c/builtin/init-db.c\n+++ w/builtin/init-db.c\n@@ -282,20 +282,6 @@ static int create_default_files(const char *template_path,\n \n \tinitialize_repository_version(fmt->hash_algo, 0);\n \n-\t/* Check filemode trustability */\n-\tpath = git_path_buf(&buf, \"config\");\n-\tfilemode = TEST_FILEMODE;\n-\tif (TEST_FILEMODE && !lstat(path, &st1)) {\n-\t\tstruct stat st2;\n-\t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n-\t\t\t\t!lstat(path, &st2) &&\n-\t\t\t\tst1.st_mode != st2.st_mode &&\n-\t\t\t\t!chmod(path, st1.st_mode));\n-\t\tif (filemode && !reinit && (st1.st_mode & S_IXUSR))\n-\t\t\tfilemode = 0;\n-\t}\n-\tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n-\n \tif (is_bare_repository())\n \t\tgit_config_set(\"core.bare\", \"true\");\n \telse {\n@@ -309,6 +295,20 @@ static int create_default_files(const char *template_path,\n \t}\n \n \tif (!reinit) {\n+\t\t/* Check filemode trustability */\n+\t\tpath = git_path_buf(&buf, \"config\");\n+\t\tfilemode = TEST_FILEMODE;\n+\t\tif (TEST_FILEMODE && !lstat(path, &st1)) {\n+\t\t\tstruct stat st2;\n+\t\t\tfilemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&\n+\t\t\t\t\t!lstat(path, &st2) &&\n+\t\t\t\t\tst1.st_mode != st2.st_mode &&\n+\t\t\t\t\t!chmod(path, st1.st_mode));\n+\t\t\tif (filemode && !reinit && (st1.st_mode & S_IXUSR))\n+\t\t\t\tfilemode = 0;\n+\t\t}\n+\t\tgit_config_set(\"core.filemode\", filemode ? \"true\" : \"false\");\n+\n \t\t/* Check if symlink is supported in the work tree */\n \t\tpath = git_path_buf(&buf, \"tXXXXXX\");\n \t\tif (!close(xmkstemp(path)) &&\n"},{"id":"419915","messageId":"20210322.143437.212295420302618690.enometh@meer.net","threadId":"55361","inReplyTo":"xmqq7dlz94by.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Madhu","fromEmail":"enometh@meer.net","sentAt":"2021-03-22T09:04:37Z","receivedAt":"2021-03-22T09:06:03Z","isPatch":true,"sender":{"key":"enometh@meer.net","avatar":null},"body":"* Junio C Hamano \nWrote on Sun, 21 Mar 2021 21:53:37 -0700\n> There are two points we should consider.\n>\n> * Historically, we've used .git/config as the sample file to check,\n> but that was not because we wanted to make sure we can chmod the\n> config file, but because we knew the file has to be there. If\n> .git/config is sometimes a symbolic link, and if chmod test\n> requires a regular file, we do not have to use .git/config as the\n> sample file. We could instead switch to use a different,\n> temporary, file. After all, the symlink check I pointed out in\n> the message you are responding to uses a brand new temporary\n> filename for that, and there is no reason why we shouldn't do the\n> same with a regular file for the filemode test.\n>\n> * If running \"git init\" in an already functioning repository can be\n> a useful way to \"re-initialize\" and/or \"correct\" various aspect\n> of the repository (e.g. perhaps core.filemode is incorrectly set\n> originally and running \"git init\" again corrects it), we would\n> want to allow that in a normal repository as well as in a\n> repository that is created by new-workdir the same way. Or if it\n> is not useful and we want \"re-initialize\" not to touch the\n> filemode, we would want to skip the check in a normal repository\n> as well as in a new-workdir repository the same way. That is why\n> \"if symlink, then skip\" is wrong---it targets the new-workdir\n> case specifically.\n\nI'd say it doesn't (target the new-workdir case specifically). The\nlstat test on `config' is incorrect if `config' (or a new file) is not\na regular file, so I think it should be fixed anyway. The new-workdir\ncase happens to be handled correctly once this is fixed.\n\n> I personally do not have a strong opinion either way, but to me, it\n> seems that \"does the filesystem support filemode?\" and \"does the\n> filesystem support symbolic link?\" are at about the same level and\n> should be treated similarly unless there is a good reason not to.\n> And the symlink check is never done in \"reinit\" case, so perhaps\n> when \"git init\" is run again in an already functioning repository,\n> we should not muck with the filemode, either.\n\nI'd think so (on the last point). While it is understandable to\nexpect consistent re-initing behaviour (which is why the spurious\ngit-init was there) the expectations may not hold if the git directory\nand work tree are on different filesystems - there is more scope for\nmaking wrong inferences.\n\nTypically checkout worktrees with new-workdir in /dev/shm for\nadvantages in the low overhead.\n\nI do have repositories where .git/config is a symlink to a config.A\nand I have other config.B files - all for the same repo but with\ndifferent upstreams. (I know better ways to do it but why should I be\nprevented from doing this)\n\n> A natural conclusion of the line of thought is that we can move the\n> \"check filemode trustability\" block (from the comment to concluding\n> git_config_set()) inside the \"if (!reinit)\" that happens a bit later\n> and we'd be fine---as an existing normal repository, as well as what\n> new-workdir creates, won't have to do the \"let's chmod +x/-x the\n> config file and see what happens\" code at all (perhaps the attached\n> patch, which hasn't even been compile tested).\n>\n> On the other hand, if it is worth \"fixing\" the filemode setting\n> while re-initializing, we probably should switch to use a temporary\n> file instead of 'config'. And we may want to reconsider the placement\n> of the \"is symlink supported?\" check---which may also have to be\n> redone to \"fix\" its existing value.\n\nI don't think the posted patch (snipped) would work as reinit is\nalways 1 and we are always a candidate for reiniting - I may be\nmissing something.\n\nUsing a new file for the filemode test would be a natural\nimprovement. But I dont see the added complexity as a win. I can\nimagine other scenarios that need to be fixed: calling git-init on a\nworktree prepared by git-new-worktree (not git-new-workdir) on a FAT\nfs with .git on ext2. Calling git-init on a worktree prepared by\ncalling git-new-worktree with a parent which is checked out by\ngit-new-workdir. (The parent has a symlinked config). The\npossibilities for fun endless :)\n\n\n"},{"id":"419963","messageId":"xmqqr1k76p8d.fsf@gitster.g","threadId":"55361","inReplyTo":"20210322.143437.212295420302618690.enometh@meer.net","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-22T18:02:42Z","receivedAt":"2021-03-22T18:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Madhu writes:\n\n> * Junio C Hamano \n>> ...\n>> And the symlink check is never done in \"reinit\" case, so perhaps\n>> when \"git init\" is run again in an already functioning repository,\n>> we should not muck with the filemode, either.\n>\n> I'd think so (on the last point)....\n\nSo, assuming that you are with me to think that reinit should not\ntouch the filemode thing, ...\n\n>> A natural conclusion of the line of thought is that we can move the\n>> \"check filemode trustability\" block (from the comment to concluding\n>> git_config_set()) inside the \"if (!reinit)\" that happens a bit later\n>> and we'd be fine---as an existing normal repository, as well as what\n>> new-workdir creates, won't have to do the \"let's chmod +x/-x the\n>> config file and see what happens\" code at all (perhaps the attached\n>> patch, which hasn't even been compile tested).\n>> ...\n\n... wouldn't the illustration patch I gave, which removed the \"check\nfilemode\" bit from the main codepath and moved it to inside an if\nblock that is executed only when \"if (!reinit)\" is true, \"skip\" the\nproblematic \"check if config is a regular file whose executable bit\ncan be flipped and flopped\" code in your use case, i.e. in an existing\nrepository?\n\n> I don't think the posted patch (snipped) would work as reinit is\n> always 1 and we are always a candidate for reiniting - I may be\n> missing something.\n\nIn other words, yes, the illustration patch you are responding to\nassumes that the \"reinit\" variable is set correctly (i.e. the HEAD\nexists and sensibly readable if you run \"git init\" in an already\nfunctioning working tree) and we can use it to avoid the filemode\ncheck.\n\n> Using a new file for the filemode test would be a natural\n> improvement. \n\nThat becomes necessary only if we want to futz with core.filemode\nwhile doing \"reinit\", as .git/config can be a symlink. When we are\ncreating a repository from scratch, we always create a regular file\nto prepare .git/config, and there is no need to do that, if we are\nhappy to set core.filemode the same way as core.symlinks, i.e. only\ncheck once when the repository is created. No?\n\nThanks.\n\n\n"},{"id":"419982","messageId":"20210323.092748.1559327071188512317.enometh@meer.net","threadId":"55361","inReplyTo":"xmqqr1k76p8d.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Madhu","fromEmail":"enometh@meer.net","sentAt":"2021-03-23T03:57:48Z","receivedAt":"2021-03-23T04:02:31Z","isPatch":true,"sender":{"key":"enometh@meer.net","avatar":null},"body":"* Junio C Hamano \nWrote on Mon, 22 Mar 2021 11:02:42 -0700\n>> I don't think the posted patch (snipped) would work as reinit is\n>> always 1 and we are always a candidate for reiniting - I may be\n>> missing something.\n>\n> In other words, yes, the illustration patch you are responding to\n> assumes that the \"reinit\" variable is set correctly (i.e. the HEAD\n> exists and sensibly readable if you run \"git init\" in an already\n> functioning working tree) and we can use it to avoid the filemode\n> check.\n\nAh yes indeed.\n\n>> Using a new file for the filemode test would be a natural\n>> improvement.\n>\n> That becomes necessary only if we want to futz with core.filemode\n> while doing \"reinit\", as .git/config can be a symlink. When we are\n> creating a repository from scratch, we always create a regular file\n> to prepare .git/config, and there is no need to do that, if we are\n> happy to set core.filemode the same way as core.symlinks, i.e. only\n> check once when the repository is created. No?\n\nAvoiding the filemode check completely during reinit is ok with me\nbecause it gave me wrong results. I can't speak for the original\nauthor of the code - if his intention was to do it explicitly as part\nof \"reinitialization\".\n\n[In the latter case personally I'd want to edit the config file by hand\nin response to a message from git - and I certainly do not want git to\nchange the config file behind my back without informing me.]\n\nThanks again.\n\n\n\n"},{"id":"419991","messageId":"xmqqr1k64bmk.fsf@gitster.g","threadId":"55361","inReplyTo":"20210323.092748.1559327071188512317.enometh@meer.net","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T06:39:31Z","receivedAt":"2021-03-23T06:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Madhu writes:\n\n> Avoiding the filemode check completely during reinit is ok with me\n> because it gave me wrong results. I can't speak for the original\n> author of the code - if his intention was to do it explicitly as part\n> of \"reinitialization\".\n\nAs the original author of the code, I know I meant filemode check to\nbe done and redone upon reinitialization in 4f629539 (init-db: check\ntemplate and repository format., 2005-11-25).\n\nBut then when 75d24499 (git-init: autodetect core.symlinks,\n2007-08-31) started to autodetect symbolic link support, I somehow\nended up doing it only upon the repository creation. Later,\n2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated\nto check case sensitivity in the same block, doing it only once.\n\nEither of these two commits would have been a good chance for us to\nrealize that filemode check should be done the same way, but somehow\nnobody noticed X-<.\n\n"},{"id":"420047","messageId":"20210323165335.urvvccwnhahxmokt@tb-raspi4","threadId":"55361","inReplyTo":"xmqqr1k64bmk.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-03-23T16:53:35Z","receivedAt":"2021-03-23T16:54:42Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Mar 22, 2021 at 11:39:31PM -0700, Junio C Hamano wrote:\n> Madhu writes:\n>\n> > Avoiding the filemode check completely during reinit is ok with me\n> > because it gave me wrong results. I can't speak for the original\n> > author of the code - if his intention was to do it explicitly as part\n> > of \"reinitialization\".\n>\n> As the original author of the code, I know I meant filemode check to\n> be done and redone upon reinitialization in 4f629539 (init-db: check\n> template and repository format., 2005-11-25).\n>\n> But then when 75d24499 (git-init: autodetect core.symlinks,\n> 2007-08-31) started to autodetect symbolic link support, I somehow\n> ended up doing it only upon the repository creation. Later,\n> 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated\n> to check case sensitivity in the same block, doing it only once.\n>\n> Either of these two commits would have been a good chance for us to\n> realize that filemode check should be done the same way, but somehow\n> nobody noticed X-<.\n>\n\nIn the very long run, there may be room for improvements:\nWhile core.filemode works for a loal repo on a local disk,\nthere are lots of cases where I whish a better handling.\n\nExporting a git repo from e.g.\nLinux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on\nLinux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off\n\nVisiting the same repo under Git-for-Windows and cygwin:\ncygwin supports the executable bit, Git-for-Windows does not.\n\nAnd now we have the worktree (which may cross filesytem borders)\n\nToday there are many use cases, where a single config variable is not ideal.\n\nIf there is a chance to have a \"core.filemode=auto\", which does probe the\nfilemode for this very OS/filesytem/worktree combination:\nI would be happy to test/review/mentor such a code change.\n"},{"id":"420053","messageId":"xmqqo8f93gsd.fsf@gitster.g","threadId":"55361","inReplyTo":"20210323165335.urvvccwnhahxmokt@tb-raspi4","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T17:45:38Z","receivedAt":"2021-03-23T17:46:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> In the very long run, there may be room for improvements:\n> While core.filemode works for a loal repo on a local disk,\n> there are lots of cases where I whish a better handling.\n\nBut that is why we have multiple worktrees and per worktree\nconfiguration, no?\n\nFundamentally, you cannot share a worktree from two systems and let\none work with filemode on while the other with filemode off, as you\nsummarized here:\n\n> Exporting a git repo from e.g.\n> Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on\n> Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off\n>\n> Visiting the same repo under Git-for-Windows and cygwin:\n> cygwin supports the executable bit, Git-for-Windows does not.\n\nThe \"new-workdirs\" was a cute hack that was quite useful before the\nmultiple worktree support materialized, and it and (proper) worktree\nshare the quirk that by default the per-repo configuration file is\nshared.\n\n> And now we have the worktree (which may cross filesytem borders)\n>\n> Today there are many use cases, where a single config variable is not ideal.\n>\n> If there is a chance to have a \"core.filemode=auto\", which does probe the\n> filemode for this very OS/filesytem/worktree combination:\n> I would be happy to test/review/mentor such a code change.\n\nI do not think we want to go there. filemode is not the only thing\nthat would be shared. What do you want to do with core.eol=native,\nfor example? Paths touched while switching branches get the 'native'\nline endings on the system that the user happened to be on when the\n\"switch\" command was run, and working tree files end up with mixture?\n"},{"id":"420061","messageId":"20210323203124.oxzqad2wmedelstu@tb-raspi4","threadId":"55361","inReplyTo":"xmqqo8f93gsd.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2021-03-23T20:31:24Z","receivedAt":"2021-03-23T20:32:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Mar 23, 2021 at 10:45:38AM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n> > In the very long run, there may be room for improvements:\n> > While core.filemode works for a loal repo on a local disk,\n> > there are lots of cases where I whish a better handling.\n>\n> But that is why we have multiple worktrees and per worktree\n> configuration, no?\n>\n> Fundamentally, you cannot share a worktree from two systems and let\n> one work with filemode on while the other with filemode off, as you\n> summarized here:\n>\n> > Exporting a git repo from e.g.\n> > Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on\n> > Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off\n> >\n> > Visiting the same repo under Git-for-Windows and cygwin:\n> > cygwin supports the executable bit, Git-for-Windows does not.\n>\n> The \"new-workdirs\" was a cute hack that was quite useful before the\n> multiple worktree support materialized, and it and (proper) worktree\n> share the quirk that by default the per-repo configuration file is\n> shared.\n>\n> > And now we have the worktree (which may cross filesytem borders)\n> >\n> > Today there are many use cases, where a single config variable is not ideal.\n> >\n> > If there is a chance to have a \"core.filemode=auto\", which does probe the\n> > filemode for this very OS/filesytem/worktree combination:\n> > I would be happy to test/review/mentor such a code change.\n>\n> I do not think we want to go there. filemode is not the only thing\n> that would be shared. What do you want to do with core.eol=native,\n> for example? Paths touched while switching branches get the 'native'\n> line endings on the system that the user happened to be on when the\n> \"switch\" command was run, and working tree files end up with mixture?\n\nI don't intend to solve all possible confusions caused by sharing all\nconfig variables - just this very one.\n\nAfter some thinking, the following may illustrate a brute-force method,\nnot nice, neither optimized, nor perfect or bug free:\n\n$ alias\nalias git='~/bin2/git.sh'\n\ncat ~/bin2/git.sh\n\n#!/bin/sh\n\nprobe_executable()\n{\n touch $$.xxx\n if test -x $$.xxx; then\n rm $$.xxx\n echo \"-c core.filemode=false\"\n return\n fi\n chmod +x $$.xxx\n if ! test -x $$.xxx; then\n rm $$.xxx\n echo \"-c core.filemode=false\"\n return\n fi\n rm $$.xxx\n return\n}\ngit $(probe_executable) \"$@\"\n"},{"id":"420067","messageId":"xmqqr1k51tnd.fsf@gitster.g","threadId":"55361","inReplyTo":"20210323203124.oxzqad2wmedelstu@tb-raspi4","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T20:50:46Z","receivedAt":"2021-03-23T20:51:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n>> I do not think we want to go there. filemode is not the only thing\n>> that would be shared. What do you want to do with core.eol=native,\n>> for example? Paths touched while switching branches get the 'native'\n>> line endings on the system that the user happened to be on when the\n>> \"switch\" command was run, and working tree files end up with mixture?\n>\n> I don't intend to solve all possible confusions caused by sharing all\n> config variables - just this very one.\n\nI intend to help users by drawing a clear red line and telling them\nthat crossing that line leads them to danger.\n\nSharing a working tree across systems that require different core.*\nsettings (filemode included) is on the other side of that line, and\nthat is the reason why I said I do not think we want to go there.\nBy saying \"this is only about core.filemode but you can share a\nworking tree between incompatible systems\", I am afraid that we end\nup training users to go where they should not get nearby.\n\n"},{"id":"427808","messageId":"20210618.094806.1369340605245207462.enometh@meer.net","threadId":"55361","inReplyTo":"xmqqr1k64bmk.fsf@gitster.g","subject":"Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.","fromName":"Madhu","fromEmail":"enometh@meer.net","sentAt":"2021-06-18T04:18:06Z","receivedAt":"2021-06-18T04:25:37Z","isPatch":true,"sender":{"key":"enometh@meer.net","avatar":null},"body":"I don't think this got any further attention? I notice git-2.32 still\nhas the problem - in which case I re-commend my original patch.\nRegards ---Madhu\n\n* Junio C Hamano \nWrote on Mon, 22 Mar 2021 23:39:31 -0700\n\n> Madhu writes:\n>> Avoiding the filemode check completely during reinit is ok with me\n>> because it gave me wrong results. I can't speak for the original\n>> author of the code - if his intention was to do it explicitly as part\n>> of \"reinitialization\".\n>\n> As the original author of the code, I know I meant filemode check to\n> be done and redone upon reinitialization in 4f629539 (init-db: check\n> template and repository format., 2005-11-25).\n>\n> But then when 75d24499 (git-init: autodetect core.symlinks,\n> 2007-08-31) started to autodetect symbolic link support, I somehow\n> ended up doing it only upon the repository creation. Later,\n> 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated\n> to check case sensitivity in the same block, doing it only once.\n>\n> Either of these two commits would have been a good chance for us to\n> realize that filemode check should be done the same way, but somehow\n> nobody noticed X-<.\n>\n"}]}