{"thread":{"id":"48592","subject":"format-patch: no 'prerequisite-patch-id' info when specifying commit range","startedAt":"2018-05-29T18:46:56Z","lastAt":"2018-06-19T18:18:40Z","messageCount":9,"participants":["Eduardo Habkost","Ye Xiaolong","Junio C Hamano","Xiaolong Ye","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348713","messageId":"20180529184651.GB14525@localhost.localdomain","threadId":"48592","inReplyTo":null,"subject":"format-patch: no 'prerequisite-patch-id' info when specifying commit range","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2018-05-29T18:46:51Z","receivedAt":"2018-05-29T18:46:56Z","isPatch":false,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"Hi,\n\nI'm trying to use git-format-patch --base to generate the list of\nprerequisite patches for a series, but the behavior of git\ndoesn't seem to match the documentation:\n\nWhen using a commit count (e.g.: \"-2\"), git-format-patch generates the\nprerequisite-patch-id lines as expected. But when using a commit range like\n\"Z..C\", the prerequisite-patch-id lines are missing.\n\nIs this intentional, or it is a bug?\n\nExample using git.git commits:\n\n $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'\n base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930\n prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb\n prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013\n (ehabkost@localhost:~/rh/proj/git (ok) 1j)\n $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'\n base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n $ git --version\n git version 2.17.1\n $ git log --graph --pretty=oneline -6 b7b1fca17\n * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink\n * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict\n * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects\n * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects\n * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content\n * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check\n $ \n\nIf I understand the documentation correctly, both \"-3 C\" or \"Z..C\" were\nsupposed to be equivalent:\n\n> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with\n> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the\n> range), the base tree information block is shown at the end of the\n> first message the command outputs (either the first patch, or the\n> cover letter), like this:\n> \n> ------------\n> base-commit: P\n> prerequisite-patch-id: X\n> prerequisite-patch-id: Y\n> prerequisite-patch-id: Z\n> ------------\n\n-- \nEduardo\n"},{"id":"348785","messageId":"20180530070448.GC27396@yexl-desktop","threadId":"48592","inReplyTo":"20180529184651.GB14525@localhost.localdomain","subject":"Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2018-05-30T07:04:48Z","receivedAt":"2018-05-30T07:08:00Z","isPatch":false,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Hi, Eduardo\n\nOn 05/29, Eduardo Habkost wrote:\n>Hi,\n>\n>I'm trying to use git-format-patch --base to generate the list of\n>prerequisite patches for a series, but the behavior of git\n>doesn't seem to match the documentation:\n>\n>When using a commit count (e.g.: \"-2\"), git-format-patch generates the\n>prerequisite-patch-id lines as expected. But when using a commit range like\n>\"Z..C\", the prerequisite-patch-id lines are missing.\n>\n>Is this intentional, or it is a bug?\n\nThanks for reporting, it seems an unexpected behavior, I'll look into it.\n\nThanks,\nXiaolong\n\n>\n>Example using git.git commits:\n>\n> $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'\n> base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n> prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930\n> prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb\n> prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013\n> (ehabkost@localhost:~/rh/proj/git (ok) 1j)\n> $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'\n> base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n> $ git --version\n> git version 2.17.1\n> $ git log --graph --pretty=oneline -6 b7b1fca17\n> * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink\n> * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict\n> * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects\n> * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects\n> * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content\n> * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check\n> $ \n>\n>If I understand the documentation correctly, both \"-3 C\" or \"Z..C\" were\n>supposed to be equivalent:\n>\n>> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with\n>> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the\n>> range), the base tree information block is shown at the end of the\n>> first message the command outputs (either the first patch, or the\n>> cover letter), like this:\n>> \n>> ------------\n>> base-commit: P\n>> prerequisite-patch-id: X\n>> prerequisite-patch-id: Y\n>> prerequisite-patch-id: Z\n>> ------------\n>\n>-- \n>Eduardo\n"},{"id":"349108","messageId":"20180603060702.GA1306@yexl-desktop","threadId":"48592","inReplyTo":"20180529184651.GB14525@localhost.localdomain","subject":"Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2018-06-03T06:07:02Z","receivedAt":"2018-06-03T06:10:17Z","isPatch":false,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Hi, Junio\n\nOn 05/29, Eduardo Habkost wrote:\n>Hi,\n>\n>I'm trying to use git-format-patch --base to generate the list of\n>prerequisite patches for a series, but the behavior of git\n>doesn't seem to match the documentation:\n>\n>When using a commit count (e.g.: \"-2\"), git-format-patch generates the\n>prerequisite-patch-id lines as expected. But when using a commit range like\n>\"Z..C\", the prerequisite-patch-id lines are missing.\n>\n\nI narrowed down the problem to revision walk, if users specify the commit range\nvia \"Z..C\" pattern, the first prepare_revision_walk function called in\ncmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,\nthus the next revision walk in prepare_bases wouldn't be able to reach\nprerequisite patches, one quick solution I can think of is to clear\nUNINTERESTING flag in reset_revision_walk, like below:\n\nvoid reset_revision_walk(void)\n{\n\tclear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);\n}\n\nThough I'm not sure whether it has some side effects, or whether it would impact\nbehavior of other reference of reset_revision_walk. If you think it's a sensible\nsolution, I'll submit a patch.\n\nThanks,\nXiaolong\n\n\n>Is this intentional, or it is a bug?\n>\n>Example using git.git commits:\n>\n> $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 -2 b7b1fca17 | egrep 'base-commit|prereq'\n> base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n> prerequisite-patch-id: 080ac2faf21a6a7f9b23cb68286866d026a92930\n> prerequisite-patch-id: e3ee77500c9aa70248e7ee814662d01f79d0dcdb\n> prerequisite-patch-id: 6d831e23e33075681e6b74553151a32b73092013\n> (ehabkost@localhost:~/rh/proj/git (ok) 1j)\n> $ git format-patch --stdout --cover-letter --stdout --base b7b1fca17~5 b7b1fca17~2..b7b1fca17 | egrep 'base-commit|prereq'\n> base-commit: 2738744426c161a98c2ec494d41241a4c5eef9ef\n> $ git --version\n> git version 2.17.1\n> $ git log --graph --pretty=oneline -6 b7b1fca17\n> * b7b1fca175f1ed7933f361028c631b9ac86d868d fsck: complain when .gitmodules is a symlink\n> * 73c3f0f704a91b6792e0199a3f3ab6e3a1971675 index-pack: check .gitmodules files with --strict\n> * 6e328d6caef218db320978e3e251009135d87d0e unpack-objects: call fsck_finish() after fscking objects\n> * 1995b5e03e1cc97116be58cdc0502d4a23547856 fsck: call fsck_finish() after fscking objects\n> * ed8b10f631c9a71df3351d46187bf7f3fa4f9b7e fsck: check .gitmodules content\n> * 2738744426c161a98c2ec494d41241a4c5eef9ef fsck: handle promisor objects in .gitmodules check\n> $ \n>\n>If I understand the documentation correctly, both \"-3 C\" or \"Z..C\" were\n>supposed to be equivalent:\n>\n>> With `git format-patch --base=P -3 C` (or variants thereof, e.g. with\n>> `--cover-letter` or using `Z..C` instead of `-3 C` to specify the\n>> range), the base tree information block is shown at the end of the\n>> first message the command outputs (either the first patch, or the\n>> cover letter), like this:\n>> \n>> ------------\n>> base-commit: P\n>> prerequisite-patch-id: X\n>> prerequisite-patch-id: Y\n>> prerequisite-patch-id: Z\n>> ------------\n>\n>-- \n>Eduardo\n"},{"id":"349200","messageId":"xmqqh8mj70w6.fsf@gitster-ct.c.googlers.com","threadId":"48592","inReplyTo":"20180603060702.GA1306@yexl-desktop","subject":"Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T02:15:21Z","receivedAt":"2018-06-04T02:15:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ye Xiaolong writes:\n\n> I narrowed down the problem to revision walk, if users specify the commit range\n> via \"Z..C\" pattern, the first prepare_revision_walk function called in\n> cmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,\n> thus the next revision walk in prepare_bases wouldn't be able to reach\n> prerequisite patches, one quick solution I can think of is to clear\n> UNINTERESTING flag in reset_revision_walk, like below:\n>\n> void reset_revision_walk(void)\n> {\n> \tclear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);\n> }\n\nWhen you are done with objects that are UNINTERESTING in your\napplication (i.e. only when \"format-patch\" is told to compute list\nof prereq patches by doing an extra revision walk), your application\ncan call clear_object_flags() on the flags you are done with, I\nwould think.\n\nBut the current callers of reset_revision_walk() do not expect any\nflags other than the ones that are used to keep track of the\ntraversal state, so it is likely you will break them if you suddenly\nstarted to clear flags randomly.\n"},{"id":"349204","messageId":"20180604024101.GO27396@yexl-desktop","threadId":"48592","inReplyTo":"xmqqh8mj70w6.fsf@gitster-ct.c.googlers.com","subject":"Re: format-patch: no 'prerequisite-patch-id' info when specifying commit range","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2018-06-04T02:41:01Z","receivedAt":"2018-06-04T02:44:17Z","isPatch":false,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"On 06/04, Junio C Hamano wrote:\n>Ye Xiaolong writes:\n>\n>> I narrowed down the problem to revision walk, if users specify the commit range\n>> via \"Z..C\" pattern, the first prepare_revision_walk function called in\n>> cmd_format_patch would mark all parents (ancestors) of Z to be uninteresting,\n>> thus the next revision walk in prepare_bases wouldn't be able to reach\n>> prerequisite patches, one quick solution I can think of is to clear\n>> UNINTERESTING flag in reset_revision_walk, like below:\n>>\n>> void reset_revision_walk(void)\n>> {\n>> \tclear_object_flags(SEEN | ADDED | SHOWN| UNINTERESTING);\n>> }\n>\n>When you are done with objects that are UNINTERESTING in your\n>application (i.e. only when \"format-patch\" is told to compute list\n>of prereq patches by doing an extra revision walk), your application\n>can call clear_object_flags() on the flags you are done with, I\n>would think.\n>\n>But the current callers of reset_revision_walk() do not expect any\n>flags other than the ones that are used to keep track of the\n>traversal state, so it is likely you will break them if you suddenly\n>started to clear flags randomly.\n\nGot it, I'll try to call clear_object_flags in format-patch related codepatch\nonly, not to touch the global reset_revision_walk.\n\nThanks,\nXiaolong\n"},{"id":"349254","messageId":"20180604150543.7304-1-xiaolong.ye@intel.com","threadId":"48592","inReplyTo":"xmqqh8mj70w6.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] format-patch: clear UNINTERESTING flag before prepare_bases","fromName":"Xiaolong Ye","fromEmail":"xiaolong.ye@intel.com","sentAt":"2018-06-04T15:05:43Z","receivedAt":"2018-06-04T15:09:17Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"When users specify the commit range with 'Z..C' pattern for format-patch, all\nthe parents of Z (including Z) would be marked as UNINTERESTING which would\nprevent revision walk in prepare_bases from getting the prerequisite commits,\nthus `git format-patch --base Z..C` won't be able to generate\nthe list of prerequisite patch ids. Clear UNINTERESTING flag with\nclear_object_flags solves this issue.\n\nReported-by: Eduardo Habkost \nSigned-off-by: Xiaolong Ye \n---\n builtin/log.c | 1 +\n t/t4014-format-patch.sh | 6 ++++--\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4686f68594..01993de6fe 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (base_commit || base_auto) {\n \t\tstruct commit *base = get_base_commit(base_commit, list, nr);\n \t\treset_revision_walk();\n+\t\tclear_object_flags(UNINTERESTING);\n \t\tprepare_bases(&bases, base, list, nr);\n \t}\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 028d5507a6..53880da7bb 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n \n test_expect_success 'format-patch --base' '\n \tgit checkout side &&\n-\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&\n+\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&\n+\tgit format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&\n \techo >expected &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \tsignature >> expected &&\n-\ttest_cmp expected actual\n+\ttest_cmp expected actual1 &&\n+\ttest_cmp expected actual2\n '\n \n test_expect_success 'format-patch --base errors out when base commit is in revision list' '\n-- \n2.16.GIT\n\n"},{"id":"349296","messageId":"20180604194241.GV7451@localhost.localdomain","threadId":"48592","inReplyTo":"20180604150543.7304-1-xiaolong.ye@intel.com","subject":"Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2018-06-04T19:42:41Z","receivedAt":"2018-06-04T19:42:46Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"On Mon, Jun 04, 2018 at 11:05:43PM +0800, Xiaolong Ye wrote:\n> When users specify the commit range with 'Z..C' pattern for format-patch, all\n> the parents of Z (including Z) would be marked as UNINTERESTING which would\n> prevent revision walk in prepare_bases from getting the prerequisite commits,\n> thus `git format-patch --base Z..C` won't be able to generate\n> the list of prerequisite patch ids. Clear UNINTERESTING flag with\n> clear_object_flags solves this issue.\n> \n> Reported-by: Eduardo Habkost \n> Signed-off-by: Xiaolong Ye \n\nThanks! The fix works for me.\n\nTested-by: Eduardo Habkost \n\n-- \nEduardo\n"},{"id":"350451","messageId":"20180619024054.GD17789@yexl-desktop","threadId":"48592","inReplyTo":"20180604150543.7304-1-xiaolong.ye@intel.com","subject":"Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases","fromName":"Ye Xiaolong","fromEmail":"xiaolong.ye@intel.com","sentAt":"2018-06-19T02:40:54Z","receivedAt":"2018-06-19T02:44:26Z","isPatch":true,"sender":{"key":"xiaolong.ye@intel.com","avatar":"https://avatars.githubusercontent.com/u/21098480?v=4"},"body":"Hi, Junio\n\nCould you help review this patch?\n\nThanks,\nXiaolong\n\nOn 06/04, Xiaolong Ye wrote:\n>When users specify the commit range with 'Z..C' pattern for format-patch, all\n>the parents of Z (including Z) would be marked as UNINTERESTING which would\n>prevent revision walk in prepare_bases from getting the prerequisite commits,\n>thus `git format-patch --base Z..C` won't be able to generate\n>the list of prerequisite patch ids. Clear UNINTERESTING flag with\n>clear_object_flags solves this issue.\n>\n>Reported-by: Eduardo Habkost \n>Signed-off-by: Xiaolong Ye \n>---\n> builtin/log.c | 1 +\n> t/t4014-format-patch.sh | 6 ++++--\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n>\n>diff --git a/builtin/log.c b/builtin/log.c\n>index 4686f68594..01993de6fe 100644\n>--- a/builtin/log.c\n>+++ b/builtin/log.c\n>@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tif (base_commit || base_auto) {\n> \t\tstruct commit *base = get_base_commit(base_commit, list, nr);\n> \t\treset_revision_walk();\n>+\t\tclear_object_flags(UNINTERESTING);\n> \t\tprepare_bases(&bases, base, list, nr);\n> \t}\n> \n>diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n>index 028d5507a6..53880da7bb 100755\n>--- a/t/t4014-format-patch.sh\n>+++ b/t/t4014-format-patch.sh\n>@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n> \n> test_expect_success 'format-patch --base' '\n> \tgit checkout side &&\n>-\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&\n>+\tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&\n>+\tgit format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&\n> \techo >expected &&\n> \techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n> \techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n> \techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n> \tsignature >> expected &&\n>-\ttest_cmp expected actual\n>+\ttest_cmp expected actual1 &&\n>+\ttest_cmp expected actual2\n> '\n> \n> test_expect_success 'format-patch --base errors out when base commit is in revision list' '\n>-- \n>2.16.GIT\n>\n"},{"id":"350518","messageId":"CAGZ79kakUs9A4jSXv6NtsAQtOcaHRPTZOA-X_t_hUHsh50p=+A@mail.gmail.com","threadId":"48592","inReplyTo":"20180604150543.7304-1-xiaolong.ye@intel.com","subject":"Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-19T18:18:25Z","receivedAt":"2018-06-19T18:18:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 4, 2018 at 8:09 AM Xiaolong Ye wrote:\n>\n> When users specify the commit range with 'Z..C' pattern for format-patch, all\n> the parents of Z (including Z) would be marked as UNINTERESTING which would\n> prevent revision walk in prepare_bases from getting the prerequisite commits,\n> thus `git format-patch --base Z..C` won't be able to generate\n> the list of prerequisite patch ids. Clear UNINTERESTING flag with\n> clear_object_flags solves this issue.\n\nThis makes sense;\nReviewed-by: Stefan Beller \n"}]}