[PATCH 1/3] git-branch: add --track and --no-track options

[PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

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 mytopic origin/topic
  git config --add branch.mytopic.remote origin
  git config --add branch.mytopic.merge refs/heads/topic

This would first fork your own 'mytopic' branch from the 'topic'
branch you track from the 'origin' repository; then it would set up two
configuration variables so that 'git pull' without parameters does the
right thing while you are on your own 'mytopic' branch.

This commit adds a --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
'remote.REMOTENAME.trackintolocalbranches' to true, you do not have to
pass the --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: Paolo Bonzini  <bonzini@gnu.org>
---
 Documentation/git-branch.txt |    9 ++-
 builtin-branch.c             |  126 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 127 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 3ea3b80..bd65b98 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git-branch' [--color | --no-color] [-r | -a]
 	   [-v [--abbrev=<length> | --no-abbrev]]
-'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>...
 
@@ -26,6 +26,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 `trackintolocalbranches` option of the
+corresponding remote.  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 d371849..f39759b 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> | --no-abbrev]]";
+  "git-branch [-r] (-d | -D) <branchname> | [--track | --no-track] [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length> | --no-abbrev]]";
 
 #define REF_UNKNOWN_TYPE    0x00
 #define REF_LOCAL_BRANCH    0x01
@@ -308,14 +308,105 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
 	free_ref_list(&ref_list);
 }
 
+
+static int config_track;
+static char *config_repo;
+static const char *start_ref;
+static int start_len;
+static int remote_len;
+
+static void get_remote_branch_name(const char* value)
+{
+	int len_first = -1, len_second = -1;
+	if (*value == '+')
+		value++;
+
+	/* Try an exact match first.  */
+	sscanf(value, "refs/%*[^:]:%n", &len_first);
+	if (len_first != -1
+	    && !strcmp(value + len_first, start_ref)) {
+		/* Truncate the value before the colon.  */
+		asprintf(&config_repo, "%.*s", len_first - 1, value);
+		return;
+	}
+
+	/* Try with a wildcard match now.  */
+	sscanf(value, "refs/%*[^/]/*:%nrefs/remotes/%*[^/]/*%n",
+	       &len_first, &len_second);
+	if (len_first != -1 && len_second != -1
+	    && (len_second - 2) - len_first == remote_len + 13
+	    && !strncmp(value + len_first, start_ref, remote_len + 13)) {
+		/* Replace the star with the remote branch name.  */
+		asprintf(&config_repo, "%.*s%s",
+			 len_first - 3, value,
+			 start_ref + remote_len + 13);
+	}
+}
+
+static int get_remote_config(const char* key, const char* value)
+{
+        if (!prefixcmp(key, "remote.") &&
+            !strncmp(key + 7, start_ref + 13, remote_len)) {
+		if (config_track == -1
+		    && !strcmp(key + 7 + remote_len, ".trackintolocalbranches"))
+			config_track = git_config_bool(key, value);
+
+		else if (!strcmp(key + 7 + remote_len, ".fetch"))
+			get_remote_branch_name(value);
+        }
+        return 0;
+}
+
+static void register_pull(const char *name, const char *real_ref, int track)
+{
+	char key[1024], value[1024];
+	const char *remote_name = real_ref + 13;
+	const char *slash = strchr(remote_name, '/');
+
+	if (!slash)
+		return;
+
+	start_ref = real_ref;
+	start_len = strlen(real_ref);
+	remote_len = slash - remote_name;
+	config_track = track;
+	git_config(get_remote_config);
+
+	/* Change to != 0 to enable this feature by default.  */
+	if (config_track == 1 && config_repo) {
+		if (snprintf(key, sizeof(key), "branch.%s.remote", name)
+		    > sizeof(key)) {
+			die("what a long branch name you have!");
+			goto out;
+		}
+		if (snprintf(value, sizeof(value), "%.*s", remote_len, remote_name)
+		    > sizeof(value)) {
+			die("what a long branch name you have!");
+			goto out;
+		}
+
+		git_config_set(key, value);
+
+		snprintf(key, sizeof(key), "branch.%s.merge", name);
+		git_config_set(key, config_repo);
+
+		printf("Branch %s set up to track remote branch %s.\n",
+		       name, real_ref);
+	}
+
+ out:
+	if (config_repo)
+		free(config_repo);
+}
+
 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];
