[PATCH] revisions: refactor init_revisions and setup_revisions.

[PATCH] revisions: refactor init_revisions and setup_revisions.

[Pierre Habouzit]

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

struct rev_info gains two new field:
* .def to store --default argument;
* .show_merge 1-bit field.

The refactor consists into the following steps:
* init_revisions now takes the --default argument to initialize .def
  properly, instead of getting it through setup_revisions.
* setup_revisions has been split in two: parse_revisions that does (almost)
  only argument parsing, to be more like what parse-options can do, and
  setup_revisions that does the rest.

Many places had no arguments to pass to setup_revisions, and those don't use
parse_revisions at all.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  This is a required patch for revisions parse-optification that splits
  the logic of revision arguments parsing in parsing, and post-processing.

  The aim is to replace parsing with parse-opt at some point.

  The final version (using parse-options) will probably need more rework
  in revisions.[hc], but I'd like to avoid rebasing this patch over and
  over, and I'd be glad if it's merged now, as it's not _that_
  intrusive.

  I've been using a git with this patch for a week without issues.


 builtin-add.c           |    4 ++--
 builtin-blame.c         |    5 +++--
 builtin-checkout.c      |    7 ++++---
 builtin-commit.c        |    8 ++++----
 builtin-diff-files.c    |    8 +++++---
 builtin-diff-index.c    |    5 +++--
 builtin-diff-tree.c     |    5 +++--
 builtin-diff.c          |    8 +++++---
 builtin-fast-export.c   |    5 +++--
 builtin-fmt-merge-msg.c |    4 ++--
 builtin-log.c           |   20 +++++++++++---------
 builtin-pack-objects.c  |    5 +++--
 builtin-prune.c         |    2 +-
 builtin-reflog.c        |    2 +-
 builtin-rev-list.c      |    5 +++--
 builtin-revert.c        |    4 ++--
 builtin-shortlog.c      |    5 +++--
 bundle.c                |   10 ++++++----
 diff-lib.c              |    2 +-
 http-push.c             |    5 +++--
 revision.c              |   36 +++++++++++++++++++-----------------
 revision.h              |    7 +++++--
 upload-pack.c           |    6 +++---
 wt-status.c             |   12 ++++++------
 24 files changed, 101 insertions(+), 79 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 820110e..3757369 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -108,8 +108,8 @@ static void update_callback(struct diff_queue_struct *q,
 void add_files_to_cache(int verbose, const char *prefix, const char **pathspec)
 {
 	struct rev_info rev;
-	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
+	init_revisions(&rev, prefix, NULL);
+	setup_revisions(&rev);
 	rev.prune_data = pathspec;
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
diff --git a/builtin-blame.c b/builtin-blame.c
index bfd562d..ac5c5eb 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2323,8 +2323,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	argv[unk++] = "--"; /* terminate the rev name */
 	argv[unk] = NULL;
 
-	init_revisions(&revs, NULL);
-	setup_revisions(unk, argv, &revs, NULL);
+	init_revisions(&revs, NULL, NULL);
+	parse_revisions(unk, argv, &revs);
+	setup_revisions(&revs);
 	memset(&sb, 0, sizeof(sb));
 
 	/*
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 6b08016..ff92ef6 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -130,7 +130,7 @@ static void show_local_changes(struct object *head)
 {
 	struct rev_info rev;
 	/* I think we want full paths, even if we're in a subdirectory. */
-	init_revisions(&rev, NULL);
+	init_revisions(&rev, NULL, NULL);
 	rev.abbrev = 0;
 	rev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;
 	add_pending_object(&rev, head, NULL);
@@ -345,8 +345,9 @@ static void report_tracking(struct branch_info *new, struct checkout_opts *opts)
 	strcpy(symmetric + 40, "...");
 	strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));
 
-	init_revisions(&revs, NULL);
-	setup_revisions(rev_argc, rev_argv, &revs, NULL);
+	init_revisions(&revs, NULL, NULL);
+	parse_revisions(rev_argc, rev_argv, &revs);
+	setup_revisions(&revs);
 	prepare_revision_walk(&revs);
 
 	/* ... and count the commits on each side. */
diff --git a/builtin-commit.c b/builtin-commit.c
index f49c22e..48dbcaa 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -522,9 +522,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 		if (get_sha1(parent, sha1))
 			commitable = !!active_nr;
 		else {
-			init_revisions(&rev, "");
+			init_revisions(&rev, "", parent);
 			rev.abbrev = 0;
-			setup_revisions(0, NULL, &rev, parent);
+			setup_revisions(&rev);
 			DIFF_OPT_SET(&rev.diffopt, QUIET);
 			DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 			run_diff_index(&rev, 1 /* cached */);
@@ -798,8 +798,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	if (!commit || parse_commit(commit))
 		die("could not parse newly created commit");
 
-	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
+	init_revisions(&rev, prefix, NULL);
+	setup_revisions(&rev);
 
 	rev.abbrev = 0;
 	rev.diff = 1;
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 4abe3c2..1f6f123 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -20,14 +20,16 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 
 	prefix = setup_git_directory_gently(&nongit);
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, NULL);
 	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
 		argc = 0;
-	else
-		argc = setup_revisions(argc, argv, &rev, NULL);
+	else {
+		argc = parse_revisions(argc, argv, &rev);
+		setup_revisions(&rev);
+	}
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	result = run_diff_files_cmd(&rev, argc, argv);
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 2b955de..43dc3bc 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -16,11 +16,12 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, NULL);
 	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 832797f..3b03d7a 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -67,12 +67,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	static struct rev_info *opt = &log_tree_opt;
 	int read_stdin = 0;
 
