Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe

From: Matheus Tavares <matheus.bernardino@usp.br>
Date: Jul 18, 2020, 03:52 UTC
Message-ID: <20200718035201.42233-1-matheus.bernardino@usp.br>
In-Reply-To: <nycvar.QRO.7.76.6.2007161214270.54@tvgsbejvaqbjf.bet>

On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 29 quoted lines

>
> Now, I am _far_ from knowing what I'm doing with Coccinelle, but I think
> this here semantic patch should get you going:
>
> -- snipsnap --
> @@
> expression E;
> @@
>   {
> ++   char hex[GIT_MAX_HEXSZ + 1];
>      ...
> -    oid_to_hex(E)
> +    oid_to_hex_r(hex, E)
>      ...
>   }
>
> @@
> expression E1, E2;
> @@
>   {
> ++   char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];
>      ...
> -    oid_to_hex(E1)
> +    oid_to_hex_r(hex1, E1)
>      ...
> -    oid_to_hex(E2)
> +    oid_to_hex_r(hex2, E2)
>      ...
>   }

Thanks for this nice example! This already worked very well in some of my tests :)

However, with my _very_ limited notion of Coccinelle, I didn't understand why some code snippets didn't match the above rules. For example, the structure below:

func(...)
{
	if (cond)
		func2("%s", oid_to_hex(a));
}

I thought it could be because the `if` statement is missing the curly brackets (and it does work if I add the brackets), but to my surprise, adding another oid_to_hex() call in an `else` case also made the code match the rule:

func(...)
{
	if (cond)
		func2("%s", oid_to_hex(a));
	else
		func2("%s", oid_to_hex(a));
}

The following snippet also correctly matches, but spatch introduces only one `hex` variable:

	if (cond)
		func2("%s, %s", oid_to_hex(a), oid_to_hex(b));
	else
		func2("%s", oid_to_hex(a));

We will probably want our semantic rules to handle an arbitrary number of `oid_to_hex()` calls in each function, but in scenarios like the above one, we only really need 2 hex buffers despite having 3 calls... That might be a little tricky, I guess.

Another thing that might be tricky in this conversion is checking for name conflicts with the added `hex` variable (but maybe Coccinelle already has a facilitator mechanism for such cases? IDK).