removed fetch_if_missing global

We are not much happy with global variable fetch_if_missing. So, in commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08) Jonathan Tan [email protected] [[email protected]] attempted to remove the need for fetch_if_missing=0 from the fetching mechanism. After that, fetch_if_missing is removed from clone and promisor-remote too.

I imitated the same logic to remove fetch_if_missing from fetch-pack & index-pack.

I'm looking forward to remove fetch_if_missing from other places too, but I not sure about how to handle it.

In fsck, fetch_if_missing is set to 0 in the beginning of cmd_fsck().

In rev-list, fetch_if_missing is set to 0 in parse_missing_action_value(), and in cmd_rev_list() while parsing the command-line parameters.(almost similar case in pack-objects)

fixes #251

From: Hariom Verma <[email protected]>

Commit 6462d5e ("fetch: remove fetch_if_missing=0", 2019-11-08) strove to remove the need for fetch_if_missing=0 from the fetching mechanism, so it is plausible to attempt removing fetch_if_missing=0 from fetch-pack as well.

Quoted 7 lines · Hariom Verma via GitGitGadget

From: Hariom Verma <[email protected]>

Commit 6462d5e ("fetch: remove fetch_if_missing=0", 2019-11-08) strove to remove the need for fetch_if_missing=0 from the fetching mechanism, so it is plausible to attempt removing fetch_if_missing=0 from fetch-pack as well.

It's ok to refer to a previous commit, but I think it would be better if you could repeat a bit the reasons why removing the fetch_if_missing global is a good idea, and not just rely on the previous commit.

"it is plausible" also doesn't make it very clear that it's what the patch is actually doing.

From: Hariom Verma <[email protected]>

Commit 6462d5e ("fetch: remove fetch_if_missing=0", 2019-11-08) strove to remove the need for fetch_if_missing=0 from the fetching mechanism, so it is plausible to attempt removing fetch_if_missing=0 from fetch-pack as well.

Signed-off-by Hariom Verma <[email protected]>

You might want to add Jonathan in Cc next time, as it could help your patches move forward. I have added him to this email.

Maybe you could add a few tests as in 6462d5eb9a.

It is ok to not take care of the other places for now. If that was the only reason why this patch series is marked as WIP, then you might want to remove WIP, especially if you add tests.

It would be nice if you could give the full URL of the bug, as there have been different bug trackers used by different people.

Quoted 6 lines · Hariom Verma via GitGitGadget

We are not much happy with global variable fetch_if_missing. So, in commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08) Jonathan Tan [email protected] [[email protected]] attempted to remove the need for fetch_if_missing=0 from the fetching mechanism. After that, fetch_if_missing is removed from clone and promisor-remote too.

You might want to add Jonathan in Cc next time, as it could help your patches move forward. I have added him to this email.

Quoted 2 lines

I imitated the same logic to remove fetch_if_missing from fetch-pack & index-pack.

Maybe you could add a few tests as in 6462d5eb9a.

Quoted 2 lines

I'm looking forward to remove fetch_if_missing from other places too, but I not sure about how to handle it.

It is ok to not take care of the other places for now. If that was the only reason why this patch series is marked as WIP, then you might want to remove WIP, especially if you add tests.

Quoted 7 lines

In fsck, fetch_if_missing is set to 0 in the beginning of cmd_fsck().

In rev-list, fetch_if_missing is set to 0 in parse_missing_action_value(), and in cmd_rev_list() while parsing the command-line parameters.(almost similar case in pack-objects)

fixes #251

It would be nice if you could give the full URL of the bug, as there have been different bug trackers used by different people.

Thanks,
Christian.

It's ok to refer to a previous commit, but I think it would be better if you could repeat a bit the reasons why removing the fetch_if_missing global is a good idea, and not just rely on the previous commit.

"it is plausible" also doesn't make it very clear that it's what the patch is actually doing.

Quoted 7 lines · Christian Couder

It's ok to refer to a previous commit, but I think it would be better if you could repeat a bit the reasons why removing the fetch_if_missing global is a good idea, and not just rely on the previous commit.

"it is plausible" also doesn't make it very clear that it's what the patch is actually doing.

I had the same reaction. You could even write a random gibberish in your patch and write "it's plausible this set of random changes made without understanding what is going on in the current code might have some chance to work" in your log message, and we would not even want to touch such a patch with 10-foot pole.

The proposed log message above unfortunately makes this patch indistinguishable from such a trash, unless we follow the codepaths that are *not* touched by this patch and think about ramifications of the removal *ourselves*. In other words, it does nothing to help the readers to support the change.

As Christian said [1], please include tests like in the commit you mentioned. For a change like this, I think that the test is the most important part.

Also include a justification for why it's safe to remove fetch_if_missing=0. You can probably cite the aforementioned commit to say that it covers the fetch_pack() method, and then go through the rest of the code to see if any may inadvertently fetch an object.

Also, the fetch-pack and index-pack parts can be sent in separate patch sets, so you might want to concentrate on one command first.

Quoted 6 lines

fetch-pack.c +0 -0

index 1734a573b01..1ca643f6491 100644

fetch-pack.c static void update_shallow(struct fetch_pack_args *args, · line 1655

Quoted 8 lines

fetch-pack.c +1 -1

@@ -1649,7 +1649,7 @@ static void update_shallow(struct fetch_pack_args *args,
              struct oid_array extra = OID_ARRAY_INIT;
              struct object_id *oid = si->shallow->oid;
              for (i = 0; i < si->shallow->nr; i++)
-                     if (has_object_file(&oid[i]))
+                     if (has_object_file_with_flags(&oid[i], OBJECT_INFO_SKIP_FETCH_OBJECT))
                              oid_array_append(&extra, &oid[i]);
              if (extra.nr) {
                      setup_alternate_shallow(&shallow_lock,

Hmm...this triggers when the user requests a clone that is both partial and shallow, and the server reports a shallow object that it didn't send back as a packfile; and it causes another fetch to be sent. This is a separate issue, but Hariom, if you'd like to take a look at this, that would work out too. You'll need to figure out how to make the server send back shallow lines referencing objects that are not in the packfile
- one way to do it is to use one-time-perl. (Search the codebase to see how it is used.) This is probably more complex, though.

Are you still working on this? If not, then I would like to take this up and write the tests, if it is worth doing. I think it would be a better exposure of the codebase and would be helpful for GSoC.

I am not so sure it will be helpful for any of the GSoC project ideas we propose, but feel free to work on it if you want.

Quoted 2 lines · Christian Couder

I am not so sure it will be helpful for any of the GSoC project ideas we propose, but feel free to work on it if you want.

Well, I wanted to work on something before I started working on my application and found this to be fun. So even if it would not really be helpful for the project ideas proposed, I would still like to work on it as something that could go into my application.

Are you still working on this? If not, then I would like to take this up and write the tests, if it is worth doing. I think it would be a better exposure of the codebase and would be helpful for GSoC.

Also, is the shallow object not sent when cloning due to the partial clone filter and hence a subsequent fetching is done to ask for this object? If so, then will such a fetch counted as an args->update_shallow?