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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 23, 2021, 02:59 UTC
Message-ID: <YUvtrTF67FW2UGhC@nand.local>
In-Reply-To: <20210922225605.2608085-1-jonathantanmy@google.com>

On Wed, Sep 22, 2021 at 03:56:05PM -0700, Jonathan Tan wrote:

Show 14 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.

Thanks, updated. Now it reads like this:

  /*
   * Adds all packs hex strings to either the fname or fname_kept_list
   * list, based on whether each pack has a corresponding .keep file or
   * not. Packs without a .keep file are not to be kept if we are going
   * to pack everything into one file.
   */

> 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.

Yeah, I think that is a reasonable suggestion. I'll do it in a separate patch, though, to keep the renaming self-contained.

Thanks, Taylor