🐛 weird? handling of git colorMoved

[martinetd]

Hello,

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

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!