[PATCH 00/19] Convert revision.c to parseopt part 1/4

[PATCH 00/19] Convert revision.c to parseopt part 1/4

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

Following the conversion in diff.c to use parse_options, this time it's
revision.c. There are about 76 patches to convert all options, split in
4 parts. After that there are about 10 more to convert rev users to use
parse_options() directly.

Full 76 patches are available here [1]. I'm not quite done with those
last "10 more" patches yet [2], still stuck at that ancient blame UI.
But I should be done by the 76 patches are merged.

[1] https://gitlab.com/pclouds/git/commits/revision-opt-parse-options
[2] https://gitlab.com/pclouds/git/commits/parse-options-step-no-more


Nguyễn Thái Ngọc Duy (19):
  revision.h: avoid bit fields in struct rev_info
  revision.h: move repo field down
  revision.c: prepare to convert handle_revision_pseudo_opt()
  rev-parseopt: convert --all
  rev-parseopt: convert --branches
  rev-parseopt: convert --bisect
  rev-parseopt: convert --tags
  rev-parseopt: convert --remotes
  rev-parseopt: convert --glob
  rev-parseopt: convert --exclude
  rev-parseopt: convert --reflog
  rev-parseopt: convert --indexed-objects
  rev-parseopt: convert --not
  rev-parseopt: convert --no-walk and --do-walk
  rev-parseopt: convert --single-worktree
  rev-parseopt: prepare to convert handle_revision_opt()
  rev-parseopt: convert --max-count
  rev-parseopt: convert --skip
  rev-parseopt: convert --min-age and --max-age

 Documentation/technical/api-diff.txt |   6 +-
 diff.c                               |  16 -
 diff.h                               |   1 -
 parse-options-cb.c                   |   8 +
 parse-options.h                      |   4 +
 revision.c                           | 465 +++++++++++++++++++--------
 revision.h                           | 172 +++++-----
 7 files changed, 441 insertions(+), 231 deletions(-)

-- 
2.21.0.1141.gd54ac2cb17

[PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[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 rev_info variables
around, memory consumption is not at all a concern.

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

diff --git a/revision.h b/revision.h
index 4134dc6029..01e4c42274 100644
--- a/revision.h
+++ b/revision.h
@@ -107,95 +107,95 @@ struct rev_info {
 
 	unsigned int early_output;
 
-	unsigned int	ignore_missing:1,
-			ignore_missing_links:1;
+	unsigned int ignore_missing;
+	unsigned int ignore_missing_links;
 
 	/* Traversal flags */
-	unsigned int	dense:1,
-			prune:1,
-			no_walk:2,
-			remove_empty_trees:1,
-			simplify_history:1,
-			topo_order:1,
-			simplify_merges:1,
-			simplify_by_decoration:1,
-			single_worktree:1,
-			tag_objects:1,
-			tree_objects:1,
-			blob_objects:1,
-			verify_objects:1,
-			edge_hint:1,
-			edge_hint_aggressive:1,
-			limited:1,
-			unpacked:1,
-			boundary:2,
-			count:1,
-			left_right:1,
-			left_only:1,
-			right_only:1,
-			rewrite_parents:1,
-			print_parents:1,
-			show_decorations:1,
-			reverse:1,
-			reverse_output_stage:1,
-			cherry_pick:1,
-			cherry_mark:1,
-			bisect:1,
-			ancestry_path:1,
-			first_parent_only:1,
-			line_level_traverse:1,
-			tree_blobs_in_commit_order:1,
-
-			/*
-			 * Blobs are shown without regard for their existence.
-			 * But not so for trees: unless exclude_promisor_objects
-			 * is set and the tree in question is a promisor object;
-			 * OR ignore_missing_links is set, the revision walker
-			 * dies with a "bad tree object HASH" message when
-			 * encountering a missing tree. For callers that can
-			 * handle missing trees and want them to be filterable
-			 * and showable, set this to true. The revision walker
-			 * will filter and show such a missing tree as usual,
-			 * but will not attempt to recurse into this tree
-			 * object.
-			 */
-			do_not_die_on_missing_tree:1,
-
-			/* for internal use only */
-			exclude_promisor_objects:1;
+	unsigned int dense;
+	unsigned int prune;
+	unsigned int no_walk;
+	unsigned int remove_empty_trees;
+	unsigned int simplify_history;
+	unsigned int topo_order;
+	unsigned int simplify_merges;
+	unsigned int simplify_by_decoration;
+	unsigned int single_worktree;
+	unsigned int tag_objects;
+	unsigned int tree_objects;
+	unsigned int blob_objects;
+	unsigned int verify_objects;
+	unsigned int edge_hint;
+	unsigned int edge_hint_aggressive;
+	unsigned int limited;
+	unsigned int unpacked;
+	unsigned int boundary;
+	unsigned int count;
+	unsigned int left_right;
+	unsigned int left_only;
+	unsigned int right_only;
+	unsigned int rewrite_parents;
+	unsigned int print_parents;
+	unsigned int show_decorations;
+	unsigned int reverse;
+	unsigned int reverse_output_stage;
+	unsigned int cherry_pick;
+	unsigned int cherry_mark;
+	unsigned int bisect;
+	unsigned int ancestry_path;
+	unsigned int first_parent_only;
+	unsigned int line_level_traverse;
+	unsigned int tree_blobs_in_commit_order;
+
+	/*
+	 * Blobs are shown without regard for their existence.
+	 * But not so for trees: unless exclude_promisor_objects
+	 * is set and the tree in question is a promisor object;
+	 * OR ignore_missing_links is set, the revision walker
+	 * dies with a "bad tree object HASH" message when
+	 * encountering a missing tree. For callers that can
+	 * handle missing trees and want them to be filterable
+	 * and showable, set this to true. The revision walker
+	 * will filter and show such a missing tree as usual,
+	 * but will not attempt to recurse into this tree
+	 * object.
+	 */
+	unsigned int do_not_die_on_missing_tree;
+
+	/* for internal use only */
+	unsigned int exclude_promisor_objects;
 
 	/* Diff flags */
-	unsigned int	diff:1,
-			full_diff:1,
-			show_root_diff:1,
-			no_commit_id:1,
-			verbose_header:1,
-			ignore_merges:1,
-			combine_merges:1,
-			combined_all_paths:1,
-			dense_combined_merges:1,
-			always_show_header:1;
+	unsigned int diff;
+	unsigned int full_diff;
+	unsigned int show_root_diff;
+	unsigned int no_commit_id;
+	unsigned int verbose_header;
+	unsigned int ignore_merges;
+	unsigned int combine_merges;
+	unsigned int combined_all_paths;
+	unsigned int dense_combined_merges;
+	unsigned int always_show_header;
 
 	/* Format info */
-	unsigned int	shown_one:1,
-			shown_dashes:1,
-			show_merge:1,
-			show_notes:1,
-			show_notes_given:1,
-			show_signature:1,
-			pretty_given:1,
-			abbrev_commit:1,
-			abbrev_commit_given:1,
-			zero_commit:1,
-			use_terminator:1,
-			missing_newline:1,
-			date_mode_explicit:1,
-			preserve_subject:1;
-	unsigned int	disable_stdin:1;
+	unsigned int shown_one;
+	unsigned int shown_dashes;
+	unsigned int show_merge;
+	unsigned int show_notes;
+	unsigned int show_notes_given;
+	unsigned int show_signature;
+	unsigned int pretty_given;
+	unsigned int abbrev_commit;
+	unsigned int abbrev_commit_given;
+	unsigned int zero_commit;
+	unsigned int use_terminator;
+	unsigned int missing_newline;
+	unsigned int date_mode_explicit;
+	unsigned int preserve_subject;
+	unsigned int disable_stdin;
 	/* --show-linear-break */
-	unsigned int	track_linear:1,
-			track_first_time:1,
-			linear:1;
+	unsigned int track_linear;
+	unsigned int track_first_time;
+	unsigned int linear;
 
 	struct date_mode date_mode;
 	int		expand_tabs_in_log; /* unset if negative */
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 02/19] revision.h: move repo field down

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

This block at the top of rev_info is "Starting list" and repo is
obviously not one. Move it to the bottom since it's not that important
to stay on top.

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

diff --git a/revision.h b/revision.h
index 01e4c42274..71e724c59c 100644
--- a/revision.h
+++ b/revision.h
@@ -74,7 +74,6 @@ struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
 	struct object_array pending;
-	struct repository *repo;
 
 	/* Parents of shown commits */
 	struct object_array boundary_commits;
@@ -278,6 +277,8 @@ struct rev_info {
 	struct revision_sources *sources;
 
 	struct topo_walk_info *topo_walk_info;
+
+	struct repository *repo;
 };
 
 int ref_excluded(struct string_list *, const char *path);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 03/19] revision.c: prepare to convert handle_revision_pseudo_opt()

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

This patch is essentially no-op. It allows to parse_options() to handle
some options. But the new option list remains empty. The option will be
moved one by one from the old manual parsing code to this list.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 44 ++++++++++++++++++++++++++++++++++----------
 revision.h |  2 ++
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index d4aaf0ef25..65d40c9255 100644
--- a/revision.c
+++ b/revision.c
@@ -26,6 +26,7 @@
 #include "argv-array.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "parse-options.h"
 #include "prio-queue.h"
 #include "hashmap.h"
 
@@ -1598,6 +1599,8 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 	return 1;
 }
 
