Re: format-patch crashes with a huge patchset
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 20, 2014, 17:41 UTC
- Message-ID
- <xmqqa9ac1h5l.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20140520064920.GB5222@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Mon, May 19, 2014 at 10:35:56PM +0300, Michael S. Tsirkin wrote: > >> I tried to fump the whole history of qemu with format-patch. >> It crashes both with v2.0.0-rc2-21-g6087111 >> and with git 1.8.3.1: >> >> ~/opt/libexec/git-core/git-format-patch --follow -o patches/all >> e63c3dc74bfb90e4522d075d0d5a7600c5145745.. > > You can't use "--follow" without specifying a single pathspec. Running > "git log --follow" notices and blocks this, but format-patch doesn't > (and segfaults later when the follow code assumes there is a pathspec). > > I think we need:
Looks sensible. Intrestingly enough, this dates all the way back to the original 750f7b66 (Finally implement "git log --follow", 2007-06-19).
Re-reading the log message of that commit was amusing for other reasons, by the way (the most interesting part comes after "One thing to look out for:" and especially "Put another way:").
Show 62 quoted lines
> -- >8 --
> Subject: move "--follow needs one pathspec" rule to diff_setup_done
>
> Because of the way "--follow" is implemented, we must have
> exactly one pathspec. "git log" enforces this restriction,
> but other users of the revision traversal code do not. For
> example, "git format-patch --follow" will segfault during
> try_to_follow_renames, as we have no pathspecs at all.
>
> We can push this check down into diff_setup_done, which is
> probably a better place anyway. It is the diff code that
> introduces this restriction, so other parts of the code
> should not need to care themselves.
>
> Reported-by: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I didn't include a test, as I don't think the current behavior is what
> we want in the long run. That is, it would be nice if eventually
> --follow learned to be more flexible. Any test for this would
> necessarily be looking for the opposite of that behavior. But maybe it's
> worth it to just have in the meantime, and anyone who works on --follow
> can rip out the test.
>
> builtin/log.c | 8 ++------
> diff.c | 3 +++
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 39e8836..3b6a6bb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -158,13 +158,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
> if (rev->show_notes)
> init_display_notes(&rev->notes_opt);
>
> - if (rev->diffopt.pickaxe || rev->diffopt.filter)
> + if (rev->diffopt.pickaxe || rev->diffopt.filter ||
> + DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES))
> rev->always_show_header = 0;
> - if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
> - rev->always_show_header = 0;
> - if (rev->diffopt.pathspec.nr != 1)
> - usage("git logs can only follow renames on one pathname at a time");
> - }
>
> if (source)
> rev->show_source = 1;
> diff --git a/diff.c b/diff.c
> index f72769a..68bb8c5 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3325,6 +3325,9 @@ void diff_setup_done(struct diff_options *options)
> }
>
> options->diff_path_counter = 0;
> +
> + if (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)
> + die(_("--follow requires exactly one pathspec"));
> }
>
> static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)