Re: [PATCH v11 11/13] ls-tree.c: introduce "--format" option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 19, 2022, 05:44 UTC
Message-ID: <220219.86r17ze72s.gmgdl@evledraar.gmail.com>
In-Reply-To: <5936004f1386388740ded0208ebeb8c5d0c88ef2.1644319434.git.dyroneteng@gmail.com>

On Tue, Feb 08 2022, Teng Long wrote:

> +	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 '('

> [...]
> +		errlen = (unsigned long)len;
> +		die(_("bad ls-tree format: %%%.*s"), errlen, start);

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.

Show 10 quoted lines

> new file mode 100755
> index 0000000000..e08c83dc47
> --- /dev/null
> +++ b/t/t3104-ls-tree-format.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +
> +test_description='ls-tree --format'
> +
> +TEST_PASSES_SANITIZE_LEAK=true

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!)

Show 10 quoted lines

> +test_ls_tree_format () {
> +	format=$1 &&
> +	opts=$2 &&
> +	fmtopts=$3 &&
> +	shift 2 &&
> +	git ls-tree $opts -r HEAD >expect.raw &&
> +	sed "s/^/> /" >expect <expect.raw &&
> +	git ls-tree --format="> $format" -r $fmtopts HEAD >actual &&
> +	test_cmp expect actual
> +}

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.