Page MenuHomePhorge

Correct a PHP8 compatibility issue when running "arc diff" with no active branch
ClosedPublic

Authored by jkimbo on May 23 2023, 12:45.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 13, 09:20
Unknown Object (File)
Sat, Apr 13, 09:16
Unknown Object (File)
Thu, Apr 11, 13:41
Unknown Object (File)
Wed, Apr 10, 20:07
Unknown Object (File)
Mon, Apr 8, 04:02
Unknown Object (File)
Mon, Apr 1, 01:44
Unknown Object (File)
Mon, Apr 1, 01:44
Unknown Object (File)
Mon, Apr 1, 01:44
Tokens
"Love" token, awarded by valerio.bozzolan.

Details

Summary

When there is no active branch name, arc diff currently fails under PHP8 when we try to strlen(null).

This change is also credited to Evan from upstream Phabricator that applied the same change:

https://secure.phabricator.com/rARCc39ab20eb3717a15aed2467842bd77d9addce96a

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Closes T15412

Test Plan

Under PHP 8.1: ran git checkout <hash of head>, then arc diff to generate this revision.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jkimbo requested review of this revision.May 23 2023, 12:45

Thanks :)

I blindly accept since I totally trust Evan and also it's reasonable to assume that a branch name will never become a native integer or a boolean or an array etc. for legitimate reasons.

The function phutil_nonempty_string() will explode as usual if it will find a non-null or non-string, and that is OK here to me.

Ready to land!

lgtm

This revision is now accepted and ready to land.May 23 2023, 12:57

@valerio.bozzolan I'm not familiar with the process here, is it up to me to land the revision or do I wait until a maintainer does it?

Also thanks for the review :)

To land this patch, you can download Arcanist and:

arc patch D25237
arc land

If you can't for some reasons, don't worry: I will land this in 8 days, hoping to be useful

Thanks again :)

To land this patch, you can download Arcanist and:

arc patch D25237
arc land

If you can't for some reasons, don't worry: I will land this in 8 days, hoping to be useful

Thanks again :)

I think it's a pretty hallucinatory experience to be tested at least once in your life

But feel free to say "naaah, not today. Land for me".

Awesome, revision landed. Thanks for the help @valerio.bozzolan

In D25237#7223, @jkimbo wrote:

Awesome, revision landed. Thanks for the help @valerio.bozzolan

Nice work! Thanks

Your name is now mentioned in Next Up (since Week 18) :D

See you later! ✨