+	char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
 	int forcing = 0;
 
 	snprintf(ref, sizeof ref, "refs/heads/%s", name);
@@ -333,7 +424,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) > 1)
+		die("Ambiguous object name: '%s'.", start_name);
+	else if (real_ref == NULL)
 		die("Not a valid object name: '%s'.", start_name);
 
 	if ((commit = lookup_commit_reference(sha1)) == NULL)
@@ -354,8 +447,17 @@ 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.  So far, this is only done for
+	   branches registered using git-remote.  */
+	if (!prefixcmp(real_ref, "refs/remotes/"))
+		register_pull(name, real_ref, track);
+
 	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,7 +499,7 @@ 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 = -1;
 	int kinds = REF_LOCAL_BRANCH;
 	int i;
 
@@ -412,6 +514,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;
@@ -498,9 +608,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);
 

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

it is a very good idea to read the information which remote tracks which 
branch from the config, i.e. if you branch "refs/remotes/hallo", to look 
if there is any remote information for that local ref.

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> diff --git a/builtin-branch.c b/builtin-branch.c
> index d371849..f39759b 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> | --no-abbrev]]";
> +  "git-branch [-r] (-d | -D) <branchname> | [--track | --no-track] [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length> | --no-abbrev]]";
>  
>  #define REF_UNKNOWN_TYPE    0x00
>  #define REF_LOCAL_BRANCH    0x01
> @@ -308,14 +308,105 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
>  	free_ref_list(&ref_list);
>  }
>  
> +

extra empty line.

> +static void get_remote_branch_name(const char* value)
> +{
> +	int len_first = -1, len_second = -1;
> +	if (*value == '+')
> +		value++;
> +

> +	/* Try an exact match first.  */
> +	sscanf(value, "refs/%*[^:]:%n", &len_first);

This is the first time I saw that sscanf format type. How portable is it?

> +	if (len_first != -1
> +	    && !strcmp(value + len_first, start_ref)) {

I really would rather do

	const char *p;
	if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
			!strcmp(p + 1, start_ref)) {

> +		/* Truncate the value before the colon.  */
> +		asprintf(&config_repo, "%.*s", len_first - 1, value);

asprintf() is a GNU extension. I guess it is better to just

	config_repo = xstrdup(value);
	config_repo[p - value] = '\0';


> +		return;
> +	}
> +
> +	/* Try with a wildcard match now.  */
> +	sscanf(value, "refs/%*[^/]/*:%nrefs/remotes/%*[^/]/*%n",
> +	       &len_first, &len_second);
> +	if (len_first != -1 && len_second != -1
> +	    && (len_second - 2) - len_first == remote_len + 13
> +	    && !strncmp(value + len_first, start_ref, remote_len + 13)) {
> +		/* Replace the star with the remote branch name.  */
> +		asprintf(&config_repo, "%.*s%s",
> +			 len_first - 3, value,
> +			 start_ref + remote_len + 13);

Same here:

	else if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
			!memcmp(p + 1, start_ref, remote_len) &&
			!strcmp(p + 1 + remote_len, "/*")) {
		config_repo = xstrdup(value);
		config_repo[p - value] = '\0';
	}

BTW I prefer to skip the curly brackets when there is only one statement 
in the block.

Just to be on the safe side, you might want to check here if there are 
more than one remotes "tracking" into start_ref. However, it might not be 
relevant in practice.

> +	}
> +}
> +
> +static int get_remote_config(const char* key, const char* value)
> +{
> +        if (!prefixcmp(key, "remote.") &&
> +            !strncmp(key + 7, start_ref + 13, remote_len)) {
> +		if (config_track == -1
> +		    && !strcmp(key + 7 + remote_len, ".trackintolocalbranches"))

FWIW I don't think .trackIntoLocalBranches" is needed. Opinions?

> +static void register_pull(const char *name, const char *real_ref, int track)

Maybe call it "set_branch_defaults()"?

> +			die("what a long branch name you have!");
> +			goto out;

die() does not return, so no need to "goto out;"... But then, it might be 
nicer to return -1, i.e.

			ret = error("what a long branch name you have!");

and saying
		if (!ret) {
			git_config_set(key, value);
			...
			printf("Branch %s setup...
		}
	}
	if (repo_config)
		free(repo_config);
	return ret;

But I see you made it return "void", so you can skip the "return ret;".

> @@ -333,7 +424,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) > 1)
> +		die("Ambiguous object name: '%s'.", start_name);

