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

[Junio C Hamano <gitster@pobox.com>]

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

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