Allow setting diff.worddiff=color via gitconfig

Allow setting diff.worddiff=color via gitconfig

[Olliver Schinagl]

Currently, the only way to set `word-diff=color` is through alias. It 
would be much nicer of course if we could do


[diff]
     worddiff=color


in git-config of course.


Thank you,

Olliver

[PATCH 0/2] Support diff.wordDiff config

[Karthik Nayak]

This patch series adds the diff.wordDiff config option. This mimics the
'--word-diff' option of `git-diff(1)`.

The first patch is more of a preparatory patch, which makes it easier to
add tests when the actual config is added in patch 2.

Karthik Nayak (2):
  t4034: extract out `diff_with_opts`
  diff: add 'diff.wordDiff' config option

 Documentation/config/diff.txt |  4 +++
 diff.c                        | 50 +++++++++++++++++++++------
 t/t4034-diff-words.sh         | 63 ++++++++++++++++++++---------------
 3 files changed, 80 insertions(+), 37 deletions(-)

-- 
2.43.GIT

[PATCH 1/2] t4034: extract out `diff_with_opts`

[Karthik Nayak]

The current `word_diff` function in t4034 uses the arguments provided to
obtain the diff and compare them.

Let's rename this to `diff_with_opts` and add a new function `word_diff`
which is specific to the `--word-diff=` option. This function will act
as a wrapper around `diff_with_opts`.

In the following commit, when we introduce a config option for
`--word-diff`, we'd want to test the existing tests also with the config
option and at this point we can expand `word_diff` to test both config
and command line options.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 t/t4034-diff-words.sh | 54 +++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 74586f3813..4f70aa6e9f 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -51,7 +51,7 @@ cat >expect.non-whitespace-is-word <<-EOF
 	<GREEN>aeff = aeff * ( aaa )<RESET>
 EOF
 
