Re: [PATCH 1/2] config: properly range-check integer values

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 20, 2013, 23:07 UTC
Message-ID: <20130820230749.GM4110@google.com>
In-Reply-To: <20130820224256.GA24766@sigill.intra.peff.net>

Jeff King wrote:

Show 6 quoted lines

> I added a test. It would not fail on existing 32-bit systems, but would
> on existing LP64 systems. It will pass with the new code on both.
> However, it will fail on ILP64 systems (because their int is large, and
> can represent 3GB). I'm not sure if any such systems are in wide use
> (SPARC64?), but we would want a prereq in that case, I guess. I'm
> inclined to wait to see if it actually fails for anybody.

Yuck.

What will go wrong if "git config --int" starts returning numbers too large to fit in an 'int'? That can already happen if "git" and a command that uses it are built for different ABIs (e.g., ILP64 git, 32-bit custom tool that calls git).

It's possible that what the test should be checking for is "either returns a sane answer or fails" (which would pass regardless of ABI). Something like:

	test_expect_success 'large integers do not confuse config --int' '
		git config giga.crash 3g &&
		test_might_fail git config --int giga.crash >actual &&
		echo 3221225472 >expect &&
		{
			test_cmp expect actual ||
			test_must_fail git config --int giga.crash
		}
	'

Sensible? Jonathan