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 <bcooks...@kde.org> wrote: > > 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 > > > > 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