On Fri, Dec 17 2021, Teng Long wrote:
> Many thanks to Junio and Ævar for your help and patient explanation.
> I noticed Ævar suggest the solution with using `--format`, but in
> this patch, the current approach continues. If this part of code needs
> to be improved or we want to support "--format" in "ls-tree" in the
> future, I'm more than glad to continue to contribute.
FWIW here's the changes I had locally & cleaned up now that did the
alternate --format approach.
I think you'll probably want to steal some of this, e.g. you're
patching the dead comment I removed in 1/6, 2-4/6 can be skipped, but
I thought they were nice.
Back when I last looked at this series, your --object-name patch was
much shorter, but now it's about the same size as the generic --format
support. So maybe it's worth considering implementing the more generic
path.
One reason I didn't submit this before is that I couldn't get past the
performance regression this would inttroduce, i.e. if moved entirely
to strbuf_expand(). Here though I'm keeping the old code, so it's no
slower than "master", unlike your patch. But I haven't dug into why
yours is slower:
$ git hyperfine -L rev origin/master,tl/object-name,avar/ls-tree-format -s 'make CFLAGS=-O3' './git -C /run/user/1001/linux ls-tree -r HEAD' --warmup 10 -r 10
Benchmark 1: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'origin/master
Time (mean ± σ): 67.8 ms ± 0.3 ms [User: 48.8 ms, System: 18.9 ms]
Range (min … max): 67.4 ms … 68.4 ms 10 runs
Benchmark 2: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'tl/object-name
Time (mean ± σ): 72.8 ms ± 0.4 ms [User: 50.6 ms, System: 22.1 ms]
Range (min … max): 72.0 ms … 73.2 ms 10 runs
Benchmark 3: ./git -C /run/user/1001/linux ls-tree -r HEAD' in 'avar/ls-tree-format
Time (mean ± σ): 67.6 ms ± 0.4 ms [User: 50.5 ms, System: 17.0 ms]
Range (min … max): 67.1 ms … 68.4 ms 10 runs
Summary
'./git -C /run/user/1001/linux ls-tree -r HEAD' in 'avar/ls-tree-format' ran
1.00 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree -r HEAD' in 'origin/master'
1.08 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree -r HEAD' in 'tl/object-name'
I then tacket a 6/6 at the end here to implement your --object-name in
terms of --format (but didn't update the comimt message etc.). That's
slower as expected:
$ git hyperfine -L rev tl/object-name,avar/ls-tree-format -s 'make CFLAGS=-O3' './git -C /run/user/1001/linux ls-tree --object-only -r HEAD' --warmup 10 -r 10
Benchmark 1: ./git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'tl/object-name
Time (mean ± σ): 58.7 ms ± 0.4 ms [User: 43.0 ms, System: 15.6 ms]
Range (min … max): 58.4 ms … 59.6 ms 10 runs
Benchmark 2: ./git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'avar/ls-tree-format
Time (mean ± σ): 65.6 ms ± 0.2 ms [User: 42.4 ms, System: 23.0 ms]
Range (min … max): 65.1 ms … 65.9 ms 10 runs
Summary
'./git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'tl/object-name' ran
1.12 ± 0.01 times faster than './git -C /run/user/1001/linux ls-tree --object-only -r HEAD' in 'avar/ls-tree-format'
But it's not too bad, so maybe it's fine & worth making it more
generic?
Anyway. Just food for thought and and FYI in case you're
interested. Junio noted already that he'd like the --object-name
approach first, so if you still want to pursue your current
implementation I don't mind.
I do think you should be making performance testing a part of your
testing & cover letter writing though. A 8-10% slowdown isn't nothing,
especially for exactly the sort of plumbing command that'll likely to
be used to e.g. slurp up all paths in a very large repo.
These patches really aren't "ready". There's no docs, and as I noted
in some earlier thread 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.
Well, that one's me not having added a test, but I'm fairly sure there
might also be hidden bugs here due to lack of testing.
Teng Long (1):
ls-tree.c: support `--object-only` option for "git-ls-tree"
Ævar Arnfjörð Bjarmason (6):
ls-tree: remove commented-out code
ls-tree: add missing braces to "else" arms
ls-tree: use "enum object_type", not {blob,tree,commit}_type
ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"
ls-tree: split up the "init" part of show_tree()
ls-tree: add a --format=<fmt> option
Documentation/git-ls-tree.txt | 7 +-
builtin/ls-tree.c | 226 ++++++++++++++++++++++++++++++----
t/t3103-ls-tree-misc.sh | 8 ++
t/t3104-ls-tree-oid.sh | 51 ++++++++
t/t3105-ls-tree-format.sh | 49 ++++++++
5 files changed, 313 insertions(+), 28 deletions(-)
create mode 100755 t/t3104-ls-tree-oid.sh
create mode 100755 t/t3105-ls-tree-format.sh
--
2.34.1.1119.g7a3fc8778ee
