patchindex-pack: remove fetch_if_missing=0

A collision test is triggered in sha1_object(), whenever there is an object file in our repo. If our repo is a partial clone, then checking for this file existence has the behavior of lazy-fetching the object because we have one or more promisor remotes.

This behavior is controlled by setting fetch_if_missing to 0, but this global was added in the first place as a temporary measure to suppress the fetching of missing objects and can be removed once the commands have been taught to handle these cases.

Hence, use has_object() to check for the existence of an object, which has the default behavior of not lazy-fetching in a partial clone.

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 builtin/index-pack.c     | 11 +----------
 t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 10 deletions(-)

Waiting for review.

Thanks

Kousik Sanagavarapu <five231003@gmail.com> writes:

Hmm...this is not true, because (as you said)...
 

...this makes it so that we don't fetch in this situation.

Yes, that's true.

I think that the author of such a commit (you) should also independently verify that this comment is true (and if it is, then yes, all the remaining cases are handled and we can remove this assignment to fetch_if_missing). I believe this comment to be true, but I haven't checked the code in a while so I'm not sure myself.

How does this check that no lazy fetch has occurred? It seems to me that you're just checking the existence of a file in the worktree, which does not indicate the presence or absence of a lazy fetch.

I think the way to test needs to be more complicated: you need to create a partial clone, fetch into it from another repo, and then verify that no fetches were made to the original partial clone.

On Tue, 28 Feb 2023 at 03:44, Jonathan Tan <jonathantanmy@google.com> wrote:

Yes, that statement is false if fetch_if_missing is set to 0. But my original thought in writing it was so that the anyone who is reading the commit message understands the motivation as to why we are setting fetch_if_missing to 0.

It seems indeed that this is the only place where lazy-fetching is possible. I checked this by looking up the calls for oid_object_info_extended() or any other function in object-file.c which depends on it.

In builtin/index-pack.c, we have (in the order that these functions appear)

- check_object()
    Call to oid_object_info(), but we return early with
    0 if we don't have an object.

- sha1_object()
    Call to has_object_file_with_flags() (which this patch replaces with
    has_object()), where lazy-fetching is possible.
    
    Calls to oid_object_info() and read_object_file(), which trigger only
    when the above has_object_file_with_flags() succeeds.

- fix_unresolved_deltas()
    Call to oid_object_info_extended(), we prefetch delta bases.

    Call to read_object_file(), but we only read data from ref_delta_entry.
    In case it was a delta base, we already prefetched it.

There are cases where we fsck objects, but lazy-fetching is already handled in fsck (although by setting fetch_if_missing to 0).

Do we need to be explicit about this in the commit message? That sha1_object() is the only place where there is a chance to lazy-fetch if it is a partial clone?

What I had in mind was if the file was lazy-fetched (because of the failure of has_object_file_with_flags() and fetch_if_missing not set to 0), then it would be unpacked and we would find it in the worktree. Since, we prevent this exact behavior by using has_object(), we should not find such a file in our repo.

So, after the fetch, during the pack indexing phase, we look for any additional fetches made. This makes more sense and it would be way more clear, to anyone reading, than what I wrote.

Will do a reroll. If there needs to be a change in the commit message as well, please let me know.

Thanks for the review

A collision test is triggered in sha1_object(), whenever there is an object file in our repo. If our repo is a partial clone, then checking for this file existence does not lazy-fetch the object (if the object is missing and if there are one or more promisor remotes) when fetch_if_missing is set to 0.

This global was added as a temporary measure to suppress the fetching of missing objects and can be removed once the commandshave been taught to handle these cases.

Hence, use has_object() to check for the existence of an object, which has the default behavior of not lazy-fetching in a partial clone. It is worth mentioning that this is the only place where there is potential for lazy-fetching and all other cases are properly handled, making it safe to remove this global here.

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---

Sorry for the late reroll, I was having semester-end exams.

Changes since v1:
- Changed the commit message to be more clear about the
  change done here.

- Changed the test according to the previous review.

 builtin/index-pack.c     | 11 +----------
 t/t5616-partial-clone.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 10 deletions(-)

Kousik Sanagavarapu <five231003@gmail.com> writes:

