Clean up fallout from branch (non)landing of bug 833982

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: akeybl, Assigned: jhford)

Tracking

unspecified
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: QARegressExclude)

(Reporter)

Description

6 years ago
Francisco emailed:

"I've found myself in a weird situation when trying to uplift the bug 836166:

https://bugzilla.mozilla.org/show_bug.cgi?id=836166


[Dialer] Without SIM Card, the dialer app still pop up an out going call screen if you dial a normal number.

As per John Ford comment, this patch doesn't apply automatically to v1-train so I was going to resolve the conflict by myself cherry picking the changes, but I found a weird surprise.

At the beginning I though that bug 833982 wasn't applied:

https://bugzilla.mozilla.org/show_bug.cgi?id=833982 (Trying to make a call when no voice network is available silently fails with no feedback to user)

But that bug has all the correct flags to have been uplifted to v1-train (I mean is a tef+ and status-b2g18-v1.0.0 fixed), so I checked again and realised that the patch is 'partially' applied to v1-train, if you check the git blame and logs:

v1-train for file apps/communications/dialer/js/telephony_helper.js:

https://github.com/mozilla-b2g/gaia/blame/v1-train/apps/communications/dialer/js/telephony_helper.js

You will see how it doesn't include any reference to bug 833982 but the changes that this bug introduced.

How this could happen? Easy, we do have another bug 833232 that was checked in, and we did a manually uplift to v1-train, look at comment 14:

Landed on master. https://github.com/mozilla-b2g/gaia/commit/4e5b4b1e696ca210be2168ec27d25eb7b610443b
Landed on v1-train. https://github.com/mozilla-b2g/gaia/commit/00c034963f436de06e517ca93434ef7e885cc5e8
Landed on v1.0.0. https://github.com/mozilla-b2g/gaia/commit/da39f0b896ef86aaa7dccba6ef6941edde745f37

Despite that this bug is a no brainer when we did the uplift to v1-train I think we added more things than expected (check what the patch for this bug and what's the PR for v1-train).

So, IMHO, maybe I'm wrong, we have an inconsistent state right now affecting the dialer app, we didn't uplift 833982, but we have it now partially cause of 833232.

Perhaps the changes are easy to solve, but will need to investigate in deep."
Summary: Inconsistencies in v1-train branch → Clean up fallout from branch (non)landing of bug 833982
The main source of confusion here is that https://bugzilla.mozilla.org/show_bug.cgi?id=833982 has flags for being uplifted but wasn't ever uplifted.  I am not sure why Ryan marked this bug as having been uplifted, it clearly hasn't been.  I looked at the history for the file on the v1-train branch and see no mention of that bug number or a similarly titled commit on the file's history.

The following things happened:

1) 833982 landed on master
2) 833982 was marked as uplifted to v1-train and v1.0.0 but wasn't actually uplifted
3) 833232 was uplifted by the patch's author to v1-train and v1.0.0
4) 833232 builds on 833982's changes to telephony_helper.js, but introduces more changes.  The full set of 833232 changes are live on branches
5) 836166 did not apply cleanly because, as Jordano points out, it depends on 833982 being on branches

The merge conflicts here are actually quite small when cherry-picking 833982 onto v1-train.  I did a merge then tried to cherry-pick 836166 onto v1-train and it applied cleanly.

What needs to happen:

1) 833982 needs to be *actually* uplifted to v1-train and v1.0.0 with merge conflicts resolved.  I don't know the code and don't feel comfortable doing this
2) 836166 needs to be cherry-picked onto v1-train and v1.0.0.  If uplifting 833982 is completed, I can take care of this uplift

This is what I did to validate that 836166 would apply cleanly and could be the basis of resolving this problem:

cd gaia
git checkout v1-train
git cherry-pick -x -m1 5df0c698b0c3f386b02e670a15e100a5d59ec3cd
<resolve  merge conflict>
git add apps/communications/dialer/js/telephony_helper.js
git commit
git checkout v1.0.0
git cherry-pick -x $(git log --pretty=%H -n1 v1-train)
git checkout v1-train
git cherry-pick -x -m1 05de5726720b1c98671cdcb983b7ee493b271e2b
git checkout v1.0.0
git cherry-pick -x $(git log --pretty=%H -n1 v1-train)
Hey Francisco, I was wondering if you'd be able to take a look at the merge of bug 833982.  I've done a simple merge to test things out, but I want to make sure that someone who knows the code reviews the merge.
Flags: needinfo?(francisco.jordano)
Hi John!

First, thanks a lot for the work, I know it's a pain in the ass deal with this merges.

I was taking a look to v1-train and v1.0.0 and couldn't see the changes from 833982, i.e. changes in the dialer.js that are added.

Perhaps I misunderstood the question and you didn't apply, or did just locally.

Thanks a lot!
Flags: needinfo?(francisco.jordano)
Hey John - can this be resolved fixed now?
blocking-b2g: tef? → tef+
Flags: needinfo?(jhford)
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → affected
(In reply to Francisco Jordano [:arcturus] from comment #3)
> Hi John!
> 
> First, thanks a lot for the work, I know it's a pain in the ass deal with
> this merges.
> 
> I was taking a look to v1-train and v1.0.0 and couldn't see the changes from
> 833982, i.e. changes in the dialer.js that are added.
> 
> Perhaps I misunderstood the question and you didn't apply, or did just
> locally.
> 
> Thanks a lot!

Np!

I only applied the merges locally.  While I'm pretty sure that the merges are OK, I think it's best for someone who knows the code to verify.


(In reply to Lukas Blakk [:lsblakk] from comment #4)
> Hey John - can this be resolved fixed now?

Nope, the merge has not yet occured.
Flags: needinfo?(jhford)
Hi all,

Just did the first cherry pick and fixed the merge problems.

Checked that the result is ok, and 'apparently' (we will need QA to verify this) it's working well.

John, I left the merge resolution in the branch:

https://github.com/arcturus/gaia/tree/v1-train-833982

(It's v1-train merged with the bug 833982)

Commit: 5a6b7199eb95302bcc8f2274e214d58b2faa2c22 (in that branch, so i guess you can apply the diff)

Do you need anything else?

Cheers!!
status-b2g18-v1.0.1: --- → affected
(In reply to Francisco Jordano [:arcturus] from comment #6)
> Do you need anything else?


Nope, thanks!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Resolution: --- → FIXED
John, do you know the commit hashes for the various branches here ?
Flags: needinfo?(jhford)
(In reply to Julien Wajsberg [:julienw] from comment #8)
> John, do you know the commit hashes for the various branches here ?

git branch --contains 5a6b7199eb95302bcc8f2274e214d58b2faa2c22
* v1-train
  v1.0.1

I believe that is the commit that resolved this bug.
Flags: needinfo?(jhford)

Comment 10

6 years ago
Does not make sense to create a regression issue.
Flags: in-moztrap-
Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 12

6 years ago
Unable to track changes, Requires more info to verify
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.