Re: [PATCH v2 4/8] builtin/repack.c: keep track of existing packs unconditionally

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 22, 2021, 22:56 UTC
Message-ID: <20210922225605.2608085-1-jonathantanmy@google.com>
In-Reply-To: <c0d045a9de1a5e75d684b0dd2009a3137b6e5c59.1631730270.git.me@ttaylorr.com>

Show 12 quoted lines

> @@ -98,8 +98,9 @@ static void remove_pack_on_signal(int signo)
>   * have a corresponding .keep file. These packs are not to
>   * be kept if we are going to pack everything into one file.
>   */
> -static void get_non_kept_pack_filenames(struct string_list *fname_list,
> -					const struct string_list *extra_keep)
> +static void collect_pack_filenames(struct string_list *fname_list,
> +				   struct string_list *fname_kept_list,
> +				   const struct string_list *extra_keep)
>  {
>  	DIR *dir;
>  	struct dirent *e;

The comment in the before-context of this hunk needs to be updated.

Also, I think that fname_list should be renamed to fname_nonkept_list. It does have the same semantics as before, but the name of the function has changed. And also, fname_list sounds like a superset of fname_kept_list, but that is not the case.

Show 8 quoted lines

> @@ -440,6 +440,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	struct string_list names = STRING_LIST_INIT_DUP;
>  	struct string_list rollback = STRING_LIST_INIT_NODUP;
>  	struct string_list existing_packs = STRING_LIST_INIT_DUP;
> +	struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
>  	struct pack_geometry *geometry = NULL;
>  	struct strbuf line = STRBUF_INIT;
>  	int i, ext, ret;

Likewise here - the introduction of a new similarly-named variable means that existing_packs should be renamed, I think.

Other than that, this patch looks good.