Re: [RFC/PATCH] Introduce branch.<name>.pushremote

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Feb 10, 2013, 15:41 UTC
Message-ID: <CALkWK0mjEmErd2A5qb8qdfNMSXG1L0dt8A=ecbSVFGPEg5MxkA@mail.gmail.com>
In-Reply-To: <7vhallw946.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 25 quoted lines

>         struct bp { const char *branch; const char *remotename; };
>
>         static int pushremote(const char *var, const char *val, void *cb)
>         {
>                 struct bp *bp = cb;
>                 const char *name, *key;
>                 int namelen;
>
>                 if (!parse_config_key(var, "branch", &name, &namelen, &key) &&
>                     namelen == strlen(bp->branch) &&
>                     !memcmp(name, bp->branch, name, namelen) &&
>                     !strcmp(key, "remotepush")) {
>                         git_config_string(&bp->remotename, var, val);
>                 }
>                 return 0;
>         }
>
>         char *find_pushremote(void) {
>                 struct bp bp;
>
>                 bp.branch = get_current_branch();
>                 bp.remotename = NULL;
>                 git_config(pushremote, &bp);
>                 return bp.remotename;
>         }

I don't get the point of your illustration. First, we never deal with a char * branch anywhere: handle_config() immediately calls make_branch() as soon as it parses the name from the config. Second, remote_name is already bundled with the branch struct: why do you need a new bp struct? This is how I would do it without relying on a global:

	static int handle_config(const char *key, const char *value, void *cb) {
		/* parse branch name here */
		/* ... */
		branch = make_branch(name);
		if (!strcmp(subkey, ".pushremote"))
			branch->pushremote_name = xstrdup(value);
	}

	struct remote *pushremote_get(struct branch *this_branch)
	{
		if (this_branch == get_current_branch())
			name = this_branch->pushremote_name;
		/* ... */
		return make_remote(name);
	}

Essentially, I need to know for which branch we're trying to get the pushremote: hence the parameter this_branch to pushremote_get().

> But there is no reason for anybody to want to know the value for the
> variables branch.$name.pushremote for all defined $name's at the
> same time, exactly because this matters only for the current branch,
> no?

Ah, yes. You are right. There's no harm in having globals because there can really be only one current branch, and only one pushremote corresponding to that. It's just that globals tend to confuse me in general, because I have to keep track of a maze of functions that are getting/ setting them.