{"thread":{"id":"49652","subject":"[PATCH] Clear --exclude list after 'git rev-parse --all'","startedAt":"2018-10-23T19:18:03Z","lastAt":"2018-10-27T07:21:00Z","messageCount":10,"participants":["Andreas Gruenbacher","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361316","messageId":"20181023191758.15138-1-agruenba@redhat.com","threadId":"49652","inReplyTo":null,"subject":"[PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-23T19:17:58Z","receivedAt":"2018-10-23T19:18:03Z","isPatch":true,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"Commit [1] added the --exclude option to revision.c. The --all,\n--branches, --tags, --remotes, and --glob options clear the exclude\nlist. Shortly therafter, commit [2] added the same to 'git rev-parse',\nbut without clearing the exclude list for the --all option. Fix that.\n\n[1] e7b432c52 (\"revision: introduce --exclude= to tame wildcards\", 2013-08-30)\n[2] 9dc01bf06 (\"rev-parse: introduce --exclude= to tame wildcards\", 2013-11-01)\n\nSigned-off-by: Andreas Gruenbacher \n---\n builtin/rev-parse.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 0f09bbbf6..c71e3b104 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n \t\t\t\tfor_each_ref(show_reference, NULL);\n+\t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--disambiguate=\", &arg)) {\n-- \n2.17.2\n\n"},{"id":"361360","messageId":"xmqqefcgotbk.fsf@gitster-ct.c.googlers.com","threadId":"49652","inReplyTo":"20181023191758.15138-1-agruenba@redhat.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T04:35:27Z","receivedAt":"2018-10-24T04:35:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Gruenbacher writes:\n\n> Commit [1] added the --exclude option to revision.c. The --all,\n> --branches, --tags, --remotes, and --glob options clear the exclude\n> list. Shortly therafter, commit [2] added the same to 'git rev-parse',\n> but without clearing the exclude list for the --all option. Fix that.\n>\n> [1] e7b432c52 (\"revision: introduce --exclude= to tame wildcards\", 2013-08-30)\n> [2] 9dc01bf06 (\"rev-parse: introduce --exclude= to tame wildcards\", 2013-11-01)\n>\n> Signed-off-by: Andreas Gruenbacher \n> ---\n> builtin/rev-parse.c | 1 +\n> 1 file changed, 1 insertion(+)\n\nAll other glob options do show_reference with for_each_ref_in() and\nthen calls clear_ref_exclusion(), and logically the patch makes\nsense. \n\nWhat is the \"problem\" this patch fixes, though? Is it easy to add a\nnew test to t/6018-rev-list-glob.sh to demonstrate that \"--glob\" (or\nwhatever that clears exclusion list without this patch) works\ncorrectly but \"--all\" misbehaves without this change?\n\nThanks.\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 0f09bbbf6..c71e3b104 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--all\")) {\n> \t\t\t\tfor_each_ref(show_reference, NULL);\n> +\t\t\t\tclear_ref_exclusion(&ref_excludes);\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (skip_prefix(arg, \"--disambiguate=\", &arg)) {\n"},{"id":"361392","messageId":"CAHc6FU6YLeTBU02k-Md7+H5kJDq2RXZmAXMaq8ZkiuSmZFXbwQ@mail.gmail.com","threadId":"49652","inReplyTo":"xmqqefcgotbk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-24T09:01:54Z","receivedAt":"2018-10-24T09:02:09Z","isPatch":true,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"On Wed, 24 Oct 2018 at 06:35, Junio C Hamano wrote:\n> Andreas Gruenbacher writes:\n>\n> > Commit [1] added the --exclude option to revision.c. The --all,\n> > --branches, --tags, --remotes, and --glob options clear the exclude\n> > list. Shortly therafter, commit [2] added the same to 'git rev-parse',\n> > but without clearing the exclude list for the --all option. Fix that.\n> >\n> > [1] e7b432c52 (\"revision: introduce --exclude= to tame wildcards\", 2013-08-30)\n> > [2] 9dc01bf06 (\"rev-parse: introduce --exclude= to tame wildcards\", 2013-11-01)\n> >\n> > Signed-off-by: Andreas Gruenbacher \n> > ---\n> > builtin/rev-parse.c | 1 +\n> > 1 file changed, 1 insertion(+)\n>\n> All other glob options do show_reference with for_each_ref_in() and\n> then calls clear_ref_exclusion(), and logically the patch makes\n> sense.\n>\n> What is the \"problem\" this patch fixes, though? Is it easy to add a\n> new test to t/6018-rev-list-glob.sh to demonstrate that \"--glob\" (or\n> whatever that clears exclusion list without this patch) works\n> correctly but \"--all\" misbehaves without this change?\n\nThe test suite doesn't cover clearing the exclusion list for any of\nthose rev-parse options and I also didn't write such a test case. I\nran into this inconsistency during code review.\n\nAndreas\n"},{"id":"361399","messageId":"xmqqr2gfn1cy.fsf@gitster-ct.c.googlers.com","threadId":"49652","inReplyTo":"CAHc6FU6YLeTBU02k-Md7+H5kJDq2RXZmAXMaq8ZkiuSmZFXbwQ@mail.gmail.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T09:24:45Z","receivedAt":"2018-10-24T09:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Gruenbacher writes:\n\n>> All other glob options do show_reference with for_each_ref_in() and\n>> then calls clear_ref_exclusion(), and logically the patch makes\n>> sense.\n>>\n>> What is the \"problem\" this patch fixes, though? Is it easy to add a\n>> new test to t/6018-rev-list-glob.sh to demonstrate that \"--glob\" (or\n>> whatever that clears exclusion list without this patch) works\n>> correctly but \"--all\" misbehaves without this change?\n>\n> The test suite doesn't cover clearing the exclusion list for any of\n> those rev-parse options and I also didn't write such a test case. I\n> ran into this inconsistency during code review.\n\nThat is why I asked what \"problem\" this patch fixes. Without\nanswering that question, it is unclear if the patch is completing\nmissing coverage for \"--all\", or it is cargo culting an useless\nclearing done for \"--glob\" and friends to code for \"--all\" that did\nnot do the same useless clearing. IOW, there are two ways to address\nthe \"inconsistency\", and the proposed log message (nor your answer\nabove) does not make a convincing argument why adding the same code\nto the \"--all\" side is the right way to achieve consistency---rather\nthan removing the call to clear from the existing ones.\n\nThanks.\n"},{"id":"361401","messageId":"CAHc6FU5CpBNpzfPs0hkNm9Hw1iYnALvUY4WL1z=sPVT10fq+zw@mail.gmail.com","threadId":"49652","inReplyTo":"xmqqr2gfn1cy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Andreas Gruenbacher","fromEmail":"agruenba@redhat.com","sentAt":"2018-10-24T09:49:06Z","receivedAt":"2018-10-24T09:49:19Z","isPatch":true,"sender":{"key":"agruenba@redhat.com","avatar":null},"body":"On Wed, 24 Oct 2018 at 11:24, Junio C Hamano wrote:\n> Andreas Gruenbacher writes:\n> >> All other glob options do show_reference with for_each_ref_in() and\n> >> then calls clear_ref_exclusion(), and logically the patch makes\n> >> sense.\n> >>\n> >> What is the \"problem\" this patch fixes, though? Is it easy to add a\n> >> new test to t/6018-rev-list-glob.sh to demonstrate that \"--glob\" (or\n> >> whatever that clears exclusion list without this patch) works\n> >> correctly but \"--all\" misbehaves without this change?\n> >\n> > The test suite doesn't cover clearing the exclusion list for any of\n> > those rev-parse options and I also didn't write such a test case. I\n> > ran into this inconsistency during code review.\n>\n> That is why I asked what \"problem\" this patch fixes. Without\n> answering that question, it is unclear if the patch is completing\n> missing coverage for \"--all\", or it is cargo culting an useless\n> clearing done for \"--glob\" and friends to code for \"--all\" that did\n> not do the same useless clearing.\n\nThis is how the --exclude option is described:\n\n --exclude=\n Do not include refs matching that the next\n--all, --branches,\n --tags, --remotes, or --glob would otherwise consider.\nRepetitions of this\n option accumulate exclusion patterns up to the next --all,\n--branches, --tags,\n --remotes, or --glob option (other options or arguments do not clear\n accumulated patterns).\n\nI'm assuming that some thought has gone into making the options behave\nin this particular way. The implementation in revision.c follows this\npattern, but the implementation in builtin/rev-parse.c only almost\ndoes.\n\nAndreas\n"},{"id":"361503","messageId":"20181025104542.GA14455@sigill.intra.peff.net","threadId":"49652","inReplyTo":"CAHc6FU5CpBNpzfPs0hkNm9Hw1iYnALvUY4WL1z=sPVT10fq+zw@mail.gmail.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T10:45:43Z","receivedAt":"2018-10-25T10:45:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 24, 2018 at 11:49:06AM +0200, Andreas Gruenbacher wrote:\n\n> > That is why I asked what \"problem\" this patch fixes. Without\n> > answering that question, it is unclear if the patch is completing\n> > missing coverage for \"--all\", or it is cargo culting an useless\n> > clearing done for \"--glob\" and friends to code for \"--all\" that did\n> > not do the same useless clearing.\n> \n> This is how the --exclude option is described:\n> \n> --exclude=\n> Do not include refs matching that the next\n> --all, --branches,\n> --tags, --remotes, or --glob would otherwise consider.\n> Repetitions of this\n> option accumulate exclusion patterns up to the next --all,\n> --branches, --tags,\n> --remotes, or --glob option (other options or arguments do not clear\n> accumulated patterns).\n> \n> I'm assuming that some thought has gone into making the options behave\n> in this particular way. The implementation in revision.c follows this\n> pattern, but the implementation in builtin/rev-parse.c only almost\n> does.\n\nYeah. I think this is just a bug in 9dc01bf063 (rev-parse: introduce\n--exclude= to tame wildcards, 2013-11-01), in that it's handling\nof --all differed from e7b432c521 (revision: introduce --exclude=\nto tame wildcards, 2013-08-30). The clearing is very much intentional\nand documented, and in general rev-parse should behave the same as\nrev-list.\n\nAn easy test is:\n\n $ git rev-list --no-walk --exclude='*' --all --all\n 3289ca716320457af5d2dd550b716282ad22da11\n ...a bunch of other tip commits...\n\n $ git rev-parse --exclude='*' --all --all\n [no output, but it should print those same tip commits]\n\n-Peff\n"},{"id":"361579","messageId":"xmqqtvl9fv86.fsf@gitster-ct.c.googlers.com","threadId":"49652","inReplyTo":"20181025104542.GA14455@sigill.intra.peff.net","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T23:43:37Z","receivedAt":"2018-10-25T23:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> An easy test is:\n>\n> $ git rev-list --no-walk --exclude='*' --all --all\n> 3289ca716320457af5d2dd550b716282ad22da11\n> ...a bunch of other tip commits...\n>\n> $ git rev-parse --exclude='*' --all --all\n> [no output, but it should print those same tip commits]\n\nI actually was hoping to see a test that contrasts \"--all\" (which\nlacks the alleged \"clear exclude\" bug) with another option that does\nhave the \"clear exclude\", both used with rev-parse, i.e.\n\n $ git rev-parse --exclude='*' --glob='*' --glob='*'\n ... all the ref tips ...\n $ git rev-parse --exclude='*' --all --all\n ... ought to be equivalent, but is empty due to the bug ...\n\nwould have been a good demonstration that shows what bug we are\nfixing d(and would have been a good test to accompany the patch.\n\n\n\n\n\n\n"},{"id":"361605","messageId":"20181026074638.GA3119@sigill.intra.peff.net","threadId":"49652","inReplyTo":"xmqqtvl9fv86.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-26T07:46:38Z","receivedAt":"2018-10-26T07:46:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 26, 2018 at 08:43:37AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > An easy test is:\n> >\n> > $ git rev-list --no-walk --exclude='*' --all --all\n> > 3289ca716320457af5d2dd550b716282ad22da11\n> > ...a bunch of other tip commits...\n> >\n> > $ git rev-parse --exclude='*' --all --all\n> > [no output, but it should print those same tip commits]\n> \n> I actually was hoping to see a test that contrasts \"--all\" (which\n> lacks the alleged \"clear exclude\" bug) with another option that does\n> have the \"clear exclude\", both used with rev-parse, i.e.\n> \n> $ git rev-parse --exclude='*' --glob='*' --glob='*'\n> ... all the ref tips ...\n> $ git rev-parse --exclude='*' --all --all\n> ... ought to be equivalent, but is empty due to the bug ...\n> \n> would have been a good demonstration that shows what bug we are\n> fixing d(and would have been a good test to accompany the patch.\n\nYeah, I agree that would be fine, too. I think there are two dimensions\nin which to look at the problem, like so:\n\n rev-list rev-parse\n -------- ---------\n--glob clears clears\n--all clears does not clear\n\nTesting either the row or the column (or both) works for me. :)\n\n-Peff\n"},{"id":"361762","messageId":"xmqq5zxnc17b.fsf@gitster-ct.c.googlers.com","threadId":"49652","inReplyTo":"20181026074638.GA3119@sigill.intra.peff.net","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-27T07:12:40Z","receivedAt":"2018-10-27T07:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I actually was hoping to see a test that contrasts \"--all\" (which\n>> lacks the alleged \"clear exclude\" bug) with another option that does\n>> have the \"clear exclude\", both used with rev-parse, i.e.\n>> \n>> $ git rev-parse --exclude='*' --glob='*' --glob='*'\n>> ... all the ref tips ...\n>> $ git rev-parse --exclude='*' --all --all\n>> ... ought to be equivalent, but is empty due to the bug ...\n>> \n>> would have been a good demonstration that shows what bug we are\n>> fixing d(and would have been a good test to accompany the patch.\n>\n> Yeah, I agree that would be fine, too. I think there are two dimensions\n> in which to look at the problem, like so:\n>\n> rev-list rev-parse\n> -------- ---------\n> --glob clears clears\n> --all clears does not clear\n>\n> Testing either the row or the column (or both) works for me. :)\n\nOK, so let's not leave this loose end untied. This may be good\nenough to squash in.\n\n builtin/rev-parse.c | 1 -\n t/t6018-rev-list-glob.sh | 12 ++++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex f4847d3008..a1e680b5e9 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -760,7 +760,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--all\")) {\n \t\t\t\tfor_each_ref(show_reference, NULL);\n-\t\t\t\tclear_ref_exclusion(&ref_excludes);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (skip_prefix(arg, \"--disambiguate=\", &arg)) {\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex d3453c583c..b28075b65d 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -141,6 +141,18 @@ test_expect_success 'rev-parse accumulates multiple --exclude' '\n \tcompare rev-parse \"--exclude=refs/remotes/* --exclude=refs/tags/* --all\" --branches\n '\n \n+test_expect_success 'rev-parse --branches clears --exclude' '\n+\tcompare rev-parse \"--exclude=* --branches --branches\" \"--branches\"\n+'\n+\n+test_expect_success 'rev-parse --tags clears --exclude' '\n+\tcompare rev-parse \"--exclude=* --tags --tags\" \"--tags\"\n+'\n+\n+test_expect_success 'rev-parse --all clears --exclude' '\n+\tcompare rev-parse \"--exclude=* --all --all\" \"--all\"\n+'\n+\n test_expect_success 'rev-list --glob=refs/heads/subspace/*' '\n \n \tcompare rev-list \"subspace/one subspace/two\" \"--glob=refs/heads/subspace/*\"\n"},{"id":"361763","messageId":"20181027072056.GA26545@sigill.intra.peff.net","threadId":"49652","inReplyTo":"xmqq5zxnc17b.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Clear --exclude list after 'git rev-parse --all'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-27T07:20:57Z","receivedAt":"2018-10-27T07:21:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 27, 2018 at 04:12:40PM +0900, Junio C Hamano wrote:\n\n> > Yeah, I agree that would be fine, too. I think there are two dimensions\n> > in which to look at the problem, like so:\n> >\n> > rev-list rev-parse\n> > -------- ---------\n> > --glob clears clears\n> > --all clears does not clear\n> >\n> > Testing either the row or the column (or both) works for me. :)\n> \n> OK, so let's not leave this loose end untied. This may be good\n> enough to squash in.\n> [...]\n> +test_expect_success 'rev-parse --branches clears --exclude' '\n> +\tcompare rev-parse \"--exclude=* --branches --branches\" \"--branches\"\n> +'\n> +\n> +test_expect_success 'rev-parse --tags clears --exclude' '\n> +\tcompare rev-parse \"--exclude=* --tags --tags\" \"--tags\"\n> +'\n> +\n> +test_expect_success 'rev-parse --all clears --exclude' '\n> +\tcompare rev-parse \"--exclude=* --all --all\" \"--all\"\n> +'\n\nYes, this looks good to me. In theory a more intricate test might catch\nother kinds of bugs (e.g., a more limited exclude and making sure it was\napplied correctly in each place), but I don't think it's really worth\nthe effort.\n\n-Peff\n"}]}