[PATCH/RFC] branch: add optional parameter to -r to specify remote

[PATCH/RFC] branch: add optional parameter to -r to specify remote

[Boris Faure]

The following patch adds an optional parameter to branch -r to use as
-r=<remote> in order to list branches on a particular remote.

'git branch -d -r one/another' can also be done with
'git branch -d -r=one another'.

I haven't found tests specially crafted for 'git branch -r -d <branch>'. I
hope the tests I have added are in the correct places.

[PATCH/RFC] branch: add optional parameter to -r to specify remote

[Boris Faure]

add '--remote' as long version for '-r'
update documentation
add tests
---
 Documentation/git-branch.txt |   10 +++--
 builtin/branch.c             |   72 ++++++++++++++++++++++++++++++++----------
 t/t3200-branch.sh            |   34 ++++++++++++++++++++
 t/t3203-branch-output.sh     |   59 ++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index c50f189..242da9c 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -8,12 +8,12 @@ git-branch - List, create, or delete branches
 SYNOPSIS
 --------
 [verse]
-'git branch' [--color[=<when>] | --no-color] [-r | -a]
+'git branch' [--color[=<when>] | --no-color] [-r[=<remote>] | -a]
 	[-v [--abbrev=<length> | --no-abbrev]]
 	[(--merged | --no-merged | --contains) [<commit>]]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
-'git branch' (-d | -D) [-r] <branchname>...
+'git branch' (-d | -D) [-r[=<remote>]] <branchname>...
 
 DESCRIPTION
 -----------
@@ -99,8 +99,10 @@ OPTIONS
 	default to color output.
 	Same as `--color=never`.
 
--r::
-	List or delete (if used with -d) the remote-tracking branches.
+-r[=<remote>]::
+--remote[=<remote>]::
+	List or delete (if used with -d) the remote-tracking branches from
+	<remote> if specified.
 
 -a::
 	List both remote-tracking branches and local branches.
diff --git a/builtin/branch.c b/builtin/branch.c
index d6ab93b..52dff04 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -17,15 +17,18 @@
 #include "revision.h"
 
 static const char * const builtin_branch_usage[] = {
-	"git branch [options] [-r | -a] [--merged | --no-merged]",
+	"git branch [options] [-r[=<remote>] | -a] [--merged | --no-merged]",
 	"git branch [options] [-l] [-f] <branchname> [<start-point>]",
-	"git branch [options] [-r] (-d | -D) <branchname>",
+	"git branch [options] [-r[=<remote>]] (-d | -D) <branchname>",
 	"git branch [options] (-m | -M) [<oldbranch>] <newbranch>",
 	NULL
 };
 
 #define REF_LOCAL_BRANCH    0x01
 #define REF_REMOTE_BRANCH   0x02
+static int kinds = REF_LOCAL_BRANCH;
+
+static const char *remote = NULL;
 
 static const char *head;
 static unsigned char head_sha1[20];
@@ -144,12 +147,12 @@ static int branch_merged(int kind, const char *name,
 	return merged;
 }
 
-static int delete_branches(int argc, const char **argv, int force, int kinds)
+static int delete_branches(int argc, const char **argv, int force)
 {
 	struct commit *rev, *head_rev = NULL;
 	unsigned char sha1[20];
 	char *name = NULL;
-	const char *fmt, *remote;
+	const char *fmt, *remote_trans;
 	int i;
 	int ret = 0;
 	struct strbuf bname = STRBUF_INIT;
@@ -158,12 +161,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 	case REF_REMOTE_BRANCH:
 		fmt = "refs/remotes/%s";
 		/* TRANSLATORS: This is "remote " in "remote branch '%s' not found" */
-		remote = _("remote ");
+		remote_trans = _("remote ");
 		force = 1;
 		break;
 	case REF_LOCAL_BRANCH:
 		fmt = "refs/heads/%s";
-		remote = "";
+		remote_trans = "";
 		break;
 	default:
 		die(_("cannot use -a with -d"));
@@ -175,7 +178,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			die(_("Couldn't look up commit object for HEAD"));
 	}
 	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
