{"thread":{"id":"49610","subject":"[PATCH v1 1/1] index_bulk_checkin(): Take off_t, not size_t","startedAt":"2018-10-18T19:11:45Z","lastAt":"2018-10-23T16:11:11Z","messageCount":3,"participants":["tboegi@web.de","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"360868","messageId":"20181018191140.23318-1-tboegi@web.de","threadId":"49610","inReplyTo":null,"subject":"[PATCH v1 1/1] index_bulk_checkin(): Take off_t, not size_t","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-10-18T19:11:40Z","receivedAt":"2018-10-18T19:11:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\nWhen streaming data from disk into a blob, use off_t instead of\nsize_t, which is a better choice for file length.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n\nThis is based on an old patch from 2017, which never made it to the list.\nI think it make sense to have off_t/size_t more consistent,\nreviews/comments are welcome.\n\nbulk-checkin.c | 4 ++--\n bulk-checkin.h | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 409ecb566b..2631e82d6c 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -189,7 +189,7 @@ static void prepare_to_stream(struct bulk_checkin_state *state,\n \n static int deflate_to_pack(struct bulk_checkin_state *state,\n \t\t\t   struct object_id *result_oid,\n-\t\t\t   int fd, size_t size,\n+\t\t\t   int fd, off_t size,\n \t\t\t   enum object_type type, const char *path,\n \t\t\t   unsigned flags)\n {\n@@ -258,7 +258,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n }\n \n int index_bulk_checkin(struct object_id *oid,\n-\t\t       int fd, size_t size, enum object_type type,\n+\t\t       int fd, off_t size, enum object_type type,\n \t\t       const char *path, unsigned flags)\n {\n \tint status = deflate_to_pack(&state, oid, fd, size, type,\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex f438f93811..09b2affdf3 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -7,7 +7,7 @@\n #include \"cache.h\"\n \n extern int index_bulk_checkin(struct object_id *oid,\n-\t\t\t      int fd, size_t size, enum object_type type,\n+\t\t\t      int fd, off_t size, enum object_type type,\n \t\t\t      const char *path, unsigned flags);\n \n extern void plug_bulk_checkin(void);\n-- \n2.19.0.271.gfe8321ec05\n\n"},{"id":"360891","messageId":"xmqq36t24s0h.fsf@gitster-ct.c.googlers.com","threadId":"49610","inReplyTo":"20181018191140.23318-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] index_bulk_checkin(): Take off_t, not size_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T01:58:06Z","receivedAt":"2018-10-19T01:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> bulk-checkin.c | 4 ++--\n>  bulk-checkin.h | 2 +-\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n\nIf you lost SP in your editor, then it is OK but if format-patch\nlost it for some reason, plasee tell me as we need to find the bug.\n\n>\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 409ecb566b..2631e82d6c 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -189,7 +189,7 @@ static void prepare_to_stream(struct bulk_checkin_state *state,\n>  \n>  static int deflate_to_pack(struct bulk_checkin_state *state,\n>  \t\t\t   struct object_id *result_oid,\n> -\t\t\t   int fd, size_t size,\n> +\t\t\t   int fd, off_t size,\n\nThe size is once casted to uintmax_t for recording in the object\nheader (which is fine), and then passed to stream_to_pack(), which\nstill takes, and more importantly, does comparisons and chunking in,\nsize_t after this patch.  Without xsize_t() around size passed in\nthe call to stream_to_pack(), you may silently be truncating off_t\ndown to size_t in this function.\n\n> @@ -258,7 +258,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n>  }\n>  \n>  int index_bulk_checkin(struct object_id *oid,\n> -\t\t       int fd, size_t size, enum object_type type,\n> +\t\t       int fd, off_t size, enum object_type type,\n>  \t\t       const char *path, unsigned flags)\n>  {\n>  \tint status = deflate_to_pack(&state, oid, fd, size, type,\n\nThis one is a thin wrapper around deflate_to_pack() above.\n\nIts sole caller is sha1-file.c::index_stream() and takes size_t from\nits callers, and passes size_t to index_bulk_checkin().\n\nThe sole caller of index_stream(), sha1-file.c::index_fd(), wants to\npass st->st_size, and it uses xsize_t() because index_stream() and\ncallchain underneath currently take size_t.  You want that callchain\nto take off_t with this patch.\n\nThe whole purpose of stream_to_pack() is to take potentially large\ninput from the file on the filesystem, chop that into manageable\nchunks and feed the underlying hashing and deflating machinery that\ntakes possibly smaller integer types to represent the sizes of data\nthey take in a single call, so once that function is taught to take\nofs_t I think you can say you converted the entire callchain from\nindex_fd() down.\n\nSo, this looks like a good starting step, but I suspect it needs one\nmore level of digging for it to become truly useful.\n"},{"id":"361295","messageId":"20181023161106.10097-1-tboegi@web.de","threadId":"49610","inReplyTo":"20181018191140.23318-1-tboegi@web.de","subject":"[PATCH/RFC v2 1/1] Use off_t instead of size_t for functions dealing with streamed checkin","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2018-10-23T16:11:06Z","receivedAt":"2018-10-23T16:11:11Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\nWhen streaming data from disk into a blob, it should be possible to commit\na file with a file size > 4 GiB using the streaming functionality in Git.\nBecause of the streaming there is no need to load the whole data into\nmemory at once.\nToday this is not possible on e.g. a 32 bit Linux system.\nThere is no good reason to limit the length of the file by using a size_t\nin the code, which is a 32 bit value.\nLoosen this restriction and use off_t instead of size_t in the call chain.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n\nThis is a suggestion for V2, changing even sha1-file.c,\nso that the whole patch makes more sense.\nThe initial commit of a >4Gib file was tested on a 32 bit system\n\nI didn't remove the wrapper functions, as I don't know\nwhat their purpose is.\n\nAnd: The commit message may need some tweaking, though\n\nbulk-checkin.c | 6 +++---\n bulk-checkin.h | 2 +-\n sha1-file.c    | 5 ++---\n 3 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 409ecb566b..34dbf5c4ea 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -96,7 +96,7 @@ static int already_written(struct bulk_checkin_state *state, struct object_id *o\n  */\n static int stream_to_pack(struct bulk_checkin_state *state,\n \t\t\t  git_hash_ctx *ctx, off_t *already_hashed_to,\n-\t\t\t  int fd, size_t size, enum object_type type,\n+\t\t\t  int fd, off_t size, enum object_type type,\n \t\t\t  const char *path, unsigned flags)\n {\n \tgit_zstream s;\n@@ -189,7 +189,7 @@ static void prepare_to_stream(struct bulk_checkin_state *state,\n \n static int deflate_to_pack(struct bulk_checkin_state *state,\n \t\t\t   struct object_id *result_oid,\n-\t\t\t   int fd, size_t size,\n+\t\t\t   int fd, off_t size,\n \t\t\t   enum object_type type, const char *path,\n \t\t\t   unsigned flags)\n {\n@@ -258,7 +258,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n }\n \n int index_bulk_checkin(struct object_id *oid,\n-\t\t       int fd, size_t size, enum object_type type,\n+\t\t       int fd, off_t size, enum object_type type,\n \t\t       const char *path, unsigned flags)\n {\n \tint status = deflate_to_pack(&state, oid, fd, size, type,\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex f438f93811..09b2affdf3 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -7,7 +7,7 @@\n #include \"cache.h\"\n \n extern int index_bulk_checkin(struct object_id *oid,\n-\t\t\t      int fd, size_t size, enum object_type type,\n+\t\t\t      int fd, off_t size, enum object_type type,\n \t\t\t      const char *path, unsigned flags);\n \n extern void plug_bulk_checkin(void);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex a4367b8f04..98d0f50ffa 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1934,7 +1934,7 @@ static int index_core(struct object_id *oid, int fd, size_t size,\n  * binary blobs, they generally do not want to get any conversion, and\n  * callers should avoid this code path when filters are requested.\n  */\n-static int index_stream(struct object_id *oid, int fd, size_t size,\n+static int index_stream(struct object_id *oid, int fd, off_t size,\n \t\t\tenum object_type type, const char *path,\n \t\t\tunsigned flags)\n {\n@@ -1959,8 +1959,7 @@ int index_fd(struct object_id *oid, int fd, struct stat *st,\n \t\tret = index_core(oid, fd, xsize_t(st->st_size), type, path,\n \t\t\t\t flags);\n \telse\n-\t\tret = index_stream(oid, fd, xsize_t(st->st_size), type, path,\n-\t\t\t\t   flags);\n+\t\tret = index_stream(oid, fd, st->st_size, type, path, flags);\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.11.0\n\n"}]}