+static void make_pseudo_options(struct rev_info *revs);
+
 void repo_init_revisions(struct repository *r,
 			 struct rev_info *revs,
 			 const char *prefix)
@@ -1638,6 +1641,7 @@ void repo_init_revisions(struct repository *r,
 	}
 
 	revs->notes_opt.use_default_notes = -1;
+	make_pseudo_options(revs);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
@@ -2355,6 +2359,25 @@ static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void
 	return for_each_bisect_ref(refs, fn, cb_data, term_good);
 }
 
+static void make_pseudo_options(struct rev_info *revs)
+{
+	/*
+	 * NOTE!
+	 *
+	 * Commands like "git shortlog" will not accept the options below
+	 * unless parse_revision_opt queues them (as opposed to erroring
+	 * out).
+	 *
+	 * When implementing your new pseudo-option, remember to
+	 * register it in the list at the top of handle_revision_opt.
+	 */
+	struct option options[] = {
+		OPT_END()
+	};
+	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
+	memcpy(revs->pseudo_options, options, sizeof(options));
+}
+
 static int handle_revision_pseudo_opt(const char *submodule,
 				struct rev_info *revs,
 				int argc, const char **argv, int *flags)
@@ -2377,16 +2400,16 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else
 		refs = get_main_ref_store(revs->repo);
 
-	/*
-	 * NOTE!
-	 *
-	 * Commands like "git shortlog" will not accept the options below
-	 * unless parse_revision_opt queues them (as opposed to erroring
-	 * out).
-	 *
-	 * When implementing your new pseudo-option, remember to
-	 * register it in the list at the top of handle_revision_opt.
-	 */
+	argc = parse_options(argc, argv, revs->prefix,
+			     revs->pseudo_options, 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 (argc)
+		return argc;
+
 	if (!strcmp(arg, "--all")) {
 		handle_refs(refs, revs, *flags, refs_for_each_ref);
 		handle_refs(refs, revs, *flags, refs_head_ref);
@@ -2685,6 +2708,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
 
+	FREE_AND_NULL(revs->pseudo_options);
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index 71e724c59c..0769c97dee 100644
--- a/revision.h
+++ b/revision.h
@@ -39,6 +39,7 @@
 #define DECORATE_FULL_REFS	2
 
 struct log_info;
+struct option;
 struct repository;
 struct rev_info;
 struct string_list;
@@ -279,6 +280,7 @@ struct rev_info {
 	struct topo_walk_info *topo_walk_info;
 
 	struct repository *repo;
+	struct option *pseudo_options;
 };
 
 int ref_excluded(struct string_list *, const char *path);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 04/19] rev-parseopt: convert --all

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 revision.c | 41 ++++++++++++++++++++++++++++++-----------
 revision.h |  2 ++
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/revision.c b/revision.c
index 65d40c9255..9a346577f6 100644
--- a/revision.c
+++ b/revision.c
@@ -30,6 +30,10 @@
 #include "prio-queue.h"
 #include "hashmap.h"
 
+#define OPT_REV_NOARG(s, l, h, cb) \
+	OPT_CALLBACK_F(s, l, revs, NULL, h, \
+		       PARSE_OPT_NONEG | PARSE_OPT_NOARG, cb)
+
 volatile show_early_output_fn_t show_early_output;
 
 static const char *term_bad;
@@ -2359,6 +2363,26 @@ static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, void
 	return for_each_bisect_ref(refs, fn, cb_data, term_good);
 }
 
