Re: [PATCH 2/2] teach git-config to output large integers
Jeff King wrote: On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote: That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. You are conflating the two patches, I think. The test we were discussing is for the _first_ patch, which fixes a bug in the range check. It is not meant to test git-config in particular, but to test that values higher than INT_MAX and lower than LONG_MAX are properly range-checked. Forget the second patch for a moment. I believe the first one is a bug fix that we would want even if we do not take the second patch at all. Sure. I'm not conflating the patches. What I mean is that tests are supposed to test desirable behavior, whatever that is --- they are not about preventing all behavior changes but only about preventing regressions. So talking about tests is a (perhaps overly roundabout) way to figure out the desirable behavior. In particular, at first glance I would think computing 3 * 2^20 instead of erroring out would be a *good* behavior, not a regression. If that's right, it doesn't make sense to me to go to careful lengths either to test that git continues to error out on most platforms, or to introduce new options to ensure git config --int continues to error out. That is what I am trying to understand. Everything about the first patch except for the test makes sense to me, but the test doesn't. As you noted, we know the test won't pass on some platforms. Why is it something we should *want* to pass? Jonathan -- 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 2/2] teach git-config to output large integers
Jeff King wrote:
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes.
Yes, something like --size would be more pleasant.
It could still use unsigned long internally. My only worry about
--size is that it does not make it clear we are talking about file
sizes and not in-memory sizes (size_t), and I'm not too worried about
that.
[...]
--- a/builtin/config.c
+++ b/builtin/config.c
[...]
@@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char
*value)
int v = git_config_int(key, value);
sprintf(normalized, %d, v);
}
+ else if (types == TYPE_ULONG)
+ sprintf(normalized, %lu,
+ git_config_ulong(key, value));
+
else if (types == TYPE_BOOL)
Style: uncuddled else, stray blank line. (The former was already
there, but it still stands out.) I think
if (types == TYPE_INT) {
...
} else if (types == TYPE_ULONG) {
...
} else if (types == TYPE_BOOL) {
...
} else if (types == TYPE_BOOL_OR_INT) {
...
} else {
...
}
would be easiest to read.
Thanks for taking this on.
Sincerely,
Jonathan
--
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 2/2] teach git-config to output large integers
On Tue, Aug 20, 2013 at 03:57:45PM -0700, Jonathan Nieder wrote:
Jeff King wrote:
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes.
Yes, something like --size would be more pleasant.
It could still use unsigned long internally. My only worry about
--size is that it does not make it clear we are talking about file
sizes and not in-memory sizes (size_t), and I'm not too worried about
that.
I almost sent it as --size with unsigned long internally. But try
writing the documentation for it. You want to say something like it's
big enough to handle file sizes. Except that on 32-bit, it's _not_.
It's only 4G.
You really want something that uses off_t internally, so 32-bit systems
with largefile support do the sane thing. But now you have no way of
emulating the way that git parses stuff internally. You cannot say git
config --size core.bigFileThreshold and get the same results that git
will have internally when it looks at that file (because it uses
unsigned long internally).
I think there is an argument to be made that git should be using off_t
internally for such things. But it is a lot of code to change and check,
and I'm not sure that anybody even really cares that much.
Style: uncuddled else, stray blank line. (The former was already
there, but it still stands out.) I think
Yes, I was trying to follow the existing style (but obviously the extra
line was just a typo).
if (types == TYPE_INT) {
...
} else if (types == TYPE_ULONG) {
...
} else if (types == TYPE_BOOL) {
...
} else if (types == TYPE_BOOL_OR_INT) {
...
} else {
...
}
would be easiest to read.
But that is adding brackets for one-liner conditional bodies that do not
need it. Which is more evil?
My usual method is to do what looks the most readable to me, but I admit
I have a hard time using my intuition with the cuddled-elses, as I think
they look terrible (yes, I'm aware they are in our style guide and I am
not arguing to take them out, only that my personal sense of looks
good is helpless with them).
-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 2/2] teach git-config to output large integers
Jeff King wrote: I almost sent it as --size with unsigned long internally. But try writing the documentation for it. You want to say something like it's big enough to handle file sizes. Except that on 32-bit, it's _not_. It's only 4G. You really want something that uses off_t internally, so 32-bit systems with largefile support do the sane thing. But now you have no way of emulating the way that git parses stuff internally. Let's take a step back for a moment. What problem is this patch solving? From the motivating example, I thought it was When reading or writing an integer config item, git sometimes encounters integer overflow and doesn't know how to deal with it. Worse, this means that some meaningful values are unrepresentable in config files. Fix it in two steps: 1. Catch overflow, and error out instead of pretending to be able to handle it. 2. Provide at least an option to use a wider integer type and handle larger meaningful values. This involves a new option --size instead of making --int use intmax_t for the following compatibility reason: ... For example, the compatibility reason could be that some scripts calling git config were not able to handle large integers and that we do not want to expose them to unexpectedly large values. But that reason doesn't sound realistic to me. So what is the actual reason not to always use a wider range? That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. Hoping that clarifies, Jonathan -- 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 2/2] teach git-config to output large integers
On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote: Jeff King wrote: I almost sent it as --size with unsigned long internally. But try writing the documentation for it. You want to say something like it's big enough to handle file sizes. Except that on 32-bit, it's _not_. It's only 4G. You really want something that uses off_t internally, so 32-bit systems with largefile support do the sane thing. But now you have no way of emulating the way that git parses stuff internally. Let's take a step back for a moment. What problem is this patch solving? That you cannot currently ask git-config for a value larger than 2g. From the motivating example, I thought it was When reading or writing an integer config item, git sometimes encounters integer overflow and doesn't know how to deal with it. Worse, this means that some meaningful values are unrepresentable in config files. Fix it in two steps: 1. Catch overflow, and error out instead of pretending to be able to handle it. No, this first step is not being added. It is already the case that we error out for overflow. The first patch is catching a case where we failed to do that properly, and instead truncated. And it has nothing to do with git-config itself; it is only that we must use git-config to test it from the shell. The same problem may happen internally (but tends not to, because large things tend to use git_config_ulong instead of git_config_int). 2. Provide at least an option to use a wider integer type and handle larger meaningful values. This involves a new option --size instead of making --int use intmax_t for the following compatibility reason: ... For example, the compatibility reason could be that some scripts calling git config were not able to handle large integers and that we do not want to expose them to unexpectedly large values. But that reason doesn't sound realistic to me. So what is the actual reason not to always use a wider range? My reason is that it does not represent the same range checks that git is doing internally (and which vary from platform to platform). So you might ask git-config what is the value of pack.deltacachelimit; I would expect it to apply the same range checks there that would be used by git-pack-objects. That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. You are conflating the two patches, I think. The test we were discussing is for the _first_ patch, which fixes a bug in the range check. It is not meant to test git-config in particular, but to test that values higher than INT_MAX and lower than LONG_MAX are properly range-checked. Forget the second patch for a moment. I believe the first one is a bug fix that we would want even if we do not take the second patch at all. -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
