[Bug report] Crash when creating patch

[Bug report] Crash when creating patch

[Johannes Postler]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Dear git-team,
I was trying to create a patch file of a commit to another directory.
However, git crashed like that:

[josi@localhost typescript]$ git format-patch -1 236a9
--output=/home/josi/tmp/patches/txt-6117.patch
0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch
free(): invalid pointer
Aborted (core dumped)

While I'm not entirely sure I interpreted the man page correctly, I
think the crash shouldn't happen anyway.

I'm using git 2.28.0, but my colleague is experiencing the same issue
with git 2.25.1.
Please find attached the output of git bugreport and some more debug
files that were collected by Fedora's ABRT. Please let me know if you
need any other output of ABRT.

Best regards,
johannes

Johannes Postler
DevOps & Consulting

Txture - The Cloud Transformation Platform
Grabenweg 68
A-6020 Innsbruck
johannes.postler@txture.io
www.txture.io

[-- Attachment #2: git-bugreport-2020-11-04-1101.txt --]
[-- Type: text/plain, Size: 4420 bytes --]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
I tried to create a patch file of a commit in another directory using the following command:
[josi@localhost typescript]$ git format-patch -1 236a9 --output=/home/josi/tmp/patches/txt-6117.patch

What did you expect to happen? (Expected behavior)
I expected a patch file at the location specified in --output

What happened instead? (Actual behavior)
git crashed:
0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch
free(): invalid pointer
Aborted (core dumped)

What's different between what you expected and what actually happened?
I didn't expect the crash :D

Anything else you want to add:
I do have a full core dump recorded by ABRT (Fedora). Please let me know at johannes.postler@txture.io if you need thaThe following is the backtrace according to ABRT:
{   "signal": 6
,   "executable": "/usr/bin/git"
,   "stacktrace":
      [ {   "crash_thread": true
        ,   "frames":
              [ {   "address": 139721002875845
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 252869
                ,   "function_name": "raise"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721002780836
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 157860
                ,   "function_name": "abort"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003147559
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 524583
                ,   "function_name": "__libc_message"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003179548
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 556572
                ,   "function_name": ".annobin_top_check.start"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003186412
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 563436
                ,   "function_name": "_int_free"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003104931
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 481955
                ,   "function_name": "fclose@@GLIBC_2.2.5"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 94420595460270
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 538798
                ,   "function_name": "cmd_format_patch"
                ,   "file_name": "/usr/bin/git"
                }
              , {   "address": 94420595060875
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 139403
                ,   "function_name": "handle_builtin.lto_priv.0"
                ,   "file_name": "/usr/bin/git"
                }
              , {   "address": 94420595041051
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 119579
                ,   "function_name": "main"
                ,   "file_name": "/usr/bin/git"
                } ]
        } ]
}

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.28.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.8.16-300.fc33.x86_64 #1 SMP Mon Oct 19 13:18:33 UTC 2020 x86_64
compiler info: gnuc: 10.2
libc info: glibc: 2.32
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]
applypatch-msg
pre-applypatch
post-applypatch
pre-commit
pre-merge-commit
prepare-commit-msg
commit-msg
post-commit
pre-rebase
post-checkout
post-merge
pre-push
pre-receive
update
post-receive
post-update
push-to-checkout
pre-auto-gc
post-rewrite
sendemail-validate

