[RFC/PATCH] git-branch: default to --track

[RFC/PATCH] git-branch: default to --track

[Johannes Schindelin]

"git branch --track" will setup config variables when branching from 
a remote branch, so that if you say "git pull" while being on that
branch, it automatically fetches the correct remote, and merges the
correct branch.

Often people complain that this is not the default for "git branch". 
Make it so.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	With 1.5.3 knocking at the door, maybe it is too late to include
	this. However, I am in favour of changing the default behaviour
	here.

 builtin-branch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index ae450b0..507b47c 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -22,7 +22,7 @@ static const char builtin_branch_usage[] =
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_track_remotes;
+static int branch_track_remotes = 1;
 
 static int branch_use_color;
 static char branch_colors[][COLOR_MAXLEN] = {

Re: [RFC/PATCH] git-branch: default to --track

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> "git branch --track" will setup config variables when branching from 
> a remote branch, so that if you say "git pull" while being on that
> branch, it automatically fetches the correct remote, and merges the
> correct branch.

While I think it would have been the right thing to do if the
code did this only for a remote branch, I think there is a bug
somewhere.  I just saw this:

	... some random changes ...
        master$ git commit -a -s -m 'Some work meant for topic.'
        master$ git branch jc/new-topic
	Branch jc/new-topic set up to track local branch refs/heads/master

Eh?  I did not want this to get applied for my local branches.

The intention of the above command sequence was to do a branch
and then "reset --hard HEAD^" to rewind the 'master', as if I
did not commit but instead did "checkout -b jc/new-topic &&
commit && checkout master".

But "checkout -b jc/newtopic" has the same problem, as it
eventually uses the same "git-branch" that defaults to --track
even for a case where I branch off of a local branch.

I do not necessarily think the command line --track is broken.
If the user explicitly says a branch tracks a local branch, so
be it.  If --track comes from autosetupmerge or built-in default
like your patch, however, I do not think it makes much sense to
pollute the config file with useless "tracking" information.

I am very tempted to revert this, but won't do so tonight, yet.

[PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Johannes Schindelin]

Junio noticed that switching on autosetupmerge unilaterally started
cluttering the config for local branches.  That is not the original
intention of branch.autosetupmerge, which was meant purely for
convenience when branching off of remote branches, but that semantics
got lost somewhere.

If you still want that "new" behavior, you can switch
branch.autosetupmerge to the value "all".  Otherwise, it is interpreted
as a boolean, which triggers setting up defaults _only_ when branching
off of a remote branch, i.e. the originally intended behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 8 Jul 2007, Junio C Hamano wrote:

	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> > "git branch --track" will setup config variables when 
	> > branching from a remote branch, so that if you say "git pull" 
	> > while being on that branch, it automatically fetches the 
	> > correct remote, and merges the correct branch.
	> 
	> While I think it would have been the right thing to do if the
	> code did this only for a remote branch, I think there is a bug
	> somewhere.  I just saw this:
	> 
	> 	... some random changes ...
	>         master$ git commit -a -s -m 'Some work meant for topic.'
	>         master$ git branch jc/new-topic
	> 	Branch jc/new-topic set up to track local branch refs/heads/master
	> 
	> Eh?  I did not want this to get applied for my local branches.

	That is certainly unexpected and unwelcomed.  Alas, I think it is 
	one of the consequences of rarely executed (and thus, tested) 
	code.

	I rarely branch, but use one long running branch to commit and 
	revert, so that a "git log -S<keyword> -p" brings me back my huge 
	debug output changes, and therefore I did not catch it.

	> I do not necessarily think the command line --track is broken.

	Me, neither.  Therefore, this patch does not change the semantics 
	of that one.  But it was really unexpected for me to see that this 
	works with anything but remote branches.

	> I am very tempted to revert this, but won't do so tonight, yet.

	Well, I marked it as RFC, and was surprised to wake up to it being 
	applied.  But thanks for not reverting right away; I think this 
	patch should fix the issue.

 builtin-branch.c  |   18 ++++++++++++------
 t/t3200-branch.sh |    9 +++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 507b47c..49195a1 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -22,7 +22,7 @@ static const char builtin_branch_usage[] =
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_track_remotes = 1;
+static int branch_track = 1; /* 0 = none, 1 = remotes, 2 = all */
 
 static int branch_use_color;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -66,8 +66,12 @@ 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"))
-		branch_track_remotes = git_config_bool(var, value);
+	if (!strcmp(var, "branch.autosetupmerge")) {
+		if (!strcmp(value, "all"))
+			branch_track = 2;
+		else
+			branch_track = git_config_bool(var, value);
+	}
 
 	return git_default_config(var, value);
 }
@@ -525,7 +529,9 @@ 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)
+	if (real_ref && (track == 2 ||
+				(track == 1 &&
+				 !prefixcmp(real_ref, "refs/remotes/"))))
 		set_branch_defaults(name, real_ref);
 
 	if (write_ref_sha1(lock, sha1, msg) < 0)
@@ -586,7 +592,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int i;
 
 	git_config(git_branch_config);
-	track = branch_track_remotes;
+	track = branch_track;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -598,7 +604,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			break;
 		}
 		if (!strcmp(arg, "--track")) {
-			track = 1;
+			track = 2;
 			continue;
 		}
 		if (!strcmp(arg, "--no-track")) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index c6f472a..a19e961 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -148,6 +148,15 @@ 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' '
+	git config branch.autosetupmerge true &&
+	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_expect_success 'test overriding tracking setup via --no-track' \
     'git config branch.autosetupmerge true &&
      git config remote.local.url . &&
-- 
1.5.3.rc0.2742.g2050

Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 	> Eh?  I did not want this to get applied for my local branches.
>
> 	That is certainly unexpected and unwelcomed.  Alas, I think it is 
> 	one of the consequences of rarely executed (and thus, tested) 
> 	code.
> ...
> +test_expect_success 'autosetupmerge = all' '
> +	git config branch.autosetupmerge true &&
> +	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
> +'

Thanks.

Having prepared the patch below, I do not think if the original
patch even wanted to have 'all' semantics.  The surrounding text
only talks about "off a remote branch" and I strongly suspect
that nobody wanted to do this for a local branch case at all.



diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4b67f0a..aeece84 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -309,7 +309,10 @@ branch.autosetupmerge::
 	so that gitlink:git-pull[1] will appropriately merge from that
 	remote branch.  Note that even if this option is not set,
 	this behavior can be chosen per-branch using the `--track`
-	and `--no-track` options.  This option defaults to false.
+	and `--no-track` options.  This option can have values
+	'false' (never touch the configuration), 'all' (do this
+	for all branches), or 'true' (do this only when
+	branching from a remote tracking branch), and defaults to 'true'.
 
 branch.<name>.remote::
 	When in branch <name>, it tells `git fetch` which remote to fetch.
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 818b720..8292952 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -52,8 +52,9 @@ OPTIONS
 	set up configuration so that git-pull will automatically
 	retrieve data from the remote branch.  Set the
 	branch.autosetupmerge configuration variable to true if you
-	want git-checkout and git-branch to always behave as if
-	'--track' were given.
+	want git-checkout and git-branch to behave as if
+	'--track' were given when you branch from a remote
+	tracking branch.
 
 --no-track::
 	When -b is given and a branch is created off a remote branch,

Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Johannes Schindelin]

Hi,

On Sun, 8 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	> Eh?  I did not want this to get applied for my local branches.
> >
> > 	That is certainly unexpected and unwelcomed.  Alas, I think it is 
> > 	one of the consequences of rarely executed (and thus, tested) 
> > 	code.
> > ...
> > +test_expect_success 'autosetupmerge = all' '
> > +	git config branch.autosetupmerge true &&
> > +	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
> > +'
> 
> Thanks.
> 
> Having prepared the patch below, I do not think if the original
> patch even wanted to have 'all' semantics.  The surrounding text
> only talks about "off a remote branch" and I strongly suspect
> that nobody wanted to do this for a local branch case at all.

