patch, 9 partspack-objects: allow `--filter` without `--stdout`

# Intro

Last year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>` and later I sent 4 versions of a patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the filtered out objects altogether which was considered very dangerous even though we implemented different safety checks in some of the latter series.

In some discussions, it was mentioned that such a feature, or a similar feature in `git gc`, or in a new standalone command (perhaps called `git prune-filtered`), should put the filtered out objects into a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving blobs from inactive repos onto cheaper storage, or moving large blobs onto cheaper storage. This lead us to rethink at repacking using a filter, but moving the filtered out objects into a separate packfile instead of deleting them.

So here is a new patch series doing that while implementing the `--filter=<filter-spec>` option in `git repack`.

This could be useful for the following purposes:

  - As a way for servers to save storage costs by for example moving
    large blobs, or blobs in inactive repos, to separate storage
    (while still making them accessible using for example the
    alternates mechanism).

  - As a way to use partial clone on a Git server to offload large
    blobs to, for example, an http server, while using multiple
    promisor remotes (to be able to access everything) on the client
    side. (In this case the packfile that contains the filtered out
    object can be manualy removed after checking that all the objects
    it contains are available through the promisor remote.)

  - As a way for clients to reclaim some space when they cloned with a
    filter to save disk space but then fetched a lot of unwanted
    objects (for example when checking out old branches) and now want
    to remove these unwanted objects. (In this case they can first
    move the packfile that contains filtered out objects to a separate
    directory or storage, then check that everything works well, and
    then manually remove the packfile after some time.)

As the features and the code are quite different from those in the previous series, I decided to start a new series instead of continuing a previous one.

# Commit overview

* 1/9 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as the first patch in the previous series. To
  be able to later repack with a filter we need `git pack-objects` to
  write packfiles when it's filtering instead of just writing the pack
  without the filtered out objects to stdout.

* 2/9 pack-objects: add `--print-filtered` to print omitted objects

  We need a way to know the objects that are filtered out of the
  packfile generated by `git pack-objects --filter=<filter-spec>`. The
  simplest way is to teach pack-objects to print their oids to stdout.

* 3/9 t/helper: add 'find-pack' test-tool

  For testing `git repack --filter=...` that we are going to
  implement, it's useful to have a test helper that can tell which
  packfiles contain a specific object.

* - 4/9 repack: refactor piping an oid to a command
  - 5/9 repack: refactor finishing pack-objects command

  These are small refactorings so that `git repack --filter=...` will
  be able to reuse useful existing functions.

* 6/9 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. It uses one
  `git pack-objects` process with both the `--filter` and the
  `--print-filtered` options. From this process it reads the oids of
  the filtered out objects and pass them to a separate `git
  pack-objects` process which will pack these objects into a separate
  packfile.

* 7/9 gc: add `gc.repackFilter` config option

  This is a gc config option so that `git gc` can also repack using a
  filter and put the filtered out objects into a separate packfile.

* 8/9 repack: implement `--filter-to` for storing filtered out objects

  For some use cases, it's interesting to create the packfile that
  contains the filtered out objects into a separate location. This is
  similar to the --expire-to option for cruft packfiles.

* 9/9 gc: add `gc.repackFilterTo` config option

  This allows specifying the location of the packfile that contains
  the filtered out objects when using `gc.repackFilter`.

Christian Couder (9):
  pack-objects: allow `--filter` without `--stdout`
  pack-objects: add `--print-filtered` to print omitted objects
  t/helper: add 'find-pack' test-tool
  repack: refactor piping an oid to a command
  repack: refactor finishing pack-objects command
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  11 ++
 Documentation/git-pack-objects.txt     |  14 ++-
 Documentation/git-repack.txt           |  11 ++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |  55 ++++++--
 builtin/repack.c                       | 166 ++++++++++++++++++-------
 t/helper/test-find-pack.c              |  35 ++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t5317-pack-objects-filter-objects.sh |  27 ++++
 t/t6500-gc.sh                          |  23 ++++
 t/t7700-repack.sh                      |  43 +++++++
 13 files changed, 345 insertions(+), 53 deletions(-)
 create mode 100644 t/helper/test-find-pack.c

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught `git pack-objects` to use `--filter`, but required the use of `--stdout` since a partial clone mechanism was not yet in place to handle missing objects. Since then, changes like 9e27beaa23 (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25) and others added support to dynamically fetch objects that were missing.

Even without a promisor remote, filtering out objects can also be useful if we can put the filtered out objects in a separate pack, and in this case it also makes sense for pack-objects to write the packfile directly to an actual file rather than on stdout.

Remove the `--stdout` requirement when using `--filter`, so that in a follow-up commit, repack can pass `--filter` to pack-objects to omit certain objects from the resulting packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt | 4 ++--
 builtin/pack-objects.c             | 8 ++------
 2 files changed, 4 insertions(+), 8 deletions(-)

On Wed, Jun 14, 2023 at 09:25:33PM +0200, Christian Couder wrote:

Makes sense.

Is there any situation in which using --stdout with --filter would be a potential foot-gun? I am not as familiar with the partial clone mechanism as others CC'd, so I have no idea one way or the other.

If it is unsafe in certain situations (or, at the very least, could produce surprising behavior), it may be worthwhile to only allow `--filter=<filter> --stdout` with some kind of `--filter-to-stdout-is-ok` flag to indicate that the caller knows what they are doing.

Presumably 'git repack --filter' would pass such a flag later on in the series.

Should there be a trivial test here? I'm thinking something on the order of writing a filtered pack to stdout and redirecting it to a file, moving it into place, and then indexing the pack to make sure that we got the expected set of objects.

Thanks, Taylor

On Wed, Jun 21, 2023 at 12:49 PM Taylor Blau <me@ttaylorr.com> wrote:

This patch allows `--filter` without `--stdout`, so `--stdout` with `--filter` was already allowed before this patch and is still allowed after it.

Besides, using `--stdout` or not using it is just a convenience. It doesn't change much what users can do with `git pack-objects`.

`git pack-objects` with `--filter` can be unsafe in some cases if users do stupid things with packfiles afterwards, but allowing it to run without --stdout doesn't significantly change that. In any case by itself it doesn't delete any data. It just creates a pack containing fewer objects.

Anyway I added a small test that checks that --filter can be used without --stdout.

When using the `--filter=<filter-spec>` option, `git pack-objects` will omit some objects from the resulting packfile(s) it produces. It could be useful to know about these omitted objects though.

For example, we might want to write these objects into a separate packfile by piping them into another `git pack-object` process. Or we might want to check if these objects are available from a promisor remote.

Anyway, this patch implements a simple way to let us know about these objects by simply printing their oid, one per line, on stdout when the new `--print-filtered` flag is passed.

As `--print-filtered` doesn't make sense without `--filter`, it is disallowed to use the former without the latter.

Using `--stdout` is likely to make the `--print-filtered` output difficult to find or parse, so we also disallow using these two options together.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt     | 10 ++++++
 builtin/pack-objects.c                 | 47 ++++++++++++++++++++++++--
 t/t5317-pack-objects-filter-objects.sh | 27 +++++++++++++++
 3 files changed, 81 insertions(+), 3 deletions(-)

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

Makes sense. It is a bit sad that we have to accumulate everything until the end at which time we have to dump the accumulated in bulk, but that is a current limitation of list-objects-filter API and not within the scope of this change. We may in the longer term want to see if we can make the collection of filtered-out objects streamable by replacing the .omits object array with a callback function, or do something along that line.

On Thu, Jun 15, 2023 at 03:50:17PM -0700, Junio C Hamano wrote:

Hmm. I think it is possible to use something like `git pack-objects`'s `--stdin-packs` mode to accomplish this without needing to keep track of the set of discarded objects (i.e. those which don't match the filter).

IIUC, the set of objects which don't match the filter is the same as the set of all objects in packs beforehand, differenced with the set of objects that shows up in the pack containing objects which *do* match the filter.

If you mark all of the "before" packs with `-` in the input to `--stdin-packs`, and then pass along the pack containing the filtered set without `-` (to indicate that the resulting pack should not contain any objects which appear in that pack), I think you would end up with the set of non-matching objects.

Thanks, Taylor

On Wed, Jun 21, 2023 at 12:52 PM Taylor Blau <me@ttaylorr.com> wrote:

I agree that it can be done like this, but I am not sure it's very efficient to do it like this. When we create the pack with filtered out objects, we know the set of objects we filtered out, so it doesn't seem efficient to make `git pack-objects --stdin-packs` read more packfiles or their indexes than necessary and compute that set of objects again.

Now I haven't checked if there is a real performance difference for large packfiles, and perhaps `git pack-objects --stdin-packs` is very efficient. But I hope that going the way I implemented it and perhaps using some optimization ideas that Junio suggested above, will make it easier to improve performance in the future.

On Wed, Jun 21, 2023 at 01:11:38PM +0200, Christian Couder wrote:

Discovering that an object appears in one of the packs whose objects we're excluding from the resulting pack is extremely cheap, since we have a cache of just those packs (see the call to `has_object_kept_pack()` in `want_found_object()`).

I'd think that in small cases the performance is probably worse, though the working set would be small enough for the differences to not matter that much in absolute terms.

In cases with many objects, storing all of their OIDs in memory would be a pain.

Thanks, Taylor

In a following commit, we will make it possible to separate objects in different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a new test-tool that can display which packfile(s) a given object is in.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 4 files changed, 38 insertions(+)
 create mode 100644 t/helper/test-find-pack.c

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

This tool would be serviceable if we only are interested in checking just a few objects, but if we were to check many objects, I have to wonder if it would be more efficient to use show-index to dump the list of objects per pack, which should be sorted by object name, so it should be trivial to run "comm" with the list of objects you want to check.

Or if you only are checking about a dozen or so, taking one or more arguments from the command line and looping over them may also be OK. The output format of course may have to be changed, if we were to go that route, though.

It really depends on the granularity at which this test helper wants to work at, I think.

Anyway...

On Fri, Jun 16, 2023 at 1:32 AM Junio C Hamano <gitster@pobox.com> wrote:

I agree that this new tool is for checking just a few objects.

Yeah, in the previous commit implementing --print-filtered, we check that all the objects that should be printed to stdout are indeed printed. So later when git repack --filter=... is implemented by using git pack-objects --print-filtered and piping the printed objects to a regular git pack-objects command, I don't think it's necessary to test a lot of objects, as hopefully both pack-objects command are supposed to work well at that point.

On Thu, Jun 15, 2023 at 04:32:35PM -0700, Junio C Hamano wrote:

I was going to say the exact same thing. Even if we were checking many objects, can't we dump the output of show-index to a file, and then grep it repeatedly? Presumably these tests are working on repositories with tens of objects, so I doubt it matters much either way.

If we do end up taking this approach to use the test-helper instead, the implementation seems reasonable.

Thanks, Taylor

Create a new finish_pack_objects_cmd() to refactor duplicated code that handles reading the packfile names from the output of a `git pack-objects` command and putting it into a string_list, as well as calling finish_command().

While at it, beautify a code comment a bit in the new function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 78 ++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

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

Computing "is it local?" based on the value of "destination" feels it belongs to the caller (one of the callers that do need the computation), not to this function, especially given that the full value of "destination" is not even used in any other way in this function. And the "is_local?" bit can instead be passesd into this helper function as a parameter.

I wondered what "beautify" was about---the original looks OK to me already, and while I do not mind to see a full sentence spelled in a more gramatically correct way like in the postimage, I do not think the change was worth wasting reviewer's time wondering if there are other improvements by pointing it out in the proposed log message.

Thanks.

On Thu, Jun 15, 2023 at 05:13:14PM -0700, Junio C Hamano wrote:

Hah. I had the same suggestion down-thread, but hadn't read your reply yet. There are either a couple of changes we could make to skip_prefix(), or foist the responsibility onto the caller (I tend to prefer the latter).

Thanks, Taylor

On Wed, Jun 21, 2023 at 1:06 PM Taylor Blau <me@ttaylorr.com> wrote:

Ok, I will make callers compute the "is_local?" bit and pass it to the helper function as a parameter.

Maybe stick the declaration above (and consider making it static), so that this can become

    if (destination)
      local = skip_prefix(destination, packdir, &scratch);

without the braces. Although it might be nice to either put this behind a "has_prefix()" convenience function (which itself owns the scratch buffer and hides that detail from the caller), or to make skip_prefix skip trying to assign into the buffer if given NULL.

Consider moving the declaration of item into this block, since it's not used elsewhere throughout the body of the loop.

Alternatively, if you want to leave it up there, it might be easier to adjust the if condition to be more in line with the comment, like:

    if (!local)
      continue;

OK, we don't even bother calling skip_prefix if given a NULL destination. I wonder if it might make sense to force the caller to compute "is this local?" ahead of time. This one would always pass "1" trivially, and the cruft case would depend on whether or not we are handling the `--expire-to` option.

Thanks, Taylor

Create a new write_oid_hex_cmd() function to send an oid to the standard input of a running command. This new function will be used in a following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/repack.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

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

I am not sure why we would want to conflate the "if we haven't started the command, auto-start it upon our first attempt to write" in these low-level "I am designed to do one thing, which is to feed the object name to the process, and do it well" function.

The caller in the original shares the same issue, so we could say that this patch is not creating a new problem, but this somehow feels it is mak ng the existing problem even worse.

And I think the error handling here shows why the API feels wrong. When auto-start fails, we have a message, but when write fails, there is no custom message---it makes as if write_oid_hex_cmd() is primarily about starting, which is so important relative to its other functionalities and deserves a custom error message, but that is not the message you want to be conveying.

I would have expected that the "refactor" at least would reduce the number of system calls by combining these two writes into one using an on-stack local variable char buf[GIT_MAX_HEZSZ+1] or something.

On Thu, Jun 15, 2023 at 04:46:43PM -0700, Junio C Hamano wrote:

I agree, the implementation of `write_oid_hex_cmd()` seems too magical to me.

Perhaps there was some awkwardness with using the pre-image w.r.t some later change? Let's see...

Thanks, Taylor

On Fri, Jun 16, 2023 at 1:46 AM Junio C Hamano <gitster@pobox.com> wrote:

Ok. I think I will rework this patch from version 2 of this series to remove that code. It will perhaps look like there is a bit of duplicated code, but I don't think it will be too bad.

Right.

Ok, I will change it to reduce the number of system calls.

After cloning with --filter=<filter-spec>, for example to avoid getting unneeded large files on a user machine, it's possible that some of these large files still get fetched for some reasons (like checking out old branches) over time.

In this case the repo size could grow too much for no good reason and a way to filter out some objects would be useful to remove the unneeded large files.

Deleting objects right away could corrupt a repo though, so it might be better to put those objects into a separate packfile instead of deleting them. The separate pack could then be removed after checking that all the objects in it are still available on a promisor remote it can access.

Also splitting a packfile into 2 packs depending on a filter could be useful in other usecases. For example some large blobs might take a lot of precious space on fast storage while they are rarely accessed, and it could make sense to move them in a separate cheaper, though slower, storage.

This commit implements a new `--filter=<filter-spec>` option in `git repack` that moves filtered out objects into a separate pack.

This is done by reading filtered out objects from `git pack-objects`'s output and piping them into a separate `git pack-objects` process that will put them into a separate packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt |  5 +++
 builtin/repack.c             | 75 ++++++++++++++++++++++++++++++++++--
 t/t7700-repack.sh            | 16 ++++++++
 3 files changed, 93 insertions(+), 3 deletions(-)

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

Makes sense.

If we repack without these objects, when the repository has a promisor remote, we should be able to rely on that remote to supply them on demand, once we need them again, no?

Hmph, could you elaborate why it is the case? Isn't it the whole point to have promisor remote and use a lazy clone with the --filter option, so that objects that _ought_ to exist from connectivity's point of view _are_ allowed to be missing because the promisor promises to make them available on-demand?

	Side note: I think I know the answer. While trying to remove
	UNNEEDED large files, doing so may discard NEEDED large
	files when done carelessly (e.g. the file may have been
	created locally and haven't been pushed back). But (1) if
	that is the problem, perhaps we should be more careful in
	the first place? (2) if it inherently is impossible to tell
	which ones are unneeded reliably, the reason why it is
	impossible, and the reason why "try sifting into two bins,
	one that we _think_ are unneeded and another for the rest,
	and verify what we _thought_ are unneeded are all available
	from the promisor remote" is the best we can do, must be
	described, I think.

Surely, sifting the objects into two bins (i.e. those that we wouldn't have received if we cloned from the promisor remote just now, which are prunable, and those that we cannot lose because the promisor remote would not have them, e.g. we created them and have not pushed them to the remote yet) without removing anything would be safe, but if the result of such sifting must be verified, doesn't it indicate that the sifting step was buggy or misdesigned? It does not sound like a very good justification to save them in a separate packfile. It does smell somewhat similar to the cruft packs but not really (the choice over there is between exploding to loose and keeping in a pack, and never involves loss of objects).

This one, outside the context of partial clone client, does make tons of sense.

I guess what I suspect is that this option, while it would be very useful for the "in other usecases" scenario above, may not become all that useful in the "our lazy clone got bloated and we want to trim objects we know we can retrieve from the promisor remote again if necessary" scenario, until the repack machinery learns to use an extra piece of information (namely "these are objects that we can fetch from the promisor remote") at the same time.

So, for example, you may say "no blobs" in the filter, and while packing the local repository with the filter, resulting in a pack that exclude all blobs, we will learn what blob objects we did not pack into that packfile. We can pack them into a separate one, and most of the blobs are what we could retrieve again from the promisor remote, but some of the blobs are what we locally created ourselves and haven't pushed back to the promisor remote yet. Now what? My earlier suspicion that this mechanism may not be all that useful for the "slim bloated lazy clone" comes from that I cannot think of a good answer to this "Now what?" question---my naive solution would involve enumerating the objects in that "separate packfile" that is a mixture of precious ones and expendable ones, and then learning which ones are precious, and creating a new pack that is a subset of that "separate packfile" with only the precious ones. But if I do so, I do not think we need this new mechanism that seems to go only the half-way.

On Thu, Jun 15, 2023 at 05:43:27PM -0700, Junio C Hamano wrote:

I think in theory, yes, but this patch series (at least up to this point) does not seem to implement that functionality by marking the relevant remote(s) as promisors, if they weren't already.

Indeed. `pack-objects`'s `--stdin-packs` and `--cruft` work similarly, and I believe that we could use `--stdin-packs` here instead of having to store the list of objects which don't meet the filter's spec. IOW, I think that this similarity is no coincidence...

Thanks, Taylor

On Wed, Jun 21, 2023 at 1:20 PM Taylor Blau <me@ttaylorr.com> wrote:

Yeah, it's not part of this patch series to implement all the features that could be useful in the case of promisor remotes. This patch series only hopes to implement a `repack --filter=...` option that can help in a number of different use cases. I'm open to opinions about whether or not the doc and commit messages should talk, and how much, about use cases related to promisor remotes.

Yeah, I agree that we could use `--stdin-packs` to implement `repack --filter=...`. I am just not sure it's the best path forward performance wise in the long run. So others' opinions are welcome about that.

Also, as Junio said, this patch series is not responsible for the fact that traverse_commit_list_filtered() stores oids into an oidset instead of using a callback function. Fixing this would likely avoid accumulating oids in memory. And creating a packfile by sending oids into pack-objects is something that is already done by repack_promisor_objects(). So even if `--filter=...` is not reusing `--stdin-packs`, it is still reusing a lot of existing mechanisms.

Thanks, Christian.

On Wed, Jun 21, 2023 at 05:04:48PM +0200, Christian Couder wrote:

I think it would almost certainly have comparable performance in most cases, and significantly better performance in large repositories. IIUC, the current system has to remember the OID of every object which did not pass the filter, and then construct a pack containing just those objects.

It would be nice from a memory-savings perspective to not have to remember these OIDs. But it also just seems error prone to me to do so: what if we lose an OID, or reorder the list?

I dunno. I feel pretty strongly that implementing this in terms of:

  - Write a filtered pack.
  - Construct the list of existing packs (marked with '-') and the
    filtered pack.
  - Pass that as input to `git pack-objects --stdin-packs`
  - If '-d' given, delete any existing pack(s).

Thanks, Taylor

On Fri, Jun 16, 2023 at 2:43 AM Junio C Hamano <gitster@pobox.com> wrote:

Yeah, sure.

Yeah, right.

Yeah, but earlier when we implemented `repack --filter=...` that was removing objects, saying that one should be very careful in the docs and implementing safeguards didn't seem to be safe enough for reviewers. Reviewers said that the feature would anyway provide a too easy way for users to shoot their own foot.

You mean described in the `repack --filter=` doc? Yeah, I can describe this use case in the doc, but see below.

It might indicate that we prefer to be safe, do things in different steps and not provide an easy way for users to shoot their own foot. For example it seems pretty safe to do things like this:

  1) put all the objects we think should be on the promisor remote in
a separate packfile
  2) start checking that each object in that packfile is available on
the promisor remote
  3) if an object in that packfile isn't on the promisor remote, try
to send it there
  4) if we couldn't send the object, error out
  5) if we haven't errored out after checking all the objects in the
packfile, it means all these objects are now available from the
promisor remote and we can safely delete the packfile

The above steps can be done while new objects are created on the repo, or fetched, or pushed into the repo. And, at least for now, it would be done by a custom script, so users writing and installing it should know what they are doing and would hopefully not complain that we provided an easy way for them to shoot their foot.

If we don't even document the above in the --filter=... doc, it makes it even less likely that they will do this and that their script might be wrong. So even if I could document it in version 2, I am not sure I should.

If we are still worried about possible loss of objects, I am Ok with not talking at all about use cases involving possible loss of objects.

Ok, so perhaps it is enough to justify this feature and patch series. And I can just avoid talking about other use cases at all?

Yeah, perhaps we should wait for a command or a repack option or some helper scripts to be able to perform steps 2) to 4) or 2) to 5) above before talking about use cases involving a promisor remote.

On the other hand, it's possible to imagine other steps than the steps
2) to 4) described above. For example, if we want to repack on a
server where new large blobs can hardly be created and where there is
a receive hook that automatically sends all the large blobs to a
promisor remote as soon as they are received, we might not need steps
3) and 4) to send objects to the promisor remote. Just checking that
they are on the promisor remote might be enough.

Also even if we think we should have features covering all the 5 steps, should we cover all the ways blobs could be sent to the promisor remote as part of step 3)? Some people or server platforms might want to use git for that purpose, but others might prefer for example FTP or plain HTTP(S) so that a transfer can be restarted if it fails.

So should we really wait until we have all possible such use cases covered by some features or scripts, or not? When does it become Ok to talk about this? And then how much is it Ok to talk about this?

I hope the above 2) to 5) steps and related explanations are a good answer to the "Now what?" question.

Thanks, Christian.

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

I may be missing something, but to me, the above sound more like a tail wagging the dog.

Instead of saying "while repacking, we'll create the new pack with the objects that we suspect that we cannot re-fetch from the promisor (allowing false positives for safety), and store the rest in a backup pack (that can immediately be discarded)", the above says "while repacking, we'll create the new pack with objects that match the filter, and store the rest to another pack". But because the object selection criteria used in the latter is not something with practical/useful meaning, in other words, it does not exactly match what we want, we fill the gaps between what we want (i.e. sift the objects into "refetchable" and "other" bins) and what we happened to have implemented (i.e. sift the objects into "match filter" and "other" bints) by sending the objects that we _should_ have included in the new pack (i.e. "not refetchable") to the promisor to make them refetchable.

I do not know what to think about that. I do not think there is even a way to guarantee that the push done for 3) will always be taken and still leave the resulting promisor usable (e.g. we can make them connected by coming up with a random new ref to point these "we are sending these only because we failed to include them in the set of objects we should consider local" objects, but then how would we avoid bloating the refs at the promisor remote side (which now has become a "dumping ground", rather than holding the objects needed for histories that project participants care about).

As an argument to salvage this series as (one of the possible ingredients to) a solution to "slim down a bloated lazy clone" problem, it sounds a bit weak.

Thanks.

On Wed, Jun 21, 2023 at 6:53 PM Junio C Hamano <gitster@pobox.com> wrote:

This might be a good idea, but what if users prefer to send to a promisor remote the objects that should be on that promisor remote as soon as possible, instead of keeping them on the local machine where they take up possibly valuable space for no good reason?

My point is that there are a lot of different strategies that people operating with a promisor remote could adopt, so it's better to iteratively give them building blocks that can help them instead of trying to find and implement right away the best solution for a special use case or for every use case.

"What we want" depends on the strategy chosen to manage objects on promisor remotes and I am not sure that the strategy you mention is always better than the example strategy I talked about. For some users it might be better for others it might not.

There are some configurations where users never want to delete any git object. In those cases it doesn't matter if the promisor remote is a "dumping ground". Users might just want a promisor remote to keep all the large files that have ever been pushed into the repo, to save more precious space on the machine hosting the regular repo.

There are configurations where users can have garanties that the push done for 3) will work with a very high probability so that the example strategy I talked about can work reliably enough.

The example strategy I talked about is just one example where having repack --filter work like it does in this patch series can be useful and safe. I don't pretend that it is always the best strategy and that some users might not prefer another better strategy for them. If that's the case perhaps they can implement another different option that just checks that filtered out objects are indeed available from a promisor remote and then just omit these objects from the resulting pack. In fact I would have nothing against such an option, and I might even implement it myself one day (no promise though).

Right now they have nearly no helpful command (except perhaps using pack-objects directly), so even if this is not the best possible help in all use cases, I am just saying that this can be useful in _some_ cases.

I don't quite agree, but anyway I will use this argument less in version 2 of this patch series, and I will talk more about the argument that `repack --filter=...` allows users to put packfiles containing some kinds of objects (like large blobs or all the blobs) on cheaper disks (when not using a promisor remote).

Thanks.

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

When one says "everything" in these sentences, I doubt one necessarily means "everything". A topic one works on will have iterations that is never pushed out, a topic one started may not even get to the state that is pushable to the central server. But the objects that need to support such a topic (and its historical versions in its reflog) would need to be retained until they are expired.

Certainly, by pushing even such objects, you can say "here is a pack with filter=blob:none, and because I sent every blob every time I create locally to the promisor, I can always refetch what is not in them by definition".

But is that a good use of everybody's resources? The key phrase in what I said was "... and still leve the resulting promisor usable".

The promisor remote is in the unfortunate and unenviable position that it cannot garbage collect anything because there may be somebody who is still depending on such an object nobody planned to use, but there is no mechanism to let it find out which ones are in active use (or if you added some recently that I am forgetting, it would change the equation---please remind me if that is the case).

So I would imagine that it would be fairly high in the priority list of server operators and project leads to make sure their promisor remotes do not become a true "dumping ground".

For "trim a bloated lazy clone" problem, I suspect that you would need to know what is currently re-fetchable from the promisor and drop those objects from your local repository, and the computation of what is currently re-fetchable would certainly involve the filter specification you had with the promisor. The remote-tracking branches you have for the promisor would serve as the other source of input to perform the computation.

For "partition local and complete object store" problem, using filter specification to sift the objects into two bins (those that match and the rest), as the code changes in the series implements, may be a useful mechansim. I briefly had to wonder if partitioning into two (and not arbitrary number N) bins is sufficient, but did not think of a scenario where we would benefit from 3 bins more than having 2 bins offhand.

Thanks.

On Wed, Jun 14, 2023 at 09:25:38PM +0200, Christian Couder wrote:

Having read through the implementation in the repack builtin, I am almost certain that my suggestion earlier in the thread to implement this in terms of 'git pack-objects --filter' and 'git pack-objects --stdin-packs' would work.

This documentation leaves me with a handful of questions about how it interacts with other options. Here are some:

  - What happens when you pass it with "-d"? Does it delete objects that
    didn't match the filter? Leave them alone? If the latter, should
    this combination be declared invalid instead of silently ignoring
    the user's request to delete redundant packs?

  - What happens with --max-pack-size? Does the filtered pack get split
    into multiple packs (as I think we would expect from such a
    combination)?

  - What about with `--cruft`? Does it split the cruft pack into two
    based on whether or not the unreachable object(s) matched or didn't
    match the filter?

  - What happens when passed with "--geometric"? I don't think there is
    a sensible interpretation (at least, I can't think of what it would
    mean to do "--filter=<spec> --geometric=<factor>" off the top of my
    head).

  - What about with "--write-bitmap-index"? Do we write one bitmap
    index? Two? If the latter, do we combine the packs into a MIDX
    before writing the bitmap? Should we?

I think it may be worth spelling out answers to some of these questions in the documentation, and codifying those answers in the form of tests.

This makes me wonder whether or not this option should belong in repack at all, or whether there should be some new special-purpose builtin that is designed to split existing pack(s) based on whether or not they meet some filter criteria.

Thanks, Taylor

On Wed, Jun 21, 2023 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:

Yeah, it works, thanks. That's what is used in the version 2 I just sent.

I have improved this doc in version 2. It now says:

--filter=<filter-spec>::
       Remove objects matching the filter specification from the
       resulting packfile and put them into a separate packfile. Note
       that objects used in the working directory are not filtered
       out. So for the split to fully work, it's best to perform it
       in a bare repo and to use the `-a` and `-d` options along with
       this option.  See linkgit:git-rev-list[1] for valid
       `<filter-spec>` forms.

For comparison the doc about --cruft is:

--cruft::
       Same as `-a`, unless `-d` is used. Then any unreachable objects
       are packed into a separate cruft pack. Unreachable objects can
       be pruned using the normal expiry rules with the next `git gc`
       invocation (see linkgit:git-gc[1]). Incompatible with `-k`.

With or without "-d", no object is deleted, objects are just put into 2 different packfiles depending on whether or not they match the filter.

"-d" is indeed about removing redundant packs. If you want to split the objects into different packs according to a filter, then you probably don't want to keep packs that contain both kinds of objects that you want to separate into different packs, so the doc now recommends using "-d" (along with "-a") when using "--filter". That's also what the tests are using.

I am not sure it's worth erroring out when "-d", or "-a", is not used. Perhaps there are some use cases where it's interesting to keep old packs, or, in case of not using "-a", to split only loose objects into separate packs, but not objects from existing packs.

Yes, in version 2 I have added a test (in the commit introducing --filter-to) that checks that objects that are filtered out and that are larger than --max-pack-size get split into their own packs.

I am not sure as I don't know well how cruft works. I would need to check the code and/or test it.

Not sure either, as I also don't know well how --geometric works.

In the tests, repack --filter is used with "-c repack.writebitmaps=false" as otherwise the bitmap writing code tends to complain that a pack is not complete or something like that. I think it would make sense to have options that would make the bitmap writing code write a regular bitmap index if all the objects are in a single pack, but a MIDX if they are in multiple packs. I don't know what --cruft or --max-pack-size are doing about this, but I think such a feature could be useful in case those options are used.

In the meantime I would be Ok with disallowing using both --write-bitmap-index and --filter. The issue is that writing a bitmap index is the default behavior, even without --write-bitmap-index, so it might be a bit more complex, but I plan to do something about it in version 3.

In version 2, the doc has been improved and now it seems comparable to the --cruft doc. About the tests, I added one in version 2 related to --max-pack-size, and I am open to adding a few others, but I don't think it's a good idea to add a lot of them. It just doesn't scale when commands have more than a few options.

The new code is reusing existing features (like --stdin-packs) that are already used by other code (like --cruft code). It is not adding new intricate mechanisms, nor a lot of new code. It is not changing code used by other features except for a few clean refactorings. This should give a good indication that it will work well with other features.

--cruft is also splitting existing packs based on some criteria. So if we go this way, perhaps --cruft code should be moved first to this new special purpose builtin?

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

It would be nice if this new different pack could be created in a different directory than the regular pack. This would make it possible to move large blobs into a pack on a different kind of storage, for example cheaper storage. Even in a different directory this pack can be accessible if, for example, the Git alternates mechanism is used to point to it.

If users want to remove a pack that contains filtered out objects after checking that they are all already on a promisor remote, creating the pack in a different directory makes it easier to do so.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt |  6 ++++++
 builtin/repack.c             | 17 ++++++++++++-----
 t/t7700-repack.sh            | 27 +++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 5 deletions(-)

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

Makes sense, I guess, for "in other usecases" scenario. I am not sure how this would be useful for the originally stated goal of unbloating a bloated repository with promisor remote(s), though.

Care to elaborate? I do not see how a separate directory would make it easier. After separating the potential cruft into a packfile, you'd walk its .idx and see if there are any objects that are not available (yet) at the promisor remotes to check if it is safe to remove. That can be done regardless of the location of the packfile that is suspected to be now removable.

On Wed, Jun 14, 2023 at 09:25:40PM +0200, Christian Couder wrote:

Here you say "only useful with --filter", but...

Would you want an "} else if (filter_to)" here to die and show the usage message, since --filter-to needs --filter? Or maybe it should imply --filter-to.

Thanks, Taylor

On Wed, Jun 21, 2023 at 1:49 PM Taylor Blau <me@ttaylorr.com> wrote:

In the doc for --expire-to=<dir> there is "Only useful with `--cruft -d`" and I don't think there is a check to see if --cruft and -d have been passed when --expire-to is passed. So I am not sure if it's better to be consistent with --expire-to or not.

On Wed, Jun 21, 2023 at 02:08:38PM +0200, Christian Couder wrote:

TBH, I don't think that my decision at the time to silently accept --expire-to without --cruft was the right one. It should at least require --cruft, or imply it. It doesn't make a ton of sense to use without -d, but doing so is OK, so I wouldn't consider that a failing condition.

In other words, I would be fine with something like:

--- 8< ---

Taylor Blau <me@ttaylorr.com> writes:

It does.  Thanks.

On Wed, Jun 21, 2023 at 1:49 PM Taylor Blau <me@ttaylorr.com> wrote:

I have added such a check in the version 2 I just sent. We now die() if --filter-to is used without --filter.

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

Users might want to perform such a cleanup regularly at the same time as they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a new gc.repackFilter config option.

Now when `git gc` will perform a repack with a <filter-spec> configured through this option and not empty, the repack process will be passed a corresponding `--filter=<filter-spec>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  5 +++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 12 ++++++++++++
 3 files changed, 23 insertions(+)

