{"thread":{"id":"63215","subject":"[PATCH] revision: fix --left/right-only use with unrelated histories","startedAt":"2025-03-30T06:03:42Z","lastAt":"2025-04-11T14:42:00Z","messageCount":9,"participants":["Matt Hunter","Johannes Sixt","Phillip Wood","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"515291","messageId":"20250330055809.1019090-1-m@lfurio.us","threadId":"63215","inReplyTo":null,"subject":"[PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Matt Hunter","fromEmail":"m@lfurio.us","sentAt":"2025-03-30T05:49:24Z","receivedAt":"2025-03-30T06:03:42Z","isPatch":true,"sender":{"key":"m@lfurio.us","avatar":"https://avatars.githubusercontent.com/u/14925125?v=4"},"body":"This is a similar fix as 023756f4eb (revision walker: --cherry-pick is a\nlimited operation), but for the --left-only and --right-only options.\n\nWhen computing a symmetric difference between two unrelated histories,\nno suitable merge base exists, and so no boundary commit is flagged as\nUNINTERESTING. Previously, we relied on the presence of such boundary\nto trigger limiting and thus consideration of either \"revs->left_only\"\nor \"revs->right_only\".\n\nA number of other entries in the option parser have started including\noverrides for \"revs->limited = 1\". Do the same for these options.\n\nSigned-off-by: Matt Hunter \n---\n\nPatch applies to the current maint branch (git v2.49.0).\n\nI made a best guess at what the most logical home for this test case\nshould be, so please let me know if somewhere else is preferred. All\ntests pass when this is cherry-picked to seen. There is only a minor\nconflict, as a few different tests have appeared in this same spot.\n\n revision.c | 2 ++\n t/t6000-rev-list-misc.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex c4390f0938..e045445bc3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2488,10 +2488,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n \t\t\t \"--left-only\", \"--right-only/--cherry\");\n \t\trevs->left_only = 1;\n+\t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--right-only\")) {\n \t\tif (revs->left_only)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--right-only\", \"--left-only\");\n \t\trevs->right_only = 1;\n+\t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--cherry\")) {\n \t\tif (revs->left_only)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cherry\", \"--left-only\");\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 6289a2e8b0..58f1f746e0 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -182,4 +182,16 @@ test_expect_success 'rev-list --unpacked' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list one-sided unrelated symmetric diff' '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m xyz &&\n+\tgit branch cmp &&\n+\tgit rebase --force-rebase --root &&\n+\n+\tgit rev-list --left-only HEAD...cmp >head &&\n+\tgit rev-list --right-only HEAD...cmp >cmp &&\n+\n+\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n+'\n+\n test_done\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.49.0\n\n"},{"id":"515293","messageId":"644ce9b5-755c-4faf-aaf8-b0383e12ff64@kdbg.org","threadId":"63215","inReplyTo":"20250330055809.1019090-1-m@lfurio.us","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-03-30T08:31:59Z","receivedAt":"2025-03-30T08:32:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.03.25 um 07:49 schrieb Matt Hunter:\n> +\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n\nProcess substitution does not work on Windows. Please use temporary files.\n\n-- Hannes\n\n"},{"id":"515295","messageId":"f8a7d089-3150-4212-8ad0-c9bbb3858776@gmail.com","threadId":"63215","inReplyTo":"20250330055809.1019090-1-m@lfurio.us","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-30T10:11:54Z","receivedAt":"2025-03-30T10:11:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Matt\n\nOn 30/03/2025 06:49, Matt Hunter wrote:\n> +test_expect_success 'rev-list one-sided unrelated symmetric diff' '\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m xyz &&\n> +\tgit branch cmp &&\n> +\tgit rebase --force-rebase --root &&\n> +\n> +\tgit rev-list --left-only HEAD...cmp >head &&\n> +\tgit rev-list --right-only HEAD...cmp >cmp &&\n> +\n> +\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n\nThank you for adding a test. We have a helper function test_line_count \nwhich provides a helpful debugging message if the comparison fails. \nUsing that and avoiding process substitutions we'd write\n\n\tsort head >sorted_head &&\n\tsort cmp >sorted_cmp &&\n\tcomm -12 sorted_head sorted_cmp >actual &&\n\ttest_line_count = 0 actual\n\nThanks\n\nPhillip\n\n> +\n> test_done\n> \n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n\n"},{"id":"515296","messageId":"D8TJMUMOGLBC.3FR8DHTTUN4M9@lfurio.us","threadId":"63215","inReplyTo":"f8a7d089-3150-4212-8ad0-c9bbb3858776@gmail.com","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Matt Hunter","fromEmail":"m@lfurio.us","sentAt":"2025-03-30T10:54:07Z","receivedAt":"2025-03-30T10:54:09Z","isPatch":true,"sender":{"key":"m@lfurio.us","avatar":"https://avatars.githubusercontent.com/u/14925125?v=4"},"body":"On Sun Mar 30, 2025 at 6:11 AM EDT, Phillip Wood wrote:\n> Thank you for adding a test. We have a helper function test_line_count \n> which provides a helpful debugging message if the comparison fails. \n> Using that and avoiding process substitutions we'd write\n>\n> \tsort head >sorted_head &&\n> \tsort cmp >sorted_cmp &&\n> \tcomm -12 sorted_head sorted_cmp >actual &&\n> \ttest_line_count = 0 actual\nThanks for that helper tip. I was just about to send a v2 when your\nmessage came in, so I'm getting that incorporated now.\n\nBy the way, I had originally wanted to write test assertions that\nchecked the actual number of commit ids returned from each of the two\ncalls to rev-list - something like:\n\n git rev-list --X-only HEAD...cmp >file &&\n test_line_count = N file\n\nBut since I'm not very familiar with this test harness yet, I couldn't\nactually figure the correct value for N. It's not 1 (the commit made in\nmy test body), and it's not 2 (that commit, plus the one from the setup\ncase at the top of the file). Any appropriate higher value wasn't\nobvious.\n\nSo I switched to what you saw in my v1. Maybe this \"no commit ids in\ncommon\" test is actually the stronger assertion?\n"},{"id":"515297","messageId":"20250330112850.2477673-1-m@lfurio.us","threadId":"63215","inReplyTo":"20250330055809.1019090-1-m@lfurio.us","subject":"[PATCH v2] revision: fix --left/right-only use with unrelated histories","fromName":"Matt Hunter","fromEmail":"m@lfurio.us","sentAt":"2025-03-30T11:24:06Z","receivedAt":"2025-03-30T11:29:04Z","isPatch":true,"sender":{"key":"m@lfurio.us","avatar":"https://avatars.githubusercontent.com/u/14925125?v=4"},"body":"This is a similar fix as 023756f4eb (revision walker: --cherry-pick is a\nlimited operation), but for the --left-only and --right-only options.\n\nWhen computing a symmetric difference between two unrelated histories,\nno suitable merge base exists, and so no boundary commit is flagged as\nUNINTERESTING. Previously, we relied on the presence of such boundary\nto trigger limiting and thus consideration of either \"revs->left_only\"\nor \"revs->right_only\".\n\nA number of other entries in the option parser have started including\noverrides for \"revs->limited = 1\". Do the same for these options.\n\nSigned-off-by: Matt Hunter \n---\n\nRange-diff against v1:\n1: 1982f14d70 ! 1: 4f5b264b26 revision: fix --left/right-only use with unrelated histories\n @@ t/t6000-rev-list-misc.sh: test_expect_success 'rev-list --unpacked' '\n +\tgit rev-list --left-only HEAD...cmp >head &&\n +\tgit rev-list --right-only HEAD...cmp >cmp &&\n +\n -+\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n ++\tsort head >head.sorted &&\n ++\tsort cmp >cmp.sorted &&\n ++\tcomm -12 head.sorted cmp.sorted >actual &&\n ++\ttest_line_count = 0 actual\n +'\n +\n test_done\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n\n revision.c | 2 ++\n t/t6000-rev-list-misc.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex c4390f0938..e045445bc3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2488,10 +2488,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n \t\t\t \"--left-only\", \"--right-only/--cherry\");\n \t\trevs->left_only = 1;\n+\t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--right-only\")) {\n \t\tif (revs->left_only)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--right-only\", \"--left-only\");\n \t\trevs->right_only = 1;\n+\t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--cherry\")) {\n \t\tif (revs->left_only)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cherry\", \"--left-only\");\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 6289a2e8b0..d338f7ecb4 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -182,4 +182,19 @@ test_expect_success 'rev-list --unpacked' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list one-sided unrelated symmetric diff' '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m xyz &&\n+\tgit branch cmp &&\n+\tgit rebase --force-rebase --root &&\n+\n+\tgit rev-list --left-only HEAD...cmp >head &&\n+\tgit rev-list --right-only HEAD...cmp >cmp &&\n+\n+\tsort head >head.sorted &&\n+\tsort cmp >cmp.sorted &&\n+\tcomm -12 head.sorted cmp.sorted >actual &&\n+\ttest_line_count = 0 actual\n+'\n+\n test_done\n-- \n2.49.0\n\n"},{"id":"515419","messageId":"xmqqwmc49pcl.fsf@gitster.g","threadId":"63215","inReplyTo":"644ce9b5-755c-4faf-aaf8-b0383e12ff64@kdbg.org","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-01T09:56:58Z","receivedAt":"2025-04-01T09:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 30.03.25 um 07:49 schrieb Matt Hunter:\n>> +\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n>\n> Process substitution does not work on Windows. Please use temporary files.\n\nAlso it is not portable across POSIX compilant shells (IIRC it is a\nbash-ism).\n"},{"id":"515519","messageId":"63e79534-db09-444f-8e82-8e01d914182d@gmail.com","threadId":"63215","inReplyTo":"D8TJMUMOGLBC.3FR8DHTTUN4M9@lfurio.us","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-04-02T13:12:07Z","receivedAt":"2025-04-02T13:12:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Matt\n\nOn 30/03/2025 11:54, Matt Hunter wrote:\n> On Sun Mar 30, 2025 at 6:11 AM EDT, Phillip Wood wrote:\n>> Thank you for adding a test. We have a helper function test_line_count\n>> which provides a helpful debugging message if the comparison fails.\n>> Using that and avoiding process substitutions we'd write\n>>\n>> \tsort head >sorted_head &&\n>> \tsort cmp >sorted_cmp &&\n>> \tcomm -12 sorted_head sorted_cmp >actual &&\n>> \ttest_line_count = 0 actual\n> Thanks for that helper tip. I was just about to send a v2 when your\n> message came in, so I'm getting that incorporated now.\n> \n> By the way, I had originally wanted to write test assertions that\n> checked the actual number of commit ids returned from each of the two\n> calls to rev-list - something like:\n> \n> git rev-list --X-only HEAD...cmp >file &&\n> test_line_count = N file\n> \n> But since I'm not very familiar with this test harness yet, I couldn't\n> actually figure the correct value for N. It's not 1 (the commit made in\n> my test body), and it's not 2 (that commit, plus the one from the setup\n> case at the top of the file). Any appropriate higher value wasn't\n> obvious.\n\nEach test in a given file runs in the same repository (this is a \nperformance optimization) so the number of commits will depend on what \nthe previous tests have done. Usually there is a setup test at the start \nof the file which creates some commits with tags. Individual tests can \nthen use those tags to establish a known state.\n\nBest Wishes\n\nPhillip\n\n\n> So I switched to what you saw in my v1. Maybe this \"no commit ids in\n> common\" test is actually the stronger assertion?\n\n"},{"id":"515633","messageId":"D8XLIW3UYCGC.1S3K2HXJ2R8BL@lfurio.us","threadId":"63215","inReplyTo":"63e79534-db09-444f-8e82-8e01d914182d@gmail.com","subject":"Re: [PATCH] revision: fix --left/right-only use with unrelated histories","fromName":"Matt Hunter","fromEmail":"m@lfurio.us","sentAt":"2025-04-04T05:13:39Z","receivedAt":"2025-04-04T05:13:46Z","isPatch":true,"sender":{"key":"m@lfurio.us","avatar":"https://avatars.githubusercontent.com/u/14925125?v=4"},"body":"On Wed Apr 2, 2025 at 9:12 AM EDT, phillip.wood123 wrote:\n> Each test in a given file runs in the same repository (this is a \n> performance optimization) so the number of commits will depend on what \n> the previous tests have done. Usually there is a setup test at the start \n> of the file which creates some commits with tags. Individual tests can \n> then use those tags to establish a known state.\nOk - that makes sense. And that being the case, I think I stand by the\ntest in v2 of my patch.\n\nThanks\n"},{"id":"515998","messageId":"xmqqecxyoj4a.fsf@gitster.g","threadId":"63215","inReplyTo":"20250330112850.2477673-1-m@lfurio.us","subject":"Re: [PATCH v2] revision: fix --left/right-only use with unrelated histories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-11T14:41:57Z","receivedAt":"2025-04-11T14:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt Hunter writes:\n\n> This is a similar fix as 023756f4eb (revision walker: --cherry-pick is a\n> limited operation), but for the --left-only and --right-only options.\n>\n> When computing a symmetric difference between two unrelated histories,\n> no suitable merge base exists, and so no boundary commit is flagged as\n> UNINTERESTING. Previously, we relied on the presence of such boundary\n> to trigger limiting and thus consideration of either \"revs->left_only\"\n> or \"revs->right_only\".\n>\n> A number of other entries in the option parser have started including\n> overrides for \"revs->limited = 1\". Do the same for these options.\n>\n> Signed-off-by: Matt Hunter \n> ---\n\nThanks. As far as I can see, the whole patch looks sensible,\nincluding its rewritten tests.\n\nLet me mark the topic for 'next'.\n\nThanks, all.\n\n\n> Range-diff against v1:\n> 1: 1982f14d70 ! 1: 4f5b264b26 revision: fix --left/right-only use with unrelated histories\n> @@ t/t6000-rev-list-misc.sh: test_expect_success 'rev-list --unpacked' '\n> +\tgit rev-list --left-only HEAD...cmp >head &&\n> +\tgit rev-list --right-only HEAD...cmp >cmp &&\n> +\n> -+\ttest $(comm -12 <(sort head) <(sort cmp) | wc -l) = \"0\"\n> ++\tsort head >head.sorted &&\n> ++\tsort cmp >cmp.sorted &&\n> ++\tcomm -12 head.sorted cmp.sorted >actual &&\n> ++\ttest_line_count = 0 actual\n> +'\n> +\n> test_done\n>\n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n>\n> revision.c | 2 ++\n> t/t6000-rev-list-misc.sh | 15 +++++++++++++++\n> 2 files changed, 17 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index c4390f0938..e045445bc3 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2488,10 +2488,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n> \t\t\t \"--left-only\", \"--right-only/--cherry\");\n> \t\trevs->left_only = 1;\n> +\t\trevs->limited = 1;\n> \t} else if (!strcmp(arg, \"--right-only\")) {\n> \t\tif (revs->left_only)\n> \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--right-only\", \"--left-only\");\n> \t\trevs->right_only = 1;\n> +\t\trevs->limited = 1;\n> \t} else if (!strcmp(arg, \"--cherry\")) {\n> \t\tif (revs->left_only)\n> \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cherry\", \"--left-only\");\n> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> index 6289a2e8b0..d338f7ecb4 100755\n> --- a/t/t6000-rev-list-misc.sh\n> +++ b/t/t6000-rev-list-misc.sh\n> @@ -182,4 +182,19 @@ test_expect_success 'rev-list --unpacked' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'rev-list one-sided unrelated symmetric diff' '\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m xyz &&\n> +\tgit branch cmp &&\n> +\tgit rebase --force-rebase --root &&\n> +\n> +\tgit rev-list --left-only HEAD...cmp >head &&\n> +\tgit rev-list --right-only HEAD...cmp >cmp &&\n> +\n> +\tsort head >head.sorted &&\n> +\tsort cmp >cmp.sorted &&\n> +\tcomm -12 head.sorted cmp.sorted >actual &&\n> +\ttest_line_count = 0 actual\n> +'\n> +\n> test_done\n"}]}