Re: [PATCH v2 01/30] object-file-convert: stubs for converting from one object format to another
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 15, 2024, 11:21 UTC
- Message-ID
- <Zc3zyi42slXWGJTC@tanuki>
- In-Reply-To
- <20231002024034.2611-1-ebiederm@gmail.com>
On Sun, Oct 01, 2023 at 09:40:05PM -0500, Eric W. Biederman wrote:
Show 78 quoted lines
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> 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" <ebiederm@xmission.com>
> ---
> 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.h
>
> diff --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.o
> diff --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.
Show 12 quoted lines
> +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.
Show 9 quoted lines
> + 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.
Show 6 quoted lines
> + 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.
Show 28 quoted lines
> 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 >