diff mbox series

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

Message ID 816c47aa414586e99aa762604396bd8be4fb11f4.1676665285.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 11e95e16e8ef70c6c757d50a99f9b310e6c795f4
Headers show
Series Teach diff to honor diff algorithms set through git attributes | expand

Commit Message

John Cai Feb. 17, 2023, 8:21 p.m. UTC 
From: John Cai <johncai86@gmail.com>

A subsequent commit will need the ability to tell if the diff algorithm
was set through the command line through setting a new member of
diff_options. While this logic can be added to the
diff_opt_diff_algorithm() callback, the `--minimal` and `--histogram`
options are handled via OPT_BIT without a callback.

Remedy this by consolidating the options parsing logic for --minimal and
--histogram into one callback. This way we can modify `diff_options` in
that function.

As an additional refactor, 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 | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 14 deletions(-)

Comments

Junio C Hamano Feb. 17, 2023, 9:27 p.m. UTC | #1 
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> A subsequent commit will need the ability to tell if the diff algorithm
> was set through the command line through setting a new member of
> diff_options. While this logic can be added to the
> diff_opt_diff_algorithm() callback, the `--minimal` and `--histogram`
> options are handled via OPT_BIT without a callback.
>
> Remedy this by consolidating the options parsing logic for --minimal and
> --histogram into one callback. This way we can modify `diff_options` in
> that function.
>
> As an additional refactor, 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 | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 14 deletions(-)

Nicely done.  Much easier to follow.  Will queue.  Thanks.
Elijah Newren Feb. 18, 2023, 1:36 a.m. UTC | #2 
On Fri, Feb 17, 2023 at 12:21 PM John Cai via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: John Cai <johncai86@gmail.com>
>
> A subsequent commit will need the ability to tell if the diff algorithm
> was set through the command line through setting a new member of
> diff_options. While this logic can be added to the
> diff_opt_diff_algorithm() callback, the `--minimal` and `--histogram`
> options are handled via OPT_BIT without a callback.
>
> Remedy this by consolidating the options parsing logic for --minimal and
> --histogram into one callback. This way we can modify `diff_options` in
> that function.
>
> As an additional refactor, 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 | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 329eebf16a0..5efc22ca06b 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,28 @@ 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 (set_diff_algorithm(options, opt->long_name))
> +               BUG("available diff algorithms include \"myers\", "
> +                              "\"minimal\", \"patience\" and \"histogram\"");
> +
>         return 0;
>  }
>
> @@ -5250,7 +5277,6 @@ static int diff_opt_patience(const struct option *opt,
>
>         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
> @@ -5259,7 +5285,8 @@ static int diff_opt_patience(const struct option *opt,
>         for (i = 0; i < options->anchors_nr; i++)
>                 free(options->anchors[i]);
>         options->anchors_nr = 0;
> -       return 0;
> +
> +       return set_diff_algorithm(options, "patience");
>  }
>
>  static int diff_opt_ignore_regex(const struct option *opt,
> @@ -5562,9 +5589,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),
> @@ -5590,9 +5618,10 @@ struct option *add_diff_options(const struct option *opts,
>                                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),
> +               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),
> --
> gitgitgadget

This patch looks good to me.
diff mbox series

Patch

diff --git a/diff.c b/diff.c
index 329eebf16a0..5efc22ca06b 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,28 @@  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 (set_diff_algorithm(options, opt->long_name))
+		BUG("available diff algorithms include \"myers\", "
+			       "\"minimal\", \"patience\" and \"histogram\"");
+
 	return 0;
 }
 
@@ -5250,7 +5277,6 @@  static int diff_opt_patience(const struct option *opt,
 
 	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
@@ -5259,7 +5285,8 @@  static int diff_opt_patience(const struct option *opt,
 	for (i = 0; i < options->anchors_nr; i++)
 		free(options->anchors[i]);
 	options->anchors_nr = 0;
-	return 0;
+
+	return set_diff_algorithm(options, "patience");
 }
 
 static int diff_opt_ignore_regex(const struct option *opt,
@@ -5562,9 +5589,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),
@@ -5590,9 +5618,10 @@  struct option *add_diff_options(const struct option *opts,
 			       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),
+		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),