threads / patch / 63807

patchrev-list: make "struct rev_list_info" static to the only user

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

## tl;dr

9 messages between Jul 18, 2025 and Jul 22, 2025. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Junio C Hamano· Jul 18, 2025, 23:58 UTC · lore

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(-)
Show changes to 2 files +9 −9

bisect.h, builtin/rev-list.c

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
Jeff King· Jul 19, 2025, 06:35 UTC · re: Junio C Hamano · lore

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

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.
Show 9 quoted lines
> @@ -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
René Scharfe· Jul 19, 2025, 12:36 UTC · re: Jeff King · lore

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

On 7/19/25 8:35 AM, Jeff King wrote:
Show 7 quoted lines
> 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*
Show 12 quoted lines
>> @@ -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é
Jeff King· Jul 20, 2025, 00:04 UTC · re: René Scharfe · lore

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

On Sat, Jul 19, 2025 at 02:36:04PM +0200, René Scharfe wrote:
Show 18 quoted lines
> >> @@ -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
Junio C Hamano· Jul 20, 2025, 00:41 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 14 quoted lines
>>>  	 * 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.
Junio C Hamano· Jul 21, 2025, 22:46 UTC · re: Junio C Hamano · lore

[PATCH] rev-list: update a NEEDSWORK comment

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(-)
Show changes to diff +13 −9
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];
Jeff King· Jul 22, 2025, 08:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list: update a NEEDSWORK comment

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
Kristoffer Haugsbakk· Jul 22, 2025, 08:41 UTC · re: Junio C Hamano · lore

Re: [PATCH] rev-list: update a NEEDSWORK comment

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
Junio C Hamano· Jul 22, 2025, 13:18 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] rev-list: update a NEEDSWORK comment

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
Show 7 quoted lines
> 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.

← back to recent threads