A previous commit implemented the `gc.repackFilter` config option to specify a filter that should be used by `git gc` when performing repacks.

Another previous commit has implemented `git repack --filter-to=<dir>` to specify the location of the packfile containing filtered out objects when using a filter.

Let's implement the `gc.repackFilterTo` config option to specify that location in the config when `gc.repackFilter` is used.

Now when `git gc` will perform a repack with a <dir> configured through this option and not empty, the repack process will be passed a corresponding `--filter-to=<dir>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  6 ++++++
 builtin/gc.c                |  4 ++++
 t/t6500-gc.sh               | 13 ++++++++++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

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

That's an obvious follow-up on the previous step.

Thanks.

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

Very interesting idea, indeed, and would be very useful. Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Overall, I have a split feeling on the series.

One side of my brain thinks that the series does a very good job to address the needs of those who want to partition their objects into two classes, and the problem I saw in the series was mostly the way it was sold (in other words, if it did not mention unbloating lazily cloned repositories at all, I would have said "Yes! It is an excellent series.", and if it said "this mechanism is not meant to be used to unbloat a lazily cloned repository, because the mechanism does not distinguish objects that are only locally available and objects that are retrievable from the promisor remotes, among those that match the filter", it would have been even better)

To the other side of my brain, it smells as if the series wanted to address the unbloating issue, but ended up with an unsatisfactory solution, and used "partitioning objects in a full repository on the server side " as an excuse for the resulting mechanism to still exist, even though it is not usable for the original purpose.

Ideally, it would be great to have a mechanism that can be used for both. The "partitioning" can be treated as a degenerate case where the repository does not have its upstream promisor (hence, any object that match the filtering criteria can be excluded from the primary pack because there are no "not available (yet) in our promisor" objects), while the "unbloat" case can know who its promisors are and ask the promisors what objects, among those that match the filtering criteria, are still available from them to exclude only those objects from the primary pack.

In the second ideal world, we may not be ready to tackle the unbloating issue, but "partitioning" alone may still be a useful feature. In that case, perhaps the series can be salvaged by updating how the feature is sold, with some comments indicating the future direction to extend the mechanism later.

Thanks.

# Intro

Last year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>` and later I sent 4 versions of a patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the filtered out objects altogether which was considered very dangerous even though we implemented different safety checks in some of the latter series.

