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

From: Jeff King <peff@peff.net>
Date: Aug 26, 2014, 20:31 UTC
Message-ID: <20140826203158.GA30651@peff.net>
In-Reply-To: <xmqq38cjhuje.fsf@gitster.dls.corp.google.com>

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

Show 14 quoted lines

> 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