[RFC] builtin/log: include From in git show --format=email

Commit Message

Antonin Godard Feb. 14, 2025, 4:31 p.m. UTC

Currently, when the format.from and format.forceInBodyFrom options are
configured, the command `git show --format=email <commit>` command does
not include "From: user <email>" in the body, even though I believe it
is expected when using this format.

While the code exists in log-tree.c to take the identity into account:

  if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
    ctx.from_ident = &opt->from_ident;

the mail_begin and name_begin would always be null pointers
because opt->from_ident is never filled in with the identity from
cmd_show.

This commit adds the `from` member to struct log_config, and reads the
user configuration to fill in rev.from_ident. It also reuses
the existing force_in_body_from variable to take this option into
account.

Signed-off-by: Antonin Godard <[e-mail]>
---
This is probably not the best solution, as we already have the from
member in struct format_config, so adding it in struct log_config is a bit
redundant. However, this is an RFC that hopefully will get my question
answered: is my belief to have the From field shown with this command
correct?
---
 builtin/log.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)


---
base-commit: 9520f7d9985d8879bddd157309928fc0679c8e92
change-id: 20250117-git-show-from-email-62f0e4004d7d

Best regards,

Comments

Junio C Hamano Feb. 14, 2025, 8:35 p.m. UTC | #1

Antonin Godard <[e-mail]> writes:

> Currently, when the format.from and format.forceInBodyFrom options are
> configured, the command `git show --format=email <commit>` command does
> not include "From: user <email>" in the body, even though I believe it
> is expected when using this format.

Aren't "format.*" configuration variables for "git format-patch",
and not "git show" or "git log"?

I do not see there is anything that needs fixing, but I may be
missing something.

Antonin Godard Feb. 17, 2025, 12:42 p.m. UTC | #2

Hi Junio,

On Fri Feb 14, 2025 at 9:35 PM CET, Junio C Hamano wrote:
> Antonin Godard <[e-mail]> writes:
>
>> Currently, when the format.from and format.forceInBodyFrom options are
>> configured, the command `git show --format=email <commit>` command does
>> not include "From: user <email>" in the body, even though I believe it
>> is expected when using this format.
>
> Aren't "format.*" configuration variables for "git format-patch",
> and not "git show" or "git log"?
>
> I do not see there is anything that needs fixing, but I may be
> missing something.

This is what the documentation seems to imply, but builtin/log.c uses these
configuration variables in git_log_config(), for example. In the same file,
cmd_show() uses git_log_config().

git show can be used with --format=email, and you can use the format.* options
to control the output of git show --format=email <ref>. For example:

  git -c format.subjectPrefix=FOO show --format=email HEAD

Will affect the subject.

With this reasoning in mind, I thought "git show --format=email" should also
benefit from the format.from and format.forceInBodyFrom variables, to correctly
display the output.

I should also mention that b4[1] uses `git show --format=email`[2] to generate the
patches. So at the moment any user using b4 does not have their format.from
variables satisfied. This is how I spotted this behavior, initially.

[1]: https://b4.docs.kernel.org/en/latest
[2]: https://git.kernel.org/pub/scm/utils/b4/b4.git/tree/src/b4/__init__.py#n3487

Antonin

Junio C Hamano Feb. 18, 2025, 9:45 p.m. UTC | #3

"Antonin Godard" <[e-mail]> writes:

> This is what the documentation seems to imply, but builtin/log.c uses these
> configuration variables in git_log_config(), for example. In the same file,
> cmd_show() uses git_log_config().

"imply"?  The documentation says so because the command was designed
to work like so when the feature was added in mid 2022.

    34bc1b10 (format-patch: allow forcing the use of in-body From: header, 2022-08-29)
    d5fc07df (format-patch: learn format.forceInBodyFrom configuration variable, 2022-08-29)

Please refrain from using words that imply value-judgement from your
analysis and stick to the facts; I'll try to do so in my messages,
too.

As the log family of commands happen to share much of the
implementation, their code paths pass the log-config structure which
is a mixed bag.  It does not necessarily mean all commands in the
family use all the members in the structure.

> With this reasoning in mind, I thought "git show --format=email" should also
> benefit from the format.from and format.forceInBodyFrom variables, to correctly
> display the output.

"correctly"?  

Changing the behaviour retroactively would mean that those who have
been relying on the fact that in-body header configuration meant for
"git format-patch" does not affect "git log" would suddenly start to
see their output differently.  So I'd worry how bad the downside
would be.

Having said all that.

In hindsight, or if we pretend we were in 2005 and starting the
project again from scratch, if we did not have any "format.*"
configuration variables and instead all of these were "log.*", I'd
agree with everything you said in the message I am responding to.
In such a hypothetical world, we may not even have added a separate
"format-patch" command, but used "git log" with an option to spit
out its output into an individual file for every commit.

Patch

diff --git a/builtin/log.c b/builtin/log.c
index e41f88945e..16a4889c01 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -113,6 +113,7 @@  struct log_config {
 	int fmt_patch_name_max;
 	char *fmt_pretty;
 	char *default_date_mode;
+	char *from;
 };
 
 static void log_config_init(struct log_config *cfg)
@@ -592,6 +593,19 @@  static int git_log_config(const char *var, const char *value,
 		cfg->default_encode_email_headers = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.from")) {
+		int b = git_parse_maybe_bool(value);
+		FREE_AND_NULL(cfg->from);
+		if (b < 0)
+			cfg->from = xstrdup(value);
+		else if (b)
+			cfg->from = xstrdup(git_committer_info(IDENT_NO_DATE));
+		return 0;
+	}
+	if (!strcmp(var, "format.forceinbodyfrom")) {
+		force_in_body_from = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "log.abbrevcommit")) {
 		cfg->default_abbrev_commit = git_config_bool(var, value);
 		return 0;
@@ -799,6 +813,13 @@  int cmd_show(int argc,
 		return ret;
 	}
 
+	if (cfg.from) {
+		if (split_ident_line(&rev.from_ident, cfg.from, strlen(cfg.from)))
+			die(_("invalid ident line: %s"), cfg.from);
+	}
+
+	rev.force_in_body_from = force_in_body_from;
+
 	rev.diffopt.no_free = 1;
 	for (i = 0; i < rev.pending.nr && !ret; i++) {
 		struct object *o = rev.pending.objects[i].item;