| Message ID | 6d264977491b3645292bba5f91479ab1623e8b80.1644319434.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-tree: "--object-only" and "--format" opts | expand |
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.
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; +} + 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);
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. Signed-off-by: Teng Long <dyroneteng@gmail.com> --- builtin/ls-tree.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)