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

From: Jeff King <peff@peff.net>
Date: Jun 1, 2015, 10:20 UTC
Message-ID: <20150601102046.GA31792@peff.net>
In-Reply-To: <1433120593-186980-1-git-send-email-sandals@crustytoothpaste.net>

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

Show 5 quoted lines

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

Show 13 quoted lines

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

Show 7 quoted lines

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