Php-cs-fixer: Short diff option

[julienfalque]

Short diff support was introduced by #2554 but reverted by #2724 because of a BC break in the option handling. We can enable this feature again by introducing a new option like diff-type.

[SpacePossum]

-1 for adding a format that is not defined.
Free formats like the short diff one are horror for BC promise as no one can tell if the output is correct or bugged because the format is not defined. This leaves us with BC supporting bugged-output even if everyone would agree it is bug; as we didn't promise anything defined through definition (like diff 1.4 vs. diff 2.0) we cannot touch it.

I think what would be best is to move away from the half-way-udiff format we got now towards defined formats only. Till than it is, IMHO, best not to another undefined format.

+1 for closing this FR

[robfrawley]

Can't we follow the GNUs definition of udiff? What defined format would you like to see implemented instead?

This said, the whole point of the diff is to either A) view the changes, or B) dump them in a format that allows you to apply them yourself. Wouldn't it then be a logical conclusion to say our short diff implementation need remain compatible with the GNU's patch utility (that their implementation provides our spec)?

Regardless, the whole-file diffs (or more specifically, the lack of short-diffs) represent the single worst "feature" of this package, and I literally grumble about it on a daily basis while using it.

[SpacePossum]

Wouldn't it then be a logical conclusion to say our short diff implementation need remain compatible with the GNU's patch utility

That is the point, however... both sebastian/diff versions 1.4 and 2.x even with all there options are _not_ GNU udiff and _not_ GNU patch compat.

So I'm still against adding the short diff as proposed in this issue through the linked PR, as it would add an short diff option (whatever that may be) that is not defined by a standard like GNU udiff.

[keradus]

does sebastian/diff v2 supports udiff format?
if not, is it rejected udiff request? could you provide link for it?

also, could you please show difference between sebastian/diff v2 and udiff?

[SpacePossum]

does sebastian/diff v2 supports udiff format?

Not OOTB, but you can write your own output builder

if not, is it rejected udiff request? could you provide link for it?

No, it was never offered IIRC. Besides diff package never promised to be GNU udiff AFAIK.

also, could you please show difference between sebastian/diff v2 and udiff?

https://github.com/sebastianbergmann/diff/blob/2.0.1/tests/DifferTestTest.php#L47
Here a bunch of tests cases are filtered out because these are known to be non-compat with patch.
These tests come from here;
https://github.com/sebastianbergmann/diff/blob/2.0.1/tests/DifferTest.php#L716
where you can see the expected part being non-GNU-udiff.

I wonder what we are discussing here though. GNU udiff has can be generated in various short and long length or full file diff. length and whatever. This can be done through various means.
I'm not down voting fixing the current diff provided by PHP CS Fixer, but am I down voting introducing another non-defined format which this issue suggests in the opening by linking to a PR that would do this.

[SpacePossum]

@julienfalque
Can you state in what format you want the "short diff" to be?

[keradus]

No, it was never offered IIRC. Besides diff package never promised to be GNU udiff AFAIK.

maybe we could request it in that case?
I did not found any php udiff implementation

[keradus]

I wonder what we are discussing here though.

having short differ.
now, we do have only long differ.

short differ, if possible, would be great to be udiff compatible, but if not, it's not requirement.

[julienfalque]

I didn't open this issue to introduce a new diff format. An implementation of a short diff output was already introduced in #2554 along with changes in --diff option to support it. The option changes were then reverted #2724 because they introduced a BC break. This issue was opened to have a BC option that supports this short diff implementation.

However this issue is now several months old and a lot has changed regarding diff output, so maybe it's not relevant anymore.

[keradus]

full-diff differ is still a pain in output...

[SpacePossum]

than there are a few routes:

• 1 use diff 1.4 diff only option (old PR suggest, -1)
• 2 use diff 2.0 DiffOnlyOutput (big :-1: )
• 3 use diff 2.0 udiff with exceptions (-1)
• 4 write new udiff output builder and propose to diff package or maintain somewhere else

Only 4. will give you a defined format stopping all the BC pain when bug fixing the output builders, not 1 so I down vote that option.

full-diff differ is still a pain in output...

Note however that this is not the case ATM. The current diff format is somewhat like udiff (without line numbers and with invalid comments) which has a trailing full diff after the first block (under some conditions). A full diff would always show from line 1 till EOF, which isn't the case either.

All details put aside; I would advice not to accept any long or short diff output that is not in a defined format. Therefore focus on getting output builders in udiff output would be a more logical first step.

[keradus]

I would advice not to accept any long or short diff output that is not in a defined format.

hard to achieve when there is no udiff implementation in php world

[SpacePossum]

hard to achieve when there is no udiff implementation in php world

this has been resolved;
https://github.com/GeckoPackages/GeckoDiffOutputBuilder

[keradus]

btw, do we want to propose it to main diff repo ?

[SpacePossum]

I plan on making a branch on the Gecko builder repo that is compat with the diff fork in the PHP-CS-Fixer namespace, allowing a nice upgrade plan here. I'm _not_ planning to move my work towards github.com/sebastianbergmann/diff as I would loose both copyright and maintenance options.

[keradus]

ok

[keradus]

not easy, https://github.com/GeckoPackages/GeckoDiffOutputBuilder requires 7.0 ;)

[SpacePossum]

wasn't _that_ hard either
https://github.com/GeckoPackages/GeckoDiffOutputBuilder/pull/2
; )

[keradus]

Closing, as we do have udiff now. Thanks @SpacePossum, once more.