Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diff,range-diff: colorize a file without performing a diff #1126

Open
chooglen opened this issue Jan 25, 2022 · 3 comments
Open

diff,range-diff: colorize a file without performing a diff #1126

chooglen opened this issue Jan 25, 2022 · 3 comments

Comments

@chooglen
Copy link

When reading a diff or a range-diff, it can be handy to have colorized output because it's easier to distinguish between the different lines. This is especially true of range-diff, which has at least 6 possible line prefixes. The git [diff|range-diff] commands can generate a colorized diff/range-diff and display it in the pager, but they are unable to colorize an existing diff/range-diff in text form (say, a patch or range-diff from an email).

Many editors support colorization of diffs, but there is little support for range-diff. Adding an option to accept a diff file and colorize it would be a big productivity win for those who'd like a CLI-friendly, editor-agnostic way of view diff files.

@dscho
Copy link
Member

dscho commented Jan 25, 2022

Most editors indeed support diff coloring, and most of the same editors also allow adding new syntax highlighting, typically using the .tmbundle format championed by TextMate

A long time ago, I played with this, coming up with this range-diff.tmbundle.json file:

{
	"$schema": "https://raw.githubusercontent.com/martinring/tmlanguage/master/tmlanguage.json",
	"name": "source.range-diff",
	"fileTypes": [
		"range-diff"
	],
	"patterns": [
		{
			"match": "^( *[1-9][0-9]*: *[0-9a-f]+ *)=( *[1-9][0-9]*: *[0-9a-f]+)(.*)$",
			"name": "meta.diff.header.range-diff"
		},
		{
			"match": "^( *[1-9][0-9]*: *[0-9a-f]+ *)(!)( *[1-9][0-9]*: *[0-9a-f]*)(.*)$",
			"captures": {
				"1": {
					"name": "markup.deleted.diff.range-diff"
				},
				"2": {
					"name": "meta.diff.header.range-diff"
				},
				"3": {
					"name": "markup.inserted.diff.range-diff"
				},
				"4": {
					"name": "meta.diff.header.range-diff"
				}
			}
		},
		{
			"match": "^( *[1-9][0-9]*: *[0-9a-f]+ *< *-: *-+.*)$",
			"name": "markup.deleted.diff.range-diff"
		},
		{
			"match": "^( *-: *-+> *[1-9][0-9]*: *[0-9a-f]+ *.*)$",
			"name": "markup.inserted.diff.range-diff"
		},
		{
			"match": "^(    @@.*)$",
			"name": "punctuation.definition.range.diff.range-diff"
		},
		{
			"match": "^(    )(-)( )",
			"name": "markup.deleted.diff.range-diff"
		},
		{
			"match": "^(    )(\\+)( )",
			"name": "markup.inserted.diff.range-diff"
		},
		{
			"match": "^(    )(-)(-.*)",
			"captures": {
				"2": {
					"name": "markup.deleted.diff.range-diff"
				},
				"3": {
					"name": "markup.italic.range-diff",
					"patterns": [
						{
							"match": ".*",
							"name": "markup.deleted.diff.range-diff"
						}
					]
				}
			}
		},
		{
			"match": "^(    )(-)(\\+.*)",
			"captures": {
				"2": {
					"name": "markup.deleted.diff.range-diff"
				},
				"3": {
					"name": "markup.italic.range-diff",
					"patterns": [
						{
							"match": ".*",
							"name": "markup.inserted.diff.range-diff"
						}
					]
				}
			}
		},
		{
			"match": "^(    )(\\+)(-.*)",
			"captures": {
				"2": {
					"name": "markup.inserted.diff.range-diff"
				},
				"3": {
					"name": "markup.bold.range-diff",
					"patterns": [
						{
							"match": ".*",
							"name": "markup.deleted.diff.range-diff"
						}
					]
				}
			}
		},
		{
			"match": "^(    )(\\+)(\\+.*)",
			"captures": {
				"2": {
					"name": "markup.inserted.diff.range-diff"
				},
				"3": {
					"name": "markup.bold.range-diff",
					"patterns": [
						{
							"match": ".*",
							"name": "markup.inserted.diff.range-diff"
						}
					]
				}
			}
		}
	],
	"scopeName": "source.range-diff"
}

