push: refactor refspec_append_mapped() for subsequent leak-fix

The set_refspecs() caller of refspec_append_mapped() (added in [1])
left open the question[2] of whether the "remote" we lazily fetch
might be NULL in the "[...]uniquely name our ref?" case, as
remote_get() can return NULL.

If we got past the "[...]uniquely name our ref?" case we'd have
already segfaulted if we tried to dereference it as
"remote->push.nr". In these cases the config mechanism & previous
remote validation will have bailed out earlier.

Let's refactor this code to clarify that, we'll now BUG() out if we
can't get a "remote", and will no longer retrieve it for these common
cases where we don't need it.

1. ca02465b413 (push: use remote.$name.push as a refmap, 2013-12-03)
2. https://lore.kernel.org/git/c0c07b89-7eaf-21cd-748e-e14ea57f09fd@web.de/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/push.c b/builtin/push.c
index 60ac8017e521f0c7c9192df89c797e1898ec60fd..97b35eca3ab9c579c0b47567b079ab3f56205b39 100644 (file)
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -63,16 +63,9 @@ static struct refspec rs = REFSPEC_INIT_PUSH;
 static struct string_list push_options_config = STRING_LIST_INIT_DUP;
 
 static void refspec_append_mapped(struct refspec *refspec, const char *ref,
-                                 struct remote *remote, struct ref *local_refs)
+                                 struct remote *remote, struct ref *matched)
 {
        const char *branch_name;
-       struct ref *matched = NULL;
-
-       /* Does "ref" uniquely name our ref? */
-       if (count_refspec_match(ref, local_refs, &matched) != 1) {
-               refspec_append(refspec, ref);
-               return;
-       }
 
        if (remote->push.nr) {
                struct refspec_item query;
@@ -120,12 +113,24 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
                                die(_("--delete only accepts plain target ref names"));
                        refspec_appendf(&rs, ":%s", ref);
                } else if (!strchr(ref, ':')) {
-                       if (!remote) {
-                               /* lazily grab remote and local_refs */
-                               remote = remote_get(repo);
+                       struct ref *matched = NULL;
+
+                       /* lazily grab local_refs */
+                       if (!local_refs)
                                local_refs = get_local_heads();
+
+                       /* Does "ref" uniquely name our ref? */
+                       if (count_refspec_match(ref, local_refs, &matched) != 1) {
+                               refspec_append(&rs, ref);
+                       } else {
+                               /* lazily grab remote */
+                               if (!remote)
+                                       remote = remote_get(repo);
+                               if (!remote)
+                                       BUG("must get a remote for repo '%s'", repo);
+
+                               refspec_append_mapped(&rs, ref, remote, matched);
                        }
-                       refspec_append_mapped(&rs, ref, remote, local_refs);
                } else
                        refspec_append(&rs, ref);
        }