Re: [PATCH 5/5] shallow: use struct 'shallow_lock' for additional safety

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 30, 2020, 00:30 UTC
Message-ID: <20200430003059.236985-1-jonathantanmy@google.com>
In-Reply-To: <c0dc5024a9b368dfbca99b81bc843f66d725f3d7.1588199705.git.me@ttaylorr.com>

Show 17 quoted lines

> @@ -428,14 +428,14 @@ void prune_shallow(unsigned options)
>  		strbuf_release(&sb);
>  		return;
>  	}
> -	fd = hold_lock_file_for_update(&shallow_lock,
> +	fd = hold_lock_file_for_update(&shallow_lock.lk,
>  				       git_path_shallow(the_repository),
>  				       LOCK_DIE_ON_ERROR);
>  	check_shallow_file_for_update(the_repository);
>  	if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
>  		if (write_in_full(fd, sb.buf, sb.len) < 0)
>  			die_errno("failed to write to %s",
> -				  get_lock_file_path(&shallow_lock));
> +				  get_lock_file_path(&shallow_lock.lk));
>  		commit_shallow_file(the_repository, &shallow_lock);
>  	} else {
>  		unlink(git_path_shallow(the_repository));

I was hoping that the inner lock ("lk") wouldn't need to be exposed, so that it wouldn't be possible to inadvertently commit the lock without going through commit_shallow_file(), but this patch is still a step towards that.

Show 16 quoted lines

> diff --git a/shallow.h b/shallow.h
> index 08e1bc4fd0..d12096fbc4 100644
> --- a/shallow.h
> +++ b/shallow.h
> @@ -10,12 +10,22 @@ void set_alternate_shallow_file(struct repository *r, const char *path, int over
>  int register_shallow(struct repository *r, const struct object_id *oid);
>  int unregister_shallow(const struct object_id *oid);
>  int is_repository_shallow(struct repository *r);
> +
> +/*
> + * shallow_lock is a thin wrapper around 'struct lock_file' in order to restrict
> + * which locks can be used with '{commit,rollback}_shallow_file()'.
> + */
> +struct shallow_lock {
> +	struct lock_file lk;
> +};

As far as I know, we don't use many abbreviations in struct members - maybe s/lk/lock/.

I couldn't find any other issues with all 5 patches in this patch series, but I would like to be able to apply the patches to be sure.