[PATCH 0/4] Support triangular workflows

[PATCH 0/4] Support triangular workflows

[Ramkumar Ramachandra]

Hi,

This series follows up a previous discussion with Junio and Jeff [1].
It attempts to support the triangular workflow, where the remote
you're fetching from is not the same as the remote you're pushing to.
`remote.<name>.pushurl` has already been discussed, and deemed as a
poor solution to the problem [2].

[1/4] is a minor cleanup patch to make other patches consistent with
the existing style.

[2/4] introduces the infrastructure needed to allow [3/4] and [4/4] to
be simple configuration-adding patches.

[3/4] and [4/4] add the proposed configuration options.  They're very
simple patches, but the documentation is not so simple: I've
documented all the side-effects of the other configuration option in
each configuration option, to give the reader a comprehensive picture
when reading one configuration option.

I've put off implementing remote.default corresponding to
remote.pushdefault, as Jeff suggested in [1], because it's currently
not an itch; apart from the obvious symmetry, I don't know what
purpose it serves: why would anyone want to fetch from a remote other
than origin by default?  Why wouldn't they simply swap that remote's
name with "origin"?  However, it's a nice thing to have for symmetry,
and it should be trivial to implement: any interested person is
welcome to pick it up.

The series works as expected, and all tests pass.

Thanks for reading.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/215763
[2]: http://thread.gmane.org/gmane.comp.version-control.git/215702/focus=215717

Ramkumar Ramachandra (4):
  remote.c: simply a bit of code using git_config_string()
  remote.c: introduce a way to have different remotes for fetch/ push
  remote.c: introduce remote.pushdefault
  remote.c: introduce branch.<name>.pushremote

 Documentation/config.txt | 23 ++++++++++++++++---
 builtin/push.c           |  2 +-
 remote.c                 | 60 +++++++++++++++++++++++++++++++++++-------------
 remote.h                 |  1 +
 4 files changed, 66 insertions(+), 20 deletions(-)

-- 
1.8.2

[PATCH 1/4] remote.c: simply a bit of code using git_config_string()

[Ramkumar Ramachandra]

A small segment where handle_config() parses the branch.remote
configuration variable can be simplified using git_config_string().

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e53a6eb..45b69d6 100644
--- a/remote.c
+++ b/remote.c
@@ -356,9 +356,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			return 0;
 		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
-			if (!value)
-				return config_error_nonbool(key);
-			branch->remote_name = xstrdup(value);
+			git_config_string(&branch->remote_name, key, value);
 			if (branch == current_branch) {
 				default_remote_name = branch->remote_name;
 				explicit_default_remote_name = 1;
-- 
1.8.2

Re: [PATCH 1/4] remote.c: simply a bit of code using git_config_string()

[Eric Sunshine]

On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> remote.c: simply a bit of code using git_config_string()

s/simply/simplify/

> A small segment where handle_config() parses the branch.remote
> configuration variable can be simplified using git_config_string().
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

[PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push

[Ramkumar Ramachandra]

Currently, do_push() in push.c calls remote_get(), which gets the
configured remote for fetching and pushing.  Replace this call with a
call to pushremote_get() instead, a new function that will return the
remote configured specifically for pushing.  This function tries to
work with the string pushremote_name, before falling back to the
codepath of remote_get().  This patch has no visible impact, but
serves to enable future patches to introduce configuration variables
to set this variable.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/push.c |  2 +-
 remote.c       | 49 ++++++++++++++++++++++++++++++++++++-------------
 remote.h       |  1 +
 3 files changed, 38 insertions(+), 14 deletions(-)

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 45b69d6..4704404 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;
@@ -669,20 +670,9 @@ static int valid_remote_nick(const char *name)
 	return !strchr(name, '/'); /* no slash */
 }
 
-struct remote *remote_get(const char *name)
+static struct remote *remote_get_1(const char *name, int name_given)
 {
-	struct remote *ret;
-	int name_given = 0;
-
-	read_config();
-	if (name)
-		name_given = 1;
-	else {
-		name = default_remote_name;
-		name_given = explicit_default_remote_name;
-	}
-
-	ret = make_remote(name, 0);
+	struct remote *ret = make_remote(name, 0);
 	if (valid_remote_nick(name)) {
 		if (!valid_remote(ret))
 			read_remotes_file(ret);
@@ -698,6 +688,39 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+struct remote *remote_get(const char *name)
+{
+	int name_given = 0;
+
+	read_config();
+	if (name)
+		name_given = 1;
+	else {
+		name = default_remote_name;
+		name_given = explicit_default_remote_name;
+	}
+	return remote_get_1(name, name_given);
+}
+
+struct remote *pushremote_get(const char *name)
+{
+	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;
+		}
+	}
+	return remote_get_1(name, name_given);
+}
+
 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.2

Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push

[Jeff King]

On Mon, Mar 18, 2013 at 06:46:13PM +0530, Ramkumar Ramachandra wrote:

> +struct remote *remote_get(const char *name)
> +{
> +	int name_given = 0;
> +
> +	read_config();
> +	if (name)
> +		name_given = 1;
> +	else {
> +		name = default_remote_name;
> +		name_given = explicit_default_remote_name;
> +	}
> +	return remote_get_1(name, name_given);
> +}
> +
> +struct remote *pushremote_get(const char *name)
> +{
> +	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;
> +		}
> +	}
> +	return remote_get_1(name, name_given);
> +}

