Re: [PATCH 6/6] odb/transaction: add transaction interface to write packfiles

[Justin Tobler <jltobler@gmail.com>]

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:59PM -0500, Justin Tobler wrote:
> > +		status = finish_command(&child);
> > +		if (status) {
> > +			strbuf_addstr(err_msg, "index-pack abnormal exit");
> > +			return -1;
> > +		}
> > +		odb_reprepare(repo->objects);
> 
> Now that this is part of the ODB transaction, do we really have to
> reprepare the whole object database? Shouldn't it suffice to reprepare
> just the one source that we've created the transaction for?

Ya, this is a good suggestion. At this point, the packfile has only been
written to the transaction source, so it should be fine to just prepare
that source. Will do in the next version.

> > diff --git a/odb/transaction.h b/odb/transaction.h
> > index ec0b27c449..491026e815 100644
> > --- a/odb/transaction.h
> > +++ b/odb/transaction.h
> > @@ -4,6 +4,51 @@
> >  #include "gettext.h"
> >  #include "odb.h"
> >  
> > +/*
> > + * Options controlling how odb_transaction_write_pack() ingests a packfile.
> > + */
> > +struct odb_transaction_write_pack_opts {
> > +	/*
> > +	 * Optional fsck severity configuration to apply when incoming objects
> > +	 * are verified.
> > +	 */
> > +	const char *fsck_msg_types;
> > +	/*
> > +	 * Path to an alternative shallow file describing the shallow boundaries
> > +	 * to honor while ingesting the pack.
> > +	 */
> > +	const char *shallow_file;
> > +	/*
> > +	 * The max size in bytes of the incoming packfile allowed. No limit is
> > +	 * enforced when set to 0.
> > +	 */
> > +	off_t max_input_size;
> > +	/*
> > +	 * Whether the validity of incoming objects should be verified.
> > +	 */
> > +	int fsck_objects;
> > +	/*
> > +	 * The threshold for the number of incoming objects required to store
> > +	 * the objects in a packfile. This option may not be relevant to
> > +	 * backends that do not store obejcts in loose/packed formats and can be
> > +	 * ignored.
> > +	 */
> > +	int unpack_limit;
> 
> I wonder whether this option should rather be handled internal in the
> backend itself, as it very likely doesn't apply to alternative backends
> anyway. I don't think we allow command line options to override this, so
> the backend could just read the configuration manually.

This was something I was also considering initially. This option doesn't
really make much sense to have as part of the generic interface though.
I'll update in the next version to have the backend read this
configuration manually.

> > +	/*
> > +	 * Whether to reject an incoming packfile if it is "thin".
> > +	 */
> > +	int reject_thin;
> > +	/*
> > +	 * Optional file descriptor for reporting progress and errors. Set to 0
> > +	 * for none.
> > +	 */
> > +	int err_fd;
> > +	/*
> > +	 * Suppresses progress reporting.
> > +	 */
> > +	int quiet;
> > +};
> 
> Nit: I think having some spacing between the different options would
> make this a bit easier to grok.

Will do.

-Justin