Re: [PATCH v4 1/3] refs: keep track of unresolved reference value in iterators

From: shejialuo <shejialuo@gmail.com>
Date: Nov 23, 2024, 08:24 UTC
Message-ID: <Z0GRNGQEeoOQHKz3@ArchLinux>
In-Reply-To: <c4f5f5b7dd8dad4f17201611faee14dd1b882bff.1723217871.git.gitgitgadget@gmail.com>

On Fri, Aug 09, 2024 at 03:37:49PM +0000, John Cai via GitGitGadget wrote:

[snip]

Show 8 quoted lines

> @@ -66,6 +69,7 @@ static void free_ref_entry(struct ref_entry *entry)
>  		 */
>  		clear_ref_dir(&entry->u.subdir);
>  	}
> +	free(entry->u.value.referent);
>  	free(entry);
>  }
>

Today, I am learning the source code of the "ref-cache.[ch]". I feel rather confused here. And I think this usage is wrong.

"free_ref_entry" will do the following things:

1. If "entry" is a directory, it will call "clear_ref_dir" which will
call "free_ref_entry" for every loose ref.
2. If "entry" is a loose ref, it will call `free(entry->u.value.referent)`
and `free(entry)`.

The problem is if "entry" is a directory, we will also execute the following statement:

    free(entry->u.value.referent);

This does not make sense. We should never access the "entry->u.value" if "entry" is a directory. So, I think the correct usage should be:

    if (entry->flag & REF_DIR) {
        ...
        clear_ref_dir(...);
    } else {
        free(entry->u.value.referent);
    }

Thanks, Jialuo