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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:05

Ramkumar Ramachandra [off-list ref] writes:
quoted hunk
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.
quoted hunk
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.)
quoted hunk
@@ -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 branch.<name>.pushremote

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:56:06

Junio C Hamano wrote:
Ramkumar Ramachandra [off-list ref] writes:
quoted
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.
quoted
+                     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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help