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

From: Stefan Beller <sbeller@google.com>
Date: Aug 13, 2018, 18:57 UTC
Message-ID: <CAGZ79kZ=bs84WGeA5TbVUTbQL2ECUSwEJanMKmeWw+xKp-rH5Q@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808131429550.71@tvgsbejvaqbjf.bet>

On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 20 quoted lines

>
> 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.

Show 15 quoted lines

>
> > 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)

Show 5 quoted lines

> > 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.

Show 11 quoted lines

> > +     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?

Show 14 quoted lines

> > +     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