From: Phillip Wood Date: Wed, 10 Oct 2018 15:26:54 GMT Subject: Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change Message-ID: In-Reply-To: On 09/10/2018 22:10, Stefan Beller wrote: >> 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. Right so are you happy for we to re-roll with a single allow-indentation-change mode based on my RFC? > >>> 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. Yes the I like the idea. Yesterday I was looking at Alban's patches to refactor the todo list handling for rebase -i and there are a lot of '.' to '->' changes which weren't particularly interesting though at least diff-highlight made it clear if that was the only change on a line. Incidentally --color-moved was very useful for looking at that series. >>>> + 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? With --color-moved=zebra the indented lines and the de-indented lines should be different colors, without the test they both ended up in the same block. Best Wishes Phillip >>> >>>> + } 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! >