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
<rguenther@suse.de>
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
<jvdelisle@gcc.gnu.org>
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 % 

On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:

Forget to mention:

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

git-2.8.4 is fine.

On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:

The issue started with:

commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue Mar 29 16:05:39 2016 -0700

    pretty: allow tweaking tabwidth in --expand-tabs

On Thu, Jun 16, 2016 at 03:29:52PM +0200, Markus Trippelsdorf wrote:

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.

Jeff King <peff@peff.net> writes:

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(+)

Jeff King <peff@peff.net> writes:

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.

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

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.

---