Jeff King [off-list ref] writes:
On Thu, Nov 22, 2007 at 04:56:06AM -0600, Dan Zwell wrote:
quoted
+ # Grab the 3 main colors in git color string format, with sane
+ # (visible) defaults:
+ $prompt_color = Git::color_to_ansi_code(
+ scalar $repo->config_default('color.interactive.prompt',
+ 'bold blue'));
And by the same token as the last message, given that config_* take only
two arguments, is there a reason not to extend them so that
$repo->config_bool('my.key', 0);
handles the default. Then I think you could simplify this to just:
$repo->config_color('color.interactive.prompt', 'bold blue');
and hide the color_to_ansi_code messiness from the script altogether.
I like the config_color() method.
I think the "config_bool with default" also makes sense but it
needs to be coded a bit carefully. Issues to consider:
(1) Non default form "$r->config_bool('key')" should keep the
original semantics; missing key in the configuration is the
same as false (i.e. "undef" in scalar, () in list context).
(2) What should be the second parameter in the form to default
to true? '1'? 'true'? Any kind of "true" value in Perl
should be accepted?
(3) Same question as (2) but for defaulting to false. Any kind
of "false"?
On Thu, Nov 22, 2007 at 01:28:34PM -0800, Junio C Hamano wrote:
I think the "config_bool with default" also makes sense but it
needs to be coded a bit carefully. Issues to consider:
Yes. It is not strictly necessary for this patch series, but I think it
is nice to stake out a claim on the third argument of config_* functions
for consistency sake. But perhaps in the name of avoiding regression, it
should come later, when somebody actually wants to use it.
(1) Non default form "$r->config_bool('key')" should keep the
original semantics; missing key in the configuration is the
same as false (i.e. "undef" in scalar, () in list context).
Yes, this is obviously the most important thing.
(2) What should be the second parameter in the form to default
to true? '1'? 'true'? Any kind of "true" value in Perl
should be accepted?
(3) Same question as (2) but for defaulting to false. Any kind
of "false"?
Hmm. I am tempted to say "yes, any true or any false value" in that the
point of config_* is to convert git config values to native perl
representations. OTOH, the moral equivalent of
config_color('my.key', 'bold red');
is probably more appropriately
config_bool('my.key', 'true');
so I am fine doing it that way, as well (though I think it makes us
duplicate the "translate these strings into bools" code into perl).
-Peff
Jeff King wrote:
quoted
(2) What should be the second parameter in the form to default
to true? '1'? 'true'? Any kind of "true" value in Perl
should be accepted?
(3) Same question as (2) but for defaulting to false. Any kind
of "false"?
Hmm. I am tempted to say "yes, any true or any false value" in that the
point of config_* is to convert git config values to native perl
representations. OTOH, the moral equivalent of
config_color('my.key', 'bold red');
is probably more appropriately
config_bool('my.key', 'true');
so I am fine doing it that way, as well (though I think it makes us
duplicate the "translate these strings into bools" code into perl).
As you said, config_* converts git values to perl values. However, that
conversion needs only be done for strings in .gitconfig. Is there any
reason why the caller of the function would need to pass a string
"false"? I just don't see the need for conversion of any kind.
Further, I think that we could return the default variable directly,
without parsing it at all. It would be much simpler, and there would
need to be no special cases for dealing with undef or 'false'. It's a
perl function, being called with perl arguments, so a user should not be
that surprised when 'false' does what perl says it should do.
Dan
On Thu, Nov 22, 2007 at 11:32:16PM -0600, Dan Zwell wrote:
Further, I think that we could return the default variable directly, without
parsing it at all. It would be much simpler, and there would need to be no
special cases for dealing with undef or 'false'. It's a perl function, being
called with perl arguments, so a user should not be that surprised when
'false' does what perl says it should do.
I think that is more elegant for config_bool, but it means that
config_bool and config_color have slightly different behaviors (the
difference being that it is easy to feed a native perl value as the
default to config_bool, but to get the same behavior for config_color,
you would call Git::color_to_ansi_code manually, which is a pain).
In this instance, I am inclined to sacrifice consistency for convenience
and make it:
my $bool = config_bool('my.key', 0);
my $color = config_color('my.key', 'bold red');
Note that there is one tricky part of config_bool, which is what
config_bool('my.key', undef) should do (is it "default false" or "no
default"?).
-Peff