defaults for where to merge from

defaults for where to merge from

[Paolo Bonzini]

As can be seen from my other messages, I'm experimenting a little with 
git and trying to understand how its workflow compares with arch.  Right 
now, my procedure for branching off a remote archive is:

   git checkout -b branchname remote/upstreambranch
   git config --add branch.branchname.remote remote
   git config --add branch.branchname.merge refs/heads/upstreambranch

Is there a reason why "git branch" and "git checkout -b" should not 
automatically do the two "git-config --add"s when the source branch is 
remote?

In case the source branch is not remote, would "origin" be a good choice 
for the "branch.branchname.remote" variable?

Paolo

Re: defaults for where to merge from

[Johannes Schindelin]

Hi,

On Wed, 28 Feb 2007, Paolo Bonzini wrote:

> As can be seen from my other messages, I'm experimenting a little with 
> git and trying to understand how its workflow compares with arch.  
> Right now, my procedure for branching off a remote archive is:
> 
>   git checkout -b branchname remote/upstreambranch
>   git config --add branch.branchname.remote remote
>   git config --add branch.branchname.merge refs/heads/upstreambranch
> 
> Is there a reason why "git branch" and "git checkout -b" should not 
> automatically do the two "git-config --add"s when the source branch is 
> remote?

I like it.

> In case the source branch is not remote, would "origin" be a good choice 
> for the "branch.branchname.remote" variable?

In case source branch is not a remote, I would not want that DWIMery.

Ciao,
Dscho

Re: defaults for where to merge from

[Andy Parkins]

On Wednesday 2007 February 28 14:53, Paolo Bonzini wrote:
> As can be seen from my other messages, I'm experimenting a little with
> git and trying to understand how its workflow compares with arch.  Right
> now, my procedure for branching off a remote archive is:
>
>    git checkout -b branchname remote/upstreambranch
>    git config --add branch.branchname.remote remote
>    git config --add branch.branchname.merge refs/heads/upstreambranch
>
> Is there a reason why "git branch" and "git checkout -b" should not
> automatically do the two "git-config --add"s when the source branch is
> remote?

I can see why that would be handy, but I often make short lived branches off a 
remote; and I wouldn't want my config cluttered up with branch defintions.

> In case the source branch is not remote, would "origin" be a good choice
> for the "branch.branchname.remote" variable?

No.  That would still reference a remote.  As in:

[remote "origin"]
        url = git://git.kernel.org/pub/scm/git/git.git
        fetch = refs/heads/master:refs/remotes/origin/master
[branch "master"]
        remote = origin
        merge = refs/heads/master

The remote = origin tells git to use the [remote "origin"] section.

I think what you want is something that I would like too.  If you specify "." 
to a git-pull it means to use the local repository not a remote.  It would be 
great if one could have:

[remote "origin"]
        url = git://git.kernel.org/pub/scm/git/git.git
        fetch = refs/heads/master:refs/remotes/origin/master
[branch "master"]
        remote = .
        merge = refs/remotes/origin/master

That way a "git pull" on master wouldn't need to make a remote connection in 
order to do a merge (which is the way I like it).  However, I remember there 
was a reason this wouldn't work, but I don't remember what it was :-)


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: defaults for where to merge from

[Paolo Bonzini]

>>    git checkout -b branchname remote/upstreambranch
>>    git config --add branch.branchname.remote remote
>>    git config --add branch.branchname.merge refs/heads/upstreambranch
>>
>> Is there a reason why "git branch" and "git checkout -b" should not
>> automatically do the two "git-config --add"s when the source branch is
>> remote?
> 
> I can see why that would be handy, but I often make short lived branches off a 
> remote; and I wouldn't want my config cluttered up with branch defintions.

So, is there a reason why the config not could be cleared out by branch -d?

(The "is there a reason" has the same meaning: it seems obvious to me, 
but I surely miss a lot of usecases because of inexperience).

Paolo

Re: defaults for where to merge from

[Julian Phillips]

On Wed, 28 Feb 2007, Andy Parkins wrote:

> On Wednesday 2007 February 28 14:53, Paolo Bonzini wrote:
>> As can be seen from my other messages, I'm experimenting a little with
>> git and trying to understand how its workflow compares with arch.  Right
>> now, my procedure for branching off a remote archive is:
>>
>>    git checkout -b branchname remote/upstreambranch
>>    git config --add branch.branchname.remote remote
>>    git config --add branch.branchname.merge refs/heads/upstreambranch
>>
>> Is there a reason why "git branch" and "git checkout -b" should not
>> automatically do the two "git-config --add"s when the source branch is
>> remote?
>
> I can see why that would be handy, but I often make short lived branches off a
> remote; and I wouldn't want my config cluttered up with branch defintions.

How about adding an option to tell checkout/branch that a tracking branch 
is wanted (-t perhaps) - or perhaps a way to say that you don't want to 
track the remote (depending on which is more popular)?

Certainly would be a nice feature to have ...

-- 
Julian

  ---
Meg Griffin:  Somebody's in the closet!
Jeff Foxworthy:  You know you're a redneck when your gun rack has a gun rack on it.
Stewie Griffin:  You suck!

Re: defaults for where to merge from

[Andy Parkins]

On Wednesday 2007 February 28 15:30, Paolo Bonzini wrote:

> So, is there a reason why the config not could be cleared out by branch -d?

Good point.  I can't think of one.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: defaults for where to merge from

[Johannes Schindelin]

Hi,

On Wed, 28 Feb 2007, Julian Phillips wrote:

> On Wed, 28 Feb 2007, Andy Parkins wrote:
> 
> > On Wednesday 2007 February 28 14:53, Paolo Bonzini wrote:
> > > As can be seen from my other messages, I'm experimenting a little with
> > > git and trying to understand how its workflow compares with arch.  Right
> > > now, my procedure for branching off a remote archive is:
> > > 
> > >    git checkout -b branchname remote/upstreambranch
> > >    git config --add branch.branchname.remote remote
> > >    git config --add branch.branchname.merge refs/heads/upstreambranch
> > > 
> > > Is there a reason why "git branch" and "git checkout -b" should not
> > > automatically do the two "git-config --add"s when the source branch is
> > > remote?
> > 
> > I can see why that would be handy, but I often make short lived 
> > branches off a remote; and I wouldn't want my config cluttered up with 
> > branch defintions.
> 
> How about adding an option to tell checkout/branch that a tracking 
> branch is wanted (-t perhaps) - or perhaps a way to say that you don't 
> want to track the remote (depending on which is more popular)?

I don't think that you should be forced to do it explicitely. If you want 
to merge in another branch, you can do that _explicitely_. So, defaulting 
to what most people want anyway is A Good Thing.

Just my 2 cents,
Dscho

Re: defaults for where to merge from

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]


> I don't think that you should be forced to do it explicitely. If you want 
> to merge in another branch, you can do that _explicitely_. So, defaulting 
> to what most people want anyway is A Good Thing.

Here is a prototype patch to implement this functionality.  One problem 
is that config.c does not remove cleaned sections, so after "git-branch 
-d mybranch" one is left with a useless "[branch "mybranch"]" section in 
.git/config.  Other than this, it seems to work well in my experiments.

Paolo

[-- Attachment #2: git-builtin-branch-config.patch --]
[-- Type: text/plain, Size: 2528 bytes --]

diff --git a/builtin-branch.c b/builtin-branch.c
index d0179b0..4c42825 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -147,8 +147,21 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
-		} else
+		} else {
+			if (kinds == REF_LOCAL_BRANCH) {
+				/* Remove git-config keys.  */
+				char *config_key = xmalloc(strlen(argv[i]) + 15);
+				sprintf(config_key, "branch.%s.remote", argv[i]);
+				git_config_set(config_key, NULL);
+
+				sprintf(config_key, "branch.%s.merge", argv[i]);
+				git_config_set(config_key, NULL);
+				sprintf(config_key, "branch.%s", argv[i]);
+				git_config_set(config_key, NULL);
+			}
+
 			printf("Deleted %sbranch %s.\n", remote, argv[i]);
+		}
 
 	}
 
