From: Junio C Hamano <hidden> Date: 2021-08-11 18:00:05
Teng Long [off-list ref] writes:
Subject: Re: [PATCH v1 1/1] list-objects.c: traverse_trees_and_blobs: rename and tree-wide
Sorry but I do not quite get the " and tree-wide" part of the title.
Function `traverse_trees_and_blobs` not only works on trees and blobs,
but also on tags, the function name is somewhat misleading. This commit
rename it to `traverse_trees_and_blobs_and_tags`.
This looks pretty much "Meh" to me.
The current name might mislead people that we are limited to only
two types, but I doubt the risk of misleading is worth reducing with
such an overly long name. The original is long enough X-<.
When we introduced it at 91904f56 (list-objects.c: factor out
traverse_trees_and_blobs, 2017-11-02), we may have been better to
call it traverse_non_commits(). The idea of traverse_commit_list(),
which is its primary caller, is for its main loop to iterate over
commits and process them one by one in each iteration, and process
objects of other types discovered therein by calling this function
once per each commit [*1*].
s/trees_and_blobs/non_commits/ will result in a name that is much
shorter and to the point, I think.
[Footnote]
*1* The call to it at the end is meant to sweep anything
leftover---we will not discover any tag while in the main loop of
the caller that iterates over commits (as they cannot contain any
tag in there).
quoted hunk
Signed-off-by: Teng Long <redacted>
---
list-objects.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2021-08-11 19:11:56
On Wed, Aug 11, 2021 at 11:00:00AM -0700, Junio C Hamano wrote:
quoted
Function `traverse_trees_and_blobs` not only works on trees and blobs,
but also on tags, the function name is somewhat misleading. This commit
rename it to `traverse_trees_and_blobs_and_tags`.
This looks pretty much "Meh" to me.
The current name might mislead people that we are limited to only
two types, but I doubt the risk of misleading is worth reducing with
such an overly long name. The original is long enough X-<.
When we introduced it at 91904f56 (list-objects.c: factor out
traverse_trees_and_blobs, 2017-11-02), we may have been better to
call it traverse_non_commits(). The idea of traverse_commit_list(),
which is its primary caller, is for its main loop to iterate over
commits and process them one by one in each iteration, and process
objects of other types discovered therein by calling this function
once per each commit [*1*].
s/trees_and_blobs/non_commits/ will result in a name that is much
shorter and to the point, I think.
FWIW, I was about reply and suggest the exact same name. :)
As something internal to list-objects.c, I don't think it matters all
that much either way. The name "traverse_commit_list()" is IMHO more
likely to confuse. It is public within the project, and of course
traverses any type of object. So "traverse_objects()" or something may
be more accurate.
OTOH I do not find it all that confusing, and it may not be worth the
disruption to the code base.
-Peff
From: Teng Long <hidden> Date: 2021-08-12 07:54:34
quoted
Sorry but I do not quite get the " and tree-wide" part of the title.
I may have some misunderstandings about "tree-wide".
Sorry, it will be resolved in the next patch.
quoted
s/trees_and_blobs/non_commits/ will result in a name that is much
shorter and to the point, I think.
Agree and thanks for explanation.
Will follow your suggestion in next patch.
I don't know why I didn't think of this naming at the time (*headwalls*).
In fact, I discovered this problem while contributing packfile-uris
related features (thinking about excluding tag objects), and finally
said to myself "So you are here" (although it didn't take so much effort),
So I decide to make this patch.
quoted
The call to it at the end is meant to sweep anything
leftover---we will not discover any tag while in the main loop of
the caller that iterates over commits (as they cannot contain any
tag in there).
Some doubts.
I think at the object relationship level: tag> commit> tree> blob
The tag is on top because of the annotated tag. So, why the tag is
in `revs->pending` to deal? I understand it should be the opposite?
A commit should pending to a tag, at least equal.
Maybe my understanding is wrong, if so, why this way?
Thank you.
From: Teng Long <hidden> Date: 2021-08-12 08:05:54
quoted
FWIW, I was about reply and suggest the exact same name. :)
As something internal to list-objects.c, I don't think it matters all
that much either way. The name "traverse_commit_list()" is IMHO more
likely to confuse. It is public within the project, and of course
traverses any type of object. So "traverse_objects()" or something may
be more accurate.
On the basis of understanding the relationship between git objects,
`traverse_commit_list` is understandable, for me.
quoted
OTOH I do not find it all that confusing, and it may not be worth the
disruption to the code base.
Agree.
It’s interesting to read your reply, because of learning a lot of
abbreviations.
Thank you.