Re: [PATCH 04/12] remote.c: provide per-branch pushremote name

On Fri, May 1, 2015 at 6:46 PM, Jeff King [off-list ref] wrote:

When remote.c loads its config, it records the
branch.*.pushremote for the current branch along with the
global remote.pushDefault value, and then binds them into a
single value: the default push for the current branch. We
then pass this value (which may be NULL) to remote_get_1
when looking up a remote for push.

This has a few downsides:

  1. It's confusing. The early-binding of the "current
     value" led to bugs like the one fixed by 98b406f
     (remote: handle pushremote config in any order,
     2014-02-24). And the fact that pushremotes fall back to
     ordinary remotes is not explicit at all; it happens
     because remote_get_1 cannot tell the difference between
     "we are not asking for the push remote" and "there is
     no push remote configured".

  2. It throws away intermediate data. After read_config()
     finishes, we have no idea what the value of
     remote.pushDefault was, because the string has been
     overwritten by the current branch's
     branch.*.pushremote.

  3. It doesn't record other data. We don't note the
     branch.*.pushremote value for anything but the current
     branch.

Let's make this more like the fetch-remote config. We'll
record the pushremote for each branch, and then explicitly
compute the correct remote for the current branch at the
time of reading.

Signed-off-by: Jeff King <redacted>
---
Versus v1, I did something a little clever by passing a function pointer
around (versus a flag and letting the caller do a conditional based on
the flag). Too clever?

FWIW: I found this "clever" version easy enough to follow.

However, if you push a tiny bit of the work into the callers of
remote_get_1(), then you can do away with the "cleverness" altogether,
can't you? Something like this:

    static struct remote_get_1(const char *name, int explicit)
    {
        struct remote *ret = make_remote(name, 0);
        ...
        if (explicit && valid_remote(ret))
            ...
        ...
    }

    struct remote *remote_get(const char *name)
    {
        int explicit = !!name;
        read_config();
        if (!name)
            name = remote_for_branch(current_branch, &explicit);
        return remote_get_1(name, explicit);
    }

    struct remote *pushremote_get(const char *name)
    {
        int explicit = !!name;
        read_config();
        if (!name)
            name = pushremote_for_branch(current_branch, &explicit);
        return remote_get_1(name, explicit);
    }