Re: [PATCH 9/9] object-file: move logic to write loose objects

From: Toon Claes <toon@iotcl.com>
Date: Jul 22, 2026, 14:26 UTC
Message-ID: <87fr1bp0bi.fsf@emacs.iotcl.com>
In-Reply-To: <20260717-pks-odb-move-loose-object-writing-v1-9-46446a3cb5b7@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 19 quoted lines

> The logic to write loose objects is split up across "object-file.c" and
> "odb/source-loose.c". This split is somewhat weird, but it is the result
> of two things:
>
>   - `force_object_loose()` used to reach into internals of how exactly
>     we write objects.
>
>   - The logic of writing objects is intertwined with potentially
>     starting a transaction.
>
> We have refactored `force_object_loose()` over preceding commits to work
> via generic interfaces now, so this reason doesn't exist anymore. But
> the second reason still does, as our management of "files" transactions
> and their ad-hoc creation is still very messy. This area definitely
> requires further work, and that work is indeed ongoing.
>
> That being said, we can already move the writing logic into the "loose"
> backend rather easily. All we have to do is to expose two functions that
> relate to the transactions.

I'm a bit on the fence that should have gone in a separte commit, but it's fine.

> Expose these two functions and move the writing logic into the "loose"
> backend accordingly so that it becomes more self-contained. Note that
> this requires us to drop a reference to `the_repository` in favor of
> using the source's repository in `start_loose_object_common()`.

Yay! Thanks for calling that out, it standed out in the zebra diff.

Show 5 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  object-file.c      | 360 +----------------------------------------------------
>  object-file.h      |  22 +---
>  odb/source-loose.c | 354 +++++++++++++++++++++++++++++++++++++++++++++++++++-

That's a pretty large diff, but luckily the zebra diff helps a lot.

> -int write_loose_object(struct odb_source_loose *loose,
> -		       const struct object_id *oid, char *hdr,
> -		       int hdrlen, const void *buf, unsigned long len,
> -		       const time_t *mtime, unsigned flags)

This line is not colored being moved because it was made static, which makes sense.

Show 18 quoted lines

> diff --git a/object-file.h b/object-file.h
> index 31781a9c53..805f2cfa28 100644
> --- a/object-file.h
> +++ b/object-file.h
> @@ -24,20 +24,6 @@ int index_path(struct index_state *istate, struct object_id *oid, const char *pa
>  struct object_info;
>  struct odb_source;
>  
> -/*
> - * Write the given stream into the loose object source. The only difference
> - * from the generic implementation of this function is that we don't perform an
> - * object existence check here.
> - *
> - * TODO: We should stop exposing this function altogether and move it into
> - * "odb/source-loose.c". This requires a couple of refactorings though to make
> - * `force_object_loose()` generic and is thus postponed to a later point in
> - * time.
> - */

This was added by you on 2026-06-01, so thanks for addressing this.

Show 28 quoted lines

> @@ -611,12 +849,120 @@ static int odb_source_loose_write_object_stream(struct odb_source *source,
>  						size_t len,
>  						struct object_id *oid)
>  {
> +	struct odb_source_loose *loose = odb_source_loose_downcast(source);
> +	const struct git_hash_algo *compat = loose->base.odb->repo->compat_hash_algo;
> +	struct object_id compat_oid;
> +	int fd, ret, err = 0, flush = 0;
> +	unsigned char compressed[4096];
> +	git_zstream stream;
> +	struct git_hash_ctx c, compat_c;
> +	struct strbuf tmp_file = STRBUF_INIT;
> +	struct strbuf filename = STRBUF_INIT;
> +	unsigned char buf[8192];
> +	int dirlen;
> +	char hdr[MAX_HEADER_LEN];
> +	int hdrlen;
> +
> +	if (batch_fsync_enabled(FSYNC_COMPONENT_LOOSE_OBJECT))
> +		odb_transaction_files_prepare(loose->base.odb->transaction);
> +
> +	/* Since oid is not determined, save tmp file to odb path. */
> +	strbuf_addf(&filename, "%s/", loose->base.path);
> +	hdrlen = format_object_header(hdr, sizeof(hdr), OBJ_BLOB, len);
> +
>  	/*
> -	 * TODO: the implementation should be moved here, see the comment on
> -	 * the called function in "object-file.h".

So this is what you did, as suggested, by yourself.

Show 9 quoted lines

> +	 * Common steps for write_loose_object and stream_loose_object to
> +	 * start writing loose objects:
> +	 *
> +	 *  - Create tmpfile for the loose object.
> +	 *  - Setup zlib stream for compression.
> +	 *  - Start to feed header to zlib stream.
>  	 */
> -	struct odb_source_loose *loose = odb_source_loose_downcast(source);
> -	return odb_source_loose_write_stream(loose, in_stream, len, oid);

This line is marked as removed in the zebra diff, but that's because the code is being inlined into this odb_source_loose_write_object_stream() function.

All good.

-- 
Cheers,
Toon