Re: [PATCH] Preserve the protection mode for the Git config files

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 22, 2009, 18:14 UTC
Message-ID: <7vab2wlh4y.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090721152435.16642.47207.stgit@pc1117.cambridge.arm.com>

Catalin Marinas <catalin.marinas@arm.com> writes:

Show 23 quoted lines

> Every time an option is set, the config file protection mode is changed
> to 0666 & ~umask even if it was different before. This patch is useful
> if people store passwords (SMTP server in the StGit case) and do not
> want others to read the .gitconfig file.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  lockfile.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index eb931ed..87ee233 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -134,7 +138,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  	if (!(flags & LOCK_NODEREF))
>  		resolve_symlink(lk->filename, sizeof(lk->filename)-5);
>  	strcat(lk->filename, ".lock");
> -	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
> +	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, st.st_mode);
>  	if (0 <= lk->fd) {
>  		if (!lock_file_list) {
>  			sigchain_push_common(remove_lock_file_on_signal);

Your log message talks about .git/config and nothing else, but I think this codepath affects everything that is created under the lock, such as the index and refs.

Later in the function we call adjust_shared_perm(), and I had to wonder if this change have an adverse effect in a shared repository setting.

For example, in a repository shared (with core.sharedrepository = true) between users whose umask is 002, if somebody makes a ref unreadable by others by mistake, the current code fixes the mistake in a later update, but with your patch, the ref will kept unreadable.

This change in behaviour is justifiable only because the only thing the user who said "core.sharedrepository = true" cares about is that refs are readable by the group members (otherwise s/he would have used a more explicit setting like "core.sharedrepository = 0660", and the adjust_shared_perm() code will do the right thing, with or without your patch).

The patch description must defend itself a bit better, perhaps by saying something like this at the end.

	This patch touches the codepath that affects not just .git/config
	but other files like the index and the loose refs, so they also
	inherit the original protection bits.  In a private repository,
	this is not an issue exactly because the repository is private,

	In a shared repository, a later call made in this function to
	adjust_shared_perm() widens the permission bits as configured.
	Because adjust_shared_perm() is designed to do so from any mode
	limited by user's umask, even though this patch changes the
	behaviour in the strict sense, it should not affect the outcome in
	a negative way and what is explicitly marked as allowed in the
	configuration will still be allowed.