patch, 3 partsImplement filtering repacks

Earlier this year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>`:

https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/

We tried to "sell" it 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 on the client side.

Even though it is still our end goal, it seems a bit far fetched for now and unnecessary as `git repack --filter=<filter-spec>` could be useful on the client side too.

For example one might want to clone with a filter to avoid too many space to be taken by some large blobs, and one might realize after some time that a number of the large blobs have still be downloaded because some old branches referencing them were checked out. In this case a filtering repack could remove some of those large blobs.

Some of the comments on the patch series that John sent were related to the possible data loss and repo corruption that a filtering repack could cause. It's indeed true that it could be very dangerous, and we agree that improvements were needed in this area.

To address this, in the patch 2/3 introducing --filter, we warn users launching such a repack on the command line and ask them if they really want to do it. If such a repack is not launched from a terminal, we die().

A new patch 3/3, though, introduces --force to allow users to launch such a repack without a terminal and without having to confirm it on the command line.

Patch 1/3 is a preparatory patch.

In short, this small patch series tries to reboot the previous one with a focus on the client side and a focus on safety.

Thanks to John Cai, who worked on the previous versions, and to Jonathan Nieder, Jonathan Tan and Taylor Blau, who recently discussed this with me at the Git Merge and Contributor Summit.

Christian Couder (3):
  pack-objects: allow --filter without --stdout
  repack: add --filter=<filter-spec> option
  repack: introduce --force to force filtering

 Documentation/git-repack.txt | 17 ++++++++++++++
 builtin/pack-objects.c       |  6 +----
 builtin/repack.c             | 44 ++++++++++++++++++++++++++++++------
 t/t7700-repack.sh            | 20 ++++++++++++++++
 4 files changed, 75 insertions(+), 12 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught 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.

Remove the --stdout requirement 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>
---
 builtin/pack-objects.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

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 `git repack --filter=<filter-spec>` would be useful to remove the unneeded large files.

This command could be dangerous to use though, as it might remove local objects that haven't been pushed which would lose data and corrupt the repo. On a server this command could also corrupt a repo unless ALL the removed objects aren't already available in another remote that clients can access.

To avoid as much as possible data to be lost and repos to be corrupted, let's warn users and ask them to confirm that they really want to use this command.

If no terminal is used, let's just die() for now. A follow-up commit will introduce a --force option that will allow using this option when no terminal is available.

It will be easier to test that --filter is working correctly in the follow-up commit that adds --force, so let's just test that we die() if no terminal is used for now.

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

A previous commit introduced --filter=<filter-spec>, but disallowed it when stdin doesn't refer to a terminal, and asked the user when it does refer to a terminal. This is because this option is dangerous as it could lose data and corrupt the repo.

In some cases people might want to run this command automatically when stdin desn't refer to a terminal or when no question should be asked. Let's introduce --force for this purpose.

Unfortunately, we cannot use OPT__FORCE() to implement this because -f is already a repack option (to pass --no-reuse-delta to git-pack-objects), so we use OPT_BOOL() instead.

This is also a good time to test that --filter works properly as it wasn't done in the previous commit that introduced --filter.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-repack.txt | 12 +++++++++++-
 builtin/repack.c             |  7 +++++--
 t/t7700-repack.sh            | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

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

The wish is understandable, but I do not think this gives a good UI.

This feature is, from an end-user's point of view, very similar to "git prune-packed", in that we prune data that is not necessary due to redundancy. Nobody runs "prune-packed" directly; most people are even unaware of it being run on their behalf when they run "git gc".

Reusing pack-objects as an underlying mechanism is OK, but this needs to be plumbed through to "git gc" for a more consistent experience for the end users.

Is there a way to check if the "promisor remote" is still willing to keep the previous promise it made, so that the users do not have to see "we may corrupt the repository as the result of this operation, you have been warned", by the way? Possibly with a protocol extension?

In a sense, once you made a partial clone, your repository is at the mercy of the remote. They can disappear any time with majority of the data you depend on, leaving only what you created locally and haven't pruned away, in a repository that may technically pass "fsck", because the things that are supposed to exist locally exists, but may not be usable in practice. So from that point of view, a simple check that asks "I earlier fetched from you with this filter and these tips of histories; are you still willing to support me?" and gets yes/no answer might be sufficient. A remote that is not trustworthy can say "yes" and still change their mind later, so such a check may not even be needed.

The above two big paragraphs is a way to say that I am not all that worried about losing objects that we should be able to refetch again by adding this feature. The perceived need for "--force" or "must run from terminal" may be overblown. I do not think this negatively affects correctness or robustness at all (as long as the pruning is not buggy, of course).

HOWEVER

Unlike prune-packed, pruning objects that could be refetched has negative performance impact. So adding an option to enable/disable such pruning is needed. If the frontmost UI entry point were "gc", it needs an opt-in option to invoke the "filtering repack", in other words. "pack-objects" should not need any more work than what you have here (with the "terminal" and "force" discarded), as "--filter" is such an opt-in option already.

On Fri, Oct 14, 2022 at 6:46 PM Junio C Hamano <gitster@pobox.com> wrote:

I am Ok with adding the --filter option to `git gc`, or a config option with a similar effect. I wonder how `git gc` should implement that option though.

If we implement a new command called for example `git filter-packed`, similar to `git prune-packed`, then this new command will call `git pack-objects --filter=...`.

`git gc` is already running `git repack` under the hood in a number of cases though. So running `git gc --filter=...` would in many cases call `git pack-objects` twice, as it would call it once through git repack and once through `git filter-packed`. Or am I missing something here?

If on the other hand --filter was implemented in some way in `git repack`, then `git gc --filter=...` could just call `git repack` once.

So even if the new feature should be run only through `git gc` and perhaps a new command possibly called `git filter-packed`, I think it might make sense for efficiency to implement it in some ways, like maybe with some undocumented option or flag, in `git repack`.

It seems to me that `git prune-packed` might only remove objects that are already in pack files. So there is no risk of losing data or corrupting the repo.

Instead, the new feature could in some cases lose data and corrupt the repo if some removed objects haven't yet been pushed. So on the client side, it seems dangerous to me to make it run automatically without a check that everything has been pushed.

Unfortunately some users might already run `git gc` automatically, in cron scripts for example, and they might be tempted to just add the `--filter=...` to their `git gc` script, or to set up a config option with a similar effect without always properly checking that everything has been pushed.

So I am Ok with trying to make the experience consistent, but I would be worrying that it would let people shoot themselves in the foot too easily.

I feel that an obscure `git repack` option would be less likely to be run automatically.

Yeah, when a user clones using --filter=..., the remote can disappear anytime, and we haven't been very worried about that.

Yeah, or a remote that is using some kind of high availability system underneath might consider that it's too expensive and useless to check if everything is properly saved everywhere, as the underlying system has been designed for that purpose and already runs enough internal checks.

I agree. I think it's more important to worry about objects that might have been added locally to the repo, but might not have been pushed somewhere else (yet).

I am Ok to remove the "must run from terminal" and "--force" if we consider that people using this feature should know what they are doing.

I think a command line option like `--filter=...` is what makes it the most obvious that something special is going on, compared to a config option.

Yeah. So to sum up, it looks like you are Ok with `git gc --filter=...` which is fine for me, even if I wonder if `git repack --filter=...` could be a good first step as it is less likely to be used automatically (so safer in a way) and it might be better for implementation related performance reasons.

On Thu, Oct 20, 2022 at 01:23:02PM +0200, Christian Couder wrote:

Conceptually, yes, the two are similar. Though `prune-filtered` is necessarily going to differ in implementation from `prune-packed`, since we will have to write new pack(s), not just delete loose objects which appear in packs already.

So it's really not just a matter of purely deleting redundant loose copies of objects like in the case of prune-packed. Here we really do care about potentially writing a new set of packs to satisfy the new filter constraint.

Presumably that tool would implement creating the new packs according to the given --filter, and would similarly delete existing packs. That is basically what your implementation in repack already does, so I am not sure what the difference would be.

If we don't intend to have `git repack --filter` part of our backwards compatibility guarantee, then I would prefer to see the implementation just live in git-gc from start to finish.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

I am OK with that, too. We do not have to expose the option to users of pack-objects at all. Just make "gc" the single entry point and that should be quite clean.

On Fri, Oct 28, 2022 at 10:26 PM Junio C Hamano <gitster@pobox.com> wrote:

You might mean "to users of `git repack`", as `git pack-objects` already has a `--filter=<filter-spec>` option.

I am Ok with that if there is a simple way to not call `git pack-objects` twice in some cases (once through git repack and once through this new feature). I may be missing something, but unfortunately I don't see one.

(Or maybe you mean that `--filter` in `git gc` should be an option that is incompatible with other `git gc` options and configuration parameters, so that when `git gc --filter=XXX` is called it only performs what `git repack --filter=XXX` performs in the implementation I sent, but then people might want to call `git gc` twice instead of once, if they also want other usual gc housekeeping tasks to be performed. I don't think that's the way `git gc` has been designed for now though.)

On Fri, Oct 28, 2022 at 9:49 PM Taylor Blau <me@ttaylorr.com> wrote:

Yeah, that's why I say `prune-filtered` will call `git pack-objects --filter=...`.

Yeah, I agree.

Indeed, there wouldn't be much difference implementation wise between a new `git filter-packed` command like Junio suggested and the current implementation I sent which implements the feature in `git repack`. (A new `git filter-packed` would just duplicate the repack features that are needed and just call `git pack-objects --filter=...`). That's why I don't really see the point of a new `git filter-packed` command and the version 2 I sent still implements the feature in `git repack`.

So I have a hard time understanding your comment unless you just agree with me.

About the implementation living in `git gc` I wrote the following:

Even if we don't have a `git filter-packed` command, if the feature is implemented in `git gc` (but not in `git repack`) it would just call `git pack-objects --filter=...` from there, which means that `git pack-objects` would be called twice (once through `git repack` and once for this new feature) by `git gc` in some cases, instead of just once if the feature was implemented in `git repack` as `git gc` could then just calls `git repack ... --filter=...` once.

That's why I think it's better for performance reasons if the feature is implemented in `git repack`. If you don't want for some reason to have `git repack --filter=...` part of our backwards compatibility guarantee, then --filter can be a hidden and undocumented option in `git repack`. Or maybe we could use a new env variable to instruct `git repack` to pass some --filter option to `git pack-objects`, but my opinion is that it's much simpler to just accept --filter to be a regular, though dangerous, `git repack` option, and then add --filter to `git gc`.

I am also Ok with adding --filter to `git gc` in this patch series and have the doc say that it's better to use `git gc --filter` instead of `git repack --filter` so that users could learn right away to use the feature through `git gc` instead of through `git repack`.

Earlier this year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>`:

