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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Rohit Ashiwal via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Subject: Re: [PATCH 1/2] archive: replace write_or_die() calls with write_block_or_die()
Date: Mon, 6 May 2019 01:07:26 -0400	[thread overview]
Message-ID: <20190506050726.GA30003@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqsgttsc9k.fsf@gitster-ct.c.googlers.com>

On Sun, May 05, 2019 at 02:25:59PM +0900, Junio C Hamano wrote:

> 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.

Right, my assumption was that the first step in the conversion would be
somebody doing s/BLOCKSIZE/global_blocksize_variable/. But that is just
a guess.

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

Yes. I suspect we're probably going down an unproductive tangent. :)

-Peff