Re: [PATCH v2] config: honor file permission bits

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 26, 2026 at 07:59:43AM -0700, Junio C Hamano wrote:
> When dotfile sources are kept under version control, you might
> protect installed files like ~/.gitconfig from accidental changes by
> making them read-only.  This fails because updating a file writes a
> new lockfile, copies and tweaks the content, and renames the
> lockfile over the original.  The original file's permission bits are
> ignored in this process.
> 
> Teach the configuration writing machinery to inspect the original
> file's permissions with access(2) and avoid replacing it when the
> file should not be writable.
> 
> This implementation targets configuration files rather than the
> lockfile API layer.  While in-tree files like .git/index do not need
> such protection, out-of-tree files like ~/.gitconfig and its XDG

Nit: a curious reader might wonder why such in-tree files don't need
such protection.

> counterparts are unique in this regard.  System-wide configuration
> like /etc/gitconfig is typically in a directory already read-only to
> normal users, making the issue irrelevant there.

Thanks for adding the motivation.

> diff --git a/config.c b/config.c
> index e0bb29b53d..2b84fb215c 100644
> --- a/config.c
> +++ b/config.c
> @@ -3066,6 +3066,28 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
>  	if (!config_filename)
>  		config_filename = filename_buf = repo_git_path(r, "config");
>  
> +	/*
> +	 * Do not allow updating a read-only configuration file; it is
> +	 * annoying and alarming that "chmod a-w $HOME/.gitconfig" does not
> +	 * prevent the global configuration file from getting clobbered by
> +	 * mistake with the usual "create a new lockfile, copy the original
> +	 * while setting a new value, propagate the permission bits to the
> +	 * lockfile, and then rename it to replace the original" dance.
> +	 */
> +	if (!access(config_filename, W_OK)) {
> +		; /* happy: we can write to it */
> +	} else if (errno == ENOENT) {
> +		; /* happy: we are creating this file anew */
> +	} else {
> +		/*
> +		 * most likely EACCES but other errors also indicate
> +		 * that we cannot write to it anyway.
> +		 */
> +		error_errno(_("file %s unwritable"), config_filename);

Should we quote '%s' here? The function is quite inconsistent, where it
sometimes quotes and sometimes doesn't. I'd say that quoting is the
better choice though.

Other than that this patch looks good to me, thanks!

Patrick