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

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

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.