Can we get rid of this duplication by having remote_get_1 take a
service-specific default argument? And then each service calls it like:

  struct remote *remote_get(const char *name)
  {
          read_config();
          return remote_get_1(name, NULL);
  }

  struct remote *pushremote_get(const char *name)
  {
          read_config();
          return remote_get_1(name, pushremote_name);
  }

and all of the name_given junk can stay in remote_get_1. And adding
"remote.default" would just be a matter of changing that NULL in
remote_get.

-Peff

Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push

[Ramkumar Ramachandra]

Jeff King wrote:
> On Mon, Mar 18, 2013 at 06:46:13PM +0530, Ramkumar Ramachandra wrote:
>
>> +struct remote *remote_get(const char *name)
>> +{
>> +     int name_given = 0;
>> +
>> +     read_config();
>> +     if (name)
>> +             name_given = 1;
>> +     else {
>> +             name = default_remote_name;
>> +             name_given = explicit_default_remote_name;
>> +     }
>> +     return remote_get_1(name, name_given);
>> +}
>> +
>> +struct remote *pushremote_get(const char *name)
>> +{
>> +     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;
>> +             }
>> +     }
>> +     return remote_get_1(name, name_given);
>> +}
>
> Can we get rid of this duplication by having remote_get_1 take a
> service-specific default argument? And then each service calls it like:
>
>   struct remote *remote_get(const char *name)
>   {
>           read_config();
>           return remote_get_1(name, NULL);
>   }
>
>   struct remote *pushremote_get(const char *name)
>   {
>           read_config();
>           return remote_get_1(name, pushremote_name);
>   }

Thanks for the dose of sanity.  While at it, why not move
read_config() to remote_get_1() as well?

Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push

[Jeff King]

On Mon, Mar 18, 2013 at 08:26:46PM +0530, Ramkumar Ramachandra wrote:

> > Can we get rid of this duplication by having remote_get_1 take a
> > service-specific default argument? And then each service calls it like:
> >
> >   struct remote *remote_get(const char *name)
> >   {
> >           read_config();
> >           return remote_get_1(name, NULL);
> >   }
> >
> >   struct remote *pushremote_get(const char *name)
> >   {
> >           read_config();
> >           return remote_get_1(name, pushremote_name);
> >   }
> 
> Thanks for the dose of sanity.  While at it, why not move
> read_config() to remote_get_1() as well?

Because it sets pushremote_name, and therefore you would just be passing
NULL if read_config has not been run yet. But if you made it:

  return remote_get_1(name, &pushremote_name);

that would work.

-Peff

Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push

[Eric Sunshine]

On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> remote.c: introduce a way to have different remotes for fetch/ push

s/ push/push/

> Currently, do_push() in push.c calls remote_get(), which gets the
> configured remote for fetching and pushing.  Replace this call with a
> call to pushremote_get() instead, a new function that will return the
> remote configured specifically for pushing.  This function tries to
> work with the string pushremote_name, before falling back to the
> codepath of remote_get().  This patch has no visible impact, but
> serves to enable future patches to introduce configuration variables
> to set this variable.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

[PATCH 3/4] remote.c: introduce remote.pushdefault

[Ramkumar Ramachandra]

This new configuration variable defines the default remote to push to,
and overrides `branch.<name>.remote` for all branches.  It is useful
in the typical triangular-workflow setup, where the remote you're
fetching from is different from the remote you're pushing to.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 13 ++++++++++---
 remote.c                 |  4 ++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..8ddd0fd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -723,9 +723,12 @@ branch.autosetuprebase::
 	This option defaults to never.
 
 branch.<name>.remote::
-	When in branch <name>, it tells 'git fetch' and 'git push' which
-	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.
+	When on branch <name>, it tells 'git fetch' and 'git push'
+	which remote to fetch from/push to.  The remote to push to
+	may be overriden with `remote.pushdefault` (for all branches).
+	If no remote is configured, or if you are not on any branch,
+	it defaults to `origin` for fetching and `remote.pushdefault`
+	for pushing.
 
 branch.<name>.merge::
 	Defines, together with branch.<name>.remote, the upstream branch
