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

[Boris Faure <billiob@gmail.com>]

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