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

"Alex Riesen" [off-list ref] writes:

On 3/1/07, Paolo Bonzini [off-list ref] wrote:

quoted

quoted

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.

quoted

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

On 3/1/07, Junio C Hamano [off-list ref] 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.

On 3/1/07, Junio C Hamano [off-list ref] 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"
  $ _

 (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  <redacted>
---
 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;

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:

quoted

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

quoted

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

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

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

Ciao,
Dscho

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.

quoted

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

quoted

+	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

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  <redacted>
---
 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;

Paolo Bonzini [off-list ref] 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.

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

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

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

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

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

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.

quoted

+	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

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

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

quoted

quoted

+	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

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

quoted

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

Johannes Schindelin wrote:

On Thu, 1 Mar 2007, Paolo Bonzini wrote:

quoted

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

quoted

+{
+     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

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

quoted

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

quoted

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

quoted

quoted

+	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

quoted

quoted

quoted

+	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

Hi,

On Fri, 2 Mar 2007, Paolo Bonzini wrote:

quoted

quoted

quoted

quoted

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

quoted

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