@@ -316,7 +330,7 @@ static void create_branch(const char *name, const char *start_name,
 	struct commit *commit;
 	unsigned char sha1[20];
 	char ref[PATH_MAX], msg[PATH_MAX + 20];
-	int forcing = 0;
+	int forcing = 0, remote = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
 	if (check_ref_format(ref))
@@ -335,6 +349,13 @@ static void create_branch(const char *name, const char *start_name,
 		hashcpy(sha1, start_sha1);
 	else if (get_sha1(start_name, sha1))
 		die("Not a valid object name: '%s'.", start_name);
+	else {
+		unsigned char remote_sha1[20];
+		remote = strchr(start_name, '/')
+			 && read_ref(mkpath("refs/remotes/%s", start_name),
+				     remote_sha1) != -1
+			 && !memcmp(sha1, remote_sha1, 20);
+	}
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
 		die("Not a valid branch point: '%s'.", start_name);
@@ -354,6 +375,23 @@ static void create_branch(const char *name, const char *start_name,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	if (remote) {
+		/* Branching off a remote branch.  Set up so that git-pull
+		   automatically merges from there.  */
+		char *config_key = xmalloc(strlen(name) + 15);
+		char *merge_value = xmalloc(strlen(start_name) + 10);
+		char *slash = strchr(start_name, '/');
+
+		char *remote_value = xstrdup(start_name);
+		remote_value[slash - start_name] = 0;
+		sprintf(config_key, "branch.%s.remote", name);
+		git_config_set(config_key, remote_value);
+
+		sprintf(merge_value, "refs/heads/%s", slash + 1);
+		sprintf(config_key, "branch.%s.merge", name);
+		git_config_set(config_key, merge_value);
+	}
+
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
 }

Re: defaults for where to merge from

[Peter Baumann]

Andy Parkins <andyparkins@gmail.com> schrieb:
> On Wednesday 2007 February 28 14:53, Paolo Bonzini wrote:
>> As can be seen from my other messages, I'm experimenting a little with
>> git and trying to understand how its workflow compares with arch.  Right
>> now, my procedure for branching off a remote archive is:
>>
>>    git checkout -b branchname remote/upstreambranch
>>    git config --add branch.branchname.remote remote
>>    git config --add branch.branchname.merge refs/heads/upstreambranch
>>
>> Is there a reason why "git branch" and "git checkout -b" should not
>> automatically do the two "git-config --add"s when the source branch is
>> remote?
>
> I can see why that would be handy, but I often make short lived branches off a 
> remote; and I wouldn't want my config cluttered up with branch defintions.
>
>> In case the source branch is not remote, would "origin" be a good choice
>> for the "branch.branchname.remote" variable?
>
> No.  That would still reference a remote.  As in:
>
> [remote "origin"]
>         url = git://git.kernel.org/pub/scm/git/git.git
>         fetch = refs/heads/master:refs/remotes/origin/master
> [branch "master"]
>         remote = origin
>         merge = refs/heads/master
>
> The remote = origin tells git to use the [remote "origin"] section.
>
> I think what you want is something that I would like too.  If you specify "." 
> to a git-pull it means to use the local repository not a remote.  It would be 
> great if one could have:
>
> [remote "origin"]
>         url = git://git.kernel.org/pub/scm/git/git.git
>         fetch = refs/heads/master:refs/remotes/origin/master
> [branch "master"]
>         remote = .
>         merge = refs/remotes/origin/master
>
> That way a "git pull" on master wouldn't need to make a remote connection in 
> order to do a merge (which is the way I like it).  However, I remember there 
> was a reason this wouldn't work, but I don't remember what it was :-)
>
>
> Andy

This doesn't work (AIUI). In your example you need to have a remote named ".",
which you havened (and I don't think . is valid for a remote name ).

But this works for me:

[remote "origin"]
        url = host:/path/to/your/repo
        fetch = refs/heads/*:refs/remotes/origin/*

[remote "local"]
        url = .
        fetch = refs/remotes/*:refs/remotes/*	# fetch into itself -> do nothing
						# needed to get the LHS of the fetch

[branch "testbranch"]
        remote = local
        merge = refs/remotes/origin/master	# specifying the LHS of the fetch


When on branch 'testbranch', a	'git pull local' merges
'refs/remotes/origin/master'.

Peter

Re: defaults for where to merge from

[Johannes Schindelin]

Hi,

On Wed, 28 Feb 2007, Paolo Bonzini wrote:

> 
> > I don't think that you should be forced to do it explicitely. If you 
> > want to merge in another branch, you can do that _explicitely_. So, 
> > defaulting to what most people want anyway is A Good Thing.
> 
> Here is a prototype patch to implement this functionality.  One problem 
> is that config.c does not remove cleaned sections, so after "git-branch 
> -d mybranch" one is left with a useless "[branch "mybranch"]" section in 
> .git/config.  Other than this, it seems to work well in my experiments.

I'd rather remove the complete section (However, that means that you have 
to introduce a function to do that in config.c) instead of unsetting 
several hardcoded values.

Also, the second part of your patch would be clearer (methinks) if you 
replaced the call to get_sha1() with dwim_ref(). You'd get the real 
refname for free...

Ciao,
Dscho

Re: defaults for where to merge from

[Alex Riesen]

On 2/28/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > How about adding an option to tell checkout/branch that a tracking
> > branch is wanted (-t perhaps) - or perhaps a way to say that you don't
> > want to track the remote (depending on which is more popular)?
>
> I don't think that you should be forced to do it explicitely. If you want
> to merge in another branch, you can do that _explicitely_. So, defaulting
> to what most people want anyway is A Good Thing.
>

As is an option to disable the feature. I.e. for scripts, which create branches
blindly, without knowing they working on a remote branch.
So, please, provide an option to do what git-checkout/git-branch did before:
which is just create the branch, nothing more.

Re: defaults for where to merge from

[Paolo Bonzini]

> As is an option to disable the feature. I.e. for scripts, which create 
> branches
> blindly, without knowing they working on a remote branch.
> So, please, provide an option to do what git-checkout/git-branch did 
> before:
> which is just create the branch, nothing more.

This does create the branch and nothing more.  It sets up "git pull" to 
do the obvious thing, but does not do anything more.  The branch is left 
in the same state than without the patch.  So the scripts will still 
work unless they create branches blindly, and blindly do a "git pull" 
expecting it to do the unobvious thing.

Paolo

Re: defaults for where to merge from

[Alex Riesen]

On 2/28/07, Paolo Bonzini <paolo.bonzini@lu.unisi.ch> wrote:
> > As is an option to disable the feature. I.e. for scripts, which create
> > branches
> > blindly, without knowing they working on a remote branch.
> > So, please, provide an option to do what git-checkout/git-branch did
> > before:
> > which is just create the branch, nothing more.
>
> This does create the branch and nothing more.

... except ...

> It sets up "git pull" to
> do the obvious thing, but does not do anything more.  The branch is left
> in the same state than without the patch.  So the scripts will still
> work unless they create branches blindly, and blindly do a "git pull"
> expecting it to do the unobvious thing.

Well, I find it unobvious for pull to magically starting merging.
Perhaps I'm using branch configuration in .git/config for too long,
and actually expect nothing to be merged if there is no appropriate
branch configuration.

Re: defaults for where to merge from

[Johannes Schindelin]

Hi,

On Thu, 1 Mar 2007, Alex Riesen wrote:

> Well, I find it unobvious for pull to magically starting merging.

Pull is _all_ about merging.

> Perhaps I'm using branch configuration in .git/config for too long, and 
> actually expect nothing to be merged if there is no appropriate branch 
> configuration.

It is not possible that you use that feature you described for too long, 
since it was not there in 1.4.x. There, whenever you said "git pull", it 
would try to blindly pull the default branch of the remote "origin", which 
might have been correct for the default branch (i.e. the branch 
automatically set up by git-clone), but not necessarily for the other 
branches.

And I want to stress a very important point: this automatic setting of the 
default remote and branch-to-merge should be _only_ triggered when 
branching from refs in $GIT_DIR/refs/remotes. Even if you _do_ branch them 
from scripts, I doubt that you'll break _anything_, except of course when 
the scripts are buggy to begin with.

There is a good chance that some user wants to merge something 
different, but in that case you have to specify what to merge _anyway_.

However, with the proposed behaviour, more new users would get less "Huh?" 
experiences.

Ciao,
Dscho

[PATCH] defaults for where to merge from (take 2)

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]


> I'd rather remove the complete section (However, that means that you have 
> to introduce a function to do that in config.c)

No problem.

> Also, the second part of your patch would be clearer (methinks) if you 
> replaced the call to get_sha1() with dwim_ref(). You'd get the real 
> refname for free...

Not only that, my previous version did not work if somebody specified
remotes/remote/upstreambranch instead of remote/upstreambranch.

Updated version attached.

Paolo



[-- Attachment #2: git-builtin-branch-config-2.patch --]
[-- Type: text/plain, Size: 7749 bytes --]

* git-branch: register where to merge from, when branching off a remote branch.

A rather standard (in 1.5) procedure for branching off a remote archive is:

  git checkout -b branchname remote/upstreambranch
  git config --add branch.branchname.remote remote
  git config --add branch.branchname.merge refs/heads/upstreambranch

In this case, we can save the user some effort if "git branch" (and
"git checkout -b") automatically do the two "git-config --add"s when the
source branch is remote.  There is a good chance that some user wants
to merge something different, but in that case they have to specify what
to merge _anyway_.

Dually, we remove the branch.branchname section when the user does
"git branch -d".


diff --git a/builtin-branch.c b/builtin-branch.c
index d0179b0..c1563cf 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -147,8 +147,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			error("Error deleting %sbranch '%s'", remote,
 			       argv[i]);
 			ret = 1;
-		} else
+		} else {
+			/* Remove git-config keys.  */
+			if (kinds == REF_LOCAL_BRANCH) {
+				char *config_key = xmalloc(strlen(argv[i]) + 8);
+				sprintf(config_key, "branch.%s", argv[i]);
+				git_config_remove_section(config_key);
+			}
+
 			printf("Deleted %sbranch %s.\n", remote, argv[i]);
+		}
 
 	}
 
@@ -308,6 +316,27 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
 	free_ref_list(&ref_list);
 }
 
+static void register_branch_pull (const char *name, const char *remote_name)
+{
+	char *slash = strchr(remote_name, '/');
+
+	char *config_key = xmalloc(strlen(name) + 15);
+	char *merge_value = xmalloc(strlen(remote_name) + 10);
+
+	char *remote_value = xstrdup(remote_name);
+	remote_value[slash - remote_name] = 0;
+	sprintf(config_key, "branch.%s.remote", name);
+	git_config_set(config_key, remote_value);
+
+	sprintf(merge_value, "refs/heads/%s", slash + 1);
+	sprintf(config_key, "branch.%s.merge", name);
+	git_config_set(config_key, merge_value);
+
+	free (config_key);
+	free (remote_value);
+	free (merge_value);
+}
+
 static void create_branch(const char *name, const char *start_name,
 			  unsigned char *start_sha1,
 			  int force, int reflog)
