threads / patch / 50790

patch, 20 partsnd/diff-parseopt the last part

Subject: [PATCH 00/20] nd/diff-parseopt the last part

## tl;dr

52 messages between Mar 20, 2019 and May 1, 2019. Diffs are folded; open one to read it.

replies: 51people: 5as markdown or json

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · lore

Gotta keep pumping until everything is out! This is the last part to finish converting diff.c to use parse_options().

The two commands 'range-diff' and 'diff --no-index' are also updated to do parse_options() directly, which is the end game (i.e. the end of diff_opt_parse(), but that can't happen until revision.c is converted).

The next step could be collect complaints about how noisy 'git range-diff -h' and 'git diff --no-index -h' is and start trimming unused options. revision.c conversion will happen in parallel.

Nguyễn Thái Ngọc Duy (20):
  diff-parseopt: convert --ws-error-highlight
  diff-parseopt: convert --ita-[in]visible-in-index
  diff-parseopt: convert -z
  diff-parseopt: convert -l
  diff-parseopt: convert -S|-G
  diff-parseopt: convert --pickaxe-all|--pickaxe-regex
  diff-parseopt: convert -O
  diff-parseopt: convert --find-object
  diff-parseopt: convert --diff-filter
  diff-parseopt: convert --[no-]abbrev
  diff-parseopt: convert --[src|dst]-prefix
  diff-parseopt: convert --line-prefix
  diff-parseopt: convert --no-prefix
  diff-parseopt: convert --inter-hunk-context
  diff-parseopt: convert --[no-]color-moved
  diff-parseopt: convert --color-moved-ws
  diff.c: allow --no-color-moved-ws
  range-diff: use parse_options() instead of diff_opt_parse()
  diff --no-index: use parse_options() instead of diff_opt_parse()
  am: avoid diff_opt_parse()

 builtin/am.c             |   4 +-
 builtin/diff.c           |  21 +--
 builtin/range-diff.c     |  26 +--
 diff-no-index.c          |  49 +++---
 diff.c                   | 337 +++++++++++++++++++--------------------
 diff.h                   |   5 +-
 parse-options-cb.c       |   4 +-
 parse-options.h          |   6 +-
 t/t4053-diff-no-index.sh |   3 +-
 9 files changed, 219 insertions(+), 236 deletions(-)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 01/20] diff-parseopt: convert --ws-error-highlight

mark one more string for translation while at there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Show changes to diff.c +12 −10

diff --git a/diff.c b/diff.c
index ce118bb326..8fdcdcc8ff 100644
--- a/diff.c
+++ b/diff.c
@@ -4801,17 +4801,18 @@ static void enable_patch_output(int *fmt)
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
-static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
+static int diff_opt_ws_error_highlight(const struct option *option,
+				       const char *arg, int unset)
 {
+	struct diff_options *opt = option->value;
 	int val = parse_ws_error_highlight(arg);
 
-	if (val < 0) {
-		error("unknown value after ws-error-highlight=%.*s",
-		      -1 - val, arg);
-		return 0;
-	}
+	BUG_ON_OPT_NEG(unset);
+	if (val < 0)
+		return error(_("unknown value after ws-error-highlight=%.*s"),
+			     -1 - val, arg);
 	opt->ws_error_highlight = val;
-	return 1;
+	return 0;
 }
 
 static int parse_objfind_opt(struct diff_options *opt, const char *arg)
@@ -5234,6 +5235,9 @@ static void prep_parse_options(struct diff_options *options)
 			 N_("show full pre- and post-image object names on the \"index\" lines")),
 		OPT_COLOR_FLAG(0, "color", &options->use_color,
 			       N_("show colored diff")),
+		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
+			       N_("highlight whitespaces errors in the context, old or new lines in the diff"),
+			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5397,9 +5401,7 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;
 		options->color_moved_ws_handling = cm;
-	} else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
-		return parse_ws_error_highlight_opt(options, arg);
-	else if (!strcmp(arg, "--ita-invisible-in-index"))
+	} else if (!strcmp(arg, "--ita-invisible-in-index"))
 		options->ita_invisible_in_index = 1;
 	else if (!strcmp(arg, "--ita-visible-in-index"))
 		options->ita_invisible_in_index = 0;

-- 
2.21.0.548.gd3c7d92dc2

Martin Ågren· Mar 20, 2019, 20:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 01/20] diff-parseopt: convert --ws-error-highlight

On Wed, 20 Mar 2019 at 12:59, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

>
> mark one more string for translation while at there.

s/^m/M/

> +               OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
> +                              N_("highlight whitespaces errors in the context, old or new lines in the diff"),
> +                              PARSE_OPT_NONEG, diff_opt_ws_error_highlight),

s/whitespaces/whitespace/

I tripped on the comma, somehow, and part of the explanation why is perhaps because this gives (a few of) the possible values, but that's not really indicated. Maybe wrap them in 'quotes', but maybe this text is already getting too long. Feel free to completely ignore this.

Martin

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 02/20] diff-parseopt: convert --ita-[in]visible-in-index

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Show changes to diff.c +7 −4

diff --git a/diff.c b/diff.c
index 8fdcdcc8ff..3eace63172 100644
--- a/diff.c
+++ b/diff.c
@@ -5352,6 +5352,12 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("specify how differences in submodules are shown"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
 			       diff_opt_submodule),
+		OPT_SET_INT_F(0, "ita-invisible-in-index", &options->ita_invisible_in_index,
+			      N_("hide 'git add -N' entries from the index"),
+			      1, PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "ita-visible-in-index", &options->ita_invisible_in_index,
+			      N_("treat 'git add -N' entries as real in the index"),
+			      0, PARSE_OPT_NONEG),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5401,10 +5407,7 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;
 		options->color_moved_ws_handling = cm;
-	} else if (!strcmp(arg, "--ita-invisible-in-index"))
-		options->ita_invisible_in_index = 1;
-	else if (!strcmp(arg, "--ita-visible-in-index"))
-		options->ita_invisible_in_index = 0;
+	}
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 03/20] diff-parseopt: convert -z

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Show changes to diff.c +3 −2

diff --git a/diff.c b/diff.c
index 3eace63172..f290dfe6be 100644
--- a/diff.c
+++ b/diff.c
@@ -5238,6 +5238,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
 			       N_("highlight whitespaces errors in the context, old or new lines in the diff"),
 			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
+		OPT_SET_INT('z', NULL, &options->line_termination,
+			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
+			    0),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5410,8 +5413,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "-z"))
-		options->line_termination = 0;
 	else if ((argcount = short_opt('l', av, &optarg))) {
 		options->rename_limit = strtoul(optarg, NULL, 10);
 		return argcount;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 04/20] diff-parseopt: convert -l

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Show changes to diff.c +2 −4

diff --git a/diff.c b/diff.c
index f290dfe6be..a2abc7e5b0 100644
--- a/diff.c
+++ b/diff.c
@@ -5283,6 +5283,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "follow", options, NULL,
 			       N_("continue listing the history of a file beyond renames"),
 			       PARSE_OPT_NOARG, diff_opt_follow),
+		OPT_INTEGER('l', NULL, &options->rename_limit,
+			    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,
@@ -5413,10 +5415,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('l', av, &optarg))) {
-		options->rename_limit = strtoul(optarg, NULL, 10);
-		return argcount;
-	}
 	else if ((argcount = short_opt('S', av, &optarg))) {
 		options->pickaxe = optarg;
 		options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 05/20] diff-parseopt: convert -S|-G

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

Show changes to diff.c +28 −9

diff --git a/diff.c b/diff.c
index a2abc7e5b0..e198129353 100644
--- a/diff.c
+++ b/diff.c
@@ -5056,6 +5056,28 @@ static int diff_opt_patience(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_pickaxe_regex(const struct option *opt,
+				  const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->pickaxe = arg;
+	options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;
+	return 0;
+}
+
+static int diff_opt_pickaxe_string(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->pickaxe = arg;
+	options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;
+	return 0;
+}
+
 static int diff_opt_relative(const struct option *opt,
 			     const char *arg, int unset)
 {
@@ -5363,6 +5385,12 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_SET_INT_F(0, "ita-visible-in-index", &options->ita_invisible_in_index,
 			      N_("treat 'git add -N' entries as real in the index"),
 			      0, PARSE_OPT_NONEG),
+		OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
+			       N_("look for differences that change the number of occurrences of the specified string"),
+			       0, diff_opt_pickaxe_string),
+		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
+			       N_("look for differences that change the number of occurrences of the specified regex"),
+			       0, diff_opt_pickaxe_regex),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5415,15 +5443,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('S', av, &optarg))) {
-		options->pickaxe = optarg;
-		options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;
-		return argcount;
-	} else if ((argcount = short_opt('G', av, &optarg))) {
-		options->pickaxe = optarg;
-		options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;
-		return argcount;
-	}
 	else if (!strcmp(arg, "--pickaxe-all"))
 		options->pickaxe_opts |= DIFF_PICKAXE_ALL;
 	else if (!strcmp(arg, "--pickaxe-regex"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 06/20] diff-parseopt: convert --pickaxe-all|--pickaxe-regex

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Show changes to diff.c +6 −4

diff --git a/diff.c b/diff.c
index e198129353..639b166c79 100644
--- a/diff.c
+++ b/diff.c
@@ -5391,6 +5391,12 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
 			       N_("look for differences that change the number of occurrences of the specified regex"),
 			       0, diff_opt_pickaxe_regex),
+		OPT_BIT_F(0, "pickaxe-all", &options->pickaxe_opts,
+			  N_("show all changes in the changeset with -S or -G"),
+			  DIFF_PICKAXE_ALL, PARSE_OPT_NONEG),
+		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
+			  N_("treat <string> in -S as extended POSIX regular expression"),
+			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5443,10 +5449,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--pickaxe-all"))
-		options->pickaxe_opts |= DIFF_PICKAXE_ALL;
-	else if (!strcmp(arg, "--pickaxe-regex"))
-		options->pickaxe_opts |= DIFF_PICKAXE_REGEX;
 	else if ((argcount = short_opt('O', av, &optarg))) {
 		options->orderfile = prefix_filename(prefix, optarg);
 		return argcount;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 07/20] diff-parseopt: convert -O

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

Show changes to diff.c +3 −20

diff --git a/diff.c b/diff.c
index 639b166c79..21c7a6b1a1 100644
--- a/diff.c
+++ b/diff.c
@@ -4617,22 +4617,6 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
 	return 1;
 }
 
-static inline int short_opt(char opt, const char **argv,
-			    const char **optarg)
-{
-	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != opt)
-		return 0;
-	if (arg[2] != '\0') {
-		*optarg = arg + 2;
-		return 1;
-	}
-	if (!argv[1])
-		die("Option '%c' requires a value", opt);
-	*optarg = argv[1];
-	return 2;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
@@ -5397,6 +5381,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
 			  N_("treat <string> in -S as extended POSIX regular expression"),
 			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
+		OPT_FILENAME('O', NULL, &options->orderfile,
+			     N_("override diff.orderFile configuration variable")),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5449,10 +5435,7 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('O', av, &optarg))) {
-		options->orderfile = prefix_filename(prefix, optarg);
-		return argcount;
-	} else if (skip_prefix(arg, "--find-object=", &arg))
+	else if (skip_prefix(arg, "--find-object=", &arg))
 		return parse_objfind_opt(options, arg);
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
 		int offending = parse_diff_filter_opt(optarg, options);

