🐛 weird? handling of git colorMoved

[martinetd]

Hello,

thanks for delta!
(running on master's top, e7dbdd4)

I just noticed weird coloration on a commit.
After investigating it is due to git's diff.colorMoved = default, left without it and right with it:

I glanced through #72 but there are no open issues about colorMoved so preferred to open a new one; now I've read about it quite a bit I can say it's not a bug: manual/src/color-moved-support.md clearly states delta passes git colors unchanged.
Remarks after output.

I'm sure there are plenty of example, but I noticed it on linux's a6809941c1f17f455db2cf4ca19c6d8c8746ec25 ; here's part of the output (to use with sed -e 's/\\33/\x1b/g' or something similar):

\33[1mdiff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c\33[m
\33[1mindex 6619e3caffe2..7a285fb0f619 100644\33[m
\33[1m--- a/drivers/pci/controller/dwc/pci-imx6.c\33[m
\33[1m+++ b/drivers/pci/controller/dwc/pci-imx6.c\33[m
\33[36m@@ -408,6 +408,11 @@\33[m \33[mstatic void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)\33[m
 			dev_err(dev, \"failed to disable vpcie regulator: %d\
\",\33[m
 				ret);\33[m
 	}\33[m
\33[32m+\33[m
\33[1;36m+\33[m	\33[1;36m/* Some boards don't have PCIe reset GPIO. */\33[m
\33[32m+\33[m	\33[32mif (gpio_is_valid(imx6_pcie->reset_gpio))\33[m
\33[1;36m+\33[m		\33[1;36mgpio_set_value_cansleep(imx6_pcie->reset_gpio,\33[m
\33[1;36m+\33[m					\33[1;36mimx6_pcie->gpio_active_high);\33[m
 }\33[m
 \33[m
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)\33[m
\33[36m@@ -540,15 +545,6 @@\33[m \33[mstatic void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)\33[m
 	/* allow the clocks to stabilize */\33[m
 	usleep_range(200, 500);\33[m
 \33[m
\33[1;35m-	/* Some boards don't have PCIe reset GPIO. */\33[m
\33[1;35m-	if (gpio_is_valid(imx6_pcie->reset_gpio)) {\33[m
\33[1;34m-		gpio_set_value_cansleep(imx6_pcie->reset_gpio,\33[m
\33[1;34m-					imx6_pcie->gpio_active_high);\33[m
\33[1;35m-		msleep(100);\33[m
\33[1;35m-		gpio_set_value_cansleep(imx6_pcie->reset_gpio,\33[m
\33[1;35m-					!imx6_pcie->gpio_active_high);\33[m
\33[31m-	}\33[m
\33[31m-\33[m
 	switch (imx6_pcie->drvdata->variant) {\33[m
 	case IMX8MQ:\33[m
 		reset_control_deassert(imx6_pcie->pciephy_reset);\33[m
\33[36m@@ -595,6 +591,15 @@\33[m \33[mstatic void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)\33[m
 		break;\33[m
 	}\33[m
 \33[m
\33[1;36m+\33[m	\33[1;36m/* Some boards don't have PCIe reset GPIO. */\33[m
\33[1;36m+\33[m	\33[1;36mif (gpio_is_valid(imx6_pcie->reset_gpio)) {\33[m
\33[1;33m+\33[m		\33[1;33mmsleep(100);\33[m
\33[1;33m+\33[m		\33[1;33mgpio_set_value_cansleep(imx6_pcie->r

So:

• first, I didn't remember I had colorMoved set in the first place... I just blindly copied it from the readme without understanding what it was doing, so it took me a while to understand what was up..
• (quite unrelated but it also took me some time to get the raw text -- the issue template lists git --no-pager but that doesn't preserve colors when piping or sending to a file so it's useless. You also need --color=always. I didn't realize that was an option so I ended up getting my output from strace....)
• Now I understand the option I can sort of read the diff? but it looks quite a bit out of place, although that might partly be due to my theme.

I'm not quite sure what to suggest here; now I understand this better I might try fiddling with themes a bit as described in #72 but that feels like quite an advanced featureand probably just shouldn't be promoted in the README without making manual/src/color-moved-support.md prominent, or making it more integrated with themes, or at least the default ones?

I realize it's not possible to make everyone happy and ultimately don't really care, just wanted to say this was 1/ surprising and 2/ made me waste quite a bit of time so I'm wasting more by venting :P

Thanks anyway :)

[dandavison]

Hi @martinetd, sorry you lost time on this. As you see, diff.colorMoved is a git feature, and the colors being shown are what you'd see if you were not using delta. What do you think the next step is with this ticket? Is the proposal to remove the diff.colorMoved setting from the example config? I don't have a terribly compelling reason for it being there; what I wanted to do was have that default config introduce people to some of the more powerful things that can be done by delta (and git) that they might not otherwise come across if it just gave vanilla config.

[martinetd]

Thanks for the reply, it's appreciated! :)

I'm not sure either -- I think the colorMoved feature in itself can be quite powerful, but the default color scheme made me at first misunderstand the diff (I thought only the green/red-background lines were added/removed as usual), then when that didn't make sense I assumed something was broken
Part of the problem is that there are colors a bit of everwhere for syntax coloration, so I thought the light blue used by git was just what the theme had in store for comments without much thinking... If the default config we provide also keeps a background color configured I think that misunderstanding would be much less likely to happen as nothing else in delta fills in background colors as far as I'm aware.

But given the examples are rather big I'm not sure it's really friendly, so just removing diff.colorMoved from the example config with a link to https://github.com/dandavison/delta/blob/master/manual/src/color-moved-support.md instead might be better?
That page has screenshots and extensive configuration so people can see if they're interested and fiddle with it if they'd like.

Conceptually it's a bit weird that delta can ignore the original colors and remap with themes, yet keep the git config for colorMoved, but I don't think there's much choice here unless we can come up with a more robust way of parsing that... And I certainly don't have any idea for that.

[julienw]

I just got the same misunderstanding when looking at a diff. I believe that having by default the same background color than for non-moved lines would go a long way already.
I'll look at the mentioned doc now, thanks for writing it already!

[ElvenSpellmaker]

Hey, my issue is to do with this one it seems.

Basically I have delta set up, default configuration (from https://dandavison.github.io/delta/configuration.html):

pager=delta
interactive.difffilter=delta --color-only
delta.navigate=true
merge.conflictstyle=diff3
diff.colormoved=default

I've "removed" (renamed and moved some bits) a file:

However, technically I've moved two of the things (output and variable) to their respective files and renamed custom.tf to app_insights.tf.

I do feel @dandavison that it'd be good to remove that from the main example config, as I had no idea I'd done that by configuring this like that as I didn't even know what that feature did until now.
It's good I understand it now thanks, and it's nice to maybe mention it as an example, but perhaps not in the main example file which is inevitably going to be copied by people who've possibly never seen it before.

diff.colormoved as default (which is currently zebra) is a cool mode it seems, but totally unintuitive to me to show a removed/renamed file with only a single removed line and some colours that indicate a possible move (which don't even seem consistent in themselves). (Also the moved lines having no code highlighting isn't great either, although I dunno how you'd then show they've moved.)

[dandavison]

Hi @ElvenSpellmaker, thanks, yes I agree. Removed in #1884

[ElvenSpellmaker]

Thanks!

[julienw]

Shouldn't this bug be kept open?

[ElvenSpellmaker]

Shouldn't this bug be kept open?

There is no bug? The only issue is if you set the mode to default then that's what git diff displays. If you don't like the output then you'd need to get git to change the zebra mode.

[julienw]

The main issue for me (see my previous comment) is that the background color is different between moved and non-moved lines. I think zebra should change the background color by default. I don't know if that's an easy thing to do or appropriate, but at the moment the default is confusing in my opinion.

[ElvenSpellmaker]

The main issue for me (see my previous comment) is that the background color is different between moved and non-moved lines. I think zebra should change the background color by default. I don't know if that's an easy thing to do or appropriate, but at the moment the default is confusing in my opinion.

That seems like a separate issue as all delta does is show the same diff the normal programme gives back. What you want in your case is extra functionality thaybthebnkemal zebra mode doesn't support.