@@ -315,8 +344,8 @@ static void create_branch(const char *name, const char *start_name,
 	struct ref_lock *lock;
 	struct commit *commit;
 	unsigned char sha1[20];
-	char ref[PATH_MAX], msg[PATH_MAX + 20];
-	int forcing = 0;
+	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
+	int forcing = 0, remote = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
 	if (check_ref_format(ref))
@@ -333,7 +362,9 @@ static void create_branch(const char *name, const char *start_name,
 	if (start_sha1)
 		/* detached HEAD */
 		hashcpy(sha1, start_sha1);
-	else if (get_sha1(start_name, sha1))
+	else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
+		remote = !prefixcmp(real_ref, "refs/remotes/");
+	else
 		die("Not a valid object name: '%s'.", start_name);
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
@@ -354,8 +385,16 @@ static void create_branch(const char *name, const char *start_name,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	/* When branching off a remote branch, set up so that git-pull
+	   automatically merges from there.  */
+	if (remote)
+		register_branch_pull (name, real_ref + 13);
+
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
+
+	if (real_ref)
+		free (real_ref);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
diff --git a/cache.h b/cache.h
index 8bbc142..797483b 100644
--- a/cache.h
+++ b/cache.h
@@ -438,6 +438,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
+extern int git_config_remove_section(const char *);
 extern int check_repository_format_version(const char *var, const char *value);
 
 #define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index 0ff413b..acd83aa 100644
--- a/config.c
+++ b/config.c
@@ -854,6 +854,33 @@ write_err_out:
 
 }
 
+static int section_name_match (const char *buf, const char *name)
+{
+	int i = 0, j = 0, dot = 0;
+	for (; buf[i] && buf[i] != ']'; i++) {
+		if (!dot && isspace(buf[i])) {
+			dot = 1;
+			if (name[j++] != '.')
+				break;
+			for (i++; isspace(buf[i]); i++)
+				; /* do nothing */
+			if (buf[i] != '"')
+				break;
+			continue;
+		}
+		if (buf[i] == '\\' && dot)
+			i++;
+		else if (buf[i] == '"' && dot) {
+			for (i++; isspace(buf[i]); i++)
+				; /* do_nothing */
+			break;
+		}
+		if (buf[i] != name[j++])
+			break;
+	}
+	return (buf[i] == ']' && name[j] == 0);
+}
+
 int git_config_rename_section(const char *old_name, const char *new_name)
 {
 	int ret = 0;
@@ -885,40 +912,15 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 		int length;
 		for (i = 0; buf[i] && isspace(buf[i]); i++)
 			; /* do nothing */
-		if (buf[i] == '[') {
+		if (buf[i] == '[' && section_name_match (&buf[i+1], old_name)) {
 			/* it's a section */
-			int j = 0, dot = 0;
-			for (i++; buf[i] && buf[i] != ']'; i++) {
-				if (!dot && isspace(buf[i])) {
-					dot = 1;
-					if (old_name[j++] != '.')
-						break;
-					for (i++; isspace(buf[i]); i++)
-						; /* do nothing */
-					if (buf[i] != '"')
-						break;
-					continue;
-				}
-				if (buf[i] == '\\' && dot)
-					i++;
-				else if (buf[i] == '"' && dot) {
-					for (i++; isspace(buf[i]); i++)
-						; /* do_nothing */
-					break;
-				}
-				if (buf[i] != old_name[j++])
-					break;
-			}
-			if (buf[i] == ']' && old_name[j] == 0) {
-				/* old_name matches */
-				ret++;
-				store.baselen = strlen(new_name);
-				if (!store_write_section(out_fd, new_name)) {
-					ret = write_error();
-					goto out;
-				}
-				continue;
+			ret++;
+			store.baselen = strlen(new_name);
+			if (!store_write_section(out_fd, new_name)) {
+				ret = write_error();
+				goto out;
 			}
+			continue;
 		}
 		length = strlen(buf);
 		if (write_in_full(out_fd, buf, length) != length) {
@@ -934,3 +936,58 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 	return ret;
 }
 
+int git_config_remove_section(const char *name)
+{
+	int ret = 0;
+	char *config_filename;
+	struct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);
+	int out_fd;
+	int removing = 0;
+	char buf[1024];
+
+	config_filename = getenv(CONFIG_ENVIRONMENT);
+	if (!config_filename) {
+		config_filename = getenv(CONFIG_LOCAL_ENVIRONMENT);
+		if (!config_filename)
+			config_filename  = git_path("config");
+	}
+	config_filename = xstrdup(config_filename);
+	out_fd = hold_lock_file_for_update(lock, config_filename, 0);
+	if (out_fd < 0) {
+		ret = error("Could not lock config file!");
+		goto out;
+	}
+
+	if (!(config_file = fopen(config_filename, "rb"))) {
+		ret = error("Could not open config file!");
+		goto out;
+	}
+
+	while (fgets(buf, sizeof(buf), config_file)) {
+		int i;
+		int length;
+		for (i = 0; buf[i] && isspace(buf[i]); i++)
+			; /* do nothing */
+		if (buf[i] == '[') {
+			if (section_name_match (&buf[i + 1], name)) {
+				/* name matches */
+				ret++;
+				removing = 1;
+			} else
+				removing = 0;
+		}
+		if (removing)
+			continue;
+		length = strlen(buf);
+		if (write_in_full(out_fd, buf, length) != length) {
+			ret = write_error();
+			goto out;
+		}
+	}
+	fclose(config_file);
+	if (close(out_fd) || commit_lock_file(lock) < 0)
+			ret = error("Cannot commit config file!");
+ out:
+	free(config_filename);
+	return ret;
+}

Re: defaults for where to merge from

[Alex Riesen]

On 3/1/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > Well, I find it unobvious for pull to magically starting merging.
>
> Pull is _all_ about merging.
>

... from what the user _conciously_ meant it to.

> > Perhaps I'm using branch configuration in .git/config for too long, and
> > actually expect nothing to be merged if there is no appropriate branch
> > configuration.
>
> It is not possible that you use that feature you described for too long,
> since it was not there in 1.4.x. There, whenever you said "git pull", it

the syntax was introduced about half a year ago. Plenty of time
to get used to it

> would try to blindly pull the default branch of the remote "origin", which
> might have been correct for the default branch (i.e. the branch
> automatically set up by git-clone), but not necessarily for the other
> branches.

which everyone hated. With this change it will not that blindly,
but still unexpectedly jump to some remote branch.

> However, with the proposed behaviour, more new users would get less "Huh?"
> experiences.

yes, it will be the old users who'd get the experiences. What do they
do, edit out unwanted tracking from .git/config everytime a branch
from remote is created?

Besides, I'm was just asking about providing an option (command-line
parameter) to change the behavior to what it was before!
Just "--no-tracking"?

Re: defaults for where to merge from

[Paolo Bonzini]

 > which everyone hated. With this change it will not that blindly,
 > but still unexpectedly jump to some remote branch.
 >
 >> However, with the proposed behaviour, more new users would get less
 >> "Huh?" experiences.
 >
 > yes, it will be the old users who'd get the experiences. What do they
 > do, edit out unwanted tracking from .git/config everytime a branch
 > from remote is created?

If everyone hated the old behavior, old users should already be careful 
about not git-pull'ing (without options) from any branch but master.  So 
they won't see any difference.

Coming from arch, which is not really a masterpiece of intuitiveness, 
this detail (as well as others for which I may try writing patches 
later) struck me as *extremely* unintuitive...

Paolo

Re: defaults for where to merge from

[Alex Riesen]

On 3/1/07, Paolo Bonzini <paolo.bonzini@gmail.com> wrote:
>
>  > which everyone hated. With this change it will not that blindly,
>  > but still unexpectedly jump to some remote branch.
>  >
>  >> However, with the proposed behaviour, more new users would get less
>  >> "Huh?" experiences.
>  >
>  > yes, it will be the old users who'd get the experiences. What do they
>  > do, edit out unwanted tracking from .git/config everytime a branch
>  > from remote is created?
>
> If everyone hated the old behavior, old users should already be careful
> about not git-pull'ing (without options) from any branch but master.  So
> they won't see any difference.

except for .git/config growing uncontrollably

Re: defaults for where to merge from

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

>> If everyone hated the old behavior, old users should already be careful
>> about not git-pull'ing (without options) from any branch but master.  So
>> they won't see any difference.
>
> except for .git/config growing uncontrollably

I think this should be a new option, not the _modified default_.
Otherwise it would be harder to sell to olde timers.

	$ git checkout -B <newbranch> remotes/<blah>
        $ git branch --track <newbranch> remotes/<blah>

might be a good compromise.

Re: defaults for where to merge from

[Paolo Bonzini]

>> If everyone hated the old behavior, old users should already be careful
>> about not git-pull'ing (without options) from any branch but master.  So
>> they won't see any difference.
> 
> except for .git/config growing uncontrollably

While I am probably going to modify the patch to satisfy Junio, have you 
noticed that "git branch -d" will delete the section, hence .git/config 
will not be growing uncontrollably?

Paolo

Re: defaults for where to merge from

[Alex Riesen]

On 3/1/07, Paolo Bonzini <paolo.bonzini@gmail.com> wrote:
>
> >> If everyone hated the old behavior, old users should already be careful
> >> about not git-pull'ing (without options) from any branch but master.  So
> >> they won't see any difference.
> >
> > except for .git/config growing uncontrollably
>
> While I am probably going to modify the patch to satisfy Junio, have you
> noticed that "git branch -d" will delete the section, hence .git/config
> will not be growing uncontrollably?
>

Maybe you also have noticed that it will remove also the reference?
How do I remove the garbage you added for TRACKING?!

Re: defaults for where to merge from

[Paolo Bonzini]

>> While I am probably going to modify the patch to satisfy Junio, have you
>> noticed that "git branch -d" will delete the section, hence .git/config
>> will not be growing uncontrollably?
> 
> Maybe you also have noticed that it will remove also the reference?

Sure I have.

> How do I remove the garbage you added for TRACKING?!

I see two possibilities:

1) I can add a "git-config --remove-section" option.  So you can do 
"git-pull" to merge onto your branches, and then remove the tracking 
section.

2) I can add a "git-branch --stop-tracking" option, which just removes 
the section.

3) Same as 2), plus I add a "git-branch --no-track" option, which does 
not add the section in the first place.  But I do believe that there is 
no reason why this cannot be the default.

Paolo

Re: defaults for where to merge from

[Alex Riesen]

On 3/1/07, Paolo Bonzini <paolo.bonzini@gmail.com> wrote:
> > How do I remove the garbage you added for TRACKING?!
>
> I see two possibilities:
>
> 1) I can add a "git-config --remove-section" option.  So you can do
> "git-pull" to merge onto your branches, and then remove the tracking
> section.

