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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Martin Ågren]

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Martin Ågren]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Ævar Arnfjörð Bjarmason]

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.

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.

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

[Duy Nguyen]

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

> 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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Martin Ågren]

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/

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

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

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

[Duy Nguyen]

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.
-- 
Duy

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

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.

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

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

[Duy Nguyen]

On Wed, May 1, 2019 at 5:12 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:>
> 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.

> (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

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

[Jeff King]

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

> > > > 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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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