Re: [PATCH v3 1/6] odb: introduce interface to generate packfiles

Patrick Steinhardt <[email protected]> writes:

> Packfiles have two primary use cases:
>
>   - They are used to store objects at rest in a Git repository.
>
>   - They are used on the transport layer to transfer objects between two
>     repositories.
>
> The first class is closely tied to a given object database backend, and
> as such this use is highly specific to how such a backend decides to
> store its data. This shows in git-pack-objects(1), which is used by
> git-repack(1) et al to optimize the object database, which supports lots
> of options that are closely coupled with how data is stored.
>
> But the second class is quite a lot more generic: we don't care about
> specifics of how the object database stores its objects, but to generate
> the packfiles we only care about the object graph itself. Still, this
> use case is also coupled with git-pack-objects(1).
>
> Unfortunately, because git-pack-objects(1) covers both classes, the
> result is that it is very hard to port the whole command to properly
> support pluggable object databases. There are simply way too many
> options that an alternative implementation will have a very hard time to
> support in the first place.
>
> And despite being hard to implement, it's also quite unnecessary to
> implement those backend-specific options. Optimizing the object database
> has already been made pluggable, and an alternative implementation is
> unlikely to care about cruft packs, unpacked objects, keep packs and the
> like. But we still need to make at least _parts_ of the packfile
> generation pluggable so that backends can generate packfiles for the
> transport layer itself.
>
> Introduce a new interface that lets backends generate a new packfile and
> implement that interface for the "files" backend. The options supported
> by the callback are exactly the set of options that are required for the
> transport layer, but nothing more.
>
> This means that git-pack-objects(1) itself cannot be ported over to this
> new interface, but as explained above that's a hard feat to pull off due
> to the backend-specific features. Ideally though, we should expose the
> ability to generate arbitrary packfiles using this interface. The intent
> of this is to eventually introduce a git-objects(1) subcommand (similar
> to git-refs(1)) that exposes generic interfaces for accessing everything
> related to the object database. In that case, we are able to expose only
> those options that are generic.
>
> Subsequent commits will convert git-upload-pack(1), git-send-pack(1) and
> git-bundle(1) to use this interface.
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb.c              |  21 ++++++++
>  odb.h              | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  odb/source-files.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  odb/source.h       |  33 ++++++++++++
>  4 files changed, 355 insertions(+)
>
> diff --git a/odb.c b/odb.c
> index caf1d0f542..cd9d5b48bc 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -1046,6 +1046,27 @@ bool odb_optimize_required(struct object_database *odb,
>  	return odb_source_optimize_required(odb->sources, opts);
>  }
>  
> +void odb_generate_pack_options_release(struct odb_generate_pack_options *opts)
> +{
> +	oid_array_clear(&opts->wants);
> +	oid_array_clear(&opts->haves);
> +	oid_array_clear(&opts->shallows);
> +}
> +
> +int odb_generate_pack(struct object_database *odb,
> +		      struct odb_pack_generator **out,
> +		      const struct odb_generate_pack_options *opts)
> +{
> +	if (!odb->sources->generate_pack)
> +		return error(_("primary object source does not support generating packfiles"));
> +	return odb_source_generate_pack(odb->sources, out, opts);
> +}

Perhaps a stupid question but the opts->pack_fd is documented:

> +struct odb_generate_pack_options {
> ...
> +	/*
> +	 * File descriptor that the generated pack shall be written to. If set
> +	 * to `-1`, a pipe will be created and exposed via the pack generator's
> +	 * `out` field. If set to `0`, the pack will be written to the standard
> +	 * output stream. Otherwise, the provided descriptor will be written to
> +	 * and is consumed by the generator.
> +	 */
> +	int pack_fd;
> +

Here I assume that "and is consumed by" refers to "generator writes
into it and then closes it when it is done"?

odb_source_generate_pack() delegate to source->generate_pack(),
which I presume goes to odb_source_files_generate_pack(), which in
turn assigns opts->pack_fd to cp->out and calls start_command(cp) to
run pack-objects.  The file descriptor is closed when the process
finishes.

What happens if the odb->sources[0] does not support .generate_pack?
Should opts->pack_fd be "consumed" here to avoid leaking it, or we
do not have to worry about it because the caller will soon exit
itself?