Skip to content

Conversation

mstv
Copy link
Member

@mstv mstv commented Oct 29, 2024

Fixes #12006

Proposed changes

  • Add argument --no-ext-diff to all 6 found git diff commands in order to suppress a possibly configured diff.external tool

Screenshots

N/A

Test methodology

  • adapt existing tests

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Oct 29, 2024
@gerhardol
Copy link
Member

GIT_EXTERNAL_DIFF overrides diff.external, but it should be OK to override just for diff
Alternative is to override diff.external
https://difftastic.wilfred.me.uk/git.html

Also override diff.tool

Also need to override for git-log and friends
https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---ext-diff

for delta https://dandavison.github.io/delta/configuration.html
set --no-pager or GIT_PAGER (not sure of precedence) https://git-scm.com/docs/git-config#Documentation/git-config.txt-corepager
For all (not sure) or at least diff, show, log, stash, reflog, add

--
Sidenote:
difftastic changed recommendation from difftool to ext-diff just recently. GE should change too. The problem is to get a sane configuration for WSL.
delta could be added too, maybe a generic difftool/ext.diff

@mstv mstv closed this Oct 29, 2024
@mstv mstv changed the title fix(GetCurrentChanges): Suppress diff.external fix(git diff): Suppress diff.external Nov 2, 2024
@mstv mstv reopened this Nov 2, 2024
@mstv mstv force-pushed the fix/12006_override_diff_external branch from 0cb35a2 to 8604ca2 Compare November 2, 2024 18:33
@gerhardol
Copy link
Member

It is probably enough to set diff.external in GitCommandConfiguration.cs but this handles overrides in gitattributes

diff.tool should be overridden in GitCommandConfiguration though

core.pager could be separate.

@mstv
Copy link
Member Author

mstv commented Nov 2, 2024

I tried to set "diff.external" to an empty string in GitCommandConfiguration.cs. It did not work in the first attempt.

Feel free to take over this PR.

@mstv
Copy link
Member Author

mstv commented Nov 2, 2024

Doesn't git recognize that the output is redirected and that's why does not use a pager?

@gerhardol
Copy link
Member

All fine:

git -c diff.external= will set an empty value, not unset so that cannot be used.
Override GIT_EXTERNAL_DIFF is not enough.

diff.tool not needed here.

setting external diff in .gitattrbutes will still cause git-log etc to fail in some situations, not sure if it applies to GE use. Not now

Doesn't git recognize that the output is redirected and that's why does not use a pager?

Seem so

@mstv
Copy link
Member Author

mstv commented Nov 3, 2024

I see very low risk in adding the argument "--no-ext-diff".
It definitely brings an improvement.
So, I am taking it into v5.1.
If something is missing, it can be added in a follow-up.

@mstv mstv merged commit 6325812 into gitextensions:master Nov 3, 2024
4 checks passed
@mstv mstv deleted the fix/12006_override_diff_external branch November 3, 2024 20:56
@RussKie RussKie added this to the 5.1 milestone Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diff viewer fails when external diff command configured
3 participants