-- 
2.21.0.548.gd3c7d92dc2

Martin Ågren· Mar 20, 2019, 20:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 07/20] diff-parseopt: convert -O

On Wed, 20 Mar 2019 at 12:48, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> +               OPT_FILENAME('O', NULL, &options->orderfile,
> +                            N_("override diff.orderFile configuration variable")),

This doesn't really tell me *why* I would want to provide -O. Or put another way, there are lots of --foo-bar for overriding some baz.fooBar config item, but they're not described like this. The primary purpose of --foo-bar, at least to me, would be to solve some actual problem, like "fooing the bar", whatever that means.

Gleaning at the manpage, an alternative would be "control the order in which files appear in the output", but maybe you avoided that deliberately? "provide an orderfile"?

Martin

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 08/20] diff-parseopt: convert --find-object

while at it, mark one more string for translation.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Show changes to diff.c +9 −5

diff --git a/diff.c b/diff.c
index 21c7a6b1a1..694f32148c 100644
--- a/diff.c
+++ b/diff.c
@@ -4799,12 +4799,15 @@ static int diff_opt_ws_error_highlight(const struct option *option,
 	return 0;
 }
 
-static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+static int diff_opt_find_object(const struct option *option,
+				const char *arg, int unset)
 {
+	struct diff_options *opt = option->value;
 	struct object_id oid;
 
+	BUG_ON_OPT_NEG(unset);
 	if (get_oid(arg, &oid))
-		return error("unable to resolve '%s'", arg);
+		return error(_("unable to resolve '%s'"), arg);
 
 	if (!opt->objfind)
 		opt->objfind = xcalloc(1, sizeof(*opt->objfind));
@@ -4813,7 +4816,7 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)
 	opt->flags.recursive = 1;
 	opt->flags.tree_in_recursive = 1;
 	oidset_insert(opt->objfind, &oid);
-	return 1;
+	return 0;
 }
 
 static int diff_opt_anchored(const struct option *opt,
@@ -5383,6 +5386,9 @@ static void prep_parse_options(struct diff_options *options)
 			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
 		OPT_FILENAME('O', NULL, &options->orderfile,
 			     N_("override diff.orderFile configuration variable")),
+		OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
+			       N_("look for differences that change the number of occurrences of the specified object"),
+			       PARSE_OPT_NONEG, diff_opt_find_object),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5435,8 +5441,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (skip_prefix(arg, "--find-object=", &arg))
-		return parse_objfind_opt(options, arg);
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
 		int offending = parse_diff_filter_opt(optarg, options);
 		if (offending)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 09/20] diff-parseopt: convert --diff-filter

while at it, mark one more string for translation

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Show changes to diff.c +9 −9

diff --git a/diff.c b/diff.c
index 694f32148c..6e84af1cce 100644
--- a/diff.c
+++ b/diff.c
@@ -4736,10 +4736,13 @@ static unsigned filter_bit_tst(char status, const struct diff_options *opt)
 	return opt->filter & filter_bit[(int) status];
 }
 
-static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
+static int diff_opt_diff_filter(const struct option *option,
+				const char *optarg, int unset)
 {
+	struct diff_options *opt = option->value;
 	int i, optch;
 
+	BUG_ON_OPT_NEG(unset);
 	prepare_filter_bits();
 
 	/*
@@ -4770,7 +4773,8 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 
 		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
 		if (!bit)
-			return optarg[i];
+			return error(_("unknown change class '%c' in --diff-filter=%s"),
+				     optarg[i], optarg);
 		if (negate)
 			opt->filter &= ~bit;
 		else
@@ -5389,6 +5393,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
 			       N_("look for differences that change the number of occurrences of the specified object"),
 			       PARSE_OPT_NONEG, diff_opt_find_object),
+		OPT_CALLBACK_F(0, "diff-filter", options, N_("[(A|C|D|M|R|T|U|X|B)...[*]]"),
+			       N_("select files by diff type"),
+			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5441,13 +5448,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
-		int offending = parse_diff_filter_opt(optarg, options);
-		if (offending)
-			die("unknown change class '%c' in --diff-filter=%s",
-			    offending, optarg);
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-abbrev"))
 		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 10/20] diff-parseopt: convert --[no-]abbrev

OPT__ABBREV() has the same behavior as the deleted code with one difference: it does check for valid number and error out if not. And the '40' change is self explanatory.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c             | 12 +-----------
 parse-options-cb.c |  4 ++--
 2 files changed, 3 insertions(+), 13 deletions(-)

Show changes to 2 files +3 −13

diff.c, parse-options-cb.c

diff --git a/diff.c b/diff.c
index 6e84af1cce..30baf21021 100644
--- a/diff.c
+++ b/diff.c
@@ -5254,6 +5254,7 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_SET_INT('z', NULL, &options->line_termination,
 			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
 			    0),
+		OPT__ABBREV(&options->abbrev),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5448,17 +5449,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--no-abbrev"))
-		options->abbrev = 0;
-	else if (!strcmp(arg, "--abbrev"))
-		options->abbrev = DEFAULT_ABBREV;
-	else if (skip_prefix(arg, "--abbrev=", &arg)) {
-		options->abbrev = strtoul(arg, NULL, 10);
-		if (options->abbrev < MINIMUM_ABBREV)
-			options->abbrev = MINIMUM_ABBREV;
-		else if (the_hash_algo->hexsz < options->abbrev)
-			options->abbrev = the_hash_algo->hexsz;
-	}
 	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
 		options->a_prefix = optarg;
 		return argcount;
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2733393546..6e2e8d6273 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -22,8 +22,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > 40)
-			v = 40;
+		else if (v > the_hash_algo->hexsz)
+			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;
 	return 0;

-- 
2.21.0.548.gd3c7d92dc2

Ævar Arnfjörð Bjarmason· Mar 20, 2019, 23:00 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 10/20] diff-parseopt: convert --[no-]abbrev

On Wed, Mar 20 2019, Nguyễn Thái Ngọc Duy wrote:

> [...]And the '40' change is self explanatory.

Let me make an attempt at being dense anyway...

Show 5 quoted lines

> -		else if (v > 40)
> -			v = 40;
> +		else if (v > the_hash_algo->hexsz)
> +			v = the_hash_algo->hexsz;
>  	}

This is obviously not a regression, it's a hardcoded 40 *now*. So we should take this patch.

But in general, I wonder how this is going to work once we get a few steps further into the SHA-256 migration. I.e. here we're still parsing the command-line, and the_hash_algo might be initialized early to SHA-1.

So if I set --abbrev=45 it'll be trimmed to --abbrev=40 by this code.

But then shortly afterwards we pass my SHA-256 object down to some machinery, and will then want to abbreviate it.

Isn't that part of the code something we're going to want to support looking up objects in either hash, even if we initially started out with SHA-1 in the_hash_algo? So we'll be over-abbreviating a SHA-256 object.

Leaving aside the sillyness of wanting to abbreviate *anything* to 45 characters, I wonder how those sorts of chicken & egg hash scenarios will go involving the_hash_algo.

Duy Nguyen· Mar 21, 2019, 00:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 10/20] diff-parseopt: convert --[no-]abbrev

On Thu, Mar 21, 2019 at 6:00 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 20 quoted lines

>
>
> On Wed, Mar 20 2019, Nguyễn Thái Ngọc Duy wrote:
>
> > [...]And the '40' change is self explanatory.
>
> Let me make an attempt at being dense anyway...
>
> > -             else if (v > 40)
> > -                     v = 40;
> > +             else if (v > the_hash_algo->hexsz)
> > +                     v = the_hash_algo->hexsz;
> >       }
>
> This is obviously not a regression, it's a hardcoded 40 *now*. So we
> should take this patch.
>
> But in general, I wonder how this is going to work once we get a few
> steps further into the SHA-256 migration. I.e. here we're still parsing
> the command-line, and the_hash_algo might be initialized early to SHA-1.

That would be wrong. the_hash_algo must be properly initialized by the time any command parsing is done (except maybe "git <options> <cmd>"). While parse_options() most of the time is just a dumb "set this variable, set that variable", it often can have callbacks to do more complicated stuff and we can't just go with "pre-initialized to SHA-1" assumption. That's as bad as "assume $CWD is worktree" until worktree is discovered.

There is a corner case though. If some command takes hash algo as an option (e.g. git hash-object should work without a repo) then yes we might have a problem since the_hash_algo might not be initialized yet, depending on option order.

Show 12 quoted lines

> So if I set --abbrev=45 it'll be trimmed to --abbrev=40 by this code.
>
> But then shortly afterwards we pass my SHA-256 object down to some
> machinery, and will then want to abbreviate it.
>
> Isn't that part of the code something we're going to want to support
> looking up objects in either hash, even if we initially started out with
> SHA-1 in the_hash_algo? So we'll be over-abbreviating a SHA-256 object.
>
> Leaving aside the sillyness of wanting to abbreviate *anything* to 45
> characters, I wonder how those sorts of chicken & egg hash scenarios
> will go involving the_hash_algo.

-- 
Duy

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 11/20] diff-parseopt: convert --[src|dst]-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c          | 14 ++++++--------
 parse-options.h |  3 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

Show changes to 2 files +8 −9

diff.c, parse-options.h

diff --git a/diff.c b/diff.c
index 30baf21021..2d34dc878e 100644
--- a/diff.c
+++ b/diff.c
@@ -5255,6 +5255,12 @@ static void prep_parse_options(struct diff_options *options)
 			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
 			    0),
 		OPT__ABBREV(&options->abbrev),
+		OPT_STRING_F(0, "src-prefix", &options->a_prefix, N_("<prefix>"),
+			     N_("show the given source prefix instead of \"a/\""),
+			     PARSE_OPT_NONEG),
+		OPT_STRING_F(0, "dst-prefix", &options->b_prefix, N_("<prefix>"),
+			     N_("show the given source prefix instead of \"b/\""),
+			     PARSE_OPT_NONEG),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5449,20 +5455,12 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
-		options->a_prefix = optarg;
-		return argcount;
-	}
 	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
 		options->line_prefix = optarg;
 		options->line_prefix_length = strlen(options->line_prefix);
 		graph_setup_line_prefix(options);
 		return argcount;
 	}
-	else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
-		options->b_prefix = optarg;
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",
diff --git a/parse-options.h b/parse-options.h
index 7d83e2971d..c95cbe26f0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -136,6 +136,7 @@ struct option {
 #define OPT_BOOL_F(s, l, v, h, f)   OPT_SET_INT_F(s, l, v, h, 1, f)
 #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)			\
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
+#define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), (v), (a), (h), (f) }
 
 #define OPT_END()                   { OPTION_END }
 #define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
@@ -157,7 +158,7 @@ struct option {
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
-#define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
+#define OPT_STRING(s, l, v, a, h)   OPT_STRING_F(s, l, v, a, h, 0)
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \
 				      (h), 0, &parse_opt_string_list }

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 12/20] diff-parseopt: convert --line-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Show changes to diff.c +15 −8

diff --git a/diff.c b/diff.c
index 2d34dc878e..4ab5b3c06a 100644
--- a/diff.c
+++ b/diff.c
@@ -5010,6 +5010,18 @@ static int diff_opt_ignore_submodules(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_line_prefix(const struct option *opt,
+				const char *optarg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->line_prefix = optarg;
+	options->line_prefix_length = strlen(options->line_prefix);
+	graph_setup_line_prefix(options);
+	return 0;
+}
+
 static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
 					     const struct option *opt,
 					     const char *arg, int unset)
@@ -5261,6 +5273,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_STRING_F(0, "dst-prefix", &options->b_prefix, N_("<prefix>"),
 			     N_("show the given source prefix instead of \"b/\""),
 			     PARSE_OPT_NONEG),
+		OPT_CALLBACK_F(0, "line-prefix", options, N_("<prefix>"),
+			       N_("prepend an additional prefix to every line of output"),
+			       PARSE_OPT_NONEG, diff_opt_line_prefix),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5418,8 +5433,6 @@ int diff_opt_parse(struct diff_options *options,
 		   const char **av, int ac, const char *prefix)
 {
 	const char *arg = av[0];
-	const char *optarg;
-	int argcount;
 
 	if (!prefix)
 		prefix = "";
@@ -5455,12 +5468,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
-		options->line_prefix = optarg;
-		options->line_prefix_length = strlen(options->line_prefix);
-		graph_setup_line_prefix(options);
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 13/20] diff-parseopt: convert --no-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Show changes to diff.c +15 −2

diff --git a/diff.c b/diff.c
index 4ab5b3c06a..045fdbb882 100644
--- a/diff.c
+++ b/diff.c
@@ -5022,6 +5022,18 @@ static int diff_opt_line_prefix(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_no_prefix(const struct option *opt,
+			      const char *optarg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(optarg);
+	options->a_prefix = "";
+	options->b_prefix = "";
+	return 0;
+}
+
 static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
 					     const struct option *opt,
 					     const char *arg, int unset)
@@ -5276,6 +5288,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "line-prefix", options, N_("<prefix>"),
 			       N_("prepend an additional prefix to every line of output"),
 			       PARSE_OPT_NONEG, diff_opt_line_prefix),
+		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
+			       N_("no not show any source or destination prefix"),
+			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5468,8 +5483,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--no-prefix"))
-		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",
 			 &options->interhunkcontext))
 		;

-- 
2.21.0.548.gd3c7d92dc2

Martin Ågren· Mar 20, 2019, 20:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 13/20] diff-parseopt: convert --no-prefix

On Wed, 20 Mar 2019 at 12:50, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> +               OPT_CALLBACK_F(0, "no-prefix", options, NULL,
> +                              N_("no not show any source or destination prefix"),
> +                              PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),

s/no not/do not/

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 14/20] diff-parseopt: convert --inter-hunk-context

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c          | 50 +++----------------------------------------------
 parse-options.h |  3 ++-
 2 files changed, 5 insertions(+), 48 deletions(-)

Show changes to 2 files +5 −48

diff.c, parse-options.h

diff --git a/diff.c b/diff.c
index 045fdbb882..e2f515a7da 100644
--- a/diff.c
+++ b/diff.c
@@ -4573,50 +4573,6 @@ void diff_setup_done(struct diff_options *options)
 	FREE_AND_NULL(options->parseopts);
 }
 
-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
-{
-	char c, *eq;
-	int len;
-
-	if (*arg != '-')
-		return 0;
-	c = *++arg;
-	if (!c)
-		return 0;
-	if (c == arg_short) {
-		c = *++arg;
-		if (!c)
-			return 1;
-		if (val && isdigit(c)) {
-			char *end;
-			int n = strtoul(arg, &end, 10);
-			if (*end)
-				return 0;
-			*val = n;
-			return 1;
-		}
-		return 0;
-	}
-	if (c != '-')
-		return 0;
-	arg++;
-	eq = strchrnul(arg, '=');
-	len = eq - arg;
-	if (!len || strncmp(arg, arg_long, len))
-		return 0;
-	if (*eq) {
-		int n;
-		char *end;
-		if (!isdigit(*++eq))
-			return 0;
-		n = strtoul(eq, &end, 10);
-		if (*end)
-			return 0;
-		*val = n;
-	}
-	return 1;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
@@ -5291,6 +5247,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
 			       N_("no not show any source or destination prefix"),
 			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
+		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
+			      N_("show context between diff hunks up to the specified number of lines"),
+			      PARSE_OPT_NONEG),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5483,9 +5442,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (opt_arg(arg, '\0', "inter-hunk-context",
-			 &options->interhunkcontext))
-		;
 	else
 		return 0;
 	return 1;
diff --git a/parse-options.h b/parse-options.h
index c95cbe26f0..cca64d04cb 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -137,6 +137,7 @@ struct option {
 #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)			\
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
 #define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), (v), (a), (h), (f) }
+#define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h), (f) }
 
 #define OPT_END()                   { OPTION_END }
 #define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
@@ -155,7 +156,7 @@ struct option {
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
-#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
+#define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
 #define OPT_STRING(s, l, v, a, h)   OPT_STRING_F(s, l, v, a, h, 0)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 15/20] diff-parseopt: convert --[no-]color-moved

mark one more string for translation while at there

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

Show changes to diff.c +25 −13

diff --git a/diff.c b/diff.c
index e2f515a7da..df4303de4e 100644
--- a/diff.c
+++ b/diff.c
@@ -4841,6 +4841,27 @@ static int diff_opt_char(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_color_moved(const struct option *opt,
+				const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	if (unset) {
+		options->color_moved = COLOR_MOVED_NO;
+	} else if (!arg) {
+		if (diff_color_moved_default)
+			options->color_moved = diff_color_moved_default;
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
+	} else {
+		int cm = parse_color_moved(arg);
+		if (cm < 0)
+			return error(_("bad --color-moved argument: %s"), arg);
+		options->color_moved = cm;
+	}
+	return 0;
+}
+
 static int diff_opt_color_words(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -5339,6 +5360,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "color-words", options, N_("<regex>"),
 			       N_("equivalent to --word-diff=color --word-diff-regex=<regex>"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_color_words),
+		OPT_CALLBACK_F(0, "color-moved", options, N_("<mode>"),
+			       N_("move lines of code are colored differently"),
+			       PARSE_OPT_OPTARG, diff_opt_color_moved),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),
@@ -5422,19 +5446,7 @@ int diff_opt_parse(struct diff_options *options,
 		return ac;
 
 	/* flags options */
-	if (!strcmp(arg, "--color-moved")) {
-		if (diff_color_moved_default)
-			options->color_moved = diff_color_moved_default;
-		if (options->color_moved == COLOR_MOVED_NO)
-			options->color_moved = COLOR_MOVED_DEFAULT;
-	} else if (!strcmp(arg, "--no-color-moved"))
-		options->color_moved = COLOR_MOVED_NO;
-	else if (skip_prefix(arg, "--color-moved=", &arg)) {
-		int cm = parse_color_moved(arg);
-		if (cm < 0)
-			return error("bad --color-moved argument: %s", arg);
-		options->color_moved = cm;
-	} else if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
+	if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
 		unsigned cm = parse_color_moved_ws(arg);
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:46 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 16/20] diff-parseopt: convert --color-moved-ws

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Show changes to diff.c +18 −17

