Re: [PATCH v2 12/14] range-diff: add section header instead of diff header
From: Johannes Schindelin <hidden>
Date: 2019-07-05 19:35:05
Hi Thomas, On Fri, 5 Jul 2019, Thomas Gummerer wrote:
Currently range-diff keeps the diff header of the inner diff intact (apart from stripping lines starting with index). This diff header is somewhat useful, especially when files get different names in different ranges. However there is no real need to keep the whole diff header for that. The main reason we currently do that is probably because it is easy to do. Introduce a new range diff hunk header, that's enclosed by "##", similar to how line numbers in diff hunks are enclosed by "@@", and give human readable information of what exactly happened to the file, including the file name. This improves the readability of the range-diff by giving more concise information to the users. For example if a file was renamed in one iteration, but not in another, the diff of the headers would be quite noisy. However the diff of a single line is concise and should be easier to understand. Additionaly, this allows us to add these range diff section headers to
s/Additionaly/Additionally/
the outer diffs hunk headers using a custom userdiff pattern, which should help making the range-diff more readable.
Makes sense.
quoted hunk ↗ jump to hunk
Signed-off-by: Thomas Gummerer <redacted> --- range-diff.c | 35 ++++++++++++---- t/t3206-range-diff.sh | 91 +++++++++++++++++++++++++++++++++++++++--- t/t3206/history.export | 84 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 193 insertions(+), 17 deletions(-)diff --git a/range-diff.c b/range-diff.c index b31fbab026..cc01f7f573 100644 --- a/range-diff.c +++ b/range-diff.c@@ -10,6 +10,7 @@ #include "commit.h" #include "pretty.h" #include "userdiff.h" +#include "apply.h" struct patch_util { /* For the search for an exact match */@@ -95,12 +96,36 @@ static int read_patches(const char *range, struct string_list *list) } if (starts_with(line, "diff --git")) { + struct patch patch;
If you append ` = { 0 }` (or ` = { NULL }`, depending on the first field
of that struct, you don't need that `memset()` later.
+ struct strbuf root = STRBUF_INIT; + int linenr = 0; + in_header = 0; strbuf_addch(&buf, '\n'); if (!util->diff_offset) util->diff_offset = buf.len; - strbuf_addch(&buf, ' '); - strbuf_addstr(&buf, line); + memset(&patch, 0, sizeof(patch)); + line[len - 1] = '\n';
I guess `parse_git_header()` cannot handle lines ending in a NUL?
+ len = parse_git_header(&root, &linenr, 1, line,
+ len, size, &patch);
+ if (len < 0)
+ die(_("could not parse git header"));Maybe include the line's contents, like ` '%.*s'", (int)len, line`?
quoted hunk ↗ jump to hunk
+ strbuf_addstr(&buf, " ## "); + if (patch.is_new > 0) + strbuf_addf(&buf, "%s (new)", patch.new_name); + else if (patch.is_delete > 0) + strbuf_addf(&buf, "%s (deleted)", patch.old_name); + else if (patch.is_rename) + strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name); + else + strbuf_addstr(&buf, patch.new_name); + + if (patch.new_mode && patch.old_mode && + patch.old_mode != patch.new_mode) + strbuf_addf(&buf, " (mode change %06o => %06o)", + patch.old_mode, patch.new_mode); + + strbuf_addstr(&buf, " ##"); } else if (in_header) { if (starts_with(line, "Author: ")) { strbuf_addstr(&buf, line);@@ -117,17 +142,13 @@ static int read_patches(const char *range, struct string_list *list) if (!(p = strstr(p, "@@"))) die(_("invalid hunk header in inner diff")); strbuf_addstr(&buf, p); - } else if (!line[0] || starts_with(line, "index ")) + } else if (!line[0]) /* * A completely blank (not ' \n', which is context) * line is not valid in a diff. We skip it * silently, because this neatly handles the blank * separator line between commits in git-log * output. - * - * We also want to ignore the diff's `index` lines - * because they contain exact blob hashes in which - * we are not interested.
Oh, are we interested in them again?
quoted hunk ↗ jump to hunk
*/ continue; else if (line[0] == '>') {diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 9f89af7178..c277756057 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh@@ -181,6 +181,85 @@ test_expect_success 'changed commit with sm config' ' test_cmp expected actual ' +test_expect_success 'renamed file' ' + git range-diff --no-color --submodule=log topic...renamed-file >actual && + sed s/Z/\ /g >expected <<-EOF && + 1: 4de457d = 1: f258d75 s/5/A/ + 2: fccce22 ! 2: 017b62d s/4/A/ + @@ + ZAuthor: Thomas Rast <trast@inf.ethz.ch> + Z + - s/4/A/ + + s/4/A/ + rename file + Z + - ## file ## + + ## file => renamed-file ##
I guess there is no good way to suppress the `- ## file ##` line in this case? It is a bit distracting...
quoted hunk ↗ jump to hunk
+ Z@@ + Z 1 + Z 2 + 3: 147e64e ! 3: 3ce7af6 s/11/B/ + @@ + Z + Z s/11/B/ + Z + - ## file ## + + ## renamed-file ## + Z@@ A + Z 8 + Z 9 + 4: a63e992 ! 4: 1e6226b s/12/B/ + @@ + Z + Z s/12/B/ + Z + - ## file ## + + ## renamed-file ## + Z@@ A + Z 9 + Z 10 + EOF + test_cmp expected actual +' + +test_expect_success 'file added and later removed' ' + git range-diff --no-color --submodule=log topic...added-removed >actual && + sed s/Z/\ /g >expected <<-EOF && + 1: 4de457d = 1: 096b1ba s/5/A/ + 2: fccce22 ! 2: d92e698 s/4/A/ + @@ + ZAuthor: Thomas Rast [off-list ref] + Z + - s/4/A/ + + s/4/A/ + new-file + Z + Z ## file ## + Z@@ + @@ + Z A + Z 6 + Z 7 + + + + ## new-file (new) ## + 3: 147e64e ! 3: 9a1db4d s/11/B/ + @@ + ZAuthor: Thomas Rast [off-list ref] + Z + - s/11/B/ + + s/11/B/ + remove file + Z + Z ## file ## + Z@@ A + @@ + Z 12 + Z 13 + Z 14 + + + + ## new-file (deleted) ## + 4: a63e992 = 4: fea3b5c s/12/B/ + EOF + test_cmp expected actual +' + test_expect_success 'no commits on one side' ' git commit --amend -m "new message" && git range-diff master HEAD@{1} HEAD@@ -197,9 +276,9 @@ test_expect_success 'changed message' ' Z + Also a silly comment here! + - Z diff --git a/file b/file - Z --- a/file - Z +++ b/file + Z ## file ## + Z@@ + Z 1 3: 147e64e = 3: b9cb956 s/11/B/ 4: a63e992 = 4: 8add5f1 s/12/B/ EOF@@ -216,9 +295,9 @@ test_expect_success 'dual-coloring' ' : <RESET> : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET> : <REVERSE><GREEN>+<RESET> - : diff --git a/file b/file<RESET> - : --- a/file<RESET> - : +++ b/file<RESET> + : ## file ##<RESET> + : <CYAN> @@<RESET> + : 1<RESET>
I am a bit confused where these last two lines come from all of a sudden... They were not there before, and I do not see any code change in this patch that would be responsible for them, either... Could you help me understand?
quoted hunk ↗ jump to hunk
:<RED>3: 0559556 <RESET><YELLOW>!<RESET><GREEN> 3: b9cb956<RESET><YELLOW> s/11/B/<RESET> : <REVERSE><CYAN>@@<RESET> : 9<RESET>diff --git a/t/t3206/history.export b/t/t3206/history.export index b8ffff0940..7bb3814962 100644 --- a/t/t3206/history.export +++ b/t/t3206/history.export@@ -22,8 +22,8 @@ data 51 19 20 -reset refs/heads/removed -commit refs/heads/removed +reset refs/heads/renamed-file +commit refs/heads/renamed-file
Hmm. Is the `removed` ref no longer required by the 'removed a commit' test case?
quoted hunk ↗ jump to hunk
mark :2 author Thomas Rast [off-list ref] 1374424921 +0200 committer Thomas Rast [off-list ref] 1374484724 +0200@@ -599,6 +599,82 @@ s/12/B/ from :46 M 100644 :28 file -reset refs/heads/removed -from :47 +commit refs/heads/added-removed +mark :48 +author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200 +committer Thomas Gummerer <t.gummerer@gmail.com> 1556574151 +0100
Neat ;-)
+data 7 +s/5/A/ +from :2 +M 100644 :3 file + +blob +mark :49 +data 0 + +commit refs/heads/added-removed +mark :50 +author Thomas Rast [off-list ref] 1374485024 +0200 +committer Thomas Gummerer [off-list ref] 1556574177 +0100 +data 18 +s/4/A/ + new-file +from :48 +M 100644 :5 file +M 100644 :49 new-file + +commit refs/heads/added-removed +mark :51 +author Thomas Rast [off-list ref] 1374485036 +0200 +committer Thomas Gummerer [off-list ref] 1556574177 +0100 +data 22 +s/11/B/ + remove file +from :50 +M 100644 :7 file +D new-file + +commit refs/heads/added-removed +mark :52 +author Thomas Rast [off-list ref] 1374485044 +0200 +committer Thomas Gummerer [off-list ref] 1556574177 +0100 +data 8 +s/12/B/ +from :51 +M 100644 :9 file + +commit refs/heads/renamed-file +mark :53 +author Thomas Rast [off-list ref] 1374485014 +0200 +committer Thomas Gummerer [off-list ref] 1556574309 +0100 +data 7 +s/5/A/ +from :2 +M 100644 :3 file + +commit refs/heads/renamed-file +mark :54 +author Thomas Rast [off-list ref] 1374485024 +0200 +committer Thomas Gummerer [off-list ref] 1556574312 +0100 +data 21 +s/4/A/ + rename file +from :53 +D file +M 100644 :5 renamed-file + +commit refs/heads/renamed-file +mark :55 +author Thomas Rast [off-list ref] 1374485036 +0200 +committer Thomas Gummerer [off-list ref] 1556574319 +0100 +data 8 +s/11/B/ +from :54 +M 100644 :7 renamed-file + +commit refs/heads/renamed-file +mark :56 +author Thomas Rast [off-list ref] 1374485044 +0200 +committer Thomas Gummerer [off-list ref] 1556574319 +0100 +data 8 +s/12/B/ +from :55 +M 100644 :9 renamed-file
I have to admit that I allowed myself not to study this script too closely, trusting that the range-diff explains better what commit history it creates than the fast-import script. Thanks, Dscho
-- 2.22.0.510.g264f2c817a