Hi Thomas,
On Fri, 5 Jul 2019, Thomas Gummerer wrote:
> When postprocessing the inner diff in range-diff, we currently replace
> the whole hunk header line with just "@@". This matches how 'git
> tbdiff' used to handle hunk headers as well.
Right, that's why I did it this way: `tbdiff` did it.
> Most likely this is being done because line numbers in the hunk header
> are not relevant without other changes.
Yep, and even worse, it also leads to uninteresting differences.
> They can for example easily change if a range is rebased, and lines are
> added/removed before a change that we actually care about in our ranges.
Exactly.
> However it can still be useful to have the function name that 'git
> diff' extracts as additional context for the change.
>
> Note that it is not guaranteed that the hunk header actually shows up
> in the range-diff, and this change only aims to improve the case where
> a hunk header would already be included in the final output.
Very important paragraph here, thank you!
> Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> ---
> range-diff.c | 8 +++++---
> t/t3206-range-diff.sh | 6 +++---
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 916afa44c0..484b1ec5a9 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -113,9 +113,11 @@ static int read_patches(const char *range, struct string_list *list)
> strbuf_addch(&buf, '\n');
> }
> continue;
> - } else if (starts_with(line, "@@ "))
> - strbuf_addstr(&buf, "@@");
> - else if (!line[0] || starts_with(line, "index "))
> + } else if (skip_prefix(line, "@@ ", &p)) {
> + if (!(p = strstr(p, "@@")))
> + die(_("invalid hunk header in inner diff"));
That's a bit more strict than the preimage. How about
p = strstr(p, "@@");
strbuf_addstr(&buf, p ? p : "@@");
instead?
The rest of the patch looks fine to me,
Dscho
> + strbuf_addstr(&buf, p);
> + } else if (!line[0] || starts_with(line, "index "))
> /*
> * A completely blank (not ' \n', which is context)
> * line is not valid in a diff. We skip it
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 048feaf6dd..aebd4e3693 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -110,7 +110,7 @@ test_expect_success 'changed commit' '
> 14
> 4: a63e992 ! 4: d966c5c s/12/B/
> @@ -8,7 +8,7 @@
> - @@
> + @@ A
> 9
> 10
> - B
> @@ -169,7 +169,7 @@ test_expect_success 'changed commit with sm config' '
> 14
> 4: a63e992 ! 4: d966c5c s/12/B/
> @@ -8,7 +8,7 @@
> - @@
> + @@ A
> 9
> 10
> - B
> @@ -231,7 +231,7 @@ test_expect_success 'dual-coloring' '
> : 14<RESET>
> :<RED>4: d966c5c <RESET><YELLOW>!<RESET><GREEN> 4: 8add5f1<RESET><YELLOW> s/12/B/<RESET>
> : <REVERSE><CYAN>@@ -8,7 +8,7 @@<RESET>
> - : <CYAN> @@<RESET>
> + : <CYAN> @@ A<RESET>
> : 9<RESET>
> : 10<RESET>
> : <REVERSE><RED>-<RESET><FAINT> BB<RESET>
> --
> 2.22.0.510.g264f2c817a
>
>