https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/

We tried to "sell" it 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 on the client side.

Even though it is still our end goal, it seems a bit far fetched for now and unnecessary as `git repack --filter=<filter-spec>` could be useful on the client side too.

For example one might want to clone with a filter to avoid too many space to be taken by some large blobs, and one might realize after some time that a number of the large blobs have still be downloaded because some old branches referencing them were checked out. In this case a filtering repack could remove some of those large blobs.

Some of the comments on the patch series that John sent were related to the possible data loss and repo corruption that a filtering repack could cause. It's indeed true that it could be very dangerous, so the first version of this patch series asked the user to confirm the command, either by answering 'Y' on the command line or by passing `--force`.

In the discussion with Junio following that first version though, it appeared that asking for such confirmation might not be necessary, so the differences in this v2 compared to the v1 are:

  - the patch 3/3 that introduced the `--force` option has been
    removed,
    
  - the test that checked that the command couldn't be launched
    without a terminal and without --force has been removed,

  - the code that checked if the command was launched from a terminal
    and that asked for confirmation on the command line has been
    removed,

  - the documentation of --filter has been improved to make it clearer
    that objects created locally which haven't been pushed could be
    deleted.

So there are only 2 patches now in this v2 series:

  - Patch 1/2 is a preparatory patch.

  - Patch 2/2 introduces the `--filter=<filter-spec>` option.

In the discussion with Junio following the first version, Junio suggested adding `--filter=<filter-spec>` to `git gc` and I am still Ok with doing it, either later in a followup patch or in a v3. I haven't done it yet, as I think it's better for performance reasons if anyway the filtering is done underneath by `git repack` (even if for example the --filter option is undocumented in `git repack`), and I'd like the approach in this v2 series to be reviewed first.

Thanks to Junio for discussing the v1, to John Cai, who worked on the previous versions, to Jonathan Nieder, Jonathan Tan and Taylor Blau, who discussed this with me at the Git Merge and Contributor Summit, and to Stolee, Taylor, Robert Coup and Junio who discussed the versions John sent.

Not sure it's very useful as it's quite big, but here is the range-diff compared to v1:

