On Tue, Feb 08 2022, Teng Long wrote:
> The generic "show_tree_fmt()" is slower than "show_tree()", so
> we want to take the fast path if possible.
>
> when "--format=<format>" is passed, "fast_path()" will determine
> whether to use "show_tree()" or insist on using "show_tree_fmt()"
> by a try of finding out if the built-int format is hit.
>
> This commit take out the related codes from "cmd_ls_tree()" and
> package them into a new funtion "fast_path()".
>
> Explain it a little bit further, whether fast_path is hit or not,
> the final correctness should not break. Abstracting a separate method
> helps improve the readability of "cmd_ls_tree()" and the cohesiveness
> and extensibility of fast path logic.
This whole commit message sounds a bit like "we're introducing this fast
path", but really it got added in 11/13, and this is just a refactoring
to split that into a function to slightly reduce the size of
cmd_ls_tree() itself.
Which I really don't mind, but it would be better if the commit message
said so, e.g.:
In a preceding commit a fast path selection was added to cmd_ls_tree(),
split it into a utility function because ...
But I got stuck on "..." because I couldn't find a reason :)
I.e. in 13/13 this isn't used at all, except by adding a new brace arm
to it, but then it could still live in cmd_ls_tree().
Personally I think the pre-image is a bit easier to read, but then again
I wrote that so I'm biased. I don't mind changing this, but structurally
for the series it seems better to squash it in if you'd want to keep it.
> Signed-off-by: Teng Long <dyroneteng@xxxxxxxxx>
> ---
> builtin/ls-tree.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 1c71e5d543..ba96bcf602 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -58,6 +58,19 @@ enum {
>
> static int cmdmode = MODE_UNSPECIFIED;
>
> +static int fast_path(void){
> + if (!strcmp(format, default_format)) {
> + return 1;
> + } else if (!strcmp(format, long_format)) {
> + shown_fields = shown_fields | FIELD_SIZE;
> + return 1;
> + } else if (!strcmp(format, name_only_format)) {
> + shown_fields = FIELD_PATH_NAME;
> + return 1;
> + }
> + return 0;
> +}
Just in terms of arranging things if you add a static function and it's
only used in one other function, here in cmd_ls_tree(), it's more
readable to add it immediately before that function.
> static void expand_objectsize(struct strbuf *line, const struct object_id *oid,
> const enum object_type type, unsigned int padded)
> {
> @@ -350,15 +363,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
> * The generic show_tree_fmt() is slower than show_tree(), so
> * take the fast path if possible.
> */
> - if (format && (!strcmp(format, default_format))) {
> - fn = show_tree;
> - } else if (format && (!strcmp(format, long_format))) {
> - shown_fields = shown_fields | FIELD_SIZE;
> - fn = show_tree;
> - } else if (format && (!strcmp(format, name_only_format))) {
> - shown_fields = FIELD_PATH_NAME;
> - fn = show_tree;
> - } else if (format)
> + if (format && !fast_path())
> fn = show_tree_fmt;
>
> return !!read_tree(the_repository, tree, &pathspec, fn, NULL);
Also in terms of structure wouldn't it be better to end up with this:
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 9819a241869..47f7e2136b0 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -355,8 +355,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
if (get_oid(argv[0], &oid))
die("Not a valid object name %s", argv[0]);
- parse_shown_fields();
-
/*
* show_recursive() rolls its own matching code and is
* generally ignorant of 'struct pathspec'. The magic mask
@@ -373,6 +371,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
tree = parse_tree_indirect(&oid);
if (!tree)
die("not a tree object");
+
+ parse_shown_fields();
/*
* The generic show_tree_fmt() is slower than show_tree(), so
* take the fast path if possible.
I.e. have the whole "shown_fields" decisions be near one another.
