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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Taylor Blau wrote:

> In previous patches, the functions 'commit_shallow_file' and
> 'rollback_shallow_file' were introduced to reset the shallowness
> validity checks on a repository after potentially modifying
> '.git/shallow'.
>
> These functions can be made safer by wrapping the 'struct lockfile *' in
> a new type, 'shallow_lock', so that they cannot be called with a raw
> lock (and potentially misused by other code that happens to possess a
> lockfile, but has nothing to do with shallowness).
>
> This patch introduces that type as a thin wrapper around 'struct
> lockfile', and updates the two aforementioned functions and their
> callers to use it.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/receive-pack.c |  2 +-
>  fetch-pack.c           |  2 +-
>  shallow.c              | 22 +++++++++++-----------
>  shallow.h              | 16 +++++++++++++---
>  4 files changed, 26 insertions(+), 16 deletions(-)

Yay!  Thanks for indulging the suggestion.

[...]
> --- 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;
> +};
> +#define SHALLOW_LOCK_INIT { LOCK_INIT }

I think I disagree with Eric here: it's useful to have a comment here
to describe the purpose of the struct (i.e., the "why" as opposed to
the "what").

I wonder if we can go further, though --- when using a shallow_lock,
how should I think of it as a caller?  In some sense, the use of
'struct lock_file' is an implementation detail, so we could say:

	/*
	 * Lock for updating the $GIT_DIR/shallow file.
	 *
	 * Use `commit_shallow_file()` to commit an update, or
	 * `rollback_shallow_file()` to roll it back.  In either case,
	 * any in-memory cached information about which commits are
	 * shallow will be appropriately invalidated so that future
	 * operations reflect the new state.
	 */

What do you think?

[...]
> --- a/shallow.c
> +++ b/shallow.c
[...]
> @@ -366,22 +366,22 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
>  	return "";
>  }
>  
> -void setup_alternate_shallow(struct lock_file *shallow_lock,
> +void setup_alternate_shallow(struct shallow_lock *shallow_lock,
>  			     const char **alternate_shallow_file,
>  			     const struct oid_array *extra)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	int fd;
>  
> -	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);

This is peeking into the underlying lock_file, so I should ask myself
whether it's hinting at some missing function in the shallow_lock
API.  My feeling is "no": setup_alternate_shallow is itself that
function. :)

[...]
> @@ -414,7 +414,7 @@ void advertise_shallow_grafts(int fd)
>   */
>  void prune_shallow(unsigned options)
>  {
> -	struct lock_file shallow_lock = LOCK_INIT;
> +	struct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;
>  	struct strbuf sb = STRBUF_INIT;
>  	unsigned flags = SEEN_ONLY;
>  	int fd;
> @@ -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);

There's no obvious helper to extract here either, so this looks good
to me.

With whatever tweaks based on Eric's and Jonathan's reviews seem
appropriate,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.