[PATCH] rev-list: make "struct rev_list_info" static to the only user

[PATCH] rev-list: make "struct rev_list_info" static to the only user

[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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * https://lore.kernel.org/git/xmqq1qdptffk.fsf@gitster.g/ had this
   #leftoverbits tangent.

 bisect.h           |  8 --------
 builtin/rev-list.c | 10 +++++++++-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/bisect.h b/bisect.h
index 944439bfac..8621460f93 100644
--- a/bisect.h
+++ b/bisect.h
@@ -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
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0984b607bf..0a89f4cbf7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -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
-- 
2.50.1-446-g8227aac02a

Re: [PATCH] rev-list: make "struct rev_list_info" static to the only user

[Jeff King]

On Fri, Jul 18, 2025 at 04:58:03PM -0700, Junio C Hamano wrote:

> 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.

> @@ -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

Re: [PATCH] rev-list: make "struct rev_list_info" static to the only user

[René Scharfe]

On 7/19/25 8:35 AM, Jeff King wrote:
> On Fri, Jul 18, 2025 at 04:58:03PM -0700, Junio C Hamano wrote:
> 
>> 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.

*nod*

>> @@ -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.

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é

Re: [PATCH] rev-list: make "struct rev_list_info" static to the only user

[Jeff King]

On Sat, Jul 19, 2025 at 02:36:04PM +0200, René Scharfe wrote:

> >> @@ -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.
> 
> 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

Re: [PATCH] rev-list: make "struct rev_list_info" static to the only user

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>>>  	 * 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.
>
> 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.

> 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] rev-list: update a NEEDSWORK comment

[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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * 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(-)

diff --git c/builtin/rev-list.c w/builtin/rev-list.c
index 2bb6360ec1..7549114635 100644
--- c/builtin/rev-list.c
+++ w/builtin/rev-list.c
@@ -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

[Jeff King]

On Mon, Jul 21, 2025 at 03:46:42PM -0700, Junio C Hamano wrote:

> 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

[Kristoffer Haugsbakk]

On Tue, Jul 22, 2025, at 00:46, Junio C Hamano wrote:
> 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

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Tue, Jul 22, 2025, at 00:46, Junio C Hamano wrote:
>> 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.