[16:14:43] MaxSem: you deprecated wfDiff() a while ago ( https://gerrit.wikimedia.org/r/c/mediawiki/core/+/198785 ) but not $wgDiff [16:14:50] what was the plan with that? [16:15:03] should the entire concept of external diffing be killed eventually? [17:08:38] tgr: I don't get the diff-bin part, I don't have real data to test this tgr, does it mean it's a no-op now? [17:09:21] all it did was change $wgDiff which would then be used by wfDiff [17:09:58] and now that wfDiff is going away, it means it's of no use, okay I get it, thanks [17:10:17] tgr: Will it make sense to deprecate $wgDiff in that patch? I guess not [17:10:31] Or maybe in a later patch after MaxSem's signal? [17:11:10] not in that patch, no [17:11:34] but the diff-bin parameter and the related code should be removed from that script [17:13:14] okay, updating PS, thanks [17:13:47] on second thought, no harm doing it in the same patch [17:14:25] tgr: yeah, updating that patch now to remove diff-bin related code [17:14:43] I guess deprecating $wgDiff can come later [17:15:35] tgr: show-diff also has to go [17:16:58] I meant to say $this->showDiff sorry [17:17:36] why? [17:19:10] There is a check below that changed $wgDiff [17:19:17] And that check depends on diff-bin [17:19:33] It saves the value in $this->showDiff [17:19:38] * xSavitar looks again.... [17:20:06] Oh, there is another check below, I think that can stay [17:20:14] Just the if() can go away [17:41:36] Krinkle: thanks! tgr, so we're deprecating $wgDiff in favor of $wgDiff3 which is the fall back right? I see that $wgDiff3 will fallback to old behavior which is now being removed in the patch being worked on [17:41:52] Does that completely eliminate the fallback possibility? [17:42:05] I don't think $wgDiff3 has anything to do with this [17:42:17] that's for 3-way merge [17:42:26] so probably edit conflicts [17:42:33] Hmm..., I'm missing something [17:42:36] * xSavitar looks again [17:42:59] (having $wgDiff, wikidiff2() and $wgDiff3 does sound like some sort of versioning scheme, but it actually isn't) [17:43:38] yeah [17:44:11] the Diff class (apparently) used to be the fallback for $wgDiff, which is typically GNU diff [17:44:47] then wfDiff was deprecated on account Diff now being faster in most situations [17:45:08] I understand your messages [17:46:29] thanks [17:47:53] Krinkle, tgr, it really indeed needs deprecation, see: https://codesearch.wmflabs.org/search/?q=global%20%5C%24wgDiff%3B&i=nope&files=&repos= [17:48:11] No more usage apart from where we want to remove it [17:48:27] xSavitar: Your search is naieve [17:48:39] That doesn't work for global $wgDiff, $wgParser; [17:49:17] Reedy: sorry [17:49:21] Better one: https://codesearch.wmflabs.org/search/?q=%5CbwgDiff%5Cb&i=nope&files=&repos= ? [17:49:48] As it happens, it does find most of the usages... [17:50:46] Yeah, sorry about the previous search, was really bad :( [17:51:47] No need to apologise [17:51:55] Just trying to help you so you don't get tripped up over these things :) [17:52:35] Reedy: And I really appreciate, thanks! [18:35:48] tgr: I deprecated it because there should be no diffing outside of DIfferenceEngine, and because generating a diff via an external program is a bad thing [18:36:37] DifferenceEngine doesn't really deal with non-content diffs though? [18:36:48] nor output formats other than HTML, sadly [18:37:22] but yeah, it seems unlikely shelling out for every diff would improve performance [18:37:39] OTOH $wgDiff3 / external content diff is not deprecated [18:37:48] so it's a bit of a mess [18:40:09] Nah, we have UnifiedDiffFormatter [19:22:22] MaxSem: DifferenceEngine can use Diff + a formatter or $wgDiff3 or wikidiff2 [19:22:34] wikidiff2 has no non-HTML output format [19:23:01] so DifferenceEngine can't have one, without breaking compatibility with wikidiff2-using wikis (like Wikimedia) [19:23:42] so it can't be used for plain-text diffs, which is what wfDiff was used for [20:56:40] anomie: hi, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/505430 is tricky [20:57:03] If we do it on the caller side, how can I avoid hitting the DB? [20:58:08] IndexPager tries building a query which at the moment doesn't exist since I'm just checking if the username is valid and return otherwise [20:58:51] But that return doesn't work on the caller side as getQueryInfo requires an array of the query [21:00:38] Special:Contributions does this but a little different? [21:08:28] ContribPager has this problem as well :(, it doesn't check if username is invalid, see: https://github.com/wikimedia/mediawiki/blob/master/includes/specials/pagers/ContribsPager.php#L272 [21:11:12] Hmm.. I figured something out, let me try it first :) [21:12:18] We don't even need to get to the NewFilesPager if the username is invalid but it's in that class that the validation of the username takes place [21:12:33] So what I think I can try doing is to perform a validation check before getting into NewFilesPager [21:17:16] * xSavitar smiles as problem is fixed :) [21:17:31] anomie: Ignore all my messages for now, you'll see the revised PS, thanks [21:58:52] tgr, Reedy, not sure if I have a better wording for the newly added key in this patch: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/505430 [21:59:03] Does that one look good? [22:26:03] xSavitar: typically you'd want some kind of namespace prefix so keys used in the same component go together and there's less chance of intercomponent conflict [22:26:27] so newfiles-invalid-user or something like that [22:27:03] or possibly newfiles-error-invalid-user to group further by function, that depends from component to component [22:27:15] check how the other keys are named, if there are any [22:27:45] Okay, thanks a lot [22:49:58] tgr: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/TitleBlacklist/+/507516