Re: [PATCH v2 3/4] add -p: handle `diff-so-fancy`'s hunk headers better
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 29, 2022, 13:32 UTC
- Message-ID
- <0q0psp09-8993-96r6-3r90-q4s368p98510@tzk.qr>
- In-Reply-To
- <xmqq5yibqxs0.fsf@gitster.g>
Hi Junio,
On Mon, 29 Aug 2022, Junio C Hamano wrote:
Show 11 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > else > > /* could not parse colored hunk header, showing nothing */ > > - header->colored_extra_start = hunk->colored_start; > > + header->colored_extra_start = line - s->colored.buf; > > This is the only thing that corresponds to the proposed log message. > The comment that says "showing nothing" is no longer correct, and > needs to be updated.
Correct.
> Everything else in this patch is about adding an extra space > depending on what is in the "funcname" part.
... because that logic was not relevant before this commit.
Show 9 quoted lines
> The code does not know or care if it is an attempt to do diff-so-fancy's > headers better by doing something we didn't do to the normal header we > managed to have parsed; rather the extra space thing is done > unconditionally and does not know or care if extra_start is truly after > " @@" or a place in the line the new code computed. > > So the following three hunks either need to be made into a separate > patch, or deserves to be explained in a new paragraph in the > proposed log message.
I've opted to split the changes out into their own patch because it improves the reviewability of the patch series.
Show 17 quoted lines
> > header->colored_extra_end = hunk->colored_start;
> >
> > return 0;
> > @@ -649,6 +650,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> > size_t len;
> > unsigned long old_offset = header->old_offset;
> > unsigned long new_offset = header->new_offset;
> > + int needs_extra_space = 0;
> >
> > if (!colored) {
> > p = s->plain.buf + header->extra_start;
> > @@ -658,6 +660,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> > p = s->colored.buf + header->colored_extra_start;
> > len = header->colored_extra_end
> > - header->colored_extra_start;
> > + if (utf8_strnwidth(p, len, 1 /* skip ANSI */) > 0)
> > + needs_extra_space = 1;Let me add a review of my own: This hunk is incorrect ;-)
Here is why: in the _regular_ case, i.e. when we have a colored hunk header like `@@ -936 +936 @@ wow()`, we manage to parse the line range, and then record the offset of the extra part that starts afterwards.
This extra part is non-empty, therefore we add an extra space.
But that part already starts with a space, so now we end up with two spaces.
A fix for this will be part of the next iteration.
Ciao, Dscho
Show 34 quoted lines
> > } > > > > if (s->mode->is_reverse) > > @@ -673,6 +677,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, > > strbuf_addf(out, ",%lu", header->new_count); > > strbuf_addstr(out, " @@"); > > > > + if (needs_extra_space) > > + strbuf_addch(out, ' '); > > if (len) > > strbuf_add(out, p, len); > > else if (colored) > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > > index 7e3c1de71f5..9deb7a87f1e 100755 > > --- a/t/t3701-add-interactive.sh > > +++ b/t/t3701-add-interactive.sh > > @@ -772,7 +772,8 @@ test_expect_success 'gracefully fail to parse colored hunk header' ' > > echo content >test && > > test_config interactive.diffFilter "sed s/@@/XX/g" && > > printf y >y && > > - force_color git add -p <y > > + force_color git add -p >output 2>&1 <y && > > + grep XX output > > ' > > It is good to make sure that XX that was munged appears in the > output. This however does not check anything about the > needs-extra-space logic. It should. If the extra-space logic is > moved to a separate patch, then this step can have the above test, > but then the separate patch needs to update it to check the > additional logic. > > Other than that, looking very good. >