Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> 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.
Show 28 quoted lines
>> + 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.
Show 23 quoted lines
>> + }
>> + }
>> }
>> -...
>> + 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.--
Felipe Contreras
Felipe Contreras <felipe.contreras@gmail.com>