From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 16 Jul 2026 10:18:07 GMT
Subject: Re: [PATCH 1/3] builtin/history: sign rewritten commits
Message-ID: <aliv3zgfDvY3JoB9@pks.im>
In-Reply-To: <20260703145037.69832-2-git@5ouma.me>

On Fri, Jul 03, 2026 at 11:50:35PM +0900, Souma wrote:
> diff --git a/builtin/history.c b/builtin/history.c
> index 091465a59e..8d669cf539 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -98,6 +98,30 @@ enum commit_tree_flags {
>  	COMMIT_TREE_EDIT_MESSAGE = (1 << 0),
>  };
>  
> +static int history_config(const char *var, const char *value,
> +			  const struct config_context *ctx, void *data)
> +{
> +	const char **sign_commit = data;
> +
> +	if (!strcmp(var, "commit.gpgsign")) {
> +		*sign_commit = git_config_bool(var, value) ? "" : NULL;
> +		return 0;
> +	}
> +
> +	return git_default_config(var, value, ctx, data);

Shouldn't we rather pass `NULL` instead of `data`? It works, sure, but
only because `git_default_config()` doesn't use `data` at all.

> @@ -160,7 +185,8 @@ static int commit_tree_ext(struct repository *repo,
>  static int commit_tree_with_edited_message(struct repository *repo,
>  					   const char *action,
>  					   struct commit *original,
> -					   struct commit **out)
> +					   struct commit **out,
> +					   const char *sign_commit)

Nit: the `out` parameter should continue to be the last one.

> @@ -515,12 +546,13 @@ static int cmd_history_fixup(int argc,
>  	bool skip_commit = false;
>  	int ret;
>  
> +	repo_config(repo, history_config, &sign_commit);
> +
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  	if (argc != 1) {
>  		ret = error(_("command expects a single revision"));
>  		goto out;
>  	}
> -	repo_config(repo, git_default_config, NULL);
>  
>  	if (action == REF_ACTION_DEFAULT)
>  		action = REF_ACTION_BRANCHES;

It might make sense to document in the commit message why we have to
change the order. I guess it's because of precedence, but not everyone
might realize that immediately.

> @@ -785,7 +822,8 @@ static int write_ondisk_index(struct repository *repo,
>  static int split_commit(struct repository *repo,
>  			struct commit *original,
>  			struct pathspec *pathspec,
> -			struct commit **out)
> +			struct commit **out,
> +			const char *sign_commit)
>  {
>  	struct interactive_options interactive_opts = INTERACTIVE_OPTIONS_INIT;
>  	struct strbuf index_file = STRBUF_INIT;

Likewise, let's ensure that the `out` parameter remains last.

> diff --git a/replay.c b/replay.c
> index da531d5bc6..683c384ef8 100644
> --- a/replay.c
> +++ b/replay.c

It might make sense to split out the changes to "replay.c" into a
preparatory commit.

One interesting question is whether it really makes sense to sign _all_
commits. It's rather likely that the history will contain commits that
aren't even owned by you, so signing them with your signature might be a
bit of a weird choice. I guess that might be okay-ish, but it's
certainly something that's worth a discussion as part of the commit
message.

Thanks!

Patrick