Details

Reviewers

Cigaryno
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15179: Diffusion: fix support for git version 1.8.3.1 ("This path was a submodule at ...") - probably since git < 2.5.0

Commits

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

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 Passed

Unit

Tests Passed

Build Status

Buildable 201
Build 201: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Mar 23 2023, 23:00

Owners added a reviewer: O1: Blessed Committers.Mar 23 2023, 23:00

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptMar 23 2023, 23:00

valerio.bozzolan requested review of this revision.Mar 23 2023, 23:00

Harbormaster completed remote builds in B166: Diff 378.Mar 23 2023, 23:00

Just word it with more clearly and less verbose.

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

Harbormaster completed remote builds in B198: Diff 421.Mar 27 2023, 21:08

fix 80 characters

Harbormaster completed remote builds in B199: Diff 422.Mar 27 2023, 21:09

fix typo, re-create line

Harbormaster completed remote builds in B200: Diff 423.Mar 27 2023, 21:11

fix typo; add comment (now ready)

Harbormaster completed remote builds in B201: Diff 424.Mar 28 2023, 05:07

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?

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 4 2023, 22:54

avivey added inline comments.Apr 5 2023, 12:13

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

valerio.bozzolan added inline comments.Apr 5 2023, 12:39

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)

Harbormaster completed remote builds in B224: Diff 452.Apr 5 2023, 12:41

valerio.bozzolan marked an inline comment as done.Apr 5 2023, 12:49

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.

Cigaryno accepted this revision.Apr 5 2023, 15:53

Cigaryno added a project: Config (archived).

sgtm

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 6 2023, 13:41

Cigaryno removed a project: Config (archived).Apr 6 2023, 14:01

In D25089#3036, @avivey wrote:

Just word it with more clearly and less verbose.

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

avivey accepted this revision.Apr 10 2023, 08:15

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

Closed by commit rP1b9da964e970: Add setup check to avoid git version 2.5.0 and below (May 2015). · Explain WhyApr 10 2023, 09:17

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP1b9da964e970: Add setup check to avoid git version 2.5.0 and below (May 2015).

Diff 424

View Options

resources/celerity/map.php

Show All 17 Lines final class PhabricatorStandardPageView extends PhabricatorBarePageView

private $applicationMenu; private $applicationMenu;

private $showFooter = true; private $showFooter = true;

private $showDurableColumn = true; private $showDurableColumn = true;

private $quicksandConfig = array(); private $quicksandConfig = array();

private $tabs; private $tabs;

private $crumbs; private $crumbs;

private $navigation; private $navigation;

private $footer; private $footer;

private $headItems = array();

public function setShowFooter($show_footer) { public function setShowFooter($show_footer) {

$this->showFooter = $show_footer; $this->showFooter = $show_footer;

return $this; return $this;

} }

public function getShowFooter() { public function getShowFooter() {

return $this->showFooter; return $this->showFooter;

▲ Show 20 Lines • Show All 336 Lines • ▼ Show 20 Lines if ($application_menu) {

$menu->setApplicationMenu($application_menu); $menu->setApplicationMenu($application_menu);

} }

$this->menuContent = $menu->render(); $this->menuContent = $menu->render();

} }

/**

* Insert a HTML element into <head> of the page to render.

* Used by PhameBlogViewController.

* @param PhutilSafeHTML HTML header to add

public function addHeadItem($html) {

if ($html instanceof PhutilSafeHTML) {

aviveyUnsubmitted

Not Done

* @param PhutilSafeHTML HTML header to add

- public function addHeadItem($html) {

- if ($html instanceof PhutilSafeHTML) {

+ public function addHeadItem(PhutilSafeHTML $html) {

$this->headItems[] = $html;

You can have PHP do the type checking, but I'm not sure that this is needed - phutil_implode_html already accepts unsafe strings and converts them to safe html.

avivey: You can have PHP do the type checking, but I'm not sure that this is needed…

aviveyUnsubmitted

Not Done

Probably should bother - head items are not normal content, and can't ever be something that isn't explicitly crafted to work in a head element.

avivey: Probably should bother - `head` items are not normal content, and can't ever be something that…

$this->headItems[] = $html;

}

protected function getHead() { protected function getHead() {

$monospaced = null; $monospaced = null;

$request = $this->getRequest(); $request = $this->getRequest();

if ($request) { if ($request) {

$user = $request->getUser(); $user = $request->getUser();

if ($user) { if ($user) {

$monospaced = $user->getUserSetting( $monospaced = $user->getUserSetting(

Show All 16 Lines if (!empty($monospaced)) {

'.PhabricatorMonospaced, '. '.PhabricatorMonospaced, '.

'.phabricator-remarkup .remarkup-code-block '. '.phabricator-remarkup .remarkup-code-block '.

'.remarkup-code { font: %s !important; } '. '.remarkup-code { font: %s !important; } '.

'</style>', '</style>',

$monospaced); $monospaced);

} }

return hsprintf( return hsprintf(

'%s%s%s', '%s%s%s%s',

parent::getHead(), parent::getHead(),

$font_css, $font_css,

phutil_implode_html('', $this->headItems),

$response->renderSingleResource('javelin-magical-init', 'phabricator')); $response->renderSingleResource('javelin-magical-init', 'phabricator'));

} }

public function setGlyph($glyph) { public function setGlyph($glyph) {

$this->glyph = $glyph; $this->glyph = $glyph;

return $this; return $this;

} }

▲ Show 20 Lines • Show All 494 Lines • Show Last 20 Lines

View Options

src/applications/config/check/PhabricatorBinariesSetupCheck.php

Show All 17 Lines final class PhabricatorStandardPageView extends PhabricatorBarePageView

private $applicationMenu; private $applicationMenu;

private $showFooter = true; private $showFooter = true;

private $showDurableColumn = true; private $showDurableColumn = true;

private $quicksandConfig = array(); private $quicksandConfig = array();

private $tabs; private $tabs;

private $crumbs; private $crumbs;

private $navigation; private $navigation;

private $footer; private $footer;

private $headItems = array();

public function setShowFooter($show_footer) { public function setShowFooter($show_footer) {

$this->showFooter = $show_footer; $this->showFooter = $show_footer;

return $this; return $this;

} }

public function getShowFooter() { public function getShowFooter() {

return $this->showFooter; return $this->showFooter;

▲ Show 20 Lines • Show All 336 Lines • ▼ Show 20 Lines if ($application_menu) {

$menu->setApplicationMenu($application_menu); $menu->setApplicationMenu($application_menu);

} }

$this->menuContent = $menu->render(); $this->menuContent = $menu->render();

} }

/**

* Insert a HTML element into <head> of the page to render.

* Used by PhameBlogViewController.

* @param PhutilSafeHTML HTML header to add

public function addHeadItem($html) {

if ($html instanceof PhutilSafeHTML) {

aviveyUnsubmitted

Not Done

* @param PhutilSafeHTML HTML header to add

- public function addHeadItem($html) {

- if ($html instanceof PhutilSafeHTML) {

+ public function addHeadItem(PhutilSafeHTML $html) {

$this->headItems[] = $html;

You can have PHP do the type checking, but I'm not sure that this is needed - phutil_implode_html already accepts unsafe strings and converts them to safe html.

avivey: You can have PHP do the type checking, but I'm not sure that this is needed…

aviveyUnsubmitted

Not Done

Probably should bother - head items are not normal content, and can't ever be something that isn't explicitly crafted to work in a head element.

avivey: Probably should bother - `head` items are not normal content, and can't ever be something that…

$this->headItems[] = $html;

}

protected function getHead() { protected function getHead() {

$monospaced = null; $monospaced = null;

$request = $this->getRequest(); $request = $this->getRequest();

if ($request) { if ($request) {

$user = $request->getUser(); $user = $request->getUser();

if ($user) { if ($user) {

$monospaced = $user->getUserSetting( $monospaced = $user->getUserSetting(

Show All 16 Lines if (!empty($monospaced)) {

'.PhabricatorMonospaced, '. '.PhabricatorMonospaced, '.

'.phabricator-remarkup .remarkup-code-block '. '.phabricator-remarkup .remarkup-code-block '.

'.remarkup-code { font: %s !important; } '. '.remarkup-code { font: %s !important; } '.

'</style>', '</style>',

$monospaced); $monospaced);

} }

return hsprintf( return hsprintf(

'%s%s%s', '%s%s%s%s',

parent::getHead(), parent::getHead(),

$font_css, $font_css,

phutil_implode_html('', $this->headItems),

$response->renderSingleResource('javelin-magical-init', 'phabricator')); $response->renderSingleResource('javelin-magical-init', 'phabricator'));

} }

public function setGlyph($glyph) { public function setGlyph($glyph) {

$this->glyph = $glyph; $this->glyph = $glyph;

return $this; return $this;

} }

▲ Show 20 Lines • Show All 494 Lines • Show Last 20 Lines

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 424

resources/celerity/map.php

src/applications/config/check/PhabricatorBinariesSetupCheck.php

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 424

resources/celerity/map.php

src/applications/config/check/PhabricatorBinariesSetupCheck.php

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

Revision Contents
Changeset List