Re: [PATCH] diff: add diff.srcprefix and diff.dstprefix option support

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2024, 18:06 UTC
Message-ID: <xmqqplw0r6c3.fsf@gitster.g>
In-Reply-To: <20240311023217.GA2345739@quokka>

Peter Hutterer <peter.hutterer@who-t.net> writes:

> Subject: Re: [PATCH] diff: add diff.srcprefix and diff.dstprefix option support

"option support" -> "configuration variables"

> The git option equivalent to --src-prefix and --dst-prefix.
> Both of these are of lower precedence than the diff.noprefix and
> diff.mnemonicprefix option.

I think it will become simpler to sell and explain if you do not mention these options, and instead say that we are tweaking the default prefixes used when none of the other options are used, something like:

	Allow the default prefixes "a/" and "b/" to be tweaked by
	diff.srcprefix and diff.dstprefix configuration variables.

> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
> +diff.srcprefix::
> +	If set, 'git diff' uses this source prefix.

Add "Defaults to 'a/'", perhaps?

Show 22 quoted lines

> @@ -3429,6 +3437,14 @@ void diff_set_default_prefix(struct diff_options *options)
>  	options->b_prefix = "b/";
>  }
>  
> +void diff_set_custom_prefix(struct diff_options *options, const char *src_prefix, const char *dst_prefix)
> +{
> +	if (src_prefix)
> +		options->a_prefix = src_prefix;
> +	if (dst_prefix)
> +		options->b_prefix = dst_prefix;
> +}
> +
>  struct userdiff_driver *get_textconv(struct repository *r,
>  				     struct diff_filespec *one)
>  {
> @@ -4736,6 +4752,8 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
>  		diff_set_noprefix(options);
>  	} else if (!diff_mnemonic_prefix) {
>  		diff_set_default_prefix(options);
> +		if (diff_src_prefix || diff_dst_prefix)
> +			diff_set_custom_prefix(options, diff_src_prefix, diff_dst_prefix);
>  	}

This feels somewhat roundabout way to do this. Instead of touching this part at all, and not adding diff_set_custom_prefix() function, how about just patching diff_set_default_prefix()? The function does not even have to be public and there is no need to touch the header file.

Here is how I would simplify the code change part if I were doing this patch. It seems to pass t4013 (including your additional ones).

Thanks.

 diff.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git i/diff.c w/diff.c
index e50def4538..4439b1a958 100644
--- i/diff.c
+++ w/diff.c
@@ -62,6 +62,8 @@ static const char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
+static const char *diff_src_prefix = "a/";
+static const char *diff_dst_prefix = "b/";
 static int diff_relative;
 static int diff_stat_name_width;
 static int diff_stat_graph_width;
@@ -408,6 +410,12 @@ int git_diff_ui_config(const char *var, const char *value,
 		diff_no_prefix = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "diff.srcprefix")) {
+		return git_config_string(&diff_src_prefix, var, value);
+	}
+	if (!strcmp(var, "diff.dstprefix")) {
+		return git_config_string(&diff_dst_prefix, var, value);
+	}
 	if (!strcmp(var, "diff.relative")) {
 		diff_relative = git_config_bool(var, value);
 		return 0;
@@ -3425,8 +3433,8 @@ void diff_set_noprefix(struct diff_options *options)
 
 void diff_set_default_prefix(struct diff_options *options)
 {
-	options->a_prefix = "a/";
-	options->b_prefix = "b/";
+	options->a_prefix = diff_src_prefix;
+	options->b_prefix = diff_dst_prefix;
 }
 
 struct userdiff_driver *get_textconv(struct repository *r,