Re: chmod failure on GVFS mounted CIFS share

From: Konrad Bucheli (PSI) <konrad.bucheli@psi.ch>
Date: Dec 20, 2024, 14:42 UTC
Message-ID: <c287fbd7-eb08-45f1-953b-5afd4fe41f9f@psi.ch>
In-Reply-To: <Z19Ymnnvt9jOixPB@tapette.crustytoothpaste.net>

I have another idea: there is no need for a chmod if both the config file and the lock file already have he same mode. Which is the case if the filesystem has no proper chmod support.

Attached patch implements this and I could successfully initialize a new git repo on a CIFS share accessed over FUSE.

What do you think?

On 15.12.24 23:30, brian m. carlson wrote:

Show 13 quoted lines

> On 2024-12-13 at 10:32:22, Konrad Bucheli (PSI) wrote:
>> My suggestion is not to silently ignore all chmod errors, only ENOTSUP for
>> config.lock which basically tells that it is not a suitable location for
>> that operation and thus also not required.
>> Would that be acceptable?
> 
> That solves your particular problem, but the WSL situation gets
> EPERM[0], so it would not solve the general problem.  We definitely
> don't want to blindly ignore EPERM in general for `config.lock` because
> it could indicate a real permissions problem or a race condition with
> other software.
> 
> [0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository

diff --git a/config.c b/config.c
index a11bb85da3..3466f2ca4c 100644
--- a/config.c
+++ b/config.c
@@ -3256,7 +3256,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 		    write_pair(fd, key, value, comment, &store) < 0)
 			goto write_err_out;
 	} else {
-		struct stat st;
+		struct stat st, st_lock;
 		size_t copy_begin, copy_end;
 		int i, new_line = 0;
 		struct config_options opts;
@@ -3336,12 +3336,20 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 		close(in_fd);
 		in_fd = -1;
 
-		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
-			error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
+		if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
+			error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
 			ret = CONFIG_NO_WRITE;
 			goto out_free;
 		}
 
+		if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
+			if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
+				error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
+				ret = CONFIG_NO_WRITE;
+				goto out_free;
+			}
+		}
+
 		if (store.seen_nr == 0) {
 			if (!store.seen_alloc) {
 				/* Did not see key nor section */