-	init_revisions(opt, prefix);
+	init_revisions(opt, prefix, NULL);
 	git_config(git_diff_basic_config); /* no "diff" UI options */
 	nr_sha1 = 0;
 	opt->abbrev = 0;
 	opt->diff = 1;
-	argc = setup_revisions(argc, argv, opt, NULL);
+	argc = parse_revisions(argc, argv, opt);
+	setup_revisions(opt);
 
 	while (--argc > 0) {
 		const char *arg = *++argv;
diff --git a/builtin-diff.c b/builtin-diff.c
index 444ff2f..19f4111 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -239,13 +239,15 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, NULL);
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
 	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
 		argc = 0;
-	else
-		argc = setup_revisions(argc, argv, &rev, NULL);
+	else {
+		argc = parse_revisions(argc, argv, &rev);
+		setup_revisions(&rev);
+	}
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 		if (diff_setup_done(&rev.diffopt) < 0)
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index e1c5630..4b788e5 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -374,8 +374,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	/* we handle encodings */
 	git_config(git_default_config);
 
-	init_revisions(&revs, prefix);
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	init_revisions(&revs, prefix, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs);
 	argc = parse_options(argc, argv, options, fast_export_usage, 0);
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index ebb3f37..9b84482 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -182,7 +182,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	if (!branch || branch->type != OBJ_COMMIT)
 		return;
 
-	setup_revisions(0, NULL, rev, NULL);
+	setup_revisions(rev);
 	rev->ignore_merges = 1;
 	add_pending_object(rev, branch, name);
 	add_pending_object(rev, &head->object, "^HEAD");
@@ -339,7 +339,7 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 		struct rev_info rev;
 
 		head = lookup_commit(head_sha1);
-		init_revisions(&rev, prefix);
+		init_revisions(&rev, prefix, NULL);
 		rev.commit_format = CMIT_FMT_ONELINE;
 		rev.ignore_merges = 1;
 		rev.limited = 1;
diff --git a/builtin-log.c b/builtin-log.c
index fe8fc6f..30d13d3 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -58,7 +58,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
-	argc = setup_revisions(argc, argv, rev, "HEAD");
+	argc = parse_revisions(argc, argv, rev);
+	setup_revisions(rev);
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;
 	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
@@ -243,7 +244,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, "HEAD");
 	rev.diff = 1;
 	rev.simplify_history = 0;
 	cmd_log_init(argc, argv, prefix, &rev);
@@ -319,7 +320,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, "HEAD");
 	rev.diff = 1;
 	rev.combine_merges = 1;
 	rev.dense_combined_merges = 1;
@@ -383,7 +384,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, "HEAD");
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
 	rev.verbose_header = 1;
@@ -415,7 +416,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, "HEAD");
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, prefix, &rev);
 	return cmd_log_walk(&rev);
@@ -601,7 +602,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids, const cha
 	init_patch_ids(ids);
 
 	/* given a range a..b get all patch ids for b..a */
-	init_revisions(&check_rev, prefix);
+	init_revisions(&check_rev, prefix, NULL);
 	o1->flags ^= UNINTERESTING;
 	o2->flags ^= UNINTERESTING;
 	add_pending_object(&check_rev, o1, "o1");
@@ -756,7 +757,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf buf;
 
 	git_config(git_format_config);
-	init_revisions(&rev, prefix);
+	init_revisions(&rev, prefix, "HEAD");
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
 	rev.diff = 1;
@@ -900,7 +901,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (numbered_files && use_stdout)
 		die ("--numbered-files and --stdout are mutually exclusive.");
 
-	argc = setup_revisions(argc, argv, &rev, "HEAD");
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev);
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
@@ -1095,7 +1097,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		usage(cherry_usage);
 	}
 
-	init_revisions(&revs, prefix);
+	init_revisions(&revs, prefix, NULL);
 	revs.diff = 1;
 	revs.combine_merges = 0;
 	revs.ignore_merges = 1;
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2799e68..94d0adc 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1871,9 +1871,10 @@ static void get_object_list(int ac, const char **av)
 	char line[1000];
 	int flags = 0;
 
-	init_revisions(&revs, NULL);
+	init_revisions(&revs, NULL, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, NULL);
+	parse_revisions(ac, av, &revs);
+	setup_revisions(&revs);
 
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
diff --git a/builtin-prune.c b/builtin-prune.c
index bb8ead9..c18f8c2 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -146,7 +146,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	}
 
 	save_commit_buffer = 0;
-	init_revisions(&revs, prefix);
+	init_revisions(&revs, prefix, NULL);
 	mark_reachable_objects(&revs, 1);
 
 	prune_object_dir(get_object_directory());