In some discussions, it was mentioned that such a feature, or a similar feature in `git gc`, or in a new standalone command (perhaps called `git prune-filtered`), should put the filtered out objects into a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving blobs from inactive repos onto cheaper storage, or moving large blobs onto cheaper storage. This lead us to rethink at repacking using a filter, but moving the filtered out objects into a separate packfile instead of deleting them.

So here is a new patch series doing that while implementing the `--filter=<filter-spec>` option in `git repack`.

# Use cases for the new feature

This could be useful for example for the following purposes:

  1) As a way for servers to save storage costs by for example moving
     large blobs, or all the blobs, or all the blobs in inactive
     repos, to separate storage (while still making them accessible
     using for example the alternates mechanism).

  2) As a way to use partial clone on a Git server to offload large
     blobs to, for example, an http server, while using multiple
     promisor remotes (to be able to access everything) on the client
     side. (In this case the packfile that contains the filtered out
     object can be manualy removed after checking that all the objects
     it contains are available through the promisor remote.)

  3) As a way for clients to reclaim some space when they cloned with
     a filter to save disk space but then fetched a lot of unwanted
     objects (for example when checking out old branches) and now want
     to remove these unwanted objects. (In this case they can first
     move the packfile that contains filtered out objects to a
     separate directory or storage, then check that everything works
     well, and then manually remove the packfile after some time.)

As the features and the code are quite different from those in the previous series, I decided to start a new series instead of continuing a previous one.

Also since this version 2 of this new series, commit messages, don't mention uses cases like 2) or 3) above, as people have different opinions on how it should be done. How it should be done could depend a lot on the way promisor remotes are used, the software and hardware setups used, etc, so it seems more difficult to "sell" this series by talking about such use cases. As use case 1) seems simpler and more appealing, it makes more sense to only talk about it in the commit messages.

# Changes since version 1

Thanks to Junio and Taylor who reviewed version 1! The changes are the following:

- I think that in the long run it might have been better for
  performance reasons to implement the `--filter=...` in `git repack`
  the way it was done in version 1.

  (It was done by first implementing `git pack-objects
  --print-filtered` to print objects omitted by a `--filter=...`
  option, then having `git repack --filter=...` launch such a command,
  read filtered out objects from it, and pipe them into a separate
  `git pack-objects` process that would put them into a separate
  packfile.)

  Anyway our list-objects-filter API is currently not well suited for
  that as it doesn't have a way to output the oids of the filtered out
  objects while the filtering is happening. So version 1 had to get
  them from an oid_set afterwards, which could use a lot of memory.

  As I don't want to have to work on improving the list-objects-filter
  API right now, I think it's better for now to just do as Taylor
  suggested, and as what the cruft code is already doing, which is to
  use the `--stdin-pack` option of `git pack-objects` and to pass it
  packfile names, some prefixed with '^', to let it compute what
  should be in the packfile(s) containing the filtered out objects and
  then to create that(/those) packfile(s).

  So patch 5/8 in version 2 which implements the
  `--filter=<filter-spec>` option in `git repack` is very different
  from patch 6/9 which implemented it in version 1.

- By doing it this way, this version gets rid of patch 2/9 in version
  1 which added the `--print-filtered` option to pack-objects.

- We also get rid of patch 4/9 in version 1 which refactored
  piping an oid to a command.

- On the other hand, we can refactor the code which finds the pack
  prefix into a new small function as this will be needed by the new
  code implementing `--filter=<filter-spec>`, so patch 4/8 in version
  2 is new.

- As suggested by Taylor, a small test has been added to verify that
  `--filter=blob:none` can work without `--stdout` in patch 1/8
  (previously 1/9).

- In patch 3/8 (previously 5/9) which refactors the code to finish a
  pack-objects command into a new function, the `is_local` bit is not
  computed inside the new function anymore as suggested by both Junio
  and Taylor.

- The commit messages of patch 5/8 (previously 6/9) and patch 7/8
  (previously 8/9) that implement the `--filter=<filter-spec>` and
  `--filter-to=<dir>` options respectively have been changed to not
  talk about use cases related to promisor remotes as explained
  towards the end of the "Use cases for the new feature" section
  above.

- Also in patch 5/8 (previously 6/9) that implements the
  `--filter=<filter-spec>`option, the documentation for this section
  has been improved, suggesting to use it on bare repos along with -a
  and -d to get the best possible filtering.

- And in patch 7/8 (previously 8/9) that implements the
  `--filter-to=<dir>` option, a new test has been added to check that
  `--filter=<filter-spec>` and `--filter-to=<dir>` work well with
  the `--max-pack-size=...` option as suggested by Taylor.

- Also in patch 7/8 (previously 8/9) we now check that
  `--filter=<filter-spec>` has been passed and error out if not, as it
  doesn't make sense to use `--filter-to=<dir>` without
  `--filter=<filter-spec>`. This was suggested by Taylor.

- To avoid small merge conflicts in the `git gc` doc, this series has
  been rebased onto 9748a68200 (The sixth batch, 2023-06-29).

# Commit overview

* 1/8 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as the first patch in the previous series and
  in v1. To be able to later repack with a filter we need `git
  pack-objects` to write packfiles when it's filtering instead of just
  writing the pack without the filtered out objects to stdout.

* 2/8 t/helper: add 'find-pack' test-tool

  For testing `git repack --filter=...` that we are going to
  implement, it's useful to have a test helper that can tell which
  packfiles contain a specific object. No change in this patch
  compared to v1.

* 3/8 repack: refactor finishing pack-objects command

  This is a small refactoring creating a new useful function, so that
  `git repack --filter=...` will be able to reuse it. The change
  compared to v1 is that the `is_local` bit is not computed inside the
  new function anymore.

* 4/8 repack: refactor finding pack prefix

  This is a new patch with a small refactoring creating a small
  function that will be reused in the next patch.

* 5/8 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. As explained
  above it works differently than in v1. It now uses one `git
  pack-objects` process with the `--filter` option. And then another
  `git pack-objects` process with the `--stdin-packs` option. Also the
  documentation of the new option has been improved compared to v1.

* 6/8 gc: add `gc.repackFilter` config option

  This is a gc config option so that `git gc` can also repack using a
  filter and put the filtered out objects into a separate packfile. No
  changes compared to v1.

* 7/8 repack: implement `--filter-to` for storing filtered out objects

  For some use cases, it's interesting to create the packfile that
  contains the filtered out objects into a separate location. This is
  similar to the `--expire-to` option for cruft packfiles. Since
  version 1 we now check that `--filter=<filter-spec>` has been passed
  as using `--filter-to` without it doesn't make sense. Also a new
  test has been added to check that these options work well with
  `--max-pack-size=...`.

* 8/8 gc: add `gc.repackFilterTo` config option

  This allows specifying the location of the packfile that contains
  the filtered out objects when using `gc.repackFilter`. No change
  since v1.

