From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Wed, 22 Feb 2012 20:15:41 GMT
Subject: Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
Message-ID: <CAMP44s2HUG_ocHBaVpcsZHWMf2Tww+=bVun5H9+S5EGkoiJHRQ@mail.gmail.com>
In-Reply-To: <7vvcn5qecl.fsf@alter.siamese.dyndns.org>

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> There's a lot of code that can be consolidated there, and will be useful
>> for next patches.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------
>>  1 files changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/remote.c b/remote.c
>> index 55d68d1..019aafc 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
>>       return errs;
>>  }
>>
>> -static const struct refspec *check_pattern_match(const struct refspec *rs,
>> -                                              int rs_nr,
>> -                                              const struct ref *src)
>> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
>> +             int send_mirror, const struct refspec **ret_pat)
>>  {
>
> For a change that not just adds parameters but removes an existing one,
> this is way under-described with neither in-code comment nor log message.

But it doesn't. src is renamed to ref.

>> +     const struct refspec *pat;
>> +     char *name;
>>       int i;
>>       int matching_refs = -1;
>>       for (i = 0; i < rs_nr; i++) {
>> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
>>                       continue;
>>               }
>>
>> -             if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
>> -                                                          NULL, NULL))
>> -                     return rs + i;
>> +             if (rs[i].pattern) {
>> +                     const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
>> +                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
>> +                             matching_refs = i;
>> +                             break;
>
> We used to discard what match_name_with_pattern() finds out by matching a
> wildcard refspec against the ref by passing two NULLs.  This updates the
> code to capture what destination ref ref->name is mapped to, by using the
> same logic as the original and only caller, i.e. 'foo' without destination
> maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.
>
> This function is not used by fetching side of the codepath, so we do not
> have to worry about its need to use different dst_side selection logic
> (i.e. 'foo' without destination maps to "do not store anywhere other than
> FETCH_HEAD").  Good.

I actually didn't parse a lot of that.

>> +                     }
>> +             }
>>       }
>> -...
>> +     if (matching_refs == -1)
>>               return NULL;
>> +
>> +     pat = rs + matching_refs;
>> +     if (pat->matching) {
>> +             /*
>> +              * "matching refs"; traditionally we pushed everything
>> +              * including refs outside refs/heads/ hierarchy, but
>> +              * that does not make much sense these days.
>> +              */
>> +             if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
>> +                     return NULL;
>> +             name = xstrdup(ref->name);
>> +     }
>
> So you are moving some code from what the sole caller of this function
> does after calling us, and that is where the new parameters come from.
> And by doing so, you do not have to run the same match_name_with_pattern()
> again.  OK.

Indeed.

-- 
Felipe Contreras