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() ?
--
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