-		strbuf_branchname(&bname, argv[i]);
+		if (kinds == REF_REMOTE_BRANCH && remote) {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_addstr(&buf, remote);
+			strbuf_addch(&buf, '/');
+			strbuf_addstr(&buf, argv[i]);
+			strbuf_branchname(&bname, buf.buf);
+			strbuf_release(&buf);
+		} else {
+			strbuf_branchname(&bname, argv[i]);
+		}
 		if (kinds == REF_LOCAL_BRANCH && !strcmp(head, bname.buf)) {
 			error(_("Cannot delete the branch '%s' "
 			      "which you are currently on."), bname.buf);
@@ -188,7 +200,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 		name = xstrdup(mkpath(fmt, bname.buf));
 		if (!resolve_ref(name, sha1, 1, NULL)) {
 			error(_("%sbranch '%s' not found."),
-					remote, bname.buf);
+			      remote_trans, bname.buf);
 			ret = 1;
 			continue;
 		}
@@ -209,12 +221,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 		}
 
 		if (delete_ref(name, sha1, 0)) {
-			error(_("Error deleting %sbranch '%s'"), remote,
+			error(_("Error deleting %sbranch '%s'"), remote_trans,
 			      bname.buf);
 			ret = 1;
 		} else {
 			struct strbuf buf = STRBUF_INIT;
-			printf(_("Deleted %sbranch %s (was %s).\n"), remote,
+			printf(_("Deleted %sbranch %s (was %s).\n"),
+			       remote_trans,
 			       bname.buf,
 			       find_unique_abbrev(sha1, DEFAULT_ABBREV));
 			strbuf_addf(&buf, "branch.%s", bname.buf);
@@ -492,7 +505,7 @@ static void show_detached(struct ref_list *ref_list)
 	}
 }
 
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)
+static int print_ref_list(int detached, int verbose, int abbrev, struct commit_list *with_commit)
 {
 	int i;
 	struct append_ref_cb cb;
@@ -533,6 +546,14 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		char *prefix = (kinds != REF_REMOTE_BRANCH &&
 				ref_list.list[i].kind == REF_REMOTE_BRANCH)
 				? "remotes/" : "";
+		if (kinds == REF_REMOTE_BRANCH && remote &&
+		    ref_list.list[i].kind == REF_REMOTE_BRANCH &&
+		    ref_list.list[i].name) {
+			int remote_len = strlen(remote);
+			if (strncmp(remote, ref_list.list[i].name, strlen(remote)) ||
+			       ref_list.list[i].name[remote_len] != '/')
+				continue;
+		}
 		print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
 			       abbrev, current, prefix);
 	}
@@ -610,13 +631,28 @@ static int opt_parse_merge_filter(const struct option *opt, const char *arg, int
 	return 0;
 }
 
+static int parse_opt_remote_cb(const struct option *opt, const char *arg,
+			       int unset)
+{
+	kinds = REF_REMOTE_BRANCH;
+	if (unset)
+		kinds = REF_LOCAL_BRANCH;
+	if (arg) {
+		if ( *arg == '=')
+			remote = arg + 1; /* skip '=' */
+		else
+			remote = arg;
+	} else
+		remote = NULL;
+	return 0;
+}
+
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, force_create = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
 	int reflog = 0;
 	enum branch_track track;
-	int kinds = REF_LOCAL_BRANCH;
 	struct commit_list *with_commit = NULL;
 
 	struct option options[] = {
@@ -628,8 +664,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
 		OPT__COLOR(&branch_use_color, "use colored output"),
-		OPT_SET_INT('r', NULL,     &kinds, "act on remote-tracking branches",
-			REF_REMOTE_BRANCH),
+		{
+			OPTION_CALLBACK, 'r', "remote", &kinds, "remote",
+			"act on remote-tracking branches",
+			PARSE_OPT_OPTARG, parse_opt_remote_cb, 0
+		},
 		{
 			OPTION_CALLBACK, 0, "contains", &with_commit, "commit",
 			"print only branches that contain the commit",
@@ -695,11 +734,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			     0);
 	if (!!delete + !!rename + !!force_create > 1)
 		usage_with_options(builtin_branch_usage, options);
-
 	if (delete)
-		return delete_branches(argc, argv, delete > 1, kinds);
+		return delete_branches(argc, argv, delete > 1);
 	else if (argc == 0)
-		return print_ref_list(kinds, detached, verbose, abbrev, with_commit);
+		return print_ref_list(detached, verbose, abbrev, with_commit);
 	else if (rename && (argc == 1))
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 9e69c8c..df2f0f8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -10,6 +10,23 @@ handled.  Specifically, that a bogus branch is not created.
 '
 . ./test-lib.sh
 
+setup_repository () {
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	>file &&
+	git add file &&
+	test_tick &&
+	git commit -m "Initial" &&
+	git checkout -b side &&
+	>elif &&
+	git add elif &&
+	test_tick &&
+	git commit -m "Second" &&
+	git checkout master
+	)
+}
+
 test_expect_success \
     'prepare a trivial repository' \
     'echo Hello > A &&
