Should we change how hunk headers look?

[scottchiefbaker]

Raw diffs include the lines numbers encoded strangely:

Note the magenta line numbers surrounded by @@. Should we:

1. Get rid of them completely
2. Change them to "* Line #XX"
3. Something totally different
4. Leave them alone

[lode]

Yeah! More readability can be added there for sure.

As I decompile it, it is the line number where the diff starts (including context lines), and the amount of lines.

For myself, I'm mostly looking for the line start, but then also of the first changed line, so I can enter it in my editor and go there, exactly there. This is (as I understand), the third number + context lines. Something alike :42 or :42 @ function foo() where 42 is the line of the first changed/added/removed line. (The syntax is taken from how sublime uses this in the goto symbol/line dialog.)

But I guess other people use the presented data for other purposes?

[scottchiefbaker]

I reworked my script to capture the @@ lines and display them differently, I'm just not sure what "differently" looks like right now. I started off with just: "* Line #3 *" as that's raw from the diff.

I use it the same way you do, "where do I go after I open up the this file". Maybe it should be the line number + the context lines (i.e. go directly to the first changed line)?

[scottchiefbaker]

This is an example of what I've come up with so far:

───────────────────────────────────────────────────────────────────────────────────────
modified: include/version.inc
───────────────────────────────────────────────────────────────────────────────────────
* Changes start on line #22 *
 */

/* PHP 7.0 Release */
$PHP_7_0_RC = false; // Current RC version (e.g., '5.6.7RC1') or false
$PHP_7_0_RC_DATE = '21 Jan 2016';
$PHP_7_0_RC = "7.0.4RC1"; // Current RC version (e.g., '5.6.7RC1') or false
$PHP_7_0_RC_DATE = '18 Feb 2016';

$PHP_7_0_VERSION         = "7.0.3";
$PHP_7_0_DATE            = "04 Feb 2016";
* Changes start on line #39 * $PHP_7_0_SHA256     = array(
);

/* PHP 5.6 Release */
$PHP_5_6_RC = false; // Current RC version (e.g., '5.6.7RC1') or false
$PHP_5_6_RC_DATE = '21 Jan 2016';
$PHP_5_6_RC = '5.6.19RC1'; // Current RC version (e.g., '5.6.7RC1') or false
$PHP_5_6_RC_DATE = '18 Feb 2016';

$PHP_5_6_VERSION         = "5.6.18";
$PHP_5_6_DATE            = "04 Feb 2016";

[lode]

Looks great! (in the screenshot)

Wouldn't it be nice to add a newline in between blocks in the same file?

Further I think we could do without the "Changes start on line". After you've started using it, that will quickly become obsolete and just extra clutter.

[paulirish]

nice! yeah let's make this easier.

the official docs on the hunk header: https://github.com/git/git/blob/master/Documentation/diff-generate-patch.txt#L154-L185

it always starts with @@ -: https://github.com/git/git/blob/f02fbc4f9433937ee0463d0342d6d7d97e1f6f1e/diff.c#L732
and it'll always have the the -X,Y followed by the +A,B block, except for a file deletion where the first block will be just -X without a ,Y bit.

Things we could say:

Changes start on line 4
line 4 @
filename.ext:4 @

12 lines added
2 lines removed

file deleted

… proposal in next comment …

[paulirish]

My ideal might be the following:

-----------------------------------------------------------
modified: libs/header_clean/header_clean.pl
-----------------------------------------------------------
@ 22 lines added, header_clean.pl:3 @ 
use strict;
use warnings;
…

bigger and details:

@ 22 lines added, header_clean.pl:3 @

• hunk header still denoted with @'s for consistency, plus @ is very contexty.
• we do the math on # of lines added or removed.
  • on my team, delta of lines added/removed is very important. here we calculate for easy summarization, though admittedly the numbers are only at the hunk level.