I know, I should have said that earlier, but I just found out myself: We 
have a config variable core.warnambiguousrefs, and maybe we should _not_ 
complain and set the defaults when the global variable warn_ambiguous_refs 
is 0.

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

>> +	/* Try an exact match first.  */
>> +	sscanf(value, "refs/%*[^:]:%n", &len_first);
> 
> This is the first time I saw that sscanf format type. How portable is it?

It is.  At the very least it is in OpenGroup.

>> +		/* Truncate the value before the colon.  */
>> +		asprintf(&config_repo, "%.*s", len_first - 1, value);
> 
> asprintf() is a GNU extension. I guess it is better to just
> 
> 	config_repo = xstrdup(value);
> 	config_repo[p - value] = '\0';

git has nfvasprintf -- I'll just use that one.

> FWIW I don't think .trackIntoLocalBranches" is needed. Opinions?

That's because I'd like to make it the default for me...  Also, look at patch 3/3.

>> @@ -333,7 +424,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) > 1)
>> +		die("Ambiguous object name: '%s'.", start_name);
> 
> I know, I should have said that earlier, but I just found out myself: We 
> have a config variable core.warnambiguousrefs, and maybe we should _not_ 
> complain and set the defaults when the global variable warn_ambiguous_refs 
> is 0.

If warn_ambiguous_ref == 0, dwim_ref is never going to answer anything > 1...

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

>> +	/* Try with a wildcard match now.  */
>> +	sscanf(value, "refs/%*[^/]/*:%nrefs/remotes/%*[^/]/*%n",
>> +	       &len_first, &len_second);
>> +	if (len_first != -1 && len_second != -1
>> +	    && (len_second - 2) - len_first == remote_len + 13
>> +	    && !strncmp(value + len_first, start_ref, remote_len + 13)) {
>> +		/* Replace the star with the remote branch name.  */
>> +		asprintf(&config_repo, "%.*s%s",
>> +			 len_first - 3, value,
>> +			 start_ref + remote_len + 13);
> 
> Same here:
> 
> 	else if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
> 			!memcmp(p + 1, start_ref, remote_len) &&
> 			!strcmp(p + 1 + remote_len, "/*")) {
> 		config_repo = xstrdup(value);
> 		config_repo[p - value] = '\0';
> 	}

