On Fri, May 31, 2019 at 7:10 AM Christian Couder
[off-list ref] wrote:

On Thu, May 30, 2019 at 7:21 PM Derrick Stolee [off-list ref] wrote:

quoted

On 4/9/2019 12:11 PM, Christian Couder wrote:

quoted

quoted

+{
+     int i, missing_nr = 0;
+     int *missing = xcalloc(oid_nr, sizeof(*missing));
+     struct object_id *old_oids = *oids;
+     struct object_id *new_oids;
+     int old_fetch_if_missing = fetch_if_missing;
+
+     fetch_if_missing = 0;

This global 'fetch_if_missing' swap seems very fragile. I'm guessing you are using
it to prevent a loop when calling oid_object_info_extended() below. Can you instead
pass a flag to the method that disables the fetch_if_missing behavior?

If such a flag existed when I wrote the function I would certainly
have used it, as I also dislike this kind of messing with a global
(and globals in general).

I will see if I can do something about it according to what you
suggest later in this thread.

In the V6 patch series I just sent, the new
OBJECT_INFO_SKIP_FETCH_OBJECT flag that you introduced is used.

quoted

quoted

+     for (i = 0; i < oid_nr; i++)
+             if (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {

A use of "the_repository" this deep in new code is asking for a refactor later to remove it.
Please try to pass a "struct repository *r" through your methods so we minimize references
to the_repository (and the amount of work required to remove them later).

Ok, I will take a look at that.

A "struct repository *r" is passed in V6. I forgot to mention that in
the cover letter.

quoted

quoted

+                     missing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);

Here is the one call, and after this assignment "missing_nr" does mean the number of missing objects.
However, I do think this could be clarified by using remaining_nr and remaining_oids.

Ok, I will take a look at using "remaining_nr" and "remaining_oids".

Done in V6 too.

quoted

quoted

+                     if (missing_nr) {
+                             to_free = 1;
+                             continue;
+                     }

Now this block took a bit to grok. You use to_free in the if(to_free) free(missing_oids); below.
But it also changes the behavior of remove_fetched_oids(). This means that the first time
remove_fetched_oids() will preserve the list (because it is the input list) but all later
calls will free the newly-created intermediate list. This checks out.

What is confusing to me: is there any reason that missing_nr would be zero in this situation?

I don't think so but I will check again, and maybe add a comment.

Actually missing_nr, or now remaining_nr, would be 0 if all the
promised objects have been fetched.

quoted

quoted

+             }
+             res = 0;
+             break;
+     }
+
+     if (to_free)
+             free(missing_oids);
+
+     return res;
+}

While the test coverage report brought this patch to my attention, it does seem correct.
I still think a test exposing this method would be good, especially one that requires
a fetch_objects() call to multiple remotes to really exercise the details of remove_fetched_oids().

Yeah, I would like to actually test it. I will take another look at
what can be done to test this. Perhaps I will look at what can be done
to still get some objects when fetching from a promisor/partial clone
remote even when it doesn't have all of the objects we request.

I haven't improved test coverage or looked at how we could better
handle a partial fetch. I plan to look at that soon.

Thanks,
Christian.