Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
On 11/06/2017 02:23 AM, Junio C Hamano wrote: > Michael Haggertywrites: > >> [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. > > That could actually be an important detail, in that even if prefix > has wildcard, we'd still append the trailing "/*" as long as the > pattern does not, right? That's correct, but I was assuming that the prefix would always be a hard-coded string like "refs/tags/" or maybe "refs/". (That is the case now.) It doesn't seem very useful to use a prefix like "refs/*/". Michael
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
On 06/11/17 01:23, Junio C Hamano wrote: > Michael Haggertywrites: > >> [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. > > That could actually be an important detail, in that even if prefix > has wildcard, we'd still append the trailing "/*" as long as the > pattern does not, right? > > So the interface might be simplified by having two functions, > > void normalize_glob_ref(normalized_pattern, prefix, pattern); > void ensure_blob(struct strbuf *pattern); I think that flag no longer makes sense. I added it just to allow '--decorate-refs' work with "exact patterns". And since that has the ugly side effect of losing the ability to use "shortcut patterns" like 'tags' to refer to 'refs/tags/*', I believe it's a good idea to remove it.
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
Michael Haggertywrites: > [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. That could actually be an important detail, in that even if prefix has wildcard, we'd still append the trailing "/*" as long as the pattern does not, right?
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
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> Signed-off-by: Rafael Ascensão > --- > 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(_pattern, "refs/"); > + strbuf_addstr(normalized_pattern, "refs/"); > else if (prefix) > - strbuf_addstr(_pattern, prefix); > - strbuf_addstr(_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(_pattern, '/'); > + strbuf_complete(normalized_pattern, '/'); > /* No need to check for '*', there is none. */ > - strbuf_addch(_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(_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, ); > > - strbuf_release(_pattern); > + strbuf_release(_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/ > + * > + * If prefix is not NULL will result in / > + * > + * 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
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
On 11/04/2017 11:45 PM, Kevin Daudt wrote: > On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote: >> I however notice that addition of /* to the tail is trying to be >> careful by using strbuf_complete('/'), but prefixing with "refs/" >> does not and we would end up with a double-slash if pattern begins >> with a slash. The contract between the caller of this function (or >> its original, which is for_each_glob_ref_in()) and the callee is >> that prefix must not begin with '/', so it may be OK, but we might >> want to add "if (*pattern == '/') BUG(...)" at the beginning. >> >> I dunno. In any case, that is totally outside the scope of this two >> patch series. > > I do think it's a good idea to make future readers of the code aware of > this contract, and adding a BUG assert does that quite well. Here is a > patch that implements it. > > This applies of course on top of this patch series. > > -- >8 -- > Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix > > normalize_glob_ref has an implicit contract of expecting 'prefix' to not > start with a '/', otherwise the pattern would end up with a > double-slash. > > Mark it as a BUG when the prefix argument of normalize_glob_ref starts > with a '/' so that future callers will be aware of this contract. > > Signed-off-by: Kevin Daudt> --- > refs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/refs.c b/refs.c > index e9ae659ae..6747981d1 100644 > --- a/refs.c > +++ b/refs.c > @@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data) > void normalize_glob_ref(struct strbuf *normalized_pattern, const char > *prefix, > const char *pattern, int flags) > { > + if (prefix && *prefix == '/') BUG("prefix cannot not start with '/'"); This should be split onto two lines. Also, "prefix cannot not start ..." has two "not". I suggest changing it to "prefix must not start ...", because that makes it clearer that the caller is at fault. What if the caller passes the empty string as prefix? In that case, the end result would be "/", which is also bogus. > + > if (!prefix && !starts_with(pattern, "refs/")) > strbuf_addstr(normalized_pattern, "refs/"); > else if (prefix) Michael
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote: > I however notice that addition of /* to the tail is trying to be > careful by using strbuf_complete('/'), but prefixing with "refs/" > does not and we would end up with a double-slash if pattern begins > with a slash. The contract between the caller of this function (or > its original, which is for_each_glob_ref_in()) and the callee is > that prefix must not begin with '/', so it may be OK, but we might > want to add "if (*pattern == '/') BUG(...)" at the beginning. > > I dunno. In any case, that is totally outside the scope of this two > patch series. I do think it's a good idea to make future readers of the code aware of this contract, and adding a BUG assert does that quite well. Here is a patch that implements it. This applies of course on top of this patch series. -- >8 -- Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix normalize_glob_ref has an implicit contract of expecting 'prefix' to not start with a '/', otherwise the pattern would end up with a double-slash. Mark it as a BUG when the prefix argument of normalize_glob_ref starts with a '/' so that future callers will be aware of this contract. Signed-off-by: Kevin Daudt--- refs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/refs.c b/refs.c index e9ae659ae..6747981d1 100644 --- a/refs.c +++ b/refs.c @@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data) void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix, const char *pattern, int flags) { + if (prefix && *prefix == '/') BUG("prefix cannot not start with '/'"); + if (!prefix && !starts_with(pattern, "refs/")) strbuf_addstr(normalized_pattern, "refs/"); else if (prefix) -- 2.15.0.rc2.57.g2f899857a9
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
On 04/11/17 02:27, Junio C Hamano wrote: Rafael Ascensãowrites: Signed-off-by: Kevin Daudt Signed-off-by: Rafael Ascensão Could you explain Kevin's sign-off we see above? It is a bit unusual (I am not yet saying it is wrong---I cannot judge until I find out why it is there) to see a patch from person X with sign off from person Y and then person X in that order. It is normal for a patch authored by person X to have sign-off by X and then Y if X wrote it, signed it off and passed to Y, and then Y resent it after signing it off (while preserving the authorship of X by adding an in-body From: header), but I do not think that is what we have here. It could be that you did pretty much all the work on this patch and Kevin helped you to polish this patch off-list, in which case the usual thing to do is to use "Helped-by: Kevin" instead. That's more or less what happened. I wouldn't say I did "pretty much all the work". Yes, I wrote the code but with great help of Kevin. The intention of the dual Signed-off-by was to equally attribute authorship of the patch. But if that creates ambiguity I will change it to "Helped-by" as suggested. It is better to use "unsigned" for a single word "flags" used as a collection of bits. In older parts of the codebase, we have codepaths that pass signed int as a flags word, simply because we didn't know better, but we do not have to spread that practice to new code. I noticed this, but chose to "mimic" the code around me. I'll correct it. On a related note is there a guideline for defining flags or are `#define FLAG (1u << 0)`, `#define FLAG (1 << 0)` `#define FLAG 1` and `#define FLAG 0x1` equally accepted? { - struct strbuf real_pattern = STRBUF_INIT; - struct ref_filter filter; - int ret; - if (!prefix && !starts_with(pattern, "refs/")) - strbuf_addstr(_pattern, "refs/"); + strbuf_addstr(normalized_pattern, "refs/"); else if (prefix) - strbuf_addstr(_pattern, prefix); - strbuf_addstr(_pattern, pattern); + strbuf_addstr(normalized_pattern, prefix); + strbuf_addstr(normalized_pattern, pattern); - if (!has_glob_specials(pattern)) { + if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) { /* Append implied '/' '*' if not present. */ - strbuf_complete(_pattern, '/'); + strbuf_complete(normalized_pattern, '/'); /* No need to check for '*', there is none. */ - strbuf_addch(_pattern, '*'); + strbuf_addch(normalized_pattern, '*'); } +} The above looks like a pure and regression-free code movement (plus a small new feature) that is faithful to the original, which is good. I however notice that addition of /* to the tail is trying to be careful by using strbuf_complete('/'), but prefixing with "refs/" does not and we would end up with a double-slash if pattern begins with a slash. The contract between the caller of this function (or its original, which is for_each_glob_ref_in()) and the callee is that prefix must not begin with '/', so it may be OK, but we might want to add "if (*pattern == '/') BUG(...)" at the beginning. I dunno. In any case, that is totally outside the scope of this two patch series. I guess it doesn't hurt adding that safety net. Thanks. Other than the above minor points, looks good to me. I'll fix the mentioned issues. Thanks.
Re: [PATCH v1 1/2] refs: extract function to normalize partial refs
Rafael Ascensãowrites: > `for_each_glob_ref_in` has some code built into it that converts > partial refs like 'heads/master' to their full qualified form s/full// (read: that "full" needs "y" at the end). > 'refs/heads/master'. It also assume a trailing '/*' if no glob s/assume// > 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 > Signed-off-by: Rafael Ascensão Could you explain Kevin's sign-off we see above? It is a bit unusual (I am not yet saying it is wrong---I cannot judge until I find out why it is there) to see a patch from person X with sign off from person Y and then person X in that order. It is normal for a patch authored by person X to have sign-off by X and then Y if X wrote it, signed it off and passed to Y, and then Y resent it after signing it off (while preserving the authorship of X by adding an in-body From: header), but I do not think that is what we have here. It could be that you did pretty much all the work on this patch and Kevin helped you to polish this patch off-list, in which case the usual thing to do is to use "Helped-by: Kevin" instead. > --- > 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) It is better to use "unsigned" for a single word "flags" used as a collection of bits. In older parts of the codebase, we have codepaths that pass signed int as a flags word, simply because we didn't know better, but we do not have to spread that practice to new code. > { > - struct strbuf real_pattern = STRBUF_INIT; > - struct ref_filter filter; > - int ret; > - > if (!prefix && !starts_with(pattern, "refs/")) > - strbuf_addstr(_pattern, "refs/"); > + strbuf_addstr(normalized_pattern, "refs/"); > else if (prefix) > - strbuf_addstr(_pattern, prefix); > - strbuf_addstr(_pattern, pattern); > + strbuf_addstr(normalized_pattern, prefix); > + strbuf_addstr(normalized_pattern, pattern); > > - if (!has_glob_specials(pattern)) { > + if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) { > /* Append implied '/' '*' if not present. */ > - strbuf_complete(_pattern, '/'); > + strbuf_complete(normalized_pattern, '/'); > /* No need to check for '*', there is none. */ > - strbuf_addch(_pattern, '*'); > + strbuf_addch(normalized_pattern, '*'); > } > +} The above looks like a pure and regression-free code movement (plus a small new feature) that is faithful to the original, which is good. I however notice that addition of /* to the tail is trying to be careful by using strbuf_complete('/'), but prefixing with "refs/" does not and we would end up with a double-slash if pattern begins with a slash. The contract between the caller of this function (or its original, which is for_each_glob_ref_in()) and the callee is that prefix must not begin with '/', so it may be OK, but we might want to add "if (*pattern == '/') BUG(...)" at the beginning. I dunno. In any case, that is totally outside the scope of this two patch series. > 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. s/full//; > + * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start > + * with 'refs/'. Results in refs/ > + * > + * If prefix is not NULL will result in / s/NULL/&,/; > + * > + * 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); > + > static inline const char *has_glob_specials(const char *pattern) > { > return strpbrk(pattern, "?*["); Thanks. Other
[PATCH v1 1/2] refs: extract function to normalize partial refs
`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 DaudtSigned-off-by: Rafael Ascensão --- 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(_pattern, "refs/"); + strbuf_addstr(normalized_pattern, "refs/"); else if (prefix) - strbuf_addstr(_pattern, prefix); - strbuf_addstr(_pattern, pattern); + strbuf_addstr(normalized_pattern, prefix); + strbuf_addstr(normalized_pattern, pattern); - if (!has_glob_specials(pattern)) { + if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) { /* Append implied '/' '*' if not present. */ - strbuf_complete(_pattern, '/'); + strbuf_complete(normalized_pattern, '/'); /* No need to check for '*', there is none. */ - strbuf_addch(_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(_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, ); - strbuf_release(_pattern); + strbuf_release(_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/ + * + * If prefix is not NULL will result in / + * + * 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); + static inline const char *has_glob_specials(const char *pattern) { return strpbrk(pattern, "?*["); -- 2.15.0