Re: [PATCH v8] ls-files: introduce "--format" option
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 21, 2022, 15:54 UTC
- Message-ID
- <220721.86ilnqtq8f.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqbktj3ct7.fsf@gitster.g>
On Wed, Jul 20 2022, Junio C Hamano wrote:
Show 10 quoted lines
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> It's been quite many iterations, so I'll just comment on the range-diff.
>
>> -+ usage_msg_opt("--format cannot used with -s, -o, -k, -t"
>> ++ usage_msg_opt("--format cannot used with -s, -o, -k, -t, "
>> + "--resolve-undo, --deduplicate, --eol",
>> + ls_files_usage, builtin_ls_files_options);
>
> Looks good.Although a nit I didn't spot before: missing _() & this should be marked for translation, surely...
Show 10 quoted lines
>> + git commit -m base
>> +'
>> +
>> +test_expect_success 'git ls-files --format objectmode v.s. -s' '
>> -+ git ls-files -s | awk "{print \$1}" >expect &&
>> ++ git ls-files -s >files &&
>> ++ cut -d" " -f1 files >expect &&
>
> Either "awk" or "cut" is fine and flipping between them is a bit
> distracting. Cutting the pipe into two is a good move.That "cut" suggestion saw mine, sorry about the churn...
> But is this testing the right thing?
On this...
Show 21 quoted lines
>> +test_expect_success 'git ls-files --format objectmode v.s. -s' ' >> + git ls-files -s >files && >> + cut -d" " -f1 files >expect && >> + git ls-files --format="%(objectmode)" >actual && >> + test_cmp expect actual >> +' > > It only looks at the first column of the "-s" output, and we are > implicitly assuming that the order of output does not change between > the "-s" output and "--format=<format>" output. I wonder if it is > more useful and less error prone to come up with a format string > that 100% reproduces the "ls-files -s" output and compare the two, > e.g. > > format="%(objectmode) %(objectname) %(stage) %(path)" && > git ls-files -s >expect && > git ls-files --format="$format" >actual && > test_cmp expect actual > > I do not know if the $format I wrote without looking at the doc is > correct, but you get the idea.
Past rounds moved some tests towards that, maybe that's a good thing here too I didn't look deeply this time around...