Re: [PATCH v2 12/14] range-diff: add section header instead of diff header

Hi Thomas,

On Mon, 8 Jul 2019, Thomas Gummerer wrote:

On 07/05, Johannes Schindelin wrote:

quoted

quoted

 			 */
 			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...

No, I can't think of a good way.  I'm also not sure it would be right
to remove it.  In this case it means that in the previous version this
was only called 'file', while in the new version it was renamend in
this patch to 'renamed-file', so it does give some useful information.

Oh, I misunderstood! You're right, this is useful information, and I just
have to learn how to read that variant of the range-diffs.

In other words: please leave this part of your patch series as-is.

quoted

quoted

@@ -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?

Sure.  The actual change (in the range-diff) here is that "Also a
silly comment here!" was added to the commit message.  The diff header
is context lines after that.

We now replace the diff header with the new "section header", which is
only a single line, so we get a couple of additional lines of the
context of the subsequent inner diff.

You know what? This is my typical mistake when reading uncolored
range-diffs: _of course_ I missed that this is talking about context
lines. I really thought they were added by the new iteration. My bad. And
thanks for explaining this patiently to me.

quoted

quoted

 	:<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?

It is, and it still exists.  I'm not entirely familar with the format
for fast-export/fast-import scripts.  What I did was just fast-import
the existing script, create the new refs that were required for the
tests and then fast-export'ed it again.

So not sure exactly why this changed, but the 'removed' ref still
exists :)

Right, it would still exist because earlier parts of the script would have
created that ref (otherwise `reset refs/heads/removed` would have failed).

Your strategy to update the script sounds like the best way to go, it just
runs afoul of topological ordering that somehow makes this patch look as
if you had removed parts of the commit history. I believe you when you say
that you didn't, of course.

Thanks,
Dscho