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

[Jonathan Nieder <jrnieder@gmail.com>]

Taylor Blau wrote:

> --- 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?

[...]
>  		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?

[...]
> --- 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