This is irrelevant in this particular context. It is just wrong here:
are you sure you _CAN_ know what it is you are removing?
Git config syntax is loosely defined, and branch or remote sections
can have important user information. Which he does not immediately
see typing git config --remove-section branch.abc _JUST_ to get
rid of the configuration he did not want in first place!

> 2) I can add a "git-branch --stop-tracking" option, which just removes
> the section.

This is independent and _probably_ not really needed. It is also
probably more complex than you think. Consider:

[remote "abc"]
  url = ...
  fetch = refs/heads/*:refs/remotes/abc/*
  fetch = refs/heads/test:refs/heads/abc-test

> 3) Same as 2), plus I add a "git-branch --no-track" option, which does
> not add the section in the first place.  But I do believe that there is
> no reason why this cannot be the default.

This is just what I asked for except for the first part.
And I see no reason for it to _BE_ the default.
Actually, how about making the default configurable _AND_ have
the --no-track option (for scripting)?

[PATCH] defaults for where to merge from (take 3)

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]


> Actually, how about making the default configurable _AND_ have
> the --no-track option (for scripting)?

Fine by me.  And also --track in case you want to try it out.  ;-)

I made it default true, environment.c can be changed by Junio if he 
applies the patch.

Patch attached.  --remove-section will go in a separate patch.

Paolo

[-- Attachment #2: git-builtin-branch-config-3.patch --]
[-- Type: text/plain, Size: 7507 bytes --]

* git-branch: register where to merge from, when branching off a remote branch.

A rather standard (in 1.5) procedure for branching off a remote archive is:

  git checkout -b branchname remote/upstreambranch
  git config --add branch.branchname.remote remote
  git config --add branch.branchname.merge refs/heads/upstreambranch

In this case, we can save the user some effort if "git branch" (and
"git checkout -b") automatically do the two "git-config --add"s when the
source branch is remote.  There is a good chance that some user wants
to merge something different, but in that case they have to specify what
to merge _anyway_.

The behavior is controlled by core.trackremotebranches, and can be
fine-grained to a specific invocation of "git branch" using the new
--track and --no-track options.

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index aa1fdd4..14dc07d 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-branch' [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]
-'git-branch' [-l] [-f] <branchname> [<start-point>]
+'git-branch' [--track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git-branch' (-m | -M) [<oldbranch>] <newbranch>
 'git-branch' (-d | -D) [-r] <branchname>...
 
@@ -25,6 +25,12 @@ It will start out with a head equal to the one given as <start-point>.
 If no <start-point> is given, the branch will be created with a head
 equal to that of the currently checked out branch.
 
+When a local branch is started off a remote branch, git will setup
+the branch so that gitlink:git-pull[1] will appropriately merge from
+that remote branch.  If this behavior is undesired, it is possible
+to change it using the `core.trackremotebranches` option, or the
+`--track` and `--no-track` options.
+
 With a '-m' or '-M' option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
 <newbranch>, and a reflog entry is created to remember the branch
diff --git a/builtin-branch.c b/builtin-branch.c
index d0179b0..20de049 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -12,7 +12,7 @@
 #include "builtin.h"
 
 static const char builtin_branch_usage[] =
-  "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
+  "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>]]";
 
 #define REF_UNKNOWN_TYPE    0x00
 #define REF_LOCAL_BRANCH    0x01
@@ -308,15 +307,36 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
 	free_ref_list(&ref_list);
 }
 
+static void register_branch_pull (const char *name, const char *remote_name)
+{
+	char *slash = strchr(remote_name, '/');
+
+	char *config_key = xmalloc(strlen(name) + 15);
+	char *merge_value = xmalloc(strlen(remote_name) + 10);
+
+	char *remote_value = xstrdup(remote_name);
+	remote_value[slash - remote_name] = 0;
+	sprintf(config_key, "branch.%s.remote", name);
+	git_config_set(config_key, remote_value);
+
+	sprintf(merge_value, "refs/heads/%s", slash + 1);
+	sprintf(config_key, "branch.%s.merge", name);
+	git_config_set(config_key, merge_value);
+
+	free (config_key);
+	free (remote_value);
+	free (merge_value);
+}
+
 static void create_branch(const char *name, const char *start_name,
 			  unsigned char *start_sha1,
-			  int force, int reflog)
+			  int force, int reflog, int track)
 {
 	struct ref_lock *lock;
 	struct commit *commit;
 	unsigned char sha1[20];
-	char ref[PATH_MAX], msg[PATH_MAX + 20];
-	int forcing = 0;
+	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
+	int forcing = 0, remote = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
 	if (check_ref_format(ref))
@@ -333,7 +353,9 @@ static void create_branch(const char *name, const char *start_name,
 	if (start_sha1)
 		/* detached HEAD */
 		hashcpy(sha1, start_sha1);
-	else if (get_sha1(start_name, sha1))
+	else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
+		remote = !prefixcmp(real_ref, "refs/remotes/");
+	else
 		die("Not a valid object name: '%s'.", start_name);
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
@@ -354,8 +376,16 @@ static void create_branch(const char *name, const char *start_name,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	/* When branching off a remote branch, set up so that git-pull
+	   automatically merges from there.  */
+	if (remote && track)
+		register_branch_pull (name, real_ref + 13);
+
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
+
+	if (real_ref)
+		free (real_ref);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -397,11 +427,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, force_delete = 0, force_create = 0;
 	int rename = 0, force_rename = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
-	int reflog = 0;
+	int reflog = 0, track;
 	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
 	git_config(git_branch_config);
+	track = track_remote_branches;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -412,6 +443,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			i++;
 			break;
 		}
+		if (!strcmp(arg, "--track")) {
+			track = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--no-track")) {
+			track = 0;
+			continue;
+		}
 		if (!strcmp(arg, "-d")) {
 			delete = 1;
 			continue;
@@ -490,9 +529,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	else if (rename && (i == argc - 2))
 		rename_branch(argv[i], argv[i + 1], force_rename);
 	else if (i == argc - 1)
-		create_branch(argv[i], head, head_sha1, force_create, reflog);
+		create_branch(argv[i], head, head_sha1, force_create, reflog,
+			      track);
 	else if (i == argc - 2)
-		create_branch(argv[i], argv[i+1], NULL, force_create, reflog);
+		create_branch(argv[i], argv[i+1], NULL, force_create, reflog,
+			      track);
 	else
 		usage(builtin_branch_usage);
 
diff --git a/cache.h b/cache.h
index 8bbc142..585a9b4 100644
--- a/cache.h
+++ b/cache.h
@@ -205,6 +205,7 @@ extern int trust_executable_bit;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
+extern int track_remote_branches;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
diff --git a/config.c b/config.c
index 0ff413b..49df7bd 100644
--- a/config.c
+++ b/config.c
@@ -294,6 +294,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.trackremotebranches")) {
+		track_remote_branches = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.legacyheaders")) {
 		use_legacy_headers = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 570e32a..e440d05 100644
--- a/environment.c
+++ b/environment.c
@@ -17,6 +17,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
+int track_remote_branches = 1;
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 char *git_commit_encoding;

Re: [PATCH] defaults for where to merge from (take 3)

[Alex Riesen]

On 3/1/07, Paolo Bonzini <paolo.bonzini@gmail.com> wrote:
> > Actually, how about making the default configurable _AND_ have
> > the --no-track option (for scripting)?
>
> Fine by me.  And also --track in case you want to try it out.  ;-)
>

Ack. Thanks!

> Patch attached.  --remove-section will go in a separate patch.

Still think it is a very dangerous operation.

Re: [PATCH] defaults for where to merge from (take 3)

[Paolo Bonzini]

>> Patch attached.  --remove-section will go in a separate patch.
> 
> Still think it is a very dangerous operation.

Agreed, that's why I put it in git-config.

Paolo

Re: [PATCH] defaults for where to merge from (take 3)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 3/1/07, Paolo Bonzini <paolo.bonzini@gmail.com> wrote:
>> > Actually, how about making the default configurable _AND_ have
>> > the --no-track option (for scripting)?
>>
>> Fine by me.  And also --track in case you want to try it out.  ;-)
>
> Ack. Thanks!

Having both --track and --no-track options is a nice touch to
give scripts dependable behaviour.  Well done.

>> Patch attached.  --remove-section will go in a separate patch.
>
> Still think it is a very dangerous operation.

I am going to bed now, but I would appreciate if the list could
help Paolo:

 (1) with styles.  I have only given a cursory look at the
     patch, but I think people already know what I like and not
     like.

 (2) by reviewing the changes to the .config writer.  That
     traditionally has been one of the more fragile parts of the
     system, and I am reluctant to look at it.

 (3) come up with a version that is easier-to-apply (including
     sending an in-line patch).

I would just feel better to see a patch like this, which is a
significant improvement to the system, to be properly signed-off
by the submitter.

Also it would be nice if you guys can fight it out about the
default value for 'tracked'.  I do not think _I_ can defend the
position to create these tracking configurations by default to
old timers (especially the ones that do not follow the git
mailing list), as I am not convinced (not yet, anyway).

Re: [PATCH] defaults for where to merge from (take 3)

[Alex Riesen]

On 3/1/07, Junio C Hamano <junkio@cox.net> wrote:
> Also it would be nice if you guys can fight it out about the
> default value for 'tracked'.  I do not think _I_ can defend the
> position to create these tracking configurations by default to
> old timers (especially the ones that do not follow the git
> mailing list), as I am not convinced (not yet, anyway).

I agree with Dscho wrt the default, partly because I don't
care as long as I can change it, partly because the learning
curve curve can never be shallow enough and the less
surprises the better.
The oldtimers will have no problems changing the default,
and the option is properly documented in the patch.

Re: [PATCH] defaults for where to merge from (take 3)

[Alex Riesen]

On 3/1/07, Junio C Hamano <junkio@cox.net> wrote:
> Also it would be nice if you guys can fight it out about the
> default value for 'tracked'.  I do not think _I_ can defend the
> position to create these tracking configurations by default to
> old timers (especially the ones that do not follow the git
> mailing list), as I am not convinced (not yet, anyway).

