diff mbox series

[4/8] midx-write.c: extract `should_include_pack()`

Message ID 3908546ea85eb36a27ce6bd681a3c2152ff005f5.1716482279.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series midx-write: miscellaneous clean-ups for incremental MIDXs | expand

Commit Message

Taylor Blau May 23, 2024, 4:38 p.m. UTC
The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is
used to add packs with .idx files to the MIDX being written.

Within this function, we have a pair of checks that discards packs
which:

  - appear in an existing MIDX, if we successfully read an existing MIDX
    from disk

  - or, appear in the "to_include" list, if invoking the MIDX write
    machinery with the `--stdin-packs` command-line argument.

In a future commit will want to call a slight variant of these checks
from the code that reuses all packs from an existing MIDX, as well as
the current location via add_pack_to_midx(). The latter will be
modified in subsequent commits to only reuse packs which appear in the
to_include list, if one was given.

Prepare for that step by extracting these checks as a subroutine that
may be called from both places.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx-write.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

Comments

Patrick Steinhardt May 29, 2024, 7:48 a.m. UTC | #1
On Thu, May 23, 2024 at 12:38:13PM -0400, Taylor Blau wrote:
> The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is
> used to add packs with .idx files to the MIDX being written.
> 
> Within this function, we have a pair of checks that discards packs
> which:
> 
>   - appear in an existing MIDX, if we successfully read an existing MIDX
>     from disk
> 
>   - or, appear in the "to_include" list, if invoking the MIDX write
>     machinery with the `--stdin-packs` command-line argument.
> 
> In a future commit will want to call a slight variant of these checks

Either s/In a/A/ or s/commit/&, we/.

Patrick
Taylor Blau May 29, 2024, 10:40 p.m. UTC | #2
On Wed, May 29, 2024 at 09:48:08AM +0200, Patrick Steinhardt wrote:
> On Thu, May 23, 2024 at 12:38:13PM -0400, Taylor Blau wrote:
> > The add_pack_to_midx() callback used via for_each_file_in_pack_dir() is
> > used to add packs with .idx files to the MIDX being written.
> >
> > Within this function, we have a pair of checks that discards packs
> > which:
> >
> >   - appear in an existing MIDX, if we successfully read an existing MIDX
> >     from disk
> >
> >   - or, appear in the "to_include" list, if invoking the MIDX write
> >     machinery with the `--stdin-packs` command-line argument.
> >
> > In a future commit will want to call a slight variant of these checks
>
> Either s/In a/A/ or s/commit/&, we/.

Thanks for your careful review, it is much appreciated! I meant to write
the first one, and will correct it in the subsequent round :-).

Thanks,
Taylor
diff mbox series

Patch

diff --git a/midx-write.c b/midx-write.c
index cf7e391b6e..f593cf1593 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -100,6 +100,32 @@  struct write_midx_context {
 	struct string_list *to_include;
 };
 
+static int should_include_pack(const struct write_midx_context *ctx,
+			       const char *file_name)
+{
+	/*
+	 * Note that at most one of ctx->m and ctx->to_include are set,
+	 * so we are testing midx_contains_pack() and
+	 * string_list_has_string() independently (guarded by the
+	 * appropriate NULL checks).
+	 *
+	 * We could support passing to_include while reusing an existing
+	 * MIDX, but don't currently since the reuse process drags
+	 * forward all packs from an existing MIDX (without checking
+	 * whether or not they appear in the to_include list).
+	 *
+	 * If we added support for that, these next two conditional
+	 * should be performed independently (likely checking
+	 * to_include before the existing MIDX).
+	 */
+	if (ctx->m && midx_contains_pack(ctx->m, file_name))
+		return 0;
+	else if (ctx->to_include &&
+		 !string_list_has_string(ctx->to_include, file_name))
+		return 0;
+	return 1;
+}
+
 static void add_pack_to_midx(const char *full_path, size_t full_path_len,
 			     const char *file_name, void *data)
 {
@@ -108,29 +134,11 @@  static void add_pack_to_midx(const char *full_path, size_t full_path_len,
 
 	if (ends_with(file_name, ".idx")) {
 		display_progress(ctx->progress, ++ctx->pack_paths_checked);
-		/*
-		 * Note that at most one of ctx->m and ctx->to_include are set,
-		 * so we are testing midx_contains_pack() and
-		 * string_list_has_string() independently (guarded by the
-		 * appropriate NULL checks).
-		 *
-		 * We could support passing to_include while reusing an existing
-		 * MIDX, but don't currently since the reuse process drags
-		 * forward all packs from an existing MIDX (without checking
-		 * whether or not they appear in the to_include list).
-		 *
-		 * If we added support for that, these next two conditional
-		 * should be performed independently (likely checking
-		 * to_include before the existing MIDX).
-		 */
-		if (ctx->m && midx_contains_pack(ctx->m, file_name))
-			return;
-		else if (ctx->to_include &&
-			 !string_list_has_string(ctx->to_include, file_name))
+
+		if (!should_include_pack(ctx, file_name))
 			return;
 
 		ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
-
 		p = add_packed_git(full_path, full_path_len, 0);
 		if (!p) {
 			warning(_("failed to add packfile '%s'"),