The real problem here is that this does not contain colors, but instead labels describing semantically what is happening. Some labels are interpreted by the editor to mean certain colors, and if the editor has dark mode, it will also define other colors for the same labels. And herein lies the rub: in range-diff, we need a way to show "old" hunks in dim, and "new" hunks in bright colors. And there is simply no established label that TextMate, Visual Studio Code and others would consistently handle as dim or bright green/red.

@phil-blain
Copy link

Maybe a roundabout way to deal with what @chooglen wants to achieve, but the following yields a range-diff colorized output, when ran in my clone of Git:

$ b4 diff https://lore.kernel.org/git/[email protected]/T/#u
Grabbing thread from lore.kernel.org/git/pull.1262.v2.git.git.1652210747614.gitgitgadget%40gmail.com/t.mbox.gz
Checking for older revisions on https://lore.kernel.org/git/
New revision: [PATCH] pull: only pass '--recurse-submodules' to subcommands
Grabbing thread from lore.kernel.org/git/[email protected]/t.mbox.gz
   Added 7 messages from that thread
---
Analyzing 7 messages in the thread
Preparing fake-am for v1: pull: only pass '--recurse-submodules' to subcommands
  range: 125d1fbcb2dc..4674749ef81b
Preparing fake-am for v2: pull: only pass '--recurse-submodules' to subcommands
  range: 52d1c158ffdb..579d74906332
Diffing v1 and v2
    Running: git range-diff 125d1fbcb2dc..4674749ef81b 52d1c158ffdb..579d74906332
---
1:  4674749ef8 ! 1:  579d749063 pull: only pass '--recurse-submodules' to subcommands
    @@ Commit message
         pull: only pass '--recurse-submodules' to subcommands
     
         Fix a bug in "git pull" where `submodule.recurse` is preferred over
    -    `fetch.recurseSubmodules` (Documentation/config/fetch.txt says that
    -    `fetch.recurseSubmodules` should be preferred.). Do this by passing the
    -    value of the "--recurse-submodules" CLI option to the underlying fetch,
    -    instead of passing a value that combines the CLI option and config
    -    variables.
    +    `fetch.recurseSubmodules` when performing a fetch
    +    (Documentation/config/fetch.txt says that `fetch.recurseSubmodules`
    +    should be preferred.). Do this by passing the value of the
    +    "--recurse-submodules" CLI option to the underlying fetch, instead of
    +    passing a value that combines the CLI option and config variables.
     
         In other words, this bug occurred because builtin/pull.c is conflating
         two similar-sounding, but different concepts:
    @@ Commit message
           fetch".
     
         Thus, when `submodule.recurse` is set, the underlying "git fetch" gets
    -    invoked with "--recurse-submodules", overriding the value of
    +    invoked with "--recurse-submodules[=value]", overriding the value of
         `fetch.recurseSubmodules`.
     
         An alternative (and more obvious) approach to fix the bug would be to
    @@ t/t5572-pull-submodule.sh: git_pull_noff ()
      
     +test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '
     +	test_commit -C child merge_strategy_5 &&
    -+	git -C parent submodule update --remote &&
    -+	git -C parent add sub &&
    -+	git -C parent commit -m "update submodule" &&
    ++	# Omit the parent commit, otherwise this passes with the
    ++	# default "pull" behavior.
     +
     +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
     +	# Check that the submodule commit was fetched
    -+	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
    ++	sub_oid=$(git -C child rev-parse HEAD) &&
     +	git -C super/sub cat-file -e $sub_oid &&
     +	# Check that the submodule worktree did not update
     +	! test_path_is_file super/sub/merge_strategy_5.t
     +'
    ++
    ++test_expect_success "fetch.recurseSubmodules takes precedence over submodule.recurse" '
    ++	test_commit -C child merge_strategy_6 &&
    ++	# Omit the parent commit, otherwise this passes with the
    ++	# default "pull" behavior.
    ++
    ++	git -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&
    ++	# Check that the submodule commit was fetched
    ++	sub_oid=$(git -C child rev-parse HEAD) &&
    ++	git -C super/sub cat-file -e $sub_oid &&
    ++	# Check that the submodule worktree did not update
    ++	! test_path_is_file super/sub/merge_strategy_6.t
    ++'
     +
      test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
      	# This tests the following scenario :

@phil-blain
Copy link

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

No branches or pull requests

3 participants