BTW, how about printing a message that the newly created
branch is a tracker of that remote branch?

  $ git branch abc origin/master
  Branch "abc" tracks "remotes/origin/master"
  $ _

Re: [PATCH] defaults for where to merge from (take 3, inline)

[Paolo Bonzini]

>  (2) by reviewing the changes to the .config writer.  That
>      traditionally has been one of the more fragile parts of the
>      system, and I am reluctant to look at it.

Just FYI, this was broken into a separate patch.

> I would just feel better to see a patch like this, which is a
> significant improvement to the system, to be properly signed-off
> by the submitter.

I hope this is better, I tried to follow the steps in
SubmittingPatches properly.  The code is actually the same
as take 3.


* git-branch: register where to merge from, when branching off a remote branch.

A rather standard (in 1.5) procedure for branching off a remote archive is:

  git checkout -b branchname remote/upstreambranch
  git config --add branch.branchname.remote remote
  git config --add branch.branchname.merge refs/heads/upstreambranch

In this case, we can save the user some effort if "git branch" (and
"git checkout -b") automatically do the two "git-config --add"s when the
source branch is remote.  There is a good chance that some user wants
to merge something different, but in that case they have to specify what
to merge _anyway_.

The behavior is controlled by core.trackremotebranches, and can be
fine-grained to a specific invocation of "git branch" using the new
--track and --no-track options.

Signed-off-by: Paolo Bonzini  <bonzini@gnu.org>
---
 Documentation/git-branch.txt |    8 +++++
 builtin-branch.c             |   58 +++++++++++++++++++++++++++++++++++++------
 cache.h                      |    1 
 config.c                     |    5 +++
 environment.c                |    1 
 5 files changed, 64 insertions(+), 9 deletions(-)


diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index aa1fdd4..14dc07d 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-branch' [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]
-'git-branch' [-l] [-f] <branchname> [<start-point>]
+'git-branch' [--track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git-branch' (-m | -M) [<oldbranch>] <newbranch>
 'git-branch' (-d | -D) [-r] <branchname>...
 
@@ -25,6 +25,12 @@ It will start out with a head equal to the one given as <start-point>.
 If no <start-point> is given, the branch will be created with a head
 equal to that of the currently checked out branch.
 
+When a local branch is started off a remote branch, git will setup
+the branch so that gitlink:git-pull[1] will appropriately merge from
+that remote branch.  If this behavior is undesired, it is possible
+to change it using the `core.trackremotebranches` option, or the
+`--track` and `--no-track` options.
+
 With a '-m' or '-M' option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
 <newbranch>, and a reflog entry is created to remember the branch
diff --git a/builtin-branch.c b/builtin-branch.c
index d0179b0..20de049 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -12,7 +12,7 @@
 #include "builtin.h"
 
 static const char builtin_branch_usage[] =
-  "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
+  "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>]]";
 
 #define REF_UNKNOWN_TYPE    0x00
 #define REF_LOCAL_BRANCH    0x01
@@ -308,15 +307,36 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
 	free_ref_list(&ref_list);
 }
 
