On Sun, 10 Feb 2008, Junio C Hamano wrote:
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
>
> > It may be a bit odd, but it automatically means that codepaths that simply
> > don't want to care about the subtle difference between "true" and "empty"
> > will just automatically work, because to them the two cases will look
> > identical, because the strings will compare the same - they have the same
> > data, just different addresses.
>
> I should have mentioned the reason why I did not suggest doing
> it this way in my [Janitor] message.
>
> It is not "suttle difference between true and empty". Empty
> means false, and with this approach, it switches the meaning of
> valueless form of config to quite the opposite.
>
> In addition to fixing existing breakages, a piece of code that
> knew NULL is true and empty is false and coded accordingly, i.e.
>
> if (!value)
> Ah we have true;
> else if (!*value)
> Ok this is false;
> else if (!strcmp(value, "something special")
> Ok, this is not bool but special;
> else
> return git_config_bool(var, value);
>
> will now need to be changed to:
>
> if (value == config_true)
> Ah we have true;
> else if (!*value)
> Ok this is false;
> else if (!strcmp(value, "something special")
> Ok, this is not bool but special;
> else
> return git_config_bool(var, value);
Shouldn't it be simply:
if (!strcmp(value, "something special"))
Ok, this is not bool but special;
else
return git_config_bool(var, value);
That is, don't check for special true or special false at all, but have
git_config_bool() sort them out? And Linus's code means that you can do
the strcmp without worrying about getting a segfault on special true.
> if you do this. And the code that was already broken:
>
> if (!strcmp(value, "somevalue")
> Ok let's use somevalue;
> else if (!strcmp(value, "someothervalue")
> Ok let's use someothervalue;
> else
> die("oops we do not understand '%s'", value);
>
> still need to be fixed to:
>
> if (value == config_true)
> die("oops '%s' is not a bool", var);
> else if (!strcmp(value, "somevalue")
> Ok let's use somevalue;
> else if (!strcmp(value, "someothervalue")
> Ok let's use someothervalue;
> else
> die("oops we do not understand '%s'", value);
If it isn't changed, you'd get the message "oops we do not understand ''"
for either true or false empty values, which seems, if anything, better.
-Daniel
*This .sig left intentionally blank*
-
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
