Re: [PATCH v5 25/27] refs: add LMDB refs storage backend

[Junio C Hamano <gitster@pobox.com>]

David Turner <dturner@twopensource.com> writes:

> Something like the following?
>
> commit aad6b84fd1869f6e1cf6ed15bcece0c2f6429e9d
> Author: David Turner <dturner@twopensource.com>
> Date:   Thu Feb 18 17:09:29 2016 -0500
>
>     refs: break out some functions from resolve_ref_1
>     
>     A bunch of resolve_ref_1 is not backend-specific, so we can
>     break it out into separate internal functions that other
>     backends can use.
>     
>     Signed-off-by: David Turner <dturner@twopensource.com>
> 
> diff --git a/refs.c b/refs.c
> index c9fa34d..680c2a5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1221,6 +1221,66 @@ int for_each_rawref(each_ref_fn fn, void
> *cb_data)
>  			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
>  }
>  
> +int parse_simple_ref(const char *buf, unsigned char *sha1, unsigned
> int *flags, int bad_name)
> +{
> +	/*
> +	 * Please note that FETCH_HEAD has a second
> +	 * line containing other data.
> +	 */

This comment is not quite correct; the reason why the latter half of
this condition is more convoluted than just !buf[40] is not because
FETCH_HEAD has a second line, but it has an additional info at the
tail on the first line.

Also the caller is expected to have already checked for "ref:"
prefix to decide not to call this.

> +	if (get_sha1_hex(buf, sha1) ||
> +	    (buf[40] != '\0' && !isspace(buf[40]))) {

> +/*
> + * Parse a refname out of the contents of a symref into a provided
> + * strbuf.  Return a pointer to the strbuf's contents.
> + */
> +char *parse_symref_data(const char *buf, struct strbuf *sb_refname)
> +{
> +	buf += 4;
> +	while (isspace(*buf))
> +		buf++;

This is expecting to be called by somebody who read "ref:..."  into
buf and the caller must decide by checking the "ref:" prefix before
calling into this function.

> ...  I'm not sure I like it, because it breaks out these weird
> tiny functions that take a lot of arguments.  But maybe it's worth
> it?  What do you think?

I wasn't Cc'ed but if you ask me, I do not think I can say I like
it, either.  Somehow it smells that the responsibility of inspecting
data and doing different things based on normal vs symbolic ref is
split between the caller and the callees at a wrong level.  The
caller also is responsible to rtrim the line before calling the
latter function if I am reading the code correctly, which smells
inconsistent given that an equivalent ltrim() is done by the "skip
the leading spaces" loop inside.