# Range-diff

 1:  f4e1cc24d2 !  1:  0bd1ad3071 pack-objects: allow `--filter` without `--stdout`
    @@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const
      
        if (stdin_packs && use_internal_rev_list)
                die(_("cannot use internal rev list with --stdin-packs"));
    +
    + ## t/t5317-pack-objects-filter-objects.sh ##
    +@@ t/t5317-pack-objects-filter-objects.sh: test_expect_success 'verify blob:none packfile has no blobs' '
    +   ! grep blob verify_result
    + '
    + 
    ++test_expect_success 'verify blob:none packfile without --stdout' '
    ++  git -C r1 pack-objects --revs --filter=blob:none mypackname >packhash <<-EOF &&
    ++  HEAD
    ++  EOF
    ++  git -C r1 verify-pack -v "mypackname-$(cat packhash).pack" >verify_result &&
    ++  ! grep blob verify_result
    ++'
    ++
    + test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
    +   git -C r1 verify-pack -v ../all.pack >verify_result &&
    +   grep -E "commit|tree" verify_result |
 2:  8cf3db088e <  -:  ---------- pack-objects: add `--print-filtered` to print omitted objects
 3:  2f3b16281c =  2:  e49cd723c7 t/helper: add 'find-pack' test-tool
 4:  0021a5e3bb <  -:  ---------- repack: refactor piping an oid to a command
 5:  dce5087cc3 !  3:  3f87772ea6 repack: refactor finishing pack-objects command
    @@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu
      
     +static int finish_pack_objects_cmd(struct child_process *cmd,
     +                             struct string_list *names,
    -+                             const char *destination)
    ++                             int local)
     +{
    -+  int local = 1;
     +  FILE *out;
     +  struct strbuf line = STRBUF_INIT;
     +
    -+  if (destination) {
    -+          const char *scratch;
    -+          local = skip_prefix(destination, packdir, &scratch);
    -+  }
    -+
     +  out = xfdopen(cmd->out, "r");
     +  while (strbuf_getline_lf(&line, out) != EOF) {
     +          struct string_list_item *item;
    @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
     -  FILE *in, *out;
     +  FILE *in;
        int ret;
    --  const char *scratch;
    --  int local = skip_prefix(destination, packdir, &scratch);
    - 
    -   prepare_pack_objects(&cmd, args, destination);
    - 
    +   const char *scratch;
    +   int local = skip_prefix(destination, packdir, &scratch);
     @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *args,
                fprintf(in, "%s.pack\n", item->string);
        fclose(in);
    @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
     -  strbuf_release(&line);
     -
     -  return finish_command(&cmd);
    -+  return finish_pack_objects_cmd(&cmd, names, destination);
    ++  return finish_pack_objects_cmd(&cmd, names, local);
      }
      
      int cmd_repack(int argc, const char **argv, const char *prefix)
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
     -  strbuf_release(&line);
     -  fclose(out);
     -  ret = finish_command(&cmd);
    -+  ret = finish_pack_objects_cmd(&cmd, &names, NULL);
    ++  ret = finish_pack_objects_cmd(&cmd, &names, 1);
        if (ret)
                goto cleanup;
      
 6:  fedde52ca1 <  -:  ---------- repack: add `--filter=<filter-spec>` option
 -:  ---------- >  4:  9997efaf33 repack: refactor finding pack prefix
 -:  ---------- >  5:  da27ecb91b repack: add `--filter=<filter-spec>` option
 7:  6ebd274334 !  6:  49e4a184b4 gc: add `gc.repackFilter` config option
    @@ Commit message
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
      ## Documentation/config/gc.txt ##
    -@@ Documentation/config/gc.txt: or rebase occurring.  Since these changes are not part of the current
    - project most users will want to expire them sooner, which is why the
    - default is more aggressive than `gc.reflogExpire`.
    +@@ Documentation/config/gc.txt: Multiple hooks are supported, but all must exit successfully, else the
    + operation (either generating a cruft pack or unpacking unreachable
    + objects) will be halted.
      
     +gc.repackFilter::
     +  When repacking, use the specified filter to move certain
 8:  5d68501b1f !  7:  243c93aad3 repack: implement `--filter-to` for storing filtered out objects
    @@ Commit message
         accessible if, for example, the Git alternates mechanism is used to
         point to it.
     
    -    If users want to remove a pack that contains filtered out objects after
    -    checking that they are all already on a promisor remote, creating the
    -    pack in a different directory makes it easier to do so.
    +    While at it, as an example to show that `--filter` and `--filter-to`
    +    work well with other options, let's also add a test to check that these
    +    options work well with `--max-pack-size`.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    +    repack: add test with --max-pack-size
    +
      ## Documentation/git-repack.txt ##
     @@ Documentation/git-repack.txt: depth is 4095.
    -   resulting packfile and put them into a separate packfile. See
    -   linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
    +   this option.  See linkgit:git-rev-list[1] for valid
    +   `<filter-spec>` forms.
      
     +--filter-to=<dir>::
     +  Write the pack containing filtered out objects to the
    @@ Documentation/git-repack.txt: depth is 4095.
        Write a reachability bitmap index as part of the repack. This
     
      ## builtin/repack.c ##
    -@@ builtin/repack.c: static void prepare_pack_filtered_cmd(struct child_process *cmd,
    - }
    - 
    - static void finish_pack_filtered_cmd(struct child_process *cmd,
    --                               struct string_list *names)
    -+                               struct string_list *names,
    -+                               const char *destination)
    - {
    -   if (cmd->in == -1) {
    -           /* No packed objects; cmd was never started */
    -@@ builtin/repack.c: static void finish_pack_filtered_cmd(struct child_process *cmd,
    - 
    -   close(cmd->in);
    - 
    --  if (finish_pack_objects_cmd(cmd, names, NULL, NULL))
    -+  if (finish_pack_objects_cmd(cmd, names, destination, NULL))
    -           die(_("could not finish pack-objects to pack filtered objects"));
    - }
    - 
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    +   int write_midx = 0;
        const char *cruft_expiration = NULL;
        const char *expire_to = NULL;
    -   struct child_process pack_filtered_cmd = CHILD_PROCESS_INIT;
     +  const char *filter_to = NULL;
      
        struct option builtin_repack_options[] = {
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
                strvec_push(&cmd.args, "--incremental");
        }
      
    --  if (po_args.filter)
    --          prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, packtmp);
    -+  if (po_args.filter) {
    -+          if (!filter_to)
    -+                  filter_to = packtmp;
    -+          prepare_pack_filtered_cmd(&pack_filtered_cmd, &po_args, filter_to);
    -+  }
    - 
    ++  if (filter_to && !po_args.filter)
    ++          die(_("option '%s' can only be used along with '%s'"), "--filter-to", "--filter");
    ++
        if (geometry)
                cmd.in = -1;
    +   else
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
        }
      
    -   if (po_args.filter)
    --          finish_pack_filtered_cmd(&pack_filtered_cmd, &names);
    -+          finish_pack_filtered_cmd(&pack_filtered_cmd, &names, filter_to);
    - 
    -   string_list_sort(&names);
    - 
    +   if (po_args.filter) {
    ++          if (!filter_to)
    ++                  filter_to = packtmp;
    ++
    +           ret = write_filtered_pack(&po_args,
    +-                                    packtmp,
    ++                                    filter_to,
    +                                     find_pack_prefix(),
    +                                     &names,
    +                                     &existing_nonkept_packs,
     
      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success 'repacking with a filter works' '
    @@ t/t7700-repack.sh: test_expect_success 'repacking with a filter works' '
     +  blob_content=$(git -C bare.git show $blob_hash) &&
     +  test "$blob_content" = "content1"
     +'
    ++
    ++test_expect_success '--filter works with --max-pack-size' '
    ++  rm -rf filtered.git &&
    ++  git init --bare filtered.git &&
    ++  git init max-pack-size &&
    ++  (
    ++          cd max-pack-size &&
    ++          test_commit base &&
    ++          # two blobs which exceed the maximum pack size
    ++          test-tool genrandom foo 1048576 >foo &&
    ++          git hash-object -w foo &&
    ++          test-tool genrandom bar 1048576 >bar &&
    ++          git hash-object -w bar &&
    ++          git add foo bar &&
    ++          git commit -m "adding foo and bar"
    ++  ) &&
    ++  git clone --no-local --bare max-pack-size max-pack-size.git &&
    ++  (
    ++          cd max-pack-size.git &&
    ++          git -c repack.writebitmaps=false repack -a -d --filter=blob:none \
    ++                  --max-pack-size=1M \
    ++                  --filter-to=../filtered.git/objects/pack/pack &&
    ++          echo $(cd .. && pwd)/filtered.git/objects >objects/info/alternates &&
    ++
    ++          # Check that the 3 blobs are in different packfiles in filtered.git
    ++          test_stdout_line_count = 3 ls ../filtered.git/objects/pack/pack-*.pack &&
    ++          test_stdout_line_count = 1 ls objects/pack/pack-*.pack &&
    ++          foo_pack=$(test-tool find-pack HEAD:foo) &&
    ++          bar_pack=$(test-tool find-pack HEAD:bar) &&
    ++          base_pack=$(test-tool find-pack HEAD:base.t) &&
    ++          test "$foo_pack" != "$bar_pack" &&
    ++          test "$foo_pack" != "$base_pack" &&
    ++          test "$bar_pack" != "$base_pack" &&
    ++          for pack in "$foo_pack" "$bar_pack" "$base_pack"
    ++          do
    ++                  case "$foo_pack" in */filtered.git/objects/pack/*) true ;; *) return 1 ;; esac
    ++          done
    ++  )
    ++'
     +
      objdir=.git/objects
      midx=$objdir/pack/multi-pack-index
 9:  ae45d9845e =  8:  8cb3faa74c gc: add `gc.repackFilterTo` config option

Christian Couder (8):
  pack-objects: allow `--filter` without `--stdout`
  t/helper: add 'find-pack' test-tool
  repack: refactor finishing pack-objects command
  repack: refactor finding pack prefix
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  11 ++
 Documentation/git-pack-objects.txt     |   4 +-
 Documentation/git-repack.txt           |  15 +++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |   8 +-
 builtin/repack.c                       | 162 ++++++++++++++++++-------
 t/helper/test-find-pack.c              |  35 ++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t5317-pack-objects-filter-objects.sh |   8 ++
 t/t6500-gc.sh                          |  23 ++++
 t/t7700-repack.sh                      |  82 +++++++++++++
 13 files changed, 311 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/test-find-pack.c

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught `git pack-objects` to use `--filter`, but required the use of `--stdout` since a partial clone mechanism was not yet in place to handle missing objects. Since then, changes like 9e27beaa23 (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25) and others added support to dynamically fetch objects that were missing.

Even without a promisor remote, filtering out objects can also be useful if we can put the filtered out objects in a separate pack, and in this case it also makes sense for pack-objects to write the packfile directly to an actual file rather than on stdout.

Remove the `--stdout` requirement when using `--filter`, so that in a follow-up commit, repack can pass `--filter` to pack-objects to omit certain objects from the resulting packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt     | 4 ++--
 builtin/pack-objects.c                 | 8 ++------
 t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
 3 files changed, 12 insertions(+), 8 deletions(-)

In a following commit, we will make it possible to separate objects in different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a new test-tool that can display which packfile(s) a given object is in.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 4 files changed, 38 insertions(+)
 create mode 100644 t/helper/test-find-pack.c

Create a new finish_pack_objects_cmd() to refactor duplicated code that handles reading the packfile names from the output of a `git pack-objects` command and putting it into a string_list, as well as calling finish_command().

While at it, beautify a code comment a bit in the new function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

Create a new find_pack_prefix() to refactor code that handles finding the pack prefix from the packtmp and packdir global variables, as we are going to need this feature again in following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

This new option puts the objects specified by `<filter-spec>` into a separate packfile.

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.

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. The result is that only the objects filtered out by the first `git pack-objects` command are in the pack resulting from the second `git pack-objects` command.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt |  9 +++++
 builtin/repack.c             | 67 ++++++++++++++++++++++++++++++++++++
 t/t7700-repack.sh            | 16 +++++++++
 3 files changed, 92 insertions(+)

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

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

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.

On Wed, Jul 5, 2023 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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.

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.

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

Just by removing one or the other, it would be quite improved, no? Moving away some blobs could move away all or just a selected subset.

As long as the set-up is not racy, either would be OK, as the names are not recorded in the end result.

If the upstream tells the downstream the temporary's name and then finializes the temporary to the final name before the downstream reacts to the input, however, then by the time downstream starts working on the file, the file may not exist under its original, temporary name, and that kind of race was what I was worried about.

Sorry, I am not understanding this quite well. Do you mean we should add one more known-to-be-racy-and-incorrect codepath because there is already a codepath that needs to be fixed anyway?

If write_cruft_pack() has a similar issue, then yeah, let's fix that first (testing might be tricky for any racy bugs). And let's use the same technique as used to fix it in this series, too, so that we do not reintroduce a similar bug due to racy setup.

Thanks.

On Mon, Jul 24, 2023 at 8:28 PM Junio C Hamano <gitster@pobox.com> wrote:

Ok, I have done that in my current version.

It doesn't seem to me that it's what happens.

We have the following order:

  - finish_pack_objects_cmd() is called for the first pack-objects
process. It populates the 'names' string_list with the temporary name
of the packfile it generated (which doesn't contain the filtered out
objects) and calls finish_command() to finish the first pack-objects
process. So as far as I understand nothing can be written anymore to
the packfile when finish_pack_objects_cmd() returns.

  - write_filtered_pack() is called. It starts the second pack-objects
process and passes it the temporary name of the packfile that was just
written, taking it from the 'names' string_list. It then calls
finish_pack_objects_cmd() for the second process which populates the
'names' string_list with the temporary name of the packfile created by
the second process and finishes the second process. So nothing can
then be written in the second packfile anymore.

  - close_object_store() is called which renames the packfiles from
the 'names' string_list giving them their final name.

So the final names are given only once both processes are finished and both packfiles have been fully written.

No.

Yeah, that's what I mean.

I am not sure the race actually exists though. I have tried to explain why it seems to me that things look correct, but from previous experience I know that you are very often right, and I might have missed something.

Thanks.

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

"which renames" -> "and then we enter a loop to rename" and close_object_store() itself and its callees do not do much, but yes, you are right. As finish_pack_objects_cmd() is synchronous, there cannot be such race as a feared (if we were feeding the pack objects process that collects the objects that would have filtered out with the final packfile paths, and if we were only renaming them to the final paths after that close_object_store() call, then the process would want to see the final names that are not there yet, but that's not a race but a bug that would reliably trigger).

Thanks for walking through the codepaths involved. We are good then.

Junio C Hamano <gitster@pobox.com> writes:

Sorry, but not so fast.

https://github.com/git/git/actions/runs/5661445152 (seen with this topic) https://github.com/git/git/actions/runs/5662517690 (seen w/o this topic)

The former fails t7700 in the linux-TEST-vars job, while the latter passes the same job.

Thanks.

On Wed, Jul 26, 2023 at 1:09 AM Junio C Hamano <gitster@pobox.com> wrote:

I think this was because I added the following test:

+test_expect_success '--filter fails with --write-bitmap-index' '
+    test_must_fail git -C bare.git repack -a -d --write-bitmap-index \
+        --filter=blob:none &&
+
+    git -C bare.git repack -a -d --no-write-bitmap-index \
+        --filter=blob:none
+'

which fails because in the linux-TEST-vars job the GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP env variable is set to 1 and this counteracts the `--write-bitmap-index` option.

I have tried to fix it like this:

+test_expect_success '--filter fails with --write-bitmap-index' '
+    GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 test_must_fail git -C
bare.git repack \
+        -a -d --write-bitmap-index --filter=blob:none
+'

but I haven't been able to check that this works on CI as all the job seems to fail these days before they even start:

https://github.com/chriscool/git/actions/runs/5791544404/job/15696524676

Thanks!

On Tue, Aug 08, 2023 at 10:45:48AM +0200, Christian Couder wrote:

Makes sense. That linux-TEST-vars job always seems to get me, too.

(As an aside, and definitely not related to your patch here, I wonder if we should consider dropping some of the older TEST variables that belong to features that we no longer consider experimental).

I think the cannonical way to do this is with env, like so:

    test_must_fail env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
      git -C bare.git repack -ad --write-bitmap-index --filter=blob:none 2>err

Thanks, Taylor

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

This remark made me worried, but (luckily) it does not seem to be the case for other topics cooking in 'next' or queued in 'seen'.

It does seem that this topic directly queued on 'master' by itself without any other topics do break the CI quite badly. Almost nothing passes:

  https://github.com/git/git/actions/runs/5812873987

but it may be something as silly as failing test lint. I didn't check very closely.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

And a bit further digging reveals that it is the case.

https://github.com/git/git/actions/runs/5812873987/job/15759211568#step:4:787

Locally you should be able to reproduce it by

    make
    make -C t test-lint

before sending your patches out.

I've queued a squashable fix-up on top of the topic.

Thanks.

---
 t/t7700-repack.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Wed, Aug 09, 2023 at 04:38:45PM -0700, Junio C Hamano wrote:

This shouldn't be necessary. "make test" is supposed to run test-lint (and does catch this case for me). Provided you haven't overridden that by setting TEST_LINT to some more limited set.

-Peff

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

After running the command with this option once, we will have two packfiles, one with objects that match and the other with objects that do not match the filter spec. Then what is the next step for the user of this feature? Moving the former to a slower storage was cited as a motivation for the feature, but can the user tell which one of these two packfiles is the one that consists of the filtered out objects? If there is no mechansim to do so, shouldn't we have one to make this feature more usable?

At the level of "pack-objects" command, we report the new packfiles so that the user does not have to take "ls .git/objects/pack" before and after the operation to compare and learn which ones are new. I do not think "repack" that is a Porcelain should do such a reporting on its standard output, but that means either the feature should probably be done at the plumbing level (i.e. "pack-objects"), or the marking of the new packfiles needs to be done in a way that tools can later find them out, e.g. on the filesystem, similar to the way ".keep" marker tells which ones are not to be repacked, etc.

On Wed, Jul 5, 2023 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:

I think commands like `git verify-pack -v ...` can already tell a bit about the content of a packfile.

Also this patch series adds `test-tool find-pack` which can help too. It could maybe be converted into a new `git verify-pack --find-pack` option if users want it.

Then, as you later found out, there is the --filter-to=<dir> option added later by this series.

To clarify this, I have added the following to the commit message in the version 3 I just sent:

"
   It's possible to find which new packfile contains the filtered out
   objects using one of the following:

     - `git verify-pack -v ...`,
     - `test-tool find-pack ...`, which a previous commit added,
     - `--filter-to=<dir>`, which a following commit will add to specify
       where the pack containing the filtered out objects will be.
"

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

Users might want to perform such a cleanup regularly at the same time as they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a new gc.repackFilter config option.

Now when `git gc` will perform a repack with a <filter-spec> configured through this option and not empty, the repack process will be passed a corresponding `--filter=<filter-spec>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  5 +++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 12 ++++++++++++
 3 files changed, 23 insertions(+)

A previous commit implemented the `gc.repackFilter` config option to specify a filter that should be used by `git gc` when performing repacks.

Another previous commit has implemented `git repack --filter-to=<dir>` to specify the location of the packfile containing filtered out objects when using a filter.

Let's implement the `gc.repackFilterTo` config option to specify that location in the config when `gc.repackFilter` is used.

Now when `git gc` will perform a repack with a <dir> configured through this option and not empty, the repack process will be passed a corresponding `--filter-to=<dir>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  6 ++++++
 builtin/gc.c                |  4 ++++
 t/t6500-gc.sh               | 13 ++++++++++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

It would be nice if this new different pack could be created in a different directory than the regular pack. This would make it possible to move large blobs into a pack on a different kind of storage, for example cheaper storage. Even in a different directory this pack can be accessible if, for example, the Git alternates mechanism is used to point to it.

While at it, as an example to show that `--filter` and `--filter-to` work well with other options, let's also add a test to check that these options work well with `--max-pack-size`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

repack: add test with --max-pack-size
---
 Documentation/git-repack.txt |  6 ++++
 builtin/repack.c             | 11 +++++-
 t/t7700-repack.sh            | 66 ++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

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

OK, this sidesteps the question I had on an earlier step rather nicely. Instead of having to find out which ones are to be moved away, just generating them in a separate location would be more straight forward.

The implementation does not seem to restrict where --filter-to directory can be placed, but shouldn't it make sure that it is one of the already specified alternates directories? Otherwise the user will end up corrupting the repository, no?

On Wed, Jul 5, 2023 at 8:26 PM Junio C Hamano <gitster@pobox.com> wrote:

I don't think it should make sure that the implementation should restrict where the --filter-to directory can be placed.

In version 3, that I just sent, I have written the following in the commit message to explain this:

"
   Even in a different directory, this pack can be accessible if, for
   example, the Git alternates mechanism is used to point to it. In fact
   not using the Git alternates mechanism can corrupt a repo as the
   generated pack containing the filtered objects might not be accessible
   from the repo any more. So setting up the Git alternates mechanism
   should be done before using this feature if the user wants the repo to
   be fully usable while this feature is used.

   In some cases, like when a repo has just been cloned or when there is no
   other activity in the repo, it's Ok to setup the Git alternates
   mechanism afterwards though. It's also Ok to just inspect the generated
   packfile containing the filtered objects and then just move it into the
   '.git/objects/pack/' directory manually. That's why it's not necessary
   for this command to check that the Git alternates mechanism has been
   already setup.
"

I haven't mentioned cases related to promisor remotes, but I think in some of those cases the feature can be very useful too while there is no need to check that the Git alternates mechanism has been set up.

In version 3, the doc for the --filter-to option and the corresponding gc.repackFilterTo config flag look like this:

+--filter-to=<dir>::
+       Write the pack containing filtered out objects to the
+       directory `<dir>`. Only useful with `--filter`. This can be
+       used for putting the pack on a separate object directory that
+       is accessed through the Git alternates mechanism. **WARNING:**
+       If the packfile containing the filtered out objects is not
+       accessible, the repo could be considered corrupt by Git as it
+       migh not be able to access the objects in that packfile. See
+       the `objects` and `objects/info/alternates` sections of
+       linkgit:gitrepository-layout[5].

+gc.repackFilterTo::
+       When repacking and using a filter, see `gc.repackFilter`, the
+       specified location will be used to create the packfile
+       containing the filtered out objects. **WARNING:** The
+       specified location should be accessible, using for example the
+       Git alternates mechanism, otherwise the repo could be
+       considered corrupt by Git as it might not be able to access the
+       objects in that packfile. See the `--filter-to=<dir>` option
+       of linkgit:git-repack[1] and the `objects/info/alternates`
+       section of linkgit:gitrepository-layout[5].

So they warn about possible issues with the feature and link to some relevant doc.

Now if we think that it's not enough, I would implement a check in the code that would warn users loudly if the directory specified by those options is not accessible using the Git alternates mechanism. It would be annoying I think that it would be too restrictive to error out in that case though.

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

"could be considered" -> "can become".

"migh" -> "might".

In all other parts of the system, we tend to avoid such an "unsafe by default" desgin, especially when the risk is known before there is an implementation, and instead allow an explicit end-user action (ranging from command line option to interactive confirmation) to opt-into more risky behaviour. Should we consider --filter-to as such an "always risky and prone to repository corruption" option (just like "--hard" to "reset" is always loses changes in the working tree without warning)?

I am OK with that myself, but others may disagree.

Come to think of it, we haven't seen much reviews from those other than Taylor. Are folks content with the direction this series is going in general?

Thanks.

Hi Junio,

On Mon, 24 Jul 2023 at 19:18, Junio C Hamano <gitster@pobox.com> wrote:

For what it's worth I have a medium-term plan similar to Gitlab's with respect to moving chunks of repositories onto lower cost storage media & to promisor remotes. Like others, I wasn't at all sure about the original approach (and commented at the time). What Christian is proposing here seems much cleaner, is usable without complex gymnastics or safety equipment, and provides a better building block for future work.

Rob :)

Robert Coup <robert.coup@koordinates.com> writes:

Nice to hear a positive feedback [*].

Thanks.

[Footnote]

 * Of course negative ones as long as they are consturctive are also
   welcome;-)

On Mon, Jul 24, 2023 at 8:18 PM Junio C Hamano <gitster@pobox.com> wrote:

Thanks for catching these, they are fixed in my current version.

# Intro

Last year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>` and later I sent 4 versions of a patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the filtered out objects altogether which was considered very dangerous even though we implemented different safety checks in some of the latter series.

In some discussions, it was mentioned that such a feature, or a similar feature in `git gc`, or in a new standalone command (perhaps called `git prune-filtered`), should put the filtered out objects into a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving blobs from inactive repos onto cheaper storage, or moving large blobs onto cheaper storage. This lead us to rethink at repacking using a filter, but moving the filtered out objects into a separate packfile instead of deleting them.

So here is a new patch series doing that while implementing the `--filter=<filter-spec>` option in `git repack`.

# Use cases for the new feature

This could be useful for example for the following purposes:

  1) As a way for servers to save storage costs by for example moving
     large blobs, or all the blobs, or all the blobs in inactive
     repos, to separate storage (while still making them accessible
     using for example the alternates mechanism).

  2) As a way to use partial clone on a Git server to offload large
     blobs to, for example, an http server, while using multiple
     promisor remotes (to be able to access everything) on the client
     side. (In this case the packfile that contains the filtered out
     object can be manualy removed after checking that all the objects
     it contains are available through the promisor remote.)

  3) As a way for clients to reclaim some space when they cloned with
     a filter to save disk space but then fetched a lot of unwanted
     objects (for example when checking out old branches) and now want
     to remove these unwanted objects. (In this case they can first
     move the packfile that contains filtered out objects to a
     separate directory or storage, then check that everything works
     well, and then manually remove the packfile after some time.)

As the features and the code are quite different from those in the previous series, I decided to start a new series instead of continuing a previous one.

Also since version 2 of this new series, commit messages, don't mention uses cases like 2) or 3) above, as people have different opinions on how it should be done. How it should be done could depend a lot on the way promisor remotes are used, the software and hardware setups used, etc, so it seems more difficult to "sell" this series by talking about such use cases. As use case 1) seems simpler and more appealing, it makes more sense to only talk about it in the commit messages.

# Changes since version 2

Thanks to Junio who reviewed both version 1 and 2, and to Taylor who reviewed version 1! The changes are the following:

- In patch 5/8, which introduces `--filter=<filter-spec>` option, some
  explanations about how to find which new packfile contains the
  filtered out objects have been added to the commit message following
  Junio's comments.

- In patch 5/8, it was clarified in the commit message that `git
  pack-objects` is run twice in row (and not in parallel) to implement
  the new option according to Junio's comments.

- In patch 5/8 also, the documentaion of the new option says that
  `--no-write-bitmap-index` (or the ++ `repack.writebitmaps` config
  option set to `false`) should be used along with the option as
  otherwise writing bitmap index will fail. And a corresponding new
  test called '--filter fails with --write-bitmap-index' has been
  added to t/t7700-repack.sh. This should address Taylor's comments
  about v1 that were not addressed by v2.

- In patch 7/8, which implements the `--filter-to=<dir>` option, the
  commit message now recommends using Git alternates mechanism before
  this option is used to make sure the directory specified by the new
  option is accessible by the repo as it could otherwise corrupt the
  repo. It also says that in some cases it might not be necessary to
  use such a mechanism, which is why the feature doesn't check that
  directory specified is accessible. The documentation of the new
  option also loudly warns that the repo could be corrupted if the Git
  alternates mechanism, and has a new link to that mechanism's
  documentation. This is to address Junio's comments.

- In patch 8/8, which implements the `gc.repackFilterTo` config
  option, a similar loud warning has been added, and similar doc
  changes have been made, to the documentation of the new config
  option (which corresponds to the `--filter-to=<dir>` command line
  option).

# Commit overview

* 1/8 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as in v1 and v2. To be able to later repack
  with a filter we need `git pack-objects` to write packfiles when
  it's filtering instead of just writing the pack without the filtered
  out objects to stdout.

* 2/8 t/helper: add 'find-pack' test-tool

  No change in this patch compared to v1 and v2. For testing `git
  repack --filter=...` that we are going to implement, it's useful to
  have a test helper that can tell which packfiles contain a specific
  object.

* 3/8 repack: refactor finishing pack-objects command

  No change in this patch compared to v2. This is a small refactoring
  creating a new useful function, so that `git repack --filter=...`
  will be able to reuse it.

* 4/8 repack: refactor finding pack prefix

  No change in this patch compared to v2. This is another small
  refactoring creating a small function that will be reused in the
  next patch.

* 5/8 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. It uses one
  `git pack-objects` process with the `--filter` option. And then
  another `git pack-objects` process with the `--stdin-packs`
  option. Only the commit message, documentation and tests have been
  changed a bit since v2.

* 6/8 gc: add `gc.repackFilter` config option

  No change in this patch compared to v2 and v1. This is a gc config
  option so that `git gc` can also repack using a filter and put the
  filtered out objects into a separate packfile.

* 7/8 repack: implement `--filter-to` for storing filtered out objects

  For some use cases, it's interesting to create the packfile that
  contains the filtered out objects into a separate location. This is
  similar to the `--expire-to` option for cruft packfiles. Only the
  commit message and the documentation have changed since version
  2. They now explain and discuss the risks of using this option
  without making sure the specified directory is not accessible by the
  repo.

* 8/8 gc: add `gc.repackFilterTo` config option

  This allows specifying the location of the packfile that contains
  the filtered out objects when using `gc.repackFilter`. As with the
  previous commit, since v2, the doc now explain and discuss the risks
  of using this option without making sure the specified directory is
  not accessible by the repo.

# Range-diff since v2

1:  0bd1ad3071 = 1:  4d75a1d7c3 pack-objects: allow `--filter` without `--stdout`
2:  e49cd723c7 = 2:  fdf9b6e8cc t/helper: add 'find-pack' test-tool
3:  3f87772ea6 = 3:  e7cfdebc78 repack: refactor finishing pack-objects command
4:  9997efaf33 = 4:  9c51063795 repack: refactor finding pack prefix
5:  da27ecb91b ! 5:  a90e8045c3 repack: add `--filter=<filter-spec>` option
    @@ Commit message
         This new option puts the objects specified by `<filter-spec>` into a
         separate packfile.
     
    -    This could be useful if, for example, some large blobs take a lot of
    +    This could be useful if, for example, some large blobs take up 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.
     
    -    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
    +    It's possible to find which new packfile contains the filtered out
    +    objects using one of the following:
    +
    +      - `git verify-pack -v ...`,
    +      - `test-tool find-pack ...`, which a previous commit added,
    +      - `--filter-to=<dir>`, which a following commit will add to specify
    +        where the pack containing the filtered out objects will be.
    +
    +    This feature is implemented by running `git pack-objects` twice in a
    +    row. The first command 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
    @@ Documentation/git-repack.txt: depth is 4095.
     +  that objects used in the working directory are not filtered
     +  out. So for the split to fully work, it's best to perform it
     +  in a bare repo and to use the `-a` and `-d` options along with
    -+  this option.  See linkgit:git-rev-list[1] for valid
    -+  `<filter-spec>` forms.
    ++  this option.  Also `--no-write-bitmap-index` (or the
    ++  `repack.writebitmaps` config option set to `false`) should be
    ++  used otherwise writing bitmap index will fail, as it supposes
    ++  a single packfile containing all the objects. See
    ++  linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
     +
      -b::
      --write-bitmap-index::
    @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila
     +  blob_pack2=$(test-tool -C bare.git find-pack HEAD:file2) &&
     +  test "$blob_pack2" = "$blob_pack"
     +'
    ++
    ++test_expect_success '--filter fails with --write-bitmap-index' '
    ++  test_must_fail git -C bare.git repack -a -d --write-bitmap-index \
    ++          --filter=blob:none &&
    ++
    ++  git -C bare.git repack -a -d --no-write-bitmap-index \
    ++          --filter=blob:none
    ++'
     +
      objdir=.git/objects
      midx=$objdir/pack/multi-pack-index
6:  49e4a184b4 = 6:  335b7f614d gc: add `gc.repackFilter` config option
7:  243c93aad3 ! 7:  b1be7f60b7 repack: implement `--filter-to` for storing filtered out objects
    @@ Commit message
         It would be nice if this new different pack could be created in a
         different directory than the regular pack. This would make it possible
         to move large blobs into a pack on a different kind of storage, for
    -    example cheaper storage. Even in a different directory this pack can be
    -    accessible if, for example, the Git alternates mechanism is used to
    -    point to it.
    +    example cheaper storage.
    +
    +    Even in a different directory, this pack can be accessible if, for
    +    example, the Git alternates mechanism is used to point to it. In fact
    +    not using the Git alternates mechanism can corrupt a repo as the
    +    generated pack containing the filtered objects might not be accessible
    +    from the repo any more. So setting up the Git alternates mechanism
    +    should be done before using this feature if the user wants the repo to
    +    be fully usable while this feature is used.
    +
    +    In some cases, like when a repo has just been cloned or when there is no
    +    other activity in the repo, it's Ok to setup the Git alternates
    +    mechanism afterwards though. It's also Ok to just inspect the generated
    +    packfile containing the filtered objects and then just move it into the
    +    '.git/objects/pack/' directory manually. That's why it's not necessary
    +    for this command to check that the Git alternates mechanism has been
    +    already setup.
     
         While at it, as an example to show that `--filter` and `--filter-to`
         work well with other options, let's also add a test to check that these
    @@ Commit message
     
      ## Documentation/git-repack.txt ##
     @@ Documentation/git-repack.txt: depth is 4095.
    -   this option.  See linkgit:git-rev-list[1] for valid
    -   `<filter-spec>` forms.
    +   a single packfile containing all the objects. See
    +   linkgit:git-rev-list[1] for valid `<filter-spec>` forms.
      
     +--filter-to=<dir>::
     +  Write the pack containing filtered out objects to the
    -+  directory `<dir>`. This can be used for putting the pack on a
    -+  separate object directory that is accessed through the Git
    -+  alternates mechanism. Only useful with `--filter`.
    ++  directory `<dir>`. Only useful with `--filter`. This can be
    ++  used for putting the pack on a separate object directory that
    ++  is accessed through the Git alternates mechanism. **WARNING:**
    ++  If the packfile containing the filtered out objects is not
    ++  accessible, the repo could be considered corrupt by Git as it
    ++  migh not be able to access the objects in that packfile. See
    ++  the `objects` and `objects/info/alternates` sections of
    ++  linkgit:gitrepository-layout[5].
     +
      -b::
      --write-bitmap-index::
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
                                          &existing_nonkept_packs,
     
      ## t/t7700-repack.sh ##
    -@@ t/t7700-repack.sh: test_expect_success 'repacking with a filter works' '
    -   test "$blob_pack2" = "$blob_pack"
    +@@ t/t7700-repack.sh: test_expect_success '--filter fails with --write-bitmap-index' '
    +           --filter=blob:none
      '
      
     +test_expect_success '--filter-to stores filtered out objects' '
8:  8cb3faa74c ! 8:  ed66511823 gc: add `gc.repackFilterTo` config option
    @@ Documentation/config/gc.txt: gc.repackFilter::
     +gc.repackFilterTo::
     +  When repacking and using a filter, see `gc.repackFilter`, the
     +  specified location will be used to create the packfile
    -+  containing the filtered out objects.  See the
    -+  `--filter-to=<dir>` option of linkgit:git-repack[1].
    ++  containing the filtered out objects. **WARNING:** The
    ++  specified location should be accessible, using for example the
    ++  Git alternates mechanism, otherwise the repo could be
    ++  considered corrupt by Git as it migh not be able to access the
    ++  objects in that packfile. See the `--filter-to=<dir>` option
    ++  of linkgit:git-repack[1] and the `objects/info/alternates`
    ++  section of linkgit:gitrepository-layout[5].
     +
      gc.rerereResolved::
        Records of conflicted merge you resolved earlier are

Christian Couder (8):
  pack-objects: allow `--filter` without `--stdout`
  t/helper: add 'find-pack' test-tool
  repack: refactor finishing pack-objects command
  repack: refactor finding pack prefix
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  16 +++
 Documentation/git-pack-objects.txt     |   4 +-
 Documentation/git-repack.txt           |  23 ++++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |   8 +-
 builtin/repack.c                       | 162 ++++++++++++++++++-------
 t/helper/test-find-pack.c              |  35 ++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t5317-pack-objects-filter-objects.sh |   8 ++
 t/t6500-gc.sh                          |  23 ++++
 t/t7700-repack.sh                      |  90 ++++++++++++++
 13 files changed, 332 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/test-find-pack.c

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught `git pack-objects` to use `--filter`, but required the use of `--stdout` since a partial clone mechanism was not yet in place to handle missing objects. Since then, changes like 9e27beaa23 (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25) and others added support to dynamically fetch objects that were missing.

Even without a promisor remote, filtering out objects can also be useful if we can put the filtered out objects in a separate pack, and in this case it also makes sense for pack-objects to write the packfile directly to an actual file rather than on stdout.

Remove the `--stdout` requirement when using `--filter`, so that in a follow-up commit, repack can pass `--filter` to pack-objects to omit certain objects from the resulting packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt     | 4 ++--
 builtin/pack-objects.c                 | 8 ++------
 t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
 3 files changed, 12 insertions(+), 8 deletions(-)

On Mon, Jul 24, 2023 at 10:59:02AM +0200, Christian Couder wrote:

Just a couple of style nits here. It's a little strange (for me, at least) to see the heredoc into a git process. I wonder if it might be clearer to write something like:

    echo HEAD >in &&
    git -C r1 pack-objects --revs --filter=blob:none $packdir/pack <in

, but I could certainly go either way on that one. I am less certain about redirecting the output into a file "packhash", only to cat it back out.

Do later tests depend on the existence of this file? If so, then what you have makes sense. If not, I would recommend storing the output in a variable, which avoids both the I/O operation, and the unnecessary "cat" sub-process.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

FWIW, "git" is after all no different from any other command and redirecting here-doc, especially with an option like "--stdin" is in effect, is perfectly sensible, I think. Preparing an input in a file (e.g. "cat >file <<EOF" followed by "git cmd <file") might give you slightly a better debuggability, but I do not sense that it is what you are worried about.

But that would make the syntax awkward. Do you mean something along this line?

	var=$(git ... <<-EOF
		here text
	EOF
	) &&
	git ... mypackname-$var.pack &&
	...

Somehow here-doc and $(command subsitution) does not visually mix well.

Also, $var will not be inspectable when running this test under "-i -v", so it hurts debuggability without taking the output in a temporary file. You could do "-x", of course, but that would make everything ultra verbose, so...

Create a new finish_pack_objects_cmd() to refactor duplicated code that handles reading the packfile names from the output of a `git pack-objects` command and putting it into a string_list, as well as calling finish_command().

While at it, beautify a code comment a bit in the new function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

On Mon, Jul 24, 2023 at 10:59:04AM +0200, Christian Couder wrote:

Everything here looks good to me. Thanks for cleaning this up into its own function and DRY-ing things up a little bit.

I'm glad to see "local" in the arguments list ;-). I think that the implementation came out nice and clean here.

Thanks, Taylor

Create a new find_pack_prefix() to refactor code that handles finding the pack prefix from the packtmp and packdir global variables, as we are going to need this feature again in following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

On Mon, Jul 24, 2023 at 10:59:05AM +0200, Christian Couder wrote:

I wonder if this might be a good opportunity to pass "packtmp" and "packdir" as arguments to the function. I know that these are globals, but it at least nudges us in the right direction away from adding more global variables.

Thanks, Taylor

On Wed, Jul 26, 2023 at 12:47 AM Taylor Blau <me@ttaylorr.com> wrote:

I have changed this in the version 4 I just sent. Now "packtmp" and "packdir" are passed as arguments to the function as you suggest. Thanks.

In a following commit, we will make it possible to separate objects in different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a new test-tool that can display which packfile(s) a given object is in.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 35 +++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 4 files changed, 38 insertions(+)
 create mode 100644 t/helper/test-find-pack.c

On Mon, Jul 24, 2023 at 10:59:03AM +0200, Christian Couder wrote:

Everything that you wrote here seems reasonable to me, and the implementation of the new test tool is very straightforward.

I'm pretty sure that everything here is correct, and we'll implicitly test the behavior of the new helper in following patches.

That said, I think that it might be prudent here to "test the tests" and write a simple test script that exercises this test helper over a more trivial case. There is definitely prior art for testing our helpers directly in the t00?? tests.

Among the test helpers that I can think of off the top of my head, I think a good handful of them have tests:

  - t0011-hashmap.sh
  - t0015-hash.sh
  - t0016-oidmap.sh
  - t0019-json-writer.sh
  - t0052-simple-ipc.sh
  - t0060-path-utils.sh
  - t0061-run-command.sh
  - t0063-string-list.sh
  - t0064-oid-array.sh
  - t0066-dir-iterator.sh
  - t0095-bloom.sh

I would definitely recommend adding a test here, too. Like I said earlier, I think that you are implicitly testing the new behavior here, but it's going to happen in much more complicated environments than something you could construct synthetically here.

Thanks, Taylor

On Wed, Jul 26, 2023 at 12:44 AM Taylor Blau <me@ttaylorr.com> wrote:

Ok, I have written a new t0080-find-pack.sh test script for this in the version 4 I just sent.

I have also changed `test-tool find-pack` so that it now accepts a `--check-count <n>` option. This addresses some of your comments on another patch in the previous version of this series. As the code is now a bit more complex, there is more justification for a test script.

Thanks.

This new option puts the objects specified by `<filter-spec>` into a separate packfile.

This could be useful if, for example, some large blobs take up 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.

It's possible to find which new packfile contains the filtered out objects using one of the following:

  - `git verify-pack -v ...`,
  - `test-tool find-pack ...`, which a previous commit added,
  - `--filter-to=<dir>`, which a following commit will add to specify
    where the pack containing the filtered out objects will be.

This feature is implemented by running `git pack-objects` twice in a row. The first command 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. The result is that only the objects filtered out by the first `git pack-objects` command are in the pack resulting from the second `git pack-objects` command.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt | 12 +++++++
 builtin/repack.c             | 67 ++++++++++++++++++++++++++++++++++++
 t/t7700-repack.sh            | 24 +++++++++++++
 3 files changed, 103 insertions(+)

On Mon, Jul 24, 2023 at 10:59:06AM +0200, Christian Couder wrote:

Very nice. I appreciate you taking my suggestion here; I'm hopeful that it simplified things and resulted in fewer new lines of code.

I think that this comment partially comes from the cruft pack code, where we use the `names` string list both to reference existing packs at the start of the repack, and to keep track of the pack we just wrote (to exclude its contents from the cruft pack).

But I think we only write into "names" via finish_pack_objects_cmd() to record the name of the pack we just wrote containing objects which didn't meet the filter's conditions.

So I think that leaving this comment in is OK, but TBH I was on the fence when I wrote that back in f9825d1cf75 (builtin/repack.c: support generating a cruft pack, 2022-05-20), so I would just as soon drop it.

I think we may only want to do this if `honor_pack_keep` is zero. Otherwise we'd avoid packing objects that appear in kept packs, even if the caller told us to include objects found in kept packs.

I suppose we're storing the filter as a string here because we're just going to pass it down to pack-objects directly. That part makes sense, but I think we are producing subtly inconsistent behavior when specifying multiple --filter options.

IIRC, passing --filter more than once down to pack-objects produces a filter whose objects match all of the individually specified sub-filters. But IIUC, using OPT_STRING here means that later `--filter`'s override earlier ones.

So I think at minimum we'd want to store the filter arguments in a strvec. But I would probably just as soon parse them into a bona-fide list_objects_filter_options struct, and then reconstruct the arguments to pack-objects based on that.

Huh! I never knew about the test_stdout_line_count function, I thought that we always just had test_line_count. Neat!

I wonder if the test-tool itself should exit with a non-zero code if it can't find the given object in any pack. It would at least allow us to drop the "test -n $foo" after every invocation of the test-helper in this test.

Arguably callers may want to ensure that an object doesn't exist in any pack, and this would be inconvenient for them, since they'd have to write something like:

    test_must_fail test-tool find-pack $obj

but I think a more direct test like

    test_must_fail git cat-file -t $obj

would do just as well.

This all looks good, but I think there are a couple of more things that we'd want to test for here:

  - That the list of all objects appears the same before and after all
    of the repacking. I think that this is tested implicitly already in
    your test, but having it written down explicitly would harden this
    against regressions that cause us to inadvertently delete an object
    we shouldn't have.

    (FWIW, I think this would be limited to running something like "git
    cat-file --batch-check='%(objectname)' --batch-all-objects" before
    and after all of the repacking, and ensuring that the two test_cmp
    without failure).

  - Another thing that I don't think we're testing here is that objects
    that *don't* match the filter don't appear in one of the filtered
    packs. I think we'd probably want to assert on the exact contents of
    the pack by dumping the list of objects into a file like "expect",
    and then dumping the actual set of objects with "git show-index
    <$idx | cut -d' ' -f2" or something.

Another thought from the OPT_STRING business above is that we probably want to test this with non-trivial filter arguments. There are probably a handful of interesting cases here, like passing `--no-filter`, passing `--filter` multiple times, passing invalid values for `--filter`, etc.

Do we want to ensure that we get the exit code corresponding with showing the usage text? I could go either way, but I do think that we should grep through the output on stderr to ensure that we get the appropriate error message.

I don't think that this test is adding anything that the above "repacking with a filter works" test isn't covering already.

Thanks, Taylor

On Wed, Jul 26, 2023 at 1:04 AM Taylor Blau <me@ttaylorr.com> wrote:

I made the comment smaller in version 4.

In version 4 I have made changes to better support kept packfiles and related options, including adding tests.

In version 4 a `list_objects_filter_options` struct is now used, and there is a test to check that more than one `--filter=<filter-spec>` option is supported.

Thanks for these suggestions, but I prefered to add the `--check-count <n>` option to `test-tool find-pack` in version 4.

This way `--check-count 0` or `-c 0` for short can be used to check that an object is in no packfile, though it could be for example in a promisor remote or a loose object file. It's also nice to be able to check that an object is in exactly 2 packfiles in some cases.

I don't think we need to test this. `git pack-objects --filter=<filter-spec>` already existed before this series and is tested elsewhere. We can trust that command and its tests, and just check that we used it correctly by checking that only a few objects are in the right packfiles.

I agree that it would not be difficult to do. I just think it's not necessary.

In version 4 we do test that for some objects, as `test-tool find-pack -c 1 $object` would error out if the object is in more than one packfile.

In version 4 there is one test passing `--filter=...` multiple times. I think this is enough, as the `list_objects_filter_options` struct and related functions and mechanisms are tested elsewhere already.

I am not sure that testing the exit code and the stderr output is always needed. Here I think that this test is more for documentation purposes than really enforcing something important. In fact if the behavior would change and `--write-bitmap-index` would understand that it should write an MIDX instead of a regular index, that behavior change could be considered in some ways as an improvement and we would only need to remove 'test_must_fail' here.

Ok, I have removed it in version 4.

Thanks!

On Tue, Aug 08, 2023 at 10:34:25AM +0200, Christian Couder wrote:

"--check-count 0" is a nice approach, thanks!

Yeah, I don't think we should be worried about whether or not pack-objects is doing the right thing here: I agree that we have sufficient coverage for that elsewhere throughout the test suite. I was more concerned at catching bugs or regressions at the 'repack' layer.

But you're more familiar with these changes than I am, so I trust your judgement.

I don't feel that strongly about it, TBH, I think I was more commenting on that we seem to have many of these tests that go

    test_must_fail git <some arguments that don't go together> 2>err &&
    grep "appropriate error message" err

throughout the suite. I don't feel strongly enough to suggest that we add more for this specific purpose.

Thanks, Taylor

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

Users might want to perform such a cleanup regularly at the same time as they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a new gc.repackFilter config option.

Now when `git gc` will perform a repack with a <filter-spec> configured through this option and not empty, the repack process will be passed a corresponding `--filter=<filter-spec>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  5 +++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 12 ++++++++++++
 3 files changed, 23 insertions(+)

On Mon, Jul 24, 2023 at 10:59:07AM +0200, Christian Couder wrote:

Makes sense.

I may be missing something, but what happens if the user has configured gc.repackFilter, but passes additional filters over the command-line arguments? I'm not sure whether these should be AND'd with the existing filters in config, or if they should reset them to zero, or something else.

Regardless, I think it would be beneficial to users if we spelled this out in git-gc(1) instead of just this patch message here.

Nit: can we wrap this across multiple lines?

I think the `test_subcommand` helper might work here, and it would allow you to avoid writing a long grep invocation.

Thanks, Taylor

On Wed, Jul 26, 2023 at 1:07 AM Taylor Blau <me@ttaylorr.com> wrote:

`git gc` doesn't recognize `--filter=<...>` arguments, only `git repack` is being teached to recognize it in this patch series. So I don't see how there could be multiple such arguments on the command line when `git gc` is used.

Also in version 4 `git repack` can be passed many such arguments anyway. So I think we are good.

We could support multiple gc.repackFilter config options, but on the other hand using something like `combine:<filter1>+<filter2>+...<filterN>` should work, as the content of the option is passed as-is to the command line. So we can leave that improvement for later if people don't like the `combine:...` and are interested in it.

I am not sure what should be spelled out. I think we refer people to the `repack --filter=...` option which in turn refers to the `rev-list --filter=...` which contains a good amount of documentation about how `--filter=...` works, including the fact that `combine:...` can be used and that multiple `--filter=...` options can be passed.

Done in version 4.

Other tests related to gc.reflogExpire above use a grep invocation similar to this one, while `test_subcommand` isn't used in the test script, so I think the grep invocation makes the whole script a bit easier to understand.

Thanks.

On Tue, Aug 08, 2023 at 10:38:26AM +0200, Christian Couder wrote:

Ah, thanks. Sorry for the misunderstanding :-).

I agree. To me it seems like there are probably relatively few people who would want to specify a multi-valued configuration directly when they could just use the "combine" trick you suggest. In either case, I agree that it can be done on top later.

Thanks, Taylor

A previous commit implemented the `gc.repackFilter` config option to specify a filter that should be used by `git gc` when performing repacks.

Another previous commit has implemented `git repack --filter-to=<dir>` to specify the location of the packfile containing filtered out objects when using a filter.

Let's implement the `gc.repackFilterTo` config option to specify that location in the config when `gc.repackFilter` is used.

Now when `git gc` will perform a repack with a <dir> configured through this option and not empty, the repack process will be passed a corresponding `--filter-to=<dir>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt | 11 +++++++++++
 builtin/gc.c                |  4 ++++
 t/t6500-gc.sh               | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

It would be nice if this new different pack could be created in a different directory than the regular pack. This would make it possible to move large blobs into a pack on a different kind of storage, for example cheaper storage.

Even in a different directory, this pack can be accessible if, for example, the Git alternates mechanism is used to point to it. In fact not using the Git alternates mechanism can corrupt a repo as the generated pack containing the filtered objects might not be accessible from the repo any more. So setting up the Git alternates mechanism should be done before using this feature if the user wants the repo to be fully usable while this feature is used.

In some cases, like when a repo has just been cloned or when there is no other activity in the repo, it's Ok to setup the Git alternates mechanism afterwards though. It's also Ok to just inspect the generated packfile containing the filtered objects and then just move it into the '.git/objects/pack/' directory manually. That's why it's not necessary for this command to check that the Git alternates mechanism has been already setup.

While at it, as an example to show that `--filter` and `--filter-to` work well with other options, let's also add a test to check that these options work well with `--max-pack-size`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

repack: add test with --max-pack-size
---
 Documentation/git-repack.txt | 11 ++++++
 builtin/repack.c             | 11 +++++-
 t/t7700-repack.sh            | 66 ++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 1 deletion(-)

On Mon, Jul 24, 2023 at 10:59:01AM +0200, Christian Couder wrote:

Apologies for not getting to the second version sooner! It fell off of my post-vacation to-do list, and I'm only just getting to the third round.

Overall I am happy with the direction here and think that this is on the right track. The major points (using --stdin-packs to implement the complementary pack, the behavior of --filter-to, etc.) all look good to me.

But I think there are a handful of smaller issues that we may want to at least discuss first. If so, I think that a handful of them merit a reroll. But I imagine that that rerolled version would be ready to get picked up.

Thanks, Taylor

# Intro

Last year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>` and later I sent 4 versions of a patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the filtered out objects altogether which was considered very dangerous even though we implemented different safety checks in some of the latter series.

In some discussions, it was mentioned that such a feature, or a similar feature in `git gc`, or in a new standalone command (perhaps called `git prune-filtered`), should put the filtered out objects into a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving blobs from inactive repos onto cheaper storage, or moving large blobs onto cheaper storage. This lead us to rethink at repacking using a filter, but moving the filtered out objects into a separate packfile instead of deleting them.

So here is a new patch series doing that while implementing the `--filter=<filter-spec>` option in `git repack`.

# Use cases for the new feature

This could be useful for example for the following purposes:

  1) As a way for servers to save storage costs by for example moving
     large blobs, or all the blobs, or all the blobs in inactive
     repos, to separate storage (while still making them accessible
     using for example the alternates mechanism).

  2) As a way to use partial clone on a Git server to offload large
     blobs to, for example, an http server, while using multiple
     promisor remotes (to be able to access everything) on the client
     side. (In this case the packfile that contains the filtered out
     object can be manualy removed after checking that all the objects
     it contains are available through the promisor remote.)

  3) As a way for clients to reclaim some space when they cloned with
     a filter to save disk space but then fetched a lot of unwanted
     objects (for example when checking out old branches) and now want
     to remove these unwanted objects. (In this case they can first
     move the packfile that contains filtered out objects to a
     separate directory or storage, then check that everything works
     well, and then manually remove the packfile after some time.)

As the features and the code are quite different from those in the previous series, I decided to start a new series instead of continuing a previous one.

Also since version 2 of this new series, commit messages, don't mention uses cases like 2) or 3) above, as people have different opinions on how it should be done. How it should be done could depend a lot on the way promisor remotes are used, the software and hardware setups used, etc, so it seems more difficult to "sell" this series by talking about such use cases. As use case 1) seems simpler and more appealing, it makes more sense to only talk about it in the commit messages.

# Changes since version 3

Thanks to Junio who reviewed both version 1, 2 and 3, and to Taylor who reviewed version 1 and 3! The changes are the following:

- In patch 2/8, which introduces `test-tool find-pack`, a new
  `--check-count <n>` option has been added to check the number of
  packfiles an object is in. To keep things simple and extendable, the
  parse-options API is now used to parse arguments and options.

- Also in patch 2/8, a test script 't0080-find-pack.sh' has been
  introduced to test `test-tool find-pack`, as suggested by Taylor.

- In patch 4/8, which refactors code into a find_pack_prefix()
  function, this function has been changed to accept a `packdir` and a
  `packtmp` argument, instead of using the global variables with the
  same names, as suggested by Taylor.

- In patch 5/8, which introduces `--filter=<filter-spec>` option, a
  `struct list_objects_filter_option` and some related functions and
  macros are now used to handle these options, instead of a character
  string. This allows more than one `--filter=<filter-spec>` option to
  be passed, and a new test has been added to check that this works,
  as suggested by Taylor.

- In patch 5/8, some changes have been made to better handle kept
  packfiles and related tests have been added to check that this works
  well, as suggested by Taylor.

- In patch 5/8, a comment about the 'names' variable has been
  shortened a lot and improved a bit with additional useful
  information, as suggested by Taylor.

- Also in patch 5/8, tests have been improved and shortened by using
  the new `--check-count <n>` option of `test-tool find-pack`.

- Also in patch 5/8, the test that checks that `--filter=...` fails
  with `--write-bitmap-index` has been changed to use
  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 which should fix a CI test
  that sets this variable to 1. This test has also been simplified by
  removing a useless call to `repack --filter=...` as suggested by
  Taylor.

- Also in patch 5/8, the commit message has been improved a bit and
  now only talks about the use case of moving some blobs that take up
  precious space to a cheaper storage, as suggested by Junio.

- In patch 6/8, which implements the `gc.repackFilter` config option,
  a line in the tests that was too long has been split over 2 lines,
  as suggested by Taylor.

- In patch 7/8, which implements the `--filter-to=<dir>` option, the
  documentation of that option talking about possible corruption has
  been clarified a bit, as suggested by Junio.

- Also in patch 7/8, tests have been improved and shortened by using
  the new `--check-count <n>` option of `test-tool find-pack`.

# Commit overview

* 1/8 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as in v1, v2 and v3. To be able to later
  repack with a filter we need `git pack-objects` to write packfiles
  when it's filtering instead of just writing the pack without the
  filtered out objects to stdout.

* 2/8 t/helper: add 'find-pack' test-tool

  For testing `git repack --filter=...` that we are going to
  implement, it's useful to have a test helper that can tell which
  packfiles contain a specific object. Since v3 the new
  `--check-count <n>` option has been added, and tests have been added
  in a new 't0080-find-pack.sh' test script.

* 3/8 repack: refactor finishing pack-objects command

  No change in this patch compared to v2 and v3. This is a small
  refactoring creating a new useful function, so that `git repack
  --filter=...` will be able to reuse it.

* 4/8 repack: refactor finding pack prefix

  This is another small refactoring creating a small function that
  will be reused in the next patch. Since v3 the new function
  introduced in this patch has been changed to accept a `packdir` and
  a `packtmp` argument, instead of using the global variables with the
  same names.

* 5/8 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. It uses one
  `git pack-objects` process with the `--filter` option. And then
  another `git pack-objects` process with the `--stdin-packs`
  option. A lot of changes have been made since v3:

    - The `list_objects_filter_option` struct and some related
      functions and macros are used to handle the new
      `--filter=<filter-spec>` option. A new test has been added to
      check that using multiple such options works.

    - Handling of kept packfiles has been improved and related tests
      have been added.

    - A comment about the 'names' variable has been shortened a lot
      and improved a bit.

    - Tests have been improved and shortened by using the new
      `--check-count <n>` option of `test-tool find-pack`.

    - The test that checks that `--filter=...` fails with
      `--write-bitmap-index` has been improved to pass a CI test and
      shortened.

    - The commit message has been improved a bit.

* 6/8 gc: add `gc.repackFilter` config option

  This is a gc config option so that `git gc` can also repack using a
  filter and put the filtered out objects into a separate
  packfile. Since v3, a line in the tests that was too long has been
  split over 2 lines.

* 7/8 repack: implement `--filter-to` for storing filtered out objects

  For some use cases, it's interesting to create the packfile that
  contains the filtered out objects into a separate location. This is
  similar to the `--expire-to` option for cruft packfiles. Since v3,
  documentation of that option talking about possible corruption has
  been clarified a bit, and tests have been improved and shortened by
  using the new `--check-count <n>` option of `test-tool find-pack`.

* 8/8 gc: add `gc.repackFilterTo` config option

  No change in this patch compared to v3. This allows specifying the
  location of the packfile that contains the filtered out objects when
  using `gc.repackFilter`.

# Range-diff since v3

(Sorry, but the range-diff doesn't show changes in patches 2/8 and 5/8 as there has been a lot of changes in them. Instead it shows that the old commit has been removed and a new one added.)

1:  4d75a1d7c3 = 1:  4d75a1d7c3 pack-objects: allow `--filter` without `--stdout`
2:  fdf9b6e8cc < -:  ---------- t/helper: add 'find-pack' test-tool
-:  ---------- > 2:  0bf9f53158 t/helper: add 'find-pack' test-tool
3:  e7cfdebc78 = 3:  54060d775e repack: refactor finishing pack-objects command
4:  9c51063795 ! 4:  948ea541ae repack: refactor finding pack prefix
    @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
        return finish_pack_objects_cmd(&cmd, names, local);
      }
      
    -+static const char *find_pack_prefix(void)
    ++static const char *find_pack_prefix(char *packdir, char *packtmp)
     +{
     +  const char *pack_prefix;
     +  if (!skip_prefix(packtmp, packdir, &pack_prefix))
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
     -                      packtmp, packdir);
     -          if (*pack_prefix == '/')
     -                  pack_prefix++;
    -+          const char *pack_prefix = find_pack_prefix();
    ++          const char *pack_prefix = find_pack_prefix(packdir, packtmp);
      
                if (!cruft_po_args.window)
                        cruft_po_args.window = po_args.window;
5:  a90e8045c3 < -:  ---------- repack: add `--filter=<filter-spec>` option
-:  ---------- > 5:  0635425289 repack: add `--filter=<filter-spec>` option
6:  335b7f614d ! 6:  bf8be2c812 gc: add `gc.repackFilter` config option
    @@ t/t6500-gc.sh: test_expect_success 'one of gc.reflogExpire{Unreachable,}=never d
     +  git -C bare.git -c gc.cruftPacks=false gc &&
     +  test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
     +
    -+  GIT_TRACE=$(pwd)/trace.out git -C bare.git -c gc.repackFilter=blob:none -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&
    ++  GIT_TRACE=$(pwd)/trace.out git -C bare.git -c gc.repackFilter=blob:none \
    ++          -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&
     +  test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
     +  grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out
     +'
7:  b1be7f60b7 ! 7:  abe7526222 repack: implement `--filter-to` for storing filtered out objects
    @@ Commit message
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    -    repack: add test with --max-pack-size
    -
      ## Documentation/git-repack.txt ##
     @@ Documentation/git-repack.txt: depth is 4095.
        a single packfile containing all the objects. See
    @@ Documentation/git-repack.txt: depth is 4095.
     +  used for putting the pack on a separate object directory that
     +  is accessed through the Git alternates mechanism. **WARNING:**
     +  If the packfile containing the filtered out objects is not
    -+  accessible, the repo could be considered corrupt by Git as it
    -+  migh not be able to access the objects in that packfile. See
    -+  the `objects` and `objects/info/alternates` sections of
    ++  accessible, the repo can become corrupt as it might not be
    ++  possible to access the objects in that packfile. See the
    ++  `objects` and `objects/info/alternates` sections of
     +  linkgit:gitrepository-layout[5].
     +
      -b::
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
        };
      
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    -           strvec_push(&cmd.args, "--incremental");
    -   }
    - 
    -+  if (filter_to && !po_args.filter)
    +   if (po_args.filter_options.choice)
    +           strvec_pushf(&cmd.args, "--filter=%s",
    +                        expand_list_objects_filter_spec(&po_args.filter_options));
    ++  else if (filter_to)
     +          die(_("option '%s' can only be used along with '%s'"), "--filter-to", "--filter");
    -+
    + 
        if (geometry)
                cmd.in = -1;
    -   else
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
        }
      
    -   if (po_args.filter) {
    +   if (po_args.filter_options.choice) {
     +          if (!filter_to)
     +                  filter_to = packtmp;
     +
                ret = write_filtered_pack(&po_args,
     -                                    packtmp,
     +                                    filter_to,
    -                                     find_pack_prefix(),
    +                                     find_pack_prefix(packdir, packtmp),
    +                                     &keep_pack_list,
                                          &names,
    -                                     &existing_nonkept_packs,
     
      ## t/t7700-repack.sh ##
    -@@ t/t7700-repack.sh: test_expect_success '--filter fails with --write-bitmap-index' '
    -           --filter=blob:none
    +@@ t/t7700-repack.sh: test_expect_success '--filter works with --pack-kept-objects and .keep packs' '
    +   )
      '
      
     +test_expect_success '--filter-to stores filtered out objects' '
    @@ t/t7700-repack.sh: test_expect_success '--filter fails with --write-bitmap-index
     +  test_stdout_line_count = 1 ls bare.git/objects/pack/pack-*.pack &&
     +  test_stdout_line_count = 1 ls filtered.git/objects/pack/pack-*.pack &&
     +
    -+  commit_pack=$(test-tool -C bare.git find-pack HEAD) &&
    -+  test -n "$commit_pack" &&
    -+  blob_pack=$(test-tool -C bare.git find-pack HEAD:file1) &&
    -+  test -z "$blob_pack" &&
    ++  commit_pack=$(test-tool -C bare.git find-pack -c 1 HEAD) &&
    ++  blob_pack=$(test-tool -C bare.git find-pack -c 0 HEAD:file1) &&
     +  blob_hash=$(git -C bare.git rev-parse HEAD:file1) &&
     +  test -n "$blob_hash" &&
    -+  blob_pack=$(test-tool -C filtered.git find-pack $blob_hash) &&
    -+  test -n "$blob_pack" &&
    ++  blob_pack=$(test-tool -C filtered.git find-pack -c 1 $blob_hash) &&
     +
     +  echo $(pwd)/filtered.git/objects >bare.git/objects/info/alternates &&
    -+  blob_pack=$(test-tool -C bare.git find-pack HEAD:file1) &&
    -+  test -n "$blob_pack" &&
    ++  blob_pack=$(test-tool -C bare.git find-pack -c 1 HEAD:file1) &&
     +  blob_content=$(git -C bare.git show $blob_hash) &&
     +  test "$blob_content" = "content1"
     +'
    @@ t/t7700-repack.sh: test_expect_success '--filter fails with --write-bitmap-index
     +          # Check that the 3 blobs are in different packfiles in filtered.git
     +          test_stdout_line_count = 3 ls ../filtered.git/objects/pack/pack-*.pack &&
     +          test_stdout_line_count = 1 ls objects/pack/pack-*.pack &&
    -+          foo_pack=$(test-tool find-pack HEAD:foo) &&
    -+          bar_pack=$(test-tool find-pack HEAD:bar) &&
    -+          base_pack=$(test-tool find-pack HEAD:base.t) &&
    ++          foo_pack=$(test-tool find-pack -c 1 HEAD:foo) &&
    ++          bar_pack=$(test-tool find-pack -c 1 HEAD:bar) &&
    ++          base_pack=$(test-tool find-pack -c 1 HEAD:base.t) &&
     +          test "$foo_pack" != "$bar_pack" &&
     +          test "$foo_pack" != "$base_pack" &&
     +          test "$bar_pack" != "$base_pack" &&
8:  ed66511823 = 8:  ccdc858f73 gc: add `gc.repackFilterTo` config option

Christian Couder (8):
  pack-objects: allow `--filter` without `--stdout`
  t/helper: add 'find-pack' test-tool
  repack: refactor finishing pack-objects command
  repack: refactor finding pack prefix
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  16 ++
 Documentation/git-pack-objects.txt     |   4 +-
 Documentation/git-repack.txt           |  23 +++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |   8 +-
 builtin/repack.c                       | 169 +++++++++++++++------
 t/helper/test-find-pack.c              |  50 +++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t0080-find-pack.sh                   |  82 +++++++++++
 t/t5317-pack-objects-filter-objects.sh |   8 +
 t/t6500-gc.sh                          |  24 +++
 t/t7700-repack.sh                      | 196 +++++++++++++++++++++++++
 14 files changed, 543 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/test-find-pack.c
 create mode 100755 t/t0080-find-pack.sh

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

It would be nice if this new different pack could be created in a different directory than the regular pack. This would make it possible to move large blobs into a pack on a different kind of storage, for example cheaper storage.

Even in a different directory, this pack can be accessible if, for example, the Git alternates mechanism is used to point to it. In fact not using the Git alternates mechanism can corrupt a repo as the generated pack containing the filtered objects might not be accessible from the repo any more. So setting up the Git alternates mechanism should be done before using this feature if the user wants the repo to be fully usable while this feature is used.

In some cases, like when a repo has just been cloned or when there is no other activity in the repo, it's Ok to setup the Git alternates mechanism afterwards though. It's also Ok to just inspect the generated packfile containing the filtered objects and then just move it into the '.git/objects/pack/' directory manually. That's why it's not necessary for this command to check that the Git alternates mechanism has been already setup.

While at it, as an example to show that `--filter` and `--filter-to` work well with other options, let's also add a test to check that these options work well with `--max-pack-size`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt | 11 +++++++
 builtin/repack.c             | 10 +++++-
 t/t7700-repack.sh            | 62 ++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

This new option puts the objects specified by `<filter-spec>` into a separate packfile.

This could be useful if, for example, some blobs take up 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.

It's possible to find which new packfile contains the filtered out objects using one of the following:

  - `git verify-pack -v ...`,
  - `test-tool find-pack ...`, which a previous commit added,
  - `--filter-to=<dir>`, which a following commit will add to specify
    where the pack containing the filtered out objects will be.

This feature is implemented by running `git pack-objects` twice in a row. The first command 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. The result is that only the objects filtered out by the first `git pack-objects` command are in the pack resulting from the second `git pack-objects` command.

As the interactions with kept packs are a bit tricky, a few related tests are added.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt |  12 ++++
 builtin/repack.c             |  75 ++++++++++++++++++++
 t/t7700-repack.sh            | 134 +++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+)

On Tue, Aug 08, 2023 at 10:26:05AM +0200, Christian Couder wrote:

Initializing `po_args`'s `filter_options` makes sense to me, but do we ever use the cruft_po_args ones? From looking at this patch, I don't think we do.

Initializing them and then calling list_objects_filter_release() on it isn't wrong, but I can't tell whether initializing these is necessary or not in the first place.

I can't remember off-hand why, but I am pretty sure that we usually write "test_must_fail env ..." before the command-line arguments instead of setting the environment variable outside of the test_must_fail call.

I *think* that this is the same issue as the single-shot environment variable assignment before function call thing that we see in some shells.

Regardless, I wonder if we should be catching this --filter + --write-bitmap-index thing earlier. The error message I get when running this is:

    warning: Failed to write bitmap index. Packfile doesn't have full closure (object ac3e272b72bbf89def8657766b855d0656630ed4 is missing)
    fatal: failed to write bitmap index

Which comes from deep within the pack-bitmap-write.c internals (specifically in a failing call to `find_object_pos()`).

I don't think that's wrong per-se, but I wonder if catching the combination earlier would allow us to carry on writing the pack even if the caller erroneously specified that they wanted a bitmap, similar to how we handle that combination with other options (see the comment in builtin/pack-objects.c that starts with "'hard reasons not to use bitmaps [...]'").

I doubt we'd see many naturally occurring instances of users running "git repack" with both the --filter spec option and --write-bitmap-index. But, I think that it would come up more often in bare repositories, where writing reachability bitmaps is the default state during repacking unless specified otherwise.

I suspect doing something like:

--- 8< ---

Create a new find_pack_prefix() to refactor code that handles finding the pack prefix from the packtmp and packdir global variables, as we are going to need this feature again in following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

On Tue, Aug 08, 2023 at 10:26:04AM +0200, Christian Couder wrote:

I'm definitely nitpicking here, but I think that both of these could be "const" to indicate that we're not modifying "packdir" or "packtmp".

But again, definitely not worth a reroll.

Thanks, Taylor

Create a new finish_pack_objects_cmd() to refactor duplicated code that handles reading the packfile names from the output of a `git pack-objects` command and putting it into a string_list, as well as calling finish_command().

While at it, beautify a code comment a bit in the new function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

Users might want to perform such a cleanup regularly at the same time as they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a new gc.repackFilter config option.

Now when `git gc` will perform a repack with a <filter-spec> configured through this option and not empty, the repack process will be passed a corresponding `--filter=<filter-spec>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  5 +++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 13 +++++++++++++
 3 files changed, 24 insertions(+)

A previous commit implemented the `gc.repackFilter` config option to specify a filter that should be used by `git gc` when performing repacks.

Another previous commit has implemented `git repack --filter-to=<dir>` to specify the location of the packfile containing filtered out objects when using a filter.

Let's implement the `gc.repackFilterTo` config option to specify that location in the config when `gc.repackFilter` is used.

Now when `git gc` will perform a repack with a <dir> configured through this option and not empty, the repack process will be passed a corresponding `--filter-to=<dir>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt | 11 +++++++++++
 builtin/gc.c                |  4 ++++
 t/t6500-gc.sh               | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught `git pack-objects` to use `--filter`, but required the use of `--stdout` since a partial clone mechanism was not yet in place to handle missing objects. Since then, changes like 9e27beaa23 (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25) and others added support to dynamically fetch objects that were missing.

Even without a promisor remote, filtering out objects can also be useful if we can put the filtered out objects in a separate pack, and in this case it also makes sense for pack-objects to write the packfile directly to an actual file rather than on stdout.

Remove the `--stdout` requirement when using `--filter`, so that in a follow-up commit, repack can pass `--filter` to pack-objects to omit certain objects from the resulting packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt     | 4 ++--
 builtin/pack-objects.c                 | 8 ++------
 t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
 3 files changed, 12 insertions(+), 8 deletions(-)

In a following commit, we will make it possible to separate objects in different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a new test-tool that can display which packfile(s) a given object is in.

Let's also make it possible to check if a given object is in the expected number of packfiles with a `--check-count <n>` option.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 50 ++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 t/t0080-find-pack.sh      | 82 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 135 insertions(+)
 create mode 100644 t/helper/test-find-pack.c
 create mode 100755 t/t0080-find-pack.sh

On Tue, Aug 08, 2023 at 10:26:02AM +0200, Christian Couder wrote:

I think there is an extra space between "die" and the opening parenthesis. But obviously not worth a reroll here.

These new tests look great, thanks for adding them :-).

Thanks, Taylor

On Tue, Aug 08, 2023 at 10:26:00AM +0200, Christian Couder wrote:

I took a look through the range-diff as well as the patches themselves again (skimming through the last three, which are much more straightforward than the preceding ones).

Everything looks good to me here, and I think that this version is ready to get picked up once we're on the other side of 2.42.

I left a couple of comments throughout, but none of them merit a reroll on their own. I think there are a couple of things we could easily ignore (marking parameters as "const", etc.), and a couple of things that we should probably take a look at after the dust has settled here.

We *may* want to fix up the test_must_fail invocation that has the environment variable on the left-hand side instead of using "test_must_fail env", but I don't know for sure.

I do think that we should take another look at disabling the bitmap machinery when given `--filter`, but I think that, too, can be done in another series.

Thanks again for being so patient with all of my review comments. I hope it wasn't too big of a pain; this area feels very fragile (to me, at least) so I wanted to give it an extra careful set of eyes.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Ah, that is a show-stopper bug. We must fix it, but the necessary change should be trivial.

Thanks for writing and reviewing.

On Wed, Aug 9, 2023 at 11:45 PM Taylor Blau <me@ttaylorr.com> wrote:

Thanks again for your review!

The version 5 I just sent should fix all the small things that you found in your review.

This is fixed by squashing Junio's 'SQUASH???' commit in version 5.

I agree. I plan to do it later when this is merged. I think it would make it easier to use the new --filter feature, but it would require changes in code, tests and documentation, which can be done later.

Thanks, Christian.

# Intro

Last year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>` and later I sent 4 versions of a patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the filtered out objects altogether which was considered very dangerous even though we implemented different safety checks in some of the latter series.

In some discussions, it was mentioned that such a feature, or a similar feature in `git gc`, or in a new standalone command (perhaps called `git prune-filtered`), should put the filtered out objects into a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving blobs from inactive repos onto cheaper storage, or moving large blobs onto cheaper storage. This lead us to rethink at repacking using a filter, but moving the filtered out objects into a separate packfile instead of deleting them.

So here is a new patch series doing that while implementing the `--filter=<filter-spec>` option in `git repack`.

# Use cases for the new feature

This could be useful for example for the following purposes:

  1) As a way for servers to save storage costs by for example moving
     large blobs, or all the blobs, or all the blobs in inactive
     repos, to separate storage (while still making them accessible
     using for example the alternates mechanism).

  2) As a way to use partial clone on a Git server to offload large
     blobs to, for example, an http server, while using multiple
     promisor remotes (to be able to access everything) on the client
     side. (In this case the packfile that contains the filtered out
     object can be manualy removed after checking that all the objects
     it contains are available through the promisor remote.)

  3) As a way for clients to reclaim some space when they cloned with
     a filter to save disk space but then fetched a lot of unwanted
     objects (for example when checking out old branches) and now want
     to remove these unwanted objects. (In this case they can first
     move the packfile that contains filtered out objects to a
     separate directory or storage, then check that everything works
     well, and then manually remove the packfile after some time.)

As the features and the code are quite different from those in the previous series, I decided to start a new series instead of continuing a previous one.

Also since version 2 of this new series, commit messages, don't mention uses cases like 2) or 3) above, as people have different opinions on how it should be done. How it should be done could depend a lot on the way promisor remotes are used, the software and hardware setups used, etc, so it seems more difficult to "sell" this series by talking about such use cases. As use case 1) seems simpler and more appealing, it makes more sense to only talk about it in the commit messages.

# Changes since version 4

Thanks to Junio who reviewed versions 1, 2, 3 and 4, and to Taylor who reviewed version 1, 3 and 4! Thanks also to Robert Coup who participated in the discussions related to version 2 and Peff who participated in the discussions related to version 4. The changes are the following:

- In patch 2/8, which introduces `test-tool find-pack`, a spurious
  space character has been removed between 'die' and '(', as suggested
  by Taylor.

- In patch 4/8, which refactors code into a find_pack_prefix()
  function, this function has been changed so that the `packdir` and
  `packtmp` arguments are now 'const', as suggested by Taylor.

- In patch 5/8, which introduces `--filter=<filter-spec>` option, the
  `filter_options` member of the 'cruft_po_args' variable is not
  initialized and freed anymore, as this member is actually unused.

- Also in patch 5/8, the '--filter fails with --write-bitmap-index'
  test has been changed to use `test_must_fail env` to fix failures
  with the 'test-lint' Makefile target, as suggested by Junio and
  Taylor. (Junio's 'SQUASH???' patch was squashed into that patch.)

- Also the series was rebased on top of v2.42.0-rc1 as it will likely
  be merged after v2.42.0 will be released and Junio's
  cc/repack-sift-filtered-objects-to-separate-pack branch is based on
  top of v2.42.0-rc0.

# Commit overview

* 1/8 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as in v1, v2, v3 and v4. To be able to later
  repack with a filter we need `git pack-objects` to write packfiles
  when it's filtering instead of just writing the pack without the
  filtered out objects to stdout.

* 2/8 t/helper: add 'find-pack' test-tool

  For testing `git repack --filter=...` that we are going to
  implement, it's useful to have a test helper that can tell which
  packfiles contain a specific object. Since v4 only a space character
  has been removed between a function name and the following '(' to
  comply with our style guide.

* 3/8 repack: refactor finishing pack-objects command

  No change in this patch compared to v2, v3 and v4. This is a small
  refactoring creating a new useful function, so that `git repack
  --filter=...` will be able to reuse it.

* 4/8 repack: refactor finding pack prefix

  This is another small refactoring creating a small function that
  will be reused in the next patch. Since v4 the new function has been
  changed so that its `packdir` and `packtmp` argument are now const.

* 5/8 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. It uses one
  `git pack-objects` process with the `--filter` option. And then
  another `git pack-objects` process with the `--stdin-packs`
  option. A few changes have been made since v4:

    - The `filter_options` member of the 'cruft_po_args' variable is
      not initialized and freed anymore, as this member is actually
      unused.

    - The test that checks that `--filter=...` fails with
      `--write-bitmap-index` has been changed to use `test_must_fail
      env` to fix failures with the 'test-lint' Makefile target.

* 6/8 gc: add `gc.repackFilter` config option

  No change in this patch compared to v4. This is a gc config option
  so that `git gc` can also repack using a filter and put the filtered
  out objects into a separate packfile.

* 7/8 repack: implement `--filter-to` for storing filtered out objects

  No change in this patch compared to v4. For some use cases, it's
  interesting to create the packfile that contains the filtered out
  objects into a separate location. This is similar to the
  `--expire-to` option for cruft packfiles.

* 8/8 gc: add `gc.repackFilterTo` config option

  No change in this patch compared to v3 and v4. This allows
  specifying the location of the packfile that contains the filtered
  out objects when using `gc.repackFilter`.

# Range-diff since v4

1:  09fd23c7d0 = 1:  bbcc368876 pack-objects: allow `--filter` without `--stdout`
2:  c75010d20c ! 2:  f1b80e5728 t/helper: add 'find-pack' test-tool
    @@ t/helper/test-find-pack.c (new)
     +          }
     +
     +  if (count > -1 && count != actual_count)
    -+          die ("bad packfile count %d instead of %d", actual_count, count);
    ++          die("bad packfile count %d instead of %d", actual_count, count);
     +
     +  return 0;
     +}
3:  28221861a0 = 3:  ffecc73960 repack: refactor finishing pack-objects command
4:  41d4faf62b ! 4:  6c2f381a88 repack: refactor finding pack prefix
    @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
        return finish_pack_objects_cmd(&cmd, names, local);
      }
      
    -+static const char *find_pack_prefix(char *packdir, char *packtmp)
    ++static const char *find_pack_prefix(const char *packdir, const char *packtmp)
     +{
     +  const char *pack_prefix;
     +  if (!skip_prefix(packtmp, packdir, &pack_prefix))
5:  a929572b96 ! 5:  134700c2ce repack: add `--filter=<filter-spec>` option
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
        };
      
     +  list_objects_filter_init(&po_args.filter_options);
    -+  list_objects_filter_init(&cruft_po_args.filter_options);
     +
        git_config(repack_config, &cruft_po_args);
      
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
        string_list_clear(&existing_kept_packs, 0);
        clear_pack_geometry(geometry);
     +  list_objects_filter_release(&po_args.filter_options);
    -+  list_objects_filter_release(&cruft_po_args.filter_options);
      
        return ret;
      }
    @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila
     +'
     +
     +test_expect_success '--filter fails with --write-bitmap-index' '
    -+  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 test_must_fail git -C bare.git repack \
    -+          -a -d --write-bitmap-index --filter=blob:none
    ++  test_must_fail \
    ++          env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
    ++          git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
     +'
     +
     +test_expect_success 'repacking with two filters works' '
6:  a22a560d74 = 6:  d3365c7b48 gc: add `gc.repackFilter` config option
7:  387b427fed = 7:  9a09382cd1 repack: implement `--filter-to` for storing filtered out objects
8:  76fac86b0e = 8:  a52e3a71db gc: add `gc.repackFilterTo` config option

Christian Couder (8):
  pack-objects: allow `--filter` without `--stdout`
  t/helper: add 'find-pack' test-tool
  repack: refactor finishing pack-objects command
  repack: refactor finding pack prefix
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  16 ++
 Documentation/git-pack-objects.txt     |   4 +-
 Documentation/git-repack.txt           |  23 +++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |   8 +-
 builtin/repack.c                       | 167 +++++++++++++++------
 t/helper/test-find-pack.c              |  50 +++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t0080-find-pack.sh                   |  82 ++++++++++
 t/t5317-pack-objects-filter-objects.sh |   8 +
 t/t6500-gc.sh                          |  24 +++
 t/t7700-repack.sh                      | 197 +++++++++++++++++++++++++
 14 files changed, 542 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/test-find-pack.c
 create mode 100755 t/t0080-find-pack.sh

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught `git pack-objects` to use `--filter`, but required the use of `--stdout` since a partial clone mechanism was not yet in place to handle missing objects. Since then, changes like 9e27beaa23 (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25) and others added support to dynamically fetch objects that were missing.

Even without a promisor remote, filtering out objects can also be useful if we can put the filtered out objects in a separate pack, and in this case it also makes sense for pack-objects to write the packfile directly to an actual file rather than on stdout.

Remove the `--stdout` requirement when using `--filter`, so that in a follow-up commit, repack can pass `--filter` to pack-objects to omit certain objects from the resulting packfile.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-pack-objects.txt     | 4 ++--
 builtin/pack-objects.c                 | 8 ++------
 t/t5317-pack-objects-filter-objects.sh | 8 ++++++++
 3 files changed, 12 insertions(+), 8 deletions(-)

In a following commit, we will make it possible to separate objects in different packfiles depending on a filter.

To make sure that the right objects are in the right packs, let's add a new test-tool that can display which packfile(s) a given object is in.

Let's also make it possible to check if a given object is in the expected number of packfiles with a `--check-count <n>` option.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile                  |  1 +
 t/helper/test-find-pack.c | 50 ++++++++++++++++++++++++
 t/helper/test-tool.c      |  1 +
 t/helper/test-tool.h      |  1 +
 t/t0080-find-pack.sh      | 82 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 135 insertions(+)
 create mode 100644 t/helper/test-find-pack.c
 create mode 100755 t/t0080-find-pack.sh

Create a new finish_pack_objects_cmd() to refactor duplicated code that handles reading the packfile names from the output of a `git pack-objects` command and putting it into a string_list, as well as calling finish_command().

While at it, beautify a code comment a bit in the new function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

Create a new find_pack_prefix() to refactor code that handles finding the pack prefix from the packtmp and packdir global variables, as we are going to need this feature again in following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
 builtin/repack.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

This new option puts the objects specified by `<filter-spec>` into a separate packfile.

This could be useful if, for example, some blobs take up 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.

It's possible to find which new packfile contains the filtered out objects using one of the following:

  - `git verify-pack -v ...`,
  - `test-tool find-pack ...`, which a previous commit added,
  - `--filter-to=<dir>`, which a following commit will add to specify
    where the pack containing the filtered out objects will be.

This feature is implemented by running `git pack-objects` twice in a row. The first command 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. The result is that only the objects filtered out by the first `git pack-objects` command are in the pack resulting from the second `git pack-objects` command.

As the interactions with kept packs are a bit tricky, a few related tests are added.

Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt |  12 ++++
 builtin/repack.c             |  73 +++++++++++++++++++
 t/t7700-repack.sh            | 135 +++++++++++++++++++++++++++++++++++
 3 files changed, 220 insertions(+)

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

Users might want to perform such a cleanup regularly at the same time as they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a new gc.repackFilter config option.

Now when `git gc` will perform a repack with a <filter-spec> configured through this option and not empty, the repack process will be passed a corresponding `--filter=<filter-spec>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt |  5 +++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 13 +++++++++++++
 3 files changed, 24 insertions(+)

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to filter out some objects from the main pack and move them into a new different pack.

It would be nice if this new different pack could be created in a different directory than the regular pack. This would make it possible to move large blobs into a pack on a different kind of storage, for example cheaper storage.

Even in a different directory, this pack can be accessible if, for example, the Git alternates mechanism is used to point to it. In fact not using the Git alternates mechanism can corrupt a repo as the generated pack containing the filtered objects might not be accessible from the repo any more. So setting up the Git alternates mechanism should be done before using this feature if the user wants the repo to be fully usable while this feature is used.

In some cases, like when a repo has just been cloned or when there is no other activity in the repo, it's Ok to setup the Git alternates mechanism afterwards though. It's also Ok to just inspect the generated packfile containing the filtered objects and then just move it into the '.git/objects/pack/' directory manually. That's why it's not necessary for this command to check that the Git alternates mechanism has been already setup.

While at it, as an example to show that `--filter` and `--filter-to` work well with other options, let's also add a test to check that these options work well with `--max-pack-size`.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt | 11 +++++++
 builtin/repack.c             | 10 +++++-
 t/t7700-repack.sh            | 62 ++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

A previous commit implemented the `gc.repackFilter` config option to specify a filter that should be used by `git gc` when performing repacks.

Another previous commit has implemented `git repack --filter-to=<dir>` to specify the location of the packfile containing filtered out objects when using a filter.

Let's implement the `gc.repackFilterTo` config option to specify that location in the config when `gc.repackFilter` is used.

Now when `git gc` will perform a repack with a <dir> configured through this option and not empty, the repack process will be passed a corresponding `--filter-to=<dir>` argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/gc.txt | 11 +++++++++++
 builtin/gc.c                |  4 ++++
 t/t6500-gc.sh               | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

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

Thanks. I do not recall if the previous version with SQUASH??? passed the tests or not, but this round seems to be breaking the exact test we had trouble with with the previous round:

  https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822

The symptom looks like that "test_must_fail env" test is not failing. Ring a bell?

Thanks.

On Mon, Aug 14, 2023 at 05:51:05PM -0700, Junio C Hamano wrote:

That does ring a bell for me, but this is a different failure than before, IIRC.

This time we're expecting to fail writing a bitmap during a filtered repack, but we succeed. I was wondering in [1] whether or not we should be catching this bad combination of options more eagerly than relying on the pack-bitmap machinery to notice that we're missing a reachability closure.

I think the reason that this succeeds is that we already have a bitmap, and it likely reuses all of the existing bitmaps before discovering that the pack we wrote doesn't contain all objects. So doing this "fixes" the immediate issue:

--- 8< ---

Taylor Blau <me@ttaylorr.com> writes:

Now I am confused.

We were asked to write bitmap index when we are going to create an incomplete pack, and the packfile we generate with the filter will not have full set of objects, and generating a bitmap with such an incomplete knowledge of what objects are reachable from what would be a disaster, so we should turn it off. But the posted patch lacked such a "we should abort when bitmap is asked to be written while filtering" logic.

Then what were we expecting for the test to fail for?

It sounds like the most direct fix.

On Tue, Aug 15, 2023 at 03:32:23PM -0700, Junio C Hamano wrote:

I was similarly confused, and started writing a patch to detect when we see objects in one bitmap but not the other when remapping. But we already handle that case, see the call to `rebuild_bitmap()` from `fill_bitmap_commit()` in pack-bitmap-write.c.

So I don't think we'd ever end up reusing an existing bitmap that refers to objects that we don't have.

But something is definitely strange here. The bitmap generated by this test claims to have three commits:

    $ ~/src/git/t/helper/test-tool bitmap list-commits
    95a9e53327b06212dcf98bd44794b0e2b913deab
    3677360288c631b6b2e1f0e1f081b1e518605e9f
    6f105e6234717c52e9b117b08840926910a68314

...but none of them actually appear to exist in the bitmap:

    $ git rev-list --test-bitmap 95a9e53327b06212dcf98bd44794b0e2b913deab
    Bitmap v1 test (3 entries loaded)
    Found bitmap for '95a9e53327b06212dcf98bd44794b0e2b913deab'. 64 bits / 8b3b6ee7 checksum
    fatal: object not in bitmap: 'ac3e272b72bbf89def8657766b855d0656630ed4'

I think what's going on here is that we attempt to create bitmaps for all three of those commits. We then try and reuse the existing bitmaps, but fail, because we are missing some objects.

So then we try and generate the bitmap from scratch, and when we get down to fill_bitmap_tree() we look up the bit position of the tree itself, and find a non-zero answer, indicating that we have already marked that tree.

And fill_bitmap_tree() correctly assumes that if we have marked the bit corresponding to the tree, that everything reachable from that tree has also been marked. So we never try and locate the bit position for the blob, since we already think that we have a blob marked in the resulting bitmap!

But why is that tree marked in the first place? It's because we attempt to rebuild the bitmap from the existing .bitmap file, but fail part of the way through (when we look up the first blob object in the reposition table). But that happens *after* we see the tree object, so its bit position is marked, even though we didn't rebuild a complete bitmap.

I don't think this matters outside of filtered repacks, but it would be a serious bug to not catch this earlier up like suggested in the (quoted) patch below.

I agree.

I think that we would be OK to not change the implementation of rebuild_bitmap(), or its caller in fill_bitmap_commit(), since this only bites us when bitmapping a filtered pack, and we should catch that case well before getting this deep into the bitmap code.

But it does seem suspect that we rebuild right into ent->bitmap, so we may want to consider doing something like:

--- >8 ---

Taylor Blau <me@ttaylorr.com> writes:

So, there is another bug lurking, other than the lack of "combining filtered repack and bitmaps are explicitly forbidden" logic? We see the tree object, we immediately mark it as "done" even we are not, then we finish in failure and the "done" mark is left behind? Do we need two bits, "under review" and "done", or something then?

Interesting.

On Tue, Aug 15, 2023 at 04:18:02PM -0700, Junio C Hamano wrote:

I think that there is a bug lurking in the sense of trying to reuse bitmaps when covering a pack that doesn't have reachability closure in this particular scenario.

But there are no "blessed" use-cases for doing this. So I think that we should indeed fix this, but I am not immediately concerned here.

No; we can either reuse a complete bitmap or not. So it's fine to OR all of the (permuted) bits into ent->bitmap, but it's not OK to fill in just part of them.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

OK.

Sounds sane.

On Wed, Aug 16, 2023 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:

I would be Ok with such a fix, if we think that we don't want to fix the underlying issue, or if we think that fixing the underlying issue is not enough...

...however I think that fixing this underlying issue is important, as it might cause other tricky issues in the future, for example if other bitmap code is copying or reusing this code.