Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Nov 5, 2017, 13:42 UTC
- Message-ID
- <4dc4eefc-56b9-1b13-ae46-83a3af9c7ee3@alum.mit.edu>
- In-Reply-To
- <20171104004144.5975-2-rafa.almas@gmail.com>
On 11/04/2017 01:41 AM, Rafael Ascensão wrote:
Show 41 quoted lines
> `for_each_glob_ref_in` has some code built into it that converts
> partial refs like 'heads/master' to their full qualified form
> 'refs/heads/master'. It also assume a trailing '/*' if no glob
> characters are present in the pattern.
>
> Extract that logic to its own function which can be reused elsewhere
> where the same behaviour is needed, and add an ENSURE_GLOB flag
> to toggle if a trailing '/*' is to be appended to the result.
>
> Signed-off-by: Kevin Daudt <me@ikke.info>
> Signed-off-by: Rafael Ascensão <rafa.almas@gmail.com>
> ---
> refs.c | 34 ++++++++++++++++++++--------------
> refs.h | 16 ++++++++++++++++
> 2 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index c590a992f..1e74b48e6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
> return ret;
> }
>
> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> - const char *prefix, void *cb_data)
> +void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,
> + const char *pattern, int flags)
> {
> - struct strbuf real_pattern = STRBUF_INIT;
> - struct ref_filter filter;
> - int ret;
> -
> if (!prefix && !starts_with(pattern, "refs/"))
> - strbuf_addstr(&real_pattern, "refs/");
> + strbuf_addstr(normalized_pattern, "refs/");
> else if (prefix)
> - strbuf_addstr(&real_pattern, prefix);
> - strbuf_addstr(&real_pattern, pattern);
> + strbuf_addstr(normalized_pattern, prefix);
> + strbuf_addstr(normalized_pattern, pattern);I realize that the old code did this too, but while you are in the area it might be nice to simplify the logic from
if (!prefix && !starts_with(pattern, "refs/")) strbuf_addstr(normalized_pattern, "refs/"); else if (prefix) strbuf_addstr(normalized_pattern, prefix);
to
if (prefix) strbuf_addstr(normalized_pattern, prefix); else if (!starts_with(pattern, "refs/")) strbuf_addstr(normalized_pattern, "refs/");
This would avoid having to check twice whether `prefix` is NULL.
Show 54 quoted lines
> - if (!has_glob_specials(pattern)) {
> + if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {
> /* Append implied '/' '*' if not present. */
> - strbuf_complete(&real_pattern, '/');
> + strbuf_complete(normalized_pattern, '/');
> /* No need to check for '*', there is none. */
> - strbuf_addch(&real_pattern, '*');
> + strbuf_addch(normalized_pattern, '*');
> }
> +}
> +
> +int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> + const char *prefix, void *cb_data)
> +{
> + struct strbuf normalized_pattern = STRBUF_INIT;
> + struct ref_filter filter;
> + int ret;
> +
> + normalize_glob_ref(&normalized_pattern, prefix, pattern, ENSURE_GLOB);
>
> - filter.pattern = real_pattern.buf;
> + filter.pattern = normalized_pattern.buf;
> filter.fn = fn;
> filter.cb_data = cb_data;
> ret = for_each_ref(filter_refs, &filter);
>
> - strbuf_release(&real_pattern);
> + strbuf_release(&normalized_pattern);
> return ret;
> }
>
> diff --git a/refs.h b/refs.h
> index a02b628c8..9f9a8bb27 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
> int for_each_rawref(each_ref_fn fn, void *cb_data);
>
> +/*
> + * Normalizes partial refs to their full qualified form.
> + * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start
> + * with 'refs/'. Results in refs/<pattern>
> + *
> + * If prefix is not NULL will result in <prefix>/<pattern>
> + *
> + * If ENSURE_GLOB is set and no glob characters are found in the
> + * pattern, a trailing </><*> will be appended to the result.
> + * (<> characters to avoid breaking C comment syntax)
> + */
> +
> +#define ENSURE_GLOB 1
> +void normalize_glob_ref (struct strbuf *normalized_pattern, const char *prefix,
> + const char *pattern, int flags);There shouldn't be a space between the function name and the open parenthesis.
You have complicated the interface by allowing an `ENSURE_BLOB` flag. This would make sense if the logic for normalizing the prefix were tangled up with the logic for adding the suffix. But in fact they are almost entirely orthogonal [1].
So the interface might be simplified by having two functions,
void normalize_glob_ref(normalized_pattern, prefix, pattern);
void ensure_blob(struct strbuf *pattern);The caller in this patch would call the functions one after the other (or the `ensure_blob` behavior could be inlined in `for_each_glob_ref_in()`, since it doesn't yet have any callers). And the callers introduced in patch 2 would only need to call the first function.
> static inline const char *has_glob_specials(const char *pattern)
> {
> return strpbrk(pattern, "?*[");
> Michael
[1] I say "almost entirely" because putting them in one function means that only `pattern` needs to be scanned for glob characters. But that is an unimportant detail.