Re: [PATCH 1/2] archive: replace write_or_die() calls with write_block_or_die()

From: Junio C Hamano <gitster@pobox.com>
Date: May 5, 2019, 05:25 UTC
Message-ID: <xmqqsgttsc9k.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190501180936.GB4109@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> FWIW, I agree with you here. These patches are not making anything worse
> (and may even make them better, since we'd probably need to swap out the
> BLOCKSIZE constant for a run-time "blocksize" variable in fewer places).

It's just that leaving the interface uneven is an easy way to introduce an unnecessary bug, e.g.

	-type function(args) {
	+type function(args, size_t blocksize) {
		decls;
	-	helper_one(BLOCKSIZE, other, args);
	+	helper_one(blocksize, other, args);
		helper_two(its, args);
	-	helper_three(BLOCKSIZE, even, more, args);
	+	helper_three(blocksize, even, more, args);
	 }

when this caller is away from the implementation of helper_two() that hardcodes the assumption that this callchain only uses BLOCKSIZE and in an implicit way.

And that can easily be avoided by defensively making helper_two() to take BLOCKSIZE as an argument as everybody else in the caller does.

I do not actually care too deeply, though. Hopefully whoever adds "-b" would be careful enough to follow all callchain, and at least look at all the callees that are file-scope static, and the one I have trouble with _is_ a file-scope static.

Or maybe nobody does "-b", in which case this ticking time bomb will not trigger, so we'd be OK.