diff mbox series

[1/2] diff: consolidate diff algorithm option parsing

Message ID f56bd38ac3f80fb3a7e8c92cadaa57d2b0754b9f.1675568781.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Teach diff to honor diff algorithms set through git attributes | expand

Commit Message

John Cai Feb. 5, 2023, 3:46 a.m. UTC 
From: John Cai <jcai@gitlab.com>

The diff option parsing for --minimal, --patience, --histgoram can all
be consolidated into one function. This is a preparatory step for the
subsequent commit which teaches diff to keep track of whether or not a

Comments

Phillip Wood Feb. 6, 2023, 4:20 p.m. UTC | #1 
Hi John

On 05/02/2023 03:46, John Cai via GitGitGadget wrote:
> From: John Cai <jcai@gitlab.com>
> 
> The diff option parsing for --minimal, --patience, --histgoram can all
> be consolidated into one function. This is a preparatory step for the
> subsequent commit which teaches diff to keep track of whether or not a
> diff algorithm has been set via the command line.
> 
> While we're at it, the logic that sets the diff algorithm in
> diff_opt_diff_algorithm() can be refactored into a helper that will
> allow multiple callsites to set the diff algorithm.

You say "while  we're at it" but isn't it a wholly necessary change for 
what you want to do?

This patch basically looks good, I've left a couple of comments below, 
thanks for separating it out as a preparatory step

> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>   diff.c | 87 ++++++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 329eebf16a0..a8a31c81fe7 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3437,6 +3437,22 @@ static int diff_filepair_is_phoney(struct diff_filespec *one,
>   	return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two);
>   }
>   
> +static int set_diff_algorithm(struct diff_options *opts,
> +			      const char *alg)
> +{
> +	long value = parse_algorithm_value(alg);
> +
> +	if (value < 0)
> +		return 1;
> +
> +	/* clear out previous settings */
> +	DIFF_XDL_CLR(opts, NEED_MINIMAL);
> +	opts->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
> +	opts->xdl_opts |= value;
> +
> +	return 0;
> +}
> +
>   static void builtin_diff(const char *name_a,
>   			 const char *name_b,
>   			 struct diff_filespec *one,
> @@ -5107,17 +5123,40 @@ static int diff_opt_diff_algorithm(const struct option *opt,
>   				   const char *arg, int unset)
>   {
>   	struct diff_options *options = opt->value;
> -	long value = parse_algorithm_value(arg);
>   
>   	BUG_ON_OPT_NEG(unset);
> -	if (value < 0)
> +
> +	if (set_diff_algorithm(options, arg))
>   		return error(_("option diff-algorithm accepts \"myers\", "
>   			       "\"minimal\", \"patience\" and \"histogram\""));
>   
> -	/* clear out previous settings */
> -	DIFF_XDL_CLR(options, NEED_MINIMAL);
> -	options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
> -	options->xdl_opts |= value;
> +	return 0;
> +}
> +
> +static int diff_opt_diff_algorithm_no_arg(const struct option *opt,
> +				   const char *arg, int unset)
> +{
> +	struct diff_options *options = opt->value;
> +
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(arg);
> +
> +	if (!strcmp(opt->long_name, "patience")) {
> +		int i;

This is copied from the existing code but as `options->anchors_nr` is a 
size_t it is probably worth converting `i` to a size_t here.

> +		/*
> +		 * Both --patience and --anchored use PATIENCE_DIFF
> +		 * internally, so remove any anchors previously
> +		 * specified.
> +		 */
> +		for (i = 0; i < options->anchors_nr; i++)
> +			free(options->anchors[i]);
> +		options->anchors_nr = 0;
> +	}
> +
> +	if (set_diff_algorithm(options, opt->long_name))
> +		return error(_("available diff algorithms include \"myers\", "
> +			       "\"minimal\", \"patience\" and \"histogram\""));

I think this should be a BUG() as it is a programming error if we reach 
this point.

Best Wishes

Phillip

> +
>   	return 0;
>   }
>   
> @@ -5242,26 +5281,6 @@ static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
>   	return 0;
>   }
>   
> -static int diff_opt_patience(const struct option *opt,
> -			     const char *arg, int unset)
> -{
> -	struct diff_options *options = opt->value;
> -	int i;
> -
> -	BUG_ON_OPT_NEG(unset);
> -	BUG_ON_OPT_ARG(arg);
> -	options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
> -	/*
> -	 * Both --patience and --anchored use PATIENCE_DIFF
> -	 * internally, so remove any anchors previously
> -	 * specified.
> -	 */
> -	for (i = 0; i < options->anchors_nr; i++)
> -		free(options->anchors[i]);
> -	options->anchors_nr = 0;
> -	return 0;
> -}
> -
>   static int diff_opt_ignore_regex(const struct option *opt,
>   				 const char *arg, int unset)
>   {
> @@ -5562,9 +5581,10 @@ struct option *add_diff_options(const struct option *opts,
>   			    N_("prevent rename/copy detection if the number of rename/copy targets exceeds given limit")),
>   
>   		OPT_GROUP(N_("Diff algorithm options")),
> -		OPT_BIT(0, "minimal", &options->xdl_opts,
> -			N_("produce the smallest possible diff"),
> -			XDF_NEED_MINIMAL),
> +		OPT_CALLBACK_F(0, "minimal", options, NULL,
> +			       N_("produce the smallest possible diff"),
> +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
> +			       diff_opt_diff_algorithm_no_arg),
>   		OPT_BIT_F('w', "ignore-all-space", &options->xdl_opts,
>   			  N_("ignore whitespace when comparing lines"),
>   			  XDF_IGNORE_WHITESPACE, PARSE_OPT_NONEG),
> @@ -5589,10 +5609,11 @@ struct option *add_diff_options(const struct option *opts,
>   		OPT_CALLBACK_F(0, "patience", options, NULL,
>   			       N_("generate diff using the \"patience diff\" algorithm"),
>   			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
> -			       diff_opt_patience),
> -		OPT_BITOP(0, "histogram", &options->xdl_opts,
> -			  N_("generate diff using the \"histogram diff\" algorithm"),
> -			  XDF_HISTOGRAM_DIFF, XDF_DIFF_ALGORITHM_MASK),
> +			       diff_opt_diff_algorithm_no_arg),
> +		OPT_CALLBACK_F(0, "histogram", options, NULL,
> +			       N_("generate diff using the \"histogram diff\" algorithm"),
> +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
> +			       diff_opt_diff_algorithm_no_arg),
>   		OPT_CALLBACK_F(0, "diff-algorithm", options, N_("<algorithm>"),
>   			       N_("choose a diff algorithm"),
>   			       PARSE_OPT_NONEG, diff_opt_diff_algorithm),
diff mbox series

Patch

diff algorithm has been set via the command line.

While we're at it, the logic that sets the diff algorithm in
diff_opt_diff_algorithm() can be refactored into a helper that will
allow multiple callsites to set the diff algorithm.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 diff.c | 87 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 329eebf16a0..a8a31c81fe7 100644
--- a/diff.c
+++ b/diff.c
@@ -3437,6 +3437,22 @@  static int diff_filepair_is_phoney(struct diff_filespec *one,
 	return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two);
 }
 
+static int set_diff_algorithm(struct diff_options *opts,
+			      const char *alg)
+{
+	long value = parse_algorithm_value(alg);
+
+	if (value < 0)
+		return 1;
+
+	/* clear out previous settings */
+	DIFF_XDL_CLR(opts, NEED_MINIMAL);
+	opts->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
+	opts->xdl_opts |= value;
+
+	return 0;
+}
+
 static void builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_filespec *one,
