Re: [PATCHv3 5/5] branch: allow pattern arguments

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Sep 6, 2011, 14:21 UTC
Message-ID: <4E662C6F.7000205@drmicha.warpmail.net>
In-Reply-To: <4E661BDE.1080204@elegosoft.com>

Michael Schubert venit, vidit, dixit 06.09.2011 15:10:

Show 14 quoted lines

> On 08/28/2011 04:54 PM, Michael J Gruber wrote:
>> +static int match_patterns(const char **pattern, const char *refname)
>> +{
>> +	if (!*pattern)
>> +		return 1; /* no pattern always matches */
>> +	while (*pattern) {
>> +		if (!fnmatch(*pattern, refname, 0))
>> +			return 1;
>> +		pattern++;
>> +	}
>> +	return 0;
>> +}
> 
> Nitpick: maybe builtin/branch.c and builtin/tag.c could share match_pattern().?

I think all commands which list refs should share code - for-each-ref, branch, tag, replace. I suggest to unify log and for-each-ref-formats first, the unify ref listers.

Show 23 quoted lines

> A second thought: the printed "remotes/" prefix could be confusing for users,
> since it seems to be part of the refname. For example:
> 
> $ git branch -a
> * master
>   maint
>   man
>   remotes/origin/master
>   remotes/origin/maint
>   remotes/origin/man
> 
> $ git branch -a --list remotes/origin*
> [no output]
> 
> but
> 
> $ git branch -a --list origin*
>   remotes/origin/master
>   remotes/origin/maint
>   remotes/origin/man
> 
> (Sorry in case I missed that) What's the reason you decided --list to show local
> branches only? Maybe --list could show all refnames without any extra prefix.?

I didn't decide anything here. "git branch" lists all local branches and has been doing that forever, and "--list" is and should be a noop without a pattern. That was the idea: -r/-a select refnames, --list/-v/-vv activate the list mode(s), and the pattern is matched against the selected refs.

(This is one reason why I didn't want it to be named --glob - the pattern is a filter, not a selector.)

That being said, I find the fact that "-a --list remotes/origin/*" does not match anything somewhat disconcerting, although it fits in with the general idea of "git branch": it deals with branch names, not ref names. Have you ever tried to delete a remote branch?

git branch -r -d origin/maint # workee git branch -r -d remotes/origin/maint # no workee

Without -r, it doesn't work either.

So, the way it is it fits in with how "git branch" works, and is different from for-each-ref (and rev-list --glob). I don't like it, because you may have a local branch origin/maint (i.e. refs/heads/origin/maint) which you can't distinguish from the remote branch by a pattern, only by using or not using -r. But it's the same as with "git branch -d", really.

Michael