[GSoC][PATCH 0/1] Use unsigned integral type for collection of bits.

[GSoC][PATCH 0/1] Use unsigned integral type for collection of bits.

[Eugenio Gigante]

One of the suggested microprojects for the GSoC is to pick a field
of signed integral type that is used as as collection of bits, and
change its type to unsigned in case the code does not take advantage
of its MSb.
This patch finds an example in 'builtin/add.c'.


Eugenio Gigante (1):
  add: use unsigned type for collection of bits

 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
-- 
2.43.0

[GSoC][PATCH 1/1] add: use unsigned type for collection of bits

[Eugenio Gigante]

The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.
Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
this commit changes the type of 'flags' to unsigned.

Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 	int i, ret = 0;
 	char *skip_worktree_seen = NULL;
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
-	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+	unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
 		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
 
 	seen = xcalloc(pathspec->nr, 1);
-- 
2.43.0

Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits

[Christian Couder]

On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
<giganteeugenio2@gmail.com> wrote:
>
> The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
> while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.

It's not clear from the patch that refresh() passes 'flags' as an
argument to refresh_index(), so it might help reviewers a bit if you
could tell that.

> Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
> this commit changes the type of 'flags' to unsigned.

We prefer to use "let's change this and that" or just "change this and
that" rather than "this commit changes this and that", see
https://git-scm.com/docs/SubmittingPatches/#imperative-mood.

It might help if you could add a bit more explanation about why it's a
good thing to use an unsigned variable instead of a signed one. For
example you could say that it documents that we are not doing anything
funny with the MSB.

> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The patch looks correct, thanks!

Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
> <giganteeugenio2@gmail.com> wrote:
>>
>> The function 'refresh' in 'builtin/add.c' declares 'flags' as
>> signed, while the function 'refresh_index' defined in
>> 'read-cache-ll.h' expects an unsigned value.
>
> It's not clear from the patch that refresh() passes 'flags' as an
> argument to refresh_index(), so it might help reviewers a bit if you
> could tell that.

Perhaps.

>> Since in this case 'flags' represents a bag of bits, whose MSB is
>> not used in special ways, this commit changes the type of 'flags'
>> to unsigned.
>
> We prefer to use "let's change this and that" or just "change this and
> that" rather than "this commit changes this and that", see
> https://git-scm.com/docs/SubmittingPatches/#imperative-mood.

Very true.

> It might help if you could add a bit more explanation about why it's a
> good thing to use an unsigned variable instead of a signed one. For
> example you could say that it documents that we are not doing anything
> funny with the MSB.

But doesn't the proposed log message already say so?

In any case, it would very much help to fold long lines and have a
blank line in between paragraphs to make the log message more
readable.

Thanks, both.



>
>> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
>> ---
>>  builtin/add.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> The patch looks correct, thanks!

[GSoC][PATCH v2 0/1] Use unsigned integral type for collection of bits.

[Eugenio Gigante]

Improve the commit log message
based on reviewers feedback.

Eugenio Gigante (1):
  add: use unsigned type for collection of bits

 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
-- 
2.43.0

[PATCH v2 1/1] add: use unsigned type for collection of bits

[Eugenio Gigante]

The 'refresh' function  in 'builtin/add.c'
declares 'flags' as signed, and passes it
as an argument to the 'refresh_index' function,
which though expects an unsigned value.

Since in this case 'flags' represents
a bag of bits, whose MSB is not used
in special ways, change the type
of 'flags' to unsigned.

Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 	int i, ret = 0;
 	char *skip_worktree_seen = NULL;
 	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
-	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+	unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
 		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
 
 	seen = xcalloc(pathspec->nr, 1);
-- 
2.43.0

Re: [PATCH v2 1/1] add: use unsigned type for collection of bits

[Junio C Hamano]

Eugenio Gigante <giganteeugenio2@gmail.com> writes:

> The 'refresh' function  in 'builtin/add.c'
> declares 'flags' as signed, and passes it
> as an argument to the 'refresh_index' function,
> which though expects an unsigned value.

OK.  This has become much easier to read by dropping the mention of
the header file where the callee happens to be declared.

Just FYI, in my private rewrite (that I'll discard), I made it like
so:

   builtin/add.c:refresh() declares 'flags' as signed, and uses it
   to call refresh_index() that expects an unsigned value.

> Since in this case 'flags' represents
> a bag of bits, whose MSB is not used
> in special ways, change the type
> of 'flags' to unsigned.

Good, too.

Will queue.

> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ada7719561..393c10cbcf 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>  	int i, ret = 0;
>  	char *skip_worktree_seen = NULL;
>  	struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
> -	int flags = REFRESH_IGNORE_SKIP_WORKTREE |
> +	unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
>  		    (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
>  
>  	seen = xcalloc(pathspec->nr, 1);