git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Feb 6, 2023, 16:20 UTC
Message-ID
<19a1c71c-d913-8118-a228-da72b5771421@dunelm.org.uk>
In-Reply-To
<f56bd38ac3f80fb3a7e8c92cadaa57d2b0754b9f.1675568781.git.gitgitgadget@gmail.com>
Hi John
On 05/02/2023 03:46, John Cai via GitGitGadget wrote:
Show 10 quoted lines
> 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

Show 62 quoted lines
> 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.

Show 13 quoted lines
> +		/*
> +		 * 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
Show 61 quoted lines
> +
>   	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),
Previous: John Cai via GitGitGadgetNext: John Cai via GitGitGadget
Message 3 of 78 in “Teach diff to honor diff algorithms set through git attributes”
  1. 0/2 Teach diff to honor diff algorithms set through git attributesJohn Cai via GitGitGadget, Feb 5, 2023
  2. 1/2 diff: consolidate diff algorithm option parsingJohn Cai via GitGitGadget, Feb 5, 2023
  3. Phillip WoodFeb 6, 2023
  4. 2/2 diff: teach diff to read gitattribute diff-algorithmJohn Cai via GitGitGadget, Feb 5, 2023
  5. Eric SunshineFeb 5, 2023
  6. John CaiFeb 6, 2023
  7. Phillip WoodFeb 6, 2023
  8. Eric SunshineFeb 6, 2023
  9. John CaiFeb 6, 2023
  10. Elijah NewrenFeb 9, 2023
  11. "bad" diffs (was: [PATCH 2/2] diff: teach diff to read gitattribute diff-algorithm)Ævar Arnfjörð Bjarmason, Feb 9, 2023
  12. John CaiFeb 9, 2023
  13. Ævar Arnfjörð BjarmasonFeb 6, 2023
  14. John CaiFeb 6, 2023
  15. Phillip WoodFeb 7, 2023
  16. John CaiFeb 7, 2023
  17. Elijah NewrenFeb 9, 2023
  18. Phillip WoodFeb 9, 2023
  19. Elijah NewrenFeb 10, 2023
  20. Phillip WoodFeb 11, 2023
  21. Jeff KingFeb 11, 2023
  22. Elijah NewrenFeb 15, 2023
  23. Jeff KingFeb 15, 2023
  24. Junio C HamanoFeb 15, 2023
  25. Phillip WoodFeb 15, 2023
  26. Jeff KingFeb 15, 2023
  27. Ævar Arnfjörð BjarmasonFeb 7, 2023
  28. Phillip WoodFeb 15, 2023
  29. Elijah NewrenFeb 9, 2023
  30. John CaiFeb 14, 2023
  31. Elijah NewrenFeb 15, 2023
  32. Elijah NewrenFeb 9, 2023
  33. Ævar Arnfjörð BjarmasonFeb 9, 2023
  34. Jeff KingFeb 11, 2023
  35. Jeff KingFeb 7, 2023
  36. Ævar Arnfjörð BjarmasonFeb 7, 2023
  37. Junio C HamanoFeb 7, 2023
  38. Ævar Arnfjörð BjarmasonFeb 7, 2023
  39. Junio C HamanoFeb 7, 2023
  40. Ævar Arnfjörð BjarmasonFeb 7, 2023
  41. John CaiFeb 9, 2023
  42. Jeff KingFeb 11, 2023
  43. 0/2 Teach diff to honor diff algorithms set through git attributesJohn Cai via GitGitGadget, Feb 14, 2023
  44. 1/2 diff: consolidate diff algorithm option parsingJohn Cai via GitGitGadget, Feb 14, 2023
  45. Junio C HamanoFeb 15, 2023
  46. John CaiFeb 15, 2023
  47. Junio C HamanoFeb 15, 2023
  48. Jeff KingFeb 16, 2023
  49. Junio C HamanoFeb 16, 2023
  50. John CaiFeb 16, 2023
  51. 2/2 diff: teach diff to read gitattribute diff-algorithmJohn Cai via GitGitGadget, Feb 14, 2023
  52. Junio C HamanoFeb 15, 2023
  53. Junio C HamanoFeb 15, 2023
  54. John CaiFeb 16, 2023
  55. 0/2 Teach diff to honor diff algorithms set through git attributesJohn Cai via GitGitGadget, Feb 17, 2023
  56. 1/2 diff: consolidate diff algorithm option parsingJohn Cai via GitGitGadget, Feb 17, 2023
  57. Junio C HamanoFeb 17, 2023
  58. Elijah NewrenFeb 18, 2023
  59. 2/2 diff: teach diff to read algorithm from diff driverJohn Cai via GitGitGadget, Feb 17, 2023
  60. Junio C HamanoFeb 17, 2023
  61. Elijah NewrenFeb 18, 2023
  62. John CaiFeb 20, 2023
  63. Elijah NewrenFeb 20, 2023
  64. John CaiFeb 20, 2023
  65. Elijah NewrenFeb 20, 2023
  66. John CaiFeb 20, 2023
  67. Jeff KingFeb 22, 2023
  68. John CaiFeb 24, 2023
  69. Elijah NewrenFeb 18, 2023
  70. John CaiFeb 20, 2023
  71. 0/2 Teach diff to honor diff algorithms set through git attributesJohn Cai via GitGitGadget, Feb 20, 2023
  72. 1/2 diff: consolidate diff algorithm option parsingJohn Cai via GitGitGadget, Feb 20, 2023
  73. 2/2 diff: teach diff to read algorithm from diff driverJohn Cai via GitGitGadget, Feb 20, 2023
  74. Junio C HamanoFeb 21, 2023
  75. Elijah NewrenFeb 21, 2023
  76. Junio C HamanoFeb 21, 2023
  77. John CaiFeb 21, 2023
  78. Elijah NewrenFeb 21, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.