Re: [PATCH v2 5/8] repack: add `--filter=<filter-spec>` option

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

Christian Couder <christian.couder@gmail.com> writes:

> This could be useful if, for example, some large blobs take a lot of
> precious space on fast storage while they are rarely accessed. It could
> make sense to move them into a separate cheaper, though slower, storage.
>
> In other use cases it might make sense to put all the blobs into
> separate storage.

Minor nit.  Aren't the above two the same use case?

> This is done by running two `git pack-objects` commands. The first one
> is run with `--filter=<filter-spec>`, using the specified filter. It
> packs objects while omitting the objects specified by the filter.
> Then another `git pack-objects` command is launched using
> `--stdin-packs`. We pass it all the previously existing packs into its
> stdin, so that it will pack all the objects in the previously existing
> packs. But we also pass into its stdin, the pack created by the previous
> `git pack-objects --filter=<filter-spec>` command as well as the kept
> packs, all prefixed with '^', so that the objects in these packs will be
> omitted from the resulting pack.

When I started reading the paragraph, the first question that came
to my mind was if these two pack-objects processes can and should be
run in parallel, which is answered in the part near the end of the
paragraph.  It may be a good idea to start the paragraph with "by
running `git pack-objects` command twice in a row" or something to
make it clear that one should (and cannot be) run before the other
completes.

In fact, isn't the call site of write_filtered_pack() in this patch
a bit too early?  The subprocess that runs with "--stdin-packs" is
started and told about the names of the pack we are going to create,
and it does not start processing until it reads everything (i.e. we
run fclose(in) in the write_filtered_pack() function), but the loop
over "names" string list in the caller that moves the tempfiles to
their final filenames comes after the call to close_object_store()
we see in the post context of the call to write_filtered_pack() that
is new in this patch.

The "--stdin-packs" one is told to exclude objects that appear in
these packs, so if the main process is a bit slow to finalize the
packfiles it created (and told the "--stdin-packs" process about),
it will not lead to repository corruption---just some objects are
included in the packfiles "--stdin-packs" one creates even though
they do not have to.  So it does not sound like a huge problem to
me, but still it somehow looks wrong.  Am I misreading the code?

Thanks.