diff --git a/diff.c b/diff.c
index df4303de4e..8f79d3f002 100644
--- a/diff.c
+++ b/diff.c
@@ -4862,6 +4862,20 @@ static int diff_opt_color_moved(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_color_moved_ws(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+	unsigned cm;
+
+	BUG_ON_OPT_NEG(unset);
+	cm = parse_color_moved_ws(arg);
+	if (cm & COLOR_MOVED_WS_ERROR)
+		return error(_("invalid mode '%s' in --color-moved-ws"), arg);
+	options->color_moved_ws_handling = cm;
+	return 0;
+}
+
 static int diff_opt_color_words(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -5363,6 +5377,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "color-moved", options, N_("<mode>"),
 			       N_("move lines of code are colored differently"),
 			       PARSE_OPT_OPTARG, diff_opt_color_moved),
+		OPT_CALLBACK_F(0, "color-moved-ws", options, N_("<mode>"),
+			       N_("how white spaces are ignored in --color-moved"),
+			       PARSE_OPT_NONEG, diff_opt_color_moved_ws),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),
@@ -5430,8 +5447,6 @@ static void prep_parse_options(struct diff_options *options)
 int diff_opt_parse(struct diff_options *options,
 		   const char **av, int ac, const char *prefix)
 {
-	const char *arg = av[0];
-
 	if (!prefix)
 		prefix = "";
 
@@ -5442,21 +5457,7 @@ int diff_opt_parse(struct diff_options *options,
 			   PARSE_OPT_ONE_SHOT |
 			   PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (ac)
-		return ac;
-
-	/* flags options */
-	if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
-		unsigned cm = parse_color_moved_ws(arg);
-		if (cm & COLOR_MOVED_WS_ERROR)
-			return -1;
-		options->color_moved_ws_handling = cm;
-	}
-
-	/* misc options */
-	else
-		return 0;
-	return 1;
+	return ac;
 }
 
 int parse_rename_score(const char **cp_p)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 17/20] diff.c: allow --no-color-moved-ws

This option is added in commit b73bcbac4a (diff: allow --no-color-moved-ws - 2018-11-23) in pw/diff-color-moved-ws-fix. To ease merge conflict resolution, re-implement the option handling here so that the conflict could be resolved by taking this side of change.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Show changes to diff.c +6 −2

diff --git a/diff.c b/diff.c
index 8f79d3f002..6ce59d0621 100644
--- a/diff.c
+++ b/diff.c
@@ -4868,7 +4868,11 @@ static int diff_opt_color_moved_ws(const struct option *opt,
 	struct diff_options *options = opt->value;
 	unsigned cm;
 
-	BUG_ON_OPT_NEG(unset);
+	if (unset) {
+		options->color_moved_ws_handling = 0;
+		return 0;
+	}
+
 	cm = parse_color_moved_ws(arg);
 	if (cm & COLOR_MOVED_WS_ERROR)
 		return error(_("invalid mode '%s' in --color-moved-ws"), arg);
@@ -5379,7 +5383,7 @@ static void prep_parse_options(struct diff_options *options)
 			       PARSE_OPT_OPTARG, diff_opt_color_moved),
 		OPT_CALLBACK_F(0, "color-moved-ws", options, N_("<mode>"),
 			       N_("how white spaces are ignored in --color-moved"),
-			       PARSE_OPT_NONEG, diff_opt_color_moved_ws),
+			       0, diff_opt_color_moved_ws),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 18/20] range-diff: use parse_options() instead of diff_opt_parse()

Diff's internal option parsing is now done with 'struct option', which makes it possible to combine all diff options to range-diff and parse everything all at once. Parsing code becomes simpler, and we get a looong 'git range-diff -h'

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/range-diff.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

Show changes to builtin/range-diff.c +6 −20

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f01a0be851..784bd19321 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -16,42 +16,27 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
 	int simple_color = -1;
-	struct option options[] = {
+	struct option range_diff_options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
 		OPT_END()
 	};
-	int i, j, res = 0;
+	struct option *options;
+	int res = 0;
 	struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
 
 	git_config(git_diff_ui_config, NULL);
 
 	repo_diff_setup(the_repository, &diffopt);
 
+	options = parse_options_concat(range_diff_options, diffopt.parseopts);
 	argc = parse_options(argc, argv, NULL, options,
-			     builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
-			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
-
-	for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
-		int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+			     builtin_range_diff_usage, 0);
 