@@ -542,4 +559,21 @@ test_expect_success 'attempt to delete a branch merged to its base' '
 	test_must_fail git branch -d my10
 '
 
+test_expect_success 'git branch -d -r=one another should delete another branch from remote one' '
+	setup_repository one &&
+	setup_repository onelong &&
+	(
+		cd one && git branch another
+	) &&
+	git remote add -f one one &&
+	git remote add -f onelong onelong &&
+	git branch -d -r=one another &&
+	test ! -f .git/refs/remotes/one/another &&
+	test ! -f .git/logs/refs/remotes/one/another'
+
+test_expect_success 'git branch -d --remote=one master should delete master branch from remote one' '
+	git branch -d --remote=one master &&
+	test ! -f .git/refs/remotes/one/master &&
+	test ! -f .git/logs/refs/remotes/one/master'
+
 test_done
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 6b7c118..aa8cba6 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -3,6 +3,23 @@
 test_description='git branch display tests'
 . ./test-lib.sh
 
+setup_repository () {
+	mkdir "$1" && (
+	cd "$1" &&
+	git init &&
+	>file &&
+	git add file &&
+	test_tick &&
+	git commit -m "Initial" &&
+	git checkout -b side &&
+	>elif &&
+	git add elif &&
+	test_tick &&
+	git commit -m "Second" &&
+	git checkout master
+	)
+}
+
 test_expect_success 'make commits' '
 	echo content >file &&
 	git add file &&
@@ -43,6 +60,26 @@ test_expect_success 'git branch -r shows remote branches' '
 '
 
 cat >expect <<'EOF'
+  origin/HEAD -> origin/branch-one
+  origin/branch-one
+  origin/branch-two
+EOF
+test_expect_success 'git branch -r=origin shows remote branches from origin' '
+	git branch -r=origin >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+  origin/HEAD -> origin/branch-one
+  origin/branch-one
+  origin/branch-two
+EOF
+test_expect_success 'git branch --remote=origin shows remote branches from origin' '
+	git branch --remote=origin >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
   branch-one
   branch-two
 * master
@@ -78,4 +115,26 @@ test_expect_success 'git branch shows detached HEAD properly' '
 	test_i18ncmp expect actual
 '
 
+
+cat >expect <<'EOF'
+  one/another
+  one/master
+  one/side
+EOF
+
+test_expect_success 'git branch -r=one shows only branches from remote one' '
+	setup_repository one &&
+	setup_repository onelong &&
+	(
+		cd one && git branch another
+	) &&
+	git remote add -f one one &&
+	git remote add -f onelong onelong &&
+	git branch -r=one >actual &&
+	test_cmp expect actual'
+
+test_expect_success 'git branch --remote=one shows only branches from remote one' '
+	git branch --remote=one >actual &&
+	test_cmp expect actual'
+
 test_done
-- 
1.7.6.rc2.4.g36bfb.dirty

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Junio C Hamano]

Boris Faure <billiob@gmail.com> writes:

> add '--remote' as long version for '-r'
> update documentation
> add tests

(style) Sentences begin with a capital letter and ends with a period.

This commit does a lot more than the above, no? It adds an optional remote
name parameter to the existing "-r" option and limits the output to the
remote tracking branches of the remote when it is specified.

> ---

