[+cc Jonathan] On Tue, Aug 27, 2013 at 02:05:01PM -0700, Junio C Hamano wrote:
> >> * jk/config-int-range-check (2013-08-21) 2 commits > [...] > > > > 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). > >> * 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 -- 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