On Sun, Feb 5, 2017 at 6:24 AM, Albert Astals Cid <aa...@kde.org> wrote: > El dissabte, 4 de febrer de 2017, a les 12:44:54 CET, Ben Cooksley va > escriure: >> On Sat, Feb 4, 2017 at 11:41 AM, Albert Astals Cid <aa...@kde.org> wrote: >> > El divendres, 3 de febrer de 2017, a les 21:06:08 CET, Ben Cooksley va >> > >> > escriure: >> >> On Fri, Feb 3, 2017 at 12:18 PM, Albert Astals Cid <aa...@kde.org> wrote: >> >> > El diumenge, 29 de gener de 2017, a les 8:32:21 CET, Ben Cooksley va >> > >> > escriure: >> >> >> Hi everyone, >> >> >> >> >> >> We've just completed the registration of all mainline repositories >> >> >> (not including Websites or Sysadmin namespaced ones) on Phabricator. >> >> >> Thanks go to Luigi Toscano for providing significant assistance with >> >> >> this process. >> >> >> >> >> >> From this point forward, communities should be moving away from >> >> >> Reviewboard to Phabricator for conducting code review. >> >> > >> >> > I just created first patch with the phabricator web interface. >> >> > >> >> > Found one minor and one major problem. >> >> > >> >> > Minor problem: >> >> > * You can't update the diff before creating a "Revision", so if you >> >> > realize >> >> > >> >> > your diff was wrong, back luck, you either leave the diff floating in >> >> > the >> >> > limbo or you create the Revision and the update the diff, showing the >> >> > world >> >> > your mistake for no reason >> >> > https://phabricator.kde.org/D4422?vs=10881&id=10882 >> >> >> >> Interesting. It might be worth asking upstream about that. >> >> >> >> > Major problem: >> >> > * It doesn't show context >> >> > >> >> > https://phabricator.kde.org/D4422 >> >> > >> >> > "Context not available." is terrible, how is one supposed to review >> >> > without >> >> > being able to read the rest of the code? >> >> > >> >> > This is a deal breaker for me. >> >> >> >> Please see https://secure.phabricator.com/T5029 >> > >> > As said on IRC, the fact that this has been open for almost 3 years is >> > more a concern than a relief. >> >> I've inquired with upstream, and they've indicated that at the moment >> T5029 isn't on their roadmap for implementation (although T5000 and >> T182 are). >> >> Their target audience is primarily corporate development workflows, >> for which requiring use of Arcanist isn't an issue. >> >> >> This only occurs when patches are uploaded from the web interface and >> >> the patch in question has minimal context. >> >> At this time Phabricator is not able to automatically resolve context >> >> using markers in the patch (there are certain complexities involved >> >> for some SCMs, particularly for SVN - which Phabricator supports) >> >> >> >> The fix for this is to either: >> >> a) Use Arcanist, the recommended tool for working with Phabricator >> >> (this is no different to rb-tools for Reviewboard) >> > >> > This is not ok, the web interface for reviewboard was as good as rb-tools >> > (i guess tbh i never used them) and "forcing" the use of a weird tool >> > noone has heard of is not a good way to attract new contributors >> >> New contributors who aren't willing to install Arcanist can use diff >> -U99 I would imagine? > > Yes, If there's an easy way for them to know they should (which afaics is not > right now).
Okay, we'll look into adding a message box or something there explaining the need to use diff -U99. > > Cheers, > Albert Regards, Ben > >> >> > Cheers, >> > >> > Albert >> >> Regards, >> Ben > >