Re: [PATCH v5 02/14] Add new parameter "carry_data" for "show_object" function

Teng Long [off-list ref] writes:

Subject: Re: [PATCH v5 02/14] Add new parameter "carry_data" for "show_object" function

Since this lacks <area>: prefix, "git shortlog" readers will have a
hard time guessing which show_object() function this commit is
about.

During the pack-objects process, "show_object" function will be called
to find the object and show the process("show_object_fn" in
"list-object.h"), the function definition contains three parameters:

	1. struct object *obj(contains object type, flags, and oid).
	2. const char *name(the object name).
	3. void *show_data(function to show progress info).

This commit adds a new parameter: "void *carry_data", the reason is
mainly based on scalability and performance considerations when showing
an object, space for time, avoid costly temporary calculations in the
"show" phase. For example, carry the ownership relationship between
blob or tree object and the referred commit to avoid redundant and
expensive calculations.

The above explains what we want to carry around extra data for
(i.e. compute something in one place, and use it later somewhere
else)

But it does not quite explain why we need another parameter to do
so, which involves changing the function signature of many
functions, instead of making show_data to point at a new structure
type that holds the original data show_data used to carry plus
another single void * member (or the set of members you'd be
carrying into these functions using this new parameter).

I also find "carry_data" a meaningless name for the parameter.  All
in-parameters into functions are used to carry some data into it
after all.  The existing "show_data" at least makes a bit more
sense; it contains data necessary for showing the object in these
code paths.  If the purpose this new thing was introduced is to
cache ownership relationship data, perhaps ownership_cache would be
a more descriptive and understandable name (be it a new parameter to
added to many functions, or a member to the new structure that
replaces show_data).

Thanks.

Since this lacks <area>: prefix, "git shortlog" readers will have a
hard time guessing which show_object() function this commit is
about.

Sorry for the late reply.
Agree, it will be fixed in next patch.

But it does not quite explain why we need another parameter to do
so, which involves changing the function signature of many
functions, instead of making show_data to point at a new structure
type that holds the original data show_data used to carry plus
another single void * member (or the set of members you'd be
carrying into these functions using this new parameter).

I also find "carry_data" a meaningless name for the parameter.  All
in-parameters into functions are used to carry some data into it
after all.  The existing "show_data" at least makes a bit more
sense; it contains data necessary for showing the object in these
code paths.  If the purpose this new thing was introduced is to
cache ownership relationship data, perhaps ownership_cache would be
a more descriptive and understandable name (be it a new parameter to
added to many functions, or a member to the new structure that
replaces show_data).

Agree.

I think "show_data to point at a new structure" is a better idea. I will
follow the idea and optimize the related code. By the way, the naming
about "ownership_cache" will be introduced in the next patch too, unless I
can find a better one t(-_-t).

Thanks.