Hi Junio,
On Mon, 29 Aug 2022, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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.
> 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.
> > 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
> > }
> >
> > 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.
>
