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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 5, 2007, 16:09 UTC
Message-ID: <Pine.LNX.4.63.0703051701331.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <45EC35B2.2070808@lu.unisi.ch>

Hi,

On Mon, 5 Mar 2007, Paolo Bonzini wrote:

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 15 quoted lines

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