Convert diff opt parser to parse_options()

PATCH In review Nguyễn Thái Ngọc Duy 3 participants 18 messages Dec 27, 2018 – Jan 9, 2019

Share & tools

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #0 lore raw

cover Convert diff opt parser to parse_options()

This is a sneak peek of converting diff_opt_parse() to 'struct option'. The end game is builtin commands can just add diff options to their 'struct option[]' array, do parse_options() like usual and have diff parsing for free.

Besides simplifying the parsing code, there are also more benefits of using struct option, e.g. tab completion and "git cmd -h".

I'm not sending full series because it's looong, the middle is not that interesting, and I would need to wait for sb/diff-color-moved-config-option-fixup (and Brian's --literally too, probably) to land before sending something mergeable. But main patches are sent here for review.

This is of course just the beginning. Once revision.c parser is converted, diff_opt_parse() will be killed off, more commands can take use diff option array directly. And perhaps the diff option array will be split up into smaller ones to give callers more control what's part of the command.

Nguyễn Thái Ngọc Duy (75):
parse-options.h: remove extern on function prototypes
parse-options: add one-shot mode
parse-options: allow keep-unknown + stop-at-non-opt combination
parse-options: disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
parse-options: add OPT_BITOP()
parse-options: stop abusing 'callback' for lowlevel callbacks
parse-options: avoid magic return codes
parse-options: allow ll_callback with OPTION_CALLBACK diff.h: keep forward struct declarations sorted
diff.h: avoid bit fields in struct diff_flags
diff.c: prepare to use parse_options() for parsing

  diff.c: convert -u|-p|--patch
  diff.c: convert -U|--unified
  diff.c: convert -W|--[no-]function-context
  diff.c: convert --raw
  diff.c: convert --patch-with-raw
  diff.c: convert --numstat and --shortstat
  diff.c: convert --dirstat and friends
  diff.c: convert --check
  diff.c: convert --summary
  diff.c: convert --patch-with-stat
  diff.c: convert --name-only
  diff.c: convert --name-status
  diff.c: convert -s|--no-patch
  diff.c: convert --stat*
  diff.c: convert --[no-]compact-summary
  diff.c: convert --output-*
  diff.c: convert -B|--break-rewrites
  diff.c: convert -M|--find-renames
  diff.c: convert -D|--irreversible-delete
  diff.c: convert -C|--find-copies

diff.c: convert --find-copies-harder
diff.c: convert --no-renames|--[no--rename-empty diff.c: convert --relative
diff.c: convert --[no-]minimal
diff.c: convert --ignore-some-changes
diff.c: convert --[no-]indent-heuristic
diff.c: convert --patience
diff.c: convert --histogram
diff.c: convert --diff-algorithm
diff.c: convert --anchored
diff.c: convert --binary
diff.c: convert --full-index
diff.c: convert -a|--text
diff.c: convert -R
diff.c: convert --[no-]follow
diff.c: convert --[no-]color
diff.c: convert --word-diff
diff.c: convert --word-diff-regex
diff.c: convert --color-words
diff.c: convert --exit-code
diff.c: convert --quiet
diff.c: convert --ext-diff
diff.c: convert --textconv
diff.c: convert --ignore-submodules
diff.c: convert --submodule
diff.c: convert --ws-error-highlight
diff.c: convert --ita-[in]visible-in-index
diff.c: convert -z
diff.c: convert -l
diff.c: convert -S|-G
diff.c: convert --pickaxe-all|--pickaxe-regex
diff.c: convert -O
diff.c: convert --find-object
diff.c: convert --diff-filter
diff.c: convert --[no-]abbrev
diff.c: convert --[src|dst]-prefix
diff.c: convert --line-prefix
diff.c: convert --no-prefix
diff.c: convert --inter-hunk-context
diff.c: convert --color-moved
diff.c: convert --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()

 Documentation/diff-options.txt |   24 +-
 builtin/am.c                   |    4 +-
 builtin/blame.c                |    2 +-
 builtin/diff.c                 |   21 +-
 builtin/merge.c                |    9 +-
 builtin/range-diff.c           |   26 +-
 builtin/update-index.c         |   41 +-
 diff-no-index.c                |   49 +-
 diff.c                         | 1132 ++++++++++++++++++++------------
 diff.h                         |   85 +--
 parse-options-cb.c             |   11 +-
 parse-options.c                |  152 +++--
 parse-options.h                |  116 ++--
 t/t4053-diff-no-index.sh       |    3 +-
 t/t7800-difftool.sh            |    4 +-
 15 files changed, 1051 insertions(+), 628 deletions(-)

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #1 lore raw

PATCH 1/75 parse-options.h: remove extern on function prototypes

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 parse-options.h | 60 ++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

parse-options.h +30 ~~-30~~

index a650a7d220..1947cb27cf 100644
@@ -174,19 +174,19 @@ struct option {
  * for translation with N_().
  * Returns the number of arguments left in argv[].
  */
-extern int parse_options(int argc, const char **argv, const char *prefix,
-                         const struct option *options,
-                         const char * const usagestr[], int flags);
+int parse_options(int argc, const char **argv, const char *prefix,
+		  const struct option *options,
+		  const char * const usagestr[], int flags);
 
-extern NORETURN void usage_with_options(const char * const *usagestr,
-                                        const struct option *options);
+NORETURN void usage_with_options(const char * const *usagestr,
+				 const struct option *options);
 
-extern NORETURN void usage_msg_opt(const char *msg,
-				   const char * const *usagestr,
-				   const struct option *options);
+NORETURN void usage_msg_opt(const char *msg,
+			    const char * const *usagestr,
+			    const struct option *options);
 
-extern int optbug(const struct option *opt, const char *reason);
-extern int opterror(const struct option *opt, const char *reason, int flags);
+int optbug(const struct option *opt, const char *reason);
+int opterror(const struct option *opt, const char *reason, int flags);
 #if defined(__GNUC__)
 #define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
 #endif

@@ -230,31 +230,31 @@ struct parse_opt_ctx_t {
 	const char *prefix;
 };
 
-extern void parse_options_start(struct parse_opt_ctx_t *ctx,
-				int argc, const char **argv, const char *prefix,
-				const struct option *options, int flags);
+void parse_options_start(struct parse_opt_ctx_t *ctx,
+			 int argc, const char **argv, const char *prefix,
+			 const struct option *options, int flags);
 
-extern int parse_options_step(struct parse_opt_ctx_t *ctx,
-			      const struct option *options,
-			      const char * const usagestr[]);
+int parse_options_step(struct parse_opt_ctx_t *ctx,
+		       const struct option *options,
+		       const char * const usagestr[]);
 
-extern int parse_options_end(struct parse_opt_ctx_t *ctx);
+int parse_options_end(struct parse_opt_ctx_t *ctx);
 
-extern struct option *parse_options_concat(struct option *a, struct option *b);
+struct option *parse_options_concat(struct option *a, struct option *b);
 
 /*----- some often used options -----*/
-extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
-extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
-extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
-extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
-extern int parse_opt_object_name(const struct option *, const char *, int);
-extern int parse_opt_commits(const struct option *, const char *, int);
-extern int parse_opt_tertiary(const struct option *, const char *, int);
-extern int parse_opt_string_list(const struct option *, const char *, int);
-extern int parse_opt_noop_cb(const struct option *, const char *, int);
-extern int parse_opt_unknown_cb(const struct option *, const char *, int);
-extern int parse_opt_passthru(const struct option *, const char *, int);
-extern int parse_opt_passthru_argv(const struct option *, const char *, int);
+int parse_opt_abbrev_cb(const struct option *, const char *, int);
+int parse_opt_expiry_date_cb(const struct option *, const char *, int);
+int parse_opt_color_flag_cb(const struct option *, const char *, int);
+int parse_opt_verbosity_cb(const struct option *, const char *, int);
+int parse_opt_object_name(const struct option *, const char *, int);
+int parse_opt_commits(const struct option *, const char *, int);
+int parse_opt_tertiary(const struct option *, const char *, int);
+int parse_opt_string_list(const struct option *, const char *, int);
+int parse_opt_noop_cb(const struct option *, const char *, int);
+int parse_opt_unknown_cb(const struct option *, const char *, int);
+int parse_opt_passthru(const struct option *, const char *, int);
+int parse_opt_passthru_argv(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #2 lore raw

PATCH 2/75 parse-options: add one-shot mode

In reply to Nguyễn Thái Ngọc Duy

This is to help reimplement diff_opt_parse() using parse_options(). The behavior of parse_options() is changed to be the same as the other:

- no argv0 in argv[], everything can be processed
- argv[] must not be updated, it's the caller's job to do that
- return the number of arguments processed
- leave all unknown options / non-options alone (this one can already be achieved with PARSE_OPT_KEEP_UNKNOWN and PARSE_OPT_STOP_AT_NON_OPTION)

This mode is NOT supposed to stay here for long. It's to help converting diff/rev option parsing. Once that work is over and we can just use parse_options() throughout the code base, this will be deleted.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 parse-options.c | 23 ++++++++++++++++++++---
 parse-options.h | 17 +++++++++++++----
 2 files changed, 33 insertions(+), 7 deletions(-)

parse-options.c +20 -3

index 6932eaff61..d47e217b07 100644
@@ -415,15 +415,23 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 const struct option *options, int flags)
 {
 	memset(ctx, 0, sizeof(*ctx));
-	ctx->argc = ctx->total = argc - 1;
-	ctx->argv = argv + 1;
-	ctx->out  = argv;
+	ctx->argc = argc;
+	ctx->argv = argv;
+	if (!(flags & PARSE_OPT_ONE_SHOT)) {
+		ctx->argc--;
+		ctx->argv++;
+	}
+	ctx->total = ctx->argc;
+	ctx->out   = argv;
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
 		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+	if ((flags & PARSE_OPT_ONE_SHOT) &&
+	    (flags & PARSE_OPT_KEEP_ARGV0))
+		BUG("Can't keep argv0 if you don't have it");
 	parse_options_check(options);
 }

@@ -535,6 +543,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 	for (; ctx->argc; ctx->argc--, ctx->argv++) {
 		const char *arg = ctx->argv[0];
 
+		if (ctx->flags & PARSE_OPT_ONE_SHOT &&
+		    ctx->argc != ctx->total)
+			break;
+
 		if (*arg != '-' || !arg[1]) {
 			if (parse_nodash_opt(ctx, arg, options) == 0)
 				continue;

@@ -609,6 +621,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 		continue;
 unknown:
+		if (ctx->flags & PARSE_OPT_ONE_SHOT)
+			break;
 		if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN))
 			return PARSE_OPT_UNKNOWN;
 		ctx->out[ctx->cpidx++] = ctx->argv[0];

@@ -622,6 +636,9 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
+	if (ctx->flags & PARSE_OPT_ONE_SHOT)
+		return ctx->total - ctx->argc;
+
 	MOVE_ARRAY(ctx->out + ctx->cpidx, ctx->argv, ctx->argc);
 	ctx->out[ctx->cpidx + ctx->argc] = NULL;
 	return ctx->cpidx + ctx->argc;

parse-options.h +13 -4

index 1947cb27cf..043d296ea4 100644
@@ -27,7 +27,8 @@ enum parse_opt_flags {
 	PARSE_OPT_STOP_AT_NON_OPTION = 2,
 	PARSE_OPT_KEEP_ARGV0 = 4,
 	PARSE_OPT_KEEP_UNKNOWN = 8,
-	PARSE_OPT_NO_INTERNAL_HELP = 16
+	PARSE_OPT_NO_INTERNAL_HELP = 16,
+	PARSE_OPT_ONE_SHOT = 32
 };
 
 enum parse_opt_option_flags {

@@ -169,10 +170,18 @@ struct option {
 	  N_("no-op (backward compatibility)"),		\
 	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
-/* parse_options() will filter out the processed options and leave the
- * non-option arguments in argv[]. usagestr strings should be marked
- * for translation with N_().
+/*
+ * parse_options() will filter out the processed options and leave the
+ * non-option arguments in argv[]. argv0 is assumed program name and
+ * skipped.
+ *
+ * usagestr strings should be marked for translation with N_().
+ *
  * Returns the number of arguments left in argv[].
+ *
+ * In one-shot mode, argv0 is not a program name, argv[] is left
+ * untouched and parse_options() returns the number of options
+ * processed.
  */
 int parse_options(int argc, const char **argv, const char *prefix,
 		  const struct option *options,

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #3 lore raw

PATCH 3/75 parse-options: allow keep-unknown + stop-at-non-opt combination

In reply to Nguyễn Thái Ngọc Duy

These two are orthogonal. One is about unknown _option_ while the other non-option.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 parse-options.c | 3 ---
 1 file changed, 3 deletions(-)

parse-options.c +0 -3

index d47e217b07..4cbcefc262 100644
@@ -426,9 +426,6 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
-	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
-	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
-		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #4 lore raw

PATCH 4/75 parse-options: disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN

In reply to Nguyễn Thái Ngọc Duy

parse-options can unambiguously find an abbreviation only if it sees all available options. This is usually the case when you use parse_options(). But there are other callers like blame or shortlog which uses parse_options_start() in combination with a custom option parser, like rev-list. parse-options cannot see all options in this case and will get abbrev detection wrong. Disable it.

t7800 needs update because --symlink no longer expands to --symlinks and will be passed down to git-diff, which will not recognize it. I still think this is the correct thing to do. But if --symlink has been actually used in the wild, we would just add an option alias for it.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 parse-options.c     | 3 ++-
 t/t7800-difftool.sh | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

parse-options.c +2 -1

index 4cbcefc262..81e66b9374 100644
@@ -265,7 +265,8 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
 		}
 		if (!rest) {
 			/* abbreviated? */
-			if (!strncmp(long_name, arg, arg_end - arg)) {
+			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
+			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*

t/t7800-difftool.sh +2 -2

index 22b9199d59..bb9a7f4ff9 100755
@@ -546,7 +546,7 @@ do
 done >actual
 EOF
 
-test_expect_success SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+test_expect_success SYMLINKS 'difftool --dir-diff --symlinks without unstaged changes' '
 	cat >expect <<-EOF &&
 	file
 	$PWD/file

@@ -555,7 +555,7 @@ test_expect_success SYMLINKS 'difftool --dir-diff --symlink without unstaged cha
 	sub/sub
 	$PWD/sub/sub
 	EOF
-	git difftool --dir-diff --symlink \
+	git difftool --dir-diff --symlinks \
 		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
 	test_cmp expect actual
 '

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:25 UTC #5 lore raw

PATCH 5/75 parse-options: add OPT_BITOP()

In reply to Nguyễn Thái Ngọc Duy

This is needed for diff_opt_parse() where we do

   value = (value & ~mask) | some_more;

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 parse-options.c | 7 +++++++
 parse-options.h | 5 +++++
 2 files changed, 12 insertions(+)

parse-options.c +7 -0

index 81e66b9374..9e19d64cc9 100644
@@ -110,6 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
 			*(int *)opt->value &= ~opt->defval;
 		return 0;
 
+	case OPTION_BITOP:
+		if (unset)
+			BUG("BITOP can't have unset form");
+		*(int *)opt->value &= ~opt->extra;
+		*(int *)opt->value |= opt->defval;
+		return 0;
+
 	case OPTION_COUNTUP:
 		if (*(int *)opt->value < 0)
 			*(int *)opt->value = 0;

parse-options.h +5 -0

index 043d296ea4..69afd0bce6 100644
@@ -10,6 +10,7 @@ enum parse_opt_type {
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,
+	OPTION_BITOP,
 	OPTION_COUNTUP,
 	OPTION_SET_INT,
 	OPTION_CMDMODE,

@@ -118,6 +119,7 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+	intptr_t extra;
 };
 
 #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \

@@ -133,6 +135,9 @@ struct option {
 				      (h), PARSE_OPT_NOARG}
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)
+#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \
+					    PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, \
+					    (set), (clear) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     OPT_COUNTUP_F(s, l, v, h, 0)

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #6 lore raw

PATCH 6/75 parse-options: stop abusing 'callback' for lowlevel callbacks

In reply to Nguyễn Thái Ngọc Duy

Lowlevel callbacks have different function signatures. Add a new field in 'struct option' with the right type for lowlevel callbacks.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/blame.c        |  2 +-
 builtin/merge.c        |  2 +-
 builtin/update-index.c | 11 ++++++-----
 parse-options-cb.c     |  3 ++-
 parse-options.c        | 15 ++++++++++++++-
 parse-options.h        | 12 ++++++++----
 6 files changed, 32 insertions(+), 13 deletions(-)

builtin/blame.c +1 -1

index 6d798f9939..8dcc55dffa 100644
@@ -814,7 +814,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 * and are only included here to get included in the "-h"
 		 * output:
 		 */
-		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
+		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental heuristic to improve diffs"), PARSE_OPT_NOARG, NULL, 0, parse_opt_unknown_cb },
 
 		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
 		OPT_STRING('S', NULL, &revs_file, N_("file"), N_("Use revisions from <file> instead of calling git-rev-list")),

builtin/merge.c +1 -1

index c3c976d471..0ad0023f97 100644
@@ -261,7 +261,7 @@ static struct option builtin_merge_options[] = {
 		option_parse_message),
 	{ OPTION_LOWLEVEL_CALLBACK, 'F', "file", &merge_msg, N_("path"),
 		N_("read message from file"), PARSE_OPT_NONEG,
-		(parse_opt_cb *) option_read_message },
+		NULL, 0, option_read_message },
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),

builtin/update-index.c +6 -5

index e19da77edc..727a8118b8 100644
@@ -985,7 +985,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("add the specified entry to the index"),
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
-			(parse_opt_cb *) cacheinfo_callback},
+			NULL, 0,
+			cacheinfo_callback},
 		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
 			N_("override the executable bit of the listed files"),
 			PARSE_OPT_NONEG,

@@ -1011,19 +1012,19 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,
 			N_("read list of paths to be updated from standard input"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
-			(parse_opt_cb *) stdin_callback},
+			NULL, 0, stdin_callback},
 		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &nul_term_line, NULL,
 			N_("add entries from standard input to the index"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
-			(parse_opt_cb *) stdin_cacheinfo_callback},
+			NULL, 0, stdin_cacheinfo_callback},
 		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &has_errors, NULL,
 			N_("repopulate stages #2 and #3 for the listed paths"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
-			(parse_opt_cb *) unresolve_callback},
+			NULL, 0, unresolve_callback},
 		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &has_errors, NULL,
 			N_("only update entries that differ from HEAD"),
 			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