Sign-off?

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index c50f189..242da9c 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> ...
> @@ -99,8 +99,10 @@ OPTIONS
>  	default to color output.
>  	Same as `--color=never`.
>  
> --r::
> -	List or delete (if used with -d) the remote-tracking branches.
> +-r[=<remote>]::
> +--remote[=<remote>]::
> +	List or delete (if used with -d) the remote-tracking branches from
> +	<remote> if specified.

It is now unspecified what the command would do when the optional <remote>
is left unspecified.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index d6ab93b..52dff04 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -17,15 +17,18 @@
>  #include "revision.h"
>  
>  static const char * const builtin_branch_usage[] = {
> -	"git branch [options] [-r | -a] [--merged | --no-merged]",
> +	"git branch [options] [-r[=<remote>] | -a] [--merged | --no-merged]",
>  	"git branch [options] [-l] [-f] <branchname> [<start-point>]",
> -	"git branch [options] [-r] (-d | -D) <branchname>",
> +	"git branch [options] [-r[=<remote>]] (-d | -D) <branchname>",
>  	"git branch [options] (-m | -M) [<oldbranch>] <newbranch>",
>  	NULL
>  };
>  
>  #define REF_LOCAL_BRANCH    0x01
>  #define REF_REMOTE_BRANCH   0x02
> +static int kinds = REF_LOCAL_BRANCH;

This used to be nicely scoped out of global space and got passed around as
parameter, but now it has become a global? I do not see a good reason for
this change.

> +static const char *remote = NULL;

Two issues.

 1. Presumably you wanted to have this change because you have too many
    remotes, way more than two, and wanted to filter the output from
    remotes that you are not interested in. Is it entirely implausible
    that you might be interested in not just one, but two remotes out of
    many remotes you have? A single string variable would not suffice for
    that but you should be able to make this an array of strings.

 2. The name suck for a global variable (same for the "kinds" that is now
    global), and caused the patch to become unnecessarily big by renaming
    concisely named "remote" to ugly "remote_trans" in delete_branches()
    function. In general, local variables are scoped narrower and readers
    can rely on the context to tell what the variable is _for_, so it is
    easy for them to see a variable called "remote" and understand what
    aspects of "remote" the variable is trying to express. On the other
    hand, global variables are used in wider context and has to have more
    descriptive names.  This is about "filtering" set of remote tracking
    branches we deal with to the subset specified by this variable, so it
    at least has to have "filter" or "limit" or something to that effect
    in its name.

