[PATCH v2] doc: add caveat about roundtripping format-patch

[kristofferhaugsbakk@fastmail.com]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

git-format-patch(1) and git-am(1) deal with formatting commits as
patches and applying them, respectively. Naturally they use a few
delimiters to mark where the commit message ends. This can lead to
surprising behavior when these delimiters are used in the commit
message itself.

git-format-patch(1) will accept any commit message and not warn or error
about these delimiters being used.[1]

Especially problematic is the presence of unindented diffs in the commit
message; the patch machinery will naturally (since the commit message
has ended) try to apply that diff and everything after it.[2]

It is unclear whether any commands in this chain will learn to warn
about this. One concern could be that users have learned to rely on
the three-dash line rule to conveniently add extra-commit message
information in the commit message, knowing that git-am(1) will
ignore it.[4]

All of this is covered already, technically. However, we should spell
out the implications.

† 1: There is also git-commit(1) to consider. However, making that
     command warn or error out over such delimiters would be disruptive
     to all Git users who never use email in their workflow.
† 2: Recently patch(1) caused this issue for a project, but it was noted
     that git-am(1) has the same behavior[3]
† 3: https://github.com/i3/i3/pull/6564#issuecomment-3858381425
† 4: https://lore.kernel.org/git/xmqqldh4b5y2.fsf@gitster.g/
     https://lore.kernel.org/git/V2_format-patch_caveats.34b@msgid.xyz/

Reported-by: Matthias Beyer <mail@beyermatthias.de>
Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
Reported-by: Matheus Tavares <matheus.tavb@gmail.com>
Reported-by: Chris Packham <judge.packham@gmail.com>
Helped-by: Jakob Haufe <sur5r@sur5r.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

---

v2:

Address feedback from Phillip Wood.

Cc: Phillip Wood <phillip.wood@dunelm.org.uk>

• Drop the code blocks with the diffs; the prose speaks for itself, no need
  to take up space
• Don’t discuss git-send-email(1). We already know that git-format-patch(1)
  is the generator. It is mentioned in git-send-email(1).
• Try to be more clear about the case where someone might be applying a
  diff. Use the example from Matthias Beyer in:

      https://lore.kernel.org/git/gfxpnecn2cdtmeiape2d4x5aybuyyqi4c7m6te3khgct34dd44@wqusigna2nsp/

  Hopefully I explained it correctly?
• Add a “this goes to show...”... which seems to emphasize the point
  without being redundant. Hopefully.

Try to address feedback from Junio C Hamano by adding more nuance: the diff
in the commit message might be applied as well, or the patch machinery
might trip on something and fail.

Finally, in the middle of discussing the three possible cmt. message
delimiters, I noticed that the three points were drifting apart. So I
decided to use the list already used in git-am(1) and be done with it in
one place.

---

It seems that the section break in git-format-patch(1) does not get
applied in the man output (according to `Documentation/doc-diff`
apparently)? Maybe this is the wrong construct? I couldn’t find any
other thematic breaks here (though there are several variations).
---
 Documentation/format-patch-caveats.adoc       | 36 +++++++++++++++++++
 .../format-patch-end-of-commit-message.adoc   |  3 ++
 Documentation/git-am.adoc                     | 15 ++++++--
 Documentation/git-format-patch.adoc           |  4 +++
 Documentation/git-send-email.adoc             |  5 +++
 5 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/format-patch-caveats.adoc
 create mode 100644 Documentation/format-patch-end-of-commit-message.adoc

