Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings

[shejialuo <shejialuo@gmail.com>]

On Fri, Dec 27, 2024 at 02:57:18PM +0100, Patrick Steinhardt wrote:
> On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote:
> > On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:
> > > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
> > >  	unsigned long size;
> > >  	enum object_type type;
> > >  	char *buf = repo_read_object_file(the_repository, oid, &type, &size);
> > > -	int offset = 0;
> > > +	unsigned long offset = 0;
> > 
> > Why here we use `unsigned long`, is this a special situation where we
> > cannot use `size_t`?
> 
> Mostly because other variables already use `unsigned long` here,
> including `repo_read_object_file()`. So given that our object layer
> doesn't support `size_t` it wouldn't make sense to use it for the
> offset, either.
> 

Make sense. Thanks for the explanation.

> > >  
> > >  	if (!buf)
> > >  		return error(_("could not read object %s"), oid_to_hex(oid));
> > >  
> > >  	assert(type == OBJ_TAG);
> > >  	while (offset < size && buf[offset] != '\n') {
> > > -		int new_offset = offset + 1;
> > > +		unsigned long new_offset = offset + 1;
> > >  		const char *ident;
> > >  		while (new_offset < size && buf[new_offset++] != '\n')
> > >  			; /* do nothing */
> > 
> > > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,
> > >  		fmt_patch_suffix = cfg.fmt_patch_suffix;
> > >  
> > >  	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
> > > -	if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
> > > +	if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix)))
> > 
> > A design question, why we don't change the type of
> > `cfg.log.fmt_patch_name_max` to be `size_t`?
> 
> The whole infra around this uses `int`s, too, so changing this variable
> here alone wouldn't suffice. We'd also have to adapt option handling,
> config handling, `struct rev_info::patch_name_max` and other bits and
> pieces. So in the end the size of required changes would likely balloon
> and thus I decided to not go down this rabbit hole.
> 

Yes, I agree. If we do this, there is a lot of efforts we need to deal
with.

> Patrick