Re: [PATCH 1/3] pack-write: lift and expose index-pack report parsing

[Patrick Steinhardt <ps@pks.im>]

On Thu, Oct 15, 2026 at 09:28:59PM -0500, Justin Tobler wrote:
> Once git-index-pack(1) finishes writing a packfile, it reports the
> hexadecimal name of that packfile on stdout prefixed with either "pack"
> or "keep" depending on whether a ".keep" file was also created alongside
> it. Callers that are required to manage the lifecycle of ".keep" files
> parse this report via `index_pack_lockfile()`, which provides the path
> to the temporary ".keep" file.
> 
> In a subsequent commit, the "files" ODB transaction begins staging
> packfiles in a transaction without eagerly creating a ".keep" file and
> must instead read git-index-pack(1) "pack" output to get the packfile
> name. Extract the generic parsing logic for the git-index-pack(1) output
> into `read_index_pack_report()` in preparation accordingly.

So if I understand correctly, the intent is to eventually stop creating
the ".keep" file in the quarantine directory? I assume that instead,
you'll then manually create that file in the final location before
moving around the pack itself.

If so that makes sense -- it's kind of pointless to create ".keep" files
in quarantines anyway, nobody but the current process should be touching
it anyway.

> diff --git a/pack-write.c b/pack-write.c
> index 85674e4b72..5f5bb894a5 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -469,33 +469,68 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
>  	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
>  }
>  
> -char *index_pack_lockfile(struct odb_source *source, int ip_out,
> -			  int *is_well_formed)
> +int read_index_pack_report(struct repository *repo, int ip_out,
> +			   struct strbuf *pack_name, int *is_keep)
>  {
> +	const int len = repo->hash_algo->hexsz + 6;
>  	char packname[GIT_MAX_HEXSZ + 6];
> -	const int len = source->odb->repo->hash_algo->hexsz + 6;
> +	const char *name;
> +	int keep;

Suggestion, please feel free to ignore:

    int is_keep_ignored;

    if (!is_keep)
        is_keep = &is_keep_ignored;

Then you can unconditionally use `is_keep` further down, which I think
is a bit less awkward.

>  	/*
> -	 * The first thing we expect from index-pack's output
> -	 * is "pack\t%40s\n" or "keep\t%40s\n" (46 bytes) where
> -	 * %40s is the newly created pack SHA1 name.  In the "keep"
> -	 * case, we need it to remove the corresponding .keep file
> -	 * later on.  If we don't get that then tough luck with it.
> +	 * The first thing we expect from index-pack's output is
> +	 * "pack\t<name>\n" or "keep\t<name>\n", where <name> is the hexadecimal
> +	 * name of the packfile it has just written. The line is therefore five
> +	 * bytes of prefix, `hexsz` bytes of name, and a trailing newline. A
> +	 * "keep" prefix tells us that index-pack has also created the
> +	 * corresponding ".keep" file.
>  	 */

Okay. I originally thought you only reflow this comment, but you fix it
to no longer assume that packfile hashes use SHA1. I would have either
pointed this out in the commit message or done this as a preparatory
commit.

> -	if (read_in_full(ip_out, packname, len) == len && packname[len-1] == '\n') {
> -		const char *name;
> +	if (read_in_full(ip_out, packname, len) != len || packname[len-1] != '\n')
> +		return -1;
> +
> +	packname[len-1] = 0;

Missing spaces around `-`.

> +	if (skip_prefix(packname, "pack\t", &name))
> +		keep = 0;
> +	else if (skip_prefix(packname, "keep\t", &name))
> +		keep = 1;
> +	else
> +		return -1;
> +
> +	strbuf_addstr(pack_name, name);
> +	if (is_keep)
> +		*is_keep = keep;
> +
> +	return 0;
> +}
> +
> +char *index_pack_lockfile(struct odb_source *source, int ip_out,
> +			  int *is_well_formed)
> +{
> +	struct strbuf pack_name = STRBUF_INIT;
> +	char *lockfile = NULL;
> +	int is_keep = 0;
>  
> +	if (read_index_pack_report(source->odb->repo, ip_out, &pack_name, &is_keep)) {
>  		if (is_well_formed)
> -			*is_well_formed = 1;
> -		packname[len-1] = 0;
> -		if (skip_prefix(packname, "keep\t", &name))
> -			return xstrfmt("%s/pack/pack-%s.keep",
> -				       source->path, name);
> +			*is_well_formed = 0;
>  		return NULL;
>  	}
> +
>  	if (is_well_formed)
> -		*is_well_formed = 0;
> -	return NULL;
> +		*is_well_formed = 1;
> +
> +	/*
> +	 * If there is no ".keep" file, then there is no lockfile to return.
> +	 */
> +	if (!is_keep) {
> +		strbuf_release(&pack_name);
> +		return NULL;
> +	}

Can we maybe have a single exit path responsible for releasing
`pack_name`?

    /* If there is no ".keep" file, then there is no lockfile to return. */
    if (is_keep)
        lockfile = xstrfmt("%s/pack/pack-%s.keep", source->path, pack_name.buf);

    strbuf_release(&pack_name);
    return lockfile;

> diff --git a/pack.h b/pack.h
> index ada506b5c5..ddf2d52934 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -106,6 +106,16 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
>  void fixup_pack_header_footer(const struct git_hash_algo *, int,
>  			      unsigned char *, const char *, uint32_t,
>  			      unsigned char *, off_t);
> +
> +/*
> + * Read the report that git-index-pack(1) writes to its standard output after
> + * the packfile is written. The hexadecimal name of the packfile is appended to
> + * `pack_name` and, if `is_keep` is non-NULL, set it to indicate whether a

s/set it to/it is set to/

> + * ".keep" file was written. Return 0 on success and a negative error code on
> + * failure.
> + */
> +int read_index_pack_report(struct repository *repo, int ip_out,
> +			   struct strbuf *pack_name, int *is_keep);
>  char *index_pack_lockfile(struct odb_source *source, int fd,
>  			  int *is_well_formed);

Patrick