On 04/11/17 02:27, Junio C Hamano wrote:
Rafael Ascensão <rafa.almas@xxxxxxxxx> writes:
Signed-off-by: Kevin Daudt <me@xxxxxxxxx>
Signed-off-by: Rafael Ascensão <rafa.almas@xxxxxxxxx>
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.
