On Wed, Apr 06 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> Haing written them I guess we could do them post-release, since the
>> important thing is to validate the changes. As noted in the commit
>> message we're now testing all combinations of the "mode" and "format"
>> options.
>>
>> builtin/ls-tree.c | 2 +-
>> t/t3104-ls-tree-format.sh | 126 +++++++++++++++++++++++++++++++++++---
>> 2 files changed, 119 insertions(+), 9 deletions(-)
>>
>> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
>> index 5dac9ee5b9d..e279be8bb63 100644
>> --- a/builtin/ls-tree.c
>> +++ b/builtin/ls-tree.c
>> @@ -255,7 +255,7 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
>> printf("%06o %s %s %7s\t", data.mode, type_name(data.type),
>> find_unique_abbrev(data.oid, abbrev), size_text);
>> show_tree_common_default_long(base, pathname, data.base->len);
>> - return 1;
>> + return recurse;
>> }
>
> OK, two people by happenstance wrote the same fix is reassuring ;-)
*nod*
>> + cat >expect &&
>> + cat <&6 >expect.-d &&
>> + cat <&7 >expect.-r &&
>> + cat <&8 >expect.-t &&
>
> Let's not go too cute like this. This forces the caller to remember
> which among 6, 7, and 8 corresponds to which option. It is too ugly
> to live.
I think it's rather elegant actually, but to be fair it would, per:
git grep '<&[1-9]| [1-9]<<-'
Be the user with the most FD's using this sort of pattern.
>> test_ls_tree_format \
>> "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
>> - ""
>> + "" \
>> + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T
>> + 040000 tree $(git rev-parse HEAD:dir) dir
>> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
>> + OUT
>> + 040000 tree $(git rev-parse HEAD:dir) dir
>> + OUT_D
>> + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t
>> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
>> + OUT_R
>> + 040000 tree $(git rev-parse HEAD:dir) dir
>> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
>> + OUT_T
>
> I do not actually mind if it were more like this:
>
> test_ls_tree_format \
> "%(objectmode) %(objecttype) %(objectname)%x09%(path)" <<-EXPECT
> git ls-tree HEAD
> 040000 tree $(git rev-parse HEAD:dir) dir
> 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
> git ls-tree -d HEAD
> 040000 tree $(git rev-parse HEAD:dir) dir
> git ls-tree -r HEAD
> 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t
> 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
> git ls-tree -t HEAD
> 040000 tree $(git rev-parse HEAD:dir) dir
> 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t
> EXPECT
>
> that is, we only read from the standard input, each section begins
> with "git ls-tree" command invocation that gets tested, followed by
> its expected output, and ends at either the end of the input or the
> beginning of the next section.
I don't see how needing to implementa while-loop parser for a custom
format just to get to the end-goal of having those things split up for
us is more elegant, when the shell can do that by splitting those up by
stream.
But if you insist on this being rewritten we could just unroll the loop,
and have each test_ls_tree_format specify the full option and one
standard input, but doing it in the function was a bit less verbosity.
Anyway, I see you replied on Josh's that you'd queue it, so it's unclear
to me if you'd like to pick this up with the version with the tests at
all, so I won't hack up a "v3" until that's clear...
