Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change

From: Stefan Beller <sbeller@google.com>
Date: Oct 9, 2018, 21:10 UTC
Message-ID: <CAGZ79kYjeqME-tt89Fp=Wt0hAW0FVAyZ00ftN5XTOkFSn7Kq9A@mail.gmail.com>
In-Reply-To: <b3d29d34-616d-5d12-bb86-19ea488a766d@talktalk.net>

> As I said above I've more or less come to the view that the correctness
> of pythonic indentation is orthogonal to move detection as it affects
> all additions, not just those that correspond to moved lines.

Makes sense.

Show 6 quoted lines

> > What is your use case, what kind of content do you process that
> > this patch would help you?
>
> I wrote this because I was re-factoring some shell code than was using a
> indentation step of four spaces but with tabs in the leading indentation
> which the current mode does not handle.

Ah that is good to know.

I was thinking whether we want to generalize the move detection into a more generic "detect and fade out uninteresting things" and not just focus on white spaces (but these are most often the uninteresting things).

Over the last year we had quite a couple of large refactorings, that
would have helped by that:
* For example the hash transition plan had a lot of patches that
  were basically s/char *sha1/struct object oid/ or some variation thereof.
* Introducing struct repository

I used the word diff to look at those patches, which helped a lot, but maybe a mode that would allow me to mark this specific replacement uninteresting would be even better. Maybe this can be done as a piggyback on top of the move detection as a "move in place, but with uninteresting pattern". The problem of this is that the pattern needs to be accounted for when hashing the entries into the hashmaps, which is easy when doing white spaces only.

Show 20 quoted lines

> >> +       if (a->s == DIFF_SYMBOL_PLUS)
> >> +               *delta = la - lb;
> >> +       else
> >> +               *delta = lb - la;
> >
> > When writing the original feature I had reasons
> > not to rely on the symbol, as you could have
> > moved things from + to - (or the other way round)
> > and added or removed indentation. That is what the
> > `current_longer` is used for. But given that you only
> > count here, we can have negative numbers, so it
> > would work either way for adding or removing indentation.
> >
> > But then, why do we need to have a different sign
> > depending on the sign of the line?
>
> The check means that we get the same delta whichever way round the lines
> are compared. I think I added this because without it the highlighting
> gets broken if there is increase in indentation followed by an identical
> decrease on the next line.

But wouldn't we want to get that highlighted? I do not quite understand the scenario, yet. Are both indented and dedented part of the same block?

Show 12 quoted lines

> >
> >> +       } else {
> >> +               BUG("no color_moved_ws_allow_indentation_change set");
> >
> > Instead of the BUG here could we have a switch/case (or if/else)
> > covering the complete space of delta->have_string instead?
> > Then we would not leave a lingering bug in the code base.
>
> I'm not sure what you mean, we cover all the existing
> color_moved_ws_handling values, I added the BUG() call to pick up future
> omissions if another mode is added. (If we go for a single mode none of
> this matters)

Ah, makes sense!