Re: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2014, 19:49 UTC
Message-ID: <xmqq61j5805g.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53BE3127.8020805@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 16 quoted lines

> Isn't the whole problem starting here:
> in config.c:
>
>     fd = hold_lock_file_for_update(lock, config_filename, 0);
> In lockfile.c:
>   /* This should return a meaningful errno on failure */
>   int hold_lock_file_for_update(struct lock_file *lk, const char
> *path, int flags)
>   {
>       int fd = lock_file(lk, path, flags);
> which leads to
>   static int lock_file(struct lock_file *lk, const char *path, int flags)
>     []
>     lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
>
> There is no way to tell which permissions the new lockfile should have.

We follow whategver user's umask says with this code.

> That is somewhat unlucky.
>
> On the other hand, shouldn't we call
> adjust_shared_perm(const char *path) from path.c on the config file?

Good question, but I am not sure. For $GIT_DIR/config, I tend to agree we should, but "git config --global foo bar" would not be a shared file anyway, and my understanding of Eric's original motivation is to keep $HOME/.gitconfig to be tighter than the user's umask normally would indicate.

Show 5 quoted lines

> And to all files which are fiddled through the lock_file API?
> In other words, the lockfile could be created with the restrictive
> permissions
> 600, and once the lockfile had been closed and renamed into the final name
> we apply adjust_shared_perm() on it ?

For all files that adjust-shared-perm should apply, yes, but I do not think it is relevant to the codepath in question.

> I think there are 2 different things missing here:
>
> - Be able to specify permissions to hold_lock_file_for_update(),
>    especially restrictive ones, like 600 and not 666.

Yes (in the sense that "yes we can add an extra parameter") and no (in the sense that "where would we get the value to pass to the extra parameter from? would it be worth to add configurations variables for different kinds of files?").

If we limit the case to "Inherit permissions from the file we are replacing by taking a lock on it", which is the topic of discussion in this thread, we do not have to worry about how to configure the value (we do not have to) and adding a new parameter to tell the mode to hold-lock-file-for-update is unneeded (the function will have a pathname of the original and can learn the current permission bits itself).

> - Adjust the permissions for "shared files" in a shared repo.
>   This is probably needed for a shared repo, when the user itself
>    has a umask which is too restrictive and adjust_shared_perm()
>    must be run to widen the permissions.

Don't we already do that for $GIT_DIR/config? In any case that will not help $HOME/.gitconfig and other files that are not shared.