+static void register_branch_pull (const char *name, const char *remote_name)
+{
+	char *slash = strchr(remote_name, '/');
+
+	char *config_key = xmalloc(strlen(name) + 15);
+	char *merge_value = xmalloc(strlen(remote_name) + 10);
+
+	char *remote_value = xstrdup(remote_name);
+	remote_value[slash - remote_name] = 0;
+	sprintf(config_key, "branch.%s.remote", name);
+	git_config_set(config_key, remote_value);
+
+	sprintf(merge_value, "refs/heads/%s", slash + 1);
+	sprintf(config_key, "branch.%s.merge", name);
+	git_config_set(config_key, merge_value);
+
+	free (config_key);
+	free (remote_value);
+	free (merge_value);
+}
+
 static void create_branch(const char *name, const char *start_name,
 			  unsigned char *start_sha1,
-			  int force, int reflog)
+			  int force, int reflog, int track)
 {
 	struct ref_lock *lock;
 	struct commit *commit;
 	unsigned char sha1[20];
-	char ref[PATH_MAX], msg[PATH_MAX + 20];
-	int forcing = 0;
+	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
+	int forcing = 0, remote = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
 	if (check_ref_format(ref))
@@ -333,7 +353,9 @@ static void create_branch(const char *name, const char *start_name,
 	if (start_sha1)
 		/* detached HEAD */
 		hashcpy(sha1, start_sha1);
-	else if (get_sha1(start_name, sha1))
+	else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
+		remote = !prefixcmp(real_ref, "refs/remotes/");
+	else
 		die("Not a valid object name: '%s'.", start_name);
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
@@ -354,8 +376,16 @@ static void create_branch(const char *name, const char *start_name,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	/* When branching off a remote branch, set up so that git-pull
+	   automatically merges from there.  */
+	if (remote && track)
+		register_branch_pull (name, real_ref + 13);
+
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
+
+	if (real_ref)
+		free (real_ref);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -397,11 +427,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, force_delete = 0, force_create = 0;
 	int rename = 0, force_rename = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
-	int reflog = 0;
+	int reflog = 0, track;
 	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
 	git_config(git_branch_config);
+	track = track_remote_branches;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -412,6 +443,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			i++;
 			break;
 		}
+		if (!strcmp(arg, "--track")) {
+			track = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--no-track")) {
+			track = 0;
+			continue;
+		}
 		if (!strcmp(arg, "-d")) {
 			delete = 1;
 			continue;
@@ -490,9 +529,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	else if (rename && (i == argc - 2))
 		rename_branch(argv[i], argv[i + 1], force_rename);
 	else if (i == argc - 1)
-		create_branch(argv[i], head, head_sha1, force_create, reflog);
+		create_branch(argv[i], head, head_sha1, force_create, reflog,
+			      track);
 	else if (i == argc - 2)
-		create_branch(argv[i], argv[i+1], NULL, force_create, reflog);
+		create_branch(argv[i], argv[i+1], NULL, force_create, reflog,
+			      track);
 	else
 		usage(builtin_branch_usage);
 
diff --git a/cache.h b/cache.h
index 8bbc142..585a9b4 100644
--- a/cache.h
+++ b/cache.h
@@ -205,6 +205,7 @@ extern int trust_executable_bit;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
+extern int track_remote_branches;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
diff --git a/config.c b/config.c
index 0ff413b..49df7bd 100644
--- a/config.c
+++ b/config.c
@@ -294,6 +294,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.trackremotebranches")) {
+		track_remote_branches = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.legacyheaders")) {
 		use_legacy_headers = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 570e32a..e440d05 100644
--- a/environment.c
+++ b/environment.c
@@ -17,6 +17,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
+int track_remote_branches = 1;
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 char *git_commit_encoding;

Re: [PATCH] defaults for where to merge from (take 3, inline)

[Johannes Schindelin]

Hi,

please don't take my comments as insults or even strict rules. It is 
purely for your consideration. (I say this because I haven't seen you so 
often on this list, so you might not know that discussions about patches 
are sometimes, erm, lively...)

On Thu, 1 Mar 2007, Paolo Bonzini wrote:

> >  (2) by reviewing the changes to the .config writer.  That
> >      traditionally has been one of the more fragile parts of the
> >      system, and I am reluctant to look at it.
> 
> Just FYI, this was broken into a separate patch.
> 
> > I would just feel better to see a patch like this, which is a
> > significant improvement to the system, to be properly signed-off
> > by the submitter.
> 
> I hope this is better, I tried to follow the steps in
> SubmittingPatches properly.  The code is actually the same
> as take 3.

According to SubmittingPatches, this is a cover letter.

	You often want to add additional explanation about the patch, 
	other than the commit message itself.  Place such "cover letter" 
	material between the three dash lines and the diffstat.

So, please put it after the three dashes and the diffstat next time.

> * git-branch: register where to merge from, when branching off a remote branch.

This is the oneline description, which should have been the Subject of the 
mail, preferably prefixed by "[PATCH]" to make it obvious that it is not 
yet another reply in a medium-sized thread, but actually a code 
contribution. SubmittingPatches is not clear about this: you can write 
_anything_ in brackets, and it will be stripped from the commit message 
automatically. In your case, I would have preferred "[PATCH, 3rd 
version]".

> A rather standard (in 1.5) procedure for branching off a remote archive is:

Since this will go into the commit message, which is usually shown in the 
output of "git log", indented, it would be nice to break lines early.

Again, I think that SubmittingPatches is not totally clear about this: I 
try to maintain a maximum width of 76 characters (which seems to be the 
default with pine -- my mail program -- anyway).

> The behavior is controlled by core.trackremotebranches,

I'd make it obvious here that it is on by default -- even if you state 
that earlier, too.

[I leave comments on documentation to others, since I cannot write them 
myself.]

> diff --git a/builtin-branch.c b/builtin-branch.c
> index d0179b0..20de049 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -12,7 +12,7 @@
>  #include "builtin.h"
>  
>  static const char builtin_branch_usage[] =
> -  "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
> +  "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>]]";
>  
>  #define REF_UNKNOWN_TYPE    0x00
>  #define REF_LOCAL_BRANCH    0x01
> @@ -308,15 +307,36 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
>  	free_ref_list(&ref_list);
>  }
>  
> +static void register_branch_pull (const char *name, const char *remote_name)

It is not yet remote_name, right? it is branch_name. You extract the 
remote_name by finding the first slash.

> +{
> +	char *slash = strchr(remote_name, '/');
> +
> +	char *config_key = xmalloc(strlen(name) + 15);
> +	char *merge_value = xmalloc(strlen(remote_name) + 10);
> +
> +	char *remote_value = xstrdup(remote_name);

I'd use "char key[1024], value[1024]" instead, erroring out if one of the 
buffers are too small. It's not like you have to be memory efficient, and 
it is easier to read.

> +	remote_value[slash - remote_name] = 0;

You should check if slash == NULL and error out before using it.

> +	sprintf(config_key, "branch.%s.remote", name);

This would be a snprintf(key, sizeof(key), "branch.%s.remote", name); and 
snprintf(value, sizeof(value), "%.*s", slash - branch_name, branch_name);

> +	git_config_set(config_key, remote_value);
> +
> +	sprintf(merge_value, "refs/heads/%s", slash + 1);
> +	sprintf(config_key, "branch.%s.merge", name);
> +	git_config_set(config_key, merge_value);
> +
> +	free (config_key);
> +	free (remote_value);
> +	free (merge_value);
> +}
> +
>  static void create_branch(const char *name, const char *start_name,
>  			  unsigned char *start_sha1,
> -			  int force, int reflog)
> +			  int force, int reflog, int track)
>  {
>  	struct ref_lock *lock;
>  	struct commit *commit;
>  	unsigned char sha1[20];
> -	char ref[PATH_MAX], msg[PATH_MAX + 20];
> -	int forcing = 0;
> +	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
> +	int forcing = 0, remote = 0;
>  
>  	snprintf(ref, sizeof ref, "refs/heads/%s", name);
>  	if (check_ref_format(ref))
> @@ -333,7 +353,9 @@ static void create_branch(const char *name, const char *start_name,
>  	if (start_sha1)
>  		/* detached HEAD */
>  		hashcpy(sha1, start_sha1);
> -	else if (get_sha1(start_name, sha1))
> +	else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
> +		remote = !prefixcmp(real_ref, "refs/remotes/");
> +	else
>  		die("Not a valid object name: '%s'.", start_name);

Yes, that is how I imagined it. The rest of your patch looks perfect to 
me.

Ciao,
Dscho

Re: [PATCH] defaults for where to merge from (take 3)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> The oldtimers will have no problems changing the default,
> and the option is properly documented in the patch.

"Old-timers will have no problems" is not a good enough
justification to change the default.  You need to justify _why_
it is _they_ who needs to do the extra configuration.

Re: [PATCH] defaults for where to merge from (take 3, inline)

[Paolo Bonzini]

> please don't take my comments as insults or even strict rules. It is 
> purely for your consideration. (I say this because I haven't seen you so 
> often on this list, so you might not know that discussions about patches 
> are sometimes, erm, lively...)

I absolutely haven't taken any of these comments in the thread as insults (the only thing I found a little dubious, was some usage of uppercase), and I got a lot of constructive criticism that outweighed the "lively" tone.  And as a mistake on my part, I probably should have lurked a bit longer than I did.

>> +static void register_branch_pull (const char *name, const char *remote_name)
> 
> It is not yet remote_name, right? it is branch_name. You extract the 
> remote_name by finding the first slash.

Yeah, it's a remote_branch_name in fact.

> I'd use "char key[1024], value[1024]" instead, erroring out if one of the 
> buffers are too small. It's not like you have to be memory efficient, and 
> it is easier to read.

Ok.

>> +	remote_value[slash - remote_name] = 0;
> 
> You should check if slash == NULL and error out before using it.

remote_name is of the form "REMOTE/BRANCH", because it comes from dwim_ref's output after stripping "refs/remotes/" from the beginning.

> Yes, that is how I imagined it. The rest of your patch looks perfect to 
> me.

I will submit again with the requested changes.  I guess the body of this message is too long to become a "cover letter".

Paolo

[PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Paolo Bonzini]

A rather standard (in 1.5) procedure for branching off a remote archive
is:

  git checkout -b branchname remote/upstreambranch
  git config --add branch.branchname.remote remote
  git config --add branch.branchname.merge refs/heads/upstreambranch

In this case, we can save the user some effort if "git branch" (and
"git checkout -b") automatically do the two "git-config --add"s when the
source branch is remote.  There is a good chance that some user wants
to merge something different, but in that case they have to specify what
to merge _anyway_.

The behavior is controlled by core.trackremotebranches (off by default;
subject to review later), and can be fine-grained to a specific invocation
of "git branch" using the new --track and --no-track options.

Signed-off-by: Paolo Bonzini  <bonzini@gnu.org>
---
 Documentation/git-branch.txt |    9 ++++++
 builtin-branch.c             |   56 ++++++++++++++++++++++++++++++++++++-------
 cache.h                      |    1 
 config.c                     |    5 +++
 environment.c                |    1 
 5 files changed, 63 insertions(+), 9 deletions(-)

	Includes comments by Johannes Schindelin on not using xmalloc for
	buffers, and better variable names.  Default is "false" in this
	version, unlike previous versions.

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index aa1fdd4..4ccbb3c 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git-branch' [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]
-'git-branch' [-l] [-f] <branchname> [<start-point>]
+'git-branch' [--track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git-branch' (-m | -M) [<oldbranch>] <newbranch>
 'git-branch' (-d | -D) [-r] <branchname>...
 
@@ -25,6 +25,13 @@ It will start out with a head equal to the one given as <start-point>.
 If no <start-point> is given, the branch will be created with a head
 equal to that of the currently checked out branch.
 
+When a local branch is started off a remote branch, git can setup
+the branch so that gitlink:git-pull[1] will appropriately merge from
+that remote branch.  If this behavior is desired, it is possible
+to make it the default using the `core.trackremotebranches` option.
+Otherwise, it can be chosen per-branch using the `--track` and
+`--no-track` options.
+
 With a '-m' or '-M' option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
 <newbranch>, and a reflog entry is created to remember the branch
diff --git a/builtin-branch.c b/builtin-branch.c
index d0179b0..96658ff 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -12,7 +12,7 @@
 #include "builtin.h"
 
 static const char builtin_branch_usage[] =
-  "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
+  "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>]]";
 
 #define REF_UNKNOWN_TYPE    0x00
 #define REF_LOCAL_BRANCH    0x01
@@ -308,15 +308,34 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
 	free_ref_list(&ref_list);
 }
 
+static void register_pull (const char *name, const char *remote_branch_name)
+{
+	char *slash = strchr(remote_branch_name, '/');
+	char key[1024], value[1024];
+
+	if (strlen(remote_branch_name) >= 1024 - 11
+	    || strlen(name) >= 1024 - 15)
+		die ("what a long branch name you have!");
+
+	snprintf(key, sizeof(key), "branch.%s.remote", name);
+	snprintf(value, sizeof(value), "%.*s", slash - remote_branch_name,
+		 remote_branch_name);
+	git_config_set(key, value);
+
+	snprintf(key, sizeof(key), "branch.%s.merge", name);
+	snprintf(value, sizeof(value), "refs/heads/%s", slash + 1);
+	git_config_set(key, value);
+}
+
 static void create_branch(const char *name, const char *start_name,
 			  unsigned char *start_sha1,
-			  int force, int reflog)
+			  int force, int reflog, int track)
 {
 	struct ref_lock *lock;
 	struct commit *commit;
 	unsigned char sha1[20];
-	char ref[PATH_MAX], msg[PATH_MAX + 20];
-	int forcing = 0;
+	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
+	int forcing = 0, remote = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
 	if (check_ref_format(ref))
@@ -333,7 +354,9 @@ static void create_branch(const char *name, const char *start_name,
 	if (start_sha1)
 		/* detached HEAD */
 		hashcpy(sha1, start_sha1);
-	else if (get_sha1(start_name, sha1))
+	else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
+		remote = !prefixcmp(real_ref, "refs/remotes/");
+	else
 		die("Not a valid object name: '%s'.", start_name);
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
@@ -354,8 +377,16 @@ static void create_branch(const char *name, const char *start_name,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	/* When branching off a remote branch, set up so that git-pull
+	   automatically merges from there.  */
+	if (remote && track)
+		register_pull (name, real_ref + 13);
+
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
+
+	if (real_ref)
+		free (real_ref);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -397,11 +428,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, force_delete = 0, force_create = 0;
 	int rename = 0, force_rename = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
-	int reflog = 0;
+	int reflog = 0, track;
 	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
 	git_config(git_branch_config);
+	track = track_remote_branches;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -412,6 +444,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			i++;
 			break;
 		}
+		if (!strcmp(arg, "--track")) {
+			track = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--no-track")) {
+			track = 0;
+			continue;
+		}
 		if (!strcmp(arg, "-d")) {
 			delete = 1;
 			continue;
@@ -490,9 +530,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	else if (rename && (i == argc - 2))
 		rename_branch(argv[i], argv[i + 1], force_rename);
 	else if (i == argc - 1)
-		create_branch(argv[i], head, head_sha1, force_create, reflog);
+		create_branch(argv[i], head, head_sha1, force_create, reflog,
+			      track);
 	else if (i == argc - 2)
-		create_branch(argv[i], argv[i+1], NULL, force_create, reflog);
+		create_branch(argv[i], argv[i+1], NULL, force_create, reflog,
+			      track);
 	else
 		usage(builtin_branch_usage);
 
diff --git a/cache.h b/cache.h
index 8bbc142..585a9b4 100644
--- a/cache.h
+++ b/cache.h
@@ -205,6 +205,7 @@ extern int trust_executable_bit;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int log_all_ref_updates;
+extern int track_remote_branches;
 extern int warn_ambiguous_refs;
 extern int shared_repository;
 extern const char *apply_default_whitespace;
diff --git a/config.c b/config.c
index 0ff413b..49df7bd 100644
--- a/config.c
+++ b/config.c
@@ -294,6 +294,11 @@ int git_default_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.trackremotebranches")) {
+		track_remote_branches = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.legacyheaders")) {
 		use_legacy_headers = git_config_bool(var, value);
 		return 0;
diff --git a/environment.c b/environment.c
index 570e32a..e440d05 100644
--- a/environment.c
+++ b/environment.c
@@ -17,6 +17,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
+int track_remote_branches = 0;
 int warn_ambiguous_refs = 1;
 int repository_format_version;
 char *git_commit_encoding;

Re: [PATCH] defaults for where to merge from (take 3)

[Alex Riesen]

On 3/2/07, Junio C Hamano <junkio@cox.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
>
> > The oldtimers will have no problems changing the default,
> > and the option is properly documented in the patch.
>
> "Old-timers will have no problems" is not a good enough
> justification to change the default.  You need to justify _why_
> it is _they_ who needs to do the extra configuration.
>

I don't know. I wont have any trouble, I think.

I don't even care either way (it is documented, it has
configurable default and it can be switched on or off
in deterministic manner), and will be only slightly
pleased if creation of a tracked branch is more
noticeable (see the mail regarding message on
branch creation).

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Junio C Hamano]

Paolo Bonzini <paolo.bonzini@gmail.com> writes:

> A rather standard (in 1.5) procedure for branching off a remote archive
> is:

Much easier to read, thanks.

Although I'll still nitpick a few points...

>
>   git checkout -b branchname remote/upstreambranch
>   git config --add branch.branchname.remote remote
>   git config --add branch.branchname.merge refs/heads/upstreambranch

Probably a rather standard procedure would be to fork from
remote tracking branch of where you cloned from, i.e. under
'remotes/origin'.  I think the above examples are a bit easier
to read if you said:

    In order to track and build on top of a branch 'topic' you
    track from your upstream repository, you often would end up
    doing this sequence:

    $ git checkout -b topic origin/topic
    $ git config --add branch.branchname.remote origin
    $ git config --add branch.branchname.merge refs/heads/topic

    to fork your own 'topic' branch from the corresponding
    branch you track from the 'origin' repository, and set up
    two configuration variables so that 'git pull' without
    parameters does the right thing while you are on your own
    'topic' branch.

    This commit teaches --track option to git-branch, so that
    "git branch --track topic origin/topic" performs the latter
    two actions when creating your 'topic' branch.  By setting
    configuration variable 'branch.trackremotebranches' to true,
    you do not have to pass --track option explicitly (the
    configuration variable is off by default, and there is a
    --no-track option to countermand it even if the variable is
    set).

    Signed-off-by: ...

I do not think a porcelain level command 'branch' should
introduce core.* configuration variables.

I have a feeling that "git checkout -b" and "git checkout -B"
should be taught to explicitly use "git branch --no-track" and
"git branch --track" to create a new branch (currently it does
not even use "git branch" as far as I can tell).  With your
patch, I suspect that you have to say "git branch topic
origin/topic" and then "git checkout topic", which means you
made the three-step process into two steps, but you could have
made it into one step.  I'll send out an untested patch to
git-checkout so that you can try it out in a separate message.

> diff --git a/builtin-branch.c b/builtin-branch.c
> index d0179b0..96658ff 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -308,15 +308,34 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
>  	free_ref_list(&ref_list);
>  }
>  
> +static void register_pull (const char *name, const char *remote_branch_name)
> +{
> +	char *slash = strchr(remote_branch_name, '/');
> +	char key[1024], value[1024];
> +
> +	if (strlen(remote_branch_name) >= 1024 - 11
> +	    || strlen(name) >= 1024 - 15)
> +		die ("what a long branch name you have!");
> +
> +	snprintf(key, sizeof(key), "branch.%s.remote", name);
> +	snprintf(value, sizeof(value), "%.*s", slash - remote_branch_name,
> +		 remote_branch_name);
> +	git_config_set(key, value);
> +
> +	snprintf(key, sizeof(key), "branch.%s.merge", name);
> +	snprintf(value, sizeof(value), "refs/heads/%s", slash + 1);
> +	git_config_set(key, value);
> +}
> +

 - (minor style) No SP between "register_pull" and "(".  Found
   elsewhere as well.

 - (minor style) I tend to prefer pure declarations before decls
   with initializer.  I.e. "char key[], value[]" first then
   "char *slash".

 - (discipline) Not 1024 in the comparison.  sizeof(key) or
   sizeof(value).

 - (micronit) Is it true that both strlen() tests are about long
   *branch* names?

 - (style and discipline) If you use snprintf(), it is usually
   easier to check its return value to see if you would have
   overflowed, without having the if() statement to check the
   length upfront.  As the code gets updated, you may need to
   change the snprintf() format strings later, and you can
   forget making a matching change to the condition in if()
   statement with the patch above.

 - (moderately serious) The code blindly trusts that
   "refs/remotes/foo/bar" tracks "refs/heads/bar" from remote
   named "foo", which is a bit disturbing.  With the default
   configuration git-clone and git-remote creates, it always is
   the case, but I suspect you might want to at least verify
   that assumption (the user can have different settings in the
   config), if not figuring them out by reading the existing
   configuration yourself.

> @@ -333,7 +354,9 @@ static void create_branch(const char *name, const char *start_name,
>  	if (start_sha1)
>  		/* detached HEAD */
>  		hashcpy(sha1, start_sha1);
> -	else if (get_sha1(start_name, sha1))
> +	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
> +		remote = !prefixcmp(real_ref, "refs/remotes/");
> +	else
>  		die("Not a valid object name: '%s'.", start_name);
>  
>  	if ((commit = lookup_commit_reference(sha1)) == NULL)

 - (pure question) What happens if dwim_ref() returns more than one?

> diff --git a/config.c b/config.c
> index 0ff413b..49df7bd 100644
> --- a/config.c
> +++ b/config.c
> @@ -294,6 +294,11 @@ int git_default_config(const char *var, const char *value)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "core.trackremotebranches")) {
> +		track_remote_branches = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (!strcmp(var, "core.legacyheaders")) {
>  		use_legacy_headers = git_config_bool(var, value);
>  		return 0;

 - (mild objection) Does this belong to git_default_config()?  I
   would have expected this to appear in git_branch_config().

> diff --git a/environment.c b/environment.c
> index 570e32a..e440d05 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -17,6 +17,7 @@ int assume_unchanged;
>  int prefer_symlink_refs;
>  int is_bare_repository_cfg = -1; /* unspecified */
>  int log_all_ref_updates = -1; /* unspecified */
> +int track_remote_branches = 0;
>  int warn_ambiguous_refs = 1;
>  int repository_format_version;
>  char *git_commit_encoding;

 - (style and discipline) No need to initialize global int to
   0.  BSS would take care of it.

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> I have a feeling that "git checkout -b" and "git checkout -B"
> should be taught to explicitly use "git branch --no-track" and
> "git branch --track" to create a new branch (currently it does
> not even use "git branch" as far as I can tell).  With your
> patch, I suspect that you have to say "git branch topic
> origin/topic" and then "git checkout topic", which means you
> made the three-step process into two steps, but you could have
> made it into one step.  I'll send out an untested patch to
> git-checkout so that you can try it out in a separate message.

This does not add -B because I haven't applied your patch to my
tree yet, but it should be obvious where to add --track/--no-track.

Oh, as usual, I only have tested it once, so it may or may not
work.


diff --git a/git-checkout.sh b/git-checkout.sh
index 14835a4..bdf5cdf 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -33,7 +33,7 @@ while [ "$#" != "0" ]; do
 			die "git checkout: we do not like '$newbranch' as a branch name."
 		;;
 	"-l")
-		newbranch_log=1
+		newbranch_log=-l
 		;;
 	"-f")
 		force=1
