Page MenuHomePhorge

Add setup check to avoid git version 2.5.0 and below (May 2015)
ClosedPublic

Authored by valerio.bozzolan on Mar 23 2023, 23:00.

Details

Summary

After this change, if you have a deprecated git version,
now you see a related information config warning:

Git 250 Phorge config deprecation warning.png (572×926 px, 70 KB)

Having said that the git version 1.8.3.1 - that for example
is provided by CentOS 7 - is surely problematic;

Some sources indicate all git versions *before* 2.5.0 as not
able to handle the '--' escape argument. The '--' arg
is used to separate normal git flags from user arguments,
so that they cannot be mistakenly exchanged. Kind of:

git <flags> -- <arguments>

The problem is, Phabricator/Phorge at the moment executes
this kind of git commands when you surf a git repository from
the web interface:

git cat-file -t -- <hash>:<file>

But, if your git version does not support "--", you can
get yourself into unhappy situations since "--" could be
just interpreted as a "wrong value", causing other
considerations and misleading exceptions, such as:

This path was a submodule at <repo>:<hash>

If you have seen the above error while surfing a git repo,
and if you have 2.5.0 or before, and if you are sure that
the mentioned path it's not a submodule, you probably
need a git update.

It is not yet clear whether Phabricator/Phorge should
support the possibility of this lack of support for "--".
In the meanwhile, just update your git on the server.

AFAIK no particular version is required on your clients.

Related information:

If you disagree, please do not simply ignore the
warning but share your experience in the Task.

Ref T15179

Test Plan
  • open Phorge, you see no warnings since you are already up to date. Nice.
  • Otherwise, you see a nice config message, and you can ignore it as usual.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25089
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsrc/applications/config/check/PhabricatorBinariesSetupCheck.php:111TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 198
Build 198: arc lint + arc unit

Event Timeline

Just word it with more clearly and less verbose.

less verbose, and use sprintf() style to simplify i18n

fix typo; add comment (now ready)

In D25089#3036, @avivey wrote:

Just word it with more clearly and less verbose.

Thank you again. Maybe now it's more nice

In D25089#3036, @avivey wrote:

Just word it with more clearly and less verbose.

Thank you again for this tip. Do you like it now?

src/applications/config/check/PhabricatorBinariesSetupCheck.php
114
src/applications/config/check/PhabricatorBinariesSetupCheck.php
114

Believe me or not but it was exactly 80 characters, now 81

adopt suggestion by the kind avivey (but trying to stay in 80 chars)

valerio.bozzolan added inline comments.
src/applications/config/check/PhabricatorBinariesSetupCheck.php
114

OK i just removed "that"

Ah, you may be interested in typing "sgtm" when you will +1 to spawn a politically-correct meme.

In D25089#3036, @avivey wrote:

Just word it with more clearly and less verbose.

Thanks for that comment. I think it's better now

This revision is now accepted and ready to land.Apr 10 2023, 08:15