Two requests.

 * Could you substantiate this claim for future readers of "git
   log"?  A reference to an old mailing list discussion or a log
   message of the commit that added the temporary measure that says
   the above plan would be perfect.

 * What exactly does "once the commands have been taught"?  Which
   commands?  Could you clarify?

This paragraph is very well explained.

OK. The comment describes the design choice we made to flip the fetch_if_missing flag off. The old world-view was that we would notice a breakage by non-functioning index-pack when a lazy clone is missing objects that we need by disabling auto-fetching, and we instead explicitly handle any missing and necessary objects by lazy fetching (like "when we lack REF_DELTA bases"). It does sound like a conservative thing to do, compared to the opposite approach we are taking with this patch, i.e. we would not fail if we tried to access objects we do not need to, because we have lazy fetching enabled, and we just ended up with bloated object store nobody may notice.

To protect us from future breakage that can come from the new approach, it is a very good thing that you added new tests to ensure no unnecessary lazy fetching is done (I am not offhand sure if that test is sufficient, though).

Looking good to me. Jonathan, who reviewed the previous round, do you have any comments?

Thanks, all.  Will queue.

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

It might be good if the "all other cases" were enumerated here in the commit message (since the consequence of missing a case might be an infinite loop of fetching).

I don't think the test is sufficient - I'll explain that below.

The file5 isn't committed?

It seems to me that this test clones a repo and then attempts to fetch from another repo: so far, so good. But I don't think this tests what we want: firstly, the file5 isn't committed, so it is never fetched. And even if it was, we only check that file3 was never fetched from "$(pwd)/ server". But file3 has nothing to do with the subsequent fetch: we are only fetching file5. It is the hash of file5 that we are checking for collisions, and thus it is file5 that we want to verify is not fetched.

So I think the way to do this is to have 3 repositories like the author
is doing now (server, client, and repo), and do it as follows:
 - create "server", one commit will do
 - clone "server" into "client" (partial clone)
 - clone "server" into "another-remote" (not partial clone)
 - add a file ("new-file") to "server", commit it, and pull from "another-remote"
 - fetch from "another-remote" into "client"

This way, "client" will need to verify that the hash of "new-file" has no collisions with any object it currently has. If there is no bug, "new-file" will never be fetched from "server", and if there is a bug, "new-file" will be fetched.

One problem is that if there is a bug, such a test will cause an infinite loop (we fetch "new-file", so we want to check it for collisions, and because of the bug, we fetch "new-file" again, which we check for collisions, and so on) which might be problematic for things like CI. But we might be able to treat timeouts as the same as test failures, so this should be OK.

Jonathan Tan <jonathantanmy@google.com> writes:

I admit I haven't thought about it any longer than anybody who touched this topic, but should "fetch_if_missing=0" really be treated as "it was a dirty hack in the past, now we do not need it, as all callers into the object layer avoids lazy fetching when they do not have to, so let's remove it"? It looks to me more and more that the old world-view to disable lazy fetching by default and have individual calls to the object layer opt into fetching as needed may give us a better resulting code, or is it just me? The possible error modes in new code that fails to follow the world-view with and without this change are:

 * If the lazy fetching is disabled by default (i.e. without this
   patch), a new code can by mistake call has_object(), which does
   not lazy fetch, when it does need to have the object and should
   be using something like has_object_file_with_flags(), and dies
   loudly.

 * If the lazy fetching is enabled by default, on the other hand, a
   new code can by mistake call has_object_file_with_flags(), which
   does lazy fetch, when it does not need to have the object.  It
   does not die, it just lazily fetches objects it does not need.
   The (performance) "bug" will stay hidden until somebody complains.

In short, the world-view of the current code seems to give us tighter control over what gets lazy fetched, simply because we do not allow lazy fetching without thinking.

Do we have other uses of fetch_if_missing (i.e. disable lazy fetching)?

    $ git grep -l fetch_if_missing
    Documentation/technical/partial-clone.txt
    builtin/fetch-pack.c
    builtin/fsck.c
    builtin/pack-objects.c
    builtin/prune.c
    builtin/rev-list.c
    cache.h
    midx.c
    object-file.c
    revision.c

As the default is 1, all these hits (outside the header, doc, and object-file.c) are to disable lazy fetching. Judging from the list of "family" that want tighter control over what gets fetched, I have a feeling that pack-index may want to stay to be in the family.

Or am I missing some big picture goal to eventually getting rid of this mechanism and always allowing lazy fetching?

