Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 23, 2020, 01:23 UTC
Message-ID: <20200423012305.GH140314@google.com>
In-Reply-To: <296e70790d7a391d471554b0bc5a58e2a091ce88.1587601501.git.me@ttaylorr.com>

Taylor Blau wrote:

Show 8 quoted lines

> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	opt.env = tmp_objdir_env(tmp_objdir);
>  	setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
>  	if (check_connected(command_singleton_iterator, cmd, &opt)) {
> -		rollback_lock_file(&shallow_lock);
> +		rollback_shallow_file(the_repository, &shallow_lock);

I like it.

I wonder, is there a way we can make it more difficult to accidentally use rollback_lock_file where rollback_shallow_file is needed? For example, what if shallow_lock has a different type "struct shallow_lock" so one would have to reach in to its lock_file member to bypass the shallow_file interface?

[...]

Show 19 quoted lines

>  		oid_array_clear(&extra);
>  		return -1;
>  	}
>  
> -	commit_lock_file(&shallow_lock);
> +	commit_shallow_file(the_repository, &shallow_lock);
>  
>  	/*
>  	 * Make sure setup_alternate_shallow() for the next ref does
> diff --git a/commit.h b/commit.h
> index 008a0fa4a0..ab91d21131 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,6 +249,8 @@ struct oid_array;
>  struct ref;
>  int register_shallow(struct repository *r, const struct object_id *oid);
>  int unregister_shallow(const struct object_id *oid);
> +int commit_shallow_file(struct repository *r, struct lock_file *lk);
> +void rollback_shallow_file(struct repository *r, struct lock_file *lk);

optional: might make sense to put this near setup_alternate_shallow for discoverability

Could this have an API doc comment?

[...]

Show 5 quoted lines

> --- a/shallow.c
> +++ b/shallow.c
> @@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)
>  
>  int is_repository_shallow(struct repository *r)

Not about this patch: it might make sense to split out a shallow.h header / API.

Thanks and hope that helps, Jonathan