Felipe Contreras <felipe.contreras@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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.
So I'll have to think aloud in this review. Take it as a chance to learn
how the thought process of other people with lessor intelligence than you
have might work, and how to help those slower than you are ;-)
> + 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.
> + }
> + }
> }
> -...
> + 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.
> + if (ret_pat)
> + *ret_pat = pat;
> + return name;
> }
You did not initialize name to anything at the beginning, but if the
earlier match_name_with_pattern() didn't find anything, we could only come
here after matching_refs is set by the if (rs[i].matching) codepath; by
the time we come here, we would have xstrdup(ref->name) in name, so we
would never return a garbage pointer to the caller. OK.
> static struct ref **tail_ref(struct ref **head)
> @@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,
> struct ref *dst_peer;
> const struct refspec *pat = NULL;
> char *dst_name;
> +
> if (ref->peer_ref)
> continue;
>
> - pat = check_pattern_match(rs, nr_refspec, ref);
> - if (!pat)
> + dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
> + if (!dst_name)
> continue;
>
> - 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/"))
> - continue;
> - dst_name = xstrdup(ref->name);
> -
> -
> - } else {
> - const char *dst_side = pat->dst ? pat->dst : pat->src;
> - if (!match_name_with_pattern(pat->src, ref->name,
> - dst_side, &dst_name))
> - die("Didn't think it matches any more");
> - }
> dst_peer = find_ref_by_name(*dst, dst_name);
> if (dst_peer) {
> if (dst_peer->peer_ref)
> /* We're already sending something to this ref. */
> goto free_name;
> -
> } else {
> if (pat->matching && !(send_all || send_mirror))
> /*
OK, it is easy to tell that the patch is trivially correct, once a reader
figures out that the patch is really about:
Move code to check_pattern_match() from its sole caller to make it
unnecessary to call match_name_with_pattern() twice.
Saying so in the log message would have prepared the reader, instead of
the "There's a lot of code that can be consolidated there." which does not
give hints on what to look for in the patch.
Also this changes the semantics (because it changed the meaning of its
return value) of check_pattern_match() so much that it would deserve a
rename, which I will address in my review of 3/3.
Otherwise this step looks good.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html