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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 19, 2011, 22:32 UTC
Message-ID: <7vtyble9k8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1308511149-10933-2-git-send-email-billiob@gmail.com>

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?

Show 15 quoted lines

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

Show 20 quoted lines

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

Show 12 quoted lines

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

Show 10 quoted lines

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