Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)

[Duy Nguyen <pclouds@gmail.com>]

On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> 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() ?

I didn't realize Mike is making a lot more changes in lockfile.c, part
of that is converting lk->filename to use strbuf [1]. Perhaps I should
just withdraw this series, wait until Mike's series is merged, then
redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.

[1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232
-- 
Duy