Re: [PATCH v4 11/16] ref-filter: introduce refname_atom_parser()

From: Jeff King <peff@peff.net>
Date: Apr 14, 2016, 20:43 UTC
Message-ID: <20160414204304.GA20875@sigill.intra.peff.net>
In-Reply-To: <1460227515-28437-12-git-send-email-Karthik.188@gmail.com>

On Sun, Apr 10, 2016 at 12:15:10AM +0530, Karthik Nayak wrote:

Show 11 quoted lines

> +static void refname_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +	if (!arg)
> +		atom->u.refname.option = R_NORMAL;
> +	else if (!strcmp(arg, "short"))
> +		atom->u.refname.option = R_SHORT;
> +	else if (skip_prefix(arg, "strip=", &arg)) {
> +		atom->u.contents.option = R_STRIP;
> +		if (strtoul_ui(arg, 10, &atom->u.refname.strip) ||
> +			atom->u.refname.strip <= 0)
> +			die(_("positive value expected refname:strip=%s"), arg);

That R_STRIP line should be setting atom->u.refname.option, right?

The same mistake happens in a later patch, too, when parsing for R_BASE and R_DIR is added. And I think the same problem is also present in objectname_atom_parser.

The compiler doesn't notice because, hey, it's C, and why bother complaining when somebody assigns the value from one enum to another? And the tests don't notice because the enum is at the front of each union member, so the compiler happens to put it in the same place (I think it _might_ even be guaranteed by the standard's type-punning rules, but that's really not something we should rely on).

-Peff