@@ -235,11 +235,7 @@ fi
 #
 if [ "$?" -eq 0 ]; then
 	if [ "$newbranch" ]; then
-		if [ "$newbranch_log" ]; then
-			mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$newbranch")
-			touch "$GIT_DIR/logs/refs/heads/$newbranch"
-		fi
-		git-update-ref -m "checkout: Created from $new_name" "refs/heads/$newbranch" $new || exit
+		git branch $newbranch_log "$newbranch" "$new_name" || exit
 		branch="$newbranch"
 	fi
 	if test -n "$branch"

Re: [PATCH] defaults for where to merge from (take 3)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> ..., and will be only slightly
> pleased if creation of a tracked branch is more
> noticeable (see the mail regarding message on
> branch creation).

I would very much agree on this part, regardless of what the
default would end up to be.

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Jeff King]

On Fri, Mar 02, 2007 at 01:52:29AM -0800, Junio C Hamano wrote:

>  - (moderately serious) The code blindly trusts that
>    "refs/remotes/foo/bar" tracks "refs/heads/bar" from remote
>    named "foo", which is a bit disturbing.  With the default
>    configuration git-clone and git-remote creates, it always is
>    the case, but I suspect you might want to at least verify
>    that assumption (the user can have different settings in the
>    config), if not figuring them out by reading the existing
>    configuration yourself.

Sorry to come into this conversation a bit late, but I am catching up on
git reading. I agree that this assumption seems a bit suspect; this is
the exact sort of thing I was envisioning a 'mergeLocal' or similar
config option for; it avoids the need to make the reverse mapping.

In fact, the way I thought about it was that branching might set the
branch.*.branched_from variable. Then any porcelain which wanted
to have a sane default for various operations (merge, rebase, etc) could
use their regular, user-specified config (e.g., merge checks
branch.*.merge; if not set, it defaults to branch.*.branched_from; if
not set, barf).

Thus you are compatible with the config features we have now, but in the
case where we would currently die, this gives another option. It is
safer to remove this variable on branch deletion (I know some people
raised concerns about automatically removing parts of the config file)
because the user would never set 'branched_from'; they would set
'merge'.

IOW, my point is that branching should write the information it _knows_
into the config: branch X was branched from branch Y. It should be up to
the programs to _use_ that information in a reasonable way. As it is
now, this patch makes the decision about how to use the information at
the time of branching, which seems to be the source of a lot of objects.

My 2 cents,
-Peff

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Paolo Bonzini]

> I have a feeling that "git checkout -b" and "git checkout -B"
> should be taught to explicitly use "git branch --no-track" and
> "git branch --track" to create a new branch (currently it does
> not even use "git branch" as far as I can tell).  With your
> patch, I suspect that you have to say "git branch topic
> origin/topic" and then "git checkout topic", which means you
> made the three-step process into two steps, but you could have
> made it into one step.

Well, yes, the next part would have been to patch git-checkout.  But this one is already proving to be complicated enough.  :-)

> I'll send out an untested patch to
> git-checkout so that you can try it out in a separate message.