diff --git a/builtin-reflog.c b/builtin-reflog.c
index 280e24e..133f6f3 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -399,7 +399,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			break;
 	}
 	if (cb.stalefix) {
-		init_revisions(&cb.revs, prefix);
+		init_revisions(&cb.revs, prefix, NULL);
 		if (cb.verbose)
 			printf("Marking reachable objects...");
 		mark_reachable_objects(&cb.revs, 0);
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index d0a1416..2a02b07 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -546,10 +546,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	int quiet = 0;
 
 	git_config(git_default_config);
-	init_revisions(&revs, prefix);
+	init_revisions(&revs, prefix, NULL);
 	revs.abbrev = 0;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs);
 
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin-revert.c b/builtin-revert.c
index 607a2f0..e538dc1 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -251,8 +251,8 @@ static char *help_msg(const unsigned char *sha1)
 static int index_is_dirty(void)
 {
 	struct rev_info rev;
-	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, "HEAD");
+	init_revisions(&rev, NULL, "HEAD");
+	setup_revisions(&rev);
 	DIFF_OPT_SET(&rev.diffopt, QUIET);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	run_diff_index(&rev, 1);
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index af31aba..2966bdd 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -252,8 +252,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		argv++;
 		argc--;
 	}
-	init_revisions(&rev, prefix);
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	init_revisions(&rev, prefix, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev);
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
diff --git a/bundle.c b/bundle.c
index 0ba5df1..24d5009 100644
--- a/bundle.c
+++ b/bundle.c
@@ -104,7 +104,7 @@ int verify_bundle(struct bundle_header *header, int verbose)
 	int i, ret = 0, req_nr;
 	const char *message = "Repository lacks these prerequisite commits:";
 
-	init_revisions(&revs, NULL);
+	init_revisions(&revs, NULL, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
 		struct object *o = parse_object(e->sha1);
@@ -120,7 +120,8 @@ int verify_bundle(struct bundle_header *header, int verbose)
 	if (revs.pending.nr != p->nr)
 		return ret;
 	req_nr = revs.pending.nr;
-	setup_revisions(2, argv, &revs, NULL);
+	parse_revisions(2, argv, &revs);
+	setup_revisions(&revs);
 
 	memset(&refs, 0, sizeof(struct object_array));
 	for (i = 0; i < revs.pending.nr; i++) {
@@ -192,7 +193,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* init revs to list objects for pack-objects later */
 	save_commit_buffer = 0;
-	init_revisions(&revs, NULL);
+	init_revisions(&revs, NULL, NULL);
 
 	/* write prerequisites */
 	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
@@ -226,7 +227,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 		return error("rev-list died");
 
 	/* write references */
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs);
 	if (argc > 1)
 		return error("unrecognized argument: %s'", argv[1]);
 
diff --git a/diff-lib.c b/diff-lib.c
index 4581b59..76e1ce2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -775,7 +775,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	}
 	active_nr = dst - active_cache;
 
-	init_revisions(&revs, NULL);
+	init_revisions(&revs, NULL, NULL);
 	revs.prune_data = opt->paths;
 	tree = parse_tree_indirect(tree_sha1);
 	if (!tree)
diff --git a/http-push.c b/http-push.c
index 5b23038..65d9f5b 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2382,8 +2382,9 @@ int main(int argc, char **argv)
 			commit_argv[3] = old_sha1_hex;
 			commit_argc++;
 		}
-		init_revisions(&revs, setup_git_directory());
-		setup_revisions(commit_argc, commit_argv, &revs, NULL);
+		init_revisions(&revs, setup_git_directory(), NULL);
+		parse_revisions(commit_argc, commit_argv, &revs);
+		setup_revisions(&revs);
 		free(new_sha1_hex);
 		if (old_sha1_hex) {
 			free(old_sha1_hex);
diff --git a/revision.c b/revision.c
index 63bf2c5..bab6228 100644
--- a/revision.c
+++ b/revision.c
@@ -717,7 +717,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)
 	return 1;
 }
 
-void init_revisions(struct rev_info *revs, const char *prefix)
+void init_revisions(struct rev_info *revs, const char *prefix, const char *def)
 {
 	memset(revs, 0, sizeof(*revs));
 
@@ -731,6 +731,7 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->lifo = 1;
 	revs->dense = 1;
 	revs->prefix = prefix;
+	revs->def = def;
 	revs->max_age = -1;
 	revs->min_age = -1;
 	revs->skip_count = -1;
@@ -892,8 +893,7 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
 		opt->regflags = REG_NEWLINE;
 		revs->grep_filter = opt;
 	}
-	append_grep_pattern(revs->grep_filter, ptn,
-			    "command line", 0, what);
+	append_grep_pattern(revs->grep_filter, ptn, "command line", 0, what);
 }
 
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
@@ -936,9 +936,9 @@ static void add_ignore_packed(struct rev_info *revs, const char *name)
  * Returns the number of arguments left that weren't recognized
  * (which are also moved to the head of the argument list)
  */
-int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)
+int parse_revisions(int argc, const char **argv, struct rev_info *revs)
 {
-	int i, flags, seen_dashdash, show_merge;
+	int i, flags, seen_dashdash;
 	const char **unrecognized = argv + 1;
 	int left = 1;
 	int all_match = 0;
@@ -959,7 +959,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		break;
 	}
 
