Subject: Re: [PATCH v2 39/43] refs: break out a ref conflict check

From: Michael Haggerty <[email-masked]>
Date: Mon, 05 Oct 2015 11:06:53 +0200

On 09/29/2015 12:02 AM, David Turner wrote:
> Create new function verify_no_descendants, to hold one of the ref
> conflict checks used in verify_refname_available.  Multiple backends
> will need this function, so it goes in the common code.
> 
> Signed-off-by: David Turner <[email-masked]>
> ---
>  refs-be-files.c | 33 ++++++++-------------------------
>  refs.c          | 28 ++++++++++++++++++++++++++++
>  refs.h          |  4 ++++
>  3 files changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/refs-be-files.c b/refs-be-files.c
> index 943604c..8815111 100644
> --- a/refs-be-files.c
> +++ b/refs-be-files.c
> @@ -753,6 +753,7 @@ static int verify_refname_available_dir(const char *refname,
>  					struct strbuf *err)
>  {
>  	const char *slash;
> +	const char *extra_refname;
>  	int pos;
>  	struct strbuf dirname = STRBUF_INIT;
>  	int ret = -1;
> @@ -858,33 +859,15 @@ static int verify_refname_available_dir(const char *refname,
>  		}
>  	}
>  
> -	if (extras) {
> -		/*
> -		 * Check for entries in extras that start with
> -		 * "$refname/". We do that by looking for the place
> -		 * where "$refname/" would be inserted in extras. If
> -		 * there is an entry at that position that starts with
> -		 * "$refname/" and is not in skip, then we have a
> -		 * conflict.
> -		 */
> -		for (pos = string_list_find_insert_index(extras, dirname.buf, 0);
> -		     pos < extras->nr; pos++) {
> -			const char *extra_refname = extras->items[pos].string;
> -
> -			if (!starts_with(extra_refname, dirname.buf))
> -				break;
> -
> -			if (!skip || !string_list_has_string(skip, extra_refname)) {
> -				strbuf_addf(err, "cannot process '%s' and '%s' at the same time",
> -					    refname, extra_refname);
> -				goto cleanup;
> -			}
> -		}
> +	extra_refname = find_descendant_ref(dirname.buf, extras, skip);
> +	if (extra_refname) {
> +		strbuf_addf(err,
> +			    "cannot process '%s' and '%s' at the same time",
> +			    refname, extra_refname);
> +	} else {
> +		ret = 0;
>  	}
>  
> -	/* No conflicts were found */
> -	ret = 0;
> -
>  cleanup:
>  	strbuf_release(&dirname);
>  	return ret;
> diff --git a/refs.c b/refs.c
> index 1c2dd79..17a364a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -994,6 +994,34 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1)
>  	return PEEL_PEELED;
>  }
>  
> +const char *find_descendant_ref(const char *refname,
> +				const struct string_list *extras,
> +				const struct string_list *skip)
> +{
> +	int pos;
> +	if (!extras)
> +		return NULL;
> +
> +	/*
> +	 * Check for entries in extras that start with "$refname/". We
> +	 * do that by looking for the place where "$refname/" would be
> +	 * inserted in extras. If there is an entry at that position
> +	 * that starts with "$refname/" and is not in skip, then we
> +	 * have a conflict.
> +	 */

Would you please turn this comment (or something like it) into a
docstring for this function in refs.h?

> +	for (pos = string_list_find_insert_index(extras, refname, 0);
> +	     pos < extras->nr; pos++) {
> +		const char *extra_refname = extras->items[pos].string;
> +
> +		if (!starts_with(extra_refname, refname))
> +			break;
> +
> +		if (!skip || !string_list_has_string(skip, extra_refname))
> +			return extra_refname;
> +	}
> +	return NULL;
> +}
> +
>  /* backend functions */
>  int refs_initdb(struct strbuf *err, int shared)
>  {
> diff --git a/refs.h b/refs.h
> index da29232..cf1780e 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -571,6 +571,10 @@ enum ref_type ref_type(const char *refname);
>  
>  int copy_reflog_msg(char *buf, const char *msg);
>  
> +const char *find_descendant_ref(const char *refname,
> +				const struct string_list *extras,
> +				const struct string_list *skip);
> +
>  int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
>  			const unsigned char *new_sha1, const char *msg,
>  			int flags, struct strbuf *err);
> 

Michael

-- 
Michael Haggerty
[email-masked]