final git bisect step leads to: "fatal: you want to use way too much memory"

To reproduce the issue, just run:

markus@x4 ~ % git clone git://gcc.gnu.org/git/gcc.git
markus@x4 gcc % git checkout -b gcc-6 origin/gcc-6-branch markus@x4 gcc % git bisect bad 23240454adf1
You need to start by "git bisect start"
Do you want me to do it for you [Y/n]? y
markus@x4 gcc % git bisect good 558525d0cf3
Bisecting: 45 revisions left to test after this (roughly 6 steps)
[c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372 *
cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is
INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree.
markus@x4 gcc % git bisect bad
Bisecting: 22 revisions left to test after this (roughly 5 steps) [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener <[email protected]>
markus@x4 gcc % git bisect bad
Bisecting: 10 revisions left to test after this (roughly 4 steps) [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const"
markus@x4 gcc % git bisect bad
Bisecting: 5 revisions left to test after this (roughly 3 steps) [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/
markus@x4 gcc % git bisect bad
Bisecting: 2 revisions left to test after this (roughly 1 step) [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path
markus@x4 gcc % git bisect good
Bisecting: 0 revisions left to test after this (roughly 1 step) [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle <[email protected]>
markus@x4 gcc % git bisect good f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit commit f216419e5c4c41df70dbe00a6ea1faea46484dc8
fatal: you want to use way too much memory
markus@x4 gcc %

Markus

git bisect start
# bad: [23240454adf160862453a3a850451a304867c29b] Backported from mainline 2016-06-04 Jakub Jelinek <[email protected]>
git bisect bad 23240454adf160862453a3a850451a304867c29b
# good: [558525d0cf372a48ae12b1dc27201dd47ab7878c] Daily bump.
git bisect good 558525d0cf372a48ae12b1dc27201dd47ab7878c
# bad: [c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372 * cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree.
git bisect bad c4727bc59ecad9e8879aed135624781fabae16b5
# bad: [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener <[email protected]> git bisect bad 6935372a7f032acccf7876e7f54ddf494e101e5e
# bad: [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const" git bisect bad 909ed6a89604db5baa55b62ad253a9bcd5d89c03
# bad: [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/
git bisect bad f216419e5c4c41df70dbe00a6ea1faea46484dc8
# good: [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path git bisect good 745d4ecd59a3430f7c7b3bf33db1083d529a018b
# good: [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle <[email protected]> git bisect good a64301f7ac47b9d8f4f816c5e935cda4c92716b7

Forget to mention:

markus@x4 ~ % git --version
git version 2.9.0

git-2.8.4 is fine.

Markus

The issue started with:

commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023

Author: Junio C Hamano <[email protected]>
Date:   Tue Mar 29 16:05:39 2016 -0700

pretty: allow tweaking tabwidth in --expand-tabs

Markus

Interesting. But git show on the commit in question (f216419e5) does not have any problems. It looks like bisect's internal "show the commit" code does not properly call setup_revisions() to finalize the "struct rev_info". That leaves the expand_tabs_in_log flag as "-1", which then ends up cast to an unsigned of 2^64 when we use it in a size computation.

And who knows what other bugs have been lurking there over the years; there are other flags that should be finalized by setup_revision(), too.

This patch should fix it.

index 6d93edb..dc13319 100644
@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	if (!opt.diffopt.output_format)
 		opt.diffopt.output_format = DIFF_FORMAT_RAW;
 
+	setup_revisions(0, NULL, &opt, NULL);
 	log_tree_commit(&opt, commit);
 }
 

Yuck

Looks sensible.

The former initializes the rev_info struct to default values, and the latter parsers any command-line arguments and finalizes the struct.

In e22278c (bisect: display first bad commit without forking a new process, 2009-05-28), a show_diff_tree() was added that calls the former but not the latter. It doesn't have any arguments to parse, but it still should do the finalizing step.

This may have caused other minor bugs over the years, but it became much more prominent after fe37a9c (pretty: allow tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves the expected tab width as "-1", rather than the true default of "8". When we see a commit with tabs to be expanded, we end up trying to add (size_t)-1 spaces to a strbuf, which complains about the integer overflow.

The fix is easy: just call setup_revisions() with no arguments.

---
Same patch as earlier, now with 100% more commit message.

I didn't add a test, as it seemed weirdly specific to be checking "can bisect show a commit with tabs in it". I.e., it's not likely to actually regress in this specific way again.

 bisect.c | 1 +
 1 file changed, 1 insertion(+)

index 6d93edb..dc13319 100644
@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	if (!opt.diffopt.output_format)
 		opt.diffopt.output_format = DIFF_FORMAT_RAW;
 
+	setup_revisions(0, NULL, &opt, NULL);
 	log_tree_commit(&opt, commit);
 }
 

2.9.0.165.g4aacdc3

The former refers to init and the latter setup?

Thanks.

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.

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

---

index 78a5381..8303e64 100644
@@ -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;
 

index d30d1c4..2677b2e 100644
@@ -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;
 }
 

index 9fac1a6..2dc6ecb 100644
@@ -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);