Page MenuHomePhabricator

DifferentialDiff->description is "text255?" but should be "text"
Closed, ResolvedPublic

Description

I created a commit with mercurial and wrote a 260 character commit message (on command line, so I was driving blind and didn't use newlines).

I couldn't find any additional related logs on server in nginx/php-fpm or mysql logfiles.

host128:java cspeck$ hg ci -m "[insert 260-character message here]"

host128:java cspeck$ arc diff --update D12 --trace
libphutil loaded from '/Users/cspeck/Projects/phacility/libphutil/src'.
arcanist loaded from '/Users/cspeck/Projects/phacility/arcanist/src'.
Config: Reading user configuration file "/Users/cspeck/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/cspeck/Source/enad/java/.arcconfig".
Working Copy: Path "/Users/cspeck/Source/enad/java" is part of `hg` working copy "/Users/cspeck/Source/enad/java".
Working Copy: Project root is at "/Users/cspeck/Source/enad/java".
Config: Did not find local configuration at "/Users/cspeck/Source/enad/java/.hg/arc/config".
>>> [0] <conduit> user.whoami() <bytes = 117>
>>> [1] <http> https://phab.mycompany.com/api/user.whoami
<<< [1] <http> 511,973 us
<<< [0] <conduit> 512,176 us
>>> [2] <exec> $ HGPLAIN=1 hg status
<<< [2] <exec> 387,786 us
>>> [3] <event> diff.didCollectChanges <listeners = 0>
<<< [3] <event> 54 us
>>> [4] <conduit> differential.query() <bytes = 146>
>>> [5] <http> https://phab.mycompany.com/api/differential.query
<<< [5] <http> 241,158 us
<<< [4] <conduit> 241,308 us
>>> [6] <conduit> differential.getcommitmessage() <bytes = 169>
>>> [7] <http> https://phab.mycompany.com/api/differential.getcommitmessage
<<< [7] <http> 86,217 us
<<< [6] <conduit> 86,375 us
>>> [8] <conduit> differential.parsecommitmessage() <bytes = 1147>
>>> [9] <http> https://phab.mycompany.com/api/differential.parsecommitmessage
<<< [9] <http> 65,885 us
<<< [8] <conduit> 66,027 us
>>> [10] <event> diff.didBuildMessage <listeners = 0>
<<< [10] <event> 32 us
>>> [11] <exec> $ nano  '/var/folders/kw/22j3hs0j08v7lr6j448pm8lw0000gn/T/edit.93gu9d5flxs8kcsw/differential-update-comments'
<<< [11] <exec> 6,198,293 us
Linting...
>>> [12] <exec> $ HGPLAIN=1 hg help phase
<<< [12] <exec> 76,761 us
>>> [13] <exec> $ HGPLAIN=1 hg branch
<<< [13] <exec> 69,983 us
>>> [14] <exec> $ HGPLAIN=1 hg log --branch 'default' -r 'draft()' --style default
<<< [14] <exec> 267,610 us
>>> [15] <exec> $ HGPLAIN=1 hg parents --style default --rev '.'
<<< [15] <exec> 87,684 us
>>> [16] <exec> $ HGPLAIN=1 hg parents --style default --rev '7c78b5dfb01f'
<<< [16] <exec> 98,653 us
>>> [17] <exec> $ HGPLAIN=1 hg parents --style default --rev '0d164fee36f0'
<<< [17] <exec> 94,965 us
>>> [18] <exec> $ HGPLAIN=1 hg parents --style default --rev 'b7a2eb14716d'
<<< [18] <exec> 95,936 us
>>> [19] <exec> $ HGPLAIN=1 hg parents --style default --rev '6c34544708d2'
<<< [19] <exec> 95,276 us
>>> [20] <exec> $ HGPLAIN=1 hg parents --style default --rev '72e6647e79eb'
<<< [20] <exec> 96,503 us
>>> [21] <exec> $ HGPLAIN=1 hg parents --style default --rev 'ad0c9585c425'
<<< [21] <exec> 94,930 us
>>> [22] <exec> $ HGPLAIN=1 hg parents --style default --rev 'a7731f7a89c6'
<<< [22] <exec> 98,602 us
>>> [23] <exec> $ HGPLAIN=1 hg parents --style default --rev '98ea4a45a933'
<<< [23] <exec> 95,340 us
No lint engine configured for this project.
Running unit tests...
No unit test engine is configured for this project.
>>> [24] <exec> $ HGPLAIN=1 hg diff --git -U32767 --rev 'e9db07b69464' -- ''
<<< [24] <exec> 343,248 us
>>> [25] <exec> $ HGPLAIN=1 hg cat --rev 'e9db07b69464' --output '/var/folders/kw/22j3hs0j08v7lr6j448pm8lw0000gn/T/8cw2tdh1ohgc840w/%p' -- 
<<< [25] <exec> 79,573 us
>>> [26] <exec> $ HGPLAIN=1 hg cat --rev '.' --output '/var/folders/kw/22j3hs0j08v7lr6j448pm8lw0000gn/T/7t28q6jqe4socwwg/%p' -- 
<<< [26] <exec> 78,830 us
>>> [27] <exec> $ HGPLAIN=1 hg log -l 1 --template '{node}' -r 'e9db07b69464' --
<<< [27] <exec> 94,095 us
>>> [28] <exec> $ HGPLAIN=1 hg bookmarks
<<< [28] <exec> 77,347 us
>>> [29] <exec> $ HGPLAIN=1 hg svn info
<<< [29] <exec> 105,832 us
>>> [30] <conduit> repository.query() <bytes = 156>
>>> [31] <http> https://phab.mycompany.com/api/repository.query
<<< [31] <http> 141,133 us
<<< [30] <conduit> 141,288 us
>>> [32] <conduit> differential.creatediff() <bytes = 887782>
>>> [33] <http> https://phab.mycompany.com/api/differential.creatediff
<<< [33] <http> 804,968 us
<<< [32] <conduit> 805,127 us
>>> [34] <event> diff.wasCreated <listeners = 0>
<<< [34] <event> 36 us
>>> [35] <exec> $ HGPLAIN=1 hg log --template '{node}{rev}{author}{date|rfc822date}{branch}{tag}{parents}{desc}' --rev '('\''e9db07b69464'\''::. - '\''e9db07b69464'\'')' --branch 'default' --
<<< [35] <exec> 100,694 us
>>> [36] <exec> $ HGPLAIN=1 hg parents --template '{node}\n' --rev '98ea4a45a933a71760b3e1bb49adda25d8d7e89e'
>>> [37] <exec> $ HGPLAIN=1 hg parents --template '{node}\n' --rev '0d164fee36f0be99cfadfaebfb7ff0af46fdb7c7'
<<< [36] <exec> 86,285 us
<<< [37] <exec> 85,842 us
>>> [38] <conduit> differential.setdiffproperty() <bytes = 9567>
>>> [39] <http> https://phab.mycompany.com/api/differential.setdiffproperty
<<< [39] <http> 156,501 us
<<< [38] <conduit> 156,656 us
>>> [40] <conduit> differential.updaterevision() <bytes = 1568>
>>> [41] <http> https://phab.mycompany.com/api/differential.updaterevision
<<< [41] <http> 193,675 us
<<< [40] <conduit> 193,814 us

