{"thread":{"id":"62629","subject":"chmod failure on GVFS mounted CIFS share","startedAt":"2024-12-12T09:14:58Z","lastAt":"2025-01-10T12:05:12Z","messageCount":8,"participants":["Konrad Bucheli (PSI)","brian m. carlson","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"509019","messageId":"d26f4b93-57a3-4536-8c32-3ed5b3e98a86@psi.ch","threadId":"62629","inReplyTo":null,"subject":"chmod failure on GVFS mounted CIFS share","fromName":"Konrad Bucheli (PSI)","fromEmail":"konrad.bucheli@psi.ch","sentAt":"2024-12-12T09:14:50Z","receivedAt":"2024-12-12T09:14:58Z","isPatch":false,"sender":{"key":"konrad.bucheli@psi.ch","avatar":null},"body":"Dear git developers\n\nBelow my bug report:\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nI do a `git init .` on a directory which is CIFS mounted via gio mount \n(FUSE).\nOn RHEL8 this needs gvfs-smb and gvfs-fuse installed.\nMount command: gio mount smb://fs01.psi.ch/my_user_name$\n\nWhat did you expect to happen? (Expected behavior)\n\nIt initializes the git repo.\n\nWhat happened instead? (Actual behavior)\n\n$ git init .\nerror: chmod on \n/run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock \nfailed: Operation not supported\nfatal: could not set 'core.filemode' to 'false'\n$\n\nWhat's different between what you expected and what actually happened?\n\nI expect that it fully initializes the repository:\n\n[buchel_k@lxdev07 foo]$ git status\nfatal: not a git repository (or any parent up to mount point \n/run/user/44951)\nStopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\n$ find .\n.\n./.git\n./.git/branches\n./.git/hooks\n./.git/hooks/applypatch-msg.sample\n./.git/hooks/commit-msg.sample\n./.git/hooks/post-update.sample\n./.git/hooks/pre-applypatch.sample\n./.git/hooks/pre-commit.sample\n./.git/hooks/pre-merge-commit.sample\n./.git/hooks/pre-push.sample\n./.git/hooks/pre-receive.sample\n./.git/hooks/push-to-checkout.sample\n./.git/hooks/sendemail-validate.sample\n./.git/hooks/update.sample\n./.git/hooks/fsmonitor-watchman.sample\n./.git/hooks/pre-rebase.sample\n./.git/hooks/prepare-commit-msg.sample\n./.git/info\n./.git/info/exclude\n./.git/description\n./.git/refs\n./.git/refs/heads\n./.git/refs/tags\n./.git/HEAD\n./.git/config\n./git-bugreport-2024-12-12-0951.txt\n$\n\n\nAnything else you want to add:\n\nI guess the failure happens at config.c:3339 \n(https://github.com/git/git/blob/master/config.c#L3339).\nWhat would it mean if the chmod there ignores \"Operation not supported\"?\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.43.5\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 4.18.0-553.30.1.el8_10.x86_64 #1 SMP Fri Nov 15 03:46:25 \nEST 2024 x86_64\ncompiler info: gnuc: 8.5\nlibc info: glibc: 2.28\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n\n\nThank you very much for your good work.\n\nKind regards\n\nKonrad\n\n-- \nPaul Scherrer Institut\nKonrad Bucheli\nLinux Systems Engineer\nCore Linux Research Services\nScience IT Infrastructure and Services department (AWI)\nOBBA/230\nForschungstrasse 111\n5232 Villigen PSI\nSwitzerland\n\nPhone: +41 56 310 27 24\nkonrad.bucheli@psi.ch\nwww.psi.ch\n\n"},{"id":"509040","messageId":"Z1us2FpyObVpJlsM@tapette.crustytoothpaste.net","threadId":"62629","inReplyTo":"d26f4b93-57a3-4536-8c32-3ed5b3e98a86@psi.ch","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-12-13T03:41:12Z","receivedAt":"2024-12-13T03:41:22Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-12-12 at 09:14:50, Konrad Bucheli (PSI) wrote:\n> Dear git developers\n> \n> Below my bug report:\n> \n> What did you do before the bug happened? (Steps to reproduce your issue)\n> \n> I do a `git init .` on a directory which is CIFS mounted via gio mount\n> (FUSE).\n> On RHEL8 this needs gvfs-smb and gvfs-fuse installed.\n> Mount command: gio mount smb://fs01.psi.ch/my_user_name$\n\nIn general, gvfs's FUSE driver isn't a good way to interact with files.\nMy experience with its SFTP driver is that it has a bunch of weird,\nnon-Unixy behaviour that's due to limitations in the gio interface.  So\nit probably doesn't provide the functionality most Unix programs will\nexpect from a file system, which will cause you a world of problems down\nthe line, as you've seen here.\n\n> What did you expect to happen? (Expected behavior)\n> \n> It initializes the git repo.\n> \n> What happened instead? (Actual behavior)\n> \n> $ git init .\n> error: chmod on /run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock\n> failed: Operation not supported\n> fatal: could not set 'core.filemode' to 'false'\n> $\n\nThis is indeed the case, since the chmod on the config file fails.  We\nalways rewrite the config file as a separate lock file, and then rename\ninto place.  (This prevents concurrent modification correctly even on\nnetwork file systems.)  The user generally wants the permissions to be\npreserved, so this is the safe default.\n\nThis also happens when using a Linux Git on a WSL Windows mount.\n\nNote that even Windows honours the read-only/read-write difference for\nfiles, so in theory chmod is useful even on CIFS and other Windows file\nsystems.\n\nIf I remember correctly, the consensus last time this came up was that\nsomeone is welcome to add a config option that ignores the chmod\nfailure, but that in general, we don't want to just silently ignore it.\nUnfortunately, nobody has added such a configuration option yet.\n\nI will note that the `mount.cifs(8)` manual page[0] indicates that the\nLinux kernel CIFS driver _does_ silently ignore chmod operations when\nthe server doesn't support them, but it also supports using and storing\nUnix permissions if the server does, so that may be a workaround.\n\n[0] https://linux.die.net/man/8/mount.cifs\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"509067","messageId":"c22a7d5e-cbfe-48a9-bdf5-e73f21ac648f@psi.ch","threadId":"62629","inReplyTo":"Z1us2FpyObVpJlsM@tapette.crustytoothpaste.net","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"Konrad Bucheli (PSI)","fromEmail":"konrad.bucheli@psi.ch","sentAt":"2024-12-13T10:32:22Z","receivedAt":"2024-12-13T10:32:32Z","isPatch":false,"sender":{"key":"konrad.bucheli@psi.ch","avatar":null},"body":"On 13.12.24 04:41, brian m. carlson wrote:\n> On 2024-12-12 at 09:14:50, Konrad Bucheli (PSI) wrote:\n>> Dear git developers\n>>\n>> Below my bug report:\n>>\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>>\n>> I do a `git init .` on a directory which is CIFS mounted via gio mount\n>> (FUSE).\n>> On RHEL8 this needs gvfs-smb and gvfs-fuse installed.\n>> Mount command: gio mount smb://fs01.psi.ch/my_user_name$\n> \n> In general, gvfs's FUSE driver isn't a good way to interact with files.\n> My experience with its SFTP driver is that it has a bunch of weird,\n> non-Unixy behaviour that's due to limitations in the gio interface.  So\n> it probably doesn't provide the functionality most Unix programs will\n> expect from a file system, which will cause you a world of problems down\n> the line, as you've seen here.\n> \nTrue, but it has a big advantage: you can mount it without being root.\n>> What did you expect to happen? (Expected behavior)\n>>\n>> It initializes the git repo.\n>>\n>> What happened instead? (Actual behavior)\n>>\n>> $ git init .\n>> error: chmod on /run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock\n>> failed: Operation not supported\n>> fatal: could not set 'core.filemode' to 'false'\n>> $\n> \n> This is indeed the case, since the chmod on the config file fails.  We\n> always rewrite the config file as a separate lock file, and then rename\n> into place.  (This prevents concurrent modification correctly even on\n> network file systems.)  The user generally wants the permissions to be\n> preserved, so this is the safe default.\n> \n> This also happens when using a Linux Git on a WSL Windows mount.\n> \n> Note that even Windows honours the read-only/read-write difference for\n> files, so in theory chmod is useful even on CIFS and other Windows file\n> systems.\n> \n> If I remember correctly, the consensus last time this came up was that\n> someone is welcome to add a config option that ignores the chmod\n> failure, but that in general, we don't want to just silently ignore it.\n> Unfortunately, nobody has added such a configuration option yet.\nMy suggestion is not to silently ignore all chmod errors, only ENOTSUP \nfor config.lock which basically tells that it is not a suitable location \nfor that operation and thus also not required.\nWould that be acceptable?\n\n\n\n\n"},{"id":"509137","messageId":"Z19Ymnnvt9jOixPB@tapette.crustytoothpaste.net","threadId":"62629","inReplyTo":"c22a7d5e-cbfe-48a9-bdf5-e73f21ac648f@psi.ch","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-12-15T22:30:50Z","receivedAt":"2024-12-15T22:39:37Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-12-13 at 10:32:22, Konrad Bucheli (PSI) wrote:\n> My suggestion is not to silently ignore all chmod errors, only ENOTSUP for\n> config.lock which basically tells that it is not a suitable location for\n> that operation and thus also not required.\n> Would that be acceptable?\n\nThat solves your particular problem, but the WSL situation gets\nEPERM[0], so it would not solve the general problem.  We definitely\ndon't want to blindly ignore EPERM in general for `config.lock` because\nit could indicate a real permissions problem or a race condition with\nother software.\n\n[0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"509392","messageId":"c287fbd7-eb08-45f1-953b-5afd4fe41f9f@psi.ch","threadId":"62629","inReplyTo":"Z19Ymnnvt9jOixPB@tapette.crustytoothpaste.net","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"Konrad Bucheli (PSI)","fromEmail":"konrad.bucheli@psi.ch","sentAt":"2024-12-20T14:42:36Z","receivedAt":"2024-12-20T14:42:43Z","isPatch":false,"sender":{"key":"konrad.bucheli@psi.ch","avatar":null},"body":"I have another idea: there is no need for a chmod if both the config \nfile and the lock file already have he same mode. Which is the case if \nthe filesystem has no proper chmod support.\n\nAttached patch implements this and I could successfully initialize a new \ngit repo on a CIFS share accessed over FUSE.\n\nWhat do you think?\n\nOn 15.12.24 23:30, brian m. carlson wrote:\n> On 2024-12-13 at 10:32:22, Konrad Bucheli (PSI) wrote:\n>> My suggestion is not to silently ignore all chmod errors, only ENOTSUP for\n>> config.lock which basically tells that it is not a suitable location for\n>> that operation and thus also not required.\n>> Would that be acceptable?\n> \n> That solves your particular problem, but the WSL situation gets\n> EPERM[0], so it would not solve the general problem.  We definitely\n> don't want to blindly ignore EPERM in general for `config.lock` because\n> it could indicate a real permissions problem or a race condition with\n> other software.\n> \n> [0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository\n\n\ndiff --git a/config.c b/config.c\nindex a11bb85da3..3466f2ca4c 100644\n--- a/config.c\n+++ b/config.c\n@@ -3256,7 +3256,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n \t\t    write_pair(fd, key, value, comment, &store) < 0)\n \t\t\tgoto write_err_out;\n \t} else {\n-\t\tstruct stat st;\n+\t\tstruct stat st, st_lock;\n \t\tsize_t copy_begin, copy_end;\n \t\tint i, new_line = 0;\n \t\tstruct config_options opts;\n@@ -3336,12 +3336,20 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,\n \t\tclose(in_fd);\n \t\tin_fd = -1;\n \n-\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n-\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n+\t\tif (stat(get_lock_file_path(&lock), &st_lock) == -1) {\n+\t\t\terror_errno(_(\"stat on %s failed\"), get_lock_file_path(&lock));\n \t\t\tret = CONFIG_NO_WRITE;\n \t\t\tgoto out_free;\n \t\t}\n \n+\t\tif ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {\n+\t\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n+\t\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n+\t\t\t\tret = CONFIG_NO_WRITE;\n+\t\t\t\tgoto out_free;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (store.seen_nr == 0) {\n \t\t\tif (!store.seen_alloc) {\n \t\t\t\t/* Did not see key nor section */\n"},{"id":"509397","messageId":"xmqq8qsafjfy.fsf@gitster.g","threadId":"62629","inReplyTo":"c287fbd7-eb08-45f1-953b-5afd4fe41f9f@psi.ch","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-20T15:44:17Z","receivedAt":"2024-12-20T15:44:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Konrad Bucheli (PSI)\" <konrad.bucheli@psi.ch> writes:\n\n> I have another idea: there is no need for a chmod if both the config\n> file and the lock file already have he same mode. Which is the case if\n> the filesystem has no proper chmod support.\n\nAnd for majority of people who have working chmod(), would it mean\none extra and unnecessary system call?\n\nInstead, how about doing the chmod() first, like we have always done,\nbut after seeing it fail, check with lstat() to see if the modes are\nalready in the desired state and refrain from complaining if that is\nthe case?  That way, we'll incur extra overhead only in the error\ncode path, which is the usual pattern we would prefer to do things.\n\nSo instead of removing this part, ...\n\n> -\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n> -\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n\n... you'd do an extra lstat() on the lock file (so you can move the\nst_lock inside this block, narrowing its scope) before calling\nerror_errno(), and only after finding out that st_lock cannot\nsomehow be obtained or the resulting mode is different, you call\nerror_errno() and arrange an error to be returned, all inside the\nif(){} block of the original.\n\nWouldn't it work even better, I wonder?\n\nThanks.\n\n> +\t\tif (stat(get_lock_file_path(&lock), &st_lock) == -1) {\n> +\t\t\terror_errno(_(\"stat on %s failed\"), get_lock_file_path(&lock));\n>  \t\t\tret = CONFIG_NO_WRITE;\n>  \t\t\tgoto out_free;\n>  \t\t}\n>  \n> +\t\tif ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {\n> +\t\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n> +\t\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n> +\t\t\t\tret = CONFIG_NO_WRITE;\n> +\t\t\t\tgoto out_free;\n> +\t\t\t}\n> +\t\t}\n> +\n>  \t\tif (store.seen_nr == 0) {\n>  \t\t\tif (!store.seen_alloc) {\n>  \t\t\t\t/* Did not see key nor section */\n\n"},{"id":"509399","messageId":"8af7ff11-c5bb-464b-894f-f1fb30df50e0@psi.ch","threadId":"62629","inReplyTo":"xmqq8qsafjfy.fsf@gitster.g","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"Konrad Bucheli (PSI)","fromEmail":"konrad.bucheli@psi.ch","sentAt":"2024-12-20T15:50:35Z","receivedAt":"2024-12-20T15:50:45Z","isPatch":false,"sender":{"key":"konrad.bucheli@psi.ch","avatar":null},"body":"\n\nOn 20.12.24 16:44, Junio C Hamano wrote:\n> \"Konrad Bucheli (PSI)\" <konrad.bucheli@psi.ch> writes:\n> \n>> I have another idea: there is no need for a chmod if both the config\n>> file and the lock file already have he same mode. Which is the case if\n>> the filesystem has no proper chmod support.\n> \n> And for majority of people who have working chmod(), would it mean\n> one extra and unnecessary system call?\n> \n\nI do not have stats, but I guess the chmod call would be needed very \nrarely as most of the time both files have default permissions. I do not \nknow which call is more expensive.\n\n> Instead, how about doing the chmod() first, like we have always done,\n> but after seeing it fail, check with lstat() to see if the modes are\n> already in the desired state and refrain from complaining if that is\n> the case?  That way, we'll incur extra overhead only in the error\n> code path, which is the usual pattern we would prefer to do things.\n> \n> So instead of removing this part, ...\n> \n>> -\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n>> -\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n> \n> ... you'd do an extra lstat() on the lock file (so you can move the\n> st_lock inside this block, narrowing its scope) before calling\n> error_errno(), and only after finding out that st_lock cannot\n> somehow be obtained or the resulting mode is different, you call\n> error_errno() and arrange an error to be returned, all inside the\n> if(){} block of the original.\n> \n> Wouldn't it work even better, I wonder?\n\nIf you think that is the way to go, I will adapt the patch.\n\n> \n> Thanks.\n> \n>> +\t\tif (stat(get_lock_file_path(&lock), &st_lock) == -1) {\n>> +\t\t\terror_errno(_(\"stat on %s failed\"), get_lock_file_path(&lock));\n>>   \t\t\tret = CONFIG_NO_WRITE;\n>>   \t\t\tgoto out_free;\n>>   \t\t}\n>>   \n>> +\t\tif ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {\n>> +\t\t\tif (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n>> +\t\t\t\terror_errno(_(\"chmod on %s failed\"), get_lock_file_path(&lock));\n>> +\t\t\t\tret = CONFIG_NO_WRITE;\n>> +\t\t\t\tgoto out_free;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>>   \t\tif (store.seen_nr == 0) {\n>>   \t\t\tif (!store.seen_alloc) {\n>>   \t\t\t\t/* Did not see key nor section */\n> \n\n"},{"id":"510311","messageId":"bc1a788b-0707-4cc0-8143-dd4d9c70394c@psi.ch","threadId":"62629","inReplyTo":"8af7ff11-c5bb-464b-894f-f1fb30df50e0@psi.ch","subject":"Re: chmod failure on GVFS mounted CIFS share","fromName":"Konrad Bucheli (PSI)","fromEmail":"konrad.bucheli@psi.ch","sentAt":"2025-01-10T12:05:03Z","receivedAt":"2025-01-10T12:05:12Z","isPatch":false,"sender":{"key":"konrad.bucheli@psi.ch","avatar":null},"body":"Hi\n\nI investigated some more git init failures on our NetApp backed CIFS \nstorage, this time not with FUSE but a real kernel mount. I found two \ndifferent situations where a seemingly working \"no-op chmod\" makes git \ninit fail.\n\na) Everyone Full Control permissions on the backend NTFS\n\n$ touch test.txt\n$ ll\ntotal 1\n-------rwx 1 buchel_k root 0 10. Jan 10:28 test.txt\n$ chmod 007 test.txt\n$ ll\ntotal 1\n-------rwx 1 buchel_k root 0 10. Jan 10:28 test.txt\n$ echo foo > test.txt\n-bash: test.txt: Permission denied\n$ rm test.txt\nrm: cannot remove 'test.txt': Permission denied\n$ chmod 600 test.txt\n$ rm test.txt\n$\n\nInvestigation in the backend filesystem showed that this created a \"User \nDeny\" rule which is stronger that the \"Everyone Full Control\" rule.\n\nb) only User Write permissions on backend NTFS\n\nThis second issue is even more wired:\n\n$ touch test.txt\n$ ll\ntotal 1\n-rwx------ 1 buchel_k root 0 10. Jan 11:00 test.txt\n$ # open the file and keep it open\n$ exec {fd}>test.txt\n$ chmod 700 test.txt\n$ # this chmod took multiple seconds\n$ ll\ntotal 0\n-rwx------ 1 buchel_k root 0 10. Jan 11:01 test.txt\n$ # write to file using the already open filehandle\n$ echo test >&$fd\n-bash: echo: write error: Permission denied\n$ # close filehandle\n$ exec {fd}>&-\n$ # repeat without chmod\n$ exec {fd}>test.txt\n$ echo test >&$fd\n$ exec {fd}>&-\n$ cat test.txt\ntest\n$\n\nSo if the chmod to a file happens after is has been already opened, then \nsubsequent write fail. Looks like a bug to me. After reopening, all is fine.\n\nFor the records:\nmount options as listed by mount:\n(rw,relatime,vers=3.1.1,sec=krb5,cruid=0,cache=strict,multiuser,uid=0,noforceuid,gid=0,noforcegid,addr=<REDACTED>,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,cifsacl,noperm,reparse=nfs,rsize=1048576,wsize=1048576,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)\nKernel: 5.14.0-503.19.1.el9_5.x86_64 (RHEL 9.5)\nBackend: NetApp FAS8700 with Release 9.13.1P6\n\n\nMy conclusion is that a no-op chmod is fine on a local, proper UNIX \nfilesystem, but in other setups it can have unexpected side effects or \neven hit bugs. So stat before chmod will make it work in more \ncircumstances and cases. And as git is nowadays a ubiquitous tool, my \nusers will use git wherever they believe it would be useful. I would \nlike to give them a seamless experience as possible.\n\nMy experience might be limited, but I have not seen in my career any \nattempt or use case where permission bits inside .git where adjusted \nafter. And only then there would be minimal price to pay with the extra \nstat, all other save a chmod. And this price is way smaller than having \ngit init/clone fail completely.\n\n\nWhat do you think?\n\nKind regards\n\nKonrad\n\n\nOn 20.12.24 16:50, Konrad Bucheli (PSI) wrote:\n> \n> \n> On 20.12.24 16:44, Junio C Hamano wrote:\n>> \"Konrad Bucheli (PSI)\" <konrad.bucheli@psi.ch> writes:\n>>\n>>> I have another idea: there is no need for a chmod if both the config\n>>> file and the lock file already have he same mode. Which is the case if\n>>> the filesystem has no proper chmod support.\n>>\n>> And for majority of people who have working chmod(), would it mean\n>> one extra and unnecessary system call?\n>>\n> \n> I do not have stats, but I guess the chmod call would be needed very \n> rarely as most of the time both files have default permissions. I do not \n> know which call is more expensive.\n> \n>> Instead, how about doing the chmod() first, like we have always done,\n>> but after seeing it fail, check with lstat() to see if the modes are\n>> already in the desired state and refrain from complaining if that is\n>> the case?  That way, we'll incur extra overhead only in the error\n>> code path, which is the usual pattern we would prefer to do things.\n>>\n>> So instead of removing this part, ...\n>>\n>>> -        if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {\n>>> -            error_errno(_(\"chmod on %s failed\"), \n>>> get_lock_file_path(&lock));\n>>\n>> ... you'd do an extra lstat() on the lock file (so you can move the\n>> st_lock inside this block, narrowing its scope) before calling\n>> error_errno(), and only after finding out that st_lock cannot\n>> somehow be obtained or the resulting mode is different, you call\n>> error_errno() and arrange an error to be returned, all inside the\n>> if(){} block of the original.\n>>\n>> Wouldn't it work even better, I wonder?\n> \n> If you think that is the way to go, I will adapt the patch.\n> \n>>\n>> Thanks.\n>>\n>>> +        if (stat(get_lock_file_path(&lock), &st_lock) == -1) {\n>>> +            error_errno(_(\"stat on %s failed\"), \n>>> get_lock_file_path(&lock));\n>>>               ret = CONFIG_NO_WRITE;\n>>>               goto out_free;\n>>>           }\n>>> +        if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {\n>>> +            if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) \n>>> < 0) {\n>>> +                error_errno(_(\"chmod on %s failed\"), \n>>> get_lock_file_path(&lock));\n>>> +                ret = CONFIG_NO_WRITE;\n>>> +                goto out_free;\n>>> +            }\n>>> +        }\n>>> +\n>>>           if (store.seen_nr == 0) {\n>>>               if (!store.seen_alloc) {\n>>>                   /* Did not see key nor section */\n>>\n> \n\n"}]}