Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/27356 )
Change subject: bitvec2freq_list(): fix handling of E-GSM ARFCNs ......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1:
this is the kind of issue where a clear-cut unit test showing the bug should be introduced first, an […]
I will add testing coverage in the next patchset version.
PS1:
I would usually have expected it above the Change-Id
This is actually interesting. In our old patches we used to have 'Related' and 'Depends' comments *below* the 'Change-Id', but at some point I started to see them *above* while doing code review. Maybe the 'commit-msg' hook was updated and I missed it?
UPDATE: I checked my .git/hooks/commit-msg, and it indeed differs from gerrit.osmocom.org:hooks/commit-msg. The recent version of this hook is based on `git interpret-trailers`, while my outdated copy does some grep magic.
File src/osmo-bsc/system_information.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/27356/comment/e47ccfa1_bdd1d204 PS1, Line 508: && bts->c0->arfcn >= 1 && bts->c0->arfcn <= 124) : pgsm = true;
I think it's confusing that we have a variable whether or not we are in the PGSM case, but it is tru […]
Ok, I agree. Will rework the patch to do what you suggest.
https://gerrit.osmocom.org/c/osmo-bsc/+/27356/comment/05214e80_8548bcd7 PS1, Line 522: /
I am adding an 'if' statement right after the 'for' loop (see line 532), which checks rc. […]
Marking this thread as resolved.