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

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

[Ramkumar Ramachandra]

This new configuration variable overrides the remote in
`branch.<name>.remote` for pushes.  It is useful in the typical
scenario, where the remote I'm pulling from is not the remote I'm
pushing to.  Although `remote.<name>.pushurl` is similar, it does not
serve the purpose as the URL would lack corresponding remote tracking
branches.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This is a first cut.  There's code duplication at the moment, but I'm
 currently trying to figure out which other remote_get() calls to
 replace with pushremote_get().  Comments are welcome.

 I will leave it to future patches to do the following things:
 1. Fix the status output to be more meaningful when pushremote is
 set.  At the moment, I'm thinking statuses like [pull: 4 behind,
 push: 3 ahead] will make sense.
 2. Introduce a remote.pushDefault (peff)
 3. Introduce a remote.default (peff)

 Documentation/config.txt |  6 ++++++
 builtin/push.c           |  2 +-
 remote.c                 | 41 +++++++++++++++++++++++++++++++++++++++++
 remote.h                 |  2 ++
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9b11597..0b3b1f8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -727,6 +727,12 @@ branch.<name>.remote::
 	remote to fetch from/push to.  It defaults to `origin` if no remote is
 	configured. `origin` is also used if you are not on any branch.
 
+branch.<name>.pushremote::
+	When in branch <name>, it tells 'git push' which remote to
+	push to.  It falls back to `branch.<name>.remote`, and
+	defaults to `origin` if no remote is configured. `origin` is
+	also used if you are not on any branch.
+
 branch.<name>.merge::
 	Defines, together with branch.<name>.remote, the upstream branch
 	for the given branch. It tells 'git fetch'/'git pull'/'git rebase' which
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
 static int do_push(const char *repo, int flags)
 {
 	int i, errs;
-	struct remote *remote = remote_get(repo);
+	struct remote *remote = pushremote_get(repo);
 	const char **url;
 	int url_nr;
 
diff --git a/remote.c b/remote.c
index e53a6eb..d6fcfc0 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
 
 static struct branch *current_branch;
 static const char *default_remote_name;
+static const char *pushremote_name;
 static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
@@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
 				default_remote_name = branch->remote_name;
 				explicit_default_remote_name = 1;
 			}
+		} else if (!strcmp(subkey, ".pushremote")) {
+			if (!value)
+				return config_error_nonbool(key);
+			branch->pushremote_name = xstrdup(value);
+			if (branch == current_branch)
+				pushremote_name = branch->pushremote_name;
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
@@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+struct remote *pushremote_get(const char *name)
+{
+	struct remote *ret;
+	int name_given = 0;
+
+	read_config();
+	if (name)
+		name_given = 1;
+	else {
+		if (pushremote_name) {
+			name = pushremote_name;
+			name_given = 1;
+		} else {
+			name = default_remote_name;
+			name_given = explicit_default_remote_name;
+		}
+	}
+
+	ret = make_remote(name, 0);
+	if (valid_remote_nick(name)) {
+		if (!valid_remote(ret))
+			read_remotes_file(ret);
+		if (!valid_remote(ret))
+			read_branches_file(ret);
+	}
+	if (name_given && !valid_remote(ret))
+		add_url_alias(ret, name);
+	if (!valid_remote(ret))
+		return NULL;
+	ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
+	ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
+	return ret;
+}
+
 int remote_is_configured(const char *name)
 {
 	int i;
diff --git a/remote.h b/remote.h
index 251d8fd..aa42ff5 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
 };
 
 struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
 int remote_is_configured(const char *name);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
@@ -130,6 +131,7 @@ struct branch {
 	const char *refname;
 
 	const char *remote_name;
+	const char *pushremote_name;
 	struct remote *remote;
 
 	const char **merge_name;
-- 
1.8.1.2.545.g2f19ada.dirty

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>  Comments are welcome.

As the first cut, I would have expected the series to start from
more general (not "only this branch"), with later follow-up to let
more specific configuration.

Also I'd prefer to see the "push" semantics (e.g. "what does
upstream mean in this new world order?") designed better first.

[RFC/PATCH] Introduce remote.pushdefault

[Ramkumar Ramachandra]

This new configuration variable overrides branch-specific
configuration `branch.<name>.remote` for pushes.  It is useful in the
typical scenario, where the remote I'm pulling from is not the remote
I'm pushing to.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Junio C Hamano wrote:
 > As the first cut, I would have expected the series to start from
 > more general (not "only this branch"), with later follow-up to let
 > more specific configuration.

 Doesn't that follow trivially from my previous patch?  I'm looking
 for comments on how best to share code between pushremote_get()/
 remote_get(), and on other remote_get() callsites.

 > Also I'd prefer to see the "push" semantics (e.g. "what does
 > upstream mean in this new world order?") designed better first.

 Why should the meaning of upstream change?  We'd probably like to
 introduce something like a branch@{downstream} pointing to the push
 remote ref sometime in the future though.  Wait, should it always be
 called downstream?

 Documentation/config.txt |  4 ++++
 builtin/push.c           |  2 +-
 remote.c                 | 45 +++++++++++++++++++++++++++++++++++++++++++--
 remote.h                 |  1 +
 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9b11597..82a4a78 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1884,6 +1884,10 @@ receive.updateserverinfo::
 	If set to true, git-receive-pack will run git-update-server-info
 	after receiving data from git-push and updating refs.
 
+remote.pushdefault::
+	The remote to push to by default.  Overrides the
+	branch-specific configuration `branch.<name>.remote`.
+
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
 	linkgit:git-push[1].
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
 static int do_push(const char *repo, int flags)
 {
 	int i, errs;
-	struct remote *remote = remote_get(repo);
+	struct remote *remote = pushremote_get(repo);
 	const char **url;
 	int url_nr;
 
diff --git a/remote.c b/remote.c
index e53a6eb..08bb803 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
 
 static struct branch *current_branch;
 static const char *default_remote_name;
+static const char *pushremote_name;
 static int explicit_default_remote_name;
 
 static struct rewrites rewrites;
@@ -349,6 +350,14 @@ static int handle_config(const char *key, const char *value, void *cb)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
+	if (!prefixcmp(key,  "remote.")) {
+		name = key + 7;
+		if (!strcmp(name, "pushdefault")) {
+			if (!value)
+				return config_error_nonbool(key);
+			pushremote_name = xstrdup(value);
+		}
+	}
 	if (!prefixcmp(key, "branch.")) {
 		name = key + 7;
 		subkey = strrchr(name, '.');
@@ -388,8 +397,6 @@ static int handle_config(const char *key, const char *value, void *cb)
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
-	if (prefixcmp(key,  "remote."))
-		return 0;
 	name = key + 7;
 	if (*name == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
@@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+struct remote *pushremote_get(const char *name)
+{
+	struct remote *ret;
+	int name_given = 0;
+
+	read_config();
+	if (name)
+		name_given = 1;
+	else {
+		if (pushremote_name) {
+			name = pushremote_name;
+			name_given = 1;
+		} else {
+			name = default_remote_name;
+			name_given = explicit_default_remote_name;
+		}
+	}
+
+	ret = make_remote(name, 0);
+	if (valid_remote_nick(name)) {
+		if (!valid_remote(ret))
+			read_remotes_file(ret);
+		if (!valid_remote(ret))
+			read_branches_file(ret);
+	}
+	if (name_given && !valid_remote(ret))
+		add_url_alias(ret, name);
+	if (!valid_remote(ret))
+		return NULL;
+	ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
+	ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
+	return ret;
+}
+
 int remote_is_configured(const char *name)
 {
 	int i;
diff --git a/remote.h b/remote.h
index 251d8fd..99a437f 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
 };
 
 struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
 int remote_is_configured(const char *name);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
-- 
1.8.1.3.535.ga923c31.dirty

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9b11597..0b3b1f8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -727,6 +727,12 @@ branch.<name>.remote::
>  	remote to fetch from/push to.  It defaults to `origin` if no remote is
>  	configured. `origin` is also used if you are not on any branch.
>  
> +branch.<name>.pushremote::
> +	When in branch <name>, it tells 'git push' which remote to
> +	push to.  It falls back to `branch.<name>.remote`, and
> +	defaults to `origin` if no remote is configured. `origin` is
> +	also used if you are not on any branch.

Sounds sensible (modulo the missing "default location to push to
that is not per remote" bit, obviously).  I think "When _on_ branch"
is more correct, to match "if you are not on any branch" at the end,
though.

> diff --git a/remote.c b/remote.c
> index e53a6eb..d6fcfc0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -48,6 +48,7 @@ static int branches_nr;
>  
>  static struct branch *current_branch;
>  static const char *default_remote_name;
> +static const char *pushremote_name;
>  static int explicit_default_remote_name;
>  
>  static struct rewrites rewrites;
> @@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
>  				default_remote_name = branch->remote_name;
>  				explicit_default_remote_name = 1;
>  			}
> +		} else if (!strcmp(subkey, ".pushremote")) {
> +			if (!value)
> +				return config_error_nonbool(key);
> +			branch->pushremote_name = xstrdup(value);

Perhaps use git_config_string()?

I also notice that git_config_string() should free (*dest) if there
already is some value, and that has to be done after auditing all
existing callers.

> +			if (branch == current_branch)
> +				pushremote_name = branch->pushremote_name;

Why is this global only when current_branch is involved?

In other words, does it make sense to read branch.$name.pushremote
for all the other irrelevant branches?  

In yet other words, perhaps adding pushremote_name to the branch
structure is unneeded, and you only need this single global
variable?

(The remainder of the patch unsnipped for others' reference.)

> @@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
>  	return ret;
>  }
>  
> +struct remote *pushremote_get(const char *name)
> +{
> +	struct remote *ret;
> +	int name_given = 0;
> +
> +	read_config();
> +	if (name)
> +		name_given = 1;
> +	else {
> +		if (pushremote_name) {
> +			name = pushremote_name;
> +			name_given = 1;
> +		} else {
> +			name = default_remote_name;
> +			name_given = explicit_default_remote_name;
> +		}
> +	}
> +
> +	ret = make_remote(name, 0);
> +	if (valid_remote_nick(name)) {
> +		if (!valid_remote(ret))
> +			read_remotes_file(ret);
> +		if (!valid_remote(ret))
> +			read_branches_file(ret);
> +	}
> +	if (name_given && !valid_remote(ret))
> +		add_url_alias(ret, name);
> +	if (!valid_remote(ret))
> +		return NULL;
> +	ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
> +	ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
> +	return ret;
> +}
> +
>  int remote_is_configured(const char *name)
>  {
>  	int i;
> diff --git a/remote.h b/remote.h
> index 251d8fd..aa42ff5 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -51,6 +51,7 @@ struct remote {
>  };
>  
>  struct remote *remote_get(const char *name);
> +struct remote *pushremote_get(const char *name);
>  int remote_is_configured(const char *name);
>  
>  typedef int each_remote_fn(struct remote *remote, void *priv);
> @@ -130,6 +131,7 @@ struct branch {
>  	const char *refname;
>  
>  	const char *remote_name;
> +	const char *pushremote_name;
>  	struct remote *remote;
>  
>  	const char **merge_name;

Re: [RFC/PATCH] Introduce remote.pushdefault

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9b11597..82a4a78 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1884,6 +1884,10 @@ receive.updateserverinfo::
>  	If set to true, git-receive-pack will run git-update-server-info
>  	after receiving data from git-push and updating refs.
>  
> +remote.pushdefault::
> +	The remote to push to by default.  Overrides the
> +	branch-specific configuration `branch.<name>.remote`.

It feels unexpected to see "I may have said while on this branch I
push there and on that branch I push somewhere else, but no, with
this single configuration I'm invalidating all these previous
statements, and all pushes go to this new place".

Shouldn't the default be the default that is to be overridden by
other configuration that is more specific?  That is, "I would
normally push to this remote and unless I say otherwise that is all
I have to say, but for this particular branch, I push to somehwere
else".

> diff --git a/builtin/push.c b/builtin/push.c
> index 42b129d..d447a80 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
>  static int do_push(const char *repo, int flags)
>  {
>  	int i, errs;
> -	struct remote *remote = remote_get(repo);
> +	struct remote *remote = pushremote_get(repo);
>  	const char **url;
>  	int url_nr;
>  
> diff --git a/remote.c b/remote.c
> index e53a6eb..08bb803 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -48,6 +48,7 @@ static int branches_nr;
>  
>  static struct branch *current_branch;
>  static const char *default_remote_name;
> +static const char *pushremote_name;
>  static int explicit_default_remote_name;
>  
>  static struct rewrites rewrites;
> @@ -349,6 +350,14 @@ static int handle_config(const char *key, const char *value, void *cb)
>  	const char *subkey;
>  	struct remote *remote;
>  	struct branch *branch;
> +	if (!prefixcmp(key,  "remote.")) {
> +		name = key + 7;
> +		if (!strcmp(name, "pushdefault")) {
> +			if (!value)
> +				return config_error_nonbool(key);
> +			pushremote_name = xstrdup(value);
> +		}
> +	}
>  	if (!prefixcmp(key, "branch.")) {
>  		name = key + 7;
>  		subkey = strrchr(name, '.');
> @@ -388,8 +397,6 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			add_instead_of(rewrite, xstrdup(value));
>  		}
>  	}
> -	if (prefixcmp(key,  "remote."))
> -		return 0;

Why is this no longer needed?

All the remainder of this function is about "remote.*" config and
this rejects other keys, like "user.name", etc.

I'm a bit confused....

>  	name = key + 7;
>  	if (*name == '/') {
>  		warning("Config remote shorthand cannot begin with '/': %s",

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/remote.c b/remote.c
>> index e53a6eb..d6fcfc0 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -48,6 +48,7 @@ static int branches_nr;
>>
>>  static struct branch *current_branch;
>>  static const char *default_remote_name;
>> +static const char *pushremote_name;
>>  static int explicit_default_remote_name;
>>
>>  static struct rewrites rewrites;
>> @@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
>>                               default_remote_name = branch->remote_name;
>>                               explicit_default_remote_name = 1;
>>                       }
>> +             } else if (!strcmp(subkey, ".pushremote")) {
>> +                     if (!value)
>> +                             return config_error_nonbool(key);
>> +                     branch->pushremote_name = xstrdup(value);
>
> Perhaps use git_config_string()?

I was just following the style of the surrounding code without
thinking.  However, it looks like the surrounding code may be dated,
so I'll include a patch to update it to use git_config_string() before
making the change here.

>> +                     if (branch == current_branch)
>> +                             pushremote_name = branch->pushremote_name;
>
> Why is this global only when current_branch is involved?
>
> In other words, does it make sense to read branch.$name.pushremote
> for all the other irrelevant branches?
>
> In yet other words, perhaps adding pushremote_name to the branch
> structure is unneeded, and you only need this single global
> variable?

Frankly, I'm unhappy with this global.  Setting a global here and
subsequently reading it in pushremote_get() feels flaky.  Why use it
at all when we have branch->remote_name, branch->remote, and (the now
introduced) branch->pushremote_name?  I left the pushremote_name field
around, with the expectation that other codepaths that use the
remote_name field might be able to use it.

Thanks.

Re: [RFC/PATCH] Introduce remote.pushdefault

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9b11597..82a4a78 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1884,6 +1884,10 @@ receive.updateserverinfo::
>>       If set to true, git-receive-pack will run git-update-server-info
>>       after receiving data from git-push and updating refs.
>>
>> +remote.pushdefault::
>> +     The remote to push to by default.  Overrides the
>> +     branch-specific configuration `branch.<name>.remote`.
>
> It feels unexpected to see "I may have said while on this branch I
> push there and on that branch I push somewhere else, but no, with
> this single configuration I'm invalidating all these previous
> statements, and all pushes go to this new place".
>
> Shouldn't the default be the default that is to be overridden by
> other configuration that is more specific?  That is, "I would
> normally push to this remote and unless I say otherwise that is all
> I have to say, but for this particular branch, I push to somehwere
> else".

Oops, I meant to have it overriden by branch-specific configuration.  Fixed now.

>> diff --git a/builtin/push.c b/builtin/push.c
>> index 42b129d..d447a80 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
>>  static int do_push(const char *repo, int flags)
>>  {
>>       int i, errs;
>> -     struct remote *remote = remote_get(repo);
>> +     struct remote *remote = pushremote_get(repo);
>>       const char **url;
>>       int url_nr;
>>
>> diff --git a/remote.c b/remote.c
>> index e53a6eb..08bb803 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -48,6 +48,7 @@ static int branches_nr;
>>
>>  static struct branch *current_branch;
>>  static const char *default_remote_name;
>> +static const char *pushremote_name;
>>  static int explicit_default_remote_name;
>>
>>  static struct rewrites rewrites;
>> @@ -349,6 +350,14 @@ static int handle_config(const char *key, const char *value, void *cb)
>>       const char *subkey;
>>       struct remote *remote;
>>       struct branch *branch;
>> +     if (!prefixcmp(key,  "remote.")) {
>> +             name = key + 7;
>> +             if (!strcmp(name, "pushdefault")) {
>> +                     if (!value)
>> +                             return config_error_nonbool(key);
>> +                     pushremote_name = xstrdup(value);
>> +             }
>> +     }
>>       if (!prefixcmp(key, "branch.")) {
>>               name = key + 7;
>>               subkey = strrchr(name, '.');
>> @@ -388,8 +397,6 @@ static int handle_config(const char *key, const char *value, void *cb)
>>                       add_instead_of(rewrite, xstrdup(value));
>>               }
>>       }
>> -     if (prefixcmp(key,  "remote."))
>> -             return 0;
>
> Why is this no longer needed?
>
> All the remainder of this function is about "remote.*" config and
> this rejects other keys, like "user.name", etc.

I'm sorry.  I read that as if (!prefixcmp(key, "remote.")), which is
an entirely different thing.  Fixed now.

Thanks.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>
>> In other words, does it make sense to read branch.$name.pushremote
>> for all the other irrelevant branches?
>>
>> In yet other words, perhaps adding pushremote_name to the branch
>> structure is unneeded, and you only need this single global
>> variable?
>
> Frankly, I'm unhappy with this global.  Setting a global here and
> subsequently reading it in pushremote_get() feels flaky.

Why unhappy?

This is expressed as a per-branch configuration, but in realitiy the
configuration on any branch that is not the current one does *not*
matter an iota.

An API call find_pushremote_for_branch() that takes a branch name
and returns the pushremote for that arbitrary branch has no use in
your designed feature.  find_pushremote() that tells you where a
lazy "git push" goes under the conditoin is the only thing you need
in your design.  You can still implement it without globals, e.g.

	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;
	}


And if you want to take default.pushremote that changes a lazy "git
push" to go to somewhere other than "origin", you make the
pushremote() callback notice that variable, add one element
"defaultremote" to struct "bp" to record that value, and then make
find_pushremote() do

	return bp.remotename ? bp.remotename : bp.defaultremote;

Sometimes "global variable" feels disturbing because then we cannot
run more than one user at the same time.  One caller may want to
know the value for "foo" branch while another want it for "bar"
branch.

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?
			

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>         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.

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

[Blind]

If I understand correctly,
in your scenario the branches with branch.<name>.pushremote
will be still included in the $git push <remote> --all?
Are you considering some way to exclude a branch from "push --all"
(branch.<name>.push = always, explicit, never... for example)?
Or maybe, if the branch is already marked as special
with branch.<name>.pushremote,
then it could be logical to push it only when is explicitly specified
on the command line (excluded from --all)?

Thanks,
Blind.

2013/2/8 Ramkumar Ramachandra <artagnon@gmail.com>:
> This new configuration variable overrides the remote in
> `branch.<name>.remote` for pushes.  It is useful in the typical
> scenario, where the remote I'm pulling from is not the remote I'm
> pushing to.  Although `remote.<name>.pushurl` is similar, it does not
> serve the purpose as the URL would lack corresponding remote tracking
> branches.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  This is a first cut.  There's code duplication at the moment, but I'm
>  currently trying to figure out which other remote_get() calls to
>  replace with pushremote_get().  Comments are welcome.
>
>  I will leave it to future patches to do the following things:
>  1. Fix the status output to be more meaningful when pushremote is
>  set.  At the moment, I'm thinking statuses like [pull: 4 behind,
>  push: 3 ahead] will make sense.
>  2. Introduce a remote.pushDefault (peff)
>  3. Introduce a remote.default (peff)
>
>  Documentation/config.txt |  6 ++++++
>  builtin/push.c           |  2 +-
>  remote.c                 | 41 +++++++++++++++++++++++++++++++++++++++++
>  remote.h                 |  2 ++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9b11597..0b3b1f8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -727,6 +727,12 @@ branch.<name>.remote::
>         remote to fetch from/push to.  It defaults to `origin` if no remote is
>         configured. `origin` is also used if you are not on any branch.
>
> +branch.<name>.pushremote::
> +       When in branch <name>, it tells 'git push' which remote to
> +       push to.  It falls back to `branch.<name>.remote`, and
> +       defaults to `origin` if no remote is configured. `origin` is
> +       also used if you are not on any branch.
> +
>  branch.<name>.merge::
>         Defines, together with branch.<name>.remote, the upstream branch
>         for the given branch. It tells 'git fetch'/'git pull'/'git rebase' which
> diff --git a/builtin/push.c b/builtin/push.c
> index 42b129d..d447a80 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport, int flags)
>  static int do_push(const char *repo, int flags)
>  {
>         int i, errs;
> -       struct remote *remote = remote_get(repo);
> +       struct remote *remote = pushremote_get(repo);
>         const char **url;
>         int url_nr;
>
> diff --git a/remote.c b/remote.c
> index e53a6eb..d6fcfc0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -48,6 +48,7 @@ static int branches_nr;
>
>  static struct branch *current_branch;
>  static const char *default_remote_name;
> +static const char *pushremote_name;
>  static int explicit_default_remote_name;
>
>  static struct rewrites rewrites;
> @@ -363,6 +364,12 @@ static int handle_config(const char *key, const char *value, void *cb)
>                                 default_remote_name = branch->remote_name;
>                                 explicit_default_remote_name = 1;
>                         }
> +               } else if (!strcmp(subkey, ".pushremote")) {
> +                       if (!value)
> +                               return config_error_nonbool(key);
> +                       branch->pushremote_name = xstrdup(value);
> +                       if (branch == current_branch)
> +                               pushremote_name = branch->pushremote_name;
>                 } else if (!strcmp(subkey, ".merge")) {
>                         if (!value)
>                                 return config_error_nonbool(key);
> @@ -700,6 +707,40 @@ struct remote *remote_get(const char *name)
>         return ret;
>  }
>
> +struct remote *pushremote_get(const char *name)
> +{
> +       struct remote *ret;
> +       int name_given = 0;
> +
> +       read_config();
> +       if (name)
> +               name_given = 1;
> +       else {
> +               if (pushremote_name) {
> +                       name = pushremote_name;
> +                       name_given = 1;
> +               } else {
> +                       name = default_remote_name;
> +                       name_given = explicit_default_remote_name;
> +               }
> +       }
> +
> +       ret = make_remote(name, 0);
> +       if (valid_remote_nick(name)) {
> +               if (!valid_remote(ret))
> +                       read_remotes_file(ret);
> +               if (!valid_remote(ret))
> +                       read_branches_file(ret);
> +       }
> +       if (name_given && !valid_remote(ret))
> +               add_url_alias(ret, name);
> +       if (!valid_remote(ret))
> +               return NULL;
> +       ret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);
> +       ret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);
> +       return ret;
> +}
> +
>  int remote_is_configured(const char *name)
>  {
>         int i;
> diff --git a/remote.h b/remote.h
> index 251d8fd..aa42ff5 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -51,6 +51,7 @@ struct remote {
>  };
>
>  struct remote *remote_get(const char *name);
> +struct remote *pushremote_get(const char *name);
>  int remote_is_configured(const char *name);
>
>  typedef int each_remote_fn(struct remote *remote, void *priv);
> @@ -130,6 +131,7 @@ struct branch {
>         const char *refname;
>
>         const char *remote_name;
> +       const char *pushremote_name;
>         struct remote *remote;
>
>         const char **merge_name;
> --
> 1.8.1.2.545.g2f19ada.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ramkumar Ramachandra]

Blind wrote:
> If I understand correctly,
> in your scenario the branches with branch.<name>.pushremote
> will be still included in the $git push <remote> --all?

Yes, this is correct.

> Are you considering some way to exclude a branch from "push --all"
> (branch.<name>.push = always, explicit, never... for example)?

No.  I don't see why push.default is limiting.

> Or maybe, if the branch is already marked as special
> with branch.<name>.pushremote,
> then it could be logical to push it only when is explicitly specified
> on the command line (excluded from --all)?

Huh?  Why would I treat this as a special case?

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

[Blind]

2013/2/19 Ramkumar Ramachandra:
> No.  I don't see why push.default is limiting.

I just want to find a way to exclude a branch (or infact a group of
branches) from $git push --all.
so when I read your thing, I thought for a second that it could be a
possibility... But seems its not the case.

... or branch.<name>.pushremote can support some kind of a "none" value?

Blind.

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

[Ramkumar Ramachandra]

Blind wrote:
> 2013/2/19 Ramkumar Ramachandra:
>> No.  I don't see why push.default is limiting.
>
> I just want to find a way to exclude a branch (or infact a group of
> branches) from $git push --all.
> so when I read your thing, I thought for a second that it could be a
> possibility... But seems its not the case.

What is your usecase?  If you have a local branch with the same name
as on the remote, why wouldn't you want to push-to-update it when you
make changes in the branch?  In other words, why doesn't push.default
= matching suffice for most practical purposes.

> ... or branch.<name>.pushremote can support some kind of a "none" value?

Not a bad idea at all, provided you tell me your usecase.

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

[Blind]

2013/2/19 Ramkumar Ramachandra:
>
> What is your usecase?  If you have a local branch with the same name
> as on the remote, why wouldn't you want to push-to-update it when you
> make changes in the branch?  In other words, why doesn't push.default
> = matching suffice for most practical purposes.
>
>> ... or branch.<name>.pushremote can support some kind of a "none" value?
>
> Not a bad idea at all, provided you tell me your usecase.

The question is infact about branches who are not on the remote.
push.default=matching wouldn't upload the branches which are not there already.
push --all on other hand pushes .. all (as expected :-))...

Its difficult for me to manage quickly, lets say, "news-to-push/*" and
"crazy-ideas/*" branches ...

Blind.

Re: [RFC/PATCH] Introduce remote.pushdefault

[Ramkumar Ramachandra]

Sorry about the horribly late response- I just got around to
re-rolling this, and had a doubt.

Junio C Hamano <gitster@pobox.com> wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9b11597..82a4a78 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1884,6 +1884,10 @@ receive.updateserverinfo::
>>       If set to true, git-receive-pack will run git-update-server-info
>>       after receiving data from git-push and updating refs.
>>
>> +remote.pushdefault::
>> +     The remote to push to by default.  Overrides the
>> +     branch-specific configuration `branch.<name>.remote`.
>
> It feels unexpected to see "I may have said while on this branch I
> push there and on that branch I push somewhere else, but no, with
> this single configuration I'm invalidating all these previous
> statements, and all pushes go to this new place".
>
> Shouldn't the default be the default that is to be overridden by
> other configuration that is more specific?  That is, "I would
> normally push to this remote and unless I say otherwise that is all
> I have to say, but for this particular branch, I push to somehwere
> else".

I'm a little confused as to where this configuration variable will be
useful.  On a fresh clone from Github, I get branch.master.remote
configured to "origin".  How will adding remote.pushdefault have any
impact, unless I explicitly remove this branch-specific remote
configuration?  Besides, without branch.<name>.remote configured, I
can't even pull and expect changes to be merged.  So, really: what is
the use of remote.pushdefault?

I'm dropping this patch, and just going with branch.<name>.pushremote,
unless you convince me otherwise.

Re: [RFC/PATCH] Introduce remote.pushdefault

[Jeff King]

On Sun, Mar 17, 2013 at 06:00:08AM +0530, Ramkumar Ramachandra wrote:

> >> +remote.pushdefault::
> >> +     The remote to push to by default.  Overrides the
> >> +     branch-specific configuration `branch.<name>.remote`.
> >
> > It feels unexpected to see "I may have said while on this branch I
> > push there and on that branch I push somewhere else, but no, with
> > this single configuration I'm invalidating all these previous
> > statements, and all pushes go to this new place".
> >
> > Shouldn't the default be the default that is to be overridden by
> > other configuration that is more specific?  That is, "I would
> > normally push to this remote and unless I say otherwise that is all
> > I have to say, but for this particular branch, I push to somehwere
> > else".
> 
> I'm a little confused as to where this configuration variable will be
> useful.  On a fresh clone from Github, I get branch.master.remote
> configured to "origin".  How will adding remote.pushdefault have any
> impact, unless I explicitly remove this branch-specific remote
> configuration?  Besides, without branch.<name>.remote configured, I
> can't even pull and expect changes to be merged.  So, really: what is
> the use of remote.pushdefault?
> 
> I'm dropping this patch, and just going with branch.<name>.pushremote,
> unless you convince me otherwise.

That is why I described the scheme I did in [1]. It uses the following
two general rules:

  1. Per-branch config trumps repo-wide config.

  2. Push-specific config (e.g., "remote.pushdefault") trumps
     non-specific config (e.g., "remote.default") for pushing.

So the push lookup list is (in order of precedence):

  1. branch.*.pushremote
  2. remote.pushdefault
  3. branch.*.remote
  4. remote.default
  5. origin

and it solves Junio's issue because the way to say "override my
remote.pushdefault for this branch" is not to set "branch.*.remote", but
to set "branch.*.pushremote".

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/215751

Re: [RFC/PATCH] Introduce remote.pushdefault

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> How will adding remote.pushdefault have any
> impact, unless I explicitly remove this branch-specific remote
> configuration?  Besides, without branch.<name>.remote configured, I
> can't even pull and expect changes to be merged.

If the triangle topology is the norm for your project, I would
expect that it would be pretty common to have all of your branches
pull from one place and have all of them push to another place.  In
the central repository workflow, it is very common that all of your
branches pull from one place and all of them push to the same place.

In the bigger picture, forcing to set the branch specific remote is
a mistake in the first place; in the longer term, you should be able
to say "My project uses the central repository workflow" once and
you should be able to pull without branch.master.remote; just record
where the default "origin" for all branches is.

I think the per-branch branch.*.pushremote is actively making things
worse by repeating the same mistake for the triangle topology.  You
should be able to say "My project uses the triangle workflow" once
and specify two remotes, where you pull from and where you push to,
no?

A per-branch override, be it branch.*.remote or brnach.*.pushremote,
is useful to give an escape hatch in order to handle special cases
with additional flexibility.  But making it the sole mechanism and
forcing the user to repeat the same "which remote does it use" all
over the place does not sound like a solid engineering.

Re: [RFC/PATCH] Introduce remote.pushdefault

[Ramkumar Ramachandra]

Jeff King wrote:
> So the push lookup list is (in order of precedence):
>
>   1. branch.*.pushremote
>   2. remote.pushdefault
>   3. branch.*.remote
>   4. remote.default
>   5. origin
>
> and it solves Junio's issue because the way to say "override my
> remote.pushdefault for this branch" is not to set "branch.*.remote", but
> to set "branch.*.pushremote".

Right, thanks for clearing that up Jeff.  I'll re-roll with
remote.pushdefault overriding branch.<name>.remote.

Ram