> Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > > > There have been a few bugs wherein Git fetches missing objects whenever > > the existence of an object is checked, even though it does not need to > > perform such a fetch. To resolve these bugs, we could look at all the > > places that has_object_file() (or a similar function) is used. As a > > first step, introduce a new function has_object() that checks for the > > existence of an object, with a default behavior of not fetching if the > > object is missing and the repository is a partial clone. As we verify > > each has_object_file() (or similar) usage, we can replace it with > > has_object(), and we will know that we are done when we can delete > > has_object_file() (and the other similar functions). > > I wonder if we want to name the two (i.e. one variant that refuses > to go to network because it is trying to see if a lazy fetch is > needed, and the other that goes to network behind caller's back for > ease of use in a lazy clone) a bit more distinctly so that which one > could potentially go outside. > > Depending on one's view which one is _normal_ access pattern, giving > an explicit adverb to one variant while leaving the other one bland > might be sufficient. For example, I _think_ most of the places do > not want to handle the details of lazily fetching themselves, and I > suspect that the traditional has_object_file() semantics without "do > not trigger lazy fetch" option would be the normal access pattern. Right now, I think that most (if not all) places would not want to fetch at all - so *with* "do not trigger lazy fetch" would be the normal access pattern. This is because (in my opinion) if a caller checks the existence of an object, it most likely can tolerate the object's absence; if the caller couldn't tolerate it, it would just directly query for its type or contents or something like that. I tried to communicate this in my documentation of the deprecated functions/macros, but perhaps it could be written better. (One other option to consider is to just change has_object_file() to never fetch, although I think this is more risky.) > In which case, renaming your new "has_object" to something like > "has_object_locally()" would be a good name for a special case > codepath that wants to care---if the object does not exist locally > and needs to be obtained lazily from elsewhere, the function would > say "no". > > And all the other names like has_object_file() that by default gives > callers a transparent access to lazily fetched objects can stay the > same. If my analysis above is wrong, then yes I agree that we should do this. But we might need to find another way to indicate which has_object_file() has been checked and which hasn't - changing away from has_object_file() completely gives us a way to indicate this, but if we're sticking with has_object_file(), we have to find another way of indicating that we've looked at this call and it is OK. > > I mentioned the idea for this change here: > > https://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@xxxxxxxxxx/ > > Yup, I think that is going in a good direction. I suspect that > apply will not be the only remaining case we need to "fix", and > using the new helper function, codepaths that have already been > "fixed" by passing "do not lazily fetch" option to the traditional > API functions would become easier to read. And if that is the case, > let's have the introduction of the helper function as a separate > patch, with each of [PATCH 2-N/N] be a fix for separate codepaths. > > Thanks. OK - I'll separate out the helper function into its own patch in version 2.
