On 11/04/2017 01:41 AM, Rafael Ascensão wrote: > `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@xxxxxxxxx> > Signed-off-by: Rafael Ascensão <rafa.almas@xxxxxxxxx> > --- > 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. > - 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.