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

[Albert Astals Cid]

El divendres, 3 de febrer de 2017, a les 10:24:08 CET, Elvis Angelaccio va 
escriure:
> On Fri, Feb 3, 2017 at 9:06 AM, Ben Cooksley <bcooksley at kde.org> wrote:
> > 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
> > 
> > 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)
> > b) Use "diff -U99" when generating your diffs for uploading to Phabricator
> 
> Would it be possible to add this info in the patch upload form? e.g.
> near or in place of this sentence: "You can also paste a diff below,
> or upload a file containing a diff (for example, from svn diff, git
> diff or hg diff --git)."
> 
> It would not be a solution but at least more people would know that
> the -U switch can be used as workaround.

Or flat out reject the diff, that's annoying but at least gives you something 
you can use to move forward.

"I need a better diff, please use -U99"

Cheers,
  Albert

> 
> > In regards to improvements in Diffusion (Repository Hosting) and
> > Differential (Code Review) please see the upstream task at
> > https://secure.phabricator.com/T12010 for the work which is currently
> > in active progress (many of these
> > 
> >> Cheers,
> >> 
> >>   Albert
> > 
> > Regards,
> > Ben
> 
> Cheers,
> Elvis