On Mon, 27 Sep 2010, Giuseppe Bilotta wrote:
> 2010/9/26 Jakub Narebski <jnareb@xxxxxxxxx>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>>
>>> +# returns a submenu for the nagivation of the refs views (tags, heads,
>>> +# remotes) with the current view disabled and the remotes view only
>>> +# available if the feature is enabled
>>> +
>>
>> Minor nitpick: this empty line here is not necessary. But I think
>> that Junio can remove it when applying.
>
> Since there have been a couple of stylistic suggestions for the
> comments in the first two patches too, I can probably resend the whole
> series including these changes, unless Junio wants to do the
> hand-tuning.
Well, the first half of this series (patches 1 to 5) are good enough,
or almost good enough, to be merged in as they are now. They might
need at most some minor stylistic corrections. It depends on Junio
whether he wants to have resend of early part of this series, or if
he prefers to hand-edit those minor corrections.
Patches 6-12 needs, I think, further discussion.
>>> +sub format_ref_views {
>>> + my ($current) = @_;
>>> + my @ref_views = qw{tags heads};
>>
>> Hmmm... should we pass it as argument, or use $action in place of
>> $current? Each solution has its advantages and disadvantages. Current
>> solution has the advantage of avoiding using global variables, solution
>> using $action has the (supposed) advantage of automatically detecting
>> current action.
>
> Not using $action has the advantage of making it possible to enable
> the $action command if it's wanted, which is something that I use in a
> subsequent patch (when enabling single-remote view). But this is of
> course debatable.
I agree that use of format_ref_views further in this series shows that
your version (with $current passed as argument) is better.
--
Jakub Narebski
Poland
--
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