I remember that the comment was correct for the first few versions.  
Somehow I missed that change in semantics.  Paolo, what was the rationale?

Ciao,
Dscho

Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Paolo Bonzini]

	> Having prepared the patch below, I do not think if the original
> patch even wanted to have 'all' semantics.  The surrounding text
> only talks about "off a remote branch" and I strongly suspect
> that nobody wanted to do this for a local branch case at all.

If I remember correctly, the problem was that you are not sure that
remote branches are in refs/remotes.

Paolo

Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> 	> Having prepared the patch below, I do not think if the original
>> patch even wanted to have 'all' semantics.  The surrounding text
>> only talks about "off a remote branch" and I strongly suspect
>> that nobody wanted to do this for a local branch case at all.
>
> If I remember correctly, the problem was that you are not sure that
> remote branches are in refs/remotes.

Yes, the user can use traditional layout (e.g. refs/heads/origin
is used as a remote tracking branch).

So the check with refs/remotes/ is not technically correct, but
it should probably look-up the configuration to check the
tracking, if we really want to be strict about it.

I personally do not care too much about it, though.

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

[Johannes Schindelin]

This patch cleans up some complicated code, and replaces it with a
cleaner version.  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.

Also, the "branch name too long" condition had an off-by-one.  Not that it 
matters in real life.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 8 Jul 2007, Junio C Hamano wrote:

	> Paolo Bonzini <bonzini@gnu.org> writes:
	> 
	> >> Having prepared the patch below, I do not think if the 
	> >> original patch even wanted to have 'all' semantics.  The 
	> >> surrounding text only talks about "off a remote branch" and I 
	> >> strongly suspect that nobody wanted to do this for a local 
	> >> branch case at all.
	> >
	> > If I remember correctly, the problem was that you are not sure 
	> > that remote branches are in refs/remotes.
	> 
	> Yes, the user can use traditional layout (e.g. refs/heads/origin
	> is used as a remote tracking branch).
	> 
	> So the check with refs/remotes/ is not technically correct, but
	> it should probably look-up the configuration to check the
	> tracking, if we really want to be strict about it.

	Okay, so here is the correct fix.  In the process, I rewrote large 
	parts of it.  I really did not like the asnprintf() parts of the 
	original part, and I am not comfortable with it anyway, so that is 
	replaced, too.

	BTW if someone wonders why the wildcards look so strange in the 
	comment: after the second compilation I got sick of the "warning: 
	/* contained within comment", and wanted to spare everybody.

	Ah yes, I tried to prepare this patch with "format-patch -B", but 
	it seems to be too much in love with the few lines that match. 

 builtin-branch.c  |  196 ++++++++++++++++++++++++-----------------------------
 t/t3200-branch.sh |   21 +++---
 2 files changed, 99 insertions(+), 118 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 49195a1..d290a7a 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -22,7 +22,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 +66,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 +345,111 @@ 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;
+static struct {
+	const char *ref;
+	int ref_len;
+	char *remote;
+	char *merge;
+	int matches;
+} tracking;
 
-static int get_remote_branch_name(const char *value)
+static int tracking_config(const char *key, const char *value)
 {
 	const char *colon;
-	const char *end;
-
-	if (*value == '+')
-		value++;
+	int key_len, value_len, match_len;
+	char *merge = NULL;
 
-	colon = strchr(value, ':');
-	if (!colon)
+	/* we want: remote.<nick>.fetch = <merge>:<ref> */
+	if (prefixcmp(key, "remote.") || (key_len = strlen(key)) < 6 ||
+			strcmp(key + key_len - 6, ".fetch") ||
+			!(colon = strchr(value, ':')))
 		return 0;
 
