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

[Albert Astals Cid]

El dilluns, 6 de febrer de 2017, a les 10:19:30 CET, Ben Cooksley va escriure:
> 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'm saying i personally think it's a severe regression and i will not be using 
Differential for the very few patches i create until i'm forced to, since 
Reviewboard is much more useful for my worflow.

Obviously, given how few patches i create or review i can not say the 
migration can not proceed.

Cheers,
  Albert

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