• reuse the filename to print filename:XX line position
  • use the line position from the +A,B block here, as it's likely the active tree on disk
  • we could use the full path for cmd-clicking to exact line position... (e.g. libs/header_clean/header_clean.pl:3) but the filename at the file header is already cmd-clickable, so reprinting it at the hunk is probably overkill

[scottchiefbaker]

The problem I'm running in to is commits where the ONLY change is a perm change. Here the first commit is ONLY a perm change and then continues on with a regular addition:

diff --git a/circle.yml b/circle.yml
old mode 100644
new mode 100755
diff --git a/foo.json b/foo.json
new file mode 100644
index 0000000..316a815
--- /dev/null
+++ b/foo.json
@@ -0,0 +1,29 @@
+{
+  "name": "diff-so-fancy",
+  "version": "0.5.1",
+  "description": "Good-lookin' diffs with diff-highlight and more",
+  "bin": {

Currently we remove the diff --git line here: https://github.com/so-fancy/diff-so-fancy/blob/master/diff-so-fancy#L41, but I need that line to output WHAT file is getting the perm change.

I can remove that sed call, but that then breaks the whitespace line @paulirish wanted after each commit. Other than that little thing I think I have it all working. I even wrote tests!

[paulirish]

K. Let's move that piece into the Perl. It seems like it's still possible to handle the line spacing but I can look once your patch of up

[scottchiefbaker]

-----------------------------------------------------------
modified: libs/header_clean/header_clean.pl
-----------------------------------------------------------
@ 22 lines added, header_clean.pl:3 @ 

You really think putting the file name twice is necessary? It seems a little redundant and "noisy" to me. I was thinking something like:

-----------------------------------------------------------
modified: libs/header_clean/header_clean.pl
-----------------------------------------------------------
@ 22 lines added at line #3 @ 

[scottchiefbaker]

There are (number of parents + 1)@characters in the chunk header for combined diff format.

There may not always be two @ signs... That'll be "fun".

See: https://github.com/git/git/blob/master/Documentation/diff-generate-patch.txt#L91

[scottchiefbaker]

I don't know if we're going to be able to (easily) calculate lines changed. Take for example this diff:

@@ -1,6 +1,6 @@
 {
   "name": "diff-so-fancy",
-  "version": "0.5.1",
+  "version": "0.5.2",

The -1 indicates the line this hunk starts at, and the first 6 indicates how many lines are in the original file. The second 6 indicates the number of lines in the changed file. In this case one line was removed, and one was added.

In another example I removed 8 lines, and added 8 totally unrelated lines and the diff still showed -14,8 +14,8. There is no "math" to do to calculate the lines changed. 👎

[lode]

You really think putting the file name twice is necessary? It seems a little redundant and "noisy" to me.

You can copy-paste the file:line in Sublime to go to that file at that line.

In another example I removed 8 lines, and added 8 totally unrelated lines and the diff still showed -14,8 +14,8. There is no "math" to do to calculate the lines changed.

That's why GitHub only shows (and communicates in their API) the amount of additions and removals, and the amount of commits in a PR, but not the amount of changed lines.

[scottchiefbaker]

Lines added/removed really only makes sense in the context of a whole file or a whole pull request. I'm a fan of not including that on a per-hunk basis.

[paulirish]

There is no "math" to do to calculate the lines changed. 👎

okey doke. Let's leave that out of scope for now.

---

so I guess we end up with either

-----------------------------------------------------------
modified: libs/header_clean/header_clean.pl
-----------------------------------------------------------
@ header_clean.pl:3 @ 

or

-----------------------------------------------------------
modified: libs/header_clean/header_clean.pl
-----------------------------------------------------------
@ line 3 @ 

I have a preference for the former (assuming basename is trivial for perl). I don't think it's noisy, plus in larger diffs, i sometimes lose track which file i'm within.

[blueyed]

I don't think it's noisy, plus in larger diffs, i sometimes lose track which file i'm within.

👍

[scottchiefbaker]

I have a preference for the former (assuming basename is trivial for perl).

Everything is trivial in Perl :)

[paulirish]

\o/