> @@ -144,12 +147,12 @@ static int branch_merged(int kind, const char *name,
>  	return merged;
>  }
>  
> -static int delete_branches(int argc, const char **argv, int force, int kinds)
> +static int delete_branches(int argc, const char **argv, int force)
>  {
>  	struct commit *rev, *head_rev = NULL;
>  	unsigned char sha1[20];
>  	char *name = NULL;
> -	const char *fmt, *remote;
> +	const char *fmt, *remote_trans;

Unwarranted change caused by a poor choice of the new global variable
above.

> @@ -610,13 +631,28 @@ static int opt_parse_merge_filter(const struct option *opt, const char *arg, int
>  	return 0;
>  }
>  
> +static int parse_opt_remote_cb(const struct option *opt, const char *arg,
> +			       int unset)
> +{
> +	kinds = REF_REMOTE_BRANCH;
> +	if (unset)
> +		kinds = REF_LOCAL_BRANCH;

What is this "unset" check about? Wouldn't that be an error if the command
line said "--no-remote"?

And you do not return but proceed to look at "arg", presumably to handle a
case where the command line said "--no-remote=foobar"?

> +	if (arg) {
> +		if ( *arg == '=')

(style) Unwanted SP after an open parenthesis.

> +			remote = arg + 1; /* skip '=' */

(style) It is clear enough what this does without the extra comment.

Does this forbid remote names that begin with a "="?  I.e.

	$ git branch -r =temporary

As to the design of the new feature, I see you tried to make it possible
to perform what

	$ git branch -d -r origin/master

does with

	$ git branch -d --remote=origin master

I do not think it is particularly a good idea. Adding yet another way to
do the same thing, unless that new way is vastly superiour (e.g. easier to
use, easier to explain, more efficient to perform, etc.), does not add
much value to the system.

It would make much more sense to restrict this feature to the "listing"
side of the branches.  It would be nice if you can do:

	$ git branch -r --match alice --match bob

to show only remote tracking branches under refs/remotes/{alice,bob}
and also

	$ git branch --match "jk/*"

to show only local topic branches whose names match the given blob.

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Johannes Sixt]

Am 6/20/2011 0:32, schrieb Junio C Hamano:
> It would make much more sense to restrict this feature to the "listing"
> side of the branches.  It would be nice if you can do:

I agree with everything you said earlier and the first sentence above. And
I would have needed the possibility to limit the branch listing to a
particular remote a lot in the past. But...

> 	$ git branch -r --match alice --match bob
> 
> to show only remote tracking branches under refs/remotes/{alice,bob}
> and also
> 
> 	$ git branch --match "jk/*"
> 
> to show only local topic branches whose names match the given blob.

I would hate having to learn a new syntax '--match "jk/*"' when we can
already say

    $ git log --remotes
    $ git log --remotes=alice --remotes=bob
    $ git log --remotes="jk/*"

IMO, it is the right approach to have a long option --remotes with an
optional argument.

-- Hannes

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Jeff King]

On Mon, Jun 20, 2011 at 08:40:32AM +0200, Johannes Sixt wrote:

> > 	$ git branch --match "jk/*"
> > 
> > to show only local topic branches whose names match the given blob.
> 
> I would hate having to learn a new syntax '--match "jk/*"' when we can
> already say
> 
>     $ git log --remotes
>     $ git log --remotes=alice --remotes=bob
>     $ git log --remotes="jk/*"
> 
> IMO, it is the right approach to have a long option --remotes with an
> optional argument.

For that matter, --match should be spelled "--glob", as we already have:

  $ git log --glob='jk/*'

I think having the ref-selection for "git branch" match that of the
revision walker makes sense.

-Peff

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Michael J Gruber]

Jeff King venit, vidit, dixit 20.06.2011 09:03:
> On Mon, Jun 20, 2011 at 08:40:32AM +0200, Johannes Sixt wrote:
> 
>>> 	$ git branch --match "jk/*"
>>>
>>> to show only local topic branches whose names match the given blob.
>>
>> I would hate having to learn a new syntax '--match "jk/*"' when we can
>> already say
>>
>>     $ git log --remotes
>>     $ git log --remotes=alice --remotes=bob
>>     $ git log --remotes="jk/*"
>>
>> IMO, it is the right approach to have a long option --remotes with an
>> optional argument.
> 
> For that matter, --match should be spelled "--glob", as we already have:
> 
>   $ git log --glob='jk/*'
> 
> I think having the ref-selection for "git branch" match that of the
> revision walker makes sense.
> 
> -Peff

Well, "branch" is about refs, and "log" about revs. I'd rather have
"branch" similar to "tag" in that respect (i.e. '-l'). I'm still meaning
to revive that series:

http://permalink.gmane.org/gmane.comp.version-control.git/172228

I've reworked a few more things compared to the RFC from back then but
was caught by lack of time and the rc phase. I'll send what I have asap.

Michael

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Jeff King]

On Mon, Jun 20, 2011 at 01:08:13PM +0200, Michael J Gruber wrote:

> > For that matter, --match should be spelled "--glob", as we already have:
> > 
> >   $ git log --glob='jk/*'
> > 
> > I think having the ref-selection for "git branch" match that of the
> > revision walker makes sense.
> 
> Well, "branch" is about refs, and "log" about revs.

Sure, and I wouldn't expect "git branch --list 1234abcd" to do anything
useful. But naming refs is a subset of naming revs. Certainly it seems
worth it to make the shorthands like "--remotes" behave the same way
where applicable.

I do agree that "git branch -l 'jk/*'" is less typing than "--glob"; it
may be worth supporting both forms to provide the least surprise to the
user (i.e., even though it may not be the shortest, users may expect the
same syntax to work in both places, and it costs us very little to
accept either).

> I'd rather have
> "branch" similar to "tag" in that respect (i.e. '-l'). I'm still meaning
> to revive that series:
> 
> http://permalink.gmane.org/gmane.comp.version-control.git/172228

