Jeff King [off-list ref] writes:
On Tue, Aug 27, 2013 at 12:22:30PM -0700, Junio C Hamano wrote:
quoted
* jk/config-int-range-check (2013-08-21) 2 commits
(merged to 'next' on 2013-08-22 at 465efb3)
+ teach git-config to output large integers
+ config: properly range-check integer values
Originally merged to 'next' on 2013-08-22
"git config --int section.var 3g" should somehow diagnose that the
number does not fit in "int" (on 32-bit platforms anyway) but it
did not.
Will cook in 'next'.
I think Jonathan had some concerns about the test in the first one, and
there was an open question in the second of whether we wanted to add
something like --ulong, call it something more agnostic like
--file-size, or simply teach --int to use 64-bit integers everywhere for
simplicity.
Thoughts?
Are the scripts that use "git config --<type>" expected to know the
representation type used by C binaries on the platform? If so,
letting them say "git config --ulong 3g" when setting a new value,
and "git config --ulong" when asking the current value with range
checking does make sense. When the underlying code uses "int" (as
opposed to "int32_t") to read the value for a variable on any
platform, then "git config --int 3g" that does not warn only because
it is running on 64-bit platform may not help very much. The users
can protect themselves by learning to use "config --int32 3g", but I
am not sure that is a sensible approach---rather, "config --int"
that makes sure that the current value or the value being set is
within range on any sensible platform may be a lot more user-friendly.
quoted
* jk/mailmap-incomplete-line (2013-08-25) 2 commits
- mailmap: avoid allocation when reading from blob
- mailmap: handle mailmap blobs without trailing newlines
Will merge to 'next'.
Did you want me to squash these? The second one more or less eradicates
the changes made to the first one. I mainly did them separately in case
we were going to only do the first half on maint.
Hmm, perhaps. Is reading mailmap from a blob commonly done and
deserves a maint update down for 1.8.3/1.8.2 series?
I'll be rewinding the 'next' soonish (either tomorrow or Thursday),
so I'll try to remember not to merge this (yet).
quoted
* jk/write-broken-index-with-nul-sha1 (2013-08-26) 1 commit
- write_index: optionally allow broken null sha1s
Am I waiting for another reroll?
Yep, just sent v3.
Thanks.
[+cc Jonathan]
On Tue, Aug 27, 2013 at 02:05:01PM -0700, Junio C Hamano wrote:
quoted
quoted
* jk/config-int-range-check (2013-08-21) 2 commits
[...]
quoted
I think Jonathan had some concerns about the test in the first one, and
there was an open question in the second of whether we wanted to add
something like --ulong, call it something more agnostic like
--file-size, or simply teach --int to use 64-bit integers everywhere for
simplicity.
Are the scripts that use "git config --<type>" expected to know the
representation type used by C binaries on the platform?
I think that's an open question. My argument was that you would want to
be able to get the same range errors that git would see internally. So I
know that if "git config --ulong pack.packSizeLimit 10g" does not fail,
then pack-objects itself will be fine when reading the value. So if you
buy the argument that such a thing is useful, then:
1. We would want to keep the range checks we have for --int.
2. We would need new types to represent items beyond --int, and they
should match what git will do internally. You can call it --ulong,
or --uint64, or --file-size, or whatever you like, but --int does
not cut it.
The counterarguments I can see are:
1. Who cares? If you want to know whether pack-objects will choke on
your huge config value, then run pack-objects.
2. Such a check would involve knowing which type we use internally to
look at packSizeLimit, and that is utterly undocumented (and
subject to change; e.g., it seems kind of senseless that we have a
4G pack-size limit on 32-bit platforms, and we may want to fix
that).
So if you do not buy the argument that communicating git's internal
range checks is useful, then we can simply say "--int is magically long
on every platform, and you can use it for everything numeric". And
implement it with int64_t. You may be able to read or write some values
for certain keys that git will barf on internally, but that is git's
problem.
The one thing it doesn't get you is that you can currently set unsigned
values to "-1" in the config to have them treated as ULONG_MAX. This is
undocumented and as far as I know not used by anyone. But it would be
one place where the interpretation of "git config key" is not the same
as what git does internally, and you do not get a warning or death, but
rather you just get a completely different value.
I don't feel too strongly either way. I mostly kept the range checks for
--int because that is how the code already worked, and I assumed that
was what was desired. But given what I know of the history of the config
code, it is probably a completely random side effect of how it is
implemented. :)
I can try to prepare a series going in that direction (we still need to
fix the internal truncation that currently happens, though).
quoted
quoted
* jk/mailmap-incomplete-line (2013-08-25) 2 commits
- mailmap: avoid allocation when reading from blob
- mailmap: handle mailmap blobs without trailing newlines
Will merge to 'next'.
Did you want me to squash these? The second one more or less eradicates
the changes made to the first one. I mainly did them separately in case
we were going to only do the first half on maint.
Hmm, perhaps. Is reading mailmap from a blob commonly done and
deserves a maint update down for 1.8.3/1.8.2 series?
Yes. The tip of jk/mailmap-from-blob turned on blob reading by default
in bare repositories. So if you have a .mailmap without a terminating
newline, "git shortlog" will segfault by default in a bare version of
your repository.
I do not know if it is so serious a fix that you need to go back to
v1.8.2 series, but I think it is definitely maint-worthy. I was worried
initially that the second part of the patch would involve too much
refactoring for maint, but it actually turned out pretty simple.
I'll prepare a squashed version that I think should be suitable for
maint.
-Peff
Am 8/27/2013 23:48, schrieb Jeff King:
The counterarguments I can see are:
1. Who cares? If you want to know whether pack-objects will choke on
your huge config value, then run pack-objects.
2. Such a check would involve knowing which type we use internally to
look at packSizeLimit, and that is utterly undocumented (and
subject to change; e.g., it seems kind of senseless that we have a
4G pack-size limit on 32-bit platforms, and we may want to fix
that).
So if you do not buy the argument that communicating git's internal
range checks is useful, then we can simply say "--int is magically long
on every platform, and you can use it for everything numeric". And
implement it with int64_t. You may be able to read or write some values
for certain keys that git will barf on internally, but that is git's
problem.
I'm in the camp of these (counter) arguments.
When my shell script asks for 'git config --int 3g', I expect to be
returned a positive 10-digit. What would I care which type Git or any
other tool is using internally? I only care whether my shell can work with
numbers that large. Or the next tool that I feed the number to. But that's
my business, not Git's.
The one thing it doesn't get you is that you can currently set unsigned
values to "-1" in the config to have them treated as ULONG_MAX. This is
undocumented and as far as I know not used by anyone.
And it better stays that way. Magic numbers should be encoded with magic
strings in the config file.
-- Hannes