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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2016, 23:08 UTC
Message-ID: <xmqq37sob8my.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1455922186.7528.97.camel@twopensource.com>

David Turner <dturner@twopensource.com> writes:

Show 30 quoted lines

> 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]))) {

Show 9 quoted lines

> +/*
> + * 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.