Re: [PATCH 1/2] ls-files: introduce "--format" option

From: ZheNing Hu <adlternative@gmail.com>
Date: Jun 18, 2022, 10:50 UTC
Message-ID: <CAOLTT8SfRz62jVrdtjsGN=-iBAQzA3w_oJgT++xtx_kbdCmdqQ@mail.gmail.com>
In-Reply-To: <220615.86a6ady983.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年6月16日周四 04:15写道：

Show 10 quoted lines

>
>
> > +static void write_name_to_buf(struct strbuf *sb, const char *name)
> > +{
> > +     name = relative_path(name, prefix_len ? prefix : NULL, sb);
> FWIW I'd find this a bit less "huh?" if we declared another variable
> here, so just:
>
>         const char *rel = relative_path(name, ...).
>

Yeah, It's just a wrong code copy.

Show 14 quoted lines

> > +     o1
> > +     EOF
> > +     git ls-files --format="%(path)" -m >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'git ls-files --format with -d' '
> > +     rm o1 &&
>
> Don't "rm o1" here, rather have the test that creates it do:
>
>     test_when_finished "rm o1" &&
>     [the command that creates o1]
>

I thought about how to test 'git ls-files -d', so maybe I need something like:

test_expect_success 'git ls-files --format with -d' '
     echo o3 >o3 &&
     git add o3 &&
     rm o3 &&
     cat >expect <<-\EOF &&
     o3
     EOF
     git ls-files --format="%(path)" -d >actual &&
     test_cmp expect actual
'

Show 44 quoted lines

> > +     test_when_finished "git restore o1" &&
> > +     cat >expect <<-EOF &&
> > +     o1
> > +     EOF
> > +     git ls-files --format="%(path)" -d >actual &&
> > +     test_cmp expect actual
> > +'
> > +
>
> These tests...:
>
> > +test_expect_success 'git ls-files --format with -s must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" -s
> > +'
> > +
> > +test_expect_success 'git ls-files --format with -o must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" -o
> > +'
> > +
> > +test_expect_success 'git ls-files --format with -k must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" -k
> > +'
> > +
> > +test_expect_success 'git ls-files --format with --resolve-undo must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" --resolve-undo
> > +'
> > +
> > +test_expect_success 'git ls-files --format with --deduplicate must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" --deduplicate
> > +'
> > +
> > +test_expect_success 'git ls-files --format with --debug must fail' '
> > +     test_must_fail git ls-files --format="%(objectname)" --debug
> > +'
>
> ...would be better done with a for-loop, so:
>
>         for flag in -s -o -k --resolve-undo [...]
>         do
>                 test_expect_success "git ls-files --format is incompatible with $flag" '
>                         test_must_fail git ls-files --format="%(objectname)" $flag
>                 '
>         done
>

Yeah, using this loop will be clear.

Show 5 quoted lines

> Note the '' on the second argument, that's intentional, as we eval it
> you don't need "".
>
> > +test_done
>

Thanks for all these code style suggestions!

ZheNing Hu