tartufo pre-commit fails to work if diff.mnemonicprefix=true in git configuration

[pmevzek-godaddy]

🐛 Bug Report

My tartufo pre-commit failed to operate on obvious secrets (that tartufo scan-local-repo was able to spot after commit), that is it was not finding them in the commit about to happen.

After long investigations I found out it is because of this snippet in my .gitconfig global configuration:

[diff]
## Tell git diff to use mnemonic prefixes (index, work tree, commit, object) instead of the standard a and b notation:
        mnemonicprefix = true

This is a completely standard and accepted configuration item for git repositories. It should impact only human displays of diff outputs, and shouldn't impact any software reading the git repository content as they should use the plumbing commands and "raw" output.

Human output is impacted (see c/ and i/):

$ git --no-pager diff --cached
diff --git c/secret.txt i/secret.txt
new file mode 100644
index 0000000..b49a4cb
--- /dev/null
+++ i/secret.txt
@@ -0,0 +1 @@
+This is a secret: 758e69784c00c9ca84d9d9ae301f8b23045f4b3c

But raw output isn't:

$ git --no-pager diff --cached --raw
:000000 100644 0000000 b49a4cb A	secret.txt

To Reproduce

$ git init test
$ cd test
$ git commit --allow-empty --allow-empty-message -m 'Start'
[master (root-commit) 620b5f9] Start
$ cat secret.txt
This is a secret: 758e69784c00c9ca84d9d9ae301f8b23045f4b3a
$ git add secret.txt
$ git config --worktree diff.mnemonicprefix true
$ tartufo -V
tartufo, version 2.4.0
$ tartufo pre-commit
Time: 2021-03-11T12:46:35.193038
All clear. No secrets detected.
$ git config --worktree diff.mnemonicprefix false
$ tartufo pre-commit
~~~~~~~~~~~~~~~~~~~~~
Reason: High Entropy
Filepath: secret.txt
Signature: 298546aa76c587308b7f82d7185dfe406a05d5a2c06ef2a3251ce686998b624c
@@ -0,0 +1 @@
+This is a secret: 758e69784c00c9ca84d9d9ae301f8b23045f4b3a

~~~~~~~~~~~~~~~~~~~~~

Expected Behavior

tartufo pre-commit (or any other tartufo command for that matter) shouldn't be affected by user configuration otherwise it means behavior is non reproductible and can be influenced, without knowing, by a specific user git configuration.

Also, personally, I would like to keep this configuration as is as it makes my git work more enjoyable, but right now that blocks my work with tartufo.

Code Example

See above section on how to reproduce.

Environment

Tested with tartufo versions 2.3.1 and 2.4.0

[jgowdy]

tartufo pre-commit (or any other tartufo command for that matter) shouldn't be affected by user configuration otherwise it means behavior is non reproductible and can be influenced, without knowing, by a specific user git configuration.

In general, I agree with your perspective that Tartufo should explicitly override this behavior via command line switch if it breaks Tartufo's behavior in a negative way. I agree it's a valid user configuration and you shouldn't have to tailor this aspect of your git configuration for Tartufo. However, I believe your statement here is overly broad. Obviously there are elements of the user's git configuration that are going to be relevant to the behavior of Tartufo. An obvious example of that would be authentication.
If you have configured your .ssh keys to be in a different directory than ~/.git in your git configuration, we should honor that.
Tartufo currently uses GitPython, which as an implementation calls the git CLI.

I would suggest that we should specifically override behaviors that break the functionality of Tartufo, but avoid making the broad statement that nothing in the user's configuration impacts Tartufo's behavior since that's clearly not the case.

[pmevzek-godaddy]

Obviously there are elements of the user's git configuration that are going to be relevant to the behavior of Tartufo

Yes, obviously. So maybe my statement was poorly phrased but the idea remains: for any configuration item that impacts just human use of git (and this specific configuration impacts only human display of git diff), the tool shouldn't depend on it for reasons that should be obvious with the above and in general.

Which is why I strongly believe, if anyone wants to tackle this issue that the correct course of action is not just to put a workaround to discard this specific configuration item when tartufo runs, but instead have a solid basis where tartufo works with a "constant" configuration, irrespective to user configuration, and taking from user configuration only what is absolutely needed to make it work, like you said authentication (but again, for tartufo pre-commit authentication shouldn't matter...) but nothing more.

Maybe this is not phrased better in fact. Sorry if I can't convey my idea properly. Feel free to disregard my comment and even the issue.

[jgowdy]

It's a valid issue report and a valid request for a fix. Given the open source nature of the project, I would suggest that anyone who takes this on can certainly solve it for the entire featureset of git if they're so inclined, or they can make a pull request specifically to fix this specific flag that's called out. Whatever pull requests show up are what's considered.

I agree with the principle you're describing, I just wouldn't want to block your workflow on waiting for someone to solve this git-feature-wide. We can fix this specific flag, and then make a separate issue that someone can choose to pick up to apply the principle broadly.

[jgowdy]

That being said, PRs welcome.

[pmevzek-godaddy]

It may need to be in another project in fact.
From a quick overview, the solution might be "as simple" as replacing [ab] on https://github.com/gitpython-developers/GitPython/blob/78d12aa7c922551dddd7168498e29eae32c9d109/git/diff.py#L254 by [abiwco12] (twice).

Or monkey patching that from inside tartufo until dependency is fixed.