Re: [PATCH v2 01/30] object-file-convert: stubs for converting from one object format to another
From: Patrick Steinhardt <hidden>
Date: 2024-02-15 11:21:54
On Sun, Oct 01, 2023 at 09:40:05PM -0500, Eric W. Biederman wrote:
quoted hunk ↗ jump to hunk
From: "Eric W. Biederman" <redacted> Two basic functions are provided: - convert_object_file Takes an object file it's type and hash algorithm and converts it into the equivalent object file that would have been generated with hash algorithm "to". For blob objects there is no conversation to be done and it is an error to use this function on them. For commit, tree, and tag objects embedded oids are replaced by the oids of the objects they refer to with those objects and their object ids reencoded in with the hash algorithm "to". Signatures are rearranged so that they remain valid after the object has been reencoded. - repo_oid_to_algop which takes an oid that refers to an object file and returns the oid of the equivalent object file generated with the target hash algorithm. The pair of files object-file-convert.c and object-file-convert.h are introduced to hold as much of this logic as possible to keep this conversion logic cleanly separated from everything else and in the hopes that someday the code will be clean enough git can support compiling out support for sha1 and the various conversion functions. Signed-off-by: "Eric W. Biederman" <redacted> --- Makefile | 1 + object-file-convert.c | 57 +++++++++++++++++++++++++++++++++++++++++++ object-file-convert.h | 24 ++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 object-file-convert.c create mode 100644 object-file-convert.hdiff --git a/Makefile b/Makefile index 577630936535..f7e824f25cda 100644 --- a/Makefile +++ b/Makefile@@ -1073,6 +1073,7 @@ LIB_OBJS += notes-cache.o LIB_OBJS += notes-merge.o LIB_OBJS += notes-utils.o LIB_OBJS += notes.o +LIB_OBJS += object-file-convert.o LIB_OBJS += object-file.o LIB_OBJS += object-name.o LIB_OBJS += object.odiff --git a/object-file-convert.c b/object-file-convert.c new file mode 100644 index 000000000000..4777aba83636 --- /dev/null +++ b/object-file-convert.c@@ -0,0 +1,57 @@ +#include "git-compat-util.h" +#include "gettext.h" +#include "strbuf.h" +#include "repository.h" +#include "hash-ll.h" +#include "object.h" +#include "object-file-convert.h" + +int repo_oid_to_algop(struct repository *repo, const struct object_id *src, + const struct git_hash_algo *to, struct object_id *dest) +{ + /* + * If the source algorithm is not set, then we're using the + * default hash algorithm for that object. + */ + const struct git_hash_algo *from = + src->algo ? &hash_algos[src->algo] : repo->hash_algo; + + if (from == to) { + if (src != dest) + oidcpy(dest, src); + return 0; + } + return -1; +}
In it's current form, `repo_oid_to_algop()` basically never does anything except for copying over the object ID because we do not handle the case where object hashes are different. I assume this is intended, as we basically only provide stubs in this commit. But still, it would help to document this in-code as well with a comment.
+int convert_object_file(struct strbuf *outbuf,
+ const struct git_hash_algo *from,
+ const struct git_hash_algo *to,
+ const void *buf, size_t len,
+ enum object_type type,
+ int gentle)
+{
+ int ret;
+
+ /* Don't call this function when no conversion is necessary */
+ if ((from == to) || (type == OBJ_BLOB))
+ BUG("Refusing noop object file conversion");The extra braces around comparisons are unneeded and to the best of my knowledge not customary in our code base. Also, error messages should start with a lower-case letter.
+ switch (type) {
+ case OBJ_COMMIT:
+ case OBJ_TREE:
+ case OBJ_TAG:
+ default:
+ /* Not implemented yet, so fail. */
+ ret = -1;
+ break;
+ }It's a bit weird that we handle all object types except for blobs separately, and then still have a `default` statement. I would've thought that we should handle the object types specifically and set `ret = -1` for all of them, and then the `default` case would instead call `BUG()` due to an unknown object type.
+ if (!ret)
+ return 0;
+ if (gentle) {
+ strbuf_release(outbuf);
+ return ret;
+ }Do you really intend to call `strbuf_release()` on the caller provided buffer, or should this rather be `strbuf_reset()`? Memory management of such an in/out parameter should typically be handled by the caller, not the callee.
+ die(_("Failed to convert object from %s to %s"),
+ from->name, to->name);
+}The error message should start with a lower-case letter.
quoted hunk ↗ jump to hunk
diff --git a/object-file-convert.h b/object-file-convert.h new file mode 100644 index 000000000000..a4f802aa8eea --- /dev/null +++ b/object-file-convert.h@@ -0,0 +1,24 @@ +#ifndef OBJECT_CONVERT_H +#define OBJECT_CONVERT_H + +struct repository; +struct object_id; +struct git_hash_algo; +struct strbuf; +#include "object.h" + +int repo_oid_to_algop(struct repository *repo, const struct object_id *src, + const struct git_hash_algo *to, struct object_id *dest); + +/* + * Convert an object file from one hash algorithm to another algorithm. + * Return -1 on failure, 0 on success. + */ +int convert_object_file(struct strbuf *outbuf, + const struct git_hash_algo *from, + const struct git_hash_algo *to, + const void *buf, size_t len, + enum object_type type, + int gentle);
It would be nice to document what `gentle` does. Patrick
+#endif /* OBJECT_CONVERT_H */ -- 2.41.0
Attachments
- signature.asc [application/pgp-signature] 833 bytes