Modulo Junio's comments on the "-l" transition, I like the idea. One
thing jumped out at me:

> -       else if (argc == 0)
> -               return print_ref_list(kinds, detached, verbose, abbrev, with_commit);
> +       else if (argc == 0 || (verbose && argc == 1))
> +               return print_ref_list(kinds, detached, verbose, abbrev, with_commit, argc ?  argv[0] : NULL);

Is there any reason not to accept:

  git branch --list jk/* mg/*

? For "tag -l", we seem to silently ignore any arguments past the first:

  $ git tag -l 'v1.7.4.*' 'v1.7.5.*'
  v1.7.4.1
  v1.7.4.2
  v1.7.4.3
  v1.7.4.4
  v1.7.4.5

We should at least warn and say "your second argument is being ignored"
or show the usage message.  But perhaps it is even friendlier to accept
a list of patterns.

-Peff

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ? For "tag -l", we seem to silently ignore any arguments past the first:
>
>   $ git tag -l 'v1.7.4.*' 'v1.7.5.*'
>   v1.7.4.1
>   v1.7.4.2
>   v1.7.4.3
>   v1.7.4.4
>   v1.7.4.5
>
> We should at least warn and say "your second argument is being ignored"
> or show the usage message.

I think it is just a bug; no need for "being ignored" warning.

> But perhaps it is even friendlier to accept
> a list of patterns.

Yes indeed; please make it so ;-)

[PATCH] tag: accept multiple patterns for --list

[Jeff King]

On Mon, Jun 20, 2011 at 08:42:32AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ? For "tag -l", we seem to silently ignore any arguments past the first:
> >
> >   $ git tag -l 'v1.7.4.*' 'v1.7.5.*'
> >   v1.7.4.1
> >   v1.7.4.2
> >   v1.7.4.3
> >   v1.7.4.4
> >   v1.7.4.5
> >
> > We should at least warn and say "your second argument is being ignored"
> > or show the usage message.
> 
> I think it is just a bug; no need for "being ignored" warning.
> 
> > But perhaps it is even friendlier to accept
> > a list of patterns.
> 
> Yes indeed; please make it so ;-)

Here's a patch.

-- >8 --
Subject: [PATCH] tag: accept multiple patterns for --list

Until now, "git tag -l foo* bar*" would silently ignore the
second argument, showing only refs starting with "foo". It's
not just unfriendly not to take a second pattern; we
actually generated subtly wrong results (from the user's
perspective) because some of the requested tags were
omitted.

This patch allows an arbitrary number of patterns on the
command line; if any of them matches, the ref is shown.

While we're tweaking the documentation, let's also make it
clear that the pattern is fnmatch.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-tag.txt |    9 ++++++---
 builtin/tag.c             |   26 +++++++++++++++++---------
 t/t7004-tag.sh            |    5 +++++
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index d82f621..fb1c0ac 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -69,8 +69,11 @@ OPTIONS
 	If the tag is not annotated, the commit message is displayed instead.
 
 -l <pattern>::
-	List tags with names that match the given pattern (or all if no pattern is given).
-	Typing "git tag" without arguments, also lists all tags.
+	List tags with names that match the given pattern (or all if no
+	pattern is given).  Running "git tag" without arguments also
+	lists all tags. The pattern is a shell wildcard (i.e., matched
+	using fnmatch(3)).  Multiple patterns may be given; if any of
+	them matches, the tag is shown.
 
 --contains <commit>::
 	Only list tags which contain the specified commit.
diff --git a/builtin/tag.c b/builtin/tag.c
index ec926fc..cef2726 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -16,7 +16,7 @@
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git tag -d <tagname>...",
-	"git tag -l [-n[<num>]] [<pattern>]",
+	"git tag -l [-n[<num>]] [<pattern>...]",
 	"git tag -v <tagname>...",
 	NULL
 };
@@ -24,17 +24,28 @@ static const char * const git_tag_usage[] = {
 static char signingkey[1000];
 
 struct tag_filter {
-	const char *pattern;
+	const char **patterns;
 	int lines;
 	struct commit_list *with_commit;
 };
 
+static int match_pattern(const char **patterns, const char *ref)
+{
+	/* no pattern means match everything */
+	if (!*patterns)
+		return 1;
+	for (; *patterns; patterns++)
+		if (!fnmatch(*patterns, ref, 0))
+			return 1;
+	return 0;
+}
+
 static int show_reference(const char *refname, const unsigned char *sha1,
 			  int flag, void *cb_data)
 {
 	struct tag_filter *filter = cb_data;
 
-	if (!fnmatch(filter->pattern, refname, 0)) {
+	if (match_pattern(filter->patterns, refname)) {
 		int i;
 		unsigned long size;
 		enum object_type type;
@@ -88,15 +99,12 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 	return 0;
 }
 
-static int list_tags(const char *pattern, int lines,
+static int list_tags(const char **patterns, int lines,
 			struct commit_list *with_commit)
 {
 	struct tag_filter filter;
 
-	if (pattern == NULL)
-		pattern = "*";
-
-	filter.pattern = pattern;
+	filter.patterns = patterns;
 	filter.lines = lines;
 	filter.with_commit = with_commit;
 
@@ -425,7 +433,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (list + delete + verify > 1)
 		usage_with_options(git_tag_usage, options);
 	if (list)
-		return list_tags(argv[0], lines == -1 ? 0 : lines,
+		return list_tags(argv, lines == -1 ? 0 : lines,
 				 with_commit);
 	if (lines != -1)
 		die(_("-n option is only allowed with -l."));
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2ac1c66..097ce2b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -257,6 +257,11 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+test_expect_success 'tag -l can accept multiple patterns' '
+	git tag -l "v1*" "v0*" >actual &&
+	test_cmp expect actual
+'
+
 # creating and verifying lightweight tags:
 
 test_expect_success \
-- 
1.7.6.rc1.38.g97f64.dirty

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> For that matter, --match should be spelled "--glob", as we already have:
>> 
>>   $ git log --glob='jk/*'
>> 
>> I think having the ref-selection for "git branch" match that of the
>> revision walker makes sense.
>> 
>> -Peff
>
> Well, "branch" is about refs, and "log" about revs.

Not really. What "log" output by traversing is about revisions, but we are
discussing how the command line argument discover the set of refs to feed
into the underlying machinery that does different things in each of the
command (branch just lists them without any traversal, log shows history
leading to the commits). So I do not see any difference and I think it
would be appropriate if we can use the same notation to specify both "I
want to see all branches that match this pattern" and "I want to see
history leading to all refs that match this pattern".

> I'd rather have
> "branch" similar to "tag" in that respect (i.e. '-l').

And "tag --glob='*'" also falls into the same class from that point of
view: "I want to see all tags that match this pattern".

Thanks.

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 20, 2011 at 08:40:32AM +0200, Johannes Sixt wrote:
>
>> > 	$ git branch --match "jk/*"
>> > 
>> > to show only local topic branches whose names match the given blob.
>> 
>> I would hate having to learn a new syntax '--match "jk/*"' when we can
>> already say
>> 
>>     $ git log --remotes
>>     $ git log --remotes=alice --remotes=bob
>>     $ git log --remotes="jk/*"
>> 
>> IMO, it is the right approach to have a long option --remotes with an
>> optional argument.

Yeah, it was just me being sloppy. I didn't mean to suggest a brand new
option name "--match"; I was merely saying that it should be something
that specifies filtering.  Except for the last one, if you are naming
remotes, that also would make sense.

But not the last one at least in the context of the example I gave in my
message. I wanted to see local topic branches whose names match jk/* so
using --remotes is actively wrong.

> For that matter, --match should be spelled "--glob", as we already have:
>
>   $ git log --glob='jk/*'
>
> I think having the ref-selection for "git branch" match that of the
> revision walker makes sense.

Yes, again I was just being sloppy and did not mean to introduce a brand
new --match; --glob indeed is a lot more appropriate in the examples I
gave in the message.

Here are two other examples of "listing" mode of the branch command:

	$ git branch --no-merged --glob="jc/*" pu
	$ git branch --with jc/some-topic --glob="maint-*" --glob=maint

I sometimes wish if they had such filtering.

Re: [PATCH/RFC] branch: add optional parameter to -r to specify remote

[Boris Faure]

On Mon, Jun 20, 2011 at 00:32, Junio C Hamano <gitster@pobox.com> wrote:
> Boris Faure <billiob@gmail.com> writes:
>
>> add '--remote' as long version for '-r'
>> update documentation
>> add tests
>
> (style) Sentences begin with a capital letter and ends with a period.
>
> This commit does a lot more than the above, no? It adds an optional remote
> name parameter to the existing "-r" option and limits the output to the
> remote tracking branches of the remote when it is specified.
>
>> ---
>
> Sign-off?

I missed it.

>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
>> index c50f189..242da9c 100644
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> ...
>> @@ -99,8 +99,10 @@ OPTIONS
>>       default to color output.
>>       Same as `--color=never`.
>>
>> --r::
>> -     List or delete (if used with -d) the remote-tracking branches.
>> +-r[=<remote>]::
>> +--remote[=<remote>]::
>> +     List or delete (if used with -d) the remote-tracking branches from
>> +     <remote> if specified.
>
> It is now unspecified what the command would do when the optional <remote>
> is left unspecified.

My english is not excellent and for sure a better wording can be found.


>>  #define REF_LOCAL_BRANCH    0x01
>>  #define REF_REMOTE_BRANCH   0x02
>> +static int kinds = REF_LOCAL_BRANCH;
>
> This used to be nicely scoped out of global space and got passed around as
> parameter, but now it has become a global? I do not see a good reason for
> this change.
>
>> +static const char *remote = NULL;

I put those variables in global space in order to access them within the
parse_opt callback.


> Two issues.
>
>  1. Presumably you wanted to have this change because you have too many
>    remotes, way more than two, and wanted to filter the output from
>    remotes that you are not interested in. Is it entirely implausible
>    that you might be interested in not just one, but two remotes out of
>    many remotes you have? A single string variable would not suffice for
>    that but you should be able to make this an array of strings.

I wanted to replace a git branch -r | grep 'someRemoteName' to filter
in fact one remote in particular.

>> +static int parse_opt_remote_cb(const struct option *opt, const char *arg,
>> +                            int unset)
>> +{
>> +     kinds = REF_REMOTE_BRANCH;
>> +     if (unset)
>> +             kinds = REF_LOCAL_BRANCH;
>
> What is this "unset" check about? Wouldn't that be an error if the command
> line said "--no-remote"?
>
> And you do not return but proceed to look at "arg", presumably to handle a
> case where the command line said "--no-remote=foobar"?

It's just a missing return.

>> +     if (arg) {
>> +             if ( *arg == '=')
>
> (style) Unwanted SP after an open parenthesis.
>
>> +                     remote = arg + 1; /* skip '=' */
>
> (style) It is clear enough what this does without the extra comment.
>
> Does this forbid remote names that begin with a "="?  I.e.
>
>        $ git branch -r =temporary

arg is '=temporary' if called with git branch -r=temporary but 'temporary'
if called with git branch --remote=temporary. I didn't know to check whether
the option triggered was from the long or the short version and skip '='
accordingly.

> As to the design of the new feature, I see you tried to make it possible
> to perform what
>
>        $ git branch -d -r origin/master
>
> does with
>
>        $ git branch -d --remote=origin master
>
> I do not think it is particularly a good idea. Adding yet another way to
> do the same thing, unless that new way is vastly superiour (e.g. easier to
> use, easier to explain, more efficient to perform, etc.), does not add
> much value to the system.
>
> It would make much more sense to restrict this feature to the "listing"
> side of the branches.  It would be nice if you can do:
>
>        $ git branch -r --match alice --match bob
>
> to show only remote tracking branches under refs/remotes/{alice,bob}
> and also
>
>        $ git branch --match "jk/*"
>
> to show only local topic branches whose names match the given blob.

I agree that it doesn't make much sense with -d option. I added the feature
with '-r=<remote>' so that it works with '-d'.
I would have preferred to just list branches from a given remote 'aa' with
'git branch -r aa' but I'll see where the discussion ends up.

-- 
Boris Faure