Re: [PATCH v1 1/2] refs: extract function to normalize partial refs

["Rafael Ascensão" <rafa.almas@gmail.com>]

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