From: Rafael Ascensão Date: Sat, 04 Nov 2017 07:33:43 GMT Subject: Re: [PATCH v1 1/2] refs: extract function to normalize partial refs Message-ID: <1e2e8f85-e13d-47f9-6661-1e685250c775@gmail.com> In-Reply-To: On 04/11/17 02:27, Junio C Hamano wrote: > Rafael Ascensão writes: > >> 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(&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); >> >> - 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, '*'); >> } >> +} > > 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.