Boris Faure <billiob@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html