Page MenuHomePhorge

Fix arc diff in Subversion for non-English languages
ClosedPublic

Authored by valerio.bozzolan on Jun 11 2024, 16:09.

Details

Summary

When using arc diff, this crash should not fire anymore on non-English shells:

Undefined array key "Repository UUID"

The error message comes to this line:

https://we.phorge.it/source/arcanist/browse/master/src/repository/api/ArcanistSubversionAPI.php;f7fcf31c7e23475e345cb3cd4abf0474ba6fd9cf$606

Look at the parser of svn info that expects English language:

https://we.phorge.it/source/arcanist/browse/master/src/repository/api/ArcanistSubversionAPI.php;f7fcf31c7e23475e345cb3cd4abf0474ba6fd9cf$326-375

The historical behavior of ExecFuture is to inherit environment variables. It may have sense, to have the user home, etc.

So, the approach is to just clear the incompatible specific environment variable that alters the output language.

Closes T15855

Test Plan
  • clone a random a Subversion repository
  • have your operating system in a non-English language
  • (e.g. export LANGUAGE=en_US:it_IT)
  • run 'arc diff'

No crash anymore. You are able to submit the patch, just like in git.

Diff Detail

Repository
rARC Arcanist
Branch
arcpatch-D25691_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1401
Build 1401: arc lint + arc unit

Event Timeline

valerio.bozzolan edited the summary of this revision. (Show Details)

I realize how little I remember SVN to set up testing, sigh... Any chance to share the related stacktrace?

Any chance to share the related stacktrace?

Yup! Sorry it was just in T15855. I copy-paste the stack-trace also here.

l2dy requested changes to this revision.Jun 21 2024, 14:50
l2dy subscribed.
l2dy added inline comments.
src/repository/api/ArcanistSubversionAPI.php
43

According to https://github.com/apache/subversion/commit/ffb5df7834920be5e5d9c1cfb485eb77a5f5c990 and the latest psvn.el file, I think LANG=C or "LC_MESSAGES=C" "LC_ALL=" should be preferred.

This revision now requires changes to proceed.Jun 21 2024, 14:50

improvement thanks to dear @l2dy

Better to also unset LANGUAGES - just because we can

Thanks again. I've found this interesting, so, added in the code itself:

https://superuser.com/a/1779369/390314

The priority of all four variables LC_ALL, LC_*, LANG, and LANGUAGE according to applicable standards:LANG=C LC_MESSAGES=C

So I think LC_ALL is enough.


But, some Subversion snippets just use LANG=C + LC_MESSAGES=C

https://github.com/apache/subversion/blob/trunk/tools/client-side/bash_completion#L81

So I'm still confused, anyway, this works.

Thanks for any additional opinionated comment. Again, I'm just confused lol

I must read this:

https://svnbook.red-bean.com/en/1.7/svn.advanced.l10n.html

Spoiler, funny phrase in Italian:

Adesso il caffè è più forte.

https://unix.stackexchange.com/a/87763 is also a useful reference, specifically:

On GNU systems, LC_ALL=C and LC_ALL=POSIX (or LC_MESSAGES=C|POSIX) override $LANGUAGE, while LC_ALL=anything-else wouldn't.

which is an interpretation of

Note: The variable LANGUAGE is ignored if the locale is set to ‘C’. In other words, you have to first enable localization, by setting LANG (or LC_ALL) to a value other than ‘C’, before you can use a language priority list through the LANGUAGE variable.

from https://www.gnu.org/software/gettext/manual/html_node/The-LANGUAGE-variable.html#The-LANGUAGE-variable

OK, I think we have something mathematical and bombproof

src/repository/api/ArcanistSubversionAPI.php
51

LC_ALL deactivates LANGUAGE, not LANG.

src/repository/api/ArcanistSubversionAPI.php
51

O sorry I mean that LC_ALL=C + LANG=C deactivate LANGUAGE

But better to proceed here ihih

T15872: Understand how to defuse localization in GNU Gettext

Thanks again

This revision is now accepted and ready to land.Jul 8 2024, 07:45