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

Johannes Schindelin [off-list ref] 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,

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

	> 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

	> 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

Hi,

On Sun, 8 Jul 2007, Paolo Bonzini wrote:

[Paolo tried to hide the fact that it was Junio who wrote this:]

quoted

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