Re: [PATCH 1/2] repack: add --filter-to-loose to write filtered-out objects as loose objects

[Junio C Hamano <gitster@pobox.com>]

"Adithya Chakilam via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adithya Chakilam <achakilam@nvidia.com>
>
> Some external object stores (like key-value) manage and retrieve git
> objects individually. Although repack's --filter-to option can separate
> objects from the main object store, it writes them into packs. Using
> those objects individually then requires an additional unpacking step.
>
> Add --filter-to-loose=<dir> to write filtered-out objects directly as
> loose objects into <dir>, treating it as an object directory. Stream
> the filtered pack into unpack-objects instead of writing an intermediate
> pack file to disk.
>
> The filtered pack is built with --stdin-packs, so this initially handles
> only objects already stored in packs. Loose objects in the source
> repository are left in place.

I haven't decided whether this new feature makes sense or not, so I
won't comment on that aspect.  Take the following review comments
with that in mind (i.e., pretend as if I said "if we assume doing
this makes sense, then..." before every sentence).

> +--filter-to-loose=<dir>::
> +	Like `--filter-to`, but treat `<dir>` as an object directory
> +	and write the filtered out objects there as loose objects
> +	instead of as a pack. Cannot be combined with `--filter-to`
> +	or `--max-pack-size`. Objects already present in `<dir>` are
> +	not rewritten. All objects are written before any existing
> +	pack is removed.

We talk about "already present in ...", so presumably we require the
directory to exist before we run.  Have a test that ensures that
passing a non-existing directory will make the command error out
before doing any real work.

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 21835453c6..140a992f1b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -41,7 +41,7 @@ static const char *const git_repack_usage[] = {
>  	N_("git repack [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m]\n"
>  	   "[--window=<n>] [--depth=<n>] [--threads=<n>] [--keep-pack=<pack-name>]\n"
>  	   "[--write-midx[=<mode>]] [--name-hash-version=<n>] [--path-walk]\n"
> -	   "[--filter=<filter-spec>] [--drop-filtered [--dry-run]]"),
> +	   "[--filter=<filter-spec> [--filter-to-loose=<dir>]] [--drop-filtered [--dry-run]]"),

The above documentation and help text consistently tell us that the
value passed is a directory (do we use the abbreviated <dir> so
commonly?  If not, it would make sense to spell it out, i.e.,
<directory>, for clarity).

> +		OPT_STRING(0, "filter-to-loose", &filter_to_loose, N_("dir"),
> +			   N_("object directory to write filtered out objects into as loose objects")),

Here, too.  

This should use OPT_FILENAME so that a "git repack" started from a
subdirectory with a relative path to the external loose object store
will find the directory at the correct place, I think.  Have a test
to validate the combination to see that such a repack that was given
a relative path to a directory find ...

	mkdir extra-loose-objects &&
	mkdir sub/dir &&
	(
		cd sub/dir &&
		git repack --filter-to-loose=../../extra-loose-objects ...
	)

the directory correct at the right place.

> diff --git a/repack-filtered.c b/repack-filtered.c
> index 869b9fc6e3..51c9dfeb13 100644
> --- a/repack-filtered.c
> +++ b/repack-filtered.c
> @@ -29,10 +50,18 @@ int write_filtered_pack(const struct write_pack_opts *opts,
>  		strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
>  
>  	cmd.in = -1;
> +	if (opts->loose)
> +		cmd.out = unpack.in;
>  
>  	ret = start_command(&cmd);
> -	if (ret)
> +	if (ret) {
> +		if (opts->loose) {
> +			close(unpack.in);

Please double check that this is not closing the same thing twice.
IIRC, a failing start_command() would close all file descriptors
associated with cmd so you may have already closed unpack.in when
start_command() gave us a failure, which presumably closed cmd.out.