-		if (!c)
-			argv[j++] = argv[i++];
-		else
-			i += c;
-	}
-	while (i < argc)
-		argv[j++] = argv[i++];
-	argc = j;
 	diff_setup_done(&diffopt);
 
-	/* Make sure that there are no unparsed options */
-	argc = parse_options(argc, argv, NULL,
-			     options + ARRAY_SIZE(options) - 1, /* OPT_END */
-			     builtin_range_diff_usage, 0);
-
 	/* force color when --dual-color was used */
 	if (!simple_color)
 		diffopt.use_color = 1;
@@ -90,6 +75,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		error(_("need two commit ranges"));
 		usage_with_options(builtin_range_diff_usage, options);
 	}
+	FREE_AND_NULL(options);
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
 			      simple_color < 1, &diffopt);

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

While at there, move exit() back to the caller. It's easier to see the flow that way than burying it in diff-no-index.c

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/diff.c           | 21 +++--------------
 diff-no-index.c          | 49 ++++++++++++++++++++++++----------------
 diff.h                   |  3 ++-
 t/t4053-diff-no-index.sh |  3 +--
 4 files changed, 35 insertions(+), 41 deletions(-)

Show changes to 4 files +35 −41

builtin/diff.c, diff-no-index.c, diff.h, t/t4053-diff-no-index.sh

diff --git a/builtin/diff.c b/builtin/diff.c
index f0393bba23..52dc3e136f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -320,26 +320,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	repo_init_revisions(the_repository, &rev, prefix);
 
-	if (no_index && argc != i + 2) {
-		if (no_index == DIFF_NO_INDEX_IMPLICIT) {
-			/*
-			 * There was no --no-index and there were not two
-			 * paths. It is possible that the user intended
-			 * to do an inside-repository operation.
-			 */
-			fprintf(stderr, "Not a git repository\n");
-			fprintf(stderr,
-				"To compare two paths outside a working tree:\n");
-		}
-		/* Give the usage message for non-repository usage and exit. */
-		usagef("git diff %s <path> <path>",
-		       no_index == DIFF_NO_INDEX_EXPLICIT ?
-		       "--no-index" : "[--no-index]");
-
-	}
 	if (no_index)
 		/* If this is a no-index diff, just run it and exit there. */
-		diff_no_index(the_repository, &rev, argc, argv);
+		exit(diff_no_index(the_repository, &rev,
+				   no_index == DIFF_NO_INDEX_IMPLICIT,
+				   argc, argv));
 
 	/* Otherwise, we are doing the usual "git" diff */
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
diff --git a/diff-no-index.c b/diff-no-index.c
index 9414e922d1..a879f45862 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -14,6 +14,7 @@
 #include "revision.h"
 #include "log-tree.h"
 #include "builtin.h"
+#include "parse-options.h"
 #include "string-list.h"
 #include "dir.h"
 
@@ -233,35 +234,43 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
 	}
 }
 
-void diff_no_index(struct repository *r,
-		   struct rev_info *revs,
-		   int argc, const char **argv)
+static const char * const diff_no_index_usage[] = {
+	N_("git diff --no-index [<options>] <path> <path>"),
+	NULL
+};
+
+int diff_no_index(struct repository *r,
+		  struct rev_info *revs,
+		  int implicit_no_index,
+		  int argc, const char **argv)
 {
-	int i;
+	int i, no_index;
 	const char *paths[2];
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
+	struct option no_index_options[] = {
+		OPT_BOOL_F(0, "no-index", &no_index, "",
+			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
+		OPT_END(),
+	};
+	struct option *options;
 
 	/*
 	 * FIXME: --no-index should not look at index and we should be
 	 * able to pass NULL repo. Maybe later.
 	 */
 	repo_diff_setup(r, &revs->diffopt);
-	for (i = 1; i < argc - 2; ) {
-		int j;
-		if (!strcmp(argv[i], "--no-index"))
-			i++;
-		else if (!strcmp(argv[i], "--"))
-			i++;
-		else {
-			j = diff_opt_parse(&revs->diffopt, argv + i, argc - i,
-					   revs->prefix);
-			if (j <= 0)
-				die("invalid diff option/value: %s", argv[i]);
-			i += j;
-		}
+	options = parse_options_concat(no_index_options,
+				       revs->diffopt.parseopts);
+	argc = parse_options(argc, argv, revs->prefix, options,
+			     diff_no_index_usage, 0);
+	if (argc != 2) {
+		if (implicit_no_index)
+			warning(_("Not a git repository. Use --no-index to "
+				  "compare two paths outside a working tree"));
+		usage_with_options(diff_no_index_usage, options);
 	}
-
+	FREE_AND_NULL(options);
 	for (i = 0; i < 2; i++) {
 		const char *p = argv[argc - 2 + i];
 		if (!strcmp(p, "-"))
@@ -293,7 +302,7 @@ void diff_no_index(struct repository *r,
 	revs->diffopt.flags.exit_with_status = 1;
 
 	if (queue_diff(&revs->diffopt, paths[0], paths[1]))
-		exit(1);
+		return 1;
 	diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
@@ -304,5 +313,5 @@ void diff_no_index(struct repository *r,
 	 * The return code for --no-index imitates diff(1):
 	 * 0 = no changes, 1 = changes, else error
 	 */
-	exit(diff_result_code(&revs->diffopt, 0));
+	return diff_result_code(&revs->diffopt, 0);
 }
diff --git a/diff.h b/diff.h
index d9ad73f0e1..03c6afda22 100644
--- a/diff.h
+++ b/diff.h
@@ -437,7 +437,8 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 
 int diff_result_code(struct diff_options *, int);
 
-void diff_no_index(struct repository *, struct rev_info *, int, const char **);
+int diff_no_index(struct repository *, struct rev_info *,
+		  int implicit_no_index, int, const char **);
 
 int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 6e0dd6f9e5..fb25cdb789 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -50,8 +50,7 @@ test_expect_success 'git diff --no-index executed outside repo gives correct err
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git diff --no-index a 2>actual.err &&
-		echo "usage: git diff --no-index <path> <path>" >expect.err &&
-		test_cmp expect.err actual.err
+		test_i18ngrep "usage: git diff --no-index" actual.err
 	)
 '

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 20, 2019, 11:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 20/20] am: avoid diff_opt_parse()

diff_opt_parse() is a heavy hammer to just set diff filter. But it's the only way because of the diff_status_letters[] mapping. Add a new API to set diff filter and use it in git-am. diff_opt_parse()'s only remaining call site in revision.c will be gone soon and having it here just because of git-am does not make sense.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/am.c | 4 ++--
 diff.c       | 6 ++++++
 diff.h       | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

Show changes to 3 files +10 −2

builtin/am.c, diff.c, diff.h

diff --git a/builtin/am.c b/builtin/am.c
index 95370313b6..0cbf285459 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1515,11 +1515,11 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 		 * review them with extra care to spot mismerges.
 		 */
 		struct rev_info rev_info;
-		const char *diff_filter_str = "--diff-filter=AM";
 
 		repo_init_revisions(the_repository, &rev_info, NULL);
 		rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
-		diff_opt_parse(&rev_info.diffopt, &diff_filter_str, 1, rev_info.prefix);
+		rev_info.diffopt.filter |= diff_filter_bit('A');
+		rev_info.diffopt.filter |= diff_filter_bit('M');
 		add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
 		diff_setup_done(&rev_info.diffopt);
 		run_diff_index(&rev_info, 1);
diff --git a/diff.c b/diff.c
index 6ce59d0621..b94befd494 100644
--- a/diff.c
+++ b/diff.c
@@ -4692,6 +4692,12 @@ static unsigned filter_bit_tst(char status, const struct diff_options *opt)
 	return opt->filter & filter_bit[(int) status];
 }
 
+unsigned diff_filter_bit(char status)
+{
+	prepare_filter_bits();
+	return filter_bit[(int) status];
+}
+
 static int diff_opt_diff_filter(const struct option *option,
 				const char *optarg, int unset)
 {
diff --git a/diff.h b/diff.h
index 03c6afda22..f88482705c 100644
--- a/diff.h
+++ b/diff.h
@@ -233,6 +233,8 @@ struct diff_options {
 	struct option *parseopts;
 };
 
+unsigned diff_filter_bit(char status);
+
 void diff_emit_submodule_del(struct diff_options *o, const char *line);
 void diff_emit_submodule_add(struct diff_options *o, const char *line);
 void diff_emit_submodule_untracked(struct diff_options *o, const char *path);

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 00/20] nd/diff-parseopt the last part

v2 fixes some help text and commit messages. No major changes.

Nguyễn Thái Ngọc Duy (20):
  diff-parseopt: convert --ws-error-highlight
  diff-parseopt: convert --ita-[in]visible-in-index
  diff-parseopt: convert -z
  diff-parseopt: convert -l
  diff-parseopt: convert -S|-G
  diff-parseopt: convert --pickaxe-all|--pickaxe-regex
  diff-parseopt: convert -O
  diff-parseopt: convert --find-object
  diff-parseopt: convert --diff-filter
  diff-parseopt: convert --[no-]abbrev
  diff-parseopt: convert --[src|dst]-prefix
  diff-parseopt: convert --line-prefix
  diff-parseopt: convert --no-prefix
  diff-parseopt: convert --inter-hunk-context
  diff-parseopt: convert --[no-]color-moved
  diff-parseopt: convert --color-moved-ws
  diff.c: allow --no-color-moved-ws
  range-diff: use parse_options() instead of diff_opt_parse()
  diff --no-index: use parse_options() instead of diff_opt_parse()
  am: avoid diff_opt_parse()

 builtin/am.c             |   4 +-
 builtin/diff.c           |  21 +--
 builtin/range-diff.c     |  26 +--
 diff-no-index.c          |  49 +++---
 diff.c                   | 337 +++++++++++++++++++--------------------
 diff.h                   |   5 +-
 parse-options-cb.c       |   4 +-
 parse-options.h          |   6 +-
 t/t4053-diff-no-index.sh |   3 +-
 9 files changed, 219 insertions(+), 236 deletions(-)

Range-diff dựa trên v1:
 1:  676bba9a4c !  1:  ed9901217e diff-parseopt: convert --ws-error-highlight
    @@ -2,7 +2,7 @@
     
         diff-parseopt: convert --ws-error-highlight
     
    -    mark one more string for translation while at there.
    +    Mark one more string for translation while at there.
     
      diff --git a/diff.c b/diff.c
      --- a/diff.c
    @@ -38,7 +38,7 @@
      		OPT_COLOR_FLAG(0, "color", &options->use_color,
      			       N_("show colored diff")),
     +		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
    -+			       N_("highlight whitespaces errors in the context, old or new lines in the diff"),
    ++			       N_("highlight whitespace errors in the 'context', 'old' or 'new' lines in the diff"),
     +			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
      		OPT_CALLBACK_F(0, "output-indicator-new",
      			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 2:  94fb8a961b =  2:  3ad3991675 diff-parseopt: convert --ita-[in]visible-in-index
 3:  7b2c38485f !  3:  ea21fa9e03 diff-parseopt: convert -z
    @@ -7,7 +7,7 @@
      +++ b/diff.c
     @@
      		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
    - 			       N_("highlight whitespaces errors in the context, old or new lines in the diff"),
    + 			       N_("highlight whitespace errors in the 'context', 'old' or 'new' lines in the diff"),
      			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
     +		OPT_SET_INT('z', NULL, &options->line_termination,
     +			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
 4:  470c29dfb6 =  4:  02b7116f53 diff-parseopt: convert -l
 5:  f950b910dd =  5:  2d43346824 diff-parseopt: convert -S|-G
 6:  a3d8dfbb9e =  6:  357b691470 diff-parseopt: convert --pickaxe-all|--pickaxe-regex
 7:  209794414b !  7:  adc09e748e diff-parseopt: convert -O
    @@ -33,7 +33,7 @@
      			  N_("treat <string> in -S as extended POSIX regular expression"),
      			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
     +		OPT_FILENAME('O', NULL, &options->orderfile,
    -+			     N_("override diff.orderFile configuration variable")),
    ++			     N_("control the order in which files appear in the output")),
      		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
      		  N_("Output to a specific file"),
      		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
 8:  11730d9a4a !  8:  ca34bbd042 diff-parseopt: convert --find-object
    @@ -2,7 +2,7 @@
     
         diff-parseopt: convert --find-object
     
    -    while at it, mark one more string for translation.
    +    While at it, mark one more string for translation.
     
      diff --git a/diff.c b/diff.c
      --- a/diff.c
    @@ -37,7 +37,7 @@
     @@
      			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
      		OPT_FILENAME('O', NULL, &options->orderfile,
    - 			     N_("override diff.orderFile configuration variable")),
    + 			     N_("control the order in which files appear in the output")),
     +		OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
     +			       N_("look for differences that change the number of occurrences of the specified object"),
     +			       PARSE_OPT_NONEG, diff_opt_find_object),
 9:  a70f80ffa5 !  9:  6a735031e0 diff-parseopt: convert --diff-filter
    @@ -2,7 +2,7 @@
     
         diff-parseopt: convert --diff-filter
     
    -    while at it, mark one more string for translation
    +    While at it, mark one more string for translation
     
      diff --git a/diff.c b/diff.c
      --- a/diff.c
10:  95f2d0e3b0 = 10:  646a5844f0 diff-parseopt: convert --[no-]abbrev
11:  58bbccd2df = 11:  20e050397a diff-parseopt: convert --[src|dst]-prefix
12:  4454256a15 = 12:  ae0d85b620 diff-parseopt: convert --line-prefix
13:  1b882aaeb1 ! 13:  288d4fd648 diff-parseopt: convert --no-prefix
    @@ -29,7 +29,7 @@
      			       N_("prepend an additional prefix to every line of output"),
      			       PARSE_OPT_NONEG, diff_opt_line_prefix),
     +		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
    -+			       N_("no not show any source or destination prefix"),
    ++			       N_("do not show any source or destination prefix"),
     +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
      		OPT_CALLBACK_F(0, "output-indicator-new",
      			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
14:  3b1b06c51b ! 14:  753173f724 diff-parseopt: convert --inter-hunk-context
    @@ -58,7 +58,7 @@
      {
     @@
      		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
    - 			       N_("no not show any source or destination prefix"),
    + 			       N_("do not show any source or destination prefix"),
      			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
     +		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
     +			      N_("show context between diff hunks up to the specified number of lines"),
15:  afc89eb973 ! 15:  51cb7b3beb diff-parseopt: convert --[no-]color-moved
    @@ -2,7 +2,7 @@
     
         diff-parseopt: convert --[no-]color-moved
     
    -    mark one more string for translation while at there
    +    Mark one more string for translation while at there
     
      diff --git a/diff.c b/diff.c
      --- a/diff.c
16:  34a2457af1 = 16:  c1a0faa93b diff-parseopt: convert --color-moved-ws
17:  cc882ae588 = 17:  1e99596a25 diff.c: allow --no-color-moved-ws
18:  60ea27b58d = 18:  d7272ff0c9 range-diff: use parse_options() instead of diff_opt_parse()
19:  96e72eb330 = 19:  09c98fb8fd diff --no-index: use parse_options() instead of diff_opt_parse()
20:  d7273eff56 = 20:  59dbe880bc am: avoid diff_opt_parse()

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 01/20] diff-parseopt: convert --ws-error-highlight

Mark one more string for translation while at there.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Show changes to diff.c +12 −10

diff --git a/diff.c b/diff.c
index ce118bb326..7bfdf08137 100644
--- a/diff.c
+++ b/diff.c
@@ -4801,17 +4801,18 @@ static void enable_patch_output(int *fmt)
 	*fmt |= DIFF_FORMAT_PATCH;
 }
 