-	flags = show_merge = 0;
+	flags = 0;
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (*arg == '-') {
@@ -1047,11 +1047,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 			if (!strcmp(arg, "--default")) {
 				if (++i >= argc)
 					die("bad --default argument");
-				def = argv[i];
+				revs->def = argv[i];
 				continue;
 			}
 			if (!strcmp(arg, "--merge")) {
-				show_merge = 1;
+				revs->show_merge = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--topo-order")) {
@@ -1315,18 +1315,23 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (revs->grep_filter) {
 		revs->grep_filter->regflags |= regflags;
 		revs->grep_filter->fixed = fixed;
+		revs->grep_filter->all_match = all_match;
 	}
+	return left;
+}
 
-	if (show_merge)
+void setup_revisions(struct rev_info *revs)
+{
+	if (revs->show_merge)
 		prepare_show_merge(revs);
-	if (def && !revs->pending.nr) {
+	if (revs->def && !revs->pending.nr) {
 		unsigned char sha1[20];
 		struct object *object;
 		unsigned mode;
-		if (get_sha1_with_mode(def, sha1, &mode))
-			die("bad default revision '%s'", def);
-		object = get_reference(revs, def, sha1, 0);
-		add_pending_object_with_mode(revs, object, def, mode);
+		if (get_sha1_with_mode(revs->def, sha1, &mode))
+			die("bad default revision '%s'", revs->def);
+		object = get_reference(revs, revs->def, sha1, 0);
+		add_pending_object_with_mode(revs, object, revs->def, mode);
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
@@ -1360,14 +1365,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		die("diff_setup_done failed");
 
 	if (revs->grep_filter) {
-		revs->grep_filter->all_match = all_match;
 		compile_grep_patterns(revs->grep_filter);
 	}
 
 	if (revs->reverse && revs->reflog_info)
 		die("cannot combine --reverse with --walk-reflogs");
-
-	return left;
 }
 
 int prepare_revision_walk(struct rev_info *revs)
diff --git a/revision.h b/revision.h
index c8b3b94..9f727d7 100644
--- a/revision.h
+++ b/revision.h
@@ -25,6 +25,7 @@ struct rev_info {
 
 	/* Basic information */
 	const char *prefix;
+	const char *def;
 	void *prune_data;
 	unsigned int early_output;
 
@@ -64,6 +65,7 @@ struct rev_info {
 
 	/* Format info */
 	unsigned int	shown_one:1,
+			show_merge:1,
 			abbrev_commit:1;
 	enum date_mode date_mode;
 
@@ -108,8 +110,9 @@ struct rev_info {
 typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
 volatile show_early_output_fn_t show_early_output;
 
-extern void init_revisions(struct rev_info *revs, const char *prefix);
-extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);
+extern void init_revisions(struct rev_info *revs, const char *prefix, const char *def);
+extern int parse_revisions(int argc, const char **argv, struct rev_info *revs);
+extern void setup_revisions(struct rev_info *revs);
 extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
 
 extern int prepare_revision_walk(struct rev_info *revs);
diff --git a/upload-pack.c b/upload-pack.c
index 660134a..8adf0f8 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -106,7 +106,7 @@ static int do_rev_list(int fd, void *create_full_pack)
 	pack_pipe = fdopen(fd, "w");
 	if (create_full_pack)
 		use_thin_pack = 0; /* no point doing it */
-	init_revisions(&revs, NULL);
+	init_revisions(&revs, NULL, NULL);
 	revs.tag_objects = 1;
 	revs.tree_objects = 1;
 	revs.blob_objects = 1;
@@ -115,7 +115,7 @@ static int do_rev_list(int fd, void *create_full_pack)
 
 	if (create_full_pack) {
 		const char *args[] = {"rev-list", "--all", NULL};
-		setup_revisions(2, args, &revs, NULL);
+		parse_revisions(2, args, &revs);
 	} else {
 		for (i = 0; i < want_obj.nr; i++) {
 			struct object *o = want_obj.objects[i].item;
@@ -128,8 +128,8 @@ static int do_rev_list(int fd, void *create_full_pack)
 			o->flags |= UNINTERESTING;
 			add_pending_object(&revs, o, NULL);
 		}
-		setup_revisions(0, NULL, &revs, NULL);
 	}
+	setup_revisions(&revs);
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
diff --git a/wt-status.c b/wt-status.c
index 32d780a..bf3df7c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -241,8 +241,8 @@ static void wt_status_print_initial(struct wt_status *s)
 static void wt_status_print_updated(struct wt_status *s)
 {
 	struct rev_info rev;
-	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, s->reference);
+	init_revisions(&rev, NULL, s->reference);
+	setup_revisions(&rev);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_print_updated_cb;
 	rev.diffopt.format_callback_data = s;
@@ -255,8 +255,8 @@ static void wt_status_print_updated(struct wt_status *s)
 static void wt_status_print_changed(struct wt_status *s)
 {
 	struct rev_info rev;
-	init_revisions(&rev, "");
-	setup_revisions(0, NULL, &rev, NULL);
+	init_revisions(&rev, "", NULL);
+	setup_revisions(&rev);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_print_changed_cb;
 	rev.diffopt.format_callback_data = s;
@@ -322,8 +322,8 @@ static void wt_status_print_verbose(struct wt_status *s)
 	if (saved_stdout < 0 ||dup2(fileno(s->fp), STDOUT_FILENO) < 0)
 		die("couldn't redirect stdout\n");
 
-	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, s->reference);
+	init_revisions(&rev, NULL, s->reference);
+	setup_revisions(&rev);
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 	rev.diffopt.detect_rename = 1;
 	run_diff_index(&rev, 1);
-- 
1.5.4.3.534.g7c43.dirty


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

    struct rev_info gains two new fields:
    
     * .def to store --default argument;
     * .show_merge 1-bit field.
    
    The refactor consists of the following steps:
    
     * init_revisions now takes the --default argument to initialize .def
       properly, instead of getting it through setup_revisions.
    
     * setup_revisions has been split in two:
    
       - parse_revisions that does (almost) only argument parsing, to be more
         like what parse-options can do, and
    
       - setup_revisions that does the rest.
    
    Many places had no arguments to pass to setup_revisions, and those don't
    use parse_revisions at all.

Conceptually, I like the split up into three, which are:

 - initializes the structure (init_revisions);

 - parses the user arguments (parse_revisions -- the naming could probably
   be improved, though);

 - prepares the derived/derivable fields for the real work
   (setup_revisions).

Was there a particular reason you moved default to init_revisions() and
not kept it with setup_revisions()?  All the callers of cmd_log_init()
needs to say "I want to default to HEAD" because of it, instead of letting
cmd_log_init() say it at only one place.  Also the caller needs to decide
upfront before calling init_revisions() what the default should be.

> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
>   This is a required patch for revisions parse-optification that splits
>   the logic of revision arguments parsing in parsing, and post-processing.
>
>   The aim is to replace parsing with parse-opt at some point.
>
>   The final version (using parse-options) will probably need more rework
>   in revisions.[hc], but I'd like to avoid rebasing this patch over and
>   over, and I'd be glad if it's merged now, as it's not _that_
>   intrusive.

>   I've been using a git with this patch for a week without issues.

... which is a nice assuring comment ;-)

> @@ -892,8 +893,7 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
>  		opt->regflags = REG_NEWLINE;
>  		revs->grep_filter = opt;
>  	}
> -	append_grep_pattern(revs->grep_filter, ptn,
> -			    "command line", 0, what);
> +	append_grep_pattern(revs->grep_filter, ptn, "command line", 0, what);
>  }

Made me go "huh?".

But everything else looked pretty mechanical conversion, and in that sense
it is not that intrusive.

Applying this to 'master' and then merging 'pu' shows that there are a few
topics that are cooking that would conflict with this change.  Merging
'next' seems to go cleanly (I haven't checked the result), so it is not
too bad for me to carrry this at this moment, if we were not this close to
the rc freeze.  I dunno.

Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

[Pierre Habouzit]

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

On Mon, Mar 10, 2008 at 07:25:25AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
>     struct rev_info gains two new fields:
>     
>      * .def to store --default argument;
>      * .show_merge 1-bit field.
>     
>     The refactor consists of the following steps:
>     
>      * init_revisions now takes the --default argument to initialize .def
>        properly, instead of getting it through setup_revisions.
>     
>      * setup_revisions has been split in two:
>     
>        - parse_revisions that does (almost) only argument parsing, to be more
>          like what parse-options can do, and
>     
>        - setup_revisions that does the rest.
>     
>     Many places had no arguments to pass to setup_revisions, and those don't
>     use parse_revisions at all.
> 
> Conceptually, I like the split up into three, which are:
> 
>  - initializes the structure (init_revisions);
> 
>  - parses the user arguments (parse_revisions -- the naming could probably
>    be improved, though);

  Well, the goal is to replace that bit with a parse_options
structure/macro at some point, so the naming isn't really important.

>  - prepares the derived/derivable fields for the real work
>    (setup_revisions).
> 
> Was there a particular reason you moved default to init_revisions() and
> not kept it with setup_revisions()?  All the callers of cmd_log_init()
> needs to say "I want to default to HEAD" because of it, instead of letting
> cmd_log_init() say it at only one place.  Also the caller needs to decide
> upfront before calling init_revisions() what the default should be.

  Yes, because the revision parsing has side effects on that default
parameter (the --default command line switch).

  We could of course have a .def member in the struct rev_info, and use
the one passed to setup_revisions then if it's still NULL, but it
doesn't really makes sense to me, and I don't really see a problem with
saying at init time that you'll default to "HEAD". Though if you really
dislike it that much, I squash a patch that does that on top of it.

> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >
> >   This is a required patch for revisions parse-optification that splits
> >   the logic of revision arguments parsing in parsing, and post-processing.
> >
> >   The aim is to replace parsing with parse-opt at some point.
> >
> >   The final version (using parse-options) will probably need more rework
> >   in revisions.[hc], but I'd like to avoid rebasing this patch over and
> >   over, and I'd be glad if it's merged now, as it's not _that_
> >   intrusive.
> 
> >   I've been using a git with this patch for a week without issues.
> 
> .... which is a nice assuring comment ;-)

  Well it has been even more since :)

> > @@ -892,8 +893,7 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
> >  		opt->regflags = REG_NEWLINE;
> >  		revs->grep_filter = opt;
> >  	}
> > -	append_grep_pattern(revs->grep_filter, ptn,
> > -			    "command line", 0, what);
> > +	append_grep_pattern(revs->grep_filter, ptn, "command line", 0, what);
> >  }
> 
> Made me go "huh?".

  Huh yeah, sorry, that's totally spurious indeed.

