Re: [PATCH v3 37/44] refs: break out a ref conflict check

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

On 10/12/2015 11:51 PM, 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 <dturner@twopensource.com>
> ---
>  refs-be-files.c | 33 ++++++++-------------------------
>  refs.c          | 22 ++++++++++++++++++++++
>  refs.h          |  7 +++++++
>  3 files changed, 37 insertions(+), 25 deletions(-)
> 
> [...]
> diff --git a/refs.c b/refs.c
> index 5a3125d..3ae0274 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1101,6 +1101,28 @@ 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;
> +
> +	/* Look for the place where "$refname/" would be inserted in extras. */

The comment above is misleading. See my note at the bottom of this email
for an explanation.

> +	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 3aad3b8..f8becea 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -637,6 +637,13 @@ 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 start with "$refname/", ignoring
> + * those in skip. If there is such an entry, then we have a conflict.
> + */
> +const char *find_descendant_ref(const char *refname,
> +				const struct string_list *extras,
> +				const struct string_list *skip);

The documentation is is not correct. As written, the function needs not
the refname as argument but the refname followed by '/'. Without the
trailing slash, "refs/heads/foo" would be blocked by "refs/heads/foobar".

>From the point of view of simplicity, it would be nice if this function
could take a refname (without the trailing slash) as argument. But then
it would basically be forced to allocate a new string, copy refname to
it, append a '/', then free the string again when it's done.
Alternatively, it could accept its refname argument in a strbuf and
temporarily append the '/'.

But neither one of these alternatives is so elegant, so maybe it's OK if
this function is documented to take a "directory name, including the
trailing '/'" as argument and rename the argument (e.g., to "dirname").

Please also document that "skip" and "extras" can be NULL, but if not
NULL they need to be sorted lists.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu