Re: [PATCH 1/2] Add for_each_remote() function, and extend remote_find_tracking()

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

Re: [PATCH 1/2] Add for_each_remote() function, and extend remote_find_tracking()

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:20

Johannes Schindelin [off-list ref] writes:
The function for_each_remote() does exactly what the name suggests.

The function remote_find_tracking() was extended to be able to search
remote refs for a given local ref.  You have to set the parameter
"reverse" to true for that behavior.

Both changes are required for the next step: simplification of
git-branch's --track functionality.

Signed-off-by: Johannes Schindelin <redacted>
---

	You're right. I completely missed that functionality. Well, a
	few tweaks were needed. If this clashes too seriously with
	Daniel's work, I will gladly redo it after his changes are
	in "next".
No offence meant to Daniel, but I am inclined to postpone the
current round of changes from him to move the stuff further
to get us closer to built-in git-fetch until 1.5.3 final is
done.  The amount of C code changes otherwise would be a bit too
much for me to be comfortable between -rc0 and -rc1.
quoted hunk
diff --git a/remote.c b/remote.c
index cf98a44..21adb0d 100644
--- a/remote.c
+++ b/remote.c
@@ -279,6 +279,26 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+int for_each_remote(each_remote_fn fn, void *priv)
+{
...
+		if ((result = fn(r, priv)))
+			break;
Just a minor style, but (you know what comes)...
quoted hunk
@@ -289,34 +309,36 @@ int remote_has_uri(struct remote *remote, const char *uri)
 	return 0;
 }
 
+int remote_find_tracking(struct remote *remote, struct refspec *refspec,
+		int reverse)
 {
 	int i;
 	for (i = 0; i < remote->fetch_refspec_nr; i++) {
 		struct refspec *fetch = &remote->fetch[i];
+		const char *src = reverse ? fetch->dst : fetch->src;
+		const char *dst = reverse ? fetch->src : fetch->dst;
I have to agree with Daniel here --- variable names src and dst
are quite confusing.  It seems to mean that "we search with
'src' to fill 'dst', but if reverse incoming refspec is given
reversed so matching refspec->src with what in fact is 'dst' in
the configuration file is fine".  Utterly confusing.

Even though this is a good opportunity for you to improve your
comment ratio in ohloh stats, I doubt any amount of explanation
can unconfuse readers of this code.
 		if (!fetch->dst)
 			continue;

 		if (fetch->pattern) {
+			if (!prefixcmp(refspec->src, src)) {
 				refspec->dst =
+					xmalloc(strlen(dst) +
 						strlen(refspec->src) -
+						strlen(src) + 1);
+				strcpy(refspec->dst, dst);
+				strcpy(refspec->dst + strlen(dst),
+				       refspec->src + strlen(src));
 				refspec->force = fetch->force;
 				return 0;
 			}
 		} else {
+			if (!strcmp(refspec->src, src)) {
+				refspec->dst = xstrdup(dst);
 				refspec->force = fetch->force;
 				return 0;
 			}
 		}
 	}
-	refspec->dst = NULL;
 	return -1;
 }
The original remote_find_tracking() took a single remote and a
refspec with src filled, and returned the given refspec after
filling its dst, if an appropriate tracking was configured for
the remote.  What you want to do is from the same remote
information and a refspec with dst filled to find a src branch
that would be stored to that dst.

In either case, incoming refspec would not have both src and dst
filled.  The caller has one side of the information and asking
for the other.  As Daniel suggests, the 'reverse' parameter is
not needed.  If you must have it, please do not call it
'reverse' -- it is more like "find_by_dst".

Following Daniel's suggestion might make the code a bit
lengthier, but I think that would not be as confusing.
quoted hunk
diff --git a/send-pack.c b/send-pack.c
index fecbda9..9fdd7b4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -305,8 +305,7 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 		if (remote) {
 			struct refspec rs;
 			rs.src = ref->name;
-			remote_find_tracking(remote, &rs);
-			if (rs.dst) {
+			if (!remote_find_tracking(remote, &rs, 0)) {
 				struct ref_lock *lock;
 				fprintf(stderr, " Also local %s\n", rs.dst);
 				if (will_delete_ref) {

Re: [PATCH 1/2] Add for_each_remote() function, and extend remote_find_tracking()

From: Daniel Barkalow <hidden>
Date: 2016-06-15 22:43:20

On Mon, 9 Jul 2007, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
	You're right. I completely missed that functionality. Well, a
	few tweaks were needed. If this clashes too seriously with
	Daniel's work, I will gladly redo it after his changes are
	in "next".
No offence meant to Daniel, but I am inclined to postpone the
current round of changes from him to move the stuff further
to get us closer to built-in git-fetch until 1.5.3 final is
done.  The amount of C code changes otherwise would be a bit too
much for me to be comfortable between -rc0 and -rc1.
That's what I'd expect; I'm posting stuff now so that I'm not proposing it 
unreviewed after 1.5.3. Certainly anything that's needed to fix current 
issues should go ahead of these changes, and it doesn't look like there 
would be any conflicts anyway, aside from maybe adding two functions in 
the same place in the file, which is trivial to fix by hand.

	-Daniel
*This .sig left intentionally blank*

[PATCH v2 1/2] Add for_each_remote() function, and extend remote_find_tracking()

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:20

The function for_each_remote() does exactly what the name suggests.

The function remote_find_tracking() was extended to be able to search
remote refs for a given local ref.  You have to set the parameter
"reverse" to true for that behavior.

Both changes are required for the next step: simplification of
git-branch's --track functionality.

Signed-off-by: Johannes Schindelin <redacted>
---

	Thanks, Daniel and Junio, for your suggestions.

 remote.c    |   60 ++++++++++++++++++++++++++++++++++++++++++++--------------
 remote.h    |    5 +++-
 send-pack.c |    4 +-
 3 files changed, 51 insertions(+), 18 deletions(-)
diff --git a/remote.c b/remote.c
index 09c4279..bb774d0 100644
--- a/remote.c
+++ b/remote.c
@@ -279,6 +279,25 @@ struct remote *remote_get(const char *name)
 	return ret;
 }
 
+int for_each_remote(each_remote_fn fn, void *priv)
+{
+	int i, result = 0;
+	read_config();
+	for (i = 0; i < allocated_remotes && !result; i++) {
+		struct remote *r = remotes[i];
+		if (!r)
+			continue;
+		if (!r->fetch)
+			r->fetch = parse_ref_spec(r->fetch_refspec_nr,
+					r->fetch_refspec);
+		if (!r->push)
+			r->push = parse_ref_spec(r->push_refspec_nr,
+					r->push_refspec);
+		result = fn(r, priv);
+	}
+	return result;
+}
+
 int remote_has_uri(struct remote *remote, const char *uri)
 {
 	int i;
@@ -291,32 +310,43 @@ int remote_has_uri(struct remote *remote, const char *uri)
 
 int remote_find_tracking(struct remote *remote, struct refspec *refspec)
 {
+	int find_src = refspec->src == NULL;
+	char *needle, **result;
 	int i;
+
+	if (find_src) {
+		if (refspec->dst == NULL)
+			return error("find_tracking: need either src or dst");
+		needle = refspec->dst;
+		result = &refspec->src;
+	} else {
+		needle = refspec->src;
+		result = &refspec->dst;
+	}
+
 	for (i = 0; i < remote->fetch_refspec_nr; i++) {
 		struct refspec *fetch = &remote->fetch[i];
+		const char *key = find_src ? fetch->dst : fetch->src;
+		const char *value = find_src ? fetch->src : fetch->dst;
 		if (!fetch->dst)
 			continue;
 		if (fetch->pattern) {
-			if (!prefixcmp(refspec->src, fetch->src)) {
-				refspec->dst =
-					xmalloc(strlen(fetch->dst) +
-						strlen(refspec->src) -
-						strlen(fetch->src) + 1);
-				strcpy(refspec->dst, fetch->dst);
-				strcpy(refspec->dst + strlen(fetch->dst),
-				       refspec->src + strlen(fetch->src));
-				refspec->force = fetch->force;
-				return 0;
-			}
-		} else {
-			if (!strcmp(refspec->src, fetch->src)) {
-				refspec->dst = xstrdup(fetch->dst);
+			if (!prefixcmp(needle, key)) {
+				*result = xmalloc(strlen(value) +
+						  strlen(needle) -
+						  strlen(key) + 1);
+				strcpy(*result, value);
+				strcpy(*result + strlen(value),
+				       needle + strlen(key));
 				refspec->force = fetch->force;
 				return 0;
 			}
+		} else if (!strcmp(needle, key)) {
+			*result = xstrdup(value);
+			refspec->force = fetch->force;
+			return 0;
 		}
 	}
-	refspec->dst = NULL;
 	return -1;
 }
 
diff --git a/remote.h b/remote.h
index 080b7da..17b8b5b 100644
--- a/remote.h
+++ b/remote.h
@@ -20,13 +20,16 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 
+typedef int each_remote_fn(struct remote *remote, void *priv);
+int for_each_remote(each_remote_fn fn, void *priv);
+
 int remote_has_uri(struct remote *remote, const char *uri);
 
 struct refspec {
 	unsigned force : 1;
 	unsigned pattern : 1;
 
-	const char *src;
+	char *src;
 	char *dst;
 };
 
diff --git a/send-pack.c b/send-pack.c
index fecbda9..9fc8a81 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -305,8 +305,8 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 		if (remote) {
 			struct refspec rs;
 			rs.src = ref->name;
-			remote_find_tracking(remote, &rs);
-			if (rs.dst) {
+			rs.dst = NULL;
+			if (!remote_find_tracking(remote, &rs)) {
 				struct ref_lock *lock;
 				fprintf(stderr, " Also local %s\n", rs.dst);
 				if (will_delete_ref) {
-- 
1.5.3.rc0.2783.gf3f7

[PATCH v2 2/2] branch --track: code cleanup and saner handling of local branches

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:20

This patch cleans up some complicated code, and replaces it with a
cleaner version, using code from remote.[ch], which got extended a
little in the process.  This also enables us to fix two cases:

The earlier "fix" to setup tracking only when the original ref started 
with "refs/remotes" is wrong.  You are absolutely allowed to use a 
separate layout for your tracking branches.  The correct fix, of course, 
is to set up tracking information only when there is a matching 
remote.<nick>.fetch line containing a colon.

Another corner case was not handled properly.  If two remotes write to
the original ref, just warn the user and do not set up tracking.

Signed-off-by: Johannes Schindelin <redacted>
---

	Sorry, but I really do not feel like manually making the patch
	nicer to read today.  It is really annoying to me to see the
	empty and the not-really-interesting lines match up.  Oh, well.

 builtin-branch.c  |  170 +++++++++++++++++------------------------------------
 t/t3200-branch.sh |   21 ++++---
 2 files changed, 66 insertions(+), 125 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 49195a1..d16d3f2 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "commit.h"
 #include "builtin.h"
+#include "remote.h"
 
 static const char builtin_branch_usage[] =
   "git-branch [-r] (-d | -D) <branchname> | [--track | --no-track] [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length> | --no-abbrev]] [--sort-by-date]";
@@ -22,7 +23,7 @@ static const char builtin_branch_usage[] =
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_track = 1; /* 0 = none, 1 = remotes, 2 = all */
+static int branch_track = 1;
 
 static int branch_use_color;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -66,12 +67,8 @@ static int git_branch_config(const char *var, const char *value)
 		color_parse(value, var, branch_colors[slot]);
 		return 0;
 	}
-	if (!strcmp(var, "branch.autosetupmerge")) {
-		if (!strcmp(value, "all"))
-			branch_track = 2;
-		else
+	if (!strcmp(var, "branch.autosetupmerge"))
 			branch_track = git_config_bool(var, value);
-	}
 
 	return git_default_config(var, value);
 }
@@ -349,125 +346,70 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev,
 	free_ref_list(&ref_list);
 }
 
-static char *config_repo;
-static char *config_remote;
-static const char *start_ref;
+struct tracking {
+	struct refspec spec;
+	char *src;
+	const char *remote;
+	int matches;
+};
 
-static int get_remote_branch_name(const char *value)
+static int find_tracked_branch(struct remote *remote, void *priv)
 {
-	const char *colon;
-	const char *end;
+	struct tracking *tracking = priv;
 
-	if (*value == '+')
-		value++;
-
-	colon = strchr(value, ':');
-	if (!colon)
-		return 0;
-
-	end = value + strlen(value);
-
-	/*
-	 * Try an exact match first.  I.e. handle the case where the
-	 * value is "$anything:refs/foo/bar/baz" and start_ref is exactly
-	 * "refs/foo/bar/baz". Then the name at the remote is $anything.
-	 */
-	if (!strcmp(colon + 1, start_ref)) {
-		/* Truncate the value before the colon. */
-		nfasprintf(&config_repo, "%.*s", colon - value, value);
-		return 1;
+	if (!remote_find_tracking(remote, &tracking->spec)) {
+		if (++tracking->matches == 1) {
+			tracking->src = tracking->spec.src;
+			tracking->remote = remote->name;
+		} else {
+			free(tracking->spec.src);
+			if (tracking->src) {
+				free(tracking->src);
+				tracking->src = NULL;
+			}
+		}
+		tracking->spec.src = NULL;
 	}
 
-	/*
-	 * Is this a wildcard match?
-	 */
-	if ((end - 2 <= value) || end[-2] != '/' || end[-1] != '*' ||
-	    (colon - 2 <= value) || colon[-2] != '/' || colon[-1] != '*')
-		return 0;
-
-	/*
-	 * Value is "refs/foo/bar/<asterisk>:refs/baz/boa/<asterisk>"
-	 * and start_ref begins with "refs/baz/boa/"; the name at the
-	 * remote is refs/foo/bar/ with the remaining part of the
-	 * start_ref.  The length of the prefix on the RHS is (end -
-	 * colon - 2), including the slash immediately before the
-	 * asterisk.
-	 */
-	if ((strlen(start_ref) < end - colon - 2) ||
-	    memcmp(start_ref, colon + 1, end - colon - 2))
-		return 0; /* does not match prefix */
-
-	/* Replace the asterisk with the remote branch name.  */
-	nfasprintf(&config_repo, "%.*s%s",
-		   (colon - 1) - value, value,
-		   start_ref + (end - colon - 2));
-	return 1;
-}
-
-static int get_remote_config(const char *key, const char *value)
-{
-	const char *var;
-	if (prefixcmp(key, "remote."))
-		return 0;
-
-	var = strrchr(key, '.');
-	if (var == key + 6 || strcmp(var, ".fetch"))
-		return 0;
-	/*
-	 * Ok, we are looking at key == "remote.$foo.fetch";
-	 */
-	if (get_remote_branch_name(value))
-		nfasprintf(&config_remote, "%.*s", var - (key + 7), key + 7);
-
 	return 0;
 }
 
-static void set_branch_merge(const char *name, const char *config_remote,
-			     const char *config_repo)
+
+/*
+ * This is called when new_ref is branched off of orig_ref, and tries
+ * to infer the settings for branch.<new_ref>.{remote,merge} from the
+ * config.
+ */
+static int setup_tracking(const char *new_ref, const char *orig_ref)
 {
 	char key[1024];
-	if (sizeof(key) <=
-	    snprintf(key, sizeof(key), "branch.%s.remote", name))
-		die("what a long branch name you have!");
-	git_config_set(key, config_remote);
-
-	/*
-	 * We do not have to check if we have enough space for
-	 * the 'merge' key, since it's shorter than the
-	 * previous 'remote' key, which we already checked.
-	 */
-	snprintf(key, sizeof(key), "branch.%s.merge", name);
-	git_config_set(key, config_repo);
-}
+	struct tracking tracking;
 
-static void set_branch_defaults(const char *name, const char *real_ref)
-{
-	/*
-	 * name is the name of new branch under refs/heads;
-	 * real_ref is typically refs/remotes/$foo/$bar, where
-	 * $foo is the remote name (there typically are no slashes)
-	 * and $bar is the branch name we map from the remote
-	 * (it could have slashes).
-	 */
-	start_ref = real_ref;
-	git_config(get_remote_config);
-	if (!config_repo && !config_remote &&
-	    !prefixcmp(real_ref, "refs/heads/")) {
-		set_branch_merge(name, ".", real_ref);
-		printf("Branch %s set up to track local branch %s.\n",
-		       name, real_ref);
-	}
+	if (strlen(new_ref) > 1024 - 7 - 7 - 1)
+		return error("Tracking not set up: name too long: %s",
+				new_ref);
 
-	if (config_repo && config_remote) {
-		set_branch_merge(name, config_remote, config_repo);
+	memset(&tracking, 0, sizeof(tracking));
+	tracking.spec.dst = (char *)orig_ref;
+	if (for_each_remote(find_tracked_branch, &tracking) ||
+			!tracking.matches)
+		return 1;
+
+	if (tracking.matches > 1)
+		return error("Not tracking: ambiguous information for ref %s",
+				orig_ref);
+
+	if (tracking.matches == 1) {
+		sprintf(key, "branch.%s.remote", new_ref);
+		git_config_set(key, tracking.remote ?  tracking.remote : ".");
+		sprintf(key, "branch.%s.merge", new_ref);
+		git_config_set(key, tracking.src);
+		free(tracking.src);
 		printf("Branch %s set up to track remote branch %s.\n",
-		       name, real_ref);
+			       new_ref, orig_ref);
 	}
 
-	if (config_repo)
-		free(config_repo);
-	if (config_remote)
-		free(config_remote);
+	return 0;
 }
 
 static void create_branch(const char *name, const char *start_name,
@@ -529,10 +471,8 @@ static void create_branch(const char *name, const char *start_name,
 	/* When branching off a remote branch, set up so that git-pull
 	   automatically merges from there.  So far, this is only done for
 	   remotes registered via .git/config.  */
-	if (real_ref && (track == 2 ||
-				(track == 1 &&
-				 !prefixcmp(real_ref, "refs/remotes/"))))
-		set_branch_defaults(name, real_ref);
+	if (real_ref && track)
+		setup_tracking(name, real_ref);
 
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
@@ -604,7 +544,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			break;
 		}
 		if (!strcmp(arg, "--track")) {
-			track = 2;
+			track = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--no-track")) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a19e961..ef1eeb7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -148,13 +148,14 @@ test_expect_success 'test tracking setup via config' \
      test $(git config branch.my3.remote) = local &&
      test $(git config branch.my3.merge) = refs/heads/master'
 
-test_expect_success 'autosetupmerge = all' '
+test_expect_success 'avoid ambiguous track' '
 	git config branch.autosetupmerge true &&
+	git config remote.ambi1.url = lalala &&
+	git config remote.ambi1.fetch = refs/heads/lalala:refs/heads/master &&
+	git config remote.ambi2.url = lilili &&
+	git config remote.ambi2.fetch = refs/heads/lilili:refs/heads/master &&
 	git branch all1 master &&
-	test -z "$(git config branch.all1.merge)" &&
-	git config branch.autosetupmerge all &&
-	git branch all2 master &&
-	test $(git config branch.all2.merge) = refs/heads/master
+	test -z "$(git config branch.all1.merge)"
 '
 
 test_expect_success 'test overriding tracking setup via --no-track' \
@@ -167,10 +168,10 @@ test_expect_success 'test overriding tracking setup via --no-track' \
      ! test "$(git config branch.my2.remote)" = local &&
      ! test "$(git config branch.my2.merge)" = refs/heads/master'
 
-test_expect_success 'test local tracking setup' \
+test_expect_success 'no tracking without .fetch entries' \
     'git branch --track my6 s &&
-     test $(git config branch.my6.remote) = . &&
-     test $(git config branch.my6.merge) = refs/heads/s'
+     test -z "$(git config branch.my6.remote)" &&
+     test -z "$(git config branch.my6.merge)"'
 
 test_expect_success 'test tracking setup via --track but deeper' \
     'git config remote.local.url . &&
@@ -182,8 +183,8 @@ test_expect_success 'test tracking setup via --track but deeper' \
 
 test_expect_success 'test deleting branch deletes branch config' \
     'git branch -d my7 &&
-     test "$(git config branch.my7.remote)" = "" &&
-     test "$(git config branch.my7.merge)" = ""'
+     test -z "$(git config branch.my7.remote)" &&
+     test -z "$(git config branch.my7.merge)"'
 
 test_expect_success 'test deleting branch without config' \
     'git branch my7 s &&
-- 
1.5.3.rc0.2783.gf3f7
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help