Re: [PATCH 5/5] bundle: generate packfiles via the object database

Patrick Steinhardt <[email protected]> writes:

> git-bundle(1) spawns git-pack-objects(1) directly to generate the pack
> data that gets appended to the bundle header. While bundles are not
> part of the wire protocol, they are a transfer mechanism for packs all
> the same, so convert them to use the pack generation interface of the
> object database as well.
>
> This makes the pack generator the single spawn point for all pack
> streams that leave the repository, leaving only local maintenance tasks
> like git-repack(1) with direct knowledge of git-pack-objects(1).

Nice to see that the series aims for completeness.

> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index bfafadc984..de86e092a6 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -69,7 +69,6 @@ static int parse_options_cmd_bundle(int argc,
>  
>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
>  			     struct repository *repo UNUSED) {
> -	struct strvec pack_opts = STRVEC_INIT;
>  	int progress = isatty(STDERR_FILENO);
>  	int version = -1;
>  	struct option options[] = {
> @@ -92,16 +91,9 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
>  			builtin_bundle_create_usage, options, &bundle_file);
>  	/* bundle internals use argv[1] as further parameters */
>  
> -	if (progress)
> -		strvec_push(&pack_opts, "--progress");
> -	else
> -		strvec_push(&pack_opts, "--quiet");
> -	strvec_push(&pack_opts, "--all-progress-implied");
> -
>  	if (!startup_info->have_repository)
>  		die(_("Need a repository to create a bundle."));
> -	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> -	strvec_clear(&pack_opts);
> +	ret = !!create_bundle(the_repository, bundle_file, argc, argv, version, progress);
>  	free(bundle_file);
>  	return ret;
>  }

At this point after we determined startup_info->have_repository is
true, we should be able to rely on "repo", not "the_repository".
But the callchain starting at the create_bundle() function might not
be ready yet.  Let's keep reading.

> diff --git a/bundle.c b/bundle.c
> index b64716f252..09afc465c0 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -325,50 +325,52 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
>  
>  
>  /* Write the pack data to bundle_fd */
> -static int write_pack_data(int bundle_fd, struct rev_info *revs, struct strvec *pack_options)
> +static int write_pack_data(int bundle_fd, struct rev_info *revs, int progress)
>  {
> -	struct child_process pack_objects = CHILD_PROCESS_INIT;
> +	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
> +	struct odb_pack_generator *generator;
> +	int ret = 0;
>  	int i;
>  
> -	strvec_pushl(&pack_objects.args,
> -		     "pack-objects",
> -		     "--stdout", "--thin", "--delta-base-offset",
> -		     NULL);
> -	strvec_pushv(&pack_objects.args, pack_options->v);
> +	opts.thin = 1;
> +	opts.ofs_delta = 1;
> +	if (progress)
> +		opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE;
>  	if (revs->filter.choice)
> -		strvec_pushf(&pack_objects.args, "--filter=%s",
> -			     list_objects_filter_spec(&revs->filter));
> -	pack_objects.in = -1;
> -	pack_objects.out = bundle_fd;
> -	pack_objects.git_cmd = 1;
> +		opts.filter_spec = list_objects_filter_spec(&revs->filter);
>  
>  	/*
> -	 * start_command() will close our descriptor if it's >1. Duplicate it
> -	 * to avoid surprising the caller.
> +	 * The pack generator will consume our descriptor if it's >1.
> +	 * Duplicate it to avoid surprising the caller.
>  	 */
> -	if (pack_objects.out > 1) {
> -		pack_objects.out = dup(pack_objects.out);
> -		if (pack_objects.out < 0) {
> -			error_errno(_("unable to dup bundle descriptor"));
> -			child_process_clear(&pack_objects);
> -			return -1;
> -		}
> +	opts.pack_fd = bundle_fd;
> +	if (opts.pack_fd > 1) {
> +		opts.pack_fd = dup(bundle_fd);
> +		if (opts.pack_fd < 0)
> +			return error_errno(_("unable to dup bundle descriptor"));
>  	}
>  
> -	if (start_command(&pack_objects))
> -		return error(_("Could not spawn pack-objects"));
> -
>  	for (i = 0; i < revs->pending.nr; i++) {
>  		struct object *object = revs->pending.objects[i].item;
>  		if (object->flags & UNINTERESTING)
> -			write_or_die(pack_objects.in, "^", 1);
> -		write_or_die(pack_objects.in, oid_to_hex(&object->oid), the_hash_algo->hexsz);
> -		write_or_die(pack_objects.in, "\n", 1);
> +			oid_array_append(&opts.haves, &object->oid);
> +		else
> +			oid_array_append(&opts.wants, &object->oid);
>  	}
> -	close(pack_objects.in);
> -	if (finish_command(&pack_objects))
> -		return error(_("pack-objects died"));
> -	return 0;
> +
> +	if (odb_generate_pack(the_repository->objects, &generator, &opts)) {
> +		ret = error(_("Could not spawn pack-objects"));
> +		goto out;
> +	}
> +
> +	if (odb_pack_generator_finish(generator)) {
> +		ret = error(_("pack-objects died"));
> +		goto out;
> +	}
> +
> +out:
> +	odb_generate_pack_options_release(&opts);
> +	return ret;
>  }

This function uses the_repository, both directly and through
the_hash_algo macro.  I think we could use revs->repo here.  An
obvious alternative is to give this function a new parameter "struct
repository *repo" but then we would have to worry about what should
happen when it and revs->repo go out of sync.

> @@ -476,7 +478,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
>  }
>  
>  int create_bundle(struct repository *r, const char *path,
> -		  int argc, const char **argv, struct strvec *pack_options, int version)
> +		  int argc, const char **argv, int version, int progress)
>  {
>  	struct lock_file lock = LOCK_INIT;
>  	int bundle_fd = -1;
> @@ -584,7 +586,7 @@ int create_bundle(struct repository *r, const char *path,
>  	}
>  
>  	/* write pack */
> -	if (write_pack_data(bundle_fd, &revs_copy, pack_options)) {
> +	if (write_pack_data(bundle_fd, &revs_copy, progress)) {
>  		ret = -1;
>  		goto out;
>  	}
> diff --git a/bundle.h b/bundle.h
> index d664b2f2d6..471da23d1b 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -27,8 +27,7 @@ int read_bundle_header(const char *path, struct bundle_header *header);
>  int read_bundle_header_fd(int fd, struct bundle_header *header,
>  			  const char *report_path);
>  int create_bundle(struct repository *r, const char *path,
> -		  int argc, const char **argv, struct strvec *pack_options,
> -		  int version);
> +		  int argc, const char **argv, int version, int progress);
>  
>  enum verify_bundle_flags {
>  	VERIFY_BUNDLE_VERBOSE = (1 << 0),