-			(parse_opt_cb *) reupdate_callback},
+			NULL, 0, reupdate_callback},
 		OPT_BIT(0, "ignore-missing", &refresh_args.flags,
 			N_("ignore files missing from worktree"),
 			REFRESH_IGNORE_MISSING),

parse-options-cb.c +2 -1

index 8c9edce52f..8d6a44a29d 100644
@@ -169,7 +169,8 @@ int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
  * "-h" output even if it's not being handled directly by
  * parse_options().
  */
-int parse_opt_unknown_cb(const struct option *opt, const char *arg, int unset)
+int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
+			 const struct option *opt, int unset)
 {
 	return -2;
 }

parse-options.c +14 -1

index 9e19d64cc9..5a717f219f 100644
@@ -94,7 +94,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 
 	switch (opt->type) {
 	case OPTION_LOWLEVEL_CALLBACK:
-		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);
+		return opt->ll_callback(p, opt, unset);
 
 	case OPTION_BIT:
 		if (unset)

@@ -407,6 +407,19 @@ static void parse_options_check(const struct option *opts)
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
 				err |= optbug(opts, "should not accept an argument");
+			break;
+		case OPTION_CALLBACK:
+			if (!opts->callback)
+				BUG("OPTION_CALLBACK needs a callback");
+			if (opts->ll_callback)
+				BUG("OPTION_CALLBACK needs no ll_callback");
+			break;
+		case OPTION_LOWLEVEL_CALLBACK:
+			if (!opts->ll_callback)
+				BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
+			if (opts->callback)
+				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+			break;
 		default:
 			; /* ok. (usually accepts an argument) */
 		}