Thanks.

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

I think this "family" concept is a good way to think of it. I did use to think that it would be better to be consistent throughout Git and choose one world-view, and if I had to choose, it would be the one without fetch_if_missing=0. But now it does make sense to me to have two families:

 (a) The more low-level code that the lazy fetching itself relies on
     (and maybe things like builtin/fsck.c as well) where we really need
     to be careful about what we fetch, and it would be better to err
     on the side of not fetching. The test cases for these would need to
     cover both the partial clone cases and the regular cases.

     For these cases, the consequence of lazy-fetching when we shouldn't
     might be as bad as an infinite loop, so it makes sense to default
     not lazy-fetching here.

 (b) The more high-level code, in which I think that it is better to err
     on the side of fetching. The test cases would generally not need to
     cover the partial clone cases (except when there are specific
     optimizations needed, such as in checkout where we bulk prefetch
     missing objects).

     For these cases, the consequences of lazy-fetching when we shouldn't
     are generally performance-related, so it might not be so bad to let
     development happen in these areas of code without great
     consideration to whether a lazy-fetch would happen if an object
     didn't exist. (I do think it would be ideal for all new code to pay
     attention to when they read objects, which would help not only in
     partial clone but also in a potential future in which we have non-
     disk object stores, but we're probably not there yet as a project.)

And indeed, pack-index would go in (a).

On Sat, 11 Mar 2023 at 03:11, Junio C Hamano <gitster@pobox.com> wrote:

I think having a single function to check for object existence, which is compatible with partial clones is better, because the end goal is to completely integrate the concept of partial clones with git's codebase and not have code that worries "Oh, there maybe bugs here because what if the user has a partial clone", everytime the code does an object existence check (that is, a call to has_object_file() or any of its related functions) and just have fetch_if_missing set to 1 or 0, according to the particular command.

It is also true that has_object() is not that "single function", because in cases where we are missing an object in partial clone and want it, has_object() has no way of fetching it. With or without flags (it only supports one flag which, when set, rechecks packed storage), it does not lazy-fetch in a partial clone. But there are cases where we need such objects, such as the commands that come into "family (b)" [1].

So, why not use oid_object_info_extended() directly, instead of wrapping it with some other function, whenever we are checking for an object's existence. We can skip lazy-fetches whenever we want with OBJECT_INFO_SKIP_FETCH_OBJECT and can also prefetch with OBJECT_INFO_FOR_PREFETCH [2].

[1] https://lore.kernel.org/git/20230311025906.4170554-1-jonathantanmy@google.com/

[2] pack-index itself is one example of where this is done.

    When we don't have REF_DELTA bases, we bulk prefetch them

	    if (has_promisor_remote()) {
                     /*
                      * Prefetch the delta bases.
                      */
                     struct oid_array to_fetch = OID_ARRAY_INIT;
		     for (i = 0; i < nr_ref_deltas; i++) {
                             struct ref_delta_entry *d = sorted_by_pos[i];
                             if (!oid_object_info_extended(the_repository, &d->oid,
                                                           NULL,
                                                           OBJECT_INFO_FOR_PREFETCH))
                                     continue;
                             oid_array_append(&to_fetch, &d->oid);
                     }
                     promisor_remote_get_direct(the_repository,
                                                to_fetch.oid, to_fetch.nr);
                     oid_array_clear(&to_fetch);
             }

    Instead of going object-by-object, which is basically like an
    infinite loop in large repos and partial clones are widely used
    in large repos.

On Sat, 11 Mar 2023 at 02:43, Jonathan Tan <jonathantanmy@google.com> wrote:

I will make the change.

That is a blunder.

So, we can lose the "promisor-remote" in the original test and make the "server" itself a promisor-remote?

Thanks for the review

On Sat, 11 Mar 2023 at 02:01, Junio C Hamano <gitster@pobox.com> wrote:

Will do the change.

Thanks.

It does seem that the tests need to be changed significantly. Will do.

Thanks

A collision test is triggered in sha1_object(), whenever there is an object file in our repo. If our repo is a partial clone, then checking for this file existence does not lazy-fetch the object (if the object is missing and if there are one or more promisor remotes) when fetch_if_missing is set to 0.

