Re: [PATCH v5 1/1] worktree add: sanitize worktree names

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2019, 06:45 UTC
Message-ID: <xmqqzhq0h9pk.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1903111401220.41@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 23 quoted lines

>> +static int check_refname_component(const char *refname, int *flags,
>> +				   struct strbuf *sanitized)
>>  {
>>  	const char *cp;
>>  	char last = '\0';
>> +	size_t component_start;
>
> This variable is uninitialized. It is then...
>
>> +
>> +	if (sanitized)
>> +		component_start = sanitized->len;
>
> ... initialized only when `sanitized` is not `NULL`, and subsequently...
> ...
>> +	if (refname[0] == '.') { /* Component starts with '.'. */
>> +		if (sanitized)
>> +			sanitized->buf[component_start] = '-';
> ...
> ... used a loooooooong time after that, also only if `sanitized` is not
> `NULL`.
>
> Apparently for some GCC versions, this is too cute, and it complains that

It does require humans (well, at least it did to this one) to be careful when reading the code to know that component_start is valid when it is used.

There unfortunately is no good "default" value to initialize the variable to. When checking a later component in a series of components, it would be looking at non-zero position, so even initializing it to 0 in this function is *not* a more sensible fallback default value than any other random garbage value (which would squelch the compiler, but it would mislead the humans nevertheless).

And that (i.e. the lack of any sensible default value when sanitized is NULL) is the reason why the variable is left uninitialized by the patch, I think. I do not think the code is trying to be cute at all.

I wonder if we make the caller pass a pointer to

	struct {
		struct strbuf result;
		size_t component_start;
	} sanitized;

	sanitized.component_start = sanitized.result.len
	check_refname_component(refname, flags, &sanitized);

and get rid of the assignment to component_start done by the callee, it would appease compilers and makes the code easier to vet. It does introduce one more ad-hoc type, which is a certain downside.

I dunno.