Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crosspost crash on startup #2691

Merged
merged 1 commit into from Mar 26, 2018
Merged

Fix crosspost crash on startup #2691

merged 1 commit into from Mar 26, 2018

Conversation

@USA-RedDragon
Copy link
Contributor

@USA-RedDragon USA-RedDragon commented Mar 25, 2018

The logic flow before was:
Check if json does not have crosspost_parent_list AND
get member 0 of json's crosspost_parent_list to check if it's not null

The problem is if the first condition is true(json does not have crosspost_parent_list),
AND can't short-circut and it will move on to evaluate the second side, so it will
try and get member 0 of a crosspost_parent_list, which will be null as we figured out
in the first conditional.

Now the logic flow is:
Check if json does not have crosspost_parent_list OR
get member 0 of json's crosspost_parent_list to check if it's null

This way, the OR can short circut on the first true condition, so the get member 0
logic won't trigger unless the json does have crosspost_parent_list.

The logic flow before was:
  Check if json does not have crosspost_parent_list AND
  get member 0 of json's crosspost_parent_list to check if it's not null

The problem is if the first condition is true(json does not have crosspost_parent_list),
AND can't short-circut and it will move on to evaluate the second side, so it will
try and get member 0 of a crosspost_parent_list, which will be null as we figured out
in the first conditional.

Now the logic flow is:
  Check if json does not have crosspost_parent_list OR
  get member 0 of json's crosspost_parent_list to check if it's null

This way, the OR can short circut on the first true condition, so the get member 0
logic won't trigger unless the json does have crosspost_parent_list.
@PiwwowPants
Copy link
Contributor

@PiwwowPants PiwwowPants commented Mar 25, 2018

This same fix exists in my Android P pull request #2681

For a compare and contrast in case we did anything different.

@ccrama
Copy link
Owner

@ccrama ccrama commented Mar 26, 2018

Updating Google Play has caused issues in the past, have you done any testing with the updated Drive library for this PR? Also I believe running gradle in parallel breaks our CI, any reason we should leave that enabled (might be a better question for @Alexendoo)?

Thank you for the PR @USA-RedDragon !

@USA-RedDragon
Copy link
Contributor Author

@USA-RedDragon USA-RedDragon commented Mar 26, 2018

I didn't mean to merge the second commit. I'll go and reset HEAD and remake the pull request since Github is weird like that

@PiwwowPants
Copy link
Contributor

@PiwwowPants PiwwowPants commented Mar 26, 2018

@USA-RedDragon git reset --hard HEAD^ | git push -f origin

Fast forwards. The way to live.

@USA-RedDragon
Copy link
Contributor Author

@USA-RedDragon USA-RedDragon commented Mar 26, 2018

Gerrit is my poison of choice, so GitHub seems archaic now. Fast forwards are evil lol

@USA-RedDragon
Copy link
Contributor Author

@USA-RedDragon USA-RedDragon commented Mar 26, 2018

There we go. Removed the personal commit

@ccrama
Copy link
Owner

@ccrama ccrama commented Mar 26, 2018

Thank you for the fix, will merge this now :)

@ccrama ccrama merged commit 67e4e94 into ccrama:master Mar 26, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Alexendoo
Copy link
Collaborator

@Alexendoo Alexendoo commented Mar 27, 2018

I don't think it would break CI but if it does I can take a look at it, what was the change exactly @ccrama?

@ccrama
Copy link
Owner

@ccrama ccrama commented Mar 27, 2018

@Alexendoo the change was removed so no worries! We will have to update travis for @PiwwowPants #2681 though, probably going to merge that today or tomorrow as I've been testing for a week with no real issues yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants