{"thread":{"id":"63807","subject":"[PATCH] rev-list: make \"struct rev_list_info\" static to the only user","startedAt":"2025-07-18T23:58:07Z","lastAt":"2025-07-22T13:18:14Z","messageCount":9,"participants":["Junio C Hamano","Jeff King","René Scharfe","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"522297","messageId":"xmqqa551127o.fsf@gitster.g","threadId":"63807","inReplyTo":null,"subject":"[PATCH] rev-list: make \"struct rev_list_info\" static to the only user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-18T23:58:03Z","receivedAt":"2025-07-18T23:58:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The structure has nothing to do with what \"git bisect\" does; as\nnobody other than \"git rev-list\" implementation uses it, move it\nas a private data type to builtin/rev-list.c\n\nSigned-off-by: Junio C Hamano \n---\n * https://lore.kernel.org/git/xmqq1qdptffk.fsf@gitster.g/ had this\n #leftoverbits tangent.\n\n bisect.h | 8 --------\n builtin/rev-list.c | 10 +++++++++-\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/bisect.h b/bisect.h\nindex 944439bfac..8621460f93 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -27,14 +27,6 @@ struct commit_list *filter_skipped(struct commit_list *list,\n #define FIND_BISECTION_ALL\t\t\t(1u<<0)\n #define FIND_BISECTION_FIRST_PARENT_ONLY\t(1u<<1)\n \n-struct rev_list_info {\n-\tstruct rev_info *revs;\n-\tint flags;\n-\tint show_timestamp;\n-\tint hdr_termination;\n-\tconst char *header_prefix;\n-};\n-\n /*\n * enum bisect_error represents the following return codes:\n * BISECT_OK: success code. Internally, it means that next\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 0984b607bf..0a89f4cbf7 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -28,6 +28,14 @@\n #include \"quote.h\"\n #include \"strbuf.h\"\n \n+struct rev_list_info {\n+\tstruct rev_info *revs;\n+\tint flags;\n+\tint show_timestamp;\n+\tint hdr_termination;\n+\tconst char *header_prefix;\n+};\n+\n static const char rev_list_usage[] =\n \"git rev-list [] ... [--] [...]\\n\"\n \"\\n\"\n@@ -652,7 +660,7 @@ int cmd_rev_list(int argc,\n \t */\n \t/*\n \t * NEEDSWORK: These loops that attempt to find presence of\n-\t * options without understanding that the options they are\n+\t * options without understanding the options they are\n \t * skipping are broken (e.g., it would not know \"--grep\n \t * --exclude-promisor-objects\" is not triggering\n \t * \"--exclude-promisor-objects\" option). We really need\n-- \n2.50.1-446-g8227aac02a\n\n"},{"id":"522305","messageId":"20250719063551.GA705356@coredump.intra.peff.net","threadId":"63807","inReplyTo":"xmqqa551127o.fsf@gitster.g","subject":"Re: [PATCH] rev-list: make \"struct rev_list_info\" static to the only user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-19T06:35:51Z","receivedAt":"2025-07-19T06:35:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 18, 2025 at 04:58:03PM -0700, Junio C Hamano wrote:\n\n> The structure has nothing to do with what \"git bisect\" does; as\n> nobody other than \"git rev-list\" implementation uses it, move it\n> as a private data type to builtin/rev-list.c\n\nNice improvement.\n\n> @@ -652,7 +660,7 @@ int cmd_rev_list(int argc,\n> \t */\n> \t/*\n> \t * NEEDSWORK: These loops that attempt to find presence of\n> -\t * options without understanding that the options they are\n> +\t * options without understanding the options they are\n> \t * skipping are broken (e.g., it would not know \"--grep\n> \t * --exclude-promisor-objects\" is not triggering\n> \t * \"--exclude-promisor-objects\" option). We really need\n\nThis tacked-on bit seems funny to me. Isn't the original more correct?\nThe loops do not understand that the options are broken.\n\n-Peff\n"},{"id":"522319","messageId":"fc4da804-37ed-4425-8dda-14cfcb607906@web.de","threadId":"63807","inReplyTo":"20250719063551.GA705356@coredump.intra.peff.net","subject":"Re: [PATCH] rev-list: make \"struct rev_list_info\" static to the only user","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-19T12:36:04Z","receivedAt":"2025-07-19T12:36:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/19/25 8:35 AM, Jeff King wrote:\n> On Fri, Jul 18, 2025 at 04:58:03PM -0700, Junio C Hamano wrote:\n> \n>> The structure has nothing to do with what \"git bisect\" does; as\n>> nobody other than \"git rev-list\" implementation uses it, move it\n>> as a private data type to builtin/rev-list.c\n> \n> Nice improvement.\n\n*nod*\n\n>> @@ -652,7 +660,7 @@ int cmd_rev_list(int argc,\n>> \t */\n>> \t/*\n>> \t * NEEDSWORK: These loops that attempt to find presence of\n>> -\t * options without understanding that the options they are\n>> +\t * options without understanding the options they are\n>> \t * skipping are broken (e.g., it would not know \"--grep\n>> \t * --exclude-promisor-objects\" is not triggering\n>> \t * \"--exclude-promisor-objects\" option). We really need\n> \n> This tacked-on bit seems funny to me. Isn't the original more correct?\n> The loops do not understand that the options are broken.\n\nNo, the options are fine, but the loops are broken -- they cannot tell\nwhat they are looking at is an option or an argument of a preceding\noption, yet they ignore that latter possibility. So the word \"that\"\nis best left out. I also don't see a connection to the struct move,\nthough.\n\nRené\n\n"},{"id":"522328","messageId":"20250720000426.GB743449@coredump.intra.peff.net","threadId":"63807","inReplyTo":"fc4da804-37ed-4425-8dda-14cfcb607906@web.de","subject":"Re: [PATCH] rev-list: make \"struct rev_list_info\" static to the only user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-20T00:04:26Z","receivedAt":"2025-07-20T00:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 19, 2025 at 02:36:04PM +0200, René Scharfe wrote:\n\n> >> @@ -652,7 +660,7 @@ int cmd_rev_list(int argc,\n> >> \t */\n> >> \t/*\n> >> \t * NEEDSWORK: These loops that attempt to find presence of\n> >> -\t * options without understanding that the options they are\n> >> +\t * options without understanding the options they are\n> >> \t * skipping are broken (e.g., it would not know \"--grep\n> >> \t * --exclude-promisor-objects\" is not triggering\n> >> \t * \"--exclude-promisor-objects\" option). We really need\n> > \n> > This tacked-on bit seems funny to me. Isn't the original more correct?\n> > The loops do not understand that the options are broken.\n> \n> No, the options are fine, but the loops are broken -- they cannot tell\n> what they are looking at is an option or an argument of a preceding\n> option, yet they ignore that latter possibility. So the word \"that\"\n> is best left out. I also don't see a connection to the struct move,\n> though.\n\nAh, yeah, that makes more sense. It is an awkward sentence either way. :)\n\n-Peff\n"},{"id":"522329","messageId":"xmqq4iv71yov.fsf@gitster.g","threadId":"63807","inReplyTo":"fc4da804-37ed-4425-8dda-14cfcb607906@web.de","subject":"Re: [PATCH] rev-list: make \"struct rev_list_info\" static to the only user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-20T00:41:04Z","receivedAt":"2025-07-20T00:41:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>>> \t * NEEDSWORK: These loops that attempt to find presence of\n>>> -\t * options without understanding that the options they are\n>>> +\t * options without understanding the options they are\n>>> \t * skipping are broken (e.g., it would not know \"--grep\n>>> \t * --exclude-promisor-objects\" is not triggering\n>>> \t * \"--exclude-promisor-objects\" option). We really need\n>> \n>> This tacked-on bit seems funny to me. Isn't the original more correct?\n>> The loops do not understand that the options are broken.\n>\n> No, the options are fine, but the loops are broken -- they cannot tell\n> what they are looking at is an option or an argument of a preceding\n> option, yet they ignore that latter possibility. So the word \"that\"\n> is best left out.\n\nOr \"... understanding the options, which they are skipping, are\nbroken\", perhaps.\n\n> I also don't see a connection to the struct move,\n> though.\n\nTrue, this has nothing to do with the main theme of the patch.\n"},{"id":"522377","messageId":"xmqqecu9w4a5.fsf@gitster.g","threadId":"63807","inReplyTo":"xmqqa551127o.fsf@gitster.g","subject":"[PATCH] rev-list: update a NEEDSWORK comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-21T22:46:42Z","receivedAt":"2025-07-21T22:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The comment was poorly phrased and it wasn't clear what it wanted to\nsay. Strongly discourage this broken pattern to be copied and\npasted to other code paths.\n\nSigned-off-by: Junio C Hamano \n---\n * Obviously, fixing this broken code is left as an exercise for\n readers, as a #leftoverbits item.\n\n builtin/rev-list.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git c/builtin/rev-list.c w/builtin/rev-list.c\nindex 2bb6360ec1..7549114635 100644\n--- c/builtin/rev-list.c\n+++ w/builtin/rev-list.c\n@@ -658,17 +658,21 @@ int cmd_rev_list(int argc,\n \t *\n \t * Let \"--missing\" to conditionally set fetch_if_missing.\n \t */\n+\n \t/*\n-\t * NEEDSWORK: These loops that attempt to find presence of\n-\t * options without understanding that the options they are\n-\t * skipping are broken (e.g., it would not know \"--grep\n+\t * NEEDSWORK: The next loop is utterly broken. It tries to\n+\t * notice an option is used, but without understanding if each\n+\t * option takes an argument, which fundamentally would not\n+\t * work. It would not know \"--grep\n \t * --exclude-promisor-objects\" is not triggering\n-\t * \"--exclude-promisor-objects\" option). We really need\n-\t * setup_revisions() to have a mechanism to allow and disallow\n-\t * some sets of options for different commands (like rev-list,\n-\t * replay, etc). Such a mechanism should do an early parsing\n-\t * of options and be able to manage the `--missing=...` and\n-\t * `--exclude-promisor-objects` options below.\n+\t * \"--exclude-promisor-objects\" option, for example.\n+\t *\n+\t * We really need setup_revisions() to have a mechanism to\n+\t * allow and disallow some sets of options for different\n+\t * commands (like rev-list, replay, etc). Such a mechanism\n+\t * should do an early parsing of options and be able to manage\n+\t * the `--missing=...` and `--exclude-promisor-objects`\n+\t * options below.\n \t */\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n"},{"id":"522407","messageId":"20250722081659.GA862006@coredump.intra.peff.net","threadId":"63807","inReplyTo":"xmqqecu9w4a5.fsf@gitster.g","subject":"Re: [PATCH] rev-list: update a NEEDSWORK comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:16:59Z","receivedAt":"2025-07-22T08:17:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 21, 2025 at 03:46:42PM -0700, Junio C Hamano wrote:\n\n> The comment was poorly phrased and it wasn't clear what it wanted to\n> say. Strongly discourage this broken pattern to be copied and\n> pasted to other code paths.\n\nThanks, I was much less confused after your re-wording.\n\n-Peff\n"},{"id":"522414","messageId":"1599cd31-436c-4ac7-bbef-be7dfbff00a3@app.fastmail.com","threadId":"63807","inReplyTo":"xmqqecu9w4a5.fsf@gitster.g","subject":"Re: [PATCH] rev-list: update a NEEDSWORK comment","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-07-22T08:41:02Z","receivedAt":"2025-07-22T08:41:23Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Tue, Jul 22, 2025, at 00:46, Junio C Hamano wrote:\n> The comment was poorly phrased and it wasn't clear what it wanted to\n> say. Strongly discourage this broken pattern to be copied and\n> pasted to other code paths.\n\nWhy “was”? Shouldn’t it be “The comment is poorly phrased ... so\n[change it]”. According to SubmittingPatches, “present-tense”.\n\n/nitpick\n"},{"id":"522438","messageId":"xmqqzfcwuzxo.fsf@gitster.g","threadId":"63807","inReplyTo":"1599cd31-436c-4ac7-bbef-be7dfbff00a3@app.fastmail.com","subject":"Re: [PATCH] rev-list: update a NEEDSWORK comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T13:18:11Z","receivedAt":"2025-07-22T13:18:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Tue, Jul 22, 2025, at 00:46, Junio C Hamano wrote:\n>> The comment was poorly phrased and it wasn't clear what it wanted to\n>> say. Strongly discourage this broken pattern to be copied and\n>> pasted to other code paths.\n>\n> Why “was”? Shouldn’t it be “The comment is poorly phrased ... so\n> [change it]”. According to SubmittingPatches, “present-tense”.\n\nTrue. Thanks for spotting.\n"}]}