Re: [PATCH 3/3] reftable: support preset file mode for writing

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 24, 2021, 04:46 UTC
Message-ID: <xmqqsfui4nkr.fsf@gitster.g>
In-Reply-To: <c01b1c335a33e5f44289c520a1634d071d882223.1640287790.git.gitgitgadget@gmail.com>

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Create files with mode 0666, so umask works as intended. Provides an override,
> which is useful to support shared repos (test t1301-shared-repo.sh).

> +	/* Default mode for creating files. If unset, use 0666 (+umask) */
> +	unsigned int default_permissions;
> +

Presumably this is primed by a call to get_shared_repository(), possibly followed by calc_shared_perm(), but having it in the interface is a good idea, as it allows us to avoid making these git-core specific calls from reftable/ library proper?

Show 10 quoted lines

> diff --git a/reftable/stack.c b/reftable/stack.c
> index df5021ebf08..56bf5f2d84a 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -469,7 +469,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  	strbuf_addstr(&add->lock_file_name, ".lock");
>  
>  	add->lock_file_fd = open(add->lock_file_name.buf,
> -				 O_EXCL | O_CREAT | O_WRONLY, 0644);
> +				 O_EXCL | O_CREAT | O_WRONLY, 0666);

This change makes sense.

Show 11 quoted lines

>  	if (add->lock_file_fd < 0) {
>  		if (errno == EEXIST) {
>  			err = REFTABLE_LOCK_ERROR;
> @@ -478,6 +478,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  		}
>  		goto done;
>  	}
> +	if (st->config.default_permissions) {
> +		if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
> +			err = REFTABLE_IO_ERROR;
> +			goto done;

This part does not exactly make sense, though.

If this were a library code meant to link with ONLY with Git, I would have recommended to make a adjust_shared_perm() call from here, without having to have "unsigned int default_permissions" to reftable_write_options structure.

I wonder if it is a better design to make the new member in the structure a pointer to a generic and opaque helper function that is called from here, i.e.

	if (st->config.adjust_perm &&
            st->config.adjust_perm(add->lock_file_name.buf) < 0) {
		err = REFTABLE_IO_ERROR;
		goto done;
	}

so that when linking with and calling from git, we can just stuff the pointer to adjust_shared_perm function to the member?

> +		}
> +	}
> +