Re: [PATCH] bisect: always call setup_revisions after init_revisions

[Jeff King <peff@peff.net>]

On Thu, Jun 16, 2016 at 05:03:53PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The former initializes the rev_info struct to default
> > values, and the latter parsers any command-line arguments
> > and finalizes the struct.
> 
> The former refers to init and the latter setup?

Yeah, sorry, I guess I was reaching back to the subject line.

Maybe (also fixing a typo):

  init_revisions() initializes the rev_info struct to default values,
  and setup_revisions() parses any command-line arguments and finalizes
  the struct.

> I wonder if we can make it even harder to make the same mistake
> again somehow.  I notice that run_diff_files() and run_diff_index()
> in diff-lib.c share the ideal name for such an easy-to-use helper
> and run_diff_tree(), which does not exist yet, could sit alongside
> with them, but the actual implementation of the former two do not
> address this issue either.  I guess that the diversity of the set of
> pre-packaged options that various callers want to use are so graet
> that we need a rather unpleasntly large API refactoring before we
> could even contemplate doing so?
> 
> In any case, this is a strict improvement.  Let's queue it for the
> first maintenance release.

I wondered about something like the patch below, to detect such problems
consistently (and not just blow up on some corner case that isn't hit in
the test suite).

But it doesn't cover every way somebody might use a "struct rev_info",
so we'd have to sprinkle more "check" functions around. And a bunch of
stuff fails in the test suite (though it looks like it's mostly rebase
stuff, so it's probably all one or two plumbing call-sites).

I do notice some sites, like builtin/pull, use init_revisions() coupled
with diff_setup_done(). That's OK if you're just doing a diff, though
I'd argue they should use setup_revisions() to be on the safe side.

---
diff --git a/log-tree.c b/log-tree.c
index 78a5381..8303e64 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -538,6 +538,12 @@ static void show_mergetag(struct rev_info *opt, struct commit *commit)
 	for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
+static void check_rev_info(struct rev_info *opt)
+{
+	if (!opt->setup_finished)
+		die("BUG: init_revisions called without setup_revisions");
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
@@ -547,6 +553,8 @@ void show_log(struct rev_info *opt)
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
 
+	check_rev_info(opt);
+
 	opt->loginfo = NULL;
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
@@ -799,6 +807,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	struct commit_list *parents;
 	struct object_id *oid;
 
+	check_rev_info(opt);
+
 	if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))
 		return 0;
 
diff --git a/revision.c b/revision.c
index d30d1c4..2677b2e 100644
--- a/revision.c
+++ b/revision.c
@@ -2341,6 +2341,8 @@ 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;
 
+	revs->setup_finished = 1;
+
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index 9fac1a6..2dc6ecb 100644
--- a/revision.h
+++ b/revision.h
@@ -213,6 +213,8 @@ struct rev_info {
 
 	struct commit_list *previous_parents;
 	const char *break_bar;
+
+	unsigned setup_finished;
 };
 
 extern int ref_excluded(struct string_list *, const char *path);