Subject: Re: [PATCH v6 02/11] replay: extract logic to pick commits

From: Phillip Wood <[email-masked]>
Date: Mon, 17 Nov 2025 16:27:40 +0000

Hi Patrick

On 27/10/2025 11:33, Patrick Steinhardt wrote:
> We're about to add a new git-history(1) command that will reuse some of
> the same infrastructure as git-replay(1). To prepare for this, extract
> the logic to pick a commit into a new "replay.c" file so that it can be
> shared between both commands.
> 
> Rename the function to have a "replay_" prefix to clearly indicate its
> subsystem.

I'm sorry it has taken me so long to get round to looking at this, I've 
been intending to read through this series ever since you re-rolled 
after the contributor summit.

This patch looks good, the only changes to the moved code are to 
namespace the function which become public. I'm very pleased to see us 
switching to using the replay machinery.

Thanks

Phillip

> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>   Makefile         |   1 +
>   builtin/replay.c | 110 ++--------------------------------------------------
>   meson.build      |   1 +
>   replay.c         | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   replay.h         |  23 +++++++++++
>   5 files changed, 143 insertions(+), 107 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 1919d35bf3f..01c171b4f03 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1261,6 +1261,7 @@ LIB_OBJS += reftable/tree.o
>   LIB_OBJS += reftable/writer.o
>   LIB_OBJS += remote.o
>   LIB_OBJS += replace-object.o
> +LIB_OBJS += replay.o
>   LIB_OBJS += repo-settings.o
>   LIB_OBJS += repository.o
>   LIB_OBJS += rerere.o
> diff --git a/builtin/replay.c b/builtin/replay.c
> index bb0420dc992..e39824912cd 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -2,7 +2,6 @@
>    * "git replay" builtin command
>    */
>   
> -#define USE_THE_REPOSITORY_VARIABLE
>   #define DISABLE_SIGN_COMPARE_WARNINGS
>   
>   #include "git-compat-util.h"
> @@ -16,6 +15,7 @@
>   #include "object-name.h"
>   #include "parse-options.h"
>   #include "refs.h"
> +#include "replay.h"
>   #include "revision.h"
>   #include "strmap.h"
>   #include <oidset.h>
> @@ -26,13 +26,6 @@ enum ref_action_mode {
>   	REF_ACTION_PRINT
>   };
>   
> -static const char *short_commit_name(struct repository *repo,
> -				     struct commit *commit)
> -{
> -	return repo_find_unique_abbrev(repo, &commit->object.oid,
> -				       DEFAULT_ABBREV);
> -}
> -
>   static struct commit *peel_committish(struct repository *repo, const char *name)
>   {
>   	struct object *obj;
> @@ -45,59 +38,6 @@ static struct commit *peel_committish(struct repository *repo, const char *name)
>   						  OBJ_COMMIT);
>   }
>   
> -static char *get_author(const char *message)
> -{
> -	size_t len;
> -	const char *a;
> -
> -	a = find_commit_header(message, "author", &len);
> -	if (a)
> -		return xmemdupz(a, len);
> -
> -	return NULL;
> -}
> -
> -static struct commit *create_commit(struct repository *repo,
> -				    struct tree *tree,
> -				    struct commit *based_on,
> -				    struct commit *parent)
> -{
> -	struct object_id ret;
> -	struct object *obj = NULL;
> -	struct commit_list *parents = NULL;
> -	char *author;
> -	char *sign_commit = NULL; /* FIXME: cli users might want to sign again */
> -	struct commit_extra_header *extra = NULL;
> -	struct strbuf msg = STRBUF_INIT;
> -	const char *out_enc = get_commit_output_encoding();
> -	const char *message = repo_logmsg_reencode(repo, based_on,
> -						   NULL, out_enc);
> -	const char *orig_message = NULL;
> -	const char *exclude_gpgsig[] = { "gpgsig", NULL };
> -
> -	commit_list_insert(parent, &parents);
> -	extra = read_commit_extra_headers(based_on, exclude_gpgsig);
> -	find_commit_subject(message, &orig_message);
> -	strbuf_addstr(&msg, orig_message);
> -	author = get_author(message);
> -	reset_ident_date();
> -	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
> -				 &ret, author, NULL, sign_commit, extra)) {
> -		error(_("failed to write commit object"));
> -		goto out;
> -	}
> -
> -	obj = parse_object(repo, &ret);
> -
> -out:
> -	repo_unuse_commit_buffer(the_repository, based_on, message);
> -	free_commit_extra_headers(extra);
> -	free_commit_list(parents);
> -	strbuf_release(&msg);
> -	free(author);
> -	return (struct commit *)obj;
> -}
> -
>   struct ref_info {
>   	struct commit *onto;
>   	struct strset positive_refs;
> @@ -246,50 +186,6 @@ static void determine_replay_mode(struct repository *repo,
>   	strset_clear(&rinfo.positive_refs);
>   }
>   
> -static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
> -				    struct commit *commit,
> -				    struct commit *fallback)
> -{
> -	khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid);
> -	if (pos == kh_end(replayed_commits))
> -		return fallback;
> -	return kh_value(replayed_commits, pos);
> -}
> -
> -static struct commit *pick_regular_commit(struct repository *repo,
> -					  struct commit *pickme,
> -					  kh_oid_map_t *replayed_commits,
> -					  struct commit *onto,
> -					  struct merge_options *merge_opt,
> -					  struct merge_result *result)
> -{
> -	struct commit *base, *replayed_base;
> -	struct tree *pickme_tree, *base_tree;
> -
> -	base = pickme->parents->item;
> -	replayed_base = mapped_commit(replayed_commits, base, onto);
> -
> -	result->tree = repo_get_commit_tree(repo, replayed_base);
> -	pickme_tree = repo_get_commit_tree(repo, pickme);
> -	base_tree = repo_get_commit_tree(repo, base);
> -
> -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
> -	merge_opt->branch2 = short_commit_name(repo, pickme);
> -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
> -
> -	merge_incore_nonrecursive(merge_opt,
> -				  base_tree,
> -				  result->tree,
> -				  pickme_tree,
> -				  result);
> -
> -	free((char*)merge_opt->ancestor);
> -	merge_opt->ancestor = NULL;
> -	if (!result->clean)
> -		return NULL;
> -	return create_commit(repo, result->tree, pickme, replayed_base);
> -}
> -
>   static int handle_ref_update(enum ref_action_mode mode,
>   			     struct ref_transaction *transaction,
>   			     const char *refname,
> @@ -483,8 +379,8 @@ int cmd_replay(int argc,
>   		if (commit->parents->next)
>   			die(_("replaying merge commits is not supported yet!"));
>   
> -		last_commit = pick_regular_commit(repo, commit, replayed_commits,
> -						  onto, &merge_opt, &result);
> +		last_commit = replay_pick_regular_commit(repo, commit, replayed_commits,
> +							 onto, &merge_opt, &result);
>   		if (!last_commit)
>   			break;
>   
> diff --git a/meson.build b/meson.build
> index cee94244759..ae8d4fef059 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -464,6 +464,7 @@ libgit_sources = [
>     'reftable/writer.c',
>     'remote.c',
>     'replace-object.c',
> +  'replay.c',
>     'repo-settings.c',
>     'repository.c',
>     'rerere.c',
> diff --git a/replay.c b/replay.c
> new file mode 100644
> index 00000000000..98be33b8545
> --- /dev/null
> +++ b/replay.c
> @@ -0,0 +1,115 @@
> +#define USE_THE_REPOSITORY_VARIABLE
> +
> +#include "git-compat-util.h"
> +#include "commit.h"
> +#include "environment.h"
> +#include "gettext.h"
> +#include "ident.h"
> +#include "object.h"
> +#include "object-name.h"
> +#include "replay.h"
> +#include "tree.h"
> +
> +static const char *short_commit_name(struct repository *repo,
> +				     struct commit *commit)
> +{
> +	return repo_find_unique_abbrev(repo, &commit->object.oid,
> +				       DEFAULT_ABBREV);
> +}
> +
> +static char *get_author(const char *message)
> +{
> +	size_t len;
> +	const char *a;
> +
> +	a = find_commit_header(message, "author", &len);
> +	if (a)
> +		return xmemdupz(a, len);
> +
> +	return NULL;
> +}
> +
> +struct commit *replay_create_commit(struct repository *repo,
> +				    struct tree *tree,
> +				    struct commit *based_on,
> +				    struct commit *parent)
> +{
> +	struct object_id ret;
> +	struct object *obj = NULL;
> +	struct commit_list *parents = NULL;
> +	char *author;
> +	char *sign_commit = NULL; /* FIXME: cli users might want to sign again */
> +	struct commit_extra_header *extra = NULL;
> +	struct strbuf msg = STRBUF_INIT;
> +	const char *out_enc = get_commit_output_encoding();
> +	const char *message = repo_logmsg_reencode(repo, based_on,
> +						   NULL, out_enc);
> +	const char *orig_message = NULL;
> +	const char *exclude_gpgsig[] = { "gpgsig", NULL };
> +
> +	commit_list_insert(parent, &parents);
> +	extra = read_commit_extra_headers(based_on, exclude_gpgsig);
> +	find_commit_subject(message, &orig_message);
> +	strbuf_addstr(&msg, orig_message);
> +	author = get_author(message);
> +	reset_ident_date();
> +	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
> +				 &ret, author, NULL, sign_commit, extra)) {
> +		error(_("failed to write commit object"));
> +		goto out;
> +	}
> +
> +	obj = parse_object(repo, &ret);
> +
> +out:
> +	repo_unuse_commit_buffer(the_repository, based_on, message);
> +	free_commit_extra_headers(extra);
> +	free_commit_list(parents);
> +	strbuf_release(&msg);
> +	free(author);
> +	return (struct commit *)obj;
> +}
> +
> +static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
> +				    struct commit *commit,
> +				    struct commit *fallback)
> +{
> +	khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid);
> +	if (pos == kh_end(replayed_commits))
> +		return fallback;
> +	return kh_value(replayed_commits, pos);
> +}
> +
> +struct commit *replay_pick_regular_commit(struct repository *repo,
> +					  struct commit *pickme,
> +					  kh_oid_map_t *replayed_commits,
> +					  struct commit *onto,
> +					  struct merge_options *merge_opt,
> +					  struct merge_result *result)
> +{
> +	struct commit *base, *replayed_base;
> +	struct tree *pickme_tree, *base_tree;
> +
> +	base = pickme->parents->item;
> +	replayed_base = mapped_commit(replayed_commits, base, onto);
> +
> +	result->tree = repo_get_commit_tree(repo, replayed_base);
> +	pickme_tree = repo_get_commit_tree(repo, pickme);
> +	base_tree = repo_get_commit_tree(repo, base);
> +
> +	merge_opt->branch1 = short_commit_name(repo, replayed_base);
> +	merge_opt->branch2 = short_commit_name(repo, pickme);
> +	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
> +
> +	merge_incore_nonrecursive(merge_opt,
> +				  base_tree,
> +				  result->tree,
> +				  pickme_tree,
> +				  result);
> +
> +	free((char*)merge_opt->ancestor);
> +	merge_opt->ancestor = NULL;
> +	if (!result->clean)
> +		return NULL;
> +	return replay_create_commit(repo, result->tree, pickme, replayed_base);
> +}
> diff --git a/replay.h b/replay.h
> new file mode 100644
> index 00000000000..d6535ee56c9
> --- /dev/null
> +++ b/replay.h
> @@ -0,0 +1,23 @@
> +#ifndef REPLAY_H
> +#define REPLAY_H
> +
> +#include "khash.h"
> +#include "merge-ort.h"
> +#include "repository.h"
> +
> +struct commit;
> +struct tree;
> +
> +struct commit *replay_create_commit(struct repository *repo,
> +				    struct tree *tree,
> +				    struct commit *based_on,
> +				    struct commit *parent);
> +
> +struct commit *replay_pick_regular_commit(struct repository *repo,
> +					  struct commit *pickme,
> +					  kh_oid_map_t *replayed_commits,
> +					  struct commit *onto,
> +					  struct merge_options *merge_opt,
> +					  struct merge_result *result);
> +
> +#endif
>