On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <tboegi@xxxxxx> wrote:
> On 08/01/2014 07:55 PM, Junio C Hamano wrote:
>>
>> Junio C Hamano <gitster@xxxxxxxxx> writes:
>>
>>> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
>>>
>>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
>>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html