Nicely explained, some comments inline
On Tue, Nov 02, 2021 at 03:46:08PM +0000, Matt Cooper via GitGitGadget wrote:
> From: Matt Cooper <vtbassmatt@xxxxxxxxx>
>
> 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 ?
> ... 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 ?
May be
This doesn't cause a problem when files smaller than 2^32 bytes are handled by Git.
>
> 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 am not a native English speaker, would "changing" be better than "altering" ?)
By changing the signature of read_blob_entry to use size_t,
> write_entry can be switched to use size_t internally (which all of its
> callers and most of its callees already used). To avoid touching dozens of
> additional files, read_blob_entry uses a local unsigned long to call a
> chain of functions which aren't prepared to accept size_t.
>
> Helped-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> Signed-off-by: Matt Cooper <vtbassmatt@xxxxxxxxx>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> entry.c | 8 +++++---
> entry.h | 2 +-
> parallel-checkout.c | 2 +-
> t/t1051-large-conversion.sh | 2 +-
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 711ee0693c7..4cb3942dbdc 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)
> return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
> }
>
> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> +void *read_blob_entry(const struct cache_entry *ce, size_t *size)
> {
> enum object_type type;
> - void *blob_data = read_object_file(&ce->oid, &type, size);
> + unsigned long ul;
> + void *blob_data = read_object_file(&ce->oid, &type, &ul);
>
> + *size = ul;
> if (blob_data) {
> if (type == OBJ_BLOB)
> return blob_data;
> @@ -270,7 +272,7 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca
> int fd, ret, fstat_done = 0;
> char *new_blob;
> struct strbuf buf = STRBUF_INIT;
> - unsigned long size;
> + size_t size;
> ssize_t wrote;
> size_t newsize = 0;
> struct stat st;
> diff --git a/entry.h b/entry.h
> index b8c0e170dc7..61ee8c17604 100644
> --- a/entry.h
> +++ b/entry.h
> @@ -51,7 +51,7 @@ int finish_delayed_checkout(struct checkout *state, int *nr_checkouts);
> */
> void unlink_entry(const struct cache_entry *ce);
>
> -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size);
> +void *read_blob_entry(const struct cache_entry *ce, size_t *size);
> int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st);
> void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
> struct stat *st);
> diff --git a/parallel-checkout.c b/parallel-checkout.c
> index 6b1af32bb3d..b6f4a25642e 100644
> --- a/parallel-checkout.c
> +++ b/parallel-checkout.c
> @@ -261,7 +261,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,
> struct stream_filter *filter;
> struct strbuf buf = STRBUF_INIT;
> char *blob;
> - unsigned long size;
> + size_t size;
> ssize_t wrote;
>
> /* Sanity check */
> diff --git a/t/t1051-large-conversion.sh b/t/t1051-large-conversion.sh
> index e7f9f0bdc56..e6d52f98b15 100755
> --- a/t/t1051-large-conversion.sh
> +++ b/t/t1051-large-conversion.sh
> @@ -85,7 +85,7 @@ test_expect_success 'ident converts on output' '
>
> # This smudge filter prepends 5GB of zeros to the file it checks out. This
> # ensures that smudging doesn't mangle large files on 64-bit Windows.
> -test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
> +test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
> 'files over 4GB convert on output' '
> test_commit test small "a small file" &&
> small_size=$(test_file_size small) &&
> --
> gitgitgadget
>
