Re: [PATCH 8/8] diff.c: rewrite emit_line_0 more understandably

[Stefan Beller <sbeller@google.com>]

On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Fri, 10 Aug 2018, Stefan Beller wrote:
>
> > emit_line_0 has no nested conditions, but puts out all its arguments
> > (if set) in order.
>
> Well, currently `emit_line_0()` *has* nested conditions: `first == '\n'`
> inside `len == 0`.
>
> And these nested conditions make things hard to read, so resolving that
> to a simpler workflow makes a ton of sense. You can sell that better by
> starting the commit message with something along the lines that you are
> making the overly complex logic easier to follow.
>
> > There is still a slight confusion with set and set_sign, but let's defer
> > that to a later patch.
>
> There is no later patch in this here patch series.

yup, I got overly excited when writing this commit message, as for
what we could try next.

> I would therefore
> appreciate it if you could spend the paragraph or two on explaining
> yourself here.

noted.

>
> > at "diff: add an internal option to dual-color diffs of diffs",
> > 2018-07-21), as there we broadened the meaning of 'first' to also
> > signal an early return.
>
> Did we? I thought we introduced the possibility of passing *just* a first
> character or *just* a "rest of the line". I might misremember, though.
>
> > The change in 'emit_line' makes sure that 'first' is never content, but
>
> <tongue-in-cheek>Awwww, you want to make 'first' sad all the time? That's
> not nice of you...</tongue-in-cheek>
>
> Seriously again, the adjective "content" has a different meaning than the
> noun and this ambiguity made this sentence very hard for me to parse.

So what is a good noun for the stuff that Git stores? ("stuff" is not a
good one)

> > always under our control, a sign or special character in the beginning
> > of the line (or 0, in which case we ignore it).
>
> It would be good to reword this paragraph to say that from now on, we will
> only pass a diff marker as `first`, or 0.

Makes sense.



> > +     if (!len && !first)
> > +             goto end_of_line;
> > +
> > +     if (reverse && want_color(o->use_color)) {
>
> Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*
> `set_sign` were passed in, and since a non-`NULL` `set` *implies* that we
> want color, you could drop that `want_color(o->use_color)` here.
>
> But as I stated above, I am not a fan of having such unintuitive
> implications in the code.

okay. So we'd want to be explicit about reverse again?

> > +     fwrite(line, len, 1, file);
> > +     needs_reset |= len > 0;
>
> First of all, this should be a `||=`, not a `|=`.
>
> And then, this code is skipped by the `if (!len) goto end_of_line;` part
> above, so `len > 0` is *always* 1 at this point.
>
> But then, I wonder why we bother all that much. After all, we want to
> reset whenever we used color. So why not simply initialize
>
>         int need_reset = reverse || set_sign || set;
>
> and be done with it?

I just removed that line and all tests still pass. :/
I could have sworn of a failing test when writing the code
that ensures that content (noun), that contains color codes,
would still look okay by having a reset at the end of the line,
so really we'd need to have

  need_reset = reverse || set_sign || set || (len > 0);

I'll dig into this again.

Thanks,
Stefan