Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

There's a lot of code that can be consolidated there, and will be useful
for next patches.

Signed-off-by: Felipe Contreras <redacted>
---
 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.

quoted

+     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.

quoted

+                     }
+             }
      }
-...
+     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