Subject: Re: [PATCH v2] ls-tree: fix --long implying -r regression in 9c4d58ff2c3

From: Junio C Hamano <[email-masked]>
Date: Wed, 06 Apr 2022 10:56:15 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> 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 ;-)

> +	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.

>  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.