@@ -1894,6 +1897,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
+	`branch.<name>.remote` for all branches.
+
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
 	linkgit:git-push[1].
diff --git a/remote.c b/remote.c
index 4704404..987edc4 100644
--- a/remote.c
+++ b/remote.c
@@ -350,6 +350,10 @@ 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.")) {
+		if (!strcmp(key + 7, "pushdefault"))
+			git_config_string(&pushremote_name, key, value);
+	}
 	if (!prefixcmp(key, "branch.")) {
 		name = key + 7;
 		subkey = strrchr(name, '.');
-- 
1.8.2

Re: [PATCH 3/4] remote.c: introduce remote.pushdefault

[Eric Sunshine]

On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
>  branch.<name>.remote::
> -       When in branch <name>, it tells 'git fetch' and 'git push' which
> -       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.
> +       When on branch <name>, it tells 'git fetch' and 'git push'
> +       which remote to fetch from/push to.  The remote to push to
> +       may be overriden with `remote.pushdefault` (for all branches).

s/overriden/overridden/

> +       If no remote is configured, or if you are not on any branch,
> +       it defaults to `origin` for fetching and `remote.pushdefault`
> +       for pushing.

[PATCH 4/4] remote.c: introduce branch.<name>.pushremote

[Ramkumar Ramachandra]

This new configuration variable overrides `remote.pushdefault` and
`branch.<name>.remote` for pushes.  In a typical triangular-workflow
setup, you would want to set `remote.pushdefault` to specify the
remote to push to for all branches, and use this option to override it
for a specific branch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++++++----
 remote.c                 |  3 +++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8ddd0fd..d0e36e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -726,9 +726,18 @@ branch.<name>.remote::
 	When on branch <name>, it tells 'git fetch' and 'git push'
 	which remote to fetch from/push to.  The remote to push to
 	may be overriden with `remote.pushdefault` (for all branches).
-	If no remote is configured, or if you are not on any branch,
-	it defaults to `origin` for fetching and `remote.pushdefault`
-	for pushing.
+	The remote to push to, for the current branch, may be further
+	overriden by `branch.<name>.pushremote`.  If no remote is
+	configured, or if you are not on any branch, it defaults to
+	`origin` for fetching and `remote.pushdefault` for pushing.
+
+branch.<name>.pushremote::
+	When on branch <name>, it overrides `branch.<name>.remote`
+	when pushing.  It also overrides `remote.pushdefault` when
+	pushing from branch <name>.  In a typical triangular-workflow
+	setup, you would want to set `remote.pushdefault` to specify
+	the remote to push to for all branches, and use this option to
+	override it for a specific branch.
 
 branch.<name>.merge::
 	Defines, together with branch.<name>.remote, the upstream branch
@@ -1899,7 +1908,8 @@ receive.updateserverinfo::
 
 remote.pushdefault::
 	The remote to push to by default.  Overrides
-	`branch.<name>.remote` for all branches.
+	`branch.<name>.remote` for all branches, and is overriden by
+	`branch.<name>.pushremote` for specific branches.
 
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
diff --git a/remote.c b/remote.c
index 987edc4..a4d3d22 100644
--- a/remote.c
+++ b/remote.c
@@ -366,6 +366,9 @@ 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 (branch == current_branch)
+				git_config_string(&pushremote_name, key, value);
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
 				return config_error_nonbool(key);
-- 
1.8.2

Re: [PATCH 4/4] remote.c: introduce branch.<name>.pushremote

[Eric Sunshine]

On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> -       If no remote is configured, or if you are not on any branch,
> -       it defaults to `origin` for fetching and `remote.pushdefault`
> -       for pushing.
> +       The remote to push to, for the current branch, may be further
> +       overriden by `branch.<name>.pushremote`.  If no remote is

s/overriden/overridden/

> +       configured, or if you are not on any branch, it defaults to
> +       `origin` for fetching and `remote.pushdefault` for pushing.
> +
>  remote.pushdefault::
>         The remote to push to by default.  Overrides
> -       `branch.<name>.remote` for all branches.
> +       `branch.<name>.remote` for all branches, and is overriden by

Ditto: s/overriden/overridden/

> +       `branch.<name>.pushremote` for specific branches.

Re: [PATCH 0/4] Support triangular workflows

[Jeff King]

On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote:

> I've put off implementing remote.default corresponding to
> remote.pushdefault, as Jeff suggested in [1], because it's currently
> not an itch; apart from the obvious symmetry, I don't know what
> purpose it serves: why would anyone want to fetch from a remote other
> than origin by default?  Why wouldn't they simply swap that remote's
> name with "origin"?  However, it's a nice thing to have for symmetry,
> and it should be trivial to implement: any interested person is
> welcome to pick it up.

Yeah, I agree that it does not have much point, aside from people who
have an unreasonable aversion to using the word "origin". There was a
series posted last summer to add remote.default:

  http://article.gmane.org/gmane.comp.version-control.git/201065

It ended up stalled and never got merged. I think the main impetus was
that "git clone -o foo" should leave "foo" in remote.default (of course,
that still leaves unanswered why anyone would really want to use "-o
foo" in the first place).

I think the symmetry makes some sense, but I also think it can come
later if somebody wants it.

>  Documentation/config.txt | 23 ++++++++++++++++---
>  builtin/push.c           |  2 +-
>  remote.c                 | 60 +++++++++++++++++++++++++++++++++++-------------
>  remote.h                 |  1 +
>  4 files changed, 66 insertions(+), 20 deletions(-)

No new tests?

-Peff

Re: [PATCH 0/4] Support triangular workflows

[Ramkumar Ramachandra]

Jeff King wrote:
> On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote:
>>  Documentation/config.txt | 23 ++++++++++++++++---
>>  builtin/push.c           |  2 +-
>>  remote.c                 | 60 +++++++++++++++++++++++++++++++++++-------------
>>  remote.h                 |  1 +
>>  4 files changed, 66 insertions(+), 20 deletions(-)
>
> No new tests?

Honestly, it slipped my mind.  Will write it now.

Thanks for the reminder.

Re: [PATCH 0/4] Support triangular workflows

[Jeff King]

On Mon, Mar 18, 2013 at 07:58:23PM +0530, Ramkumar Ramachandra wrote:

> Jeff King wrote:
> > On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote:
> >>  Documentation/config.txt | 23 ++++++++++++++++---
> >>  builtin/push.c           |  2 +-
> >>  remote.c                 | 60 +++++++++++++++++++++++++++++++++++-------------
> >>  remote.h                 |  1 +
> >>  4 files changed, 66 insertions(+), 20 deletions(-)
> >
> > No new tests?
> 
> Honestly, it slipped my mind.  Will write it now.
> 
> Thanks for the reminder.

Thanks. Other than the suggestion I made on 2/4, I do not see anything
wrong with the series.

-Peff

Re: [PATCH 0/4] Support triangular workflows

[Marc Branchaud]

On 13-03-18 10:25 AM, Jeff King wrote:
> On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote:
> 
>> I've put off implementing remote.default corresponding to
>> remote.pushdefault, as Jeff suggested in [1], because it's currently
>> not an itch; apart from the obvious symmetry, I don't know what
>> purpose it serves: why would anyone want to fetch from a remote other
>> than origin by default?  Why wouldn't they simply swap that remote's
>> name with "origin"?  However, it's a nice thing to have for symmetry,
>> and it should be trivial to implement: any interested person is
>> welcome to pick it up.
> 
> Yeah, I agree that it does not have much point, aside from people who
> have an unreasonable aversion to using the word "origin". There was a
> series posted last summer to add remote.default:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/201065
> 
> It ended up stalled and never got merged. I think the main impetus was
> that "git clone -o foo" should leave "foo" in remote.default (of course,
> that still leaves unanswered why anyone would really want to use "-o
> foo" in the first place).

I'm the guy who dropped the ball on that series.  I still intend to pick it
up (honest!) but I just haven't had the time.

The impetus was originally getting relative submodule paths to work on
detached HEADs [1].  My patch for that doesn't work when someone does "clone
-o", because various parts of git assume there's a remote named "origin".
The discussion led to the idea of using the remote name specified during the
initial clone, and implementing that as a remote.default config value.

As for why "clone -o" exists, it was added in v1.1.0:

    commit e6c310fd0d7384973efc6b1d5999a5e8a5b2f3bd
    Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    Date:   Thu Dec 22 23:37:24 2005 +0100

    git-clone: Support changing the origin branch with -o

    Earlier, git-clone stored upstream's master in the branch named 'origin',
    possibly overwriting an existing such branch.

    Now you can change it by calling git-clone with '-o <other_name>'.

    [jc: added ref format check, subdirectory safety, documentation
     and usage string.]

    Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <junkio@cox.net>

It sounds like the original need for the -o option is no longer pertinent.
OTOH, for folks who deal with several remotes it's nice to name them, and
"origin" isn't necessarily a useful or intuitive name.

		M.

[1] http://thread.gmane.org/gmane.comp.version-control.git/200145