On Fri, May 1, 2015 at 6:46 PM, Jeff King <peff@xxxxxxxx> 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 <peff@xxxxxxxx>
> ---
> 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);
}
> diff --git a/remote.c b/remote.c
> index a27f795..9f84ea3 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -704,20 +697,31 @@ const char *remote_for_branch(struct branch *branch, int *explicit)
> return "origin";
> }
>
> -static struct remote *remote_get_1(const char *name, const char *pushremote_name)
> +const char *pushremote_for_branch(struct branch *branch, int *explicit)
> +{
> + if (branch && branch->pushremote_name) {
> + if (explicit)
> + *explicit = 1;
> + return branch->pushremote_name;
> + }
> + if (pushremote_name) {
> + if (explicit)
> + *explicit = 1;
> + return pushremote_name;
> + }
> + return remote_for_branch(branch, explicit);
> +}
> +
> +static struct remote *remote_get_1(const char *name,
> + const char *(*get_default)(struct branch *, int *))
> {
> struct remote *ret;
> int name_given = 0;
>
> if (name)
> name_given = 1;
> - else {
> - if (pushremote_name) {
> - name = pushremote_name;
> - name_given = 1;
> - } else
> - name = remote_for_branch(current_branch, &name_given);
> - }
> + else
> + name = get_default(current_branch, &name_given);
>
> ret = make_remote(name, 0);
> if (valid_remote_nick(name)) {
> @@ -738,13 +742,13 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name
> struct remote *remote_get(const char *name)
> {
> read_config();
> - return remote_get_1(name, NULL);
> + return remote_get_1(name, remote_for_branch);
> }
>
> struct remote *pushremote_get(const char *name)
> {
> read_config();
> - return remote_get_1(name, pushremote_name);
> + return remote_get_1(name, pushremote_for_branch);
> }
>
> int remote_is_configured(const char *name)
--
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