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

[Christian Couder <christian.couder@gmail.com>]

On Wed, Jul 5, 2023 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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?

In the first case only some large blobs are moved to slower storage
and in the other case all the blobs are moved to slower storage. So
yeah the use cases are very similar. Not sure if and how I can improve
the above wording though.

> > 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.

Ok, in version 3 that I just sent, that paragraph starts with:

"
   This is done by running `git pack-objects` twice in a row. The first
   command is run with `--filter=<filter-spec>`, using the specified
   filter.
"

> 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.

I think it can work if the call to write_filtered_pack() is either
before the call to close_object_store() or after it. It would just use
the tempfiles with their temporary name in the first case and with
their final name in the second case.

write_filtered_pack() is very similar to write_cruft_pack() which is
called before the call to close_object_store(), so I prefer to keep it
before that call too, if possible, for consistency.

> 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?

I would have thought that as finish_pack_objects_cmd() calls
finish_command() the first pack-objects command (which is called
without --stdout) should be completely finished and the packfiles
fully created when write_filtered_pack() (or write_cruft_pack()) is
called, even if the object store is not closed, but you might be
right.

Perhaps this could be dealt with separately though, as I think we
might want to fix write_cruft_pack() first then.