> But everything else looked pretty mechanical conversion, and in that sense
> it is not that intrusive.
> 
> Applying this to 'master' and then merging 'pu' shows that there are a few
> topics that are cooking that would conflict with this change.  Merging
> 'next' seems to go cleanly (I haven't checked the result), so it is not
> too bad for me to carrry this at this moment, if we were not this close to
> the rc freeze.  I dunno.

  Well I can wait longer, I'd just like to see it merged in a not too
far future, because I have to check for new places that would need
conversions at each reabase :) (though the init/setup_revisions
functions changed their number of arguments so I _think_ I should miss
none each time but …).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   We could of course have a .def member in the struct rev_info, and use
> the one passed to setup_revisions then if it's still NULL, but it
> doesn't really makes sense to me, and I don't really see a problem with
> saying at init time that you'll default to "HEAD". Though if you really
> dislike it that much, I squash a patch that does that on top of it.

Well, it was not liking or disliking.  Although I thought "default" that
sets a value to the default after the parser finds that the user did not
give anything (the approach you described in the above quoted paragraph)
is a natural implementation, probably more so than what you did, I do not
have strong preference either way.

When I see a change where I do not see a reason to, I get suspicious,
wondering if I am missing some bigger reason (e.g. "by moving it there
this and that would become much easier and cleaner, even though it now
forces callers of cmd_log_init() to duplicate the default values").  There
must be an obvious justification you had when you changed it, which I am
not seeing.  Hence that question.

>> Applying this to 'master' and then merging 'pu' shows that there are a few
>> topics that are cooking that would conflict with this change.  Merging
>> 'next' seems to go cleanly (I haven't checked the result), so it is not
>> too bad for me to carrry this at this moment, if we were not this close to
>> the rc freeze.  I dunno.
>
>   Well I can wait longer, I'd just like to see it merged in a not too
> far future, because I have to check for new places that would need
> conversions at each reabase :)

Yeah, that burden can be shifted to me, in other words ;-)

Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

