🐛 submodules SHA are truncated to a too small value

[matttbe]

Hello,

First, thank you for this very nice project, it really improves many git commands output!

I recently noticed that when a submodule is modified, the 'diff' shows truncated SHA, e.g.

Δ kernelspace
1234567..7654321

Limiting the output to 7 chars is an issue with repos having a lot of commits (e.g. the Linux kernel) and you can get errors like this one if you are unlucky:

error: short object ID 1234567 is ambiguous
hint: The candidates are:
hint:   1234567xxxxxx commit (...)
hint:   1234567yyyyyy tree

It looks like the limitation to 7 chars is hardcoded:

delta/src/handlers/submodule.rs

Lines 39 to 44 in ce41a39

	self.config
	.minus_style
	.paint(minus_commit.chars().take(7).collect::<String>()),
	self.config
	.plus_style
	.paint(commit.chars().take(7).collect::<String>()),

Could it be possible to make this number configurable?
Or showing more chars by default? For the Linux kernel, they recommend to use min 12 digits (but this number might increase in the future)

[dandavison]

Hi @matttbe, thanks! How about we bump it to 12? It's not going to have a big effect on the UI if it only affects submodules. Do you feel like making the change?

[matttbe]

Hi @dandavison

Thank you for your quick reply!

Yes, bumping it to 12 would of course be OK for me.
I don't know if there are many (real) repos with more than 1M of commits.

[joshtriplett]

Hardcoding it to 12 just makes the issue less likely, not impossible. And it's possible to brute-force generate objects with partial collisions that shortened hashes would hit.

[matttbe]

Or having a config option?

If not, 12, 14, 16 might be a good compromise. Even with the full sha1, we can have (very unlikely) collisions

[dandavison]

Yep exactly, collisions are always possible, even using all the bytes in git's object hashes! It's standard in Git UIs to display shorter versions of hashes in places, accepting the fact that the probability of collision is higher. It's just a question of a sensible length; I don't think a config option is necessary.