I saw it.  I tested your patch and it seems to work.
 
>  - (micronit) Is it true that both strlen() tests are about long
>    *branch* names?

If you consider something like "origin/next" to be a branch name, yes.

>  - (moderately serious) The code blindly trusts that
>    "refs/remotes/foo/bar" tracks "refs/heads/bar" from remote
>    named "foo", which is a bit disturbing.  With the default
>    configuration git-clone and git-remote creates, it always is
>    the case, but I suspect you might want to at least verify
>    that assumption (the user can have different settings in the
>    config), if not figuring them out by reading the existing
>    configuration yourself.

Ouch.  Absolutely right, but this means I will prepare the patch later then.

>> +	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
>> +		remote = !prefixcmp(real_ref, "refs/remotes/");
> 
>  - (pure question) What happens if dwim_ref() returns more than one?

Then, real_ref is the one matching sha1.

Considering your other objection about the naming of the variable, what about enabling/disabling the tracking using remote.REMOTENAME.tracklocalsubbranches?

Paolo

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Paolo Bonzini]

> IOW, my point is that branching should write the information it _knows_
> into the config: branch X was branched from branch Y. It should be up to
> the programs to _use_ that information in a reasonable way. As it is
> now, this patch makes the decision about how to use the information at
> the time of branching, which seems to be the source of a lot of objects.

The problem is that this will pollute the configuration file a lot.  I'm starting to wonder if all this branch information really belongs in .git/config at all.

Paolo

Re: [PATCH] defaults for where to merge from (take 3, inline)

[Johannes Schindelin]

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

> >> +	remote_value[slash - remote_name] = 0;
> > 
> > You should check if slash == NULL and error out before using it.
> 
> remote_name is of the form "REMOTE/BRANCH", because it comes from 
> dwim_ref's output after stripping "refs/remotes/" from the beginning.

You can create a new "remote" branch anytime by

	$ git update-ref refs/remotes/lirumlarum HEAD

Your code would realize that it is a remote ref, strip "refs/remotes/", 
and then call strchr("lirumlarum", '/'), which returns NULL.

On a related note, it _might_ make sense to check that the remote 
information is set as expected:

	static const char *remote_name;
	static int found_remote, remote_name_len;

	static int find_remote(const char *key, const char *value) {
		/*
		 * This checks if
		 * remote.<bla>.fetch == refs/heads/*:refs/heads/<bla>/*
		 * where <bla> is the remote_name.
		 */
		if (!prefixcmp(key, "remote.") &&
				!prefixcmp(key + 7, remote_name) && 
				!strcmp(key + 7 + remote_name_len,
					 ".fetch") &&
				value &&
				!prefixcmp(value, 
					"refs/heads/*:refs/remotes/") &&
				!prefixcmp(value + 26, remote_name) &&
				!strcmp(value + 26 + remote_name, "/*")) {
			found_remote = 1;
			return -1; /* stop parsing config */
		}
		return 0;
	}

and then in remote_pull():

	found_remote = 0;
	remote_name = branch_name;
	remote_name_len = slash - branch_name;
	git_config(find_remote);
	if (!found_remote) {
		warn("Remote %s was not created with git-remote; "
			"will not add it as default merge source to %s",
			branch_name, name);
		return;
	}

We could rely on new git users not to fiddle with the remote information 
in the config, and old-timers still using .git/remotes/ or .git/branches 
would be told why the default merge information was not set up.

Ciao,
Dscho

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Jeff King]

On Fri, Mar 02, 2007 at 12:15:59PM +0100, Paolo Bonzini wrote:

> > the time of branching, which seems to be the source of a lot of objects.

Err, this should be "the source of a lot of objections" of course.

> The problem is that this will pollute the configuration file a lot.
> I'm starting to wonder if all this branch information really belongs
> in .git/config at all.

I'm not sure how this pollutes any more than the existing proposal. But
I have always been a bit uncomfortable with automatic editing of the
user config (having used such programs in the past, it always seems to
cause subtle annoyances -- however, I find I don't even use the per-repo
config in most cases, but just the ~/.gitconfig).

Perhaps if we had an inclusion mechanism, all automatically written
configuration could go into $GIT_DIR/auto_config with a big warning at
the top, and the .git/config could include it.

Or are you concerned with just polluting the config namespace? Obviously
we could store per-branch metadata somewhere else, but I think there has
been a push to put it _into_ the config over the past several months,
instead of in a separate file.

-Peff

Re: [PATCH] defaults for where to merge from (take 3, inline)

[Jakub Narebski]

Johannes Schindelin wrote:
>On Thu, 1 Mar 2007, Paolo Bonzini wrote:

>>  
>> +static void register_branch_pull (const char *name, const char
*remote_name)
> 
> It is not yet remote_name, right? it is branch_name. You extract the 
> remote_name by finding the first slash.

I'm bit reluctant about this, as it is not required that remote
names cannot contain slashes. But remotes with slashes would make
separation into remote and tracking branch part more difficult.

>> +{
>> +     char *slash = strchr(remote_name, '/');
>> +
>> +     char *config_key = xmalloc(strlen(name) + 15);
>> +     char *merge_value = xmalloc(strlen(remote_name) + 10);
>> +
>> +     char *remote_value = xstrdup(remote_name);
> 
> I'd use "char key[1024], value[1024]" instead, erroring out if one of the 
> buffers are too small. It's not like you have to be memory efficient, and 
> it is easier to read.

The config parser has lengths limits on fully qualified key name (with
section name and subsection name) _and_ on value name. Use them, please,
instead of dynamic allocation and troubles with those.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: defaults for where to merge from

[J. Bruce Fields]

On Thu, Mar 01, 2007 at 12:18:58AM -0800, Junio C Hamano wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
> 
> >> If everyone hated the old behavior, old users should already be careful
> >> about not git-pull'ing (without options) from any branch but master.  So
> >> they won't see any difference.
> >
> > except for .git/config growing uncontrollably
> 
> I think this should be a new option, not the _modified default_.
> Otherwise it would be harder to sell to olde timers.
> 
> 	$ git checkout -B <newbranch> remotes/<blah>
>         $ git branch --track <newbranch> remotes/<blah>
> 
> might be a good compromise.

Personally I usually use <blah> as <newbranch>.  I might have gone for
something like

	git remote checkout [remote] blah [-b newbranch]

with defaulted to newbranch = blah.

--b.

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Johannes Schindelin]

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

> >  - (micronit) Is it true that both strlen() tests are about long
> >    *branch* names?

Yes. "name" refers to the _new_ branch, and "remote_branch_name" refers to 
the remote branch.

> >  - (moderately serious) The code blindly trusts that
> >    "refs/remotes/foo/bar" tracks "refs/heads/bar" from remote
> >    named "foo", which is a bit disturbing.  With the default
> >    configuration git-clone and git-remote creates, it always is
> >    the case, but I suspect you might want to at least verify
> >    that assumption (the user can have different settings in the
> >    config), if not figuring them out by reading the existing
> >    configuration yourself.
> 
> Ouch.  Absolutely right, but this means I will prepare the patch later 
> then.

I really recommend doing what I said in another reply: check that 
the remote information in the config for that remote meets our 
expectations. And do nothing at all if it does not (maybe warn that no 
branch.<foo> voodoo was done).

> >> +	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
> >> +		remote = !prefixcmp(real_ref, "refs/remotes/");
> > 
> >  - (pure question) What happens if dwim_ref() returns more than one?
> 
> Then, real_ref is the one matching sha1.

Which one ;-)

What Junio tried to get at: if you have "refs/heads/my" and 
"refs/remotes/origin/my", dwim_ref("my", ...) returns 2 (or even more, if 
you have other refs ending in "/my").

Please test if the return value is exactly 1, and if it is not, do 
nothing.

Ciao,
Dscho

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Paolo Bonzini]

>>>> +	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
>>>> +		remote = !prefixcmp(real_ref, "refs/remotes/");
>>>  - (pure question) What happens if dwim_ref() returns more than one?
>> Then, real_ref is the one matching sha1.
> 
> Which one ;-)

Matching "the variable named" sha1. :-)

> What Junio tried to get at: if you have "refs/heads/my" and 
> "refs/remotes/origin/my", dwim_ref("my", ...) returns 2 (or even more, if 
> you have other refs ending in "/my").

But the sha1 and the real_ref are always consistent.  If I get refs/heads/my (and a non-remote will always override the remote), the sha1 is non remote.  If I get refs/remotes/origin/my (which triggers the magic), the sha1 is remote.

Still, let's put this patch on hold, I have to understand more about git before proposing something that is clearly beyond my knowledge (as the criticism shows).

Paolo

Re: [PATCH, 4th version] git-branch: register where to merge from, when branching off a remote branch

[Johannes Schindelin]

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

> >>>> +	else if (dwim_ref(start_name, strlen(start_name), sha1, &real_ref))
> >>>> +		remote = !prefixcmp(real_ref, "refs/remotes/");
> >>>  - (pure question) What happens if dwim_ref() returns more than one?
> >> Then, real_ref is the one matching sha1.
> > 
> > Which one ;-)
> 
> Matching "the variable named" sha1. :-)
> 
> > What Junio tried to get at: if you have "refs/heads/my" and 
> > "refs/remotes/origin/my", dwim_ref("my", ...) returns 2 (or even more, 
> > if you have other refs ending in "/my").
> 
> But the sha1 and the real_ref are always consistent.  If I get 
> refs/heads/my (and a non-remote will always override the remote), the 
> sha1 is non remote.  If I get refs/remotes/origin/my (which triggers the 
> magic), the sha1 is remote.

Okay, but you can have "refs/remotes/origin/my" and 
"refs/remotes/paolo/my".

> Still, let's put this patch on hold, I have to understand more about git 
> before proposing something that is clearly beyond my knowledge (as the 
> criticism shows).

I think you are doing fine. Since there is a lot of discussion about this 
feature, it does clearly not fail the mark. A little bit more work, and it 
is ready for inclusion IMHO.

Ciao,
Dscho