Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 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 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 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 > > > > 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
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Mon, Feb 6, 2017 at 8:39 AM, Albert Astals Cid 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 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 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 > 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 -U9" 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 > >
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 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 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 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. Cheers, Albert > > > Cheers, > > > > Albert > > Regards, > Ben > > >> > Cheers, > >> > > >> > Albert > >> > >> Regards, > >> Ben
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Sun, Feb 5, 2017 at 6:24 AM, Albert Astals Cid 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 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 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 > >
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 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 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). Cheers, Albert > > > Cheers, > > > > Albert > > Regards, > Ben
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Sat, Feb 4, 2017 at 1:15 PM, Kai Uwe Broulik wrote: >> 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 > > Agreed. Especially since arc is some mental php script that randomly amends > wrong revisions and/or adds files deliberately left unstaged. Faults with Arcanist should be reported to upstream, at https://secure.phabricator.com/ Some of the issues you are seeing could be edge cases which Arcanist is not handling properly. Please make sure you have updated to the latest versions of Arcanist and libphutil before you do so. > > As for the context I usually end up uploading diffs with -U99 but that's not > ideal and it had never been an issue with ReviewBoard. > > I also find the way to access revision patchset diffs cumbersome. The fact > that online comments aren't versioned and stick around even if the diff > changed significantly in later iterations is quite annoying. They also don't > provide code snippets in the list of comments, you always have to jump to the > specific line to see what's up there. The request for code snippets in the comments area would be a feature request - which should be filed with upstream. In relation to versioning, I can see your point, however I can also see the rationale behind retaining them until they've been resolved. This is a behavioural difference between Phabricator and Reviewboard. You are welcome to discuss the issue with upstream. > > The repository search when uploading a diff is also pretty dumb, always > defaulting to the least sensible option: Querying for "plasma workspace" > automatically chooses Plasma Workspace Wallpapers, "kio" ends up with KIO > AudioCD or Gopher... There are upcoming changes to Differential and Diffusion which may affect this. I do know that there have been some changes to search and typeaheads within Phabricator recently. See https://secure.phabricator.com/T12010 for more details on the changes which they'll be undertaking over the next few months. We've yet to deploy any of those changes, but will probably do so fairly soon. There are some significant infrastructural changes to Differential which will form part of the changes, and which could possibly resolve the minor issue Albert pointed out in the first mail of this thread. > > Nevertheless I got quite used to Phabricator, at least Differential, and > don't really miss RB especially since it got painfully slow ever since a > gazillion Frameworks repositories were created. In conjunction with Plasma > 5.9's drag and drop notification feature and Spectacle creating illustrated > revisions is a breeze. Thanks for the positive feedback. Cheers, Ben
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
> 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 Agreed. Especially since arc is some mental php script that randomly amends wrong revisions and/or adds files deliberately left unstaged. As for the context I usually end up uploading diffs with -U99 but that's not ideal and it had never been an issue with ReviewBoard. I also find the way to access revision patchset diffs cumbersome. The fact that online comments aren't versioned and stick around even if the diff changed significantly in later iterations is quite annoying. They also don't provide code snippets in the list of comments, you always have to jump to the specific line to see what's up there. The repository search when uploading a diff is also pretty dumb, always defaulting to the least sensible option: Querying for "plasma workspace" automatically chooses Plasma Workspace Wallpapers, "kio" ends up with KIO AudioCD or Gopher... Nevertheless I got quite used to Phabricator, at least Differential, and don't really miss RB especially since it got painfully slow ever since a gazillion Frameworks repositories were created. In conjunction with Plasma 5.9's drag and drop notification feature and Spectacle creating illustrated revisions is a breeze.
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Sat, Feb 4, 2017 at 11:41 AM, Albert Astals Cid 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 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? > > Cheers, > Albert Regards, Ben
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 wrote: > > On Fri, Feb 3, 2017 at 12:18 PM, Albert Astals Cid 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
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 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. > > 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 Cheers, Albert
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Fri, Feb 3, 2017 at 9:06 AM, Ben Cooksley wrote: > On Fri, Feb 3, 2017 at 12:18 PM, Albert Astals Cid 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. > > 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
Re: Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
On Fri, Feb 3, 2017 at 12:18 PM, Albert Astals Cid 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 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
Phabricator differential is not good - WAS - Re: Phabricator: All repositories registered - upcoming workflow changes
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 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. Cheers, Albert