-static int parse_ws_error_highlight_opt(struct diff_options *opt, const char *arg)
+static int diff_opt_ws_error_highlight(const struct option *option,
+				       const char *arg, int unset)
 {
+	struct diff_options *opt = option->value;
 	int val = parse_ws_error_highlight(arg);
 
-	if (val < 0) {
-		error("unknown value after ws-error-highlight=%.*s",
-		      -1 - val, arg);
-		return 0;
-	}
+	BUG_ON_OPT_NEG(unset);
+	if (val < 0)
+		return error(_("unknown value after ws-error-highlight=%.*s"),
+			     -1 - val, arg);
 	opt->ws_error_highlight = val;
-	return 1;
+	return 0;
 }
 
 static int parse_objfind_opt(struct diff_options *opt, const char *arg)
@@ -5234,6 +5235,9 @@ static void prep_parse_options(struct diff_options *options)
 			 N_("show full pre- and post-image object names on the \"index\" lines")),
 		OPT_COLOR_FLAG(0, "color", &options->use_color,
 			       N_("show colored diff")),
+		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
+			       N_("highlight whitespace errors in the 'context', 'old' or 'new' lines in the diff"),
+			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5397,9 +5401,7 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;
 		options->color_moved_ws_handling = cm;
-	} else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
-		return parse_ws_error_highlight_opt(options, arg);
-	else if (!strcmp(arg, "--ita-invisible-in-index"))
+	} else if (!strcmp(arg, "--ita-invisible-in-index"))
 		options->ita_invisible_in_index = 1;
 	else if (!strcmp(arg, "--ita-visible-in-index"))
 		options->ita_invisible_in_index = 0;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 02/20] diff-parseopt: convert --ita-[in]visible-in-index

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Show changes to diff.c +7 −4

diff --git a/diff.c b/diff.c
index 7bfdf08137..8255db6966 100644
--- a/diff.c
+++ b/diff.c
@@ -5352,6 +5352,12 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("specify how differences in submodules are shown"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
 			       diff_opt_submodule),
+		OPT_SET_INT_F(0, "ita-invisible-in-index", &options->ita_invisible_in_index,
+			      N_("hide 'git add -N' entries from the index"),
+			      1, PARSE_OPT_NONEG),
+		OPT_SET_INT_F(0, "ita-visible-in-index", &options->ita_invisible_in_index,
+			      N_("treat 'git add -N' entries as real in the index"),
+			      0, PARSE_OPT_NONEG),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5401,10 +5407,7 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;
 		options->color_moved_ws_handling = cm;
-	} else if (!strcmp(arg, "--ita-invisible-in-index"))
-		options->ita_invisible_in_index = 1;
-	else if (!strcmp(arg, "--ita-visible-in-index"))
-		options->ita_invisible_in_index = 0;
+	}
 
 	/* misc options */
 	else if (!strcmp(arg, "-z"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 03/20] diff-parseopt: convert -z

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Show changes to diff.c +3 −2

diff --git a/diff.c b/diff.c
index 8255db6966..2b1d015553 100644
--- a/diff.c
+++ b/diff.c
@@ -5238,6 +5238,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "ws-error-highlight", options, N_("<kind>"),
 			       N_("highlight whitespace errors in the 'context', 'old' or 'new' lines in the diff"),
 			       PARSE_OPT_NONEG, diff_opt_ws_error_highlight),
+		OPT_SET_INT('z', NULL, &options->line_termination,
+			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
+			    0),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5410,8 +5413,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "-z"))
-		options->line_termination = 0;
 	else if ((argcount = short_opt('l', av, &optarg))) {
 		options->rename_limit = strtoul(optarg, NULL, 10);
 		return argcount;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 04/20] diff-parseopt: convert -l

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Show changes to diff.c +2 −4

diff --git a/diff.c b/diff.c
index 2b1d015553..6b19c97499 100644
--- a/diff.c
+++ b/diff.c
@@ -5283,6 +5283,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "follow", options, NULL,
 			       N_("continue listing the history of a file beyond renames"),
 			       PARSE_OPT_NOARG, diff_opt_follow),
+		OPT_INTEGER('l', NULL, &options->rename_limit,
+			    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,
@@ -5413,10 +5415,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('l', av, &optarg))) {
-		options->rename_limit = strtoul(optarg, NULL, 10);
-		return argcount;
-	}
 	else if ((argcount = short_opt('S', av, &optarg))) {
 		options->pickaxe = optarg;
 		options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 05/20] diff-parseopt: convert -S|-G

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

Show changes to diff.c +28 −9

diff --git a/diff.c b/diff.c
index 6b19c97499..8f01fbc2c1 100644
--- a/diff.c
+++ b/diff.c
@@ -5056,6 +5056,28 @@ static int diff_opt_patience(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_pickaxe_regex(const struct option *opt,
+				  const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->pickaxe = arg;
+	options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;
+	return 0;
+}
+
+static int diff_opt_pickaxe_string(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->pickaxe = arg;
+	options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;
+	return 0;
+}
+
 static int diff_opt_relative(const struct option *opt,
 			     const char *arg, int unset)
 {
@@ -5363,6 +5385,12 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_SET_INT_F(0, "ita-visible-in-index", &options->ita_invisible_in_index,
 			      N_("treat 'git add -N' entries as real in the index"),
 			      0, PARSE_OPT_NONEG),
+		OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
+			       N_("look for differences that change the number of occurrences of the specified string"),
+			       0, diff_opt_pickaxe_string),
+		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
+			       N_("look for differences that change the number of occurrences of the specified regex"),
+			       0, diff_opt_pickaxe_regex),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5415,15 +5443,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('S', av, &optarg))) {
-		options->pickaxe = optarg;
-		options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;
-		return argcount;
-	} else if ((argcount = short_opt('G', av, &optarg))) {
-		options->pickaxe = optarg;
-		options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;
-		return argcount;
-	}
 	else if (!strcmp(arg, "--pickaxe-all"))
 		options->pickaxe_opts |= DIFF_PICKAXE_ALL;
 	else if (!strcmp(arg, "--pickaxe-regex"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 06/20] diff-parseopt: convert --pickaxe-all|--pickaxe-regex

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Show changes to diff.c +6 −4

diff --git a/diff.c b/diff.c
index 8f01fbc2c1..699f8605d0 100644
--- a/diff.c
+++ b/diff.c
@@ -5391,6 +5391,12 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
 			       N_("look for differences that change the number of occurrences of the specified regex"),
 			       0, diff_opt_pickaxe_regex),
