Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream

[Jeff King <peff@peff.net>]

On Mon, Jun 01, 2015 at 01:03:13AM +0000, brian m. carlson wrote:

> format-patch would segfault if provided a tag as one of the range
> endpoints in conjunction with --ignore-if-in-upstream, as it assumed the
> object was a commit and attempted to cast it to struct commit.
> Dereference the tag as soon as possible to prevent this, but not until
> after copying the necessary flags.

I bisected Bruce's case earlier to 895c5ba (revision: do not peel tags
used in range notation, 2013-09-19). This is an obvious fallout from
that commit; unlike most traversals which read from rev->commits, we
read straight from rev->pending here. So I wondered briefly if that
commit was not being sufficiently careful.

But as it turns out, this code was buggy long before then. 895c5ba only
changed the range notation. Even before then, if you did:

  git format-patch --ignore-if-in-upstream ^v2.2.0 v2.2.1

we would segfault. Anybody reading from rev->pending should be ready to
handle any kind of object.

Which also makes me wonder about...

> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..e0465ba 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)
>  	o2 = rev->pending.objects[1].item;
>  	flags2 = o2->flags;
>  
> +	o1 = deref_tag(o1, NULL, 0);
> +	o2 = deref_tag(o2, NULL, 0);
> +
> +	if (!o1 || !o2)
> +		die(_("Invalid tag."));

This will dereference tags, but it won't help at all with:

  git format-patch --ignore-if-in-upstream ^HEAD:Makefile HEAD:Documentation

where we end up with blobs. That is ridiculous, of course, but we should
complain, not segfault.

So I think what you really want is lookup_commit_reference. And the
error message is really not "invalid tag", but "not a commit". I think
you can just use lookup_commit_or_die.

>  	if ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))
>  		die(_("Not a range."));

As an aside, now that we are dereferencing, these flags are from the
wrong object. They _should_ be the same (we mark the tag as
UNINTERESTING, too), but it's a little weird that at the end of the
function we restore the saved flags from the tag object onto the commit.
Just bumping the assignment of flags{1,2} would work (or just bump up
the lookup_commit_or_die call to where we assign to o{1,2}).

> +test_expect_success "format-patch --ignore-if-in-upstream handles tags" '
> +
> +	git tag -a v1 -m tag side &&
> +	git format-patch --stdout \
> +		--ignore-if-in-upstream master..v1 >patch1 &&
> +	cnt=$(grep "^From " patch1 | wc -l) &&
> +	test $cnt = 2

I think this avoids the usual "wc" whitespace pitfall because you don't
use double-quotes. But maybe:

  grep "^From " patch1 >count &&
  test_line_count = 2 patch1

would be more idiomatic.

-Peff