rev-list: make "struct rev_list_info" static to the only user
PATCH 1/1 rev-list: make "struct rev_list_info" static to the only user
The structure has nothing to do with what "git bisect" does; as nobody other than "git rev-list" implementation uses it, move it as a private data type to builtin/rev-list.c
---
* https://lore.kernel.org/git/[email protected]/ had this #leftoverbits tangent.
bisect.h | 8 -------- builtin/rev-list.c | 10 +++++++++- 2 files changed, 9 insertions(+), 9 deletions(-)
bisect.h +0 -8
@@ -27,14 +27,6 @@ struct commit_list *filter_skipped(struct commit_list *list,#define FIND_BISECTION_ALL (1u<<0)#define FIND_BISECTION_FIRST_PARENT_ONLY (1u<<1) -struct rev_list_info { -struct rev_info *revs; -int flags; -int show_timestamp; -int hdr_termination; -const char *header_prefix; -}; -/* * enum bisect_error represents the following return codes: * BISECT_OK: success code. Internally, it means that next
builtin/rev-list.c +9 -1
@@ -28,6 +28,14 @@#include "quote.h" #include "strbuf.h" +struct rev_list_info { +struct rev_info *revs; +int flags; +int show_timestamp; +int hdr_termination; +const char *header_prefix; +}; +static const char rev_list_usage[] ="git rev-list [<options>] <commit>... [--] [<path>...]\n" "\n"
@@ -652,7 +660,7 @@ int cmd_rev_list(int argc,*/ /* * NEEDSWORK: These loops that attempt to find presence of -* options without understanding that the options they are +* options without understanding the options they are * skipping are broken (e.g., it would not know "--grep * --exclude-promisor-objects" is not triggering * "--exclude-promisor-objects" option). We really need
Signature · 1 line
2.50.1-446-g8227aac02a
Quoted 3 lines · Junio C Hamano
The structure has nothing to do with what "git bisect" does; as nobody other than "git rev-list" implementation uses it, move it as a private data type to
builtin/rev-list.c
Nice improvement.
builtin/rev-list.c int cmd_rev_list(int argc, · line 666
Quoted 9 lines
builtin/rev-list.c +1
-1@@ -652,7 +660,7 @@ int cmd_rev_list(int argc,*/ /* * NEEDSWORK: These loops that attempt to find presence of -* options without understanding that the options they are +* options without understanding the options they are * skipping are broken (e.g., it would not know "--grep * --exclude-promisor-objects" is not triggering * "--exclude-promisor-objects" option). We really need
This tacked-on bit seems funny to me. Isn't the original more correct? The loops do not understand that the options are broken.
-Peff
Quoted 7 lines · Jeff King
Quoted 3 lines · Junio C Hamano
The structure has nothing to do with what "git bisect" does; as nobody other than "git rev-list" implementation uses it, move it as a private data type to
builtin/rev-list.cNice improvement.
*nod*
Quoted 4 lines
Quoted 4 lines
builtin/rev-list.c +0
-0@@ -652,7 +660,7 @@ int cmd_rev_list(int argc,*/ /* * NEEDSWORK: These loops that attempt to find presence of
Quoted 8 lines
builtin/rev-list.c int cmd_rev_list(int argc, · line 666
Quoted 5 lines
builtin/rev-list.c +1
-1-* options without understanding that the options they are +* options without understanding the options they are * skipping are broken (e.g., it would not know "--grep * --exclude-promisor-objects" is not triggering * "--exclude-promisor-objects" option). We really need This tacked-on bit seems funny to me. Isn't the original more correct? The loops do not understand that the options are broken.
No, the options are fine, but the loops are broken -- they cannot tell what they are looking at is an option or an argument of a preceding option, yet they ignore that latter possibility. So the word "that" is best left out. I also don't see a connection to the struct move, though.
René
Quoted 13 lines · René Scharfe
Quoted 4 lines
Quoted 4 lines
builtin/rev-list.c +0
-0@@ -652,7 +660,7 @@ int cmd_rev_list(int argc,*/ /* * NEEDSWORK: These loops that attempt to find presence of Quoted 8 lines
builtin/rev-list.c int cmd_rev_list(int argc, · line 666
Quoted 5 lines
builtin/rev-list.c +1
-1-* options without understanding that the options they are +* options without understanding the options they are * skipping are broken (e.g., it would not know "--grep * --exclude-promisor-objects" is not triggering * "--exclude-promisor-objects" option). We really need This tacked-on bit seems funny to me. Isn't the original more correct? The loops do not understand that the options are broken.
Quoted 5 lines
No, the options are fine, but the loops are broken -- they cannot tell what they are looking at is an option or an argument of a preceding option, yet they ignore that latter possibility. So the word "that" is best left out. I also don't see a connection to the struct move, though.
Ah, yeah, that makes more sense. It is an awkward sentence either way. :)
-Peff
Quoted 10 lines · René Scharfe
Quoted 9 lines
builtin/rev-list.c int cmd_rev_list(int argc, · line 666
Quoted 6 lines
builtin/rev-list.c +1
-1* NEEDSWORK: These loops that attempt to find presence of -* options without understanding that the options they are +* options without understanding the options they are * skipping are broken (e.g., it would not know "--grep * --exclude-promisor-objects" is not triggering * "--exclude-promisor-objects" option). We really need This tacked-on bit seems funny to me. Isn't the original more correct? The loops do not understand that the options are broken.
Quoted 4 lines
No, the options are fine, but the loops are broken -- they cannot tell what they are looking at is an option or an argument of a preceding option, yet they ignore that latter possibility. So the word "that" is best left out.
Or "... understanding the options, which they are skipping, are broken", perhaps.
Quoted 2 lines
I also don't see a connection to the struct move,
though.
True, this has nothing to do with the main theme of the patch.
PATCH 1/1 rev-list: update a NEEDSWORK comment
In reply to Junio C HamanoThe comment was poorly phrased and it wasn't clear what it wanted to say. Strongly discourage this broken pattern to be copied and pasted to other code paths.
---
* Obviously, fixing this broken code is left as an exercise for readers, as a #leftoverbits item.
builtin/rev-list.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
builtin/rev-list.c +13 -9
@@ -658,17 +658,21 @@ int cmd_rev_list(int argc,* * Let "--missing" to conditionally set fetch_if_missing. */ +/* -* NEEDSWORK: These loops that attempt to find presence of -* options without understanding that the options they are -* skipping are broken (e.g., it would not know "--grep +* NEEDSWORK: The next loop is utterly broken. It tries to +* notice an option is used, but without understanding if each +* option takes an argument, which fundamentally would not +* work. It would not know "--grep * --exclude-promisor-objects" is not triggering -* "--exclude-promisor-objects" option). We really need -* setup_revisions() to have a mechanism to allow and disallow -* some sets of options for different commands (like rev-list, -* replay, etc). Such a mechanism should do an early parsing -* of options and be able to manage the `--missing=...` and -* `--exclude-promisor-objects` options below. +* "--exclude-promisor-objects" option, for example. +* +* We really need setup_revisions() to have a mechanism to +* allow and disallow some sets of options for different +* commands (like rev-list, replay, etc). Such a mechanism +* should do an early parsing of options and be able to manage +* the `--missing=...` and `--exclude-promisor-objects` +* options below. */ for (i =1 ; i < argc; i++) {const char *arg = argv[i];
Re: [PATCH] rev-list: update a NEEDSWORK comment
Quoted 3 lines · Junio C Hamano
The comment was poorly phrased and it wasn't clear what it wanted to say. Strongly discourage this broken pattern to be copied and pasted to other code paths.
Thanks, I was much less confused after your re-wording.
-Peff
Re: [PATCH] rev-list: update a NEEDSWORK comment
In reply to Junio C HamanoQuoted 3 lines · Junio C Hamano
The comment was poorly phrased and it wasn't clear what it wanted to say. Strongly discourage this broken pattern to be copied and pasted to other code paths.
Why “was”? Shouldn’t it be “The comment is poorly phrased ... so [change it]”. According to SubmittingPatches, “present-tense”.
/nitpick
Re: [PATCH] rev-list: update a NEEDSWORK comment
Quoted 7 lines · Kristoffer Haugsbakk
Quoted 3 lines · Junio C Hamano
The comment was poorly phrased and it wasn't clear what it wanted to say. Strongly discourage this broken pattern to be copied and pasted to other code paths.
Why “was”? Shouldn’t it be “The comment is poorly phrased ... so [change it]”. According to SubmittingPatches, “present-tense”.
True. Thanks for spotting.