Re: [PATCH v2] object-file: reprepare alternates when necessary

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 10, 2023, 21:29 UTC
Message-ID: <20230310212916.4138690-1-jonathantanmy@google.com>
In-Reply-To: <pull.1490.v2.git.1678301252360.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

>     Update in v2
>     ============
>     
>      * Instead of creating a new public reprepare_alt_odb() method, inline
>        its implementation inside reprepare_packed_git().

[snip]

Show 17 quoted lines

> diff --git a/packfile.c b/packfile.c
> index 79e21ab18e7..06419c8e8ca 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1008,6 +1008,16 @@ void reprepare_packed_git(struct repository *r)
>  	struct object_directory *odb;
>  
>  	obj_read_lock();
> +
> +	/*
> +	 * Reprepare alt odbs, in case the alternates file was modified
> +	 * during the course of this process. This only _adds_ odbs to
> +	 * the linked list, so existing odbs will continue to exist for
> +	 * the lifetime of the process.
> +	 */
> +	r->objects->loaded_alternates = 0;
> +	prepare_alt_odb(r);

I understand that the change to inline what was originally reprepare_alt_odb() was in response to a reviewer comment, but I would prefer the original version, since this assumes that prepare_alt_odb() only adds to what's there instead of first clearing the odb linked list and odb_by_path hashmap.

But I guess clearing a linked list and hashmap can be a bit cumbersome in C, so maybe it would be reasonable to assume that this behavior would not change. In any case, maybe a comment should be added to prepare_alt_odb() saying that if an update of the alternates is needed, one can do so by clearing loaded_alternates and re- invoking prepare_alt_odb() (at least so that a developer changing prepare_alt_odb() can see the comment and understand what behaviors this function needs to preserve).

Everything else (the new functionality, the commit message etc.) looks good, of course.