This is not what my code does.  Are you saying that yours is correct, or that it should actually be the same in practice, or just that I should avoid sscanf/asprintf (which I won't do, since I got mildly insane after writing half of the function without sscanf -- and then decided that the C library is there to be used).

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> >> +	/* Try with a wildcard match now.  */
> >> +	sscanf(value, "refs/%*[^/]/*:%nrefs/remotes/%*[^/]/*%n",
> >> +	       &len_first, &len_second);
> >> +	if (len_first != -1 && len_second != -1
> >> +	    && (len_second - 2) - len_first == remote_len + 13
> >> +	    && !strncmp(value + len_first, start_ref, remote_len + 13)) {
> >> +		/* Replace the star with the remote branch name.  */
> >> +		asprintf(&config_repo, "%.*s%s",
> >> +			 len_first - 3, value,
> >> +			 start_ref + remote_len + 13);
> > 
> > Same here:
> > 
> > 	else if (!prefixcmp(value, "refs/") && (p = strchr(value, ':')) &&
> > 			!memcmp(p + 1, start_ref, remote_len) &&
> > 			!strcmp(p + 1 + remote_len, "/*")) {
> > 		config_repo = xstrdup(value);
> > 		config_repo[p - value] = '\0';
> > 	}
> 
> This is not what my code does.

You are completely correct. I overlooked the need to check for "/*" 
in the first part. So, this should go before the memcmp() line:

			!prefixcmp(p - 2, "/*") &&

I also forgot to replace "*" by the branch name. The if block should look 
like this, then:

		config_repo = xmalloc(p - value + start_len - remote_len);
		strncpy(config_repo, value, p - 1 - value);
		strcat(config_repo, start_ref + remote_len);

As usual, completely untested.

> Are you saying that yours is correct, or that it should actually be the 
> same in practice, or just that I should avoid sscanf/asprintf (which I 
> won't do, since I got mildly insane after writing half of the function 
> without sscanf -- and then decided that the C library is there to be 
> used).

Well, you make a good case there. I am only mildly concerned that this 
might not work on some obscure platforms (including Windows and SunOS), 
and that we are not even realizing that because you do not check the 
return value of sscanf().

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> >> +	/* Try an exact match first.  */
> >> +	sscanf(value, "refs/%*[^:]:%n", &len_first);
> > 
> > This is the first time I saw that sscanf format type. How portable is it?
> 
> It is.  At the very least it is in OpenGroup.

That is not very reassuring for me. However, I learnt something new about 
sscanf(), so it was not lost on me.

> >> +		/* Truncate the value before the colon.  */
> >> +		asprintf(&config_repo, "%.*s", len_first - 1, value);
> > 
> > asprintf() is a GNU extension. I guess it is better to just
> > 
> > 	config_repo = xstrdup(value);
> > 	config_repo[p - value] = '\0';
> 
> git has nfvasprintf -- I'll just use that one.

Which forces you to build a va_list first, and which is horribly 
inefficient. Not that it matters for git-branch...

> > FWIW I don't think .trackIntoLocalBranches" is needed. Opinions?
> 
> That's because I'd like to make it the default for me...  Also, look at 
> patch 3/3.

I am not only mildly opposed to switching trackIntoLocalBranches off. It 
is a _very_ useful feature for new Git users, as it will (I am sure) 
reduce the number of "Huh?" moments dramatically.

Also, I am opposed to make this a remote.<name>.trackIntoLocalBranches 
variable.

IMHO a global flag should be enough, if you really need it at all. Your 
code now makes sure that the defaults will be set only if there is a 
proper fetch entry for the corresponding remote name, so it is safe. 
People wanting to prevent that behaviour can use "--no-track", and those 
who don't know about "--no-track" are probably better off _with_ DWIM 
defaults for the new branch.

> >> @@ -333,7 +424,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) > 1)
> >> +		die("Ambiguous object name: '%s'.", start_name);
> > 
> > I know, I should have said that earlier, but I just found out myself: 
> > We have a config variable core.warnambiguousrefs, and maybe we should 
> > _not_ complain and set the defaults when the global variable 
> > warn_ambiguous_refs is 0.
> 
> If warn_ambiguous_ref == 0, dwim_ref is never going to answer anything > 
> 1...

Right. Thank you!

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

> Well, you make a good case there. I am only mildly concerned that this 
> might not work on some obscure platforms (including Windows and SunOS), 
> and that we are not even realizing that because you do not check the 
> return value of sscanf().

I check that the %n's are written to, instead.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> > Well, you make a good case there. I am only mildly concerned that this 
> > might not work on some obscure platforms (including Windows and 
> > SunOS), and that we are not even realizing that because you do not 
> > check the return value of sscanf().
> 
> I check that the %n's are written to, instead.

Yes, you also check real_ref instead of checking if dwim_ref() returned 0. 
I feel a little bit uneasy about that, since there is no guarantee that 
these values are left untouched, whereas the return value is guaranteed to 
behave as expected.

I also feel a little uneasy about having to parse a format in order to 
parse a string, when you know what that string should look like. For 
example, you could make the code even more compact by asking 
"(p = strstr(value, "/*:refs/remotes/"))".

But if other people do not feel as uneasy about these issues as I do, I 
certainly will not reject your patch.

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

> Yes, you also check real_ref instead of checking if dwim_ref() returned 0. 
> I feel a little bit uneasy about that, since there is no guarantee that 
> these values are left untouched, whereas the return value is guaranteed to 
> behave as expected.

There is.  The man page says "Scanning stops when an input character does not match such a format character."  Scanning includes not processing %n elements, either.

Regarding dwim_ref, dwim_ref says:

int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
{
        const char **p, *r;
        int refs_found = 0;

        *ref = NULL;

and since this "*ref" is not used anyway in the rest of the routine, I figured out that it's part of the interface to set "*ref" to NULL if no ref is found.  Of course it could help (hint hint) if extern functions had a comment stating the interface.

I see however that I also have a "real_ref = NULL" that is actually pointless; I can take it away of course.

> I also feel a little uneasy about having to parse a format in order to 
> parse a string, when you know what that string should look like. For 
> example, you could make the code even more compact by asking 
> "(p = strstr(value, "/*:refs/remotes/"))".

Go down this way and you will say that printf is useless too.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

>>>> +	/* Try an exact match first.  */
>>>> +	sscanf(value, "refs/%*[^:]:%n", &len_first);
>>> This is the first time I saw that sscanf format type. How portable is it?
>> It is.  At the very least it is in OpenGroup.
> 
> That is not very reassuring for me. However, I learnt something new about 
> sscanf(), so it was not lost on me.

Google books reveals that it's called a "scanset" (I didn't know that either).  I found it in "C: The Complete Reference" by Herb Schildt (2000) and "POSIX Programmer's Guide" by D.A. Lewine (1991).  I don't have K&R at hand to check.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> > Yes, you also check real_ref instead of checking if dwim_ref() 
> > returned 0. I feel a little bit uneasy about that, since there is no 
> > guarantee that these values are left untouched, whereas the return 
> > value is guaranteed to behave as expected.
> 
> There is.  The man page says "Scanning stops when an input character 
> does not match such a format character."  Scanning includes not 
> processing %n elements, either.

So, if you want to be that precise, it never says that it does not touch 
the pointers passed to it. But it states very clearly what the return 
value is in case of a failure.

> Regarding dwim_ref, dwim_ref says:
> 
> int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
> {
>         const char **p, *r;
>         int refs_found = 0;
> 
>         *ref = NULL;

Yes, this is what the source does. But again, the return value is what you 
should -- and indeed forever can -- rely on. I am not really happy that 
dwim_ref() touches ref, even if nothing was found, but it is an 
_implementation detail_.

> > I also feel a little uneasy about having to parse a format in order to 
> > parse a string, when you know what that string should look like. For 
> > example, you could make the code even more compact by asking "(p = 
> > strstr(value, "/*:refs/remotes/"))".
> 
> Go down this way and you will say that printf is useless too.

Nah. You are not fair.

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

>> There is.  The man page says "Scanning stops when an input character 
>> does not match such a format character."  Scanning includes not 
>> processing %n elements, either.
> 
> So, if you want to be that precise, it never says that it does not touch 
> the pointers passed to it.

It cannot know that there *are* any pointers unless it goes on scanning the format string.  Since the function it varargs, it cannot *guess* that there are any without scanning that the documentation says does not happen.  Anyway...

> But it states very clearly what the return 
> value is in case of a failure.