+		OPT_BIT_F(0, "pickaxe-all", &options->pickaxe_opts,
+			  N_("show all changes in the changeset with -S or -G"),
+			  DIFF_PICKAXE_ALL, PARSE_OPT_NONEG),
+		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
+			  N_("treat <string> in -S as extended POSIX regular expression"),
+			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5443,10 +5449,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--pickaxe-all"))
-		options->pickaxe_opts |= DIFF_PICKAXE_ALL;
-	else if (!strcmp(arg, "--pickaxe-regex"))
-		options->pickaxe_opts |= DIFF_PICKAXE_REGEX;
 	else if ((argcount = short_opt('O', av, &optarg))) {
 		options->orderfile = prefix_filename(prefix, optarg);
 		return argcount;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 07/20] diff-parseopt: convert -O

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

Show changes to diff.c +3 −20

diff --git a/diff.c b/diff.c
index 699f8605d0..849620aea5 100644
--- a/diff.c
+++ b/diff.c
@@ -4617,22 +4617,6 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
 	return 1;
 }
 
-static inline int short_opt(char opt, const char **argv,
-			    const char **optarg)
-{
-	const char *arg = argv[0];
-	if (arg[0] != '-' || arg[1] != opt)
-		return 0;
-	if (arg[2] != '\0') {
-		*optarg = arg + 2;
-		return 1;
-	}
-	if (!argv[1])
-		die("Option '%c' requires a value", opt);
-	*optarg = argv[1];
-	return 2;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
@@ -5397,6 +5381,8 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
 			  N_("treat <string> in -S as extended POSIX regular expression"),
 			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
+		OPT_FILENAME('O', NULL, &options->orderfile,
+			     N_("control the order in which files appear in the output")),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5449,10 +5435,7 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = short_opt('O', av, &optarg))) {
-		options->orderfile = prefix_filename(prefix, optarg);
-		return argcount;
-	} else if (skip_prefix(arg, "--find-object=", &arg))
+	else if (skip_prefix(arg, "--find-object=", &arg))
 		return parse_objfind_opt(options, arg);
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
 		int offending = parse_diff_filter_opt(optarg, options);

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 08/20] diff-parseopt: convert --find-object

While at it, mark one more string for translation.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Show changes to diff.c +9 −5

diff --git a/diff.c b/diff.c
index 849620aea5..99871d2546 100644
--- a/diff.c
+++ b/diff.c
@@ -4799,12 +4799,15 @@ static int diff_opt_ws_error_highlight(const struct option *option,
 	return 0;
 }
 
-static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+static int diff_opt_find_object(const struct option *option,
+				const char *arg, int unset)
 {
+	struct diff_options *opt = option->value;
 	struct object_id oid;
 
+	BUG_ON_OPT_NEG(unset);
 	if (get_oid(arg, &oid))
-		return error("unable to resolve '%s'", arg);
+		return error(_("unable to resolve '%s'"), arg);
 
 	if (!opt->objfind)
 		opt->objfind = xcalloc(1, sizeof(*opt->objfind));
@@ -4813,7 +4816,7 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)
 	opt->flags.recursive = 1;
 	opt->flags.tree_in_recursive = 1;
 	oidset_insert(opt->objfind, &oid);
-	return 1;
+	return 0;
 }
 
 static int diff_opt_anchored(const struct option *opt,
@@ -5383,6 +5386,9 @@ static void prep_parse_options(struct diff_options *options)
 			  DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
 		OPT_FILENAME('O', NULL, &options->orderfile,
 			     N_("control the order in which files appear in the output")),
+		OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
+			       N_("look for differences that change the number of occurrences of the specified object"),
+			       PARSE_OPT_NONEG, diff_opt_find_object),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5435,8 +5441,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (skip_prefix(arg, "--find-object=", &arg))
-		return parse_objfind_opt(options, arg);
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
 		int offending = parse_diff_filter_opt(optarg, options);
 		if (offending)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 09/20] diff-parseopt: convert --diff-filter

While at it, mark one more string for translation

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Show changes to diff.c +9 −9

diff --git a/diff.c b/diff.c
index 99871d2546..47146c33f5 100644
--- a/diff.c
+++ b/diff.c
@@ -4736,10 +4736,13 @@ static unsigned filter_bit_tst(char status, const struct diff_options *opt)
 	return opt->filter & filter_bit[(int) status];
 }
 
