Re: odb_mkstemp's 0444 permission broke write/delete access on AFP

On 17/02/15 17:58, Fairuzan Roslan wrote:

quoted

On Feb 17, 2015, at 4:51 PM, Matthieu Moy [off-list ref] wrote:

Fairuzan Roslan [off-list ref] writes:

quoted

$ git clone https://github.com/robbyrussell/oh-my-zsh.git
Cloning into 'oh-my-zsh'...
remote: Counting objects: 11830, done.
remote: Total 11830 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (11830/11830), 2.12 MiB | 481.00 KiB/s, done.
Resolving deltas: 100% (6510/6510), done.
warning: unable to unlink /Volumes/installer/oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted

This should be fixable from Git itself, by replacing the calls to
"unlink" with something like

int unlink_or_chmod(...) {
	if (unlink(...)) {
		chmod(...); // give user write permission
		return unlink(...);
	}
}

This does not add extra cost in the normal case, and would fix this
particular issue for afp shares. So, I think that would fix the biggest
problem for afp-share users without disturbing others. It seems
reasonable to me to do that unconditionnally.

quoted

$ rm -rf oh-my-zsh/.git/objects/pack/tmp_*
rm: oh-my-zsh/.git/objects/pack/tmp_idx_oUN1sb: Operation not permitted
rm: oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted

What happens if you do "rm -fr oh-my-zsh/.git/objects/pack/" (i.e.
remove the directory, not the files)?

If you can still remove the directory, then I'd say the solution above
could be sufficient: the user isn't supposed to interfer with the
content of .git/objects other than by using Git, and if he or she does,
then asking a chmod prior to an rm seems reasonable.

If you can't, then it's another problematic use-case (basically, you
can't just "rm -fr" a whole clone), and then it deserves at least an
opt-in configuration to get writable pack files.

(Unfortunately, I suspect we're in the later case)

quoted

If you insist on setting the tmp idx & pack file permission to 0444 at
least give it a u+w permission whenever you try to unlink and rename
it so it won’t fail.

Yes. In case you hadn't guessed, this is precisely what I had in mind
when I asked "Is it a problem when using Git [...] or when trying to
remove files outside Git?".

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Yes. It’s a problem when using Git where it fails to unlink and rename the tmp idx and pack files.
The reason I tries to rm -rf the tmp_idx_XXXXXX and tmp_pack_XXXXXX is to proof a point why Git fails

Perhaps my explanation wasn’t clear enough. Maybe it’s hard for you to understand without having to test it yourself on a AFP filesystem.

Let me explain why AFP filesystem is more strict and different from your typical filesystem like ext4,hfs+,etc.

$ mkdir testdir; chmod 0755 testdir; touch testdir/testfile; chmod 0444 testdir/testfile; ls -la testdir
total 0
drwxr-xr-x  1 user  staff  264 Feb 18 00:26 .
drwx------  1 user  staff  264 Feb 18 00:26 ..
-r--r--r--  1 user  staff    0 Feb 18 00:26 testfile

$ rm -rf testdir
rm: testdir/testfile: Operation not permitted
rm: testdir: Directory not empty

This works on my system (Mac OS 10.9 as server and client)

$ chmod +w testdir/testfile; ls -la testdir
total 0
drwxr-xr-x  1 riaf  staff  264 Feb 18 00:26 .
drwx------  1 riaf  staff  264 Feb 18 00:26 ..
-rw-r—r--  1 riaf  staff    0 Feb 18 00:26 testfile

$ rm -rf testdir <—— No error message

This show that you cannot delete a directory or a file without a write permission in AFP filesystem.

The problem with Git failing is not because its inability to delete a directory but its inability to unlink and rename tmp_idx_XXXXXX and tmp_pack_XXXXXX because those files were set to 0444 by odb_mkstemp.
Try google for “Git AFP” and you will see a lot people are facing with the same problem.

Yes, (at least to my knowledge) you seem to be one of the first to report it here, thanks for that.

Regarding your suggestion, yes I think it would work but you have to take care (chmod) every calls that rename or unlink or delete files with 0444 permission.

Regards,
Fairuzan

The "right" solution is to make a wrapper function, and to re-define unlink() and rename() with help
of the preprocessor.

git-compat-util.h has an example for fopen(), so that can be used for a patch.

And no, as I can not reproduce it here, I can only help with reviews or so.