diff --git a/Documentation/format-patch-caveats.adoc b/Documentation/format-patch-caveats.adoc
new file mode 100644
index 00000000000..c666d709742
--- /dev/null
+++ b/Documentation/format-patch-caveats.adoc
@@ -0,0 +1,36 @@
+Patches produced by linkgit:git-format-patch[1] are inline. This means
+that the output from that command can lead to a different commit message
+when applied with linkgit:git-am[1]. It can also mean that the patch
+that is applied is not the same as the one that was generated, or that
+the patch application fails outright.
+ifdef::git-am[]
+See the <<discussion,DISCUSSION>> section above for the syntactic rules.
+endif::git-am[]
+
+ifndef::git-am[]
+Any line that is of the form:
+
+include::format-patch-end-of-commit-message.adoc[]
+
+will terminate the commit message and cause the patch machinery to start
+searching for patches to apply.
+endif::git-am[]
+
+Note that this is especially problematic for unindented diffs that occur
+in the commit message; the diff in the commit message might get applied
+along with the patch section, or the patch application machinery might
+trip up because the patch target doesn't apply. This could for example
+be caused by a diff in a GitHub Markdown code block.
+
+This loss of fidelity might be simple to notice if you are applying
+patches directly from a mailbox. However, changes originating from Git
+could be applied in bulk, in which case this would be much harder to
+notice. This could for example be a Linux distribution which uses patch
+files to apply changes on top of the commits from the upstream
+repositories. This goes to show that this behavior does not only impact
+email workflows.
+
+Given these limitations, one might be tempted to use a general-purpose
+utility like patch(1) instead. However, patch(1) will not only look for
+unindented diffs (like linkgit:git-am[1]) but will try to apply indented
+diffs as well.
diff --git a/Documentation/format-patch-end-of-commit-message.adoc b/Documentation/format-patch-end-of-commit-message.adoc
new file mode 100644
index 00000000000..47399ae7266
--- /dev/null
+++ b/Documentation/format-patch-end-of-commit-message.adoc
@@ -0,0 +1,3 @@
+* three-dashes and end-of-line, or
+* a line that begins with "diff -", or
+* a line that begins with "Index: "
diff --git a/Documentation/git-am.adoc b/Documentation/git-am.adoc
index 0c94776e296..756dfd722b9 100644
--- a/Documentation/git-am.adoc
+++ b/Documentation/git-am.adoc
@@ -231,10 +231,11 @@ applying.
 --allow-empty::
 	After a patch failure on an input e-mail message lacking a patch,
 	create an empty commit with the contents of the e-mail message
 	as its log message.
 
+[[discussion]]
 DISCUSSION
 ----------
 
 The commit author name is taken from the "From: " line of the
 message, and commit author date is taken from the "Date: " line
@@ -252,17 +253,18 @@ where the patch begins.  Excess whitespace at the end of each
 line is automatically stripped.
 
 The patch is expected to be inline, directly following the
 message.  Any line that is of the form:
 
-* three-dashes and end-of-line, or
-* a line that begins with "diff -", or
-* a line that begins with "Index: "
+include::format-patch-end-of-commit-message.adoc[]
 
 is taken as the beginning of a patch, and the commit log message
 is terminated before the first occurrence of such a line.
 
+This means that the contents of the commit message can inadvertently
+interrupt the processing (see the <<caveats,CAVEATS>> section below).
+
 When initially invoking `git am`, you give it the names of the mailboxes
 to process.  Upon seeing the first patch that does not apply, it
 aborts in the middle.  You can recover from this in one of two ways:
 
 . skip the current patch by re-running the command with the `--skip`
@@ -281,10 +283,17 @@ Before any patches are applied, ORIG_HEAD is set to the tip of the
 current branch.  This is useful if you have problems with multiple
 commits, like running 'git am' on the wrong branch or an error in the
 commits that is more easily fixed by changing the mailbox (e.g.
 errors in the "From:" lines).
 
+[[caveats]]
+CAVEATS
+-------
+
+:git-am: 1
+include::format-patch-caveats.adoc[]
+
 HOOKS
 -----
 This command can run `applypatch-msg`, `pre-applypatch`,
 and `post-applypatch` hooks.  See linkgit:githooks[5] for more
 information.