parse-options.h +8 -4

index 69afd0bce6..277a879c1c 100644
@@ -100,13 +100,16 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *			 the option takes optional argument.
  *
  * `callback`::
- *   pointer to the callback to use for OPTION_CALLBACK or
- *   OPTION_LOWLEVEL_CALLBACK.
+ *   pointer to the callback to use for OPTION_CALLBACK
  *
  * `defval`::
  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
  *   OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
  *   CALLBACKS can use it like they want.
+ *
+ * `ll_callback`::
+ *   pointer to the callback to use for OPTION_LOWLEVEL_CALLBACK
+ *
  */
 struct option {
 	enum parse_opt_type type;

@@ -119,6 +122,7 @@ struct option {
 	int flags;
 	parse_opt_cb *callback;
 	intptr_t defval;
+	parse_opt_ll_cb *ll_callback;
 	intptr_t extra;
 };

@@ -137,7 +141,7 @@ struct option {
 #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)
 #define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \
 					    PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, \
-					    (set), (clear) }
+					    (set), NULL, (clear) }
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     OPT_COUNTUP_F(s, l, v, h, 0)

@@ -266,7 +270,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
-int parse_opt_unknown_cb(const struct option *, const char *, int);
+int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, int);
 int parse_opt_passthru(const struct option *, const char *, int);
 int parse_opt_passthru_argv(const struct option *, const char *, int);

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #7 lore raw

