Re: [PATCH v11 12/13] ls-tree: introduce function "fast_path()"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 19, 2022, 05:32 UTC
- Message-ID
- <220219.86v8xbe7kb.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <6d264977491b3645292bba5f91479ab1623e8b80.1644319434.git.dyroneteng@gmail.com>
On Tue, Feb 08 2022, Teng Long wrote:
Show 14 quoted lines
> 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.
Show 25 quoted lines
> 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.
Show 20 quoted lines
> 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.