Page MenuHomePhabricator
Create Task
Maniphest T11738

Improve the heuristic which guesses which function/method/class line best provides context for diff snippets
Open, WishlistPublic
Actions

Description

Even with diffs that have full-file context available, the diff is shown without a line showing which function each change is in. Showing the function as "git diff" or "diff --show-c-function" do would help a lot to put the change in context and aid reviewing.

Seen with C++ code in e.g. https://phabricator.kde.org/D2934 [1] but also in https://secure.phabricator.com/D16677 : src/applications/conpherence/application/PhabricatorConpherenceApplication.php

[1] "git log -p" for this change shows:

diff --git a/daemon/powerdevilcore.cpp b/daemon/powerdevilcore.cpp
index 30adb64..6c264ac 100644
--- a/daemon/powerdevilcore.cpp
+++ b/daemon/powerdevilcore.cpp
@@ -388,6 +388,8 @@ void Core::loadProfile(bool force)
     }
 
     // If the lid is closed, retrigger the lid close signal
+    // so that "switching profile then closing the lid" has the same result as
+    // "closing lid then switching profile".
     if (m_backend->isLidClosed()) {
         Q_EMIT m_backend->buttonPressed(PowerDevil::BackendInterface::LidClose);
     }

i.e. it includes the enclosing "void Core::loadProfile(bool force)" function signature in the context.

(I notice your syntax highlighting agrees that c-function contexts are useful! )

Revisions and Commits

rP Phabricator
D20171rP92abe3c8fb84 Extract scope line selection logic from the diff rendering engine so it can…

Related Objects

Event Timeline

oliverhenshaw created this task.Oct 6 2016, 4:31 PM
Herald added a subscriber: eadler. · View Herald TranscriptOct 6 2016, 4:31 PM
chad added a subscriber: chad.Oct 6 2016, 5:01 PM

Are you referring to the function in the grey area above the diff?

pasted_file (234×1 px, 79 KB)

oliverhenshaw added a comment.Oct 6 2016, 5:10 PM

I'm referring to that area in general, but I'm talking about e.g. the src/applications/conpherence/application/PhabricatorConpherenceApplication.php section. The grey area above the diff shows

'/conpherence/' => array(

when it should say something like

public function getRoutes() {
chad added a comment.Oct 6 2016, 5:19 PM

What problem specifically are you trying to solve? See Contributing Feature Requests and Describing Root Problems.

chad added a subscriber: epriestley.Oct 6 2016, 5:25 PM
epriestley triaged this task as Wishlist priority.Oct 6 2016, 5:42 PM

We use a heuristic to guess which function, class, etc., the context is in. Git uses some similar heuristic.

This heuristic we use is crude, and primarily based on indentation levels. In some cases, it produces especially bad results -- for example, if code is written in this style:

if (condition)
{
  ...
}

..I believe we select { as the context for the ... content, which is useless.

There are some ways we can improve the heuristic in general (for example, ignore line containing a single opening { as candidates for context inclusion).

However, at some point I expect that we'll be unable to continue meaningfully improving the heuristic in general, and need to start adding rules based on the file language (for example, look for function signatures in C, keywords like "function" and "class" in PHP, patterns like ".prototype" in Javascript, lots of opening braces in Lisp, etc).

Getting the best possible result (e.g., the line a human would select) in all cases requires us to have substantial knowledge of how to parse every programming language. We can modularize this and chip away at it, but it's a bottomless amount of work.

epriestley renamed this task from add function to diff context to Improve the heuristic which guesses which function/method/class line best provides context for diff snippets.Oct 6 2016, 5:43 PM
epriestley moved this task from Backlog to Future Work on the Differential board.Dec 29 2016, 3:41 PM
epriestley moved this task from Future Work to Far Future Work on the Differential board.
epriestley mentioned this in T9758: Diff context for Objective-C doesn't show method name if open brace is on newline.Dec 29 2016, 4:37 PM
epriestley merged a task: T9758: Diff context for Objective-C doesn't show method name if open brace is on newline.
epriestley added subscribers: edibiase, shrinidhirao, joshuaspence.
epriestley mentioned this in T12525: amckinley's Onboarding.Apr 9 2017, 1:46 PM
epriestley mentioned this in T13249: 2019 Week 7 - 10 Bonus Content.Feb 13 2019, 12:59 PM
epriestley added a comment.Feb 14 2019, 5:49 PM

However, at some point I expect that we'll be unable to continue meaningfully improving the heuristic in general, and need to start adding rules based on the file language (for example, look for function signatures in C, keywords like "function" and "class" in PHP, patterns like ".prototype" in Javascript, lots of opening braces in Lisp, etc).

This is, of course, what git itself does, in userdiff.c:

userdiff.c
...

PATTERNS("php",
	 "^[\t ]*(((public|protected|private|static)[\t ]+)*function.*)$\n"
	 "^[\t ]*((((final|abstract)[\t ]+)?class|interface|trait).*)$",
	 /* -- */
	 "[a-zA-Z_][a-zA-Z0-9_]*"
	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+"
	 "|[-+*/<>%&^|=!.]=|--|\\+\\+|<<=?|>>=?|===|&&|\\|\\||::|->"),

...
epriestley added a revision: D20171: Extract scope line selection logic from the diff rendering engine so it can reasonably be iterated on.Feb 14 2019, 6:11 PM
epriestley added a commit: rP92abe3c8fb84: Extract scope line selection logic from the diff rendering engine so it can….Feb 19 2019, 6:55 PM
epriestley mentioned this in 2019 Week 8 (Late February).Feb 23 2019, 2:18 PM
epriestley mentioned this in T13263: Plans: 2019 Week 11-15 Bonus Content.Mar 9 2019, 6:43 PM
epriestley mentioned this in T13289: Plans: 2019 Week 21-23 Bonus Content.May 16 2019, 9:37 PM
Phabricator · Built by Phacility