+static int rev_opt_all(const struct option *opt, const char *arg, int unset)
+{
+	struct rev_info		*revs  = opt->value;
+	int			 flags = *revs->pseudo_flags;
+	struct ref_store	*refs  = revs->pseudo_refs;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+	handle_refs(refs, revs, flags, refs_for_each_ref);
+	handle_refs(refs, revs, flags, refs_head_ref);
+	if (!revs->single_worktree) {
+		struct all_refs_cb cb;
+
+		init_all_refs_cb(&cb, revs, flags);
+		other_head_refs(handle_one_ref, &cb);
+	}
+	clear_ref_exclusion(&revs->ref_excludes);
+	return 0;
+}
+
 static void make_pseudo_options(struct rev_info *revs)
 {
 	/*
@@ -2372,6 +2396,9 @@ static void make_pseudo_options(struct rev_info *revs)
 	 * register it in the list at the top of handle_revision_opt.
 	 */
 	struct option options[] = {
+		OPT_REV_NOARG(0, "all",
+			      N_("include all refs in refs/ and HEAD"),
+			      rev_opt_all),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2400,6 +2427,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else
 		refs = get_main_ref_store(revs->repo);
 
+	revs->pseudo_flags = flags;
+	revs->pseudo_refs = refs;
 	argc = parse_options(argc, argv, revs->prefix,
 			     revs->pseudo_options, NULL,
 			     PARSE_OPT_KEEP_DASHDASH |
@@ -2410,17 +2439,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--all")) {
-		handle_refs(refs, revs, *flags, refs_for_each_ref);
-		handle_refs(refs, revs, *flags, refs_head_ref);
-		if (!revs->single_worktree) {
-			struct all_refs_cb cb;
-
-			init_all_refs_cb(&cb, revs, *flags);
-			other_head_refs(handle_one_ref, &cb);
-		}
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (!strcmp(arg, "--branches")) {
+	if (!strcmp(arg, "--branches")) {
 		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
diff --git a/revision.h b/revision.h
index 0769c97dee..cec5215c04 100644
--- a/revision.h
+++ b/revision.h
@@ -281,6 +281,8 @@ struct rev_info {
 
 	struct repository *repo;
 	struct option *pseudo_options;
+	int *pseudo_flags;
+	struct ref_store *pseudo_refs;
 };
 
 int ref_excluded(struct string_list *, const char *path);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 05/19] rev-parseopt: convert --branches

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

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

diff --git a/revision.c b/revision.c
index 9a346577f6..7db1109b57 100644
--- a/revision.c
+++ b/revision.c
@@ -34,6 +34,10 @@
 	OPT_CALLBACK_F(s, l, revs, NULL, h, \
 		       PARSE_OPT_NONEG | PARSE_OPT_NOARG, cb)
 
+#define OPT_REV_OPTARG(s, l, a, h, cb) \
+	OPT_CALLBACK_F(s, l, revs, a, h, \
+		       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, cb)
+
 volatile show_early_output_fn_t show_early_output;
 
 static const char *term_bad;
@@ -2383,6 +2387,27 @@ static int rev_opt_all(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int rev_opt_branches(const struct option *opt,
+			    const char *arg, int unset)
+{
+	struct rev_info		*revs  = opt->value;
+	int			 flags = *revs->pseudo_flags;
+	struct ref_store	*refs  = revs->pseudo_refs;
+
+	BUG_ON_OPT_NEG(unset);
+	if (arg) {
+		struct all_refs_cb cb;
+
+		init_all_refs_cb(&cb, revs, flags);
+		for_each_glob_ref_in(handle_one_ref, arg, "refs/heads/", &cb);
+		clear_ref_exclusion(&revs->ref_excludes);
+	} else {
+		handle_refs(refs, revs, flags, refs_for_each_branch_ref);
+		clear_ref_exclusion(&revs->ref_excludes);
+	}
+	return 0;
+}
+
 static void make_pseudo_options(struct rev_info *revs)
 {
 	/*
@@ -2399,6 +2424,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_NOARG(0, "all",
 			      N_("include all refs in refs/ and HEAD"),
 			      rev_opt_all),
+		OPT_REV_OPTARG(0, "branches", N_("<pattern>"),
+			       N_("include all refs in refs/heads (optionally matches pattern)"),
+			       rev_opt_branches),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2439,10 +2467,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--branches")) {
-		handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (!strcmp(arg, "--bisect")) {
+	if (!strcmp(arg, "--bisect")) {
 		read_bisect_terms(&term_bad, &term_good);
 		handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
 		handle_refs(refs, revs, *flags ^ (UNINTERESTING | BOTTOM),
@@ -2463,11 +2488,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (skip_prefix(arg, "--branches=", &optarg)) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 06/19] rev-parseopt: convert --bisect

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

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

diff --git a/revision.c b/revision.c
index 7db1109b57..0d34f81716 100644
--- a/revision.c
+++ b/revision.c
@@ -2387,6 +2387,23 @@ static int rev_opt_all(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int rev_opt_bisect(const struct option *opt,
+			  const char *arg, int unset)
+{
+	struct rev_info		*revs  = opt->value;
+	int			 flags = *revs->pseudo_flags;
+	struct ref_store	*refs  = revs->pseudo_refs;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+	read_bisect_terms(&term_bad, &term_good);
+	handle_refs(refs, revs, flags, for_each_bad_bisect_ref);
+	handle_refs(refs, revs, flags ^ (UNINTERESTING | BOTTOM),
+		    for_each_good_bisect_ref);
+	revs->bisect = 1;
+	return 0;
+}
+
 static int rev_opt_branches(const struct option *opt,
 			    const char *arg, int unset)
 {
@@ -2427,6 +2444,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_OPTARG(0, "branches", N_("<pattern>"),
 			       N_("include all refs in refs/heads (optionally matches pattern)"),
 			       rev_opt_branches),
+		OPT_REV_NOARG(0, "bisect",
+			      N_("synonym to refs/bisect/good-* --not refs/bisect/bad"),
+			      rev_opt_bisect),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2467,13 +2487,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--bisect")) {
-		read_bisect_terms(&term_bad, &term_good);
-		handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
-		handle_refs(refs, revs, *flags ^ (UNINTERESTING | BOTTOM),
-			    for_each_good_bisect_ref);
-		revs->bisect = 1;
-	} else if (!strcmp(arg, "--tags")) {
+	if (!strcmp(arg, "--tags")) {
 		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--remotes")) {
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 07/19] rev-parseopt: convert --tags

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

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

diff --git a/revision.c b/revision.c
index 0d34f81716..5183cdf66d 100644
--- a/revision.c
+++ b/revision.c
@@ -2425,6 +2425,27 @@ static int rev_opt_branches(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_tags(const struct option *opt,
+			const char *arg, int unset)
+{
+	struct rev_info		*revs  = opt->value;
+	int			 flags = *revs->pseudo_flags;
+	struct ref_store	*refs  = revs->pseudo_refs;
+
+	BUG_ON_OPT_NEG(unset);
+	if (arg) {
+		struct all_refs_cb cb;
+
+		init_all_refs_cb(&cb, revs, flags);
+		for_each_glob_ref_in(handle_one_ref, arg, "refs/tags/", &cb);
+		clear_ref_exclusion(&revs->ref_excludes);
+	} else {
+		handle_refs(refs, revs, flags, refs_for_each_tag_ref);
+		clear_ref_exclusion(&revs->ref_excludes);
+	}
+	return 0;
+}
+
 static void make_pseudo_options(struct rev_info *revs)
 {
 	/*
@@ -2447,6 +2468,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_NOARG(0, "bisect",
 			      N_("synonym to refs/bisect/good-* --not refs/bisect/bad"),
 			      rev_opt_bisect),
+		OPT_REV_OPTARG(0, "tags", N_("<pattern>"),
+			       N_("include all refs in refs/tags (optionally matches pattern)"),
+			       rev_opt_tags),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2487,10 +2511,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--tags")) {
-		handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (!strcmp(arg, "--remotes")) {
+	if (!strcmp(arg, "--remotes")) {
 		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
@@ -2502,11 +2523,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (skip_prefix(arg, "--tags=", &optarg)) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 08/19] rev-parseopt: convert --remotes

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

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

diff --git a/revision.c b/revision.c
index 5183cdf66d..bcfca8856f 100644
--- a/revision.c
+++ b/revision.c
@@ -2425,6 +2425,27 @@ static int rev_opt_branches(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_remotes(const struct option *opt,
+			   const char *arg, int unset)
+{
+	struct rev_info		*revs  = opt->value;
+	int			 flags = *revs->pseudo_flags;
+	struct ref_store	*refs  = revs->pseudo_refs;
+
+	BUG_ON_OPT_NEG(unset);
+	if (arg) {
+		struct all_refs_cb cb;
+
+		init_all_refs_cb(&cb, revs, flags);
+		for_each_glob_ref_in(handle_one_ref, arg, "refs/remotes/", &cb);
+		clear_ref_exclusion(&revs->ref_excludes);
+	} else {
+		handle_refs(refs, revs, flags, refs_for_each_remote_ref);
+		clear_ref_exclusion(&revs->ref_excludes);
+	}
+	return 0;
+}
+
 static int rev_opt_tags(const struct option *opt,
 			const char *arg, int unset)
 {
@@ -2471,6 +2492,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_OPTARG(0, "tags", N_("<pattern>"),
 			       N_("include all refs in refs/tags (optionally matches pattern)"),
 			       rev_opt_tags),
+		OPT_REV_OPTARG(0, "remotes", N_("<pattern>"),
+			       N_("include all refs in refs/remotes (optionally matches pattern)"),
+			       rev_opt_remotes),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2511,10 +2535,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--remotes")) {
-		handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
-		clear_ref_exclusion(&revs->ref_excludes);
-	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
+	if ((argcount = parse_long_opt("glob", argv, &optarg))) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
 		for_each_glob_ref(handle_one_ref, optarg, &cb);
@@ -2523,11 +2544,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 09/19] rev-parseopt: convert --glob

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

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

diff --git a/revision.c b/revision.c
index bcfca8856f..013b8ec19f 100644
--- a/revision.c
+++ b/revision.c
@@ -30,6 +30,9 @@
 #include "prio-queue.h"
 #include "hashmap.h"
 
+#define OPT_REV(s, l, a, h, cb)		 \
+	OPT_CALLBACK_F(s, l, revs, a, h, PARSE_OPT_NONEG, cb)
+
 #define OPT_REV_NOARG(s, l, h, cb) \
 	OPT_CALLBACK_F(s, l, revs, NULL, h, \
 		       PARSE_OPT_NONEG | PARSE_OPT_NOARG, cb)
@@ -2425,6 +2428,20 @@ static int rev_opt_branches(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_glob(const struct option *opt,
+			const char *arg, int unset)
+{
+	struct rev_info	*revs  = opt->value;
+	int flags = *revs->pseudo_flags;
+	struct all_refs_cb cb;
+
+	BUG_ON_OPT_NEG(unset);
+	init_all_refs_cb(&cb, revs, flags);
+	for_each_glob_ref(handle_one_ref, arg, &cb);
+	clear_ref_exclusion(&revs->ref_excludes);
+	return 0;
+}
+
 static int rev_opt_remotes(const struct option *opt,
 			   const char *arg, int unset)
 {
@@ -2495,6 +2512,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_OPTARG(0, "remotes", N_("<pattern>"),
 			       N_("include all refs in refs/remotes (optionally matches pattern)"),
 			       rev_opt_remotes),
+		OPT_REV(0, "glob", N_("<pattern>"),
+			N_("include all refs matching shell glob"),
+			rev_opt_glob),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2535,13 +2555,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if ((argcount = parse_long_opt("glob", argv, &optarg))) {
-		struct all_refs_cb cb;
-		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref(handle_one_ref, optarg, &cb);
-		clear_ref_exclusion(&revs->ref_excludes);
-		return argcount;
-	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
+	if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
 	} else if (!strcmp(arg, "--reflog")) {
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 10/19] rev-parseopt: convert --exclude

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

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

diff --git a/revision.c b/revision.c
index 013b8ec19f..d34e17984d 100644
--- a/revision.c
+++ b/revision.c
@@ -2428,6 +2428,16 @@ static int rev_opt_branches(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_exclude(const struct option *opt,
+			   const char *arg, int unset)
+{
+	struct rev_info	*revs  = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	add_ref_exclusion(&revs->ref_excludes, arg);
+	return 0;
+}
+
 static int rev_opt_glob(const struct option *opt,
 			const char *arg, int unset)
 {
@@ -2515,6 +2525,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV(0, "glob", N_("<pattern>"),
 			N_("include all refs matching shell glob"),
 			rev_opt_glob),
+		OPT_REV(0, "exclude", N_("<pattern>"),
+			N_("exclude refs matching glob pattern"),
+			rev_opt_exclude),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2528,7 +2541,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	const char *arg = argv[0];
 	const char *optarg;
 	struct ref_store *refs;
-	int argcount;
 
 	if (submodule) {
 		/*
@@ -2555,10 +2567,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
-		add_ref_exclusion(&revs->ref_excludes, optarg);
-		return argcount;
-	} else if (!strcmp(arg, "--reflog")) {
+	if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
 		add_index_objects_to_pending(revs, *flags);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 11/19] rev-parseopt: convert --reflog

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

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

diff --git a/revision.c b/revision.c
index d34e17984d..6efa9bee1e 100644
--- a/revision.c
+++ b/revision.c
@@ -2452,6 +2452,18 @@ static int rev_opt_glob(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_reflog(const struct option *opt,
+			  const char *arg, int unset)
+{
+	struct rev_info *revs  = opt->value;
+	int flags = *revs->pseudo_flags;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+	add_reflogs_to_pending(revs, flags);
+	return 0;
+}
+
 static int rev_opt_remotes(const struct option *opt,
 			   const char *arg, int unset)
 {
@@ -2528,6 +2540,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV(0, "exclude", N_("<pattern>"),
 			N_("exclude refs matching glob pattern"),
 			rev_opt_exclude),
+		OPT_REV_NOARG(0, "reflog",
+			      N_("include all refs from reflog"),
+			      rev_opt_reflog),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2567,9 +2582,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--reflog")) {
-		add_reflogs_to_pending(revs, *flags);
-	} else if (!strcmp(arg, "--indexed-objects")) {
+	if (!strcmp(arg, "--indexed-objects")) {
 		add_index_objects_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING | BOTTOM;
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 12/19] rev-parseopt: convert --indexed-objects

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

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

diff --git a/revision.c b/revision.c
index 6efa9bee1e..9cfa4dc151 100644
--- a/revision.c
+++ b/revision.c
@@ -2438,6 +2438,18 @@ static int rev_opt_exclude(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_indexed_objects(const struct option *opt,
+				   const char *arg, int unset)
+{
+	struct rev_info *revs  = opt->value;
+	int flags = *revs->pseudo_flags;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+	add_index_objects_to_pending(revs, flags);
+	return 0;
+}
+
 static int rev_opt_glob(const struct option *opt,
 			const char *arg, int unset)
 {
@@ -2543,6 +2555,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_NOARG(0, "reflog",
 			      N_("include all refs from reflog"),
 			      rev_opt_reflog),
+		OPT_REV_NOARG(0, "indexed-objects",
+			      N_("include all trees and blobs used by the index"),
+			      rev_opt_indexed_objects),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2582,9 +2597,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--indexed-objects")) {
-		add_index_objects_to_pending(revs, *flags);
-	} else if (!strcmp(arg, "--not")) {
+	if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
 		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 13/19] rev-parseopt: convert --not

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

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

diff --git a/revision.c b/revision.c
index 9cfa4dc151..b02cb4660b 100644
--- a/revision.c
+++ b/revision.c
@@ -2464,6 +2464,17 @@ static int rev_opt_glob(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_not(const struct option *opt,
+		       const char *arg, int unset)
+{
+	struct rev_info *revs  = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	BUG_ON_OPT_ARG(arg);
+	*revs->pseudo_flags ^= UNINTERESTING | BOTTOM;
+	return 0;
+}
+
 static int rev_opt_reflog(const struct option *opt,
 			  const char *arg, int unset)
 {
@@ -2558,6 +2569,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_NOARG(0, "indexed-objects",
 			      N_("include all trees and blobs used by the index"),
 			      rev_opt_indexed_objects),
+		OPT_REV_NOARG(0, "not",
+			      N_("reverse the meaning of '^' for all following revisions"),
+			      rev_opt_not),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2597,9 +2611,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--not")) {
-		*flags ^= UNINTERESTING | BOTTOM;
-	} else if (!strcmp(arg, "--no-walk")) {
+	if (!strcmp(arg, "--no-walk")) {
 		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
 	} else if (skip_prefix(arg, "--no-walk=", &optarg)) {
 		/*
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 14/19] rev-parseopt: convert --no-walk and --do-walk

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

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

diff --git a/revision.c b/revision.c
index b02cb4660b..f04eb7f140 100644
--- a/revision.c
+++ b/revision.c
@@ -2464,6 +2464,29 @@ static int rev_opt_glob(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_no_walk(const struct option *opt,
+			   const char *arg, int unset)
+{
+	struct rev_info *revs  = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	if (!arg) {
+		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+	} else {
+		/*
+		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
+		 * not allowed, since the argument is optional.
+		 */
+		if (!strcmp(arg, "sorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
+		else if (!strcmp(arg, "unsorted"))
+			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
+		else
+			return error(_("invalid argument to --no-walk"));
+	}
+	return 0;
+}
+
 static int rev_opt_not(const struct option *opt,
 		       const char *arg, int unset)
 {
@@ -2572,6 +2595,12 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_REV_NOARG(0, "not",
 			      N_("reverse the meaning of '^' for all following revisions"),
 			      rev_opt_not),
+		OPT_REV_OPTARG(0, "no-walk", N_("(sorted|unsorted)"),
+			       N_("only show given commits but do not traverse their ancestors"),
+			       rev_opt_no_walk),
+		OPT_SET_INT_F(0, "do-walk", &revs->no_walk,
+			      N_("override a previous --no-walk"),
+			      0, PARSE_OPT_NONEG),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2583,7 +2612,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 				int argc, const char **argv, int *flags)
 {
 	const char *arg = argv[0];
-	const char *optarg;
 	struct ref_store *refs;
 
 	if (submodule) {
@@ -2611,22 +2639,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	if (argc)
 		return argc;
 
-	if (!strcmp(arg, "--no-walk")) {
-		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-	} else if (skip_prefix(arg, "--no-walk=", &optarg)) {
-		/*
-		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
-		 * not allowed, since the argument is optional.
-		 */
-		if (!strcmp(optarg, "sorted"))
-			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-		else if (!strcmp(optarg, "unsorted"))
-			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
-		else
-			return error("invalid argument to --no-walk");
-	} else if (!strcmp(arg, "--do-walk")) {
-		revs->no_walk = 0;
-	} else if (!strcmp(arg, "--single-worktree")) {
+	if (!strcmp(arg, "--single-worktree")) {
 		revs->single_worktree = 1;
 	} else {
 		return 0;
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 15/19] rev-parseopt: convert --single-worktree

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

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

diff --git a/revision.c b/revision.c
index f04eb7f140..dd22ac5c39 100644
--- a/revision.c
+++ b/revision.c
@@ -2601,6 +2601,9 @@ static void make_pseudo_options(struct rev_info *revs)
 		OPT_SET_INT_F(0, "do-walk", &revs->no_walk,
 			      N_("override a previous --no-walk"),
 			      0, PARSE_OPT_NONEG),
+		OPT_BOOL_F(0, "single-worktree", &revs->single_worktree,
+			   N_("only consider refs from the current worktree"),
+			   PARSE_OPT_NONEG),
 		OPT_END()
 	};
 	ALLOC_ARRAY(revs->pseudo_options, ARRAY_SIZE(options));
@@ -2611,7 +2614,6 @@ static int handle_revision_pseudo_opt(const char *submodule,
 				struct rev_info *revs,
 				int argc, const char **argv, int *flags)
 {
-	const char *arg = argv[0];
 	struct ref_store *refs;
 
 	if (submodule) {
@@ -2636,16 +2638,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 			     PARSE_OPT_NO_INTERNAL_HELP |
 			     PARSE_OPT_ONE_SHOT |
 			     PARSE_OPT_STOP_AT_NON_OPTION);
-	if (argc)
-		return argc;
-
-	if (!strcmp(arg, "--single-worktree")) {
-		revs->single_worktree = 1;
-	} else {
-		return 0;
-	}
-
-	return 1;
+	return argc;
 }
 
 static void NORETURN diagnose_missing_default(const char *def)
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 16/19] rev-parseopt: prepare to convert handle_revision_opt()

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

Besides preparing an empty option table to be added in. The table is
also concatenated with diff option table so we don't need
diff_opt_parse() anymore. Merging with pseudo-opt table though will
not happen until we kill parse_revision_opt().

--abbrev has to be converted right away to override the same one from
the diff option parser (which runs first now, if not overriden)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/api-diff.txt |  6 +--
 diff.c                               | 16 -------
 diff.h                               |  1 -
 revision.c                           | 64 +++++++++++++++++++++-------
 revision.h                           |  1 +
 5 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/Documentation/technical/api-diff.txt b/Documentation/technical/api-diff.txt
index 30fc0e9c93..1e4e6968f4 100644
--- a/Documentation/technical/api-diff.txt
+++ b/Documentation/technical/api-diff.txt
@@ -22,9 +22,9 @@ Calling sequence
   sets up the vanilla default.
 
 * Fill in the options structure to specify desired output format, rename
-  detection, etc.  `diff_opt_parse()` can be used to parse options given
-  from the command line in a way consistent with existing git-diff
-  family of programs.
+  detection, etc. `parseopts[]` can be used with parse_options() to
+  parse options from the command line in a way consistent with
+  existing git-diff family of programs.
 
 * Call `diff_setup_done()`; this inspects the options set up so far for
   internal consistency and make necessary tweaking to it (e.g. if
diff --git a/diff.c b/diff.c
index 4d3cf83a27..ef0eb2a160 100644
--- a/diff.c
+++ b/diff.c
@@ -5522,22 +5522,6 @@ static void prep_parse_options(struct diff_options *options)
 	memcpy(options->parseopts, parseopts, sizeof(parseopts));
 }
 
-int diff_opt_parse(struct diff_options *options,
-		   const char **av, int ac, const char *prefix)
-{
-	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);
-
-	return ac;
-}
-
 int parse_rename_score(const char **cp_p)
 {
 	unsigned long num, scale;
diff --git a/diff.h b/diff.h
index b20cbcc091..c75480a998 100644
--- a/diff.h
+++ b/diff.h
@@ -351,7 +351,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb);
 #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts)
 #endif
 void repo_diff_setup(struct repository *, struct diff_options *);
-int diff_opt_parse(struct diff_options *, const char **, int, const char *);
 void diff_setup_done(struct diff_options *);
 int git_config_rename(const char *var, const char *value);
 
diff --git a/revision.c b/revision.c
index dd22ac5c39..f15aa3e62d 100644
--- a/revision.c
+++ b/revision.c
@@ -1611,6 +1611,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 }
 
 static void make_pseudo_options(struct rev_info *revs);
+static void make_rev_options(struct rev_info *revs);
 
 void repo_init_revisions(struct repository *r,
 			 struct rev_info *revs,
@@ -1653,6 +1654,7 @@ void repo_init_revisions(struct repository *r,
 
 	revs->notes_opt.use_default_notes = -1;
 	make_pseudo_options(revs);
+	make_rev_options(revs);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
@@ -1953,6 +1955,38 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
 	add_grep(revs, pattern, GREP_PATTERN_BODY);
 }
 
+static int rev_opt_abbrev(const struct option *opt,
+			  const char *optarg, int unset)
+{
+	struct rev_info *revs = opt->value;
+
+	if (unset) {
+		revs->abbrev = 0;
+	} else if (!optarg) {
+		revs->abbrev = DEFAULT_ABBREV;
+	} else {
+		const unsigned hexsz = the_hash_algo->hexsz;
+
+		revs->abbrev = strtoul(optarg, NULL, 10);
+		if (revs->abbrev < MINIMUM_ABBREV)
+			revs->abbrev = MINIMUM_ABBREV;
+		else if (revs->abbrev > hexsz)
+			revs->abbrev = hexsz;
+	}
+	return 0;
+}
+
+static void make_rev_options(struct rev_info *revs)
+{
+	struct option options[] = {
+		OPT_CALLBACK_F(0, "abbrev", revs, N_("n"),
+			       N_("show the given source prefix instead of \"a/\""),
+			       PARSE_OPT_OPTARG, rev_opt_abbrev),
+		OPT_END(),
+	};
+	revs->options = parse_options_concat(options, revs->diffopt.parseopts);
+}
+
 static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,
 			       int *unkc, const char **unkv,
 			       const struct setup_revision_opt* opt)
@@ -1960,7 +1994,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	const char *arg = argv[0];
 	const char *optarg;
 	int argcount;
-	const unsigned hexsz = the_hash_algo->hexsz;
 
 	/* pseudo revision arguments */
 	if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
@@ -1977,6 +2010,18 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		return 1;
 	}
 
+	revs->pseudo_flags = NULL;
+	revs->pseudo_refs = NULL;
+	argc = parse_options(argc, argv, revs->prefix,
+			     revs->options, 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 (argc)
+		return argc;
+
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
 		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
@@ -2243,16 +2288,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->no_commit_id = 1;
 	} else if (!strcmp(arg, "--always")) {
 		revs->always_show_header = 1;
-	} else if (!strcmp(arg, "--no-abbrev")) {
-		revs->abbrev = 0;
-	} else if (!strcmp(arg, "--abbrev")) {
-		revs->abbrev = DEFAULT_ABBREV;
-	} else if (skip_prefix(arg, "--abbrev=", &optarg)) {
-		revs->abbrev = strtoul(optarg, NULL, 10);
-		if (revs->abbrev < MINIMUM_ABBREV)
-			revs->abbrev = MINIMUM_ABBREV;
-		else if (revs->abbrev > hexsz)
-			revs->abbrev = hexsz;
 	} else if (!strcmp(arg, "--abbrev-commit")) {
 		revs->abbrev_commit = 1;
 		revs->abbrev_commit_given = 1;
@@ -2324,10 +2359,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			BUG("exclude_promisor_objects can only be used when fetch_if_missing is 0");
 		revs->exclude_promisor_objects = 1;
 	} else {
-		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
-		if (!opts)
-			unkv[(*unkc)++] = arg;
-		return opts;
+		unkv[(*unkc)++] = arg;
+		return 0;
 	}
 	if (revs->graph && revs->track_linear)
 		die("--show-linear-break and --graph are incompatible");
@@ -2861,6 +2894,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
 
 	FREE_AND_NULL(revs->pseudo_options);
+	FREE_AND_NULL(revs->options);
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index cec5215c04..4e840c8eb1 100644
--- a/revision.h
+++ b/revision.h
@@ -283,6 +283,7 @@ struct rev_info {
 	struct option *pseudo_options;
 	int *pseudo_flags;
 	struct ref_store *pseudo_refs;
+	struct option *options;
 };
 
 int ref_excluded(struct string_list *, const char *path);
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 17/19] rev-parseopt: convert --max-count

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

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

diff --git a/revision.c b/revision.c
index f15aa3e62d..c53347d362 100644
--- a/revision.c
+++ b/revision.c
@@ -1976,12 +1976,26 @@ static int rev_opt_abbrev(const struct option *opt,
 	return 0;
 }
 
+static int rev_opt_max_count(const struct option *opt,
+			     const char *arg, int unset)
+{
+	struct rev_info *revs  = opt->value;
+
+	BUG_ON_OPT_NEG(unset);
+	revs->max_count = atoi(arg);
+	revs->no_walk = 0;
+	return 0;
+}
+
 static void make_rev_options(struct rev_info *revs)
 {
 	struct option options[] = {
 		OPT_CALLBACK_F(0, "abbrev", revs, N_("n"),
 			       N_("show the given source prefix instead of \"a/\""),
 			       PARSE_OPT_OPTARG, rev_opt_abbrev),
+		OPT_REV('n', "max-count", N_("<n>"),
+			N_("limit the number of commis to stdout"),
+			rev_opt_max_count),
 		OPT_END(),
 	};
 	revs->options = parse_options_concat(options, revs->diffopt.parseopts);
@@ -2022,11 +2036,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	if (argc)
 		return argc;
 
-	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
-		revs->max_count = atoi(optarg);
-		revs->no_walk = 0;
-		return argcount;
-	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+	if ((argcount = parse_long_opt("skip", argv, &optarg))) {
 		revs->skip_count = atoi(optarg);
 		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
@@ -2035,15 +2045,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		    revs->max_count < 0)
 			die("'%s': not a non-negative integer", arg + 1);
 		revs->no_walk = 0;
-	} else if (!strcmp(arg, "-n")) {
-		if (argc <= 1)
-			return error("-n requires an argument");
-		revs->max_count = atoi(argv[1]);
-		revs->no_walk = 0;
-		return 2;
-	} else if (skip_prefix(arg, "-n", &optarg)) {
-		revs->max_count = atoi(optarg);
-		revs->no_walk = 0;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
 		return argcount;
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 18/19] rev-parseopt: convert --skip

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

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

diff --git a/revision.c b/revision.c
index c53347d362..42d466cd08 100644
--- a/revision.c
+++ b/revision.c
@@ -1996,6 +1996,9 @@ static void make_rev_options(struct rev_info *revs)
 		OPT_REV('n', "max-count", N_("<n>"),
 			N_("limit the number of commis to stdout"),
 			rev_opt_max_count),
+		OPT_INTEGER_F(0, "skip", &revs->skip_count,
+			      N_("skip a number of commits before starting to show"),
+			      PARSE_OPT_NONEG),
 		OPT_END(),
 	};
 	revs->options = parse_options_concat(options, revs->diffopt.parseopts);
@@ -2036,10 +2039,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	if (argc)
 		return argc;
 
-	if ((argcount = parse_long_opt("skip", argv, &optarg))) {
-		revs->skip_count = atoi(optarg);
-		return argcount;
-	} else if ((*arg == '-') && isdigit(arg[1])) {
+	if ((*arg == '-') && isdigit(arg[1])) {
 		/* accept -<digit>, like traditional "head" */
 		if (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||
 		    revs->max_count < 0)
-- 
2.21.0.1141.gd54ac2cb17

[PATCH 19/19] rev-parseopt: convert --min-age and --max-age

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 parse-options-cb.c |  8 ++++++++
 parse-options.h    |  4 ++++
 revision.c         | 10 ++++------
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 6e2e8d6273..7cdbbf5f6d 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -39,6 +39,14 @@ int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
 	return 0;
 }
 
+int parse_opt_timestamp_cb(const struct option *opt,
+			   const char *arg, int unset)
+{
+	BUG_ON_OPT_NEG(unset);
+	*(timestamp_t *)opt->value = atoi(arg);
+	return 0;
+}
+
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
 			    int unset)
 {
diff --git a/parse-options.h b/parse-options.h
index cc9230adac..7637864c41 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -168,6 +168,9 @@ struct option {
 #define OPT_EXPIRY_DATE(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0,	\
 	  parse_opt_expiry_date_cb }
+#define OPT_TIMESTAMP(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), N_("timestamp"),(h), \
+	  PARSE_OPT_NONEG, parse_opt_timestamp_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) OPT_CALLBACK_F(s, l, v, a, h, 0, f)
 #define OPT_NUMBER_CALLBACK(v, h, f) \
 	{ OPTION_NUMBER, 0, NULL, (v), NULL, (h), \
@@ -275,6 +278,7 @@ struct option *parse_options_concat(struct option *a, struct option *b);
 /*----- some often used options -----*/
 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_timestamp_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);
diff --git a/revision.c b/revision.c
index 42d466cd08..0c28b67978 100644
--- a/revision.c
+++ b/revision.c
@@ -1999,6 +1999,10 @@ static void make_rev_options(struct rev_info *revs)
 		OPT_INTEGER_F(0, "skip", &revs->skip_count,
 			      N_("skip a number of commits before starting to show"),
 			      PARSE_OPT_NONEG),
+		OPT_TIMESTAMP(0, "min-age", &revs->min_age,
+			      N_("limit the commits output to a specified time range")),
+		OPT_TIMESTAMP(0, "max-age", &revs->max_age,
+			      N_("limit the commits output to a specified time range")),
 		OPT_END(),
 	};
 	revs->options = parse_options_concat(options, revs->diffopt.parseopts);
@@ -2045,18 +2049,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		    revs->max_count < 0)
 			die("'%s': not a non-negative integer", arg + 1);
 		revs->no_walk = 0;