[-- Attachment #3: core_backtrace --]
[-- Type: application/octet-stream, Size: 2746 bytes --]

{   "signal": 6
,   "executable": "/usr/bin/git"
,   "stacktrace":
      [ {   "crash_thread": true
        ,   "frames":
              [ {   "address": 139721002875845
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 252869
                ,   "function_name": "raise"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721002780836
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 157860
                ,   "function_name": "abort"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003147559
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 524583
                ,   "function_name": "__libc_message"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003179548
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 556572
                ,   "function_name": ".annobin_top_check.start"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003186412
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 563436
                ,   "function_name": "_int_free"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 139721003104931
                ,   "build_id": "2602cd2de6dc672a5ec4378ace0b4a68ea5b13ea"
                ,   "build_id_offset": 481955
                ,   "function_name": "fclose@@GLIBC_2.2.5"
                ,   "file_name": "/lib64/libc.so.6"
                }
              , {   "address": 94420595460270
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 538798
                ,   "function_name": "cmd_format_patch"
                ,   "file_name": "/usr/bin/git"
                }
              , {   "address": 94420595060875
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 139403
                ,   "function_name": "handle_builtin.lto_priv.0"
                ,   "file_name": "/usr/bin/git"
                }
              , {   "address": 94420595041051
                ,   "build_id": "ba2b8ad417de884d751cd7503b17e9ce357b2dcf"
                ,   "build_id_offset": 119579
                ,   "function_name": "main"
                ,   "file_name": "/usr/bin/git"
                } ]
        } ]
}

[-- Attachment #4: open_fds --]
[-- Type: application/octet-stream, Size: 348 bytes --]

0:/dev/pts/1
pos:	0
flags:	0102002
mnt_id:	28

1:/dev/pts/1
pos:	0
flags:	0102002
mnt_id:	28

2:/dev/pts/1
pos:	0
flags:	0102002
mnt_id:	28

3:/home/josi/tmp/patches/txt-6117.patch
pos:	0
flags:	0100001
mnt_id:	157

5:/home/josi/Txture/txture/.git/objects/pack/pack-8b55dea840db94cdc272330dfe69032eb8077991.pack
pos:	12
flags:	02100000
mnt_id:	157

[-- Attachment #5: crash_function --]
[-- Type: application/octet-stream, Size: 19 bytes --]

fclose@@GLIBC_2.2.5

[-- Attachment #6: maps --]
[-- Type: application/octet-stream, Size: 9436 bytes --]

55e002051000-55e00206c000 r--p 00000000 fd:01 2648988                    /usr/bin/git
55e00206c000-55e0022e1000 r-xp 0001b000 fd:01 2648988                    /usr/bin/git
55e0022e1000-55e00237f000 r--p 00290000 fd:01 2648988                    /usr/bin/git
55e002380000-55e002384000 r--p 0032e000 fd:01 2648988                    /usr/bin/git
55e002384000-55e002393000 rw-p 00332000 fd:01 2648988                    /usr/bin/git
55e002393000-55e0023d7000 rw-p 00000000 00:00 0 
55e002bfc000-55e002dba000 rw-p 00000000 00:00 0                          [heap]
7f1305631000-7f130567c000 r--p 00000000 fd:03 8127297                    /home/josi/Txture/txture/.git/objects/pack/pack-7c460afc8bff98d24b36ba5b8125abdfbbc5b661.pack
7f130567c000-7f134567c000 r--p 00000000 fd:03 8134171                    /home/josi/Txture/txture/.git/objects/pack/pack-8b55dea840db94cdc272330dfe69032eb8077991.pack
7f134567c000-7f13456f7000 r--p 00000000 fd:03 8126889                    /home/josi/Txture/txture/.git/objects/pack/pack-3fd239dbf9b0f3650a2b23982ee50f3272e99397.pack
7f13456f7000-7f1345702000 r--p 00000000 fd:03 8130325                    /home/josi/Txture/txture/.git/objects/pack/pack-5baab37654978c8292beed5151e18172669aa9cf.pack
7f1345702000-7f1345724000 r--p 00000000 fd:03 8129593                    /home/josi/Txture/txture/.git/objects/pack/pack-9969fb20a67801ec3e7eac49dfaedba5954e6d75.pack
7f1345724000-7f1346a7f000 r--p 00000000 fd:03 8130355                    /home/josi/Txture/txture/.git/objects/pack/pack-0aa11d9d23cd3fbce0baf771ed5f3d9941083e44.pack
7f1346a7f000-7f1346aff000 rw-p 00000000 00:00 0 
7f1346aff000-7f1346c22000 r--p 00000000 fd:03 8126860                    /home/josi/Txture/txture/.git/objects/info/commit-graph
7f1346c22000-7f1346c24000 r--p 00000000 fd:03 8130313                    /home/josi/Txture/txture/.git/objects/pack/pack-06d109d54d152773ff33656e55e64d79a8fc44a2.idx
7f1346c24000-7f1346c27000 r--p 00000000 fd:03 8130328                    /home/josi/Txture/txture/.git/objects/pack/pack-3fd239dbf9b0f3650a2b23982ee50f3272e99397.idx
7f1346c27000-7f1346c29000 r--p 00000000 fd:03 8130336                    /home/josi/Txture/txture/.git/objects/pack/pack-5baab37654978c8292beed5151e18172669aa9cf.idx
7f1346c29000-7f1346c2d000 r--p 00000000 fd:03 8130327                    /home/josi/Txture/txture/.git/objects/pack/pack-7c460afc8bff98d24b36ba5b8125abdfbbc5b661.idx
7f1346c2d000-7f1346c2f000 r--p 00000000 fd:03 8130347                    /home/josi/Txture/txture/.git/objects/pack/pack-7399aca83c64ecad4577dea0fcb33fff32837534.idx
7f1346c2f000-7f1346c34000 r--p 00000000 fd:03 8129572                    /home/josi/Txture/txture/.git/objects/pack/pack-a4d71fb1f73bea84d5a719d03107498eb19a0e84.idx
7f1346c34000-7f1346c39000 r--p 00000000 fd:03 8130361                    /home/josi/Txture/txture/.git/objects/pack/pack-0aa11d9d23cd3fbce0baf771ed5f3d9941083e44.idx
7f1346c39000-7f1346c3c000 r--p 00000000 fd:03 8127202                    /home/josi/Txture/txture/.git/objects/pack/pack-111b8c23e1b65fb3d870edde87b540ed359e2aec.idx
7f1346c3c000-7f1346c3e000 r--p 00000000 fd:03 8130346                    /home/josi/Txture/txture/.git/objects/pack/pack-9969fb20a67801ec3e7eac49dfaedba5954e6d75.idx
7f1346c3e000-7f1346c44000 r--p 00000000 fd:03 8129577                    /home/josi/Txture/txture/.git/objects/pack/pack-544060bb71fd868605c8a8065a895bdbe83fe718.idx
7f1346c44000-7f1346c46000 r--p 00000000 fd:03 8126870                    /home/josi/Txture/txture/.git/objects/pack/pack-3904e72de55d812f81041e0a324d7bb0102ab486.idx
7f1346c46000-7f1346c48000 r--p 00000000 fd:03 8129783                    /home/josi/Txture/txture/.git/objects/pack/pack-33677f12c9f70a519d3b2002fc644e446226ff37.idx
7f1346c48000-7f1346c4c000 r--p 00000000 fd:03 8129794                    /home/josi/Txture/txture/.git/objects/pack/pack-d34856887573db4eb0ee5099ca7db2eacf3987f7.idx
7f1346c4c000-7f1346c54000 r--p 00000000 fd:03 8129376                    /home/josi/Txture/txture/.git/objects/pack/pack-6d3f2b554d83df74b1577bbf6152d3f19e1478bb.idx
7f1346c54000-7f1346c58000 r--p 00000000 fd:03 8127894                    /home/josi/Txture/txture/.git/objects/pack/pack-bd767cf18a61f1360d5d6d6478236d55ffd4c560.idx
7f1346c58000-7f1346c5a000 r--p 00000000 fd:03 8127893                    /home/josi/Txture/txture/.git/objects/pack/pack-10e7ca3fd0389a97663cde6d8648f2153f5db3cd.idx
7f1346c5a000-7f1347659000 r--p 00000000 fd:03 8135274                    /home/josi/Txture/txture/.git/objects/pack/pack-8b55dea840db94cdc272330dfe69032eb8077991.idx
7f1347659000-7f134765c000 r--p 00000000 fd:03 8128277                    /home/josi/Txture/txture/.git/objects/pack/pack-3b618964d48aca5db679d52d629a485940c2b107.idx
7f134765c000-7f134765f000 r--p 00000000 fd:03 8128492                    /home/josi/Txture/txture/.git/objects/pack/pack-3844d029d438d274a602c59f9b8b563269f57970.idx
7f134765f000-7f1347661000 r--p 00000000 fd:03 8129251                    /home/josi/Txture/txture/.git/objects/pack/pack-b7c311e0942fa8924feddc8e6b141dd8aaf8d421.idx
7f1347661000-7f1347667000 r--p 00000000 fd:03 8128534                    /home/josi/Txture/txture/.git/objects/pack/pack-6440d87425558a5185baee37e0258207f28b4042.idx
7f1347667000-7f134766b000 r--p 00000000 fd:03 8128510                    /home/josi/Txture/txture/.git/objects/pack/pack-ddad15715f38cb98dae8af2fe0e5b4f391dc6d02.idx
7f134766b000-7f134766f000 r--p 00000000 fd:03 8127811                    /home/josi/Txture/txture/.git/objects/pack/pack-bdfc4bdd3d6a8dff2c0c439f1024c5ff09ee9d72.idx
7f134766f000-7f1347671000 r--p 00000000 fd:03 8128231                    /home/josi/Txture/txture/.git/objects/pack/pack-dd70f2c433a180964fedb9fae8278c8a4fa1e247.idx
7f1347671000-7f1347676000 r--p 00000000 fd:03 8128549                    /home/josi/Txture/txture/.git/objects/pack/pack-d3be3a64eeef10f43f948259609ec0b5cf17fd39.idx
7f1347676000-7f1347678000 r--p 00000000 fd:03 8128563                    /home/josi/Txture/txture/.git/objects/pack/pack-b00082a2dfb14e2effbf5fe4cb1a4dd93ef38179.idx
7f1347678000-7f1354ba8000 r--p 00000000 fd:01 2674896                    /usr/lib/locale/locale-archive
7f1354ba8000-7f1354baa000 rw-p 00000000 00:00 0 
7f1354baa000-7f1354bd0000 r--p 00000000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354bd0000-7f1354d1f000 r-xp 00026000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354d1f000-7f1354d6b000 r--p 00175000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354d6b000-7f1354d6c000 ---p 001c1000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354d6c000-7f1354d6f000 r--p 001c1000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354d6f000-7f1354d72000 rw-p 001c4000 fd:01 2630541                    /usr/lib64/libc-2.32.so
7f1354d72000-7f1354d76000 rw-p 00000000 00:00 0 
7f1354d76000-7f1354d7d000 r--p 00000000 fd:01 2674914                    /usr/lib64/libpthread-2.32.so
7f1354d7d000-7f1354d8d000 r-xp 00007000 fd:01 2674914                    /usr/lib64/libpthread-2.32.so
7f1354d8d000-7f1354d92000 r--p 00017000 fd:01 2674914                    /usr/lib64/libpthread-2.32.so
7f1354d92000-7f1354d93000 r--p 0001b000 fd:01 2674914                    /usr/lib64/libpthread-2.32.so
7f1354d93000-7f1354d94000 rw-p 0001c000 fd:01 2674914                    /usr/lib64/libpthread-2.32.so
7f1354d94000-7f1354d98000 rw-p 00000000 00:00 0 
7f1354d98000-7f1354d9b000 r--p 00000000 fd:01 2633211                    /usr/lib64/libz.so.1.2.11
7f1354d9b000-7f1354da9000 r-xp 00003000 fd:01 2633211                    /usr/lib64/libz.so.1.2.11
7f1354da9000-7f1354daf000 r--p 00011000 fd:01 2633211                    /usr/lib64/libz.so.1.2.11
7f1354daf000-7f1354db0000 ---p 00017000 fd:01 2633211                    /usr/lib64/libz.so.1.2.11
7f1354db0000-7f1354db1000 r--p 00017000 fd:01 2633211                    /usr/lib64/libz.so.1.2.11
7f1354db1000-7f1354db2000 rw-p 00000000 00:00 0 
7f1354db2000-7f1354db4000 r--p 00000000 fd:01 2671528                    /usr/lib64/libpcre2-8.so.0.10.0
7f1354db4000-7f1354e1d000 r-xp 00002000 fd:01 2671528                    /usr/lib64/libpcre2-8.so.0.10.0
7f1354e1d000-7f1354e46000 r--p 0006b000 fd:01 2671528                    /usr/lib64/libpcre2-8.so.0.10.0
7f1354e46000-7f1354e47000 r--p 00093000 fd:01 2671528                    /usr/lib64/libpcre2-8.so.0.10.0
7f1354e47000-7f1354e48000 rw-p 00094000 fd:01 2671528                    /usr/lib64/libpcre2-8.so.0.10.0
7f1354e48000-7f1354e4b000 rw-p 00000000 00:00 0 
7f1354e4b000-7f1354e66000 r--p 00000000 fd:03 8134188                    /home/josi/Txture/txture/.git/packed-refs
7f1354e66000-7f1354e67000 r--p 00000000 fd:01 2627250                    /usr/lib64/ld-2.32.so
7f1354e67000-7f1354e88000 r-xp 00001000 fd:01 2627250                    /usr/lib64/ld-2.32.so
7f1354e88000-7f1354e91000 r--p 00022000 fd:01 2627250                    /usr/lib64/ld-2.32.so
7f1354e91000-7f1354e92000 r--p 0002a000 fd:01 2627250                    /usr/lib64/ld-2.32.so
7f1354e92000-7f1354e94000 rw-p 0002b000 fd:01 2627250                    /usr/lib64/ld-2.32.so
7fff7d2e1000-7fff7d303000 rw-p 00000000 00:00 0                          [stack]
7fff7d355000-7fff7d359000 r--p 00000000 00:00 0                          [vvar]
7fff7d359000-7fff7d35b000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]

Re: [Bug report] Crash when creating patch

[Jeff King]

On Wed, Nov 04, 2020 at 11:18:10AM +0100, Johannes Postler wrote:

> [josi@localhost typescript]$ git format-patch -1 236a9
> --output=/home/josi/tmp/patches/txt-6117.patch
> 0001-Txture-Core-Implemented-whitelisting-for-groovy-scri.patch
> free(): invalid pointer
> Aborted (core dumped)

Thanks for the report. I was able to reproduce the problem here.

The issue is that "--output" was never supposed to work with
format-patch. But a subtle change in the option parsing a while back
caused it to be respected. And as you noticed, the documentation
mistakenly mentions the option, since format-patch includes the standard
diff-options text.

So one obvious fix would be to forbid it and adjust the documentation.
But because of the way the option parsers interact, it's surprisingly
hard to do so cleanly. It's actually easier to just make it do something
useful (i.e., behave like --stdout but sent to a file). So I did that
instead.

  [1/3]: format-patch: refactor output selection
  [2/3]: format-patch: tie file-opening logic to output_directory
  [3/3]: format-patch: support --output option

 builtin/log.c           | 37 ++++++++++++++++++++++---------------
 t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 15 deletions(-)

-Peff

[PATCH 1/3] format-patch: refactor output selection

[Jeff King]

The --stdout and --output-directory options are mutually exclusive, but
it's hard to tell from reading the code. We have three separate
conditionals that check for use_stdout, and it's only after we've set up
the output_directory fully that we check whether the user also specified
--stdout.

Instead, let's check the exclusion explicitly first, then have a single
conditional that handles stdout versus an output directory. This is
slightly easier to follow now, and also will keep things sane when we
add another output mode in a future patch.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9f939e6cdf..4c391ba3ca 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	if (!output_directory && !use_stdout)
-		output_directory = config_output_directory;
+	if (use_stdout + !!output_directory > 1)
+		die(_("specify only one of --stdout, --output, and --output-directory"));
 
-	if (!use_stdout)
-		output_directory = set_outdir(prefix, output_directory);
-	else
+	if (use_stdout) {
 		setup_pager();
-
-	if (output_directory) {
+	} else {
 		int saved;
+
+		if (!output_directory)
+			output_directory = config_output_directory;
+		output_directory = set_outdir(prefix, output_directory);
+
 		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
 			rev.diffopt.use_color = GIT_COLOR_NEVER;
-		if (use_stdout)
-			die(_("standard output, or directory, which one?"));
 		/*
 		 * We consider <outdir> as 'outside of gitdir', therefore avoid
 		 * applying adjust_shared_perm in s-c-l-d.
-- 
2.29.2.559.g8ec94df761

Re: [PATCH 1/3] format-patch: refactor output selection

[Eric Sunshine]

On Wed, Nov 4, 2020 at 8:25 AM Jeff King <peff@peff.net> wrote:
> The --stdout and --output-directory options are mutually exclusive, but
> it's hard to tell from reading the code. We have three separate
> conditionals that check for use_stdout, and it's only after we've set up
> the output_directory fully that we check whether the user also specified
> --stdout.
>
> Instead, let's check the exclusion explicitly first, then have a single
> conditional that handles stdout versus an output directory. This is
> slightly easier to follow now, and also will keep things sane when we
> add another output mode in a future patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> +       if (use_stdout + !!output_directory > 1)
> +               die(_("specify only one of --stdout, --output, and --output-directory"));

Is mention of --output intentional here? The commit message only talks
about --stdout and --output-directory.

It's subjective, but "mutually exclusive" sounds a bit more consistent
with other similar error messages elsewhere:

    --stdout, --output, and --output-directory are mutually exclusive

Re: [PATCH 1/3] format-patch: refactor output selection

[Jeff King]

On Wed, Nov 04, 2020 at 12:01:53PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > +       if (use_stdout + !!output_directory > 1)
> > +               die(_("specify only one of --stdout, --output, and --output-directory"));
> 
> Is mention of --output intentional here? The commit message only talks
> about --stdout and --output-directory.

Whoops, thanks. I wrote this line after adding the new feature, but
forgot to revise it when I rebased.

> It's subjective, but "mutually exclusive" sounds a bit more consistent
> with other similar error messages elsewhere:
> 
>     --stdout, --output, and --output-directory are mutually exclusive

Yeah, that reads better. I remember I reworded it a few times to try to
get it not-awkward, but I'm not sure how I failed to come up with that
quite obvious wording. ;)

-Peff

[PATCH 2/3] format-patch: tie file-opening logic to output_directory

[Jeff King]

In format-patch we're either outputting to stdout or to individual files
in an output directory (which maybe just "./"). Our logic for whether to
open a new file for each patch is checked with "!use_stdout", but it is
equally correct to check for a non-NULL output_directory.

The distinction will matter when we add a new single-stream output in a
future patch, when only one of the three methods will want individual
files. Let's swap the logic here in preparation.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4c391ba3ca..27c6d612e4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1153,7 +1153,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)
 	}
 }
 
-static void make_cover_letter(struct rev_info *rev, int use_stdout,
+static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
@@ -1173,7 +1173,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	committer = git_committer_info(0);
 
-	if (!use_stdout &&
+	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		die(_("failed to create cover-letter file"));
 
@@ -2117,7 +2117,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout,
+		make_cover_letter(&rev, !!output_directory,
 				  origin, nr, list, branch_name, quiet);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
@@ -2172,7 +2172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, oid_to_hex(&commit->object.oid));
 		}
 
-		if (!use_stdout &&
+		if (output_directory &&
 		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
@@ -2185,7 +2185,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		 * the log; when using one file per patch, we do
 		 * not want the extra blank line.
 		 */
-		if (!use_stdout)
+		if (output_directory)
 			rev.shown_one = 0;
 		if (shown) {
 			print_bases(&bases, rev.diffopt.file);
@@ -2196,7 +2196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			else
 				print_signature(rev.diffopt.file);
 		}
-		if (!use_stdout)
+		if (output_directory)
 			fclose(rev.diffopt.file);
 	}
 	stop_progress(&progress);
-- 
2.29.2.559.g8ec94df761

Re: [PATCH 2/3] format-patch: tie file-opening logic to output_directory

[Eric Sunshine]

On Wed, Nov 4, 2020 at 8:27 AM Jeff King <peff@peff.net> wrote:
> In format-patch we're either outputting to stdout or to individual files
> in an output directory (which maybe just "./"). Our logic for whether to

s/maybe/may be/

> open a new file for each patch is checked with "!use_stdout", but it is
> equally correct to check for a non-NULL output_directory.
>
> The distinction will matter when we add a new single-stream output in a
> future patch, when only one of the three methods will want individual
> files. Let's swap the logic here in preparation.
>
> Signed-off-by: Jeff King <peff@peff.net>

[PATCH 3/3] format-patch: support --output option

[Jeff King]

We've never intended to support diff's --output option in format-patch.
And until baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We
first parse the format-patch options before handing the remainder off to
setup_revisions(). Before that commit, we'd accept "--output=foo" as an
abbreviation for "--output-directory=foo". But afterwards, we don't
check abbreviations, and --output gets passed to the diff code.

This results in nonsense behavior and bugs. The diff code will have
opened a filehandle at rev.diffopt.file, but we'll overwrite that with
our own handles that we open for each individual patch file. So the
--output file will always just be empty. But worse, the diff code also
sets rev.diffopt.close_file, so log_tree_commit() will close the
filehandle itself. And then the main loop in cmd_format_patch() will try
to close it again, resulting in a double-free.

The simplest solution would be to just disallow --output with
format-patch, as nobody ever intended it to work. However, we have
accidentally documented it (because format-patch includes diff-options).
And it does work with "git log", which writes the whole output to the
specified file. It's easy enough to make that work for format-patch,
too: it's really the same as --stdout, but pointed at a specific file.

We can detect the use of the --output option by the "close_file" flag
(note that we can't use rev.diffopt.file, since the diff setup will
otherwise set it to stdout). So we just need to unset that flag, but
don't have to do anything else. Our situation is otherwise exactly like
--stdout (note that we don't fclose() the file, but nor does the stdout
case; exiting the program takes care of that for us).

Reported-by: Johannes Postler <johannes.postler@txture.io>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c           |  9 ++++++++-
 t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 27c6d612e4..ddb3ea7326 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	if (use_stdout + !!output_directory > 1)
+	if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
 		die(_("specify only one of --stdout, --output, and --output-directory"));
 
 	if (use_stdout) {
 		setup_pager();
+	} else if (rev.diffopt.close_file) {
+		/*
+		 * The diff code parsed --output; it has already opened the
+		 * file, but but we must instruct it not to close after each
+		 * diff.
+		 */
+		rev.diffopt.close_file = 0;
 	} else {
 		int saved;
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 294e76c860..64574c51c1 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 	test_path_is_dir patchset
 '
 
+test_expect_success 'format-patch forbids multiple outputs' '
+	rm -fr outfile outdir &&
+	test_must_fail \
+		git format-patch --stdout --output=outfile &&
+	test_must_fail \
+		git format-patch --stdout --output-directory=outdir &&
+	test_must_fail \
+		git format-patch --output=outfile --output-directory=outdir
+'
+
+test_expect_success 'configured outdir does not conflict with output options' '
+	rm -fr outfile outdir &&
+	test_config format.outputDirectory outdir &&
+	git format-patch --stdout &&
+	test_path_is_missing outdir &&
+	git format-patch --output=outfile &&
+	test_path_is_missing outdir
+'
+
+test_expect_success 'format-patch --output' '
+	rm -fr outfile &&
+	git format-patch -3 --stdout HEAD >expect &&
+	git format-patch -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
+test_expect_success 'format-patch --cover-letter --output' '
+	rm -fr outfile &&
+	git format-patch --cover-letter -3 --stdout HEAD >expect &&
+	git format-patch --cover-letter -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
 test_expect_success 'format-patch --base' '
 	git checkout patchid &&
 
-- 
2.29.2.559.g8ec94df761

Re: [PATCH 3/3] format-patch: support --output option

[Eric Sunshine]

On Wed, Nov 4, 2020 at 8:29 AM Jeff King <peff@peff.net> wrote:
> [...]
> The simplest solution would be to just disallow --output with
> format-patch, as nobody ever intended it to work. However, we have
> accidentally documented it (because format-patch includes diff-options).
> And it does work with "git log", which writes the whole output to the
> specified file. It's easy enough to make that work for format-patch,
> too: it's really the same as --stdout, but pointed at a specific file.
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> +       } else if (rev.diffopt.close_file) {
> +               /*
> +                * The diff code parsed --output; it has already opened the
> +                * file, but but we must instruct it not to close after each
> +                * diff.
> +                */
> +               rev.diffopt.close_file = 0;
>         } else {

The commit message's justification for supporting --output seems
reasonable. However, my knee-jerk reaction to the implementation was
that it feels overly magical and a bit too hacky. I can see the logic
in it but it also leaves a bad taste when the implementation has to
"undo" a side-effect of some other piece of code, which makes it feel
unplanned and fragile. The question which popped into my mind
immediately was "why not handle --output explicitly via
builtin_format_patch_options[] along with other first-class options?".
This review comment may or may not be actionable; it's just expressing
surprise and a bit of nose-wrinkling I experienced.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -1919,6 +1919,39 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
> +test_expect_success 'format-patch forbids multiple outputs' '
> +       rm -fr outfile outdir &&
> +       test_must_fail \
> +               git format-patch --stdout --output=outfile &&
> +       test_must_fail \
> +               git format-patch --stdout --output-directory=outdir &&
> +       test_must_fail \
> +               git format-patch --output=outfile --output-directory=outdir
> +'

I would have expected to see this test added in patch [1/3], and then
extended by this patch with the addition of the --output case(s).

Re: [PATCH 3/3] format-patch: support --output option

[Jeff King]

On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > +       } else if (rev.diffopt.close_file) {
> > +               /*
> > +                * The diff code parsed --output; it has already opened the
> > +                * file, but but we must instruct it not to close after each
> > +                * diff.
> > +                */
> > +               rev.diffopt.close_file = 0;
> >         } else {
> 
> The commit message's justification for supporting --output seems
> reasonable. However, my knee-jerk reaction to the implementation was
> that it feels overly magical and a bit too hacky. I can see the logic
> in it but it also leaves a bad taste when the implementation has to
> "undo" a side-effect of some other piece of code, which makes it feel
> unplanned and fragile. The question which popped into my mind
> immediately was "why not handle --output explicitly via
> builtin_format_patch_options[] along with other first-class options?".
> This review comment may or may not be actionable; it's just expressing
> surprise and a bit of nose-wrinkling I experienced.

I agree it's a bit magical. I didn't really consider writing a new
"--output" option for format-patch, because I came at it from the angle
of "let's unbreak the existing diff --output option". Which isn't
necessarily a defense, but just an explanation.

FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log
does it. So while I agree it's a bit ugly, this is the intended way for
--output to be used across multiple diffs, and with log_tree_commit().
I'd prefer to go this way for now, and if somebody wants to make it less
ugly, they can clean up all of the callers in one go.

Amusingly, 6ea57703f6 (log: prepare log/log-tree to reuse the
diffopt.close_file attribute, 2016-06-22) which introduced this code
foresaw the format-patch bug here.

-Peff

Re: [PATCH 3/3] format-patch: support --output option

[Eric Sunshine]

On Wed, Nov 4, 2020 at 2:16 PM Jeff King <peff@peff.net> wrote:
> On Wed, Nov 04, 2020 at 12:27:30PM -0500, Eric Sunshine wrote:
> > The commit message's justification for supporting --output seems
> > reasonable. However, my knee-jerk reaction to the implementation was
> > that it feels overly magical and a bit too hacky. I can see the logic
> > in it but it also leaves a bad taste when the implementation has to
> > "undo" a side-effect of some other piece of code, which makes it feel
> > unplanned and fragile. [...]
>
> FWIW, that unsetting of rev.diffopt.close_file is exactly how git-log
> does it. So while I agree it's a bit ugly, this is the intended way for
> --output to be used across multiple diffs, and with log_tree_commit().
> I'd prefer to go this way for now, and if somebody wants to make it less
> ugly, they can clean up all of the callers in one go.

If you intend to re-roll, it might make sense to include this
explanation in the commit message since existing precedent helps sell
the ugliness (bad taste), making for a less negative knee-jerk
reaction.

Re: [PATCH 3/3] format-patch: support --output option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +test_expect_success 'format-patch --output' '
> +	rm -fr outfile &&
> +	git format-patch -3 --stdout HEAD >expect &&
> +	git format-patch -3 --output=outfile HEAD &&
> +	test_cmp expect outfile
> +'
> +
> +test_expect_success 'format-patch --cover-letter --output' '
> +	rm -fr outfile &&
> +	git format-patch --cover-letter -3 --stdout HEAD >expect &&
> +	git format-patch --cover-letter -3 --output=outfile HEAD &&
> +	test_cmp expect outfile
> +'

It is pleasing to see an obvious and clear demonstration that
"--output=X" is equivalent to "--stdout >X".

[PATCH v2] format-patch --output

[Jeff King]

On Wed, Nov 04, 2020 at 08:24:28AM -0500, Jeff King wrote:

> The issue is that "--output" was never supposed to work with
> format-patch. But a subtle change in the option parsing a while back
> caused it to be respected. And as you noticed, the documentation
> mistakenly mentions the option, since format-patch includes the standard
> diff-options text.
> 
> So one obvious fix would be to forbid it and adjust the documentation.
> But because of the way the option parsers interact, it's surprisingly
> hard to do so cleanly. It's actually easier to just make it do something
> useful (i.e., behave like --stdout but sent to a file). So I did that
> instead.
> 
>   [1/3]: format-patch: refactor output selection
>   [2/3]: format-patch: tie file-opening logic to output_directory
>   [3/3]: format-patch: support --output option

Here's a re-roll taking into account the comments from Eric. The only
thing I didn't do is rewrite --output as a format-patch option. As I
said in the thread, I'd rather keep parity with how git-log works here
(though I don't mind if somebody wants to do further clean up on top).

  [1/3]: format-patch: refactor output selection
  [2/3]: format-patch: tie file-opening logic to output_directory
  [3/3]: format-patch: support --output option

 builtin/log.c           | 37 ++++++++++++++++++++++---------------
 t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 15 deletions(-)


Range diff from v1:

1:  49e8b54549 ! 1:  9206d6852b format-patch: refactor output selection
    @@ Commit message
         slightly easier to follow now, and also will keep things sane when we
         add another output mode in a future patch.
     
    +    We'll add a few tests as well, covering the mutual exclusion and the
    +    fact that we are not confused by a configured output directory.
    +
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## builtin/log.c ##
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
     -	if (!output_directory && !use_stdout)
     -		output_directory = config_output_directory;
     +	if (use_stdout + !!output_directory > 1)
    -+		die(_("specify only one of --stdout, --output, and --output-directory"));
    ++		die(_("--stdout and --output-directory are mutually exclusive"));
      
     -	if (!use_stdout)
     -		output_directory = set_outdir(prefix, output_directory);
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		/*
      		 * We consider <outdir> as 'outside of gitdir', therefore avoid
      		 * applying adjust_shared_perm in s-c-l-d.
    +
    + ## t/t4014-format-patch.sh ##
    +@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
    + 	test_path_is_dir patchset
    + '
    + 
    ++test_expect_success 'format-patch forbids multiple outputs' '
    ++	rm -fr outdir &&
    ++	test_must_fail \
    ++		git format-patch --stdout --output-directory=outdir
    ++'
    ++
    ++test_expect_success 'configured outdir does not conflict with output options' '
    ++	rm -fr outfile outdir &&
    ++	test_config format.outputDirectory outdir &&
    ++	git format-patch --stdout &&
    ++	test_path_is_missing outdir
    ++'
    ++
    + test_expect_success 'format-patch --base' '
    + 	git checkout patchid &&
    + 
2:  884c06861d ! 2:  9dc30924b2 format-patch: tie file-opening logic to output_directory
    @@ Commit message
         format-patch: tie file-opening logic to output_directory
     
         In format-patch we're either outputting to stdout or to individual files
    -    in an output directory (which maybe just "./"). Our logic for whether to
    -    open a new file for each patch is checked with "!use_stdout", but it is
    -    equally correct to check for a non-NULL output_directory.
    +    in an output directory (which may be just "./"). Our logic for whether
    +    to open a new file for each patch is checked with "!use_stdout", but it
    +    is equally correct to check for a non-NULL output_directory.
     
         The distinction will matter when we add a new single-stream output in a
         future patch, when only one of the three methods will want individual
3:  8befceb150 ! 3:  2b0fab9b50 format-patch: support --output option
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		load_display_notes(&rev.notes_opt);
      
     -	if (use_stdout + !!output_directory > 1)
    +-		die(_("--stdout and --output-directory are mutually exclusive"));
     +	if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
    - 		die(_("specify only one of --stdout, --output, and --output-directory"));
    ++		die(_("--stdout, --output, and --output-directory are mutually exclusive"));
      
      	if (use_stdout) {
      		setup_pager();
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
     
      ## t/t4014-format-patch.sh ##
     @@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
    - 	test_path_is_dir patchset
    - '
    - 
    -+test_expect_success 'format-patch forbids multiple outputs' '
    -+	rm -fr outfile outdir &&
    + test_expect_success 'format-patch forbids multiple outputs' '
    + 	rm -fr outdir &&
    + 	test_must_fail \
    +-		git format-patch --stdout --output-directory=outdir
    ++		git format-patch --stdout --output-directory=outdir &&
     +	test_must_fail \
     +		git format-patch --stdout --output=outfile &&
     +	test_must_fail \
    -+		git format-patch --stdout --output-directory=outdir &&
    -+	test_must_fail \
     +		git format-patch --output=outfile --output-directory=outdir
    -+'
    -+
    -+test_expect_success 'configured outdir does not conflict with output options' '
    -+	rm -fr outfile outdir &&
    -+	test_config format.outputDirectory outdir &&
    -+	git format-patch --stdout &&
    + '
    + 
    + test_expect_success 'configured outdir does not conflict with output options' '
    + 	rm -fr outfile outdir &&
    + 	test_config format.outputDirectory outdir &&
    + 	git format-patch --stdout &&
     +	test_path_is_missing outdir &&
     +	git format-patch --output=outfile &&
    -+	test_path_is_missing outdir
    -+'
    -+
    + 	test_path_is_missing outdir
    + '
    + 
     +test_expect_success 'format-patch --output' '
     +	rm -fr outfile &&
     +	git format-patch -3 --stdout HEAD >expect &&

[PATCH v2 1/3] format-patch: refactor output selection

[Jeff King]

The --stdout and --output-directory options are mutually exclusive, but
it's hard to tell from reading the code. We have three separate
conditionals that check for use_stdout, and it's only after we've set up
the output_directory fully that we check whether the user also specified
--stdout.

Instead, let's check the exclusion explicitly first, then have a single
conditional that handles stdout versus an output directory. This is
slightly easier to follow now, and also will keep things sane when we
add another output mode in a future patch.

We'll add a few tests as well, covering the mutual exclusion and the
fact that we are not confused by a configured output directory.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c           | 18 +++++++++---------
 t/t4014-format-patch.sh | 13 +++++++++++++
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9f939e6cdf..fbff5493d2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1942,20 +1942,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	if (!output_directory && !use_stdout)
-		output_directory = config_output_directory;
+	if (use_stdout + !!output_directory > 1)
+		die(_("--stdout and --output-directory are mutually exclusive"));
 
-	if (!use_stdout)
-		output_directory = set_outdir(prefix, output_directory);
-	else
+	if (use_stdout) {
 		setup_pager();
-
-	if (output_directory) {
+	} else {
 		int saved;
+
+		if (!output_directory)
+			output_directory = config_output_directory;
+		output_directory = set_outdir(prefix, output_directory);
+
 		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
 			rev.diffopt.use_color = GIT_COLOR_NEVER;
-		if (use_stdout)
-			die(_("standard output, or directory, which one?"));
 		/*
 		 * We consider <outdir> as 'outside of gitdir', therefore avoid
 		 * applying adjust_shared_perm in s-c-l-d.
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 294e76c860..e8d6156a6a 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1919,6 +1919,19 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 	test_path_is_dir patchset
 '
 
+test_expect_success 'format-patch forbids multiple outputs' '
+	rm -fr outdir &&
+	test_must_fail \
+		git format-patch --stdout --output-directory=outdir
+'
+
+test_expect_success 'configured outdir does not conflict with output options' '
+	rm -fr outdir &&
+	test_config format.outputDirectory outdir &&
+	git format-patch --stdout &&
+	test_path_is_missing outdir
+'
+
 test_expect_success 'format-patch --base' '
 	git checkout patchid &&
 
-- 
2.29.2.559.g8ec94df761

[PATCH v2 2/3] format-patch: tie file-opening logic to output_directory

[Jeff King]

In format-patch we're either outputting to stdout or to individual files
in an output directory (which may be just "./"). Our logic for whether
to open a new file for each patch is checked with "!use_stdout", but it
is equally correct to check for a non-NULL output_directory.

The distinction will matter when we add a new single-stream output in a
future patch, when only one of the three methods will want individual
files. Let's swap the logic here in preparation.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index fbff5493d2..927156fb85 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1153,7 +1153,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)
 	}
 }
 
-static void make_cover_letter(struct rev_info *rev, int use_stdout,
+static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
@@ -1173,7 +1173,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	committer = git_committer_info(0);
 
-	if (!use_stdout &&
+	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		die(_("failed to create cover-letter file"));
 
@@ -2117,7 +2117,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout,
+		make_cover_letter(&rev, !!output_directory,
 				  origin, nr, list, branch_name, quiet);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
@@ -2172,7 +2172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, oid_to_hex(&commit->object.oid));
 		}
 
-		if (!use_stdout &&
+		if (output_directory &&
 		    open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
@@ -2185,7 +2185,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		 * the log; when using one file per patch, we do
 		 * not want the extra blank line.
 		 */
-		if (!use_stdout)
+		if (output_directory)
 			rev.shown_one = 0;
 		if (shown) {
 			print_bases(&bases, rev.diffopt.file);
@@ -2196,7 +2196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			else
 				print_signature(rev.diffopt.file);
 		}
-		if (!use_stdout)
+		if (output_directory)
 			fclose(rev.diffopt.file);
 	}
 	stop_progress(&progress);
-- 
2.29.2.559.g8ec94df761

[PATCH v2 3/3] format-patch: support --output option

[Jeff King]

We've never intended to support diff's --output option in format-patch.
And until baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27), it was impossible to trigger. We
first parse the format-patch options before handing the remainder off to
setup_revisions(). Before that commit, we'd accept "--output=foo" as an
abbreviation for "--output-directory=foo". But afterwards, we don't
check abbreviations, and --output gets passed to the diff code.

This results in nonsense behavior and bugs. The diff code will have
opened a filehandle at rev.diffopt.file, but we'll overwrite that with
our own handles that we open for each individual patch file. So the
--output file will always just be empty. But worse, the diff code also
sets rev.diffopt.close_file, so log_tree_commit() will close the
filehandle itself. And then the main loop in cmd_format_patch() will try
to close it again, resulting in a double-free.

The simplest solution would be to just disallow --output with
format-patch, as nobody ever intended it to work. However, we have
accidentally documented it (because format-patch includes diff-options).
And it does work with "git log", which writes the whole output to the
specified file. It's easy enough to make that work for format-patch,
too: it's really the same as --stdout, but pointed at a specific file.

We can detect the use of the --output option by the "close_file" flag
(note that we can't use rev.diffopt.file, since the diff setup will
otherwise set it to stdout). So we just need to unset that flag, but
don't have to do anything else. Our situation is otherwise exactly like
--stdout (note that we don't fclose() the file, but nor does the stdout
case; exiting the program takes care of that for us).

Reported-by: Johannes Postler <johannes.postler@txture.io>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c           | 11 +++++++++--
 t/t4014-format-patch.sh | 26 +++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 927156fb85..662c041de2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1942,11 +1942,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.show_notes)
 		load_display_notes(&rev.notes_opt);
 
-	if (use_stdout + !!output_directory > 1)
-		die(_("--stdout and --output-directory are mutually exclusive"));
+	if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
+		die(_("--stdout, --output, and --output-directory are mutually exclusive"));
 
 	if (use_stdout) {
 		setup_pager();
+	} else if (rev.diffopt.close_file) {
+		/*
+		 * The diff code parsed --output; it has already opened the
+		 * file, but but we must instruct it not to close after each
+		 * diff.
+		 */
+		rev.diffopt.close_file = 0;
 	} else {
 		int saved;
 
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e8d6156a6a..42588bf6e1 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1920,18 +1920,38 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 '
 
 test_expect_success 'format-patch forbids multiple outputs' '
-	rm -fr outdir &&
+	rm -fr outfile outdir &&
 	test_must_fail \
-		git format-patch --stdout --output-directory=outdir
+		git format-patch --stdout --output-directory=outdir &&
+	test_must_fail \
+		git format-patch --stdout --output=outfile &&
+	test_must_fail \
+		git format-patch --output=outfile --output-directory=outdir
 '
 
 test_expect_success 'configured outdir does not conflict with output options' '
-	rm -fr outdir &&
+	rm -fr outfile outdir &&
 	test_config format.outputDirectory outdir &&
 	git format-patch --stdout &&
+	test_path_is_missing outdir &&
+	git format-patch --output=outfile &&
 	test_path_is_missing outdir
 '
 
+test_expect_success 'format-patch --output' '
+	rm -fr outfile &&
+	git format-patch -3 --stdout HEAD >expect &&
+	git format-patch -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
+test_expect_success 'format-patch --cover-letter --output' '
+	rm -fr outfile &&
+	git format-patch --cover-letter -3 --stdout HEAD >expect &&
+	git format-patch --cover-letter -3 --output=outfile HEAD &&
+	test_cmp expect outfile
+'
+
 test_expect_success 'format-patch --base' '
 	git checkout patchid &&
 
-- 
2.29.2.559.g8ec94df761

Re: [PATCH v2] format-patch --output

[Johannes Postler]

Thanks a lot everyone for fixing this so quickly!
Please let me know if there is anything else I can do.
Best,
johannes

Johannes Postler
DevOps & Consulting


Txture - The Cloud Transformation Platform
Grabenweg 68
A-6020 Innsbruck
johannes.postler@txture.io
www.txture.io



Johannes Postler
DevOps & Consulting



Txture - The Cloud Transformation Platform
Grabenweg 68
A-6020 Innsbruck
johannes.postler@txture.io
www.txture.io

Follow us:


On Wed, Nov 4, 2020 at 8:26 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Nov 04, 2020 at 08:24:28AM -0500, Jeff King wrote:
>
> > The issue is that "--output" was never supposed to work with
> > format-patch. But a subtle change in the option parsing a while back
> > caused it to be respected. And as you noticed, the documentation
> > mistakenly mentions the option, since format-patch includes the standard
> > diff-options text.
> >
> > So one obvious fix would be to forbid it and adjust the documentation.
> > But because of the way the option parsers interact, it's surprisingly
> > hard to do so cleanly. It's actually easier to just make it do something
> > useful (i.e., behave like --stdout but sent to a file). So I did that
> > instead.
> >
> >   [1/3]: format-patch: refactor output selection
> >   [2/3]: format-patch: tie file-opening logic to output_directory
> >   [3/3]: format-patch: support --output option
>
> Here's a re-roll taking into account the comments from Eric. The only
> thing I didn't do is rewrite --output as a format-patch option. As I
> said in the thread, I'd rather keep parity with how git-log works here
> (though I don't mind if somebody wants to do further clean up on top).
>
>   [1/3]: format-patch: refactor output selection
>   [2/3]: format-patch: tie file-opening logic to output_directory
>   [3/3]: format-patch: support --output option
>
>  builtin/log.c           | 37 ++++++++++++++++++++++---------------
>  t/t4014-format-patch.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 15 deletions(-)
>
>
> Range diff from v1:
>
> 1:  49e8b54549 ! 1:  9206d6852b format-patch: refactor output selection
>     @@ Commit message
>          slightly easier to follow now, and also will keep things sane when we
>          add another output mode in a future patch.
>
>     +    We'll add a few tests as well, covering the mutual exclusion and the
>     +    fact that we are not confused by a configured output directory.
>     +
>          Signed-off-by: Jeff King <peff@peff.net>
>
>       ## builtin/log.c ##
>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
>      -  if (!output_directory && !use_stdout)
>      -          output_directory = config_output_directory;
>      +  if (use_stdout + !!output_directory > 1)
>     -+          die(_("specify only one of --stdout, --output, and --output-directory"));
>     ++          die(_("--stdout and --output-directory are mutually exclusive"));
>
>      -  if (!use_stdout)
>      -          output_directory = set_outdir(prefix, output_directory);
>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
>                 /*
>                  * We consider <outdir> as 'outside of gitdir', therefore avoid
>                  * applying adjust_shared_perm in s-c-l-d.
>     +
>     + ## t/t4014-format-patch.sh ##
>     +@@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
>     +   test_path_is_dir patchset
>     + '
>     +
>     ++test_expect_success 'format-patch forbids multiple outputs' '
>     ++  rm -fr outdir &&
>     ++  test_must_fail \
>     ++          git format-patch --stdout --output-directory=outdir
>     ++'
>     ++
>     ++test_expect_success 'configured outdir does not conflict with output options' '
>     ++  rm -fr outfile outdir &&
>     ++  test_config format.outputDirectory outdir &&
>     ++  git format-patch --stdout &&
>     ++  test_path_is_missing outdir
>     ++'
>     ++
>     + test_expect_success 'format-patch --base' '
>     +   git checkout patchid &&
>     +
> 2:  884c06861d ! 2:  9dc30924b2 format-patch: tie file-opening logic to output_directory
>     @@ Commit message
>          format-patch: tie file-opening logic to output_directory
>
>          In format-patch we're either outputting to stdout or to individual files
>     -    in an output directory (which maybe just "./"). Our logic for whether to
>     -    open a new file for each patch is checked with "!use_stdout", but it is
>     -    equally correct to check for a non-NULL output_directory.
>     +    in an output directory (which may be just "./"). Our logic for whether
>     +    to open a new file for each patch is checked with "!use_stdout", but it
>     +    is equally correct to check for a non-NULL output_directory.
>
>          The distinction will matter when we add a new single-stream output in a
>          future patch, when only one of the three methods will want individual
> 3:  8befceb150 ! 3:  2b0fab9b50 format-patch: support --output option
>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
>                 load_display_notes(&rev.notes_opt);
>
>      -  if (use_stdout + !!output_directory > 1)
>     +-          die(_("--stdout and --output-directory are mutually exclusive"));
>      +  if (use_stdout + rev.diffopt.close_file + !!output_directory > 1)
>     -           die(_("specify only one of --stdout, --output, and --output-directory"));
>     ++          die(_("--stdout, --output, and --output-directory are mutually exclusive"));
>
>         if (use_stdout) {
>                 setup_pager();
>     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
>
>       ## t/t4014-format-patch.sh ##
>      @@ t/t4014-format-patch.sh: test_expect_success 'format-patch -o overrides format.outputDirectory' '
>     -   test_path_is_dir patchset
>     - '
>     -
>     -+test_expect_success 'format-patch forbids multiple outputs' '
>     -+  rm -fr outfile outdir &&
>     + test_expect_success 'format-patch forbids multiple outputs' '
>     +   rm -fr outdir &&
>     +   test_must_fail \
>     +-          git format-patch --stdout --output-directory=outdir
>     ++          git format-patch --stdout --output-directory=outdir &&
>      +  test_must_fail \
>      +          git format-patch --stdout --output=outfile &&
>      +  test_must_fail \
>     -+          git format-patch --stdout --output-directory=outdir &&
>     -+  test_must_fail \
>      +          git format-patch --output=outfile --output-directory=outdir
>     -+'
>     -+
>     -+test_expect_success 'configured outdir does not conflict with output options' '
>     -+  rm -fr outfile outdir &&
>     -+  test_config format.outputDirectory outdir &&
>     -+  git format-patch --stdout &&
>     + '
>     +
>     + test_expect_success 'configured outdir does not conflict with output options' '
>     +   rm -fr outfile outdir &&
>     +   test_config format.outputDirectory outdir &&
>     +   git format-patch --stdout &&
>      +  test_path_is_missing outdir &&
>      +  git format-patch --output=outfile &&
>     -+  test_path_is_missing outdir
>     -+'
>     -+
>     +   test_path_is_missing outdir
>     + '
>     +
>      +test_expect_success 'format-patch --output' '
>      +  rm -fr outfile &&
>      +  git format-patch -3 --stdout HEAD >expect &&