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

On Mon, Jun 20, 2011 at 00:32, Junio C Hamano [off-list ref] wrote:

Boris Faure [off-list ref] writes:

quoted

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.

quoted

---

Sign-off?

I missed it.

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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

(style) Unwanted SP after an open parenthesis.

quoted

+                     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