[2015-04-23 21:58:49] EXCEPTION: (HTTPFutureHTTPResponseStatus) [HTTP/500] Internal Server Error
{"result":null,"error_code":"ERR-CONDUIT-CORE","error_info":"#1406: Data too long for column 'description' at row 1"} at [<phutil>/src/future/http/BaseHTTPFuture.php:337]
arcanist(head=master, ref.master=9ddf37b9ee3e), phutil(head=master, ref.master=20d43107280d)
  #0 BaseHTTPFuture::parseRawHTTPResponse(string) called at [<phutil>/src/future/http/HTTPSFuture.php:414]
  #1 HTTPSFuture::isReady() called at [<phutil>/src/future/Future.php:39]
  #2 Future::resolve(NULL) called at [<phutil>/src/future/FutureProxy.php:36]
  #3 FutureProxy::resolve() called at [<phutil>/src/conduit/ConduitClient.php:57]
  #4 ConduitClient::callMethodSynchronous(string, array) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:525]
  #5 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:378]

Event Timeline

cspeckmim raised the priority of this task from to Needs Triage.
cspeckmim updated the task description. (Show Details)
cspeckmim added a subscriber: cspeckmim.
epriestley renamed this task from Arcanist returns HTTP/500 "data too long for column 'description'" when performing `arc diff --update` with long commit message on first line. to DifferentialDiff->description is "text255?" but should be "text".Apr 23 2015, 11:08 PM

Per the title, the issue is that the description column on DifferentialDiff is declared as text255? for historical reasons that have been lost to the ages, but should just be text.

If it was text255 (instead of text255? with a ?), it would be sufficient to delete the 255 part and let bin/storage adjust fix this on its own, although this will potentially trigger a relatively expensive migration for some installs (the oldest/largest installs have at least several million diffs). Still, this is probably OK to do with a note in the changelog.

The ? part means that it's nullable, which it should not be. Because the migration will take some time for large installs, I'd ideally to fix the nullability and the 255 limit in one migration, instead of two separate migrations.

Getting rid of the ? means hunting down all the write pathways and making sure they set a string value here in all cases (i.e., empty string for no value, instead of null for no value). Once that's done we can do the migration and fix both nullability and the limit at once.

We could do something silly here like use PhutilUTF8StringTruncator to shorten the text, but I think this is probably not appreciably more difficult to fix correctly.

This field is only populated when you update a revision, and always contains the first line of the update message, which is also posted as a comment.

I think the value of this field is generally low and this UI should probably be revisited (perhaps after whatever is to become of T1508 occurs) but text255? is actually fine. In particular, we should not be storing more than a line of text. I'll fix the fatal with PhutilUTF8StringTruncator for now and we can revisit this more broadly later.

I may have missed it but it doesn't look like this was mentioned in any changelog in December/January - or is this not yet in stable?

chad added a comment.Jan 7 2017, 2:37 AM

I don't think we mention every commit in the changelog?

Yeah, this is in stable, just didn't make the cut for individual mention in the changelog. I think it promoted in Week 52 and I just lumped it into "Differential got infrastructure changes".