Actually ISTR that %n is not counted in the return value and that's why I wrote the code this way.

> Yes, this is what the source does. But again, the return value is what you 
> should -- and indeed forever can -- rely on. I am not really happy that 
> dwim_ref() touches ref, even if nothing was found, but it is an 
> _implementation detail_.

It's not fair to not have any documentation in the code, force the contributor to reverse engineer the documentation, and say it is relying on an implementation detail.  In otherwise undocumented code, implementation == interface.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Junio C Hamano]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

> Actually ISTR that %n is not counted in the return value and that's why I wrote the code this way.

Yes, that is correct.  Some C lib implementations seem to count
%n and others don't so it is not reliable.  That is one of the
reasons I personally have stayed away from fancier sscanf()
constructs, both inside and outside git project.

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Johannes Schindelin]

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

> > Yes, this is what the source does. But again, the return value is what 
> > you should -- and indeed forever can -- rely on. I am not really happy 
> > that dwim_ref() touches ref, even if nothing was found, but it is an 
> > _implementation detail_.
> 
> It's not fair to not have any documentation in the code, force the 
> contributor to reverse engineer the documentation, and say it is relying 
> on an implementation detail.  In otherwise undocumented code, 
> implementation == interface.

Ah, sorry. The "return 0 on success, other values mean different sorts of 
errors" concept is so ubiquituous in Unix-like systems, that nobody 
bothered documenting that.

Actually, we usually use "-1" to mean error, and depending on the function 
have different meanings for ">0".

But the best documentation for a function is actually the code using it, 
and I thought that you had a look at them. Very often, I do "git grep bla" 
to see where the function called "bla()" was used. For me, examples are 
worth a thousand man pages...

A supplemental remark on the sscanf() thing: when I was forced to work on 
Windows, I made it a habit not to rely on specs. So often, things just did 
not work as described, and complaining did not help. For example, we had 
to throw out _all_ uses of templates in a certain project, because the C 
compiler would choke with an internal error.

So, my hesitation regarding sscanf() stems mainly from the fact that it 
appears rarely used to me, and such constructs are more likely to break 
expectations when trying to port to different architectures.

And _that_ is why I do not hesitate using printf(): it is much more likely 
that an incompatibility in printf() is fixed, whereas with sscanf() it 
will likely end up worked-around all the time instead of being fixed.

Ciao,
Dscho

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> 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 mytopic origin/topic
>   git config --add branch.mytopic.remote origin
>   git config --add branch.mytopic.merge refs/heads/topic
>
> This would first fork your own 'mytopic' branch from the 'topic'
> branch you track from the 'origin' repository; then it would set up two
> configuration variables so that 'git pull' without parameters does the
> right thing while you are on your own 'mytopic' branch.
>
> This commit adds a --track option to git-branch, so that "git
> branch --track topic origin/topic" performs the latter two actions
> when creating your 'topic' branch.

Hmm. I think your use of 'mytopic' is very good for the purpose
of illustration. It makes clear which configuration takes name
from what. So I like your first paragraph. However, one reason
people like the separate remote layout is that it allows you to
name your own branch identically with that of the other side, so
in that sense, the description in your second paragraph matches
the real-life usage better. What I am getting at is that (1)
these two paragraphs are inconsistent, (2) there is a reason to
prefer the description in the first paragraph, and (3) there is
another reason to prefer the description in the second one.

> By setting configuration variable
> 'remote.REMOTENAME.trackintolocalbranches' to true, you do not have to
> pass the --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).

As Johannes already pointed this out, I think allowing this to
be controlled per remote is nice but overkill.  A single boolean
configuration, say "branch.autosetupmerge", would suffice.

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

> Yes, that is correct.  Some C lib implementations seem to count
> %n and others don't so it is not reliable.  That is one of the
> reasons I personally have stayed away from fancier sscanf()
> constructs, both inside and outside git project.

So, should I remove it?  As I said, the code using sscanf is already the second try, and the first was utterly illegible, so I'd rather stay with it.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

> Hmm. I think your use of 'mytopic' is very good for the purpose
> of illustration. It makes clear which configuration takes name
> from what. So I like your first paragraph. However, one reason
> people like the separate remote layout is that it allows you to
> name your own branch identically with that of the other side, so
> in that sense, the description in your second paragraph matches
> the real-life usage better. What I am getting at is that (1)
> these two paragraphs are inconsistent, (2) there is a reason to
> prefer the description in the first paragraph, and (3) there is
> another reason to prefer the description in the second one.

Ok, I'll prefer using "mytopic" anywhere.

> As Johannes already pointed this out, I think allowing this to
> be controlled per remote is nice but overkill.  A single boolean
> configuration, say "branch.autosetupmerge", would suffice.

In this case, patch 2/3 should also be withdrawn, right?

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Junio C Hamano]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

>> Hmm. I think your use of 'mytopic' is very good for the purpose
>> of illustration. It makes clear which configuration takes name
>> from what. So I like your first paragraph. However, one reason
>> people like the separate remote layout is that it allows you to
>> name your own branch identically with that of the other side, so
>> in that sense, the description in your second paragraph matches
>> the real-life usage better. What I am getting at is that (1)
>> these two paragraphs are inconsistent, (2) there is a reason to
>> prefer the description in the first paragraph, and (3) there is
>> another reason to prefer the description in the second one.
>
> Ok, I'll prefer using "mytopic" anywhere.
>
>> As Johannes already pointed this out, I think allowing this to
>> be controlled per remote is nice but overkill.  A single boolean
>> configuration, say "branch.autosetupmerge", would suffice.
>
> In this case, patch 2/3 should also be withdrawn, right?

Do you mean you would agree that it is overkill?  Just in case;
you do not have to necessarily agree with me but convince me
your way, if you feel I am wrong.

If so, yeah, 2/3 needs a minor adjustment since the configuration
will not be on remote.* but one configuration variable.

Also I agree with many points Dscho made.  I understand you
agreed to avoid asprintf() from portability worries, which I
think is a sensible thing to do.

While I do not think we should avoid sscanf("%n"), I suspect
that the code in your patch is not helped by using it that much.

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Paolo Bonzini]

> Do you mean you would agree that it is overkill?

It means I don't have much interest in trying to convince you your way.  I will try just once, and I have two arguments to make:

1) I added it in the first place because it made the design of patch 2/3 obvious.  If the shared configuration variable wins, I would withdraw patch 2/3 completely and just let the user use git-config (or "vi"...) to modify the default.

2) Also, I liked per-remote configuration because it gives you a quick view of which remotes you have just because you sometimes cherrypick from them, and which remotes you have because you are basing your work on them.

If you're convinced, I'll send the updated (final?) patch later today (which means you'll get it tomorrow morning in your timezone).  If you're not, I'm not sure I can update the patch today to use the shared configuration variable, but I'll get to that too.

> Also I agree with many points Dscho made.  I understand you
> agreed to avoid asprintf() from portability worries, which I
> think is a sensible thing to do.

Sure, I was somehow convinced that git was already providing a portable version of it.

> While I do not think we should avoid sscanf("%n"), I suspect
> that the code in your patch is not helped by using it that much.

No, it's not.  But it's helped a lot by using sscanf itself, and "%n" is the only way I know to reliably test the return code of sscanf and, in the process, save one strchr and one strlen.

Paolo

Re: [PATCH 1/3] git-branch: add --track and --no-track options

[Junio C Hamano]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

> No, it's not.  But it's helped a lot by using sscanf itself,
> and "%n" is the only way I know to reliably test the return
> code of sscanf and, in the process, save one strchr and one
> strlen.

Ok, let's see if that is true in the next round, then.  I do not
have much against sscanf(%n) as long as its return value is not
depended upon (the portability issue as you mentioned).