Page MenuHomePhabricator

Support for SVN patches, created by PhpStorm
Open, WishlistPublic

Description

When you have an SVN-based project and create a patch (to be uploaded into Differential), then PhpStorm adds some extra metadata that it can use later on.

Here is the example patch contents:

Index: core/kernel/db/dbitem.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- core/kernel/db/dbitem.php	(revision 16016)
+++ core/kernel/db/dbitem.php	(revision )
@@ -962,6 +962,8 @@
 			// insert into temp table (id is not auto-increment field)
 			$insert_id = $this->FieldValues[$this->IDField];
 		}
+
+		$temp_id = $this->GetID();
 		$this->setID($insert_id);
 
 		$this->OriginalFieldValues = $this->FieldValues;
@@ -980,7 +982,7 @@
 			$this->setTempID();
 		}
 
-		$this->raiseEvent('OnAfterItemCreate');
+		$this->raiseEvent('OnAfterItemCreate', null, array('temp_id' => $temp_id));
 		$this->Loaded = true;
 
 		return true;

as you can see the PhpStorm has added some extra info to the patch (e.g. "IDEA additional info") which causes Differential to throw an exception when parsing such a patch file:

{F135842}

I've originally posted this task on GitHub: https://github.com/facebook/phabricator/issues/550

Event Timeline

aik099 created this task.Mar 29 2014, 6:03 PM
aik099 raised the priority of this task from to Needs Triage.
aik099 updated the task description. (Show Details)
aik099 added a project: Differential.
aik099 added a subscriber: aik099.
epriestley triaged this task as Wishlist priority.Apr 24 2014, 1:19 PM
epriestley added a subscriber: epriestley.

(This is fairly easy, but requires installing and setting up PHPStorm to test, and we haven't seen other requests for it.)

I have PhpStorm installed and I can generate as many patches as you need. Then you can tests based on provided patches and that's all.

It appears, that the PhpStom only adds

IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8

between Index: filename_goes_here and ===================================================================.

Maybe you can just always strip that part before validating given patch file (or patch pasted into Differential textarea).

@epriestley, can this change be made to allow uploading PhpStorm created patches?

Similar issues were created for PhpStorm (not to add these 3 lines to make every other app happy):

but so far they are not fixed.

This is not a priority for us.

That I understand. But does it really take that much time to just replace 3 mentioned lines in each file within a patch? I bet you spent more time explaining to me twice that you don't care about PhpStorm users.

Yes, this is a low priority because we believe there are other things we could be doing which will give us more value for the time spent.

Some of your feedback is pretty adversarial. I understand it can be frustrating to not get things that seem easy fixed quickly, but they often aren't that easy to fix and we have a huge number of users who need help with all sorts of things. We can only work on a small number of things at one time, which means most issues don't get worked on. You can find more discussion in T4778.

It's fine if you think we're incompetent, but you're the one choosing to use our software. If you aren't happy with how we're prioritizing or handling your issues, you're free to use other software.

aik099 added a comment.EditedSep 19 2014, 1:46 PM

Phabricator is great and when I started using it my life changed and I've been code review guru ever since. I also understand that when you choose to install a software, then you're guaranteed to have only functionality it has at that time and can't make any expectation on it beyond that and especially free of charge.

Of course different Phabricator installations have different needs. From single user perspective only tasks relevant to that user matter and other don't. Unlucky for me the only user who needs this is myself, so I understand your position too.

By the way do you have any paid plans, which would allow to make issue priority higher if a company needing it would just pay for it's development? If so, then how does this work and how much $ would cost to developer such feature.

chad added a subscriber: chad.Sep 19 2014, 4:21 PM

We have offered contracts in the past, but only for large scale features that were already on the roadmap. Supporting additional external editors just isn't something we can add right now. Even if a task is "easy" supporting something in the upstream for years comes at a much higher cost. We'd rather pass until we have the ability to support it the right way, and for everyone.

Maintaining a local patch is likely the best way to go. If you're unable to do this yourself, I've seen people on Freelancer offering Phabricator customization services (and likely at a much reduced cost than us).

I've created my own fork, so I guess now I can change something locally to make it work.

@epriestley, what would be the best place in code to strip these non-standard patch pieces (from uploaded patch OR from patch pasted into textarea) before validation takes place.

I will do that, thanks.

I've made necessary changes and now trying to use arc diff to submit them here. However I encounter the problem, where linter is complaining about trailing whitespaces and tabs within new patch fixture I've used. Linter validations are normal for PHP files, but I think that they shouldn't apply to patch files.

I guess I need to skip linting then.