Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes

[Ben Cooksley]

On Sun, Feb 5, 2017 at 6:24 AM, Albert Astals Cid <aacid at 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 <aacid at 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 <aacid at 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
>
>