-static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
+static int diff_opt_diff_filter(const struct option *option,
+				const char *optarg, int unset)
 {
+	struct diff_options *opt = option->value;
 	int i, optch;
 
+	BUG_ON_OPT_NEG(unset);
 	prepare_filter_bits();
 
 	/*
@@ -4770,7 +4773,8 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 
 		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
 		if (!bit)
-			return optarg[i];
+			return error(_("unknown change class '%c' in --diff-filter=%s"),
+				     optarg[i], optarg);
 		if (negate)
 			opt->filter &= ~bit;
 		else
@@ -5389,6 +5393,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
 			       N_("look for differences that change the number of occurrences of the specified object"),
 			       PARSE_OPT_NONEG, diff_opt_find_object),
+		OPT_CALLBACK_F(0, "diff-filter", options, N_("[(A|C|D|M|R|T|U|X|B)...[*]]"),
+			       N_("select files by diff type"),
+			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
 		  N_("Output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
@@ -5441,13 +5448,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
-		int offending = parse_diff_filter_opt(optarg, options);
-		if (offending)
-			die("unknown change class '%c' in --diff-filter=%s",
-			    offending, optarg);
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-abbrev"))
 		options->abbrev = 0;
 	else if (!strcmp(arg, "--abbrev"))

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 10/20] diff-parseopt: convert --[no-]abbrev

OPT__ABBREV() has the same behavior as the deleted code with one difference: it does check for valid number and error out if not. And the '40' change is self explanatory.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c             | 12 +-----------
 parse-options-cb.c |  4 ++--
 2 files changed, 3 insertions(+), 13 deletions(-)

Show changes to 2 files +3 −13

diff.c, parse-options-cb.c

diff --git a/diff.c b/diff.c
index 47146c33f5..8d387280a4 100644
--- a/diff.c
+++ b/diff.c
@@ -5254,6 +5254,7 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_SET_INT('z', NULL, &options->line_termination,
 			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
 			    0),
+		OPT__ABBREV(&options->abbrev),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5448,17 +5449,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--no-abbrev"))
-		options->abbrev = 0;
-	else if (!strcmp(arg, "--abbrev"))
-		options->abbrev = DEFAULT_ABBREV;
-	else if (skip_prefix(arg, "--abbrev=", &arg)) {
-		options->abbrev = strtoul(arg, NULL, 10);
-		if (options->abbrev < MINIMUM_ABBREV)
-			options->abbrev = MINIMUM_ABBREV;
-		else if (the_hash_algo->hexsz < options->abbrev)
-			options->abbrev = the_hash_algo->hexsz;
-	}
 	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
 		options->a_prefix = optarg;
 		return argcount;
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 2733393546..6e2e8d6273 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -22,8 +22,8 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 				     opt->long_name);
 		if (v && v < MINIMUM_ABBREV)
 			v = MINIMUM_ABBREV;
-		else if (v > 40)
-			v = 40;
+		else if (v > the_hash_algo->hexsz)
+			v = the_hash_algo->hexsz;
 	}
 	*(int *)(opt->value) = v;
 	return 0;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 11/20] diff-parseopt: convert --[src|dst]-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c          | 14 ++++++--------
 parse-options.h |  3 ++-
 2 files changed, 8 insertions(+), 9 deletions(-)

Show changes to 2 files +8 −9

diff.c, parse-options.h

diff --git a/diff.c b/diff.c
index 8d387280a4..574fc511ff 100644
--- a/diff.c
+++ b/diff.c
@@ -5255,6 +5255,12 @@ static void prep_parse_options(struct diff_options *options)
 			    N_("do not munge pathnames and use NULs as output field terminators in --raw or --numstat"),
 			    0),
 		OPT__ABBREV(&options->abbrev),
+		OPT_STRING_F(0, "src-prefix", &options->a_prefix, N_("<prefix>"),
+			     N_("show the given source prefix instead of \"a/\""),
+			     PARSE_OPT_NONEG),
+		OPT_STRING_F(0, "dst-prefix", &options->b_prefix, N_("<prefix>"),
+			     N_("show the given source prefix instead of \"b/\""),
+			     PARSE_OPT_NONEG),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5449,20 +5455,12 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
-		options->a_prefix = optarg;
-		return argcount;
-	}
 	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
 		options->line_prefix = optarg;
 		options->line_prefix_length = strlen(options->line_prefix);
 		graph_setup_line_prefix(options);
 		return argcount;
 	}
-	else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
-		options->b_prefix = optarg;
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",
diff --git a/parse-options.h b/parse-options.h
index 7d83e2971d..c95cbe26f0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -136,6 +136,7 @@ struct option {
 #define OPT_BOOL_F(s, l, v, h, f)   OPT_SET_INT_F(s, l, v, h, 1, f)
 #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)			\
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
+#define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), (v), (a), (h), (f) }
 
 #define OPT_END()                   { OPTION_END }
 #define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
@@ -157,7 +158,7 @@ struct option {
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
-#define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
+#define OPT_STRING(s, l, v, a, h)   OPT_STRING_F(s, l, v, a, h, 0)
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \
 				      (h), 0, &parse_opt_string_list }

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 12/20] diff-parseopt: convert --line-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Show changes to diff.c +15 −8

diff --git a/diff.c b/diff.c
index 574fc511ff..e1675a32ec 100644
--- a/diff.c
+++ b/diff.c
@@ -5010,6 +5010,18 @@ static int diff_opt_ignore_submodules(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_line_prefix(const struct option *opt,
+				const char *optarg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	options->line_prefix = optarg;
+	options->line_prefix_length = strlen(options->line_prefix);
+	graph_setup_line_prefix(options);
+	return 0;
+}
+
 static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
 					     const struct option *opt,
 					     const char *arg, int unset)
@@ -5261,6 +5273,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_STRING_F(0, "dst-prefix", &options->b_prefix, N_("<prefix>"),
 			     N_("show the given source prefix instead of \"b/\""),
 			     PARSE_OPT_NONEG),
+		OPT_CALLBACK_F(0, "line-prefix", options, N_("<prefix>"),
+			       N_("prepend an additional prefix to every line of output"),
+			       PARSE_OPT_NONEG, diff_opt_line_prefix),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5418,8 +5433,6 @@ int diff_opt_parse(struct diff_options *options,
 		   const char **av, int ac, const char *prefix)
 {
 	const char *arg = av[0];
-	const char *optarg;
-	int argcount;
 
 	if (!prefix)
 		prefix = "";
@@ -5455,12 +5468,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
-		options->line_prefix = optarg;
-		options->line_prefix_length = strlen(options->line_prefix);
-		graph_setup_line_prefix(options);
-		return argcount;
-	}
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 13/20] diff-parseopt: convert --no-prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Show changes to diff.c +15 −2

diff --git a/diff.c b/diff.c
index e1675a32ec..6029f8de2d 100644
--- a/diff.c
+++ b/diff.c
@@ -5022,6 +5022,18 @@ static int diff_opt_line_prefix(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_no_prefix(const struct option *opt,
+			      const char *optarg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(optarg);
+	options->a_prefix = "";
+	options->b_prefix = "";
+	return 0;
+}
+
 static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
 					     const struct option *opt,
 					     const char *arg, int unset)
@@ -5276,6 +5288,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "line-prefix", options, N_("<prefix>"),
 			       N_("prepend an additional prefix to every line of output"),
 			       PARSE_OPT_NONEG, diff_opt_line_prefix),
+		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
+			       N_("do not show any source or destination prefix"),
+			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5468,8 +5483,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (!strcmp(arg, "--no-prefix"))
-		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",
 			 &options->interhunkcontext))
 		;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 14/20] diff-parseopt: convert --inter-hunk-context

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c          | 50 +++----------------------------------------------
 parse-options.h |  3 ++-
 2 files changed, 5 insertions(+), 48 deletions(-)

Show changes to 2 files +5 −48

diff.c, parse-options.h

diff --git a/diff.c b/diff.c
index 6029f8de2d..8b65b03c0d 100644
--- a/diff.c
+++ b/diff.c
@@ -4573,50 +4573,6 @@ void diff_setup_done(struct diff_options *options)
 	FREE_AND_NULL(options->parseopts);
 }
 
-static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
-{
-	char c, *eq;
-	int len;
-
-	if (*arg != '-')
-		return 0;
-	c = *++arg;
-	if (!c)
-		return 0;
-	if (c == arg_short) {
-		c = *++arg;
-		if (!c)
-			return 1;
-		if (val && isdigit(c)) {
-			char *end;
-			int n = strtoul(arg, &end, 10);
-			if (*end)
-				return 0;
-			*val = n;
-			return 1;
-		}
-		return 0;
-	}
-	if (c != '-')
-		return 0;
-	arg++;
-	eq = strchrnul(arg, '=');
-	len = eq - arg;
-	if (!len || strncmp(arg, arg_long, len))
-		return 0;
-	if (*eq) {
-		int n;
-		char *end;
-		if (!isdigit(*++eq))
-			return 0;
-		n = strtoul(eq, &end, 10);
-		if (*end)
-			return 0;
-		*val = n;
-	}
-	return 1;
-}
-
 int parse_long_opt(const char *opt, const char **argv,
 		   const char **optarg)
 {
@@ -5291,6 +5247,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
 			       N_("do not show any source or destination prefix"),
 			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
+		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
+			      N_("show context between diff hunks up to the specified number of lines"),
+			      PARSE_OPT_NONEG),
 		OPT_CALLBACK_F(0, "output-indicator-new",
 			       &options->output_indicators[OUTPUT_INDICATOR_NEW],
 			       N_("<char>"),
@@ -5483,9 +5442,6 @@ int diff_opt_parse(struct diff_options *options,
 	}
 
 	/* misc options */
-	else if (opt_arg(arg, '\0', "inter-hunk-context",
-			 &options->interhunkcontext))
-		;
 	else
 		return 0;
 	return 1;
diff --git a/parse-options.h b/parse-options.h
index c95cbe26f0..cca64d04cb 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -137,6 +137,7 @@ struct option {
 #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)			\
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
 #define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), (v), (a), (h), (f) }
+#define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h), (f) }
 
 #define OPT_END()                   { OPTION_END }
 #define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
@@ -155,7 +156,7 @@ struct option {
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
 #define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
-#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
+#define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
 #define OPT_STRING(s, l, v, a, h)   OPT_STRING_F(s, l, v, a, h, 0)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 15/20] diff-parseopt: convert --[no-]color-moved

Mark one more string for translation while at there

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

Show changes to diff.c +25 −13

diff --git a/diff.c b/diff.c
index 8b65b03c0d..de6c09e95a 100644
--- a/diff.c
+++ b/diff.c
@@ -4841,6 +4841,27 @@ static int diff_opt_char(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_color_moved(const struct option *opt,
+				const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+
+	if (unset) {
+		options->color_moved = COLOR_MOVED_NO;
+	} else if (!arg) {
+		if (diff_color_moved_default)
+			options->color_moved = diff_color_moved_default;
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
+	} else {
+		int cm = parse_color_moved(arg);
+		if (cm < 0)
+			return error(_("bad --color-moved argument: %s"), arg);
+		options->color_moved = cm;
+	}
+	return 0;
+}
+
 static int diff_opt_color_words(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -5339,6 +5360,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "color-words", options, N_("<regex>"),
 			       N_("equivalent to --word-diff=color --word-diff-regex=<regex>"),
 			       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_color_words),
+		OPT_CALLBACK_F(0, "color-moved", options, N_("<mode>"),
+			       N_("move lines of code are colored differently"),
+			       PARSE_OPT_OPTARG, diff_opt_color_moved),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),
@@ -5422,19 +5446,7 @@ int diff_opt_parse(struct diff_options *options,
 		return ac;
 
 	/* flags options */
-	if (!strcmp(arg, "--color-moved")) {
-		if (diff_color_moved_default)
-			options->color_moved = diff_color_moved_default;
-		if (options->color_moved == COLOR_MOVED_NO)
-			options->color_moved = COLOR_MOVED_DEFAULT;
-	} else if (!strcmp(arg, "--no-color-moved"))
-		options->color_moved = COLOR_MOVED_NO;
-	else if (skip_prefix(arg, "--color-moved=", &arg)) {
-		int cm = parse_color_moved(arg);
-		if (cm < 0)
-			return error("bad --color-moved argument: %s", arg);
-		options->color_moved = cm;
-	} else if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
+	if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
 		unsigned cm = parse_color_moved_ws(arg);
 		if (cm & COLOR_MOVED_WS_ERROR)
 			return -1;

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 16/20] diff-parseopt: convert --color-moved-ws

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Show changes to diff.c +18 −17

diff --git a/diff.c b/diff.c
index de6c09e95a..1a24bbfb69 100644
--- a/diff.c
+++ b/diff.c
@@ -4862,6 +4862,20 @@ static int diff_opt_color_moved(const struct option *opt,
 	return 0;
 }
 
+static int diff_opt_color_moved_ws(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct diff_options *options = opt->value;
+	unsigned cm;
+
+	BUG_ON_OPT_NEG(unset);
+	cm = parse_color_moved_ws(arg);
+	if (cm & COLOR_MOVED_WS_ERROR)
+		return error(_("invalid mode '%s' in --color-moved-ws"), arg);
+	options->color_moved_ws_handling = cm;
+	return 0;
+}
+
 static int diff_opt_color_words(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -5363,6 +5377,9 @@ static void prep_parse_options(struct diff_options *options)
 		OPT_CALLBACK_F(0, "color-moved", options, N_("<mode>"),
 			       N_("move lines of code are colored differently"),
 			       PARSE_OPT_OPTARG, diff_opt_color_moved),
+		OPT_CALLBACK_F(0, "color-moved-ws", options, N_("<mode>"),
+			       N_("how white spaces are ignored in --color-moved"),
+			       PARSE_OPT_NONEG, diff_opt_color_moved_ws),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),
@@ -5430,8 +5447,6 @@ static void prep_parse_options(struct diff_options *options)
 int diff_opt_parse(struct diff_options *options,
 		   const char **av, int ac, const char *prefix)
 {
-	const char *arg = av[0];
-
 	if (!prefix)
 		prefix = "";
 
@@ -5442,21 +5457,7 @@ int diff_opt_parse(struct diff_options *options,
 			   PARSE_OPT_ONE_SHOT |
 			   PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (ac)
-		return ac;
-
-	/* flags options */
-	if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
-		unsigned cm = parse_color_moved_ws(arg);
-		if (cm & COLOR_MOVED_WS_ERROR)
-			return -1;
-		options->color_moved_ws_handling = cm;
-	}
-
-	/* misc options */
-	else
-		return 0;
-	return 1;
+	return ac;
 }
 
 int parse_rename_score(const char **cp_p)

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 17/20] diff.c: allow --no-color-moved-ws

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Show changes to diff.c +6 −2

diff --git a/diff.c b/diff.c
index 1a24bbfb69..8b770cd396 100644
--- a/diff.c
+++ b/diff.c
@@ -4868,7 +4868,11 @@ static int diff_opt_color_moved_ws(const struct option *opt,
 	struct diff_options *options = opt->value;
 	unsigned cm;
 
-	BUG_ON_OPT_NEG(unset);
+	if (unset) {
+		options->color_moved_ws_handling = 0;
+		return 0;
+	}
+
 	cm = parse_color_moved_ws(arg);
 	if (cm & COLOR_MOVED_WS_ERROR)
 		return error(_("invalid mode '%s' in --color-moved-ws"), arg);
@@ -5379,7 +5383,7 @@ static void prep_parse_options(struct diff_options *options)
 			       PARSE_OPT_OPTARG, diff_opt_color_moved),
 		OPT_CALLBACK_F(0, "color-moved-ws", options, N_("<mode>"),
 			       N_("how white spaces are ignored in --color-moved"),
-			       PARSE_OPT_NONEG, diff_opt_color_moved_ws),
+			       0, diff_opt_color_moved_ws),
 
 		OPT_GROUP(N_("Diff other options")),
 		OPT_CALLBACK_F(0, "relative", options, N_("<prefix>"),

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 18/20] range-diff: use parse_options() instead of diff_opt_parse()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/range-diff.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

Show changes to builtin/range-diff.c +6 −20

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f01a0be851..784bd19321 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -16,42 +16,27 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
 	int simple_color = -1;
-	struct option options[] = {
+	struct option range_diff_options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
 		OPT_END()
 	};
-	int i, j, res = 0;
+	struct option *options;
+	int res = 0;
 	struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
 
 	git_config(git_diff_ui_config, NULL);
 
 	repo_diff_setup(the_repository, &diffopt);
 
+	options = parse_options_concat(range_diff_options, diffopt.parseopts);
 	argc = parse_options(argc, argv, NULL, options,
-			     builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
-			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
-
-	for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
-		int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+			     builtin_range_diff_usage, 0);
 
-		if (!c)
-			argv[j++] = argv[i++];
-		else
-			i += c;
-	}
-	while (i < argc)
-		argv[j++] = argv[i++];
-	argc = j;
 	diff_setup_done(&diffopt);
 
-	/* Make sure that there are no unparsed options */
-	argc = parse_options(argc, argv, NULL,
-			     options + ARRAY_SIZE(options) - 1, /* OPT_END */
-			     builtin_range_diff_usage, 0);
-
 	/* force color when --dual-color was used */
 	if (!simple_color)
 		diffopt.use_color = 1;
@@ -90,6 +75,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		error(_("need two commit ranges"));
 		usage_with_options(builtin_range_diff_usage, options);
 	}
+	FREE_AND_NULL(options);
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
 			      simple_color < 1, &diffopt);

-- 
2.21.0.548.gd3c7d92dc2

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

While at there, move exit() back to the caller. It's easier to see the flow that way than burying it in diff-no-index.c

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/diff.c           | 21 +++--------------
 diff-no-index.c          | 49 ++++++++++++++++++++++++----------------
 diff.h                   |  3 ++-
 t/t4053-diff-no-index.sh |  3 +--
 4 files changed, 35 insertions(+), 41 deletions(-)

Show changes to 4 files +35 −41

builtin/diff.c, diff-no-index.c, diff.h, t/t4053-diff-no-index.sh

diff --git a/builtin/diff.c b/builtin/diff.c
index f0393bba23..52dc3e136f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -320,26 +320,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	repo_init_revisions(the_repository, &rev, prefix);
 