-word_diff () {
+diff_with_opts () {
 	pre=$(git rev-parse --short $(git hash-object pre)) &&
 	post=$(git rev-parse --short $(git hash-object post)) &&
 	test_must_fail git diff --no-index "$@" pre post >output &&
@@ -60,6 +60,10 @@ word_diff () {
 	test_cmp expected output.decrypted
 }
 
+word_diff () {
+	diff_with_opts "--word-diff=$1" $(echo "$@" | cut -d' ' -s -f 2-)
+}
+
 test_language_driver () {
 	lang=$1
 	test_expect_success "diff driver '$lang'" '
@@ -67,11 +71,11 @@ test_language_driver () {
 			"$TEST_DIRECTORY/t4034/'"$lang"'/post" \
 			"$TEST_DIRECTORY/t4034/'"$lang"'/expect" . &&
 		echo "* diff='"$lang"'" >.gitattributes &&
-		word_diff --color-words
+		diff_with_opts --color-words
 	'
 	test_expect_success "diff driver '$lang' in Islandic" '
 		LANG=is_IS.UTF-8 LANGUAGE=is LC_ALL="$is_IS_locale" \
-		word_diff --color-words
+		diff_with_opts --color-words
 	'
 }
 
@@ -81,7 +85,7 @@ test_expect_success setup '
 	git config diff.color.func magenta
 '
 
-test_expect_success 'set up pre and post with runs of whitespace' '
+test_expect_success 'setup pre and post with runs of whitespace' '
 	cp pre.simple pre &&
 	cp post.simple post
 '
@@ -101,9 +105,9 @@ test_expect_success 'word diff with runs of whitespace' '
 
 		<GREEN>aeff = aeff * ( aaa )<RESET>
 	EOF
-	word_diff --color-words &&
-	word_diff --word-diff=color &&
-	word_diff --color --word-diff=color
+	diff_with_opts --color-words &&
+	word_diff color &&
+	diff_with_opts --color --word-diff=color
 '
 
 test_expect_success '--word-diff=porcelain' '
@@ -127,7 +131,7 @@ test_expect_success '--word-diff=porcelain' '
 		+aeff = aeff * ( aaa )
 		~
 	EOF
-	word_diff --word-diff=porcelain
+	word_diff porcelain
 '
 
 test_expect_success '--word-diff=plain' '
@@ -145,8 +149,8 @@ test_expect_success '--word-diff=plain' '
 
 		{+aeff = aeff * ( aaa )+}
 	EOF
-	word_diff --word-diff=plain &&
-	word_diff --word-diff=plain --no-color
+	diff_with_opts --word-diff=plain &&
+	word_diff plain --no-color
 '
 
 test_expect_success '--word-diff=plain --color' '
@@ -164,7 +168,7 @@ test_expect_success '--word-diff=plain --color' '
 
 		<GREEN>{+aeff = aeff * ( aaa )+}<RESET>
 	EOF
-	word_diff --word-diff=plain --color
+	word_diff plain --color
 '
 
 test_expect_success 'word diff without context' '
@@ -181,17 +185,17 @@ test_expect_success 'word diff without context' '
 
 		<GREEN>aeff = aeff * ( aaa )<RESET>
 	EOF
-	word_diff --color-words --unified=0
+	diff_with_opts --color-words --unified=0
 '
 
 test_expect_success 'word diff with a regular expression' '
 	cp expect.letter-runs-are-words expect &&
-	word_diff --color-words="[a-z]+"
+	diff_with_opts --color-words="[a-z]+"
 '
 
 test_expect_success 'word diff with zero length matches' '
 	cp expect.letter-runs-are-words expect &&
-	word_diff --color-words="[a-z${LF}]*"
+	diff_with_opts --color-words="[a-z${LF}]*"
 '
 
 test_expect_success 'set up a diff driver' '
@@ -204,12 +208,12 @@ test_expect_success 'set up a diff driver' '
 
 test_expect_success 'option overrides .gitattributes' '
 	cp expect.letter-runs-are-words expect &&
-	word_diff --color-words="[a-z]+"
+	diff_with_opts --color-words="[a-z]+"
 '
 
 test_expect_success 'use regex supplied by driver' '
 	cp expect.non-whitespace-is-word expect &&
-	word_diff --color-words
+	diff_with_opts --color-words
 '
 
 test_expect_success 'set up diff.wordRegex option' '
@@ -218,7 +222,7 @@ test_expect_success 'set up diff.wordRegex option' '
 
 test_expect_success 'command-line overrides config' '
 	cp expect.letter-runs-are-words expect &&
-	word_diff --color-words="[a-z]+"
+	diff_with_opts --color-words="[a-z]+"
 '
 
 test_expect_success 'command-line overrides config: --word-diff-regex' '
@@ -236,12 +240,12 @@ test_expect_success 'command-line overrides config: --word-diff-regex' '
 
 		<GREEN>{+aeff = aeff * ( aaa+}<RESET> )
 	EOF
-	word_diff --color --word-diff-regex="[a-z]+"
+	diff_with_opts --color --word-diff-regex="[a-z]+"
 '
 
 test_expect_success '.gitattributes override config' '
 	cp expect.non-whitespace-is-word expect &&
-	word_diff --color-words
+	diff_with_opts --color-words
 '
 
 test_expect_success 'setup: remove diff driver regex' '
@@ -263,7 +267,7 @@ test_expect_success 'use configured regex' '
 
 		<GREEN>aeff = aeff * ( aaa<RESET> )
 	EOF
-	word_diff --color-words
+	diff_with_opts --color-words
 '
 
 test_expect_success 'test parsing words for newline' '
@@ -279,7 +283,7 @@ test_expect_success 'test parsing words for newline' '
 		<CYAN>@@ -1 +1 @@<RESET>
 		aaa (aaa) <GREEN>aaa<RESET>
 	EOF
-	word_diff --color-words="a+"
+	diff_with_opts --color-words="a+"
 '
 
 test_expect_success 'test when words are only removed at the end' '
@@ -295,7 +299,7 @@ test_expect_success 'test when words are only removed at the end' '
 		<CYAN>@@ -1 +1 @@<RESET>
 		(<RED>:<RESET>
 	EOF
-	word_diff --color-words=.
+	diff_with_opts --color-words=.
 '
 
 test_expect_success '--word-diff=none' '
@@ -312,7 +316,7 @@ test_expect_success '--word-diff=none' '
 		-(:
 		+(
 	EOF
-	word_diff --word-diff=plain --word-diff=none
+	word_diff plain --word-diff=none
 '
 
 test_expect_success 'unset default driver' '
@@ -363,7 +367,7 @@ test_expect_success 'word-diff with diff.sbe' '
 	[-b-]{+c+}
 	EOF
 	test_config diff.suppress-blank-empty true &&
-	word_diff --word-diff=plain
+	word_diff plain
 '
 
 test_expect_success 'word-diff with no newline at EOF' '
@@ -379,7 +383,7 @@ test_expect_success 'word-diff with no newline at EOF' '
 	@@ -1 +1 @@
 	a a [-a-]{+ab+} a a
 	EOF
-	word_diff --word-diff=plain
+	word_diff plain
 '
 
 test_expect_success 'setup history with two files' '
-- 
2.43.GIT

[PATCH 2/2] diff: add 'diff.wordDiff' config option

[Karthik Nayak]

The git-diff(1) command supports the `--word-diff` which allows the
users to specify how to delimit word diffs. Provide this option also as
a config param 'diff.wordDiff'.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/config/diff.txt |  4 +++
 diff.c                        | 50 +++++++++++++++++++++++++++--------
 t/t4034-diff-words.sh         | 11 ++++++--
 3 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
index bd5ae0c337..00459beee2 100644
--- a/Documentation/config/diff.txt
+++ b/Documentation/config/diff.txt
@@ -225,3 +225,7 @@ diff.colorMovedWS::
 	When moved lines are colored using e.g. the `diff.colorMoved` setting,
 	this option controls the `<mode>` how spaces are treated
 	for details of valid modes see '--color-moved-ws' in linkgit:git-diff[1].
+
+diff.wordDiff::
+	Show a word diff, using the `<mode>` to delimit changed words.
+	For details of valid modes see '--word-diff' in linkgit:git-diff[1].
diff --git a/diff.c b/diff.c
index e50def4538..050d83ef85 100644
--- a/diff.c
+++ b/diff.c
@@ -69,6 +69,7 @@ static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
+static int diff_word_diff_default;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char *value)
 	return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
 }
 
+static int parse_word_diff_value(const char *value)
+{
+	if (!value)
+		return -1;
+	else if (!strcmp(value, "plain"))
+		return DIFF_WORDS_PLAIN;
+	else if (!strcmp(value, "color")) {
+		return DIFF_WORDS_COLOR;
+	}
+	else if (!strcmp(value, "porcelain"))
+		return DIFF_WORDS_PORCELAIN;
+	else if (!strcmp(value, "none"))
+		return DIFF_WORDS_NONE;
+
+	return -1;
+}
+
 long parse_algorithm_value(const char *value)
 {
 	if (!value)
@@ -452,6 +470,18 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 
+	if (!strcmp(var, "diff.worddiff")) {
+		if (!value)
+			return config_error_nonbool(var);
+		diff_word_diff_default = parse_word_diff_value(value);
+		if (diff_word_diff_default < 0)
+			return error(_("unknown value for config '%s': %s"),
+				     var, value);
+		if (diff_word_diff_default == DIFF_WORDS_COLOR)
+			diff_use_color_default = 1;
+		return 0;
+	}
+
 	if (git_color_config(var, value, cb) < 0)
 		return -1;
 
@@ -4724,6 +4754,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 	options->use_color = diff_use_color_default;
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
+	options->word_diff |= diff_word_diff_default;
 	if (diff_indent_heuristic)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
@@ -5504,21 +5535,18 @@ static int diff_opt_word_diff(const struct option *opt,
 			      const char *arg, int unset)
 {
 	struct diff_options *options = opt->value;
+	int value;
 
 	BUG_ON_OPT_NEG(unset);
+
 	if (arg) {
-		if (!strcmp(arg, "plain"))
-			options->word_diff = DIFF_WORDS_PLAIN;
-		else if (!strcmp(arg, "color")) {
-			options->use_color = 1;
-			options->word_diff = DIFF_WORDS_COLOR;
-		}
-		else if (!strcmp(arg, "porcelain"))
-			options->word_diff = DIFF_WORDS_PORCELAIN;
-		else if (!strcmp(arg, "none"))
-			options->word_diff = DIFF_WORDS_NONE;
-		else
+		value = parse_word_diff_value(arg);
+		if (value < 0)
 			return error(_("bad --word-diff argument: %s"), arg);
+		if (value == DIFF_WORDS_COLOR)
+			options->use_color = 1;
+
+		options->word_diff = value;
 	} else {
 		if (options->word_diff == DIFF_WORDS_NONE)
 			options->word_diff = DIFF_WORDS_PLAIN;
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 4f70aa6e9f..2cc391c339 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -56,12 +56,19 @@ diff_with_opts () {
 	post=$(git rev-parse --short $(git hash-object post)) &&
 	test_must_fail git diff --no-index "$@" pre post >output &&
 	test_decode_color <output >output.decrypted &&
-	sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected
+	sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected &&
 	test_cmp expected output.decrypted
 }
 
 word_diff () {
-	diff_with_opts "--word-diff=$1" $(echo "$@" | cut -d' ' -s -f 2-)
+	# Capture the rest of the arguments to passthrough.
+	rest=$(echo "$@" | cut -d' ' -s -f 2-) &&
+	# Test via the config route.
+	git config diff.wordDiff $1 &&
+	diff_with_opts $rest &&
+	git config --unset diff.wordDiff &&
+	# Test via the command option route.
+	diff_with_opts "--word-diff=$1" $rest
 }
 
 test_language_driver () {
-- 
2.43.GIT

Re: [PATCH 2/2] diff: add 'diff.wordDiff' config option

[Eric Sunshine]

On Sat, Mar 2, 2024 at 4:58 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> The git-diff(1) command supports the `--word-diff` which allows the
> users to specify how to delimit word diffs. Provide this option also as
> a config param 'diff.wordDiff'.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/diff.c b/diff.c
> @@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char *value)
> +static int parse_word_diff_value(const char *value)
> +{
> +       if (!value)
> +               return -1;
> +       else if (!strcmp(value, "plain"))
> +               return DIFF_WORDS_PLAIN;
> +       else if (!strcmp(value, "color")) {
> +               return DIFF_WORDS_COLOR;
> +       }
> +       else if (!strcmp(value, "porcelain"))
> +               return DIFF_WORDS_PORCELAIN;
> +       else if (!strcmp(value, "none"))
> +               return DIFF_WORDS_NONE;
> +
> +       return -1;
> +}

Why is one arm surrounded by curly braces when none of the others are?

Re: [PATCH 2/2] diff: add 'diff.wordDiff' config option

[Karthik Nayak]

On Sat, Mar 2, 2024 at 11:25 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Mar 2, 2024 at 4:58 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> > The git-diff(1) command supports the `--word-diff` which allows the
> > users to specify how to delimit word diffs. Provide this option also as
> > a config param 'diff.wordDiff'.
> >
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > diff --git a/diff.c b/diff.c
> > @@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char *value)
> > +static int parse_word_diff_value(const char *value)
> > +{
> > +       if (!value)
> > +               return -1;
> > +       else if (!strcmp(value, "plain"))
> > +               return DIFF_WORDS_PLAIN;
> > +       else if (!strcmp(value, "color")) {
> > +               return DIFF_WORDS_COLOR;
> > +       }
> > +       else if (!strcmp(value, "porcelain"))
> > +               return DIFF_WORDS_PORCELAIN;
> > +       else if (!strcmp(value, "none"))
> > +               return DIFF_WORDS_NONE;
> > +
> > +       return -1;
> > +}
>
> Why is one arm surrounded by curly braces when none of the others are?

Seems like I missed that one, if the patch series is being taken
forward, I will fix
it. Thanks

Re: [PATCH 2/2] diff: add 'diff.wordDiff' config option

[Jean-Noël AVILA]

Hello,

On Saturday, 2 March 2024 10:57:51 CET Karthik Nayak wrote:
> The git-diff(1) command supports the `--word-diff` which allows the
> users to specify how to delimit word diffs. Provide this option also as
> a config param 'diff.wordDiff'.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/config/diff.txt |  4 +++
>  diff.c                        | 50 +++++++++++++++++++++++++++--------
>  t/t4034-diff-words.sh         | 11 ++++++--
>  3 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> index bd5ae0c337..00459beee2 100644
> --- a/Documentation/config/diff.txt
> +++ b/Documentation/config/diff.txt
> @@ -225,3 +225,7 @@ diff.colorMovedWS::
>  	When moved lines are colored using e.g. the `diff.colorMoved` 
setting,
>  	this option controls the `<mode>` how spaces are treated
>  	for details of valid modes see '--color-moved-ws' in linkgit:git-
diff[1].
> +
> +diff.wordDiff::
> +	Show a word diff, using the `<mode>` to delimit changed words.
> +	For details of valid modes see '--word-diff' in linkgit:git-diff[1].

Let's enforce the new formatting rules: placeholders are _emphasized_  and 
options are `verbatim`

+
 +diff.wordDiff::
 +	Show a word diff, using the _<mode>_ to delimit changed words.
 +	For details of valid modes see `--word-diff` in linkgit:git-diff[1].


> diff --git a/diff.c b/diff.c
> index e50def4538..050d83ef85 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -69,6 +69,7 @@ static int diff_dirstat_permille_default = 30;
>  static struct diff_options default_diff_options;
>  static long diff_algorithm;
>  static unsigned ws_error_highlight_default = WSEH_NEW;
> +static int diff_word_diff_default;
>  
>  static char diff_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_RESET,
> @@ -209,6 +210,23 @@ int git_config_rename(const char *var, const char 
*value)
>  	return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
>  }
>  
> +static int parse_word_diff_value(const char *value)
> +{
> +	if (!value)
> +		return -1;
> +	else if (!strcmp(value, "plain"))
> +		return DIFF_WORDS_PLAIN;
> +	else if (!strcmp(value, "color")) {
> +		return DIFF_WORDS_COLOR;
> +	}
> +	else if (!strcmp(value, "porcelain"))
> +		return DIFF_WORDS_PORCELAIN;
> +	else if (!strcmp(value, "none"))
> +		return DIFF_WORDS_NONE;
> +
> +	return -1;
> +}
> +
>  long parse_algorithm_value(const char *value)
>  {
>  	if (!value)
> @@ -452,6 +470,18 @@ int git_diff_ui_config(const char *var, const char 
*value,
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "diff.worddiff")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		diff_word_diff_default = parse_word_diff_value(value);
> +		if (diff_word_diff_default < 0)
> +			return error(_("unknown value for config '%s': 
%s"),

Thanks for reusing already an already existing translatable string.

> +				     var, value);
> +		if (diff_word_diff_default == DIFF_WORDS_COLOR)
> +			diff_use_color_default = 1;
> +		return 0;
> +	}
> +
>  	if (git_color_config(var, value, cb) < 0)
>  		return -1;
>  
> @@ -4724,6 +4754,7 @@ void repo_diff_setup(struct repository *r, struct 
diff_options *options)
>  	options->use_color = diff_use_color_default;
>  	options->detect_rename = diff_detect_rename_default;
>  	options->xdl_opts |= diff_algorithm;
> +	options->word_diff |= diff_word_diff_default;
>  	if (diff_indent_heuristic)
>  		DIFF_XDL_SET(options, INDENT_HEURISTIC);
>  
> @@ -5504,21 +5535,18 @@ static int diff_opt_word_diff(const struct option 
*opt,
>  			      const char *arg, int unset)
>  {
>  	struct diff_options *options = opt->value;
> +	int value;
>  
>  	BUG_ON_OPT_NEG(unset);
> +
>  	if (arg) {
> -		if (!strcmp(arg, "plain"))
> -			options->word_diff = DIFF_WORDS_PLAIN;
> -		else if (!strcmp(arg, "color")) {
> -			options->use_color = 1;
> -			options->word_diff = DIFF_WORDS_COLOR;
> -		}
> -		else if (!strcmp(arg, "porcelain"))
> -			options->word_diff = DIFF_WORDS_PORCELAIN;
> -		else if (!strcmp(arg, "none"))
> -			options->word_diff = DIFF_WORDS_NONE;
> -		else
> +		value = parse_word_diff_value(arg);
> +		if (value < 0)
>  			return error(_("bad --word-diff argument: 
%s"), arg);
> +		if (value == DIFF_WORDS_COLOR)
> +			options->use_color = 1;
> +
> +		options->word_diff = value;
>  	} else {
>  		if (options->word_diff == DIFF_WORDS_NONE)
>  			options->word_diff = DIFF_WORDS_PLAIN;
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 4f70aa6e9f..2cc391c339 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -56,12 +56,19 @@ diff_with_opts () {
>  	post=$(git rev-parse --short $(git hash-object post)) &&
>  	test_must_fail git diff --no-index "$@" pre post >output &&
>  	test_decode_color <output >output.decrypted &&
> -	sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected
> +	sed -e "2s/index [^ ]*/index $pre..$post/" expect >expected &&
>  	test_cmp expected output.decrypted
>  }
>  
>  word_diff () {
> -	diff_with_opts "--word-diff=$1" $(echo "$@" | cut -d' ' -s -f 2-)
> +	# Capture the rest of the arguments to passthrough.
> +	rest=$(echo "$@" | cut -d' ' -s -f 2-) &&
> +	# Test via the config route.
> +	git config diff.wordDiff $1 &&
> +	diff_with_opts $rest &&
> +	git config --unset diff.wordDiff &&
> +	# Test via the command option route.
> +	diff_with_opts "--word-diff=$1" $rest
>  }
>  
>  test_language_driver () {
> 

Thanks

Re: [PATCH 0/2] Support diff.wordDiff config

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This patch series adds the diff.wordDiff config option. This mimics the
> '--word-diff' option of `git-diff(1)`.

Is it even be sensible to introduce this configuration variable in
the first place?  What would this do to users who set this variable
and use third-party or their own scripts that run "git diff" under
the hood?

The usual answer is "these tools should be using the low-level
plumbing commands like diff-files, diff-index, and diff-tree", so I
am not worried about it too much myself, and the above is purely the
devil's advocate comment.

Having said that, running

	$ git grep -e 'git diff '

in the collection of scripts I use [*] to work on this project, I am
reminded that I may have to be a bit more conservative than I
currently am about the risk of breaking scripts with the changes
like the one being proposed.

The proposed feature also may break those who use the git-prompt and
diff-highlight available in conrib/, even though I am not sure how
badly they would break, because I only looked at the lines given by
this command:

	$ git grep -e 'git diff ' -- \*.sh ':!t/'

and didn't check how the output from 'git diff' is used.


[Footnote]

 * They can be seen in the 'todo' branch, if anybody is interested.

Re: [PATCH 0/2] Support diff.wordDiff config

[Karthik Nayak]

On Sat, Mar 2, 2024 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > This patch series adds the diff.wordDiff config option. This mimics the
> > '--word-diff' option of `git-diff(1)`.
>
> Is it even be sensible to introduce this configuration variable in
> the first place?  What would this do to users who set this variable
> and use third-party or their own scripts that run "git diff" under
> the hood?

This is definitely a good question to ask. I'm primarily not a user of this
option, and this patch series was more of to start this discussion, based
on the request. I'm comfortable dropping the patch series too if it doesn't
make much sense.

>
> The usual answer is "these tools should be using the low-level
> plumbing commands like diff-files, diff-index, and diff-tree", so I
> am not worried about it too much myself, and the above is purely the
> devil's advocate comment.
>
> Having said that, running
>
>         $ git grep -e 'git diff '
>
> in the collection of scripts I use [*] to work on this project, I am
> reminded that I may have to be a bit more conservative than I
> currently am about the risk of breaking scripts with the changes
> like the one being proposed.
>
> The proposed feature also may break those who use the git-prompt and
> diff-highlight available in conrib/, even though I am not sure how
> badly they would break, because I only looked at the lines given by
> this command:
>
>         $ git grep -e 'git diff ' -- \*.sh ':!t/'
>
> and didn't check how the output from 'git diff' is used.
>
>
> [Footnote]
>
>  * They can be seen in the 'todo' branch, if anybody is interested.

Having said that, wouldn't this cause a problem only if the config is set up?
Meaning the user must explicitly set `diff.wordDiff` for their scripts
to potentially
break. In that sense, is it a breaking feature?

Re: [PATCH 0/2] Support diff.wordDiff config

[Kristoffer Haugsbakk]

Hi

On Sat, Mar 2, 2024, at 19:02, Karthik Nayak wrote:
> On Sat, Mar 2, 2024 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> > This patch series adds the diff.wordDiff config option. This mimics the
>> > '--word-diff' option of `git-diff(1)`.
>>
>> Is it even be sensible to introduce this configuration variable in
>> the first place?  What would this do to users who set this variable
>> and use third-party or their own scripts that run "git diff" under
>> the hood?
>
> This is definitely a good question to ask. I'm primarily not a user of this
> option, and this patch series was more of to start this discussion, based
> on the request. I'm comfortable dropping the patch series too if it doesn't
> make much sense.

This looks similar to the discussion from a [stash] topic:

• Proposed introducing config variables which change how `git stash
  push` and `git stash save` behave (what they save)
• Concern about how that could break third-party scripts

Like here it would be opt-in. But the user might have no idea what kind
of scripts/programs that they use that happen to use git-stash(1).

(That’s at least how I read the thread)

I guess the concern might be worse for git-stash(1) since it seems very
natural to use that command in scripts in order to deal with a working
tree that might be in a who-knows condition: just get these things out
of the way so I can do what I want.

🔗 [stash]: https://lore.kernel.org/git/xmqq34tnyhhf.fsf@gitster.g/

-- 
Kristoffer Haugsbakk

Re: [PATCH 0/2] Support diff.wordDiff config

[Chris Torek]

Continuing the digression a bit:

On Sat, Mar 2, 2024 at 11:58 AM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> This looks similar to the discussion from a [stash] topic:
>
> • Proposed introducing config variables which change how `git stash
>   push` and `git stash save` behave (what they save)
> • Concern about how that could break third-party scripts
[snippage]
> 🔗 [stash]: https://lore.kernel.org/git/xmqq34tnyhhf.fsf@gitster.g/

As I see it, the general issue here is the tension between Git
commands that are used for scripting -- which ideally should
always be plumbing commands -- and those used by end-users.

This tension is relieved somewhat when there *are* separate
plumbing commands, such as `git diff-index` and `git diff-tree`
and so on, or `git rev-list` vs `git log`. Unfortunately there
are some commands, including `git log` itself, that have options
that are missing from the roughly-equivalent plumbing command,
and there are commands (such as `git stash` and `git status`)
that either do not have, or at one time lacked, plumbing command
equivalents or options.

The `git status` command shows one way out of this problem:
we can *add* `--porcelain` options.  Perhaps every command (or
every non-plumbing-only one) should have `--porcelain[=<version>]`.

This doesn't fix the situation today, but provides an obvious
future-proofing path.

Chris

Re: [PATCH 0/2] Support diff.wordDiff config

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> This tension is relieved somewhat when there *are* separate
> plumbing commands, such as `git diff-index` and `git diff-tree`
> and so on, or `git rev-list` vs `git log`. Unfortunately there
> are some commands, including `git log` itself, that have options
> that are missing from the roughly-equivalent plumbing command,
> and there are commands (such as `git stash` and `git status`)
> that either do not have, or at one time lacked, plumbing command
> equivalents or options.

Yup.  It is my pet peeve that more and more contributors got lazy
and tweaked only Porcelain commands, without bothering to improve
plumbing commands to match, while adding more features during the
last decade.  Unfortunately there is no easy remedy after such sins
have been committed.  Once people start using `git log` in their
scripts, it is way too late to tell them to update their scripts to
use `git log --porcelain`.  The fact that you need to tell them is
an admission that you already broke their scripts.

Re: [PATCH 0/2] Support diff.wordDiff config

[Olliver Schinagl]

On 03-03-2024 18:45, Junio C Hamano wrote:
> Chris Torek <chris.torek@gmail.com> writes:
> 
>> This tension is relieved somewhat when there *are* separate
>> plumbing commands, such as `git diff-index` and `git diff-tree`
>> and so on, or `git rev-list` vs `git log`. Unfortunately there
>> are some commands, including `git log` itself, that have options
>> that are missing from the roughly-equivalent plumbing command,
>> and there are commands (such as `git stash` and `git status`)
>> that either do not have, or at one time lacked, plumbing command
>> equivalents or options.
> 
> Yup.  It is my pet peeve that more and more contributors got lazy
> and tweaked only Porcelain commands, without bothering to improve
> plumbing commands to match, while adding more features during the
> last decade.  Unfortunately there is no easy remedy after such sins
> have been committed.  Once people start using `git log` in their
> scripts, it is way too late to tell them to update their scripts to
> use `git log --porcelain`.  The fact that you need to tell them is
> an admission that you already broke their scripts.
> 
To avoid this request from dieing quietly, I will ask (complain) again. 
Who's the client for. How important is the human UX?

Even introducing a new cli, 'git-cli-for-humans' it will be abused again 
for sure. So what's a good way forward? Personally, as I mentioned 
before, it's in the docs to not script around non-plumbing commands, 
which gives an opening to the admission. And why is admitting things a 
bad thing, when it improves things for the human? Even if it hurts.

One could argue 'git3 will break things! Human and machine control is 
split. Use --porcelain (or plumbing commands) in your scripts or expect 
breakage from time to time. You have been warned!'

We do in the end want progress, do we not? :)

Olliver

Re: [PATCH 0/2] Support diff.wordDiff config

[Dragan Simic]

On 2024-03-22 23:08, Olliver Schinagl wrote:
> On 03-03-2024 18:45, Junio C Hamano wrote:
>> Chris Torek <chris.torek@gmail.com> writes:
>> 
>>> This tension is relieved somewhat when there *are* separate
>>> plumbing commands, such as `git diff-index` and `git diff-tree`
>>> and so on, or `git rev-list` vs `git log`. Unfortunately there
>>> are some commands, including `git log` itself, that have options
>>> that are missing from the roughly-equivalent plumbing command,
>>> and there are commands (such as `git stash` and `git status`)
>>> that either do not have, or at one time lacked, plumbing command
>>> equivalents or options.
>> 
>> Yup.  It is my pet peeve that more and more contributors got lazy
>> and tweaked only Porcelain commands, without bothering to improve
>> plumbing commands to match, while adding more features during the
>> last decade.  Unfortunately there is no easy remedy after such sins
>> have been committed.  Once people start using `git log` in their
>> scripts, it is way too late to tell them to update their scripts to
>> use `git log --porcelain`.  The fact that you need to tell them is
>> an admission that you already broke their scripts.
>> 
> To avoid this request from dieing quietly, I will ask (complain)
> again. Who's the client for. How important is the human UX?
> 
> Even introducing a new cli, 'git-cli-for-humans' it will be abused
> again for sure. So what's a good way forward? Personally, as I
> mentioned before, it's in the docs to not script around non-plumbing
> commands, which gives an opening to the admission. And why is
> admitting things a bad thing, when it improves things for the human?
> Even if it hurts.
> 
> One could argue 'git3 will break things! Human and machine control is
> split. Use --porcelain (or plumbing commands) in your scripts or
> expect breakage from time to time. You have been warned!'
> 
> We do in the end want progress, do we not? :)

Maybe, but just maybe, a possible solution for introducing such new
configuration options could be introduce a new category of configuration
options, which could be set in the user's git configuration only?

That way, a repository enabling some troublesome configuration option
wouldn't cause the user's scripts to break.

Re: [PATCH 0/2] Support diff.wordDiff config

[Olliver Schinagl]

On 03-03-2024 08:23, Chris Torek wrote:
> Continuing the digression a bit:
> 
> On Sat, Mar 2, 2024 at 11:58 AM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> This looks similar to the discussion from a [stash] topic:
>>
>> • Proposed introducing config variables which change how `git stash
>>    push` and `git stash save` behave (what they save)
>> • Concern about how that could break third-party scripts
> [snippage]
>> 🔗 [stash]: https://lore.kernel.org/git/xmqq34tnyhhf.fsf@gitster.g/
> 
> As I see it, the general issue here is the tension between Git
> commands that are used for scripting -- which ideally should
> always be plumbing commands -- and those used by end-users.
> 
> This tension is relieved somewhat when there *are* separate
> plumbing commands, such as `git diff-index` and `git diff-tree`
> and so on, or `git rev-list` vs `git log`. Unfortunately there
> are some commands, including `git log` itself, that have options
> that are missing from the roughly-equivalent plumbing command,
> and there are commands (such as `git stash` and `git status`)
> that either do not have, or at one time lacked, plumbing command
> equivalents or options.

I think you are spot on; and as I mentioned earlier, who is git for? 
Which commands are for whom. Human or machine. I suppose you can never 
avoid commands being abused, but should we favor machine abuse for very 
poor UX for the human? I would say 'absolutly not' but up until now, 
this has always been the case.

Maybe it comes from Linus's strict 'never break userspace'. But I think 
the situation is different. We want to enrich the user experience, we 
are constantly degrading it in favor of machines. Even though the 
manuals and interwebs are full of 'don't use these commands in scripts, 
they may break'. In that light, I would say 'lets break it in favor of UX :p

> 
> The `git status` command shows one way out of this problem:
> we can *add* `--porcelain` options.  Perhaps every command (or
> every non-plumbing-only one) should have `--porcelain[=<version>]`.

That's a fair argument, and makes it clear, however, there's tons of 
scripts that probably don't do this right now.

> 
> This doesn't fix the situation today, but provides an obvious
> future-proofing path.
> 
> Chris

Re: [PATCH 0/2] Support diff.wordDiff config

[Olliver Schinagl]

On 02-03-2024 19:02, Karthik Nayak wrote:
> On Sat, Mar 2, 2024 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> This patch series adds the diff.wordDiff config option. This mimics the
>>> '--word-diff' option of `git-diff(1)`.
>>
>> Is it even be sensible to introduce this configuration variable in
>> the first place?  What would this do to users who set this variable
>> and use third-party or their own scripts that run "git diff" under
>> the hood?
> 
> This is definitely a good question to ask. I'm primarily not a user of this
> option, and this patch series was more of to start this discussion, based
> on the request. I'm comfortable dropping the patch series too if it doesn't
> make much sense.

As a human user, I would very much like to see this feature :) It helps 
much to visually distinct things, but `git diff` sits way to deep in my 
muscle memory to remember my alias. Also, we have a configuration system 
to set configuration options for many things.

I wonder though, how many other configuration options we already have, 
that potentially break 'random scripts' because the user has set it ...

> 
>>
>> The usual answer is "these tools should be using the low-level
>> plumbing commands like diff-files, diff-index, and diff-tree", so I
>> am not worried about it too much myself, and the above is purely the
>> devil's advocate comment.
>>
>> Having said that, running
>>
>>          $ git grep -e 'git diff '
>>
>> in the collection of scripts I use [*] to work on this project, I am
>> reminded that I may have to be a bit more conservative than I
>> currently am about the risk of breaking scripts with the changes
>> like the one being proposed.
>>
>> The proposed feature also may break those who use the git-prompt and
>> diff-highlight available in conrib/, even though I am not sure how
>> badly they would break, because I only looked at the lines given by
>> this command:
>>
>>          $ git grep -e 'git diff ' -- \*.sh ':!t/'
>>
>> and didn't check how the output from 'git diff' is used.
>>
>>
>> [Footnote]
>>
>>   * They can be seen in the 'todo' branch, if anybody is interested.
> 
> Having said that, wouldn't this cause a problem only if the config is set up?
> Meaning the user must explicitly set `diff.wordDiff` for their scripts
> to potentially
> break. In that sense, is it a breaking feature?

Re: [PATCH 0/2] Support diff.wordDiff config

[Olliver Schinagl]

Hey list,

On 02-03-2024 18:03, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
>> This patch series adds the diff.wordDiff config option. This mimics the
>> '--word-diff' option of `git-diff(1)`.
> 
> Is it even be sensible to introduce this configuration variable in
> the first place?

Of course it is :p as a human, I crave it :p

On a slightly more serious note though, I always have to use an alias, 
or the command line option I cannot use `git diff` with this as default. 
 From a human UX point of view, this is odd, and we have tons of 
configuration options to do exactly what is desired, without aliases.

I suppose the deeper discussion would be, do we distinct between user 
(human) facing options and arguments, and machine facing options and 
argument (in theory, yes we do; in practice things get abused).

Git is often blamed due to its horrible UX. I think the problem comes 
from the deeper issue mentioned above. Because things get abused, they 
can no longer be touched, not even to improve UX for the human.

> What would this do to users who set this variable
> and use third-party or their own scripts that run "git diff" under
> the hood?
> 
> The usual answer is "these tools should be using the low-level
> plumbing commands like diff-files, diff-index, and diff-tree", so I
> am not worried about it too much myself, and the above is purely the
> devil's advocate comment.
> 
> Having said that, running
> 
> 	$ git grep -e 'git diff '
> 
> in the collection of scripts I use [*] to work on this project, I am
> reminded that I may have to be a bit more conservative than I
> currently am about the risk of breaking scripts with the changes
> like the one being proposed.
> 
> The proposed feature also may break those who use the git-prompt and
> diff-highlight available in conrib/, even though I am not sure how
> badly they would break, because I only looked at the lines given by
> this command:
> 
> 	$ git grep -e 'git diff ' -- \*.sh ':!t/'
> 
> and didn't check how the output from 'git diff' is used.
> 
> 
> [Footnote]
> 
>   * They can be seen in the 'todo' branch, if anybody is interested.