diff mbox series

[v4,3/8] object-store: add function to free object_info contents

Message ID 20220502170904.2770649-4-calvinwan@google.com (mailing list archive)
State New, archived
Headers show
Series cat-file: add --batch-command remote-object-info command | expand

Commit Message

Calvin Wan May 2, 2022, 5:08 p.m. UTC 
Introduce free_object_info_contents.

---
 object-file.c  | 16 ++++++++++++++++
 object-store.h |  3 +++
 2 files changed, 19 insertions(+)

Comments

Junio C Hamano May 2, 2022, 11:23 p.m. UTC | #1 
Calvin Wan <calvinwan@google.com> writes:

> Introduce free_object_info_contents.
>
> ---
>  object-file.c  | 16 ++++++++++++++++
>  object-store.h |  3 +++
>  2 files changed, 19 insertions(+)
>
> diff --git a/object-file.c b/object-file.c
> index 5ffbf3d4fd..34a6e34adf 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2659,3 +2659,19 @@ int read_loose_object(const char *path,
>  		munmap(map, mapsize);
>  	return ret;
>  }
> +
> +void free_object_info_contents(struct object_info *object_info)
> +{
> +	if (!object_info)
> +		return;

This is OK, but ...


> +	if (object_info->typep)
> +		free(object_info->typep);
> +	if (object_info->sizep)
> +		free(object_info->sizep);
> +	if (object_info->disk_sizep)
> +		free(object_info->disk_sizep);
> +	if (object_info->delta_base_oid)
> +		free(object_info->delta_base_oid);
> +	if (object_info->type_name)
> +		free(object_info->type_name);

	if (PTR)
		free(PTR);

can and should be written as

	free(PTR);

If we are reusing object_info after calling this function, we
_might_ want to use FREE_AND_NULL() instead of free().

> +}
> \ No newline at end of file

Let's avoid such incomplete lines.

> diff --git a/object-store.h b/object-store.h
> index bd2322ed8c..840e04b56f 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -533,4 +533,7 @@ int for_each_object_in_pack(struct packed_git *p,
>  int for_each_packed_object(each_packed_object_fn, void *,
>  			   enum for_each_object_flags flags);
>  
> +/* Free pointers inside of object_info, but not object_info itself */
> +void free_object_info_contents(struct object_info *object_info);
> +
>  #endif /* OBJECT_STORE_H */
Junio C Hamano May 4, 2022, 7:09 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

>> +}
>> \ No newline at end of file
>
> Let's avoid such incomplete lines.

As this breaks my build cycle ("make sparse" is part of my
post-integration check), I have added this workaround on the tip of
the topic, but please make sure I do not have to redo that when you
reroll.

Thanks.

Subject: [PATCH] SQUASH??? compilation fix

"make sparse" rightfully complains that this file ends in an
incomplete line.
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-file.c b/object-file.c
index 34a6e34adf..c6addb6969 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2674,4 +2674,4 @@ void free_object_info_contents(struct object_info *object_info)
 		free(object_info->delta_base_oid);
 	if (object_info->type_name)
 		free(object_info->type_name);
-}
\ No newline at end of file
+}
Junio C Hamano May 5, 2022, 12:15 a.m. UTC | #3 
Junio C Hamano <gitster@pobox.com> writes:

>> +	if (object_info->typep)
>> +		free(object_info->typep);
>> +	if (object_info->sizep)
>> +		free(object_info->sizep);
>> +	if (object_info->disk_sizep)
>> +		free(object_info->disk_sizep);
>> +	if (object_info->delta_base_oid)
>> +		free(object_info->delta_base_oid);
>> +	if (object_info->type_name)
>> +		free(object_info->type_name);
>
> 	if (PTR)
> 		free(PTR);
>
> can and should be written as
>
> 	free(PTR);
>
> If we are reusing object_info after calling this function, we
> _might_ want to use FREE_AND_NULL() instead of free().

> As this breaks my build cycle ("make sparse" is part of my
> post-integration check), I have added this workaround on the tip of
> the topic, but please make sure I do not have to redo that when you
> reroll.
>
> Thanks.

Again, this breaks the build at GitHub CI (static-analysis), I added
this workaround on the tip of the topic, merged it to 'seen' and
pushed the result out, but please make sure I do not have to redo
that when you reroll.

Thanks.
Calvin Wan May 5, 2022, 4:47 p.m. UTC | #4 
Apologies for the mistakes. Getting a script from Josh Steadmon to
make sure these things never happen again :)

On Wed, May 4, 2022 at 5:15 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +    if (object_info->typep)
> >> +            free(object_info->typep);
> >> +    if (object_info->sizep)
> >> +            free(object_info->sizep);
> >> +    if (object_info->disk_sizep)
> >> +            free(object_info->disk_sizep);
> >> +    if (object_info->delta_base_oid)
> >> +            free(object_info->delta_base_oid);
> >> +    if (object_info->type_name)
> >> +            free(object_info->type_name);
> >
> >       if (PTR)
> >               free(PTR);
> >
> > can and should be written as
> >
> >       free(PTR);
> >
> > If we are reusing object_info after calling this function, we
> > _might_ want to use FREE_AND_NULL() instead of free().
>
> > As this breaks my build cycle ("make sparse" is part of my
> > post-integration check), I have added this workaround on the tip of
> > the topic, but please make sure I do not have to redo that when you
> > reroll.
> >
> > Thanks.
>
> Again, this breaks the build at GitHub CI (static-analysis), I added
> this workaround on the tip of the topic, merged it to 'seen' and
> pushed the result out, but please make sure I do not have to redo
> that when you reroll.
>
> Thanks.
Junio C Hamano May 5, 2022, 5:01 p.m. UTC | #5 
Calvin Wan <calvinwan@google.com> writes:

> Apologies for the mistakes. Getting a script from Josh Steadmon to
> make sure these things never happen again :)

Don't fret, mistakes happen.  The static-analysis job is quite picky
and catch these for us.

Sample runs are found here:

https://github.com/git/git/runs/6299058086?check_suite_focus=true
https://github.com/git/git/runs/6295167966?check_suite_focus=true
diff mbox series

Patch

diff --git a/object-file.c b/object-file.c
index 5ffbf3d4fd..34a6e34adf 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2659,3 +2659,19 @@  int read_loose_object(const char *path,
 		munmap(map, mapsize);
 	return ret;
 }
+
+void free_object_info_contents(struct object_info *object_info)
+{
+	if (!object_info)
+		return;
+	if (object_info->typep)
+		free(object_info->typep);
+	if (object_info->sizep)
+		free(object_info->sizep);
+	if (object_info->disk_sizep)
+		free(object_info->disk_sizep);
+	if (object_info->delta_base_oid)
+		free(object_info->delta_base_oid);
+	if (object_info->type_name)
+		free(object_info->type_name);
+}
\ No newline at end of file
diff --git a/object-store.h b/object-store.h
index bd2322ed8c..840e04b56f 100644
--- a/object-store.h
+++ b/object-store.h
@@ -533,4 +533,7 @@  int for_each_object_in_pack(struct packed_git *p,
 int for_each_packed_object(each_packed_object_fn, void *,
 			   enum for_each_object_flags flags);
 
+/* Free pointers inside of object_info, but not object_info itself */
+void free_object_info_contents(struct object_info *object_info);
+
 #endif /* OBJECT_STORE_H */