Re: [PATCH v5 25/26] refs: break out ref conflict checks

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

On 10/28/2015 03:14 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.
> 
> rename_ref_available also moves to the common code, because alternate
> backends might need it and it has no files-backend-specific code.

It is preferred that log messages be written in the imperative voice:

    ... so move it to the common code.

    Also move rename_ref_available ...

> Signed-off-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs-be-files.c | 49 ++++++++-----------------------------------------
>  refs.c          | 38 ++++++++++++++++++++++++++++++++++++++
>  refs.h          | 15 +++++++++++++++
>  3 files changed, 61 insertions(+), 41 deletions(-)
> 
> diff --git a/refs-be-files.c b/refs-be-files.c
> index 0eabea9..97a5983 100644
> --- a/refs-be-files.c
> +++ b/refs-be-files.c
> @@ -729,6 +729,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;
> @@ -834,33 +835,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.
> -		 */

In your version, the above comment is replaced with

    /* Look for the place where dirname would be inserted in extras. */

which is pretty obvious, I think, from the following line. It's
debatable whether the long-winded old comment was worthwhile, but I
think the new comment is clearly not. I suggest either restoring a
longer explanation or eliminating the comment altogether.

> -		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;
>  	}

Project practice is to leave out unnecessary braces.

>  
> -	/* No conflicts were found */
> -	ret = 0;
> -
>  cleanup:
>  	strbuf_release(&dirname);
>  	return ret;
> @@ -2460,22 +2443,6 @@ out:
> [...]
> diff --git a/refs.c b/refs.c
> index 056c172..5d8b6ea 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> diff --git a/refs.h b/refs.h
> index f97a2e4..88fea3e 100644
> --- a/refs.h
> +++ b/refs.h
> [...]
> @@ -623,6 +625,19 @@ 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);
>  
> +/*
> + * Check for entries in extras that are within the specified
> + * directory, where dirname is a reference directory name including
> + * the trailing slash (e.g., "refs/heads/master/"). Ignore any
> + * conflicting references that are found in skip. If there is a
> + * conflicting reference, return its name.
> + *
> + * extras and skip must be sorted lists of reference names. skip can
> + * be NULL; extras cannot.

The last sentence is incorrect; the function can handle extras == NULL.

> + */
> +const char *find_descendant_ref(const char *dirname,
> +				const struct string_list *extras,
> +				const struct string_list *skip);
>  
>  enum expire_reflog_flags {
>  	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu