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

[Ben Cooksley]

On Mon, Feb 6, 2017 at 8:39 AM, Albert Astals Cid <aacid at kde.org> wrote:
> El dilluns, 6 de febrer de 2017, a les 8:18:04 CET, Ben Cooksley va escriure:
>> 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.
>
> FWIW i thought that diff -U99 would give you the same behaviour that when
> using arc (somehow i thought phabricator was not very smart and needed more
> lines to actually match the patch against the whole file).
>
> But no, using diff -U99 only gives you more lines because you used -U99, but
> you can't still expand the whole file like when using arc.

Are you essentially saying that the migration cannot proceed, because
Phabricator doesn't know how to expand patches?

I'd suggest using "diff -U99999999999999999" or some other massively
long number, if having the full file available is a hard requirement.

>
> Cheers,
>   Albert

Regards,
Ben

>
>>
>> > Cheers,
>> >
>> >   Albert
>>
>> Regards,
>> Ben
>>
>> >> > Cheers,
>> >> >
>> >> >   Albert
>> >>
>> >> Regards,
>> >> Ben
>
>