diff mbox series

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

Message ID c0d045a9de1a5e75d684b0dd2009a3137b6e5c59.1631730270.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series repack: introduce `--write-midx` | expand

Commit Message

Taylor Blau Sept. 15, 2021, 6:24 p.m. UTC 
In order to be able to write a multi-pack index during repacking, `git
repack` must keep track of which packs it wants to write into the MIDX.
This set is the union of existing packs which will not be deleted,
new pack(s) generated as a result of the repack, and .keep packs.

Prior to this patch, `git repack` populated the list of existing packs
only when repacking all-into-one (i.e., with `-A` or `-a`), but we will
soon need to know this list when repacking when writing a MIDX without
a-i-o.

Populate the list of existing packs unconditionally, and guard removing
packs from that list only when repacking a-i-o.

Additionally, keep track of filenames of kept packs separately, since
this, too, will be used in an upcoming patch.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 49 ++++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

Comments

Jonathan Tan Sept. 22, 2021, 10:56 p.m. UTC | #1 
> @@ -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.

> @@ -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.
Taylor Blau Sept. 23, 2021, 2:59 a.m. UTC | #2 
On Wed, Sep 22, 2021 at 03:56:05PM -0700, Jonathan Tan wrote:
> > @@ -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
diff mbox series

Patch

diff --git a/builtin/repack.c b/builtin/repack.c
index 27158a897b..e55a650de5 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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;
@@ -112,21 +113,20 @@  static void get_non_kept_pack_filenames(struct string_list *fname_list,
 		size_t len;
 		int i;
 
+		if (!strip_suffix(e->d_name, ".pack", &len))
+			continue;
+
 		for (i = 0; i < extra_keep->nr; i++)
 			if (!fspathcmp(e->d_name, extra_keep->items[i].string))
 				break;
-		if (extra_keep->nr > 0 && i < extra_keep->nr)
-			continue;
-
-		if (!strip_suffix(e->d_name, ".pack", &len))
-			continue;
 
 		fname = xmemdupz(e->d_name, len);
 
-		if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
-			string_list_append_nodup(fname_list, fname);
+		if ((extra_keep->nr > 0 && i < extra_keep->nr) ||
+		    (file_exists(mkpath("%s/%s.keep", packdir, fname))))
+			string_list_append_nodup(fname_kept_list, fname);
 		else
-			free(fname);
+			string_list_append_nodup(fname_list, fname);
 	}
 	closedir(dir);
 }
@@ -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;
@@ -572,9 +573,10 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (use_delta_islands)
 		strvec_push(&cmd.args, "--delta-islands");
 
+	collect_pack_filenames(&existing_packs, &existing_kept_packs,
+			       &keep_pack_list);
+
 	if (pack_everything & ALL_INTO_ONE) {
-		get_non_kept_pack_filenames(&existing_packs, &keep_pack_list);
-
 		repack_promisor_objects(&po_args, &names);
 
 		if (existing_packs.nr && delete_redundant) {
@@ -683,17 +685,19 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	reprepare_packed_git(the_repository);
 
 	if (delete_redundant) {
-		const int hexsz = the_hash_algo->hexsz;
 		int opts = 0;
-		string_list_sort(&names);
-		for_each_string_list_item(item, &existing_packs) {
-			char *sha1;
-			size_t len = strlen(item->string);
-			if (len < hexsz)
-				continue;
-			sha1 = item->string + len - hexsz;
-			if (!string_list_has_string(&names, sha1))
-				remove_redundant_pack(packdir, item->string);
+		if (pack_everything & ALL_INTO_ONE) {
+			const int hexsz = the_hash_algo->hexsz;
+			string_list_sort(&names);
+			for_each_string_list_item(item, &existing_packs) {
+				char *sha1;
+				size_t len = strlen(item->string);
+				if (len < hexsz)
+					continue;
+				sha1 = item->string + len - hexsz;
+				if (!string_list_has_string(&names, sha1))
+					remove_redundant_pack(packdir, item->string);
+			}
 		}
 
 		if (geometry) {
@@ -739,6 +743,7 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&names, 0);
 	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_packs, 0);
+	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
 	strbuf_release(&line);