[Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning

[Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning

[Octavio Carneiro]

A comparison between graph_line->width (of type size_t) and git_graph->width (of type int) causes -Wsign-compare to complain.

Looking at the git_graph struct definition, its size variables are int-typed.

Therefore, I changed the type of graph_line->width to also be a int, thus removing the warning trigger.

Signed-off-by: Octavio Carneiro <ocarneiro1@gmail.com>
---
 graph.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/graph.c b/graph.c
index 26f6fbf000..cb2221700e 100644
--- a/graph.c
+++ b/graph.c
@@ -1,5 +1,3 @@
-#define DISABLE_SIGN_COMPARE_WARNINGS
-
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "config.h"
@@ -115,7 +113,7 @@ static const char *column_get_color_code(unsigned short color)
 
 struct graph_line {
 	struct strbuf *buf;
-	size_t width;
+	int width;
 };
 
 static inline void graph_line_addch(struct graph_line *line, int c)
-- 
2.34.1

Re: [Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning

[Junio C Hamano]

Octavio Carneiro <ocarneiro1@gmail.com> writes:

> A comparison between graph_line->width (of type size_t) and git_graph->width (of type int) causes -Wsign-compare to complain.
>
> Looking at the git_graph struct definition, its size variables are int-typed.
>
> Therefore, I changed the type of graph_line->width to also be a int, thus removing the warning trigger.

An obvious question after reading the above explanation is if it
also would be a valid solution to change the type of git_graph.width
to match the type of graph_line.width instead.  And if so, what was
the reason why you chose to match their types in this direction?

    Side note: I personally prefer, when not dealing with things
    whose size MUST be expressed with size_t, to use platform
    natural "int" to count them, and on-display width of "git log
    --graph" output is certainly something that should not exceed
    thousands for sane people, so I am OK with using "int" myself,
    but if there are obvious two choices and a commit chose one, we
    want to see the reasoning behind the choice explained in the
    proposed commit log message.

By the way, these lines are overly long.  We aim to limit our (physical) line
length to around 70 chars or so by line wrapping.

> diff --git a/graph.c b/graph.c
> index 26f6fbf000..cb2221700e 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1,5 +1,3 @@
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
>  #include "git-compat-util.h"
>  #include "gettext.h"
>  #include "config.h"
> @@ -115,7 +113,7 @@ static const char *column_get_color_code(unsigned short color)
>  
>  struct graph_line {
>  	struct strbuf *buf;
> -	size_t width;
> +	int width;
>  };

When functions like graph_line_addstr() widens the line width, it
does things like

	line->width += strlen(s);

which could make line->width eventually overflow no matter whether
its type is "int" or "size_t".  Making .width that used to be
"size_t" into "int" may make it easier (simply because "int" is
often much narrower than "size_t") to happen, but your change does
not make the code worse than the original.

Since I anticipate some senior developers advocate for using
"size_t" uniformly (instead of the platform natural "int"), I'll let
them speak up before touching this patch.

Thanks.


P.S.  

A totally unrelated tangent.

I notice that graph->width and line->width code assumes that a
single byte is always one display column wide.  It might be an
intersting GSoC or Outreachy project to use Unicode graphics instead
of limiting us to ASCII art to draw these graph.

Re: [Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning

[O. C.]

On Wed, May 21, 2025 at 8:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> An obvious question after reading the above explanation is if it
> also would be a valid solution to change the type of git_graph.width
> to match the type of graph_line.width instead.  And if so, what was
> the reason why you chose to match their types in this direction?

While writing the patch, I saw how the git_graph and graph_line structs
related to each other, and understood git_graph as being the "more
important" struct in this context. So, it seemed more reasonable to
have graph_line.width comply with git_graph.width type.

I do see the opposite point, taking notice of graph_line.width interactions
with "size_t". With those in mind, having the type as "size_t" does
also make a lot of sense.

However, considering that:
> "git log --graph" output is certainly something that should not exceed
> thousands for sane people
It feels intuitive to " use platform natural "int" " as you put it.

> By the way, these lines are overly long.  We aim to limit our (physical) line
> length to around 70 chars or so by line wrapping.
Sorry about that, I'll keep them shorter.

Lastly, thanks for the quick and careful response to the patch. As a
first timer
I appreciate the discussion.

- Octavio