| Message ID | 20190705170630.27500-11-t.gummerer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/14] apply: replace marc.info link with public-inbox | expand |
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@gmail.com> > --- > 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 > >
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")); + 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>
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. Most likely this is being done because line numbers in the hunk header are not relevant without other changes. 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. 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. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- range-diff.c | 8 +++++--- t/t3206-range-diff.sh | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-)