Re: [PATCH v2 01/43] refs.c: create a public version of verify_refname_available

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/29/2015 12:01 AM, David Turner wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Create a public version of verify_refname_available that backends can
> provide.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  refs.c | 51 +++++++++++++++++++++++++++++----------------------
>  refs.h | 17 +++++++++++++++++
>  2 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 132eff5..ce551e9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -279,7 +279,7 @@ struct ref_dir {
>   * presence of an empty subdirectory does not block the creation of a
>   * similarly-named reference.  (The fact that reference names with the
>   * same leading components can conflict *with each other* is a
> - * separate issue that is regulated by verify_refname_available().)
> + * separate issue that is regulated by verify_refname_available_dir().)
>   *
>   * Please note that the name field contains the fully-qualified
>   * reference (or subdirectory) name.  Space could be saved by only
> @@ -911,11 +911,11 @@ static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
>   *
>   * extras and skip must be sorted.
>   */
> -static int verify_refname_available(const char *refname,
> -				    const struct string_list *extras,
> -				    const struct string_list *skip,
> -				    struct ref_dir *dir,
> -				    struct strbuf *err)
> +static int verify_refname_available_dir(const char *refname,
> +					const struct string_list *extras,
> +					const struct string_list *skip,
> +					struct ref_dir *dir,
> +					struct strbuf *err)
>  {
>  	const char *slash;
>  	int pos;
> [...]
> diff --git a/refs.h b/refs.h
> index 6d30c98..79ea220 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -218,6 +218,23 @@ extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct st
>  int pack_refs(unsigned int flags);
>  
>  /*
> + * Return true iff a reference named refname could be created without
> + * conflicting with the name of an existing reference.  If
> + * skip is non-NULL, ignore potential conflicts with refs in skip
> + * (e.g., because they are scheduled for deletion in the same
> + * operation).
> + *
> + * Two reference names conflict if one of them exactly matches the
> + * leading components of the other; e.g., "foo/bar" conflicts with
> + * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
> + * "foo/barbados".
> + *
> + * skip must be sorted.
> + */

This comment is approximately a copy of the comment for
verify_refname_available_dir(). It seems unnecessary to keep both of
them (and is also a small maintenance burden). I suggest you shorten the
comment at verify_refname_available_dir() and make it refer to the
comment for this function for the details.

> +int verify_refname_available(const char *newname, struct string_list *extra,
> +			     struct string_list *skip, struct strbuf *err);
> +
> +/*
>   * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
>   * REF_NODEREF: act on the ref directly, instead of dereferencing
>   *              symbolic references.
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu