{"thread":{"id":"60249","subject":"[PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin","startedAt":"2023-09-20T03:52:36Z","lastAt":"2023-09-28T09:40:34Z","messageCount":12,"participants":["Eric W. Biederman","Junio C Hamano","Taylor Blau","Oswald Buddenhagen"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"482046","messageId":"878r918ps3.fsf@gmail.froward.int.ebiederm.org","threadId":"60249","inReplyTo":null,"subject":"[PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-20T03:52:28Z","receivedAt":"2023-09-20T03:52:36Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"\nAs the code is written today index_bulk_checkin only accepts blobs.\nRemove the enum object_type parameter and rename index_bulk_checkin to\nindex_blob_bulk_checkin, index_stream to index_blob_stream,\ndeflate_to_pack to deflate_blob_to_pack, stream_to_pack to\nstream_blobk_to_pack, to make this explicit.\n\nNot supporting commits, tags, or trees has no downside as it is not\ncurrently supported now, and commits, tags, and trees being smaller by\ndesign do not have the problem that the problem that index_bulk_checkin\nwas built to solve.\n\nWhat is more this is very desiable from the context of the hash function\ntransition.\n\nFor blob objects it is straight forward to compute multiple hash\nfunctions during index_bulk_checkin as the object header and content of\na blob is the same no matter which hash function is being used to\ncompute the oid of a blob.\n\nFor commits, tress, and tags the object header and content that need to\nbe hashed ard different for different hashes. Even worse the object\nheader can not be known until the size of the content that needs to be\nhashed is known. The size of the content that needs to be hashed can\nnot be known until a complete pass is made through all of the variable\nlength entries of the original object.\n\nAs far as I can tell this extra pass defeats most of the purpose of\nstreaming, and it is much easier to implement with in memory buffers.\n\nSo if it is needed to write commits, trees, and tags directly to pack\nfiles writing a separate function to do the would be needed.\n\nSo let's just simplify the code base for now, simplify the development\nneeded for the hash function transition and only support blobs with the\nexisting bulk_checkin code.\n\nInspired-by: brian m. carlson \nSigned-off-by: \"Eric W. Biederman\" \n---\n bulk-checkin.c | 35 +++++++++++++++++------------------\n bulk-checkin.h | 6 +++---\n object-file.c | 12 ++++++------\n 3 files changed, 26 insertions(+), 27 deletions(-)\n\nThis is just a v2 of the description, that addresses Junio's\ncapitalization concern, and hopefully makes the justification clear to\nother people.\n\nI am sending it now mostly because the original version did not\nland on the mailing list for some reason. So I have switched\nwhich email account I am using for now.\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 73bff3a23d27..223562b4e748 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -155,10 +155,10 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n * status before calling us just in case we ask it to call us again\n * with a new pack.\n */\n-static int stream_to_pack(struct bulk_checkin_packfile *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 const char *path, unsigned flags)\n+static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n+\t\t\t int fd, size_t size, const char *path,\n+\t\t\t unsigned flags)\n {\n \tgit_zstream s;\n \tunsigned char ibuf[16384];\n@@ -170,7 +170,7 @@ static int stream_to_pack(struct bulk_checkin_packfile *state,\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n@@ -247,11 +247,10 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,\n \t\tdie_errno(\"unable to write pack header\");\n }\n \n-static int deflate_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t struct object_id *result_oid,\n-\t\t\t int fd, size_t size,\n-\t\t\t enum object_type type, const char *path,\n-\t\t\t unsigned flags)\n+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t\tstruct object_id *result_oid,\n+\t\t\t\tint fd, size_t size,\n+\t\t\t\tconst char *path, unsigned flags)\n {\n \toff_t seekback, already_hashed_to;\n \tgit_hash_ctx ctx;\n@@ -265,7 +264,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,\n \t\treturn error(\"cannot find the current offset\");\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n-\t\t\t\t\t type, size);\n+\t\t\t\t\t OBJ_BLOB, size);\n \tthe_hash_algo->init_fn(&ctx);\n \tthe_hash_algo->update_fn(&ctx, obuf, header_len);\n \n@@ -282,8 +281,8 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tidx->offset = state->offset;\n \t\t\tcrc32_begin(state->f);\n \t\t}\n-\t\tif (!stream_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t fd, size, type, path, flags))\n+\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n+\t\t\t\t\t fd, size, path, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -350,12 +349,12 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t}\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 const char *path, unsigned flags)\n+int index_blob_bulk_checkin(struct object_id *oid,\n+\t\t\t int fd, size_t size,\n+\t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,\n-\t\t\t\t path, flags);\n+\tint status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,\n+\t\t\t\t\t path, flags);\n \tif (!odb_transaction_nesting)\n \t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n \treturn status;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 48fe9a6e9171..aa7286a7b3e1 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -9,9 +9,9 @@\n void prepare_loose_object_bulk_checkin(void);\n void fsync_loose_object_bulk_checkin(int fd, const char *filename);\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 const char *path, unsigned flags);\n+int index_blob_bulk_checkin(struct object_id *oid,\n+\t\t\t int fd, size_t size,\n+\t\t\t const char *path, unsigned flags);\n \n /*\n * Tell the object database to optimize for adding\ndiff --git a/object-file.c b/object-file.c\nindex 7dc0c4bfbba8..7c7afe579364 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2446,11 +2446,11 @@ static int index_core(struct index_state *istate,\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-\t\t\tenum object_type type, const char *path,\n-\t\t\tunsigned flags)\n+static int index_blob_stream(struct object_id *oid, int fd, size_t size,\n+\t\t\t const char *path,\n+\t\t\t unsigned flags)\n {\n-\treturn index_bulk_checkin(oid, fd, size, type, path, flags);\n+\treturn index_blob_bulk_checkin(oid, fd, size, path, flags);\n }\n \n int index_fd(struct index_state *istate, struct object_id *oid,\n@@ -2472,8 +2472,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, 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_blob_stream(oid, fd, xsize_t(st->st_size), path,\n+\t\t\t\t\tflags);\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.41.0\n\nEric\n"},{"id":"482049","messageId":"xmqqr0mtcosy.fsf@gitster.g","threadId":"60249","inReplyTo":"878r918ps3.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T06:59:57Z","receivedAt":"2023-09-20T07:00:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric W. Biederman\" writes:\n\n> As the code is written today index_bulk_checkin only accepts blobs.\n> Remove the enum object_type parameter and rename index_bulk_checkin to\n> index_blob_bulk_checkin, index_stream to index_blob_stream,\n> deflate_to_pack to deflate_blob_to_pack, stream_to_pack to\n> stream_blobk_to_pack, to make this explicit.\n\n> Not supporting commits, tags, or trees has no downside as it is not\n> currently supported now, and commits, tags, and trees being smaller by\n> design do not have the problem that the problem that index_bulk_checkin\n> was built to solve.\n\nExactly. The streaming was primarily to help dealing with huge\nblobs that cannot be held in-core. Of course other parts (like\ncomparing them) of the system would require to hold them in-core\nso some things may not work for them, but at least it is a start\nto be able to _hash_ them to store them in the object store and to\ngive them names.\n\n> What is more this is very desiable from the context of the hash function\n> transition.\n\nA bit hard to parse; perhaps want a comma before \"this\"?\n\n> For blob objects it is straight forward to compute multiple hash\n> functions during index_bulk_checkin as the object header and content of\n> a blob is the same no matter which hash function is being used to\n> compute the oid of a blob.\n\nOK.\n\n> For commits, tress, and tags the object header and content that need to\n> be hashed ard different for different hashes. Even worse the object\n> header can not be known until the size of the content that needs to be\n> hashed is known. The size of the content that needs to be hashed can\n> not be known until a complete pass is made through all of the variable\n> length entries of the original object.\n\n\"tress\" -> \"trees\". Also a comma after \"worse\".\n\n> As far as I can tell this extra pass defeats most of the purpose of\n> streaming, and it is much easier to implement with in memory buffers.\n\nThe purpose of streaming being the ability to hash and compute the\nobject name without having to hold the entirety of the object, I am\nnot sure the above is a good argument. You can run multiple passes\nby streaming the same data twice if you needed to, and how much\neasier the implementation may become if you can assume that you can\nhold everything in-core, what you cannot fit in-core would not fit\nin-core, so ...\n\n> So if it is needed to write commits, trees, and tags directly to pack\n> files writing a separate function to do the would be needed.\n\nBut I am OK with this conclusion. As the way to compute the\nfallback hashes for different types of objects are very different,\ncompared to a single-hash world where as long as you come up with a\nserialization you have only a single way to hash and name the\nobject. We would end up having separate helper functions per target\ntype anyway, even if we kept a single entry point function like\nindex_stream(). The single entry point function will only be used\nto just dispatch to type specific ones, so renaming what we have today\nand making it clear they are for \"blobs\" does make sense.\n"},{"id":"482054","messageId":"87zg1h58xa.fsf@gmail.froward.int.ebiederm.org","threadId":"60249","inReplyTo":"xmqqr0mtcosy.fsf@gitster.g","subject":"Re: [PATCH v2] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-20T12:24:49Z","receivedAt":"2023-09-20T12:25:19Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Junio C Hamano writes:\n\n> \"Eric W. Biederman\" writes:\n>\n>> As far as I can tell this extra pass defeats most of the purpose of\n>> streaming, and it is much easier to implement with in memory buffers.\n>\n> The purpose of streaming being the ability to hash and compute the\n> object name without having to hold the entirety of the object, I am\n> not sure the above is a good argument. You can run multiple passes\n> by streaming the same data twice if you needed to, and how much\n> easier the implementation may become if you can assume that you can\n> hold everything in-core, what you cannot fit in-core would not fit\n> in-core, so ...\n\nYes this wording needs to be clarified.\n\nIf streaming to handle objects that don't fit in memory is the purpose,\nI agree there are slow multi-pass ways to deal with trees, commits and\ntags.\n\nIf writing directly to the pack is the purpose, using an in-core\nbuffer for trees, commits, and tags is better.\n\nI will put on the wording on the back burner and see what I come up\nwith.\n\n>> So if it is needed to write commits, trees, and tags directly to pack\n>> files writing a separate function to do the would be needed.\n>\n> But I am OK with this conclusion. As the way to compute the\n> fallback hashes for different types of objects are very different,\n> compared to a single-hash world where as long as you come up with a\n> serialization you have only a single way to hash and name the\n> object. We would end up having separate helper functions per target\n> type anyway, even if we kept a single entry point function like\n> index_stream(). The single entry point function will only be used\n> to just dispatch to type specific ones, so renaming what we have today\n> and making it clear they are for \"blobs\" does make sense.\n\nGood. I am glad I am able to step back and successfully explain the\nwhys of things.\n\nEric\n\n"},{"id":"482343","messageId":"87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org","threadId":"60249","inReplyTo":"87zg1h58xa.fsf@gmail.froward.int.ebiederm.org","subject":"[PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-26T15:58:43Z","receivedAt":"2023-09-26T15:58:55Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"\nAs the code is written today index_bulk_checkin only accepts blobs.\nRemove the enum object_type parameter and rename index_bulk_checkin to\nindex_blob_bulk_checkin, index_stream to index_blob_stream,\ndeflate_to_pack to deflate_blob_to_pack, stream_to_pack to\nstream_blob_to_pack, to make this explicit.\n\nNot supporting commits, tags, or trees has no downside as it is not\ncurrently supported now, and commits, tags, and trees being smaller by\ndesign do not have the problem that the problem that index_bulk_checkin\nwas built to solve.\n\nBefore we start adding code to support the hash function transition\nsupporting additional objects types in index_bulk_checkin has no real\nadditional cost, just an extra function parameter to know what the\nobject type is. Once we begin the hash function transition this is not\nthe case.\n\nThe hash function transition document specifies that a repository with\ncompatObjectFormat enabled will compute and store both the SHA-1 and\nSHA-256 hash of every object in the repository.\n\nWhat makes this a challenge is that it is not just an additional hash\nover the same object. Instead the hash function transition document\nspecifies that the compatibility hash (specified with\ncompatObjectFormat) be computed over the equivalent object that another\ngit repository whose storage hash (specified with objectFormat) would\nstore. When comparing equivalent repositories built with different\nstorage hash functions, the oids embedded in objects used to refer to\nother objects differ and the location of signatures within objects\ndiffer.\n\nAs blob objects have neither oids referring to other objects nor stored\nsignatures their storage hash and their compatibility hash are computed\nover the same object.\n\nThe other kinds of objects: trees, commits, and tags, all store oids\nreferring to other objects. Signatures are stored in commit and tag\nobjects. As oids and the tags to store signatures are not the same size\nin repositories built with different storage hashes the size of the\nequivalent objects are also different.\n\nA version of index_bulk_checkin that supports more than just blobs when\ncomputing both the SHA-1 and the SHA-256 of every object added would\nneed a different, and more expensive structure. The structure is more\nexpensive because it would be required to temporarily buffering the\nequivalent object the compatibility hash needs to be computed over.\n\nA temporary object is needed, because before a hash over an object can\ncomputed it's object header needs to be computed. One of the members of\nthe object header is the entire size of the object. To know the size of\nan equivalent object an entire pass over the original object needs to be\nmade, as trees, commits, and tags are composed of a variable number of\nvariable sized pieces. Unfortunately there is no formula to compute the\nsize of an equivalent object from just the size of the original object.\n\nAvoid all of those future complications by limiting index_bulk_checkin\nto only work on blobs.\n\nInspired-by: brian m. carlson \nSigned-off-by: \"Eric W. Biederman\" \n---\n bulk-checkin.c | 35 +++++++++++++++++------------------\n bulk-checkin.h | 6 +++---\n object-file.c | 12 ++++++------\n 3 files changed, 26 insertions(+), 27 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 73bff3a23d27..223562b4e748 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -155,10 +155,10 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id\n * status before calling us just in case we ask it to call us again\n * with a new pack.\n */\n-static int stream_to_pack(struct bulk_checkin_packfile *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 const char *path, unsigned flags)\n+static int stream_blob_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t git_hash_ctx *ctx, off_t *already_hashed_to,\n+\t\t\t int fd, size_t size, const char *path,\n+\t\t\t unsigned flags)\n {\n \tgit_zstream s;\n \tunsigned char ibuf[16384];\n@@ -170,7 +170,7 @@ static int stream_to_pack(struct bulk_checkin_packfile *state,\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \n@@ -247,11 +247,10 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,\n \t\tdie_errno(\"unable to write pack header\");\n }\n \n-static int deflate_to_pack(struct bulk_checkin_packfile *state,\n-\t\t\t struct object_id *result_oid,\n-\t\t\t int fd, size_t size,\n-\t\t\t enum object_type type, const char *path,\n-\t\t\t unsigned flags)\n+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,\n+\t\t\t\tstruct object_id *result_oid,\n+\t\t\t\tint fd, size_t size,\n+\t\t\t\tconst char *path, unsigned flags)\n {\n \toff_t seekback, already_hashed_to;\n \tgit_hash_ctx ctx;\n@@ -265,7 +264,7 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,\n \t\treturn error(\"cannot find the current offset\");\n \n \theader_len = format_object_header((char *)obuf, sizeof(obuf),\n-\t\t\t\t\t type, size);\n+\t\t\t\t\t OBJ_BLOB, size);\n \tthe_hash_algo->init_fn(&ctx);\n \tthe_hash_algo->update_fn(&ctx, obuf, header_len);\n \n@@ -282,8 +281,8 @@ static int deflate_to_pack(struct bulk_checkin_packfile *state,\n \t\t\tidx->offset = state->offset;\n \t\t\tcrc32_begin(state->f);\n \t\t}\n-\t\tif (!stream_to_pack(state, &ctx, &already_hashed_to,\n-\t\t\t\t fd, size, type, path, flags))\n+\t\tif (!stream_blob_to_pack(state, &ctx, &already_hashed_to,\n+\t\t\t\t\t fd, size, path, flags))\n \t\t\tbreak;\n \t\t/*\n \t\t * Writing this object to the current pack will make\n@@ -350,12 +349,12 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t}\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 const char *path, unsigned flags)\n+int index_blob_bulk_checkin(struct object_id *oid,\n+\t\t\t int fd, size_t size,\n+\t\t\t const char *path, unsigned flags)\n {\n-\tint status = deflate_to_pack(&bulk_checkin_packfile, oid, fd, size, type,\n-\t\t\t\t path, flags);\n+\tint status = deflate_blob_to_pack(&bulk_checkin_packfile, oid, fd, size,\n+\t\t\t\t\t path, flags);\n \tif (!odb_transaction_nesting)\n \t\tflush_bulk_checkin_packfile(&bulk_checkin_packfile);\n \treturn status;\ndiff --git a/bulk-checkin.h b/bulk-checkin.h\nindex 48fe9a6e9171..aa7286a7b3e1 100644\n--- a/bulk-checkin.h\n+++ b/bulk-checkin.h\n@@ -9,9 +9,9 @@\n void prepare_loose_object_bulk_checkin(void);\n void fsync_loose_object_bulk_checkin(int fd, const char *filename);\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 const char *path, unsigned flags);\n+int index_blob_bulk_checkin(struct object_id *oid,\n+\t\t\t int fd, size_t size,\n+\t\t\t const char *path, unsigned flags);\n \n /*\n * Tell the object database to optimize for adding\ndiff --git a/object-file.c b/object-file.c\nindex 7dc0c4bfbba8..7c7afe579364 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2446,11 +2446,11 @@ static int index_core(struct index_state *istate,\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-\t\t\tenum object_type type, const char *path,\n-\t\t\tunsigned flags)\n+static int index_blob_stream(struct object_id *oid, int fd, size_t size,\n+\t\t\t const char *path,\n+\t\t\t unsigned flags)\n {\n-\treturn index_bulk_checkin(oid, fd, size, type, path, flags);\n+\treturn index_blob_bulk_checkin(oid, fd, size, path, flags);\n }\n \n int index_fd(struct index_state *istate, struct object_id *oid,\n@@ -2472,8 +2472,8 @@ int index_fd(struct index_state *istate, struct object_id *oid,\n \t\tret = index_core(istate, oid, fd, xsize_t(st->st_size),\n \t\t\t\t type, path, 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_blob_stream(oid, fd, xsize_t(st->st_size), path,\n+\t\t\t\t\tflags);\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.41.0\n\n"},{"id":"482358","messageId":"xmqqmsx8mwr4.fsf@gitster.g","threadId":"60249","inReplyTo":"87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-26T21:48:31Z","receivedAt":"2023-09-26T22:52:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric W. Biederman\" writes:\n\n> As the code is written today index_bulk_checkin only accepts blobs.\n> Remove the enum object_type parameter and rename index_bulk_checkin to\n> index_blob_bulk_checkin, index_stream to index_blob_stream,\n> deflate_to_pack to deflate_blob_to_pack, stream_to_pack to\n> stream_blob_to_pack, to make this explicit.\n>\n> Not supporting commits, tags, or trees has no downside as it is not\n> currently supported now, and commits, tags, and trees being smaller by\n> design do not have the problem that the problem that index_bulk_checkin\n> was built to solve.\n>\n> Before we start adding code to support the hash function transition\n> supporting additional objects types in index_bulk_checkin has no real\n> additional cost, just an extra function parameter to know what the\n> object type is. Once we begin the hash function transition this is not\n> the case.\n>\n> The hash function transition document specifies that a repository with\n> compatObjectFormat enabled will compute and store both the SHA-1 and\n> SHA-256 hash of every object in the repository.\n>\n> What makes this a challenge is that it is not just an additional hash\n> over the same object. Instead the hash function transition document\n> specifies that the compatibility hash (specified with\n> compatObjectFormat) be computed over the equivalent object that another\n> git repository whose storage hash (specified with objectFormat) would\n> store. When comparing equivalent repositories built with different\n> storage hash functions, the oids embedded in objects used to refer to\n> other objects differ and the location of signatures within objects\n> differ.\n>\n> As blob objects have neither oids referring to other objects nor stored\n> signatures their storage hash and their compatibility hash are computed\n> over the same object.\n>\n> The other kinds of objects: trees, commits, and tags, all store oids\n> referring to other objects. Signatures are stored in commit and tag\n> objects. As oids and the tags to store signatures are not the same size\n> in repositories built with different storage hashes the size of the\n> equivalent objects are also different.\n>\n> A version of index_bulk_checkin that supports more than just blobs when\n> computing both the SHA-1 and the SHA-256 of every object added would\n> need a different, and more expensive structure. The structure is more\n> expensive because it would be required to temporarily buffering the\n> equivalent object the compatibility hash needs to be computed over.\n>\n> A temporary object is needed, because before a hash over an object can\n> computed it's object header needs to be computed. One of the members of\n> the object header is the entire size of the object. To know the size of\n> an equivalent object an entire pass over the original object needs to be\n> made, as trees, commits, and tags are composed of a variable number of\n> variable sized pieces. Unfortunately there is no formula to compute the\n> size of an equivalent object from just the size of the original object.\n>\n> Avoid all of those future complications by limiting index_bulk_checkin\n> to only work on blobs.\n\nThanks. Will queue.\n"},{"id":"482361","messageId":"ZROHrSmmZOIE6bl9@nand.local","threadId":"60249","inReplyTo":"xmqqmsx8mwr4.fsf@gitster.g","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-27T01:38:53Z","receivedAt":"2023-09-27T02:16:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 26, 2023 at 02:48:31PM -0700, Junio C Hamano wrote:\n> > Avoid all of those future complications by limiting index_bulk_checkin\n> > to only work on blobs.\n>\n> Thanks. Will queue.\n\nHmm. I wonder if retaining some flexibility in the bulk-checkin\nmechanism may be worthwhile. We discussed at the Contributor's\nSummit[^1] today that the bulk-checkin system may be a good fit for\npacking any blobs/trees created by `merge-tree` or `replay` instead of\nwriting them out as loose objects.\n\nBeing able to write trees in addition to blobs is definitely important\nthere, so we may want to wait on merging this down until that direction\nsolidifies a bit more. (FWIW, I started working on that today and hope\nto have patches on the list in the next day or two).\n\nAlternatively, if there is an urgency to merge these down, we can always\ncome back to it in the future and revert it if need be. Either way :-).\n\nThanks,\nTaylor\n\n[^1]: I'll clean up our notes in the next day or two and share them with\n the list here.\n"},{"id":"482366","messageId":"xmqqil7wmf50.fsf@gitster.g","threadId":"60249","inReplyTo":"ZROHrSmmZOIE6bl9@nand.local","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-27T04:08:59Z","receivedAt":"2023-09-27T05:26:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Hmm. I wonder if retaining some flexibility in the bulk-checkin\n> mechanism may be worthwhile. We discussed at the Contributor's\n> Summit[^1] today that the bulk-checkin system may be a good fit for\n> packing any blobs/trees created by `merge-tree` or `replay` instead of\n> writing them out as loose objects.\n\nBut see the last paragraph of my review comments for the earlier\nround upthread. This particular function implements logic that is\nonly applicable to blob objects, and streaming trees, commits, and\ntags will need their own separate helper functions. And when they\nare written, the top-level stream_to_pack() function can be\nreintroduced, which will be a thin dispatcher to the four\ntype-specific helpers.\n"},{"id":"482376","messageId":"ZRQ9aSeu/wpJERuV@nand.local","threadId":"60249","inReplyTo":"xmqqil7wmf50.fsf@gitster.g","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-27T14:34:17Z","receivedAt":"2023-09-27T14:34:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 26, 2023 at 09:08:59PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Hmm. I wonder if retaining some flexibility in the bulk-checkin\n> > mechanism may be worthwhile. We discussed at the Contributor's\n> > Summit[^1] today that the bulk-checkin system may be a good fit for\n> > packing any blobs/trees created by `merge-tree` or `replay` instead of\n> > writing them out as loose objects.\n>\n> But see the last paragraph of my review comments for the earlier\n> round upthread. This particular function implements logic that is\n> only applicable to blob objects, and streaming trees, commits, and\n> tags will need their own separate helper functions. And when they\n> are written, the top-level stream_to_pack() function can be\n> reintroduced, which will be a thin dispatcher to the four\n> type-specific helpers.\n\nI am not sure that I follow. If we have an address in memory from which\nwe want to stream raw bytes directly to the packfile, that should work\nfor all objects regardless of type, no?\n\nHaving stream_to_pack() take a non-OBJ_BLOB 'type' argument would be OK\nprovided that the file descriptor 'fd' contains the raw contents of an\nobject which matches type 'type'.\n\nIIUC, for callers like in the ORT backend which assemble e.g. the raw\nbytes of a tree in its merge-ort.c::write_tree() function like so:\n\n for (i = 0; i < nr; i++) {\n struct merged_info *mi = versions->items[offset+i].util;\n struct version_info *ri = &mi->result;\n\n strbuf_addf(&buf, \"%o %s%c\", ri->mode,\n versions->items[offset+i].string, '\\0');\n strbuf_add(&buf, ri->oid.hash, hash_size);\n }\n\nwe'd want some variant of stream_to_pack() that acts on a 'void *,\nsize_t' pair rather than an 'int (fd), size_t' pair. Likely its\nsignature would look something like:\n\n /* write raw bytes to a bulk-checkin pack */\n static int write_to_pack(struct bulk_checkin_packfile *state,\n git_hash_ctx *ctx, off_t *already_hashed_to,\n void *ptr, size_t size, enum object_type type,\n unsigned flags);\n\n /* write an object from memory to a bulk-checkin pack */\n static int deflate_to_pack_mem(struct bulk_checkin_packfile *state,\n struct object_id *result_oid,\n void *ptr, size_t size,\n enum object_type type, unsigned flags);\n\n, where the above are analogous to `stream_to_pack()` and\n`deflate_to_pack()`, respectively. ORT would be taught to conditionally\nreplace calls like:\n\n write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);\n\nwith:\n\n deflate_to_pack_mem(&state, result_oid, buf.buf, buf.len,\n OBJ_TREE, HASH_WRITE_OBJECT);\n\nI guess after writing all of that out, you'd never have any callers of\nthe existing `deflate_to_pack()` function that pass a file descriptor\ncontaining the contents of a non-blob object. So in that sense, I don't\nthink that my proposal would change anything about this patch.\n\nBut I worry that I am missing something here, so having a sanity check\nwould be appreciated ;-).\n\nThanks,\nTaylor\n"},{"id":"482378","messageId":"xmqq7cobmvjt.fsf@gitster.g","threadId":"60249","inReplyTo":"ZRQ9aSeu/wpJERuV@nand.local","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-27T16:26:46Z","receivedAt":"2023-09-27T16:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I am not sure that I follow. If we have an address in memory from which\n> we want to stream raw bytes directly to the packfile, that should work\n> for all objects regardless of type, no?\n\nFor a single hash world, yes. For keeping track of \"the other hash\"\nand correspondence, you need to (1) interpret the contents of the\nobject (e.g., if you received a tree contents for SHA-1 repository,\nyou'd need to split them into tree entries and know which parts of\nthe bytestream are SHA-1 hashes of the tree contebnts), (2) come\nup with the corresponding tree contents in the SHA-256 world (you\nshould be able to do that now you know SHA-1 names of the objects\ndirectly referred to by the tree) and hash that using SHA-256, and\n(3) remember the SHA-1 and the SHA-256 name correspondence of the\ntree object you just hashed, in addition to the usual (4) hashing\nthe contents using SHA-1 hash algorithm without caring what the byte\nstream represents.\n"},{"id":"482412","messageId":"87a5t7js8f.fsf@gmail.froward.int.ebiederm.org","threadId":"60249","inReplyTo":"xmqq7cobmvjt.fsf@gitster.g","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-27T20:06:40Z","receivedAt":"2023-09-27T20:06:47Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Junio C Hamano writes:\n\n> Taylor Blau writes:\n>\n>> I am not sure that I follow. If we have an address in memory from which\n>> we want to stream raw bytes directly to the packfile, that should work\n>> for all objects regardless of type, no?\n>\n> For a single hash world, yes. For keeping track of \"the other hash\"\n> and correspondence, you need to (1) interpret the contents of the\n> object (e.g., if you received a tree contents for SHA-1 repository,\n> you'd need to split them into tree entries and know which parts of\n> the bytestream are SHA-1 hashes of the tree contebnts), (2) come\n> up with the corresponding tree contents in the SHA-256 world (you\n> should be able to do that now you know SHA-1 names of the objects\n> directly referred to by the tree) and hash that using SHA-256, and\n> (3) remember the SHA-1 and the SHA-256 name correspondence of the\n> tree object you just hashed, in addition to the usual (4) hashing\n> the contents using SHA-1 hash algorithm without caring what the byte\n> stream represents.\n\nIf it helps I just posted a patchset that implements what it takes\nto deal with objects small enough to live in-core.\n\nYou can read object-file-convert.c to see what it takes to generate\nan object in the other hash function world.\n\nThe exercise for the reader is how to apply this to objects that\nare too large to fit in memory.\n\nEric\n\n"},{"id":"482413","messageId":"87pm23idci.fsf@gmail.froward.int.ebiederm.org","threadId":"60249","inReplyTo":"ZROHrSmmZOIE6bl9@nand.local","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2023-09-27T20:13:33Z","receivedAt":"2023-09-27T20:13:38Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Taylor Blau writes:\n\n> On Tue, Sep 26, 2023 at 02:48:31PM -0700, Junio C Hamano wrote:\n>> > Avoid all of those future complications by limiting index_bulk_checkin\n>> > to only work on blobs.\n>>\n>> Thanks. Will queue.\n>\n> Hmm. I wonder if retaining some flexibility in the bulk-checkin\n> mechanism may be worthwhile. We discussed at the Contributor's\n> Summit[^1] today that the bulk-checkin system may be a good fit for\n> packing any blobs/trees created by `merge-tree` or `replay` instead of\n> writing them out as loose objects.\n>\n> Being able to write trees in addition to blobs is definitely important\n> there, so we may want to wait on merging this down until that direction\n> solidifies a bit more. (FWIW, I started working on that today and hope\n> to have patches on the list in the next day or two).\n>\n> Alternatively, if there is an urgency to merge these down, we can always\n> come back to it in the future and revert it if need be. Either way\n> :-).\n\nThere are two things that index_bulk_checkin does.\n- Handle objects that are too large to fit into a memory\n- Place objects immediately in a pack.\n\nDo I read things correctly that you want to take an object that is small\nenough to fit into memory, and to immediately into a pack?\n\nIf so you essentially want write_object_file that directly writes to a\npack?\n\nA version of write_object_file that that directly writes to a pack is\nmuch easier than the chunking that index_bulk_checkin does.\n\nPerhaps your version could be called index_pack_checkin?\n\nEric\n"},{"id":"482430","messageId":"ZRVJ7pjQ35Stw9X4@ugly","threadId":"60249","inReplyTo":"87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-09-28T09:39:58Z","receivedAt":"2023-09-28T09:40:34Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"just language nits on the commit message:\n\nOn Tue, Sep 26, 2023 at 10:58:43AM -0500, Eric W. Biederman wrote:\n>Not supporting commits, tags, or trees has no downside as it is not\n>currently supported now, and commits, tags, and trees being smaller by\n>design do not have the problem that the problem that index_bulk_checkin\n\t\t\t\t ^^^^^^^^^^^^^^^^\n\t\t\t\t duplicated!\n\n>was built to solve.\n\n>A version of index_bulk_checkin that supports more than just blobs when\n>computing both the SHA-1 and the SHA-256 of every object added would\n>need a different, and more expensive structure. The structure is more\n>expensive because it would be required to temporarily buffering the\n\t\t\t\t\t\t\t ^^^\n\t\t\t\t\t\t\tno 'ing' here.\n\n>equivalent object the compatibility hash needs to be computed over.\n\n\n>A temporary object is needed, because before a hash over an object can\n>computed it's\n>\n\"be computed, its\"\n\n>object header needs to be computed. One of the members of\n\nregards\n"}]}