Re: [PATCH v5 25/27] refs: add LMDB refs storage backend
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:08:19
David Turner [off-list ref] writes:
quoted hunk
Something like the following? commit aad6b84fd1869f6e1cf6ed15bcece0c2f6429e9d Author: David Turner [off-list ref] 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 [off-list ref]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.