Re: [PATCH v11 12/13] ls-tree: introduce function "fast_path()"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com>
> ---
>  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.