[Pierre Habouzit]

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

On Mon, Mar 10, 2008 at 07:36:20PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   We could of course have a .def member in the struct rev_info, and use
> > the one passed to setup_revisions then if it's still NULL, but it
> > doesn't really makes sense to me, and I don't really see a problem with
> > saying at init time that you'll default to "HEAD". Though if you really
> > dislike it that much, I squash a patch that does that on top of it.
> 
> Well, it was not liking or disliking.  Although I thought "default" that
> sets a value to the default after the parser finds that the user did not
> give anything (the approach you described in the above quoted paragraph)
> is a natural implementation, probably more so than what you did, I do not
> have strong preference either way.

  Well Okay, I'll let others comment, and will implement this way if
more are in favor of it than against then :)

> >> Applying this to 'master' and then merging 'pu' shows that there are a few
> >> topics that are cooking that would conflict with this change.  Merging
> >> 'next' seems to go cleanly (I haven't checked the result), so it is not
> >> too bad for me to carrry this at this moment, if we were not this close to
> >> the rc freeze.  I dunno.
> >
> >   Well I can wait longer, I'd just like to see it merged in a not too
> > far future, because I have to check for new places that would need
> > conversions at each reabase :)
> 
> Yeah, that burden can be shifted to me, in other words ;-)

  Heh, fair enough. Well, I can wait a bit longer :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] revisions: refactor init_revisions and setup_revisions.

[Pierre Habouzit]

struct rev_info gains two new field:
* .def to store --default argument;
* .show_merge 1-bit field.

setup_revisions has been split in two: parse_revisions that does (almost)
only argument parsing, to be more like what parse-options can do, and
setup_revisions that does the rest.

Many places had no arguments to pass to setup_revisions, and those don't use
parse_revisions at all.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  This patch was submitted long time ago, Junio did some remarks I've now
  integrated (wrt the default argument that I passed to init_revisions once,
  and now is back as a setup_revisions argument).

 builtin-add.c           |    2 +-
 builtin-blame.c         |    3 ++-
 builtin-checkout.c      |    3 ++-
 builtin-commit.c        |    4 ++--
 builtin-diff-files.c    |    3 ++-
 builtin-diff-index.c    |    3 ++-
 builtin-diff-tree.c     |    3 ++-
 builtin-diff.c          |    3 ++-
 builtin-fast-export.c   |    3 ++-
 builtin-fmt-merge-msg.c |    2 +-
 builtin-log.c           |    7 ++++---
 builtin-pack-objects.c  |    3 ++-
 builtin-rev-list.c      |    3 ++-
 builtin-revert.c        |    2 +-
 builtin-shortlog.c      |    3 ++-
 bundle.c                |    6 ++++--
 http-push.c             |    3 ++-
 revision.c              |   32 ++++++++++++++++++--------------
 revision.h              |    7 +++++--
 upload-pack.c           |    4 ++--
 wt-status.c             |    6 +++---
 21 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 1da22ee..c60d310 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -121,7 +121,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
+	setup_revisions(&rev, NULL);
 	rev.prune_data = pathspec;
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
diff --git a/builtin-blame.c b/builtin-blame.c
index cf41511..1e26b88 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2425,7 +2425,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	argv[unk] = NULL;
 
 	init_revisions(&revs, NULL);
-	setup_revisions(unk, argv, &revs, NULL);
+	parse_revisions(unk, argv, &revs);
+	setup_revisions(&revs, NULL);
 	memset(&sb, 0, sizeof(sb));
 
 	sb.revs = &revs;
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 1ea017f..9d83fdb 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -357,7 +357,8 @@ static void report_tracking(struct branch_info *new, struct checkout_opts *opts)
 	strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));
 
 	init_revisions(&revs, NULL);
-	setup_revisions(rev_argc, rev_argv, &revs, NULL);
+	parse_revisions(rev_argc, rev_argv, &revs);
+	setup_revisions(&revs, NULL);
 	prepare_revision_walk(&revs);
 
 	/* ... and count the commits on each side. */