PATCH 7/75 parse-options: avoid magic return codes

In reply to Nguyễn Thái Ngọc Duy

Give names to these magic negative numbers. Make parse_opt_ll_cb return an enum to make clear it can actually control parse_options() with different return values (parse_opt_cb can too, but nobody needs it).

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/merge.c        |  5 +--
 builtin/update-index.c | 20 ++++++------
 parse-options-cb.c     |  6 ++--
 parse-options.c        | 69 +++++++++++++++++++++++++++---------------
 parse-options.h        | 14 ++++-----
 5 files changed, 68 insertions(+), 46 deletions(-)

builtin/merge.c +3 -2

index 0ad0023f97..1c8652e455 100644
@@ -112,8 +112,9 @@ static int option_parse_message(const struct option *opt,
 	return 0;
 }
 
-static int option_read_message(struct parse_opt_ctx_t *ctx,
-			       const struct option *opt, int unset)
+static enum parse_opt_result option_read_message(struct parse_opt_ctx_t *ctx,
+						 const struct option *opt,
+						 int unset)
 {
 	struct strbuf *buf = opt->value;
 	const char *arg;

builtin/update-index.c +10 ~~-10~~

index 727a8118b8..21c84e5590 100644
@@ -847,8 +847,8 @@ static int parse_new_style_cacheinfo(const char *arg,
 	return 0;
 }
 
-static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
-				const struct option *opt, int unset)
+static enum parse_opt_result cacheinfo_callback(
+	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
 {
 	struct object_id oid;
 	unsigned int mode;

@@ -873,8 +873,8 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
 	return 0;
 }
 
-static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,
-			      const struct option *opt, int unset)
+static enum parse_opt_result stdin_cacheinfo_callback(
+	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
 {
 	int *nul_term_line = opt->value;

@@ -887,8 +887,8 @@ static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,
 	return 0;
 }
 
-static int stdin_callback(struct parse_opt_ctx_t *ctx,
-				const struct option *opt, int unset)
+static enum parse_opt_result stdin_callback(
+	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
 {
 	int *read_from_stdin = opt->value;

@@ -900,8 +900,8 @@ static int stdin_callback(struct parse_opt_ctx_t *ctx,
 	return 0;
 }
 
-static int unresolve_callback(struct parse_opt_ctx_t *ctx,
-				const struct option *opt, int unset)
+static enum parse_opt_result unresolve_callback(
+	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;

@@ -919,8 +919,8 @@ static int unresolve_callback(struct parse_opt_ctx_t *ctx,
 	return 0;
 }
 
-static int reupdate_callback(struct parse_opt_ctx_t *ctx,
-				const struct option *opt, int unset)
+static enum parse_opt_result reupdate_callback(
+	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;

parse-options-cb.c +3 -3

index 8d6a44a29d..720a8663eb 100644
@@ -169,10 +169,10 @@ int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
  * "-h" output even if it's not being handled directly by
  * parse_options().
  */
-int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
-			 const struct option *opt, int unset)
+enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
+					   const struct option *opt, int unset)
 {
-	return -2;
+	return PARSE_OPT_UNKNOWN;
 }
 
 /**

parse-options.c +45 ~~-24~~

153 lines not on this page · raw message

parse-options.h +7 -7

index 277a879c1c..617bca8efe 100644
@@ -49,8 +49,8 @@ struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
 struct parse_opt_ctx_t;
-typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
-				const struct option *opt, int unset);
+typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
+					      const struct option *opt, int unset);
 
 /*
  * `type`::

@@ -225,12 +225,12 @@ int opterror(const struct option *opt, const char *reason, int flags);
 
 /*----- incremental advanced APIs -----*/
 
-enum {
-	PARSE_OPT_COMPLETE = -2,
-	PARSE_OPT_HELP = -1,
-	PARSE_OPT_DONE,
+enum parse_opt_result {
+	PARSE_OPT_COMPLETE = -3,
+	PARSE_OPT_HELP = -2,
+	PARSE_OPT_ERROR = -1,	/* must be the same as error() */
+	PARSE_OPT_DONE = 0,	/* fixed so that "return 0" works */
 	PARSE_OPT_NON_OPTION,
-	PARSE_OPT_ERROR,
 	PARSE_OPT_UNKNOWN
 };

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #8 lore raw

PATCH 8/75 parse-options: allow ll_callback with OPTION_CALLBACK

In reply to Nguyễn Thái Ngọc Duy

OPTION_CALLBACK is much simpler/safer to use, but parse_opt_cb does not allow access to parse_opt_ctx_t, which sometimes is useful (e.g. to obtain the prefix).

Extending parse_opt_cb to take parse_opt_cb could result in a lot of changes. Instead let's just allow ll_callback to be used with OPTION_CALLBACK. The user will have to be careful, not to change anything in ctx, or return wrong result code. But that's the price for ll_callback.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 builtin/merge.c        |  2 ++
 builtin/update-index.c | 20 +++++++++++++++-----
 parse-options-cb.c     |  4 +++-
 parse-options.c        | 42 ++++++++++++++++++++++++++++--------------
 parse-options.h        |  5 +++--
 5 files changed, 51 insertions(+), 22 deletions(-)

builtin/merge.c +2 -0

index 1c8652e455..b4f4b401eb 100644
@@ -114,11 +114,13 @@ static int option_parse_message(const struct option *opt,
 
 static enum parse_opt_result option_read_message(struct parse_opt_ctx_t *ctx,
 						 const struct option *opt,
+						 const char *arg_not_used,
 						 int unset)
 {
 	struct strbuf *buf = opt->value;
 	const char *arg;
 
+	BUG_ON_OPT_ARG(arg_not_used);
 	if (unset)
 		BUG("-F cannot be negated");

builtin/update-index.c +15 -5

index 21c84e5590..7abde20169 100644
@@ -848,13 +848,15 @@ static int parse_new_style_cacheinfo(const char *arg,
 }
 
 static enum parse_opt_result cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	struct object_id oid;
 	unsigned int mode;
 	const char *path;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, &oid, &path)) {
 		if (add_cacheinfo(mode, &oid, path, 0))

@@ -874,11 +876,13 @@ static enum parse_opt_result cacheinfo_callback(
 }
 
 static enum parse_opt_result stdin_cacheinfo_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *nul_term_line = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);

@@ -888,11 +892,13 @@ static enum parse_opt_result stdin_cacheinfo_callback(
 }
 
 static enum parse_opt_result stdin_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *read_from_stdin = opt->value;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	if (ctx->argc != 1)
 		return error("option '%s' must be the last argument", opt->long_name);

@@ -901,12 +907,14 @@ static enum parse_opt_result stdin_callback(
 }
 
 static enum parse_opt_result unresolve_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
 	*has_errors = do_unresolve(ctx->argc, ctx->argv,

@@ -920,12 +928,14 @@ static enum parse_opt_result unresolve_callback(
 }
 
 static enum parse_opt_result reupdate_callback(
-	struct parse_opt_ctx_t *ctx, const struct option *opt, int unset)
+	struct parse_opt_ctx_t *ctx, const struct option *opt,
+	const char *arg, int unset)
 {
 	int *has_errors = opt->value;
 	const char *prefix = startup_info->prefix;
 
 	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
 
 	/* consume remaining arguments. */
 	setup_work_tree();

parse-options-cb.c +3 -1

index 720a8663eb..b336b4fa97 100644
@@ -170,8 +170,10 @@ int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
  * parse_options().
  */
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
-					   const struct option *opt, int unset)
+					   const struct option *opt,
+					   const char *arg, int unset)
 {
+	BUG_ON_OPT_ARG(arg);
 	return PARSE_OPT_UNKNOWN;
 }

parse-options.c +28 ~~-14~~

68 lines not on this page · raw message

parse-options.h +3 -2

index 617bca8efe..75c953a887 100644
@@ -50,7 +50,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
 struct parse_opt_ctx_t;
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
-					      const struct option *opt, int unset);
+					      const struct option *opt,
+					      const char *arg, int unset);
 
 /*
  * `type`::

@@ -270,7 +271,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
-int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, int);
+int parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx, const struct option *, const char *, int);
 int parse_opt_passthru(const struct option *, const char *, int);
 int parse_opt_passthru_argv(const struct option *, const char *, int);

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #9 lore raw

PATCH 9/75 diff.h: keep forward struct declarations sorted

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 diff.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff.h +6 -6

index 9e8061ca29..4b65ff739f 100644
@@ -9,16 +9,16 @@
 #include "object.h"
 #include "oidset.h"
 
-struct rev_info;
+struct combine_diff_path;
+struct commit;
+struct diff_filespec;
 struct diff_options;
 struct diff_queue_struct;
-struct strbuf;
-struct diff_filespec;
-struct userdiff_driver;
 struct oid_array;
-struct commit;
-struct combine_diff_path;
 struct repository;
+struct rev_info;
+struct strbuf;
+struct userdiff_driver;
 
 typedef int (*pathchange_fn_t)(struct diff_options *options,
 		 struct combine_diff_path *path);

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #10 lore raw

PATCH 10/75 diff.h: avoid bit fields in struct diff_flags

In reply to Nguyễn Thái Ngọc Duy

Bitfield addresses cannot be passed around in a pointer. This makes it hard to use parse-options to set/unset them. Turn this struct to normal integers. This of course increases the size of this struct multiple times, but since we only have a handful of diff_options variables around, memory consumption is not at all a concern.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 diff.h | 66 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff.h +33 ~~-33~~

index 4b65ff739f..b71062c2d2 100644
@@ -64,39 +64,39 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 
 #define DIFF_FLAGS_INIT { 0 }
 struct diff_flags {
-	unsigned recursive:1;
-	unsigned tree_in_recursive:1;
-	unsigned binary:1;
-	unsigned text:1;
-	unsigned full_index:1;
-	unsigned silent_on_remove:1;
-	unsigned find_copies_harder:1;
-	unsigned follow_renames:1;
-	unsigned rename_empty:1;
-	unsigned has_changes:1;
-	unsigned quick:1;
-	unsigned no_index:1;
-	unsigned allow_external:1;
-	unsigned exit_with_status:1;
-	unsigned reverse_diff:1;
-	unsigned check_failed:1;
-	unsigned relative_name:1;
-	unsigned ignore_submodules:1;
-	unsigned dirstat_cumulative:1;
-	unsigned dirstat_by_file:1;
-	unsigned allow_textconv:1;
-	unsigned textconv_set_via_cmdline:1;
-	unsigned diff_from_contents:1;
-	unsigned dirty_submodules:1;
-	unsigned ignore_untracked_in_submodules:1;
-	unsigned ignore_dirty_submodules:1;
-	unsigned override_submodule_config:1;
-	unsigned dirstat_by_line:1;
-	unsigned funccontext:1;
-	unsigned default_follow_renames:1;
-	unsigned stat_with_summary:1;
-	unsigned suppress_diff_headers:1;
-	unsigned dual_color_diffed_diffs:1;
+	unsigned recursive;
+	unsigned tree_in_recursive;
+	unsigned binary;
+	unsigned text;
+	unsigned full_index;
+	unsigned silent_on_remove;
+	unsigned find_copies_harder;
+	unsigned follow_renames;
+	unsigned rename_empty;
+	unsigned has_changes;
+	unsigned quick;
+	unsigned no_index;
+	unsigned allow_external;
+	unsigned exit_with_status;
+	unsigned reverse_diff;
+	unsigned check_failed;
+	unsigned relative_name;
+	unsigned ignore_submodules;
+	unsigned dirstat_cumulative;
+	unsigned dirstat_by_file;
+	unsigned allow_textconv;
+	unsigned textconv_set_via_cmdline;
+	unsigned diff_from_contents;
+	unsigned dirty_submodules;
+	unsigned ignore_untracked_in_submodules;
+	unsigned ignore_dirty_submodules;
+	unsigned override_submodule_config;
+	unsigned dirstat_by_line;
+	unsigned funccontext;
+	unsigned default_follow_renames;
+	unsigned stat_with_summary;
+	unsigned suppress_diff_headers;
+	unsigned dual_color_diffed_diffs;
 };
 
 static inline void diff_flags_or(struct diff_flags *a,

Signature · 1 line

2.20.0.482.g66447595a7

Stefan Beller

2019-01-09 20:22 UTC #11 lore raw

Re: [PATCH 10/75] diff.h: avoid bit fields in struct diff_flags

Quoted 6 lines · Nguyễn Thái Ngọc Duy

Bitfield addresses cannot be passed around in a pointer. This makes it hard to use parse-options to set/unset them. Turn this struct to normal integers. This of course increases the size of this struct multiple times, but since we only have a handful of diff_options variables around, memory consumption is not at all a concern.

Makes sense, and continues
8cc633286a (Merge branch 'bw/diff-opt-impl-to-bitfields', 2017-11-09)

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #12 lore raw

PATCH 11/75 diff.c: prepare to use parse_options() for parsing

In reply to Nguyễn Thái Ngọc Duy

This is a preparation step to start using parse_options() to parse diff/revision options instead of what we have now. There are a couple of good things from using parse_options():

- better help usage
- easier to add new options
- better completion support
- help usage generation
- better integration with main command option parser. We can just concat the main command's option array and diffopt's together and parse all in one go.
- detect colidding options (e.g. --reverse is used by revision code, so diff code can't use it as long name for -R)
- consistent syntax, e.g. option that takes mandatory argument will now accept both "--option=value" and "--option value".

The plan is migrate all diff/rev options to parse_options(). Then we could get rid of diff_opt_parse() and expose parseopts[] directly to the caller.

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 diff.c | 27 +++++++++++++++++++++++++++
 diff.h |  2 ++
 2 files changed, 29 insertions(+)

diff.c +27 -0

index 6954f4e39a..a866f13c76 100644
@@ -23,6 +23,7 @@
 #include "argv-array.h"
 #include "graph.h"
 #include "packfile.h"
+#include "parse-options.h"
 #include "help.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY

@@ -4424,6 +4425,8 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 	builtin_checkdiff(name, other, attr_path, p->one, p->two, o);
 }
 
+static void prep_parse_options(struct diff_options *options);
+
 void repo_diff_setup(struct repository *r, struct diff_options *options)
 {
 	memcpy(options, &default_diff_options, sizeof(*options));

@@ -4465,6 +4468,8 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 
 	options->color_moved = diff_color_moved_default;
 	options->color_moved_ws_handling = diff_color_moved_ws_default;
+
+	prep_parse_options(options);
 }
 
 void diff_setup_done(struct diff_options *options)

@@ -4568,6 +4573,8 @@ void diff_setup_done(struct diff_options *options)
 
 	if (!options->use_color || external_diff())
 		options->color_moved = 0;
+
+	FREE_AND_NULL(options->parseopts);
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)

@@ -4858,6 +4865,16 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)
 	return 1;
 }
 
+static void prep_parse_options(struct diff_options *options)
+{
+	struct option parseopts[] = {
+		OPT_END()
+	};
+
+	ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts));
+	memcpy(options->parseopts, parseopts, sizeof(parseopts));
+}
+
 int diff_opt_parse(struct diff_options *options,
 		   const char **av, int ac, const char *prefix)
 {

@@ -4868,6 +4885,16 @@ int diff_opt_parse(struct diff_options *options,
 	if (!prefix)
 		prefix = "";
 
+	ac = parse_options(ac, av, prefix, options->parseopts, NULL,
+			   PARSE_OPT_KEEP_DASHDASH |
+			   PARSE_OPT_KEEP_UNKNOWN |
+			   PARSE_OPT_NO_INTERNAL_HELP |
+			   PARSE_OPT_ONE_SHOT |
+			   PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (ac)
+		return ac;
+
 	/* Output format options */
 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
 	    || opt_arg(arg, 'U', "unified", &options->context))

