Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 27, 2023, 01:51 UTC
Message-ID: <xmqqwmu42ccb.fsf@gitster.g>
In-Reply-To: <bf848477-b4dd-49d3-8e4b-de0fc3948570@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 20 quoted lines

>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index b4ab972c5a..8a8ad23e98 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -1600,6 +1600,13 @@ static int checkout_branch(struct checkout_opts *opts,
>>   	if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
>>   		die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
>>   +	/* "git checkout -B <branch>" */
>> +	if (opts->new_branch_force) {
>> +		char *full_ref = xstrfmt("refs/heads/%s", opts->new_branch);
>> +		die_if_switching_to_a_branch_in_use(opts, full_ref);
>> +		free(full_ref);
>
> At the moment this is academic as neither of the test scripts changed
> by this patch are leak free and so I don't think we need to worry
> about it but it raises an interesting question about how we should
> handle memory leaks when dying. Leaving the leak when dying means that
> a test script that tests an expected failure will never be leak free
> but using UNLEAK() would mean we miss a leak being introduced in the
> successful case should the call to "free()" ever be removed.

Is there a leak here? The piece of memory is pointed at by an on-stack variable full_ref when leak sanitizer starts scanning the heap and the stack just before the process exits due to die, so I do not see a reason to worry about this particular variable over all the other on stack variables we accumulated before the control reached this point of the code.

Are you worried about optimizing compilers that behave more cleverly than their own good to somehow lose the on-stack reference to full_ref while calling die_if_switching_to_a_branch_in_use()? We might need to squelch them with UNLEAK() but that does not mean we have to remove the free() we see above, and I suspect a more productive use of our time to solve that issue is ensure that our leak-sanitizing build will not triger such an unwanted optimization anyway.

Thanks.