{"thread":{"id":"59337","subject":"[PATCH] format-patch: output header for empty commits","startedAt":"2023-03-03T16:27:44Z","lastAt":"2023-03-08T20:44:06Z","messageCount":6,"participants":["John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472962","messageId":"20230303160301.3659328-1-john@keeping.me.uk","threadId":"59337","inReplyTo":null,"subject":"[PATCH] format-patch: output header for empty commits","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2023-03-03T16:03:01Z","receivedAt":"2023-03-03T16:27:44Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When formatting an empty commit, it is surprising that a totally empty\nfile is generated.  Set the flag to always print the header, matching\nthe behaviour of git-log.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/log.c           |  1 +\n t/t4014-format-patch.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex a70fba198f..87b4fb2edc 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2097,6 +2097,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \n \t/* Always generate a patch */\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n+\trev.always_show_header = 1;\n \n \trev.zero_commit = zero_commit;\n \trev.patch_name_max = fmt_patch_name_max;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex f3313b8c58..ffc7c60680 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -59,6 +59,10 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"patchid 3\" &&\n \n+\tgit checkout -b empty main &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"empty commit\" &&\n+\n \tgit checkout main\n '\n \n@@ -128,6 +132,12 @@ test_expect_success 'replay did not screw up the log message' '\n \tgrep \"^Side .* with .* backslash-n\" actual\n '\n \n+test_expect_success 'format-patch empty commit' '\n+\tgit format-patch --stdout main..empty >empty &&\n+\tgrep \"^From \" empty >from &&\n+\ttest_line_count = 1 from\n+'\n+\n test_expect_success 'extra headers' '\n \tgit config format.headers \"To: R E Cipient <rcipient@example.com>\n \" &&\n-- \n2.39.2\n\n"},{"id":"472964","messageId":"xmqqwn3xg3m0.fsf@gitster.g","threadId":"59337","inReplyTo":"20230303160301.3659328-1-john@keeping.me.uk","subject":"Re: [PATCH] format-patch: output header for empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T17:13:27Z","receivedAt":"2023-03-03T17:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> When formatting an empty commit, it is surprising that a totally empty\n> file is generated.  Set the flag to always print the header, matching\n> the behaviour of git-log.\n\nDon't these empty files help send-email as safety against sending\nthem out?  Unless existing tools depend on the current behaviour in\nsuch a way, I think this is quite a sensible change.\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index f3313b8c58..ffc7c60680 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -59,6 +59,10 @@ test_expect_success setup '\n>  \ttest_tick &&\n>  \tgit commit -m \"patchid 3\" &&\n>  \n> +\tgit checkout -b empty main &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m \"empty commit\" &&\n> +\n>  \tgit checkout main\n>  '\n>  \n> @@ -128,6 +132,12 @@ test_expect_success 'replay did not screw up the log message' '\n>  \tgrep \"^Side .* with .* backslash-n\" actual\n>  '\n>  \n> +test_expect_success 'format-patch empty commit' '\n> +\tgit format-patch --stdout main..empty >empty &&\n> +\tgrep \"^From \" empty >from &&\n> +\ttest_line_count = 1 from\n> +'\n> +\n>  test_expect_success 'extra headers' '\n>  \tgit config format.headers \"To: R E Cipient <rcipient@example.com>\n>  \" &&\n"},{"id":"472998","messageId":"ZAMhOehmuIov/KM8@keeping.me.uk","threadId":"59337","inReplyTo":"xmqqwn3xg3m0.fsf@gitster.g","subject":"Re: [PATCH] format-patch: output header for empty commits","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2023-03-04T10:45:20Z","receivedAt":"2023-03-04T10:53:46Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Mar 03, 2023 at 09:13:27AM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > When formatting an empty commit, it is surprising that a totally empty\n> > file is generated.  Set the flag to always print the header, matching\n> > the behaviour of git-log.\n> \n> Don't these empty files help send-email as safety against sending\n> them out?  Unless existing tools depend on the current behaviour in\n> such a way, I think this is quite a sensible change.\n\nYes, send-email fails trying to send an empty file, but to me this feels\nmore like an accident than an intentional safeguard.  If there were\nsomething intentional I'd expect format-patch to fail with --allow-empty\nas an option to bypass that safety check.\n\nSince there are checks in place to avoid unintentionally creating empty\ncommits, it seems reasonable for format-patch to create output that\nrepresents what is present in the repository without needing extra\noptions.\n\n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > index f3313b8c58..ffc7c60680 100755\n> > --- a/t/t4014-format-patch.sh\n> > +++ b/t/t4014-format-patch.sh\n> > @@ -59,6 +59,10 @@ test_expect_success setup '\n> >  \ttest_tick &&\n> >  \tgit commit -m \"patchid 3\" &&\n> >  \n> > +\tgit checkout -b empty main &&\n> > +\ttest_tick &&\n> > +\tgit commit --allow-empty -m \"empty commit\" &&\n> > +\n> >  \tgit checkout main\n> >  '\n> >  \n> > @@ -128,6 +132,12 @@ test_expect_success 'replay did not screw up the log message' '\n> >  \tgrep \"^Side .* with .* backslash-n\" actual\n> >  '\n> >  \n> > +test_expect_success 'format-patch empty commit' '\n> > +\tgit format-patch --stdout main..empty >empty &&\n> > +\tgrep \"^From \" empty >from &&\n> > +\ttest_line_count = 1 from\n> > +'\n> > +\n> >  test_expect_success 'extra headers' '\n> >  \tgit config format.headers \"To: R E Cipient <rcipient@example.com>\n> >  \" &&\n"},{"id":"473061","messageId":"xmqqlek9byeb.fsf@gitster.g","threadId":"59337","inReplyTo":"ZAMhOehmuIov/KM8@keeping.me.uk","subject":"Re: [PATCH] format-patch: output header for empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T17:08:44Z","receivedAt":"2023-03-06T17:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Fri, Mar 03, 2023 at 09:13:27AM -0800, Junio C Hamano wrote:\n>> John Keeping <john@keeping.me.uk> writes:\n>> \n>> > When formatting an empty commit, it is surprising that a totally empty\n>> > file is generated.  Set the flag to always print the header, matching\n>> > the behaviour of git-log.\n>> \n>> Don't these empty files help send-email as safety against sending\n>> them out?  Unless existing tools depend on the current behaviour in\n>> such a way, I think this is quite a sensible change.\n>\n> Yes, send-email fails trying to send an empty file, but to me this feels\n> more like an accident than an intentional safeguard.  If there were\n> something intentional I'd expect format-patch to fail with --allow-empty\n> as an option to bypass that safety check.\n>\n> Since there are checks in place to avoid unintentionally creating empty\n> commits,...\n\nSpeaking as the original implementer of format-patch, the original\nintention was to forbid such a message to be sent out.  But it was\ndesigned back in the days when an empty commit were not used as \"a\nmarker in the history\" as widely as these days.  IOW, the original\nintention does not matter all that much when we have to determine if\nthe code with the proposed change would negatively affect _today's_\nusers.  What the users would see is that they have been protected\nfrom sending out such a message by mistake (an empty commit may not\nbe something you created but you pulled from your colleages), but\nwith this change the protection is no longer there.\n\nAnother worry is if the receiving end is prepared to see such a\n\"patch\".\n\nOverall, if we were designing format-patch/send-email/am today with\ntoday's use cases in mind without any existing users of these three\ncommands, I think these three would be designed to pass an empty\ncommit through the chain unconditionally.  But we do not live in\nsuch a world, so perhaps some sort of opting in may be appropriate.\n\nThanks.\n"},{"id":"473219","messageId":"ZAjxL2MIXCNZgYj/@keeping.me.uk","threadId":"59337","inReplyTo":"xmqqlek9byeb.fsf@gitster.g","subject":"Re: [PATCH] format-patch: output header for empty commits","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2023-03-08T20:33:51Z","receivedAt":"2023-03-08T20:34:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Mar 06, 2023 at 09:08:44AM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > On Fri, Mar 03, 2023 at 09:13:27AM -0800, Junio C Hamano wrote:\n> >> John Keeping <john@keeping.me.uk> writes:\n> >> \n> >> > When formatting an empty commit, it is surprising that a totally empty\n> >> > file is generated.  Set the flag to always print the header, matching\n> >> > the behaviour of git-log.\n> >> \n> >> Don't these empty files help send-email as safety against sending\n> >> them out?  Unless existing tools depend on the current behaviour in\n> >> such a way, I think this is quite a sensible change.\n> >\n> > Yes, send-email fails trying to send an empty file, but to me this feels\n> > more like an accident than an intentional safeguard.  If there were\n> > something intentional I'd expect format-patch to fail with --allow-empty\n> > as an option to bypass that safety check.\n> >\n> > Since there are checks in place to avoid unintentionally creating empty\n> > commits,...\n> \n> Speaking as the original implementer of format-patch, the original\n> intention was to forbid such a message to be sent out.  But it was\n> designed back in the days when an empty commit were not used as \"a\n> marker in the history\" as widely as these days.  IOW, the original\n> intention does not matter all that much when we have to determine if\n> the code with the proposed change would negatively affect _today's_\n> users.  What the users would see is that they have been protected\n> from sending out such a message by mistake (an empty commit may not\n> be something you created but you pulled from your colleages), but\n> with this change the protection is no longer there.\n> \n> Another worry is if the receiving end is prepared to see such a\n> \"patch\".\n> \n> Overall, if we were designing format-patch/send-email/am today with\n> today's use cases in mind without any existing users of these three\n> commands, I think these three would be designed to pass an empty\n> commit through the chain unconditionally.  But we do not live in\n> such a world, so perhaps some sort of opting in may be appropriate.\n\nDoes that mean you want to see format-patch die on empty commits unless\n--allow-empty is specified?\n\nI think it's in a slightly strange place because it's both a \"creation\"\ncommand and an \"inspection\" command.  Elsewhere the creation commands\n(like commit or cherry-pick) require --allow-empty but inspection\ncommands (like log or show) always show all commits.\n\nMy mental model groups format-patch in the inspection commands and I\nwouldn't send anything out without inspecting the patch files first (but\nthen I get caught out by this empty commit behaviour when I use\nformat-patch for non-email use and grep doesn't find something I'm sure\nshould be there in an empty commit's message!).\n"},{"id":"473221","messageId":"xmqqo7p3rn26.fsf@gitster.g","threadId":"59337","inReplyTo":"ZAjxL2MIXCNZgYj/@keeping.me.uk","subject":"Re: [PATCH] format-patch: output header for empty commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T20:43:45Z","receivedAt":"2023-03-08T20:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>> Overall, if we were designing format-patch/send-email/am today with\n>> today's use cases in mind without any existing users of these three\n>> commands, I think these three would be designed to pass an empty\n>> commit through the chain unconditionally.  But we do not live in\n>> such a world, so perhaps some sort of opting in may be appropriate.\n>\n> Does that mean you want to see format-patch die on empty commits unless\n> --allow-empty is specified?\n\nNo, what I (with Devil's advocate hat on) suggested was to hide the\n\"instead of leaving an empty file, fill the file with the log message\"\nfeature this patch adds behind --allow-empty option, and when the\noption is not given, keep the current behaviour.\n"}]}