Re: [PATCH v7 00/16] Parial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 11, 2017, 19:19 UTC
Message-ID: <20171211111916.b3ea2deacba67f6e8416d285@google.com>
In-Reply-To: <20171208223010.GF140529@google.com>

On Fri, 8 Dec 2017 14:30:10 -0800 Brandon Williams <bmwill@google.com> wrote:

Show 5 quoted lines

> I just finished reading through parts 1-3.  Overall I like the series.
> There are a few point's that I'm not a big fan of but i wasn't able to
> come up with a better alternative.  One of these being the need for a
> global variable to tell the fetch-object logic to not go to the server
> to try and fetch a missing object.

I didn't really like that approach too but I went with that because, like you, I couldn't come up with a better one. The main issue is that too many functions (e.g. parse_commit() in commit.c) indirectly read objects, and I couldn't find a better way to control them all. Ideally, we should have a "struct object_store" (or maybe "struct repository" could do this too) on which we can set "fetch_if_missing", and have all object-reading functions take a pointer to this struct. Or completely separate the object-reading and object-parsing code (e.g. commit.c should not be able to read objects at all). Or both.

Any of these would be major undertakings, though, and there are good reasons for why the same function does the reading and parsing (for example, parse_commit() does not perform any reading if the object has been already parsed).

Show 7 quoted lines

> One other thing i noticed was it looks like when you discover that you
> are missing a blob you you'll try to fault it in from the server without
> first checking its an object the server would even have.  Shouldn't you
> first do a check to verify that the object in question is a promised
> object before you go out to contact the server to request it?  You may
> have already ruled this out for some reason I'm not aware of (maybe its
> too costly to compute?).

It is quite costly to compute - in the worst case, we would need to read every object in every promisor packfile of one or more certain types (e.g. if we know that we're fetching a blob, we need to read every tree) to find out if the object we want is a promisor object.

Such a check would be better at surfacing mistakes (e.g. the user giving the wrong SHA-1) early, but beyond that, I don't think that having the check is very important. Consider these two very common situations:

 (1) Fetching a single branch by its tip's SHA-1. A naive implementation
     will first check if we have that SHA-1, which triggers the dynamic
     fetch (since it is an object read), and assuming success, notice
     that we indeed have that tip, and not fetch anything else. The
     check you describe will avoid this situation.
 (2) Dynamically fetching a missing blob by its SHA-1. A naive
     implementation will first check if we have that SHA-1, which
     triggers the dynamic fetch, and that fetch will first check if we
     have that SHA-1, and so on (thus, an infinite loop). The check you
     describe will not avoid that situation.

The check solves (1), but we still need a solution to (2) - I used "fetch_if_missing", as discussed in your previous question and my answer to that. A solution to (2) is usually also a solution to (1), so the check wouldn't help much here.