Re: [RFC/PATCH] packfile: use extra variable to clarify code in use_pack()

From: Jeff King <peff@peff.net>
Date: Mar 12, 2019, 20:26 UTC
Message-ID: <20190312202605.GA24350@sigill.intra.peff.net>
In-Reply-To: <b4106d4b-5f9e-4577-c8ed-641df33d4fb5@ramsayjones.plus.com>

On Tue, Mar 12, 2019 at 05:39:13PM +0000, Ramsay Jones wrote:

> On 12/03/2019 16:55, Ramsay Jones wrote:
> > From: Jeff King <peff@peff.net>
> > 
> > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

Could definitely use a commit message. I think it's something like:

  We use the "offset" variable for two purposes. It's the offset into
  the packfile that the caller provides us (which is rightly an off_t,
  since we might have a packfile much larger than memory). But later we
  also use it as the offset within a given mmap'd window, and that
  window cannot be larger than a size_t.

  For the second use, the fact that we have an off_t leads to some
  confusion when we assign it to the "left" variable, is a size_t. It is
  in fact correct (because our earlier "offset -= win->offset" means we
  must be within the pack window), but using a separate variable of the
  right type makes that much more obvious.

You'll note that I snuck in the assumption that "left" is a size_t, which as you noted is not quite valid yet. :)

Show 7 quoted lines

> Heh, of course I should have tried applying on top of today's
> codebase before sending it out! :(
> 
> Having just done so, it quickly showed that this patch assumes
> that the 'left' parameter to use_pack() has been changed from
> an 'unsigned long *' to an 'size_t *' as part of the series
> that was being discussed in the above link.

Yep. Until then, I do not think there is much point (and in fact I'd suspect this code behaves incorrectly on Windows, where "unsigned long" is too short; hopefully they clamp pack windows to 4GB by default there, which would work around it).

But I would be very happy if you wanted to resurrect the "left" patch and then do this on top. :)

-Peff