1:  0ac7ebf48c = 1:  d1c65ff1f5 pack-objects: allow --filter without --stdout
2:  144356a97e ! 2:  ac21b4ec8f repack: add --filter=<filter-spec> option
    @@ Commit message
     
         This command could be dangerous to use though, as it might remove
         local objects that haven't been pushed which would lose data and
    -    corrupt the repo. On a server this command could also corrupt a
    +    corrupt the repo. On a server, this command could also corrupt a
         repo unless ALL the removed objects aren't already available in
         another remote that clients can access.
     
    -    To avoid as much as possible data to be lost and repos to be
    -    corrupted, let's warn users and ask them to confirm that they
    -    really want to use this command.
    -
    -    If no terminal is used, let's just die() for now. A follow-up
    -    commit will introduce a --force option that will allow using
    -    this option when no terminal is available.
    -
    -    It will be easier to test that --filter is working correctly
    -    in the follow-up commit that adds --force, so let's just test
    -    that we die() if no terminal is used for now.
    -
         Signed-off-by: John Cai <johncai86@gmail.com>
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ Documentation/git-repack.txt: depth is 4095.
     +--filter=<filter-spec>::
     +  Omits certain objects (usually blobs) from the resulting
     +  packfile. WARNING: this could easily corrupt the current repo
    -+  and lose data if ANY of the omitted objects hasn't been
    -+  already pushed to a remote. See linkgit:git-rev-list[1] for
    -+  valid `<filter-spec>` forms.
    ++  and lose data if ANY of the omitted objects hasn't been already
    ++  pushed to a remote. Be very careful about objects that might
    ++  have been created locally! See linkgit:git-rev-list[1] for valid
    ++  `<filter-spec>` forms.
     +
      -b::
      --write-bitmap-index::
        Write a reachability bitmap index as part of the repack. This
     
      ## builtin/repack.c ##
    -@@
    - #include "pack.h"
    - #include "pack-bitmap.h"
    - #include "refs.h"
    -+#include "prompt.h"
    - 
    - #define ALL_INTO_ONE 1
    - #define LOOSEN_UNREACHABLE 2
     @@ builtin/repack.c: struct pack_objects_args {
        const char *depth;
        const char *threads;
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
                OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
                                N_("repack objects in packs marked with .keep")),
                OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
    -@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    -                   die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
    -   }
    - 
    -+  if (po_args.filter) {
    -+          const char *yesno;
    -+
    -+          if (!isatty(STDIN_FILENO))
    -+                  die (_("Repacking with a filter is not allowed "
    -+                         "yet unless a terminal is used!"));
    -+
    -+          /*
    -+           * TRANSLATORS: Make sure to include [y] and [N] in your translation.
    -+           * The program will only accept English input at this point.
    -+           */
    -+          yesno = git_prompt(_("Repacking with a filter will lose data and corrupt the repo\n"
    -+                               "if ANY of the filtered out object hasn't been already pushed!\n"
    -+                               "Repack with a filter anyway [y/N]? "), PROMPT_ECHO);
    -+          if (tolower(*yesno) != 'y')
    -+                  exit(1);
    -+  }
    -+
    -   if (write_bitmaps < 0) {
    -           if (!write_midx &&
    -               (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
                if (line.len != the_hash_algo->hexsz)
                        die(_("repack: Expecting full hex object ID lines only from pack-objects."));
    @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila
        test_must_be_empty actual
      '
      
    -+test_expect_success 'repacking with a filter is not allowed' '
    -+  test_must_fail git repack -a -d --filter=blob:none 2>actual &&
    -+  test_i18ngrep "Repacking with a filter is not allowed" actual
    ++test_expect_success 'repacking with a filter works' '
    ++  test_when_finished "rm -rf server client" &&
    ++  test_create_repo server &&
    ++  git -C server config uploadpack.allowFilter true &&
    ++  git -C server config uploadpack.allowAnySHA1InWant true &&
    ++  test_commit -C server 1 &&
    ++  git clone --bare --no-local server client &&
    ++  git -C client config remote.origin.promisor true &&
    ++  git -C client rev-list --objects --all --missing=print >objects &&
    ++  test $(grep "^?" objects | wc -l) = 0 &&
    ++  git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
    ++  git -C client rev-list --objects --all --missing=print >objects &&
    ++  test $(grep "^?" objects | wc -l) = 1
     +'
     +
      objdir=.git/objects
3:  a23e19796e < -:  ---------- repack: introduce --force to force filtering

Christian Couder (2):
  pack-objects: allow --filter without --stdout
  repack: add --filter=<filter-spec> option

 Documentation/git-repack.txt |  8 ++++++++
 builtin/pack-objects.c       |  6 +-----
 builtin/repack.c             | 22 +++++++++++++++-------
 t/t7700-repack.sh            | 15 +++++++++++++++
 4 files changed, 39 insertions(+), 12 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught 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.

Remove the --stdout requirement 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>
---
 builtin/pack-objects.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

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 `git repack --filter=<filter-spec>` would be useful to remove the unneeded large files.

This command could be dangerous to use though, as it might remove local objects that haven't been pushed which would lose data and corrupt the repo. On a server, this command could also corrupt a repo unless ALL the removed objects aren't already available in another remote that clients can access.

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

On Tue, Oct 25, 2022 at 02:28:54PM +0200, Christian Couder wrote:

One thing that I wasn't clear on in this or the previous round(s) was how we handle setting remote.<name>.promisor and partialclonefilter.

If there is a single remote, then it's obvious that we should set promisor to "true" and partialCloneFilter to whatever value of `--filter` the user provided when repacking / GCing.

But what happens if there are multiple remotes? Which get the new configuration settings modified?

I wonder what breakage happens if we fail to do that (and why such breakage isn't yet noticed by CI).

Thanks, Taylor

On Fri, Oct 28, 2022 at 9:54 PM Taylor Blau <me@ttaylorr.com> wrote:

Yeah, I agree that it's an interesting question that I overlooked.

I would be Ok to setting remote.<name>.promisor to true in this case, but I am not sure we really need to do it.

Maybe the user is mostly interested in reducing the size of the repo for now and plans to set up a promisor remote afterwards.

Another perhaps better way to handle this would be to just die() if no remote.<name>.promisor is set to true. This way we can make sure that users will not forget to set up at least one promisor remote. This could also give users the opportunity to think about whether their configured remotes contain all the objects they are going to remove.

About remote.<name>.partialclonefilter I don't think we need to do anything. Maybe the user would be Ok with having different filters when fetching and when cleaning up.

I agree that if we want this feature to modify settings, then there is no good and simple solution in this case.

If we want to avoid breakages as much as possible, then die()ing when no remote.<name>.promisor is set to true seems to be the best solution, instead of trying to modify settings.

Earlier this year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>`:

https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/

We tried to "sell" it 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 on the client side.

Even though it is still our end goal, it seems a bit far fetched for now and unnecessary as `git repack --filter=<filter-spec>` could be useful on the client side too.

For example one might want to clone with a filter to avoid too many space to be taken by some large blobs, and one might realize after some time that a number of the large blobs have still be downloaded because some old branches referencing them were checked out. In this case a filtering repack could remove some of those large blobs.

Some of the comments on the patch series that John sent were related to the possible data loss and repo corruption that a filtering repack could cause. It's indeed true that it could be very dangerous, so the first version of this patch series asked the user to confirm the command, either by answering 'Y' on the command line or by passing `--force`.

In the discussion with Junio following that first version though, it appeared that asking for such confirmation might not be necessary, so the v2 removed those checks. Taylor though asked what would happen to the remote.<name>.promisor and remote.<name>.partialclonefilter config variables when a filtering repack is run. I replied that it seems to me that we should just check that a promisor remote has been configured and fail if that's not the case.

In the discussions with Junio and Taylor following the first and second versions, Junio suggested adding `--filter=<filter-spec>` to `git gc` and I am still Ok with doing it, either later in a followup patch or in a v4. I haven't done it yet, as it's not clear how to implement it efficiently only in `git gc`.

`git gc` is already running `git repack` when it's passed some options (either on the command line or via the config), so it would be just simpler for `git gc` to just pass on the --filter=<filter-spec> it would be given to `git repack`. If `git gc` would implement that by calling `git pack-objects` directly, then `git pack-objects` would possibly be called twice from `git gc`: once throught `git repack` and once triggered by the --filter option.

So the only changes in this v3 compared to v2 are the following:

  - rebased on top of a0789512c5 (The thirteenth batch, 2022-11-18) to
    avoid a simple conflict,

  - patch 2/2 uses has_promisor_remote() to check if a promisor remote
    is configured and die() otherwise.

Thanks to Junio and Taylor for discussing the v1 and v2, to John Cai, who worked on the previous versions, to Jonathan Nieder, Jonathan Tan and Taylor Blau, who discussed this with me at the Git Merge and Contributor Summit, and to Stolee, Taylor, Robert Coup and Junio who discussed the versions John sent.

Range diff with v2:

1:  d1c65ff1f5 = 1:  1e64cac782 pack-objects: allow --filter without --stdout
2:  ac21b4ec8f ! 2:  7216a7bc05 repack: add --filter=<filter-spec> option
    @@ Commit message
         repo unless ALL the removed objects aren't already available in
         another remote that clients can access.
     
    +    To mitigate that risk, we check that a promisor remote has at
    +    least been configured.
    +
         Signed-off-by: John Cai <johncai86@gmail.com>
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ builtin/repack.c: static void prepare_pack_objects(struct child_process *cmd,
        if (args->no_reuse_delta)
                strvec_pushf(&cmd->args, "--no-reuse-delta");
        if (args->no_reuse_object)
    -@@ builtin/repack.c: static unsigned populate_pack_exts(char *name)
    -   return ret;
    +@@ builtin/repack.c: static struct generated_pack_data *populate_pack_exts(const char *name)
    +   return data;
      }
      
     +static void write_promisor_file_1(char *p)
    @@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_
     -          write_promisor_file(promisor_name, NULL, 0);
     -
     +          write_promisor_file_1(line.buf);
    -           item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
    +           item->util = populate_pack_exts(item->string);
     -
     -          free(promisor_name);
        }
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
                OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
                                N_("repack objects in packs marked with .keep")),
                OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
    +@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    +                   die(_("options '%s' and '%s' cannot be used together"), "--cruft", "-k");
    +   }
    + 
    ++  if (po_args.filter && !has_promisor_remote())
    ++          die("a promisor remote must be setup\n"
    ++              "Also please push all the objects "
    ++              "that might be filtered to that remote!\n"
    ++              "Otherwise they will be lost!");
    ++
    +   if (write_bitmaps < 0) {
    +           if (!write_midx &&
    +               (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
                if (line.len != the_hash_algo->hexsz)
                        die(_("repack: Expecting full hex object ID lines only from pack-objects."));
    -           string_list_append(&names, line.buf);
    +           item = string_list_append(&names, line.buf);
     +          if (po_args.filter)
     +                  write_promisor_file_1(line.buf);
    +           item->util = populate_pack_exts(item->string);
        }
        fclose(out);
    -   ret = finish_command(&cmd);
     
      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavailable' '

Christian Couder (2):
  pack-objects: allow --filter without --stdout
  repack: add --filter=<filter-spec> option

 Documentation/git-repack.txt |  8 ++++++++
 builtin/pack-objects.c       |  6 +-----
 builtin/repack.c             | 28 +++++++++++++++++++++-------
 t/t7700-repack.sh            | 15 +++++++++++++++
 4 files changed, 45 insertions(+), 12 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught 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.

Remove the --stdout requirement 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>
---
 builtin/pack-objects.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

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 `git repack --filter=<filter-spec>` would be useful to remove the unneeded large files.

This command could be dangerous to use though, as it might remove local objects that haven't been pushed which would lose data and corrupt the repo. On a server, this command could also corrupt a repo unless ALL the removed objects aren't already available in another remote that clients can access.

To mitigate that risk, we check that a promisor remote has at least been configured.

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

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

Did I make such a suggestion? I only said --filter=<filter-spec> does not seem like a good option to surface and stress at the end-user level for "git pack-objects".

The similarity of "git prune-packed" with this new operation was brought up to illustrate the point that users of "git gc" do not want to even know about having to remove redundantly available objects via "prune-packed" by giving an extra option to "git gc" (hence --prune is the default), and honoring the filter spec when objects are known to be available via the configured promisor remote is likely what users at the "git gc" level would want to see happen automatically. IOW, adding more options to "gc" would be the last thing I would want to see.

On Wed, Nov 23, 2022 at 1:31 AM Junio C Hamano <gitster@pobox.com> wrote:

In https://lore.kernel.org/git/xmqqilkm9wv6.fsf@gitster.g/ you wrote:

-> Unlike prune-packed, pruning objects that could be refetched has -> negative performance impact. So adding an option to enable/disable -> such pruning is needed. If the frontmost UI entry point were "gc", -> it needs an opt-in option to invoke the "filtering repack", in other -> words. "pack-objects" should not need any more work than what you -> have here (with the "terminal" and "force" discarded), as "--filter" -> is such an opt-in option already.

I understood the above to mean that you would be Ok with adding a "--filter" flag to `git gc`.

I still think that users should be able to control if `git gc` performs such a filtering when repacking, and I don't see how it could be done without any new option at all. I think that for example only checking the 'remote.<name>.partialclonefilter' config variable to decide if it should be done or not is just too dangerous for people already using this variable.

In the v4 I will send really soon now, I tentatively implemented a 'gc.repackFilter' config option for the purpose of telling `git gc` that it should perform filtering repacks with the specified filter. I hope that this will help move the discussion forward.

BTW in the latest "What's cooking in git.git" emails there is the following about this patch series:

-> Seems to break CI. -> cf. https://github.com/git/git/actions/runs/3560918726

I took a look at the failures in the different failing tests and they don't seem related to this patch series and seem quite random:

=== Failed test: t3106-ls-tree-pattern ===
The full logs are in the 'print test failures' step below.
See also the 'failed-tests-*' artifacts attached to this run.
Error: failed: t3106.3 combine with "--object-only"
failure: t3106.3 combine with "--object-only"
Error: failed: t3106.5 combine with "--long"
failure: t3106.5 combine with "--long"

=== Failed test: t5601-clone ===
The full logs are in the 'print test failures' step below.
See also the 'failed-tests-*' artifacts attached to this run.
skip: t5601.60 clone c:temp is dos drive
skip: t5601.101 colliding file detection
Error: failed: t5601.110 auto-discover bundle URI from HTTP clone
failure: t5601.110 auto-discover bundle URI from HTTP clone
Error: failed: t5601.111 auto-discover multiple bundles from HTTP clone
failure: t5601.111 auto-discover multiple bundles from HTTP clone

=== Failed test: t5556-http-auth ===
The full logs are in the 'print test failures' step below.
See also the 'failed-tests-*' artifacts attached to this run.
Error: failed: t5556.2 http auth anonymous no challenge
failure: t5556.2 http auth anonymous no challenge
Error: failed: t5556.3 http auth www-auth headers to credential helper
bearer valid
failure: t5556.3 http auth www-auth headers to credential helper bearer valid

=== Failed test: t4203-mailmap ===
The full logs are in the 'print test failures' step below.
See also the 'failed-tests-*' artifacts attached to this run.
Error: failed: t4203.66 git cat-file -s returns correct size with --use-mailmap
failure: t4203.66 git cat-file -s returns correct size with --use-mailmap
Error: failed: t4203.67 git cat-file -s returns correct size with
--use-mailmap for tag objects
failure: t4203.67 git cat-file -s returns correct size with
--use-mailmap for tag objects

etc

I tried different build flags but can't reproduce locally.

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

Both sounds like sensible things to do.  Thanks.

Earlier this year, John Cai sent 2 versions of a patch series to implement `git repack --filter=<filter-spec>`:

https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/

We tried to "sell" it 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 on the client side.

Even though it is still our end goal, it seems a bit far fetched for now and unnecessary as `git repack --filter=<filter-spec>` could be useful on the client side too.

For example one might want to clone with a filter to avoid too many space to be taken by some large blobs, and one might realize after some time that a number of the large blobs have still be downloaded because some old branches referencing them were checked out. In this case a filtering repack could remove some of those large blobs.

Some of the comments on the patch series that John sent were related to the possible data loss and repo corruption that a filtering repack could cause. It's indeed true that it could be very dangerous, so the first version of this patch series asked the user to confirm the command, either by answering 'Y' on the command line or by passing `--force`.

In the discussion with Junio following that first version though, it appeared that asking for such confirmation might not be necessary, so the v2 removed those checks.

Taylor though asked what would happen to the 'remote.<name>.promisor' and 'remote.<name>.partialclonefilter' config variables when a filtering repack is run. As it seemed to me that we should just check that a promisor remote has been configured and fail if that's not the case, that was implemented in the third version of this patch series.

In the discussions following the first, second and third versions, Junio commented that `git gc` was a better way for users to launch filtering repacks then `git repack`, so in this v4 a new 'gc.repackFilter' config option is implemented that allows `git gc` to perform filtering repacks. When this config option is set to a non empty string, `git gc` will just add a `--filter=<filter-spec>` argument to the repack processes it launches, with '<filter-spec>' set to the value of 'gc.repackFilter'.

So the changes in this v4 compared to v3 are the following:

  - rebased on top of 57e2c6ebbe (Start the 2.40 cycle, 2022-12-14) to
    avoid a simple conflict,

  - simplified the test in patch 2/3 by using `grep -c ...` instead of
    `grep ... | wc -l`,

  - added patch 3/3 which implements a new 'gc.repackFilter' config
    option so that `git gc` can perform filtering repacks.

Thanks to Junio and Taylor for discussing the v1, v2 and v3, to John Cai, who worked on the previous versions, to Jonathan Nieder, Jonathan Tan and Taylor, who discussed this with me at the Git Merge and Contributor Summit, and to Stolee, Taylor, Robert Coup and Junio who discussed the versions John sent.

Range diff with v3:

1:  1e64cac782 < -:  ---------- pack-objects: allow --filter without --stdout
-:  ---------- > 1:  c2dca82dee pack-objects: allow --filter without --stdout
2:  7216a7bc05 ! 2:  1dcdba4b1d repack: add --filter=<filter-spec> option
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
     +                  write_promisor_file_1(line.buf);
                item->util = populate_pack_exts(item->string);
        }
    -   fclose(out);
    +   strbuf_release(&line);
     
      ## t/t7700-repack.sh ##
     @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavailable' '
    @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila
     +  git clone --bare --no-local server client &&
     +  git -C client config remote.origin.promisor true &&
     +  git -C client rev-list --objects --all --missing=print >objects &&
    -+  test $(grep "^?" objects | wc -l) = 0 &&
    ++  test $(grep -c "^?" objects) = 0 &&
     +  git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
     +  git -C client rev-list --objects --all --missing=print >objects &&
    -+  test $(grep "^?" objects | wc -l) = 1
    ++  test $(grep -c "^?" objects) = 1
     +'
     +
      objdir=.git/objects
-:  ---------- > 3:  6bb98b4b00 gc: add gc.repackFilter config option

Christian Couder (3):
  pack-objects: allow --filter without --stdout
  repack: add --filter=<filter-spec> option
  gc: add gc.repackFilter config option

 Documentation/config/gc.txt  |  9 +++++++++
 Documentation/git-repack.txt |  8 ++++++++
 builtin/gc.c                 |  6 ++++++
 builtin/pack-objects.c       |  8 ++------
 builtin/repack.c             | 28 +++++++++++++++++++++-------
 t/t6500-gc.sh                | 19 +++++++++++++++++++
 t/t7700-repack.sh            | 15 +++++++++++++++
 7 files changed, 80 insertions(+), 13 deletions(-)

From: Christian Couder <chriscool@tuxfamily.org>

9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21) taught 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.

Remove the --stdout requirement 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>
---
 builtin/pack-objects.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

On Wed, Dec 21, 2022 at 05:04:44AM +0100, Christian Couder wrote:

Can we add a test to exercise this now-allowed combination of options?

Patrick

From: Christian Couder <chriscool@tuxfamily.org>

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 `git repack --filter=<filter-spec>` would be useful to remove the unneeded large files.

This command could be dangerous to use though, as it might remove local objects that haven't been pushed which would lose data and corrupt the repo. On a server, this command could also corrupt a repo unless ALL the removed objects aren't already available in another remote that clients can access.

To mitigate that risk, we check that a promisor remote has at least been configured.

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

On Wed, Dec 21, 2022 at 05:04:45AM +0100, Christian Couder wrote:

While this is a nice safeguard, I wonder whether it is sufficient. Suppose you for example have a non-bare repository that already has blobs checked out that would become removed by the filtering repack -- does Git handle this situation gracefully?

A quick check seems to indicate that it does. But not quite as well as I'd have hoped: when I switch to a detached HEAD with an arbitrary commit and then execute `git repack --filter=blob:none` then it also removes blobs that are referenced by the currently checked-out commit. This may or may not be what the user is asking for, but I'd rather lean towards this behaviour being surprising.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Hmph, the user asked not to have blobs that came from remote locally and instead refetch them from the promisor on-demand, so I would expect some pruning to happen (I am not a lazy-clone user, though).

As long as we do the pruning sensibly, that is.

Unless you are always following somebody else without doing any work on your own, you are likely to have objects that exist only locally and nowhere else. It would be unexpected and surprising, if we lost them only because they are of type 'blob' and because there is a promisor remote configured.

Even if that is documented, that would be an unacceptable foot-gun misfeature. It is not just a local repository corruption that can be recovered by cloning from elsewhere. We are looking at lost work that cannot be recovered.

I wonder if this topic can be salvaged by making it less aggressive in pruning, perhaps by traversing from the tips of remote-tracking branches of the promisor remote to identify which blobs can safely be pruned (by definition, promisor remote cannot lose objects that it once published, or its cloners will immediately have corrupt repositories). That may turn this from a misfeature into a feature.

Thanks.

A previous commit has implemented `git repack --filter=<filter-spec>` to allow users to remove objects that are available on a promisor remote but that they don't want locally.

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 |  9 +++++++++
 builtin/gc.c                |  6 ++++++
 t/t6500-gc.sh               | 19 +++++++++++++++++++
 3 files changed, 34 insertions(+)

On Wed, Dec 21, 2022 at 05:04:46AM +0100, Christian Couder wrote:

This limitation means that no user can ever configure this unless they also set `gc.auto=0`, or otherwise Git might periodically delete their local objects without any way to restore them.

I see though that this might be a useful thing to have for partial clone repositories: right now they only ever grow in size, so it would be nice if Git could gracefully cull that size every now and then. But for this to be automatic I'd expect a few safeguards:

    - Given a remote with a partial clone filter we only ever delete
      objects that seem to exist on the remote. This might for example
      be determined via a graph-walk of the remote's references. This
      protects against deleting objects that only exist locally.

    - Objects that are referenced by any of the currently checked-out
      HEADs should not get puned. This protects against deleting objects
      that you'd have to re-download anyway.

    - It would be great to have a grace period after which objects get
      pruned. Given that we have no access times for packed objects
      though this doesn't seem that easy to implement though. Still,
      this would protect the user from objects getting deleted when they
      frequently switch between different commits.

From my point of view we should not expose a high-level interface around git-gc(1) at this point in time because of these missing safeguards. Otherwise it is only a footgun waiting to go off.

Until then I don't see much of a problem with exposing the low-level interface via git-repack(1) though.

Patrick

`git pack-objects` already supports a `--missing=<missing-action>` option, so that it can avoid erroring out if some objects aren't available.

It is interesting to have `git upload-pack` support a similar way to avoid sending some objects in case they aren't available on the server.

For example, in case both the server and the client are using a separate promisor remote that contain some objects, it can be better if the server doesn't try to send such objects back to the client, but instead let the client get those objects separately from the promisor remote. (The client needs to have the separate promisor remote configured, for that to work.)

This could work better if there was something, like perhaps a capability, for the client to tell the server something like:

  "I know how to fetch missing objects from this and that
  promisor remotes, so if you choose to, you may omit objects
  that you know are available from these promisor remotes when
  sending objects to me."

But that capability could be added later as other similar capabilities in this area could be very useful. For example in case of a client cloning, something like the following is currently needed:

  GIT_NO_LAZY_FETCH=0 git clone
      -c remote.my_promisor.promisor=true \
      -c remote.my_promisor.fetch="+refs/heads/*:refs/remotes/my_promisor/*" \
      -c remote.my_promisor.url=<MY_PROMISOR_URL> \
      --filter="blob:limit=5k" server

But it would be nice if there was a capability for the client to say that it would like the server to give it information about the promisor that it could use, so that the user doesn't have to pass all the "remote.my_promisor.XXX" config options on the command like. (It would then be a bit similar to the bundle-uri feature where all the bundle related information comes from the server.)

Another example use of this feature could be a server where some objects have been corrupted or deleted. It could still be useful for clients who could get those objects from another source, like perhaps a different client, to be able to fetch or clone from the server.

The fact that the new `uploadpack.missingAction` configuration variable has to be set to a non default value on the server means that regular client users cannot hurt themselves with this feature.

As `git rev-list` also supports a `--missing=<missing-action>` option, the first 2 patches in this series are about refactoring related code from both `git rev-list` and `git pack-objects` into new "missing.{c,h}" files. Patch 3/3 then adds a new `uploadpack.missingAction` configuration variable.

The changes since v1 are the following:

  - In patch 1/3 the refactored parse_missing_action_value() function
    doesn't change the fetch_if_missing global variable anymore. The
    clients of this function change it by themselves instead.

  - In patch 1/3 a minor typo was also fixed.

  - Old patch 2/4 that added a 'int print_ok' argument to
    parse_missing_action_value() was removed, and now callers deal
    with only the values they recognise.

Thanks to Junio for his review and for suggesting of the above changes.

Christian Couder (3):
  rev-list: refactor --missing=<missing-action>
  pack-objects: use the missing action API
  upload-pack: allow configuring a missing-action

 Documentation/config/uploadpack.txt |   9 ++
 Makefile                            |   1 +
 builtin/pack-objects.c              |  48 ++++++-----
 builtin/rev-list.c                  |  43 ++--------
 missing.c                           |  36 ++++++++
 missing.h                           |  19 +++++
 t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++
 upload-pack.c                       |  19 +++++
 8 files changed, 242 insertions(+), 58 deletions(-)
 create mode 100644 missing.c
 create mode 100644 missing.h
 create mode 100755 t/t5706-upload-pack-missing.sh

Both `git rev-list` and `git pack-objects` support a `--missing=<missing-action>` feature, but they currently don't share any code for that.

Refactor the support for `--missing=<missing-action>` in "builtin/rev-list.c" into new "missing.{c,h}" files. In a following commit, that refactored code will be used in "builtin/pack-objects.c" too.

In yet a following commit, we are going to add support for a similar 'missing-action' feature to another command, and we are also going to reuse code from the new "missing.{c,h}" files.

As `enum missing_action` and parse_missing_action_value() are moved to "missing.{c,h}", we need to modify the latter a bit, so that it stops updating any global variable, but instead returns the parsed value or -1 on error.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile           |  1 +
 builtin/rev-list.c | 43 ++++++++-----------------------------------
 missing.c          | 20 ++++++++++++++++++++
 missing.h          | 17 +++++++++++++++++
 4 files changed, 46 insertions(+), 35 deletions(-)
 create mode 100644 missing.c
 create mode 100644 missing.h

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

All makes sense.  Will queue.  Thanks.

Both `git rev-list` and `git pack-objects` support a `--missing=<missing-action>` option. Previous commits created an API in "missing.{c,h}" to help supporting that option, but only `git rev-list` has been using that API so far.

Let's make `git pack-objects` use it too.

This involves creating a new show_object_fn_from_action() function to set the `fn_show_object` variable independently from parsing the missing action, which is now performed by the parse_missing_action_value() API function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/pack-objects.c | 48 ++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

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

Interesting. This used to be private to this file, shared the same name and most of the values with the one used in rev-list, but not identical (i.e. the new "missing" API knows about MA_PRINT but this side has been unaware of that value).

As this is BUG() to catch programming error, ("%d" % action) is OK; if this were end-user facint, we would also want to pass the "arg" string the caller had only for error reporting.

What is our expectation for how <missing.h> API would evolve over time? I think it is a given that it will always be a superset of the need of rev-list and the need of pack-objects, but if we were to add a new value of MA_FOO, do we expect that all of the new ones are not handled by pack-objects, Some but not all? Or none of the new ones are handled by pack-objects?

Regardless of the answer to that question, I think a simple helper is warranted here, which will also help the [3/3] which adds exactly the same code to upload-pack.c:upload_pack_config(), so that the callers can do

	res = parse_missing_action_value_for_packing(arg);
	if (res < 0)
        	die(_("invalid value for '%s': '%s'"), "--missing", arg);

something like

	int parse_missing_action_value_for_packing(const char *arg)
	{
		int res = parse_missing_action_value(arg);

                if (res < 0)
                	return res;

		switch (res) {
		case MA_ERROR:
		case MA_ALLOW_ANY:
		case MA_ALLOW_PROMISOR:
			return res;
		default:
                	return -2 - res;
		}
	}

here, and also in the other place [3/3] adds. This thin wrapper returns:

	0 <= res : MA_FOO values that are OK for packing
	-1 = res : parse_missing_action_value() failed
	-1 > res : (2 - res) is the MA_FOO which is unsuitable for packing

to allow the caller to recover which value the user gave us that is unsuitable for packing, if it wanted to.

Hmph, wouldn't a small array of show_object_fn suffice, making the whole thing more like:

	static show_object_fn const fn[] = {
		[MA_ERROR] = show_object,
		[MA_ALLOW_ANY] = show_object__ma_allow_any,
		[MA_ALLOW_PROMISOR] = show_object__ma_allow_promisor,
	};

	res = parse_missing_action_value_for_packing(arg);
	if (res < 0 || ARRAY_SIZE[fn] <= res)
        	die(_("invalid value for '%s': '%s'"), "--missing", arg);
	fn_show_object = fn[res];
	return 0;

without the need for show_object_fn_from_action() helper function?

Other than that, the intention of the code is very clear.

Will queue.  Thanks.

On Wed, May 15, 2024 at 6:46 PM Junio C Hamano <gitster@pobox.com> wrote:

I don't think the <missing.h> API would evolve much over time. At least I don't think we have plans to make it evolve. Perhaps other options similar to MA_PRINT could be added though, maybe MA_TRACE or MA_LOG. Maybe such a new option could be handled by pack-object, maybe not. I think for now it's better to be flexible and not guess too much.

Ok I implemented this in the v3 I just sent.

Ok with the small array. I implemented it in v3 too.

Thanks for the suggestions.

From: Christian Couder <chriscool@tuxfamily.org>

In case some objects are missing from a server, it might still be useful to be able to fetch or clone from it if the client already has the missing objects or can get them in some way.

For example, in case both the server and the client are using a separate promisor remote that contain some objects, it can be better if the server doesn't try to send such objects back to the client, but instead let the client get those objects separately from the promisor remote. (The client needs to have the separate promisor remote configured, for that to work.)

Another example could be a server where some objects have been corrupted or deleted. It could still be useful for clients who could get those objects from another source, like perhaps a different client, to be able to fetch or clone from the server.

To configure what a server should do if there are such missing objects, let's teach `git upload-pack` a new `uploadpack.missingAction` configuration variable.

This new missing-action works in a similar way as what `git rev-list` and `git pack-objects` already support with their `--missing=<missing-action>` command line options. In fact when `uploadpack.missingAction` is set to something different than "error", `git upload-pack` will just pass the corresponding `--missing=<missing-action>` to `git pack-objects`.

So this new missing-action has the same limitations as `git pack-objects --missing=<missing-action>`. Especially when not using promisor remotes, 'allow-any' works only for blobs.

Another limitation comes from `git upload-pack` itself which requires setting `GIT_NO_LAZY_FETCH` to 0 since 7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16) for lazy fetching from a promisor remote to work on the server side.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/uploadpack.txt |   9 ++
 missing.c                           |  16 ++++
 missing.h                           |   2 +
 t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++
 upload-pack.c                       |  19 +++++
 5 files changed, 171 insertions(+)
 create mode 100755 t/t5706-upload-pack-missing.sh

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

Be more assertive. We do not want to add a new feature randomly only because it _might_ be useful to somebody in a strange and narrow use case that _might_ exist.

Is it "it can be better", or is it "it is always better"? Pick an example that you can say the latter to make your example more convincing.

Repository S borrows from its "promisor" X, and repository C which initially cloned from S borrows from its "promisor" S. Even if C wants an object in order to fill in the gap in its object graph, S may not have it (and S itself may have no need for that object), and in such a case, bypassing S and let C go directly to X would make sense.

I am puzzled by this new option.

It feels utterly irresponsible to give an option to set up a server that essentially declares: I'll serve objects you ask me as best efforts basis, the pack stream I'll give you may not have all objects you asked for and missing some objects, and when I do so, I am not telling you which objects I omitted.

How do you ensure that a response with an incomplete pack data would not corrupt the repository when the sending side configures upload-pack with this option? How does the receiving end know which objects it needs to ask from elsewhere?

Or is the data integrity of the receiving repository is the responsibility of the receiving user that talks with such a server? If that is the case, I am not sure if I want to touch such a feature even with 10ft pole.

Is there anything the sender can do but does not do to help the receiver locate where to fetch these missing objects to fill the "unfilled promises"?

For example, the sending side _could_ say that "Sorry, I do not have all objects that you asked me to---but you could try these other repositories".

On Wed, May 15, 2024 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:

Ok, I have changed "it might still be useful" to "it is sometimes useful" in the v3 I just sent.

Ok, I use your example above in v3.

I don't think it's irresponsible. The client anyways checks that it got something usable in the same way as it does when it performs a partial fetch or clone. The fetch or clone fails if that's not the case. For example if the checkout part of a clone needs some objects but cannot get them, the whole clone fails.

Git already has support for multiple promisor remotes. When a repo is configured with 2 promisor remotes, let's call them A and B, then A cannot guarantee that B has all the objects that are missing in A. And it's the same for B, it cannot guarantee A has all the objects missing in B. Also when fetching or cloning from A for example, then no list of missing objects is transfered.

Yes, it's the same as when a repo is configured with multiple promisor remotes.

When using bundle-uri, it's also the responsibility of the receiving user to check that the bundle it gets from a separate endpoint is correct.

Also when only large blobs are on remotes except for the main remote, then, after cloning or fetching from the main remote, the client knows the hashes of the objects it should get from the other remotes. So there is still data integrity.

To give you some context, at GitLab we have a very large part of the disk space on our servers taken by large blobs which often don't compress well (see https://gitlab.com/gitlab-org/gitaly/-/issues/5699#note_1794464340). It makes a lot of sense for us at GitLab to try to move those large blobs to some cheaper separate storage.

With repack --filter=... we can put large blobs on a separate path,
but it has some drawbacks:
  - they are still part of a packfile, which means that storing and
accessing the objects is expensive in terms of CPU and memory (large
binary files are often already compressed and might not delta well
with other objects),
  - mounting object storage on a machine might not be easy or might
not perform as well as using an object storage API.

So using a separate remote along with a remote helper for large blobs makes sense. And when a client is cloning, it makes sense for a regular Git server, to let the client fetch the large blobs directly from a remote that has them.

In the cover letter of this v2 and the v3, I suggested the following:

--- For example in case of a client cloning, something like the following is currently needed:

  GIT_NO_LAZY_FETCH=0 git clone
      -c remote.my_promisor.promisor=true \
      -c remote.my_promisor.fetch="+refs/heads/*:refs/remotes/my_promisor/*" \
      -c remote.my_promisor.url=<MY_PROMISOR_URL> \
      --filter="blob:limit=5k" server

But it would be nice if there was a capability for the client to say that it would like the server to give it information about the promisor that it could use, so that the user doesn't have to pass all the "remote.my_promisor.XXX" config options on the command like. (It would then be a bit similar to the bundle-uri feature where all the bundle related information comes from the server.) ---

So yes we are thinking about adding such a way to "help the receiver locate where to fetch these missing objects" soon. We needed to start somewhere and we decided to start with this series, because this patch series is quite small and let us already experiment with offloading blobs to object storage (see https://gitlab.com/gitlab-org/gitaly/-/issues/5987).

Also the client will anyway very likely store the information about where it can get the missing objects as a promisor remote configuration in its config. So after the clone, the resulting repo will very likely be very similar as what it would be with the clone command above.

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

But then what can the repository C do after seeing such a failure?

With the design, S does not even consult C to see if C knows about X. Without knowing that, it cannot safely decide that it does not have to send objects that can be obtained from X to C. Instead, S simply say "if C requests an object that I do not have, just ignore it and let C grab it from somewhere else". How would it not be an irresponsible design?

On Fri, May 24, 2024 at 11:51 PM Junio C Hamano <gitster@pobox.com> wrote:

It's basically the same as when a regular clone or a partial clone or a clone using bundle-uri fails or when using a regular bundle fails. If it failed because the remote was not properly configured, then that config can be fixed. If it fails because the remote doesn't have some objects, then maybe the missing objects can be transferred to the remote. And so on.

The feature doesn't create any new kind of failure. In particular, when you use a partial clone, even a very simple one with a single remote, there is always the risk of not being able to get some missing objects as there is the risk of the remote being unreachable for some reason (like if you take a plane and don't have an internet connection, or if there is an outage on the server side). There are some added risks because the feature requires added configuration and it can be wrong like any configuration, and because there are 2 remotes instead of just one. But these are not new kinds of risks. These risks already exist if one uses multiple promisor remotes.

If S is managed by a company like GitLab or GitHub, then S will certainly advertise, for example by showing a command that can easily be copy-pasted from the web page of the project onto the user's command line, some way for C to use X.

In the cover letter I give the example of the following command that can be used (and advertised by S):

  GIT_NO_LAZY_FETCH=0 git clone
      -c remote.my_promisor.promisor=true \
      -c remote.my_promisor.fetch="+refs/heads/*:refs/remotes/my_promisor/*" \
      -c remote.my_promisor.url=<MY_PROMISOR_URL> \
      --filter="blob:limit=5k" server

I also agree in the cover letter that this is not the most user friendly clone command and I suggest that I could work on improving on that by saying:

"it would be nice if there was a capability for the client to say that it would like the server to give it information about the promisor that it could use, so that the user doesn't have to pass all the "remote.my_promisor.XXX" config options on the command like."

and by saying that this could be added later.

If you think that such a capability should definitely be part of this work, for example because it wouldn't be sane to require users to use such a long and complex command and it could avoid difficult to debug failures, then I would be willing to work on this and add it to this series.

In the above command C is asking for a partial clone, as it uses a --filter option. This means that C knows very well that it might not get from S all the objects needed for a complete object graph. So why can't S safely decide not to send some objects to C? Why would it be Ok if C wanted a partial clone but didn't want to get some objects from X at the same time, but would not be Ok if C wants the same partial clone but also with the possibility to get some of the objects from X right away? To me it seems less risky to ask for some objects from X right away.

Again when using a regular partial clone omitting the same set of objects, C also requests some objects that S doesn't have. And this is not considered an issue or something irresponsible. It already works like this. And then C still has the possibility to configure X as a promisor remote and get missing objects from there. So why is it Ok when it's done in several steps but not in one?

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

Can such an update happen transparently, or does it need changes to end-user experience? It is of dubious value to leave the initial series be incomplete if the latter is the case.

Hmph. If C asks a partial clone and S is willing to be the promisor for C, S is essentially saying that it will serve C any objects on demand that are reachable from any object it served C in the past, forever, no? It might not get from S initially all the objects, but if it wants later, S promises to let C have them.

"S doesn't have" is because S has pruned objects that it shouldn't have in order to keep the promise it made earlier to C, right? If that is the case, I would very much say S is being irresponsible in that case.

You are right that S can decide to unilaterally break the promise it made C, so this update is not making it any worse than giving users a broken implementation of promisor remotes. I wouldn't call it OK, though.

If somebody identifies that even without this series, S can lead to repository corruption at C by pruning objects it does need to keep its promise to C, the next action I expect from healthy project is to try coming up with a mechanism to make it less likely that such a pruning happens by accident (e.g., by noticing allowAnySHA1InWant as a sign that the repository has promised others to serve anything that used to be reachable from anything it historically served, disabling repack "-d" and instead send the currently unreachable objects to an archived pack, and something equally silly like that). It certainly is not to add a new mechanism to make it even easier to configure S to break promised it made to C.

So, I dunno.

On Tue, May 28, 2024 at 5:54 PM Junio C Hamano <gitster@pobox.com> wrote:

I think adding the capability could happen transparently. I think the capability should be a generic one about the server suggesting some config options and perhaps some command line options, and the client agreeing (or not) to setting the config options and using the command line options that the server suggests. Of course the important thing when implementing this capability is to properly limit the config and command line options (and perhaps their values) that the client accepts to set and use. But I think it could be a generic capability that could be extended to other options in the future. That's why I think it could be a separate patch series.

This promise is not broken, as S can still get the missing objects from X and then pass them to C. There is even the following test case in the patch that shows that it works when uploadpack.missingAction is unset (and thus default to "error" which is the same as how things currently work):

+test_expect_success "fetch without uploadpack.missingAction=allow
-promisor" '
+       git -C server config --unset uploadpack.missingAction &&
+
+       # Clone from server to create a client
+       GIT_NO_LAZY_FETCH=0 git clone -c remote.server2.promisor=true \
+               -c remote.server2.fetch="+refs/heads/*:refs/remotes/server2/*" \
+               -c remote.server2.url="file://$(pwd)/server2" \
+               --no-local --filter="blob:limit=5k" server client &&
+       test_when_finished "rm -rf client" &&
+
+       # Check that the largest object is not missing on the server anymore
+       check_missing_objects server 0 ""
+'

The only difference with the case when uploadpack.missingAction is set to "allow-promisor" is that then C gets the missing objects directly from X without S getting them first. This is why there is:

+       # Check that the largest object is still missing on the server
+       check_missing_objects server 1 "$oid"

at the end of the test case when uploadpack.missingAction is set to "allow-promisor". This is to check that S didn't get the missing object (which means that the missing object went directly from X to C).

So S keeps its promise to let C have any object through S if C wants. It's just that having large objects through S (instead of directly from X) is not a good idea because it copies the large objects to S first which uses up space (and memory and CPU when creating packfiles) on S while the objects are already on X where disk space is probably cheaper (and where less CPU and memory will be used to send the objects).

Yes, S has pruned some objects, but these objects have been moved to X first and S has access to X, so S can still get the objects. So I don't understand why you say S shouldn't have pruned them and S is irresponsible. S is still capable of fulfilling the promise it made.

And even if S was not capable of directly fulfilling the promise it made, it would still not be clear to me that S is irresponsible, as S and X can be seen as a single entity from the client point of view, and S and X together would still be capable of fulfilling the promise that was made to C.

When bundle-uri is used, for example, the server is not fulfilling the promise alone. It needs the bundle server to work properly for the client to get everything. When using Git LFS, the server is not fulfilling the promise alone either. For things to work properly both the Git server and the LFS server have to work together.

So if it's Ok for other features to require an additional server to fulfill the promise, why is it not Ok in the case of S + X?

I don't understand why you compare this to a "broken" implementation of promisor remotes. What could then be a non-broken one that would store large blobs on a separate server in your opinion? I am really interested in answers to this question. It's not a rhetorical one.

Objects are not pruned if you consider S + X instead of just S. It's the same as when people are using Git LFS or bundle-uri. Nothing is really pruned on the server side, no promise is broken. Some objects are just moved to a separate server that is sometimes called to provide some objects on top of those the Git server still sends.

When people managing server S decide to move some objects to X and make S access X, it's not accidental pruning. But maybe you are talking about the case when uploadpack.missingAction is set to "allow-any" instead of "allow-promisor"?

This new mechanism has some important benefits. Yes, there are risks, but it's our opinion at GitLab, with some data from GitLab servers (see the link I previously sent to a GitLab issue) to back it up, that the rewards are well worth the added risks.

I agree that ideally a Git server should be able to handle every kind of object very well, but that's unfortunately not the case. Git LFS has some drawbacks, bundle-uri also has some drawbacks, but there are reasons why they exist. Multiple promisor remote is a solution that already exists in Git too. Yes, it also has some drawbacks, but they are in many ways similar to those of bundle-uri and Git LFS. Yes, this patch is making multiple promisor remotes easier to use, but it doesn't fundamentally change the risks associated with using multiple promisor remotes.

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

And the whole point of that configuration is to make it easier for S to break that promise, no? uploadPack.missingAction is set at S and is not under control of C, right?

I am questioning the design that does not give C any say in the decision if it is a good idea or not to ask S relay large objects. S unilaterally decides that it does not want to and does not serve such large objects to C, and without even checking with C if it can reach X to fetch directly, silently assuming that C will do so, right? It is quite different from the contract between C and S in the simpler world.

You as S would tell C "I want you to go to X because I am not serving objects X and Y". Or at least make sure that C knows about X before deciding to omit what X ought to have. Not doing anything and silently assuming that C will get them from elsewhere is simply irresponsible, especially if C is not even told about X over the protocol, no?

On Sat, Jun 1, 2024 at 11:43 AM Junio C Hamano <gitster@pobox.com> wrote:

Ok, so if there was a way for S to suggest config options and perhaps also command line options to C (like I mentioned in a previous email), and if S would suggest C to configure X as a promisor remote and C would accept to do that, then it would be fine for the feature to work, right?

Alternatively, if C would pass a new option called for example --known-promisor=X on top of all other options, then that could be Ok too?

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

Why do you need a new option "--known-promisor=X" in the first place? Doesn't it known by the repository already and you do not have to bother the end user to supply it from the command line, no?

In any case, a protocol extension that lets S tell C that S wants C to fetch missing objects from X (which means that if C knows about X in its ".git/config" then there is no need for end-user interaction at all), or a protocol extension that C tells S that C is willing to see objects available from X omitted when S does not have them (again, this could be done by looking at ".git/config" at C, but there may be security implications???). Either would work OK and I offhand do not see much preference between the two.

What is important is to ensure that such deliberate omission by S is safe and acceptable by C.

Thanks.

[Footnote]

By the way, I briefly wondered: would "X" that is valid for "C" be always valid for "S" these days?

I as "S" may prefer you as "C" to go ssh://gitlab.com/x.git but you may only be able to go https://gitlab.com/x.git for networking reasons. Silly situation like that may make communicating "X" between "S" and "C" a bit harder than we would like. Of course this is not a high priority issue as the user with such networking need are probably already using url.X.insteadof to rewrite the submodule paths recommended in .gitmodules after they clone a superproject, iow, even if it were a problem, it is a solved one.

So, in short, yes, exchanging the known promisor "X" between "S" and "C", regardless of which side speaks first, as long as the negotiation makes sure both sides agree, would be a good direction to go, I would think.

Sorry everyone, it looks like I put the wrong message Id in the --in-reply-to= option of my `git send-email` command and this appears to be part of a different thread. The right message ID should have been:

20240418184043.2900955-1-christian.couder@gmail.com

Sorry again.

On Wed, May 15, 2024 at 3:25 PM Christian Couder <christian.couder@gmail.com> wrote: