Re: [PATCH v6 2/6] Add git_env_ulong() to parse environment variable

Jeff King [off-list ref] writes:

On Tue, Aug 26, 2014 at 05:23:21PM +0200, Steffen Prohaska wrote:

quoted

+/*
+ * Use default if environment variable is unset or empty string.
+ */
+unsigned long git_env_ulong(const char *k, unsigned long val)
+{
+	const char *v = getenv(k);
+	if (v && *v && !git_parse_ulong(v, &val))
+		die("failed to parse %s", k);
+	return val;
+}

I think the "empty string" behavior here is sensible. I notice that
git_env_bool is not so careful. I think we should probably do this
(independent of your series):

-- >8 --
Subject: git_env_bool: treat empty string as "not set"

If an environment variable we treat as a boolean is not set,
we use some default value provided by the caller. If it is
set but is the empty string, however, we treat it as
"false". This can be rather confusing, as it is easy to set
the variable to the empty string in the shell (e.g., by
calling GIT_SMART_HTTP= instead of "unset").

I think different people have different confusion criteria.
To me, these two are very different operations:

    $ VAR=
    $ unset VAR

I think it boils down to that I see that the distance between "unset
vs set to empty" is far larger than the distance between "empty vs
false".  You probably see these two distances the other way,
i.e. "set to empty is almost like unset" and "empty is not a valid
way to say false".

Due to this difference, the new test confused me and had me read it
three times.

The test before this one explicitly sets GIT_SMART_HTTP to "0" and
expects the fetch to fail.  It is sensible to you because "0" is a
lot more explicit "false" than an empty string to you, and you
equate an empty and unset, hence the new one should succeed.

But it looks nonsensical to me that the new one expects to succeed,
because "0" and an empty string are both valid way to say "false"
to me, and it should behave the same way as the "0" one.

I view the *v check before git_parse_ulong() being unnecessarily
defensive to avoid triggering "die()".  An empty string is obviously
not a number (somebody could argue that it is the same as zero,
though), but nevertheless the user _is_ telling us to use that value
by setting and exporting the variable.  If we cannot parse it,
barfing is what the user would appreciate.

So, I am not sure the patch in the message I am responding to, and I
am not sure about that *v check in Steffen's patch, either.

On Tue, Aug 26, 2014 at 01:20:53PM -0700, Junio C Hamano wrote:

I think different people have different confusion criteria.
To me, these two are very different operations:

    $ VAR=
    $ unset VAR

I think it boils down to that I see that the distance between "unset
vs set to empty" is far larger than the distance between "empty vs
false".  You probably see these two distances the other way,
i.e. "set to empty is almost like unset" and "empty is not a valid
way to say false".

Due to this difference, the new test confused me and had me read it
three times.

I agree that it is rather a subjective decision.

So, I am not sure the patch in the message I am responding to, and I
am not sure about that *v check in Steffen's patch, either.

If it is truly "some people prefer it one way and some the other", I am
not sure if we should leave it as-is (that is preferring one way). The
middle ground would be to die(). That does not seem super-friendly, but
then we would also die with GIT_SMART_HTTP=foobar, so perhaps it is not
unreasonable to just consider it a syntax error.

I dunno. I can live with leaving it as-is. Certainly the existing
behavior is not what I expected, but it is not like it came up in the
real world (and I would not expect it to do so often). And it is
consistent with the config, which treats:

  [foo]
  bar =

as boolean false. That _also_ seems weird to me, but that is not
something I think we can easily change or outlaw at this point anyway.

-Peff