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!