diff.h +2 -0

index b71062c2d2..8c4748ebcf 100644
@@ -15,6 +15,7 @@ struct diff_filespec;
 struct diff_options;
 struct diff_queue_struct;
 struct oid_array;
+struct option;
 struct repository;
 struct rev_info;
 struct strbuf;

@@ -229,6 +230,7 @@ struct diff_options {
 	unsigned color_moved_ws_handling;
 
 	struct repository *repo;
+	struct option *parseopts;
 };
 
 void diff_emit_submodule_del(struct diff_options *o, const char *line);

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #13 lore raw

PATCH 12/75 diff.c: convert -u|-p|--patch

In reply to Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
 diff.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff.c +8 -2

index a866f13c76..59132ceaff 100644
@@ -4868,6 +4868,13 @@ static int parse_objfind_opt(struct diff_options *opt, const char *arg)
 static void prep_parse_options(struct diff_options *options)
 {
 	struct option parseopts[] = {
+		OPT_GROUP(N_("Diff output format options")),
+		OPT_BITOP('p', "patch", &options->output_format,
+			  N_("generate patch"),
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
+		OPT_BITOP('u', NULL, &options->output_format,
+			  N_("generate patch"),
+			  DIFF_FORMAT_PATCH, DIFF_FORMAT_NO_OUTPUT),
 		OPT_END()
 	};

@@ -4896,8 +4903,7 @@ int diff_opt_parse(struct diff_options *options,
 		return ac;
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
-	    || opt_arg(arg, 'U', "unified", &options->context))
+	if (opt_arg(arg, 'U', "unified", &options->context))
 		enable_patch_output(&options->output_format);
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #14 lore raw

PATCH 73/75 range-diff: use parse_options() instead of diff_opt_parse()

In reply to 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 <[email protected]>

---
 builtin/range-diff.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

builtin/range-diff.c +6 ~~-20~~

index f01a0be851..784bd19321 100644
@@ -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);

Signature · 1 line

2.20.0.482.g66447595a7

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #15 lore raw

PATCH 74/75 diff --no-index: use parse_options() instead of diff_opt_parse()

In reply to 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 then burying it in diff-no-index.c

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

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

builtin/diff.c +3 ~~-18~~

index f0393bba23..52dc3e136f 100644
@@ -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-no-index.c +29 ~~-20~~

82 lines not on this page · raw message

diff.h +2 -1

index 8c4748ebcf..44843d8929 100644
@@ -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(const char *def, const struct diff_flags *flags,
 		       int ita_invisible_in_index);

t/t4053-diff-no-index.sh +1 -2

index 6e0dd6f9e5..fb25cdb789 100755
@@ -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
 	)
 '

Signature · 1 line

2.20.0.482.g66447595a7

Eric Sunshine

2018-12-27 20:34 UTC #16 lore raw

Re: [PATCH 74/75] diff --no-index: use parse_options() instead of diff_opt_parse()

Quoted 2 lines · 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 then burying it in diff-no-index.c

s/then/than/

Quoted 1 line

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

Nguyễn Thái Ngọc Duy

author

2018-12-27 16:26 UTC #17 lore raw

PATCH 75/75 am: avoid diff_opt_parse()

In reply to 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 <[email protected]>

---
 builtin/am.c | 4 ++--
 diff.c       | 6 ++++++
 diff.h       | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

builtin/am.c +2 -2

index 8f27f3375b..a98e65f440 100644
@@ -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.c +6 -0

index 43ae8d4d2c..8a7011b928 100644
@@ -4756,6 +4756,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.h +2 -0

index 44843d8929..48cfa3f0a6 100644
@@ -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);

Signature · 1 line

2.20.0.482.g66447595a7