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

[Steffen Prohaska]

On Aug 27, 2014, at 4:47 PM, Junio C Hamano gits...@pobox.com wrote:

 Jeff King p...@peff.net writes:
 
 On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:
 
 A worse position is to have git_env_bool() that says empty is
 false and add a new git_env_ulong() that says empty is unset.
 
 We should pick one or the other and use it for both.
 
 Yeah, I agree they should probably behave the same.
 
 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.
 
 Hmm, I am not sure if dying is better.  Unless we decide to make
 empty string no longer false everywhere and warn now and then later
 die as part of a 3.0 transition plan or something, that is.
 
 I think it is better in the sense that while it may be unexpected, it
 does not unexpectedly do something that the user cannot easily undo.
 
 I really do not think this topic is worth the effort of a long-term
 deprecation scheme (which I agree _is_ required for a change to the
 config behavior). Let's just leave it as-is. We've seen zero real-world
 complaints, only my own surprise after reading the code (and Steffen's
 patch should be tweaked to match).
 
 OK, then let's do that at least for now and move on.

Ok.  I saw that you tweaked my patch on pu.  Maybe remove the outdated
comment above the function completely:

diff --git a/config.c b/config.c
index 87db755..010bcd0 100644
--- a/config.c
+++ b/config.c
@@ -1122,9 +1122,6 @@ int git_env_bool(const char *k, int def)
return v ? git_config_bool(k, v) : def;
 }

-/*
- * 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);

Steffen--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Steffen Prohaska proha...@zib.de writes:

 OK, then let's do that at least for now and move on.

 Ok.  I saw that you tweaked my patch on pu.  Maybe remove the outdated
 comment above the function completely:

 diff --git a/config.c b/config.c
 index 87db755..010bcd0 100644
 --- a/config.c
 +++ b/config.c
 @@ -1122,9 +1122,6 @@ int git_env_bool(const char *k, int def)
 return v ? git_config_bool(k, v) : def;
  }

 -/*
 - * Use default if environment variable is unset or empty string.
 - */

Thanks, will do.

  unsigned long git_env_ulong(const char *k, unsigned long val)
  {
 const char *v = getenv(k);

   Steffen--
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:

 A worse position is to have git_env_bool() that says empty is
 false and add a new git_env_ulong() that says empty is unset.
 
 We should pick one or the other and use it for both.

 Yeah, I agree they should probably behave the same.

  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.
 
 Hmm, I am not sure if dying is better.  Unless we decide to make
 empty string no longer false everywhere and warn now and then later
 die as part of a 3.0 transition plan or something, that is.

 I think it is better in the sense that while it may be unexpected, it
 does not unexpectedly do something that the user cannot easily undo.

 I really do not think this topic is worth the effort of a long-term
 deprecation scheme (which I agree _is_ required for a change to the
 config behavior). Let's just leave it as-is. We've seen zero real-world
 complaints, only my own surprise after reading the code (and Steffen's
 patch should be tweaked to match).

OK, then let's do that at least for now and move on.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Steffen Prohaska]

The new function will be used to parse GIT_MMAP_LIMIT and
GIT_ALLOC_LIMIT.

Signed-off-by: Steffen Prohaska proha...@zib.de
---
 cache.h  |  1 +
 config.c | 11 +++
 2 files changed, 12 insertions(+)

diff --git a/cache.h b/cache.h
index fcb511d..b820b6a 100644
--- a/cache.h
+++ b/cache.h
@@ -1318,6 +1318,7 @@ extern int git_config_rename_section_in_file(const char 
*, const char *, const c
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, 
void *cb);
 extern int git_env_bool(const char *, int);
+extern unsigned long git_env_ulong(const char *, unsigned long);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__)
diff --git a/config.c b/config.c
index 058505c..178721f 100644
--- a/config.c
+++ b/config.c
@@ -1122,6 +1122,17 @@ int git_env_bool(const char *k, int def)
return v ? git_config_bool(k, v) : def;
 }
 
+/*
+ * 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;
+}
+
 int git_config_system(void)
 {
return !git_env_bool(GIT_CONFIG_NOSYSTEM, 0);
-- 
2.1.0.8.gf3a29c8

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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

 +/*
 + * 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).

Instead, let's treat unset and empty variables the same.
This should not have any negative backwards-compatibility
consequences, because:

  1. The existing behavior was confusing and undocumented in
 the first place.

  2. For most variables, the default _is_ false, and so this
 change is a noop. The only affected variables are
 GIT_IMPLICIT_WORK_TREE (which is undocumented and
 internally always set to 0 or 1) and GIT_SMART_HTTP
 (which is also undocumented, and we use only for
 testing).

Since there won't be any fallout with the current variables,
this is a good time to make the switch (before any other
variables are added).

Signed-off-by: Jeff King p...@peff.net
---
 config.c| 2 +-
 t/t5551-http-fetch-smart.sh | 7 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 058505c..7bf0704 100644
--- a/config.c
+++ b/config.c
@@ -1119,7 +1119,7 @@ const char *git_etc_gitconfig(void)
 int git_env_bool(const char *k, int def)
 {
const char *v = getenv(k);
-   return v ? git_config_bool(k, v) : def;
+   return v  *v ? git_config_bool(k, v) : def;
 }
 
 int git_config_system(void)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6cbc12d..831f9e4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -168,6 +168,13 @@ test_expect_success 'GIT_SMART_HTTP can disable smart 
http' '
 test_must_fail git fetch)
 '
 
+test_expect_success 'empty GIT_SMART_HTTP leaves smart http enabled' '
+   (GIT_SMART_HTTP= 
+export GIT_SMART_HTTP 
+cd clone 
+git fetch)
+'
+
 test_expect_success 'invalid Content-Type rejected' '
test_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2actual
grep not valid: actual
-- 
2.1.0.346.ga0367b9

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King p...@peff.net writes:

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

 +/*
 + * 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.

 +test_expect_success 'empty GIT_SMART_HTTP leaves smart http enabled' '
 + (GIT_SMART_HTTP= 
 +  export GIT_SMART_HTTP 
 +  cd clone 
 +  git fetch)
 +'

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.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King p...@peff.net writes:

 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). 

A worse position is to have git_env_bool() that says empty is
false and add a new git_env_ulong() that says empty is unset.

We should pick one or the other and use it for both.

As you pointed out in the later part of the message, an empty string
is a valid way to spell false in the config subsystem since the
beginning at 17712991 (Add .git/config file parser, 2005-10-10);
consistency with it probably is sensible.

But perhaps my brain is rotten with too much Perl and Python X-.
I do not know where Linus picked up that, though ;-)

 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.

Hmm, I am not sure if dying is better.  Unless we decide to make
empty string no longer false everywhere and warn now and then later
die as part of a 3.0 transition plan or something, that is.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Tue, Aug 26, 2014 at 02:54:11PM -0700, Junio C Hamano wrote:

 A worse position is to have git_env_bool() that says empty is
 false and add a new git_env_ulong() that says empty is unset.
 
 We should pick one or the other and use it for both.

Yeah, I agree they should probably behave the same.

  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.
 
 Hmm, I am not sure if dying is better.  Unless we decide to make
 empty string no longer false everywhere and warn now and then later
 die as part of a 3.0 transition plan or something, that is.

I think it is better in the sense that while it may be unexpected, it
does not unexpectedly do something that the user cannot easily undo.

I really do not think this topic is worth the effort of a long-term
deprecation scheme (which I agree _is_ required for a change to the
config behavior). Let's just leave it as-is. We've seen zero real-world
complaints, only my own surprise after reading the code (and Steffen's
patch should be tweaked to match).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html