diff --git a/Documentation/git-format-patch.adoc b/Documentation/git-format-patch.adoc
index 9a7807ca71a..36851aaf5e1 100644
--- a/Documentation/git-format-patch.adoc
+++ b/Documentation/git-format-patch.adoc
@@ -796,10 +796,14 @@ CAVEATS
 Note that `format-patch` will omit merge commits from the output, even
 if they are part of the requested range. A simple "patch" does not
 include enough information for the receiving end to reproduce the same
 merge commit.
 
+'''
+
+include::format-patch-caveats.adoc[]
+
 SEE ALSO
 --------
 linkgit:git-am[1], linkgit:git-send-email[1]
 
 GIT
diff --git a/Documentation/git-send-email.adoc b/Documentation/git-send-email.adoc
index ebe8853e9f5..0b118df6498 100644
--- a/Documentation/git-send-email.adoc
+++ b/Documentation/git-send-email.adoc
@@ -690,10 +690,15 @@ Links of a few such community maintained helpers are:
 	  (cross platform client that can send emails using the ProtonMail API)
 
 	- https://github.com/AdityaGarg8/git-credential-email[git-msgraph]
 	  (cross platform client that can send emails using the Microsoft Graph API)
 
+CAVEATS
+-------
+
+include::format-patch-caveats.adoc[]
+
 SEE ALSO
 --------
 linkgit:git-format-patch[1], linkgit:git-imap-send[1], mbox(5)
 
 GIT

Interdiff against v1:
  diff --git a/Documentation/format-patch-caveats.adoc b/Documentation/format-patch-caveats.adoc
  index 2accf2763fd..c666d709742 100644
  --- a/Documentation/format-patch-caveats.adoc
  +++ b/Documentation/format-patch-caveats.adoc
  @@ -1,39 +1,36 @@
  -Patches produced by linkgit:git-format-patch[1] or
  -linkgit:git-send-email[1] are inline. This means that the output of
  -these two commands can lead to a different commit message when applied
  -with linkgit:git-am[1]. It can also mean that the patch is not applied
  -correctly.
  +Patches produced by linkgit:git-format-patch[1] are inline. This means
  +that the output from that command can lead to a different commit message
  +when applied with linkgit:git-am[1]. It can also mean that the patch
  +that is applied is not the same as the one that was generated, or that
  +the patch application fails outright.
  +ifdef::git-am[]
  +See the <<discussion,DISCUSSION>> section above for the syntactic rules.
  +endif::git-am[]
   
  -The commit message might contain a three-dash line (`---`) which was
  -perhaps meant to be a thematic break. That means that the commit message
  -will be cut short. The presence of a line starting with "Index: " can
  -cause the patch not to be found, giving an error about an empty patch.
  +ifndef::git-am[]
  +Any line that is of the form:
   
  -Furthermore, the presence of an unindented diff in the commit message
  -will not only cut the message short but cause that very diff to be
  -applied, along with the patch in the patch section. The commit message
  -might for example have a diff in a GitHub MarkDown code fence:
  +include::format-patch-end-of-commit-message.adoc[]
   
  -----
  -```
  -diff ...
  -```
  -----
  +will terminate the commit message and cause the patch machinery to start
  +searching for patches to apply.
  +endif::git-am[]
   
  -The solution for this is to indent the diff instead:
  -
  -----
  -    diff ...
  -----
  +Note that this is especially problematic for unindented diffs that occur
  +in the commit message; the diff in the commit message might get applied
  +along with the patch section, or the patch application machinery might
  +trip up because the patch target doesn't apply. This could for example
  +be caused by a diff in a GitHub Markdown code block.
   
   This loss of fidelity might be simple to notice if you are applying
  -patches directly from a mailbox. However, a commit authored long ago
  -might be applied in a different context, perhaps because many changes
  -are being integrated via patch files and the
  -linkgit:git-format-patch[1] format is trusted to import changes of a
  -Git origin.
  +patches directly from a mailbox. However, changes originating from Git
  +could be applied in bulk, in which case this would be much harder to
  +notice. This could for example be a Linux distribution which uses patch
  +files to apply changes on top of the commits from the upstream
  +repositories. This goes to show that this behavior does not only impact
  +email workflows.
   
  -One might want to use a general-purpose utility like patch(1) instead,
  -given these limitations. However, patch(1) will not only look for
  +Given these limitations, one might be tempted to use a general-purpose
  +utility like patch(1) instead. However, patch(1) will not only look for
   unindented diffs (like linkgit:git-am[1]) but will try to apply indented
   diffs as well.
  diff --git a/Documentation/format-patch-end-of-commit-message.adoc b/Documentation/format-patch-end-of-commit-message.adoc
  new file mode 100644
  index 00000000000..47399ae7266
  --- /dev/null
  +++ b/Documentation/format-patch-end-of-commit-message.adoc
  @@ -0,0 +1,3 @@
  +* three-dashes and end-of-line, or
  +* a line that begins with "diff -", or
  +* a line that begins with "Index: "
  diff --git a/Documentation/git-am.adoc b/Documentation/git-am.adoc
  index 18f5b950825..756dfd722b9 100644
  --- a/Documentation/git-am.adoc
  +++ b/Documentation/git-am.adoc
  @@ -231,10 +231,11 @@ applying.
   --allow-empty::
   	After a patch failure on an input e-mail message lacking a patch,
   	create an empty commit with the contents of the e-mail message
   	as its log message.
   
  +[[discussion]]
   DISCUSSION
   ----------
   
   The commit author name is taken from the "From: " line of the
   message, and commit author date is taken from the "Date: " line
  @@ -252,18 +253,16 @@ where the patch begins.  Excess whitespace at the end of each
   line is automatically stripped.
   
   The patch is expected to be inline, directly following the
   message.  Any line that is of the form:
   
  -* three-dashes and end-of-line, or
  -* a line that begins with "diff -", or
  -* a line that begins with "Index: "
  +include::format-patch-end-of-commit-message.adoc[]
   
   is taken as the beginning of a patch, and the commit log message
   is terminated before the first occurrence of such a line.
   
  -This means that the content of the commit message can inadverently
  +This means that the contents of the commit message can inadvertently
   interrupt the processing (see the <<caveats,CAVEATS>> section below).
   
   When initially invoking `git am`, you give it the names of the mailboxes
   to process.  Upon seeing the first patch that does not apply, it
   aborts in the middle.  You can recover from this in one of two ways:
  @@ -288,10 +287,11 @@ errors in the "From:" lines).
   
   [[caveats]]
   CAVEATS
   -------
   
  +:git-am: 1
   include::format-patch-caveats.adoc[]
   
   HOOKS
   -----
   This command can run `applypatch-msg`, `pre-applypatch`,

Range-diff against v1:
1:  4bed8f55b98 ! 1:  c54f394bb33 doc: add caveat about roundtripping format-patch
    @@ Metadata
      ## Commit message ##
         doc: add caveat about roundtripping format-patch
     
    -    git-format-patch(1), git-send-email(1), and git-am(1) deal with
    -    formatting commits as patches, sending them (perhaps directly), and
    -    applying them, respectively. Naturally they use a few delimiters to mark
    -    where the commit message ends. This can lead to surprising behavior when
    -    these delimiters are used in the commit message itself.
    +    git-format-patch(1) and git-am(1) deal with formatting commits as
    +    patches and applying them, respectively. Naturally they use a few
    +    delimiters to mark where the commit message ends. This can lead to
    +    surprising behavior when these delimiters are used in the commit
    +    message itself.
     
    -    git-format-patch(1) and git-send-email(1) will accept any commit message
    -    and not warn or error about these delimiters being used.[1]
    +    git-format-patch(1) will accept any commit message and not warn or error
    +    about these delimiters being used.[1]
     
    -    Moreover, the presence of unindented diffs in the commit message will
    -    cause git-am(1) to apply both the diffs from the commit message as well
    -    as the patch section.[2]
    +    Especially problematic is the presence of unindented diffs in the commit
    +    message; the patch machinery will naturally (since the commit message
    +    has ended) try to apply that diff and everything after it.[2]
     
         It is unclear whether any commands in this chain will learn to warn
         about this. One concern could be that users have learned to rely on
    @@ Commit message
         information in the commit message, knowing that git-am(1) will
         ignore it.[4]
     
    -    All of this is covered already, technically, However, we should spell
    +    All of this is covered already, technically. However, we should spell
         out the implications.
     
         † 1: There is also git-commit(1) to consider. However, making that
              command warn or error out over such delimiters would be disruptive
              to all Git users who never use email in their workflow.
    -    [2]: Recently patch(1) caused this issue for a project, but it was noted
    +    † 2: Recently patch(1) caused this issue for a project, but it was noted
              that git-am(1) has the same behavior[3]
    -    [3]: https://github.com/i3/i3/pull/6564#issuecomment-3858381425
    -    [4]: https://lore.kernel.org/git/xmqqldh4b5y2.fsf@gitster.g/
    +    † 3: https://github.com/i3/i3/pull/6564#issuecomment-3858381425
    +    † 4: https://lore.kernel.org/git/xmqqldh4b5y2.fsf@gitster.g/
    +         https://lore.kernel.org/git/V2_format-patch_caveats.34b@msgid.xyz/
     
         Reported-by: Matthias Beyer <mail@beyermatthias.de>
         Reported-by: Christoph Anton Mitterer <calestyo@scientia.org>
    @@ Commit message
         Helped-by: Jakob Haufe <sur5r@sur5r.net>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +    ---
    +
    +    v2:
    +
    +    Address feedback from Phillip Wood.
    +
    +    Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
    +
    +    • Drop the code blocks with the diffs; the prose speaks for itself, no need
    +      to take up space
    +    • Don’t discuss git-send-email(1). We already know that git-format-patch(1)
    +      is the generator. It is mentioned in git-send-email(1).
    +    • Try to be more clear about the case where someone might be applying a
    +      diff. Use the example from Matthias Beyer in:
    +
    +          https://lore.kernel.org/git/gfxpnecn2cdtmeiape2d4x5aybuyyqi4c7m6te3khgct34dd44@wqusigna2nsp/
    +
    +      Hopefully I explained it correctly?
    +    • Add a “this goes to show...”... which seems to emphasize the point
    +      without being redundant. Hopefully.
    +
    +    Try to address feedback from Junio C Hamano by adding more nuance: the diff
    +    in the commit message might be applied as well, or the patch machinery
    +    might trip on something and fail.
    +
    +    Finally, in the middle of discussing the three possible cmt. message
    +    delimiters, I noticed that the three points were drifting apart. So I
    +    decided to use the list already used in git-am(1) and be done with it in
    +    one place.
    +
    +    ---
    +
    +    It seems that the section break in git-format-patch(1) does not get
    +    applied in the man output (according to `Documentation/doc-diff`
    +    apparently)? Maybe this is the wrong construct? I couldn’t find any
    +    other thematic breaks here (though there are several variations).
    +
      ## Documentation/format-patch-caveats.adoc (new) ##
     @@
    -+Patches produced by linkgit:git-format-patch[1] or
    -+linkgit:git-send-email[1] are inline. This means that the output of
    -+these two commands can lead to a different commit message when applied
    -+with linkgit:git-am[1]. It can also mean that the patch is not applied
    -+correctly.
    -+
    -+The commit message might contain a three-dash line (`---`) which was
    -+perhaps meant to be a thematic break. That means that the commit message
    -+will be cut short. The presence of a line starting with "Index: " can
    -+cause the patch not to be found, giving an error about an empty patch.
    ++Patches produced by linkgit:git-format-patch[1] are inline. This means
    ++that the output from that command can lead to a different commit message
    ++when applied with linkgit:git-am[1]. It can also mean that the patch
    ++that is applied is not the same as the one that was generated, or that
    ++the patch application fails outright.
    ++ifdef::git-am[]
    ++See the <<discussion,DISCUSSION>> section above for the syntactic rules.
    ++endif::git-am[]
     +
    -+Furthermore, the presence of an unindented diff in the commit message
    -+will not only cut the message short but cause that very diff to be
    -+applied, along with the patch in the patch section. The commit message
    -+might for example have a diff in a GitHub MarkDown code fence:
    ++ifndef::git-am[]
    ++Any line that is of the form:
     +
    -+----
    -+```
    -+diff ...
    -+```
    -+----
    ++include::format-patch-end-of-commit-message.adoc[]
     +
    -+The solution for this is to indent the diff instead:
    ++will terminate the commit message and cause the patch machinery to start
    ++searching for patches to apply.
    ++endif::git-am[]
     +
    -+----
    -+    diff ...
    -+----
    ++Note that this is especially problematic for unindented diffs that occur
    ++in the commit message; the diff in the commit message might get applied
    ++along with the patch section, or the patch application machinery might
    ++trip up because the patch target doesn't apply. This could for example
    ++be caused by a diff in a GitHub Markdown code block.
     +
     +This loss of fidelity might be simple to notice if you are applying
    -+patches directly from a mailbox. However, a commit authored long ago
    -+might be applied in a different context, perhaps because many changes
    -+are being integrated via patch files and the
    -+linkgit:git-format-patch[1] format is trusted to import changes of a
    -+Git origin.
    ++patches directly from a mailbox. However, changes originating from Git
    ++could be applied in bulk, in which case this would be much harder to
    ++notice. This could for example be a Linux distribution which uses patch
    ++files to apply changes on top of the commits from the upstream
    ++repositories. This goes to show that this behavior does not only impact
    ++email workflows.
     +
    -+One might want to use a general-purpose utility like patch(1) instead,
    -+given these limitations. However, patch(1) will not only look for
    ++Given these limitations, one might be tempted to use a general-purpose
    ++utility like patch(1) instead. However, patch(1) will not only look for
     +unindented diffs (like linkgit:git-am[1]) but will try to apply indented
     +diffs as well.
     
    + ## Documentation/format-patch-end-of-commit-message.adoc (new) ##
    +@@
    ++* three-dashes and end-of-line, or
    ++* a line that begins with "diff -", or
    ++* a line that begins with "Index: "
    +
      ## Documentation/git-am.adoc ##
    -@@ Documentation/git-am.adoc: message.  Any line that is of the form:
    +@@ Documentation/git-am.adoc: applying.
    + 	create an empty commit with the contents of the e-mail message
    + 	as its log message.
    + 
    ++[[discussion]]
    + DISCUSSION
    + ----------
    + 
    +@@ Documentation/git-am.adoc: line is automatically stripped.
    + The patch is expected to be inline, directly following the
    + message.  Any line that is of the form:
    + 
    +-* three-dashes and end-of-line, or
    +-* a line that begins with "diff -", or
    +-* a line that begins with "Index: "
    ++include::format-patch-end-of-commit-message.adoc[]
    + 
      is taken as the beginning of a patch, and the commit log message
      is terminated before the first occurrence of such a line.
      
    -+This means that the content of the commit message can inadverently
    ++This means that the contents of the commit message can inadvertently
     +interrupt the processing (see the <<caveats,CAVEATS>> section below).
     +
      When initially invoking `git am`, you give it the names of the mailboxes
    @@ Documentation/git-am.adoc: commits, like running 'git am' on the wrong branch or
     +CAVEATS
     +-------
     +
    ++:git-am: 1
     +include::format-patch-caveats.adoc[]
     +
      HOOKS

base-commit: 3e0db84c88c57e70ac8be8c196dfa92c5d656fbc
-- 
2.53.0.26.g2afa8602a26