-	end = value + strlen(value);
+	if (*value == '+')
+		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.
+	 * A remote.<name>.fetch value can have two forms:
+	 *
+	 * - exact:
+	 *
+	 *	refs/heads/gnu:refs/heads/my-upstream
+	 *
+	 * - wildcard:
+	 *
+	 *	refs/heads/ *:refs/remotes/gnu/ *
+	 *
+	 * try exact match first:
 	 */
-	if (!strcmp(colon + 1, start_ref)) {
+	if (!strcmp(colon + 1, tracking.ref))
 		/* Truncate the value before the colon. */
-		nfasprintf(&config_repo, "%.*s", colon - value, value);
-		return 1;
+		merge = xstrndup(value, colon - value);
+
+	/* wildcard; match_len is the length of the matching prefix */
+	else if ((value_len = strlen(value)) > tracking.ref_len &&
+			!strcmp(value + value_len - 2, "/*") &&
+			(match_len = value_len - (colon - value) - 2) > 0 &&
+			match_len < tracking.ref_len &&
+			!memcmp(colon + 1, tracking.ref, match_len)) {
+		int postfix_len = tracking.ref_len - match_len;
+		int replace_len = colon - 1 - value;
+		merge = xmalloc(replace_len + postfix_len);
+		memcpy(merge, value, replace_len);
+		memcpy(merge + replace_len,
+				tracking.ref + match_len, postfix_len);
 	}
 
-	/*
-	 * 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."))
+	if (!merge)
 		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);
+	tracking.matches++;
+	if (tracking.merge)
+		free(merge);
+	else {
+		tracking.merge = merge;
+		tracking.remote = xstrndup(key + 7, key_len - 7 - 6);
+	}
 
 	return 0;
 }
 
-static void set_branch_merge(const char *name, const char *config_remote,
-			     const char *config_repo)
-{
-	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);
-}
-
-static void set_branch_defaults(const char *name, const char *real_ref)
+/*
+ * 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 void setup_tracking(const char *new_ref, const char *orig_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 (config_repo && config_remote) {
-		set_branch_merge(name, config_remote, config_repo);
-		printf("Branch %s set up to track remote branch %s.\n",
-		       name, real_ref);
+	tracking.ref = orig_ref;
+	tracking.ref_len = strlen(tracking.ref);
+	git_config(tracking_config);
+
+	if (tracking.matches > 1)
+		error("Not tracking: ambiguous information for ref %s",
+				orig_ref);
+	else if (tracking.matches == 1) {
+		char key[1024];
+		int n = snprintf(key, sizeof(key), "branch.%s.remote",
+				new_ref);
+		if (n < sizeof(key) - 1) {
+			git_config_set(key, tracking.remote ?
+					tracking.remote : ".");
+
+			snprintf(key, sizeof(key), "branch.%s.merge", new_ref);
+			git_config_set(key, tracking.merge);
+
+			printf("Branch %s set up to track remote branch %s.\n",
+			       new_ref, orig_ref);
+		} else
+			error("Tracking not set up: name too long: %s",
+				new_ref);
 	}
 
-	if (config_repo)
-		free(config_repo);
-	if (config_remote)
-		free(config_remote);
+	if (tracking.remote)
+		free(tracking.remote);
+	if (tracking.merge)
+		free(tracking.merge);
 }
 
 static void create_branch(const char *name, const char *start_name,
@@ -529,10 +511,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 +584,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.2769.gd9be2

Re: [PATCH] branch --track: code cleanup and saner handling of local branches

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> @@ -349,125 +345,111 @@ 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;
> +static struct {
> +	const char *ref;
> +	int ref_len;
> +	char *remote;
> +	char *merge;
> +	int matches;
> +} tracking;
>  
> -static int get_remote_branch_name(const char *value)
> +static int tracking_config(const char *key, const char *value)
>  {
>  	const char *colon;
> +	int key_len, value_len, match_len;
> +	char *merge = NULL;
>  
> -	colon = strchr(value, ':');
> -	if (!colon)
> +	/* we want: remote.<nick>.fetch = <merge>:<ref> */
> +	if (prefixcmp(key, "remote.") || (key_len = strlen(key)) < 6 ||
> +			strcmp(key + key_len - 6, ".fetch") ||
> +			!(colon = strchr(value, ':')))
>  		return 0;
>  
> -	end = value + strlen(value);
> +	if (*value == '+')
> +		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.
> +	 * A remote.<name>.fetch value can have two forms:
> +	 *
> +	 * - exact:
> +	 *
> +	 *	refs/heads/gnu:refs/heads/my-upstream
> +	 *
> +	 * - wildcard:
> +	 *
> +	 *	refs/heads/ *:refs/remotes/gnu/ *
> +	 *
> +	 * try exact match first:
>  	 */

It strikes me a bit odd if Daniel's remote.[ch] infrastructure
does not give you easy access to this kind of information...

Re: [PATCH] branch --track: code cleanup and saner handling of local branches

[Johannes Schindelin]

Hi,

On Mon, 9 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > -static int get_remote_branch_name(const char *value)
> > +static int tracking_config(const char *key, const char *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.
> > +	 * A remote.<name>.fetch value can have two forms:
> > +	 *
> > +	 * - exact:
> > +	 *
> > +	 *	refs/heads/gnu:refs/heads/my-upstream
> > +	 *
> > +	 * - wildcard:
> > +	 *
> > +	 *	refs/heads/ *:refs/remotes/gnu/ *
> > +	 *
> > +	 * try exact match first:
> >  	 */
> 
> It strikes me a bit odd if Daniel's remote.[ch] infrastructure
> does not give you easy access to this kind of information...

Yes, probably.  However, at the time he was sending that patch, I was 
already preparing the patch.  Besides, we can always go back and change 
the code, if you really want to pull in the remotes stuff into 
builtin-branch (as of yet, they are almost independent).

My vote is to have it indpendent for now, if only to fix existing issues.  
But in the end it is your choice.

Ciao,
Dscho

Re: [PATCH] branch --track: code cleanup and saner handling of local branches

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> It strikes me a bit odd if Daniel's remote.[ch] infrastructure
>> does not give you easy access to this kind of information...
>
> Yes, probably.  However, at the time he was sending that patch, I was 
> already preparing the patch.

I was talking about existing remote.[ch] patches, which has been
in tree since late May this year, not his recent round of
"approach to builtin git-fetch" series.

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

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	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".

 remote.c    |   42 ++++++++++++++++++++++++++++++++----------
 remote.h    |    7 ++++++-
 send-pack.c |    3 +--
 3 files changed, 39 insertions(+), 13 deletions(-)

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)
+{
+	int i, result = 0;
+	read_config();
+	for (i = 0; i < allocated_remotes; 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);
+		if ((result = fn(r, priv)))
+			break;
+	}
+	return result;
+}
+
 int remote_has_uri(struct remote *remote, const char *uri)
 {
 	int i;
@@ -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 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;
 		if (!fetch->dst)
 			continue;
 		if (fetch->pattern) {
-			if (!prefixcmp(refspec->src, fetch->src)) {
+			if (!prefixcmp(refspec->src, src)) {
 				refspec->dst =
-					xmalloc(strlen(fetch->dst) +
+					xmalloc(strlen(dst) +
 						strlen(refspec->src) -
-						strlen(fetch->src) + 1);
-				strcpy(refspec->dst, fetch->dst);
-				strcpy(refspec->dst + strlen(fetch->dst),
-				       refspec->src + strlen(fetch->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, fetch->src)) {
-				refspec->dst = xstrdup(fetch->dst);
+			if (!strcmp(refspec->src, src)) {
+				refspec->dst = xstrdup(dst);
 				refspec->force = fetch->force;
 				return 0;
 			}
 		}
 	}
-	refspec->dst = NULL;
 	return -1;
 }
 
diff --git a/remote.h b/remote.h
index 01dbcef..9ab7eb6 100644
--- a/remote.h
+++ b/remote.h
@@ -20,6 +20,9 @@ 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 {
@@ -35,7 +38,9 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 
 /*
  * For the given remote, reads the refspec's src and sets the other fields.
+ * If reverse is 1, the given src is the local ref, and we want the remote.
  */
-int remote_find_tracking(struct remote *remote, struct refspec *refspec);
+int remote_find_tracking(struct remote *remote, struct refspec *refspec,
+	int reverse);
 
 #endif
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) {
-- 
1.5.3.rc0.2769.gd9be2

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

[Daniel Barkalow]

On Tue, 10 Jul 2007, Johannes Schindelin wrote:

> 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.

I think I'd like this better if reverse meant that it looked at 
refspec->dst and set refspec->src, rather than returning the refspec 
reversed; the current version sets the refspec so that it's effectively 
something from the list, which makes it easier to understand.

Maybe make it so the user calls it with at most one of src and dst NULL, 
and it returns with neither NULL or returns -1 if it can't find anything?

	-Daniel
*This .sig left intentionally blank*

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

[Johannes Schindelin]

Hi,

On Mon, 9 Jul 2007, Daniel Barkalow wrote:

> On Tue, 10 Jul 2007, Johannes Schindelin wrote:
> 
> > 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.
> 
> I think I'd like this better if reverse meant that it looked at 
> refspec->dst and set refspec->src, rather than returning the refspec 
> reversed; the current version sets the refspec so that it's effectively 
> something from the list, which makes it easier to understand.
> 
> Maybe make it so the user calls it with at most one of src and dst NULL, 
> and it returns with neither NULL or returns -1 if it can't find anything?

Will do.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 <johannes.schindelin@gmx.de>
> ---
>
> 	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.

> 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)...

> @@ -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.

> 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()

[Daniel Barkalow]

On Mon, 9 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	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()

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

The updated patch does not use "reverse" but the old description
is still there.

Daniel, one thing I fear about your "I want to store the message
in the object store so that I can reuse even after I re-polish
the series" desire on the cover letter topic is this kind of
gotcha, and that is why I suggested "*** BLURB GOES HERE ***".
Both the summary (diffstat and shortlog) part and the
description part should be kept fresh in the updated 0/N; while
we can automate the summary part whenever we re-generate 0/N,
you cannot automate the description part.

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

[Johannes Schindelin]

Hi,

On Tue, 10 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
> 
> The updated patch does not use "reverse" but the old description
> is still there.

Urgh. Right. May I ask you to paste this instead?

The function remote_find_tracking() was extended to be able to search 
remote refs for a given local ref.  You have to set either src or dst in 
the refspec, and remote_find_tracking() will fill in the other and return 
0.

> Daniel, one thing I fear about your "I want to store the message
> in the object store so that I can reuse even after I re-polish
> the series" desire on the cover letter topic is this kind of
> gotcha, and that is why I suggested "*** BLURB GOES HERE ***".

I am happy that my fsckup served a purpose, then.  (And maybe this would 
be a good hint in rebase -i's man page, too, since that is how the error 
was introduced here.)

Ciao,
Dscho

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

[Daniel Barkalow]

On Tue, 10 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
> 
> The updated patch does not use "reverse" but the old description
> is still there.
> 
> Daniel, one thing I fear about your "I want to store the message
> in the object store so that I can reuse even after I re-polish
> the series" desire on the cover letter topic is this kind of
> gotcha, and that is why I suggested "*** BLURB GOES HERE ***".
> Both the summary (diffstat and shortlog) part and the
> description part should be kept fresh in the updated 0/N; while
> we can automate the summary part whenever we re-generate 0/N,
> you cannot automate the description part.

It seems to me that commit messages are much more likely to mention the 
sorts of details that are affected by review than cover letters are. 
Furthermore, if the message is coming out of a tag on the head of the 
series, whatever is used to put the tag onto the new head of the series 
would present the buffer for editting again, just like commit --amend 
does. So the user would be just as likely to think to update a series 
header as a commit message, and less likely to need to.

	-Daniel
*This .sig left intentionally blank*

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

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	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

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

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---

	... and again I was bitten by the minimality of the script. The
	original diff was _unreadable_, because it matched some empty 
	lines here and there, or some lonely curly brackets.

	Therefore I manually tweaked it to show first the cruft that was 
	removed, and then the rewrite using remote.[ch]. As a consequence, 
	the diffstat is not the one of this patch, but the one "git show 
	--stat" showed.

 builtin-branch.c  |  167 ++++++++++++++++------------------------------------
 t/t3200-branch.sh |   21 ++++---
 2 files changed, 63 insertions(+), 125 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 49195a1..0dbd6d7 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,67 @@ 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;
-
-static int get_remote_branch_name(const char *value)
-{
-	const char *colon;
-	const char *end;
-
-	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;
-	}
-
-	/*
-	 * 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)
-{
-	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);
-}
-
-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 (config_repo && config_remote) {
-		set_branch_merge(name, config_remote, config_repo);
-		printf("Branch %s set up to track remote branch %s.\n",
-		       name, real_ref);
-	}
-
-	if (config_repo)
-		free(config_repo);
-	if (config_remote)
-		free(config_remote);
+struct tracking {
+	struct refspec spec;
+	char *dst;
+	const char *remote;
+	int matches;
+};
+
+static int find_tracked_branch(struct remote *remote, void *priv)
+{
+	struct tracking *tracking = priv;
+
+	if (!remote_find_tracking(remote, &tracking->spec, 1)) {
+		if (++tracking->matches == 1) {
+			tracking->dst = tracking->spec.dst;
+			tracking->remote = remote->name;
+		} else {
+			free(tracking->spec.dst);
+			free(tracking->dst);
+			tracking->dst = NULL;
+		}
+	}
+
+	return 0;
+}
+
+
+/*
+ * 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];
+	struct tracking tracking;
+
+	if (strlen(new_ref) > 1024 - 7 - 7 - 1)
+		return error("Tracking not set up: name too long: %s",
+				new_ref);
+
+	memset(&tracking, 0, sizeof(tracking));
+	tracking.spec.src = 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.dst);
+		free(tracking.dst);
+		printf("Branch %s set up to track remote branch %s.\n",
+			       new_ref, orig_ref);
+	}
+
+	return 0;
 }
 
 static void create_branch(const char *name, const char *start_name,
@@ -529,10 +468,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 +541,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.2769.gd9be2

Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"

[Johannes Schindelin]

Hi,

On Sun, 8 Jul 2007, Paolo Bonzini wrote:

> [Paolo tried to hide the fact that it was Junio who wrote this:]
>
> > Having prepared the patch below, I do not think if the original patch 
> > even wanted to have 'all' semantics.  The surrounding text only talks 
> > about "off a remote branch" and I strongly suspect that nobody wanted 
> > to do this for a local branch case at all.
> 
> If I remember correctly, the problem was that you are not sure that 
> remote branches are in refs/remotes.

Then you code is incorrect.

Basically, you use a confusing set of four functions to do the following:

- read the config, and
- write the branch.<name>.{remote,merge} variables

Two functions would have been sufficient, and easier to read.  And as I 
fully expect with non-simple code, a bug was lurking.  This time in 
set_branch_defaults():

you check if neither config_repo nor config_remote (which is a misnomer, 
as it does not contain a "remote", but a "remote branch") is set.  But 
that happens when there was no information in the config, too!

Also you miss the case that there is ambiguous information:

[remote "hello"]
	url = git://blub/x.git
	fetch = refs/heads/master:refs/heads/origin

[remote "bello"]
	url = git://yaddayadda/x.git
	fetch = refs/heads/master:refs/heads/origin

See? Your code just uses "bello".

Will send out a fix shortly.

Ciao,
Dscho