[...] about the using "strbuf_addf(line, "%7s" , "-");" or
"strbuf_addstr(line, " -");". [...]
Why I prefer more of the former that is because, for the single line,
it's more readable I think.
I strongly disagree. Using a format requires the reader to interpret a
`printf()` format, to remember (if they ever knew) the rules about padding
with `%<number>s` formats, and then to satisfy themselves that the result
is correct.
That's quite the cognitive load you put on the reader for something as
trivial as " -".
Not a fan,
Johannes
I think you can argue that, but saying that this series must change that
existing "%7s" format just because it happened to trip over an existin
coccinelle rule as code was changed from printf() to strbuf_addf() is
going overboard.
Also, the ls-tree output has existing alignment issues, and the
documentation says:
"right-justified with minimum width of 7 characters"
So I'd think we'd want to keep the %7s, and in some future change change
that format to be dynamic so we'd align things properly if some fields
were longer than 7 characters.
From: Martin Ågren <hidden> Date: 2022-01-10 19:41:47
Hi Teng,
On Fri, 7 Jan 2022 at 06:34, Teng Long [off-list ref] wrote:
+--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
Above, there is a missing space just before "are". That causes the
manpage to render a little bit funny.
+ 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`.
+
+Customized format:
+
+It's support to print customized format by `%(fieldname)` with `--format` option.
I had to re-read this to understand. How about the following?
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.
Just a suggestion. Feel free to tweak or ignore. :-)
+For example, if you want to only print the <object> and <file> fields with a
+JSON style, executing with a specific "--format" like
+
+ git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>
+
+The output format changes to:
+
+ {"object":"<object>", "file":"<file>"}
+
From: Teng Long <hidden> Date: 2022-01-11 09:35:08
On Tue, Jan 11, 2022 at 3:41 AM Martin Ågren [off-list ref] wrote:
Above, there is a missing space just before "are". That causes the
manpage to render a little bit funny.
quoted
+ 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`.
+
Thanks for reviewing. will be fixed in the next patch.
quoted
+Customized format:
+
+It's support to print customized format by `%(fieldname)` with `--format` option.
I had to re-read this to understand. How about the following?
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.
Just a suggestion. Feel free to tweak or ignore. :-)
Yours reads more smoothly than mine, it will be applied in next patch.
Thanks.
Let's refer to commits like this:
28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)
I find it helpful to have an alias like:
$ git config alias.ll
!git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short
in my $HOME/.gitconfig so that I can easily format commits in the
standard way.
I think that this alias came from Peff, but I can't remember.
Thanks,
Taylor
Let's refer to commits like this:
28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)
I find it helpful to have an alias like:
$ git config alias.ll
!git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short
in my $HOME/.gitconfig so that I can easily format commits in the
standard way.
You can shorten "--pretty='tformat:%h (%s, %ad)' --date=short" to
"--pretty=reference" or "--format=reference". For me that's easy enough
to remember that I don't need an alias.
Silly question, going further off-topic: What's "git always" doing?
René
From: Taylor Blau <hidden> Date: 2022-01-11 20:11:22
On Tue, Jan 11, 2022 at 08:06:00PM +0100, René Scharfe wrote:
Am 11.01.22 um 17:42 schrieb Taylor Blau:
quoted
I find it helpful to have an alias like:
$ git config alias.ll
!git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short
in my $HOME/.gitconfig so that I can easily format commits in the
standard way.
You can shorten "--pretty='tformat:%h (%s, %ad)' --date=short" to
"--pretty=reference" or "--format=reference". For me that's easy enough
to remember that I don't need an alias.
Ah, of course. Peff's copy likely predates `--pretty=reference`, and I
inherited the cruft from him. Your suggestion has the nice benefit of
colorizing the output when going to the terminal.
Silly question, going further off-topic: What's "git always" doing?
Oops, I should have mentioned. It's another alias to ensure that the
following command is always run in a Git repository (either the current
one or a hand-picked default):
$ git config alias.always
!git rev-parse 2>/dev/null || cd ~/src/git; git
I often read mail out of my home directory, and the above works with my
`:Git` command in Vim (which passes its arguments to `git always` and
inserts the result back into my buffer). That way I don't have to first
`:cd ~/src/git` and then `:Git ll xyz`, I can just `:Git ll xyz` and it
does what I meant most of the time.
Thanks,
Taylor
Let's refer to commits like this:
28c23cd4c39 (strbuf.cocci: suggest strbuf_addbuf() to add one strbuf to an other, 2019-01-25)
I find it helpful to have an alias like:
$ git config alias.ll
!git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short
in my $HOME/.gitconfig so that I can easily format commits in the
standard way.
I think that this alias came from Peff, but I can't remember.
Nowadays you can do this as:
git show -s --pretty=reference
See Documentation/SubmittingPatches
I use:
$ git help reference
'reference' is aliased to '!git --no-pager log --pretty=reference -1'
From: Teng Long <hidden> Date: 2022-01-13 03:29:12
On Wed, Jan 12, 2022 at 12:42 AM Taylor Blau [off-list ref] wrote:
I find it helpful to have an alias like:
$ git config alias.ll
!git always --no-pager log -1 --pretty='tformat:%h (%s, %ad)' --date=short
in my $HOME/.gitconfig so that I can easily format commits in the
standard way.
I think that this alias came from Peff, but I can't remember.
From: Teng Long <hidden> Date: 2022-01-13 03:34:24
On Wed, Jan 12, 2022 at 4:11 AM Taylor Blau [off-list ref] wrote:
quoted
Silly question, going further off-topic: What's "git always" doing?
Oops, I should have mentioned. It's another alias to ensure that the
following command is always run in a Git repository (either the current
one or a hand-picked default):
$ git config alias.always
!git rev-parse 2>/dev/null || cd ~/src/git; git
I often read mail out of my home directory, and the above works with my
`:Git` command in Vim (which passes its arguments to `git always` and
inserts the result back into my buffer). That way I don't have to first
`:cd ~/src/git` and then `:Git ll xyz`, I can just `:Git ll xyz` and it
does what I meant most of the time.
The same question is clear now。
Thanks for the explanations from Taylor Blau and René Scharfe.
From: Teng Long <hidden> Date: 2022-01-13 03:35:28
On Wed, Jan 12, 2022 at 4:40 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
Nowadays you can do this as:
git show -s --pretty=reference
See Documentation/SubmittingPatches
I use:
$ git help reference
'reference' is aliased to '!git --no-pager log --pretty=reference -1'
From: Teng Long <hidden> Date: 2022-01-13 03:42:25
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-01-13 03:42:31
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-01-13 03:42:34
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-01-13 03:42:36
The variable which "show_tree()" return is named "retval", a name that's
a little hard to understand. This commit tries to make the variable
and the related codes more clear in the context.
The commit firstly rename "retval" to "recurse" which is a more
meaningful name than before. Secondly, "get_type()" is introduced
to setup the "type" by "mode", this will remove some of the nested if.
After this, The codes here become a little bit clearer, so we do not
need to take a look at "read_tree_at()" in "tree.c" to make sure the
context of the return value.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
From: Teng Long <hidden> Date: 2022-01-13 03:42:40
We usually pipe the output from `git ls-trees` to tools like
`sed` or `cut` when we only want to extract some fields.
When we want only the pathname component, we can pass
`--name-only` option to omit such a pipeline, but there are no
options for extracting other fields.
Teach the "--object-only" option to the command to only show the
object name. This option cannot be used together with
"--name-only" or "--long" , they are mutually exclusive (actually
"--name-only" and "--long" can be combined together before, this
commit by the way fix this bug).
A simple refactoring was done to the "show_tree" function, intead by
using bitwise operations to recognize the format for printing to
stdout. The reason for doing this is that we don't want to increase
the readability difficulty with the addition of "-object-only",
making this part of the logic easier to read and expand.
In terms of performance, there is no loss comparing to the
"master" (2ae0a9cb8298185a94e5998086f380a355dd8907), here are the
results of the performance tests in my environment based on linux
repository:
$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.8 ms ± 2.7 ms [User: 85.7 ms, System: 20.0 ms]
Range (min … max): 101.5 ms … 111.3 ms 28 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
Time (mean ± σ): 105.0 ms ± 3.0 ms [User: 83.7 ms, System: 21.2 ms]
Range (min … max): 99.3 ms … 109.5 ms 27 runs
$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 ± σ): 337.4 ms ± 10.9 ms [User: 308.3 ms, System: 29.0 ms]
Range (min … max): 323.0 ms … 355.0 ms 10 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
Time (mean ± σ): 337.6 ms ± 6.2 ms [User: 309.4 ms, System: 28.1 ms]
Range (min … max): 330.4 ms … 349.9 ms 10 runs
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 7 +-
builtin/ls-tree.c | 141 +++++++++++++++++++++++++---------
t/t3104-ls-tree-oid.sh | 51 ++++++++++++
3 files changed, 160 insertions(+), 39 deletions(-)
create mode 100755 t/t3104-ls-tree-oid.sh
@@ -59,6 +59,11 @@ 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`. --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object
@@ -129,12 +190,14 @@ 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",&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_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,
@@ -165,6 +228,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-01-13 03:42:47
"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: Teng Long <redacted>
---
builtin/ls-tree.c | 44 +++++++++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 15 deletions(-)
From: Teng Long <hidden> Date: 2022-01-13 03:42:49
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-01-13 03:43:01
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].
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/
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 51 +++++++++++++-
builtin/ls-tree.c | 129 +++++++++++++++++++++++++++++++++-
t/t3105-ls-tree-format.sh | 55 +++++++++++++++
3 files changed, 230 insertions(+), 5 deletions(-)
create mode 100755 t/t3105-ls-tree-format.sh
@@ -10,9 +10,9 @@ SYNOPSIS -------- [verse] 'git ls-tree' [-d] [-r] [-t] [-l] [-z]- [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]]- <tree-ish> [<path>...]-+ [--name-only] [--name-status] [--object-only]+ [--full-name] [--full-tree] [--abbrev[=<n>]]+ [--format=<format>] <tree-ish> [<path>...] DESCRIPTION ----------- Lists the contents of a given tree object, like what "/bin/ls -a" does
@@ -79,6 +79,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
@@ -87,6 +97,9 @@ OPTIONS Output Format -------------++Default format:+ <mode> SP <type> SP <object> TAB <file> This output format is compatible with what `--index-info --stdin` of
@@ -105,6 +118,38 @@ 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 want to only print the <object> and <file> fields with a+JSON style, executing with a specific "--format" like++ git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>++The output format changes to:++ {"object":"<object>", "file":"<file>"}++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:++mode::+ The mode of the object.+type::+ The type of the object (`blob` or `tree`).+object::+ The name of the object.+size[:padded]::+ The size of the object ("-" if it's a tree).+ It also supports a padded format of size with "%(size:padded)".+file::+ The filename of the object.+ GIT --- Part of the linkgit:git[1] suite
@@ -76,6 +82,72 @@ static int parse_shown_fields(void)return1;}+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*line,constchar*start,+void*context)+{+structshow_tree_data*data=context;+constchar*end;+constchar*p;+unsignedinterrlen;+size_tlen=strbuf_expand_literal_cb(line,start,NULL);++if(len)+returnlen;+if(*start!='(')+die(_("bad ls-tree format: as '%s'"),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,"(mode)",&p)){+strbuf_addf(line,"%06o",data->mode);+}elseif(skip_prefix(start,"(type)",&p)){+strbuf_addstr(line,type_name(data->type));+}elseif(skip_prefix(start,"(size:padded)",&p)){+expand_objectsize(line,data->oid,data->type,1);+}elseif(skip_prefix(start,"(size)",&p)){+expand_objectsize(line,data->oid,data->type,0);+}elseif(skip_prefix(start,"(object)",&p)){+strbuf_add_unique_abbrev(line,data->oid,abbrev);+}elseif(skip_prefix(start,"(file)",&p)){+constchar*name=data->base->buf;+constchar*prefix=chomp_prefix?ls_tree_prefix:NULL;+structstrbufquoted=STRBUF_INIT;+structstrbufsb=STRBUF_INIT;+strbuf_addstr(data->base,data->pathname);+name=relative_path(data->base->buf,prefix,&sb);+quote_c_style(name,"ed,NULL,0);+strbuf_addbuf(line,"ed);+strbuf_release(&sb);+strbuf_release("ed);+}else{+errlen=(unsignedlong)len;+die(_("bad ls-tree format: %%%.*s"),errlen,start);+}+returnlen;+}+staticintshow_recursive(constchar*base,size_tbaselen,constchar*pathname){
@@ -116,6 +188,38 @@ static enum object_type get_type(unsigned int mode):OBJ_BLOB);}+staticintshow_tree_fmt(conststructobject_id*oid,structstrbuf*base,+constchar*pathname,unsignedmode,void*context)+{+size_tbaselen;+intrecurse=0;+structstrbufline=STRBUF_INIT;+enumobject_typetype=get_type(mode);++structshow_tree_datadata={+.mode=mode,+.type=type,+.oid=oid,+.pathname=pathname,+.base=base,+};++if(type==OBJ_TREE&&show_recursive(base->buf,base->len,pathname))+recurse=READ_TREE_RECURSIVE;+if(type==OBJ_TREE&&recurse&&!(ls_options&LS_SHOW_TREES))+returnrecurse;+if(type==OBJ_BLOB&&(ls_options&LS_TREE_ONLY))+return0;++baselen=base->len;+strbuf_expand(&line,format,expand_show_tree,&data);+strbuf_addch(&line,line_termination);+fwrite(line.buf,line.len,1,stdout);+strbuf_release(&line);+strbuf_setlen(base,baselen);+returnrecurse;+}+staticintshow_default(structshow_tree_data*data){size_tbaselen=data->base->len;
@@ -195,6 +299,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *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),
@@ -217,6 +322,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()};
@@ -237,6 +345,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))
@@ -260,6 +372,19 @@ 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&&+(!strcmp(format,default_format)||+!strcmp(format,long_format)||+!strcmp(format,name_only_format)||+!strcmp(format,object_only_format)))+fn=show_tree;+elseif(format)+fn=show_tree_fmt;++return!!read_tree(the_repository,tree,&pathspec,fn,NULL);}
On Thu, Jan 13 2022, Teng Long wrote:
Re the $subject: Is "optimize naming" here just referring to the
s/retval/recurse/g?
Personally I think just a s/retval/ret/g here would make more senes if
we're doing any change at all, and in either case having this variable
re-rename split up as its own commit would make the proposed control
flow changes clearer.
quoted hunk
The variable which "show_tree()" return is named "retval", a name that's
a little hard to understand. This commit tries to make the variable
and the related codes more clear in the context.
The commit firstly rename "retval" to "recurse" which is a more
meaningful name than before. Secondly, "get_type()" is introduced
to setup the "type" by "mode", this will remove some of the nested if.
After this, The codes here become a little bit clearer, so we do not
need to take a look at "read_tree_at()" in "tree.c" to make sure the
context of the return value.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
We usually pipe the output from `git ls-trees` to tools like
`sed` or `cut` when we only want to extract some fields.
When we want only the pathname component, we can pass
`--name-only` option to omit such a pipeline, but there are no
options for extracting other fields.
Teach the "--object-only" option to the command to only show the
object name. This option cannot be used together with
"--name-only" or "--long" , they are mutually exclusive (actually
"--name-only" and "--long" can be combined together before, this
commit by the way fix this bug).
In the RFC series I sent this was first implemented in terms of the
--format option, and I skipped the custom implementation you're adding
here:
https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/
I think in terms of patch series structure it would make sense to do
that, and then have this custom --object-only implementation in terms of
not-"--format " follow from that, and thus with the tests for the two
(we'd add the tests you're adding here first, just for a
--format="%(objectname)" or whatever) we'd see that the two are 1=1
equivalent in terms of functionality, but that this one is <X>% more
optimized.
"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.
Is the only reason this is split off from 9/9 because you're injecting a
8/9 commit for the coccinelle rule change, and wanted to find some
logical cut-off between the two?
Let's split up this re-flow only change into its own commit? I.e. the
only non-whitespace change here is beginning with [--format].
If it was the right thing to do to re-flow this then we didn't need
[--format=<format>] to exist to do so...
Removing this \n breaks the formatting in the file. See "make man && man
./Documentation/git-ls-tree.1". The ./Documentation/doc-diff utility is
also handy for sanity checking the documentation formatting.
quoted hunk
-----------
Lists the contents of a given tree object, like what "/bin/ls -a" does
@@ -79,6 +79,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`.+
These new docs make sense & seem to cover all the basis, thanks!
Here because we've added --format discussing the previous pseudo-format
as a "default" format becomes confusing. Let's instead say:
The output format of `ls-tree` is determined by either the `--format` option,
or other format-altering options such as `--name-long` 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:
---
%(mode) %(type) %(object)%x09%(file)
---
Or something like that. We could then discuss e.g. --name-long being
`%(mode) %(type) %(object) %(size:padded)%x09%(file)` when we discuss
that option.
quoted hunk
This output format is compatible with what `--index-info --stdin` of
@@ -105,6 +118,38 @@ 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 want to only print the <object> and <file> fields with a+JSON style, executing with a specific "--format" like++ git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>++The output format changes to:++ {"object":"<object>", "file":"<file>"}
This one-liner is guaranteed to result in invalid JSON on some
repositories, both because JSON is inherently a bad fit for git's data
model (JSON needs to be in one Unicode encoding, Git's tree data might
me in a mixture of encodings), and because it'll break if the file
includes a '"'.
I think it's better to just replace this with some example involving -z,
or at least prominently note that this is broken in the general case,
but can be used ad-hoc to quickly check things with "jq" or whatever.
+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:
+
+mode::
+ The mode of the object.
+type::
+ The type of the object (`blob` or `tree`).
+object::
+ The name of the object.
+size[:padded]::
+ The size of the object ("-" if it's a tree).
+ It also supports a padded format of size with "%(size:padded)".
+file::
+ The filename of the object.
In
https://lore.kernel.org/git/cover.1641043500.git.dyroneteng@gmail.com/
you noted that you changed the field names of e.g. "objectname" to
"object" etc. You're right that I picked these as-is from the
git-for-each-ref formatting.
1/3 of your reasoning for doing so was to make it consistent with the
documentation examples of e.g.:
<mode> SP <type> SP <object> TAB <file>
I think in any case (as noted above) we should change those to use the
--format), so that leaves just:
- "I prefer to make the name more simple to memorize and type"
- "I think the names with "object" prefix are [from git-for-each-ref
and the object* prefixes aren't redundant there, but would be here]".
I think both of those still apply, but I think having these consistent
with git-for-each-ref outweighs the slight benefit of shorter names.
Right now only a handful of things support these sort of --format
directives, but we've already got RFC/WIP patches to add that to
git-cat-file, and are likely to add more in the future.
I'd also like us to eventually be able to combine what are now separate
built-ins with their own --format to expose more deeply some internal
APIs via IPC. E.g. now you can do this:
git for-each-ref --format='%(refname) %(tree)'
But to list each of those trees you'd need to pipe that output into this
new 'git ls-tree --format. But imagine being able to do something like:
git for-each-ref --format='%(refname) %(git-ls-tree --format %%(objectname) %(tree))'
Where we'd just invoke git-ls-tree for you without running a full
sub-process. I think both for that hypothetical and working with the two
--formats now having to use %(type) in some places but %(objecttype)
etc. in others is just needlessly confusing. Let's just consistently use
the same format names everywhere.
Specifically for your s/path/file/ name change, that's just inaccurate, consider:
$ ./git ls-tree --format="%(mode) %(type) %(file)" -t HEAD -- t/README
040000 tree t
100644 blob t/README
And:
$ $ (cd t && ../git ls-tree --format="%(mode) %(type) %(file)" -t -r HEAD -- README)
040000 tree ./
100644 blob README
I.e. we talk about <path> in the existing SYNOPSIS for a reason. That we
had a "<file>" in the existing format demo was a bug/shorthand that we
shouldn't be propagating further.
One advantage of keeping the variable names I picked in
https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
is that they align, so you can instantly see that the first two are
equivalent until the "%x09".
It also makes it easier to review to avoid such churn, to see what you
really changed I'm looking at a local version of a range-diff where I
renamed these, the struct you renamed etc. back just to see what you
/really/ changed. I.e. what are functional v.s. renaming changes.
quoted hunk
static int parse_shown_fields(void)
{
if (cmdmode == MODE_NAME_ONLY) {
@@ -76,6 +82,72 @@ static int parse_shown_fields(void) return 1; }+static void expand_objectsize(struct strbuf *line, const struct object_id *oid,+ const enum object_type type, unsigned int padded)+{+ if (type == OBJ_BLOB) {+ unsigned long size;+ 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);
Here you changed my '"%"PRIuMAX' to '"%" PRIuMAX'. The former is the
prevailing style in this codebase, and avoiding the formatting churn
makes the inter-diff easier to read.
Ditto some harder to review interdiff due to renaming
churn. I.e. s/line/sb/ in both this and expand_show_tree(). I really
wouldn't care at all except because of all the manual work in reviewing
the inter-diff between my original version & this derived version.
In the case of "line" that's not even an improvement. With a --format
we're not building a "line", the user is free to insert any arbitrary
directives including \n's, so we might be working on multiple lines.
This & several other changes v.s. my version are good, e.g. here I seem
to have repeated the logic error I noted for your version (i.e omitting
"HEAD"), oops!
As I noted in my RFC CL (https://lore.kernel.org/git/RFC-cover-0.7-00000000000-20211217T131635Z-avarab@gmail.com/):
"the tests for ls-tree are really
lacking. E.g. I seem to have a rather obvious bug in how -t and the
--format interact here, but no test catches it."
So first, in my version of adding --format I was careful to make
--name-only etc. imply a given --format, and then only at the last
minute would we take the "fast path":
https://lore.kernel.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
You rewrote that in
https://lore.kernel.org/git/e0add802fbbabde7e7b3743127b2d4047f1ce760.1641043500.git.dyroneteng@gmail.com/
and qremoved the limited "GIT_TEST_LS_TREE_FORMAT_BACKEND" testing I
added, so now the internal --format machinery can't be run through the
existing tests we do have.
Even with that re-added I really wouldn't trust that this code is doing
the right thing (and as noted, I don't trust my own RFC version
either). I think e.g. our "coverage" Makefile targets would be a good
start as a first approximation, i.e. running the /ls-tree/ tests and
seeing if we have full coverage.
Signed-off-by: Teng Long <redacted>
As I noted in 7/9 I think this patch is 9/9 still mostly something I
wrote, so that the "author" and Signed-off-by should be preserved. The
below is a range-diff of an amended version I've been looking at in
trying to review this. It undoes several (but not all) of your
formatting/renaming-only changes, just so that I could see what the
non-formatting changes were:
1: 6c96dff15c5 ! 1: 917bb168d45 ls-tree: add a --format=<fmt> option
@@
## Metadata ##
-Author: Ævar Arnfjörð Bjarmason [off-list ref]
+Author: Teng Long [off-list ref]
## Commit message ##
- ls-tree: add a --format=<fmt> option
+ ls-tree.c: introduce "--format" option
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
@@ Commit message
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_format() in if
+ 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.
- "Slower" here can bee seen via the the following "hyperfine"
- command. This uses GIT_TEST_LS_TREE_FORMAT_BACKEND=<bool> to force the
- use of the new backend:
-
- $ hyperfine -L env false,true -L f "-r,-r -l,-r --name-only,-r --format='%(objectname)'" 'GIT_TEST_LS_TREE_FORMAT_BACKEND={env} ./git -C ~/g/linux ls-tree {f} HEAD' -r 10
- Benchmark 1: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD
- Time (mean ± σ): 86.1 ms ± 0.6 ms [User: 65.2 ms, System: 20.9 ms]
- Range (min … max): 85.2 ms … 87.5 ms 10 runs
+ 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.
- Benchmark 2: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD
- Time (mean ± σ): 122.5 ms ± 0.6 ms [User: 101.3 ms, System: 21.1 ms]
- Range (min … max): 121.8 ms … 123.4 ms 10 runs
+ 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].
- Benchmark 3: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD
- Time (mean ± σ): 277.7 ms ± 1.3 ms [User: 234.6 ms, System: 43.0 ms]
- Range (min … max): 275.9 ms … 279.7 ms 10 runs
+ Here is the statistics about performance tests:
- Benchmark 4: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD
- Time (mean ± σ): 332.8 ms ± 2.6 ms [User: 282.0 ms, System: 50.7 ms]
- Range (min … max): 329.6 ms … 338.2 ms 10 runs
+ 1. Default format (hitten the builtin formats):
- Benchmark 5: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD
- Time (mean ± σ): 71.8 ms ± 0.4 ms [User: 54.1 ms, System: 17.6 ms]
- Range (min … max): 71.2 ms … 72.5 ms 10 runs
+ "git ls-tree <tree-ish>" vs "--format='%(mode) %(type) %(object)%x09%(file)'"
- Benchmark 6: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD
- Time (mean ± σ): 86.6 ms ± 0.5 ms [User: 65.7 ms, System: 20.7 ms]
- Range (min … max): 85.9 ms … 87.4 ms 10 runs
+ $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
- Benchmark 7: GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD
- Time (mean ± σ): 85.8 ms ± 0.6 ms [User: 66.2 ms, System: 19.5 ms]
- Range (min … max): 85.0 ms … 86.9 ms 10 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
- Benchmark 8: GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD
- Time (mean ± σ): 85.3 ms ± 0.2 ms [User: 66.6 ms, System: 18.7 ms]
- Range (min … max): 85.0 ms … 85.7 ms 10 runs
+ 2. Default format includes object size (hitten the builtin formats):
- Summary
- 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --name-only HEAD' ran
- 1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD'
- 1.19 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r --format='%(objectname)' HEAD'
- 1.20 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r HEAD'
- 1.21 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r --name-only HEAD'
- 1.71 ± 0.01 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r HEAD'
- 3.87 ± 0.03 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=false ./git -C ~/g/linux ls-tree -r -l HEAD'
- 4.64 ± 0.05 times faster than 'GIT_TEST_LS_TREE_FORMAT_BACKEND=true ./git -C ~/g/linux ls-tree -r -l HEAD'
+ "git ls-tree -l <tree-ish>" vs "--format='%(mode) %(type) %(object) %(size:padded)%x09%(file)'"
- 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.
+ $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
- But even a --format='%(objectname)' is fast with the new backend, so
- this is viable as a replacement for adding new formats, and we'll pay
- for this added complexity as a one-off, and not again every time a new
- format needs to be added. See [1] for an example of what it would
- otherwise take to add an --object-name flag.
+ $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
- 1. https://lore.kernel.org/git/2e449d1c792ff81da5f22c8bf65ed33c393d62f8.1639721750.git.dyroneteng@gmail.com/
+ Links:
+ [1] https://public-inbox.org/git/RFC-patch-6.7-eac299f06ff-20211217T131635Z-avarab@gmail.com/
- Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+ Signed-off-by: Teng Long [off-list ref]
- ## builtin/ls-tree.c ##
-@@ builtin/ls-tree.c: static struct pathspec pathspec;
- static int chomp_prefix;
- static const char *ls_tree_prefix;
+ ## Documentation/git-ls-tree.txt ##
+@@ Documentation/git-ls-tree.txt: SYNOPSIS
+ --------
+ [verse]
+ 'git ls-tree' [-d] [-r] [-t] [-l] [-z]
+- [--name-only] [--name-status] [--object-only] [--full-name] [--full-tree] [--abbrev[=<n>]]
+- <tree-ish> [<path>...]
++ [--name-only] [--name-status] [--object-only]
++ [--full-name] [--full-tree] [--abbrev[=<n>]]
++ [--format=<format>] <tree-ish> [<path>...]
-+/*
-+ * The format equivalents that show_tree() is prepared to handle.
-+ */
-+static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)";
-+static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";
-+static const char *ls_tree_format_n = "%(path)";
+ DESCRIPTION
+ -----------
+@@ Documentation/git-ls-tree.txt: 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`.
+
- static const char * const ls_tree_usage[] = {
- N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
- NULL
- };
+ [<path>...]::
+ When paths are given, show them (note that this isn't really raw
+ pathnames, but rather a list of patterns to match). Otherwise
+@@ Documentation/git-ls-tree.txt: OPTIONS
-+struct read_tree_ls_tree_data {
-+ const char *format;
-+ struct strbuf sb_scratch;
-+ struct strbuf sb_tmp;
-+};
+ Output Format
+ -------------
++
++Default format:
++
+ <mode> SP <type> SP <object> TAB <file>
+
+ This output format is compatible with what `--index-info --stdin` of
+@@ Documentation/git-ls-tree.txt: 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 want to only print the <object> and <file> fields with a
++JSON style, executing with a specific "--format" like
++
++ git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>
++
++The output format changes to:
++
++ {"object":"<object>", "file":"<file>"}
++
++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:
++
++mode::
++ The mode of the object.
++type::
++ The type of the object (`blob` or `tree`).
++object::
++ The name of the object.
++size[:padded]::
++ The size of the object ("-" if it's a tree).
++ It also supports a padded format of size with "%(size:padded)".
++file::
++ The filename of the object.
++
+ GIT
+ ---
+ Part of the linkgit:git[1] suite
+
+ ## builtin/ls-tree.c ##
+@@ builtin/ls-tree.c: static unsigned int shown_fields;
+ #define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */
+ #define FIELD_LONG_DEFAULT (FIELD_DEFAULT | FIELD_SIZE)
+
+struct expand_ls_tree_data {
+ unsigned mode;
+ enum object_type type;
+ const struct object_id *oid;
+ const char *pathname;
-+ const char *basebuf;
-+ struct strbuf *sb_scratch;
-+ struct strbuf *sb_tmp;
++ struct strbuf *base;
+};
+
- static int show_recursive(const char *base, size_t baselen, const char *pathname)
+ static const char * const ls_tree_usage[] = {
+ N_("git ls-tree [<options>] <tree-ish> [<path>...]"),
+ NULL
+@@ builtin/ls-tree.c: enum {
+
+ static int cmdmode = MODE_UNSPECIFIED;
+
++static const char *format;
++static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)%x09%(path)";
++static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)";
++static const char *ls_tree_format_n = "%(path)";
++static const char *ls_tree_format_o = "%(objectname)";
++
+ static int parse_shown_fields(void)
{
- int i;
-@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, const char *pathname
- return 0;
+ if (cmdmode == MODE_NAME_ONLY) {
+@@ builtin/ls-tree.c: static int parse_shown_fields(void)
+ return 1;
}
-+static void expand_objectsize(struct strbuf *sb,
-+ const struct object_id *oid,
-+ const enum object_type type,
-+ unsigned int padded)
++static void expand_objectsize(struct strbuf *sb, const struct object_id *oid,
++ const enum object_type type, unsigned int padded)
+{
+ if (type == OBJ_BLOB) {
+ unsigned long size;
+ if (oid_object_info(the_repository, oid, &size) < 0)
-+ die(_("could not get object info about '%s'"), oid_to_hex(oid));
++ die(_("could not get object info about '%s'"),
++ oid_to_hex(oid));
+ if (padded)
+ strbuf_addf(sb, "%7"PRIuMAX, (uintmax_t)size);
+ else
@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
+ }
+}
+
-+static size_t expand_show_tree(struct strbuf *sb,
-+ const char *start,
++static size_t expand_show_tree(struct strbuf *sb, const char *start,
+ void *context)
+{
+ struct expand_ls_tree_data *data = context;
+ const char *end;
+ const char *p;
-+ size_t len;
++ unsigned int errlen;
++ size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+
-+ len = strbuf_expand_literal_cb(sb, start, NULL);
+ if (len)
+ return len;
-+
+ if (*start != '(')
-+ die(_("bad format as of '%s'"), start);
++ die(_("bad ls-tree format: as '%s'"), start);
++
+ end = strchr(start + 1, ')');
+ if (!end)
-+ die(_("ls-tree format element '%s' does not end in ')'"),
-+ start);
-+ len = end - start + 1;
++ 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);
+ } else if (skip_prefix(start, "(objecttype)", &p)) {
@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen, c
+ } else if (skip_prefix(start, "(objectsize)", &p)) {
+ expand_objectsize(sb, data->oid, data->type, 0);
+ } else if (skip_prefix(start, "(objectname)", &p)) {
-+ strbuf_addstr(sb, find_unique_abbrev(data->oid, abbrev));
++ strbuf_add_unique_abbrev(sb, data->oid, abbrev);
+ } else if (skip_prefix(start, "(path)", &p)) {
-+ const char *name = data->basebuf;
++ const char *name = data->base->buf;
+ const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
-+
-+ if (prefix)
-+ name = relative_path(name, prefix, data->sb_scratch);
-+ quote_c_style(name, data->sb_tmp, NULL, 0);
-+ strbuf_add(sb, data->sb_tmp->buf, data->sb_tmp->len);
-+
-+ strbuf_reset(data->sb_tmp);
-+ /* The relative_path() function resets "scratch" */
++ struct strbuf quoted = STRBUF_INIT;
++ struct strbuf s = STRBUF_INIT;
++ strbuf_addstr(data->base, data->pathname);
++ name = relative_path(data->base->buf, prefix, &s);
++ quote_c_style(name, "ed, NULL, 0);
++ strbuf_addbuf(sb, "ed);
++ strbuf_release(&s);
++ strbuf_release("ed);
+ } else {
-+ unsigned int errlen = (unsigned long)len;
-+ die(_("bad ls-tree format specifiec %%%.*s"), errlen, start);
++ errlen = (unsigned long)len;
++ die(_("bad ls-tree format: %%%.*s"), errlen, start);
+ }
-+
+ return len;
+}
+
- static int show_tree_init(enum object_type *type, struct strbuf *base,
- const char *pathname, unsigned mode, int *retval)
+ static int show_recursive(const char *base, size_t baselen,
+ const char *pathname)
{
-@@ builtin/ls-tree.c: static int show_tree_init(enum object_type *type, struct strbuf *base,
+@@ builtin/ls-tree.c: static int show_recursive(const char *base, size_t baselen,
return 0;
}
+-static int show_default(const struct object_id *oid, enum object_type type,
+- const char *pathname, unsigned mode,
+- struct strbuf *base)
+static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
+ const char *pathname, unsigned mode, void *context)
-+{
-+ struct read_tree_ls_tree_data *data = context;
-+ struct expand_ls_tree_data my_data = {
+ {
+- size_t baselen = base->len;
++ size_t baselen;
++ int recurse = 0;
++ struct strbuf line = STRBUF_INIT;
++ enum object_type type = object_type(mode);
++
++ struct expand_ls_tree_data data = {
+ .mode = mode,
-+ .type = OBJ_BLOB,
++ .type = type,
+ .oid = oid,
+ .pathname = pathname,
-+ .sb_scratch = &data->sb_scratch,
-+ .sb_tmp = &data->sb_tmp,
++ .base = base,
+ };
-+ struct strbuf sb = STRBUF_INIT;
-+ int retval = 0;
-+ size_t baselen;
+
-+ if (show_tree_init(&my_data.type, base, pathname, mode, &retval))
-+ return retval;
++ if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
++ recurse = READ_TREE_RECURSIVE;
++ if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
++ return recurse;
++ if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
++ return 0;
+
+ baselen = base->len;
-+ strbuf_addstr(base, pathname);
-+ strbuf_reset(&sb);
-+ my_data.basebuf = base->buf;
-+
-+ strbuf_expand(&sb, data->format, expand_show_tree, &my_data);
-+ strbuf_addch(&sb, line_termination);
-+ fwrite(sb.buf, sb.len, 1, stdout);
++ strbuf_expand(&line, format, expand_show_tree, &data);
++ strbuf_addch(&line, line_termination);
++ fwrite(line.buf, line.len, 1, stdout);
++ strbuf_release(&line);
+ strbuf_setlen(base, baselen);
-+
-+ return retval;
++ return recurse;
+}
+
- static int show_tree(const struct object_id *oid, struct strbuf *base,
- const char *pathname, unsigned mode, void *context)
- {
++static int show_default(struct expand_ls_tree_data *data)
++{
++ size_t baselen = data->base->len;
+
+ if (shown_fields & FIELD_SIZE) {
+ char size_text[24];
+- if (type == OBJ_BLOB) {
++ if (data->type == OBJ_BLOB) {
+ unsigned long size;
+- if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
++ if (oid_object_info(the_repository, data->oid, &size) == OBJ_BAD)
+ xsnprintf(size_text, sizeof(size_text), "BAD");
+ else
+ xsnprintf(size_text, sizeof(size_text),
+@@ builtin/ls-tree.c: static int show_default(const struct object_id *oid, enum object_type type,
+ } else {
+ xsnprintf(size_text, sizeof(size_text), "-");
+ }
+- printf("%06o %s %s %7s\t", mode, type_name(type),
+- find_unique_abbrev(oid, abbrev), size_text);
++ printf("%06o %s %s %7s\t", data->mode, type_name(data->type),
++ find_unique_abbrev(data->oid, abbrev), size_text);
+ } else {
+- printf("%06o %s %s\t", mode, type_name(type),
+- find_unique_abbrev(oid, abbrev));
++ printf("%06o %s %s\t", data->mode, type_name(data->type),
++ find_unique_abbrev(data->oid, abbrev));
+ }
+- baselen = base->len;
+- strbuf_addstr(base, pathname);
+- write_name_quoted_relative(base->buf,
++ baselen = data->base->len;
++ strbuf_addstr(data->base, data->pathname);
++ write_name_quoted_relative(data->base->buf,
+ chomp_prefix ? ls_tree_prefix : NULL, stdout,
+ line_termination);
+- strbuf_setlen(base, baselen);
++ strbuf_setlen(data->base, baselen);
+ return 1;
+ }
+
+@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
+ size_t baselen;
+ enum object_type type = object_type(mode);
+
++ struct expand_ls_tree_data data = {
++ .mode = mode,
++ .type = type,
++ .oid = oid,
++ .pathname = pathname,
++ .base = base,
++ };
++
+ if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
+ recurse = READ_TREE_RECURSIVE;
+ if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
+@@ builtin/ls-tree.c: static int show_tree(const struct object_id *oid, struct strbuf *base,
+ }
+
+ if (shown_fields >= FIELD_DEFAULT)
+- show_default(oid, type, pathname, mode, base);
++ show_default(&data);
+
+ return recurse;
+ }
@@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
struct object_id oid;
struct tree *tree;
int i, full_tree = 0;
-+ const char *implicit_format = NULL;
-+ const char *format = NULL;
-+ struct read_tree_ls_tree_data read_tree_cb_data = {
-+ .sb_scratch = STRBUF_INIT,
-+ .sb_tmp = STRBUF_INIT,
-+ };
++ read_tree_fn_t fn = show_tree;
const struct option ls_tree_options[] = {
OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
LS_TREE_ONLY),
@@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref
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_STRING_F(0, "format", &format, N_("format"),
++ N_("format to use for the output"),
++ PARSE_OPT_NONEG),
OPT__ABBREV(&abbrev),
OPT_END()
};
-+ read_tree_fn_t fn = show_tree;
-
- git_config(git_default_config, NULL);
- ls_tree_prefix = prefix;
@@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
- if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
ls_options |= LS_SHOW_TREES;
-+ if (ls_options & LS_NAME_ONLY)
-+ implicit_format = ls_tree_format_n;
-+ if (ls_options & LS_SHOW_SIZE)
-+ implicit_format = ls_tree_format_l;
-+
-+ if (format && implicit_format)
-+ usage_msg_opt(_("providing --format cannot be combined with other format-altering options"),
-+ ls_tree_usage, ls_tree_options);
-+ if (implicit_format)
-+ format = implicit_format;
-+ if (!format)
-+ format = ls_tree_format_d;
++ 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))
@@ builtin/ls-tree.c: 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);
+
+ /*
+ * The generic show_tree_fmt() is slower than show_tree(), so
+ * take the fast path if possible.
+ */
-+ if (format && (!strcmp(format, ls_tree_format_d) ||
-+ !strcmp(format, ls_tree_format_l) ||
-+ !strcmp(format, ls_tree_format_n)))
++ if (format &&
++ (!strcmp(format, ls_tree_format_d) ||
++ !strcmp(format, ls_tree_format_l) ||
++ !strcmp(format, ls_tree_format_n) ||
++ !strcmp(format, ls_tree_format_o)))
+ fn = show_tree;
+ else if (format)
+ fn = show_tree_fmt;
-+ /*
-+ * Allow forcing the show_tree_fmt(), to test that it can
-+ * handle the test suite.
-+ */
-+ if (git_env_bool("GIT_TEST_LS_TREE_FORMAT_BACKEND", 0))
-+ fn = show_tree_fmt;
+
-+ read_tree_cb_data.format = format;
- return !!read_tree(the_repository, tree,
-- &pathspec, show_tree, NULL);
-+ &pathspec, fn, &read_tree_cb_data);
++ return !!read_tree(the_repository, tree, &pathspec, fn, NULL);
}
## t/t3105-ls-tree-format.sh (new) ##
@@ t/t3105-ls-tree-format.sh (new)
+. ./test-lib.sh
+
+test_expect_success 'ls-tree --format usage' '
-+ test_expect_code 129 git ls-tree --format=fmt -l &&
-+ test_expect_code 129 git ls-tree --format=fmt --name-only &&
-+ test_expect_code 129 git ls-tree --format=fmt --name-status
++ test_expect_code 129 git ls-tree --format=fmt -l HEAD &&
++ test_expect_code 129 git ls-tree --format=fmt --name-only HEAD &&
++ test_expect_code 129 git ls-tree --format=fmt --name-status HEAD &&
++ test_expect_code 129 git ls-tree --format=fmt --object-only HEAD
+'
+
+test_expect_success 'setup' '
@@ t/t3105-ls-tree-format.sh (new)
+ test_ls_tree_format \
+ "%(path)" \
+ "--name-only"
++'
+
++test_expect_success 'ls-tree --format=<object-only-like>' '
++ test_ls_tree_format \
++ "%(objectname)" \
++ "--object-only"
+'
+
+test_done
From: Teng Long <hidden> Date: 2022-01-14 07:59:14
On Thu, Jan 13, 2022 at 2:55 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
Re the $subject: Is "optimize naming" here just referring to the
s/retval/recurse/g?
Yes.
Personally I think just a s/retval/ret/g here would make more senes if
we're doing any change at all, and in either case having this variable
re-rename split up as its own commit would make the proposed control
flow changes clearer.
Do you mean that I can split the current one into two commits, one does
the renaming work and another one does the left work?
If so, I will do this in the next patch.
This new function is a re-invention of the object_type() utility in
cache.h, and isn't needed. I.e....
...just drop it and do this:
- enum object_type type = get_type(mode);
+ enum object_type type = object_type(mode);
You are absolutely correct.
I will replace get_type() to object_type() in the next patch.
From: Teng Long <hidden> Date: 2022-01-14 08:18:47
On Thu, Jan 13, 2022 at 3:02 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
In the RFC series I sent this was first implemented in terms of the
--format option, and I skipped the custom implementation you're adding
here:
https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/
I think in terms of patch series structure it would make sense to do
that, and then have this custom --object-only implementation in terms of
not-"--format " follow from that, and thus with the tests for the two
Sorry, the "not-"--format" means?
(we'd add the tests you're adding here first, just for a
--format="%(objectname)" or whatever) we'd see that the two are 1=1
equivalent in terms of functionality, but that this one is <X>% more
optimized.
Please allow me to understand your advice, if we put the commit of
introducing "--format" before the commit of introducing "--object-only", will
be better because it's possible to supply more optimized performance
(if we have) information in the commit message.
From: Teng Long <hidden> Date: 2022-01-14 09:13:02
On Thu, Jan 13, 2022 at 3:07 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
"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.
Is the only reason this is split off from 9/9 because you're injecting a
8/9 commit for the coccinelle rule change, and wanted to find some
logical cut-off between the two?
I hope "show_tree()" and "the show_tree_format()" to share this structure,
so I made this a pre-prepared and non-functional commit.
After that, in the 9/9, the structure can be used directly and focus on the
functionality changes. If we merge this commit with 9/9, 9/9 will contain a part
of the changes that let "show_tree()" use the new structure, which has nothing
to do with "show_tree_format()" actually, because we designed them to go
through different execution logic. So, personally, I prefer not to mix them
together.
So, the commit of "show_tree_data()" originally was not for "coccinelle".
The only thing that is certain is that coccinelle also should go before 9/9
I think. With regard to 8/9 and 7/9, I think the current order is OK because
they're not related.
On Thu, Jan 13, 2022 at 3:02 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
In the RFC series I sent this was first implemented in terms of the
--format option, and I skipped the custom implementation you're adding
here:
https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/
I think in terms of patch series structure it would make sense to do
that, and then have this custom --object-only implementation in terms of
not-"--format " follow from that, and thus with the tests for the two
Sorry, the "not-"--format" means?
Sorry about not being clear. I mean there's two potential
implementations. One that't is terms of --format='%(objectname)', and
the other with your custom (faster) code to implement it.
quoted
(we'd add the tests you're adding here first, just for a
--format="%(objectname)" or whatever) we'd see that the two are 1=1
equivalent in terms of functionality, but that this one is <X>% more
optimized.
Please allow me to understand your advice, if we put the commit of
introducing "--format" before the commit of introducing "--object-only", will
be better because it's possible to supply more optimized performance
(if we have) information in the commit message.
Yes, you get the functionality you need with a simple alias of
--format='%(objectname)' to --object-name (or whatever), so the only
reason to carry the extra code is for optimization.
I wonder if the extra difference in performance is still something you
care about, or if just the --format implementation would be OK.
But in any case, starting with a simpler implementation and testing it
makes the progression easier to reason about.
On Thu, Jan 13, 2022 at 2:55 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
Re the $subject: Is "optimize naming" here just referring to the
s/retval/recurse/g?
Yes.
quoted
Personally I think just a s/retval/ret/g here would make more senes if
we're doing any change at all, and in either case having this variable
re-rename split up as its own commit would make the proposed control
flow changes clearer.
Do you mean that I can split the current one into two commits, one does
the renaming work and another one does the left work?
If so, I will do this in the next patch.
Yes, at least I would find it easier to read :)
quoted
This new function is a re-invention of the object_type() utility in
cache.h, and isn't needed. I.e....
...just drop it and do this:
- enum object_type type = get_type(mode);
+ enum object_type type = object_type(mode);
You are absolutely correct.
I will replace get_type() to object_type() in the next patch.
From: Teng Long <hidden> Date: 2022-01-18 09:55:36
On Fri, Jan 14, 2022 at 7:59 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
Yes, you get the functionality you need with a simple alias of
--format='%(objectname)' to --object-name (or whatever), so the only
reason to carry the extra code is for optimization.
I wonder if the extra difference in performance is still something you
care about, or if just the --format implementation would be OK.
But in any case, starting with a simpler implementation and testing it
makes the progression easier to reason about.
Actually, at first, I wanted to achieve this in a simple way, as the
"--object-only" implementation.
With the discussion in the community, I think both of them can achieve
this purpose. "--object-only" is more intuitive, while "--format "is
more flexible.
For example, if the terminal supports automatic completion, the function of
this option can be clearly known with typing TAB and lower costs of use and
understanding. "--format" also works, but maybe have to check the help
document to see if there are fields that support the same purpose.
Because the community had a different opinion about it. Junio, might prefer
an "--object-only" approach, if I understand the context correctly.
So I have some inclination to support both. However, I can accept that only
"--format" is supported.
So in the next patch, I hope to do some refactoring of the commit to support
"--object-only" as the top commit. If in the end, we decide that "--format" is
enough, we can discard the top "--object-only" commit.
I know you guys currently are busy on the new 2.35 release, so a later reply
is OK.
Thanks.
Let's split up this re-flow only change into its own commit? I.e. the
only non-whitespace change here is beginning with [--format].
If it was the right thing to do to re-flow this then we didn't need
[--format=<format>] to exist to do so...
Agree, especially if "--format" comes earlier as you mentioned in
another reply.
The doc change should only include the new "--format" here, so
we just:
Removing this \n breaks the formatting in the file. See "make man && man
./Documentation/git-ls-tree.1". The ./Documentation/doc-diff utility is
also handy for sanity checking the documentation formatting.
This is my mistake and will be corrected in the next patch.
quoted
-----------
Lists the contents of a given tree object, like what "/bin/ls -a" does
@@ -79,6 +79,16 @@ OPTION 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`.+
These new docs make sense & seem to cover all the basis, thanks!
Actually, the content is not from me, I borrowed it from other
documents, but I'm glad if it's described and placed here correctly.
Here because we've added --format discussing the previous pseudo-format
as a "default" format becomes confusing. Let's instead say:
The output format of `ls-tree` is determined by either the `--format` option,
or other format-altering options such as `--name-long` 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:
---
%(mode) %(type) %(object)%x09%(file)
---
Make sense.
I will use this paragraph instead in next patch except a tiny
nit (s/--name-long/--name-only/).
quoted
+The output format changes to:
+
+ {"object":"<object>", "file":"<file>"}
This one-liner is guaranteed to result in invalid JSON on some
repositories, both because JSON is inherently a bad fit for git's data
model (JSON needs to be in one Unicode encoding, Git's tree data might
me in a mixture of encodings), and because it'll break if the file
includes a '"'.
Correct and especially we use a "quote_c" style behind.
I think it's better to just replace this with some example involving -z,
or at least prominently note that this is broken in the general case,
but can be used ad-hoc to quickly check things with "jq" or whatever.
Your suggestion is great, but personally I don't want to introduce more
complexity and other tools in here, and try to describe it in a simple way.
I think the below maybe is enough:
@@ -117,14 +127,10 @@ 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 want to only print the <object> and <file> fields with a-JSON style, executing with a specific "--format" like-- git ls-tree --format='{"object":"%(object)", "file":"%(file)"}' <tree-ish>--The output format changes to:+For example, if you only care about the <object> and <file> fields, you can+execute with a specific "--format" like- {"object":"<object>", "file":"<file>"}+ git ls-tree --format="%(object) %(file)" <tree-ish> FIELD NAMES -----------
quoted
+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:
+
+mode::
+ The mode of the object.
+type::
+ The type of the object (`blob` or `tree`).
+object::
+ The name of the object.
+size[:padded]::
+ The size of the object ("-" if it's a tree).
+ It also supports a padded format of size with "%(size:padded)".
+file::
+ The filename of the object.
In
https://lore.kernel.org/git/cover.1641043500.git.dyroneteng@gmail.com/
you noted that you changed the field names of e.g. "objectname" to
"object" etc. You're right that I picked these as-is from the
git-for-each-ref formatting.
1/3 of your reasoning for doing so was to make it consistent with the
documentation examples of e.g.:
<mode> SP <type> SP <object> TAB <file>
I think in any case (as noted above) we should change those to use the
--format), so that leaves just:
- "I prefer to make the name more simple to memorize and type"
- "I think the names with "object" prefix are [from git-for-each-ref
and the object* prefixes aren't redundant there, but would be here]".
I think both of those still apply, but I think having these consistent
with git-for-each-ref outweighs the slight benefit of shorter names.
Right now only a handful of things support these sort of --format
directives, but we've already got RFC/WIP patches to add that to
git-cat-file, and are likely to add more in the future.
New and important input for me on this.
I'd also like us to eventually be able to combine what are now separate
built-ins with their own --format to expose more deeply some internal
APIs via IPC. E.g. now you can do this:
git for-each-ref --format='%(refname) %(tree)'
But to list each of those trees you'd need to pipe that output into this
new 'git ls-tree --format. But imagine being able to do something like:
git for-each-ref --format='%(refname) %(git-ls-tree --format %%(objectname) %(tree))'
Make sense.
Where we'd just invoke git-ls-tree for you without running a full
sub-process. I think both for that hypothetical and working with the two
--formats now having to use %(type) in some places but %(objecttype)
etc. in others is just needlessly confusing. Let's just consistently use
the same format names everywhere.
Specifically for your s/path/file/ name change, that's just inaccurate, consider:
$ ./git ls-tree --format="%(mode) %(type) %(file)" -t HEAD -- t/README
040000 tree t
100644 blob t/README
And:
$ $ (cd t && ../git ls-tree --format="%(mode) %(type) %(file)" -t -r HEAD -- README)
040000 tree ./
100644 blob README
I.e. we talk about <path> in the existing SYNOPSIS for a reason. That we
had a "<file>" in the existing format demo was a bug/shorthand that we
shouldn't be propagating further.
Should use "path" instead of "file" in here.
Make sense.
Ha. Thanks.I think they not align in here is because my variables' names not align :)
Actually, I was hesitating to use like "object" or follow the "objectname" like
rules. I would like git to have unified naming style on this, but I didn't have
that much input (other usage already on the way) at the time, so I chose to use
a shorter and probably more memorable name based on the document.
But now, I agree with you, to use the same naming conventions because on the whole,
especially multiple commands have the same appeal on format naming, uniformity is
more important than memorability of a single command, and I also think maybe we
might need to describe and maintain these rules of <fieldname> in a document
somewhere.
It also makes it easier to review to avoid such churn, to see what you
really changed I'm looking at a local version of a range-diff where I
renamed these, the struct you renamed etc. back just to see what you
/really/ changed. I.e. what are functional v.s. renaming changes.
Here you changed my '"%"PRIuMAX' to '"%" PRIuMAX'. The former is the
prevailing style in this codebase, and avoiding the formatting churn
makes the inter-diff easier to read.
Will fix it in next patch.
Ditto some harder to review interdiff due to renaming
churn. I.e. s/line/sb/ in both this and expand_show_tree(). I really
wouldn't care at all except because of all the manual work in reviewing
the inter-diff between my original version & this derived version.
In the case of "line" that's not even an improvement. With a --format
we're not building a "line", the user is free to insert any arbitrary
directives including \n's, so we might be working on multiple lines.
I'm not sure I understand all the meanings above. I think I have forgot about the content here.
and I have to add some tests for other options like "-t" combined with "--format".
Please correct me if I misunderstood.
I thought the "GIT_TEST_LS_TREE_FORMAT_BACKEND" is only used in your RFC for testing conveniently,
and should be removed in the end (non-RFC). So I removed it...
So should we add it back?
Even with that re-added I really wouldn't trust that this code is doing
the right thing (and as noted, I don't trust my own RFC version
either). I think e.g. our "coverage" Makefile targets would be a good
start as a first approximation, i.e. running the /ls-tree/ tests and
seeing if we have full coverage.
Yeah. I haven't tried it yet, but I will try to run coverage detection when
the next patch is completed
As I noted in 7/9 I think this patch is 9/9 still mostly something I
wrote, so that the "author" and Signed-off-by should be preserved. The
below is a range-diff of an amended version I've been looking at in
trying to review this. It undoes several (but not all) of your
formatting/renaming-only changes, just so that I could see what the
non-formatting changes were:
Sorry for that. I misunderstand something here.
I will fix in the next patch.
Thanks.
On Fri, Jan 14, 2022 at 7:59 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
Yes, you get the functionality you need with a simple alias of
--format='%(objectname)' to --object-name (or whatever), so the only
reason to carry the extra code is for optimization.
I wonder if the extra difference in performance is still something you
care about, or if just the --format implementation would be OK.
But in any case, starting with a simpler implementation and testing it
makes the progression easier to reason about.
Actually, at first, I wanted to achieve this in a simple way, as the
"--object-only" implementation.
With the discussion in the community, I think both of them can achieve
this purpose. "--object-only" is more intuitive, while "--format "is
more flexible.
For example, if the terminal supports automatic completion, the function of
this option can be clearly known with typing TAB and lower costs of use and
understanding. "--format" also works, but maybe have to check the help
document to see if there are fields that support the same purpose.
Because the community had a different opinion about it. Junio, might prefer
an "--object-only" approach, if I understand the context correctly.
So I have some inclination to support both. However, I can accept that only
"--format" is supported.
I'm only talking about how it's implemented internally, not whether we
have an --object-only option in the UI. I think it's good to have the
option for completion etc.
I.e. in my RFC implementation of it here it's just a trivial wrapper
around specifying a --format:
https://lore.kernel.org/git/RFC-patch-7.7-5e34df4f8dd-20211217T131635Z-avarab@gmail.com/;
Implementing it is 6 lines of trivial C code boilerplate.
But when you picked that up & ran with it you ended up carrying your
original implementation:
https://lore.kernel.org/git/e0274f079a7d381b9a936bfcd53bad64167c18b8.1641440700.git.dyroneteng@gmail.com/
I'm not saying we shouldn't have that, but that in any case a sequence of:
1. Add a --format option
2. Add a --object-only alias for a --format (what my RFC 7/7 does)
3. Add a custom more optimized --object-only implementation
Would make the patch progression much easier to read, and we'd consider
the correctness of --object-only (1 and 2) separate from the
optimization question (3).
But maybe we won't need (3) at all in the end, i.e. is (1 and 2) fast
enough for it not to matter (I think probably "yes", but I don't have a
strong opinion on that).
So in the next patch, I hope to do some refactoring of the commit to support
"--object-only" as the top commit. If in the end, we decide that "--format" is
enough, we can discard the top "--object-only" commit.
*nod*, now that I read ahead I think you pretty much agree with that plan :)
I know you guys currently are busy on the new 2.35 release, so a later reply
is OK.
Now would be a good time :)
I was reminded of this because Junio's proposed it for next at
https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/
I think per the above & other replies of mine (including not matters of
code arrangement opinion, but e.g. the doc formatting bug) we'll need at
least one more re-roll of this. Thanks for sticking with this & working
on this!
I'll indicate that in a reply to that "What's Cooking" report.
From: Teng Long <hidden> Date: 2022-02-07 02:22:39
On Fri, Feb 4, 2022 at 9:04 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
I'm not saying we shouldn't have that, but that in any case a sequence of:
1. Add a --format option
2. Add a --object-only alias for a --format (what my RFC 7/7 does)
3. Add a custom more optimized --object-only implementation
Would make the patch progression much easier to read, and we'd consider
the correctness of --object-only (1 and 2) separate from the
optimization question (3).
But maybe we won't need (3) at all in the end, i.e. is (1 and 2) fast
enough for it not to matter (I think probably "yes", but I don't have a
strong opinion on that).
Sorry for the late reply, I had a vacation in the last two weeks (Chinese
New Year).
I have to say it's a very valuable recommendation and at the same time
I recognise that
spending more time on organizing commits ahead is important and make
small steps(or commits) sufficiently.
Now would be a good time :)
I was reminded of this because Junio's proposed it for next at
https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/
I think per the above & other replies of mine (including not matters of
code arrangement opinion, but e.g. the doc formatting bug) we'll need at
least one more re-roll of this. Thanks for sticking with this & working
on this!
I'll indicate that in a reply to that "What's Cooking" report.
Thanks for mentioning that. I will back work on it this week.
Thanks.
From: Teng Long <hidden> Date: 2022-02-08 13:14:55
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-02-08 13:15:15
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-02-08 13:15:15
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 <redacted>
---
builtin/ls-tree.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
From: Teng Long <hidden> Date: 2022-02-08 13:15:15
We usually pipe the output from `git ls-trees` to tools like
`sed` or `cut` when we only want to extract some fields.
When we want only the pathname component, we can pass
`--name-only` option to omit such a pipeline, but there are no
options for extracting other fields.
Teach the "--object-only" option to the command to only show the
object name. This option cannot be used together with
"--name-only" or "--long" , they are mutually exclusive (actually
"--name-only" and "--long" can be combined together before, this
commit by the way fix this bug).
In terms of performance, there is no loss comparing to the
"master" (2ae0a9c), here are the
results of the performance tests in my environment based on linux
repository:
$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.8 ms ± 2.7 ms [User: 85.7 ms, System: 20.0 ms]
Range (min … max): 101.5 ms … 111.3 ms 28 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
Time (mean ± σ): 105.0 ms ± 3.0 ms [User: 83.7 ms, System: 21.2 ms]
Range (min … max): 99.3 ms … 109.5 ms 27 runs
$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 ± σ): 337.4 ms ± 10.9 ms [User: 308.3 ms, System: 29.0 ms]
Range (min … max): 323.0 ms … 355.0 ms 10 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
Time (mean ± σ): 337.6 ms ± 6.2 ms [User: 309.4 ms, System: 28.1 ms]
Range (min … max): 330.4 ms … 349.9 ms 10 runs
Signed-off-by: Teng Long <redacted>
---
Documentation/git-ls-tree.txt | 7 ++++-
builtin/ls-tree.c | 16 ++++++++++-
t/t3104-ls-tree-format.sh | 12 +++++++++
t/t3105-ls-tree-oid.sh | 51 +++++++++++++++++++++++++++++++++++
4 files changed, 84 insertions(+), 2 deletions(-)
create mode 100755 t/t3105-ls-tree-oid.sh
@@ -59,6 +59,11 @@ 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`. --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object
@@ -304,6 +316,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,
@@ -78,4 +84,10 @@ test_expect_success 'ls-tree hit fast-path with --format=<name-only-like>' 'gitls-tree--format="%(path)"-rHEAD>actual&&test_cmpexpectactual'++test_expect_success'ls-tree hit fast-path with --format=<object-only-like>''+gitls-tree-r--object-onlyHEAD>expect&&+gitls-tree--format="%(objectname)"-rHEAD>actual&&+test_cmpexpectactual+' test_done
From: Teng Long <hidden> Date: 2022-02-08 13:15:16
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-02-08 13:15:16
This commit use "object_type()" to get the type, then remove
some of the nested if to let the codes here become more cleaner.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
From: Teng Long <hidden> Date: 2022-02-08 13:15:16
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-02-08 13:15:16
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 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -120,12 +120,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-02-08 13:15:16
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 | 126 ++++++++++++++++++++++++++++++++--------------
1 file changed, 89 insertions(+), 37 deletions(-)
@@ -120,12 +170,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,
@@ -156,6 +206,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-02-08 13:15:17
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-02-08 13:15:17
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 | 43 ++++++++++++++++++++++++++++---------------
1 file changed, 28 insertions(+), 15 deletions(-)
From: Teng Long <hidden> Date: 2022-02-08 13:15:17
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].
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/
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 | 81 +++++++++++++++++++++
3 files changed, 262 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
@@ -33,7 +33,10 @@ static unsigned int shown_fields;#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;
@@ -55,6 +58,72 @@ enum {staticintcmdmode=MODE_UNSPECIFIED;+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: as '%s'"),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(void){if(cmdmode==MODE_NAME_ONLY){
@@ -174,6 +275,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *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),
@@ -194,6 +296,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()};
@@ -214,6 +319,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))
@@ -237,6 +346,20 @@ 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&&(!strcmp(format,default_format))){+fn=show_tree;+}elseif(format&&(!strcmp(format,long_format))){+shown_fields=shown_fields|FIELD_SIZE;+fn=show_tree;+}elseif(format&&(!strcmp(format,name_only_format))){+shown_fields=FIELD_PATH_NAME;+fn=show_tree;+}elseif(format)+fn=show_tree_fmt;++return!!read_tree(the_repository,tree,&pathspec,fn,NULL);}
We usually pipe the output from `git ls-trees` to tools like
`sed` or `cut` when we only want to extract some fields.
When we want only the pathname component, we can pass
`--name-only` option to omit such a pipeline, but there are no
options for extracting other fields.
Teach the "--object-only" option to the command to only show the
object name. This option cannot be used together with
"--name-only" or "--long" , they are mutually exclusive (actually
"--name-only" and "--long" can be combined together before, this
commit by the way fix this bug).
In terms of performance, there is no loss comparing to the
"master" (2ae0a9c), here are the
results of the performance tests in my environment based on linux
repository:
I think given the re-arrangement in this v11 it would make sense to
change the commit messageto say:
* This is an alias for --format=%(objectname)
* Per benchmark XYZ it's faster
I.e. this:
$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.8 ms ± 2.7 ms [User: 85.7 ms, System: 20.0 ms]
Range (min … max): 101.5 ms … 111.3 ms 28 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r HEAD
Time (mean ± σ): 105.0 ms ± 3.0 ms [User: 83.7 ms, System: 21.2 ms]
Range (min … max): 99.3 ms … 109.5 ms 27 runs
$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 ± σ): 337.4 ms ± 10.9 ms [User: 308.3 ms, System: 29.0 ms]
Range (min … max): 323.0 ms … 355.0 ms 10 runs
$hyperfine --warmup=10 "/opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD"
Benchmark 1: /opt/git/ls-tree-oid-only/bin/git ls-tree -r -l HEAD
Time (mean ± σ): 337.6 ms ± 6.2 ms [User: 309.4 ms, System: 28.1 ms]
Range (min … max): 330.4 ms … 349.9 ms 10 runs
Is surely more relevant if compared to master & that --format.
@@ -59,6 +59,11 @@ 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`.
Hrm, I regret that in my version of v11 11/13 I didn't add to all of these something like:
This is equivalent to specifying `--format=...`, but for both this
option and that exact format the command takes a hand-optimized
codepath instead of going through the generic formatting mechanism.
Or whatever, and perhaps have everything after ", but for[...]" part of
the generic FORMAT section (no need to say it for every option).
quoted hunk
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
@@ -304,6 +316,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,
@@ -78,4 +84,10 @@ test_expect_success 'ls-tree hit fast-path with --format=<name-only-like>' ' git ls-tree --format="%(path)" -r HEAD >actual && test_cmp expect actual '++test_expect_success 'ls-tree hit fast-path with --format=<object-only-like>' '+ git ls-tree -r --object-only HEAD >expect &&+ git ls-tree --format="%(objectname)" -r HEAD >actual &&+ test_cmp expect actual+' test_done
So, you and I came up with independent tests for these two.
I wonder if this can be re-arranged so that we can share the tests, and
perhaps test all for both the --format and --object-onnly in some
for-loop, or maybe it's not worth it.
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.
quoted hunk
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
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.
+ if (*start != '(')
+ die(_("bad ls-tree format: as '%s'"), start);
My typo surely, but I think I menat "as of" not just "as" there:
$ ./git ls-tree --format="%[blah)" -r HEAD
fatal: bad ls-tree format: as of '[blah)'
+
+ end = strchr(start + 1, ')');
+ if (!end)
+ die(_("bad ls-tree format: element '%s' does not end in ')'"), start);
Or actually:
$ ./git ls-tree --format="%(blah]" -r HEAD
fatal: bad ls-tree format: element '(blah]' does not end in ')'
We could rather say for the first one:
$ ./git ls-tree --format="%[blah)" -r HEAD
fatal: bad ls-tree format: element '[blah)' does not start with '('
I wondered why that %% is there (and I probably wrote this in the first
place, I didn't check:). But it makes sense, because strbuf_expand()
skips past the % for us, and we'd like to say e.g. %(foobar) here, not
(foobar) or whatever.
I notice now after commenting on your 13/13 that you should add
TEST_PASSES_SANITIZE_LEAK=true to it (assuming it doesn't leak, which I
don't think it does, but test with SANITIZE=leak first!)
I also forgot I wrote this, but also per my comment on 13/13 you can
just add your tests added in 13/13 to this file, then we'll assert that
-r etc. work the same for both.
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()").
I think this and the 09/13 really don't make sense in combination.
Now, I clearly prefer to put options for the command into its own little
struct to pass around, I think it makes for easier reading than the
globals you end up with.
But tastes differ, and some built-ins use one, and some the other
pattern.
But this is really the worst of both worlds, let's just pick one or the
other, not effectively some some ptions in that struct in 09/13, and
some in globals here...
+static unsigned int shown_fields;
+#define FIELD_PATH_NAME 1
+#define FIELD_SIZE (1 << 1)
+#define FIELD_OBJECT_NAME (1 << 2)
+#define FIELD_TYPE (1 << 3)
+#define FIELD_MODE (1 << 4)
+#define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */
Why do we need some FIELD_DEFAULT here as opposed to just having it by
an enum field with a valu of 0?
I still don't really get why we can't just use the one MODE_*
here. E.g. doesn't MODE_LONG map to FIELD_LONG_DEFAULT, MODE_NAME_ONLY
to FIELD_PATH_NAME etc?
Is this all so we can do "shown_fields & FIELD_SIZE" in show_default()
as opposed to e.g. checking "default format or long format?" ?
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 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -120,12 +120,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,
This seems like a sensible fix, but let's add a test for it. See:
git grep 'test_expect_code 129'
For examples.
This commit use "object_type()" to get the type, then remove
some of the nested if to let the codes here become more cleaner.
Signed-off-by: Teng Long <redacted>
---
builtin/ls-tree.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
I think the use of object_type() here is good, but in any case I think
doing a minimal change first for the "type" and then this proposed
refactoring would be easier to look at, and to independently decide on
the two.
I find this much easier to read, both as a diff and as end-state:
Unrolling this from a logical if/else if to an if/if/if I think also
doesn't make sense. At the cost of a slightly larger diff (could be done
on top) we get rid of the show_recursive() branch too:
Which, I think is also nicer to read, we're not checking "is it a
tree?", setting "recursive", and then using that "recursive" as a
boolean for no reason. Let's just continue on that "else if" chain we're
already in instead...
From: Teng Long <hidden> Date: 2022-03-04 10:43:09
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-04 10:43:12
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-04 10:43:17
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-04 10:43:26
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-04 10:43:28
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-04 10:43:32
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-04 10:43:39
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 | 124 ++++++++++++++++++++++++++++++++--------------
1 file changed, 87 insertions(+), 37 deletions(-)
@@ -123,12 +171,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 +207,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-04 10:43:42
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-04 10:43:53
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-04 10:43:57
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].
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/
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 | 81 +++++++++++++++++++++
3 files changed, 262 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
@@ -32,7 +32,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;
@@ -53,6 +56,72 @@ static enum mutx_option {MODE_LONG,}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){
@@ -178,6 +279,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),
@@ -198,6 +300,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()};
@@ -218,6 +323,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-04 10:43:59
'--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 | 18 +++++++++++++++++-
t/t3103-ls-tree-misc.sh | 7 +++++--
t/t3104-ls-tree-format.sh | 19 +++++++++++++++++++
4 files changed, 51 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
@@ -128,7 +131,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))
@@ -295,6 +306,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,
@@ -78,4 +91,10 @@ test_expect_success 'ls-tree hit fast-path with --format=<name-only-like>' 'gitls-tree--format="%(path)"-rHEAD>actual&&test_cmpexpectactual'++test_expect_success'ls-tree hit fast-path with --format=<object-only-like>''+gitls-tree-r--object-onlyHEAD>expect&&+gitls-tree--format="%(objectname)"-rHEAD>actual&&+test_cmpexpectactual+' test_done
Re
https://lore.kernel.org/git/20220308080551.18538-1-dyroneteng@gmail.com/
sorry about the delay in reviewing this. I did look it over & come up
with the below almost a week ago, but forgot/didn't have time to turn
it into inline comments.
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 up
with while reviewing it. Brief notes below:
Ævar Arnfjörð Bjarmason (7):
ls-tree tests: add tests for --name-status
Optional, but maybe a good idea to add to the series. I noticed the
zero --name-status test coverage when testing it.
ls-tree tests: exhaustively test fast & slow path for --format
Probably a good idea to add to it. I.e. the tests for the --format
drifted from the RFC version I had so that we weren't testing the
optimized v.s. non-optimized path.
ls-tree: remove dead labels
Removes dead code in the proposed series (from an earlier iteration?)
ls-tree: remove unused "MODE_UNSPECIFIED"
The MODE_UNSPECIFIED is also dead code in this series, but...
ls-tree: detect and error on --name-only --name-status
We should die on --name-status --name-only (two option synonyms), but
don't on "master", so. This does make some subsequent code here
slightly simpler though...
ls-tree: remove FIELD_*, just use MODE_*
The main outstanding thing for me when trying to understand the code
in this series is why we ended up with FIELD_* defines *and* the
MODE_*. This moves to just using MODE_*. I think it makes things
easier to understand. I.e. it's one less thing to pass around, and
what was in parse_shown_fields() is just embedded in show_tree() now
without the indirection.
There's also a change there to add an array of format <-> fast path fn
mappings, instead of the current if/else if chain with harcdoded
strcmp().
ls-tree: split up "fast path" callbacks
Maybe a good idea, maybe not. Splits up all the "fast path" callbacks
into individual functions, instead of a monolithic show_default().
I think the resulting code is easier to read, since it's clear what
data we need to set up for what, but maybe we're off into the weeds
here...
builtin/ls-tree.c | 279 ++++++++++++++++++++++---------------
t/t3101-ls-tree-dirname.sh | 55 ++++----
t/t3103-ls-tree-misc.sh | 15 +-
t/t3104-ls-tree-format.sh | 89 +++++-------
4 files changed, 235 insertions(+), 203 deletions(-)
--
2.35.1.1295.g6b025d3e231
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>
---
t/t3101-ls-tree-dirname.sh | 55 ++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 26 deletions(-)
Change these tests adapted from my initial version in [1] to
exhaustively test both the fast and slow paths of --format.
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.
1. https://lore.kernel.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>
---
t/t3104-ls-tree-format.sh | 89 +++++++++++++++------------------------
1 file changed, 33 insertions(+), 56 deletions(-)
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>
---
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))
Remove the "MODE_UNSPECIFIED" added in a preceding commit. We don't
need it, instead let's start the "command mode" enum at 1, as
e.g. builtin/help.c does for its similar pattern.
In addition let's change this from "mutx_option" to a more obvious
"ls-tree_cmdmode", even if the type name isn't used anywhere else. The
previous name sounded as though it had something to do with
a (thread?) mutex.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
These have not been used since a preceding "ls-tree: slightly refactor
`show_tree()`" commit, and should be squashed into that.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 3 ---
1 file changed, 3 deletions(-)
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.
A preceding commit removed "MODE_UNSPECIFIED", now there is a use for
such a thing, as we'll need to add this to our mapping so we'll choose
the "show_tree" fast-path here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/ls-tree.c | 98 ++++++++++++++++++++++-------------------------
1 file changed, 46 insertions(+), 52 deletions(-)
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>
---
builtin/ls-tree.c | 198 +++++++++++++++++++++++++++++-----------------
1 file changed, 124 insertions(+), 74 deletions(-)