From: Teng Long <hidden> Date: 2022-03-17 09:51:57
Ævar Arnfjörð Bjarmason wrote on Thu, 10 Mar 2022 14:56:56 +0100
I don't think all of these need to be squashed or fixed up into the
proposed series, but are just various small issues/questions I came
with while reviewing it. Brief notes
Ævar Arnfjörð Bjarmason (7):
...
I looked these commits in "RFC/REVIEW" and I think each one is a good
improvement. So next, to make the squashes or keep it alone I think
maybe it's like:
(1) ls-tree tests: add tests for --name-status
They are new test codes that better than old ones.
Action: Keep it individually.
(2) ls-tree tests: exhaustively test fast & slow path for --format
They are better test codes for the correctness formatting mechanism.
Action: Squash into commit 'ls-tree: introduce "--format" option'
(3) ls-tree: remove dead labels
They remove the dead labels, and you mentioned it should be squash into
the commit which brought them in.
Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
(4) ls-tree: remove unused "MODE_UNSPECIFIED"
As the subject describes, remove unused "MODE_UNSPECIFIED" and make
"mutx_option" to a better name "mutx_option". It's the prepared commit
for 'ls-tree: remove FIELD_*, just use MODE_*''
Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
(5) ls-tree: detect and error on --name-only --name-status
Optimized the incompatible detecting tests codes in "t/t3103-ls-tree-misc.sh"
and add a new 'MODE_NAME_STATUS'.
Action: Keep it individually.
(6) ls-tree: remove FIELD_*, just use MODE_*
Using MODE directlly and make a format-mode mappings for fast-path detection.
Action: Keep it individually.
(7) ls-tree: split up "fast path" callbacks
Expand "ls_tree_cmdmode_format" structure for each formats with the specific
show function and split up the current show functions name.
Action: Keep it individually.
This is the way I try to continue this work, please let me know if I understand you wrong.
I look forward to your reply and I will decide the next step based on the reply. If we have
a consistent understanding of the next actions, I will try to make a pull request to your
Git fork first. If there is no problem, I will continue to send patches to the mailing list.
Thanks.
Ævar Arnfjörð Bjarmason wrote on Thu, 10 Mar 2022 14:56:56 +0100
quoted
I don't think all of these need to be squashed or fixed up into the
proposed series, but are just various small issues/questions I came
with while reviewing it. Brief notes
Ævar Arnfjörð Bjarmason (7):
...
I looked these commits in "RFC/REVIEW" and I think each one is a good
improvement. So next, to make the squashes or keep it alone I think
maybe it's like:
(1) ls-tree tests: add tests for --name-status
They are new test codes that better than old ones.
Action: Keep it individually.
(2) ls-tree tests: exhaustively test fast & slow path for --format
They are better test codes for the correctness formatting mechanism.
Action: Squash into commit 'ls-tree: introduce "--format" option'
(3) ls-tree: remove dead labels
They remove the dead labels, and you mentioned it should be squash into
the commit which brought them in.
Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
(4) ls-tree: remove unused "MODE_UNSPECIFIED"
As the subject describes, remove unused "MODE_UNSPECIFIED" and make
"mutx_option" to a better name "mutx_option". It's the prepared commit
for 'ls-tree: remove FIELD_*, just use MODE_*''
Action: Squash into commit 'ls-tree: slightly refactor `show_tree()`'
(5) ls-tree: detect and error on --name-only --name-status
Optimized the incompatible detecting tests codes in "t/t3103-ls-tree-misc.sh"
and add a new 'MODE_NAME_STATUS'.
Action: Keep it individually.
(6) ls-tree: remove FIELD_*, just use MODE_*
Using MODE directlly and make a format-mode mappings for fast-path detection.
Action: Keep it individually.
(7) ls-tree: split up "fast path" callbacks
Expand "ls_tree_cmdmode_format" structure for each formats with the specific
show function and split up the current show functions name.
Action: Keep it individually.
All sounds good, or rather. I really meant those as "hey maybe it's
useful, you decide what to do with it". So I'm happy with whatever you
picked here :)
This is the way I try to continue this work, please let me know if I understand you wrong.
I look forward to your reply and I will decide the next step based on the reply. If we have
a consistent understanding of the next actions, I will try to make a pull request to your
Git fork first. If there is no problem, I will continue to send patches to the mailing list.
Hi, thanks for looking at it. I think it's better to just post a re-roll
on the list for discussion, with whatever changed you think are
appropriate. Thanks!
From: Teng Long <hidden> Date: 2022-03-21 07:33:48
From: Ævar Arnfjörð Bjarmason <redacted>
Remove code added in f35a6d3bce7 (Teach core object handling functions
about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output
flushing before fork(), 2008-08-04), and then finally ending up in its
current form in d3bee161fef (tree.c: allow read_tree_recursive() to
traverse gitlink entries, 2009-01-25). All while being commented-out!
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 9 ---------
1 file changed, 9 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:33:50
From: Ævar Arnfjörð Bjarmason <redacted>
Change the ls-tree.c code to use type_name() on the enum instead of
using the string constants. This doesn't matter either way for
performance, but makes this a bit easier to read as we'll no longer
need a strcmp() here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:33:56
From: Ævar Arnfjörð Bjarmason <redacted>
The "struct strbuf"'s "len" member is a "size_t", not an "int", so
let's change our corresponding types accordingly. This also changes
the "len" and "speclen" variables, which are likewise used to store
the return value of strlen(), which returns "size_t", not "int".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:33:58
The variable which "show_tree()" return is named "retval", a name that's
a little hard to understand. The commit rename "retval" to "recurse"
which is a more meaningful name than before in the context. We do not
need to take a look at "read_tree_at()" in "tree.c" to make sure what
does "retval" mean.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:00
Use the object_type() function to determine the object type from the
"mode" passed to us by read_tree(), instead of doing so with the S_*()
macros.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:04
If we execute "git ls-tree" with combined "--name-only" and "--long"
, only the pathname will be printed, the size is omitted (the original
discoverer was Peff in [1]).
This commit fix this issue by using `OPT_CMDMODE()` instead to make both
of them mutually exclusive.
[1] https://public-inbox.org/git/YZK0MKCYAJmG+pSU@coredump.intra.peff.net/
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 12 ++++++------
t/t3103-ls-tree-misc.sh | 9 +++++++++
2 files changed, 15 insertions(+), 6 deletions(-)
@@ -123,12 +123,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)LS_SHOW_TREES),OPT_SET_INT('z',NULL,&line_termination,N_("terminate entries with NUL byte"),0),-OPT_BIT('l',"long",&ls_options,N_("include object size"),-LS_SHOW_SIZE),-OPT_BIT(0,"name-only",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),-OPT_BIT(0,"name-status",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),+OPT_CMDMODE('l',"long",&ls_options,N_("include object size"),+LS_SHOW_SIZE),+OPT_CMDMODE(0,"name-only",&ls_options,N_("list only filenames"),+LS_NAME_ONLY),+OPT_CMDMODE(0,"name-status",&ls_options,N_("list only filenames"),+LS_NAME_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
From: Teng Long <hidden> Date: 2022-03-21 07:34:15
This is a non-functional change, we use a new int "shown_fields" to mark
which columns to output, and `parse_shown_fields()` to calculate the
value of "shown_fields".
This has the advantage of making the show_tree logic simpler and more
readable, as well as making it easier to extend new options (for example,
if we want to add a "--object-only" option, we just need to add a similar
"if (shown_fields == FIELD_OBJECT_NAME)" short-circuit logic in
"show_tree()").
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 121 ++++++++++++++++++++++++++++++++--------------
1 file changed, 84 insertions(+), 37 deletions(-)
@@ -123,12 +168,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)LS_SHOW_TREES),OPT_SET_INT('z',NULL,&line_termination,N_("terminate entries with NUL byte"),0),-OPT_CMDMODE('l',"long",&ls_options,N_("include object size"),-LS_SHOW_SIZE),-OPT_CMDMODE(0,"name-only",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),-OPT_CMDMODE(0,"name-status",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),+OPT_CMDMODE('l',"long",&cmdmode,N_("include object size"),+MODE_LONG),+OPT_CMDMODE(0,"name-only",&cmdmode,N_("list only filenames"),+MODE_NAME_ONLY),+OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),+MODE_NAME_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
@@ -159,6 +204,8 @@ 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()rollsitsownmatchingcodeandis*generallyignorantof'structpathspec'.Themagicmask
From: Teng Long <hidden> Date: 2022-03-21 07:34:16
From: Ævar Arnfjörð Bjarmason <redacted>
"show_tree_data" is a struct that packages the necessary fields for
"show_tree()". This commit is a pre-prepared commit for supporting
"--format" option and it does not affect any existing functionality.
At the same time, we also move global "shown_fields" into "show_tree_data"
, the purpose of this is to avoid use both global variable and pass-around
struct.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 69 +++++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 26 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:19
From: Johannes Schindelin <redacted>
A convenient way to pad strings is to use something like
`strbuf_addf(&buf, "%20s", "Hello, world!")`.
However, the Coccinelle rule that forbids a format `"%s"` with a
constant string argument cast too wide a net, and also forbade such
padding.
The original rule was introduced by commit:
28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Teng Long <redacted>
---
contrib/coccinelle/strbuf.cocci | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:21
From: Ævar Arnfjörð Bjarmason <redacted>
Add a --format option to ls-tree. It has an existing default output,
and then --long and --name-only options to emit the default output
along with the objectsize and, or to only emit object paths.
Rather than add --type-only, --object-only etc. we can just support a
--format using a strbuf_expand() similar to "for-each-ref
--format". We might still add such options in the future for
convenience.
The --format implementation is slower than the existing code, but this
change does not cause any performance regressions. We'll leave the
existing show_tree() unchanged, and only run show_tree_fmt() in if
a --format different than the hardcoded built-in ones corresponding to
the existing modes is provided.
I.e. something like the "--long" output would be much slower with
this, mainly due to how we need to allocate various things to do with
quote.c instead of spewing the output directly to stdout.
The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's
idea and suggestion, this commit makes modifications in terms of the
original discussion on community [1].
In [1] there was a "GIT_TEST_LS_TREE_FORMAT_BACKEND" variable to
ensure that we had test coverage for passing tests that would
otherwise use show_tree() through show_tree_fmt(), and thus that the
formatting mechanism could handle all the same cases as the
non-formatting options.
Somewhere in subsequent re-rolls of that we seem to have drifted away
from what the goal of these tests should be. We're trying to ensure
correctness of show_tree_fmt(). We can't tell if we "hit [the]
fast-path" here, and instead of having an explicit test for that, we
can just add it to something our "test_ls_tree_format" tests for.
Here is the statistics about performance tests:
1. Default format (hitten the builtin formats):
"git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
Time (mean ± σ): 105.2 ms ± 3.3 ms [User: 84.3 ms, System: 20.8 ms]
Range (min … max): 99.2 ms … 113.2 ms 28 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)' HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)' HEAD
Time (mean ± σ): 106.4 ms ± 2.7 ms [User: 86.1 ms, System: 20.2 ms]
Range (min … max): 100.2 ms … 110.5 ms 29 runs
2. Default format includes object size (hitten the builtin formats):
"git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
Time (mean ± σ): 335.1 ms ± 6.5 ms [User: 304.6 ms, System: 30.4 ms]
Range (min … max): 327.5 ms … 348.4 ms 10 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD
Time (mean ± σ): 337.2 ms ± 8.2 ms [User: 309.2 ms, System: 27.9 ms]
Range (min … max): 328.8 ms … 349.4 ms 10 runs
Links:
[1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
[2] https://lore.kernel.org/git/cb717d08be87e3239117c6c667cb32caabaad33d.1646390152.git.dyroneteng@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 59 ++++++++++++++--
builtin/ls-tree.c | 129 +++++++++++++++++++++++++++++++++-
t/t3104-ls-tree-format.sh | 67 ++++++++++++++++++
3 files changed, 248 insertions(+), 7 deletions(-)
create mode 100755 t/t3104-ls-tree-format.sh
@@ -74,6 +74,16 @@ OPTIONS Do not limit the listing to the current working directory. Implies --full-name.+--format=<format>::+ A string that interpolates `%(fieldname)` from the result+ being shown. It also interpolates `%%` to `%`, and+ `%xx` where `xx` are hex digits interpolates to character+ with hex code `xx`; for example `%00` interpolates to+ `\0` (NUL), `%09` to `\t` (TAB) and `%0a` to `\n` (LF).+ When specified, `--format` cannot be combined with other+ format-altering options, including `--long`, `--name-only`+ and `--object-only`.+ [<path>...]:: When paths are given, show them (note that this isn't really raw pathnames, but rather a list of patterns to match). Otherwise
@@ -82,16 +92,29 @@ OPTIONS Output Format -------------- <mode> SP <type> SP <object> TAB <file>++The output format of `ls-tree` is determined by either the `--format`+option, or other format-altering options such as `--name-only` etc.+(see `--format` above).++The use of certain `--format` directives is equivalent to using those+options, but invoking the full formatting machinery can be slower than+using an appropriate formatting option.++In cases where the `--format` would exactly map to an existing option+`ls-tree` will use the appropriate faster path. Thus the default format+is equivalent to:++ %(objectmode) %(objecttype) %(objectname)%x09%(path) This output format is compatible with what `--index-info --stdin` of 'git update-index' expects. When the `-l` option is used, format changes to- <mode> SP <type> SP <object> SP <object size> TAB <file>+ %(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)-Object size identified by <object> is given in bytes, and right-justified+Object size identified by <objectname> is given in bytes, and right-justified with minimum width of 7 characters. Object size is given only for blobs (file) entries; for other entries `-` character is used in place of size.
@@ -100,6 +123,34 @@ quoted as explained for the configuration variable `core.quotePath` (see linkgit:git-config[1]). Using `-z` the filename is output verbatim and the line is terminated by a NUL byte.+Customized format:++It is possible to print in a custom format by using the `--format` option,+which is able to interpolate different fields using a `%(fieldname)` notation.+For example, if you only care about the "objectname" and "path" fields, you+can execute with a specific "--format" like++ git ls-tree --format='%(objectname) %(path)' <tree-ish>++FIELD NAMES+-----------++Various values from structured fields can be used to interpolate+into the resulting output. For each outputing line, the following+names can be used:++objectmode::+ The mode of the object.+objecttype::+ The type of the object (`blob` or `tree`).+objectname::+ The name of the object.+objectsize[:padded]::+ The size of the object ("-" if it's a tree).+ It also supports a padded format of size with "%(size:padded)".+path::+ The pathname of the object.+ GIT --- Part of the linkgit:git[1] suite
@@ -30,7 +30,10 @@ static const char *ls_tree_prefix;#define FIELD_MODE (1 << 4)#define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */#define FIELD_LONG_DEFAULT (FIELD_DEFAULT | FIELD_SIZE)-+staticconstchar*format;+staticconstchar*default_format="%(objectmode) %(objecttype) %(objectname)%x09%(path)";+staticconstchar*long_format="%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";+staticconstchar*name_only_format="%(path)";structshow_tree_data{unsignedmode;enumobject_typetype;
@@ -50,6 +53,72 @@ static enum ls_tree_cmdmode {MODE_NAME_ONLY,}cmdmode;+staticvoidexpand_objectsize(structstrbuf*line,conststructobject_id*oid,+constenumobject_typetype,unsignedintpadded)+{+if(type==OBJ_BLOB){+unsignedlongsize;+if(oid_object_info(the_repository,oid,&size)<0)+die(_("could not get object info about '%s'"),+oid_to_hex(oid));+if(padded)+strbuf_addf(line,"%7"PRIuMAX,(uintmax_t)size);+else+strbuf_addf(line,"%"PRIuMAX,(uintmax_t)size);+}elseif(padded){+strbuf_addf(line,"%7s","-");+}else{+strbuf_addstr(line,"-");+}+}++staticsize_texpand_show_tree(structstrbuf*sb,constchar*start,+void*context)+{+structshow_tree_data*data=context;+constchar*end;+constchar*p;+unsignedinterrlen;+size_tlen=strbuf_expand_literal_cb(sb,start,NULL);++if(len)+returnlen;+if(*start!='(')+die(_("bad ls-tree format: element '%s' does not start with '('"),start);++end=strchr(start+1,')');+if(!end)+die(_("bad ls-tree format: element '%s' does not end in ')'"),start);++len=end-start+1;+if(skip_prefix(start,"(objectmode)",&p)){+strbuf_addf(sb,"%06o",data->mode);+}elseif(skip_prefix(start,"(objecttype)",&p)){+strbuf_addstr(sb,type_name(data->type));+}elseif(skip_prefix(start,"(objectsize:padded)",&p)){+expand_objectsize(sb,data->oid,data->type,1);+}elseif(skip_prefix(start,"(objectsize)",&p)){+expand_objectsize(sb,data->oid,data->type,0);+}elseif(skip_prefix(start,"(objectname)",&p)){+strbuf_add_unique_abbrev(sb,data->oid,abbrev);+}elseif(skip_prefix(start,"(path)",&p)){+constchar*name=data->base->buf;+constchar*prefix=chomp_prefix?ls_tree_prefix:NULL;+structstrbufquoted=STRBUF_INIT;+structstrbufsbuf=STRBUF_INIT;+strbuf_addstr(data->base,data->pathname);+name=relative_path(data->base->buf,prefix,&sbuf);+quote_c_style(name,"ed,NULL,0);+strbuf_addbuf(sb,"ed);+strbuf_release(&sbuf);+strbuf_release("ed);+}else{+errlen=(unsignedlong)len;+die(_("bad ls-tree format: %%%.*s"),errlen,start);+}+returnlen;+}+staticintparse_shown_fields(unsignedint*shown_fields){if(cmdmode==MODE_NAME_ONLY){
@@ -175,6 +276,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)structtree*tree;inti,full_tree=0;unsignedintshown_fields=0;+read_tree_fn_tfn=show_tree;conststructoptionls_tree_options[]={OPT_BIT('d',NULL,&ls_options,N_("only show trees"),LS_TREE_ONLY),
@@ -195,6 +297,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"full-tree",&full_tree,N_("list entire tree; not just current directory ""(implies --full-name)")),+OPT_STRING_F(0,"format",&format,N_("format"),+N_("format to use for the output"),+PARSE_OPT_NONEG),OPT__ABBREV(&abbrev),OPT_END()};
@@ -215,6 +320,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)((LS_TREE_ONLY|LS_RECURSIVE)&ls_options))ls_options|=LS_SHOW_TREES;+if(format&&cmdmode)+usage_msg_opt(+_("--format can't be combined with other format-altering options"),+ls_tree_usage,ls_tree_options);if(argc<1)usage_with_options(ls_tree_usage,ls_tree_options);if(get_oid(argv[0],&oid))
From: Teng Long <hidden> Date: 2022-03-21 07:34:23
'--object-only' is an alias for '--format=%(objectname)'. It cannot
be used together other format-altering options like '--name-only',
'--long' or '--format', they are mutually exclusive.
The "--name-only" option outputs <filepath> only. Likewise, <objectName>
is another high frequency used field, so implement '--object-only' option
will bring intuitive and clear semantics for this scenario. Using
'--format=%(objectname)' we can achieve a similar effect, but the former
is with a lower learning cost(without knowing the format requirement
of '--format' option).
Even so, if a user is prefer to use "--format=%(objectname)", this is entirely
welcome because they are not only equivalent in function, but also have almost
identical performance. The reason is this commit also add the specific of
"--format=%(objectname)" to the current fast-pathes (builtin formats) to
avoid running unnecessary parsing mechanisms.
The following performance benchmarks are based on torvalds/linux.git:
When hit the fast-path:
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --object-only HEAD
Time (mean ± σ): 83.6 ms ± 2.0 ms [User: 59.4 ms, System: 24.1 ms]
Range (min … max): 80.4 ms … 87.2 ms 35 runs
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(objectname)' HEAD
Time (mean ± σ): 84.1 ms ± 1.8 ms [User: 61.7 ms, System: 22.3 ms]
Range (min … max): 80.9 ms … 87.5 ms 35 runs
But for a customized format, it will be slower:
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='oid: %(objectname)' HEAD
Time (mean ± σ): 96.5 ms ± 2.5 ms [User: 72.9 ms, System: 23.5 ms]
Range (min … max): 93.1 ms … 104.1 ms 31 runs
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 11 ++++++++++-
builtin/ls-tree.c | 17 ++++++++++++++++-
t/t3103-ls-tree-misc.sh | 7 +++++--
t/t3104-ls-tree-format.sh | 9 +++++++++
4 files changed, 40 insertions(+), 4 deletions(-)
@@ -59,6 +59,15 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line.+ Cannot be combined with `--object-only`.++--object-only::+ List only names of the objects, one per line. Cannot be combined+ with `--name-only` or `--name-status`.+ This is equivalent to specifying `--format='%(objectname)'`, but+ for both this option and that exact format the command takes a+ hand-optimized codepath instead of going through the generic+ formatting mechanism. --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object
@@ -125,7 +127,10 @@ static int parse_shown_fields(unsigned int *shown_fields)*shown_fields=FIELD_PATH_NAME;return0;}-+if(cmdmode==MODE_OBJECT_ONLY){+*shown_fields=FIELD_OBJECT_NAME;+return0;+}if(!ls_options||(ls_options&LS_RECURSIVE)||(ls_options&LS_SHOW_TREES)||(ls_options&LS_TREE_ONLY))
@@ -292,6 +302,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)MODE_NAME_ONLY),OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),MODE_NAME_ONLY),+OPT_CMDMODE(0,"object-only",&cmdmode,N_("list only objects"),+MODE_OBJECT_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
From: Teng Long <hidden> Date: 2022-03-21 07:34:24
From: Ævar Arnfjörð Bjarmason <redacted>
The --name-status synonym for --name-only added in
c639a5548a5 (ls-tree: --name-only, 2005-12-01) had no tests, let's
make sure it works the same way as its sibling.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
t/t3101-ls-tree-dirname.sh | 55 ++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 26 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:28
From: Ævar Arnfjörð Bjarmason <redacted>
The --name-only and --name-status options are synonyms, but let's
detect and error if both are provided.
In addition let's add explicit --format tests for the combination of
these various options.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 11 ++++++++++-
t/t3103-ls-tree-misc.sh | 15 +++++++++------
2 files changed, 19 insertions(+), 7 deletions(-)
@@ -301,7 +302,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)OPT_CMDMODE(0,"name-only",&cmdmode,N_("list only filenames"),MODE_NAME_ONLY),OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),-MODE_NAME_ONLY),+MODE_NAME_STATUS),OPT_CMDMODE(0,"object-only",&cmdmode,N_("list only objects"),MODE_OBJECT_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,
@@ -327,6 +328,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)ls_tree_prefix=prefix=NULL;chomp_prefix=0;}+/*+*Wewantedtodetectconflictsbetween--name-onlyand+*--name-status,butoncewe'redonewiththatsubsequent+*codeshouldonlyneedtochecktheprimaryname.+*/+if(cmdmode==MODE_NAME_STATUS)+cmdmode=MODE_NAME_ONLY;+/* -d -r should imply -t, but -d by itself should not have to. */if((LS_TREE_ONLY|LS_RECURSIVE)==((LS_TREE_ONLY|LS_RECURSIVE)&ls_options))
From: Teng Long <hidden> Date: 2022-03-21 07:34:29
From: Ævar Arnfjörð Bjarmason <redacted>
When we're picking where we should go in the optimized "show_tree"
path there's no reason for why we need to convert our "cmdmode" of
e.g. MODE_LONG into a FIELD_LONG_DEFAULT. Instead we can simply do
those checks in the show_tree() function itself.
Let's also make this code more future-proof by unrolling the hardcoded
strmp() if/else if chain into something that checks a new "static
struct" providing a bidirectional mapping between optimized formats
and the ls_tree_cmdmode.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 98 ++++++++++++++++++++++-------------------------
1 file changed, 46 insertions(+), 52 deletions(-)
From: Teng Long <hidden> Date: 2022-03-21 07:34:47
From: Ævar Arnfjörð Bjarmason <redacted>
Make the various if/else in the callbacks for the "fast path" a lot
easier to read by just using common functions for the parts that are
common, and have per-format callbacks for those parts that are
different.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 198 +++++++++++++++++++++++++++++-----------------
1 file changed, 124 insertions(+), 74 deletions(-)
From: Ævar Arnfjörð Bjarmason <redacted>
When we're picking where we should go in the optimized "show_tree"
path there's no reason for why we need to convert our "cmdmode" of
e.g. MODE_LONG into a FIELD_LONG_DEFAULT. Instead we can simply do
those checks in the show_tree() function itself.
Let's also make this code more future-proof by unrolling the hardcoded
strmp() if/else if chain into something that checks a new "static
struct" providing a bidirectional mapping between optimized formats
and the ls_tree_cmdmode.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
For this & any other changes I suggested which amend code introduced
earlier in the series: Let's just fix that up into the respective
commits, if you agree this is a good direction that is.
I.e. if it's good to do Y let's not first do X and then change it to Y,
we can just start with Y.
Maybe that means none of these fix-up commitsa are left at the end, and
that's OK.
From: Ævar Arnfjörð Bjarmason <redacted>
Make the various if/else in the callbacks for the "fast path" a lot
easier to read by just using common functions for the parts that are
common, and have per-format callbacks for those parts that are
different.
FWIW I didn't do any exhaustive benchmarks of this, but I checked a few
things against origin/master on linux.git and all the reported
"hyperfine" timings were the same/within the +/- interval.
From: Ævar Arnfjörð Bjarmason <redacted>
The --name-status synonym for --name-only added in
c639a5548a5 (ls-tree: --name-only, 2005-12-01) had no tests, let's
make sure it works the same way as its sibling.
Let's move this to the front of the series, or rather have any test-only
changes go first. Then it's clear with:
git rebase -i -x 'make test' @{u}
What does and doesn't introduce behavior changes, i.e. this is just
testing existing functionality.
From: Teng Long <hidden> Date: 2022-03-23 09:13:37
From: Ævar Arnfjörð Bjarmason <redacted>
The --name-status synonym for --name-only added in
c639a5548a5 (ls-tree: --name-only, 2005-12-01) had no tests, let's
make sure it works the same way as its sibling.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
t/t3101-ls-tree-dirname.sh | 55 ++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 26 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:13:39
From: Ævar Arnfjörð Bjarmason <redacted>
Remove code added in f35a6d3bce7 (Teach core object handling functions
about gitlinks, 2007-04-09), later patched in 7d0b18a4da1 (Add output
flushing before fork(), 2008-08-04), and then finally ending up in its
current form in d3bee161fef (tree.c: allow read_tree_recursive() to
traverse gitlink entries, 2009-01-25). All while being commented-out!
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 9 ---------
1 file changed, 9 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:13:42
From: Ævar Arnfjörð Bjarmason <redacted>
Change the ls-tree.c code to use type_name() on the enum instead of
using the string constants. This doesn't matter either way for
performance, but makes this a bit easier to read as we'll no longer
need a strcmp() here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:13:44
From: Ævar Arnfjörð Bjarmason <redacted>
The "struct strbuf"'s "len" member is a "size_t", not an "int", so
let's change our corresponding types accordingly. This also changes
the "len" and "speclen" variables, which are likewise used to store
the return value of strlen(), which returns "size_t", not "int".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:13:46
The variable which "show_tree()" return is named "retval", a name that's
a little hard to understand. The commit rename "retval" to "recurse"
which is a more meaningful name than before in the context. We do not
need to take a look at "read_tree_at()" in "tree.c" to make sure what
does "retval" mean.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:13:58
Use the object_type() function to determine the object type from the
"mode" passed to us by read_tree(), instead of doing so with the S_*()
macros.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:14:00
If we execute "git ls-tree" with combined "--name-only" and "--long"
, only the pathname will be printed, the size is omitted (the original
discoverer was Peff in [1]).
This commit fix this issue by using `OPT_CMDMODE()` instead to make both
of them mutually exclusive.
[1] https://public-inbox.org/git/YZK0MKCYAJmG+pSU@coredump.intra.peff.net/
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 12 ++++++------
t/t3103-ls-tree-misc.sh | 9 +++++++++
2 files changed, 15 insertions(+), 6 deletions(-)
@@ -123,12 +123,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)LS_SHOW_TREES),OPT_SET_INT('z',NULL,&line_termination,N_("terminate entries with NUL byte"),0),-OPT_BIT('l',"long",&ls_options,N_("include object size"),-LS_SHOW_SIZE),-OPT_BIT(0,"name-only",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),-OPT_BIT(0,"name-status",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),+OPT_CMDMODE('l',"long",&ls_options,N_("include object size"),+LS_SHOW_SIZE),+OPT_CMDMODE(0,"name-only",&ls_options,N_("list only filenames"),+LS_NAME_ONLY),+OPT_CMDMODE(0,"name-status",&ls_options,N_("list only filenames"),+LS_NAME_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
From: Teng Long <hidden> Date: 2022-03-23 09:14:02
This is a non-functional change, we introduce an enum "ls_tree_cmdmode"
then use it to mark which columns to output.
This has the advantage of making the show_tree logic simpler and more
readable, as well as making it easier to extend new options (for example,
if we want to add a "--object-only" option, we just need to add a similar
"short-circuit logic in "show_tree()").
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 98 +++++++++++++++++++++++++++++------------------
1 file changed, 61 insertions(+), 37 deletions(-)
@@ -123,12 +147,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)LS_SHOW_TREES),OPT_SET_INT('z',NULL,&line_termination,N_("terminate entries with NUL byte"),0),-OPT_CMDMODE('l',"long",&ls_options,N_("include object size"),-LS_SHOW_SIZE),-OPT_CMDMODE(0,"name-only",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),-OPT_CMDMODE(0,"name-status",&ls_options,N_("list only filenames"),-LS_NAME_ONLY),+OPT_CMDMODE('l',"long",&cmdmode,N_("include object size"),+MODE_LONG),+OPT_CMDMODE(0,"name-only",&cmdmode,N_("list only filenames"),+MODE_NAME_ONLY),+OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),+MODE_NAME_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
From: Teng Long <hidden> Date: 2022-03-23 09:14:02
From: Ævar Arnfjörð Bjarmason <redacted>
"show_tree_data" is a struct that packages the necessary fields for
"show_tree()". This commit is a pre-prepared commit for supporting
"--format" option and it does not affect any existing functionality.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:14:04
From: Johannes Schindelin <redacted>
A convenient way to pad strings is to use something like
`strbuf_addf(&buf, "%20s", "Hello, world!")`.
However, the Coccinelle rule that forbids a format `"%s"` with a
constant string argument cast too wide a net, and also forbade such
padding.
The original rule was introduced by commit:
28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Teng Long <redacted>
---
contrib/coccinelle/strbuf.cocci | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:14:06
From: Ævar Arnfjörð Bjarmason <redacted>
Add a --format option to ls-tree. It has an existing default output,
and then --long and --name-only options to emit the default output
along with the objectsize and, or to only emit object paths.
Rather than add --type-only, --object-only etc. we can just support a
--format using a strbuf_expand() similar to "for-each-ref
--format". We might still add such options in the future for
convenience.
The --format implementation is slower than the existing code, but this
change does not cause any performance regressions. We'll leave the
existing show_tree() unchanged, and only run show_tree_fmt() in if
a --format different than the hardcoded built-in ones corresponding to
the existing modes is provided.
I.e. something like the "--long" output would be much slower with
this, mainly due to how we need to allocate various things to do with
quote.c instead of spewing the output directly to stdout.
The new option of '--format' comes from Ævar Arnfjörð Bjarmasonn's
idea and suggestion, this commit makes modifications in terms of the
original discussion on community [1].
In [1] there was a "GIT_TEST_LS_TREE_FORMAT_BACKEND" variable to
ensure that we had test coverage for passing tests that would
otherwise use show_tree() through show_tree_fmt(), and thus that the
formatting mechanism could handle all the same cases as the
non-formatting options.
Somewhere in subsequent re-rolls of that we seem to have drifted away
from what the goal of these tests should be. We're trying to ensure
correctness of show_tree_fmt(). We can't tell if we "hit [the]
fast-path" here, and instead of having an explicit test for that, we
can just add it to something our "test_ls_tree_format" tests for.
Here is the statistics about performance tests:
1. Default format (hitten the builtin formats):
"git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
Time (mean ± σ): 105.2 ms ± 3.3 ms [User: 84.3 ms, System: 20.8 ms]
Range (min … max): 99.2 ms … 113.2 ms 28 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)' HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object)%x09%(file)' HEAD
Time (mean ± σ): 106.4 ms ± 2.7 ms [User: 86.1 ms, System: 20.2 ms]
Range (min … max): 100.2 ms … 110.5 ms 29 runs
2. Default format includes object size (hitten the builtin formats):
"git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r -l HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r -l HEAD
Time (mean ± σ): 335.1 ms ± 6.5 ms [User: 304.6 ms, System: 30.4 ms]
Range (min … max): 327.5 ms … 348.4 ms 10 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD
Time (mean ± σ): 337.2 ms ± 8.2 ms [User: 309.2 ms, System: 27.9 ms]
Range (min … max): 328.8 ms … 349.4 ms 10 runs
Links:
[1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
[2] https://lore.kernel.org/git/cb717d08be87e3239117c6c667cb32caabaad33d.1646390152.git.dyroneteng@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 59 ++++++++++++-
builtin/ls-tree.c | 153 +++++++++++++++++++++++++++++++++-
t/t3104-ls-tree-format.sh | 67 +++++++++++++++
3 files changed, 272 insertions(+), 7 deletions(-)
create mode 100755 t/t3104-ls-tree-format.sh
@@ -74,6 +74,16 @@ OPTIONS Do not limit the listing to the current working directory. Implies --full-name.+--format=<format>::+ A string that interpolates `%(fieldname)` from the result+ being shown. It also interpolates `%%` to `%`, and+ `%xx` where `xx` are hex digits interpolates to character+ with hex code `xx`; for example `%00` interpolates to+ `\0` (NUL), `%09` to `\t` (TAB) and `%0a` to `\n` (LF).+ When specified, `--format` cannot be combined with other+ format-altering options, including `--long`, `--name-only`+ and `--object-only`.+ [<path>...]:: When paths are given, show them (note that this isn't really raw pathnames, but rather a list of patterns to match). Otherwise
@@ -82,16 +92,29 @@ OPTIONS Output Format -------------- <mode> SP <type> SP <object> TAB <file>++The output format of `ls-tree` is determined by either the `--format`+option, or other format-altering options such as `--name-only` etc.+(see `--format` above).++The use of certain `--format` directives is equivalent to using those+options, but invoking the full formatting machinery can be slower than+using an appropriate formatting option.++In cases where the `--format` would exactly map to an existing option+`ls-tree` will use the appropriate faster path. Thus the default format+is equivalent to:++ %(objectmode) %(objecttype) %(objectname)%x09%(path) This output format is compatible with what `--index-info --stdin` of 'git update-index' expects. When the `-l` option is used, format changes to- <mode> SP <type> SP <object> SP <object size> TAB <file>+ %(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)-Object size identified by <object> is given in bytes, and right-justified+Object size identified by <objectname> is given in bytes, and right-justified with minimum width of 7 characters. Object size is given only for blobs (file) entries; for other entries `-` character is used in place of size.
@@ -100,6 +123,34 @@ quoted as explained for the configuration variable `core.quotePath` (see linkgit:git-config[1]). Using `-z` the filename is output verbatim and the line is terminated by a NUL byte.+Customized format:++It is possible to print in a custom format by using the `--format` option,+which is able to interpolate different fields using a `%(fieldname)` notation.+For example, if you only care about the "objectname" and "path" fields, you+can execute with a specific "--format" like++ git ls-tree --format='%(objectname) %(path)' <tree-ish>++FIELD NAMES+-----------++Various values from structured fields can be used to interpolate+into the resulting output. For each outputing line, the following+names can be used:++objectmode::+ The mode of the object.+objecttype::+ The type of the object (`blob` or `tree`).+objectname::+ The name of the object.+objectsize[:padded]::+ The size of the object ("-" if it's a tree).+ It also supports a padded format of size with "%(size:padded)".+path::+ The pathname of the object.+ GIT --- Part of the linkgit:git[1] suite
@@ -23,6 +23,8 @@ static int ls_options;staticstructpathspecpathspec;staticintchomp_prefix;staticconstchar*ls_tree_prefix;+staticconstchar*format;+structshow_tree_data{unsignedmode;enumobject_typetype;
@@ -37,10 +39,77 @@ static const char * const ls_tree_usage[] = {};staticenumls_tree_cmdmode{-MODE_LONG=1,+MODE_DEFAULT=0,+MODE_LONG,MODE_NAME_ONLY,}cmdmode;+staticvoidexpand_objectsize(structstrbuf*line,conststructobject_id*oid,+constenumobject_typetype,unsignedintpadded)+{+if(type==OBJ_BLOB){+unsignedlongsize;+if(oid_object_info(the_repository,oid,&size)<0)+die(_("could not get object info about '%s'"),+oid_to_hex(oid));+if(padded)+strbuf_addf(line,"%7"PRIuMAX,(uintmax_t)size);+else+strbuf_addf(line,"%"PRIuMAX,(uintmax_t)size);+}elseif(padded){+strbuf_addf(line,"%7s","-");+}else{+strbuf_addstr(line,"-");+}+}++staticsize_texpand_show_tree(structstrbuf*sb,constchar*start,+void*context)+{+structshow_tree_data*data=context;+constchar*end;+constchar*p;+unsignedinterrlen;+size_tlen=strbuf_expand_literal_cb(sb,start,NULL);++if(len)+returnlen;+if(*start!='(')+die(_("bad ls-tree format: element '%s' does not start with '('"),start);++end=strchr(start+1,')');+if(!end)+die(_("bad ls-tree format: element '%s' does not end in ')'"),start);++len=end-start+1;+if(skip_prefix(start,"(objectmode)",&p)){+strbuf_addf(sb,"%06o",data->mode);+}elseif(skip_prefix(start,"(objecttype)",&p)){+strbuf_addstr(sb,type_name(data->type));+}elseif(skip_prefix(start,"(objectsize:padded)",&p)){+expand_objectsize(sb,data->oid,data->type,1);+}elseif(skip_prefix(start,"(objectsize)",&p)){+expand_objectsize(sb,data->oid,data->type,0);+}elseif(skip_prefix(start,"(objectname)",&p)){+strbuf_add_unique_abbrev(sb,data->oid,abbrev);+}elseif(skip_prefix(start,"(path)",&p)){+constchar*name=data->base->buf;+constchar*prefix=chomp_prefix?ls_tree_prefix:NULL;+structstrbufquoted=STRBUF_INIT;+structstrbufsbuf=STRBUF_INIT;+strbuf_addstr(data->base,data->pathname);+name=relative_path(data->base->buf,prefix,&sbuf);+quote_c_style(name,"ed,NULL,0);+strbuf_addbuf(sb,"ed);+strbuf_release(&sbuf);+strbuf_release("ed);+}else{+errlen=(unsignedlong)len;+die(_("bad ls-tree format: %%%.*s"),errlen,start);+}+returnlen;+}+staticintshow_recursive(constchar*base,size_tbaselen,constchar*pathname){inti;
@@ -145,11 +246,33 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,returnrecurse;}+structls_tree_cmdmode_to_fmt{+enumls_tree_cmdmodemode;+constchar*constfmt;+};++staticstructls_tree_cmdmode_to_fmtls_tree_cmdmode_format[]={+{+.mode=MODE_DEFAULT,+.fmt="%(objectmode) %(objecttype) %(objectname)%x09%(path)",+},+{+.mode=MODE_LONG,+.fmt="%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)",+},+{+.mode=MODE_NAME_ONLY,/* And MODE_NAME_STATUS */+.fmt="%(path)",+},+{0},+};+intcmd_ls_tree(intargc,constchar**argv,constchar*prefix){structobject_idoid;structtree*tree;inti,full_tree=0;+read_tree_fn_tfn=show_tree;conststructoptionls_tree_options[]={OPT_BIT('d',NULL,&ls_options,N_("only show trees"),LS_TREE_ONLY),
@@ -170,6 +293,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)OPT_BOOL(0,"full-tree",&full_tree,N_("list entire tree; not just current directory ""(implies --full-name)")),+OPT_STRING_F(0,"format",&format,N_("format"),+N_("format to use for the output"),+PARSE_OPT_NONEG),OPT__ABBREV(&abbrev),OPT_END()};
@@ -190,6 +316,10 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)((LS_TREE_ONLY|LS_RECURSIVE)&ls_options))ls_options|=LS_SHOW_TREES;+if(format&&cmdmode)+usage_msg_opt(+_("--format can't be combined with other format-altering options"),+ls_tree_usage,ls_tree_options);if(argc<1)usage_with_options(ls_tree_usage,ls_tree_options);if(get_oid(argv[0],&oid))
@@ -211,6 +341,23 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)tree=parse_tree_indirect(&oid);if(!tree)die("not a tree object");-return!!read_tree(the_repository,tree,-&pathspec,show_tree,NULL);+/*+*Thegenericshow_tree_fmt()isslowerthanshow_tree(),so+*takethefastpathifpossible.+*/+if(format){+structls_tree_cmdmode_to_fmt*m2f;++fn=show_tree_fmt;+for(m2f=ls_tree_cmdmode_format;m2f->fmt;m2f++){+if(strcmp(format,m2f->fmt))+continue;++cmdmode=m2f->mode;+fn=show_tree;+break;+}+}++return!!read_tree(the_repository,tree,&pathspec,fn,NULL);}
From: Teng Long <hidden> Date: 2022-03-23 09:14:09
'--object-only' is an alias for '--format=%(objectname)'. It cannot
be used together other format-altering options like '--name-only',
'--long' or '--format', they are mutually exclusive.
The "--name-only" option outputs <filepath> only. Likewise, <objectName>
is another high frequency used field, so implement '--object-only' option
will bring intuitive and clear semantics for this scenario. Using
'--format=%(objectname)' we can achieve a similar effect, but the former
is with a lower learning cost(without knowing the format requirement
of '--format' option).
Even so, if a user is prefer to use "--format=%(objectname)", this is entirely
welcome because they are not only equivalent in function, but also have almost
identical performance. The reason is this commit also add the specific of
"--format=%(objectname)" to the current fast-pathes (builtin formats) to
avoid running unnecessary parsing mechanisms.
The following performance benchmarks are based on torvalds/linux.git:
When hit the fast-path:
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --object-only HEAD
Time (mean ± σ): 83.6 ms ± 2.0 ms [User: 59.4 ms, System: 24.1 ms]
Range (min … max): 80.4 ms … 87.2 ms 35 runs
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='%(objectname)' HEAD
Time (mean ± σ): 84.1 ms ± 1.8 ms [User: 61.7 ms, System: 22.3 ms]
Range (min … max): 80.9 ms … 87.5 ms 35 runs
But for a customized format, it will be slower:
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r --format='oid: %(objectname)' HEAD
Time (mean ± σ): 96.5 ms ± 2.5 ms [User: 72.9 ms, System: 23.5 ms]
Range (min … max): 93.1 ms … 104.1 ms 31 runs
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 11 ++++++++++-
builtin/ls-tree.c | 13 ++++++++++++-
t/t3103-ls-tree-misc.sh | 7 +++++--
t/t3104-ls-tree-format.sh | 9 +++++++++
4 files changed, 36 insertions(+), 4 deletions(-)
@@ -59,6 +59,15 @@ OPTIONS --name-only:: --name-status:: List only filenames (instead of the "long" output), one per line.+ Cannot be combined with `--object-only`.++--object-only::+ List only names of the objects, one per line. Cannot be combined+ with `--name-only` or `--name-status`.+ This is equivalent to specifying `--format='%(objectname)'`, but+ for both this option and that exact format the command takes a+ hand-optimized codepath instead of going through the generic+ formatting mechanism. --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object
@@ -288,6 +297,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)MODE_NAME_ONLY),OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),MODE_NAME_ONLY),+OPT_CMDMODE(0,"object-only",&cmdmode,N_("list only objects"),+MODE_OBJECT_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,N_("use full path names"),0),OPT_BOOL(0,"full-tree",&full_tree,
From: Teng Long <hidden> Date: 2022-03-23 09:14:29
From: Ævar Arnfjörð Bjarmason <redacted>
The --name-only and --name-status options are synonyms, but let's
detect and error if both are provided.
In addition let's add explicit --format tests for the combination of
these various options.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 11 ++++++++++-
t/t3103-ls-tree-misc.sh | 15 +++++++++------
2 files changed, 19 insertions(+), 7 deletions(-)
@@ -296,7 +297,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)OPT_CMDMODE(0,"name-only",&cmdmode,N_("list only filenames"),MODE_NAME_ONLY),OPT_CMDMODE(0,"name-status",&cmdmode,N_("list only filenames"),-MODE_NAME_ONLY),+MODE_NAME_STATUS),OPT_CMDMODE(0,"object-only",&cmdmode,N_("list only objects"),MODE_OBJECT_ONLY),OPT_SET_INT(0,"full-name",&chomp_prefix,
@@ -322,6 +323,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)ls_tree_prefix=prefix=NULL;chomp_prefix=0;}+/*+*Wewantedtodetectconflictsbetween--name-onlyand+*--name-status,butoncewe'redonewiththatsubsequent+*codeshouldonlyneedtochecktheprimaryname.+*/+if(cmdmode==MODE_NAME_STATUS)+cmdmode=MODE_NAME_ONLY;+/* -d -r should imply -t, but -d by itself should not have to. */if((LS_TREE_ONLY|LS_RECURSIVE)==((LS_TREE_ONLY|LS_RECURSIVE)&ls_options))
From: Teng Long <hidden> Date: 2022-03-23 09:14:38
From: Ævar Arnfjörð Bjarmason <redacted>
Make the various if/else in the callbacks for the "fast path" a lot
easier to read by just using common functions for the parts that are
common, and have per-format callbacks for those parts that are
different.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 199 +++++++++++++++++++++++++++++-----------------
1 file changed, 125 insertions(+), 74 deletions(-)
From: Teng Long <hidden> Date: 2022-03-23 09:59:02
On Mon, 21 Mar 2022 10:20:34 +0100, Ævar Arnfjörð Bjarmason wrote:
FWIW I didn't do any exhaustive benchmarks of this, but I checked a few
things against origin/master on linux.git and all the reported
"hyperfine" timings were the same/within the +/- interval.
I sended a reply on gmail but seems not updated on public-inbox,
so I sended again use git send-mail this time. If the duplicated
message appears, sorry for that.
I tested the performance between fast-paths and non-fast-paths again on linux.git.
And I found the default format appeared a performance regression which maybe was
brought by this commit:
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r HEAD
Time (mean ± σ): 111.3 ms ± 2.3 ms [User: 86.2 ms, System: 25.0 ms]
Range (min … max): 107.8 ms … 115.1 ms 25 runs
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r --format='%(objectmode) %(objecttype) %(objectname)%x09%(path)' HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r --format='%(objectmode) %(objecttype) %(objectname)%x09%(path)' HEAD
Time (mean ± σ): 159.1 ms ± 4.7 ms [User: 131.8 ms, System: 27.3 ms]
Range (min … max): 152.3 ms … 170.4 ms 19 runs
Further, the other fast-paths except "default format" seem like what we expect, then I made this change:
After the scenario has been tested agin, the result seems OK:
$hyperfine --warmup=10 "/opt/git/master/bin/git ls-tree -r --format='%(objectmode) %(objecttype) %(objectname)%x09%(path)' HEAD"
Benchmark 1: /opt/git/master/bin/git ls-tree -r --format='%(objectmode) %(objecttype) %(objectname)%x09%(path)' HEAD
Time (mean ± σ): 112.2 ms ± 2.5 ms [User: 86.3 ms, System: 25.9 ms]
Range (min … max): 108.8 ms … 117.4 ms 25 runs
Thanks.
From: Ævar Arnfjörð Bjarmason <redacted>
Make the various if/else in the callbacks for the "fast path" a lot
easier to read by just using common functions for the parts that are
common, and have per-format callbacks for those parts that are
different.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 199 +++++++++++++++++++++++++++++-----------------
1 file changed, 125 insertions(+), 74 deletions(-)
This commit makes `git ls-tree -l ...` list files recursively,
regardless of whether `-r` was provided or not. I believe it's due to
show_tree_long() returning 1 instead of the value of recurse. Is there a
reason why we unconditionally return 1 here?
In 9c4d58ff2c (ls-tree: split up "fast path" callbacks, 2022-03-23), a
refactoring of the various read_tree_at() callbacks caused us to
unconditionally recurse into directories if `-l` (long format) was
passed on the command line, regardless of whether or not we also pass
the `-r` (recursive) flag.
Fix this by making show_tree_long() return the value of `recurse`,
rather than always returning 1. This value is interpreted by
read_tree_at() to be a signal on whether or not to recurse.
Signed-off-by: Josh Steadmon <redacted>
---
I believe this is the correct fix for the change in `git ls-tree -l`
output. I would also like to add tests in a future fix, but I do not
have time to add them today.
builtin/ls-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Fix a regression introduced in 9c4d58ff2c3 (ls-tree: split up "fast
path" callbacks, 2022-03-23), and improve the tests added in the
1041d58b4d9 (Merge branch 'tl/ls-tree-oid-only', 2022-04-04) topic it
was merged as part of to test the full expected output of various
"ls-tree" options with and without -r.
Let's fix it, and also add tests not only for that blindspot, but also
any other potential blindspots. To that end test the "modes" of -d, -r
and -t (as well as "no mode") against all of the format options.
These tests all pass with that topic reverted (except those that would
fail because they're testing the new --object-only feature introduced
in that topic), which should give us confidence that there were no
further regressions in this area.
Reported-By: Josh Steadmon <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Mon, Apr 04 2022, Josh Steadmon wrote:
I believe this is the correct fix for the change in `git ls-tree -l`
output. I would also like to add tests in a future fix, but I do not
have time to add them today.
Indeed. I guess that makes this a proposed v2,
I refreshed my E-Mail when I was just about to submit this and spotted
that you'd sent your fix in, but I came up with this (in retrospect a
pretty obvious think-o) fix independently, sorry about the bug.
The tests took me a bit longer though...
Haing written them I guess we could do them post-release, since the
important thing is to validate the changes. As noted in the commit
message we're now testing all combinations of the "mode" and "format"
options.
builtin/ls-tree.c | 2 +-
t/t3104-ls-tree-format.sh | 126 +++++++++++++++++++++++++++++++++++---
2 files changed, 119 insertions(+), 9 deletions(-)
Indeed. I guess that makes this a proposed v2,
I refreshed my E-Mail when I was just about to submit this and spotted
that you'd sent your fix in, but I came up with this (in retrospect a
pretty obvious think-o) fix independently, sorry about the bug.
The tests took me a bit longer though...
Haing written them I guess we could do them post-release, since the
important thing is to validate the changes. As noted in the commit
message we're now testing all combinations of the "mode" and "format"
options.
Thanks for the quickly fix.
+ git ls-tree ${opt:+$opt }$opts $opt HEAD >actual &&
I think maybe the "$opt" here should be removed, because "${opt:+$opt }"
will not append it if "$opt" is null or unset, or will append "$opt ",
like: