threads / rfc / 32855

RFC patchIntroduce branch.<name>.pushremote

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

## tl;dr

18 messages between Feb 8, 2013 and Mar 18, 2013. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Ramkumar Ramachandra· Feb 8, 2013, 07:19 UTC · lore

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(-)
Show changes to 4 files +50 −1

Documentation/config.txt, builtin/push.c, remote.c, remote.h

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
Junio C Hamano· Feb 8, 2013, 08:21 UTC · re: Ramkumar Ramachandra · lore

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

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.

Ramkumar Ramachandra· Feb 8, 2013, 09:02 UTC · re: Junio C Hamano · lore

[RFC/PATCH] Introduce remote.pushdefault

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(-)
Show changes to 4 files +49 −3

Documentation/config.txt, builtin/push.c, remote.c, remote.h

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
Junio C Hamano· Feb 8, 2013, 20:11 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 11 quoted lines
> 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".

Show 46 quoted lines
> 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",
Ramkumar Ramachandra· Feb 9, 2013, 07:50 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

Junio C Hamano wrote:
Show 24 quoted lines
> 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.
Show 51 quoted lines
>> 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.
Ramkumar Ramachandra· Mar 17, 2013, 00:30 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

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:
Show 24 quoted lines
> 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.

Jeff King· Mar 17, 2013, 05:48 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

On Sun, Mar 17, 2013 at 06:00:08AM +0530, Ramkumar Ramachandra wrote:
Show 25 quoted lines
> >> +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
Ramkumar Ramachandra· Mar 18, 2013, 09:02 UTC · re: Jeff King · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

Jeff King wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Mar 17, 2013, 06:10 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC/PATCH] Introduce remote.pushdefault

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.

Junio C Hamano· Feb 8, 2013, 20:04 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 13 quoted lines
> 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.

Show 20 quoted lines
> 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.)
Show 61 quoted lines
> @@ -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;
Ramkumar Ramachandra· Feb 9, 2013, 07:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 24 quoted lines
> 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.

Show 11 quoted lines
>> +                     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.
Junio C Hamano· Feb 9, 2013, 20:00 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 11 quoted lines
> 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?
			
Ramkumar Ramachandra· Feb 10, 2013, 15:41 UTC · re: Junio C Hamano · lore

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

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.

Blind· Feb 11, 2013, 06:16 UTC · re: Ramkumar Ramachandra · lore

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

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>:
Show 149 quoted lines
> 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
Ramkumar Ramachandra· Feb 19, 2013, 09:27 UTC · re: Blind · lore

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

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?
Blind· Feb 19, 2013, 11:18 UTC · re: Ramkumar Ramachandra · lore

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

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.
Ramkumar Ramachandra· Feb 19, 2013, 11:30 UTC · re: Blind · lore

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

Blind wrote:
Show 7 quoted lines
> 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.
Blind· Feb 19, 2013, 11:51 UTC · re: Ramkumar Ramachandra · lore

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

2013/2/19 Ramkumar Ramachandra:
Show 9 quoted lines
>
> 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.

← back to recent threads