Re: [PATCH v4 5/8] odb: teach read_blob_entry to use size_t

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 4, 2021, 00:09 UTC
Message-ID: <nycvar.QRO.7.76.6.2111040102560.56@tvgsbejvaqbjf.bet>
In-Reply-To: <20211102204040.nnndsqrzu7ozpva7@tb-raspi4>

Hi Torsten,

On Tue, 2 Nov 2021, Torsten Bögershausen wrote:

Show 10 quoted lines

> On Tue, Nov 02, 2021 at 03:46:08PM +0000, Matt Cooper via GitGitGadget wrote:
> > From: Matt Cooper <vtbassmatt@gmail.com>
> >
> > There is mixed use of size_t and unsigned long to deal with sizes in the
> > codebase. Recall that Windows defines unsigned long as 32 bits even on
> > 64-bit platforms, meaning that converting size_t to unsigned long narrows
> > the range. This mostly doesn't cause a problem since Git rarely deals
> > with files larger than 2^32 bytes.
>
> What does this mean ?

I found the explanation to be quite clear (otherwise I would have changed it before submitting, obviously).

Git is rarely used with large files, as it was meant to track source code files, and it is pretty rare that a source code file is larger than 4GB. Therefore, the described issues are edge cases rather than common ones.

> > ... This mostly doesn't cause a problem since Git rarely deals
> > with files larger than 2^32 bytes.
>
> Is "mostly" is a good wording here ?

I do think so.

> May be

s/May be/Maybe/ since you're nitpicking wording ;-)

> This doesn't cause a problem when files smaller than 2^32 bytes are handled by Git.

That would lose the rather important fact that it is common to encounter only tracked files that are much smaller than 4GB.

Show 11 quoted lines

> > But adjunct systems such as Git LFS, which use smudge/clean filters to
> > keep huge files out of the repository, may have huge file contents passed
> > through some of the functions in entry.c and convert.c. On Windows, this
> > results in a truncated file being written to the workdir. I traced this to
> > one specific use of unsigned long in write_entry (and a similar instance
> > in write_pc_item_to_fd for parallel checkout). That appeared to be for
> > the call to read_blob_entry, which expects a pointer to unsigned long.
> >
> > By altering the signature of read_blob_entry to expect a size_t,
>
> "expect" -> "use"

I would say that in the context of talking about a signature, "expect" is a better verb than "use".

But then, just like you I am not a native speaker, so I think we should maybe stop telling a native speaker like Matt how to use his native tongue...

> (I am not a native English speaker, would "changing" be better than "altering" ?)
>  By changing the signature of read_blob_entry to use size_t,

As I said, I am not a native English speaker, either. So I believe that Matt knows better than the two of us together how to phrase things in English.

Since you had nothing to say about the patch itself, may I assume that you're fine with it?

Ciao, Dscho