This global was added as a temporary measure to suppress the fetching
of missing objects [1] and can be removed once the remaining commands:
 - fetch-pack
 - fsck
 - pack-objects
 - prune
 - rev-list
can handle lazy-fetching without fetch_if_missing.

Hence, use has_object() to check for the existence of an object, which has the default behavior of not lazy-fetching in a partial clone. It is worth mentioning that this is the only place where there is potential for lazy-fetching and all other cases [2] are properly handled, making it safe to remove this global here.

[1] See 8b4c0103a9 (sha1_file: support lazily fetching missing objects,
		   2017-12-08)
[2] These cases are:
    - When we check objects, but we return with 0 early if the object
      doesn't exist.
    - We prefetch delta bases in a partial clone, if we don't have them
      (as the comment outlines).
    - There are some cases where we fsck objects, but lazy-fetching is
      already handled in fsck.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 builtin/index-pack.c     | 11 +----------
 t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 10 deletions(-)

Kousik Sanagavarapu <five231003@gmail.com> writes:

Thanks for this reference.

The way I read the "lazy fetching is by default not suppressed, and this is a temporary measure" described in the log message is quite opposite from where this patch wants to go, though.

I think the commit envisioned the world where all the accesses to the object layer are aware of the characteristics of a lazy clone (e.g. has_object() reporting "we do not have that object locally (yet)" is not an immediate sign of a repository corruption) and when to lazily fetch and when to tolerate locally missing objects is controlled more tightly, and to reach that world one step at a time, introduced the global, so that for now everybody lazily fetches, but the commands individual patches concentrates to "fix" can turn off the "by default all missing objects are lazily fetched" so that they can either allow certain objects to be locally missing, or fetch them from promisor remotes when they need the contents of such objects. By fixing each commands one by one, eventually we would be able to wean ourselves away from this "by default everything is lazily fetched" global---in other words, in the ideal endgame, the fetch_if_missing should be set to 0 everywhere.

So, if this patch was made in reaction to the "it was a temporary measure" in 8b4c0103 (sha1_file: support lazily fetching missing objects, 2017-12-08), I think it goes in the completely opposite direction. If the patch shared the cause with 8b4c0103 and wanted to help realize the ideal world, it instead should have left this command who can already work with fetch_if_missing=0 alone and fixed somebody else who still depends on fetch_if_missing=1, I think.

Now it is a separate issue to argue if "everybody knows exactly when to trigger lazy fetching and fetch_if_missing is set to false everywhere" is really the ideal endgame. I do not think "Future patches will update some commands to either tolerate missing objects or be more efficient in fetching them." proposed by the commit from late 2017 has seen that much advance recently.

But for commands that need to deal with many missing objects, enumerating the objects that are missing locally and need fetching first and then requesting them in a batch should be vastly more efficient than the default lazy fetch logic that lets the caller request a single object, realize it is missing locally, make a connection to fetch that single object and disconnect. So I have to suspect that ...

... this "can handle" may be a misguided direction to go in. They were taught not to lazy fetch because blindly lazy fetching was bad, weren't they?

Kousik Sanagavarapu <five231003@gmail.com> writes:

Thanks for the reference.

The way I read the "lazy fetching is by default not suppressed, and this is a temporary measure" described in the log message is quite opposite from where this patch wants to go, though.

I think the commit envisioned the world where all the accesses to the object layer are aware of the characteristics of a lazy clone (e.g. has_object() reporting "we do not have that object locally (yet)" is not an immediate sign of a repository corruption) and when to lazily fetch and when to tolerate locally missing objects is controlled more tightly, and to reach that world one step at a time, introduced the global, so that for now everybody lazily fetches, but the commands individual patches concentrates to "fix" can turn off the "by default all missing objects are lazily fetched" so that they can either allow certain objects to be locally missing, or fetch them from promisor remotes when they need the contents of such objects. By fixing each commands one by one, eventually we would be able to wean ourselves away from this "by default everything is lazily fetched" global---in other words, in the ideal endgame, the fetch_if_missing should be set to 0 everywhere.

So, if this patch was made in reaction to the "it was a temporary measure" in 8b4c0103 (sha1_file: support lazily fetching missing objects, 2017-12-08), I think it goes in the completely opposite direction. If the patch shared the cause with 8b4c0103 and wanted to help realize the ideal world, it instead should have left this command who can already work with fetch_if_missing=0 alone and fixed somebody else who still depends on fetch_if_missing=1, I think.

