1633662 - Remove unused mozversioncontrol functions

Status: RESOLVED FIXED

(Firefox Build System :: General, task, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Those include .base_ref, .head_ref, and get_upstream.

Comment 1

[Ricky Stewart]

base_ref and head_ref are going to be called directly or indirectly by tools/tryselect/selectors/coverage.py, or at least that seems to be the case from quick grepping. get_upstream can probably go though.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 3

[Ho Cheung]

Can I take over and work on this bug?

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

I guess you can, although the reviewer should be #firefox-build-system-reviewers this time. base_ref and head_ref are now used in https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/tools/tryselect/selectors/compare.py#36,40 as Ricky said above, while get_upstream can go.

Comment 5

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #4)

I guess you can, although the reviewer should be #firefox-build-system-reviewers this time. base_ref and head_ref are now used in https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/tools/tryselect/selectors/compare.py#36,40 as Ricky said above, while get_upstream can go.

I still see .base_ref, .head_ref, and get_upstream. functions in the codebase.

Is there anything else I should be aware of when dealing with this bug?

Also, does the compare.py file you mentioned need to be modified?

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Ho Cheung from comment #5)>

I still see .base_ref, .head_ref, and get_upstream. functions in the codebase.

Where do you see get_upstream? If it's in testing/web-platform/tests/, you can ignore it as it has its own definition.

Is there anything else I should be aware of when dealing with this bug?

Also, does the compare.py file you mentioned need to be modified?

No, only https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/__init__.py should be modified.

Comment 7

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #6)

(In reply to Ho Cheung from comment #5)>

I still see .base_ref, .head_ref, and get_upstream. functions in the codebase.

Where do you see get_upstream? If it's in testing/web-platform/tests/, you can ignore it as it has its own definition.

Is there anything else I should be aware of when dealing with this bug?

Also, does the compare.py file you mentioned need to be modified?

No, only https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/__init__.py should be modified.

In addition to the testing directory, I found get_upstream in the python directory and taskcluster directory

I will post a commit about this bug in the near future

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

In addition to the testing directory, I found get_upstream in the python directory

Those are the ones we want to remove here!

and taskcluster directory

That one is get_upstream_task_ref, not get_upstream.

Comment 9

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #8)

In addition to the testing directory, I found get_upstream in the python directory

Those are the ones we want to remove here!

and taskcluster directory

That one is get_upstream_task_ref, not get_upstream.

I just looked at the code and found that the three functions .base_ref, .head_ref and get_upstream are still in use (maybe my conclusion is incorrect?).

These three functions seem to pass some parameters.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

The former two are indeed in use as said above. Not sure what you saw about get_upstream, can you point me to the file and the line number?

Comment 11

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #10)

The former two are indeed in use as said above. Not sure what you saw about get_upstream, can you point me to the file and the line number?

Please see line 632 of the file in the link below

https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/__init__.py#170,433,632

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Good catch, and those are exactly the lines we want to remove here. Those are not "in use", rather those are function definitions that are never called and thus are redundant.

Comment 13

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #12)

Good catch, and those are exactly the lines we want to remove here. Those are not "in use", rather those are function definitions that are never called and thus are redundant.

I plan to do this in an upcoming commit:

Delete lines 169-172, delete lines 433-435, and modify def get_upstream(self): in line 632 to def upstream(self):

Am I doing this correctly?

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

I would remove them all, any specific reason you want to keep line 632?

Comment 15

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #14)

I would remove them all, any specific reason you want to keep line 632?

I see that the function below also passes some parameters and returns some hints. There are also some function calls

I'm worried that removing this function like this will cause breaking changes

If this function can be removed, I will remove it later.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

I see that the function below also passes some parameters and returns some hints.

Can you point to the lines you are worrying about? I'm pretty sure nothing calls get_upstream. If it's not called, returning anything is not useful at all and can be removed.

Comment 17

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #16)

I see that the function below also passes some parameters and returns some hints.

Can you point to the lines you are worrying about? I'm pretty sure nothing calls get_upstream. If it's not called, returning anything is not useful at all and can be removed.

Mainly lines 633-634 caused my concern, because this involves the modification of the build system, there are many developers and CI test robots should be using files related to the build system.

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Ho Cheung from comment #17)

Mainly lines 633-634 caused my concern, because this involves the modification of the build system, there are many developers and CI test robots should be using files related to the build system.

Searchfox shows no caller of get_upstream, and that means there's no way for the developers and CI test robots to call that function. That means line 633-634 never runs, and thus can be removed.

Comment 19

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #18)

(In reply to Ho Cheung from comment #17)

Mainly lines 633-634 caused my concern, because this involves the modification of the build system, there are many developers and CI test robots should be using files related to the build system.

Searchfox shows no caller of get_upstream, and that means there's no way for the developers and CI test robots to call that function. That means line 633-634 never runs, and thus can be removed.

ok i totally understand, thanks for your explanation

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Ho Cheung from comment #19)

ok i totally understand, thanks for your explanation

👍

Feel free to ask whenever you have any more doubts!

Comment 21

[Ho Cheung]

Attachment: Bug 1633662 - Remove unused mozversioncontrol functions r=saschanaz

Remove unused mozversioncontrol functions in code

Depends on D167348

Comment 22

[Ho Cheung]

(In reply to Kagami [:saschanaz] from comment #20)

(In reply to Ho Cheung from comment #19)

ok i totally understand, thanks for your explanation

👍

Feel free to ask whenever you have any more doubts!

Is the reviewed code automatically merged into the code tree? Or do you manually push to the code tree?

Comment 23

[Kagami Rosylight [:saschanaz] (they/them)]

Landing is done manually by the reviewer. I'll do that, thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99bb9f2c4e3e
Remove unused mozversioncontrol functions r=saschanaz,firefox-build-system-reviewers,glandium

Comment 25

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/99bb9f2c4e3e