Support for SVN patches, created by PhpStorm
Open, WishlistPublic
Actions

Assigned To

None

Authored By

	aik099
	Mar 29 2014, 6:03 PM

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

Revisions and Commits

rARC Arcanist
	Needs Review	D10770 Added support for parsing PhpStorm produced patches

Related Objects
Search...

		Status	Assigned	Task
		Open	None	T12664 Update diff/patch parsing to extract more metadata and parse a wider range of formats
		Open	None	T4698 Support for SVN patches, created by PhpStorm

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.

(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.

chris.nz added a subscriber: chris.nz.Jul 13 2014, 12:00 PM

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.

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.

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.

Add example patches to arcanist/src/parser/__tests__/diff/.
Add structured test cases for these patches in ArcanistDiffParserTestCase (that file has plenty of examples). These checks make sure the parse is correct.
Run arc unit. Your new tests should fail; the existing tests should pass.
Adjust ArcanistDiffParser until all tests pass.
You can find a (more complex) example of stripping a header here: https://secure.phabricator.com/diffusion/ARC/browse/master/src/parser/ArcanistDiffParser.php;52947cfd92b331916adffa1ff3233546446c881b$194-208

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.

aik099 added a revision: D10770: Added support for parsing PhpStorm produced patches.Nov 1 2014, 11:45 AM

btrahan added a project: Subversion.Nov 8 2014, 7:43 AM

epriestley added a parent task: T12664: Update diff/patch parsing to extract more metadata and parse a wider range of formats.Feb 10 2018, 7:43 PM

Herald added a subscriber: eadler. · View Herald TranscriptFeb 10 2018, 7:43 PM

• GoogleLegacy added a subscriber: • GoogleLegacy.Feb 19 2018, 4:35 AM

epriestley moved this task from Backlog to Subversion on the Differential board.Mar 17 2021, 6:55 PM