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

[Justin Tobler <jltobler@gmail.com>]

On 26/10/16 08:58AM, Patrick Steinhardt wrote:
> 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.

That is correct. Instead of migrating the ".keep" file in the quarantine
with all the other object files, the intent is for the ".keep" file to
be manually created during transaction commit first. I'll update the
commit message here a bit to allude to this a bit more directly.

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

Yup, creating the lockfile in the quarantine doesn't really tell us
anything. It also makes it a bit more complicated during transaction
commit to detect whether there is already a lockfile for the same pack
that exists.

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

Good suggestion. Will use in the next version.

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

Will split out the code comment change into 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 `-`.

Will fix.

> > +	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;

That is indeed much better. Will use in the next version. Thanks.

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

Thanks for the careful eyes.

-Justin