-	if (no_index && argc != i + 2) {
-		if (no_index == DIFF_NO_INDEX_IMPLICIT) {
-			/*
-			 * There was no --no-index and there were not two
-			 * paths. It is possible that the user intended
-			 * to do an inside-repository operation.
-			 */
-			fprintf(stderr, "Not a git repository\n");
-			fprintf(stderr,
-				"To compare two paths outside a working tree:\n");
-		}
-		/* Give the usage message for non-repository usage and exit. */
-		usagef("git diff %s <path> <path>",
-		       no_index == DIFF_NO_INDEX_EXPLICIT ?
-		       "--no-index" : "[--no-index]");
-
-	}
 	if (no_index)
 		/* If this is a no-index diff, just run it and exit there. */
-		diff_no_index(the_repository, &rev, argc, argv);
+		exit(diff_no_index(the_repository, &rev,
+				   no_index == DIFF_NO_INDEX_IMPLICIT,
+				   argc, argv));
 
 	/* Otherwise, we are doing the usual "git" diff */
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
diff --git a/diff-no-index.c b/diff-no-index.c
index 9414e922d1..a879f45862 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -14,6 +14,7 @@
 #include "revision.h"
 #include "log-tree.h"
 #include "builtin.h"
+#include "parse-options.h"
 #include "string-list.h"
 #include "dir.h"
 
@@ -233,35 +234,43 @@ static void fixup_paths(const char **path, struct strbuf *replacement)
 	}
 }
 
-void diff_no_index(struct repository *r,
-		   struct rev_info *revs,
-		   int argc, const char **argv)
+static const char * const diff_no_index_usage[] = {
+	N_("git diff --no-index [<options>] <path> <path>"),
+	NULL
+};
+
+int diff_no_index(struct repository *r,
+		  struct rev_info *revs,
+		  int implicit_no_index,
+		  int argc, const char **argv)
 {
-	int i;
+	int i, no_index;
 	const char *paths[2];
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
+	struct option no_index_options[] = {
+		OPT_BOOL_F(0, "no-index", &no_index, "",
+			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
+		OPT_END(),
+	};
+	struct option *options;
 
 	/*
 	 * FIXME: --no-index should not look at index and we should be
 	 * able to pass NULL repo. Maybe later.
 	 */
 	repo_diff_setup(r, &revs->diffopt);
-	for (i = 1; i < argc - 2; ) {
-		int j;
-		if (!strcmp(argv[i], "--no-index"))
-			i++;
-		else if (!strcmp(argv[i], "--"))
-			i++;
-		else {
-			j = diff_opt_parse(&revs->diffopt, argv + i, argc - i,
-					   revs->prefix);
-			if (j <= 0)
-				die("invalid diff option/value: %s", argv[i]);
-			i += j;
-		}
+	options = parse_options_concat(no_index_options,
+				       revs->diffopt.parseopts);
+	argc = parse_options(argc, argv, revs->prefix, options,
+			     diff_no_index_usage, 0);
+	if (argc != 2) {
+		if (implicit_no_index)
+			warning(_("Not a git repository. Use --no-index to "
+				  "compare two paths outside a working tree"));
+		usage_with_options(diff_no_index_usage, options);
 	}
-
+	FREE_AND_NULL(options);
 	for (i = 0; i < 2; i++) {
 		const char *p = argv[argc - 2 + i];
 		if (!strcmp(p, "-"))
@@ -293,7 +302,7 @@ void diff_no_index(struct repository *r,
 	revs->diffopt.flags.exit_with_status = 1;
 
 	if (queue_diff(&revs->diffopt, paths[0], paths[1]))
-		exit(1);
+		return 1;
 	diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
@@ -304,5 +313,5 @@ void diff_no_index(struct repository *r,
 	 * The return code for --no-index imitates diff(1):
 	 * 0 = no changes, 1 = changes, else error
 	 */
-	exit(diff_result_code(&revs->diffopt, 0));
+	return diff_result_code(&revs->diffopt, 0);
 }
diff --git a/diff.h b/diff.h
index d9ad73f0e1..03c6afda22 100644
--- a/diff.h
+++ b/diff.h
@@ -437,7 +437,8 @@ int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 
 int diff_result_code(struct diff_options *, int);
 
-void diff_no_index(struct repository *, struct rev_info *, int, const char **);
+int diff_no_index(struct repository *, struct rev_info *,
+		  int implicit_no_index, int, const char **);
 
 int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 6e0dd6f9e5..fb25cdb789 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -50,8 +50,7 @@ test_expect_success 'git diff --no-index executed outside repo gives correct err
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git diff --no-index a 2>actual.err &&
-		echo "usage: git diff --no-index <path> <path>" >expect.err &&
-		test_cmp expect.err actual.err
+		test_i18ngrep "usage: git diff --no-index" actual.err
 	)
 '

-- 
2.21.0.548.gd3c7d92dc2

Johannes Schindelin· Apr 30, 2019, 01:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

Hi Duy,

On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:

> While at there, move exit() back to the caller. It's easier to see the
> flow that way than burying it in diff-no-index.c

I just noticed that this commit message is missing more than just a trailing period. It does not explain the change of behavior: previously, `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the external diff, it would have required an explicit `--ext-diff` to pick it up.

After this change, it is necessary to pass `--no-ext-diff` to reinstate the original behavior.

While this might seem like a desirable change, it is a backwards-incompatible change and would do with a *lot* more of a message to users whose scripts now potentially break.

Ciao, Johannes

Duy Nguyen· Apr 30, 2019, 04:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

On Tue, Apr 30, 2019 at 8:02 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted lines

>
> Hi Duy,
>
> On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:
>
> > While at there, move exit() back to the caller. It's easier to see the
> > flow that way than burying it in diff-no-index.c
>
> I just noticed that this commit message is missing more than just a
> trailing period. It does not explain the change of behavior: previously,
> `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the
> external diff, it would have required an explicit `--ext-diff` to pick it
> up.

Because I was not aware of the behavior change.

-- 
Duy

Johannes Schindelin· Apr 30, 2019, 22:12 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

Hi Duy,

On Tue, 30 Apr 2019, Duy Nguyen wrote:

Show 17 quoted lines

> On Tue, Apr 30, 2019 at 8:02 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > Hi Duy,
> >
> > On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:
> >
> > > While at there, move exit() back to the caller. It's easier to see the
> > > flow that way than burying it in diff-no-index.c
> >
> > I just noticed that this commit message is missing more than just a
> > trailing period. It does not explain the change of behavior: previously,
> > `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the
> > external diff, it would have required an explicit `--ext-diff` to pick it
> > up.
>
> Because I was not aware of the behavior change.

Well, your patch removes an early return in favor of a later return that allows plenty of diff options to be configured in a different way than before.

So while it is obvious (and understandable) that you were not aware of this behavior change, the real question is what we should do about this, now that this patch is already in `master` and on its way into v2.22.0.

Ciao, Johannes

Duy Nguyen· May 1, 2019, 09:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

On Wed, May 1, 2019 at 5:12 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:>

Show 25 quoted lines

> Hi Duy,
>
> On Tue, 30 Apr 2019, Duy Nguyen wrote:
>
> > On Tue, Apr 30, 2019 at 8:02 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > Hi Duy,
> > >
> > > On Sun, 24 Mar 2019, Nguyễn Thái Ngọc Duy wrote:
> > >
> > > > While at there, move exit() back to the caller. It's easier to see the
> > > > flow that way than burying it in diff-no-index.c
> > >
> > > I just noticed that this commit message is missing more than just a
> > > trailing period. It does not explain the change of behavior: previously,
> > > `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the
> > > external diff, it would have required an explicit `--ext-diff` to pick it
> > > up.
> >
> > Because I was not aware of the behavior change.
>
> Well, your patch removes an early return in favor of a later return that
> allows plenty of diff options to be configured in a different way than
> before.

No (and I was terse because I did not have time to look more into it). The code flow is the same, the number of option parsing is the same. Even post option processing is the same.

Bisecting points to 287ab28bfa (diff: reuse diff setup for --no-index case, 2019-02-16). From the description (i.e. "miss out some settings like --ext-diff...") the behavior change seems delibrate. Adding Jeff for clarification/

> So while it is obvious

I probably have problem understanding. The "commit message is missing" seems to imply I knew about this but chose not to mention it.

Show 6 quoted lines

> (and understandable) that you were not aware of
> this behavior change, the real question is what we should do about this,
> now that this patch is already in `master` and on its way into v2.22.0.
>
> Ciao,
> Johannes

-- 
Duy

Jeff King· May 1, 2019, 18:46 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 19/20] diff --no-index: use parse_options() instead of diff_opt_parse()

On Wed, May 01, 2019 at 04:53:44PM +0700, Duy Nguyen wrote:

Show 10 quoted lines

> > > > I just noticed that this commit message is missing more than just a
> > > > trailing period. It does not explain the change of behavior: previously,
> > > > `GIT_EXTERNAL_DIFF=heya git diff --no-index a b` would silently ignore the
> > > > external diff, it would have required an explicit `--ext-diff` to pick it
> > > > up.
> [...]
> Bisecting points to 287ab28bfa (diff: reuse diff setup for --no-index
> case, 2019-02-16). From the description (i.e. "miss out some settings
> like --ext-diff...") the behavior change seems delibrate. Adding Jeff
> for clarification/

Yeah, that was the purpose of that change: whether in no-index mode or not, git-diff should behave consistently.

-Peff

Nguyễn Thái Ngọc Duy· Mar 24, 2019, 08:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 20/20] am: avoid diff_opt_parse()

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/am.c | 4 ++--
 diff.c       | 6 ++++++
 diff.h       | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

Show changes to 3 files +10 −2

builtin/am.c, diff.c, diff.h

diff --git a/builtin/am.c b/builtin/am.c
index 95370313b6..0cbf285459 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1515,11 +1515,11 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 		 * review them with extra care to spot mismerges.
 		 */
 		struct rev_info rev_info;
-		const char *diff_filter_str = "--diff-filter=AM";
 
 		repo_init_revisions(the_repository, &rev_info, NULL);
 		rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
-		diff_opt_parse(&rev_info.diffopt, &diff_filter_str, 1, rev_info.prefix);
+		rev_info.diffopt.filter |= diff_filter_bit('A');
+		rev_info.diffopt.filter |= diff_filter_bit('M');
 		add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
 		diff_setup_done(&rev_info.diffopt);
 		run_diff_index(&rev_info, 1);
diff --git a/diff.c b/diff.c
index 8b770cd396..11f26285c3 100644
--- a/diff.c
+++ b/diff.c
@@ -4692,6 +4692,12 @@ static unsigned filter_bit_tst(char status, const struct diff_options *opt)
 	return opt->filter & filter_bit[(int) status];
 }
 
+unsigned diff_filter_bit(char status)
+{
+	prepare_filter_bits();
+	return filter_bit[(int) status];
+}
+
 static int diff_opt_diff_filter(const struct option *option,
 				const char *optarg, int unset)
 {
diff --git a/diff.h b/diff.h
index 03c6afda22..f88482705c 100644
--- a/diff.h
+++ b/diff.h
@@ -233,6 +233,8 @@ struct diff_options {
 	struct option *parseopts;
 };
 
+unsigned diff_filter_bit(char status);
+
 void diff_emit_submodule_del(struct diff_options *o, const char *line);
 void diff_emit_submodule_add(struct diff_options *o, const char *line);
 void diff_emit_submodule_untracked(struct diff_options *o, const char *path);

-- 
2.21.0.548.gd3c7d92dc2