diff --git a/builtin-commit.c b/builtin-commit.c
index 07872c8..59b3dc2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -600,7 +600,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 		else {
 			init_revisions(&rev, "");
 			rev.abbrev = 0;
-			setup_revisions(0, NULL, &rev, parent);
+			setup_revisions(&rev, parent);
 			DIFF_OPT_SET(&rev.diffopt, QUIET);
 			DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 			run_diff_index(&rev, 1 /* cached */);
@@ -834,7 +834,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		die("could not parse newly created commit");
 
 	init_revisions(&rev, prefix);
-	setup_revisions(0, NULL, &rev, NULL);
+	setup_revisions(&rev, NULL);
 
 	rev.abbrev = 0;
 	rev.diff = 1;
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 384d871..e7e202a 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -23,7 +23,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev, NULL);
 	while (1 < argc && argv[1][0] == '-') {
 		if (!strcmp(argv[1], "--base"))
 			rev.max_count = 1;
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 2f44ebf..286a9de 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -20,7 +20,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev, NULL);
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 9d2a48f..0900175 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -72,7 +72,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	nr_sha1 = 0;
 	opt->abbrev = 0;
 	opt->diff = 1;
-	argc = setup_revisions(argc, argv, opt, NULL);
+	argc = parse_revisions(argc, argv, opt);
+	setup_revisions(opt, NULL);
 
 	while (--argc > 0) {
 		const char *arg = *++argv;
diff --git a/builtin-diff.c b/builtin-diff.c
index 4c289e7..a92dbd4 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -283,7 +283,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	if (nongit)
 		die("Not a git repository");
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev, NULL);
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 		if (diff_setup_done(&rev.diffopt) < 0)
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 1dfc01e..59b60b5 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -371,7 +371,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 
 	init_revisions(&revs, prefix);
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs, NULL);
 	argc = parse_options(argc, argv, options, fast_export_usage, 0);
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index b892621..dad81b8 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -187,7 +187,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	if (!branch || branch->type != OBJ_COMMIT)
 		return;
 
-	setup_revisions(0, NULL, rev, NULL);
+	setup_revisions(rev, NULL);
 	rev->ignore_merges = 1;
 	add_pending_object(rev, branch, name);
 	add_pending_object(rev, &head->object, "^HEAD");
diff --git a/builtin-log.c b/builtin-log.c
index 9817d6f..dde7192 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -68,8 +68,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	argc = setup_revisions(argc, argv, rev, "HEAD");
-
+	argc = parse_revisions(argc, argv, rev);
+	setup_revisions(rev, "HEAD");
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;
 	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
@@ -927,7 +927,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (numbered_files && use_stdout)
 		die ("--numbered-files and --stdout are mutually exclusive.");
 
-	argc = setup_revisions(argc, argv, &rev, "HEAD");
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev, "HEAD");
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 70d2f5d..46a8abc 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1981,7 +1981,8 @@ static void get_object_list(int ac, const char **av)
 
 	init_revisions(&revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, NULL);
+	parse_revisions(ac, av, &revs);
+	setup_revisions(&revs, NULL);
 
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 11a7eae..ed60227 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -605,7 +605,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	init_revisions(&revs, prefix);
 	revs.abbrev = 0;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs, NULL);
 
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin-revert.c b/builtin-revert.c
index 0270f9b..0adff14 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -253,7 +253,7 @@ static int index_is_dirty(void)
 {
 	struct rev_info rev;
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, "HEAD");
+	setup_revisions(&rev, "HEAD");
 	DIFF_OPT_SET(&rev.diffopt, QUIET);
 	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 	run_diff_index(&rev, 1);
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index e6a2865..3428bf6 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -256,7 +256,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		argc--;
 	}
 	init_revisions(&rev, prefix);
-	argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = parse_revisions(argc, argv, &rev);
+	setup_revisions(&rev, NULL);
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
diff --git a/bundle.c b/bundle.c
index 0ba5df1..c4848d7 100644
--- a/bundle.c
+++ b/bundle.c
@@ -120,7 +120,8 @@ int verify_bundle(struct bundle_header *header, int verbose)
 	if (revs.pending.nr != p->nr)
 		return ret;
 	req_nr = revs.pending.nr;
-	setup_revisions(2, argv, &revs, NULL);
+	parse_revisions(2, argv, &revs);
+	setup_revisions(&revs, NULL);
 
 	memset(&refs, 0, sizeof(struct object_array));
 	for (i = 0; i < revs.pending.nr; i++) {
@@ -226,7 +227,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 		return error("rev-list died");
 
 	/* write references */
-	argc = setup_revisions(argc, argv, &revs, NULL);
+	argc = parse_revisions(argc, argv, &revs);
+	setup_revisions(&revs, NULL);
 	if (argc > 1)
 		return error("unrecognized argument: %s'", argv[1]);
 
diff --git a/http-push.c b/http-push.c
index f173dcd..fa757ff 100644
--- a/http-push.c
+++ b/http-push.c
@@ -2406,7 +2406,8 @@ int main(int argc, char **argv)
 			commit_argc++;
 		}
 		init_revisions(&revs, setup_git_directory());
