Page MenuHomePhabricator

obsmarker: crash more helpfully when metadata fields are >255 bytes (issue5681)
ClosedPublic

Authored by swhitaker on Oct 1 2017, 6:31 AM.

Details

Summary

Various mutators fail when attempting to write obsmarkers with
metadata fields longer than 255 bytes, since the length of
mwetadata fields is stored in u8s. This change raises a more
helpful error in such circumstances.

Fixes https://bz.mercurial-scm.org/show_bug.cgi?id=5681

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

swhitaker created this revision.Oct 1 2017, 6:31 AM
ikostia requested changes to this revision.Oct 1 2017, 6:42 AM
ikostia added a subscriber: ikostia.

I would rather see this thing do raise error.ProrgrammingError('metadata cannot be longer than 255 bytes') (or some other message).

This revision now requires changes to proceed.Oct 1 2017, 6:42 AM
pulkit added a subscriber: pulkit.Oct 1 2017, 6:51 AM

Also we append the issue number in the commit message at the end like (issueXXXX) so that the Bugzilla bot can automatically close that. For reference: https://www.mercurial-scm.org/repo/hg-committed/log?rev=issue&revcount=20

@ikostia Sure, I can change this.

What should we do if the metadata value is not under user control? In the case of a username longer than 255 bytes, the user can change that in their hgrc (although it's debatable whether they should; "error, your name is too long" is a pretty poor UX, especially as this limit isn't enforced across Mercurial). But it's possible that functionality exists (or could be added in the future) that writes values to the metadata dictionary that the user can't easily fix. (e.g. a contrived example: we decide it would be a great idea to add the old commit message as metadata when performing a metaedit. If the commit message is > 255 bytes the user will have no simple way of fixing that, since fixing it would require the functionality that is now broken.)

swhitaker retitled this revision from obsmarker: fix crash when metadata fields are >255 bytes to obsmarker: fix crash when metadata fields are >255 bytes (issue5681).Oct 1 2017, 7:04 AM

That is why we raise ProgrammingError, not some sort of UserError. If your extension writes metadata longer than 255 chars, it is a bad extension. Your proposal is weird, because it is quietly modifying things, IMO this is much worse. I'd rather be forced to change my name to something shorter than having it truncated at some arbitrary position.

swhitaker updated this revision to Diff 2255.Oct 1 2017, 7:51 AM
ikostia accepted this revision.Oct 1 2017, 7:53 AM

LGTM

swhitaker edited the summary of this revision. (Show Details)Oct 1 2017, 7:54 AM
swhitaker retitled this revision from obsmarker: fix crash when metadata fields are >255 bytes (issue5681) to obsmarker: crash more helpfully when metadata fields are >255 bytes (issue5681).
yuja accepted this revision.Oct 1 2017, 10:33 AM
yuja added a subscriber: yuja.

Removed translation marker and queued, thanks.

obsmarker: crash more helpfully when metadata fields are >255 bytes (issue5681)

s/255 bytes/255bytes/ to silence test-check-commit.t

This revision is now accepted and ready to land.Oct 1 2017, 10:33 AM
This revision was automatically updated to reflect the committed changes.