On Mon, Jun 20, 2011 at 00:32, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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?
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
--
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