-		setup_revisions(commit_argc, commit_argv, &revs, NULL);
+		parse_revisions(commit_argc, commit_argv, &revs);
+		setup_revisions(&revs, NULL);
 		free(new_sha1_hex);
 		if (old_sha1_hex) {
 			free(old_sha1_hex);
diff --git a/revision.c b/revision.c
index 5a1a948..4abe410 100644
--- a/revision.c
+++ b/revision.c
@@ -964,9 +964,9 @@ static void add_ignore_packed(struct rev_info *revs, const char *name)
  * Returns the number of arguments left that weren't recognized
  * (which are also moved to the head of the argument list)
  */
-int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def)
+int parse_revisions(int argc, const char **argv, struct rev_info *revs)
 {
-	int i, flags, seen_dashdash, show_merge;
+	int i, flags, seen_dashdash;
 	const char **unrecognized = argv + 1;
 	int left = 1;
 	int all_match = 0;
@@ -987,7 +987,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		break;
 	}
 
-	flags = show_merge = 0;
+	flags = 0;
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (*arg == '-') {
@@ -1075,11 +1075,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 			if (!strcmp(arg, "--default")) {
 				if (++i >= argc)
 					die("bad --default argument");
-				def = argv[i];
+				revs->def = argv[i];
 				continue;
 			}
 			if (!strcmp(arg, "--merge")) {
-				show_merge = 1;
+				revs->show_merge = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--topo-order")) {
@@ -1361,18 +1361,25 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (revs->grep_filter) {
 		revs->grep_filter->regflags |= regflags;
 		revs->grep_filter->fixed = fixed;
+		revs->grep_filter->all_match = all_match;
 	}
+	return left;
+}
 
-	if (show_merge)
+void setup_revisions(struct rev_info *revs, const char *def)
+{
+	if (revs->def == NULL)
+		revs->def = def;
+	if (revs->show_merge)
 		prepare_show_merge(revs);
-	if (def && !revs->pending.nr) {
+	if (revs->def && !revs->pending.nr) {
 		unsigned char sha1[20];
 		struct object *object;
 		unsigned mode;
-		if (get_sha1_with_mode(def, sha1, &mode))
-			die("bad default revision '%s'", def);
-		object = get_reference(revs, def, sha1, 0);
-		add_pending_object_with_mode(revs, object, def, mode);
+		if (get_sha1_with_mode(revs->def, sha1, &mode))
+			die("bad default revision '%s'", revs->def);
+		object = get_reference(revs, revs->def, sha1, 0);
+		add_pending_object_with_mode(revs, object, revs->def, mode);
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
@@ -1406,7 +1413,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		die("diff_setup_done failed");
 
 	if (revs->grep_filter) {
-		revs->grep_filter->all_match = all_match;
 		compile_grep_patterns(revs->grep_filter);
 	}
 
@@ -1423,8 +1429,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
-
-	return left;
 }
 
 static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
diff --git a/revision.h b/revision.h
index dcf08e0..72ab144 100644
--- a/revision.h
+++ b/revision.h
@@ -25,6 +25,7 @@ struct rev_info {
 
 	/* Basic information */
 	const char *prefix;
+	const char *def;
 	void *prune_data;
 	unsigned int early_output;
 
@@ -65,8 +66,9 @@ struct rev_info {
 
 	/* Format info */
 	unsigned int	shown_one:1,
-			abbrev_commit:1,
+			show_merge:1,
 			use_terminator:1,
+			abbrev_commit:1,
 			missing_newline:1;
 	enum date_mode date_mode;
 
@@ -116,7 +118,8 @@ typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
 volatile show_early_output_fn_t show_early_output;
 
 extern void init_revisions(struct rev_info *revs, const char *prefix);
-extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);
+extern int parse_revisions(int argc, const char **argv, struct rev_info *revs);
+extern void setup_revisions(struct rev_info *revs, const char *def);
 extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
 
 extern int prepare_revision_walk(struct rev_info *revs);
diff --git a/upload-pack.c b/upload-pack.c
index b46dd36..9d031fc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -116,7 +116,7 @@ static int do_rev_list(int fd, void *create_full_pack)
 
 	if (create_full_pack) {
 		const char *args[] = {"rev-list", "--all", NULL};
-		setup_revisions(2, args, &revs, NULL);
+		parse_revisions(2, args, &revs);
 	} else {
 		for (i = 0; i < want_obj.nr; i++) {
 			struct object *o = want_obj.objects[i].item;
@@ -129,8 +129,8 @@ static int do_rev_list(int fd, void *create_full_pack)
 			o->flags |= UNINTERESTING;
 			add_pending_object(&revs, o, NULL);
 		}
-		setup_revisions(0, NULL, &revs, NULL);
 	}
+	setup_revisions(&revs, NULL);
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
diff --git a/wt-status.c b/wt-status.c
index 5b4d74c..7fa6f59 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -204,7 +204,7 @@ static void wt_status_print_updated(struct wt_status *s)
 {
 	struct rev_info rev;
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, s->reference);
+	setup_revisions(&rev, s->reference);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_print_updated_cb;
 	rev.diffopt.format_callback_data = s;
@@ -218,7 +218,7 @@ static void wt_status_print_changed(struct wt_status *s)
 {
 	struct rev_info rev;
 	init_revisions(&rev, "");
-	setup_revisions(0, NULL, &rev, NULL);
+	setup_revisions(&rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_print_changed_cb;
 	rev.diffopt.format_callback_data = s;
@@ -306,7 +306,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 	struct rev_info rev;
 
 	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, s->reference);
+	setup_revisions(&rev, s->reference);
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.file = s->fp;
-- 
1.5.6.rc0.145.ge0aba