{"thread":{"id":"39470","subject":"seg fault in \"git format-patch\"","startedAt":"2015-05-31T19:13:16Z","lastAt":"2015-06-01T22:46:08Z","messageCount":21,"participants":["Bruce Korb","Christian Couder","brian m. carlson","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"262526","messageId":"556B5D4C.4030406@gmail.com","threadId":"39470","inReplyTo":null,"subject":"seg fault in \"git format-patch\"","fromName":"Bruce Korb","fromEmail":"bruce.korb@gmail.com","sentAt":"2015-05-31T19:13:16Z","receivedAt":"2015-05-31T19:13:16Z","isPatch":false,"sender":{"key":"bruce.korb@gmail.com","avatar":"https://gravatar.com/avatar/86d91467dc7cc8466a9d133a7b93a5d21233052017c144c9a6f6f7e5110344d0?d=mp&s=160"},"body":"$ git format-patch -o patches --ignore-if-in-upstream 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\nSegmentation fault\n/u/gnu/proj/gnu-pw-mgr\n$ git format-patch -o patches 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\npatches/0001-remove-dead-code.patch\npatches/0002-dead-code-removal.patch\npatches/0003-add-sort-pw-cfg-program.patch\npatches/0004-add-doc-for-sort-pw-cfg.patch\npatches/0005-clean-up-doc-makefile.patch\npatches/0006-clean-up-doc-makefile.patch\npatches/0007-happy-2015-and-add-delete-option.patch\npatches/0008-fix-doc-Makefile.am.patch\npatches/0009-re-fix-copyright.patch\npatches/0010-finish-debugging-remove_pwid.patch\npatches/0011-only-update-file-if-something-was-removed.patch\npatches/0012-update-NEWS.patch\npatches/0013-bootstrap-cleanup.patch\n"},{"id":"262530","messageId":"CAP8UFD0Pi3_hF0+S3AXktD5NkBL_Q1mU_oN4fULyZemDEUr8Jg@mail.gmail.com","threadId":"39470","inReplyTo":"556B5D4C.4030406@gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-31T20:26:15Z","receivedAt":"2015-05-31T20:26:15Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, May 31, 2015 at 9:13 PM, Bruce Korb wrote:\n> $ git format-patch -o patches --ignore-if-in-upstream\n> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n> Segmentation fault\n> /u/gnu/proj/gnu-pw-mgr\n> $ git format-patch -o patches\n> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n> patches/0001-remove-dead-code.patch\n> patches/0002-dead-code-removal.patch\n> patches/0003-add-sort-pw-cfg-program.patch\n> patches/0004-add-doc-for-sort-pw-cfg.patch\n> patches/0005-clean-up-doc-makefile.patch\n> patches/0006-clean-up-doc-makefile.patch\n> patches/0007-happy-2015-and-add-delete-option.patch\n> patches/0008-fix-doc-Makefile.am.patch\n> patches/0009-re-fix-copyright.patch\n> patches/0010-finish-debugging-remove_pwid.patch\n> patches/0011-only-update-file-if-something-was-removed.patch\n> patches/0012-update-NEWS.patch\n> patches/0013-bootstrap-cleanup.patch\n\nCould you tell us which git version you are using? You can use \"git --version\".\nThe operating system you are using could also be useful.\nAnd maybe you could also run git under gdb and give us the output of\nthe \"bt\" (backtrace) gdb command when it crashes?\n\nThanks,\nChristian.\n"},{"id":"262532","messageId":"CAKRnqNKVfzt_qMqoxsjMpunUYDNYd8C0jACM69HxGhJHEeVY-Q@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD0Pi3_hF0+S3AXktD5NkBL_Q1mU_oN4fULyZemDEUr8Jg@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Bruce Korb","fromEmail":"bruce.korb@gmail.com","sentAt":"2015-05-31T20:41:29Z","receivedAt":"2015-05-31T20:41:29Z","isPatch":false,"sender":{"key":"bruce.korb@gmail.com","avatar":"https://gravatar.com/avatar/86d91467dc7cc8466a9d133a7b93a5d21233052017c144c9a6f6f7e5110344d0?d=mp&s=160"},"body":"bt won't help much:\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x000000000047e62f in ?? ()\n(gdb) bt\n#0 0x000000000047e62f in ?? ()\n#1 0x000000000047e6ba in ?? ()\n#2 0x000000000043cb9a in ?? ()\n#3 0x000000000043e9cc in ?? ()\n#4 0x000000000040647d in ?? ()\n#5 0x0000000000405863 in ?? ()\n#6 0x00007ffff6fc3be5 in __libc_start_main () from /lib64/libc.so.6\n#7 0x0000000000405cd5 in ?? ()\n\n$ git --version\ngit version 1.8.4.5\n$ rpm -q -a|grep '^git'\ngit-email-1.8.4.5-3.8.4.x86_64\ngitg-0.2.7-3.1.4.x86_64\ngit-cvs-1.8.4.5-3.8.4.x86_64\ngit-svn-1.8.4.5-3.8.4.x86_64\ngit-web-1.8.4.5-3.8.4.x86_64\ngit-gui-1.8.4.5-3.8.4.x86_64\ngitg-lang-0.2.7-3.1.4.noarch\ngit-1.8.4.5-3.8.4.x86_64\ngit-core-1.8.4.5-3.8.4.x86_64\ngitk-1.8.4.5-3.8.4.x86_64\n\n$ head -n 300 /etc/*eleas*\n==> /etc/SuSE-release <==\nopenSUSE 13.1 (x86_64)\nVERSION = 13.1\nCODENAME = Bottle\n# /etc/SuSE-release is deprecated and will be removed in the future,\nuse /etc/os-release instead\n\n==> /etc/lsb-release <==\nLSB_VERSION=\"core-2.0-noarch:core-3.2-noarch:core-4.0-noarch:core-2.0-x86_64:core-3.2-x86_64:core-4.0-x86_64\"\n\n==> /etc/lsb-release.d <==\nhead: error reading '/etc/lsb-release.d': Is a directory\n\n==> /etc/os-release <==\nNAME=openSUSE\nVERSION=\"13.1 (Bottle)\"\nVERSION_ID=\"13.1\"\nPRETTY_NAME=\"openSUSE 13.1 (Bottle) (x86_64)\"\nID=opensuse\nANSI_COLOR=\"0;32\"\nCPE_NAME=\"cpe:/o:opensuse:opensuse:13.1\"\nBUG_REPORT_URL=\"https://bugs.opensuse.org\"\nHOME_URL=\"https://opensuse.org/\"\nID_LIKE=\"suse\"\n\n\nOn Sun, May 31, 2015 at 1:26 PM, Christian Couder\n wrote:\n> On Sun, May 31, 2015 at 9:13 PM, Bruce Korb wrote:\n>> $ git format-patch -o patches --ignore-if-in-upstream\n>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>> Segmentation fault\n>> /u/gnu/proj/gnu-pw-mgr\n>> $ git format-patch -o patches\n>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>> patches/0001-remove-dead-code.patch\n>> patches/0002-dead-code-removal.patch\n>> patches/0003-add-sort-pw-cfg-program.patch\n>> patches/0004-add-doc-for-sort-pw-cfg.patch\n>> patches/0005-clean-up-doc-makefile.patch\n>> patches/0006-clean-up-doc-makefile.patch\n>> patches/0007-happy-2015-and-add-delete-option.patch\n>> patches/0008-fix-doc-Makefile.am.patch\n>> patches/0009-re-fix-copyright.patch\n>> patches/0010-finish-debugging-remove_pwid.patch\n>> patches/0011-only-update-file-if-something-was-removed.patch\n>> patches/0012-update-NEWS.patch\n>> patches/0013-bootstrap-cleanup.patch\n>\n> Could you tell us which git version you are using? You can use \"git --version\".\n> The operating system you are using could also be useful.\n> And maybe you could also run git under gdb and give us the output of\n> the \"bt\" (backtrace) gdb command when it crashes?\n>\n> Thanks,\n> Christian.\n"},{"id":"262533","messageId":"CAKRnqNJnaLioQPWYDmSiBfLCSMGdFR21bAEXRzdpkChDBf2wgw@mail.gmail.com","threadId":"39470","inReplyTo":"CAKRnqNKVfzt_qMqoxsjMpunUYDNYd8C0jACM69HxGhJHEeVY-Q@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Bruce Korb","fromEmail":"bruce.korb@gmail.com","sentAt":"2015-05-31T20:45:11Z","receivedAt":"2015-05-31T20:45:11Z","isPatch":false,"sender":{"key":"bruce.korb@gmail.com","avatar":"https://gravatar.com/avatar/86d91467dc7cc8466a9d133a7b93a5d21233052017c144c9a6f6f7e5110344d0?d=mp&s=160"},"body":"Oh, you can also clone the gnu-pw-mgr and likely get the same result:\n\n$ cat .git/config\n[core]\n repositoryformatversion = 0\n filemode = true\n bare = false\n logallrefupdates = true\n[remote \"origin\"]\n fetch = +refs/heads/*:refs/remotes/origin/*\n url = ssh://git.sv.gnu.org/srv/git/gnu-pw-mgr.git\n[branch \"master\"]\n remote = origin\n merge = refs/heads/master\n\nOn Sun, May 31, 2015 at 1:41 PM, Bruce Korb wrote:\n> bt won't help much:\n>\n> Program received signal SIGSEGV, Segmentation fault.\n> 0x000000000047e62f in ?? ()\n> (gdb) bt\n> #0 0x000000000047e62f in ?? ()\n> #1 0x000000000047e6ba in ?? ()\n> #2 0x000000000043cb9a in ?? ()\n> #3 0x000000000043e9cc in ?? ()\n> #4 0x000000000040647d in ?? ()\n> #5 0x0000000000405863 in ?? ()\n> #6 0x00007ffff6fc3be5 in __libc_start_main () from /lib64/libc.so.6\n> #7 0x0000000000405cd5 in ?? ()\n>\n> $ git --version\n> git version 1.8.4.5\n> $ rpm -q -a|grep '^git'\n> git-email-1.8.4.5-3.8.4.x86_64\n> gitg-0.2.7-3.1.4.x86_64\n> git-cvs-1.8.4.5-3.8.4.x86_64\n> git-svn-1.8.4.5-3.8.4.x86_64\n> git-web-1.8.4.5-3.8.4.x86_64\n> git-gui-1.8.4.5-3.8.4.x86_64\n> gitg-lang-0.2.7-3.1.4.noarch\n> git-1.8.4.5-3.8.4.x86_64\n> git-core-1.8.4.5-3.8.4.x86_64\n> gitk-1.8.4.5-3.8.4.x86_64\n>\n> $ head -n 300 /etc/*eleas*\n> ==> /etc/SuSE-release <==\n> openSUSE 13.1 (x86_64)\n> VERSION = 13.1\n> CODENAME = Bottle\n> # /etc/SuSE-release is deprecated and will be removed in the future,\n> use /etc/os-release instead\n>\n> ==> /etc/lsb-release <==\n> LSB_VERSION=\"core-2.0-noarch:core-3.2-noarch:core-4.0-noarch:core-2.0-x86_64:core-3.2-x86_64:core-4.0-x86_64\"\n>\n> ==> /etc/lsb-release.d <==\n> head: error reading '/etc/lsb-release.d': Is a directory\n>\n> ==> /etc/os-release <==\n> NAME=openSUSE\n> VERSION=\"13.1 (Bottle)\"\n> VERSION_ID=\"13.1\"\n> PRETTY_NAME=\"openSUSE 13.1 (Bottle) (x86_64)\"\n> ID=opensuse\n> ANSI_COLOR=\"0;32\"\n> CPE_NAME=\"cpe:/o:opensuse:opensuse:13.1\"\n> BUG_REPORT_URL=\"https://bugs.opensuse.org\"\n> HOME_URL=\"https://opensuse.org/\"\n> ID_LIKE=\"suse\"\n>\n>\n> On Sun, May 31, 2015 at 1:26 PM, Christian Couder\n> wrote:\n>> On Sun, May 31, 2015 at 9:13 PM, Bruce Korb wrote:\n>>> $ git format-patch -o patches --ignore-if-in-upstream\n>>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>>> Segmentation fault\n>>> /u/gnu/proj/gnu-pw-mgr\n>>> $ git format-patch -o patches\n>>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>>> patches/0001-remove-dead-code.patch\n>>> patches/0002-dead-code-removal.patch\n>>> patches/0003-add-sort-pw-cfg-program.patch\n>>> patches/0004-add-doc-for-sort-pw-cfg.patch\n>>> patches/0005-clean-up-doc-makefile.patch\n>>> patches/0006-clean-up-doc-makefile.patch\n>>> patches/0007-happy-2015-and-add-delete-option.patch\n>>> patches/0008-fix-doc-Makefile.am.patch\n>>> patches/0009-re-fix-copyright.patch\n>>> patches/0010-finish-debugging-remove_pwid.patch\n>>> patches/0011-only-update-file-if-something-was-removed.patch\n>>> patches/0012-update-NEWS.patch\n>>> patches/0013-bootstrap-cleanup.patch\n>>\n>> Could you tell us which git version you are using? You can use \"git --version\".\n>> The operating system you are using could also be useful.\n>> And maybe you could also run git under gdb and give us the output of\n>> the \"bt\" (backtrace) gdb command when it crashes?\n>>\n>> Thanks,\n>> Christian.\n"},{"id":"262547","messageId":"CAP8UFD1rKmKgKqCsffCLyOCny3JEACxgmBN_eqOj_=3zBW-MZg@mail.gmail.com","threadId":"39470","inReplyTo":"CAKRnqNJnaLioQPWYDmSiBfLCSMGdFR21bAEXRzdpkChDBf2wgw@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-31T23:14:41Z","receivedAt":"2015-05-31T23:14:41Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, May 31, 2015 at 10:45 PM, Bruce Korb wrote:\n> Oh, you can also clone the gnu-pw-mgr and likely get the same result:\n\nYeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git\nI get the following backtrace:\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\ncommit=0x84e8d0, mark=139) at commit.c:528\n528 while ((parents = parents->next))\n(gdb) bt\n#0 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\ncommit=0x84e8d0, mark=139) at commit.c:528\n#1 0x00000000004b2743 in clear_commit_marks_many (nr=-1,\ncommit=0x7fffffffbfa0, mark=139) at commit.c:544\n#2 0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,\nmark=139) at commit.c:549\n#3 0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,\nids=0x7fffffffc910) at builtin/log.c:832\n#4 0x0000000000455580 in cmd_format_patch (argc=1,\nargv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425\n#5 0x0000000000405807 in run_builtin (p=0x80cac8 ,\nargc=5, argv=0x7fffffffdc20) at git.c:350\n#6 0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)\nat git.c:532\n#7 0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,\nargv=0x7fffffffdb10) at git.c:578\n#8 0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686\n\n(Please don't top post if you reply to this email as it is frown upon\non this list.)\n"},{"id":"262549","messageId":"CAP8UFD0_RCOHUF6BgczgS5kWAFc0QKdw4cUy_bpB2jhd+kYWdw@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD1rKmKgKqCsffCLyOCny3JEACxgmBN_eqOj_=3zBW-MZg@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-31T23:53:45Z","receivedAt":"2015-05-31T23:53:45Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder\n wrote:\n> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb wrote:\n>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:\n>\n> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git\n> I get the following backtrace:\n>\n> Program received signal SIGSEGV, Segmentation fault.\n> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n> commit=0x84e8d0, mark=139) at commit.c:528\n> 528 while ((parents = parents->next))\n> (gdb) bt\n> #0 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n> commit=0x84e8d0, mark=139) at commit.c:528\n> #1 0x00000000004b2743 in clear_commit_marks_many (nr=-1,\n> commit=0x7fffffffbfa0, mark=139) at commit.c:544\n> #2 0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,\n> mark=139) at commit.c:549\n> #3 0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,\n> ids=0x7fffffffc910) at builtin/log.c:832\n> #4 0x0000000000455580 in cmd_format_patch (argc=1,\n> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425\n> #5 0x0000000000405807 in run_builtin (p=0x80cac8 ,\n> argc=5, argv=0x7fffffffdc20) at git.c:350\n> #6 0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)\n> at git.c:532\n> #7 0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,\n> argv=0x7fffffffdb10) at git.c:578\n> #8 0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686\n>\n> (Please don't top post if you reply to this email as it is frown upon\n> on this list.)\n\nWhen running the command that gives the above segfault:\n\n$ git format-patch -o patches --ignore-if-in-upstream\n14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n\nIt is interesting to note that the last sha1 refers to a tag:\n\n$ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d\nobject 524ccbdbe319068ab18a3950119b9e9a5d135783\ntype commit\ntag v1.4\ntagger Bruce Korb 1428847577 -0700\n\nRelease 1.4\n\n* sort-pw-cfg: a sort/merge program for combining and organizing\n configurations.\n\n* --delete: a new option to remove any entries for a password id\n\nIt works when the tag is replaced by the commit it points to, and the\nsegfault happens because the we try to access the \"parents\" field of\nthe tag object as if it was a commit.\n"},{"id":"262550","messageId":"CAP8UFD1phg8E0JCgkz88CMUo9H-W=s5JDuKeCMOkf1=UYBJt+g@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD0_RCOHUF6BgczgS5kWAFc0QKdw4cUy_bpB2jhd+kYWdw@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-06-01T00:01:22Z","receivedAt":"2015-06-01T00:01:22Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 1, 2015 at 1:53 AM, Christian Couder\n wrote:\n> On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder\n> wrote:\n>> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb wrote:\n>>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:\n>>\n>> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git\n>> I get the following backtrace:\n>>\n>> Program received signal SIGSEGV, Segmentation fault.\n>> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n>> commit=0x84e8d0, mark=139) at commit.c:528\n>> 528 while ((parents = parents->next))\n>> (gdb) bt\n>> #0 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n>> commit=0x84e8d0, mark=139) at commit.c:528\n>> #1 0x00000000004b2743 in clear_commit_marks_many (nr=-1,\n>> commit=0x7fffffffbfa0, mark=139) at commit.c:544\n>> #2 0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,\n>> mark=139) at commit.c:549\n>> #3 0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,\n>> ids=0x7fffffffc910) at builtin/log.c:832\n>> #4 0x0000000000455580 in cmd_format_patch (argc=1,\n>> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425\n>> #5 0x0000000000405807 in run_builtin (p=0x80cac8 ,\n>> argc=5, argv=0x7fffffffdc20) at git.c:350\n>> #6 0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)\n>> at git.c:532\n>> #7 0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,\n>> argv=0x7fffffffdb10) at git.c:578\n>> #8 0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686\n>>\n>> (Please don't top post if you reply to this email as it is frown upon\n>> on this list.)\n>\n> When running the command that gives the above segfault:\n>\n> $ git format-patch -o patches --ignore-if-in-upstream\n> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>\n> It is interesting to note that the last sha1 refers to a tag:\n>\n> $ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d\n> object 524ccbdbe319068ab18a3950119b9e9a5d135783\n> type commit\n> tag v1.4\n> tagger Bruce Korb 1428847577 -0700\n>\n> Release 1.4\n>\n> * sort-pw-cfg: a sort/merge program for combining and organizing\n> configurations.\n>\n> * --delete: a new option to remove any entries for a password id\n>\n> It works when the tag is replaced by the commit it points to, and the\n> segfault happens because the we try to access the \"parents\" field of\n> the tag object as if it was a commit.\n\nYeah, in builtin/log.c we are doing:\n\n o2 = rev->pending.objects[1].item;\n\nand then we are casting the object into a commit when passing it to\nclear_commit_marks():\n\n clear_commit_marks((struct commit *)o2,\n SEEN | UNINTERESTING | SHOWN | ADDED);\n\nbut I don't know where we should have peeled the tag to get a commit,\nand it's late here so I will leave it someone else to find a fix.\n\nBest,\nChristian.\n"},{"id":"262552","messageId":"1433120593-186980-1-git-send-email-sandals@crustytoothpaste.net","threadId":"39470","inReplyTo":"CAP8UFD1phg8E0JCgkz88CMUo9H-W=s5JDuKeCMOkf1=UYBJt+g@mail.gmail.com","subject":"[PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-06-01T01:03:13Z","receivedAt":"2015-06-01T01:03:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"format-patch would segfault if provided a tag as one of the range\nendpoints in conjunction with --ignore-if-in-upstream, as it assumed the\nobject was a commit and attempted to cast it to struct commit.\nDereference the tag as soon as possible to prevent this, but not until\nafter copying the necessary flags.\n\nReported-by: Bruce Korb \nDiagnosed-by: Christian Couder \nSigned-off-by: brian m. carlson \n---\n builtin/log.c | 6 ++++++\n t/t4014-format-patch.sh | 10 ++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..e0465ba 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \to2 = rev->pending.objects[1].item;\n \tflags2 = o2->flags;\n \n+\to1 = deref_tag(o1, NULL, 0);\n+\to2 = deref_tag(o2, NULL, 0);\n+\n+\tif (!o1 || !o2)\n+\t\tdie(_(\"Invalid tag.\"));\n+\n \tif ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))\n \t\tdie(_(\"Not a range.\"));\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex c39e500..60b9875 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -57,6 +57,16 @@ test_expect_success \"format-patch --ignore-if-in-upstream\" '\n \n '\n \n+test_expect_success \"format-patch --ignore-if-in-upstream handles tags\" '\n+\n+\tgit tag -a v1 -m tag side &&\n+\tgit format-patch --stdout \\\n+\t\t--ignore-if-in-upstream master..v1 >patch1 &&\n+\tcnt=$(grep \"^From \" patch1 | wc -l) &&\n+\ttest $cnt = 2\n+\n+'\n+\n test_expect_success \"format-patch doesn't consider merge commits\" '\n \n \tgit checkout -b slave master &&\n-- \n2.4.0\n"},{"id":"262574","messageId":"20150601102046.GA31792@peff.net","threadId":"39470","inReplyTo":"1433120593-186980-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T10:20:46Z","receivedAt":"2015-06-01T10:20:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 01:03:13AM +0000, brian m. carlson wrote:\n\n> format-patch would segfault if provided a tag as one of the range\n> endpoints in conjunction with --ignore-if-in-upstream, as it assumed the\n> object was a commit and attempted to cast it to struct commit.\n> Dereference the tag as soon as possible to prevent this, but not until\n> after copying the necessary flags.\n\nI bisected Bruce's case earlier to 895c5ba (revision: do not peel tags\nused in range notation, 2013-09-19). This is an obvious fallout from\nthat commit; unlike most traversals which read from rev->commits, we\nread straight from rev->pending here. So I wondered briefly if that\ncommit was not being sufficiently careful.\n\nBut as it turns out, this code was buggy long before then. 895c5ba only\nchanged the range notation. Even before then, if you did:\n\n git format-patch --ignore-if-in-upstream ^v2.2.0 v2.2.1\n\nwe would segfault. Anybody reading from rev->pending should be ready to\nhandle any kind of object.\n\nWhich also makes me wonder about...\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..e0465ba 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n> \to2 = rev->pending.objects[1].item;\n> \tflags2 = o2->flags;\n> \n> +\to1 = deref_tag(o1, NULL, 0);\n> +\to2 = deref_tag(o2, NULL, 0);\n> +\n> +\tif (!o1 || !o2)\n> +\t\tdie(_(\"Invalid tag.\"));\n\nThis will dereference tags, but it won't help at all with:\n\n git format-patch --ignore-if-in-upstream ^HEAD:Makefile HEAD:Documentation\n\nwhere we end up with blobs. That is ridiculous, of course, but we should\ncomplain, not segfault.\n\nSo I think what you really want is lookup_commit_reference. And the\nerror message is really not \"invalid tag\", but \"not a commit\". I think\nyou can just use lookup_commit_or_die.\n\n> \tif ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))\n> \t\tdie(_(\"Not a range.\"));\n\nAs an aside, now that we are dereferencing, these flags are from the\nwrong object. They _should_ be the same (we mark the tag as\nUNINTERESTING, too), but it's a little weird that at the end of the\nfunction we restore the saved flags from the tag object onto the commit.\nJust bumping the assignment of flags{1,2} would work (or just bump up\nthe lookup_commit_or_die call to where we assign to o{1,2}).\n\n> +test_expect_success \"format-patch --ignore-if-in-upstream handles tags\" '\n> +\n> +\tgit tag -a v1 -m tag side &&\n> +\tgit format-patch --stdout \\\n> +\t\t--ignore-if-in-upstream master..v1 >patch1 &&\n> +\tcnt=$(grep \"^From \" patch1 | wc -l) &&\n> +\ttest $cnt = 2\n\nI think this avoids the usual \"wc\" whitespace pitfall because you don't\nuse double-quotes. But maybe:\n\n grep \"^From \" patch1 >count &&\n test_line_count = 2 patch1\n\nwould be more idiomatic.\n\n-Peff\n"},{"id":"262581","messageId":"20150601112212.GA140991@vauxhall.crustytoothpaste.net","threadId":"39470","inReplyTo":"20150601102046.GA31792@peff.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-06-01T11:22:12Z","receivedAt":"2015-06-01T11:22:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jun 01, 2015 at 06:20:46AM -0400, Jeff King wrote:\n> So I think what you really want is lookup_commit_reference. And the\n> error message is really not \"invalid tag\", but \"not a commit\". I think\n> you can just use lookup_commit_or_die.\n\nThanks. That does seem to be what I want.\n\n> As an aside, now that we are dereferencing, these flags are from the\n> wrong object. They _should_ be the same (we mark the tag as\n> UNINTERESTING, too), but it's a little weird that at the end of the\n> function we restore the saved flags from the tag object onto the commit.\n> Just bumping the assignment of flags{1,2} would work (or just bump up\n> the lookup_commit_or_die call to where we assign to o{1,2}).\n\nI tried looking up the flags after dereferencing the tags, but that led\nto the die(\"Not a range.\") being triggered. That's why the commit\nmessage ended up mentioning loading the flags before dereferencing.\n\n> I think this avoids the usual \"wc\" whitespace pitfall because you don't\n> use double-quotes. But maybe:\n> \n> grep \"^From \" patch1 >count &&\n> test_line_count = 2 patch1\n> \n> would be more idiomatic.\n\nI can certainly make that change. I made the test as similar as\npossible to other tests in the area, but I wasn't aware of\ntest_line_count.\n\nI'll reroll the patch later today.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"262583","messageId":"20150601114729.GA5160@peff.net","threadId":"39470","inReplyTo":"20150601112212.GA140991@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T11:47:29Z","receivedAt":"2015-06-01T11:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 11:22:12AM +0000, brian m. carlson wrote:\n\n> > As an aside, now that we are dereferencing, these flags are from the\n> > wrong object. They _should_ be the same (we mark the tag as\n> > UNINTERESTING, too), but it's a little weird that at the end of the\n> > function we restore the saved flags from the tag object onto the commit.\n> > Just bumping the assignment of flags{1,2} would work (or just bump up\n> > the lookup_commit_or_die call to where we assign to o{1,2}).\n> \n> I tried looking up the flags after dereferencing the tags, but that led\n> to the die(\"Not a range.\") being triggered. That's why the commit\n> message ended up mentioning loading the flags before dereferencing.\n\nOh, sorry, I somehow totally missed that mention in the commit message.\n\nIt seems doubly wrong then to pull the flags from the tag and then later\napply them to the commit at the end. And in fact, if you do not have the\nUNINTERESTING flag on your commit here, that is a real problem. If we\nmake your test:\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 60b9875..37bf70a 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -60,8 +60,9 @@ test_expect_success \"format-patch --ignore-if-in-upstream\" '\n test_expect_success \"format-patch --ignore-if-in-upstream handles tags\" '\n \n \tgit tag -a v1 -m tag side &&\n+\tgit tag -a v2 -m tag master &&\n \tgit format-patch --stdout \\\n-\t\t--ignore-if-in-upstream master..v1 >patch1 &&\n+\t\t--ignore-if-in-upstream v2..v1 >patch1 &&\n \tcnt=$(grep \"^From \" patch1 | wc -l) &&\n \ttest $cnt = 2\n \n\nthen it fails (the key is having the tag on the left-hand side, because\nthat is where we need the UNINTERESTING flag to be).\n\nNormally this flag is propagated to the dereferenced commit as part of\nprepare_revision_walk, but we are looking at the flags before that gets\ncalled. So you'll have to either propagate it manually here, or just\nfeed the original tags to the sub-traversal. I think the latter is\nprobably simpler. Something like:\n\n 1. Check the flags on the original objects (o1 and o2).\n\n 2. Peel them to commits; complain if they're not both commits. Store\n the result in another variable (e.g., commit1, commit2).\n\n 3. Feed o1 and o2 to the new check_rev traversal.\n\n 4. Clear the commit flags off of commit1 and commit2.\n\n 5. Restore the original flags to o1 and o2.\n\nYeesh. I would have thought that we could just do this as part of the\nnormal traversal by using \"--cherry-pick\" (I think format-patch predates\nthat option). We have to have a symmetric range to do that, but I wonder\nif we could simulate it by converting \"foo..bar\" into \"--cherry-pick\n--right-only foo...bar\".\n\nI guess that is basically \"--cherry\", but we still have to massage\n\"foo..bar\" into \"foo...bar\". I think that is basically just:\n\n o1 ^= ~UNINTERESTING;\n o1 |= SYMMETRIC_LEFT;\n\nbut there might be a hidden catch I am not considering.\n\n> > I think this avoids the usual \"wc\" whitespace pitfall because you don't\n> > use double-quotes. But maybe:\n> > \n> > grep \"^From \" patch1 >count &&\n> > test_line_count = 2 patch1\n> > \n> > would be more idiomatic.\n> \n> I can certainly make that change. I made the test as similar as\n> possible to other tests in the area, but I wasn't aware of\n> test_line_count.\n\nAh, I just looked at the context in your patch, not at the whole test. I\ndon't mind matching the surrounding code. But I also don't mind a\npreparatory modernization patch to the test script. :)\n\n-Peff\n"},{"id":"262589","messageId":"CAP8UFD2KYSCMG7p22J78U8yVy49380PCxiXuvartXZdTGm1JFQ@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD1phg8E0JCgkz88CMUo9H-W=s5JDuKeCMOkf1=UYBJt+g@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-06-01T13:44:02Z","receivedAt":"2015-06-01T13:44:02Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 1, 2015 at 2:01 AM, Christian Couder\n wrote:\n> On Mon, Jun 1, 2015 at 1:53 AM, Christian Couder\n> wrote:\n>> On Mon, Jun 1, 2015 at 1:14 AM, Christian Couder\n>> wrote:\n>>> On Sun, May 31, 2015 at 10:45 PM, Bruce Korb wrote:\n>>>> Oh, you can also clone the gnu-pw-mgr and likely get the same result:\n>>>\n>>> Yeah, after cloning from http://git.savannah.gnu.org/r/gnu-pw-mgr.git\n>>> I get the following backtrace:\n>>>\n>>> Program received signal SIGSEGV, Segmentation fault.\n>>> 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n>>> commit=0x84e8d0, mark=139) at commit.c:528\n>>> 528 while ((parents = parents->next))\n>>> (gdb) bt\n>>> #0 0x00000000004b26b1 in clear_commit_marks_1 (plist=0x7fffffffbf78,\n>>> commit=0x84e8d0, mark=139) at commit.c:528\n>>> #1 0x00000000004b2743 in clear_commit_marks_many (nr=-1,\n>>> commit=0x7fffffffbfa0, mark=139) at commit.c:544\n>>> #2 0x00000000004b2771 in clear_commit_marks (commit=0x84e8d0,\n>>> mark=139) at commit.c:549\n>>> #3 0x00000000004537cc in get_patch_ids (rev=0x7fffffffd190,\n>>> ids=0x7fffffffc910) at builtin/log.c:832\n>>> #4 0x0000000000455580 in cmd_format_patch (argc=1,\n>>> argv=0x7fffffffdc20, prefix=0x0) at builtin/log.c:1425\n>>> #5 0x0000000000405807 in run_builtin (p=0x80cac8 ,\n>>> argc=5, argv=0x7fffffffdc20) at git.c:350\n>>> #6 0x0000000000405a15 in handle_builtin (argc=5, argv=0x7fffffffdc20)\n>>> at git.c:532\n>>> #7 0x0000000000405b31 in run_argv (argcp=0x7fffffffdafc,\n>>> argv=0x7fffffffdb10) at git.c:578\n>>> #8 0x0000000000405d29 in main (argc=5, av=0x7fffffffdc18) at git.c:686\n>>>\n>>> (Please don't top post if you reply to this email as it is frown upon\n>>> on this list.)\n>>\n>> When running the command that gives the above segfault:\n>>\n>> $ git format-patch -o patches --ignore-if-in-upstream\n>> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>>\n>> It is interesting to note that the last sha1 refers to a tag:\n>>\n>> $ git cat-file tag 2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>> object 524ccbdbe319068ab18a3950119b9e9a5d135783\n>> type commit\n>> tag v1.4\n>> tagger Bruce Korb 1428847577 -0700\n>>\n>> Release 1.4\n>>\n>> * sort-pw-cfg: a sort/merge program for combining and organizing\n>> configurations.\n>>\n>> * --delete: a new option to remove any entries for a password id\n>>\n>> It works when the tag is replaced by the commit it points to, and the\n>> segfault happens because the we try to access the \"parents\" field of\n>> the tag object as if it was a commit.\n>\n> Yeah, in builtin/log.c we are doing:\n>\n> o2 = rev->pending.objects[1].item;\n>\n> and then we are casting the object into a commit when passing it to\n> clear_commit_marks():\n>\n> clear_commit_marks((struct commit *)o2,\n> SEEN | UNINTERESTING | SHOWN | ADDED);\n>\n> but I don't know where we should have peeled the tag to get a commit,\n> and it's late here so I will leave it someone else to find a fix.\n\nThe following seems to fix it, but I am not sure it is the right fix:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..0ab9360 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -792,6 +792,16 @@ static int reopen_stdout(struct commit *commit,\nconst char *subject,\n return 0;\n }\n\n+static void clear_object_marks(struct object *obj)\n+{\n+ struct commit *c = (struct commit *)peel_to_type(NULL, 0, obj,\n+ OBJ_COMMIT);\n+ if (!c)\n+ die(_(\"could not convert %s into a commit\"),\n+ sha1_to_hex(obj->sha1));\n+ clear_commit_marks(c, SEEN | UNINTERESTING | SHOWN | ADDED);\n+}\n+\n static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n {\n struct rev_info check_rev;\n@@ -827,10 +837,8 @@ static void get_patch_ids(struct rev_info *rev,\nstruct patch_ids *ids)\n }\n\n /* reset for next revision walk */\n- clear_commit_marks((struct commit *)o1,\n- SEEN | UNINTERESTING | SHOWN | ADDED);\n- clear_commit_marks((struct commit *)o2,\n- SEEN | UNINTERESTING | SHOWN | ADDED);\n+ clear_object_marks(o1);\n+ clear_object_marks(o2);\n o1->flags = flags1;\n o2->flags = flags2;\n }\n"},{"id":"262595","messageId":"CAP8UFD0kRJfqEgKNhbqKsPxSW4jvr_8o2Hrtu_b3_raONUN7YQ@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD2KYSCMG7p22J78U8yVy49380PCxiXuvartXZdTGm1JFQ@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-06-01T14:17:21Z","receivedAt":"2015-06-01T14:17:21Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 1, 2015 at 3:44 PM, Christian Couder\n wrote:\n>\n> The following seems to fix it, but I am not sure it is the right fix:\n\nOoops, I had not seen that Brian and Peff are already discussing a fix\nin this thread:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/270371\n"},{"id":"262599","messageId":"CAKRnqNKWb5upY5Xm07UgJvk5SfPVdAWhR=MFkAbRDy4RFaLxRQ@mail.gmail.com","threadId":"39470","inReplyTo":"CAP8UFD0_RCOHUF6BgczgS5kWAFc0QKdw4cUy_bpB2jhd+kYWdw@mail.gmail.com","subject":"Re: seg fault in \"git format-patch\"","fromName":"Bruce Korb","fromEmail":"bruce.korb@gmail.com","sentAt":"2015-06-01T14:47:18Z","receivedAt":"2015-06-01T14:47:18Z","isPatch":false,"sender":{"key":"bruce.korb@gmail.com","avatar":"https://gravatar.com/avatar/86d91467dc7cc8466a9d133a7b93a5d21233052017c144c9a6f6f7e5110344d0?d=mp&s=160"},"body":"On Sun, May 31, 2015 at 4:53 PM, Christian Couder\n wrote:\n>> (Please don't top post if you reply to this email as it is frown upon\n>> on this list.)\n\nWRT \"top posting\", two points:\n\n1. Too many sites/lists now *require* top posting\n2. MUA's (like Google Mail) hide the old mail as an obscure squiggle\nat the bottom of the page.\n\nTime to just get over it and accept the fact that there are two\nconventions. I did. I only gave up a decade ago.\n\n> $ git format-patch -o patches --ignore-if-in-upstream\n> 14949fa8f39d29e44b43f4332ffaf35f11546502..2de9eef391259dfc8748dbaf76a5d55427f37b0d\n>\n> It is interesting to note that the last sha1 refers to a tag:\n\nThey both do. I tried \"git format-patch v1.3..v1.4\" but that didn't work\nand I knew that SHA1..SHA2 would, so I got the SHA's for the tags.\n\n> It works when the tag is replaced by the commit it points to, and the\n> segfault happens because the we try to access the \"parents\" field of\n> the tag object as if it was a commit.\n\nThe other discussion started *after* this one, but thanks for the pointer.\n"},{"id":"262601","messageId":"xmqqr3pv8okj.fsf@gitster.dls.corp.google.com","threadId":"39470","inReplyTo":"1433120593-186980-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T14:56:28Z","receivedAt":"2015-06-01T14:56:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index dd8f3fc..e0465ba 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n> \to2 = rev->pending.objects[1].item;\n> \tflags2 = o2->flags;\n> \n> +\to1 = deref_tag(o1, NULL, 0);\n> +\to2 = deref_tag(o2, NULL, 0);\n> +\n> +\tif (!o1 || !o2)\n> +\t\tdie(_(\"Invalid tag.\"));\n\nShouldn't you ensure o1 and o2 are commits here?\n"},{"id":"262626","messageId":"xmqq6177728a.fsf@gitster.dls.corp.google.com","threadId":"39470","inReplyTo":"xmqqr3pv8okj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T17:44:21Z","receivedAt":"2015-06-01T17:44:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index dd8f3fc..e0465ba 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -807,6 +807,12 @@ static void get_patch_ids(struct rev_info *rev,\n>> struct patch_ids *ids)\n>> \to2 = rev->pending.objects[1].item;\n>> \tflags2 = o2->flags;\n>> \n>> +\to1 = deref_tag(o1, NULL, 0);\n>> +\to2 = deref_tag(o2, NULL, 0);\n>> +\n>> +\tif (!o1 || !o2)\n>> +\t\tdie(_(\"Invalid tag.\"));\n>\n> Shouldn't you ensure o1 and o2 are commits here?\n\nHeh, I should have read the remainder of the thread before\nresponding.\n\nHow about doing it this way? We know and trust that existing\nrevision traversal machinery is doing the right thing, and it is\nonly that the clear_commit_marks() calls are botched.\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dd8f3fc..23a42fa 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -795,7 +795,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,\n static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n {\n \tstruct rev_info check_rev;\n-\tstruct commit *commit;\n+\tstruct commit *commit, *c1, *c2;\n \tstruct object *o1, *o2;\n \tunsigned flags1, flags2;\n \n@@ -803,9 +803,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \t\tdie(_(\"Need exactly one range.\"));\n \n \to1 = rev->pending.objects[0].item;\n-\tflags1 = o1->flags;\n \to2 = rev->pending.objects[1].item;\n+\tflags1 = o1->flags;\n \tflags2 = o2->flags;\n+\tc1 = lookup_commit_reference(o1->sha1);\n+\tc2 = lookup_commit_reference(o2->sha1);\n \n \tif ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))\n \t\tdie(_(\"Not a range.\"));\n@@ -827,10 +829,8 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \t}\n \n \t/* reset for next revision walk */\n-\tclear_commit_marks((struct commit *)o1,\n-\t\t\tSEEN | UNINTERESTING | SHOWN | ADDED);\n-\tclear_commit_marks((struct commit *)o2,\n-\t\t\tSEEN | UNINTERESTING | SHOWN | ADDED);\n+\tclear_commit_marks(c1, SEEN | UNINTERESTING | SHOWN | ADDED);\n+\tclear_commit_marks(c2, SEEN | UNINTERESTING | SHOWN | ADDED);\n \to1->flags = flags1;\n \to2->flags = flags2;\n }\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex c39e500..890db11 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -57,6 +57,14 @@ test_expect_success \"format-patch --ignore-if-in-upstream\" '\n \n '\n \n+test_expect_success \"format-patch --ignore-if-in-upstream handles tags\" '\n+\tgit tag -a v1 -m tag side &&\n+\tgit tag -a v2 -m tag master &&\n+\tgit format-patch --stdout --ignore-if-in-upstream v2..v1 >patch1 &&\n+\tcnt=$(grep \"^From \" patch1 | wc -l) &&\n+\ttest $cnt = 2\n+'\n+\n test_expect_success \"format-patch doesn't consider merge commits\" '\n \n \tgit checkout -b slave master &&\n"},{"id":"262628","messageId":"20150601174712.GA18364@peff.net","threadId":"39470","inReplyTo":"xmqq6177728a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T17:47:12Z","receivedAt":"2015-06-01T17:47:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 01, 2015 at 10:44:21AM -0700, Junio C Hamano wrote:\n\n> > Shouldn't you ensure o1 and o2 are commits here?\n> \n> Heh, I should have read the remainder of the thread before\n> responding.\n> \n> How about doing it this way? We know and trust that existing\n> revision traversal machinery is doing the right thing, and it is\n> only that the clear_commit_marks() calls are botched.\n\nYeah, I think this matches the recommendation I gave in the last round.\n\nI do still think we could get rid of this \"second\" traversal entirely in\nfavor of using \"--cherry\", but that is a much larger topic. Even if\nsomebody wants to pursue that, the immediate fix should look like this.\n\n-Peff\n"},{"id":"262630","messageId":"xmqq1thv71kv.fsf@gitster.dls.corp.google.com","threadId":"39470","inReplyTo":"xmqq6177728a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T17:58:24Z","receivedAt":"2015-06-01T17:58:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> How about doing it this way? We know and trust that existing\n> revision traversal machinery is doing the right thing, and it is\n> only that the clear_commit_marks() calls are botched.\n\nAnother alternative may be to allow any object to clear_commit_marks()\nand have the callee dereference as needed. After all, the revision\nwalking machinery does such a dereferencing when leaving these marks\nthat the function wants to clear, so it might make sense from that\npoint of view.\n\nA quick \"git grep clear_commit_marks()\" tells me that most of the\ncodepaths do make sure the object is a commit when they cast their\nfirst argument to (struct commit *) when calling this function, but\nsome of them do look suspicous.\n"},{"id":"262651","messageId":"xmqq4mmr5fqy.fsf@gitster.dls.corp.google.com","threadId":"39470","inReplyTo":"20150601174712.GA18364@peff.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T20:35:17Z","receivedAt":"2015-06-01T20:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jun 01, 2015 at 10:44:21AM -0700, Junio C Hamano wrote:\n>\n>> > Shouldn't you ensure o1 and o2 are commits here?\n>> \n>> Heh, I should have read the remainder of the thread before\n>> responding.\n>> \n>> How about doing it this way? We know and trust that existing\n>> revision traversal machinery is doing the right thing, and it is\n>> only that the clear_commit_marks() calls are botched.\n>\n> Yeah, I think this matches the recommendation I gave in the last round.\n>\n> I do still think we could get rid of this \"second\" traversal entirely in\n> favor of using \"--cherry\", but that is a much larger topic. Even if\n> somebody wants to pursue that, the immediate fix should look like this.\n>\n> -Peff\n\nThanks.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Mon, 1 Jun 2015 10:44:21 -0700\nSubject: [PATCH] format-patch: do not feed tags to clear_commit_marks()\n\n\"git format-patch --ignore-if-in-upstream A..B\", when either A or B\nis a tag, failed miserably.\n\nThis is because the code passes the tips it used for traversal to\nclear_commit_marks(), after running a temporary revision traversal\nto enumerate the commits on both branches to find if they have\ncommits that make equivalent changes. The revision traversal\nmachinery knows how to enumerate commits reachable starting from a\ntag, but clear_commit_marks() wants to take nothing but a commit.\n\nIn the longer term, it might be a more correct fix to teach\nclear_commit_marks() to do the same \"committish to commit\"\ndereferncing that is done in the revision traversal machinery, but\nfor now this fix should suffice.\n\nReported-by: Bruce Korb \nHelped-by: Christian Couder \nHelped-by: brian m. carlson \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/log.c | 12 ++++++------\n t/t4014-format-patch.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 734aab3..39181e2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -795,7 +795,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,\n static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n {\n \tstruct rev_info check_rev;\n-\tstruct commit *commit;\n+\tstruct commit *commit, *c1, *c2;\n \tstruct object *o1, *o2;\n \tunsigned flags1, flags2;\n \n@@ -803,9 +803,11 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \t\tdie(_(\"Need exactly one range.\"));\n \n \to1 = rev->pending.objects[0].item;\n-\tflags1 = o1->flags;\n \to2 = rev->pending.objects[1].item;\n+\tflags1 = o1->flags;\n \tflags2 = o2->flags;\n+\tc1 = lookup_commit_reference(o1->sha1);\n+\tc2 = lookup_commit_reference(o2->sha1);\n \n \tif ((flags1 & UNINTERESTING) == (flags2 & UNINTERESTING))\n \t\tdie(_(\"Not a range.\"));\n@@ -827,10 +829,8 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \t}\n \n \t/* reset for next revision walk */\n-\tclear_commit_marks((struct commit *)o1,\n-\t\t\tSEEN | UNINTERESTING | SHOWN | ADDED);\n-\tclear_commit_marks((struct commit *)o2,\n-\t\t\tSEEN | UNINTERESTING | SHOWN | ADDED);\n+\tclear_commit_marks(c1, SEEN | UNINTERESTING | SHOWN | ADDED);\n+\tclear_commit_marks(c2, SEEN | UNINTERESTING | SHOWN | ADDED);\n \to1->flags = flags1;\n \to2->flags = flags2;\n }\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 256affc..2ea12dd 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -57,6 +57,14 @@ test_expect_success \"format-patch --ignore-if-in-upstream\" '\n \n '\n \n+test_expect_success \"format-patch --ignore-if-in-upstream handles tags\" '\n+\tgit tag -a v1 -m tag side &&\n+\tgit tag -a v2 -m tag master &&\n+\tgit format-patch --stdout --ignore-if-in-upstream v2..v1 >patch1 &&\n+\tcnt=$(grep \"^From \" patch1 | wc -l) &&\n+\ttest $cnt = 2\n+'\n+\n test_expect_success \"format-patch doesn't consider merge commits\" '\n \n \tgit checkout -b slave master &&\n-- \n2.4.2-558-g3ddf4bb\n"},{"id":"262656","messageId":"20150601223409.GB140991@vauxhall.crustytoothpaste.net","threadId":"39470","inReplyTo":"xmqq4mmr5fqy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-06-01T22:34:09Z","receivedAt":"2015-06-01T22:34:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jun 01, 2015 at 01:35:17PM -0700, Junio C Hamano wrote:\n> -- >8 --\n> From: Junio C Hamano \n> Date: Mon, 1 Jun 2015 10:44:21 -0700\n> Subject: [PATCH] format-patch: do not feed tags to clear_commit_marks()\n> \n> \"git format-patch --ignore-if-in-upstream A..B\", when either A or B\n> is a tag, failed miserably.\n> \n> This is because the code passes the tips it used for traversal to\n> clear_commit_marks(), after running a temporary revision traversal\n> to enumerate the commits on both branches to find if they have\n> commits that make equivalent changes. The revision traversal\n> machinery knows how to enumerate commits reachable starting from a\n> tag, but clear_commit_marks() wants to take nothing but a commit.\n> \n> In the longer term, it might be a more correct fix to teach\n> clear_commit_marks() to do the same \"committish to commit\"\n> dereferncing that is done in the revision traversal machinery, but\n\n\"dereferencing\". Otherwise, looks exactly like what I would have\nwritten in my reroll had you not gotten to it before me.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"262658","messageId":"xmqqwpzn3v4f.fsf@gitster.dls.corp.google.com","threadId":"39470","inReplyTo":"20150601223409.GB140991@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] format-patch: dereference tags with --ignore-if-in-upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T22:46:08Z","receivedAt":"2015-06-01T22:46:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> In the longer term, it might be a more correct fix to teach\n>> clear_commit_marks() to do the same \"committish to commit\"\n>> dereferncing that is done in the revision traversal machinery, but\n>\n> \"dereferencing\". Otherwise, looks exactly like what I would have\n> written in my reroll had you not gotten to it before me.\n\nHeh thanks.\n\nI do not mind if you sent in a replacement. What I sent was done\nprimarily because I saw multiple people coming up with essentially\nthe same solution and I was afraid everybody would say \"it is being\ntaken care of by others\" and we end up not having any patch ;-).\n"}]}