@@ -5107,17 +5123,40 @@  static int diff_opt_diff_algorithm(const struct option *opt,
 				   const char *arg, int unset)
 {
 	struct diff_options *options = opt->value;
-	long value = parse_algorithm_value(arg);
 
 	BUG_ON_OPT_NEG(unset);
-	if (value < 0)
+
+	if (set_diff_algorithm(options, arg))
 		return error(_("option diff-algorithm accepts \"myers\", "
 			       "\"minimal\", \"patience\" and \"histogram\""));
 
-	/* clear out previous settings */
-	DIFF_XDL_CLR(options, NEED_MINIMAL);
-	options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
-	options->xdl_opts |= value;
+	return 0;
+}
+
+static int diff_opt_diff_algorithm_no_arg(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+
+	if (!strcmp(opt->long_name, "patience")) {
+		int i;
+		/*
+		 * Both --patience and --anchored use PATIENCE_DIFF
+		 * internally, so remove any anchors previously
+		 * specified.
+		 */
+		for (i = 0; i < options->anchors_nr; i++)
+			free(options->anchors[i]);
+		options->anchors_nr = 0;
+	}
+
+	if (set_diff_algorithm(options, opt->long_name))
+		return error(_("available diff algorithms include \"myers\", "
+			       "\"minimal\", \"patience\" and \"histogram\""));
+
 	return 0;
 }
 
@@ -5242,26 +5281,6 @@  static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
 	return 0;
 }
 
-static int diff_opt_patience(const struct option *opt,
-			     const char *arg, int unset)
-{
-	struct diff_options *options = opt->value;
-	int i;
-
-	BUG_ON_OPT_NEG(unset);
-	BUG_ON_OPT_ARG(arg);
-	options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
-	/*
-	 * Both --patience and --anchored use PATIENCE_DIFF
-	 * internally, so remove any anchors previously
-	 * specified.
-	 */
-	for (i = 0; i < options->anchors_nr; i++)
-		free(options->anchors[i]);
-	options->anchors_nr = 0;
-	return 0;
-}
-
 static int diff_opt_ignore_regex(const struct option *opt,
 				 const char *arg, int unset)
 {
@@ -5562,9 +5581,10 @@  struct option *add_diff_options(const struct option *opts,
 			    N_("prevent rename/copy detection if the number of rename/copy targets exceeds given limit")),
 
 		OPT_GROUP(N_("Diff algorithm options")),
-		OPT_BIT(0, "minimal", &options->xdl_opts,
-			N_("produce the smallest possible diff"),
-			XDF_NEED_MINIMAL),
+		OPT_CALLBACK_F(0, "minimal", options, NULL,
+			       N_("produce the smallest possible diff"),
+			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			       diff_opt_diff_algorithm_no_arg),
 		OPT_BIT_F('w', "ignore-all-space", &options->xdl_opts,
 			  N_("ignore whitespace when comparing lines"),
 			  XDF_IGNORE_WHITESPACE, PARSE_OPT_NONEG),
@@ -5589,10 +5609,11 @@  struct option *add_diff_options(const struct option *opts,
 		OPT_CALLBACK_F(0, "patience", options, NULL,
 			       N_("generate diff using the \"patience diff\" algorithm"),
 			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
-			       diff_opt_patience),
-		OPT_BITOP(0, "histogram", &options->xdl_opts,
-			  N_("generate diff using the \"histogram diff\" algorithm"),
-			  XDF_HISTOGRAM_DIFF, XDF_DIFF_ALGORITHM_MASK),
+			       diff_opt_diff_algorithm_no_arg),
+		OPT_CALLBACK_F(0, "histogram", options, NULL,
+			       N_("generate diff using the \"histogram diff\" algorithm"),
+			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			       diff_opt_diff_algorithm_no_arg),
 		OPT_CALLBACK_F(0, "diff-algorithm", options, N_("<algorithm>"),
 			       N_("choose a diff algorithm"),
 			       PARSE_OPT_NONEG, diff_opt_diff_algorithm),