From: Torsten Bögershausen <tboegi@web.de>
Date: Sat, 02 Aug 2014 18:13:40 GMT
Subject: Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)
Message-ID: <53DD2A54.1030403@web.de>
In-Reply-To: <xmqqtx5wuma8.fsf@gitster.dls.corp.google.com>

On 08/01/2014 07:55 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> Somewhat underexplained, given that it seems to add some new
>> semantics.
>>
>>> +static void clear_filename(struct lock_file *lk)
>>> +{
>>> +	free(lk->filename);
>>> +	lk->filename = NULL;
>>> +}
>> It is good to abstract out lk->filename[0] = '\0', which used to be
>> the way we say that we are done with the lock.  But I am somewhat
>> surprised to see that there aren't so many locations that used to
>> check !!lk->filename[0] to see if we are done with the lock to require
>> a corresponding wrapper.
>>
>>>   static void remove_lock_file(void)
>>>   {
>>>   	pid_t me = getpid();
>>>   
>>>   	while (lock_file_list) {
>>>   		if (lock_file_list->owner == me &&
>>> -		    lock_file_list->filename[0]) {
>>> +		    lock_file_list->filename) {
>> ... and this seems to be the only location?
> While looking at possible fallout of merging this topic to any
> branch, I am starting to suspect that it is probably a bad idea for
> clear-filename to free lk->filename.  I am wondering if it would be
> safer to do:
>
>   - in lock_file(), free lk->filename if it already exists before
>     what you do in that function with your series;
>
>   - update "is this lock already held?" check !!lk->filename[0] to
>     check for (lk->filename && !!lk->filename[0]);
>
>   - in clear_filename(), clear lk->filename[0] = '\0', but do not
>     free lk->filename itself.
>
> Then existing callers that never suspected that lk->filename can be
> NULL and thought that it does not need freeing can keep doing the
> same thing as before without leaking nor breaking.
>
> If we want to adopt the new world order at once, alternatively, you
> can keep the code in this series but then lk->filename needs to be
> renamed to something that the current code base has not heard of to
> force breakage at the link time for us to notice.
>
> I grepped for 'lk->filename' and checked if the ones in read-cache.c
> and refs.c are OK (they seem to be), but that is not a very robust
> check.
>
> I dunno.

My first impression reading this patch was to rename
clear_filename() into free_and_clear_filename() or better free_filename(),
but I never pressed the send button ;-)

Reading the discussion above makes me wonder if lk->filename may be replaced by a strbuf
some day, and in this case clear_filename() will become reset_filenmae() ?