-	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
-		revs->max_age = atoi(optarg);
-		return argcount;
 	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
 		revs->max_age = approxidate(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("after", argv, &optarg))) {
 		revs->max_age = approxidate(optarg);
 		return argcount;
-	} else if ((argcount = parse_long_opt("min-age", argv, &optarg))) {
-		revs->min_age = atoi(optarg);
-		return argcount;
 	} else if ((argcount = parse_long_opt("before", argv, &optarg))) {
 		revs->min_age = approxidate(optarg);
 		return argcount;
-- 
2.21.0.1141.gd54ac2cb17

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[Derrick Stolee]

On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote:
> 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 rev_info variables
> around, memory consumption is not at all a concern.

I think you are right that this memory trade-off shouldn't be a problem.

What worries me instead is that we are using an "internal" data structure
for option parsing. Would it make more sense to create a struct for use
in the parse_opts method and a method that translates those options into
the bitfield in struct rev_info?

Thanks,
-Stolee

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[Duy Nguyen]

On Wed, May 8, 2019 at 9:07 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote:
> > 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 rev_info variables
> > around, memory consumption is not at all a concern.
>
> I think you are right that this memory trade-off shouldn't be a problem.
>
> What worries me instead is that we are using an "internal" data structure
> for option parsing. Would it make more sense to create a struct for use
> in the parse_opts method and a method that translates those options into
> the bitfield in struct rev_info?

But we are doing that now (option parsing) using the same data
structure. Why would changing from a custom parser to parse_options()
affect what fields it should or should not touch in rev_info? Genuine
question. Maybe you could elaborate more about "internal". I probably
missed something. Or maybe this is a good opportunity to separate
intermediate option parsing variables from the rest of rev_info?
-- 
Duy

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[Derrick Stolee]

On 5/8/2019 10:41 AM, Duy Nguyen wrote:
> On Wed, May 8, 2019 at 9:07 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote:
>>> 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 rev_info variables
>>> around, memory consumption is not at all a concern.
>>
>> I think you are right that this memory trade-off shouldn't be a problem.
>>
>> What worries me instead is that we are using an "internal" data structure
>> for option parsing. Would it make more sense to create a struct for use
>> in the parse_opts method and a method that translates those options into
>> the bitfield in struct rev_info?
> 
> But we are doing that now (option parsing) using the same data
> structure. Why would changing from a custom parser to parse_options()
> affect what fields it should or should not touch in rev_info? Genuine
> question. Maybe you could elaborate more about "internal". I probably
> missed something. Or maybe this is a good opportunity to separate
> intermediate option parsing variables from the rest of rev_info?

You're right. I was unclear.

rev_info stores a lot of data. Some of the fields are important
in-memory structures that are crucial to the workings of revision.c
and are never used by builtin/rev-list.c. Combining this purpose
with the option parsing seems smelly to me.

Thinking more on it, I would prefer a more invasive change that may
pay off in the long term. These options, along with the "starting list"
values, could be extracted to a 'struct rev_options' that contains these
integers and the commit list. Then your option parsing changes could be
limited to a rev_options struct, which is later inserted into a rev_info
struct during setup_revisions().

Generally, the rev_info struct has too many members and could be split
into smaller pieces according to purpose. I created the topo_walk_info
struct as a way to not make the situation worse, but doesn't fix existing
pain.

My ramblings are mostly complaining about old code that grew organically
across many many quality additions. It is definitely hard to understand
the revision-walking code, and perhaps it would be easier to understand
with a little more structure.

The biggest issue with my suggestion is that it requires changing the
consumers of the options, as they would no longer live directly on the
rev_info struct. That would be a big change, even if it could be done
with string replacement.

Thanks,
-Stolee 

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[Duy Nguyen]

On Wed, May 8, 2019 at 10:52 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 5/8/2019 10:41 AM, Duy Nguyen wrote:
> > On Wed, May 8, 2019 at 9:07 PM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote:
> >>> 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 rev_info variables
> >>> around, memory consumption is not at all a concern.
> >>
> >> I think you are right that this memory trade-off shouldn't be a problem.
> >>
> >> What worries me instead is that we are using an "internal" data structure
> >> for option parsing. Would it make more sense to create a struct for use
> >> in the parse_opts method and a method that translates those options into
> >> the bitfield in struct rev_info?
> >
> > But we are doing that now (option parsing) using the same data
> > structure. Why would changing from a custom parser to parse_options()
> > affect what fields it should or should not touch in rev_info? Genuine
> > question. Maybe you could elaborate more about "internal". I probably
> > missed something. Or maybe this is a good opportunity to separate
> > intermediate option parsing variables from the rest of rev_info?
>
> You're right. I was unclear.
>
> rev_info stores a lot of data. Some of the fields are important
> in-memory structures that are crucial to the workings of revision.c
> and are never used by builtin/rev-list.c. Combining this purpose
> with the option parsing seems smelly to me.
>
> Thinking more on it, I would prefer a more invasive change that may
> pay off in the long term. These options, along with the "starting list"
> values, could be extracted to a 'struct rev_options' that contains these
> integers and the commit list. Then your option parsing changes could be
> limited to a rev_options struct, which is later inserted into a rev_info
> struct during setup_revisions().
>
> Generally, the rev_info struct has too many members and could be split
> into smaller pieces according to purpose. I created the topo_walk_info
> struct as a way to not make the situation worse, but doesn't fix existing
> pain.
>
> My ramblings are mostly complaining about old code that grew organically
> across many many quality additions. It is definitely hard to understand
> the revision-walking code, and perhaps it would be easier to understand
> with a little more structure.

Thanks. This is much better.

> The biggest issue with my suggestion is that it requires changing the
> consumers of the options, as they would no longer live directly on the
> rev_info struct. That would be a big change, even if it could be done
> with string replacement.

I agree rev_info has grown "wild". It's quite ancient code. As you
noted, it's a big change. And since my series is already long (76
patches), I would rather focus on just one thing, rewriting the
parsing code with minimum changes to anything else, preferably retain
the exact same old behavior.

After that work is done (and no regression found), we could focus on
reorganizing rev_info, which could be quite "interesting". Some fields
may be overloaded with different purposes, which I just can't spend
time investigating now. There's also the problem with freeing
resources after rev-list is done, which I think we have ignored so
far.
-- 
Duy

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info

[Derrick Stolee]

On 5/9/2019 5:56 AM, Duy Nguyen wrote:
> On Wed, May 8, 2019 at 10:52 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> The biggest issue with my suggestion is that it requires changing the
>> consumers of the options, as they would no longer live directly on the
>> rev_info struct. That would be a big change, even if it could be done
>> with string replacement.
> 
> I agree rev_info has grown "wild". It's quite ancient code. As you
> noted, it's a big change. And since my series is already long (76
> patches), I would rather focus on just one thing, rewriting the
> parsing code with minimum changes to anything else, preferably retain
> the exact same old behavior.
> 
> After that work is done (and no regression found), we could focus on
> reorganizing rev_info, which could be quite "interesting". Some fields
> may be overloaded with different purposes, which I just can't spend
> time investigating now. There's also the problem with freeing
> resources after rev-list is done, which I think we have ignored so
> far.

Thanks for humoring me. I agree with your reasoning.

This series looks good to me.

Thanks,
-Stolee