Now it is a separate issue to argue if "everybody knows exactly when to trigger lazy fetching and fetch_if_missing is set to false everywhere" is really the ideal endgame. I do not think "Future patches will update some commands to either tolerate missing objects or be more efficient in fetching them." proposed by the commit from late 2017 has seen that much advance recently.

But for commands that need to deal with many missing objects, enumerating the objects that are missing locally and need fetching first and then requesting them in a batch should be vastly more efficient than the default lazy fetch logic that lets the caller request a single object, realize it is missing locally, make a connection to fetch that single object and disconnect. So I have to suspect that ...

... this "can handle" may be a misguided direction to go in. They were taught not to lazy fetch because blindly lazy fetching was bad, weren't they?

A collision test is triggered in sha1_object(), whenever there is an object file in our repo. If our repo is a partial clone, then checking for this file existence does not lazy-fetch the object (if the object is missing and if there are one or more promisor remotes) when fetch_if_missing is set to 0.

Though this global lets us control lazy-fetching in regions of code, it prevents multi-threading [1].

Hence, use has_object() to check for the existence of an object, which has the default behavior of not lazy-fetching in a partial clone, even when fetch_if_missing is set to 1. It is worth mentioning that this is the only place where there is potential for lazy-fetching and all other cases [2] are properly handled, making it safe to remove this global here.

[1] See https://lore.kernel.org/git/xmqqv9sdeeif.fsf@gitster-ct.c.googlers.com/
    and the discussion that follows from it.

[2] These cases are:
    - When we check objects, but we return with 0 early if the object
      doesn't exist.
    - We prefetch delta bases in a partial clone, if we don't have them
      (as the comment outlines).
    - There are some cases where we fsck objects, but lazy-fetching is
      already handled in fsck.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---

Changes since v3:
- Changed the commit message to give a stronger reason as to
  why we should reduce the use of this global and in this
  process remove it from builtin/index-pack.c

- Also made an addition to the test which I overlooked last time
  and without which the does not make sense

 builtin/index-pack.c     | 11 +----------
 t/t5616-partial-clone.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

Kousik Sanagavarapu <five231003@gmail.com> writes:

Sorry, but I really do not see the point.

We already have read_lock/read_unlock to prevent multiple threads from stomping on the in-core object database structure either way.

If somebody needs to dynamically change the value of fetch_if_missing after the program started and spawned multiple threads, yes, the update to the single variable would become a problem point in multi-threading.

But that is not what we are doing, and you already discovered that this was done as "a temporary measure" to selectively let some programs use 0 and others use 1 for lazy-fetching, at a very early part of these programs.

If we are to reduce this global, perhaps we should teach more codepaths not to lazy fetch by default. Once everybody gets converted like so, then index-pack can lose the assignment of 0 to the variable, as the global variable would be initialized to 0 and nobody will flip it to 1 to "temporarily opt into lazy fetching by default until it gets fixed". At that point, we can lose the global variable.

So "we want to reduce the use of this global" is not a good reason to do this change at all, without a convincing argument that says why everybody should do automatic lazy fetching of objects. If everybody should avoid doing automatic lazy fetching, a good first step to reduce the use of this global is not to touch index-pack that has already been fixed not to do so, no?

On 18/03/23 04:28, Junio C Hamano wrote:

Thanks for the review.

Also, thanks for pointing out the direction of work in this area.

Really helpful.

Kousik Sanagavarapu <five231003@gmail.com> writes:

Any chance this fixes behavior[1] I was seeing where pushing from a depth=1, treeless clone was fetching content?

[1]: https://lore.kernel.org/git/m0lekp4rjv.fsf@epic96565.epic.com/T/#m83ba30c1bf91106fca61efff6619c491543034e4

-- Sean Allred

Sean Allred <allred.sean@gmail.com> writes:

If I understand correctly, Kousik's change is meant to be a clean-up that does not change any behaviour, so if it changes some behaviour for you, it means the patch is buggy.

But I think you can build a version of Git without the patch (earlier this week would have been a good time to do so to test the release candidate #2 for the upcoming 2.40) to see if you have the behaviour still, and then apply this patch to see if it fixes it, and then report your findings here. It would make a great contribution to the community.

Thanks.