patch, 3 partsgit-branch: add --track and --no-track options

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

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:

extra empty line.

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

I really would rather do

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

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

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

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.

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

Maybe call it "set_branch_defaults()"?

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

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

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

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

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

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

Paolo

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

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

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

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.

Right. Thank you!

Ciao, Dscho

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

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

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

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.

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

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

Paolo

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.

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

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.

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

Paolo

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

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.

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

Nah. You are not fair.

Ciao, Dscho

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

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

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

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

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

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

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

Paolo Bonzini <bonzini@gnu.org> 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.

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.

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

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

Paolo

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

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.

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.

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

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

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

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